Studio: fetch bare hostnames as https instead of refusing them - #7427
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb0a10e750
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45a943a963
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8573050ca0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
str.isdigit() is True for digit-class characters int() refuses (superscript
two, circled digit one), so _normalize_url_scheme reached int(port) and raised
ValueError out of _fetch_url_raw, which runs before its try block. A
web_search url of "example.com:<superscript two>" surfaced a generic tool
exception instead of the Blocked: message it returned before this branch.
Match the port against an anchored [0-9]{1,5} instead; the five-digit cap that
kept the range check from converting an unbounded integer is now in the
pattern.
_fetch_url_raw wraps the initial parsed.port in try/except ValueError, but the redirect hop reads rp.port unguarded, so a server answering Location: https://example.org:99999/next fell through to the broad handler as "Failed to fetch URL: Port out of range 0-65535" rather than a deliberate block. No request is dispatched either way; this just makes the two paths report the same way.
for more information, see https://pre-commit.ci
|
Thanks for this. The direction is right, and normalizing before 1. Unicode digits in the port raised instead of blocking
Same for Replaced 2. The invalid-port guard did not cover redirect targets
Both changes come with cases in your What I checkedI merged your branch onto current main first, since main is 32 commits ahead and #7416 rewrote the request construction block inside
Left alone, worth a follow up
|
The formatter expands a signature carrying a spaced kwarg default, which put the stub opener on eleven lines. **kw absorbs the timeout the fetch loop passes and leaves the whole stub on four.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0f26234b0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return url | ||
| if port and not (_PORT_RE.fullmatch(port) and 1 <= int(port) <= 65535): | ||
| return url | ||
| return "https://" + rest |
There was a problem hiding this comment.
When a bare URL uses the newly supported explicit-port form, such as example.com:8443/path, this normalization reaches the fetch loop, but that loop still sends Host: example.com rather than Host: example.com:8443. Servers that route or validate requests using the full authority can therefore reject or misroute the request, so the advertised bare host:port case does not reliably work; preserve the non-default port when constructing the Host header.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not reproducible on the current head. #7416 builds the Host header from validated_netloc, which carries the port, so the bare host:port form already advertises the full authority. Measured with the resolver and opener stubbed:
example.com:8443/path Host='example.com:8443' request='https://93.184.216.34:8443/path'
https://example.com:8443/path Host='example.com:8443' request='https://93.184.216.34:8443/path'
sub.example.co.uk:8080 Host='sub.example.co.uk:8080' request='https://93.184.216.34:8080'
The bare and explicitly schemed forms produce an identical Host header, and the pinned request URL keeps the port too. This review was against a merge base that predates #7416.
| authority = re.split(r"[/?#]", rest, maxsplit = 1)[0] | ||
| host, _, port = authority.partition(":") | ||
| if not _DOTTED_HOST_RE.fullmatch(host): | ||
| return url |
There was a problem hiding this comment.
When the direct-fetch input is a valid bare or protocol-relative public IPv6 literal, such as [2606:4700:4700::1111]/ or //[2606:4700:4700::1111]/, splitting the authority at the first colon makes host equal to [2606, so this regex check leaves the URL schemeless and _fetch_url_raw blocks it. The fetch path already supports bracketed IPv6 addresses and the new tests explicitly support schemeless IPv4 literals, so recognize bracketed IPv6 here and let the existing resolution/SSRF validation decide whether it is fetchable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accurate, and reproducible on the current head, but leaving it for a separate PR rather than widening the host grammar here.
'[2606:4700:4700::1111]/' -> Blocked: only http/https URLs are allowed.
'münich.de/path' -> Blocked: only http/https URLs are allowed.
'example.com.' -> Blocked: only http/https URLs are allowed.
'https://example.com.' -> fetched
Reasoning: none of these is a regression. All three were refused before this branch too, so the branch narrows nothing, it just does not widen to cover them. The bug it set out to fix, the misleading (got '') on a plain dotted host, is fixed.
The IDN case in particular deserves its own review rather than a regex widening tacked onto this one. _DOTTED_HOST_RE is the gate in front of an SSRF-sensitive path, and non-ASCII host characters are exactly where urlsplit gets sharp edges:
https://exam/ple.de (U+FF0F fullwidth solidus) -> ValueError: netloc contains invalid characters under NFKC normalization
https://exam@ple.de (U+FF20 fullwidth at) -> ValueError: netloc contains invalid characters under NFKC normalization
https://example。test (U+3002 ideographic stop) -> parses, and NFKC-folds to a label separator
So accepting IDN labels means deciding which confusables fold into ., /, @ and : before the authority is split, which is a security decision worth its own PR and its own tests. Same for bracketed IPv6, which changes how the authority is split on :.
Trailing dot is the cheap one of the three and I have no objection to it landing, just not bundled in here.
|
Correction to my note above, plus what a cross-platform simulation turned up. CorrectionI wrote that the unicode-port raise "loses the What the bug actually cost still justifies the fix: an exception escaping the documented SimulationI lifted the shipped
Two pre-existing issues the corpus surfacedBoth behave identically on main, so neither belongs in this PR.
Happy to send a separate PR for those two if useful. |
The URL is model-supplied, so every bad form should come back as one of the documented (error, body, content_type) strings. Three gaps remained: urlparse itself raises on an unmatched IPv6 bracket and on a netloc that NFKC-decomposes into a delimiter (//exam(fullwidth-solidus)ple.com), and both calls sat outside a guard. getaddrinfo raises UnicodeError, which is a ValueError and not the OSError _validate_and_resolve_host catches, when IDNA encoding rejects a hostname. Over a 3158 URL corpus that injects tabs, newlines, C0 controls, delimiters and NFKC confusables at every position, main raises 42 times and this raises none. Also strip surrounding whitespace in _normalize_url_scheme. _web_search already stripped, but normalization moved down to the fetch layer, so a direct _fetch_page_text caller did not get it.
status_for_tool and the web-search tool card both required an explicit scheme before reading the hostname, so every URL this branch newly makes fetchable showed the generic "Reading page..." and "Read page" instead of the host. Under permission_mode=ask that means the approval card named no destination for exactly the inputs the branch enables. The backend reuses _normalize_url_scheme. The frontend cannot, since new URL() throws on a bare host, so RE_BARE_HOST mirrors the same grammar: only a dotted host with an optional in-range port gets the https prefix, leaving /login, javascript: and userinfo forms to render generically as before. Also mention bare hostnames in the url parameter description, since they are part of the accepted interface now.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 83b5d07ad1
ℹ️ 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".
| if not _DOTTED_HOST_RE.fullmatch(host): | ||
| return url |
There was a problem hiding this comment.
When a user or model supplies a valid Unicode hostname without a scheme, such as münich.de/path or 例え.テスト, this ASCII-only match returns the input unchanged, after which _fetch_url_raw rejects it for lacking an HTTP(S) scheme. Explicitly schemed Unicode hosts already proceed through getaddrinfo and its IDNA handling, so the new bare-host behavior should recognize IDN labels as well rather than remaining limited to ASCII domains.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accurate, and reproducible on the current head, but leaving it for a separate PR rather than widening the host grammar here.
'[2606:4700:4700::1111]/' -> Blocked: only http/https URLs are allowed.
'münich.de/path' -> Blocked: only http/https URLs are allowed.
'example.com.' -> Blocked: only http/https URLs are allowed.
'https://example.com.' -> fetched
Reasoning: none of these is a regression. All three were refused before this branch too, so the branch narrows nothing, it just does not widen to cover them. The bug it set out to fix, the misleading (got '') on a plain dotted host, is fixed.
The IDN case in particular deserves its own review rather than a regex widening tacked onto this one. _DOTTED_HOST_RE is the gate in front of an SSRF-sensitive path, and non-ASCII host characters are exactly where urlsplit gets sharp edges:
https://exam/ple.de (U+FF0F fullwidth solidus) -> ValueError: netloc contains invalid characters under NFKC normalization
https://exam@ple.de (U+FF20 fullwidth at) -> ValueError: netloc contains invalid characters under NFKC normalization
https://example。test (U+3002 ideographic stop) -> parses, and NFKC-folds to a label separator
So accepting IDN labels means deciding which confusables fold into ., /, @ and : before the authority is split, which is a security decision worth its own PR and its own tests. Same for bracketed IPv6, which changes how the authority is split on :.
Trailing dot is the cheap one of the three and I have no objection to it landing, just not bundled in here.
Deep Research (unslothai#7219) added a website access policy gate, check_url_access, which requires an http(s) scheme and reads .port inside its own try/except. Resolved so normalization runs before that gate in both _fetch_url_raw and _fetch_page_text. Ordering matters twice over: after the gate a bare host is refused as a non-http URL and this branch does nothing, and the gate is also what applies the domain allowlist, so normalizing first is what makes allowedDomains cover google.com the same way it covers https://google.com. Dropped the guards the gate now subsumes: it validates the scheme, rejects credentials, percent-encoded netlocs, backslashes and control characters, and parses the port before either call site reads it, so the initial and redirect parses can no longer raise. Kept the normalizer's own parse guard, since it still sees raw model output. Three tests asserted wording the gate now owns and assert the class of result instead; the gate deliberately does not echo the URL, which is the credential-safe choice.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02c8cafb12
ℹ️ 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".
| # URLs the fetch layer accepts. | ||
| from core.inference.tools import _normalize_url_scheme | ||
|
|
||
| parsed = urlparse(_normalize_url_scheme(url)) |
There was a problem hiding this comment.
When a model emits a URL that makes urlparse raise, such as https://[::1 or //exam／ple.com, _normalize_url_scheme catches the first parse error and returns the original string, but this second unguarded parse raises again. Because ToolLoopController.prepare_call() builds status_text before executing the fetch, the entire tool turn crashes instead of _fetch_url_raw returning the intended user-facing blocked error; handle ValueError here and fall back to Reading page....
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, fixed in 4decdd3.
Confirmed reachable: status_for_tool runs inside prepare_call, and the call site at safetensors_agentic.py:1163 is outside the except Exception that wraps stream_tool_execution, so the ValueError ended the turn before the fetch ran and the user never saw the blocked message. _normalize_url_scheme catches its own parse error and returns the original string, so the second parse still needed guarding, as you said.
One correction for the record: this predates the branch rather than being introduced by it. status_for_tool on origin/main raises identically:
origin/main 'https://[::1' -> ValueError: Invalid IPv6 URL
origin/main '//exam/ple.com' -> ValueError: netloc contains invalid characters under NFKC normalization
Fixing it here anyway, since the badge is a line this branch touches and the rest of the branch already promises no malformed URL escapes as an exception. Guarded the parse with a fall back to Reading page..., plus four regression cases that fail without the guard.
status_for_tool runs inside prepare_call, before the fetch and outside the handler that wraps tool execution, so a ValueError from urlparse ends the whole turn instead of letting _fetch_url_raw return its blocked message. _normalize_url_scheme catches its own parse error and hands back the original string, so the parse here still has to be guarded. Reachable with https://[::1 or a host that NFKC-decomposes into a delimiter. This predates the branch, main raises identically, but the badge is one of the lines this branch touches and the rest of it already promises no malformed URL escapes as an exception.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4decdd3051
ℹ️ 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".
|
|
||
| authority = re.split(r"[/?#]", rest, maxsplit = 1)[0] | ||
| host, _, port = authority.partition(":") | ||
| if not _DOTTED_HOST_RE.fullmatch(host): |
There was a problem hiding this comment.
When the input is a valid fully qualified bare hostname such as example.com. or example.com.:8443/path, this full match fails and leaves the URL schemeless, so check_url_access rejects it instead of fetching it over HTTPS. This is inconsistent with explicitly schemed forms, because normalize_domain() deliberately canonicalizes a terminal DNS root dot with rstrip("."); allow that dot when recognizing the raw bare authority.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Accurate, and reproducible on the current head, but leaving it for a separate PR rather than widening the host grammar here.
'[2606:4700:4700::1111]/' -> Blocked: only http/https URLs are allowed.
'münich.de/path' -> Blocked: only http/https URLs are allowed.
'example.com.' -> Blocked: only http/https URLs are allowed.
'https://example.com.' -> fetched
Reasoning: none of these is a regression. All three were refused before this branch too, so the branch narrows nothing, it just does not widen to cover them. The bug it set out to fix, the misleading (got '') on a plain dotted host, is fixed.
The IDN case in particular deserves its own review rather than a regex widening tacked onto this one. _DOTTED_HOST_RE is the gate in front of an SSRF-sensitive path, and non-ASCII host characters are exactly where urlsplit gets sharp edges:
https://exam/ple.de (U+FF0F fullwidth solidus) -> ValueError: netloc contains invalid characters under NFKC normalization
https://exam@ple.de (U+FF20 fullwidth at) -> ValueError: netloc contains invalid characters under NFKC normalization
https://example。test (U+3002 ideographic stop) -> parses, and NFKC-folds to a label separator
So accepting IDN labels means deciding which confusables fold into ., /, @ and : before the authority is split, which is a security decision worth its own PR and its own tests. Same for bracketed IPv6, which changes how the authority is split on :.
Trailing dot is the cheap one of the three and I have no objection to it landing, just not bundled in here.
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
The premise of this branch is that models already emit bare hostnames unprompted, which is why the fetch layer had to stop refusing them. Advertising the bare form in the tool schema does not enable anything, it just steers models toward it, and that is the form carrying every edge case: ambiguous with dotted custom schemes, and unlike an explicit scheme it does not cover IPv6 literals, IDN or trailing-dot FQDNs. The fetch layer tolerates bare hosts. The schema should keep recommending a full URL. This also drops the one change here with no regression test.
|
@codex review |
1 similar comment
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 173aedc7ad
ℹ️ 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".
| if not _DOTTED_HOST_RE.fullmatch(host): | ||
| return url |
There was a problem hiding this comment.
When the model supplies a schemeless internationalized domain such as münich.example/path, this ASCII-only match leaves it unchanged, so check_url_access rejects it for lacking an HTTP scheme. The equivalent https://münich.example/path is already accepted and IDNA-normalized by normalize_domain, making bare-host support inconsistent for valid Unicode domains; validate or IDNA-normalize the raw host before deciding whether to prepend HTTPS.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same call as on the earlier thread for this (comment 3654979772), declining again for consistency rather than because it was re-raised.
Your reasoning is right on the detail I went to check: normalize_domain does IDNA-encode at web_access_policy.py:44, so https://münich.example/path really is accepted and punycoded, and the bare form really is inconsistent with it.
It is still not a regression. Bare IDN was refused before this branch too, so nothing narrowed. And _DOTTED_HOST_RE runs before check_url_access, which is the point: the IDNA that makes the schemed form work happens downstream of the gate, so supporting the bare form means widening the pre-gate host grammar, not reusing normalize_domain. That grammar is what decides where the authority ends, and non-ASCII is where urlsplit gets sharp:
https://exam/ple.de (U+FF0F fullwidth solidus) -> ValueError: netloc contains invalid characters under NFKC normalization
https://example。test (U+3002 ideographic stop) -> parses, and folds to a label separator
Deciding which confusables fold into ., /, @ and : before the split is a security decision that deserves its own PR and its own tests, not a character-class widening added to a branch that is otherwise a fetch-layer robustness fix.
| const RE_URL = /URL:\s*(.+)/; | ||
| const RE_SNIPPET = /Snippet:\s*(.+)/s; | ||
| // Mirrors _DOTTED_HOST_RE: the bare hosts the backend fetches as https. | ||
| const RE_BARE_HOST = /^[A-Za-z0-9-]+(\.[A-Za-z0-9-]+)+(:\d{1,5})?([/?#]|$)/; |
There was a problem hiding this comment.
For inputs such as example.com:/path or //example.com:, the backend intentionally normalizes the empty port and fetches over the default HTTPS port, but this regex requires at least one digit after the colon. The frontend consequently passes the raw string to new URL, interprets example.com: as a non-HTTP scheme, and leaves displayDomain empty, so a successful fetch is mislabeled as “Read page” instead of naming the host.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, fixed in 7a7abbb.
Confirmed the divergence first, backend versus the card, before and after:
backend host card (before) card (after)
example.com:/path example.com "" example.com
//example.com: example.com "" example.com
example.com: example.com "" example.com
Worth noting the obvious fix would have introduced the opposite bug. Simply allowing an empty port makes example.com:0 match, and new URL accepts port 0, so the card would name a host the backend refuses (its range check is 1-65535). That mismatch was already present before this change. So rather than loosening the digit count I mirrored the backend rule, an empty port or one in 1-65535, and ran the card against the normalizer output for all 35 inputs in its own matrix: 0 mismatches, typecheck clean.
The card's bare-host pattern required at least one digit after the colon, but the backend fetches an empty port (example.com: and example.com:/path go to the default HTTPS port), so a successful fetch rendered as "Read page" with no host. Allowing an empty port alone would have swung it the other way: example.com:0 is refused by the backend but new URL() accepts it, so the card would have named a host that is never fetched. That mismatch was there before this change too. Mirror the backend rule instead, an empty port or one in 1-65535, checked against every case in the normalizer's own matrix.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a7abbbcfc
ℹ️ 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".
| return None, b"".join(chunks) | ||
|
|
||
|
|
||
| _DOTTED_HOST_RE = re.compile(r"[A-Za-z0-9-]+(\.[A-Za-z0-9-]+)+") |
There was a problem hiding this comment.
When a model supplies a valid internationalized hostname without a scheme, such as münich.de, this ASCII-only pattern rejects it and leaves it unchanged, after which check_url_access returns “only http/https URLs are allowed.” The same hostname works when written as https://münich.de because normalize_domain already converts Unicode domains through IDNA, so the new bare-host behavior should apply that canonicalization rather than excluding all internationalized domains.
Useful? React with 👍 / 👎.
2 participants
_fetch_url_rawrefused any URL without an explicit scheme, so google.com and www.google.com the form models naturally emit were blocked. The error also printed the scheme (got ''), which reads like an empty argument and sends you looking in the wrong place.Bare hosts are now normalized to
https://before validation, and the message shows the URL. Non-http schemes (ftp://, file://, javascript:) are still refused and SSRF checks are unaffected since normalization happens before host resolution