GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-87389: avoid treating path as URI with netloc by nascheme · Pull Request #93894 · python/cpython

Conversation

@nascheme

@nascheme nascheme commented Jun 16, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I believe this is a more correct fix for gh-87389 and may fix security bugs in other applications using urlunsplit(). We should not be confusing the path argument as a netloc, IMHO. I fixed http.server by not using urllib.parse on the path. For http.server, that part of the HTTP request is not treated as a full URL but instead a path + optional query + optional fragment. So just parsing it "by hand" and not using urllib.parse avoids the bug.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@nascheme
wip: alternative fix for pythongh-87389
4f76c44 
still needs test for urlunsplit() change
@nascheme nascheme added type-security A security issue 3.11 only security fixes labels Jun 16, 2022
@nascheme nascheme assigned gpshead and nascheme Jun 16, 2022
@bedevere-bot bedevere-bot added the awaiting core review label Jun 16, 2022
@nascheme nascheme mentioned this pull request Jun 16, 2022
gh-87389: Fix an open redirection vulnerability in http.server. #93879
Merged
nascheme added 2 commits June 16, 2022 10:52
@nascheme
Don't strip slashes if not building relative URL.
06b3879
@nascheme
Add unit test for urlunsplit relative URL case.
00a3a92
gpshead
gpshead requested changes Jun 16, 2022
View reviewed changes
Comment thread Lib/urllib/parse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/urllib/parse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/http/server.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting core review label Jun 16, 2022
@bedevere-bot

bedevere-bot commented Jun 16, 2022

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Jun 16, 2022
@gpshead gpshead added needs backport to 3.7 needs backport to 3.11 only security fixes labels Jun 16, 2022
nascheme added 4 commits June 16, 2022 12:10
@nascheme
sanitize paths that can be confused as scheme
a8e1cc2
@nascheme
Add blurb.
83c8332
@nascheme
Improve name and comment for _get_redirect_url().
f99e80b
@nascheme
Add urllib.parse.pathsplit() function.
e542578
@nascheme nascheme changed the title gh-87389: alternative fix, WIP gh-87389: avoid treating path as URI with netloc Jun 16, 2022
@nascheme

nascheme commented Jun 16, 2022

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

I added urllib.parse.pathsplit() as suggested. Not sure about the name but it seems consistent with the module. Hopefully I got the doc markup correct.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added awaiting change review and removed awaiting changes labels Jun 16, 2022
@bedevere-bot

bedevere-bot commented Jun 16, 2022

Copy link
Copy Markdown

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot
bedevere-bot requested a review from gpshead June 16, 2022 19:55
nascheme added 2 commits June 16, 2022 12:58
@nascheme
Fix blurb formating, add note about pathsplit().
b7b0b15
@nascheme
Improve comments and docs.
6915331 
The pathsplit() function will work correctly on relative paths too so
don't say "absolute paths".  Improve comment for _get_redirect_url().
gpshead
gpshead reviewed Jun 16, 2022
View reviewed changes
Comment thread Lib/http/server.py Outdated
# reported in gh-87389, a path starting with a double slash should not be
# treated as a relative URI. Also, a path with a colon in the first
# component could also be parsed wrongly.
parts = urllib.parse.pathsplit(path)

@gpshead gpshead Jun 16, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Unfortunately BaseHTTPRequestHandler self.path isn't guaranteed to be only a pathname by anything.

GET http://netloc/path/to/thing HTTP/1.0

Is a valid HTTP query to many http servers. Our existing http server code ignores the scheme and netloc on that and serves up /path/to/thing properly. This code might break things doing that? ie: if http://netloc winds up in the returned path, the it seems like that'd move through pathsplit and urlunsplit properly only by chance rather than design just given the name and purpose of the pathsplit API vs what was just handed to it as input.

Check out my updated test in #87389.

I have no idea if anything depends on this as it should be unusual for any client not pretending to be talking to an HTTP proxy to make requests that way as that isn't normal per the http/1.0 and 1.1 RFCs but as it worked before and other servers like Apache continue to support it, I don't think we can break that - at least not in a bugfix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@nascheme nascheme Jun 16, 2022

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Based on my reading of the RFC, GET http://netloc/path/to/thing HTTP/1.0 should technically be allowed. However, http.server doesn't handle it, before or after this PR. Instead, it tries to use the second word as an absolute file path. I feel like we should not attempt to fix http.server to handle the above. For HTTP 1.0, using only a file path was required. For curl I notice that even with the --http1.1 flag, it passes a path and not the URL with the scheme+netloc.

The bug causing #87389, IMHO, is that http.server was not consistent in how it was treating self.path. The translate_path() method was treating it as a path and not a URL. But, the redirect to add a trailing slash (done only if a folder with that name was found), parsed it as allowing a full URL. It needs to do it one way or the other consistently. The _request_path_split() method I added does this. It doesn't make sense to add this to urllib.parse since what http.server is doing is not modern HTTP.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
gpshead reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Lib/urllib/parse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Misc/NEWS.d/next/Security/2022-06-16-12-13-55.gh-issue-87389.MS9wAR.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_urlparse.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Lib/test/test_httpservers.py Resolved

Uh oh!

There was an error while loading. Please reload this page.
nascheme added 5 commits June 16, 2022 14:27
@nascheme
Use pathsplit() in translate_path().
899f512 
This avoids "manual parsing" of the Request-URI part of the request and
matches what _get_redirect_url() does.
@nascheme
Improved unit tests from gps.
a00656c
@nascheme
Add test for Request-URI containing scheme.
8985853
@nascheme
Make _get_redirect_url() into a method.
f1f94ae 
Possible that someone could override this so a method is nicer.
@nascheme
Futher cleanups, remove urllib.parse.pathsplit().
8a34cd0 
Since pathsplit() doesn't seem like a generally useful public API,
remove it.  Instead, add a _request_path_split() method.  This ensures
that the redirect logic and the translate_path() method use the same
path parsing.
@ambv

ambv commented Jun 22, 2022

Copy link
Copy Markdown
Contributor

@gpshead what do you think about landing this change for 3.12 only as an enhancement?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead
reword news
23d4b56
@gpshead gpshead added type-feature A feature request or enhancement stdlib Standard Library Python modules in the Lib/ directory 3.12 only security fixes and removed needs backport to 3.7 3.11 only security fixes needs backport to 3.11 only security fixes labels Jul 3, 2022
@gpshead

gpshead commented Jul 3, 2022

Copy link
Copy Markdown
Member

@gpshead what do you think about landing this change for 3.12 only as an enhancement?

Yeah I think this landing in 3.12 as a feature makes sense. this PR branch needs syncing now that the other PR has gone in.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@nascheme

nascheme commented Jul 4, 2022

Copy link
Copy Markdown
Member Author

A few thoughts about this. I can rework the PR so that changes to urlib.parse are separated from the httpserver change. The fix made is 3.11 is sufficient for fixing the security bug so the change in this PR isn't strictly required.

The change to urllib.parse.urlunsplit needs careful consideration. Not merging it into 3.11 was the right move. Thinking about it more, I'm having doubts that we can change this. Maybe we need a new API or maybe we just can't change it. It seems almost certain that some code expects the existing (IMHO insecure) behaviour of urlunsplit(). E.g. putting the full URL in the path arg, like this::

>>> urllib.parse.urlunsplit(('', '', 'http://foo.bar/baz', '', ''))
'http://foo.bar/baz'

We could try changing it an alpha and see what breaks. Or, we can try to analyze publicly available code and see how urlunsplit() is used.

Ideally we should analyze all the arguments of urlunsplit() and determine if we want to do sanity checking or cleanup on them. My change adds checking of the path to ensure it doesn't get confused with scheme or netloc. I suspect there are other sanity checks that could be done. E.g. preventing special characters like ? and # in parameters. Maybe we need urlunsplit_safe().
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead

gpshead commented Jul 18, 2022

Copy link
Copy Markdown
Member

Thinking about it more, I'm having doubts that we can change this. Maybe we need a new API or maybe we just can't change it. It seems almost certain that some code expects the existing (IMHO insecure) behaviour of urlunsplit(). E.g. putting the full URL in the path arg

Agreed, it's a challenge. I'm not personally motivated to try and push for this change to the level of doing code analysis of code-at-large's urlunsplit API calls.

I like your thinking that further analysis of all the safety checks required being good. There are probably useful things to learn from what other libraries (in any language) do for URL construction from their parts libraries. Adding this is always easier as a _safe API or with a bool flag to enable strict/pedantic/safe/bikeshedcolor mode.

I suggest this get tracked as its own Enhancement request Issue. This PR can presumably be tied to that and closed for now given more not yet fully defined work is needed if it is going to be done at all.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@gpshead gpshead removed their assignment May 19, 2023
@gpshead gpshead removed the 3.12 only security fixes label May 19, 2023
vadmium
vadmium reviewed Apr 28, 2024
View reviewed changes

@vadmium vadmium left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There seem to be three behaviour changes proposed:

  1. Changing urlunsplit(('', '', '//path', '', '')) to return '/path' instead of '//path'

Another option is to prefix with double slash, representing an empty host e.g. '////path'. This is proposed in issue #78457 and pull request #113563. I think I prefer that four-slash option, because it also fixes some urlsplit → urlunsplit round-trip cases.

  1. Changing urlunsplit(('', '', 'colon:path', '', '')) → './colon:path'

This seems a reasonable change, and it is kind of suggested in RFC 3986. (Another option might be to encode the first colon, and return 'colon%3Apath'.)

  1. SimpleHTTPRequestHandler’s handling of GET https://example.net/dir

This is a legal HTTP 1.0 and 1.1 request, but is mainly for proxy servers, which is not what SimpleHTTPRequestHandler does. In this case the server looks up https:/example.net/dir as a path in its filesystem (which is not in spirit of HTTP), and decides to redirect with a trailing slash.

Currently it looks like the code sends Location: https://example.net/dir/. I don’t think there is anything really wrong with that.

The proposed changes would send Location: ./https://example.net/dir/. This new redirect is a path-relative URL. The base URL is supposed to be the original target https://example.net/dir, so the redirect would resolve to https://example.net/https://example.net/dir/, which is not intended.

If you want to fix anything in the HTTP server, I would make the server ignore the scheme and authority components, and just look up the path component. But I don’t think anyone is complaining about that, so it may not be worth fixing.

If the urllib.parse changes are too disruptive, perhaps a deprecation warning is the best way forward, and either add an opt-in way to get the new behaviour, or change the warning to an exception in the future?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/urllib/parse.py

# Returns true if path can confused with a scheme. I.e. a relative path
# without leading dot that includes a colon in the first component.
_is_scheme_like = re.compile(r'[^/.][^/]*:').match

@vadmium vadmium Apr 28, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why the special allowance for a leading dot? Is there a test case for it? Yes, a scheme cannot start with a dot, but a path-noscheme component of .: is no more legal than https: according to RFC 3986.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Lib/test/test_urlparse.py
# expected result is a relative URL without netloc and scheme
(('', 'a', '', '', ''), '//a'),
# extra leading slashes need to be stripped to avoid confusion
# with a relative URL

@vadmium vadmium Apr 28, 2024

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

confusion with a protocol-relative URL? [as opposed to a host-relative URL]

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@serhiy-storchaka
Merge branch 'main' into urlunsplit_relative
7a71381
@bedevere-app bedevere-app Bot mentioned this pull request May 14, 2024
[security] CVE-2021-28861: http.server: Open Redirection if the URL path starts with // #87389
Closed
@serhiy-storchaka
Merge branch 'main' into urlunsplit_relative
d18bbd9
@github-actions

github-actions Bot commented Apr 11, 2026

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 11, 2026
@ambv

ambv commented Oct 20, 2026

Copy link
Copy Markdown
Contributor

This PR is attached to gh-87389, which is already completed: the fix for that issue landed through other changes, and this PR's approach was not taken or has since become obsolete. Closing to cull the number of open pull requests. Do not reopen this pull request without getting a go-ahead from a triager or core developer.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ambv ambv closed this Oct 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead requested changes
@vadmium vadmium vadmium left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@nascheme nascheme

Labels

awaiting change review stale Stale PR or inactive for long period of time. stdlib Standard Library Python modules in the Lib/ directory type-feature A feature request or enhancement type-security A security issue

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@nascheme @bedevere-bot @ambv @gpshead @vadmium @serhiy-storchaka
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.