Skip to content

Commit 83da6b2

Browse files
aileenaileen
committed
feat(ssl) improve error messages to log orignal error
refs #587 - fixes an issue, where failed requests with `got` wouldn't get catched properly - differentiates between `ProcessError` for `stderr` and `CliError` for everything else - `CliError` can be passed the original error and will log the `message` and `stack` to the log file, or log in the UI when in verbose mode - Adds `err` property with original error whereever we have it so it'll be passed along and printed in the log file
1 parent c25c141 commit 83da6b2

File tree

5 files changed

+95
-27
lines changed

5 files changed

+95
-27
lines changed

‎extensions/nginx/acme.js

+27-11
Original file line numberDiff line numberDiff line change
@@ -27,14 +27,13 @@ function install(ui, task) {
2727
ui.logVerbose('ssl: downloading acme.sh to temporary directory', 'green');
2828
return fs.emptyDir(acmeTmpDir)
2929
}).then(() => got(acmeApiUrl)).then((response) => {
30-
if (response.statusCode !== 200) {
31-
return Promise.reject(new cli.errors.CliError('Unable to query GitHub for ACME download URL'));
32-
}
33-
3430
try {
3531
response = JSON.parse(response.body).tarball_url;
3632
} catch (e) {
37-
return Promise.reject(new cli.errors.CliError('Unable to parse Github api response for acme'));
33+
return Promise.reject(new cli.errors.CliError({
34+
message: 'Unable to parse Github api response for acme',
35+
err: e
36+
}));
3837
}
3938

4039
return download(response, acmeTmpDir, {extract: true});
@@ -49,7 +48,21 @@ function install(ui, task) {
4948

5049
// Installs acme.sh into /etc/letsencrypt
5150
return ui.sudo('./acme.sh --install --home /etc/letsencrypt', {cwd: acmeCodeDir});
52-
}).catch((error) => Promise.reject(new cli.errors.ProcessError(error)));
51+
}).catch((error) => {
52+
// CASE: error is already a cli error, just pass it along
53+
if (error instanceof cli.errors.CliError) {
54+
return Promise.reject(error);
55+
}
56+
57+
// catch any request errors first, which isn't a ProcessError
58+
if (!error.stderr) {
59+
return Promise.reject(new cli.errors.CliError({
60+
message: 'Unable to query GitHub for ACME download URL',
61+
err: error
62+
}));
63+
}
64+
return Promise.reject(new cli.errors.ProcessError(error));
65+
});
5366
}
5467

5568
function generateCert(ui, domain, webroot, email, staging) {
@@ -64,13 +77,16 @@ function generateCert(ui, domain, webroot, email, staging) {
6477

6578
if (error.stderr.match(/Verify error:(Fetching|Invalid Response)/)) {
6679
// Domain verification failed
67-
return Promise.reject(new cli.errors.SystemError(
68-
'Your domain name is not pointing to the correct IP address of your server, please update it and run `ghost setup ssl` again'
69-
));
80+
return Promise.reject(new cli.errors.SystemError('Your domain name is not pointing to the correct IP address of your server, please update it and run `ghost setup ssl` again'));
81+
} else if (error.stderr) {
82+
// Use ProcessError only for errors with stderr
83+
return Promise.reject(new cli.errors.ProcessError(error));
7084
}
7185

72-
// It's not an error we expect might happen, throw a ProcessError instead.
73-
return Promise.reject(new cli.errors.ProcessError(error));
86+
return Promise.reject(new cli.errors.CliError({
87+
message: 'Error trying to generate your SSL certificate',
88+
err: error
89+
}));
7490
});
7591
}
7692

‎extensions/nginx/test/acme-spec.js

+10-9
Original file line numberDiff line numberDiff line change
@@ -71,13 +71,11 @@ describe('Unit: Extensions > Nginx > Acme', function () {
7171
});
7272

7373
it('Errors when github is down', function () {
74-
const dwUrl = 'https://ghost.org/download'
75-
const fakeResponse = {
76-
body: JSON.stringify({tarball_url: dwUrl}),
77-
statusCode: 502
78-
};
79-
80-
const gotStub = sinon.stub().resolves(fakeResponse);
74+
const err = new Error('Not Found');
75+
err.statusCode = '404';
76+
// got resolves only, when statusCode = 2xx
77+
// see https://github.com/sindresorhus/got#gothttperror
78+
const gotStub = sinon.stub().rejects(err);
8179
const existsStub = sinon.stub().returns(false);
8280
const emptyStub = sinon.stub();
8381
const rdsStub = sinon.stub().returns(['cake']);
@@ -96,6 +94,7 @@ describe('Unit: Extensions > Nginx > Acme', function () {
9694
}).catch((reject) => {
9795
expect(reject).to.exist;
9896
expect(reject.message).to.match(/query github/i);
97+
expect(reject.err.message).to.match(/not found/i);
9998
expect(logStub.calledTwice).to.be.true;
10099
expect(sudoStub.calledOnce).to.be.true;
101100
expect(emptyStub.calledOnce).to.be.true;
@@ -129,6 +128,7 @@ describe('Unit: Extensions > Nginx > Acme', function () {
129128
}).catch((reject) => {
130129
expect(reject).to.exist;
131130
expect(reject.message).to.match(/parse github/i);
131+
expect(reject.err.message).to.match(/unexpected token/i);
132132
expect(logStub.calledTwice).to.be.true;
133133
expect(sudoStub.calledOnce).to.be.true;
134134
expect(emptyStub.calledOnce).to.be.true;
@@ -138,7 +138,7 @@ describe('Unit: Extensions > Nginx > Acme', function () {
138138
});
139139

140140
it('Rejects when acme.sh fails', function () {
141-
const gotStub = sinon.stub().rejects(new Error('Uh-oh'));
141+
const gotStub = sinon.stub().rejects({stderr: 'CODE: ENOTFOUND'});
142142
const emptyStub = sinon.stub();
143143
const existsStub = sinon.stub().returns(false);
144144
const downloadStub = sinon.stub().resolves();
@@ -155,7 +155,8 @@ describe('Unit: Extensions > Nginx > Acme', function () {
155155
return acme.install({sudo: sudoStub, logVerbose: logStub}).then(() => {
156156
expect(false, 'Promise should have been rejected').to.be.true;
157157
}).catch((reject) => {
158-
expect(reject.message).to.equal('Uh-oh');
158+
expect(reject.message).to.equal('An error occurred.');
159+
expect(reject.options.stderr).to.equal('CODE: ENOTFOUND');
159160
expect(logStub.calledTwice).to.be.true;
160161
expect(sudoStub.calledOnce).to.be.true;
161162
expect(emptyStub.calledOnce).to.be.true;

‎lib/errors.js

+42-6
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,30 @@ class CliError extends Error {
2424

2525
this.context = options.context || {};
2626
this.options = options;
27+
28+
const originalError = {};
29+
2730
this.help = 'Please refer to https://docs.ghost.org/v1/docs/troubleshooting#section-cli-errors for troubleshooting.'
31+
32+
if (options.err) {
33+
if (typeof options.err === 'string') {
34+
options.err = new Error(options.err);
35+
}
36+
37+
Object.getOwnPropertyNames(options.err).forEach((property) => {
38+
if (['response', 'headers'].indexOf(property) !== -1) {
39+
return;
40+
}
41+
42+
// TODO: we receive all possible properties now, except the excluded ones above
43+
// Currently we're logging only the message and the stack property.
44+
// This part of the code could probably be simplyfied if we won't need other
45+
// properties in the future
46+
originalError[property] = options.err[property];
47+
});
48+
}
49+
50+
this.err = originalError;
2851
}
2952

3053
/**
@@ -52,7 +75,15 @@ class CliError extends Error {
5275
let output = `${chalk.yellow('Message:')} ${this.message}\n`;
5376

5477
if (verbose) {
55-
output += `${chalk.yellow('Stack:')} ${this.stack}\n`;
78+
output += `${chalk.yellow('Stack:')} ${this.stack}\n\n`;
79+
80+
if (this.err && this.err.message) {
81+
output += `${chalk.green('Original Error Message:')}\n`
82+
output += `${chalk.gray('Message:')}: ${this.err.message}\n`
83+
if (this.err.stack) {
84+
output += `${chalk.gray('Stack:')}: ${this.err.stack}\n`
85+
}
86+
}
5687
}
5788

5889
if (this.options.help) {
@@ -83,11 +114,16 @@ class ProcessError extends CliError {
83114
output += chalk.yellow(`Exit code: ${this.options.code}\n\n`);
84115
}
85116

86-
if (verbose && this.options.stdout) {
87-
output += chalk.grey('--------------- stdout ---------------\n') +
88-
`${this.options.stdout}\n\n` +
89-
chalk.grey('--------------- stderr ---------------\n') +
90-
`${this.options.stderr}\n`;
117+
if (verbose) {
118+
if (this.options.stdout) {
119+
output += chalk.grey('--------------- stdout ---------------\n') +
120+
`${this.options.stdout}\n\n`;
121+
}
122+
123+
if (this.options.stderr) {
124+
output += chalk.grey('--------------- stderr ---------------\n') +
125+
`${this.options.stderr}\n`;
126+
}
91127
}
92128

93129
return output;

‎lib/utils/resolve-version.js

+2-1
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,8 @@ module.exports = function resolveVersion(version, update) {
5353
} catch (e) {
5454
return Promise.reject(new errors.CliError({
5555
message: 'Ghost-CLI was unable to load versions from Yarn.',
56-
log: false
56+
log: false,
57+
err: e
5758
}));
5859
}
5960
});

‎test/unit/errors-spec.js

+14
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,20 @@ describe('Unit: Errors', function () {
4646
expect(errorOutput).to.match(/Message: some error/);
4747
expect(errorOutput).to.match(/Help: some help message/);
4848
});
49+
50+
it('logs original error message and stack trace if verbose is set', function () {
51+
const originalError = new Error('something aweful happened here');
52+
const verboseError = new errors.CliError({
53+
message: 'some error',
54+
err: originalError
55+
});
56+
57+
const errorOutput = stripAnsi(verboseError.toString(true));
58+
expect(errorOutput).to.match(/Message: some error/);
59+
expect(errorOutput).to.match(/Original Error Message:/);
60+
expect(errorOutput).to.match(/Message:: something aweful happened here/);
61+
expect(errorOutput.indexOf(verboseError.stack)).to.not.equal(-1);
62+
});
4963
});
5064

5165
describe('ProcessError', function () {

0 commit comments

Comments
 (0)