Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace 404 response with 403 and 500 when appropriate #18

Merged
merged 1 commit into from Jan 21, 2015

Conversation

@kanongil
Copy link
Member

kanongil commented Jan 16, 2015

This fixes a somewhat serious issue where clients would get a 404 responses when open fails because of EMFILE, etc. This should not happen for this kind of ephemeral error.

The patch will now only return 404 when the filesystem reports ENOENT. Otherwise, it will detect permission errors and return 403 responses to indicate this, and finally it can fail with an internal error, exposing the actual error.

@kanongil kanongil added the bug label Jan 16, 2015
lib/file.js Outdated
return callback(Boom.notFound());
if (err.code === 'ENOENT') {
return callback(Boom.notFound());
} else if (err.code === 'EACCES' | err.code === 'EPERM') {

This comment has been minimized.

Copy link
@hueniverse

hueniverse Jan 16, 2015

Member

No need for else.
Is the | a typo for ||?

This comment has been minimized.

Copy link
@kanongil

kanongil Jan 18, 2015

Author Member

I used a single | to retain code coverage. The new tests will trigger EACCES on linux but on windows EPERM is triggered instead. Afaik both values indicate a permission issue.

This comment has been minimized.

Copy link
@hueniverse

hueniverse Jan 20, 2015

Member

ugh. you should find another way.

This comment has been minimized.

Copy link
@kanongil

kanongil Jan 21, 2015

Author Member

Ok, I fixed it with additional mocking. I suspect a more proper solution would be to write a new module that converts syserr to Boom errors and use that instead. Any thoughts on such a module @arb ?

@hueniverse hueniverse self-assigned this Jan 16, 2015
@kanongil kanongil force-pushed the kanongil:open-error-details branch from de55d05 to 811d51f Jan 21, 2015
@hueniverse hueniverse added this to the 2.1.1 milestone Jan 21, 2015
return callback(Boom.forbidden());
}

return callback(Boom.wrap(err, null, 'failed to open file'));

This comment has been minimized.

Copy link
@hueniverse

hueniverse Jan 21, 2015

Member

Style nit: Capitalize failed, also line 205 missing space after function

hueniverse added a commit that referenced this pull request Jan 21, 2015
Replace 404 response with 403 and 500 when appropriate
@hueniverse hueniverse merged commit b6c935b into hapijs:master Jan 21, 2015
1 check passed
1 check passed
continuous-integration/travis-ci The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.