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

Should the path be normalized? #51

Open
dlmr opened this Issue Jan 20, 2016 · 10 comments

Comments

5 participants
@dlmr

dlmr commented Jan 20, 2016

I'm using koa-send in my Koa server and I have the problem that I get Malicious Path from resolve-path when the request path is something along the lines of http://localhost:3000//some/path/image.jpg.

Would it be a bad idea to path.normalize the path in koa-send before processing it further? This could for instance be done here https://github.com/koajs/send/blob/master/index.js#L46 before doing substring on the path.

@Pumpuli

This comment has been minimized.

Pumpuli commented Feb 27, 2016

I'm on Windows and I get the same error with all requests because resolve-path thinks they are absolute paths. That is because the path always starts with a slash. I assume this line is supposed to remove the leading slash, but on Windows path.parse returns '' for root for paths that start with a slash. Therefore the path will have a leading slash when it's passed to resolve-path.

@coderhaoxin

This comment has been minimized.

Member

coderhaoxin commented Feb 28, 2016

but on Windows path.parse returns '' for root for paths that start with a slash

test with node@4 on win10(VM)

path.parse('/a/b')
{root: '/', ...}
@coderhaoxin

This comment has been minimized.

Member

coderhaoxin commented Feb 28, 2016

@Pumpuli I think your problem is koajs/static#77 ?

@Pumpuli

This comment has been minimized.

Pumpuli commented Feb 28, 2016

I think your problem is koajs/static#77 ?

Yep, seems to be a Node issue, works on Node 5.6.0.

@zbinlin

This comment has been minimized.

zbinlin commented Feb 29, 2016

@coderhaoxin
In Node 5.7.0

path.parse("/a/b")

returns

{ root: '', dir: '/a', base: 'b', ext: '', name: 'b' }

I suggest use path.posix.parse instead of path.parse.

@coderhaoxin

This comment has been minimized.

Member

coderhaoxin commented Feb 29, 2016

@zbinlin I saw your PR to node, 5.7.1 will fix this. right?

@zbinlin

This comment has been minimized.

zbinlin commented Feb 29, 2016

@coderhaoxin right, but I also suggest that use path.posix.parse instead of path.parse in koa-send.

@tejasmanohar

This comment has been minimized.

Member

tejasmanohar commented Feb 29, 2016

I also suggest that use path.posix.parse instead of path.parse in koa-send.

@zbinlin why so? shouldn't we use path#parse and let Node determine how to resolve the path dependent on the environment.

@zbinlin

This comment has been minimized.

zbinlin commented Mar 1, 2016

@tejasmanohar I think the path is usually unix path(or url path), so we can only remove first forward slash of the path use path.posix.parse;

@Pumpuli

This comment has been minimized.

Pumpuli commented Mar 1, 2016

Yeah, I think the point is that path#parse is for parsing filesystem paths, but in this case we're parsing the path part of a URL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment