◐ Shell
reader mode source ↗
Skip to content

gh-87389: avoid treating path as URI with netloc#93894

Open
nascheme wants to merge 19 commits into
python:mainfrom
nascheme:urlunsplit_relative
Open

gh-87389: avoid treating path as URI with netloc#93894
nascheme wants to merge 19 commits into
python:mainfrom
nascheme:urlunsplit_relative

Conversation

@nascheme

@nascheme nascheme commented Jun 16, 2022 •
edited
Loading

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.

@nascheme
wip: alternative fix for pythongh-87389 …
4f76c44 
still needs test for urlunsplit() change
@nascheme nascheme added type-security 3.11 only security fixes labels Jun 16, 2022
@nascheme nascheme mentioned this pull request Jun 16, 2022
Merged
@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.

@nascheme nascheme changed the title gh-87389: alternative fix, WIP 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.

@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.

@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().
9 hidden items Load more…
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?

@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 needs backport to 3.10 only security fixes 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.

@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().

@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.

@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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

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?

@bedevere-app bedevere-app Bot mentioned this pull request May 14, 2024
Closed
@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.

@github-actions github-actions Bot added the stale label Apr 11, 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

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.

6 participants

@nascheme @bedevere-bot @ambv @gpshead @vadmium @serhiy-storchaka