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

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<string, boolean>();
+  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);

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)

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