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

When .hidden is false, also hide files from a hidden directory #14

Closed
wants to merge 0 commits into from
Closed

When .hidden is false, also hide files from a hidden directory #14

wants to merge 0 commits into from

Conversation

dzcpy
Copy link
Contributor

@dzcpy dzcpy commented Feb 3, 2015

See koajs/file-server#5
We should protect files in directories like /webroot/public/static/.git/config, but not /webroot/.www/public/static/index.html

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.0% when pulling 0ad4919 on andyhu:fix-hidden into e86357b on koajs:master.

@dzcpy
Copy link
Contributor Author

dzcpy commented Feb 3, 2015

It's weird that even I don't touch anything with the original git clone of this repo, I still get a lot of errors in the test.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.0% when pulling 4585109 on andyhu:fix-hidden into e86357b on koajs:master.

@haoxins
Copy link
Member

haoxins commented Feb 3, 2015

@andyhu it seems that the gzip.json.gz is wrong.

@haoxins
Copy link
Member

haoxins commented Feb 3, 2015

@andyhu rebase with master, and it should be OK.

@dzcpy
Copy link
Contributor Author

dzcpy commented Feb 3, 2015

will try to fix it

@dzcpy
Copy link
Contributor Author

dzcpy commented Feb 3, 2015

Now it should be ok, all the tests are passed

// (^|[\/]) matches a path separator or start of the string, . matches leading dot
// while (?!.[/\]) makes sure that something like /../ should not be matched
// and is passed to resolve-path to get the correct error response
return /(^|[\\\/])\.(?!\.[\\\/])/.test(path);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyhu you check path before normalize, so /./ will treated as is hidden.

I'd rather this
https://github.com/pillarjs/send/blob/master/index.js#L736-L744

wait @jonathanong 's decision

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like this method better simply because it's easier to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I agree with that. Using regex is actually more verbose since I have to explain it in the comment..

@dzcpy
Copy link
Contributor Author

dzcpy commented Feb 3, 2015

@coderhaoxin I see what you mean, will modify the code to address it
The reason why I put the check before normalize is that if the user put the webroot to let's say /home/.www/public, then all of static files will be hidden. But as you pointed out, it's not correct. (I've fixed that in the commit below)

@coveralls
Copy link

Coverage Status

Coverage remained the same at 95.0% when pulling c39a7f1 on andyhu:fix-hidden into 3e9bc12 on koajs:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.08%) to 95.08% when pulling d2c3c46 on andyhu:fix-hidden into 3e9bc12 on koajs:master.

@jonathanong
Copy link
Member

i was looking to use https://github.com/pillarjs/resolve-path for this module, but ran into issues. ideally, this would be solved by resolve-path and this module would simply require it.

@jonathanong
Copy link
Member

actually no... this isn't a security issue...

@dzcpy
Copy link
Contributor Author

dzcpy commented Feb 3, 2015

Yes it's probably not a security issue, the end user has some responsibility to take care of their public directories. So for this specific issue, do you suggest to raise an issue or PR on resolve-path? I'm building a framework based on a bunch of koa-* modules and used this module to serve static files, so I hope it will be rock solid and configurable to the end user.

@jonathanong
Copy link
Member

it makes sense for it to be covered by .hidden.

i just merged your other PR so you're going to need to rebase this PR!

@jonathanong
Copy link
Member

btw you should be using git rebase more!

@dzcpy
Copy link
Contributor Author

dzcpy commented Feb 6, 2015

Thanks! It's a shame that I never read git doc carefully

@jonathanong
Copy link
Member

will you be rebasing soon?

@dzcpy dzcpy closed this Feb 8, 2015
@dzcpy
Copy link
Contributor Author

dzcpy commented Feb 8, 2015

Sorry, not sure why it's automatically closed after rebase, but I will open a new PR soon

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants