feat: add security tests and userId injection protection
Security improvements: - Add comprehensive e2e tests for userId injection attacks - Explicitly strip userId from abacus-settings PATCH request body - Add security comments to player update routes - Tests verify foreign key and unique constraints prevent attacks - Document that API layer security is critical (DB constraints insufficient) Test coverage: - 12 tests for abacus-settings API (including 3 security tests) - 11 tests for players API (including 3 security tests) - All 23 tests passing Key findings documented in tests: - Database foreign keys prevent invalid userId references - Primary key constraints prevent duplicate userIds (abacus_settings) - For players, userId CAN be changed to another valid userId at DB level - API layer MUST filter userId from request body and use session-derived userId - WHERE clauses scope all queries to current user's data Defense in depth: 1. Session-derived userId (JWT cookie) 2. Explicit userId filtering from request body 3. WHERE clauses limiting scope to user's own data 4. Foreign key constraints (fallback) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This commit is contained in:
Thomas Hallock
parent
92ef1360a4
commit
aa1ad315ef
|
|
@ -0,0 +1,350 @@
|
|||
/**
|
||||
* @vitest-environment node
|
||||
*/
|
||||
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest'
|
||||
import { db, schema } from '../src/db'
|
||||
import { eq } from 'drizzle-orm'
|
||||
|
||||
/**
|
||||
* API Abacus Settings E2E Tests
|
||||
*
|
||||
* These tests verify the abacus-settings API endpoints work correctly.
|
||||
*/
|
||||
|
||||
describe('Abacus Settings API', () => {
|
||||
let testUserId: string
|
||||
let testGuestId: string
|
||||
|
||||
beforeEach(async () => {
|
||||
// Create a test user with unique guest ID
|
||||
testGuestId = `test-guest-${Date.now()}-${Math.random().toString(36).slice(2)}`
|
||||
const [user] = await db
|
||||
.insert(schema.users)
|
||||
.values({ guestId: testGuestId })
|
||||
.returning()
|
||||
testUserId = user.id
|
||||
})
|
||||
|
||||
afterEach(async () => {
|
||||
// Clean up: delete test user (cascade deletes settings)
|
||||
await db.delete(schema.users).where(eq(schema.users.id, testUserId))
|
||||
})
|
||||
|
||||
describe('GET /api/abacus-settings', () => {
|
||||
it('creates settings with defaults if none exist', async () => {
|
||||
const [settings] = await db
|
||||
.insert(schema.abacusSettings)
|
||||
.values({ userId: testUserId })
|
||||
.returning()
|
||||
|
||||
expect(settings).toBeDefined()
|
||||
expect(settings.colorScheme).toBe('place-value')
|
||||
expect(settings.beadShape).toBe('diamond')
|
||||
expect(settings.colorPalette).toBe('default')
|
||||
expect(settings.hideInactiveBeads).toBe(false)
|
||||
expect(settings.coloredNumerals).toBe(false)
|
||||
expect(settings.scaleFactor).toBe(1.0)
|
||||
expect(settings.showNumbers).toBe(true)
|
||||
expect(settings.animated).toBe(true)
|
||||
expect(settings.interactive).toBe(false)
|
||||
expect(settings.gestures).toBe(false)
|
||||
expect(settings.soundEnabled).toBe(true)
|
||||
expect(settings.soundVolume).toBe(0.8)
|
||||
})
|
||||
|
||||
it('returns existing settings', async () => {
|
||||
// Create settings
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: testUserId,
|
||||
colorScheme: 'monochrome',
|
||||
beadShape: 'circle',
|
||||
soundEnabled: false,
|
||||
soundVolume: 0.5,
|
||||
})
|
||||
|
||||
const settings = await db.query.abacusSettings.findFirst({
|
||||
where: eq(schema.abacusSettings.userId, testUserId),
|
||||
})
|
||||
|
||||
expect(settings).toBeDefined()
|
||||
expect(settings?.colorScheme).toBe('monochrome')
|
||||
expect(settings?.beadShape).toBe('circle')
|
||||
expect(settings?.soundEnabled).toBe(false)
|
||||
expect(settings?.soundVolume).toBe(0.5)
|
||||
})
|
||||
})
|
||||
|
||||
describe('PATCH /api/abacus-settings', () => {
|
||||
it('creates new settings if none exist', async () => {
|
||||
const [settings] = await db
|
||||
.insert(schema.abacusSettings)
|
||||
.values({
|
||||
userId: testUserId,
|
||||
soundEnabled: false,
|
||||
})
|
||||
.returning()
|
||||
|
||||
expect(settings).toBeDefined()
|
||||
expect(settings.soundEnabled).toBe(false)
|
||||
})
|
||||
|
||||
it('updates existing settings', async () => {
|
||||
// Create initial settings
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: testUserId,
|
||||
colorScheme: 'place-value',
|
||||
beadShape: 'diamond',
|
||||
})
|
||||
|
||||
// Update
|
||||
const [updated] = await db
|
||||
.update(schema.abacusSettings)
|
||||
.set({
|
||||
colorScheme: 'heaven-earth',
|
||||
beadShape: 'square',
|
||||
})
|
||||
.where(eq(schema.abacusSettings.userId, testUserId))
|
||||
.returning()
|
||||
|
||||
expect(updated.colorScheme).toBe('heaven-earth')
|
||||
expect(updated.beadShape).toBe('square')
|
||||
})
|
||||
|
||||
it('updates only provided fields', async () => {
|
||||
// Create initial settings
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: testUserId,
|
||||
colorScheme: 'place-value',
|
||||
soundEnabled: true,
|
||||
soundVolume: 0.8,
|
||||
})
|
||||
|
||||
// Update only soundEnabled
|
||||
const [updated] = await db
|
||||
.update(schema.abacusSettings)
|
||||
.set({ soundEnabled: false })
|
||||
.where(eq(schema.abacusSettings.userId, testUserId))
|
||||
.returning()
|
||||
|
||||
expect(updated.soundEnabled).toBe(false)
|
||||
expect(updated.colorScheme).toBe('place-value') // unchanged
|
||||
expect(updated.soundVolume).toBe(0.8) // unchanged
|
||||
})
|
||||
|
||||
it('prevents setting invalid userId via foreign key constraint', async () => {
|
||||
// Create initial settings
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: testUserId,
|
||||
})
|
||||
|
||||
// Try to update with invalid userId - should fail
|
||||
await expect(async () => {
|
||||
await db
|
||||
.update(schema.abacusSettings)
|
||||
.set({
|
||||
userId: 'HACKER_ID_INVALID',
|
||||
soundEnabled: false,
|
||||
})
|
||||
.where(eq(schema.abacusSettings.userId, testUserId))
|
||||
}).rejects.toThrow()
|
||||
})
|
||||
|
||||
it('allows updating all display settings', async () => {
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: testUserId,
|
||||
})
|
||||
|
||||
const [updated] = await db
|
||||
.update(schema.abacusSettings)
|
||||
.set({
|
||||
colorScheme: 'alternating',
|
||||
beadShape: 'circle',
|
||||
colorPalette: 'colorblind',
|
||||
hideInactiveBeads: true,
|
||||
coloredNumerals: true,
|
||||
scaleFactor: 1.5,
|
||||
showNumbers: false,
|
||||
animated: false,
|
||||
interactive: true,
|
||||
gestures: true,
|
||||
soundEnabled: false,
|
||||
soundVolume: 0.3,
|
||||
})
|
||||
.where(eq(schema.abacusSettings.userId, testUserId))
|
||||
.returning()
|
||||
|
||||
expect(updated.colorScheme).toBe('alternating')
|
||||
expect(updated.beadShape).toBe('circle')
|
||||
expect(updated.colorPalette).toBe('colorblind')
|
||||
expect(updated.hideInactiveBeads).toBe(true)
|
||||
expect(updated.coloredNumerals).toBe(true)
|
||||
expect(updated.scaleFactor).toBe(1.5)
|
||||
expect(updated.showNumbers).toBe(false)
|
||||
expect(updated.animated).toBe(false)
|
||||
expect(updated.interactive).toBe(true)
|
||||
expect(updated.gestures).toBe(true)
|
||||
expect(updated.soundEnabled).toBe(false)
|
||||
expect(updated.soundVolume).toBe(0.3)
|
||||
})
|
||||
})
|
||||
|
||||
describe('Cascade delete behavior', () => {
|
||||
it('deletes settings when user is deleted', async () => {
|
||||
// Create settings
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: testUserId,
|
||||
soundEnabled: false,
|
||||
})
|
||||
|
||||
// Verify settings exist
|
||||
let settings = await db.query.abacusSettings.findFirst({
|
||||
where: eq(schema.abacusSettings.userId, testUserId),
|
||||
})
|
||||
expect(settings).toBeDefined()
|
||||
|
||||
// Delete user
|
||||
await db.delete(schema.users).where(eq(schema.users.id, testUserId))
|
||||
|
||||
// Verify settings are gone
|
||||
settings = await db.query.abacusSettings.findFirst({
|
||||
where: eq(schema.abacusSettings.userId, testUserId),
|
||||
})
|
||||
expect(settings).toBeUndefined()
|
||||
})
|
||||
})
|
||||
|
||||
describe('Data isolation', () => {
|
||||
it('ensures settings are isolated per user', async () => {
|
||||
// Create another user
|
||||
const testGuestId2 = `test-guest-2-${Date.now()}-${Math.random().toString(36).slice(2)}`
|
||||
const [user2] = await db
|
||||
.insert(schema.users)
|
||||
.values({ guestId: testGuestId2 })
|
||||
.returning()
|
||||
|
||||
try {
|
||||
// Create settings for both users
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: testUserId,
|
||||
colorScheme: 'monochrome',
|
||||
})
|
||||
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: user2.id,
|
||||
colorScheme: 'place-value',
|
||||
})
|
||||
|
||||
// Verify isolation
|
||||
const settings1 = await db.query.abacusSettings.findFirst({
|
||||
where: eq(schema.abacusSettings.userId, testUserId),
|
||||
})
|
||||
const settings2 = await db.query.abacusSettings.findFirst({
|
||||
where: eq(schema.abacusSettings.userId, user2.id),
|
||||
})
|
||||
|
||||
expect(settings1?.colorScheme).toBe('monochrome')
|
||||
expect(settings2?.colorScheme).toBe('place-value')
|
||||
} finally {
|
||||
// Clean up second user
|
||||
await db.delete(schema.users).where(eq(schema.users.id, user2.id))
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
describe('Security: userId injection prevention', () => {
|
||||
it('rejects attempts to update settings with non-existent userId', async () => {
|
||||
// Create initial settings
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: testUserId,
|
||||
soundEnabled: true,
|
||||
})
|
||||
|
||||
// Attempt to inject a fake userId
|
||||
await expect(async () => {
|
||||
await db
|
||||
.update(schema.abacusSettings)
|
||||
.set({
|
||||
userId: 'HACKER_ID_NON_EXISTENT',
|
||||
soundEnabled: false,
|
||||
})
|
||||
.where(eq(schema.abacusSettings.userId, testUserId))
|
||||
}).rejects.toThrow(/FOREIGN KEY constraint failed/)
|
||||
})
|
||||
|
||||
it('prevents modifying another user\'s settings via userId injection', async () => {
|
||||
// Create victim user
|
||||
const victimGuestId = `victim-${Date.now()}-${Math.random().toString(36).slice(2)}`
|
||||
const [victimUser] = await db
|
||||
.insert(schema.users)
|
||||
.values({ guestId: victimGuestId })
|
||||
.returning()
|
||||
|
||||
try {
|
||||
// Create settings for both users
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: testUserId,
|
||||
colorScheme: 'monochrome',
|
||||
soundEnabled: true,
|
||||
})
|
||||
|
||||
await db.insert(schema.abacusSettings).values({
|
||||
userId: victimUser.id,
|
||||
colorScheme: 'place-value',
|
||||
soundEnabled: true,
|
||||
})
|
||||
|
||||
// Attacker tries to change userId to victim's ID
|
||||
// This is rejected because userId is PRIMARY KEY (UNIQUE constraint)
|
||||
await expect(async () => {
|
||||
await db
|
||||
.update(schema.abacusSettings)
|
||||
.set({
|
||||
userId: victimUser.id, // Trying to inject victim's ID
|
||||
soundEnabled: false,
|
||||
})
|
||||
.where(eq(schema.abacusSettings.userId, testUserId))
|
||||
}).rejects.toThrow(/UNIQUE constraint failed/)
|
||||
|
||||
// Verify victim's settings are unchanged
|
||||
const victimSettings = await db.query.abacusSettings.findFirst({
|
||||
where: eq(schema.abacusSettings.userId, victimUser.id),
|
||||
})
|
||||
expect(victimSettings?.soundEnabled).toBe(true)
|
||||
expect(victimSettings?.colorScheme).toBe('place-value')
|
||||
} finally {
|
||||
await db.delete(schema.users).where(eq(schema.users.id, victimUser.id))
|
||||
}
|
||||
})
|
||||
|
||||
it('prevents creating settings for another user via userId injection', async () => {
|
||||
// Create victim user
|
||||
const victimGuestId = `victim-${Date.now()}-${Math.random().toString(36).slice(2)}`
|
||||
const [victimUser] = await db
|
||||
.insert(schema.users)
|
||||
.values({ guestId: victimGuestId })
|
||||
.returning()
|
||||
|
||||
try {
|
||||
// Try to create settings for victim with attacker's data
|
||||
// This will succeed because foreign key exists, but in the real API
|
||||
// the userId comes from session, not request body
|
||||
const [maliciousSettings] = await db
|
||||
.insert(schema.abacusSettings)
|
||||
.values({
|
||||
userId: victimUser.id,
|
||||
colorScheme: 'alternating', // Attacker's preference
|
||||
})
|
||||
.returning()
|
||||
|
||||
// This test shows that at the DB level, we CAN insert for any valid userId
|
||||
// The security comes from the API layer filtering userId from request body
|
||||
// and deriving it from the session cookie instead
|
||||
expect(maliciousSettings.userId).toBe(victimUser.id)
|
||||
expect(maliciousSettings.colorScheme).toBe('alternating')
|
||||
} finally {
|
||||
await db.delete(schema.users).where(eq(schema.users.id, victimUser.id))
|
||||
}
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
@ -219,4 +219,115 @@ describe('Players API', () => {
|
|||
expect(players).toHaveLength(0)
|
||||
})
|
||||
})
|
||||
|
||||
describe('Security: userId injection prevention', () => {
|
||||
it('rejects creating player with non-existent userId', async () => {
|
||||
// Attempt to create a player with a fake userId
|
||||
await expect(async () => {
|
||||
await db.insert(schema.players).values({
|
||||
userId: 'HACKER_ID_NON_EXISTENT',
|
||||
name: 'Hacker Player',
|
||||
emoji: '🦹',
|
||||
color: '#ff0000',
|
||||
})
|
||||
}).rejects.toThrow(/FOREIGN KEY constraint failed/)
|
||||
})
|
||||
|
||||
it('prevents modifying another user\'s player via userId injection (DB layer alone is insufficient)', async () => {
|
||||
// Create victim user and their player
|
||||
const victimGuestId = `victim-${Date.now()}-${Math.random().toString(36).slice(2)}`
|
||||
const [victimUser] = await db
|
||||
.insert(schema.users)
|
||||
.values({ guestId: victimGuestId })
|
||||
.returning()
|
||||
|
||||
try {
|
||||
// Create attacker's player
|
||||
const [attackerPlayer] = await db
|
||||
.insert(schema.players)
|
||||
.values({
|
||||
userId: testUserId,
|
||||
name: 'Attacker Player',
|
||||
emoji: '😈',
|
||||
color: '#ff0000',
|
||||
})
|
||||
.returning()
|
||||
|
||||
const [victimPlayer] = await db
|
||||
.insert(schema.players)
|
||||
.values({
|
||||
userId: victimUser.id,
|
||||
name: 'Victim Player',
|
||||
emoji: '👤',
|
||||
color: '#00ff00',
|
||||
isActive: true,
|
||||
})
|
||||
.returning()
|
||||
|
||||
// IMPORTANT: At the DB level, changing userId to another valid userId SUCCEEDS
|
||||
// This is why API layer MUST filter userId from request body!
|
||||
const [updated] = await db
|
||||
.update(schema.players)
|
||||
.set({
|
||||
userId: victimUser.id, // This WILL succeed at DB level!
|
||||
name: 'Stolen Player',
|
||||
})
|
||||
.where(eq(schema.players.id, attackerPlayer.id))
|
||||
.returning()
|
||||
|
||||
// The update succeeded - the player now belongs to victim!
|
||||
expect(updated.userId).toBe(victimUser.id)
|
||||
expect(updated.name).toBe('Stolen Player')
|
||||
|
||||
// This test demonstrates why the API route MUST:
|
||||
// 1. Strip userId from request body
|
||||
// 2. Derive userId from session cookie
|
||||
// 3. Use WHERE clause to scope updates to current user's data only
|
||||
} finally {
|
||||
await db.delete(schema.users).where(eq(schema.users.id, victimUser.id))
|
||||
}
|
||||
})
|
||||
|
||||
it('ensures players are isolated per user', async () => {
|
||||
// Create another user
|
||||
const user2GuestId = `user2-${Date.now()}-${Math.random().toString(36).slice(2)}`
|
||||
const [user2] = await db
|
||||
.insert(schema.users)
|
||||
.values({ guestId: user2GuestId })
|
||||
.returning()
|
||||
|
||||
try {
|
||||
// Create players for both users
|
||||
await db.insert(schema.players).values({
|
||||
userId: testUserId,
|
||||
name: 'User 1 Player',
|
||||
emoji: '🎮',
|
||||
color: '#0000ff',
|
||||
})
|
||||
|
||||
await db.insert(schema.players).values({
|
||||
userId: user2.id,
|
||||
name: 'User 2 Player',
|
||||
emoji: '🎯',
|
||||
color: '#ff00ff',
|
||||
})
|
||||
|
||||
// Verify each user only sees their own players
|
||||
const user1Players = await db.query.players.findMany({
|
||||
where: eq(schema.players.userId, testUserId),
|
||||
})
|
||||
const user2Players = await db.query.players.findMany({
|
||||
where: eq(schema.players.userId, user2.id),
|
||||
})
|
||||
|
||||
expect(user1Players).toHaveLength(1)
|
||||
expect(user1Players[0].name).toBe('User 1 Player')
|
||||
|
||||
expect(user2Players).toHaveLength(1)
|
||||
expect(user2Players[0].name).toBe('User 2 Player')
|
||||
} finally {
|
||||
await db.delete(schema.users).where(eq(schema.users.id, user2.id))
|
||||
}
|
||||
})
|
||||
})
|
||||
})
|
||||
|
|
|
|||
|
|
@ -45,6 +45,10 @@ export async function PATCH(req: NextRequest) {
|
|||
try {
|
||||
const viewerId = await getViewerId()
|
||||
const body = await req.json()
|
||||
|
||||
// Security: Strip userId from request body - it must come from session only
|
||||
const { userId: _, ...updates } = body
|
||||
|
||||
const user = await getOrCreateUser(viewerId)
|
||||
|
||||
// Ensure settings exist
|
||||
|
|
@ -56,7 +60,7 @@ export async function PATCH(req: NextRequest) {
|
|||
// Create new settings with updates
|
||||
const [newSettings] = await db
|
||||
.insert(schema.abacusSettings)
|
||||
.values({ userId: user.id, ...body })
|
||||
.values({ userId: user.id, ...updates })
|
||||
.returning()
|
||||
return NextResponse.json({ settings: newSettings })
|
||||
}
|
||||
|
|
@ -64,7 +68,7 @@ export async function PATCH(req: NextRequest) {
|
|||
// Update existing settings
|
||||
const [updatedSettings] = await db
|
||||
.update(schema.abacusSettings)
|
||||
.set(body)
|
||||
.set(updates)
|
||||
.where(eq(schema.abacusSettings.userId, user.id))
|
||||
.returning()
|
||||
|
||||
|
|
|
|||
|
|
@ -27,6 +27,7 @@ export async function PATCH(
|
|||
)
|
||||
}
|
||||
|
||||
// Security: Only allow updating specific fields (excludes userId)
|
||||
// Update player (only if it belongs to this user)
|
||||
const [updatedPlayer] = await db
|
||||
.update(schema.players)
|
||||
|
|
@ -35,6 +36,7 @@ export async function PATCH(
|
|||
...(body.emoji !== undefined && { emoji: body.emoji }),
|
||||
...(body.color !== undefined && { color: body.color }),
|
||||
...(body.isActive !== undefined && { isActive: body.isActive }),
|
||||
// userId is explicitly NOT included - it comes from session
|
||||
})
|
||||
.where(
|
||||
and(
|
||||
|
|
|
|||
Loading…
Reference in New Issue