fix(lab-ops-hub): keep the empty-query marker before the fragment (#654)

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"))

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);

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