From f5f57e60d924bbbddfaf9ef578b56b7c489e75cf Mon Sep 17 00:00:00 2001 From: Dorian Date: Fri, 31 Jul 2026 12:06:36 -0400 Subject: [PATCH] fix(security): POST /api/auth/update trusted a client-supplied pubkey (IDOR) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found live while wiring the "existing bot" AI-config settings UI: this route parsed `pubkey` from the request body and used it directly to select which bot row to update, with no check that it matched the caller's actual authenticated identity. Any unauthenticated caller could POST an arbitrary victim's pubkey plus a malicious webhookUrl, profilePicUrl, or customization payload and silently hijack that bot (e.g. redirect its webhook to an attacker-controlled endpoint). Contrast with GET /me and POST /regenerate-secret, which both correctly derive pubkey from the verified JWT via extractPubkeyFromAuth and never trust a client-claimed identity — this was the one route that didn't follow that pattern. Fixed by deriving pubkey from the JWT exclusively; updateBotSchema no longer declares a pubkey field at all (was the only schema-level signal that the vulnerable code path existed). Frontend callers updated to stop sending a pubkey they no longer need. Added a regression suite (auth-update.test.ts) covering: 401 with no/garbage auth, hijack-attempt-via-body-pubkey now 404s and leaves the victim's row untouched, and legitimate self-updates still work when the body happens to carry an unrelated pubkey field (ignored, not trusted). Full server suite: 829/829 passing. tsc --noEmit clean (server + frontend). Co-Authored-By: Claude --- frontend/src/composables/useNostr.ts | 4 +- server/src/lib/validators.test.ts | 12 ++- server/src/lib/validators.ts | 7 +- server/src/routes/auth-update.test.ts | 111 ++++++++++++++++++++++++++ server/src/routes/auth.ts | 18 ++++- 5 files changed, 145 insertions(+), 7 deletions(-) create mode 100644 server/src/routes/auth-update.test.ts diff --git a/frontend/src/composables/useNostr.ts b/frontend/src/composables/useNostr.ts index 86a975a..646521c 100644 --- a/frontend/src/composables/useNostr.ts +++ b/frontend/src/composables/useNostr.ts @@ -380,7 +380,7 @@ export function useNostr() { const res = await authFetch('/api/auth/update', { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ pubkey: pubkey.value, customization }), + body: JSON.stringify({ customization }), }) if (!res.ok) { @@ -404,7 +404,7 @@ export function useNostr() { const res = await authFetch('/api/auth/update', { method: 'POST', headers: { 'Content-Type': 'application/json' }, - body: JSON.stringify({ pubkey: pubkey.value, webhookUrl: newUrl }), + body: JSON.stringify({ webhookUrl: newUrl }), }) const data = await res.json() diff --git a/server/src/lib/validators.test.ts b/server/src/lib/validators.test.ts index 86113b4..1a9bda6 100644 --- a/server/src/lib/validators.test.ts +++ b/server/src/lib/validators.test.ts @@ -132,14 +132,22 @@ describe('registerHumanSchema', () => { }) describe('updateBotSchema', () => { - const base = { pubkey: 'a'.repeat(64) } - it('accepts pubkey only (no updates)', () => { expect(updateBotSchema.safeParse(base).success).toBe(true) }) + const base = {} + it('accepts empty body (no updates)', () => { expect(updateBotSchema.safeParse(base).success).toBe(true) }) it('accepts webhook update', () => { expect(updateBotSchema.safeParse({ ...base, webhookUrl: 'https://new.com/hook' }).success).toBe(true) }) it('rejects file:// webhook', () => { expect(updateBotSchema.safeParse({ ...base, webhookUrl: 'file:///etc/passwd' }).success).toBe(false) }) + // SECURITY REGRESSION: pubkey must never be a schema field here. POST + // /api/auth/update derives identity from the verified JWT + // (extractPubkeyFromAuth), not from client body — see server/src/routes/auth.ts. + // A pubkey field in this schema previously let an unauthenticated caller + // claim any bot as their own and hijack its webhook/customization. + it('does not declare a pubkey field (identity comes from the JWT, not the body)', () => { + expect('pubkey' in updateBotSchema.shape).toBe(false) + }) }) // --- Fight schemas --- diff --git a/server/src/lib/validators.ts b/server/src/lib/validators.ts index d53a703..2928ae1 100644 --- a/server/src/lib/validators.ts +++ b/server/src/lib/validators.ts @@ -42,8 +42,13 @@ export const registerHumanSchema = z.object({ avatarSeed: z.string().min(1).max(50).optional(), }) +// pubkey is intentionally NOT part of this schema: POST /api/auth/update +// derives the caller's identity from their verified JWT (extractPubkeyFromAuth), +// never from the request body — a client-supplied pubkey here would let any +// caller act as any other bot owner. Kept accepting-but-ignoring the field +// would be more confusing than just not declaring it; the frontend no longer +// sends it either. export const updateBotSchema = z.object({ - pubkey: pubkeySchema, webhookUrl: httpUrlSchema.max(2048).optional(), profilePicUrl: httpUrlSchema.max(2048).optional(), customization: z.record(z.string(), z.unknown()).optional().nullable(), diff --git a/server/src/routes/auth-update.test.ts b/server/src/routes/auth-update.test.ts new file mode 100644 index 0000000..bd4eb1a --- /dev/null +++ b/server/src/routes/auth-update.test.ts @@ -0,0 +1,111 @@ +import { describe, it, expect } from 'vitest' +import { Hono } from 'hono' +import { authRouter } from './auth.js' +import { generateSecretKey, getPublicKey } from 'nostr-tools' +import { createJwt } from '../middleware/jwt.js' +import { db, schema } from '../db/index.js' +import { eq } from 'drizzle-orm' +import { createHash, randomBytes } from 'crypto' +import { nanoid } from 'nanoid' + +const app = new Hono() +app.route('/api/auth', authRouter) + +async function seedBot(pubkey: string, overrides: Partial = {}) { + const id = overrides.id || nanoid(12) + const secret = randomBytes(32).toString('hex') + await db.insert(schema.bots).values({ + id, + name: overrides.name || `t${Date.now().toString(36).slice(-8)}`, + webhookUrl: overrides.webhookUrl || 'http://poll.local/', + avatarSeed: overrides.avatarSeed || 'seed', + archetype: overrides.archetype || 'standard', + secretHash: createHash('sha256').update(secret).digest('hex'), + publicKey: pubkey, + profilePicUrl: overrides.profilePicUrl ?? null, + customization: overrides.customization ?? null, + createdAt: overrides.createdAt || new Date().toISOString(), + }) + return id +} + +// SECURITY REGRESSION SUITE for POST /api/auth/update. +// +// This route previously trusted a client-supplied `pubkey` field in the +// request body with no verification against the caller's actual identity — +// any unauthenticated caller could pass a victim's pubkey and hijack their +// bot's webhook/profilePicUrl/customization. Found live during 09-06 +// (ai-config UI work) by contrast with GET /me and POST /regenerate-secret, +// which both correctly derive identity from the verified JWT via +// extractPubkeyFromAuth. Fixed to always derive pubkey from the JWT; the +// body no longer even has a pubkey field (see updateBotSchema). +describe('POST /api/auth/update — identity comes from the JWT, not the body', () => { + it('rejects an unauthenticated request (no Authorization header)', async () => { + const res = await app.request('/api/auth/update', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ customization: { archetype: 'tank' } }), + }) + expect(res.status).toBe(401) + }) + + it('rejects a garbage/tampered Bearer token', async () => { + const res = await app.request('/api/auth/update', { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: 'Bearer not-a-real-jwt' }, + body: JSON.stringify({ customization: { archetype: 'tank' } }), + }) + expect(res.status).toBe(401) + }) + + it('cannot hijack another bot by passing its pubkey in the body', async () => { + const victimSk = generateSecretKey() + const victimPk = getPublicKey(victimSk) + const victimId = await seedBot(victimPk, { webhookUrl: 'http://victim.local/original' }) + + // Attacker has their own valid session (their own JWT) but a DIFFERENT + // bot — no bot row at all, in this case. + const attackerPk = getPublicKey(generateSecretKey()) + const attackerToken = createJwt(attackerPk) + + const res = await app.request('/api/auth/update', { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${attackerToken}` }, + // Old exploit shape: claim to be the victim via a body field. + body: JSON.stringify({ pubkey: victimPk, webhookUrl: 'http://poll.local/attacker-controlled' }), + }) + + // Attacker has no bot of their own -> 404, NOT a successful update of + // the victim's bot. + expect(res.status).toBe(404) + + const rows = await db.select({ webhookUrl: schema.bots.webhookUrl }) + .from(schema.bots) + .where(eq(schema.bots.id, victimId)) + .limit(1) + expect(rows[0].webhookUrl).toBe('http://victim.local/original') + }) + + it('updates the caller\'s own bot, derived from their JWT, ignoring a body pubkey', async () => { + const sk = generateSecretKey() + const pk = getPublicKey(sk) + const id = await seedBot(pk, { webhookUrl: 'http://poll.local/' }) + const token = createJwt(pk) + + const someoneElsesPk = getPublicKey(generateSecretKey()) + const res = await app.request('/api/auth/update', { + method: 'POST', + headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${token}` }, + // Even if a stale client sends an unrelated pubkey, the server must + // still act on the JWT-derived identity, not this field. + body: JSON.stringify({ pubkey: someoneElsesPk, customization: { archetype: 'shark' } }), + }) + expect(res.status).toBe(200) + + const rows = await db.select({ customization: schema.bots.customization }) + .from(schema.bots) + .where(eq(schema.bots.id, id)) + .limit(1) + expect(JSON.parse(rows[0].customization || '{}').archetype).toBe('shark') + }) +}) diff --git a/server/src/routes/auth.ts b/server/src/routes/auth.ts index 3663e8c..73309ad 100644 --- a/server/src/routes/auth.ts +++ b/server/src/routes/auth.ts @@ -324,11 +324,25 @@ authRouter.post('/register-human', rateLimit(600_000, 10), async (c) => { // Update bot webhook and/or customization (requires pubkey match) authRouter.post('/update', rateLimit(60_000, 10), async (c) => { + // SECURITY: pubkey MUST come from the verified JWT, never from the request + // body. This handler previously trusted a client-supplied `pubkey` field + // with no cross-check against the caller's actual authenticated identity — + // any unauthenticated caller could POST an arbitrary victim's pubkey plus + // a malicious webhookUrl/profilePicUrl/customization and silently hijack + // that bot (e.g. redirect its webhook to an attacker-controlled endpoint). + // Found live during the ai-config UI work (09-06) by contrast with + // /regenerate-secret and GET /me, which both correctly derive pubkey from + // extractPubkeyFromAuth and never trust a client-claimed identity. + const pubkey = extractPubkeyFromAuth(c.req.header('Authorization')) + if (!pubkey) { + return c.json({ error: 'Authentication required.' }, 401) + } + const parsed = updateBotSchema.safeParse(await c.req.json().catch(() => ({}))) if (!parsed.success) { - return c.json({ error: 'Invalid pubkey.' }, 400) + return c.json({ error: 'Invalid request body.' }, 400) } - const { pubkey, webhookUrl, profilePicUrl, customization: rawCustomization } = parsed.data + const { webhookUrl, profilePicUrl, customization: rawCustomization } = parsed.data const rows = await db.select({ id: schema.bots.id, customization: schema.bots.customization }) .from(schema.bots)