Redact credentials embedded in provider endpoints, and bound the provider name

docs/modules/inference/credentials.md

@@ -22,6 +22,43 @@ The manifest can only ever *name* a slot (`[inference].api_key_secret`), never
 hold a value, and validation rejects a value there that looks like a pasted
 credential.
 
+### The fifth place a credential can hide: the endpoint
+
+A URL can carry userinfo — `http://alice:hunter2@127.0.0.1:8597/v1` — and a
+`base_url` is none of the things that make the four slots above safe. It is
+stored as written, it is returned by `GET {scope}/inference`, which is
+`ScopedCompany` rather than admin, so every console reader receives it on every
+page load, and it is interpolated into operator-facing failure text. A password
+in an endpoint is therefore a password in all three at once.
+
+Two independent mechanisms, because they cover different populations:
+
+- **Refused wherever an endpoint is accepted.**
+  `catalogue::endpoint_has_credentials` gates `normalize_local_endpoint`, which
+  every stored endpoint passes through, and `validate_parts`, which is the
+  manifest and console-`PUT` half of the same rule. It is the same class of
+  rule as the `api_key_secret` check beside it: a credential belongs in the
+  write-only slot, not in a field that is read back. The draft probe refuses
+  one too, rather than putting a basic-auth credential on the wire.
+- **Redacted wherever an endpoint is said.** `catalogue::redact_endpoint`
+  replaces the userinfo with `***`. Refusal cannot reach a value stored before
+  the rule existed, or one arriving from a `company.toml` or
+  `OPENCOMPANY_INFERENCE_URL` this host does not own — so every DTO field,
+  operator-facing message and log line that names an endpoint goes through it.
+  The endpoint used to *make* the request does not, which is the whole
+  distinction between the two call sites.
+
+The subtle half is the failure text. `reqwest` already masks userinfo in its own
+`Display`, so an error that quotes the upstream string looks safe — and then a
+`format!` that adds our copy of the URL beside it puts the credential straight
+back. Three separate messages did exactly that.
+
+Strict manifest `validate()` runs only on a first boot with no persisted record
+(`src/runtime/builder.rs`), so the refusal cannot brick a company that is
+already running on such an endpoint. That company is covered by redaction
+instead, and its next edit of the endpoint is refused with a sentence saying
+where to put the credential.
+
 ## How resolution worked before this rework
 
 **Superseded — kept because the defect it describes is why the chain exists.**

frontend/src/inference/ProviderConnectDialog.tsx

@@ -13,9 +13,12 @@ import { Input } from "@/components/ui/input";
 import { Label } from "@/components/ui/label";
 import {
   MANAGED_OPTION_SLUG,
+  MAX_PROVIDER_NAME_CHARS,
+  checkProviderName,
   checkSlug,
   credentialAsk,
   customProviderReady,
+  endpointHasCredentials,
   normalizeEndpoint,
   slugErrorCopy,
   slugify,
@@ -104,7 +107,9 @@ export function ProviderConnectDialog({
   }, [optionSlug, open]);
 
   const slug = slugify(label);
-  const slugError = custom ? checkSlug(providers, slug) : null;
+  // The name's own bound is reported before the slug's, because a name past the
+  // limit is what the operator can actually see and fix — the slug is derived.
+  const slugError = custom ? (checkProviderName(label) ?? checkSlug(providers, slug)) : null;
   const endpointOk = !ask.needsEndpoint || normalizeEndpoint(baseUrl) !== null;
   const ready = custom
     ? customProviderReady(providers, { label, baseUrl })
@@ -143,6 +148,9 @@ export function ProviderConnectDialog({
                 value={label}
                 placeholder="My Provider"
                 autoComplete="off"
+                // The host holds this rule; the attribute only stops a paste
+                // becoming a 400 the operator has to read to understand.
+                maxLength={MAX_PROVIDER_NAME_CHARS}
                 onChange={(e) => setLabel(e.target.value)}
               />
               {/* The slug is what a routing entry will say, so the operator
@@ -179,7 +187,9 @@ export function ProviderConnectDialog({
               />
               {baseUrl.trim() && !endpointOk && (
                 <p className="text-xs text-status-blocked-text">
-                  That must be an http or https address.
+                  {endpointHasCredentials(baseUrl)
+                    ? "Remove the username and password from the URL and put the credential in the API key field — an endpoint is stored as written and is readable by everyone who can see this company's settings."
+                    : "That must be an http or https address."}
                 </p>
               )}
             </div>

frontend/src/inference/connect.ts

@@ -294,15 +294,27 @@ export function slugify(label: string): string {
   return out.replace(/-+$/, "");
 }
 
+/**
+ * The longest a provider name may be, in characters.
+ *
+ * Mirrors `store::MAX_PROVIDER_NAME_CHARS` on the host, which is where the rule
+ * actually lives — the name becomes the address of a secret
+ * (`provider/<slug>/key`), and an unbounded name produced an unbounded path
+ * that 500ed a credential read and truncated a stored key on the way to failing
+ * the delete. This copy only spares the operator a round trip to find that out.
+ */
+export const MAX_PROVIDER_NAME_CHARS = 80;
+
 /** Why a slug cannot be used. */
-export type SlugError = "empty" | "taken" | "reserved";
+export type SlugError = "empty" | "taken" | "reserved" | "too-long";
 
 /**
  * Whether a derived slug may be used for a **custom** provider.
  *
- * Three named failures rather than a boolean, because they need three different
- * sentences: one is "pick another name", one is "you already have this", and one
- * is "that name belongs to something we ship".
+ * Four named failures rather than a boolean, because they need four different
+ * sentences: one is "pick another name", one is "you already have this", one
+ * is "that name belongs to something we ship", and one is "that name is too
+ * long".
  *
  * The catalogue check applies to custom providers only. Adding the catalogue's
  * own `groq` entry *should* take the slug `groq` — that is the same provider,
@@ -311,6 +323,7 @@ export type SlugError = "empty" | "taken" | "reserved";
 export function checkSlug(providers: readonly Provider[], slug: string): SlugError | null {
   const trimmed = slug.trim();
   if (!trimmed) return "empty";
+  if ([...trimmed].length > MAX_PROVIDER_NAME_CHARS) return "too-long";
   if (providers.some((p) => p.slug === trimmed)) return "taken";
   if (isReservedSlug(trimmed)) return "reserved";
   return null;
@@ -325,6 +338,8 @@ export function slugErrorCopy(error: SlugError): string {
       return "This company already has a provider with that name.";
     case "reserved":
       return "That name belongs to a built-in provider.";
+    case "too-long":
+      return `A provider name can be at most ${MAX_PROVIDER_NAME_CHARS} characters.`;
   }
 }
 
@@ -339,6 +354,7 @@ export function slugErrorCopy(error: SlugError): string {
  */
 export function normalizeEndpoint(raw: string): string | null {
   const trimmed = raw.trim().replace(/\/+$/, "");
+  if (endpointHasCredentials(trimmed)) return null;
   const split = trimmed.indexOf("://");
   if (split === -1) return null;
   const scheme = trimmed.slice(0, split).toLowerCase();
@@ -349,6 +365,28 @@ export function normalizeEndpoint(raw: string): string | null {
   return trimmed;
 }
 
+/**
+ * Whether an endpoint URL carries a credential in its authority
+ * (`http://user:password@host/v1`).
+ *
+ * Mirrors `catalogue::endpoint_has_credentials`. The host refuses such an
+ * endpoint at every point one can be set, and redacts it anywhere one is said;
+ * this copy exists so the operator is told *why* beside the field rather than
+ * after a round trip.
+ *
+ * The `@` has to be inside the authority — a path may legitimately contain one
+ * (`https://host/v1/@me`), and that is not a credential.
+ */
+export function endpointHasCredentials(raw: string): boolean {
+  const trimmed = raw.trim();
+  const split = trimmed.indexOf("://");
+  const start = split === -1 ? 0 : split + 3;
+  const rest = trimmed.slice(start);
+  const end = rest.search(/[/?#]/);
+  const authority = end === -1 ? rest : rest.slice(0, end);
+  return authority.includes("@");
+}
+
 /**
  * Whether the custom-provider dialog's Add button may be pressed.
  *
@@ -360,7 +398,22 @@ export function customProviderReady(
   draft: { label: string; baseUrl: string },
 ): boolean {
   return (
+    checkProviderName(draft.label) === null &&
     checkSlug(providers, slugify(draft.label)) === null &&
     normalizeEndpoint(draft.baseUrl) !== null
   );
 }
+
+/**
+ * Whether a typed provider **name** may be used, before any slug is derived.
+ *
+ * Mirrors `store::check_provider_name`. Separate from {@link checkSlug} for the
+ * same reason it is separate on the host: a name can be long while its slug is
+ * short, because `slugify` drops everything that is not alphanumeric.
+ */
+export function checkProviderName(label: string): SlugError | null {
+  const trimmed = label.trim();
+  if (!trimmed) return "empty";
+  if ([...trimmed].length > MAX_PROVIDER_NAME_CHARS) return "too-long";
+  return null;
+}

frontend/src/inference/types.ts

@@ -9,6 +9,14 @@
 // carries `keyConfigured: boolean` and nothing else. Four independent mechanisms
 // keep credentials off the wire in this subsystem, and the easiest way to break
 // all four at once is to put one on a record for convenience.
+//
+// `baseUrl` is the field that made that sentence briefly untrue. A URL can carry
+// userinfo (`http://user:password@host/v1`), and this shape comes back from a
+// `ScopedCompany` route every console reader can call. The host now refuses such
+// an endpoint everywhere one can be set and redacts it everywhere one is said
+// (`catalogue::endpoint_has_credentials` / `redact_endpoint`), so what arrives
+// here is `http://***@host/v1` at worst — but a `baseUrl` is still a place a
+// credential can hide, which is why it is called out rather than trusted.
 
 /** How a provider expects its credential presented. */
 export type AuthStyle = "bearer" | "anthropic" | "none";

frontend/test/unit/inference-connect.test.ts

@@ -9,8 +9,11 @@
 import { describe, expect, it } from "vitest";
 
 import {
+  MAX_PROVIDER_NAME_CHARS,
   addOptions,
+  checkProviderName,
   checkSlug,
+  endpointHasCredentials,
   credentialAsk,
   customProviderReady,
   isConnected,
@@ -115,6 +118,30 @@ describe("the slug, which is derived and never typed", () => {
     expect(slugErrorCopy("empty")).toBe("Enter a provider name to generate a slug.");
     expect(slugErrorCopy("taken")).toContain("already has a provider");
     expect(slugErrorCopy("reserved")).toContain("built-in");
+    expect(slugErrorCopy("too-long")).toContain(String(MAX_PROVIDER_NAME_CHARS));
+  });
+
+  it("bounds the name, because the name becomes the address of a secret", () => {
+    // Mirrors `store::MAX_PROVIDER_NAME_CHARS`. The host holds the rule; this
+    // only spares the operator a round trip. An unbounded name produced an
+    // unbounded secret key, which 500ed a credential read and truncated a
+    // stored key on the way to failing the delete that truncated it.
+    const atLimit = "a".repeat(MAX_PROVIDER_NAME_CHARS);
+    const pastLimit = "a".repeat(MAX_PROVIDER_NAME_CHARS + 1);
+
+    expect(checkProviderName(atLimit)).toBeNull();
+    expect(checkProviderName(pastLimit)).toBe("too-long");
+    expect(checkProviderName("   ")).toBe("empty");
+
+    expect(checkSlug([], atLimit)).toBeNull();
+    expect(checkSlug([], pastLimit)).toBe("too-long");
+
+    expect(customProviderReady([], { label: atLimit, baseUrl: "https://a.example/v1" })).toBe(
+      true,
+    );
+    expect(customProviderReady([], { label: pastLimit, baseUrl: "https://a.example/v1" })).toBe(
+      false,
+    );
   });
 });
 
@@ -135,6 +162,31 @@ describe("the endpoint an operator types", () => {
       expect(normalizeEndpoint(bad)).toBeNull();
     }
   });
+
+  it("refuses one that carries a credential, because an endpoint is stored as written", () => {
+    // Mirrors `catalogue::endpoint_has_credentials`. A `baseUrl` is returned
+    // by a `ScopedCompany` route every console reader calls, so a password in
+    // one is a password on the wire for everybody.
+    for (const bad of [
+      "http://alice:hunter2@127.0.0.1:8597/v1",
+      "https://alice@api.acme.example/v1",
+      "http://alice:hun@ter2@127.0.0.1:8597/v1",
+    ]) {
+      expect(endpointHasCredentials(bad)).toBe(true);
+      expect(normalizeEndpoint(bad)).toBeNull();
+    }
+  });
+
+  it("does not mistake an @ in the path for a credential", () => {
+    for (const good of [
+      "https://api.acme.example/v1/@me",
+      "https://api.acme.example/v1?to=a@b",
+      "https://api.acme.example/v1#a@b",
+    ]) {
+      expect(endpointHasCredentials(good)).toBe(false);
+      expect(normalizeEndpoint(good)).toBe(good);
+    }
+  });
 });
 
 describe("the custom-provider dialog's Add button", () => {

src/company/inference.rs

@@ -1489,13 +1489,18 @@ fn validate_parts(
     }
 
     let base_url = base_url.map(str::trim).filter(|s| !s.is_empty());
+    // Every echo of the typed URL below is redacted. A `base_url` is quoted back
+    // in a rejection the console renders, and a rejection is the one moment a
+    // malformed URL — the kind most likely to have been typed by hand with a
+    // password in it — is guaranteed to be shown to somebody.
     match provider {
         "ollama" | "openai_compatible" => match base_url {
             None => problems.push(format!(
                 "`[inference].base_url` is required for provider `{provider}` — give the OpenAI-compatible endpoint URL."
             )),
             Some(url) if !is_http_url(url) => problems.push(format!(
-                "`[inference].base_url` must be an `http://` or `https://` URL — you wrote `{url}`."
+                "`[inference].base_url` must be an `http://` or `https://` URL — you wrote `{}`.",
+                catalogue::redact_endpoint(url)
             )),
             _ => {}
         },
@@ -1504,12 +1509,30 @@ fn validate_parts(
                 && !is_http_url(url)
             {
                 problems.push(format!(
-                    "`[inference].base_url` must be an `http://` or `https://` URL — you wrote `{url}`."
+                    "`[inference].base_url` must be an `http://` or `https://` URL — you wrote `{}`.",
+                    catalogue::redact_endpoint(url)
                 ));
             }
         }
     }
 
+    // A credential in the endpoint, refused for the same reason
+    // `api_key_secret` refuses a pasted token just below: a `base_url` is stored
+    // as written, returned to every console reader on the company status read,
+    // and interpolated into operator-facing failure text. The console's own
+    // endpoint fields refuse this before anything is written
+    // (`catalogue::normalize_local_endpoint`); this is the manifest and
+    // console-`PUT` half of the same rule, so the two ways to set an endpoint
+    // cannot disagree about it.
+    if let Some(url) = base_url
+        && catalogue::endpoint_has_credentials(url)
+    {
+        problems.push(format!(
+            "`[inference].base_url` carries a username or password in the URL — you wrote `{}`. Remove them and store the credential in the key slot instead; an endpoint is readable by everyone who can see this company's settings.",
+            catalogue::redact_endpoint(url)
+        ));
+    }
+
     // The credential must be a *key name*, not the token itself. Reject values
     // that look like a pasted credential so a secret never lands in the manifest.
     if let Some(secret) = api_key_secret.map(str::trim).filter(|s| !s.is_empty())
@@ -2051,6 +2074,39 @@ mod tests {
         assert!(problems.iter().any(|p| p.contains("http")), "{problems:?}");
     }
 
+    #[test]
+    fn a_base_url_carrying_a_credential_is_rejected_and_never_echoed() {
+        // Same rule as `api_key_secret` below, one field over: a credential
+        // belongs in the write-only key slot, and a `base_url` is stored as
+        // written and read back by every console reader.
+        let mut m = inference("openai_compatible");
+        m.base_url = Some("http://alice:hunter2@127.0.0.1:8597/v1".into());
+        let problems = validate_inference(&m);
+        assert!(
+            problems.iter().any(|p| p.contains("username or password")),
+            "{problems:?}"
+        );
+        // The refusal is the one moment this value is guaranteed to be shown to
+        // somebody, so it must not quote the credential back.
+        for problem in &problems {
+            assert!(
+                !problem.contains("hunter2") && !problem.contains("alice"),
+                "a rejection echoed the credential it was rejecting: {problem}"
+            );
+        }
+
+        // A malformed URL is quoted back redacted too — and the malformed ones
+        // are the likeliest to have been typed by hand with a password in them.
+        let mut bad = inference("openai_compatible");
+        bad.base_url = Some("ftp://alice:hunter2@127.0.0.1/v1".into());
+        for problem in validate_inference(&bad) {
+            assert!(
+                !problem.contains("hunter2"),
+                "a rejection echoed the credential it was rejecting: {problem}"
+            );
+        }
+    }
+
     #[test]
     fn inline_credential_in_key_name_is_rejected() {
         let mut m = inference("openrouter");