From 14d48ca0f91e00c2a52988c86a667f30512c3171 Mon Sep 17 00:00:00 2001 From: Padreug Date: Sun, 21 Jun 2026 12:44:36 +0200 Subject: [PATCH] fix(acl): hard-reject a lapsed token binding instead of prompting (#36) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A request that finds no live token grant exited `checkIfPubkeyAllowed` at `undefined` regardless of *why* — whether the binding never existed or had simply lapsed (expired / token-revoked). `undefined` routes the caller into the admin-prompt path, which for an unattended client (an ATM spire) means the request hangs until a BunkerTimeoutError. The Sintra smoke proved the divergence directly: a KeyUser-level revoke exits at step 2 with `false` and the spire sees a clean BunkerRejected ("Pairing Required"), but a TTL expiry fell through to `undefined` and the spire saw a BunkerTimeout ("Signer Unreachable") — same operator intent ("this pairing is over"), two different, one-broken outcomes. Classify the no-live-grant case before returning: if a token bound to this KeyUser *would* have granted the request (its policy carries a matching rule; for `connect`, any bound token) but is now expired or token-revoked, return `false` so the client re-pairs immediately. Only a genuinely never-granted (method/kind) request stays `undefined` so an admin can still approve new permission out-of-band. Usage-cap exhaustion is left at `undefined` deliberately: a windowed cap is a temporary rate-limit that refills as the window rolls, not a permanent lapse, so it must not be reclassed as the re-pair signal. A dedicated rate-limit reply is a separate follow-up. Tests: the #24 expired-token and token-revoke guards now assert `false`; added connect-lapse, and two distinction cases proving a never-granted method (live token, or a method the lapsed token never covered) stays `undefined`. --- src/daemon/lib/acl/index.ts | 73 +++++++++++++++++++++++++---------- tests/acl.integration.test.ts | 39 ++++++++++++++++--- 2 files changed, 85 insertions(+), 27 deletions(-) diff --git a/src/daemon/lib/acl/index.ts b/src/daemon/lib/acl/index.ts index b8f72fa..62de873 100644 --- a/src/daemon/lib/acl/index.ts +++ b/src/daemon/lib/acl/index.ts @@ -94,6 +94,32 @@ export async function checkIfPubkeyAllowed( // policy has a matching PolicyRule. The live filter is what closes #24: // an expired or revoked token simply stops matching here, every request, // with no photocopy to outlive it. + // PolicyRule.kind matching: + // - exact match against the stringified payload kind (matches the + // create_new_policy.ts storage format `rule.kind.toString()`) + // - 'all' literal matches any kind + // - NULL kind is a defensive wildcard — no current writer emits a + // null-kind rule, but treat it as a wildcard rather than failing + // closed silently if one ever appears (raw SQL, future code). + const payloadKindString = + method === 'sign_event' && typeof payload === 'object' && payload?.kind !== undefined + ? payload.kind.toString() + : undefined; + + const kindMatchers: Array<{ kind: string | null }> = [{ kind: null }, { kind: 'all' }]; + if (payloadKindString !== undefined) { + kindMatchers.push({ kind: payloadKindString }); + } + + // A token "grants" this request when its policy carries a matching rule — + // except `connect`, which any bound token grants (it IS the pairing). This + // predicate is reused below to tell a lapsed binding from a never-granted + // one (#36). + const policyMatch = + method === 'connect' + ? {} + : { policy: { rules: { some: { method, OR: kindMatchers } } } }; + if (method === 'connect') { const liveToken = await prisma.token.findFirst({ where: { keyUserId: keyUser.id, ...live }, @@ -103,23 +129,6 @@ export async function checkIfPubkeyAllowed( return true; } } else { - // PolicyRule.kind matching: - // - exact match against the stringified payload kind (matches the - // create_new_policy.ts storage format `rule.kind.toString()`) - // - 'all' literal matches any kind - // - NULL kind is a defensive wildcard — no current writer emits a - // null-kind rule, but treat it as a wildcard rather than failing - // closed silently if one ever appears (raw SQL, future code). - const payloadKindString = - method === 'sign_event' && typeof payload === 'object' && payload?.kind !== undefined - ? payload.kind.toString() - : undefined; - - const kindMatchers: Array<{ kind: string | null }> = [{ kind: null }, { kind: 'all' }]; - if (payloadKindString !== undefined) { - kindMatchers.push({ kind: payloadKindString }); - } - // Find live tokens bound to this KeyUser whose policy has at least one // rule matching (method, kind), and pull the rules so usage caps can be // enforced live off the SigningLog (#28). @@ -127,7 +136,7 @@ export async function checkIfPubkeyAllowed( where: { keyUserId: keyUser.id, ...live, - policy: { rules: { some: { method, OR: kindMatchers } } }, + ...policyMatch, }, include: { policy: { include: { rules: true } } }, }); @@ -157,7 +166,11 @@ export async function checkIfPubkeyAllowed( }, }); if (used >= rule.maxUsageCount) { - // Cap exhausted — deny (step 5 / caller may still prompt). + // Cap exhausted. Left as `undefined` deliberately: a windowed + // cap is a temporary rate-limit (it refills as the window + // rolls), not a permanent lapse, so it must NOT be reclassed + // as the re-pair signal below. A dedicated rate-limit reply + // is a separate follow-up to #36. return undefined; } } @@ -165,8 +178,26 @@ export async function checkIfPubkeyAllowed( } } - // Step 5: no live override and no live token grant matched. Caller's - // requestPermission flow may still prompt the admin out-of-band. + // Step 5: no LIVE grant. Distinguish a LAPSED binding — a token that would + // have granted this request but is now expired or token-revoked — from a + // request that was never granted at all. A lapsed binding hard-rejects + // (`false`) so an unattended client (e.g. a spire) re-pairs immediately + // instead of hanging on the admin-prompt path; a never-granted request + // stays `undefined` so the caller's requestPermission flow can still prompt + // an admin to approve a genuinely new permission. See aiolabs/nsecbunkerd#36. + const lapsedGrant = await prisma.token.findFirst({ + where: { + keyUserId: keyUser.id, + ...policyMatch, + OR: [{ revokedAt: { not: null } }, { expiresAt: { lte: now } }], + }, + }); + + if (lapsedGrant) { + return false; + } + + // Step 6: genuinely no grant for this (method, kind). return undefined; } diff --git a/tests/acl.integration.test.ts b/tests/acl.integration.test.ts index 6907f70..fe7fd48 100644 --- a/tests/acl.integration.test.ts +++ b/tests/acl.integration.test.ts @@ -149,18 +149,25 @@ test('live token + matching policy rule -> sign_event allowed', async () => { assert.equal(await checkIfPubkeyAllowed(KEY, PUB, 'sign_event', signEvt), true); }); -test('expired token -> sign_event denied [#24 regression guard]', async () => { +// A token bound to the KeyUser that has lapsed (expiry or token-revoke) means +// the pairing WAS granted and is now spent. It must hard-reject with `false` so +// an unattended client re-pairs immediately, NOT `undefined` (which routes to +// the admin-prompt path and hangs an ATM until a BunkerTimeoutError). The smoke +// on the Sintra proved the divergence: revoke -> clean reject, expiry -> hang. +// See aiolabs/nsecbunkerd#36 (and #24, which made the expired token stop +// granting in the first place). +test('expired token -> sign_event hard-rejected (false) [#24 + #36]', async () => { const pid = await seedPolicy('sign_event', '1'); const ku = await seedKeyUser(); await seedToken(ku, pid, { expiresAt: past() }); - assert.equal(await checkIfPubkeyAllowed(KEY, PUB, 'sign_event', signEvt), undefined); + assert.equal(await checkIfPubkeyAllowed(KEY, PUB, 'sign_event', signEvt), false); }); -test('revoked token -> sign_event denied', async () => { +test('token-revoked -> sign_event hard-rejected (false) [#36]', async () => { const pid = await seedPolicy('sign_event', '1'); const ku = await seedKeyUser(); await seedToken(ku, pid, { revokedAt: past() }); - assert.equal(await checkIfPubkeyAllowed(KEY, PUB, 'sign_event', signEvt), undefined); + assert.equal(await checkIfPubkeyAllowed(KEY, PUB, 'sign_event', signEvt), false); }); test('live token -> connect allowed (pairing)', async () => { @@ -170,11 +177,31 @@ test('live token -> connect allowed (pairing)', async () => { assert.equal(await checkIfPubkeyAllowed(KEY, PUB, 'connect'), true); }); -test('expired token -> connect denied', async () => { +test('expired token -> connect hard-rejected (false) [#36]', async () => { const pid = await seedPolicy('sign_event', '1'); const ku = await seedKeyUser(); await seedToken(ku, pid, { expiresAt: past() }); - assert.equal(await checkIfPubkeyAllowed(KEY, PUB, 'connect'), undefined); + assert.equal(await checkIfPubkeyAllowed(KEY, PUB, 'connect'), false); +}); + +// The reject above is reserved for a binding that LAPSED. A request whose method +// was never in the (still-live) token's policy is genuinely new permission and +// must stay `undefined` so an admin could approve it out-of-band — it must NOT +// be swept up by the #36 re-pair signal. +test('live token, method outside its policy -> undefined (never granted, not lapsed) [#36]', async () => { + const pid = await seedPolicy('sign_event', '1'); + const ku = await seedKeyUser(); + await seedToken(ku, pid, {}); + assert.equal(await checkIfPubkeyAllowed(KEY, PUB, 'nip44_encrypt'), undefined); +}); + +// And a lapsed token only rejects the method IT covered: a different method has +// no lapsed grant of its own, so it stays a never-granted `undefined`. +test('expired token, request a method it never covered -> undefined [#36]', async () => { + const pid = await seedPolicy('sign_event', '1'); + const ku = await seedKeyUser(); + await seedToken(ku, pid, { expiresAt: past() }); + assert.equal(await checkIfPubkeyAllowed(KEY, PUB, 'nip44_encrypt'), undefined); }); test('KeyUser.revokedAt denies (false) and beats a live token', async () => {