diff --git a/pkgs/lab-ops-hub/container/app.py b/pkgs/lab-ops-hub/container/app.py index 0a690286a..7723a2b22 100644 --- a/pkgs/lab-ops-hub/container/app.py +++ b/pkgs/lab-ops-hub/container/app.py @@ -737,7 +737,6 @@ def _url_with_host(u, new_host, source_url=None): # the caller passes the source spelling its markers are re-attached # here — otherwise this geturl() rebuild would drop them before # the normalizer downstream ever sees them. - markers = _empty_query_fragment_markers(source_url) if source_url else "" netloc = u.netloc if "@" in netloc: userinfo, _, hostport = netloc.rpartition("@") @@ -752,13 +751,15 @@ def _url_with_host(u, new_host, source_url=None): rebuild = u._replace(path=urlsplit(source_url).path, params="") if source_url else u if hostport.startswith("["): _, _, suffix = hostport.partition("]") - return ( - rebuild._replace(netloc=f"{prefix}[{new_host}]{suffix}").geturl() + markers - ) - if ":" in hostport: + rebuilt = rebuild._replace(netloc=f"{prefix}[{new_host}]{suffix}").geturl() + elif ":" in hostport: _, _, port = hostport.partition(":") - return rebuild._replace(netloc=f"{prefix}{new_host}:{port}").geturl() + markers - return rebuild._replace(netloc=prefix + new_host).geturl() + markers + rebuilt = rebuild._replace(netloc=f"{prefix}{new_host}:{port}").geturl() + else: + rebuilt = rebuild._replace(netloc=prefix + new_host).geturl() + if source_url: + rebuilt = _attach_empty_query_fragment_markers(rebuilt, source_url) + return rebuilt _ASCII_LOWER = str.maketrans("ABCDEFGHIJKLMNOPQRSTUVWXYZ", "abcdefghijklmnopqrstuvwxyz") @@ -806,8 +807,8 @@ def _empty_query_fragment_markers(url): # "http://example.com/path?" and "http://example.com/path" to the # same parts, and geturl() drops the marker. WHATWG keeps it — the # query/fragment is present but empty — and the Worker returns it - # (#639), so the markers are computed from the spelling as a suffix - # and re-attached after each geturl() rebuild. A "?" after the "#" + # (#639), so the markers are computed from the spelling and + # re-attached after each geturl() rebuild. A "?" after the "#" # is fragment data, not a query marker, and a marker is only owed # when the parsed component is empty (a non-empty one is rebuilt # by geturl() itself). @@ -820,6 +821,34 @@ def _empty_query_fragment_markers(url): return markers +def _attach_empty_query_fragment_markers(rebuilt, source_url): + # Re-attach the empty "?" / "#" markers (#639) to a geturl() + # rebuild in their WHATWG positions, not as a plain suffix: when + # the query is empty but the fragment is non-empty, geturl() + # already ends the rebuilt URL with "#frag", so suffixing "?" + # put it after the fragment — "path?#frag" came back as + # "path#frag?", a different URL whose fragment is corrupted and + # whose empty query is absent (#654). The empty-query marker + # belongs before the "#" that introduces the fragment; the + # empty-fragment marker, whose fragment is empty by definition, + # is a suffix. + u = urlparse(source_url) + query_marker = not u.query and "?" in source_url.partition("#")[0] + fragment_marker = not u.fragment and "#" in source_url + if not query_marker and not fragment_marker: + return rebuilt + result = rebuilt + if query_marker: + idx = result.find("#") + if idx != -1: + result = result[:idx] + "?" + result[idx:] + else: + result += "?" + if fragment_marker: + result += "#" + return result + + def _normalize_validated_url(url): # Return a validated URL in the WHATWG-normalized form the Worker # returns (#633), so both crawlers record — and fetch — the same @@ -880,11 +909,14 @@ def _normalize_validated_url(url): # the path "/" in WHATWG ("http://example.com" is # "http://example.com/") (#639). resolved_path = "/" - return u._replace( - netloc=f"{prefix}{host_norm}{port_norm}", - path=resolved_path, - params="", - ).geturl() + _empty_query_fragment_markers(url) + return _attach_empty_query_fragment_markers( + u._replace( + netloc=f"{prefix}{host_norm}{port_norm}", + path=resolved_path, + params="", + ).geturl(), + url, + ) # WHATWG's per-component percent-encode sets for the ASCII punctuation @@ -967,13 +999,16 @@ def _url_with_encoded_components(url): # urlparse's split parts lose a bare trailing ";" — #655). The # params tail was encoded with the same _PATH_EXTRA set, so one # pass over the whole path is equivalent for non-empty params. - return u._replace( - netloc=netloc, - path=_encode_url_component(urlsplit(url).path, _PATH_EXTRA), - params="", - query=_encode_url_component(u.query, _QUERY_EXTRA), - fragment=_encode_url_component(u.fragment, _FRAGMENT_EXTRA), - ).geturl() + _empty_query_fragment_markers(url) + return _attach_empty_query_fragment_markers( + u._replace( + netloc=netloc, + path=_encode_url_component(urlsplit(url).path, _PATH_EXTRA), + params="", + query=_encode_url_component(u.query, _QUERY_EXTRA), + fragment=_encode_url_component(u.fragment, _FRAGMENT_EXTRA), + ).geturl(), + url, + ) # Backwards-compatible alias: the #615 name for the same helper. @@ -1039,8 +1074,13 @@ def _split_userinfo(url): username = unquote(username) password = unquote(password) # Keep the full path: a geturl() rebuild from urlparse's split - # parts drops a bare trailing ";" in the path (#655). - stripped = u._replace(netloc=hostport, path=urlsplit(url).path, params="").geturl() + # parts drops a bare trailing ";" in the path (#655). The rebuild + # also drops the empty "?" / "#" markers, so re-attach them in + # their WHATWG positions (#639, #654). + stripped = _attach_empty_query_fragment_markers( + u._replace(netloc=hostport, path=urlsplit(url).path, params="").geturl(), + url, + ) if not username and not password: return stripped, None token = base64.b64encode(f"{username}:{password}".encode("utf-8")) diff --git a/pkgs/lab-ops-hub/test/lib.test.ts b/pkgs/lab-ops-hub/test/lib.test.ts index e2c172eee..489fac7d0 100644 --- a/pkgs/lab-ops-hub/test/lib.test.ts +++ b/pkgs/lab-ops-hub/test/lib.test.ts @@ -120,6 +120,12 @@ test("validateCrawlUrl returns empty components normalized (#639)", () => { ["http://example.com?", "http://example.com/?"], ["http://example.com/path?#", "http://example.com/path?#"], ["http://user@example.com", "http://user@example.com/"], + // Empty query in front of a non-empty fragment: the "?" stays + // before the "#" (#654 — the container's suffix re-attachment + // returned "path#frag?" for these spellings). + ["http://example.com/path?#frag", "http://example.com/path?#frag"], + ["http://example.com?#frag", "http://example.com/?#frag"], + ["http://example.com/a?#b?c", "http://example.com/a?#b?c"], ]; for (const [raw, expected] of cases) { const r = validateCrawlUrl(raw); diff --git a/pkgs/lab-ops-hub/test/test_container_validate.py b/pkgs/lab-ops-hub/test/test_container_validate.py index fe8af66db..590c20353 100644 --- a/pkgs/lab-ops-hub/test/test_container_validate.py +++ b/pkgs/lab-ops-hub/test/test_container_validate.py @@ -1103,6 +1103,57 @@ def test_returns_empty_components_normalized(self): ) self.assertEqual(followed.full_url, "http://example.org/") + def test_returns_empty_query_marker_before_a_nonempty_fragment(self): + # #639's marker re-attachment appended the markers as a suffix + # after geturl(), so an empty query in front of a non-empty + # fragment came back after it: "path?#frag" returned as + # "path#frag?" — a different URL (fragment corrupted, empty + # query absent) than the Worker returns (#654). The "?" + # belongs before the "#", at every rebuild site. + cases = { + "http://example.com/path?#frag": "http://example.com/path?#frag", + "http://example.com?#frag": "http://example.com/?#frag", + "http://example.com/a?#b?c": "http://example.com/a?#b?c", + "http://example.com/path?#frag?x": "http://example.com/path?#frag?x", + # Through the percent-decoded-host and canonical-IP + # rebuilds (_url_with_host), and with userinfo / a + # non-empty fragment that itself needs encoding. + "http://%65xample.com/path?#frag": "http://example.com/path?#frag", + "http://8%2e8%2e8%2e8/path?#frag": "http://8.8.8.8/path?#frag", + "http://user@example.com/path?#frag": ( + "http://user@example.com/path?#frag" + ), + "http://example.com/path?#café": "http://example.com/path?#caf%C3%A9", + # Controls: the #639 cases stay correct — both markers + # empty, empty fragment only, non-empty query, and a "?" + # inside the fragment staying data. + "http://example.com/path?#": "http://example.com/path?#", + "http://example.com/path#": "http://example.com/path#", + "http://example.com/path?x#": "http://example.com/path?x#", + "http://example.com/path#frag?notquery": ( + "http://example.com/path#frag?notquery" + ), + } + for raw, expected in cases.items(): + self.assertEqual(validate_crawl_url(raw), expected, raw) + # The fetcher's userinfo strip rebuilds the URL too: it must + # keep the marker in front of the fragment (#654). + stripped, _ = app._split_userinfo("http://user@example.com/path?#frag") + self.assertEqual(stripped, "http://example.com/path?#frag") + # Verdicts are unchanged: blocked targets stay refused. + self.assertRefused("http://127.0.0.1/path?#frag") + self.assertRefused("http://localhost/path?#frag") + # Redirect targets inherit the fix: the handler fetches the + # validated form of a raw Location carrying "?#frag". + import urllib.request + + handler = app._ValidatingRedirectHandler() + req = urllib.request.Request("http://example.com/") + followed = handler.redirect_request( + req, None, 302, "Found", {}, "http://example.org/path?#frag" + ) + self.assertEqual(followed.full_url, "http://example.org/path?#frag") + def test_returns_the_root_dot_for_percent_encoded_spellings(self): # The container stripped the DNS root dot before judging and # rebuilt the returned URL from the stripped host, so the