From 0d0c3b4bfd50d9b9b46e4bf904a7ff762bc48026 Mon Sep 17 00:00:00 2001 From: freeqaz Date: Tue, 13 Oct 2026 19:48:41 +0000 Subject: [PATCH 1/3] fix(presets): bound the url userinfo checks on every url field 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. --- .../src/drivers/configuration/model-auth.ts | 53 +++++++++++-------- packages/occ/src/model-provider-settings.ts | 15 ++++-- .../configuration-controller.test.mjs | 2 +- 3 files changed, 44 insertions(+), 26 deletions(-) diff --git a/apps/controller/src/drivers/configuration/model-auth.ts b/apps/controller/src/drivers/configuration/model-auth.ts index b7d1195457..a55a50f409 100644 --- a/apps/controller/src/drivers/configuration/model-auth.ts +++ b/apps/controller/src/drivers/configuration/model-auth.ts @@ -27,12 +27,23 @@ const urlCredentialMessage = (path: string): string => // `NO_PROXY` lists hosts, not a proxy. const PROXY_ENVIRONMENT = /^(?!no_proxy$)(?:\w*_)?proxy$/i; -const environmentCostMessage = (path: string): string => - `Configuration field ${path}, with the environment values before it, holds too many URLs or \${NAME} references to check for a user name or password. Shorten these values, or make each whole value a \${NAME} reference.`; +const urlCostMessage = (path: string): string => + `Configuration field ${path}, with the URLs and environment values before it, holds too many URLs or \${NAME} references to check for a user name or password. Shorten these values, or make each whole value a \${NAME} reference.`; -// The most work `environmentUrls` and the userinfo checks may do on one document's environment -// values, in characters read (each rendering of each URL counts): about 0.1 s. -const ENVIRONMENT_URL_BUDGET = 1 << 22; +// The most work `environmentUrls` and the userinfo checks may do on one document's URL fields and +// environment values, in characters read (each rendering of each URL counts): about 0.1 s. +const URL_CHECK_BUDGET = 1 << 22; + +// The work `urlHoldsUserinfo` does on `literals`: it renders the text three times plus once per +// `${VAR:-fallback}` reference, and reads it whole each time (finding 1265). +function userinfoWork(literals: readonly string[]): number { + const text = literals.join("\0"); + let fallbacks = 0; + for (const [, escaped, fallback] of text.matchAll(environmentReference)) { + fallbacks += escaped === undefined && fallback !== undefined ? 1 : 0; + } + return text.length * (3 + fallbacks); +} // Each URL `value` may hold, from its scheme to the end of its authority, where `\0` marks an // unfilled Preset variable; `undefined` when checking them would exceed the budget. URL parsers @@ -45,12 +56,8 @@ const ENVIRONMENT_URL_BUDGET = 1 << 22; // reference whose fallback could end that authority. A backslash does not end an authority here: // WHATWG parsers read it as a slash after a special scheme, but Python and curl read it as part // of the userinfo, so each URL is also checked with it percent-encoded. `spent` carries the work -// done on the document so far; a proxy variable's whole-value check is charged to it too. -function environmentUrls( - value: string, - spent: { work: number }, - proxy: boolean, -): string[] | undefined { +// done on the document so far. +function environmentUrls(value: string, spent: { work: number }): string[] | undefined { const text = value.replaceAll(/[\t\n\r]/g, ""); // Each reference, by its start and by its end; `inside` marks the start of the one at each // index, and `count` how many start before it. @@ -70,9 +77,6 @@ function environmentUrls( count[index] = (count[index] ?? 0) + (count[index - 1] ?? 0); } let work = spent.work + text.length; - if (proxy) { - work += 2 * text.length * (3 + (count[text.length] ?? 0)); - } const skip = (from: number): [number, number] => { let index = from; let slashes = 0; @@ -138,7 +142,7 @@ function environmentUrls( // Each of the two checks renders every fallback in the URL, so its cost grows with each // reference. work += 2 * (end - start) * (3 + (count[end] ?? 0) - (count[start] ?? 0)); - if (work > ENVIRONMENT_URL_BUDGET) { + if (work > URL_CHECK_BUDGET) { return undefined; } covered = { from: authority, to: end - 1, ends }; @@ -196,6 +200,9 @@ export function validateModelCredentialReferences( } throw new ModelCredentialValueError(path); }; + // The work the userinfo checks have done on this document: each URL field and environment value + // is charged before it is checked, and one past the budget is refused as too costly to check. + const spent = { work: 0 }; // The refusal names the field, never its value. A Preset save passes `unresolved`, the text // around each unfilled variable at that path. const refuseUrlUserinfo = ( @@ -207,11 +214,17 @@ export function validateModelCredentialReferences( return; } const [head = "", ...tail] = options.unresolved?.(path) ?? [value]; + const withScheme = + schemeless && !/^[\0- ]*[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(head) + ? [`http://${head}`, ...tail] + : undefined; + spent.work += userinfoWork([head, ...tail]) + (withScheme ? userinfoWork(withScheme) : 0); + if (spent.work > URL_CHECK_BUDGET) { + throw new ModelCredentialValueError(pointer(...path), urlCostMessage); + } if ( urlHoldsUserinfo(value, [head, ...tail]) || - (schemeless && - !/^[\0- ]*[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(head) && - urlHoldsUserinfo(value, [`http://${head}`, ...tail])) + (withScheme !== undefined && urlHoldsUserinfo(value, withScheme)) ) { throw new ModelCredentialValueError(pointer(...path), urlCredentialMessage); } @@ -220,7 +233,6 @@ export function validateModelCredentialReferences( // 1262): pip, npm, git and Go send the userinfo in `PIP_INDEX_URL`, `npm_config_registry`, // `GIT_CONFIG_VALUE_0` or `GOPROXY` as Basic auth, and a list such as `GOPROXY` holds several. // A proxy variable is also read from its start, without a scheme as http. - const spent = { work: 0 }; const refuseEnvironmentUserinfo = (path: SettingPath, value: unknown, proxy: boolean): void => { if (typeof value !== "string") { return; @@ -231,10 +243,9 @@ export function validateModelCredentialReferences( const urls = environmentUrls( proxy && !/^[\0- ]*[A-Za-z][A-Za-z0-9+.-]*:\/\//.test(text) ? `http://${text}` : text, spent, - proxy, ); if (urls === undefined) { - throw new ModelCredentialValueError(pointer(...path), environmentCostMessage); + throw new ModelCredentialValueError(pointer(...path), urlCostMessage); } if (proxy) { refuseUrlUserinfo(path, value, true); diff --git a/packages/occ/src/model-provider-settings.ts b/packages/occ/src/model-provider-settings.ts index 4243333efa..3820a56983 100644 --- a/packages/occ/src/model-provider-settings.ts +++ b/packages/occ/src/model-provider-settings.ts @@ -217,13 +217,20 @@ export function urlHoldsUserinfo(value: string, literals: readonly string[] = [v } // `literals` as the runtime may render its environment references: every reference set, every -// fallback rendered, and each fallback alone with the rest set. -function environmentRenderings(literals: readonly string[]): string[][] { +// fallback rendered, and each of the first `alone` fallbacks alone with the rest set. Each +// rendering reads the whole text, so all of them cost time quadratic in its length (finding 1265). +// `writesSchemeSlashes` reads only the text before the first variable, which rendering a later +// fallback alone leaves as when every reference is set (fallbacks are numbered in order), so it +// needs only the first fallback alone (`alone` 1). +function environmentRenderings(literals: readonly string[], alone = Infinity): string[][] { const { fallbacks } = splitEnvironmentReferences(literals, () => false); return [ () => false, () => true, - ...Array.from({ length: fallbacks }, (_, only) => (index: number) => index === only), + ...Array.from( + { length: Math.min(fallbacks, alone) }, + (_, only) => (index: number) => index === only, + ), ].map((fallback) => splitEnvironmentReferences(literals, fallback)); } @@ -310,7 +317,7 @@ function validateSettings( const literals = unresolved([...path, "baseUrl"]); if ( environment(baseUrl) - ? !environmentRenderings(literals ?? [String(baseUrl)]).every(writesSchemeSlashes) + ? !environmentRenderings(literals ?? [String(baseUrl)], 1).every(writesSchemeSlashes) : literals === undefined ? !isRuntimeUrl(baseUrl) : !fillsAsRuntimeUrl(literals) diff --git a/tests/integration/configuration-controller.test.mjs b/tests/integration/configuration-controller.test.mjs index 412f821788..8f3fd8ef47 100644 --- a/tests/integration/configuration-controller.test.mjs +++ b/tests/integration/configuration-controller.test.mjs @@ -2323,7 +2323,7 @@ test("Configuration and Preset saves refuse a user name or password in a URL in assert.equal(response.status, 400, JSON.stringify(response.body)); assert.equal( response.body.error.message, - "Configuration field /skills/entries/x/env/X10, with the environment values before it, holds too many URLs or ${NAME} references to check for a user name or password. Shorten these values, or make each whole value a ${NAME} reference.", + "Configuration field /skills/entries/x/env/X10, with the URLs and environment values before it, holds too many URLs or ${NAME} references to check for a user name or password. Shorten these values, or make each whole value a ${NAME} reference.", ); }); // Nothing refused was stored. From 022ee6169ed2d79b1d7ec5f81be96e8604cddc15 Mon Sep 17 00:00:00 2001 From: freeqaz Date: Tue, 13 Oct 2026 19:53:22 +0000 Subject: [PATCH 2/3] test(presets): check url fields of many env fallbacks in bounded time 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. --- .../configuration-controller.test.mjs | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/tests/integration/configuration-controller.test.mjs b/tests/integration/configuration-controller.test.mjs index 8f3fd8ef47..7fe6c17a0b 100644 --- a/tests/integration/configuration-controller.test.mjs +++ b/tests/integration/configuration-controller.test.mjs @@ -2426,6 +2426,91 @@ test("Configuration and Preset saves refuse a user name or password in a URL in assert.equal(templated.status, 201, JSON.stringify(templated.body)); }); +test("Configuration and Preset saves check a URL of many ${NAME:-fallback} references in bounded time", async (t) => { + // Finding 1265: the userinfo rule renders a URL once per `${NAME:-fallback}` reference and reads + // it whole each time, so a body-sized baseUrl of fallbacks held the controller for seconds. + // Every URL field now spends the document's work budget, which refuses such a value at once. + const root = await mkdtemp(join(tmpdir(), "occ-url-userinfo-cost-")); + t.after(() => rm(root, { recursive: true, force: true })); + const configurationDriver = new FilesystemConfigurationDriver(root); + const computeDriver = createReadyComputeDriver("compute-url-userinfo-cost"); + const { namespace, ...context } = await fixture({ configurationDriver, computeDriver }); + await context.controller.transact((state) => + state.namespaces.transitionNamespaceStatus(namespace.id, "provisioning", "ready"), + ); + const configurations = `/namespaces/${namespace.id}/configurations`; + const presets = `/namespaces/${namespace.id}/presets`; + const cost = (field) => + `Configuration field ${field}, with the URLs and environment values before it, holds too many URLs or \${NAME} references to check for a user name or password. Shorten these values, or make each whole value a \${NAME} reference.`; + // 60 KB of fallbacks, the most the 64 KiB request body holds, in the path and in the authority. + const fallbacks = (prefix, suffix) => + `${prefix}${"${A:-x}".repeat(Math.floor((60 * 1024 - prefix.length - suffix.length) / 7))}${suffix}`; + const urls = [fallbacks("https://h.example/", ""), fallbacks("https://", "@h.example/v1")]; + const fields = [ + [(url) => ({ models: { providers: { p: { baseUrl: url } } } }), "/models/providers/p/baseUrl"], + [ + (url) => ({ + models: { + providers: { p: { baseUrl: "https://h.example/v1", request: { proxy: { url } } } }, + }, + }), + "/models/providers/p/request/proxy/url", + ], + [(proxyUrl) => ({ proxy: { proxyUrl } }), "/proxy/proxyUrl"], + [(proxy) => ({ channels: { telegram: { proxy } } }), "/channels/telegram/proxy"], + [(url) => ({ mcp: { servers: { docs: { url } } } }), "/mcp/servers/docs/url"], + ]; + const timed = async (path, body) => { + const start = performance.now(); + const response = await request(context.app, "POST", path, { body }); + return { ...response, elapsed: performance.now() - start }; + }; + for (const [values, field] of fields) { + for (const [index, url] of urls.entries()) { + await t.test( + `refuses ${field} of fallbacks (${index === 0 ? "path" : "authority"})`, + async () => { + const response = await timed(configurations, { kind: "agent", values: values(url) }); + assert.equal(response.status, 400, JSON.stringify(response.body)); + assert.equal(response.body.error.message, cost(field)); + // Unbounded, this took 4 to 14 s; bounded, a few milliseconds. + assert.ok(response.elapsed < 500, `took ${response.elapsed} ms`); + }, + ); + } + } + await t.test("refuses a Preset URL of fallbacks around a variable", async () => { + const response = await timed(presets, { + name: "Costly", + template: { + variables: { host: { type: "string" } }, + configuration: { values: { proxy: { proxyUrl: `${urls[1]}{{ vars.host }}` } } }, + }, + }); + assert.equal(response.status, 400, JSON.stringify(response.body)); + assert.equal(response.body.error.message, cost("/proxy/proxyUrl")); + assert.ok(response.elapsed < 500, `took ${response.elapsed} ms`); + }); + // A URL with a few fallbacks is still checked whole. + await t.test("still refuses userinfo in a fallback and accepts a plain one", async () => { + const refused = await request(context.app, "POST", configurations, { + body: { kind: "agent", values: { proxy: { proxyUrl: "http://${HOST:-u:pw@proxy}:3128" } } }, + }); + assert.equal(refused.status, 400, JSON.stringify(refused.body)); + assert.equal( + refused.body.error.message, + "Configuration field /proxy/proxyUrl carries a user name or password in the URL. Remove it, or make the whole URL a ${NAME} reference.", + ); + const saved = await request(context.app, "POST", configurations, { + body: { + kind: "agent", + values: { proxy: { proxyUrl: "http://${HOST:-proxy}:${PORT:-3128}" } }, + }, + }); + assert.equal(saved.status, 201, JSON.stringify(saved.body)); + }); +}); + test("Preset and Configuration saves refuse a baseUrl or api that a variable fills with no host or no string", async (t) => { // Finding 1256: `https://{{ vars.h }}/v1` filled empty renders `https:///v1`, whose first path // segment the URL parser reads as the host. Finding 1257: a number or boolean variable that is From 60e163fcdacb16277aa8123d49ec98408299dff2 Mon Sep 17 00:00:00 2001 From: freeqaz Date: Tue, 13 Oct 2026 20:13:23 +0000 Subject: [PATCH 3/3] fix(presets): charge preset variables and read each reference's end once 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. --- .../src/drivers/configuration/model-auth.ts | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/apps/controller/src/drivers/configuration/model-auth.ts b/apps/controller/src/drivers/configuration/model-auth.ts index a55a50f409..68a6411965 100644 --- a/apps/controller/src/drivers/configuration/model-auth.ts +++ b/apps/controller/src/drivers/configuration/model-auth.ts @@ -31,18 +31,20 @@ const urlCostMessage = (path: string): string => `Configuration field ${path}, with the URLs and environment values before it, holds too many URLs or \${NAME} references to check for a user name or password. Shorten these values, or make each whole value a \${NAME} reference.`; // The most work `environmentUrls` and the userinfo checks may do on one document's URL fields and -// environment values, in characters read (each rendering of each URL counts): about 0.1 s. +// environment values, in characters read (each rendering of each URL counts): about 0.1 s for +// each check of a document. const URL_CHECK_BUDGET = 1 << 22; // The work `urlHoldsUserinfo` does on `literals`: it renders the text three times plus once per -// `${VAR:-fallback}` reference, and reads it whole each time (finding 1265). +// `${VAR:-fallback}` reference, and reads it whole each time (finding 1265). Each unfilled Preset +// variable between literals costs about as much as a dozen characters in every rendering. function userinfoWork(literals: readonly string[]): number { const text = literals.join("\0"); let fallbacks = 0; for (const [, escaped, fallback] of text.matchAll(environmentReference)) { fallbacks += escaped === undefined && fallback !== undefined ? 1 : 0; } - return text.length * (3 + fallbacks); + return (text.length + 12 * literals.length) * (3 + fallbacks); } // Each URL `value` may hold, from its scheme to the end of its authority, where `\0` marks an @@ -94,6 +96,18 @@ function environmentUrls(value: string, spent: { work: number }): string[] | und } } }; + // Whether a reference's text holds a character in `ends`, read once per reference and pattern. + const endsIn = new Map(); + const holdsEnd = (reference: number, ends: RegExp): boolean => { + const key = `${ends.source}${reference}`; + const known = endsIn.get(key); + if (known !== undefined) { + return known; + } + const found = ends.test(text.slice(reference, after.get(reference))); + endsIn.set(key, found); + return found; + }; const spans: [number, number][] = []; let covered = { from: 0, to: 0, ends: /["/?#]/ }; for (const { index: colon } of text.matchAll(/:/g)) { @@ -120,7 +134,7 @@ function environmentUrls(value: string, spent: { work: number }): string[] | und if ( colon >= covered.from && authority < covered.to && - (reference === -1 || !covered.ends.test(text.slice(reference, after.get(reference)))) + (reference === -1 || !holdsEnd(reference, covered.ends)) ) { continue; }