urljoin fails with messy relative URLs

[mlissner]

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]

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]

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]

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

[mlissner]

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

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]

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]

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

[demianbrecht]

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]

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]

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

[pitrou]

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]

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.