Skip to content

Commit

Permalink
feat(ssl) improve error messages for acme
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
- first step on attaching original error properties to `CliError`
  • Loading branch information
aileen committed Jan 24, 2018
1 parent c25c141 commit f0486aa
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 19 deletions.
36 changes: 26 additions & 10 deletions extensions/nginx/acme.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,6 @@ 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) {
Expand All @@ -49,7 +45,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 +74,19 @@ 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({
message: 'Your domain name is not pointing to the correct IP address of your server, please update it and run `ghost setup ssl` again',
err: error
}));
} 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
15 changes: 6 additions & 9 deletions extensions/nginx/test/acme-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,9 @@ 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);
// got resolves only, when statusCode = 2xx
// see https://github.com/sindresorhus/got#gothttperror
const gotStub = sinon.stub().rejects(new Error({statusMessage: 'Not found', statusCode: 404}));
const existsStub = sinon.stub().returns(false);
const emptyStub = sinon.stub();
const rdsStub = sinon.stub().returns(['cake']);
Expand Down Expand Up @@ -138,7 +134,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 +151,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
3 changes: 3 additions & 0 deletions lib/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,10 @@ class CliError extends Error {
Error.captureStackTrace(this, this.constructor);

this.context = options.context || {};
// TODO: Do something with the original error message which we can receive now as the `err` property
this.err = options.err || {};
this.options = options;

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

Expand Down

0 comments on commit f0486aa

Please sign in to comment.