Skip to content
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

urljoin works incorrectly for two path-relative URLs involving . and .. #96015

Open
andersk opened this issue Aug 16, 2022 · 8 comments
Open
Assignees
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error

Comments

@andersk
Copy link
Contributor

andersk commented Aug 16, 2022

Bug report

urllib.parse.urljoin is usually used to join a normalized absolute URL with a relative URL, and it generally works for that purpose. But if it’s used to join two path-relative URLs, it produces incorrect results in many cases when . or .. is involved.

>>> from urllib.parse import urljoin
>>> urljoin('a', 'b')  # ok
'b'
>>> urljoin('a/', 'b')  # ok
'a/b'
>>> urljoin('a', '.')  # expected . or ./
'/'
>>> urljoin('a', '..')  # expected .. or ../
'/'
>>> urljoin('..', 'b')  # expected ../b
'b'
>>> urljoin('../a', 'b')  # expected ../b
'b'
>>> urljoin('a', '../b')  # expected ../b
'b'
>>> urljoin('../a', '../b')  # expected ../../b
'b'
>>> urljoin('a/..', 'b')  # expected b
'a/b'

There are also some problems when the base is a non-normalized absolute URL:

>>> urljoin('http://host/a/..', 'b')  # expected http://host/b
'http://host/a/b'

Your environment

  • CPython versions tested on: 3.10.5, 3.11.0rc1
  • Operating system and architecture: NixOS 21.11 amd64
@andersk andersk added the type-bug An unexpected behavior, bug, or error label Aug 16, 2022
@6t8k
Copy link
Contributor

6t8k commented Nov 24, 2022

According to the docs on urllib.urlparse.urljoin and the docstring in its code, the function is meant to construct an absolute URL, so I think that's what should be expected from its return value.

At least as far as RFC 3986 is concerned, which is mentioned in the linked docs, the "Base URI" is also expected to be absolute. Maybe the docs could be a bit clearer.

@andersk
Copy link
Contributor Author

andersk commented Nov 25, 2022

Even that excuse doesn’t apply to urljoin('http://host/a/..', 'b').

@6t8k
Copy link
Contributor

6t8k commented Nov 25, 2022

That indeed appears to be a more subtle case. Some clues, without a conclusion from me:

  • RFC 3986 sections 3.3 and 6.2.2.3 say dot-segments are intended for use at the beginning of a relative-path reference
  • 5.2.1 says normalization of the base URI, as described in 6.2.2 and 6.2.3, is optional (for historical/compatibility reasons, I'd assume)
  • In urllib.urlparse.urljoin, the last segment of the base URI, here a dot-segment, is removed here (this is not using the remove_dot_segments algorithm from 5.2.4 – effectively the segment is ignored)
  • The URL API, in major web browsers, does produce http://host/b (but for relative bases, it throws errors). It seems that they opt to not skip normailzation of the base URI; the new URL(url, base) constructor's, and following this, basic URL parser's path state specs seem to indicate this:

Firefox 107.0

let baseUrl1 = "http://host/a/..";
undefined
let A = new URL("b", baseUrl1);
undefined
A.href
"http://host/b"
let baseUrl2 = "../a";
undefined
let B = new URL("b", baseUrl2);
Uncaught TypeError: URL constructor: ../a is not a valid URL.
    <anonymous> debugger eval code:1
debugger eval code:1:9 

Chromium 107.0.5304.121

let baseUrl1 = "http://host/a/..";
undefined
let A = new URL("b", baseUrl1);
undefined
A.href
'http://host/b'
let baseUrl2 = "../a";
undefined
let B = new URL("b", baseUrl2);
VM520:1 Uncaught TypeError: Failed to construct 'URL': Invalid base URL
    at <anonymous>:1:15
(anonymous) @ VM520:1

The question then is, I suppose, whether this behavior is unexpected/hard to work around enough that fixing it would be worthwhile to the point of making the risk of breaking existing code acceptable. So far I'm not aware of any standards-breaking behavior, at least.

@andersk
Copy link
Contributor Author

andersk commented Nov 25, 2022

The expected semantics are obvious. These URLs are equivalent:

"http://host/a/.." ~ "http://host/"

so we expect these URLs to be equvalent:

urljoin("http://host/a/..", "b") ~ urljoin("http://host/", "b").

In general, we always expect:

  • if a ~ a1 and b ~ b1, then urljoin(a, b) ~ urljoin(a1, b1)

(where a ~ a1 is the equivalence relation normalize(a) == normalize(a1)).

Any time equivalent inputs lead to nonequivalent outputs is an opportunity for bugs. If the standard makes it optional to produce an expected result for legacy reasons, that doesn’t make it a good idea to produce an unexpected result.


Another law we always expect is

  • urljoin(a, urljoin(b, c)) ~ urljoin(urljoin(a, b), c)

which makes it obvious how urljoin(b, c) is expected to behave for relative b (if it’s not going to throw an error). For example, we should have

urljoin("http://host/w/x/y/z", urljoin("../b", "../c"))
~ urljoin(urljoin("http://host/w/x/y/z", "../b"), "../c")
== urljoin("http://host/w/x/b", "../c")
== "http://host/w/c"
== urljoin("http://host/w/x/y/z", "../../c")

which can only work if urljoin("../b", "../c") ~ "../../c".

@serhiy-storchaka
Copy link
Member

Unfortunately your expectations do not align with RFC 3986. For example, for urljoin('a', '..'), paths a and .. are first merged in .. according to the following rule:

   o  return a string consisting of the reference's path component
      appended to all but the last segment of the base URI's path (i.e.,
      excluding any characters after the right-most "/" in the base URI
      path, or excluding the entire base URI path if it does not contain
      any "/" characters).

Then .. is eliminated according to the following rule:

       D.  if the input buffer consists only of "." or "..", then remove
           that from the input buffer; otherwise,

So the result should be an empty string. Currently urljoin() returns a different result, but this will be changed by #126679.

urljoin('http://host/a/..', 'b') is a different case. The current result is wrong and #126679 does not fix it. But this is the case where RFC 3986 seems is not applied, because it expects normalized base path. I think that urljoin() should either automatically normalize the base path before merging paths or raise error.

@andersk
Copy link
Contributor Author

andersk commented Nov 11, 2024

@serhiy-storchaka The RFC 3986 algorithm is defined for an absolute base URI and a relative-path reference. It does not make sense to indiscriminately apply that same algorithm to two relative-path references. §5.1:

A base URI must conform to the <absolute-URI> syntax rule (Section 4.3).

§5.2.1:

Note that only the scheme component is required to be present in a base URI; the other components may be empty or undefined.

If we are to insist on using the algorithm as written, we’d have to throw an error when the base URI is missing a scheme.

However, it does make sense, and is useful in practice, to specify a generalized urljoin(a, b) operation that aligns with the RFC operation when a is an absolute URI, and that also satisfies the expected properties I listed above. We do this by preserving extra initial .. components in the output when given non–RFC 3986 input where the base URI is path-relative (undefined scheme, undefined authority, and path not beginning with /).

@serhiy-storchaka
Copy link
Member

An absolute base URI can have an undefined authority and a rootless or empty path.

@andersk
Copy link
Contributor Author

andersk commented Nov 12, 2024

Yes it can, if its scheme is defined and the parser is “strict” in the RFC sense. If we intend to behave as a strict parser, we use the RFC algorithm for that case and there is no associativity issue.

A non-strict parser considers such a URI to be a relative reference. §5.4.2:

Some parsers allow the scheme name to be present in a relative reference if it is the same as the base URI scheme. This is considered to be a loophole in prior specifications of partial URI [RFC1630]. Its use should be avoided but is allowed for backward compatibility.

"http:g"        =  "http:g"         ; for strict parsers
                /  "http://a/b/c/g" ; for backward compatibility

§5.2.2:

-- A non-strict parser may ignore a scheme in the reference
-- if it is identical to the base URI's scheme.
--
if ((not strict) and (R.scheme == Base.scheme)) then
   undefine(R.scheme);
endif;

If we intend to behave as a non-strict parser, then urljoin('http:../a', 'http:../b') again falls outside the scope of the RFC, but associativity mandates that we should preserve initial .. components and return http:../../b, if we’re being totally pedantic about our non-strictness. (I’m not too concerned about this obscure case.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

4 participants