From d4b868ff551ac7ba7f4c4d709e27d2eb1e1c9a30 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 14:10:11 -0600 Subject: [PATCH 1/5] fix(health): prompt for sign-in from a runtime-observed provider invalidation proof A browser-session connection prompted for sign-in only when a run failed with a session or 401 reason. No production code produced the verified provider_invalidation_proof that connection-health already accepts, so the projection input was always null. The runtime now records a proof when a run fails and the last top-level page URL of its own leased browser surface matches the connector manifest's optional runtime_requirements.bindings.browser.sign_in_url_pattern. Visiting the sign-in page on a healthy run is not proof, and nothing reads connector-reported gaps or message text. A succeeded run marks earlier proofs repaired, which suppresses replay. connection-health now evaluates a verified proof before the credentials-accepted branch, so a run with no failure reason no longer hides it. The summary path loads the durable proof into browserSurfaceRepair. Connectors without a pattern behave as before. Fixes #647 Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../browser-surface/sign-in-proof-recorder.ts | 67 +++++ .../runtime/browser-surface/sign-in-proof.ts | 116 +++++++++ .../runtime/connection-health.ts | 7 + .../runtime/controller.ts | 42 ++++ .../server/backup-table-policy.ts | 5 + .../server/connector-manifest-validation.ts | 7 + reference-implementation/server/db.ts | 12 + .../server/postgres-storage.ts | 11 + .../server/ref-control.ts | 30 +++ .../provider-invalidation-proof-store.ts | 144 +++++++++++ .../test/connection-health.test.ts | 32 +++ ...ectors-provider-invalidation-proof.test.ts | 235 ++++++++++++++++++ .../test/sign-in-proof.test.ts | 109 ++++++++ 13 files changed, 817 insertions(+) create mode 100644 reference-implementation/runtime/browser-surface/sign-in-proof-recorder.ts create mode 100644 reference-implementation/runtime/browser-surface/sign-in-proof.ts create mode 100644 reference-implementation/server/stores/provider-invalidation-proof-store.ts create mode 100644 reference-implementation/test/ref-connectors-provider-invalidation-proof.test.ts create mode 100644 reference-implementation/test/sign-in-proof.test.ts diff --git a/reference-implementation/runtime/browser-surface/sign-in-proof-recorder.ts b/reference-implementation/runtime/browser-surface/sign-in-proof-recorder.ts new file mode 100644 index 000000000..afc850a81 --- /dev/null +++ b/reference-implementation/runtime/browser-surface/sign-in-proof-recorder.ts @@ -0,0 +1,67 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +import type { ProviderInvalidationProofStore } from "../../server/stores/provider-invalidation-proof-store.ts"; +import { + deriveSignInProof, + observeLastTopLevelUrl, + signInUrlPatternFromManifest, +} from "./sign-in-proof.ts"; + +export interface RecordSignInProofInput { + /** The leased surface's DevTools base URL, or null when the run had no leased surface. */ + readonly cdpUrl: string | null; + readonly connectionId: string; + readonly connectorId: string; + readonly manifest: Parameters[0]; + readonly nowIso: string; + readonly runId: string; + /** The run's terminal status from the runtime result. */ + readonly status: string | null | undefined; +} + +export interface RecordSignInProofDeps { + readonly fetchImpl?: typeof fetch; + readonly store: ProviderInvalidationProofStore; +} + +/** + * Run-completion writer. A failed run on a connector that declares a sign-in + * pattern records a proof when the run's last top-level URL matches it. A + * succeeded run marks earlier proofs repaired so they cannot prompt again. + * Connectors without a pattern, and every other status, change nothing. + */ +export async function recordSignInProofAfterRun( + deps: RecordSignInProofDeps, + input: RecordSignInProofInput, +): Promise<"recorded" | "repaired" | "none"> { + const pattern = signInUrlPatternFromManifest(input.manifest); + if (!pattern) { + return "none"; + } + if (input.status === "succeeded") { + await deps.store.markRepaired(input.connectionId, input.nowIso); + return "repaired"; + } + if (input.status !== "failed" || !input.cdpUrl) { + return "none"; + } + const lastTopLevelUrl = await observeLastTopLevelUrl( + input.cdpUrl, + deps.fetchImpl, + ).catch(() => null); + const proof = deriveSignInProof({ + connectionId: input.connectionId, + lastTopLevelUrl, + observedAt: input.nowIso, + pattern, + provider: input.connectorId, + runFailed: true, + runId: input.runId, + }); + if (!proof) { + return "none"; + } + await deps.store.record(proof); + return "recorded"; +} diff --git a/reference-implementation/runtime/browser-surface/sign-in-proof.ts b/reference-implementation/runtime/browser-surface/sign-in-proof.ts new file mode 100644 index 000000000..46a6d2c55 --- /dev/null +++ b/reference-implementation/runtime/browser-surface/sign-in-proof.ts @@ -0,0 +1,116 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +import type { ProviderInvalidationProof } from "./repair-decision.ts"; + +/** + * Runtime-side provider-invalidation proof. + * + * A proof exists only when the run FAILED and the runtime's own observation of + * the run's browser (its last top-level page URL) matched the sign-in URL + * pattern the connector manifest declares. A visit to the sign-in page on a + * healthy run is not proof, and nothing here reads connector-reported gaps or + * message text. + */ + +const MAX_PATTERN_LENGTH = 512; +const MAX_URL_LENGTH = 2048; + +/** Declared at `runtime_requirements.bindings.browser.sign_in_url_pattern`. */ +export function signInUrlPatternFromManifest(manifest: { + readonly runtime_requirements?: { + readonly bindings?: Record; + }; +}): string | null { + const browser = manifest.runtime_requirements?.bindings?.browser; + if ( + browser === null || + typeof browser !== "object" || + Array.isArray(browser) + ) { + return null; + } + const pattern = (browser as { readonly sign_in_url_pattern?: unknown }) + .sign_in_url_pattern; + return typeof pattern === "string" && + pattern.trim().length > 0 && + pattern.length <= MAX_PATTERN_LENGTH + ? pattern.trim() + : null; +} + +/** Anchored glob over the whole URL; `*` matches any run of characters, everything else is literal. */ +export function signInUrlMatchesPattern(pattern: string, url: string): boolean { + if (pattern.length > MAX_PATTERN_LENGTH || url.length > MAX_URL_LENGTH) { + return false; + } + const source = pattern + .split("*") + .map((part) => part.replace(/[.+?^${}()|[\]\\]/g, "\\$&")) + .join(".*"); + return new RegExp(`^${source}$`).test(url); +} + +/** + * Reads the most recent top-level page URL from the run's own browser surface + * (DevTools target list on the surface the runtime leased). It sends nothing + * to the provider. + */ +export async function observeLastTopLevelUrl( + cdpUrl: string, + fetchImpl: typeof fetch = globalThis.fetch.bind(globalThis), + timeoutMs = 3000, +): Promise { + const signal = AbortSignal.timeout(timeoutMs); + const response = await fetchImpl(new URL("/json/list", cdpUrl), { signal }); + if (!response.ok) { + return null; + } + const targets: unknown = await response.json(); + if (!Array.isArray(targets)) { + return null; + } + for (const target of targets as readonly { + readonly type?: unknown; + readonly url?: unknown; + }[]) { + if ( + target?.type === "page" && + typeof target.url === "string" && + /^https?:/i.test(target.url) + ) { + return target.url; + } + } + return null; +} + +export interface SignInProofInput { + readonly connectionId: string; + readonly lastTopLevelUrl: string | null; + readonly observedAt: string; + readonly pattern: string | null; + readonly provider: string; + readonly runFailed: boolean; + readonly runId: string; +} + +/** Pure rule: failed run AND last top-level URL matches the declared pattern. */ +export function deriveSignInProof( + input: SignInProofInput, +): ProviderInvalidationProof | null { + if (!(input.runFailed && input.pattern && input.lastTopLevelUrl)) { + return null; + } + if (!signInUrlMatchesPattern(input.pattern, input.lastTopLevelUrl)) { + return null; + } + return { + connection_id: input.connectionId, + evidence_id: input.runId, + kind: "provider_invalidation_proof", + observed_at: input.observedAt, + provider: input.provider, + verified: true, + }; +} diff --git a/reference-implementation/runtime/connection-health.ts b/reference-implementation/runtime/connection-health.ts index 09560f7eb..c555f4181 100644 --- a/reference-implementation/runtime/connection-health.ts +++ b/reference-implementation/runtime/connection-health.ts @@ -2601,6 +2601,13 @@ function credentialsValidCondition(input: ComputeConnectionHealthInput): Connect if (credentialAbsent) { return credentialRequiredCondition(); } + // A runtime-verified provider invalidation proof outranks the success + // branch below: a run with no failure reason must not read as "credentials + // accepted" while an unrepaired proof stands. Replay suppression is inside + // `browserSessionRepairAuthorized`. + if (browserSessionRepairAuthorized(input)) { + return browserSessionRequiredCondition(null); + } if (input.run?.latestStatus === "succeeded") { return condition({ message: "The latest successful run proved credentials were accepted.", diff --git a/reference-implementation/runtime/controller.ts b/reference-implementation/runtime/controller.ts index 9cb891996..d36312a2a 100644 --- a/reference-implementation/runtime/controller.ts +++ b/reference-implementation/runtime/controller.ts @@ -64,6 +64,7 @@ import { getDefaultBrowserSurfaceReplacementReceiptStore, } from "../server/stores/browser-surface-replacement-ledger-store.ts"; import { getDefaultConnectorAttentionStore } from "../server/stores/connector-attention-store.ts"; +import { getDefaultProviderInvalidationProofStore } from "../server/stores/provider-invalidation-proof-store.ts"; import { getDefaultConnectorDetailGapStore } from "../server/stores/connector-detail-gap-store.ts"; import { createPostgresConnectorInstanceStore, @@ -85,6 +86,7 @@ import { observeDynamicBrowserSurfaceRuntimeInventory, observeDynamicBrowserSurfaceRuntimeSurfaces, } from "./browser-surface/allocator-observation.ts"; +import { recordSignInProofAfterRun } from "./browser-surface/sign-in-proof-recorder.ts"; import { type BrowserSurfaceManagerDeps, type BrowserSurfaceReadinessProbe, @@ -3381,6 +3383,38 @@ export function createController(opts: ControllerOptions = {}): Controller { } } + // Runtime-side provider-invalidation proof: the run's own browser surface + // is read while the lease is still held. Never lets a recording failure + // change the run outcome. + async function recordSignInProofForRun(input: { + readonly browserSurfaceLease: BrowserSurfaceLease | null; + readonly connectorId: string; + readonly connectorInstanceId: string; + readonly manifest: ConnectorManifest; + readonly runId: string; + readonly status: string | null | undefined; + }): Promise { + try { + const surfaceId = input.browserSurfaceLease?.surface_id; + const cdpUrl = surfaceId ? (browserSurfaceLeaseManager?.getSurface(surfaceId)?.cdp_url ?? null) : null; + await recordSignInProofAfterRun( + { store: getDefaultProviderInvalidationProofStore() }, + { + cdpUrl, + connectionId: input.connectorInstanceId, + connectorId: input.connectorId, + manifest: input.manifest, + nowIso: nowIso(), + runId: input.runId, + status: input.status, + } + ); + } catch (err) { + const message = err instanceof Error ? err.message : String(err); + log.warn?.(`[controller] failed to record sign-in proof for ${input.runId}: ${message}`); + } + } + async function finalizeRunCleanup(input: { readonly browserSurfaceLease: BrowserSurfaceLease | null; readonly connectorId: string; @@ -4499,6 +4533,14 @@ export function createController(opts: ControllerOptions = {}): Controller { // biome-ignore lint/complexity/noExcessiveCognitiveComplexity: This protocol transition owns ordered state invariants that must remain local. .then(async (result) => { runResult = result; + await recordSignInProofForRun({ + browserSurfaceLease, + connectorId, + connectorInstanceId, + manifest, + runId, + status: result?.status, + }); // A managed dynamic surface can pass pre-flight readiness and still // wedge before the connector reaches any record/progress. The // connector-runtime source boundary (browser-launch.ts's diff --git a/reference-implementation/server/backup-table-policy.ts b/reference-implementation/server/backup-table-policy.ts index 39bb7cc12..a7eccd66a 100644 --- a/reference-implementation/server/backup-table-policy.ts +++ b/reference-implementation/server/backup-table-policy.ts @@ -324,6 +324,11 @@ export const BACKUP_TABLE_INVENTORY: Record = classification: "backup_required", reason: "Provider app secrets/config depend on the credential encryption key.", }, + provider_invalidation_proofs: { + classification: "backup_required", + reason: + "Runtime-observed sign-in proofs and their repaired markers; losing the markers would replay an already-handled sign-in prompt.", + }, record_acquisition_provenance: { classification: "backup_required", reason: "Acquisition provenance must match restored records and artifacts.", diff --git a/reference-implementation/server/connector-manifest-validation.ts b/reference-implementation/server/connector-manifest-validation.ts index b2213ea93..0abe054f6 100644 --- a/reference-implementation/server/connector-manifest-validation.ts +++ b/reference-implementation/server/connector-manifest-validation.ts @@ -356,6 +356,13 @@ function validateRuntimeBindings(req: Record, code: string): bo if (reqObj.required !== undefined && typeof reqObj.required !== "boolean") { throw invalidConnectorManifest(`runtime_requirements.bindings.${binding}.required must be a boolean`, code); } + const signInPattern = reqObj.sign_in_url_pattern; + if (signInPattern !== undefined && (typeof signInPattern !== "string" || signInPattern.trim().length === 0)) { + throw invalidConnectorManifest( + `runtime_requirements.bindings.${binding}.sign_in_url_pattern must be a non-empty string`, + code + ); + } } return true; } diff --git a/reference-implementation/server/db.ts b/reference-implementation/server/db.ts index 7bb43d7bd..ea015c404 100644 --- a/reference-implementation/server/db.ts +++ b/reference-implementation/server/db.ts @@ -2518,6 +2518,18 @@ CREATE TABLE IF NOT EXISTS storage_migration_ledger ( completed_at TEXT ); +-- Runtime-observed provider-invalidation proofs: one row per failed run whose +-- last top-level navigation matched the connector's declared sign-in URL +-- pattern. repaired_at is the replay-suppression record for that proof key. +CREATE TABLE IF NOT EXISTS provider_invalidation_proofs ( + connection_id TEXT NOT NULL, + run_id TEXT NOT NULL, + provider TEXT NOT NULL, + observed_at TEXT NOT NULL, + repaired_at TEXT, + PRIMARY KEY (connection_id, run_id) +); + -- Durable scheduling cursors for bounded, periodic maintenance sweeps -- (name-keyed, one row per stage). Not evidence, not owner-visible data. CREATE TABLE IF NOT EXISTS connector_maintenance_cursor ( diff --git a/reference-implementation/server/postgres-storage.ts b/reference-implementation/server/postgres-storage.ts index 05dbe3c00..fc79ab4b9 100644 --- a/reference-implementation/server/postgres-storage.ts +++ b/reference-implementation/server/postgres-storage.ts @@ -3518,6 +3518,17 @@ async function bootstrapPostgresSchemaOnce({ CREATE INDEX IF NOT EXISTS idx_pg_connector_summary_evidence_connector ON connector_summary_evidence(connector_id); + -- Runtime-observed provider-invalidation proofs (see the SQLite DDL in + -- server/db.ts). repaired_at records replay suppression per proof key. + CREATE TABLE IF NOT EXISTS provider_invalidation_proofs ( + connection_id TEXT NOT NULL, + run_id TEXT NOT NULL, + provider TEXT NOT NULL, + observed_at TEXT NOT NULL, + repaired_at TEXT, + PRIMARY KEY (connection_id, run_id) + ); + -- Scheduling state only: name-keyed cursors resume bounded maintenance -- passes after restart without becoming evidence or owner-visible data. CREATE TABLE IF NOT EXISTS connector_maintenance_cursor ( diff --git a/reference-implementation/server/ref-control.ts b/reference-implementation/server/ref-control.ts index e580b7395..381c552de 100644 --- a/reference-implementation/server/ref-control.ts +++ b/reference-implementation/server/ref-control.ts @@ -205,6 +205,7 @@ import { import { getDefaultBrowserSurfaceLeaseStore } from "./stores/browser-surface-lease-store.ts"; import { getDefaultConnectorAttentionStore } from "./stores/connector-attention-store.ts"; import { getDefaultConnectorCoverageHorizonStore } from "./stores/connector-coverage-horizon-store.ts"; +import { getDefaultProviderInvalidationProofStore } from "./stores/provider-invalidation-proof-store.ts"; import { getDefaultConnectorDetailGapStore } from "./stores/connector-detail-gap-store.ts"; import { type CredentialStateChange, @@ -6848,6 +6849,8 @@ interface ConnectorSummarySynthesisInput { * `openspec/changes/add-coverage-horizon-and-actionability-banner/design.md`. */ readonly coverageHorizons: readonly ConnectionCoverageHorizon[]; + /** Durable runtime-observed sign-in proof for this connection, pre-fetched by the caller; `null` when none. */ + readonly browserSurfaceRepair: BrowserSurfaceRepairContext | null; /** * Durable stored-credential presence evidence for this connection, pre- * fetched by the caller (kept out of this function so it stays a pure @@ -7013,6 +7016,7 @@ function synthesizeConnectorSummary(input: ConnectorSummarySynthesisInput): Conn // credential row. authenticates: connectionAuthenticatesToProvider(manifest), browserSessionRepairCapable, + browserSurfaceRepair: input.browserSurfaceRepair, collectionRate: authoritativeCollectionRate, coverageHorizons: input.coverageHorizons, credential, @@ -7717,6 +7721,29 @@ function connectorUsesNekoSurface( .some((entry) => entry === connectorId || (canonicalConnectorKey(entry) ?? entry) === canonicalTarget); } +// Loads the durable runtime-observed sign-in proof for a browser-session +// connection. A store read failure yields no proof (never a fabricated one). +async function loadBrowserSurfaceRepair(input: { + readonly connectorId: string; + readonly connectorInstanceId: string; + readonly instance: ConnectorInstanceRow; + readonly manifest: ConnectorManifest; +}): Promise { + if (!connectionHasBrowserSessionRepairCapability(input.instance, input.manifest)) { + return null; + } + try { + const { proof, repairedProofKeys } = await getDefaultProviderInvalidationProofStore().load( + input.connectorInstanceId + ); + return proof + ? { connectionId: input.connectorInstanceId, evidence: proof, provider: input.connectorId, repairedProofKeys } + : null; + } catch { + return null; + } +} + function connectionHasBrowserSessionRepairCapability( instance: ConnectorInstanceRow, manifest: ConnectorManifest @@ -7963,6 +7990,7 @@ async function projectConnectorSummaryForInstance( acquisitionCoverage, credentialMetadata, coverageHorizons, + browserSurfaceRepair, ] = await Promise.all([ schedulePromise, hydrateRunSummaries ? getLatestRunSummaryForConnectionId(connectorInstanceId) : Promise.resolve(null), @@ -8025,6 +8053,7 @@ async function projectConnectorSummaryForInstance( : getDefaultConnectorCoverageHorizonStore() .getCurrentCoverageHorizons(connectorInstanceId) .catch(() => []), + loadBrowserSurfaceRepair({ connectorId, connectorInstanceId, instance, manifest }), ]); // Connections that are not static-secret-bound (browser-session connections, // or non-static-secret connectors): the ternary above resolved @@ -8073,6 +8102,7 @@ async function projectConnectorSummaryForInstance( collectionRate, connectorId, connectorImplementation, + browserSurfaceRepair, connectorInstanceId, coverageHorizons, credential, diff --git a/reference-implementation/server/stores/provider-invalidation-proof-store.ts b/reference-implementation/server/stores/provider-invalidation-proof-store.ts new file mode 100644 index 000000000..e675c7ae0 --- /dev/null +++ b/reference-implementation/server/stores/provider-invalidation-proof-store.ts @@ -0,0 +1,144 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +/** Durable per-connection provider-invalidation proofs and their repaired markers. */ + +import { + execDynamicSqlAcknowledged, + iterateDynamicSqlAcknowledged, +} from "../../lib/db.ts"; +import type { ProviderInvalidationProof } from "../../runtime/browser-surface/repair-decision.ts"; +import { + getStorageBackendKind, + isPostgresStorageBackend, + postgresQuery, +} from "../postgres-storage.ts"; + +export interface LoadedProviderInvalidationProofs { + /** The newest proof for the connection, or null. */ + readonly proof: ProviderInvalidationProof | null; + /** `connection\nprovider\nevidence_id` keys already marked repaired. */ + readonly repairedProofKeys: readonly string[]; +} + +export interface ProviderInvalidationProofStore { + load: (connectionId: string) => Promise; + /** Marks every unrepaired proof for the connection repaired (replay suppression). */ + markRepaired: (connectionId: string, repairedAt: string) => Promise; + record: (proof: ProviderInvalidationProof) => Promise; +} + +interface ProofRow { + readonly connection_id: string; + readonly observed_at: string; + readonly provider: string; + readonly repaired_at: string | null; + readonly run_id: string; +} + +function toProof(row: ProofRow): ProviderInvalidationProof { + return { + connection_id: row.connection_id, + evidence_id: row.run_id, + kind: "provider_invalidation_proof", + observed_at: row.observed_at, + provider: row.provider, + verified: true, + }; +} + +function toLoaded(rows: readonly ProofRow[]): LoadedProviderInvalidationProofs { + const newest = [...rows].sort( + (a, b) => Date.parse(b.observed_at) - Date.parse(a.observed_at), + )[0]; + return { + proof: newest ? toProof(newest) : null, + repairedProofKeys: rows + .filter((row) => row.repaired_at !== null) + .map((row) => `${row.connection_id}\n${row.provider}\n${row.run_id}`), + }; +} + +const SELECT_COLUMNS = + "connection_id, run_id, provider, observed_at, repaired_at"; + +function createSqliteStore(): ProviderInvalidationProofStore { + return { + load(connectionId) { + // REVIEWED-DYNAMIC: fixed, parameterised lookup keyed by the connection id. + const rows = [ + ...iterateDynamicSqlAcknowledged( + `SELECT ${SELECT_COLUMNS} FROM provider_invalidation_proofs WHERE connection_id = ?`, + [connectionId], + ), + ]; + return Promise.resolve(toLoaded(rows)); + }, + markRepaired(connectionId, repairedAt) { + // REVIEWED-DYNAMIC: fixed, parameterised update keyed by the connection id. + execDynamicSqlAcknowledged( + "UPDATE provider_invalidation_proofs SET repaired_at = ? WHERE connection_id = ? AND repaired_at IS NULL", + [repairedAt, connectionId], + ); + return Promise.resolve(); + }, + record(proof) { + // REVIEWED-DYNAMIC: fixed, parameterised insert; replays of one run id are ignored. + execDynamicSqlAcknowledged( + `INSERT INTO provider_invalidation_proofs (connection_id, run_id, provider, observed_at) + VALUES (?, ?, ?, ?) ON CONFLICT(connection_id, run_id) DO NOTHING`, + [ + proof.connection_id, + proof.evidence_id, + proof.provider, + proof.observed_at, + ], + ); + return Promise.resolve(); + }, + }; +} + +function createPostgresStore(): ProviderInvalidationProofStore { + return { + async load(connectionId) { + const result = await postgresQuery( + `SELECT ${SELECT_COLUMNS} FROM provider_invalidation_proofs WHERE connection_id = $1`, + [connectionId], + ); + return toLoaded(result.rows); + }, + async markRepaired(connectionId, repairedAt) { + await postgresQuery( + "UPDATE provider_invalidation_proofs SET repaired_at = $2 WHERE connection_id = $1 AND repaired_at IS NULL", + [connectionId, repairedAt], + ); + }, + async record(proof) { + await postgresQuery( + `INSERT INTO provider_invalidation_proofs (connection_id, run_id, provider, observed_at) + VALUES ($1, $2, $3, $4) ON CONFLICT (connection_id, run_id) DO NOTHING`, + [ + proof.connection_id, + proof.evidence_id, + proof.provider, + proof.observed_at, + ], + ); + }, + }; +} + +let defaultStore: ProviderInvalidationProofStore | null = null; +let defaultBackend: string | null = null; + +export function getDefaultProviderInvalidationProofStore(): ProviderInvalidationProofStore { + const backend = getStorageBackendKind(); + if (!defaultStore || defaultBackend !== backend) { + defaultStore = isPostgresStorageBackend() + ? createPostgresStore() + : createSqliteStore(); + defaultBackend = backend; + } + return defaultStore; +} diff --git a/reference-implementation/test/connection-health.test.ts b/reference-implementation/test/connection-health.test.ts index 48c36aace..d575e7e6e 100644 --- a/reference-implementation/test/connection-health.test.ts +++ b/reference-implementation/test/connection-health.test.ts @@ -622,6 +622,38 @@ test("surface: an exact verified provider proof remains a monotonic one-repair a } }); +test("surface: a verified proof is evaluated before the credentials-accepted branch when the run has no failure reason", () => { + const proof = providerInvalidationProof(); + const context = { connectionId: proof.connection_id, evidence: proof, provider: proof.provider }; + for (const latestStatus of ["failed", "succeeded"] as const) { + const proven = computeConnectionHealth( + managedBrowserSessionInput({ + browserSurfaceRepair: context, + run: run({ latestStatus, reasonCode: null }), + }) + ); + assert.equal( + findCondition(proven, "CredentialsValid")?.remediation?.surface?.kind, + "browser_session", + `a verified proof must prompt for sign-in on a ${latestStatus} run with no reason` + ); + } + const withoutProof = computeConnectionHealth( + managedBrowserSessionInput({ run: run({ latestStatus: "succeeded", reasonCode: null }) }) + ); + assert.equal(findCondition(withoutProof, "CredentialsValid")?.remediation, null); + const repaired = computeConnectionHealth( + managedBrowserSessionInput({ + browserSurfaceRepair: { + ...context, + repairedProofKeys: [`${proof.connection_id}\n${proof.provider}\n${proof.evidence_id}`], + }, + run: run({ latestStatus: "succeeded", reasonCode: null }), + }) + ); + assert.equal(findCondition(repaired, "CredentialsValid")?.remediation, null); +}); + test("surface: structured attention remains the owner-action authority", () => { const snap = computeConnectionHealth( managedBrowserSessionInput({ diff --git a/reference-implementation/test/ref-connectors-provider-invalidation-proof.test.ts b/reference-implementation/test/ref-connectors-provider-invalidation-proof.test.ts new file mode 100644 index 000000000..c78ab8622 --- /dev/null +++ b/reference-implementation/test/ref-connectors-provider-invalidation-proof.test.ts @@ -0,0 +1,235 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +import assert from "node:assert/strict"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import test from "node:test"; +import { recordSignInProofAfterRun } from "../runtime/browser-surface/sign-in-proof-recorder.ts"; +import { closeDb, getDb, initDb } from "../server/db.ts"; +import { + getConnectorSummaryForRoute, + invalidateConnectorSummariesCache, +} from "../server/ref-control.ts"; +import { createSqliteConnectorInstanceStore } from "../server/stores/connector-instance-store.ts"; +import { getDefaultProviderInvalidationProofStore } from "../server/stores/provider-invalidation-proof-store.ts"; +import { createSqliteSchedulerStore } from "../server/stores/scheduler-store.ts"; + +const CONNECTOR_ID = "provider_invalidation_browser_connector"; +const INSTANCE_ID = "cin_test_provider_invalidation"; +const NOW = "2026-05-20T12:00:00.000Z"; +const CDP_URL = "http://127.0.0.1:9222"; +const SIGN_IN_PATTERN = "https://accounts.example.test/signin*"; +const SIGN_IN_URL = "https://accounts.example.test/signin?continue=app"; +const APP_URL = "https://app.example.test/home"; + +function withTmpDb(fn: () => Promise): () => Promise { + return async () => { + const dir = mkdtempSync(join(tmpdir(), "pdpp-provider-invalidation-")); + initDb(join(dir, "pdpp.sqlite")); + try { + await fn(); + } finally { + closeDb(); + rmSync(dir, { force: true, recursive: true }); + } + }; +} + +function manifestWith(pattern: string | null) { + return { + capabilities: { public_listing: { tier: "supported" } }, + connector_id: CONNECTOR_ID, + display_name: "Provider invalidation browser connector", + protocol_version: "0.1.0", + runtime_requirements: { + bindings: { + browser: { + required: true, + ...(pattern ? { sign_in_url_pattern: pattern } : {}), + }, + network: { required: true }, + }, + }, + streams: [{ name: "messages", primary_key: ["id"] }], + version: "1.0.0", + }; +} + +async function seedConnection(pattern: string | null): Promise { + getDb() + .prepare( + "INSERT INTO connectors(connector_id, manifest, created_at) VALUES (?, ?, ?)", + ) + .run(CONNECTOR_ID, JSON.stringify(manifestWith(pattern)), NOW); + await createSqliteConnectorInstanceStore().upsert({ + connectorId: CONNECTOR_ID, + connectorInstanceId: INSTANCE_ID, + createdAt: NOW, + displayName: "Browser connection", + ownerSubjectId: "owner_local", + sourceBinding: { kind: "account" }, + sourceBindingKey: "account-browser", + sourceKind: "account", + status: "active", + updatedAt: NOW, + }); +} + +/** A run that kept going and reported per-item 401 detail gaps: the run reason is specific and non-credential, and the 401 is only connector message text. */ +async function appendRun( + status: "failed" | "succeeded", + runId: string, +): Promise { + const scheduler = createSqliteSchedulerStore(); + await Promise.resolve( + scheduler.appendRunHistory({ + attempt: 1, + checkpointSummary: null, + completedAt: NOW, + connectorError: null, + connectorId: CONNECTOR_ID, + connectorInstanceId: INSTANCE_ID, + failureReason: status === "failed" ? "collection_incomplete" : null, + knownGaps: + status === "succeeded" + ? [] + : [ + { + kind: "detail_unavailable", + message: "GET /item/1 returned 401", + reason: "detail_fetch_failed", + severity: "actionable", + stream: "messages", + }, + ], + recordsEmitted: 1, + reportedRecordsEmitted: 1, + runId, + source: { id: CONNECTOR_ID, kind: "connector" }, + startedAt: NOW, + status, + terminalReason: null, + traceId: `trc_${runId}`, + }), + ); + await Promise.resolve( + createSqliteSchedulerStore().upsertLastRunTime( + INSTANCE_ID, + Date.parse(NOW), + NOW, + CONNECTOR_ID, + ), + ); +} + +function fetchReturningPageUrl(url: string): typeof fetch { + return (() => + Promise.resolve( + new Response(JSON.stringify([{ type: "page", url }]), { + headers: { "content-type": "application/json" }, + }), + )) as typeof fetch; +} + +function writer( + url: string, + pattern: string | null, + status: "failed" | "succeeded", + runId: string, +) { + return recordSignInProofAfterRun( + { + fetchImpl: fetchReturningPageUrl(url), + store: getDefaultProviderInvalidationProofStore(), + }, + { + cdpUrl: CDP_URL, + connectionId: INSTANCE_ID, + connectorId: CONNECTOR_ID, + manifest: manifestWith(pattern), + nowIso: NOW, + runId, + status, + }, + ); +} + +async function reauthSurface(): Promise { + invalidateConnectorSummariesCache(); + const summary = await getConnectorSummaryForRoute(INSTANCE_ID); + assert.ok(summary, "the browser-session connection projects a summary"); + return summary.rendered_verdict.required_actions.find( + (action) => action.kind === "reauth", + )?.surface?.kind; +} + +test( + "(a) a failed run whose last navigation matched the sign-in pattern prompts a browser_session reauth", + withTmpDb(async () => { + await seedConnection(SIGN_IN_PATTERN); + await appendRun("failed", "run_a"); + assert.equal( + await writer(SIGN_IN_URL, SIGN_IN_PATTERN, "failed", "run_a"), + "recorded", + ); + assert.equal(await reauthSurface(), "browser_session"); + }), +); + +test( + "(b) a failed run whose last navigation did not match the pattern gets no reauth", + withTmpDb(async () => { + await seedConnection(SIGN_IN_PATTERN); + await appendRun("failed", "run_b"); + assert.equal( + await writer(APP_URL, SIGN_IN_PATTERN, "failed", "run_b"), + "none", + ); + assert.equal(await reauthSurface(), undefined); + }), +); + +test( + "(c) the sign-in page on a healthy run records no proof and prompts nothing", + withTmpDb(async () => { + await seedConnection(SIGN_IN_PATTERN); + await appendRun("succeeded", "run_c"); + assert.equal( + await writer(SIGN_IN_URL, SIGN_IN_PATTERN, "succeeded", "run_c"), + "repaired", + ); + assert.equal( + (await getDefaultProviderInvalidationProofStore().load(INSTANCE_ID)) + .proof, + null, + ); + assert.equal(await reauthSurface(), undefined); + }), +); + +test( + "(d) a proof already marked repaired is suppressed", + withTmpDb(async () => { + await seedConnection(SIGN_IN_PATTERN); + await appendRun("failed", "run_d"); + await writer(SIGN_IN_URL, SIGN_IN_PATTERN, "failed", "run_d"); + assert.equal(await reauthSurface(), "browser_session"); + await getDefaultProviderInvalidationProofStore().markRepaired( + INSTANCE_ID, + NOW, + ); + assert.equal(await reauthSurface(), undefined); + }), +); + +test( + "(e) a connector that declares no pattern behaves as before", + withTmpDb(async () => { + await seedConnection(null); + await appendRun("failed", "run_e"); + assert.equal(await writer(SIGN_IN_URL, null, "failed", "run_e"), "none"); + assert.equal(await reauthSurface(), undefined); + }), +); diff --git a/reference-implementation/test/sign-in-proof.test.ts b/reference-implementation/test/sign-in-proof.test.ts new file mode 100644 index 000000000..c2325f176 --- /dev/null +++ b/reference-implementation/test/sign-in-proof.test.ts @@ -0,0 +1,109 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +import assert from "node:assert/strict"; +import test from "node:test"; +import { + deriveSignInProof, + observeLastTopLevelUrl, + signInUrlMatchesPattern, + signInUrlPatternFromManifest, +} from "../runtime/browser-surface/sign-in-proof.ts"; +import { validateRuntimeRequirements } from "../server/connector-manifest-validation.ts"; + +const PATTERN = "https://accounts.example.test/signin*"; +const base = { + connectionId: "c1", + lastTopLevelUrl: "https://accounts.example.test/signin?x=1", + observedAt: "2026-05-20T12:00:00.000Z", + pattern: PATTERN, + provider: "p", + runFailed: true, + runId: "r1", +}; + +test("a proof needs a failed run and a last top-level URL that matches the declared pattern", () => { + assert.equal(deriveSignInProof(base)?.evidence_id, "r1"); + assert.equal(deriveSignInProof({ ...base, runFailed: false }), null); + assert.equal( + deriveSignInProof({ + ...base, + lastTopLevelUrl: "https://app.example.test/home", + }), + null, + ); + assert.equal(deriveSignInProof({ ...base, lastTopLevelUrl: null }), null); + assert.equal(deriveSignInProof({ ...base, pattern: null }), null); +}); + +test("the pattern is an anchored glob with literal dots", () => { + assert.equal( + signInUrlMatchesPattern(PATTERN, "https://accounts.example.test/signin"), + true, + ); + assert.equal( + signInUrlMatchesPattern(PATTERN, "https://accountsXexample.test/signin"), + false, + ); + assert.equal( + signInUrlMatchesPattern( + PATTERN, + "https://evil.test/?https://accounts.example.test/signin", + ), + false, + ); +}); + +test("the manifest pattern is read from the browser binding only", () => { + const manifest = { + runtime_requirements: { + bindings: { browser: { required: true, sign_in_url_pattern: PATTERN } }, + }, + }; + assert.equal(signInUrlPatternFromManifest(manifest), PATTERN); + assert.equal( + signInUrlPatternFromManifest({ + runtime_requirements: { bindings: { browser: { required: true } } }, + }), + null, + ); +}); + +test("manifest validation rejects a non-string sign_in_url_pattern", () => { + assert.throws(() => + validateRuntimeRequirements( + { + runtime_requirements: { + bindings: { browser: { required: true, sign_in_url_pattern: 5 } }, + }, + }, + "invalid_connector_manifest", + ), + ); + assert.doesNotThrow(() => + validateRuntimeRequirements( + { + runtime_requirements: { + bindings: { + browser: { required: true, sign_in_url_pattern: PATTERN }, + }, + }, + }, + "invalid_connector_manifest", + ), + ); +}); + +test("the surface observation reads the first http(s) page target and nothing else", async () => { + const targets = [ + { type: "service_worker", url: "https://a.test/sw.js" }, + { type: "page", url: "about:blank" }, + { type: "page", url: "https://accounts.example.test/signin" }, + ]; + const fetchImpl = (() => + Promise.resolve(Response.json(targets))) as typeof fetch; + assert.equal( + await observeLastTopLevelUrl("http://127.0.0.1:9222", fetchImpl), + "https://accounts.example.test/signin", + ); +}); From 362f7b92381321e50ec61d6681a401fdac275be3 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 14:51:46 -0600 Subject: [PATCH 2/5] fix(health): fence and harden the sign-in proof writer - Match the sign-in pattern with a segment scan instead of a regular expression, so a hostile pattern cannot backtrack on the controller loop. - Fence the run-completion writer on the run's slot ownership and generation, so a late result from a reclaimed run cannot mark a newer run's proof repaired or record another run's page. - Capture failure evidence on the watchdog and rejection paths before the lease is released. - Return a page URL only when exactly one page target qualifies; several targets make the last navigation unknowable, so no proof is recorded. - Cascade proof rows when the connection is deleted, on both backends. - Reject sign_in_url_pattern values over the runtime limit, on the browser binding only. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../runtime/browser-surface/sign-in-proof.ts | 83 ++++-- .../runtime/controller.ts | 59 ++++- .../server/connector-manifest-validation.ts | 12 +- reference-implementation/server/db.ts | 3 +- .../server/postgres-storage.ts | 2 +- .../controller-sign-in-proof-fencing.test.ts | 242 ++++++++++++++++++ ...-invalidation-proof-store-postgres.test.ts | 70 +++++ ...ectors-provider-invalidation-proof.test.ts | 23 ++ .../test/sign-in-proof.test.ts | 76 ++++++ 9 files changed, 534 insertions(+), 36 deletions(-) create mode 100644 reference-implementation/test/controller-sign-in-proof-fencing.test.ts create mode 100644 reference-implementation/test/provider-invalidation-proof-store-postgres.test.ts diff --git a/reference-implementation/runtime/browser-surface/sign-in-proof.ts b/reference-implementation/runtime/browser-surface/sign-in-proof.ts index 46a6d2c55..ef1a18c9a 100644 --- a/reference-implementation/runtime/browser-surface/sign-in-proof.ts +++ b/reference-implementation/runtime/browser-surface/sign-in-proof.ts @@ -13,7 +13,7 @@ import type { ProviderInvalidationProof } from "./repair-decision.ts"; * message text. */ -const MAX_PATTERN_LENGTH = 512; +export const MAX_SIGN_IN_PATTERN_LENGTH = 512; const MAX_URL_LENGTH = 2048; /** Declared at `runtime_requirements.bindings.browser.sign_in_url_pattern`. */ @@ -32,29 +32,58 @@ export function signInUrlPatternFromManifest(manifest: { } const pattern = (browser as { readonly sign_in_url_pattern?: unknown }) .sign_in_url_pattern; - return typeof pattern === "string" && - pattern.trim().length > 0 && - pattern.length <= MAX_PATTERN_LENGTH - ? pattern.trim() + const trimmed = typeof pattern === "string" ? pattern.trim() : ""; + return trimmed.length > 0 && trimmed.length <= MAX_SIGN_IN_PATTERN_LENGTH + ? trimmed : null; } -/** Anchored glob over the whole URL; `*` matches any run of characters, everything else is literal. */ +/** + * Anchored glob over the whole URL; `*` matches any run of characters and + * everything else is literal. Segments are matched left to right with + * `indexOf`, never a regular expression, so a hostile pattern cannot trigger + * backtracking; both inputs are length-capped, which bounds the work. + */ export function signInUrlMatchesPattern(pattern: string, url: string): boolean { - if (pattern.length > MAX_PATTERN_LENGTH || url.length > MAX_URL_LENGTH) { + if ( + pattern.length > MAX_SIGN_IN_PATTERN_LENGTH || + url.length > MAX_URL_LENGTH + ) { + return false; + } + const parts = pattern.split("*"); + if (parts.length === 1) { + return pattern === url; + } + const first = parts[0] ?? ""; + const last = parts.at(-1) ?? ""; + if ( + !(url.startsWith(first) && url.endsWith(last)) || + first.length + last.length > url.length + ) { return false; } - const source = pattern - .split("*") - .map((part) => part.replace(/[.+?^${}()|[\]\\]/g, "\\$&")) - .join(".*"); - return new RegExp(`^${source}$`).test(url); + const end = url.length - last.length; + let position = first.length; + for (const part of parts.slice(1, -1)) { + if (part === "") { + continue; + } + const found = url.indexOf(part, position); + if (found < 0 || found + part.length > end) { + return false; + } + position = found + part.length; + } + return true; } /** - * Reads the most recent top-level page URL from the run's own browser surface - * (DevTools target list on the surface the runtime leased). It sends nothing - * to the provider. + * Reads the run's top-level page URL from its own browser surface (DevTools + * target list). It sends nothing to the provider. The list does not carry + * navigation order or run ownership, so it fails closed: the URL is returned + * only when exactly one page target has an http(s) URL. Several candidates + * (another tab, a popup) make "the last navigation" unknowable, so no proof. */ export async function observeLastTopLevelUrl( cdpUrl: string, @@ -70,19 +99,17 @@ export async function observeLastTopLevelUrl( if (!Array.isArray(targets)) { return null; } - for (const target of targets as readonly { - readonly type?: unknown; - readonly url?: unknown; - }[]) { - if ( - target?.type === "page" && - typeof target.url === "string" && - /^https?:/i.test(target.url) - ) { - return target.url; - } - } - return null; + const candidates = ( + targets as readonly { readonly type?: unknown; readonly url?: unknown }[] + ) + .filter( + (target) => + target?.type === "page" && + typeof target.url === "string" && + /^https?:/i.test(target.url), + ) + .map((target) => target.url as string); + return candidates.length === 1 ? (candidates[0] ?? null) : null; } export interface SignInProofInput { diff --git a/reference-implementation/runtime/controller.ts b/reference-implementation/runtime/controller.ts index d36312a2a..345a5705a 100644 --- a/reference-implementation/runtime/controller.ts +++ b/reference-implementation/runtime/controller.ts @@ -554,6 +554,11 @@ export interface ControllerOptions { }) => Promise | void; /** Awaited before a managed surface lease becomes reusable after run cleanup. */ beforeBrowserSurfaceLeaseRelease?: (args: { readonly runId: string }) => Promise | void; + /** Run-completion sign-in proof writer. Defaults to the durable store-backed recorder; tests may observe it. */ + recordSignInProof?: ( + deps: Parameters[0], + input: Parameters[1] + ) => ReturnType; /** * Resolves the host allocator's recorded start failure (code + message) for a * run whose browser-surface lease just went surface_failed. Wired to the host @@ -3386,18 +3391,28 @@ export function createController(opts: ControllerOptions = {}): Controller { // Runtime-side provider-invalidation proof: the run's own browser surface // is read while the lease is still held. Never lets a recording failure // change the run outcome. + // + // Fenced on run identity: only the run that still owns the connection slot, + // at the generation it was admitted under, may write. A late result from a + // run the watchdog already reclaimed must neither clear a newer run's proof + // nor attribute another run's page to itself. async function recordSignInProofForRun(input: { readonly browserSurfaceLease: BrowserSurfaceLease | null; readonly connectorId: string; readonly connectorInstanceId: string; + readonly generation: number; + readonly key: string; readonly manifest: ConnectorManifest; readonly runId: string; readonly status: string | null | undefined; }): Promise { + if (activeRuns.get(input.key)?.run_id !== input.runId || runGenerations.get(input.key) !== input.generation) { + return; + } try { const surfaceId = input.browserSurfaceLease?.surface_id; const cdpUrl = surfaceId ? (browserSurfaceLeaseManager?.getSurface(surfaceId)?.cdp_url ?? null) : null; - await recordSignInProofAfterRun( + await (opts.recordSignInProof ?? recordSignInProofAfterRun)( { store: getDefaultProviderInvalidationProofStore() }, { cdpUrl, @@ -4176,14 +4191,17 @@ export function createController(opts: ControllerOptions = {}): Controller { readonly browserSurfaceLease: BrowserSurfaceLease | null; readonly connectorId: string; readonly connectorInstanceId: string; + readonly generation: number; readonly key: string; + readonly manifest: ConnectorManifest; readonly runId: string; readonly traceContext: SpineTraceContext; }): void { if (!Number.isFinite(maxRunWallClockMs) || maxRunWallClockMs <= 0) { return; } - const { browserSurfaceLease, connectorId, connectorInstanceId, key, runId, traceContext } = input; + const { browserSurfaceLease, connectorId, connectorInstanceId, generation, key, manifest, runId, traceContext } = + input; let resolveSettlement!: () => void; const settlementPromise = new Promise((resolve) => { resolveSettlement = resolve; @@ -4233,6 +4251,17 @@ export function createController(opts: ControllerOptions = {}): Controller { const emitMessage = emitErr instanceof Error ? emitErr.message : String(emitErr); log.warn?.(`[controller] watchdog: failed to emit run_timed_out terminal for ${runId}: ${emitMessage}`); } + // Capture terminal evidence while the lease is still held. + await recordSignInProofForRun({ + browserSurfaceLease, + connectorId, + connectorInstanceId, + generation, + key, + manifest, + runId, + status: "failed", + }); await finalizeRunCleanup({ browserSurfaceLease, connectorId, connectorInstanceId, key, runId, traceContext }); }; emitAndFinalize() @@ -4537,6 +4566,8 @@ export function createController(opts: ControllerOptions = {}): Controller { browserSurfaceLease, connectorId, connectorInstanceId, + generation: myRunGeneration, + key, manifest, runId, status: result?.status, @@ -4613,6 +4644,19 @@ export function createController(opts: ControllerOptions = {}): Controller { ); return; } + // The run rejected before a result existed: capture failure evidence + // while the lease is still held (the fence above and in the recorder + // keep a reclaimed run out). + await recordSignInProofForRun({ + browserSurfaceLease, + connectorId, + connectorInstanceId, + generation: myRunGeneration, + key, + manifest, + runId, + status: "failed", + }); // Close the phantom-202 window: a throw before the runtime's // `run.started` emit (env/spawn prep) used to leave a 202-returned // run id with ZERO spine events — log-and-forget. Emit a typed @@ -4704,7 +4748,16 @@ export function createController(opts: ControllerOptions = {}): Controller { // in activeRuns that blocks all future run-nows with 409 until restart. // armRunWatchdog bounds this: it force-finalizes the run after the budget // expires and is a no-op when maxRunWallClockMs is Infinity or zero. - armRunWatchdog({ browserSurfaceLease, connectorId, connectorInstanceId, key, runId, traceContext }); + armRunWatchdog({ + browserSurfaceLease, + connectorId, + connectorInstanceId, + generation: myRunGeneration, + key, + manifest, + runId, + traceContext, + }); return { run_id: runId, status: "started", trace_id: traceContext.trace_id, ...automationMetadata }; } diff --git a/reference-implementation/server/connector-manifest-validation.ts b/reference-implementation/server/connector-manifest-validation.ts index 0abe054f6..00f1dd78c 100644 --- a/reference-implementation/server/connector-manifest-validation.ts +++ b/reference-implementation/server/connector-manifest-validation.ts @@ -11,6 +11,7 @@ */ import { collectionProfileDeclarationError } from "@pdpp/connector-protocol"; +import { MAX_SIGN_IN_PATTERN_LENGTH } from "../runtime/browser-surface/sign-in-proof.ts"; import { tuningEnvironmentKeyOwner } from "../runtime/connector-child-environment.ts"; import { normalizeStaticSecretCredentialCapture, @@ -356,10 +357,15 @@ function validateRuntimeBindings(req: Record, code: string): bo if (reqObj.required !== undefined && typeof reqObj.required !== "boolean") { throw invalidConnectorManifest(`runtime_requirements.bindings.${binding}.required must be a boolean`, code); } - const signInPattern = reqObj.sign_in_url_pattern; - if (signInPattern !== undefined && (typeof signInPattern !== "string" || signInPattern.trim().length === 0)) { + const signInPattern = binding === "browser" ? reqObj.sign_in_url_pattern : undefined; + if ( + signInPattern !== undefined && + (typeof signInPattern !== "string" || + signInPattern.trim().length === 0 || + signInPattern.trim().length > MAX_SIGN_IN_PATTERN_LENGTH) + ) { throw invalidConnectorManifest( - `runtime_requirements.bindings.${binding}.sign_in_url_pattern must be a non-empty string`, + `runtime_requirements.bindings.browser.sign_in_url_pattern must be a non-empty string of at most ${MAX_SIGN_IN_PATTERN_LENGTH} characters`, code ); } diff --git a/reference-implementation/server/db.ts b/reference-implementation/server/db.ts index ea015c404..c2beef896 100644 --- a/reference-implementation/server/db.ts +++ b/reference-implementation/server/db.ts @@ -2527,7 +2527,8 @@ CREATE TABLE IF NOT EXISTS provider_invalidation_proofs ( provider TEXT NOT NULL, observed_at TEXT NOT NULL, repaired_at TEXT, - PRIMARY KEY (connection_id, run_id) + PRIMARY KEY (connection_id, run_id), + FOREIGN KEY(connection_id) REFERENCES connector_instances(connector_instance_id) ON DELETE CASCADE ); -- Durable scheduling cursors for bounded, periodic maintenance sweeps diff --git a/reference-implementation/server/postgres-storage.ts b/reference-implementation/server/postgres-storage.ts index fc79ab4b9..de791c42c 100644 --- a/reference-implementation/server/postgres-storage.ts +++ b/reference-implementation/server/postgres-storage.ts @@ -3521,7 +3521,7 @@ async function bootstrapPostgresSchemaOnce({ -- Runtime-observed provider-invalidation proofs (see the SQLite DDL in -- server/db.ts). repaired_at records replay suppression per proof key. CREATE TABLE IF NOT EXISTS provider_invalidation_proofs ( - connection_id TEXT NOT NULL, + connection_id TEXT NOT NULL REFERENCES connector_instances(connector_instance_id) ON DELETE CASCADE, run_id TEXT NOT NULL, provider TEXT NOT NULL, observed_at TEXT NOT NULL, diff --git a/reference-implementation/test/controller-sign-in-proof-fencing.test.ts b/reference-implementation/test/controller-sign-in-proof-fencing.test.ts new file mode 100644 index 000000000..14d169410 --- /dev/null +++ b/reference-implementation/test/controller-sign-in-proof-fencing.test.ts @@ -0,0 +1,242 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +/** + * The run-completion sign-in proof writer is fenced on run identity and + * captures terminal failure evidence for the watchdog and rejection paths. + */ + +import assert from "node:assert/strict"; +import type { TestContext } from "node:test"; +import test from "node:test"; + +import { + __resetControllerInteractionStateForTests, + createController, +} from "../runtime/controller.ts"; +import type { RuntimeRunConnectorResult } from "../runtime/index.ts"; +import { closeDb, getDb, initDb } from "../server/db.ts"; +import { createSqliteConnectorInstanceStore } from "../server/stores/connector-instance-store.ts"; +import { getDefaultProviderInvalidationProofStore } from "../server/stores/provider-invalidation-proof-store.ts"; +import type { + ActiveRunRecord, + SchedulerStore, +} from "../server/stores/scheduler-store.ts"; +import { makeTemporaryDbPath } from "./helpers/temp-dir.ts"; + +const CONNECTOR_ID = + "https://registry.pdpp.dev/connectors/sign-in-proof-fence-test"; +const NOW = "2026-05-20T12:00:00.000Z"; +const INSTANCE_ID = "cin_sign_in_proof_fence"; +const UNREACHABLE_BUDGET_MS = 600_000; +const MANIFEST = { + connector_id: CONNECTOR_ID, + name: "Sign-in proof fence test", + runtime_requirements: { + bindings: { + browser: { + required: true, + sign_in_url_pattern: "https://accounts.example.test/signin*", + }, + }, + }, + streams: [], + version: "1.0.0", +}; + +function unimplemented(name: string): never { + throw new Error(`test fake: ${name} is not implemented`); +} + +function admit() { + return ({ + connectorId, + connectorInstanceId, + ownerSubjectId, + }: { + connectorId: string; + connectorInstanceId: string | null; + ownerSubjectId: string | null; + }) => + Promise.resolve({ + connectorId, + connectorInstanceId: connectorInstanceId ?? INSTANCE_ID, + ownerSubjectId: ownerSubjectId || "owner_local", + }); +} + +function schedulerStore(): SchedulerStore { + const active = new Map(); + return { + appendRunHistory: () => undefined, + createSchedule: () => unimplemented("createSchedule"), + deleteActiveRun: (instanceId: string, runId: string) => { + if (active.get(instanceId)?.run_id === runId) { + active.delete(instanceId); + } + }, + deleteSchedule: () => unimplemented("deleteSchedule"), + getActiveRun: (instanceId: string) => active.get(instanceId) ?? null, + getLatestRunHistoryForConnection: () => + unimplemented("getLatestRunHistoryForConnection"), + getSchedule: () => null, + listActiveRuns: () => [...active.values()], + listLastRunTimes: () => [], + listRunHistory: () => [], + listSchedules: () => [], + setScheduleEnabled: () => unimplemented("setScheduleEnabled"), + updateSchedule: () => unimplemented("updateSchedule"), + upsertActiveRun: (record: ActiveRunRecord) => { + active.set(record.connector_instance_id ?? record.connector_id, record); + return true; + }, + upsertLastRunTime: () => undefined, + }; +} + +async function freshDb(t: TestContext): Promise { + closeDb(); + initDb(makeTemporaryDbPath("pdpp-sign-in-fence-")); + __resetControllerInteractionStateForTests(); + getDb() + .prepare( + "INSERT INTO connectors(connector_id, manifest, created_at) VALUES (?, ?, ?)", + ) + .run(CONNECTOR_ID, JSON.stringify(MANIFEST), NOW); + await createSqliteConnectorInstanceStore().upsert({ + connectorId: CONNECTOR_ID, + connectorInstanceId: INSTANCE_ID, + createdAt: NOW, + displayName: "Fence", + ownerSubjectId: "owner_local", + sourceBinding: { kind: "account" }, + sourceBindingKey: "k", + sourceKind: "account", + status: "active", + updatedAt: NOW, + }); + const keepAlive = setInterval(() => undefined, 10); + t.after(() => { + clearInterval(keepAlive); + __resetControllerInteractionStateForTests(); + closeDb(); + }); +} + +interface RecorderCall { + readonly activeRunIdAtCall: string | null; + readonly runId: string; + readonly status: string | null | undefined; +} + +function makeController( + calls: RecorderCall[], + impl: () => Promise, + maxRunWallClockMs: number, + store: SchedulerStore, +) { + const controller = createController({ + admitRunConnection: admit(), + connectorPathResolver: () => "/tmp/connector.js", + logger: { error: () => undefined, warn: () => undefined }, + maxRunWallClockMs, + recordSignInProof: (_deps, input) => { + calls.push({ + activeRunIdAtCall: + controller.getActiveRun(CONNECTOR_ID, { + connectorInstanceId: INSTANCE_ID, + })?.run_id ?? null, + runId: input.runId, + status: input.status, + }); + return Promise.resolve("none"); + }, + runConnectorImpl: impl, + schedulerStore: store, + }); + return controller; +} + +const runOptions = (runId: string) => ({ + connectorInstanceId: INSTANCE_ID, + manifest: MANIFEST, + ownerToken: "owner-token", + runId, +}); + +test("the watchdog captures failure evidence while the run still owns its slot", async (t) => { + await freshDb(t); + const calls: RecorderCall[] = []; + const controller = makeController( + calls, + () => new Promise(() => undefined), + 20, + schedulerStore(), + ); + await controller.runNow(CONNECTOR_ID, runOptions("run_wd")); + await new Promise((resolve) => setTimeout(resolve, 300).unref()); + assert.deepEqual(calls, [ + { activeRunIdAtCall: "run_wd", runId: "run_wd", status: "failed" }, + ]); +}); + +test("a rejected run captures failure evidence while the run still owns its slot", async (t) => { + await freshDb(t); + const calls: RecorderCall[] = []; + const controller = makeController( + calls, + () => Promise.reject(new Error("launch failed")), + UNREACHABLE_BUDGET_MS, + schedulerStore(), + ); + await controller.runNow(CONNECTOR_ID, runOptions("run_rej")); + await controller.drainActiveRuns(1000); + assert.deepEqual(calls, [ + { activeRunIdAtCall: "run_rej", runId: "run_rej", status: "failed" }, + ]); +}); + +test("a late succeeded result from a reclaimed run cannot clear a newer run's proof", async (t) => { + await freshDb(t); + const store = schedulerStore(); + const calls: RecorderCall[] = []; + const releases: ((result: RuntimeRunConnectorResult) => void)[] = []; + const hanging = () => + new Promise((resolve) => { + releases.push(resolve); + }); + const first = makeController(calls, hanging, 20, store); + const second = makeController(calls, hanging, UNREACHABLE_BUDGET_MS, store); + await first.runNow(CONNECTOR_ID, runOptions("run_old")); + await new Promise((resolve) => setTimeout(resolve, 300).unref()); + await second.runNow(CONNECTOR_ID, runOptions("run_new")); + + // A valid, unrepaired proof for the newer run's failure. + await getDefaultProviderInvalidationProofStore().record({ + connection_id: INSTANCE_ID, + evidence_id: "run_new_proof", + kind: "provider_invalidation_proof", + observed_at: "2026-05-20T12:00:00.000Z", + provider: CONNECTOR_ID, + verified: true, + }); + calls.length = 0; + + releases[0]?.({ records_emitted: 0, status: "succeeded" }); + await new Promise((resolve) => setTimeout(resolve, 50).unref()); + + assert.deepEqual(calls, [], "the reclaimed run must not reach the recorder"); + assert.deepEqual( + (await getDefaultProviderInvalidationProofStore().load(INSTANCE_ID)) + .repairedProofKeys, + [], + ); + + releases[1]?.({ records_emitted: 0, status: "succeeded" }); + await second.drainActiveRuns(1000); + assert.deepEqual( + calls.map((call) => [call.runId, call.status]), + [["run_new", "succeeded"]], + "the current run still records its own result", + ); +}); diff --git a/reference-implementation/test/provider-invalidation-proof-store-postgres.test.ts b/reference-implementation/test/provider-invalidation-proof-store-postgres.test.ts new file mode 100644 index 000000000..a4977977c --- /dev/null +++ b/reference-implementation/test/provider-invalidation-proof-store-postgres.test.ts @@ -0,0 +1,70 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +import assert from "node:assert/strict"; +import test from "node:test"; +import { + closePostgresStorage, + initPostgresStorage, + postgresQuery, +} from "../server/postgres-storage.ts"; +import { createPostgresConnectorInstanceStore } from "../server/stores/connector-instance-store.ts"; +import { getDefaultProviderInvalidationProofStore } from "../server/stores/provider-invalidation-proof-store.ts"; +import { dedicatedPostgresTestUrl } from "./helpers/dedicated-postgres-test-url.ts"; + +const POSTGRES_URL = dedicatedPostgresTestUrl( + process.env.PDPP_TEST_POSTGRES_URL, +); +const CONNECTOR_ID = "provider_invalidation_pg_connector"; +const INSTANCE_ID = "cin_test_provider_invalidation_pg"; +const NOW = "2026-05-20T12:00:00.000Z"; + +test("dedicated PostgreSQL: deleting a connection removes its provider invalidation proofs", { + skip: POSTGRES_URL + ? false + : "set PDPP_TEST_POSTGRES_URL to the dedicated loopback listener", +}, async () => { + assert.ok( + POSTGRES_URL, + "dedicated PostgreSQL URL is configured when this test runs", + ); + await initPostgresStorage({ backend: "postgres", databaseUrl: POSTGRES_URL }); + try { + await postgresQuery( + "INSERT INTO connectors(connector_id, manifest) VALUES ($1, '{}'::jsonb) ON CONFLICT DO NOTHING", + [CONNECTOR_ID], + ); + await createPostgresConnectorInstanceStore().upsert({ + connectorId: CONNECTOR_ID, + connectorInstanceId: INSTANCE_ID, + createdAt: NOW, + displayName: "Browser connection", + ownerSubjectId: "owner_local", + sourceBinding: { kind: "account" }, + sourceBindingKey: "account-browser", + sourceKind: "account", + status: "active", + updatedAt: NOW, + }); + const store = getDefaultProviderInvalidationProofStore(); + await store.record({ + connection_id: INSTANCE_ID, + evidence_id: "run_pg_1", + kind: "provider_invalidation_proof", + observed_at: NOW, + provider: CONNECTOR_ID, + verified: true, + }); + assert.ok((await store.load(INSTANCE_ID)).proof); + await postgresQuery( + "DELETE FROM connector_instances WHERE connector_instance_id = $1", + [INSTANCE_ID], + ); + assert.equal((await store.load(INSTANCE_ID)).proof, null); + } finally { + await postgresQuery("DELETE FROM connectors WHERE connector_id = $1", [ + CONNECTOR_ID, + ]); + await closePostgresStorage(); + } +}); diff --git a/reference-implementation/test/ref-connectors-provider-invalidation-proof.test.ts b/reference-implementation/test/ref-connectors-provider-invalidation-proof.test.ts index c78ab8622..1bfd87b84 100644 --- a/reference-implementation/test/ref-connectors-provider-invalidation-proof.test.ts +++ b/reference-implementation/test/ref-connectors-provider-invalidation-proof.test.ts @@ -233,3 +233,26 @@ test( assert.equal(await reauthSurface(), undefined); }), ); + +test( + "deleting the connection removes its proofs", + withTmpDb(async () => { + await seedConnection(SIGN_IN_PATTERN); + await appendRun("failed", "run_del"); + await writer(SIGN_IN_URL, SIGN_IN_PATTERN, "failed", "run_del"); + assert.ok( + (await getDefaultProviderInvalidationProofStore().load(INSTANCE_ID)) + .proof, + ); + getDb() + .prepare( + "DELETE FROM connector_instances WHERE connector_instance_id = ?", + ) + .run(INSTANCE_ID); + assert.equal( + (await getDefaultProviderInvalidationProofStore().load(INSTANCE_ID)) + .proof, + null, + ); + }), +); diff --git a/reference-implementation/test/sign-in-proof.test.ts b/reference-implementation/test/sign-in-proof.test.ts index c2325f176..d78f60474 100644 --- a/reference-implementation/test/sign-in-proof.test.ts +++ b/reference-implementation/test/sign-in-proof.test.ts @@ -5,6 +5,7 @@ import assert from "node:assert/strict"; import test from "node:test"; import { deriveSignInProof, + MAX_SIGN_IN_PATTERN_LENGTH, observeLastTopLevelUrl, signInUrlMatchesPattern, signInUrlPatternFromManifest, @@ -107,3 +108,78 @@ test("the surface observation reads the first http(s) page target and nothing el "https://accounts.example.test/signin", ); }); + +const ADVERSARIAL_BUDGET_MS = 50; + +test("a hostile pattern cannot stall the matcher (no backtracking)", () => { + const pattern = `https://example.test/${"*a".repeat(14)}b`; + const started = performance.now(); + assert.equal( + signInUrlMatchesPattern(pattern, `https://example.test/${"a".repeat(26)}c`), + false, + ); + assert.equal( + signInUrlMatchesPattern(`${"*a".repeat(200)}b`, `${"a".repeat(2000)}c`), + false, + ); + assert.ok( + performance.now() - started < ADVERSARIAL_BUDGET_MS, + "adversarial non-match must stay fast", + ); +}); + +test("glob matching is anchored, ordered and non-overlapping", () => { + assert.equal(signInUrlMatchesPattern("a*b*c", "aXbYc"), true); + assert.equal(signInUrlMatchesPattern("a*b*c", "acb"), false); + assert.equal(signInUrlMatchesPattern("ab*ba", "aba"), false); + assert.equal(signInUrlMatchesPattern("*", "anything"), true); + assert.equal(signInUrlMatchesPattern("exact", "exact"), true); + assert.equal(signInUrlMatchesPattern("exact", "exactly"), false); +}); + +test("an ambiguous target list yields no observation, in either order", async () => { + const signIn = { type: "page", url: "https://accounts.example.test/signin" }; + const app = { type: "page", url: "https://app.example.test/home" }; + for (const targets of [ + [signIn, app], + [app, signIn], + [], + [{ type: "page", url: "about:blank" }], + ]) { + const fetchImpl = (() => + Promise.resolve(Response.json(targets))) as typeof fetch; + assert.equal( + await observeLastTopLevelUrl("http://127.0.0.1:9222", fetchImpl), + null, + ); + } +}); + +test("the declared pattern length is bounded at the runtime limit and only on the browser binding", () => { + const atLimit = "a".repeat(MAX_SIGN_IN_PATTERN_LENGTH); + const check = (bindings: Record) => + validateRuntimeRequirements( + { runtime_requirements: { bindings } }, + "invalid_connector_manifest", + ); + assert.doesNotThrow(() => + check({ browser: { required: true, sign_in_url_pattern: atLimit } }), + ); + assert.throws(() => + check({ browser: { required: true, sign_in_url_pattern: `${atLimit}a` } }), + ); + assert.throws(() => + check({ browser: { required: true, sign_in_url_pattern: " " } }), + ); + assert.doesNotThrow(() => + check({ network: { required: true, sign_in_url_pattern: 5 } }), + ); + assert.equal( + signInUrlPatternFromManifest({ + runtime_requirements: { + bindings: { browser: { sign_in_url_pattern: `${atLimit}a` } }, + }, + }), + null, + ); +}); From f9b2364cfd654bbc3820686256af5858def37bbb Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 14:52:24 -0600 Subject: [PATCH 3/5] test(health): keep the fencing assertion type-safe Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../test/controller-sign-in-proof-fencing.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/reference-implementation/test/controller-sign-in-proof-fencing.test.ts b/reference-implementation/test/controller-sign-in-proof-fencing.test.ts index 14d169410..73d92697e 100644 --- a/reference-implementation/test/controller-sign-in-proof-fencing.test.ts +++ b/reference-implementation/test/controller-sign-in-proof-fencing.test.ts @@ -225,7 +225,7 @@ test("a late succeeded result from a reclaimed run cannot clear a newer run's pr releases[0]?.({ records_emitted: 0, status: "succeeded" }); await new Promise((resolve) => setTimeout(resolve, 50).unref()); - assert.deepEqual(calls, [], "the reclaimed run must not reach the recorder"); + assert.equal(calls.length, 0, "the reclaimed run must not reach the recorder"); assert.deepEqual( (await getDefaultProviderInvalidationProofStore().load(INSTANCE_ID)) .repairedProofKeys, From 168261fde15c3badf5d4ee04d7c2ebf8ee0fe4c6 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 15:41:12 -0600 Subject: [PATCH 4/5] fix(health): re-check run identity before writing a sign-in proof The recorder wrote after an await with no identity re-check, so a reclaimed run's delayed DevTools read could insert an unrepaired proof that outranks the newer run's success. The write now re-checks run, generation and slot after the await. The first terminal path to reach a run owns the capture. The watchdog records under the run's actual terminal status. A success repairs earlier proofs before the manifest-pattern gate. Assisted-by: AI Signed-off-by: Tim Nunamaker --- .../browser-surface/sign-in-proof-recorder.ts | 17 ++- .../runtime/controller.ts | 17 ++- .../controller-sign-in-proof-fencing.test.ts | 102 ++++++++++++++++-- .../test/sign-in-proof-recorder.test.ts | 79 ++++++++++++++ 4 files changed, 197 insertions(+), 18 deletions(-) create mode 100644 reference-implementation/test/sign-in-proof-recorder.test.ts diff --git a/reference-implementation/runtime/browser-surface/sign-in-proof-recorder.ts b/reference-implementation/runtime/browser-surface/sign-in-proof-recorder.ts index afc850a81..c96645bf7 100644 --- a/reference-implementation/runtime/browser-surface/sign-in-proof-recorder.ts +++ b/reference-implementation/runtime/browser-surface/sign-in-proof-recorder.ts @@ -13,6 +13,11 @@ export interface RecordSignInProofInput { readonly cdpUrl: string | null; readonly connectionId: string; readonly connectorId: string; + /** + * Re-checked after the DevTools read and before the write; false means the + * run was superseded while the read was in flight, so nothing is written. + */ + readonly isCurrent?: () => boolean; readonly manifest: Parameters[0]; readonly nowIso: string; readonly runId: string; @@ -35,14 +40,16 @@ export async function recordSignInProofAfterRun( deps: RecordSignInProofDeps, input: RecordSignInProofInput, ): Promise<"recorded" | "repaired" | "none"> { - const pattern = signInUrlPatternFromManifest(input.manifest); - if (!pattern) { - return "none"; - } + // Repair first: a success must clear earlier proofs even when the manifest + // no longer declares a pattern. if (input.status === "succeeded") { await deps.store.markRepaired(input.connectionId, input.nowIso); return "repaired"; } + const pattern = signInUrlPatternFromManifest(input.manifest); + if (!pattern) { + return "none"; + } if (input.status !== "failed" || !input.cdpUrl) { return "none"; } @@ -59,7 +66,7 @@ export async function recordSignInProofAfterRun( runFailed: true, runId: input.runId, }); - if (!proof) { + if (!proof || input.isCurrent?.() === false) { return "none"; } await deps.store.record(proof); diff --git a/reference-implementation/runtime/controller.ts b/reference-implementation/runtime/controller.ts index 345a5705a..fa2277d3b 100644 --- a/reference-implementation/runtime/controller.ts +++ b/reference-implementation/runtime/controller.ts @@ -1145,6 +1145,9 @@ const runWatchdogSettlements = new Map; resolve // Growth is bounded by the number of distinct connector_instance keys ever // admitted, not by run count. const runGenerations = new Map(); +// Runs whose terminal sign-in evidence capture has been claimed. The first +// terminal path to reach a run owns the capture; later paths skip it. +const signInProofClaimedRuns = new Set(); // Keyed by run_id. Interaction broker state is intentionally in-memory: // dashboard-submitted values satisfy the current live run only and are never // persisted to `.env.local`, SQLite config/state, or spine event payloads. @@ -3406,9 +3409,12 @@ export function createController(opts: ControllerOptions = {}): Controller { readonly runId: string; readonly status: string | null | undefined; }): Promise { - if (activeRuns.get(input.key)?.run_id !== input.runId || runGenerations.get(input.key) !== input.generation) { + const stillCurrent = () => + activeRuns.get(input.key)?.run_id === input.runId && runGenerations.get(input.key) === input.generation; + if (!stillCurrent() || signInProofClaimedRuns.has(input.runId)) { return; } + signInProofClaimedRuns.add(input.runId); try { const surfaceId = input.browserSurfaceLease?.surface_id; const cdpUrl = surfaceId ? (browserSurfaceLeaseManager?.getSurface(surfaceId)?.cdp_url ?? null) : null; @@ -3418,6 +3424,7 @@ export function createController(opts: ControllerOptions = {}): Controller { cdpUrl, connectionId: input.connectorInstanceId, connectorId: input.connectorId, + isCurrent: stillCurrent, manifest: input.manifest, nowIso: nowIso(), runId: input.runId, @@ -3438,6 +3445,7 @@ export function createController(opts: ControllerOptions = {}): Controller { readonly runId: string; readonly traceContext: SpineTraceContext; }): Promise { + signInProofClaimedRuns.delete(input.runId); // Idempotency guard: the watchdog and the run's own .finally() can both // call finalizeRunCleanup. Only the first call does real work; subsequent // calls are silent no-ops. We detect "already finalized" by checking @@ -4251,7 +4259,10 @@ export function createController(opts: ControllerOptions = {}): Controller { const emitMessage = emitErr instanceof Error ? emitErr.message : String(emitErr); log.warn?.(`[controller] watchdog: failed to emit run_timed_out terminal for ${runId}: ${emitMessage}`); } - // Capture terminal evidence while the lease is still held. + // Capture terminal evidence while the lease is still held, under the + // run's actual terminal status (a run that already completed must not + // record failure evidence). + const terminalStatus = await getRunTerminalStatus(runId).catch(() => null); await recordSignInProofForRun({ browserSurfaceLease, connectorId, @@ -4260,7 +4271,7 @@ export function createController(opts: ControllerOptions = {}): Controller { key, manifest, runId, - status: "failed", + status: terminalStatus === "completed" ? "succeeded" : (terminalStatus ?? "failed"), }); await finalizeRunCleanup({ browserSurfaceLease, connectorId, connectorInstanceId, key, runId, traceContext }); }; diff --git a/reference-implementation/test/controller-sign-in-proof-fencing.test.ts b/reference-implementation/test/controller-sign-in-proof-fencing.test.ts index 73d92697e..677ee8570 100644 --- a/reference-implementation/test/controller-sign-in-proof-fencing.test.ts +++ b/reference-implementation/test/controller-sign-in-proof-fencing.test.ts @@ -9,7 +9,7 @@ import assert from "node:assert/strict"; import type { TestContext } from "node:test"; import test from "node:test"; - +import { emitSpineEvent } from "../lib/spine.ts"; import { __resetControllerInteractionStateForTests, createController, @@ -124,6 +124,7 @@ async function freshDb(t: TestContext): Promise { } interface RecorderCall { + readonly isCurrent: (() => boolean) | undefined; readonly activeRunIdAtCall: string | null; readonly runId: string; readonly status: string | null | undefined; @@ -134,14 +135,16 @@ function makeController( impl: () => Promise, maxRunWallClockMs: number, store: SchedulerStore, + hold?: () => Promise, ) { const controller = createController({ admitRunConnection: admit(), connectorPathResolver: () => "/tmp/connector.js", logger: { error: () => undefined, warn: () => undefined }, maxRunWallClockMs, - recordSignInProof: (_deps, input) => { + recordSignInProof: async (_deps, input) => { calls.push({ + isCurrent: input.isCurrent, activeRunIdAtCall: controller.getActiveRun(CONNECTOR_ID, { connectorInstanceId: INSTANCE_ID, @@ -149,7 +152,8 @@ function makeController( runId: input.runId, status: input.status, }); - return Promise.resolve("none"); + await hold?.(); + return "none"; }, runConnectorImpl: impl, schedulerStore: store, @@ -175,9 +179,14 @@ test("the watchdog captures failure evidence while the run still owns its slot", ); await controller.runNow(CONNECTOR_ID, runOptions("run_wd")); await new Promise((resolve) => setTimeout(resolve, 300).unref()); - assert.deepEqual(calls, [ - { activeRunIdAtCall: "run_wd", runId: "run_wd", status: "failed" }, - ]); + assert.deepEqual( + calls.map(({ activeRunIdAtCall, runId, status }) => ({ + activeRunIdAtCall, + runId, + status, + })), + [{ activeRunIdAtCall: "run_wd", runId: "run_wd", status: "failed" }], + ); }); test("a rejected run captures failure evidence while the run still owns its slot", async (t) => { @@ -191,9 +200,14 @@ test("a rejected run captures failure evidence while the run still owns its slot ); await controller.runNow(CONNECTOR_ID, runOptions("run_rej")); await controller.drainActiveRuns(1000); - assert.deepEqual(calls, [ - { activeRunIdAtCall: "run_rej", runId: "run_rej", status: "failed" }, - ]); + assert.deepEqual( + calls.map(({ activeRunIdAtCall, runId, status }) => ({ + activeRunIdAtCall, + runId, + status, + })), + [{ activeRunIdAtCall: "run_rej", runId: "run_rej", status: "failed" }], + ); }); test("a late succeeded result from a reclaimed run cannot clear a newer run's proof", async (t) => { @@ -225,7 +239,11 @@ test("a late succeeded result from a reclaimed run cannot clear a newer run's pr releases[0]?.({ records_emitted: 0, status: "succeeded" }); await new Promise((resolve) => setTimeout(resolve, 50).unref()); - assert.equal(calls.length, 0, "the reclaimed run must not reach the recorder"); + assert.equal( + calls.length, + 0, + "the reclaimed run must not reach the recorder", + ); assert.deepEqual( (await getDefaultProviderInvalidationProofStore().load(INSTANCE_ID)) .repairedProofKeys, @@ -240,3 +258,67 @@ test("a late succeeded result from a reclaimed run cannot clear a newer run's pr "the current run still records its own result", ); }); + +test("a run reaching two terminal paths records evidence once, and a held capture sees itself superseded", async (t) => { + await freshDb(t); + const store = schedulerStore(); + const calls: RecorderCall[] = []; + let openGate: () => void = () => undefined; + const gate = new Promise((resolve) => { + openGate = resolve; + }); + let rejectRun: (err: Error) => void = () => undefined; + const first = makeController( + calls, + () => + new Promise((_resolve, reject) => { + rejectRun = reject; + }), + 20, + store, + () => gate, + ); + await first.runNow(CONNECTOR_ID, runOptions("run_a")); + // The watchdog fires and its capture is held open; the run promise then + // rejects, reaching a second terminal path for the same run. + await new Promise((resolve) => setTimeout(resolve, 200).unref()); + rejectRun(new Error("late rejection")); + await new Promise((resolve) => setTimeout(resolve, 100).unref()); + assert.equal(calls.length, 1, "only one terminal path may capture per run"); + openGate(); + await first.drainActiveRuns(1000); + await new Promise((resolve) => setTimeout(resolve, 100).unref()); + const isCurrent = calls[0]?.isCurrent; + assert.ok(isCurrent, "the recorder is given a post-await currency check"); + assert.equal(isCurrent(), false, "a released run is no longer current"); +}); + +test("a watchdog on a run that already completed does not record failure evidence", async (t) => { + await freshDb(t); + const calls: RecorderCall[] = []; + const controller = makeController( + calls, + async () => { + await emitSpineEvent({ + actor_id: CONNECTOR_ID, + actor_type: "runtime", + data: { connector_instance_id: INSTANCE_ID }, + event_type: "run.completed", + object_id: "run_done", + object_type: "run", + run_id: "run_done", + status: "completed", + }); + return new Promise(() => undefined); + }, + 20, + schedulerStore(), + ); + await controller.runNow(CONNECTOR_ID, runOptions("run_done")); + await new Promise((resolve) => setTimeout(resolve, 300).unref()); + assert.equal( + calls.filter((call) => call.status === "failed").length, + 0, + "no failure evidence for a run that already succeeded", + ); +}); diff --git a/reference-implementation/test/sign-in-proof-recorder.test.ts b/reference-implementation/test/sign-in-proof-recorder.test.ts new file mode 100644 index 000000000..6b566fff6 --- /dev/null +++ b/reference-implementation/test/sign-in-proof-recorder.test.ts @@ -0,0 +1,79 @@ +// Copyright The PDP-Connect Contributors +// SPDX-License-Identifier: Apache-2.0 + +import assert from "node:assert/strict"; +import test from "node:test"; + +import type { ProviderInvalidationProof } from "../runtime/browser-surface/repair-decision.ts"; +import { recordSignInProofAfterRun } from "../runtime/browser-surface/sign-in-proof-recorder.ts"; +import type { ProviderInvalidationProofStore } from "../server/stores/provider-invalidation-proof-store.ts"; + +const WITH_PATTERN = { + runtime_requirements: { + bindings: { + browser: { sign_in_url_pattern: "https://accounts.example.test/signin*" }, + }, + }, +}; +const WITHOUT_PATTERN = { runtime_requirements: { bindings: { browser: {} } } }; + +function fakeStore() { + const recorded: ProviderInvalidationProof[] = []; + const repaired: string[] = []; + const store = { + load: () => Promise.reject(new Error("unused")), + markRepaired: (connectionId: string) => { + repaired.push(connectionId); + return Promise.resolve(); + }, + record: (proof: ProviderInvalidationProof) => { + recorded.push(proof); + return Promise.resolve(); + }, + } as unknown as ProviderInvalidationProofStore; + return { recorded, repaired, store }; +} + +const base = { + cdpUrl: "http://127.0.0.1:1", + connectionId: "cin_1", + connectorId: "c", + nowIso: "2026-05-20T12:00:00.000Z", + runId: "run_1", +}; + +test("a succeeded run repairs old proofs even after the manifest pattern was removed", async () => { + const { repaired, store } = fakeStore(); + const outcome = await recordSignInProofAfterRun( + { store }, + { ...base, manifest: WITHOUT_PATTERN, status: "succeeded" }, + ); + assert.equal(outcome, "repaired"); + assert.deepEqual(repaired, ["cin_1"]); +}); + +test("a capture that finished after its run was superseded writes nothing", async () => { + const { recorded, store } = fakeStore(); + let current = true; + const fetchImpl = (() => { + current = false; // the run is reclaimed while the DevTools read is in flight + return Promise.resolve( + new Response( + JSON.stringify([ + { type: "page", url: "https://accounts.example.test/signin?x=1" }, + ]), + ), + ); + }) as unknown as typeof fetch; + const outcome = await recordSignInProofAfterRun( + { fetchImpl, store }, + { + ...base, + isCurrent: () => current, + manifest: WITH_PATTERN, + status: "failed", + }, + ); + assert.equal(outcome, "none"); + assert.deepEqual(recorded, []); +}); From 3e3982c8d7955e8bc8c8e70159e3289197952fb5 Mon Sep 17 00:00:00 2001 From: Tim Nunamaker Date: Fri, 6 Nov 2026 15:50:21 -0600 Subject: [PATCH 5/5] fix(health): await the claimed sign-in capture before releasing the slot A second terminal path returned immediately, so cleanup released the slot while the first path's DevTools read was in flight and the proof was dropped as superseded. The capture is now one per-run promise that every terminal path and slot cleanup awaits. Also registers the proof-store Postgres test in the template eligibility inventory, which failed CI shard 4. Assisted-by: AI Signed-off-by: Tim Nunamaker --- PG-PROFILE-51-REPORT.md | 2 +- .../runtime/controller.ts | 39 +++++++++-- .../scripts/postgres-template-eligibility.ts | 1 + .../controller-sign-in-proof-fencing.test.ts | 69 +++++++++++++++++++ 4 files changed, 104 insertions(+), 7 deletions(-) diff --git a/PG-PROFILE-51-REPORT.md b/PG-PROFILE-51-REPORT.md index ec4534cf9..d0209d32d 100644 --- a/PG-PROFILE-51-REPORT.md +++ b/PG-PROFILE-51-REPORT.md @@ -168,7 +168,7 @@ tail: The completed profile receipt recorded 1,034 files; that historical receipt is unchanged. The template equivalence registry is a different measure: -156 eligible plus 37 cold-required files, for **193** total. The +157 eligible plus 37 cold-required files, for **194** total. The handle-generation, issuance-cutover, opaque-cursor and search-index repair tests remain registered. These counts are recomputed from the merged registry arrays. diff --git a/reference-implementation/runtime/controller.ts b/reference-implementation/runtime/controller.ts index fa2277d3b..64bb9c881 100644 --- a/reference-implementation/runtime/controller.ts +++ b/reference-implementation/runtime/controller.ts @@ -1145,9 +1145,10 @@ const runWatchdogSettlements = new Map; resolve // Growth is bounded by the number of distinct connector_instance keys ever // admitted, not by run count. const runGenerations = new Map(); -// Runs whose terminal sign-in evidence capture has been claimed. The first -// terminal path to reach a run owns the capture; later paths skip it. -const signInProofClaimedRuns = new Set(); +// The in-flight terminal sign-in evidence capture per run. The first terminal +// path to reach a run starts it; every later path, and slot cleanup, awaits +// the same promise so the slot is not released while the capture is in flight. +const signInProofCaptures = new Map>(); // Keyed by run_id. Interaction broker state is intentionally in-memory: // dashboard-submitted values satisfy the current live run only and are never // persisted to `.env.local`, SQLite config/state, or spine event payloads. @@ -3411,10 +3412,30 @@ export function createController(opts: ControllerOptions = {}): Controller { }): Promise { const stillCurrent = () => activeRuns.get(input.key)?.run_id === input.runId && runGenerations.get(input.key) === input.generation; - if (!stillCurrent() || signInProofClaimedRuns.has(input.runId)) { + const claimed = signInProofCaptures.get(input.runId); + if (claimed) { + await claimed; return; } - signInProofClaimedRuns.add(input.runId); + if (!stillCurrent()) { + return; + } + const capture = captureSignInProof(input, stillCurrent); + signInProofCaptures.set(input.runId, capture); + await capture; + } + + async function captureSignInProof( + input: { + readonly browserSurfaceLease: BrowserSurfaceLease | null; + readonly connectorId: string; + readonly connectorInstanceId: string; + readonly manifest: ConnectorManifest; + readonly runId: string; + readonly status: string | null | undefined; + }, + stillCurrent: () => boolean + ): Promise { try { const surfaceId = input.browserSurfaceLease?.surface_id; const cdpUrl = surfaceId ? (browserSurfaceLeaseManager?.getSurface(surfaceId)?.cdp_url ?? null) : null; @@ -3445,7 +3466,13 @@ export function createController(opts: ControllerOptions = {}): Controller { readonly runId: string; readonly traceContext: SpineTraceContext; }): Promise { - signInProofClaimedRuns.delete(input.runId); + // Let a claimed capture finish before the slot is released, so its + // currency check still sees the run as the slot owner. + const capture = signInProofCaptures.get(input.runId); + if (capture) { + await capture; + signInProofCaptures.delete(input.runId); + } // Idempotency guard: the watchdog and the run's own .finally() can both // call finalizeRunCleanup. Only the first call does real work; subsequent // calls are silent no-ops. We detect "already finalized" by checking diff --git a/reference-implementation/scripts/postgres-template-eligibility.ts b/reference-implementation/scripts/postgres-template-eligibility.ts index 3e82b9010..77a855bb4 100644 --- a/reference-implementation/scripts/postgres-template-eligibility.ts +++ b/reference-implementation/scripts/postgres-template-eligibility.ts @@ -128,6 +128,7 @@ export const POSTGRES_TEMPLATE_ELIGIBLE_FILES: readonly string[] = [ "test/postgres-test-database-guard.test.ts", "test/postgres-transaction-connector-instance-lock.test.ts", "test/provider-app-config-store-postgres.test.ts", + "test/provider-invalidation-proof-store-postgres.test.ts", "test/reconcile-active-summary-evidence-oracle.test.ts", "test/reconcile-schedule-and-lifecycle-checkpoints.test.ts", "test/reconcile-summary-evidence-failure-persistence-postgres.test.ts", diff --git a/reference-implementation/test/controller-sign-in-proof-fencing.test.ts b/reference-implementation/test/controller-sign-in-proof-fencing.test.ts index 677ee8570..da8e83763 100644 --- a/reference-implementation/test/controller-sign-in-proof-fencing.test.ts +++ b/reference-implementation/test/controller-sign-in-proof-fencing.test.ts @@ -10,6 +10,7 @@ import assert from "node:assert/strict"; import type { TestContext } from "node:test"; import test from "node:test"; import { emitSpineEvent } from "../lib/spine.ts"; +import { recordSignInProofAfterRun } from "../runtime/browser-surface/sign-in-proof-recorder.ts"; import { __resetControllerInteractionStateForTests, createController, @@ -322,3 +323,71 @@ test("a watchdog on a run that already completed does not record failure evidenc "no failure evidence for a run that already succeeded", ); }); + +async function persistedProofSurvivesCompetingTerminalPath( + t: TestContext, + first: "watchdog" | "result", +): Promise { + await freshDb(t); + let openGate: () => void = () => undefined; + const gate = new Promise((resolve) => { + openGate = resolve; + }); + // The real recorder, with only its DevTools read held open and answering + // with a page that matches the manifest's sign-in pattern. + const fetchImpl = (async () => { + await gate; + return new Response( + JSON.stringify([ + { type: "page", url: "https://accounts.example.test/signin?next=1" }, + ]), + ); + }) as unknown as typeof fetch; + let rejectRun: (err: Error) => void = () => undefined; + let resolveRun: (result: RuntimeRunConnectorResult) => void = () => undefined; + const controller = createController({ + admitRunConnection: admit(), + connectorPathResolver: () => "/tmp/connector.js", + logger: { error: () => undefined, warn: () => undefined }, + maxRunWallClockMs: 20, + recordSignInProof: (deps, input) => + recordSignInProofAfterRun( + { ...deps, fetchImpl }, + { ...input, cdpUrl: "http://127.0.0.1:1" }, + ), + runConnectorImpl: () => + new Promise((resolve, reject) => { + resolveRun = resolve; + rejectRun = reject; + }), + schedulerStore: schedulerStore(), + }); + await controller.runNow(CONNECTOR_ID, runOptions("run_race")); + if (first === "watchdog") { + await new Promise((resolve) => setTimeout(resolve, 200).unref()); + rejectRun(new Error("late rejection")); + } else { + resolveRun({ records_emitted: 0, status: "failed" }); + await new Promise((resolve) => setTimeout(resolve, 200).unref()); + } + await new Promise((resolve) => setTimeout(resolve, 100).unref()); + // Release the DevTools read only now, after the competing path ran. + openGate(); + await controller.drainActiveRuns(2000); + await new Promise((resolve) => setTimeout(resolve, 100).unref()); + const loaded = + await getDefaultProviderInvalidationProofStore().load(INSTANCE_ID); + assert.equal( + loaded.proof?.evidence_id, + "run_race", + "the claimed capture finished before the lease was released, so its proof is durable", + ); +} + +test("a held capture still persists its proof when the run's rejection races the watchdog", async (t) => { + await persistedProofSurvivesCompetingTerminalPath(t, "watchdog"); +}); + +test("a held capture still persists its proof when the watchdog races a failed result", async (t) => { + await persistedProofSurvivesCompetingTerminalPath(t, "result"); +});