Skip to content

fix: harden against confirmed review findings - #2

Merged
conorbronsdon merged 1 commit into
mainfrom
fix/hardening-review
Jul 7, 2026
Merged

fix: harden against confirmed review findings#2
conorbronsdon merged 1 commit into
mainfrom
fix/hardening-review

Conversation

@conorbronsdon

Copy link
Copy Markdown
Owner

From an automated multi-agent review (personal-context#62); implemented + verified by Claude Code. (PR auto-recovered — the agent completed and pushed this branch but a transient API error interrupted it before it could open the PR.)


fix: harden unquote UTF-8 decode and urljoin CPython parity

Fixes four confirmed review findings, each reproduced against CPython
urllib.parse ground truth before and after the change, and covered by
new fixtures/regression cases (fixtures.txt regenerated from CPython).

  1. unquote: reject overlong UTF-8 (security). unquote("%E0%80%AF")
    previously passed the raw invalid bytes e0 80 af through; CPython
    emits three U+FFFD. The UTF-8 decoder now enforces the lead-byte-
    specific second-byte range (RFC 3629 / Unicode Table 3-7), so
    overlongs, surrogates (ED A0..BF), and out-of-range 4-byte leads are
    rejected instead of decoding to raw/invalid bytes.

  2. unquote: Unicode maximal-subpart replacement. A truncated multibyte
    escape (e.g. %E6%97, %F0%9F%98) emitted one U+FFFD per byte;
    CPython emits exactly one U+FFFD and resumes past the maximal valid
    subpart. The decoder now advances by the consumed subpart length.

  3. urljoin: digit-led pseudo-scheme. urljoin("http://a/b/", "10:30.html") returned "10:30.html" (treating "10" as a scheme);
    CPython returns "http://a/b/10:30.html". urljoin is now a faithful
    port of CPython's algorithm and defers scheme detection to urlparse,
    which already requires an alpha-led scheme.

  4. urljoin: CPython backward-compat + absolute-ref dot segments.
    urljoin(base, "http:g") returned "http:g"; CPython resolves a
    same-scheme reference relatively -> "http://a/b/c/g". An absolute
    reference with a differing scheme is now returned verbatim (no dot-
    segment removal), matching CPython (e.g. "http://x/../y" stays as-is).
    The RFC 3986 5.4 table row for http:g (dropped to keep 41/41) is
    restored with CPython's value, so the table is now 42/42.

Also fixes the pixi Mojo pin (>=1.0.0b3 sorts below dev nightlies, so
pixi install fails to solve) to >=1.0.0b3.dev0,<2 as a build
prerequisite. README conformance/fixture counts updated where a fix made
them false (41/41 -> 42/42, 161 -> 173 fixtures) and the urljoin scheme
note corrected to describe the new CPython-matching behavior.

Co-Authored-By: Claude noreply@anthropic.com

Fixes four confirmed review findings, each reproduced against CPython
urllib.parse ground truth before and after the change, and covered by
new fixtures/regression cases (fixtures.txt regenerated from CPython).

1. unquote: reject overlong UTF-8 (security). `unquote("%E0%80%AF")`
   previously passed the raw invalid bytes `e0 80 af` through; CPython
   emits three U+FFFD. The UTF-8 decoder now enforces the lead-byte-
   specific second-byte range (RFC 3629 / Unicode Table 3-7), so
   overlongs, surrogates (ED A0..BF), and out-of-range 4-byte leads are
   rejected instead of decoding to raw/invalid bytes.

2. unquote: Unicode maximal-subpart replacement. A truncated multibyte
   escape (e.g. `%E6%97`, `%F0%9F%98`) emitted one U+FFFD per byte;
   CPython emits exactly one U+FFFD and resumes past the maximal valid
   subpart. The decoder now advances by the consumed subpart length.

3. urljoin: digit-led pseudo-scheme. `urljoin("http://a/b/",
   "10:30.html")` returned "10:30.html" (treating "10" as a scheme);
   CPython returns "http://a/b/10:30.html". urljoin is now a faithful
   port of CPython's algorithm and defers scheme detection to urlparse,
   which already requires an alpha-led scheme.

4. urljoin: CPython backward-compat + absolute-ref dot segments.
   `urljoin(base, "http:g")` returned "http:g"; CPython resolves a
   same-scheme reference relatively -> "http://a/b/c/g". An absolute
   reference with a differing scheme is now returned verbatim (no dot-
   segment removal), matching CPython (e.g. "http://x/../y" stays as-is).
   The RFC 3986 5.4 table row for http:g (dropped to keep 41/41) is
   restored with CPython's value, so the table is now 42/42.

Also fixes the pixi Mojo pin (`>=1.0.0b3` sorts below dev nightlies, so
pixi install fails to solve) to `>=1.0.0b3.dev0,<2` as a build
prerequisite. README conformance/fixture counts updated where a fix made
them false (41/41 -> 42/42, 161 -> 173 fixtures) and the urljoin scheme
note corrected to describe the new CPython-matching behavior.

Co-Authored-By: Claude <noreply@anthropic.com>
@conorbronsdon

Copy link
Copy Markdown
Owner Author

🤖 Independent Claude review: Ready to mark for review — the four target fixes are all correct and well-tested. One low-severity parity gap noted below (non-blocking).

Verified (differential-tested against CPython 3.9):

  • UTF-8 hardening decoder is faithful. Traced all 8 new fixtures: per-position second-byte ranges reject overlongs/surrogates/out-of-range 4-byte leads, and maximal-subpart resumption emits exactly one U+FFFD and resumes at the first non-subpart byte. Confirmed the old decoder decoded overlong %E0%80%AF to a literal / (the real security bug) and emitted 2× U+FFFD for truncated %E6%97 — the new regression fixtures genuinely FAIL on pre-fix code, not vacuous.
  • http:ghttp://a/b/c/g, 10:30.htmlhttp://a/b/10:30.html, http://x/../y returned verbatim (no dot-seg removal) all match CPython. Alpha-led scheme check correctly rejects the digit pseudo-scheme.
  • _uses_relative/_uses_netloc/_uses_params lists match CPython exactly.
  • pixi pin: confirmed 1.0.0b3.dev5 >= 1.0.0b3 is False, so the old pin excluded dev nightlies — the .dev0 fix is correct.

Non-blocking findings:

  1. src/url/parse.mojo urljoin parses the reference as urlparse(url) (empty default scheme) rather than CPython's urlparse(url, bscheme). Since _uses_params("") is always True, ;params is always split off the reference. For a scheme in uses_relative but NOT uses_params (file, ws, wss, gopher, nntp, wais, svn, svn+ssh) with a dot-segment-plus-param reference, this diverges: urljoin("ws://a/b/", ".;x") → this lib ws://a/b/;x vs CPython ws://a/b/.;x; urljoin("file://h/a/b/", "..;p")file://h/a/;p vs CPython file://h/a/b/..;p. Root cause is a regression from this rewrite (the old struct splitter did no params split). Realistically unreachable (dot-segments carrying ;params are not real URLs), so it does not block, but it contradicts the unqualified "byte-for-byte" docstring claim. Fix: gate the reference's params split on the resolved scheme (mirror urlparse(url, bscheme)), or skip params splitting on the ref entirely and let urlunparse recompose.
  2. test/data/fixtures.txt has a duplicate http:g row (lines 140 and 172); gen_fixtures adds it in both RFC3986_ABNORMAL and urljoin_extra. Harmless, just inflates the 173 count by one.
  3. Doc nit: README/docstring still call it "the canonical Section 5.4 conformance table (42/42)" while the http:g row deliberately uses CPython's value, not the RFC's strict http:g. The "deliberately does NOT do" section discloses this, but "canonical … conformance table" slightly overclaims for that row.

@conorbronsdon
conorbronsdon marked this pull request as ready for review July 6, 2026 07:58
@conorbronsdon

Copy link
Copy Markdown
Owner Author

Code review (opus, static — verify CI): SHIP-WITH-NITS

All four fixes correct. _utf8_lossy rewrite implements RFC 3629 / Table 3-7 lead-byte ranges properly (hand-traced all 8 fixtures vs bytes.decode("utf-8","replace")); urljoin port to CPython's algorithm verified against http:ghttp://a/b/c/g, 10:30.html, http://x/../y; digit-led scheme fix correctly delegates to urlparse.

Nits: parse.mojo:641 segments.append(seg) omits the .copy() used at :651/:662 (style); parse.mojo:63 _uses_netloc lists rtsps which isn't in CPython (pre-existing, only reformatted here). Confirm CI green before merge (no toolchain in review).

@conorbronsdon
conorbronsdon merged commit 6de7bb1 into main Jul 7, 2026
1 check passed
@conorbronsdon
conorbronsdon deleted the fix/hardening-review branch July 7, 2026 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant