fix(providers): compare the untrimmed credential, and cover the guard's branches

The guard trimmed the incoming value before comparing it, which catches a paste
carrying whitespace the password does not have. It missed the mirror case:
neither the login route nor the set-password route trims, so a dashboard
password may itself begin or end with a space, and an autofill reproducing it
exactly was trimmed into a value that no longer matched the stored hash. The
write then went through, which is the state this guard exists to prevent. Both
forms are compared now, the second only when the first fails on a string that
differs, so an ordinary key still costs a single bcrypt round.

Two branches carried no coverage and both are load-bearing. The catch that logs
and allows is the only path that lets a write through; a stored hash bcrypt
cannot parse reaches it without needing a mock, since the shape check accepts an
impossible cost factor that the comparison then rejects. The early return is
what keeps a token renewal -- a write carrying tokens but no apiKey -- from
paying for a settings read and a bcrypt round every time it fires, and the same
unparseable hash makes that path observable, so an absent warning is proof the
return happened.

The narrower scope is deliberate and now says so in the code: the OAuth tokens
arrive from a provider's token endpoint rather than from a form, so extending
the comparison to them would charge every renewal for a field no autofill can
reach.

Signed-off-by: Minxi Hou <houminxi@gmail.com>

src/lib/db/providers.ts

@@ -71,10 +71,17 @@ export class ManagementPasswordAsCredentialError extends Error {
  * Only an actual match blocks the write. A settings row that cannot be read,
  * or a bcrypt call that throws, logs and allows -- a guard against one specific
  * operator mistake must not become a way to lock out every connection write.
+ *
+ * Deliberately narrower than CONNECTION_CREDENTIAL_FIELDS. The OAuth tokens
+ * arrive from a provider's token endpoint, and the refresh path writes them
+ * back through updateProviderConnection on every renewal, so checking them
+ * would put a bcrypt round on a renewal path to defend a field no autofill
+ * reaches. apiKey is the only credential an operator types into a form.
  */
 async function assertApiKeyIsNotManagementPassword(apiKey: unknown): Promise<void> {
-  const candidate = typeof apiKey === "string" ? apiKey.trim() : "";
-  if (!candidate) return;
+  if (typeof apiKey !== "string") return;
+  const trimmed = apiKey.trim();
+  if (!trimmed) return;
 
   try {
     const settings = (await getSettings()) as JsonRecord;
@@ -82,7 +89,14 @@ async function assertApiKeyIsNotManagementPassword(apiKey: unknown): Promise<voi
     // Only a stored bcrypt hash is comparable. A fresh install that has never
     // bootstrapped a password has nothing to collide with.
     if (!isBcryptHash(stored)) return;
-    if (!(await verifyManagementPassword(candidate, stored))) return;
+    // Both forms of the value, because neither the login route nor the
+    // set-password route trims: a paste carries whitespace the password does
+    // not have, and a password is allowed to carry whitespace of its own. The
+    // second comparison only runs when the first fails on a different string.
+    const matches =
+      (await verifyManagementPassword(trimmed, stored)) ||
+      (trimmed !== apiKey && (await verifyManagementPassword(apiKey, stored)));
+    if (!matches) return;
   } catch (err) {
     console.warn(
       "[Providers] could not check the credential against the dashboard password:",

tests/unit/reject-management-password-as-apikey.test.ts

@@ -83,6 +83,26 @@ describe("management password as a provider credential", () => {
     );
   });
 
+  it("a password that carries its own whitespace is caught too", async () => {
+    // Neither the login route nor the set-password route trims, so this is a
+    // password an operator can really have. Comparing only the trimmed value
+    // would miss it and store the password the guard exists to reject.
+    const padded = `  ${DASHBOARD_PASSWORD}  `;
+    await storeDashboardPassword(padded);
+
+    await assert.rejects(
+      () =>
+        providersDb.createProviderConnection({
+          provider: "openai",
+          authType: "apikey",
+          name: "autofilled",
+          apiKey: padded,
+          isActive: true,
+        }),
+      /dashboard login password/
+    );
+  });
+
   it("a real API key is stored normally", async () => {
     await storeDashboardPassword(DASHBOARD_PASSWORD);
 
@@ -167,15 +187,74 @@ describe("management password as a provider credential", () => {
   });
 
   it("an OAuth connection carrying no apiKey never reaches the bcrypt round", async () => {
-    await storeDashboardPassword(DASHBOARD_PASSWORD);
+    // Storing a hash bcrypt cannot parse turns the expensive path into an
+    // observable one: reaching it throws and the catch logs. Silence is then
+    // proof the guard returned before touching settings at all, which is what
+    // keeps a token renewal -- a write that carries tokens but no apiKey --
+    // from paying for a database read and a bcrypt round on every renewal.
+    await settingsDb.updateSettings({ password: `$2a$99$${"a".repeat(53)}` });
 
-    const conn = await providersDb.createProviderConnection({
-      provider: "claude",
-      authType: "oauth",
-      name: "oauth",
-      accessToken: DASHBOARD_PASSWORD,
-      isActive: true,
-    });
-    assert.ok(conn?.id, "the guard covers apiKey only; OAuth tokens are not operator-typed");
+    const warnings: string[] = [];
+    const realWarn = console.warn;
+    console.warn = (...args: unknown[]) => {
+      warnings.push(args.map((a) => String(a)).join(" "));
+    };
+
+    try {
+      const conn = await providersDb.createProviderConnection({
+        provider: "claude",
+        authType: "oauth",
+        name: "oauth",
+        accessToken: DASHBOARD_PASSWORD,
+        isActive: true,
+      });
+      assert.ok(conn?.id, "the guard covers apiKey only");
+    } finally {
+      console.warn = realWarn;
+    }
+
+    assert.equal(
+      warnings.filter((w) => w.includes("could not check the credential")).length,
+      0,
+      "a write carrying no apiKey must not reach settings or bcrypt"
+    );
+  });
+
+  it("a check that cannot complete allows the write instead of blocking it", async () => {
+    // isBcryptHash validates shape, not semantics, so a stored hash carrying an
+    // impossible cost factor passes it and then makes bcrypt.compare throw. That
+    // reaches the same branch as an unreadable settings row, without a mock.
+    const unparseable = `$2a$99$${"a".repeat(53)}`;
+
+    // Asserted rather than assumed. Should bcrypt ever resolve instead of
+    // throwing here, the guard would take its no-match return and this test
+    // would quietly stop covering the catch branch, so name the reason first.
+    await assert.rejects(() => mgmt.verifyManagementPassword("anything", unparseable));
+
+    await settingsDb.updateSettings({ password: unparseable });
+
+    const warnings: string[] = [];
+    const realWarn = console.warn;
+    console.warn = (...args: unknown[]) => {
+      warnings.push(args.map((a) => String(a)).join(" "));
+    };
+
+    try {
+      const conn = await providersDb.createProviderConnection({
+        provider: "openai",
+        authType: "apikey",
+        name: "unverifiable",
+        apiKey: DASHBOARD_PASSWORD,
+        isActive: true,
+      });
+      assert.ok(conn?.id, "one broken settings row must not block every connection write");
+    } finally {
+      console.warn = realWarn;
+    }
+
+    assert.ok(
+      warnings.some((w) => w.includes("could not check the credential")),
+      "failing open silently would hide that the guard stopped guarding"
+    );
   });
 });