diff --git a/apps/controller/src/drivers/configuration/model-auth.ts b/apps/controller/src/drivers/configuration/model-auth.ts index b7d1195457..68a6411965 100644 --- a/apps/controller/src/drivers/configuration/model-auth.ts +++ b/apps/controller/src/drivers/configuration/model-auth.ts @@ -27,12 +27,25 @@ 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 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). 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 + 12 * literals.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 +58,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 +79,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; @@ -90,6 +96,18 @@ function environmentUrls( } } }; + // 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)) { @@ -116,7 +134,7 @@ function environmentUrls( 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; } @@ -138,7 +156,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 +214,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 +228,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 +247,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 +257,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..7fe6c17a0b 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. @@ -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