Skip to content

fix(presets): bound the url userinfo checks on every url field - #2359

Merged
freeqaz merged 3 commits into
mainfrom
fix-1265
Oct 13, 2026
Merged

freeqaz merged 3 commits into
mainfrom
fix-1265

Conversation

@freeqaz

@freeqaz freeqaz commented Oct 13, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

urlHoldsUserinfo renders a URL once per ${NAME:-fallback} reference (plus three more times) and reads the whole text each time, so its cost is quadratic in the number of fallbacks. #2353 bounded this for environment values with a per-document work budget, but every other URL field it checks was still unbounded: baseUrl, request.proxy.url, proxy.proxyUrl, channel and account proxies, MCP server URLs, Codex and speech proxies. The baseUrl scheme-slash rule (writesSchemeSlashes, #2351) used the same renderings. A member can write Configurations and Presets, so one save could hold the controller for seconds.

Measured on main (abf6647), one 64 KB value of ${A:-x} fallbacks, validateModelCredentialReferences / validateModelProviderSettings:

field fallbacks in the path fallbacks in the authority
baseUrl 14.1 s / 10.4 s 4.0 s / 7.9 s
request.proxy.url 12.5 s 4.7 s
proxy.proxyUrl 12.0 s 6.1 s
channel proxy 12.3 s 5.2 s
MCP url 12.5 s 4.8 s
env value (#2353 budget) 6 ms 3 ms (refused as too costly)

Change

  • refuseUrlUserinfo (every URL field) now charges the document's work budget that fix(presets): refuse url userinfo in every env map and any env value with a scheme #2353 added for environment values, before it checks: the text length times (3 + its ${NAME:-fallback} references), once per check (twice for a schemeless proxy). Past the budget (2^22 characters read) the field is refused with the existing "too many URLs or ${NAME} references" message, which now says "with the URLs and environment values before it", as URL fields come first. A proxy environment variable's whole-value check is charged there once instead of inside environmentUrls.

  • The budget never skips a check: a value is either fully checked as before or refused. So it cannot accept anything main refuses; it only refuses values whose check would exceed the budget (about 5 KB of back-to-back fallbacks in one URL, or about 1 MB without any, beyond the 64 KiB request body).

  • writesSchemeSlashes reads only the text before the first unset reference. Rendering one fallback alone leaves that text as when every reference is set, except for the first fallback (fallbacks are numbered in order), so the baseUrl rule now renders only none, all, and the first fallback alone: three renderings, linear time, same result.

  • Review follow-ups: each unfilled Preset variable is charged about a dozen characters per rendering (a 63 KB Preset URL of 6,000 variables cost about 0.5 s under the budget; now refused at once), and environmentUrls (fix(presets): refuse url userinfo in every env map and any env value with a scheme #2353) remembers whether each reference's text ends an authority instead of reading it once per colon inside it (70-125 ms on a 60 KB value, uncharged). The second is exact: same answer, read once.

Why not a single-pass algorithm for holdsUserinfo: each per-fallback rendering changes where the scheme, authority, brackets and port end, and the URL parser's view of them. A rewrite that provably keeps every accept and refuse result of #2347/#2350/#2351/#2353 is a large change to a credential check; the budget keeps the existing check unchanged for every value it admits.

Verification

  • Timing after the fix, same 64 KB values: every URL field refused as too costly in 0-2 ms; provider settings 6-7 ms. At just under the budget, the slowest document shapes take 29-250 ms on a loaded host (load 29).
  • New test Configuration and Preset saves check a URL of many ${NAME:-fallback} references in bounded time: the five URL fields in both shapes and a Preset save, each refused with the cost message in under 500 ms (a few ms measured), plus a short fallback that still carries userinfo (refused) and a plain one (saved). Against main's sources the same test fails 12 of 13 subtests (saved with 201, or refused for userinfo after seconds).
  • Exactness of the scheme-slash reduction: a differential fuzz over 500,000 random literal lists (references, escaped references, fallbacks of /, \, https:, tabs, controls, several Preset literals) compared every rendering against the three: 0 mismatches. A negative control rendering no fallback alone found 232 mismatches in 200,000.
  • Differential run over the fix(presets): find url userinfo when a variable fills the host or port #2347/fix(presets): refuse url userinfo in proxy, channel, mcp and proxy env fields, an empty baseUrl host and non-string url variables #2350/fix(presets): require two slashes after an http(s) baseUrl scheme and refuse userinfo in mcp env and plugin proxies #2351/fix(presets): refuse url userinfo in every env map and any env value with a scheme #2353 cases: instrumented both rules and ran configuration-controller.test.mjs (51 tests), kubernetes-compute.test.mjs (349) and kubernetes-configuration.test.mjs (23): every baseUrl scheme-slash call gave the same result both ways, and none of the 324 URL userinfo checks hit the budget. All pass.
  • configuration-controller.test.mjs, kubernetes-compute.test.mjs and kubernetes-configuration.test.mjs 20/20 pinned to 2 CPUs: 20/20 each, 0 failures.
  • An adversarial review tried to bypass the change: a main-vs-fix differential fuzz (30,000 small and 1,500 large cases over baseUrl, proxy.proxyUrl, HTTPS_PROXY, PIP_INDEX_URL and a channel proxy, as Configurations and as Presets) found no value main refuses that the fix accepts; rerun on the final head, the only differences are 7 proxy env values main refused on cost alone (it charged them more); main with its budget removed accepts all 7, so none holds a credential.
  • tsc --build tsconfig.json, eslint and prettier on the touched files.

Risks

  • A document whose URL fields together exceed the budget is now refused at save, where before it saved after seconds of work. No realistic URL comes near it; the message names the field and how to fix it.
  • Stored documents are not affected: the URL checks do not run on a stored read (stored).
  • The cost message for environment values changed wording ("with the URLs and environment values before it").

urlHoldsUserinfo renders a value once per ${VAR:-fallback} reference and
reads it whole each time, so a 64 KB URL field of fallbacks took 4-14 s
(finding 1265). Charge every URL field to the document's work budget
that env values already spend, refusing past it as too costly to check,
and render only the first fallback alone for the baseUrl scheme-slash
rule, which reads only the text before the first variable.
Each of the five URL fields, with 60 KB of fallbacks in the path or the
authority, and a Preset URL around a variable, are refused as too costly
in under 500 ms (main: 4-14 s, or saved). A short fallback with userinfo
is still refused and a plain one saved.
@freeqaz
freeqaz requested a review from a team as a code owner October 13, 2026 19:58
@clawsweeper

clawsweeper Bot commented Oct 13, 2026 •

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review blocked

Automated review did not run, so no review verdict was produced.

Reason: The input-safety check rejected material in this revision. No detected value, path, or scanner output is reproduced here.

Automatic review is on hold, including after source changes. Request a fresh re-review after resolving the failure; maintainers can also release the hold through an intentional scanner-policy update.

Next step: If this is a genuine credential, remove and rotate it. If it is an intentional test fixture, a maintainer must review and qualify it.

View the workflow run.

Review nits: a Preset URL of thousands of variables cost about 0.5 s
under the budget, as each variable costs every rendering a fixed amount;
charge about a dozen characters for each. environmentUrls tested a
covered reference's whole text for an authority end once per colon in
it (70-125 ms on a 60 KB value, uncharged); remember the answer per
reference instead.

This branch was successfully deployed

1 active deployment
integration-qa-pr — 60e163fc Deployed Oct 13, 2026 by freeqaz via QA advisory (compose) #719
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