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

Do not strip leading slash from path #1511

Merged
merged 3 commits into from Dec 28, 2017

Conversation

Projects
None yet
4 participants
@YuppY
Contributor

YuppY commented May 11, 2017

No description provided.

@benoitc

This comment has been minimized.

Owner

benoitc commented May 14, 2017

why this PR? What are you trying to fix?

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Jun 14, 2017

This may be relevant:

A URL-path-segment string must be one of the following:

* zero or more URL units, excluding U+002F (/) and U+003F (?), that together are not a single-dot path segment or a double-dot path segment.

Source: https://url.spec.whatwg.org/#url-path-segment-string

Note "zero or more", so "//" is valid in a URL and I guess distinct from "/". We probably shouldn't strip it unless we have a good reason to.

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Jul 11, 2017

@benoitc what do you think?

@tilgovi

This comment has been minimized.

Collaborator

tilgovi commented Dec 10, 2017

This needs a rebase, but I think this should move forward.

@YuppY

This comment has been minimized.

Contributor

YuppY commented Dec 10, 2017

Rebased.

By the way, it fixes security issue: if we configure reverse proxy for gunicorn app that denies access to '/admin/', then an attacker can bypass this rule by requesting 'https://example.org//admin/'.

We patched our proxy configurations to redirect requests from '//+(.+)' to '/$1' but there could be more apps with such vulnerability.

@@ -1,3 +1,4 @@
coverage>=4.0,<4.4 # TODO: https://github.com/benoitc/gunicorn/issues/1548
mock; python_version < "3"

This comment has been minimized.

@tilgovi

tilgovi Dec 10, 2017

Collaborator

This is already handled in setup.py.

This comment has been minimized.

@tilgovi

tilgovi Dec 10, 2017

Collaborator

I think this form is cleaner, but maybe we do that for mock and unittest2 in a separate change.

This comment has been minimized.

@YuppY

YuppY Dec 10, 2017

Contributor

ok. removed this commit from pr

@tilgovi tilgovi requested a review from berkerpeksag Dec 10, 2017

# When the path starts with //, urlsplit considers it as a
# relative uri while the RFC says we should consider it as abs_path
# http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1.2
parts = _compat.urlsplit("." + uri)

This comment has been minimized.

@benoitc

benoitc Dec 10, 2017

Owner

not sure i understand this line. What trigger the "." here ?

This comment has been minimized.

@tilgovi

tilgovi Dec 10, 2017

Collaborator

I'm not sure the comment makes sense, but a quick test in the console shows that this forces urlsplit to consider the string as a path—with no scheme or netloc—rather than parsing this as a scheme-relative URL with the first path component as the netloc.

This comment has been minimized.

@YuppY

YuppY Dec 11, 2017

Contributor

"." forces urlsplit to parse path part as RFC says:

In [5]: urlsplit('//path?query#fragment')
Out[5]: SplitResult(scheme='', netloc='path', path='', query='query', fragment='fragment')

In [6]: urlsplit('.//path?query#fragment')
Out[6]: SplitResult(scheme='', netloc='', path='.//path', query='query', fragment='fragment')

This comment has been minimized.

@berkerpeksag

berkerpeksag Dec 11, 2017

Collaborator

Can you add a test case in tests/test_util.py? It's easy to introduce a regression when fixing another issue in the urllib module. This way we can know early if something has changed in the development version of Python.

This comment has been minimized.

@benoitc

benoitc Dec 12, 2017

Owner

@tilgovi thanks. imo that worlrth a comment for readability.

@@ -312,18 +312,10 @@ def parse_request_line(self, line_bytes):
self.method = bits[0].upper()
# URI

This comment has been minimized.

@berkerpeksag

berkerpeksag Dec 11, 2017

Collaborator

Can you delete this line too? New code is descriptive enough so we don't need it anymore.

This comment has been minimized.

@YuppY

YuppY Dec 11, 2017

Contributor

It is consistent with other comments in this function: Method and Version.

# When the path starts with //, urlsplit considers it as a
# relative uri while the RFC says we should consider it as abs_path
# http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html#sec5.1.2
parts = _compat.urlsplit("." + uri)

This comment has been minimized.

@berkerpeksag

berkerpeksag Dec 11, 2017

Collaborator

Can you add a test case in tests/test_util.py? It's easy to introduce a regression when fixing another issue in the urllib module. This way we can know early if something has changed in the development version of Python.

@YuppY

This comment has been minimized.

Contributor

YuppY commented Dec 20, 2017

made test and comment in split_request_uri

@tilgovi

Looks great. It appears our linting is failing for PRs in CI, but everything else checks out and that's not your fault!

@YuppY

This comment has been minimized.

Contributor

YuppY commented Dec 28, 2017

do I need to fix something to get it merged?

@berkerpeksag

It looks like we hit https://github.com/landscapeio/prospector/issues/200. I don't see a reason to not merge this because of an unrelated failure.

@berkerpeksag berkerpeksag merged commit 5953148 into benoitc:master Dec 28, 2017

4 of 8 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
couverture-io/py26 Not all changes are covered
Details
couverture-io/py35 Not all changes are covered
Details
couverture-io/py36 Not all changes are covered
Details
couverture-io/py27 Code coverage OK
Details
couverture-io/py34 Code coverage OK
Details
couverture-io/py36-dev Code coverage OK
Details
couverture-io/py37 Code coverage OK
Details
@berkerpeksag

This comment has been minimized.

Collaborator

berkerpeksag commented Dec 28, 2017

Thanks!

berkerpeksag added a commit to berkerpeksag/gunicorn that referenced this pull request Jan 11, 2018

fofanov pushed a commit to fofanov/gunicorn that referenced this pull request Mar 16, 2018

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