From a9cdf5ce0f4e4f622997780352c11705ca51539d Mon Sep 17 00:00:00 2001 From: Ben Thomas Date: Fri, 30 Oct 2026 15:35:29 -0700 Subject: [PATCH 1/2] fix(hub): preserve canonical provider keys in audits Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../index.db.test.ts | 15 ++-- .../index.db.test.ts | 24 +++++-- .../index.test.ts | 70 +++++++++++++++++++ .../create-security-audit-context/index.ts | 46 +++++++++--- .../create-security-audit/index.test.ts | 25 +++++++ .../get-security-audit-data/index.ts | 18 ++--- .../parse-security-audit-event/index.ts | 4 +- .../parse-security-audit-event/types.ts | 21 ++++++ .../utils/parse-provider-key/index.test.ts | 15 ++-- hub/server/utils/parse-provider-key/index.ts | 7 +- 10 files changed, 203 insertions(+), 42 deletions(-) create mode 100644 hub/server/repositories/security-audits/create-security-audit-context/index.test.ts create mode 100644 hub/server/repositories/security-audits/parse-security-audit-event/types.ts diff --git a/hub/server/domains/identity/resolve-or-create-external-identity/index.db.test.ts b/hub/server/domains/identity/resolve-or-create-external-identity/index.db.test.ts index c77256b8..b5cdac85 100644 --- a/hub/server/domains/identity/resolve-or-create-external-identity/index.db.test.ts +++ b/hub/server/domains/identity/resolve-or-create-external-identity/index.db.test.ts @@ -52,7 +52,8 @@ function controlledConcurrentTransaction(): IdentityCreationTransaction { describe("resolveOrCreateExternalIdentity()", () => { it("atomically creates a user, immutable external identity, and success audit", async () => { - const result = await resolveOrCreateExternalIdentity(tuple, dependencies()) + const acceptedTuple = { ...tuple, provider: "secret:workforce" } + const result = await resolveOrCreateExternalIdentity(acceptedTuple, dependencies()) expect(result.status).toBe("created") await expect(prisma.user.findMany()).resolves.toEqual([ @@ -62,7 +63,7 @@ describe("resolveOrCreateExternalIdentity()", () => { expect.objectContaining({ id: result.externalIdentityId, userId: result.userId, - ...tuple, + ...acceptedTuple, }), ]) await expect(prisma.securityAudit.findMany()).resolves.toEqual([ @@ -70,7 +71,7 @@ describe("resolveOrCreateExternalIdentity()", () => { action: "identity.external.created", outcome: "SUCCESS", targetId: result.externalIdentityId, - context: { provider: tuple.provider, issuer: tuple.issuer }, + context: { provider: acceptedTuple.provider, issuer: acceptedTuple.issuer }, }), ]) }) @@ -145,6 +146,7 @@ describe("resolveOrCreateExternalIdentity()", () => { { ...tuple, id: randomUUID() }, { ...tuple, userId: victimUserId }, { ...tuple, provider: { value: tuple.provider } }, + { ...tuple, provider: "provider key" }, new IdentityInput(), ]) { await expect(resolveOrCreateExternalIdentity(input, dependencies())).rejects.toThrow() @@ -177,6 +179,7 @@ describe("resolveOrCreateExternalIdentity()", () => { }) it("rolls back identity state when the final success audit insert fails", async () => { + const acceptedTuple = { ...tuple, provider: "secret:workforce" } const rawErrorMarker = "raw-success-audit-error-marker" const base = dependencies() async function runTransaction( @@ -191,7 +194,7 @@ describe("resolveOrCreateExternalIdentity()", () => { } await expect( - resolveOrCreateExternalIdentity(tuple, { ...base, runTransaction }), + resolveOrCreateExternalIdentity(acceptedTuple, { ...base, runTransaction }), ).rejects.toThrow(rawErrorMarker) await expect(prisma.user.count()).resolves.toBe(0) await expect(prisma.externalIdentity.count()).resolves.toBe(0) @@ -199,8 +202,8 @@ describe("resolveOrCreateExternalIdentity()", () => { expect(audits).toHaveLength(1) expect(audits[0]?.action).toBe("identity.external.create_failed") expect(audits[0]?.context).toEqual({ - issuer: tuple.issuer, - provider: tuple.provider, + issuer: acceptedTuple.issuer, + provider: acceptedTuple.provider, reason: "transaction_error", }) expect(JSON.stringify(audits)).not.toContain(rawErrorMarker) diff --git a/hub/server/domains/identity/validate-and-persist-provider-revision/index.db.test.ts b/hub/server/domains/identity/validate-and-persist-provider-revision/index.db.test.ts index 241fe06f..6c29cd69 100644 --- a/hub/server/domains/identity/validate-and-persist-provider-revision/index.db.test.ts +++ b/hub/server/domains/identity/validate-and-persist-provider-revision/index.db.test.ts @@ -114,7 +114,7 @@ describe("validateAndPersistProviderRevision()", () => { }) it("uses the same opaque provider key for provider state and typed audits", async () => { - const providerKey = "GitHub.App/V2" + const providerKey = "secret:workforce" const result = await validateAndPersistProviderRevision( { ...input(1), providerKey }, @@ -122,8 +122,15 @@ describe("validateAndPersistProviderRevision()", () => { ) await expect( - prisma.authenticationProvider.findUnique({ where: { id: result.providerId } }), - ).resolves.toMatchObject({ key: providerKey }) + prisma.authenticationProvider.findUnique({ + where: { id: result.providerId }, + include: { selectedRevision: true }, + }), + ).resolves.toMatchObject({ + key: providerKey, + selectedRevision: { id: result.revisionId, revision: 1 }, + }) + await expect(prisma.authenticationProviderRevision.count()).resolves.toBe(1) await expect( prisma.securityAudit.findFirst({ where: { action: "identity.provider_revision.selected" }, @@ -136,6 +143,7 @@ describe("validateAndPersistProviderRevision()", () => { ), ).rejects.toThrow() await expect(prisma.authenticationProvider.count()).resolves.toBe(1) + await expect(prisma.securityAudit.count()).resolves.toBe(1) }) it("persists no validated configuration value beyond its digest", async () => { @@ -332,7 +340,11 @@ describe("validateAndPersistProviderRevision()", () => { }) it("rolls back revision selection when the final success audit insert fails", async () => { - const first = await validateAndPersistProviderRevision(input(1), dependencies()) + const providerKey = "secret:workforce" + const first = await validateAndPersistProviderRevision( + { ...input(1), providerKey }, + dependencies(), + ) const rawErrorMarker = "raw-provider-success-audit-error-marker" const base = dependencies() async function runTransaction( @@ -347,7 +359,7 @@ describe("validateAndPersistProviderRevision()", () => { } await expect( - validateAndPersistProviderRevision(input(2), { ...base, runTransaction }), + validateAndPersistProviderRevision({ ...input(2), providerKey }, { ...base, runTransaction }), ).rejects.toThrow(rawErrorMarker) await expect(prisma.authenticationProviderRevision.count()).resolves.toBe(1) await expect( @@ -360,7 +372,7 @@ describe("validateAndPersistProviderRevision()", () => { expect(failureAudits).toHaveLength(1) expect(failureAudits[0]?.action).toBe("identity.provider_revision.persist_failed") expect(failureAudits[0]?.context).toEqual({ - provider: "workforce", + provider: providerKey, reason: "transaction_error", revision: 2, }) diff --git a/hub/server/repositories/security-audits/create-security-audit-context/index.test.ts b/hub/server/repositories/security-audits/create-security-audit-context/index.test.ts new file mode 100644 index 00000000..4caaddd1 --- /dev/null +++ b/hub/server/repositories/security-audits/create-security-audit-context/index.test.ts @@ -0,0 +1,70 @@ +import { describe, expect, it } from "vitest" + +import { parseProviderKey } from "../../../utils/parse-provider-key" +import { createSecurityAuditContext, createSecurityAuditContextWithProviderKey } from "." + +describe("createSecurityAuditContext()", () => { + it.each([ + "secret:workforce", + "Bearer opaque-value", + ["-----BEGIN PRIVATE", " KEY-----"].join(""), + ])("rejects the untyped credential-shaped value %s", (value) => { + expect(() => createSecurityAuditContext({ value })).toThrow( + "cannot contain credential or key material", + ) + }) + + it("rejects an untyped client secret field", () => { + expect(() => createSecurityAuditContext({ clientSecret: "opaque-value" })).toThrow( + "cannot contain secret-bearing fields", + ) + }) + + it("retains generic context string, entry, and byte bounds", () => { + expect(() => createSecurityAuditContext({ value: "a".repeat(501) })).toThrow( + "cannot exceed 500 characters", + ) + expect(() => + createSecurityAuditContext({ + first: 1, + second: 2, + third: 3, + fourth: 4, + fifth: 5, + sixth: 6, + seventh: 7, + eighth: 8, + ninth: 9, + }), + ).toThrow("cannot exceed 8 fields") + expect(() => + createSecurityAuditContext({ + first: "界".repeat(500), + second: "界".repeat(500), + third: "界".repeat(500), + fourth: "界".repeat(500), + fifth: "界".repeat(500), + sixth: "界".repeat(500), + seventh: "界".repeat(500), + eighth: "界".repeat(500), + }), + ).toThrow("cannot exceed 4096 bytes") + }) + + it("retains context bounds for a canonically parsed provider key", () => { + const provider = parseProviderKey("secret:workforce") + + expect(() => + createSecurityAuditContextWithProviderKey({ + provider, + first: "界".repeat(500), + second: "界".repeat(500), + third: "界".repeat(500), + fourth: "界".repeat(500), + fifth: "界".repeat(500), + sixth: "界".repeat(500), + seventh: "界".repeat(500), + }), + ).toThrow("cannot exceed 4096 bytes") + }) +}) diff --git a/hub/server/repositories/security-audits/create-security-audit-context/index.ts b/hub/server/repositories/security-audits/create-security-audit-context/index.ts index 6ddbfa80..b6203c92 100644 --- a/hub/server/repositories/security-audits/create-security-audit-context/index.ts +++ b/hub/server/repositories/security-audits/create-security-audit-context/index.ts @@ -1,15 +1,39 @@ import { normalizeBoundedJson } from "../../../utils/normalize-bounded-json/index.ts" +import type { ProviderKey } from "../../../utils/parse-provider-key/index.ts" -export function createSecurityAuditContext( - value: Record, +type SecurityAuditContextValue = boolean | number | string | null + +const limits = { + maxArrayItems: 0, + maxBytes: 4_096, + maxDepth: 1, + maxEntries: 8, + maxKeyLength: 100, + maxObjectFields: 8, + maxStringLength: 500, +} + +function assertContextByteLimit(value: object) { + if (Buffer.byteLength(JSON.stringify(value), "utf8") > limits.maxBytes) { + throw new Error(`JSON data cannot exceed ${limits.maxBytes} bytes.`) + } +} + +export function createSecurityAuditContext(value: Record) { + return normalizeBoundedJson(value, limits) +} + +export function createSecurityAuditContextWithProviderKey( + value: Record & { provider: ProviderKey }, ) { - return normalizeBoundedJson(value, { - maxArrayItems: 0, - maxBytes: 4_096, - maxDepth: 1, - maxEntries: 8, - maxKeyLength: 100, - maxObjectFields: 8, - maxStringLength: 500, - }) + const normalized = normalizeBoundedJson( + { + ...value, + provider: "x".repeat(value.provider.length), + }, + limits, + ) + const context = { ...normalized, provider: value.provider } + assertContextByteLimit(context) + return context } diff --git a/hub/server/repositories/security-audits/create-security-audit/index.test.ts b/hub/server/repositories/security-audits/create-security-audit/index.test.ts index 07d74ba4..fb2246d2 100644 --- a/hub/server/repositories/security-audits/create-security-audit/index.test.ts +++ b/hub/server/repositories/security-audits/create-security-audit/index.test.ts @@ -4,6 +4,31 @@ import type { Prisma } from "../../../generated/prisma/client" import { createSecurityAudit } from "." describe("createSecurityAudit()", () => { + it.each(["secret:workforce", "GitHub.App/V2", "Entra:Tenant", "custom+提供者"])( + "stores the exact canonical provider key %s through the typed audit mapping", + async (providerKey) => { + const create = vi.fn<(input: { data: Prisma.SecurityAuditCreateInput }) => Promise>() + create.mockResolvedValue(undefined) + + await createSecurityAudit( + { securityAudit: { create } }, + { + id: "5dd31031-6f1c-4c16-87c2-aee5342145cc", + type: "provider_revision_selected", + providerId: "df75faa4-ed4c-44ed-8db0-443cc3b426ed", + providerKey, + revision: 1, + }, + ) + + expect(create).toHaveBeenCalledOnce() + expect(create.mock.calls[0]?.[0].data.context).toEqual({ + provider: providerKey, + revision: 1, + }) + }, + ) + it("maps each kernel event to its allowlisted audit record", async () => { const create = vi.fn<(input: { data: Prisma.SecurityAuditCreateInput }) => Promise>() create.mockResolvedValue(undefined) diff --git a/hub/server/repositories/security-audits/get-security-audit-data/index.ts b/hub/server/repositories/security-audits/get-security-audit-data/index.ts index 4e9a8391..d1bd9213 100644 --- a/hub/server/repositories/security-audits/get-security-audit-data/index.ts +++ b/hub/server/repositories/security-audits/get-security-audit-data/index.ts @@ -1,10 +1,10 @@ import type { Prisma } from "../../../generated/prisma/client.ts" import { SecurityAuditOutcome } from "../../../generated/prisma/enums.ts" -import { createSecurityAuditContext } from "../create-security-audit-context/index.ts" -import type { CreateSecurityAuditEvent } from "../create-security-audit/types.ts" +import { createSecurityAuditContextWithProviderKey } from "../create-security-audit-context/index.ts" +import type { ParsedSecurityAuditEvent } from "../parse-security-audit-event/types.ts" export function getSecurityAuditData( - event: CreateSecurityAuditEvent, + event: ParsedSecurityAuditEvent, ): Prisma.SecurityAuditCreateInput { switch (event.type) { case "external_identity_created": @@ -14,7 +14,7 @@ export function getSecurityAuditData( outcome: SecurityAuditOutcome.SUCCESS, targetType: "external_identity", targetId: event.externalIdentityId, - context: createSecurityAuditContext({ + context: createSecurityAuditContextWithProviderKey({ provider: event.provider, issuer: event.issuer, }), @@ -25,7 +25,7 @@ export function getSecurityAuditData( action: "identity.external.create_failed", outcome: SecurityAuditOutcome.FAILURE, targetType: "external_identity", - context: createSecurityAuditContext({ + context: createSecurityAuditContextWithProviderKey({ provider: event.provider, issuer: event.issuer, reason: event.reason, @@ -38,7 +38,7 @@ export function getSecurityAuditData( outcome: SecurityAuditOutcome.SUCCESS, targetType: "authentication_provider", targetId: event.providerId, - context: createSecurityAuditContext({ + context: createSecurityAuditContextWithProviderKey({ provider: event.providerKey, revision: event.revision, }), @@ -49,7 +49,7 @@ export function getSecurityAuditData( action: "identity.provider_revision.validation_failed", outcome: SecurityAuditOutcome.FAILURE, targetType: "authentication_provider", - context: createSecurityAuditContext({ + context: createSecurityAuditContextWithProviderKey({ provider: event.providerKey, revision: event.revision, reason: "validation_failed", @@ -61,7 +61,7 @@ export function getSecurityAuditData( action: "identity.provider_revision.denied", outcome: SecurityAuditOutcome.DENIED, targetType: "authentication_provider", - context: createSecurityAuditContext({ + context: createSecurityAuditContextWithProviderKey({ provider: event.providerKey, revision: event.revision, reason: event.reason, @@ -73,7 +73,7 @@ export function getSecurityAuditData( action: "identity.provider_revision.persist_failed", outcome: SecurityAuditOutcome.FAILURE, targetType: "authentication_provider", - context: createSecurityAuditContext({ + context: createSecurityAuditContextWithProviderKey({ provider: event.providerKey, revision: event.revision, reason: event.reason, diff --git a/hub/server/repositories/security-audits/parse-security-audit-event/index.ts b/hub/server/repositories/security-audits/parse-security-audit-event/index.ts index 3a346fd0..f92f2be6 100644 --- a/hub/server/repositories/security-audits/parse-security-audit-event/index.ts +++ b/hub/server/repositories/security-audits/parse-security-audit-event/index.ts @@ -1,7 +1,7 @@ import { z } from "zod" import { parseProviderKey } from "../../../utils/parse-provider-key/index.ts" -import type { CreateSecurityAuditEvent } from "../create-security-audit/types.ts" +import type { ParsedSecurityAuditEvent } from "./types.ts" const eventSchema = z.discriminatedUnion("type", [ z @@ -59,7 +59,7 @@ const eventSchema = z.discriminatedUnion("type", [ .strict(), ]) -export function parseSecurityAuditEvent(input: unknown): CreateSecurityAuditEvent { +export function parseSecurityAuditEvent(input: unknown): ParsedSecurityAuditEvent { const event = eventSchema.parse(input) switch (event.type) { case "external_identity_created": diff --git a/hub/server/repositories/security-audits/parse-security-audit-event/types.ts b/hub/server/repositories/security-audits/parse-security-audit-event/types.ts new file mode 100644 index 00000000..6f6f9b9c --- /dev/null +++ b/hub/server/repositories/security-audits/parse-security-audit-event/types.ts @@ -0,0 +1,21 @@ +import type { ProviderKey } from "../../../utils/parse-provider-key/index.ts" +import type { + ExternalIdentityCreatedAudit, + ExternalIdentityCreateFailedAudit, + ProviderRevisionDeniedAudit, + ProviderRevisionPersistFailedAudit, + ProviderRevisionSelectedAudit, + ProviderRevisionValidationFailedAudit, +} from "../create-security-audit/types.ts" + +export type ParsedSecurityAuditEvent = + | (Omit & { provider: ProviderKey }) + | (Omit & { provider: ProviderKey }) + | (Omit & { + providerKey: ProviderKey + }) + | (Omit & { providerKey: ProviderKey }) + | (Omit & { providerKey: ProviderKey }) + | (Omit & { + providerKey: ProviderKey + }) diff --git a/hub/server/utils/parse-provider-key/index.test.ts b/hub/server/utils/parse-provider-key/index.test.ts index 7fcb3119..6d3602b8 100644 --- a/hub/server/utils/parse-provider-key/index.test.ts +++ b/hub/server/utils/parse-provider-key/index.test.ts @@ -3,12 +3,15 @@ import { describe, expect, it } from "vitest" import { parseProviderKey, providerKeyMaxBytes, providerKeyMaxCharacters } from "." describe("parseProviderKey()", () => { - it.each(["GitHub.App/V2", "Entra:Tenant", "custom+adapter@example", "提供者"])( - "preserves the opaque case-sensitive key %s", - (key) => { - expect(parseProviderKey(key)).toBe(key) - }, - ) + it.each([ + "secret:workforce", + "GitHub.App/V2", + "Entra:Tenant", + "custom+adapter@example", + "提供者", + ])("preserves the opaque case-sensitive key %s", (key) => { + expect(parseProviderKey(key)).toBe(key) + }) it("accepts exact character and UTF-8 byte boundaries", () => { expect(parseProviderKey("a".repeat(providerKeyMaxCharacters))).toHaveLength( diff --git a/hub/server/utils/parse-provider-key/index.ts b/hub/server/utils/parse-provider-key/index.ts index ca46d940..5d0574f6 100644 --- a/hub/server/utils/parse-provider-key/index.ts +++ b/hub/server/utils/parse-provider-key/index.ts @@ -3,9 +3,12 @@ import { z } from "zod" export const providerKeyMaxCharacters = 100 export const providerKeyMaxBytes = 400 +declare const providerKeyBrand: unique symbol +export type ProviderKey = string & { readonly [providerKeyBrand]: true } + const providerKeySchema = z.string().min(1) -export function parseProviderKey(input: unknown): string { +export function parseProviderKey(input: unknown): ProviderKey { const key = providerKeySchema.parse(input) if ( [...key].length > providerKeyMaxCharacters || @@ -16,5 +19,5 @@ export function parseProviderKey(input: unknown): string { if (/[\p{C}\p{Z}\s]/u.test(key)) { throw new Error("Provider key cannot contain control or whitespace characters.") } - return key + return key as ProviderKey } From 64f8ee98ee137e85e24ce9d1d96b008f2dfebea4 Mon Sep 17 00:00:00 2001 From: Ben Thomas Date: Fri, 30 Oct 2026 19:59:39 -0700 Subject: [PATCH 2/2] test(hub): cover provider audit context boundaries Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../index.test.ts | 71 ++++++++++++++++--- 1 file changed, 60 insertions(+), 11 deletions(-) diff --git a/hub/server/repositories/security-audits/create-security-audit-context/index.test.ts b/hub/server/repositories/security-audits/create-security-audit-context/index.test.ts index 4caaddd1..6ede5d34 100644 --- a/hub/server/repositories/security-audits/create-security-audit-context/index.test.ts +++ b/hub/server/repositories/security-audits/create-security-audit-context/index.test.ts @@ -51,20 +51,69 @@ describe("createSecurityAuditContext()", () => { ).toThrow("cannot exceed 4096 bytes") }) - it("retains context bounds for a canonically parsed provider key", () => { - const provider = parseProviderKey("secret:workforce") + it("accepts 4096 restored bytes and rejects 4097 after placeholder validation", () => { + const provider = parseProviderKey("界".repeat(100)) + const atLimit = { + first: `${"界".repeat(116)}${"a".repeat(352)}`, + second: "a".repeat(500), + third: "a".repeat(500), + fourth: "a".repeat(500), + fifth: "a".repeat(500), + sixth: "a".repeat(500), + seventh: "a".repeat(500), + provider, + } + const overLimit = { + ...atLimit, + first: `${"界".repeat(116)}é${"a".repeat(351)}`, + } + const placeholder = { ...atLimit, provider: "x".repeat(provider.length) } + const overLimitPlaceholder = { + ...overLimit, + provider: "x".repeat(provider.length), + } + expect(Buffer.byteLength(JSON.stringify(placeholder), "utf8")).toBe(3_896) + expect(Buffer.byteLength(JSON.stringify(overLimitPlaceholder), "utf8")).toBe(3_897) + expect(Buffer.byteLength(JSON.stringify(atLimit), "utf8")).toBe(4_096) + expect(Buffer.byteLength(JSON.stringify(overLimit), "utf8")).toBe(4_097) + expect(createSecurityAuditContext(overLimitPlaceholder)).toEqual(overLimitPlaceholder) + expect(createSecurityAuditContextWithProviderKey(atLimit)).toEqual(atLimit) + expect(() => createSecurityAuditContextWithProviderKey(overLimit)).toThrow( + "cannot exceed 4096 bytes", + ) + }) + + it.each([ + ["issuer", ["Bear", "er opaque-value"].join("")], + ["reason", ["-----BEGIN ", "PRIVATE KEY-----"].join("")], + ["metadata", ["client-", "secret:opaque-value"].join("")], + ])("rejects credential-shaped %s outside the trusted provider field", (key, value) => { expect(() => createSecurityAuditContextWithProviderKey({ - provider, - first: "界".repeat(500), - second: "界".repeat(500), - third: "界".repeat(500), - fourth: "界".repeat(500), - fifth: "界".repeat(500), - sixth: "界".repeat(500), - seventh: "界".repeat(500), + provider: parseProviderKey("secret:workforce"), + [key]: value, }), - ).toThrow("cannot exceed 4096 bytes") + ).toThrow("cannot contain credential or key material") + }) + + it("accepts safe ordinary metadata beside the trusted provider field", () => { + expect( + createSecurityAuditContextWithProviderKey({ + provider: parseProviderKey("secret:workforce"), + issuer: "https://id.example.test", + reason: "transaction_error", + revision: 2, + retryable: false, + detail: null, + }), + ).toEqual({ + detail: null, + issuer: "https://id.example.test", + provider: "secret:workforce", + reason: "transaction_error", + retryable: false, + revision: 2, + }) }) })