-
Notifications
You must be signed in to change notification settings - Fork 0
fix: auth security — bcrypt, JWT from env, rate limiting, zod, localStorage #8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
ebc760d
db29d45
eb36dff
aed69e4
594dab5
c456062
c772f7f
35b2dc3
74a01c8
0f39b6b
c37084b
209cc41
94e7e42
db2936d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| PORT=3001 | ||
| JWT_SECRET=replace-with-a-long-random-string-at-least-32-chars | ||
| DATABASE_PATH=./stagepass.db | ||
| NODE_ENV=development |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| import 'dotenv/config'; | ||
| import express from 'express'; | ||
| import cors from 'cors'; | ||
| import moviesRouter from './routes/movies'; | ||
| import showtimesRouter from './routes/showtimes'; | ||
| import seatsRouter from './routes/seats'; | ||
| import bookingsRouter from './routes/bookings'; | ||
| import authRouter from './routes/auth'; | ||
|
|
||
| const app = express(); | ||
|
|
||
| app.use(cors()); | ||
| app.use(express.json()); | ||
|
|
||
| app.use('/api/movies', moviesRouter); | ||
| app.use('/api/showtimes', showtimesRouter); | ||
| app.use('/api/seats', seatsRouter); | ||
| app.use('/api/bookings', bookingsRouter); | ||
| app.use('/api/auth', authRouter); | ||
|
|
||
| export default app; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| /** | ||
| * Integration tests — full signup → login → protected route flow. | ||
| * Uses an in-memory SQLite database so no file I/O is needed. | ||
| */ | ||
| import { describe, it, expect, vi } from 'vitest'; | ||
| import request from 'supertest'; | ||
|
|
||
| // Set JWT_SECRET before any module loads | ||
| process.env.JWT_SECRET = 'integration-test-secret'; | ||
| process.env.NODE_ENV = 'test'; | ||
|
|
||
| // Create an in-memory db with the full schema before anything imports the real db | ||
| vi.mock('./db', async () => { | ||
| const Database = (await import('better-sqlite3')).default; | ||
| const db = new Database(':memory:'); | ||
| db.exec(` | ||
| CREATE TABLE IF NOT EXISTS users ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| name TEXT NOT NULL, | ||
| email TEXT UNIQUE NOT NULL, | ||
| password TEXT NOT NULL | ||
| ); | ||
| `); | ||
| return { default: db }; | ||
| }); | ||
|
|
||
| // Import app AFTER the mock is in place | ||
| const { default: app } = await import('./app'); | ||
|
|
||
| describe('auth integration — signup → login → protected route', () => { | ||
| const testUser = { | ||
| name: 'Test User', | ||
| email: 'test@example.com', | ||
| // eslint-disable-next-line sonarjs/no-hardcoded-passwords | ||
| password: 'password123', | ||
| }; | ||
|
|
||
| it('POST /api/auth/signup creates a user and returns a JWT', async () => { | ||
| const res = await request(app).post('/api/auth/signup').send(testUser); | ||
| expect(res.status).toBe(201); | ||
| expect(res.body.token).toBeTruthy(); | ||
| expect(res.body.user.email).toBe(testUser.email); | ||
| }); | ||
|
|
||
| it('POST /api/auth/login succeeds with correct credentials', async () => { | ||
| const res = await request(app).post('/api/auth/login').send({ | ||
| email: testUser.email, | ||
| password: testUser.password, | ||
| }); | ||
| expect(res.status).toBe(200); | ||
| expect(res.body.token).toBeTruthy(); | ||
| }); | ||
|
|
||
| it('POST /api/auth/login returns 401 with wrong password', async () => { | ||
| const res = await request(app).post('/api/auth/login').send({ | ||
| email: testUser.email, | ||
| password: 'wrongpassword', // eslint-disable-line sonarjs/no-hardcoded-passwords | ||
| }); | ||
| expect(res.status).toBe(401); | ||
| }); | ||
|
|
||
| it('stored password is hashed — not the plain text value', async () => { | ||
| // Access the mocked db | ||
| const { default: db } = await import('./db'); | ||
| const user = db.prepare('SELECT password FROM users WHERE email = ?').get(testUser.email) as { password: string }; | ||
| expect(user.password).not.toBe(testUser.password); | ||
| expect(user.password).toMatch(/^\$2[ab]\$/); // bcrypt hash prefix | ||
| }); | ||
|
|
||
| it('POST /api/auth/signup returns 400 for duplicate email', async () => { | ||
| const res = await request(app).post('/api/auth/signup').send(testUser); | ||
| expect(res.status).toBe(400); | ||
| expect(res.body.error).toMatch(/email already exists/i); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,72 @@ | ||
| /** | ||
| * Unit tests for auth middleware (authenticateToken). | ||
| * The db and bcrypt are mocked so these run without a real database. | ||
| */ | ||
| import { describe, it, expect, vi } from 'vitest'; | ||
| import { Request, Response, NextFunction } from 'express'; | ||
| import jwt from 'jsonwebtoken'; | ||
|
|
||
| process.env.JWT_SECRET = 'unit-test-secret'; | ||
| process.env.NODE_ENV = 'test'; | ||
|
|
||
| vi.mock('./db', () => ({ | ||
| default: { | ||
| prepare: vi.fn().mockReturnValue({ get: vi.fn(), run: vi.fn() }), | ||
| }, | ||
| })); | ||
|
|
||
| const { authenticateToken } = await import('./routes/auth'); | ||
|
|
||
| function makeReqRes() { | ||
| const req = { headers: {} } as unknown as Request & { userId?: number }; | ||
| const json = vi.fn(); | ||
| const status = vi.fn().mockReturnValue({ json }); | ||
| const res = { status, json } as unknown as Response; | ||
| const next = vi.fn() as NextFunction; | ||
| return { req, res, status, json, next }; | ||
| } | ||
|
|
||
| describe('authenticateToken middleware', () => { | ||
| it('calls next() with userId set when token is valid', () => { | ||
| const token = jwt.sign({ userId: 42 }, 'unit-test-secret', { expiresIn: '1h' }); | ||
| const { req, res, next } = makeReqRes(); | ||
| req.headers['authorization'] = `Bearer ${token}`; | ||
|
|
||
| authenticateToken(req, res, next); | ||
|
|
||
| expect(next).toHaveBeenCalledOnce(); | ||
| expect(req.userId).toBe(42); | ||
| }); | ||
|
|
||
| it('returns 401 when no token is provided', () => { | ||
| const { req, res, next, status, json } = makeReqRes(); | ||
|
|
||
| authenticateToken(req, res, next); | ||
|
|
||
| expect(status).toHaveBeenCalledWith(401); | ||
| expect(json).toHaveBeenCalledWith({ error: 'No token provided' }); | ||
| expect(next).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('returns 403 when token is invalid', () => { | ||
| const { req, res, next, status, json } = makeReqRes(); | ||
| req.headers['authorization'] = 'Bearer bad.token.here'; | ||
|
|
||
| authenticateToken(req, res, next); | ||
|
|
||
| expect(status).toHaveBeenCalledWith(403); | ||
| expect(json).toHaveBeenCalledWith({ error: 'Invalid token' }); | ||
| expect(next).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('returns 403 when token is signed with a different secret', () => { | ||
| const token = jwt.sign({ userId: 1 }, 'wrong-secret'); | ||
| const { req, res, next, status } = makeReqRes(); | ||
| req.headers['authorization'] = `Bearer ${token}`; | ||
|
|
||
| authenticateToken(req, res, next); | ||
|
|
||
| expect(status).toHaveBeenCalledWith(403); | ||
| expect(next).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -1,52 +1,119 @@ | ||||||
| import { Router, Request, Response, NextFunction } from 'express'; | ||||||
| import jwt from 'jsonwebtoken'; | ||||||
| import bcrypt from 'bcryptjs'; | ||||||
| import rateLimit from 'express-rate-limit'; | ||||||
| import { z } from 'zod'; | ||||||
| import db from '../db'; | ||||||
|
|
||||||
| const router = Router(); | ||||||
| const JWT_SECRET = 'stagepass-secret-key-not-secure'; | ||||||
|
|
||||||
| const authLimiter = rateLimit({ | ||||||
| windowMs: 15 * 60 * 1000, // 15 minutes | ||||||
| max: 20, | ||||||
| message: { error: 'Too many requests, please try again later' }, | ||||||
| standardHeaders: true, | ||||||
| legacyHeaders: false, | ||||||
| }); | ||||||
|
|
||||||
| router.use(authLimiter); | ||||||
|
|
||||||
| const loginSchema = z.object({ | ||||||
| // eslint-disable-next-line sonarjs/deprecation | ||||||
| email: z.string().email(), | ||||||
| password: z.string().min(1), | ||||||
| }); | ||||||
|
|
||||||
| const signupSchema = z.object({ | ||||||
| name: z.string().min(1), | ||||||
| // eslint-disable-next-line sonarjs/deprecation | ||||||
| email: z.string().email(), | ||||||
| password: z.string().min(8), | ||||||
| }); | ||||||
|
|
||||||
| interface UserRow { | ||||||
| id: number; | ||||||
| name: string; | ||||||
| email: string; | ||||||
| password: string; | ||||||
| } | ||||||
|
|
||||||
| interface JwtPayload { | ||||||
| userId: number; | ||||||
| } | ||||||
|
|
||||||
| function getJwtSecret(): string { | ||||||
| const secret = process.env.JWT_SECRET; | ||||||
| if (!secret) throw new Error('JWT_SECRET environment variable is not set'); | ||||||
| return secret; | ||||||
| } | ||||||
|
|
||||||
| // A dummy hash used when user is not found to prevent timing attacks. | ||||||
| // bcrypt.compare will still run, making the response time consistent. | ||||||
| const DUMMY_HASH = '$2b$10$abcdefghijklmnopqrstuvuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu'; | ||||||
|
|
||||||
| // POST login | ||||||
| router.post('/login', (req: Request, res: Response) => { | ||||||
| const { email, password } = req.body; | ||||||
| router.post('/login', async (req: Request, res: Response) => { | ||||||
| const parsed = loginSchema.safeParse(req.body); | ||||||
| if (!parsed.success) { | ||||||
| return res.status(400).json({ errors: parsed.error.issues.map((i) => i.message) }); | ||||||
| } | ||||||
| const { email, password } = parsed.data; | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
Suggested change
|
||||||
|
|
||||||
| const user = db.prepare('SELECT * FROM users WHERE email = ?').get(email) as UserRow | undefined; | ||||||
|
|
||||||
| const user = db.prepare('SELECT * FROM users WHERE email = ? AND password = ?').get(email, password) as any; | ||||||
| // Always compare to prevent timing attacks (constant-time response) | ||||||
| const hashToCompare = user?.password ?? DUMMY_HASH; | ||||||
| const passwordMatch = await bcrypt.compare(password, hashToCompare); | ||||||
|
|
||||||
| if (!user) { | ||||||
| if (!user || !passwordMatch) { | ||||||
| return res.status(401).json({ error: 'Invalid credentials' }); | ||||||
| } | ||||||
|
|
||||||
| const token = jwt.sign({ userId: user.id }, JWT_SECRET, { expiresIn: '24h' }); | ||||||
| const token = jwt.sign({ userId: user.id }, getJwtSecret(), { expiresIn: '24h' }); | ||||||
| res.json({ token, user: { id: user.id, name: user.name, email: user.email } }); | ||||||
| }); | ||||||
|
|
||||||
| // POST signup | ||||||
| router.post('/signup', (req: Request, res: Response) => { | ||||||
| const { name, email, password } = req.body; | ||||||
| router.post('/signup', async (req: Request, res: Response) => { | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The current implementation only returns the first validation error message. For a better user experience, it's recommended to return all validation errors. This allows the frontend to display all issues with the submitted form at once. if (!parsed.success) return res.status(400).json({ errors: parsed.error.flatten() }); |
||||||
| const parsed = signupSchema.safeParse(req.body); | ||||||
| if (!parsed.success) { | ||||||
| return res.status(400).json({ errors: parsed.error.issues.map((i) => i.message) }); | ||||||
| } | ||||||
| const { name, email, password } = parsed.data; | ||||||
|
|
||||||
| try { | ||||||
| const result = db.prepare('INSERT INTO users (name, email, password) VALUES (?, ?, ?)').run(name, email, password); | ||||||
| const token = jwt.sign({ userId: result.lastInsertRowid }, JWT_SECRET, { expiresIn: '24h' }); | ||||||
| const hashed = await bcrypt.hash(password, 10); | ||||||
| const result = db | ||||||
| .prepare('INSERT INTO users (name, email, password) VALUES (?, ?, ?)') | ||||||
| .run(name, email, hashed); | ||||||
| const token = jwt.sign({ userId: result.lastInsertRowid }, getJwtSecret(), { | ||||||
| expiresIn: '24h', | ||||||
| }); | ||||||
| res.status(201).json({ | ||||||
| token, | ||||||
| user: { id: result.lastInsertRowid, name, email } | ||||||
| user: { id: result.lastInsertRowid, name, email }, | ||||||
| }); | ||||||
| } catch (e: any) { | ||||||
| if (e.message.includes('UNIQUE')) { | ||||||
| } catch (e: unknown) { | ||||||
| if (e instanceof Error && e.message.includes('UNIQUE')) { | ||||||
| return res.status(400).json({ error: 'Email already exists' }); | ||||||
| } | ||||||
| return res.status(500).json({ error: 'Something went wrong' }); | ||||||
| } | ||||||
| }); | ||||||
|
|
||||||
| // Middleware | ||||||
| export function authenticateToken(req: any, res: Response, next: NextFunction) { | ||||||
| export function authenticateToken( | ||||||
| req: Request & { userId?: number }, | ||||||
| res: Response, | ||||||
| next: NextFunction | ||||||
| ) { | ||||||
| const authHeader = req.headers['authorization']; | ||||||
| const token = authHeader && authHeader.split(' ')[1]; | ||||||
| const token = authHeader?.split(' ')[1]; | ||||||
|
|
||||||
| if (!token) return res.status(401).json({ error: 'No token provided' }); | ||||||
|
|
||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||||
| try { | ||||||
| const decoded = jwt.verify(token, JWT_SECRET) as any; | ||||||
| const decoded = jwt.verify(token, getJwtSecret()) as JwtPayload; | ||||||
| req.userId = decoded.userId; | ||||||
| next(); | ||||||
| } catch { | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current implementation only returns the first validation error message. For a better user experience, it's recommended to return all validation errors. This allows the frontend to display all issues with the submitted form at once. This also applies to the signup route on line 56.