fix: auth security — bcrypt, JWT from env, rate limiting, zod, localStorage#8
Conversation
Installs packages needed for Phase 2d security fixes and server tests: bcryptjs — password hashing, dotenv — env config, express-rate-limit — auth rate limiting, zod — request validation, vitest + supertest — tests.
Adds .env.example documenting all required env vars. Server now loads .env via dotenv/config on startup and reads PORT from the environment instead of hardcoding 3001.
- JWT_SECRET is now read from process.env; throws on startup if unset - Signup hashes password with bcrypt (cost factor 10) before storing - Login fetches user by email only, then compares with bcrypt.compare - Plain text password comparison in SQL query is gone
20 requests per 15-minute window per IP. Mitigates credential stuffing and brute-force attacks on login and signup endpoints.
- Login: email must be valid, password must be non-empty - Signup: name required, email valid, password min 8 chars - Invalid bodies get a 400 with the first validation error message
- Exits with error code 1 if NODE_ENV === 'production' - Seed users now have bcrypt-hashed passwords (cost 10) so they work with the updated login endpoint
- login and signup write token + user to localStorage - logout removes both keys - useEffect on mount reads from localStorage to restore session so a page refresh does not log the user out
Separates app creation from server startup. index.ts now just calls .listen(). Tests can import app.ts directly without binding a port.
Login and signup now write token + user to localStorage. Logout clears both. A useEffect on mount reads them back so the session survives a page reload.
Seed users now have bcrypt-hashed passwords (cost 10) so they work with the new bcrypt.compare() login check. Adds a NODE_ENV=production guard that exits with an error if someone tries to seed a live database.
auth.test.ts — 4 unit tests for authenticateToken middleware (valid token, missing token, invalid token, wrong secret) auth.integration.test.ts — 5 integration tests against in-memory SQLite: signup returns JWT, login succeeds, wrong password 401, password is hashed in DB, duplicate email 400 AuthContext.test.tsx — 3 unit tests: login persists to localStorage, logout clears localStorage, page reload rehydrates from localStorage
Summary of ChangesHello @alichherawalla, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the security posture of the authentication system by introducing robust measures such as password hashing, environment-based secret management, and request rate limiting. It also improves data integrity through schema validation and provides a more seamless user experience by persisting authentication state. The changes also lay the groundwork for more reliable testing by refactoring the server application structure. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
… password warnings
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a major security and robustness upgrade for the authentication system. It correctly introduces password hashing with bcrypt, moves secrets to environment variables, adds rate limiting, and implements input validation with Zod. The code is also refactored for better testability, with comprehensive new tests added. My review focuses on further hardening security against timing attacks, improving type safety by removing as any casts, enhancing error handling and logging, and improving code maintainability through deduplication.
| const user = db.prepare('SELECT * FROM users WHERE email = ?').get(email) as any; | ||
|
|
||
| if (!user) { | ||
| if (!user || !(await bcrypt.compare(password, user.password))) { |
There was a problem hiding this comment.
This implementation is vulnerable to a timing attack. Because the bcrypt.compare function is only called when a user is found, an attacker can measure the response time to determine if a username exists in the database. To mitigate this, you should ensure that bcrypt.compare is always called, regardless of whether the user was found. You can do this by comparing against a dummy hash if the user does not exist.
// To mitigate timing attacks, always run bcrypt.compare to ensure constant-time responses.
// If user is not found, compare against a dummy hash. The dummy hash should have the same cost factor.
const hash = user ? user.password : (process.env.DUMMY_HASH || '$2a$10$invalidhashxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx');
const match = await bcrypt.compare(password, hash);
if (!user || !match) {
return res.status(401).json({ error: 'Invalid credentials' });
}| // POST login | ||
| router.post('/login', (req: Request, res: Response) => { | ||
| const { email, password } = req.body; | ||
| router.post('/login', async (req: Request, res: Response) => { |
There was a problem hiding this comment.
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.
| router.post('/login', async (req: Request, res: Response) => { | |
| if (!parsed.success) return res.status(400).json({ errors: parsed.error.flatten() }); |
| router.post('/login', async (req: Request, res: Response) => { | ||
| const parsed = loginSchema.safeParse(req.body); | ||
| if (!parsed.success) return res.status(400).json({ error: parsed.error.issues[0].message }); | ||
| const { email, password } = parsed.data; |
There was a problem hiding this comment.
Using as any bypasses TypeScript's type safety and should be avoided. It's better to define an interface for the expected object shape and cast to that. This improves code readability and maintainability, and helps catch bugs at compile time. A similar issue exists on line 83 for the decoded JWT payload.
| const { email, password } = parsed.data; | |
| const user = db.prepare('SELECT * FROM users WHERE email = ?').get(email) as { id: number; name: string; email: string; password: string } | undefined; |
| // 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.
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() });| @@ -46,7 +82,7 @@ export function authenticateToken(req: any, res: Response, next: NextFunction) { | |||
| if (!token) return res.status(401).json({ error: 'No token provided' }); | |||
|
|
|||
There was a problem hiding this comment.
Using as any bypasses TypeScript's type safety. It's better to define an interface for the decoded JWT payload and cast to that specific type.
const decoded = jwt.verify(token, getJwtSecret()) as { userId: number };| }); | ||
| if (!res.ok) throw new Error('Login failed'); | ||
| const data = await res.json(); | ||
| const data = await res.json() as { token: string; user: User }; |
There was a problem hiding this comment.
Blindly casting the API response with as { token: string; user: User } is unsafe. If the API response format changes or an error is returned, this could lead to runtime errors on the client. It's a good practice to validate data from external sources, including your own API. Consider using a type guard or a validation library like Zod on the client-side as well to ensure the data structure is what you expect before using it. This same issue exists in the signup function on line 62.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…on errors
- Fix timing attack: always call bcrypt.compare even when user not found
(DUMMY_HASH fallback) so response time is constant regardless of email
- Type UserRow and JwtPayload interfaces; remove all `as any` casts
- Return all zod validation errors as { errors: string[] } instead of
only the first error message
- Use e instanceof Error guard in catch block instead of `e: any`
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Addresses Gemini PR #8 medium-priority comment: the blind `as { token: string; user: User }` cast is replaced with a runtime type guard (isAuthResponse). If the server returns an unexpected shape, an error is thrown before state is updated. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
## Summary
- Adds `isAuthResponse()` type guard to validate server response has `token: string` and `user: object`
- Replaces blind `as { token: string; user: User }` casts in both `login` and `signup` with runtime check
- Addresses Gemini's medium-priority comment from PR #8
🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 participant
Summary
Full Phase 2d security overhaul for the auth system:
bcrypt.compare; seed data also hashedgetJwtSecret()throws on startup if unset;.env.exampleaddedexpress-rate-limitapplied to all/api/auth/*routes (20 req / 15 min)loginSchemaandsignupSchemavalidate request bodies; returns 400 with the first validation errorAuthContextwrites token + user tolocalStorageon login/signup, clears on logout, rehydrates on mountseed.tsexits with error ifNODE_ENV=productionapp.tsso tests can import without binding a portTests
auth.test.ts(4 unit tests) — authenticateToken: valid token, missing token, invalid token, wrong secretauth.integration.test.ts(5 integration tests, in-memory SQLite) — signup → login → 401 wrong password → hashed in DB → duplicate email 400AuthContext.test.tsx(3 unit tests) — login stores to localStorage, logout clears, mount rehydrates38/38 tests passing (29 web + 9 server)
Test plan
pnpm --filter @stagepass/web test— 29/29 passingpnpm --filter @stagepass/server test— 9/9 passingdocs/ISSUES_IN_THE_CODEBASE.mdaddressed🤖 Generated with Claude Code