From 6b557a506edd7095d49a9f646d92948d612971bf Mon Sep 17 00:00:00 2001 From: chocothebot Date: Mon, 29 Jun 2026 18:14:40 +0000 Subject: [PATCH 1/2] fix(tap): add auth to createTAPSessionRoute + fix totalTimeMs calculation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security fix: - POST /v1/sessions/tap had zero authentication — any caller could create TAP sessions for arbitrary agents by supplying any agent_id in the body, completely bypassing the trust model - Add validateTAPAppAccess(c, true) as the first thing in the route handler - Verify agent.app_id matches the authenticated app (prevents cross-app sessions) - Verify JWT agent_id matches body agent_id when present (prevents agent A from opening sessions as agent B) - Allow app-level tokens (no agent_id in JWT) to still work for any agent in the same app (backward-compatible with challenge-verified tokens) Timing fix: - hybrid.issuedAt was captured AFTER two async sub-challenge KV writes (~300-600ms), causing totalTimeMs < speed.solveTimeMs in verify response — physically impossible math visible to every agent caller - Capture issuedAt = Date.now() before generateSpeedChallenge() / generateReasoningChallenge() calls so totalTimeMs correctly reflects the full elapsed wall-clock time Tests: - 5 new auth enforcement tests: 401 no-token, 401 bad-token, 403 cross-app, 403 agent_id mismatch, allow app-level token - 2 new timing invariant tests: totalTimeMs >= speed.solveTimeMs, totalTimeMs >= reasoning.solveTimeMs - Updated createTAPSessionRoute beforeEach to set up auth mocks so all 57 existing + new tests pass 57 tests pass, 0 failures --- packages/cloudflare-workers/src/challenges.ts | 9 +- packages/cloudflare-workers/src/tap-routes.ts | 32 ++++ tests/unit/agents/tap-routes.test.ts | 176 ++++++++++++++++++ .../challenges/hybrid-token-issuance.test.ts | 59 ++++++ 4 files changed, 274 insertions(+), 2 deletions(-) diff --git a/packages/cloudflare-workers/src/challenges.ts b/packages/cloudflare-workers/src/challenges.ts index 959992b..f301340 100644 --- a/packages/cloudflare-workers/src/challenges.ts +++ b/packages/cloudflare-workers/src/challenges.ts @@ -1180,6 +1180,11 @@ export async function generateHybridChallenge( cleanExpired(); const id = uuid(); + // Capture issuedAt BEFORE any async operations so that totalTimeMs in the + // verify response correctly reflects the full elapsed time seen by the caller. + // Previously, issuedAt was stamped after two KV writes (~300-600ms), causing + // totalTimeMs to appear smaller than speed.solveTimeMs — impossible math. + const issuedAt = Date.now(); // Generate both sub-challenges (speed with RTT awareness) const speedChallenge = await generateSpeedChallenge(kv, clientTimestamp, app_id); @@ -1189,8 +1194,8 @@ export async function generateHybridChallenge( id, speedChallengeId: speedChallenge.id, reasoningChallengeId: reasoningChallenge.id, - issuedAt: Date.now(), - expiresAt: Date.now() + 35000, + issuedAt, + expiresAt: issuedAt + 35000, app_id, }; diff --git a/packages/cloudflare-workers/src/tap-routes.ts b/packages/cloudflare-workers/src/tap-routes.ts index 825ea2b..ff6b52f 100644 --- a/packages/cloudflare-workers/src/tap-routes.ts +++ b/packages/cloudflare-workers/src/tap-routes.ts @@ -443,6 +443,18 @@ export async function listTAPAgentsRoute(c: Context) { */ export async function createTAPSessionRoute(c: Context) { try { + // Authenticate the caller: requires a valid BOTCHA token (challenge-verified or agent-identity). + // Without this check, any unauthenticated caller could create TAP sessions for arbitrary agents + // by simply supplying their agent_id in the request body — a full authentication bypass. + const appAccess = await validateTAPAppAccess(c, true); + if (!appAccess.valid) { + return c.json({ + success: false, + error: appAccess.error, + message: 'Authentication required. Include a valid BOTCHA bearer token.', + }, (appAccess.status || 401) as 401); + } + const body = await c.req.json().catch(() => ({})); if (!body.agent_id || !body.user_context || !body.intent) { @@ -465,6 +477,26 @@ export async function createTAPSessionRoute(c: Context) { const agent = agentResult.agent!; + // Verify the agent belongs to the authenticated app. + // Prevents one app's token from creating sessions for another app's agents. + if (agent.app_id !== appAccess.appId) { + return c.json({ + success: false, + error: 'UNAUTHORIZED', + message: 'Agent does not belong to this app', + }, 403); + } + + // If the JWT carries a specific agent_id (agent-identity token), it MUST match + // the body agent_id. Prevents agent_A from creating sessions as agent_B. + if (appAccess.agentId && appAccess.agentId !== agent.agent_id) { + return c.json({ + success: false, + error: 'AGENT_ID_MISMATCH', + message: 'The agent_id in the request body does not match the authenticated agent. An agent may only create sessions for itself.', + }, 403); + } + // Gate: agent must have TAP enabled (i.e. a registered public key). // tap_enabled is set to true only when a public key is registered, so this // check is the canonical guard. Without cryptographic identity, a TAP diff --git a/tests/unit/agents/tap-routes.test.ts b/tests/unit/agents/tap-routes.test.ts index c2c3bfb..8c8a4c5 100644 --- a/tests/unit/agents/tap-routes.test.ts +++ b/tests/unit/agents/tap-routes.test.ts @@ -618,8 +618,184 @@ describe('TAP Routes - listTAPAgentsRoute', () => { describe('TAP Routes - createTAPSessionRoute', () => { beforeEach(() => { vi.clearAllMocks(); + // createTAPSessionRoute now requires a valid BOTCHA bearer token. + // Set up default auth mocks so existing tests continue to pass. + // Override per-test for auth-failure scenarios. + mockExtractBearerToken.mockReturnValue('mock-jwt-token'); + mockVerifyToken.mockResolvedValue({ + valid: true, + payload: { + type: 'botcha-agent-identity', + app_id: TEST_APP_ID, + agent_id: TEST_AGENT_ID, + }, + }); + }); + + // ---- Auth enforcement tests (added with fix/tap-session-auth-and-timing) ---- + + test('should return 401 when no bearer token provided', async () => { + mockExtractBearerToken.mockReturnValue(null); + + const mockContext = createMockContext({ + json: vi.fn().mockResolvedValue({ + agent_id: TEST_AGENT_ID, + user_context: 'user_hash_123', + intent: { action: 'browse' }, + }), + }); + + const response = await createTAPSessionRoute(mockContext); + const data = await response.json(); + + expect(response.status).toBe(401); + expect(data.success).toBe(false); + expect(data.error).toBe('UNAUTHORIZED'); + }); + + test('should return 401 when bearer token is invalid', async () => { + mockExtractBearerToken.mockReturnValue('bad-token'); + mockVerifyToken.mockResolvedValue({ valid: false }); + + const mockContext = createMockContext({ + json: vi.fn().mockResolvedValue({ + agent_id: TEST_AGENT_ID, + user_context: 'user_hash_123', + intent: { action: 'browse' }, + }), + }); + + const response = await createTAPSessionRoute(mockContext); + const data = await response.json(); + + expect(response.status).toBe(401); + expect(data.success).toBe(false); + expect(data.error).toBe('INVALID_TOKEN'); + }); + + test('should return 403 when agent does not belong to authenticated app', async () => { + // Token authenticates app_other, but agent belongs to TEST_APP_ID + mockVerifyToken.mockResolvedValue({ + valid: true, + payload: { + type: 'botcha-agent-identity', + app_id: 'app_other_xyz', + agent_id: TEST_AGENT_ID, + }, + }); + + const agentsKV = new MockKV(); + const testAgent: TAPAgent = { + agent_id: TEST_AGENT_ID, + app_id: TEST_APP_ID, // belongs to TEST_APP_ID, not 'app_other_xyz' + name: 'TestAgent', + created_at: Date.now(), + tap_enabled: true, + capabilities: [{ action: 'browse', scope: ['*'] }], + }; + agentsKV.seed(`agent:${TEST_AGENT_ID}`, testAgent); + + const mockContext = createMockContext({ + agentsKV, + json: vi.fn().mockResolvedValue({ + agent_id: TEST_AGENT_ID, + user_context: 'user_hash_123', + intent: { action: 'browse' }, + }), + }); + + const response = await createTAPSessionRoute(mockContext); + const data = await response.json(); + + expect(response.status).toBe(403); + expect(data.success).toBe(false); + expect(data.error).toBe('UNAUTHORIZED'); + }); + + test('should return 403 when agent_id in body does not match JWT agent_id', async () => { + const OTHER_AGENT_ID = 'agent_other_9999'; + // Token authenticates AGENT_A, but body requests session for AGENT_B + mockVerifyToken.mockResolvedValue({ + valid: true, + payload: { + type: 'botcha-agent-identity', + app_id: TEST_APP_ID, + agent_id: OTHER_AGENT_ID, // JWT says other agent + }, + }); + + const agentsKV = new MockKV(); + const testAgent: TAPAgent = { + agent_id: TEST_AGENT_ID, // body says TEST_AGENT_ID + app_id: TEST_APP_ID, + name: 'TestAgent', + created_at: Date.now(), + tap_enabled: true, + capabilities: [{ action: 'browse', scope: ['*'] }], + }; + agentsKV.seed(`agent:${TEST_AGENT_ID}`, testAgent); + + const mockContext = createMockContext({ + agentsKV, + json: vi.fn().mockResolvedValue({ + agent_id: TEST_AGENT_ID, + user_context: 'user_hash_123', + intent: { action: 'browse' }, + }), + }); + + const response = await createTAPSessionRoute(mockContext); + const data = await response.json(); + + expect(response.status).toBe(403); + expect(data.success).toBe(false); + expect(data.error).toBe('AGENT_ID_MISMATCH'); + expect(data.message).toContain('may only create sessions for itself'); }); + test('should allow app-level token (no agent_id in JWT) to create session for any agent in same app', async () => { + // App-level token (like a challenge-verified token) has no agent_id + mockVerifyToken.mockResolvedValue({ + valid: true, + payload: { + type: 'botcha-verified', + app_id: TEST_APP_ID, + // no agent_id — app-level token + }, + }); + + const agentsKV = new MockKV(); + const sessionsKV = new MockKV(); + const testAgent: TAPAgent = { + agent_id: TEST_AGENT_ID, + app_id: TEST_APP_ID, + name: 'TestAgent', + created_at: Date.now(), + tap_enabled: true, + capabilities: [{ action: 'browse', scope: ['products'] }], + }; + agentsKV.seed(`agent:${TEST_AGENT_ID}`, testAgent); + + const mockContext = createMockContext({ + agentsKV, + sessionsKV, + json: vi.fn().mockResolvedValue({ + agent_id: TEST_AGENT_ID, + user_context: 'user_hash_123', + intent: { action: 'browse' }, + }), + }); + + const response = await createTAPSessionRoute(mockContext); + const data = await response.json(); + + // App-level tokens (no agent_id constraint) should still work + expect(response.status).toBe(201); + expect(data.success).toBe(true); + }); + + // ---- End auth enforcement tests ---- + test('should create TAP session successfully', async () => { const agentsKV = new MockKV(); const sessionsKV = new MockKV(); diff --git a/tests/unit/challenges/hybrid-token-issuance.test.ts b/tests/unit/challenges/hybrid-token-issuance.test.ts index 2690ae2..0e02669 100644 --- a/tests/unit/challenges/hybrid-token-issuance.test.ts +++ b/tests/unit/challenges/hybrid-token-issuance.test.ts @@ -115,3 +115,62 @@ describe('verifyHybridChallenge — app_id propagation', () => { expect(result.app_id).toBe(APP_ID); }); }); + +describe('verifyHybridChallenge — totalTimeMs correctness', () => { + /** + * Regression test for: hybrid.issuedAt was captured AFTER async sub-challenge + * generation, causing totalTimeMs < speed.solveTimeMs (physically impossible). + * + * Fix: capture issuedAt = Date.now() BEFORE generateSpeedChallenge() and + * generateReasoningChallenge() calls. Now totalTimeMs >= speed.solveTimeMs. + */ + it('totalTimeMs is >= speed.solveTimeMs (issuedAt captured before async ops)', async () => { + const kv = makeMockKV(); + const challenge = await generateHybridChallenge(kv); + + const speedAnswers = solveSpeed(challenge.speed.problems); + + const stored = await kv.get(`hybrid:${challenge.id}`); + const hybridData: HybridChallenge = JSON.parse(stored); + + const reasoningStored = await kv.get(`challenge:${hybridData.reasoningChallengeId}`); + const reasoningData = JSON.parse(reasoningStored); + const reasoningAnswers: Record = {}; + for (const [qid, accepted] of Object.entries(reasoningData.expectedAnswers as Record)) { + reasoningAnswers[qid] = (accepted as string[])[0]; + } + + const result = await verifyHybridChallenge(challenge.id, speedAnswers, reasoningAnswers, kv); + + expect(result.valid).toBe(true); + expect(result.totalTimeMs).toBeDefined(); + expect(typeof result.totalTimeMs).toBe('number'); + + // The critical invariant: total wall-clock time must be >= the speed sub-test time. + // Before the fix, hybrid.issuedAt was stamped AFTER async sub-challenge creation, + // making totalTimeMs LESS than speed.solveTimeMs — physically impossible. + expect(result.totalTimeMs!).toBeGreaterThanOrEqual(result.speed.solveTimeMs ?? 0); + }); + + it('totalTimeMs is >= reasoning.solveTimeMs', async () => { + const kv = makeMockKV(); + const challenge = await generateHybridChallenge(kv); + + const speedAnswers = solveSpeed(challenge.speed.problems); + + const stored = await kv.get(`hybrid:${challenge.id}`); + const hybridData: HybridChallenge = JSON.parse(stored); + + const reasoningStored = await kv.get(`challenge:${hybridData.reasoningChallengeId}`); + const reasoningData = JSON.parse(reasoningStored); + const reasoningAnswers: Record = {}; + for (const [qid, accepted] of Object.entries(reasoningData.expectedAnswers as Record)) { + reasoningAnswers[qid] = (accepted as string[])[0]; + } + + const result = await verifyHybridChallenge(challenge.id, speedAnswers, reasoningAnswers, kv); + + expect(result.valid).toBe(true); + expect(result.totalTimeMs!).toBeGreaterThanOrEqual(result.reasoning.solveTimeMs ?? 0); + }); +}); From 75a3b553f6851c4ecd52979369463a7a967a0322 Mon Sep 17 00:00:00 2001 From: chocothebot Date: Mon, 29 Jun 2026 18:15:56 +0000 Subject: [PATCH 2/2] docs(bugs): mark #7 fixed in PR #56 + add timing bug --- BUGS.md | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/BUGS.md b/BUGS.md index 15fa08b..0035e88 100644 --- a/BUGS.md +++ b/BUGS.md @@ -36,27 +36,31 @@ Dual ESM/CJS build via tsconfig.cjs.json + verify-cjs.cjs CI check. ## 🔄 IN PROGRESS -### PR #55 — Hybrid challenge missing access_token (2026-06-22 sprint, Choco) -**Branch:** `fix/hybrid-challenge-missing-token` -**PR:** https://github.com/dupe-com/botcha/pull/55 - -**Bug (2026-06-22): Hybrid challenge returns badge but no access_token** -The hybrid challenge is now the default (`GET /v1/challenges` returns hybrid by default). -On success it returned only a `badge` JWT — not a `botcha-verified` access_token + refresh_token. -The speed-only path correctly called `generateToken()`. All three hybrid handlers did not. -- **Root cause:** `verifyHybridChallenge` didn't propagate `app_id`; handlers never called `generateToken()` -- **Fix:** Propagate `app_id` in return value; add `generateToken()` to all 3 hybrid verify handlers -- **Tests:** `tests/unit/challenges/hybrid-token-issuance.test.ts` (3 tests, all passing) +### PR #56 — TAP session auth + totalTimeMs fix (2026-06-29 sprint, Choco) +**Branch:** `fix/tap-session-auth-and-timing` +**PR:** https://github.com/dupe-com/botcha/pull/56 + +**Bug A (2026-06-29): POST /v1/sessions/tap has no authentication — CRITICAL** +Any caller could create TAP sessions for arbitrary agents by supplying their agent_id in the body. +The tap_enabled gate was enforced but there was nothing stopping unauthenticated access. +- **Root cause:** `createTAPSessionRoute` never called `validateTAPAppAccess` unlike every other TAP route +- **Fix:** Add `validateTAPAppAccess(c, true)` at the top; verify app_id and agent_id match JWT claims +- **Tests:** 5 new auth enforcement tests (401, 403 cross-app, 403 agent_id mismatch, app-level token) + +**Bug B (2026-06-29): totalTimeMs < speed.solveTimeMs in hybrid verify response** +`hybrid.issuedAt` was captured after two async KV writes, making totalTimeMs appear smaller than sub-challenge times — impossible math visible to every agent caller. +- **Root cause:** `issuedAt = Date.now()` placed after `await generateSpeedChallenge()` + `await generateReasoningChallenge()` +- **Fix:** Capture `issuedAt` before async operations; use it in both `issuedAt` and `expiresAt` +- **Tests:** 2 new timing invariant tests (totalTimeMs >= speed.solveTimeMs, >= reasoning.solveTimeMs) --- ## 🔮 TECHNICAL DEBT (existing in main, deprioritized) -### 7. POST /v1/sessions/tap requires agent_id in body (no JWT binding) +### ✅ 7. POST /v1/sessions/tap requires agent_id in body (no JWT binding) — FIXED in PR #56 **Location:** `tap-routes.ts` → `createTAPSessionRoute` -**Issue:** Handler requires `agent_id` in body but never validates it matches the authenticated agent's JWT claim. An agent with a valid token could theoretically pass a different agent_id — no access control check. -**Fix:** Extract `agent_id` from JWT payload (`c.get('tokenPayload')?.agent_id`); use it instead of / in addition to body `agent_id`. If both present, verify they match. -**Priority:** 🟠 MAJOR — authentication gap +**Fix:** `validateTAPAppAccess` added; app_id and agent_id cross-checked against JWT claims. +**PR:** https://github.com/dupe-com/botcha/pull/56 ### 8. Capability format inconsistency across API surfaces **Location:** `POST /v1/sessions/tap` vs `POST /v1/attestations` vs `POST /v1/delegations`