Skip to content

Commit

Permalink
feat(ssl) improve error messages to log orignal error
Browse files Browse the repository at this point in the history 
refs TryGhost#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
  • Loading branch information
@aileen
aileen committed Jan 25, 2018
1 parent c25c141 commit 0cc6821
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 26 deletions.
38 changes: 27 additions & 11 deletions extensions/nginx/acme.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,14 +27,13 @@ function install(ui, task) {
ui.logVerbose('ssl: downloading acme.sh to temporary directory', 'green');
return fs.emptyDir(acmeTmpDir)
}).then(() => got(acmeApiUrl)).then((response) => {
if (response.statusCode !== 200) {
return Promise.reject(new cli.errors.CliError('Unable to query GitHub for ACME download URL'));
}

try {
response = JSON.parse(response.body).tarball_url;
} catch (e) {
return Promise.reject(new cli.errors.CliError('Unable to parse Github api response for acme'));
return Promise.reject(new cli.errors.CliError({
message: 'Unable to parse Github api response for acme',
err: e
}));
}

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

// Installs acme.sh into /etc/letsencrypt
return ui.sudo('./acme.sh --install --home /etc/letsencrypt', {cwd: acmeCodeDir});
}).catch((error) => Promise.reject(new cli.errors.ProcessError(error)));
}).catch((error) => {
// CASE: error is already a cli error, just pass it along
if (error instanceof cli.errors.CliError) {
return Promise.reject(error);
}

// catch any request errors first, which isn't a ProcessError
if (!error.stderr) {
return Promise.reject(new cli.errors.CliError({
message: 'Unable to query GitHub for ACME download URL',
err: error
}));
}
return Promise.reject(new cli.errors.ProcessError(error));
});
}

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

if (error.stderr.match(/Verify error:(Fetching|Invalid Response)/)) {
// Domain verification failed
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'
));
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'));
} else if (error.stderr) {
// Use ProcessError only for errors with stderr
return Promise.reject(new cli.errors.ProcessError(error));
}

// It's not an error we expect might happen, throw a ProcessError instead.
return Promise.reject(new cli.errors.ProcessError(error));
return Promise.reject(new cli.errors.CliError({
message: 'Error trying to generate your SSL certificate',
err: error
}));
});
}

Expand Down
19 changes: 10 additions & 9 deletions extensions/nginx/test/acme-spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,11 @@ describe('Unit: Extensions > Nginx > Acme', function () {
});

it('Errors when github is down', function () {
const dwUrl = 'https://ghost.org/download'
const fakeResponse = {
body: JSON.stringify({tarball_url: dwUrl}),
statusCode: 502
};

const gotStub = sinon.stub().resolves(fakeResponse);
const err = new Error('Not Found');
err.statusCode = '404';
// got resolves only, when statusCode = 2xx
// see https://github.com/sindresorhus/got#gothttperror
const gotStub = sinon.stub().rejects(err);
const existsStub = sinon.stub().returns(false);
const emptyStub = sinon.stub();
const rdsStub = sinon.stub().returns(['cake']);
Expand All @@ -96,6 +94,7 @@ describe('Unit: Extensions > Nginx > Acme', function () {
}).catch((reject) => {
expect(reject).to.exist;
expect(reject.message).to.match(/query github/i);
expect(reject.err.message).to.match(/not found/i);
expect(logStub.calledTwice).to.be.true;
expect(sudoStub.calledOnce).to.be.true;
expect(emptyStub.calledOnce).to.be.true;
Expand Down Expand Up @@ -129,6 +128,7 @@ describe('Unit: Extensions > Nginx > Acme', function () {
}).catch((reject) => {
expect(reject).to.exist;
expect(reject.message).to.match(/parse github/i);
expect(reject.err.message).to.match(/unexpected token/i);
expect(logStub.calledTwice).to.be.true;
expect(sudoStub.calledOnce).to.be.true;
expect(emptyStub.calledOnce).to.be.true;
Expand All @@ -138,7 +138,7 @@ describe('Unit: Extensions > Nginx > Acme', function () {
});

it('Rejects when acme.sh fails', function () {
const gotStub = sinon.stub().rejects(new Error('Uh-oh'));
const gotStub = sinon.stub().rejects({stderr: 'CODE: ENOTFOUND'});
const emptyStub = sinon.stub();
const existsStub = sinon.stub().returns(false);
const downloadStub = sinon.stub().resolves();
Expand All @@ -155,7 +155,8 @@ describe('Unit: Extensions > Nginx > Acme', function () {
return acme.install({sudo: sudoStub, logVerbose: logStub}).then(() => {
expect(false, 'Promise should have been rejected').to.be.true;
}).catch((reject) => {
expect(reject.message).to.equal('Uh-oh');
expect(reject.message).to.equal('An error occurred.');
expect(reject.options.stderr).to.equal('CODE: ENOTFOUND');
expect(logStub.calledTwice).to.be.true;
expect(sudoStub.calledOnce).to.be.true;
expect(emptyStub.calledOnce).to.be.true;
Expand Down
49 changes: 43 additions & 6 deletions lib/errors.js
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';
const chalk = require('chalk');
const each = require('lodash/each');

/**
* Base CLI Error class. Extends error and augments it
Expand All @@ -24,7 +25,30 @@ class CliError extends Error {

this.context = options.context || {};
this.options = options;

const originalError = {};

this.help = 'Please refer to https://docs.ghost.org/v1/docs/troubleshooting#section-cli-errors for troubleshooting.'

if (options.err) {
if (typeof options.err === 'string') {
options.err = new Error(options.err);
}

Object.getOwnPropertyNames(options.err).forEach((property) => {
if (['response', 'headers'].indexOf(property) !== -1) {
return;
}

// TODO: we receive all possible properties now, except the excluded ones above
// Currently we're logging only the message and the stack property.
// This part of the code could probably be simplyfied if we won't need other
// properties in the future
originalError[property] = options.err[property];
});
}

this.err = originalError;
}

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

if (verbose) {
output += `${chalk.yellow('Stack:')} ${this.stack}\n`;
output += `${chalk.yellow('Stack:')} ${this.stack}\n\n`;

if (this.err && this.err.message) {
output += `${chalk.green('Original Error Message:')}\n`
output += `${chalk.gray('Message:')}: ${this.err.message}\n`
if (this.err.stack) {
output += `${chalk.gray('Stack:')}: ${this.err.stack}\n`
}
}
}

if (this.options.help) {
Expand Down Expand Up @@ -83,11 +115,16 @@ class ProcessError extends CliError {
output += chalk.yellow(`Exit code: ${this.options.code}\n\n`);
}

if (verbose && this.options.stdout) {
output += chalk.grey('--------------- stdout ---------------\n') +
`${this.options.stdout}\n\n` +
chalk.grey('--------------- stderr ---------------\n') +
`${this.options.stderr}\n`;
if (verbose) {
if (this.options.stdout) {
output += chalk.grey('--------------- stdout ---------------\n') +
`${this.options.stdout}\n\n`;
}

if (this.options.stderr) {
output += chalk.grey('--------------- stderr ---------------\n') +
`${this.options.stderr}\n`;
}
}

return output;
Expand Down
14 changes: 14 additions & 0 deletions test/unit/errors-spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,20 @@ describe('Unit: Errors', function () {
expect(errorOutput).to.match(/Message: some error/);
expect(errorOutput).to.match(/Help: some help message/);
});

it('logs original error message and stack trace if verbose is set', function () {
const originalError = new Error('something aweful happened here');
const verboseError = new errors.CliError({
message: 'some error',
err: originalError
});

const errorOutput = stripAnsi(verboseError.toString(true));
expect(errorOutput).to.match(/Message: some error/);
expect(errorOutput).to.match(/Original Error Message:/);
expect(errorOutput).to.match(/Message:: something aweful happened here/);
expect(errorOutput.indexOf(verboseError.stack)).to.not.equal(-1);
});
});

describe('ProcessError', function () {
Expand Down

0 comments on commit 0cc6821

Please sign in to comment.