diff --git a/src/core.ts b/src/core.ts index 7c99fd2..1eb75d3 100644 --- a/src/core.ts +++ b/src/core.ts @@ -891,17 +891,71 @@ export function createAgentScoreCore(options: AgentScoreCoreOptions): AgentScore const decision = data.decision as string | null | undefined; const decisionReasons = (data.decision_reasons as string[]) ?? []; - const allow = decision === 'allow' || decision == null; - cache.set(cacheKey, { allow, decision: decision ?? undefined, reasons: decisionReasons, raw: data }); + // FAIL CLOSED on a response we cannot read as an approval. + // + // This previously read `decision === 'allow' || decision == null`, so a + // response carrying no decision at all was treated as an approval. Combined + // with the API silently ignoring an unrecognised policy key, a merchant + // could send a misspelled rule and have both layers agree to let the request + // through. Only the literal string 'allow' is an approval now; null, + // undefined, and any value we add later are not. + // + // The two failures below are deliberately NOT handled the same way, because + // they are not the same kind of failure. + const decisionIsAllow = decision === 'allow'; + const decisionIsUnreadable = decision == null; - // Compose the per-request signer verdicts and ride them on the RETURN VALUE so the adapter can - // stash them on its per-request state (NOT a shared slot — see buildSignerVerdict). guard: - // wallet-only identity (operator-token / AIT win and signer-match is deliberately not enforced). + // Computed here rather than after the allow decision: the fail-closed + // branches below return early and still owe the adapter these verdicts. const signerVerdict = buildSignerVerdict(identity, data); - // Rides the same assess response, so reading it later costs nothing extra. const operatorHandle = readOperatorHandle(data); + // A policy we SENT must come back evaluated and passing. `policy_result` is + // null for every ungated call, which is correct and must stay an allow, so + // this is conditioned on having sent a policy rather than on the field being + // null. Denying on a null policy_result unconditionally would deny every + // ungated merchant request in production. + const policySent = Object.keys(policy).length > 0; + const policyResult = data.policy_result as { all_passed?: boolean } | null | undefined; + const policySatisfied = !policySent || policyResult?.all_passed === true; + + if (decisionIsUnreadable) { + // No decision field. Indistinguishable from a truncated or proxied + // response, so this is an INFRASTRUCTURE failure and honours the + // merchant's explicit `failOpen` choice, exactly as an unreachable API + // does above. What changed is that it is no longer a silent allow for + // merchants who did NOT opt into that. + console.warn('[gate] /v1/assess returned no decision; treating as api_error'); + if (failOpen) return { kind: 'allow', degraded: true, infraReason: 'api_error' }; + return { kind: 'deny', reason: { code: 'api_error' } }; + } + + if (decisionIsAllow && !policySatisfied) { + // An allow carrying no passing policy_result, for a request that DID send + // a policy. Not an availability problem: the gate was asked to enforce + // something and the answer does not show it was enforced. Denies + // regardless of `failOpen`, because failing open here would reinstate the + // exact hole this closes. + console.warn('[gate] /v1/assess allowed a request whose policy was not evaluated; denying'); + return { + kind: 'deny', + reason: { + code: 'api_error', + decision: decision ?? undefined, + reasons: decisionReasons, + data: data as unknown as AssessResult, + }, + ...(signerVerdict !== undefined && { signerVerdict }), + ...(operatorHandle !== undefined && { operatorHandle }), + }; + } + + const allow = decisionIsAllow && policySatisfied; + + cache.set(cacheKey, { allow, decision: decision ?? undefined, reasons: decisionReasons, raw: data }); + + if (allow) { // SDK populates `quota` on the assess response from X-Quota-* headers when the // API emits them. Surface up to the adapter so merchants can monitor approach-to-cap. diff --git a/tests/express.test.ts b/tests/express.test.ts index 9abdbba..6250af1 100644 --- a/tests/express.test.ts +++ b/tests/express.test.ts @@ -377,28 +377,163 @@ describe('agentscoreGate middleware — cache', () => { }); -describe('agentscoreGate middleware — decision null/undefined treated as allow', () => { +describe('agentscoreGate middleware — a response with no decision fails closed', () => { afterEach(() => { vi.restoreAllMocks(); }); - it('allows when decision is null', async () => { + // These two previously asserted the OPPOSITE, under the name "decision + // null/undefined treated as allow": a response carrying no decision was let + // through. A penetration test found that combined with the API silently + // ignoring an unrecognised policy key, so a misspelled rule could pass both + // layers. A missing decision is now indistinguishable-from-broken and is + // treated as an infrastructure failure, which denies unless the merchant has + // explicitly opted into failOpen. + it('denies when decision is null', async () => { mockFetchOk({ ...ALLOW_RESPONSE, decision: null }); const mw = agentscoreGate({ apiKey: API_KEY }); const req = makeReq(WALLET); - const { res } = makeRes(); + const { res, status, json } = makeRes(); const next = makeNext(); await mw(req, res, next); - expect(next).toHaveBeenCalledOnce(); + expect(next).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(503); + // api_error at 503, not wallet_not_trusted at 403: the response was + // unreadable, which is our failure and retryable, not a compliance verdict + // against the buyer. The distinction matters to an agent choosing whether to + // retry or to go and get verified. + expect(json).toHaveBeenCalledWith(expect.objectContaining({ + error: expect.objectContaining({ code: 'api_error' }), + })); }); - it('allows when decision field is missing (undefined)', async () => { + it('denies when the decision field is missing (undefined)', async () => { const { decision: _, ...noDecision } = ALLOW_RESPONSE; mockFetchOk(noDecision); const mw = agentscoreGate({ apiKey: API_KEY }); const req = makeReq(WALLET); + const { res, status } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(503); + }); + + // The merchant's explicit availability choice still applies here, exactly as + // it does for an unreachable API: a missing decision is an unreadable + // response, not a compliance verdict. + it('honours failOpen when the merchant opted into it', async () => { + mockFetchOk({ ...ALLOW_RESPONSE, decision: null }); + const mw = agentscoreGate({ apiKey: API_KEY, failOpen: true }); + const req = makeReq(WALLET); + const { res } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).toHaveBeenCalledOnce(); + }); + + // An unrecognised decision value must not be an allow either, so adding a new + // decision on the API side cannot silently open the gate on old SDKs. This one + // denies at 403 rather than 503: the API returned a READABLE decision that is + // simply not an approval, which is a verdict rather than an infrastructure + // failure, so it is not retryable and must not honour failOpen. + it('denies an unrecognised decision value', async () => { + mockFetchOk({ ...ALLOW_RESPONSE, decision: 'review' }); + const mw = agentscoreGate({ apiKey: API_KEY }); + const req = makeReq(WALLET); + const { res, status } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(403); + }); +}); + +describe('agentscoreGate middleware — a sent policy must come back evaluated', () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + // The other half of the same finding. The API used to ignore an unrecognised + // policy key and answer allow with policy_result null, so the gate approved a + // request whose policy never ran. The API rejects that now, but the gate must + // not depend on the API being correct: if we ASKED for enforcement and the + // answer does not show it happened, that is a deny. + it('denies an allow whose policy_result is null when a policy was sent', async () => { + mockFetchOk({ ...ALLOW_RESPONSE, decision: 'allow', policy_result: null }); + const mw = agentscoreGate({ apiKey: API_KEY, requireKyc: true }); + const req = makeReq(WALLET); + const { res, status } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(503); + }); + + it('denies an allow whose policy_result did not pass', async () => { + mockFetchOk({ + ...ALLOW_RESPONSE, + decision: 'allow', + policy_result: { all_passed: false, checks: [] }, + }); + const mw = agentscoreGate({ apiKey: API_KEY, requireKyc: true }); + const req = makeReq(WALLET); + const { res, status } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).not.toHaveBeenCalled(); + expect(status).toHaveBeenCalledWith(503); + }); + + // failOpen is an AVAILABILITY setting. It must not reinstate the hole: we + // asked for enforcement and did not get it, which is not an outage. + it('denies even under failOpen, because this is not an availability failure', async () => { + mockFetchOk({ ...ALLOW_RESPONSE, decision: 'allow', policy_result: null }); + const mw = agentscoreGate({ apiKey: API_KEY, failOpen: true, requireKyc: true }); + const req = makeReq(WALLET); + const { res } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).not.toHaveBeenCalled(); + }); + + // The guard is conditioned on having SENT a policy. A null policy_result is + // the correct, normal response for every ungated call, and denying on it + // unconditionally would deny every ungated merchant request in production. + it('still allows an ungated request whose policy_result is null', async () => { + mockFetchOk({ ...ALLOW_RESPONSE, decision: 'allow', policy_result: null }); + const mw = agentscoreGate({ apiKey: API_KEY }); + const req = makeReq(WALLET); + const { res } = makeRes(); + const next = makeNext(); + + await mw(req, res, next); + + expect(next).toHaveBeenCalledOnce(); + }); + + it('allows when a sent policy came back passing', async () => { + mockFetchOk({ + ...ALLOW_RESPONSE, + decision: 'allow', + policy_result: { all_passed: true, checks: [{ passed: true, rule: 'require_kyc' }] }, + }); + const mw = agentscoreGate({ apiKey: API_KEY, requireKyc: true }); + const req = makeReq(WALLET); const { res } = makeRes(); const next = makeNext();