fix(security): validate credential payload reads
This commit is contained in:
1 parent
8b0b06f765
commit
254ee7c8d8
3 files changed
+135
-6
No files matched your search
@@ -1,12 +1,39 @@
|
||||
import type { AppDatabase } from "@main/database";
|
||||
import type { CredentialBackend, CredentialKind } from "@main/database/credentials";
|
||||
import type { CredentialStatus } from "@main/database/credentials";
|
||||
import type { CredentialSummary } from "@shared/contracts/schemas-credentials";
|
||||
import { credentialPayloadSchema, type CredentialSummary } from "@shared/contracts/schemas-credentials";
|
||||
import { z } from "zod";
|
||||
import { UpstreamError } from "../errors/index.js";
|
||||
import type { StoredCredentialPayload } from "./types";
|
||||
|
||||
const HINT_SUFFIX_LEN = 4;
|
||||
const HINT_PREFIX_LEN = 6;
|
||||
|
||||
const oauthCredentialPayloadSchema = z.object({
|
||||
refresh: z.string().min(1),
|
||||
access: z.string().min(1),
|
||||
expires: z.number().finite(),
|
||||
}).passthrough();
|
||||
|
||||
const storedCredentialPayloadSchema = z.union([
|
||||
credentialPayloadSchema,
|
||||
z.object({ kind: z.literal("oauth"), value: oauthCredentialPayloadSchema }),
|
||||
]);
|
||||
|
||||
export class CredentialStoreReadError extends UpstreamError {
|
||||
readonly connectorId: string;
|
||||
|
||||
constructor(connectorId: string, message: string) {
|
||||
super({
|
||||
code: "CREDENTIAL_INVALID",
|
||||
message,
|
||||
context: { connectorId },
|
||||
});
|
||||
this.name = "CredentialStoreReadError";
|
||||
this.connectorId = connectorId;
|
||||
}
|
||||
}
|
||||
|
||||
function maskApiKey(value: string): string {
|
||||
if (value.length <= HINT_PREFIX_LEN + HINT_SUFFIX_LEN) {
|
||||
return `${value.slice(0, 2)}...`;
|
||||
@@ -48,6 +75,36 @@ export interface CredentialStoreDeps {
|
||||
db: AppDatabase;
|
||||
}
|
||||
|
||||
function formatCredentialPayloadError(error: z.ZodError): string {
|
||||
return error.issues
|
||||
.map((issue) => {
|
||||
const path = issue.path.length > 0 ? issue.path.join(".") : "payload";
|
||||
return `${path}: ${issue.message}`;
|
||||
})
|
||||
.join("; ");
|
||||
}
|
||||
|
||||
function parseStoredCredentialPayload(
|
||||
connectorId: string,
|
||||
kind: CredentialKind,
|
||||
json: string,
|
||||
): StoredCredentialPayload {
|
||||
let value: unknown;
|
||||
try {
|
||||
value = JSON.parse(json);
|
||||
} catch {
|
||||
throw new CredentialStoreReadError(connectorId, "Stored credential payload is not valid JSON.");
|
||||
}
|
||||
const parsed = storedCredentialPayloadSchema.safeParse({ kind, value });
|
||||
if (!parsed.success) {
|
||||
throw new CredentialStoreReadError(
|
||||
connectorId,
|
||||
`Stored credential payload failed schema validation: ${formatCredentialPayloadError(parsed.error)}`,
|
||||
);
|
||||
}
|
||||
return parsed.data as StoredCredentialPayload;
|
||||
}
|
||||
|
||||
export class CredentialStore {
|
||||
constructor(private readonly deps: CredentialStoreDeps) {}
|
||||
|
||||
@@ -99,8 +156,7 @@ export class CredentialStore {
|
||||
row.backend,
|
||||
Buffer.isBuffer(row.ciphertext) ? row.ciphertext : Buffer.from(row.ciphertext),
|
||||
);
|
||||
const value = JSON.parse(json);
|
||||
return { kind: row.kind, value } as StoredCredentialPayload;
|
||||
return parseStoredCredentialPayload(connectorId, row.kind, json);
|
||||
}
|
||||
|
||||
remove(connectorId: string): void {
|
||||
|
||||
@@ -4,6 +4,7 @@ import { join } from "node:path";
|
||||
import { afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest";
|
||||
import { AppDatabase } from "../../src/main/database";
|
||||
import { CredentialStore } from "../../src/main/credentials/credential-store";
|
||||
import { CredentialStoreReadError } from "../../src/main/credentials/credential-store";
|
||||
import { CredentialResolver, CredentialError } from "../../src/main/credentials/credential-resolver";
|
||||
import { AppError } from "../../src/main/errors";
|
||||
|
||||
@@ -136,7 +137,7 @@ describe("CredentialResolver", () => {
|
||||
thrown = error;
|
||||
}
|
||||
|
||||
expect(thrown).toBeInstanceOf(CredentialError);
|
||||
expect(thrown).toBeInstanceOf(CredentialStoreReadError);
|
||||
expect(thrown).toMatchObject({
|
||||
kind: "upstream",
|
||||
code: "CREDENTIAL_INVALID",
|
||||
@@ -198,7 +199,7 @@ describe("CredentialResolver", () => {
|
||||
thrown = error;
|
||||
}
|
||||
|
||||
expect(thrown).toBeInstanceOf(CredentialError);
|
||||
expect(thrown).toBeInstanceOf(CredentialStoreReadError);
|
||||
expect(thrown).toMatchObject({
|
||||
kind: "upstream",
|
||||
code: "CREDENTIAL_INVALID",
|
||||
|
||||
@@ -3,7 +3,7 @@ import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { afterEach, beforeAll, beforeEach, describe, expect, it } from "vitest";
|
||||
import { AppDatabase } from "../../src/main/database";
|
||||
import { CredentialStore } from "../../src/main/credentials/credential-store";
|
||||
import { CredentialStore, CredentialStoreReadError } from "../../src/main/credentials/credential-store";
|
||||
|
||||
beforeAll(() => {
|
||||
// SecretVault throws unless safeStorage is available or this gate is set.
|
||||
@@ -62,6 +62,78 @@ describe("CredentialStore", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("round-trips an oauth credential through the read schema", () => {
|
||||
store.save("claude-pro", {
|
||||
kind: "oauth",
|
||||
value: {
|
||||
refresh: "refresh-token",
|
||||
access: "access-token",
|
||||
expires: Date.now() + 60_000,
|
||||
domain: "anthropic.com",
|
||||
},
|
||||
});
|
||||
|
||||
const decrypted = store.read("claude-pro");
|
||||
expect(decrypted?.kind).toBe("oauth");
|
||||
if (decrypted?.kind === "oauth") {
|
||||
expect(decrypted.value.refresh).toBe("refresh-token");
|
||||
expect(decrypted.value.access).toBe("access-token");
|
||||
expect(decrypted.value.domain).toBe("anthropic.com");
|
||||
}
|
||||
});
|
||||
|
||||
it("rejects stored credentials whose decrypted payload does not match the row kind", () => {
|
||||
const encrypted = db.vault.encrypt(JSON.stringify({ source: "literal", value: "sk-test-abcdefghij" }));
|
||||
db.core.credentials.upsert({
|
||||
connectorId: "amazon-bedrock",
|
||||
kind: "aws",
|
||||
backend: encrypted.backend,
|
||||
ciphertext: encrypted.payload,
|
||||
displayHint: "bad",
|
||||
status: "active",
|
||||
});
|
||||
|
||||
let thrown: unknown;
|
||||
try {
|
||||
store.read("amazon-bedrock");
|
||||
} catch (error) {
|
||||
thrown = error;
|
||||
}
|
||||
|
||||
expect(thrown).toBeInstanceOf(CredentialStoreReadError);
|
||||
expect(thrown).toMatchObject({
|
||||
code: "CREDENTIAL_INVALID",
|
||||
context: { connectorId: "amazon-bedrock" },
|
||||
});
|
||||
expect(thrown).toHaveProperty("message", expect.stringContaining("failed schema validation"));
|
||||
});
|
||||
|
||||
it("rejects stored credentials whose ciphertext is not valid JSON", () => {
|
||||
const encrypted = db.vault.encrypt("{not-json");
|
||||
db.core.credentials.upsert({
|
||||
connectorId: "anthropic",
|
||||
kind: "api_key",
|
||||
backend: encrypted.backend,
|
||||
ciphertext: encrypted.payload,
|
||||
displayHint: "bad",
|
||||
status: "active",
|
||||
});
|
||||
|
||||
let thrown: unknown;
|
||||
try {
|
||||
store.read("anthropic");
|
||||
} catch (error) {
|
||||
thrown = error;
|
||||
}
|
||||
|
||||
expect(thrown).toBeInstanceOf(CredentialStoreReadError);
|
||||
expect(thrown).toMatchObject({
|
||||
code: "CREDENTIAL_INVALID",
|
||||
context: { connectorId: "anthropic" },
|
||||
});
|
||||
expect(thrown).toHaveProperty("message", "Stored credential payload is not valid JSON.");
|
||||
});
|
||||
|
||||
it("removes a credential", () => {
|
||||
store.save("openai", { kind: "api_key", value: { source: "literal", value: "sk-test-abcdefghij" } });
|
||||
expect(store.list().some((row) => row.connectorId === "openai")).toBe(true);
|
||||
|
||||
Reference in new issue
Block a user