Repository navigation
lab-ops-hub: judge slash-run authority spellings by the WHATWG host (#586) - #587
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing: 2
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1140d1d238
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # spellings are then refused by the ordinary checks below, for the | ||
| # right reason. | ||
| if re.match(r"(?i)https?:", raw): | ||
| raw = re.sub(r"(?i)^(https?:)/*", r"\g<1>//", raw) |
There was a problem hiding this comment.
When an actual response contains Location: http:///127.0.0.1/admin, HTTPRedirectHandler.http_error_302 calls urljoin(req.full_url, newurl) before _ValidatingRedirectHandler.redirect_request, so this line receives http://example.com/127.0.0.1/admin and never sees or rejects the WHATWG loopback target. The added test invokes redirect_request directly and therefore misses the real flow; intercept and validate the raw Location before stdlib resolution, otherwise slash-run redirects continue to be followed on the wrong host.
Useful? React with 👍 / 👎.
| # spellings are then refused by the ordinary checks below, for the | ||
| # right reason. | ||
| if re.match(r"(?i)https?:", raw): | ||
| raw = re.sub(r"(?i)^(https?:)/*", r"\g<1>//", raw) |
There was a problem hiding this comment.
For inputs containing WHATWG-ignored tab, CR, or LF characters in the delimiter run, such as http:/\n/example.com/x, this substitution runs before urlparse strips the control and produces http://\n/example.com/x, which then parses as hostless. The parent validator accepted this input and new URL(...) normalizes it to http://example.com/x, so direct callers can now reject URLs the Worker accepts; remove those parser-ignored characters before determining the slash run.
Useful? React with 👍 / 👎.
| # spellings are then refused by the ordinary checks below, for the | ||
| # right reason. | ||
| if re.match(r"(?i)https?:", raw): | ||
| raw = re.sub(r"(?i)^(https?:)/*", r"\g<1>//", raw) |
There was a problem hiding this comment.
When this line newly admits a spelling such as http:example.com/?q=\value, the earlier global backslash replacement returns http://example.com/?q=/value, whereas Bun/WHATWG returns http://example.com/?q=\value. Backslashes are special delimiters while parsing the authority and path but are data in the query, so a direct container caller now fetches a different resource than the Worker for these newly accepted slash-run URLs; limit normalization to the pre-query URL states.
Useful? React with 👍 / 👎.
| # spellings are then refused by the ordinary checks below, for the | ||
| # right reason. | ||
| if re.match(r"(?i)https?:", raw): | ||
| raw = re.sub(r"(?i)^(https?:)/*", r"\g<1>//", raw) |
There was a problem hiding this comment.
For a direct input such as http:xx.com/ containing unassigned U+0378, this new normalization accepts and returns http://xx.com/, while Bun's new URL and the Worker validator reject the host outright. Python's built-in IDNA codec can encode it as xn--xx-g4b.com, so the container may fetch a DNS target that was never valid under the Worker's WHATWG policy; validate newly admitted hosts with compatible UTS-46 rules before returning them.
Useful? React with 👍 / 👎.
| # spellings are then refused by the ordinary checks below, for the | ||
| # right reason. | ||
| if re.match(r"(?i)https?:", raw): | ||
| raw = re.sub(r"(?i)^(https?:)/*", r"\g<1>//", raw) |
There was a problem hiding this comment.
For a direct input such as http:example.com/café, this line now returns http://example.com/café, whereas the Worker serializes it as http://example.com/caf%C3%A9; urllib subsequently raises UnicodeEncodeError instead of crawling it. Raw spaces and C0 controls similarly pass validation and then raise InvalidURL, so these newly accepted slash-run URLs produce a container failure despite being valid to the Worker; apply WHATWG-compatible percent-encoding before returning the URL.
Useful? React with 👍 / 👎.
| # spellings are then refused by the ordinary checks below, for the | ||
| # right reason. | ||
| if re.match(r"(?i)https?:", raw): | ||
| raw = re.sub(r"(?i)^(https?:)/*", r"\g<1>//", raw) |
There was a problem hiding this comment.
When a direct caller supplies http:example.com/a/../b, this line newly accepts it as http://example.com/a/../b and urllib sends that path verbatim, while the Worker serializes the same URL as http://example.com/b. Encoded dot forms such as %2e%2e also remain unnormalized here, and servers or proxies do not handle these spellings uniformly, so the container can crawl a different resource than the Worker; apply WHATWG dot-segment normalization to the returned URL.
Useful? React with 👍 / 👎.
Fixes #586
What
WHATWG URL parsing consumes the whole delimiter run after an http/https scheme colon —
http:example.com/x,http:/…,http:///…,http:////…,http:\…,http:\\\…,http://\…all name hostexample.com. The container'svalidate_crawl_urlonly saw a netloc behind exactly//(the #577\→/replacement helps only when the run is exactly two), so every other run length was refused asurl has no host, while the Worker'svalidateCrawlUrlaccepts and normalizes them all.validate_crawl_urlnow collapses the leading/run after the scheme colon to exactly//beforeurlparse(after the existing backslash normalization), and judges — and returns — the normalized spelling, following the #577 pattern. Blocked hosts in these spellings are refused by the ordinary non-public/local checks, for the right reason, not as hostless URLs.Exposure is redirect hops (the container's redirect handler validates raw
Locationvalues; the Worker fallback resolves them with WHATWGnew URL(...)) and direct container callers; the main dispatch path already passes the Worker's normalized URL.Verification
Location.node --test), ruff format clean.container/app.pyreverted, the new acceptance test errors; with the fix, all pass.xn--A-labels that Bun and the container both reject): the 70-spelling slash-run matrix plus the IPv4/IPv6/A-label/IDN/percent/port batches now show zero verdict differences.Need help on this PR? Tag
@codesmith-botwith what you need.Summary by CodeRabbit