Repository navigation
fix(lab-ops-hub): container validator refuses numeric-final-label hosts and malformed ports - #503
Conversation
…ts and malformed ports (#502) validate_crawl_url fell through to 'a public DNS name' for hosts that parse as neither ipaddress nor inet_aton IPv4 — including invalid IPv4 spellings (256.1.1.1, 4294967296) and names with a numeric final label (example.123) — and never inspected the port, so :abc/:99999/:80abc were allowed too. The Worker's WHATWG validator refuses every one. Mirror it: refuse a host whose final label is numeric (decimal or 0x hex) when it is not a valid IPv4 address (WHATWG ends-in-a-number), and read urlparse's .port in the validator so malformed/out-of-range ports raise there instead of lazily inside the fetcher. Port 0 and non-numeric-final-label names (dead.beef, 123.example.com) stay allowed, exactly as on the Worker side. Differential probe over ~600 inputs now shows full TS/Python parity. New tests ablation-verified: both fail with only app.py reverted.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 35 minutes. View limit details
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 |
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6ff8f35473
ℹ️ 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".
| # refuse it instead of reclassifying it as a public DNS | ||
| # name, matching the Worker's validator (#502). | ||
| last_label = host.rsplit(".", 1)[-1] | ||
| if re.fullmatch(r"0[xX][0-9a-fA-F]+|[0-9]+", last_label): |
There was a problem hiding this comment.
When the container validates a direct request or redirect ending in 0x or 0X (for example, http://example.0x/), this regex requires at least one following hexadecimal digit and therefore accepts the URL as a DNS name. The Worker's WHATWG parser rejects the same URL because a bare hexadecimal prefix still triggers its ends-in-a-number handling, so the validators remain inconsistent and the container proceeds to a DNS fetch that the Worker refuses. Allow the hexadecimal digit portion to be empty or otherwise handle these two labels explicitly.
Useful? React with 👍 / 👎.
| try: | ||
| u.port | ||
| except ValueError: | ||
| raise ValueError("url has an invalid port") from None |
There was a problem hiding this comment.
For a redirect target such as http://example.com:80\\/path, the Worker's WHATWG URL parser treats the backslash as a path separator and accepts it as http://example.com//path, but urlparse leaves the backslash in the port string, so reading u.port now rejects the target. Because redirects are passed through this validator before the container fetches them, an otherwise Worker-valid redirect unnecessarily abandons the container path and breaks the validator-parity goal; normalize special-scheme backslashes or parse with equivalent semantics before applying the port check.
Useful? React with 👍 / 👎.
Fixes #502
The container crawler's
validate_crawl_urlfell through to "a public DNS name" for hosts that parse as neitheripaddressnor inet_aton IPv4, and never inspected the port — so invalid IPv4 spellings (256.1.1.1,1.2.3.4.5,4294967296), numeric-final-label names (example.123,example.0x7f) and malformed/out-of-range ports (:abc,:99999,:80abc) were all allowed, while the Worker's WHATWG validator refuses every one (differential probe in #502).0xhex) when it is not a valid IPv4 address — WHATWG's ends-in-a-number rule. Valid inet_aton spellings are still judged by the address they resolve to (lab-ops-hub container crawler accepts inet_aton IPv4 spellings (127.1, 2130706433, 0x7f.1) — loopback SSRF bypass in validate_crawl_url #468 unchanged).urlparse(...).portinside the validator so a malformed or out-of-range port raises there instead of lazily inside the fetcher. Port 0 stays allowed — both validators accept it today (probe-verified), so parity is kept._ValidatingRedirectHandler's reuse of the same function.Verification:
npm testinpkgs/lab-ops-hub: TS 18/18, Python 9/9 (2 new tests: numeric-final-label hosts, malformed ports; public-target controls extended withdead.beef,a1.beef,123.example.com,:8080).app.pyreverted, both new tests fail; restored, all pass.Need help on this PR? Tag
@codesmith-botwith what you need.