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)