urljoin fails with messy relative URLs #66316

mlissner · 2014-08-01T13:38:50Z

BPO	22118
Nosy	@ncoghlan, @orsenthil, @pitrou, @scoder, @ezio-melotti, @demianbrecht, @mlissner
Files	issue22118.patch issue22118_1.patch issue22118_2.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2014-08-21.23:17:52.022>
created_at = <Date 2014-08-01.13:38:49.756>
labels = ['easy', 'type-bug', 'library']
title = 'urljoin fails with messy relative URLs'
updated_at = <Date 2014-09-22.07:49:30.042>
user = 'https://github.com/mlissner'

bugs.python.org fields:

activity = <Date 2014-09-22.07:49:30.042>
actor = 'python-dev'
assignee = 'none'
closed = True
closed_date = <Date 2014-08-21.23:17:52.022>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2014-08-01.13:38:49.756>
creator = 'Mike.Lissner'
dependencies = []
files = ['36278', '36296', '36347']
hgrepos = []
issue_num = 22118
keywords = ['patch', 'easy']
message_count = 21.0
messages = ['224500', '224503', '224864', '224870', '224872', '224876', '224877', '224893', '224896', '224984', '225144', '225156', '225173', '225186', '225187', '225191', '225619', '225620', '225661', '225662', '227253']
nosy_count = 8.0
nosy_names = ['ncoghlan', 'orsenthil', 'pitrou', 'scoder', 'ezio.melotti', 'python-dev', 'demian.brecht', 'Mike.Lissner']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue22118'
versions = ['Python 3.5']

mlissner · 2014-08-01T13:38:49Z

Not sure if this is desired behavior, but it's making my code break, so I figured I'd get it filed.

I'm trying to crawl this website: https://www.appeals2.az.gov/ODSPlus/recentDecisions2.cfm

Unfortunately, most of the URLs in the HTML are relative, taking the form:

'../../some/path/to/some/pdf.pdf'

I'm using lxml's make_links_absolute() function, which calls urljoin creating invalid urls like:

https://www.appeals2.az.gov/../Decisions/CR20130096OPN.pdf

If you put that into Firefox or wget or whatever, it works, despite being invalid and making no sense.

**It works because those clients fix the problem,** joining the invalid path and the URL into:

https://www.appeals2.az.gov/Decisions/CR20130096OPN.pdf

I know this will mean making urljoin have a workaround to fix bad HTML, but this seems to be what wget, Chrome, Firefox, etc. all do.

I've never filed a Python bugs before, but is this something we could consider?

mlissner · 2014-08-01T13:59:11Z

FWIW, the workaround that I've just created for this problem is this:

u = 'https://www.appeals2.az.gov/../Decisions/CR20130096OPN.pdf'
# Split the url and rejoin it, nuking any '/..' patterns at the
# beginning of the path.
s = urlsplit(u)
urlunsplit(s[:2] + (re.sub('^(/\.\.)+', '', s.path),) + s[3:])

pitrou · 2014-08-05T17:52:51Z

The thing is, urljoin() isn't HTTP-specific, and such URLs *may* make sense for other protocols.

mlissner · 2014-08-05T18:31:26Z

@pitrou, I haven't delved into URLs in a long while, but the general idea is:

scheme://domain:port/path?query_string#fragment_id

When would it ever make sense to have something up a level from the root of the domain?

pitrou · 2014-08-05T18:38:20Z

Actually, according to RFC 3986, it seems you are right and we should remove extraneous leading "/../" and "/./" components in the path.
http://tools.ietf.org/html/rfc3986#section-5.4

demianbrecht · 2014-08-05T18:58:23Z

I've only had a couple minutes to look into this so far, but the bug does indeed seem to be valid across all versions. In fact, the line "# XXX The stuff below is bogus in various ways..." in urljoin tipped me off to something potentially being askew ;)

Unless the OP wants to provide a patch, I was going to take a look at possibly updating urljoin (at least the part below that line) to follow http://tools.ietf.org/html/rfc3986#section-5.2 a little more closely.

mlissner · 2014-08-05T18:59:45Z

@demian.brecht, that'd make me very pleased if you took this over. I won't have time to devote to it, unfortunately.

demianbrecht · 2014-08-06T00:45:05Z

Here's an initial patch with a fix that passes all the test cases other than the ones that are incorrect based on examples and pseudocode in RFC 3986. I haven't checked obsoleted RFCs yet to ensure consistency, but will do so when I get a chance (likely tomorrow morning). Posting this now to see if anyone's opposed to the change, or at least the direction of.

Note: It /does/ break backwards compatibility, but it seems that previous logic was incorrect (based on my upcoming checking for consistency between RFCs). As such, I'm not sure that it should be fixed < 3.5. Thoughts?

pitrou · 2014-08-06T02:51:25Z

It /does/ break backwards compatibility, but it seems that previous
logic was incorrect (based on my upcoming checking for consistency
between RFCs). As such, I'm not sure that it should be fixed < 3.5.
Thoughts?

Actually, the logic seems to be correct according to RFC 1808 (which the variable names in the tests seem to hint at, as well). I think it's fine to upgrade the semantics to newer RFCs, but we should only do it in 3.5 indeed.

demianbrecht · 2014-08-07T03:59:30Z

Update based on review comments, added legacy support, fixed the tests up.

pitrou · 2014-08-10T15:50:56Z

Demian, thanks for the update! I'm not sure the rfc1808 flag is necessary. I would be fine with switching wholesale to the new semantics. Nick, since you've done work in the past on URIs, what do you think?

demianbrecht · 2014-08-10T18:12:40Z

FWIW, I think that it would be ever so slightly preferable to maintain support for the old behaviour but perhaps add a deprecation note in the docs. It's a little difficult to tell how much of an impact this change will have to existing code. Maintaining support for the old behaviour for 3.5 would make the change a little more easy to deal with.

That said, it's nothing that I'd argue heatedly over.

ncoghlan · 2014-08-10T23:38:14Z

Unfortunately, I haven't looked at the RFC compliance side of things in
this space in years. At the time, we were considering a new module, leaving
the old one alone.

These days, I'd be more inclined to just fix it for 3.5, and suggest anyone
really needing the old behaviour fork an older version of the module.

Extracting the test case inputs from that old "uriparse" rewrite might
still be a good idea, though.

demianbrecht · 2014-08-11T14:10:00Z

Uploaded new patch. Removed support for RFC1808-specific behaviour. Extracted non-compliant tests into comment blocks indicating the behaviour is no longer supported.

mlissner · 2014-08-11T14:14:34Z

Just hopping in here to say that the work going down here is beautiful. I've filed a lot of bugs. This one's not particularly difficult, but damn, I appreciate the speed and quality going into fixing it.

Glad to see the Python language is a happy place with fast, quality fixes.

demianbrecht · 2014-08-11T14:49:00Z

Thanks Mike, it's always nice to get positive feedback :)

python-dev · 2014-08-21T23:16:47Z

New changeset b116489d31ff by Antoine Pitrou in branch 'default':
Issue bpo-22118: Switch urllib.parse to use RFC 3986 semantics for the resolution of relative URLs, rather than RFCs 1808 and 2396.
http://hg.python.org/cpython/rev/b116489d31ff

pitrou · 2014-08-21T23:17:52Z

The patch is now committed to the future Python 3.5. Thank you very much for this contribution!

scoder · 2014-08-22T11:43:50Z

I'm now getting duplicated slashes in URLs, e.g.:

https://new//foo.html
http://my.little.server/url//logo.gif

In both cases, the base URL that gets joined with the postfix had a trailing slash, e.g.

"http://my.little.server/url/" + "logo.gif" -> "http://my.little.server/url//logo.gif"

ncoghlan · 2014-08-22T12:15:47Z

Issue bpo-1500504 (the "urischemes" proposal that never got turned into a PyPI package) has several additional test cases. This particular attachment is one AMK cleaned up to run on Py3k:

http://bugs.python.org/file32591/urischemes.py

The module itself likely isn't worth salvaging, but the additional examples in the test cases were very useful in flushing out various RFC compliance issues.

python-dev · 2014-09-22T07:49:30Z

New changeset 901e4e52b20a by Senthil Kumaran in branch 'default':
Issue bpo-22278: Fix urljoin problem with relative urls, a regression observed
https://hg.python.org/cpython/rev/901e4e52b20a

mlissner mannequin added stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error labels Aug 1, 2014

ezio-melotti added the easy label Aug 5, 2014

pitrou closed this as completed Aug 21, 2014

ezio-melotti transferred this issue from another repository Apr 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

urljoin fails with messy relative URLs #66316

urljoin fails with messy relative URLs #66316

mlissner mannequin commented Aug 1, 2014

mlissner mannequin commented Aug 1, 2014

mlissner mannequin commented Aug 1, 2014

pitrou commented Aug 5, 2014

mlissner mannequin commented Aug 5, 2014

pitrou commented Aug 5, 2014

demianbrecht mannequin commented Aug 5, 2014

mlissner mannequin commented Aug 5, 2014

demianbrecht mannequin commented Aug 6, 2014

pitrou commented Aug 6, 2014

demianbrecht mannequin commented Aug 7, 2014

pitrou commented Aug 10, 2014

demianbrecht mannequin commented Aug 10, 2014

ncoghlan commented Aug 10, 2014

demianbrecht mannequin commented Aug 11, 2014

mlissner mannequin commented Aug 11, 2014

demianbrecht mannequin commented Aug 11, 2014

python-dev mannequin commented Aug 21, 2014

pitrou commented Aug 21, 2014

scoder commented Aug 22, 2014

ncoghlan commented Aug 22, 2014

python-dev mannequin commented Sep 22, 2014

urljoin fails with messy relative URLs #66316

urljoin fails with messy relative URLs #66316

Comments

mlissner mannequin commented Aug 1, 2014

mlissner mannequin commented Aug 1, 2014

mlissner mannequin commented Aug 1, 2014

pitrou commented Aug 5, 2014

mlissner mannequin commented Aug 5, 2014

pitrou commented Aug 5, 2014

demianbrecht mannequin commented Aug 5, 2014

mlissner mannequin commented Aug 5, 2014

demianbrecht mannequin commented Aug 6, 2014

pitrou commented Aug 6, 2014

demianbrecht mannequin commented Aug 7, 2014

pitrou commented Aug 10, 2014

demianbrecht mannequin commented Aug 10, 2014

ncoghlan commented Aug 10, 2014

demianbrecht mannequin commented Aug 11, 2014

mlissner mannequin commented Aug 11, 2014

demianbrecht mannequin commented Aug 11, 2014

python-dev mannequin commented Aug 21, 2014

pitrou commented Aug 21, 2014

scoder commented Aug 22, 2014

ncoghlan commented Aug 22, 2014

python-dev mannequin commented Sep 22, 2014