Fixes #65: Correctly resolving directory traversal

[dave-newson]

Fixes issue #65; directory traversal in a URL is now resolved when the file path is truncated for the minifier.

Affects \BWP_MINIFY::process_media_source.

../ instances now get removed, including the directory immediately above. http://example.com/foo/bar/../baz.js becomes foo/baz.js

Where traversal cannot be actioned (tries to go above the root), ../ instances are removed entirely. http://example.com/foo/../../bar.js becomes bar.js

This behaviour mimics what most browsers do to URLs with similar directory traversal.

Previously these would not resolve correctly, simply replacing ../ with ./, breaking paths.

[kminh]

This has been fixed locally. We will not replace ../ anymore because it will be handled correctly later on.

IMO it's up to users to provide correct paths to assets, so cases like http://example.com/foo/../../bar.js should not happen. We will simply throw error instead of trying to auto-correct them.

[dave-newson]

@kminh what do you mean by locally? Is it part of master? Looking at master I can't see how this has been resolved.

Regarding the "invalid" urls, id argue it makes sense for bwp minify to not behave different to the browser. If it's more picky about asset paths then you could end up in a situation where assets work with bwp disabled, but error when it's active. That's not a great outcome, right?

[kminh]

By locally I mean my local repository. I will push an update to this repository next week.

Hmm may I have a link to spec that states how a browser should handle http://example.com/foo/../../bar.js ? How can we know whether bar.js can be found at http://example.com/bar.js, or the original asset url is simply invalid (error by user)?

[dave-newson]

@kminh Resolving the directory traversal is described in RFC-3986 Section 5.2.4: Removing Dot Segments

In the case of excessive directory traversal, either cases C or D apply:

  C.  if the input buffer begins with a prefix of "/../" or "/..",
       where ".." is a complete path segment, then replace that
       prefix with "/" in the input buffer and remove the last
       segment and its preceding "/" (if any) from the output
       buffer; otherwise,

  D.  if the input buffer consists only of "." or "..", then remove
       that from the input buffer; otherwise,

To clarify about invalid paths: With BWP disabled and a bad path you'd see a 404 for that asset, but the page would still load. It's up to the developer to debug that asset.

With BWP enabled I'd expect the page to still load, and the asset to still be missing. It's up to the developer to look inside the Minify file and discover the "Missing asset" warning that minify provides.

Also, any reason for not putting your local code to a new branch on github? I wouldn't have bothered writing the PR if I'd have seen the fix in an unreleased branch with more commits, I could have just cherry-picked the fix instead.

[kminh]

I'm not sure if I want to follow the specs, it complicates things and the benefit is still unclear to me.

With BWP disabled and a bad path you'd see a 404 for that asset, but the page would still load. It's up to the developer to debug that asset. With BWP enabled I'd expect the page to still load, and the asset to still be missing. It's up to the developer to look inside the Minify file and discover the "Missing asset" warning that minify provides.

That is the current behaviour I believe. Are you seeing otherwise?

Also, any reason for not putting your local code to a new branch on github? I wouldn't have bothered writing the PR if I'd have seen the fix in an unreleased branch with more commits, I could have just cherry-picked the fix instead.

I'm experimenting a lot of things and I don't feel like they're ready for everyone to see. Your PR is still appreciated because it addresses other things as well, not just the relative path bug.

[dave-newson]

That is the current behaviour I believe. Are you seeing otherwise?

The current behaviour chokes on paths with ../ because of the erroneous substitution of ../ for ./. With the PR this issue is fixed. I'm not sure what your local code does because I can't see it.

The reason I'm still talking about this is because you said:

This has been fixed locally. We will not replace ../ anymore because it will be handled correctly later on.

That's a worry because you mention ../ but not ./.
If you still have the ./ -> / substitution, but don't handle ../ then the paths will still be broken, and the same bug will still exist.

If you meant that none of the path traversal will be processed in BWP anymore, then that also fixes the bug. Again, I've no idea exactly what you meant because I can't see your code.

I'm not sure if I want to follow the specs, it complicates things and the benefit is still unclear to me.

If you're going to resolve the path, you'll want to follow the spec. The existing code doesn't adhere, which is why this bug exists. If you choose not to do the path resolve in BWP, that's fine, because minify / browser will take care of it instead. Whatever you decide, it should follow the spec.

I'm experimenting a lot of things and I don't feel like they're ready for everyone to see.

I see where you're coming from, I too like to experiment and write potentially weird code in private, but while you hide away your local changes, and deny PRs because they conflict with your local changes, without releasing those local changes to the public, you're basically stonewalling contributions to the project.
How is anyone to know if the bug they're looking at hasn't already been fixed, or the file they change hasn't been totally written?

A better method would be:

1. Keep your experiments and ongoing dev-for-next-release as separate branches. Branch off Dev, fix a thing, send it up to GitHub.
2. If you make a bugfix as part of a much larger piece of ongoing work, cherry-pick it out and add it to Dev, and stick it back up on GitHub.
3. Accept GitHub is the source of truth, even for you, and accept PRs over your own local and hidden efforts. Resolve merge conflicts as necessary. Cherry pick your own code back to Dev and use that instead if it's better than the PR.
4. Bugs aren't fixed unless the code is on GitHub.

Sorry if this is coming off as a bit dickish. I like BWP-minify, this is just a very convoluted way of working.

[kminh]

If you meant that none of the path traversal will be processed in BWP anymore, then that also fixes the bug.

Yes that's what I meant.

I appreciate your feedback, and I do love pull requests. However I don't want to manage bug reports/pull requests related to the current refactoring from 1.3.x to 1.4 yet, because everything changes very fast (I change my mind very often).

Anyway, this is getting a little bit off-topic, so we should stop here. Let's check this issue again next week. Thank you.