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

Add confine option that confines file responses #47

Merged
merged 2 commits into from May 9, 2016

Conversation

@kanongil
Copy link
Member

kanongil commented Oct 24, 2015

The is a breaking change, since I decided to use true as the default value.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Oct 29, 2015

The baseDir option should probably also apply to the directory handler, to prevent serving files outside of relativeTo by default.

I plan to amend the PR with this feature.

@kanongil kanongil force-pushed the kanongil:confine-file-path branch 2 times, most recently from eed1994 to 6468c7e Nov 8, 2015
@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Nov 8, 2015

Rebased patch and split into sub patches, renaming the option to confine.

I decided not to include any confine option for the directory handler, as I think it will be more of a nuisance than an actual security feature.

I plan to merge this soon unless there are any objections or concerns?

@kanongil kanongil changed the title Add baseDir option that confines file responses Add confine option that confines file responses Nov 8, 2015
@hueniverse hueniverse added the security label Nov 9, 2015
@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 10, 2015

I don't get it.

I understand how this can be useful in a directory handler where you are serving content based on the request, but is there a real use case for file when you specify the file you want?

Also, directory already have this: https://github.com/hapijs/inert/blob/master/lib/directory.js#L81-L83
Is that not enough?

Seems like an odd feature for the file handler. I can see how it might be useful for the reply.file() method in case you pass it a runtime filename.

@kanongil

This comment has been minimized.

Copy link
Member Author

kanongil commented Nov 10, 2015

Yeah, it is primarily targeted at reply.file() but also the plain file handler with a function based path.

The other target is the directory handler, where it allows me to remove the hack you mention and fix #5.

I am definitely open to change the defaults for the file handler if you think it will cause issues?

@hueniverse

This comment has been minimized.

Copy link
Member

hueniverse commented Nov 10, 2015

Ok. We can give this a try. See how many people report issues. But need to apply to directory.

@kanongil kanongil force-pushed the kanongil:confine-file-path branch from 283ed23 to 41732aa Dec 23, 2015
@chrisisbeef

This comment has been minimized.

Copy link

chrisisbeef commented Feb 3, 2016

@hueniverse / @kanongil - I am curious why this PR hasn't been merged in and pushed yet? This seems to resolve this particular issue, and more importantly is solves a very real security issue. Can you guys provide us with an update on this PR?

@kanongil kanongil force-pushed the kanongil:confine-file-path branch from 41732aa to 98b502c May 9, 2016
@kanongil kanongil force-pushed the kanongil:confine-file-path branch from 98b502c to 7251728 May 9, 2016
@kanongil kanongil merged commit d53a0ec into hapijs:master May 9, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@kanongil kanongil added this to the 4.0.0 milestone May 9, 2016
@kanongil kanongil self-assigned this May 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.