Skip to content

Commit

Permalink
Fix incorrect error handling (#342) (#346)
Browse files Browse the repository at this point in the history 
* Convert error handler to more modern class declaration.
* Make GitHub and GitService actually throw errors rather than return them
* Fix some broken unit and acceptance tests
  • Loading branch information
@alexwaibel
alexwaibel authored Jul 6, 2020
1 parent 31ab851 commit 5d7ed77
Show file tree
Hide file tree
Showing 6 changed files with 173 additions and 115 deletions.
114 changes: 58 additions & 56 deletions lib/ErrorHandler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,81 +17,83 @@ class ApiError {
}
}

const ErrorHandler = function () {
this.ERROR_MESSAGES = {
'missing-input-secret': 'reCAPTCHA: The secret parameter is missing',
'invalid-input-secret': 'reCAPTCHA: The secret parameter is invalid or malformed',
'missing-input-response': 'reCAPTCHA: The response parameter is missing',
'invalid-input-response': 'reCAPTCHA: The response parameter is invalid or malformed',
'RECAPTCHA_MISSING_CREDENTIALS': 'Missing reCAPTCHA API credentials',
'RECAPTCHA_FAILED_DECRYPT': 'Could not decrypt reCAPTCHA secret',
'RECAPTCHA_CONFIG_MISMATCH': 'reCAPTCHA options do not match Staticman config',
'PARSING_ERROR': 'Error whilst parsing config file',
'GITHUB_AUTH_TOKEN_MISSING': 'The site requires a valid GitHub authentication token to be supplied in the `options[github-token]` field',
'MISSING_CONFIG_BLOCK': 'Error whilst parsing Staticman config file'
class ErrorHandler {
constructor () {
this.ERROR_MESSAGES = {
'missing-input-secret': 'reCAPTCHA: The secret parameter is missing',
'invalid-input-secret': 'reCAPTCHA: The secret parameter is invalid or malformed',
'missing-input-response': 'reCAPTCHA: The response parameter is missing',
'invalid-input-response': 'reCAPTCHA: The response parameter is invalid or malformed',
'RECAPTCHA_MISSING_CREDENTIALS': 'Missing reCAPTCHA API credentials',
'RECAPTCHA_FAILED_DECRYPT': 'Could not decrypt reCAPTCHA secret',
'RECAPTCHA_CONFIG_MISMATCH': 'reCAPTCHA options do not match Staticman config',
'PARSING_ERROR': 'Error whilst parsing config file',
'GITHUB_AUTH_TOKEN_MISSING': 'The site requires a valid GitHub authentication token to be supplied in the `options[github-token]` field',
'MISSING_CONFIG_BLOCK': 'Error whilst parsing Staticman config file'
}

this.ERROR_CODE_ALIASES = {
'missing-input-secret': 'RECAPTCHA_MISSING_INPUT_SECRET',
'invalid-input-secret': 'RECAPTCHA_INVALID_INPUT_SECRET',
'missing-input-response': 'RECAPTCHA_MISSING_INPUT_RESPONSE',
'invalid-input-response': 'RECAPTCHA_INVALID_INPUT_RESPONSE'
}
}

this.ERROR_CODE_ALIASES = {
'missing-input-secret': 'RECAPTCHA_MISSING_INPUT_SECRET',
'invalid-input-secret': 'RECAPTCHA_INVALID_INPUT_SECRET',
'missing-input-response': 'RECAPTCHA_MISSING_INPUT_RESPONSE',
'invalid-input-response': 'RECAPTCHA_INVALID_INPUT_RESPONSE'
getErrorCode (error) {
return this.ERROR_CODE_ALIASES[error] || error
}
}

ErrorHandler.prototype.getErrorCode = function (error) {
return this.ERROR_CODE_ALIASES[error] || error
}
getMessage (error) {
return this.ERROR_MESSAGES[error]
}

ErrorHandler.prototype.getMessage = function (error) {
return this.ERROR_MESSAGES[error]
}
log (err, instance) {
let parameters = {}
let prefix = ''

ErrorHandler.prototype.log = function (err, instance) {
let parameters = {}
let prefix = ''
if (instance) {
parameters = instance.getParameters()

if (instance) {
parameters = instance.getParameters()
prefix += `${parameters.username}/${parameters.repository}`
}

prefix += `${parameters.username}/${parameters.repository}`
console.log(`${prefix}`, err)
}

console.log(`${prefix}`, err)
}
_save (errorCode, data = {}) {
const {err} = data

ErrorHandler.prototype._save = function (errorCode, data = {}) {
const {err} = data
if (err) {
err._smErrorCode = err._smErrorCode || errorCode

if (err) {
err._smErrorCode = err._smErrorCode || errorCode
// Re-wrap API request errors as these could expose
// request/response details that the user should not
// be allowed to see e.g. access tokens.
// `request-promise` is the primary offender here,
// but we similarly do not want others to leak too.
if (
err instanceof StatusCodeError ||
err instanceof RequestError
) {
const statusCode = err.statusCode || err.code

// Re-wrap API request errors as these could expose
// request/response details that the user should not
// be allowed to see e.g. access tokens.
// `request-promise` is the primary offender here,
// but we similarly do not want others to leak too.
if (
err instanceof StatusCodeError ||
err instanceof RequestError
) {
const statusCode = err.statusCode || err.code
return new ApiError(err.message, statusCode, err._smErrorCode)
}

return new ApiError(err.message, statusCode, err._smErrorCode)
return err
}

return err
}
let payload = {
_smErrorCode: errorCode
}

let payload = {
_smErrorCode: errorCode
}
if (data.data) {
payload.data = data.data
}

if (data.data) {
payload.data = data.data
return payload
}

return payload
}

const errorHandler = new ErrorHandler()
Expand Down
4 changes: 2 additions & 2 deletions lib/GitHub.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ class GitHub extends GitService {
console.log(err)
}

return Promise.reject(errorHandler('GITHUB_WRITING_FILE', {err}))
return Promise.reject(errorHandler('GITHUB_WRITING_FILE'))
})
}

Expand Down Expand Up @@ -182,7 +182,7 @@ class GitHub extends GitService {
try {
return await super.readFile(filePath, getFullResponse)
} catch (err) {
return errorHandler('GITHUB_READING_FILE', {err})
throw errorHandler('GITHUB_READING_FILE', {err})
}
}

Expand Down
4 changes: 2 additions & 2 deletions lib/GitService.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ class GitService {
try {
content = Buffer.from(res.content, 'base64').toString()
} catch (err) {
return errorHandler('GITHUB_READING_FILE', {err})
throw errorHandler('GITHUB_READING_FILE', {err})
}

try {
Expand Down Expand Up @@ -85,7 +85,7 @@ class GitService {
errorData.data = err.message
}

return errorHandler('PARSING_ERROR', errorData)
throw errorHandler('PARSING_ERROR', errorData)
}
}

Expand Down
99 changes: 56 additions & 43 deletions test/acceptance/api.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ afterAll(done => {
})

describe('Connect endpoint', () => {
test('accepts the invitation if one is found and replies with "OK!"', () => {
test('accepts the invitation if one is found and replies with "OK!"', async () => {
const invitationId = 123

const reqListInvititations = nock('https://api.github.com', {
Expand All @@ -52,15 +52,13 @@ describe('Connect endpoint', () => {
.patch(`/user/repository_invitations/${invitationId}`)
.reply(204)

return request('/v2/connect/johndoe/foobar')
.then(response => {
expect(reqListInvititations.isDone()).toBe(true)
expect(reqAcceptInvitation.isDone()).toBe(true)
expect(response).toBe('OK!')
})
let response = await request('/v2/connect/johndoe/foobar')
expect(reqListInvititations.isDone()).toBe(true)
expect(reqAcceptInvitation.isDone()).toBe(true)
expect(response).toBe('OK!')
})

test('returns a 404 and an error message if a matching invitation is not found', () => {
test('returns a 404 and an error message if a matching invitation is not found', async () => {
const invitationId = 123
const reqListInvititations = nock('https://api.github.com', {
reqheaders: {
Expand All @@ -85,18 +83,21 @@ describe('Connect endpoint', () => {
.patch(`/user/repository_invitations/${invitationId}`)
.reply(204)

return request('/v2/connect/johndoe/foobar')
.catch(err => {
expect.assertions(4)

try {
await request('/v2/connect/johndoe/foobar')
} catch (err) {
expect(reqListInvititations.isDone()).toBe(true)
expect(reqAcceptInvitation.isDone()).toBe(false)
expect(err.response.body).toBe('Invitation not found')
expect(err.statusCode).toBe(404)
})
}
})
})

describe('Entry endpoint', () => {
test('outputs a RECAPTCHA_CONFIG_MISMATCH error if reCaptcha options do not match (wrong site key)', () => {
test('outputs a RECAPTCHA_CONFIG_MISMATCH error if reCaptcha options do not match (wrong site key)', async () => {
const data = Object.assign({}, helpers.getParameters(), {
path: 'staticman.yml'
})
Expand Down Expand Up @@ -136,23 +137,27 @@ describe('Entry endpoint', () => {
}
const formData = querystring.stringify(form)

return request({
body: formData,
method: 'POST',
uri: `/v2/entry/${data.username}/${data.repository}/${data.branch}/${data.property}`,
headers: {
'content-type': 'application/x-www-form-urlencoded'
}
}).catch((response) => {
expect.assertions(3)

try {
await request({
body: formData,
method: 'POST',
uri: `/v2/entry/${data.username}/${data.repository}/${data.branch}/${data.property}`,
headers: {
'content-type': 'application/x-www-form-urlencoded'
}
})
} catch(response) {
const error = JSON.parse(response.error)

expect(error.success).toBe(false)
expect(error.errorCode).toBe('RECAPTCHA_CONFIG_MISMATCH')
expect(error.message).toBe('reCAPTCHA options do not match Staticman config')
})
}
})

test('outputs a RECAPTCHA_CONFIG_MISMATCH error if reCaptcha options do not match (wrong secret)', () => {
test('outputs a RECAPTCHA_CONFIG_MISMATCH error if reCaptcha options do not match (wrong secret)', async () => {
const data = Object.assign({}, helpers.getParameters(), {
path: 'staticman.yml'
})
Expand Down Expand Up @@ -192,23 +197,27 @@ describe('Entry endpoint', () => {
}
const formData = querystring.stringify(form)

return request({
body: formData,
method: 'POST',
uri: '/v2/entry/johndoe/foobar/master/comments',
headers: {
'content-type': 'application/x-www-form-urlencoded'
}
}).catch(response => {
expect.assertions(3)

try {
await request({
body: formData,
method: 'POST',
uri: '/v2/entry/johndoe/foobar/master/comments',
headers: {
'content-type': 'application/x-www-form-urlencoded'
}
})
} catch (response) {
const error = JSON.parse(response.error)

expect(error.success).toBe(false)
expect(error.errorCode).toBe('RECAPTCHA_CONFIG_MISMATCH')
expect(error.message).toBe('reCAPTCHA options do not match Staticman config')
})
}
})

test('outputs a MISSING_CONFIG_BLOCK error if the site config is malformed', () => {
test('outputs a PARSING_ERROR error if the site config is malformed', async () => {
const data = Object.assign({}, helpers.getParameters(), {
path: 'staticman.yml'
})
Expand Down Expand Up @@ -243,20 +252,24 @@ describe('Entry endpoint', () => {
}
const formData = querystring.stringify(form)

return request({
body: formData,
method: 'POST',
uri: '/v2/entry/johndoe/foobar/master/comments',
headers: {
'content-type': 'application/x-www-form-urlencoded'
}
}).catch(response => {
expect.assertions(4)

try {
await request({
body: formData,
method: 'POST',
uri: '/v2/entry/johndoe/foobar/master/comments',
headers: {
'content-type': 'application/x-www-form-urlencoded'
}
})
} catch (response) {
const error = JSON.parse(response.error)

expect(error.success).toBe(false)
expect(error.errorCode).toBe('MISSING_CONFIG_BLOCK')
expect(error.message).toBe('Error whilst parsing Staticman config file')
expect(error.errorCode).toBe('PARSING_ERROR')
expect(error.message).toBe('Error whilst parsing config file')
expect(error.rawError).toBeDefined()
})
}
})
})
7 changes: 7 additions & 0 deletions test/helpers/sampleData.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,13 @@ module.exports.config3 = `comments:
siteKey: "123456789"
secret: "@reCaptchaSecret@"`

module.exports.configInvalidYML = `invalid:
- x
y
foo
bar
`

module.exports.prBody1 = `Dear human,
Here's a new entry for your approval. :tada:
Expand Down
Loading

0 comments on commit 5d7ed77

Please sign in to comment.