Security: Implement JWT auth, auth rate limiting, and secure error handling (OWASP Top 10)#474
Security: Implement JWT auth, auth rate limiting, and secure error handling (OWASP Top 10)#474devin-ai-integration[bot] wants to merge 1 commit intomainfrom
Conversation
…andling Security fixes for top 3 OWASP findings: 1. CRITICAL - Broken Authentication: Replace insecure email-header auth with JWT Bearer token verification. Login now issues signed JWT tokens (24h expiry). Protected routes verify tokens via Authorization header instead of trusting raw x-user-email header. 2. HIGH - Hardcoded JWT Secret: Update .env.example to remove guessable default secret and add generation instructions. Add runtime warning when secret is weak or missing. 3. HIGH - Auth Rate Limiting + Error Handler: Add strict rate limiter on auth endpoints (5 attempts/15min) to prevent brute-force. Improve error handler to hide internal details in production mode. Frontend updated to store/send JWT tokens instead of email headers. All 160 tests updated and passing.
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // Simple email-based authentication middleware | ||
| const JWT_SECRET = process.env.JWT_SECRET; | ||
|
|
||
| if (!JWT_SECRET || JWT_SECRET.includes('change-this')) { |
There was a problem hiding this comment.
🔴 Placeholder detection check doesn't match the new placeholder value
The startup warning check at backend/src/middleware/auth.js:6 uses JWT_SECRET.includes('change-this') to detect if the developer forgot to set a real secret. However, the new .env.example placeholder was changed to CHANGE_ME_generate_a_random_secret_with_the_command_above (backend/.env.example:11), which does not contain the substring change-this. A developer who copies .env.example to .env without changing the secret will not see the warning, and the server will silently use the well-known placeholder as the JWT signing secret.
| if (!JWT_SECRET || JWT_SECRET.includes('change-this')) { | |
| if (!JWT_SECRET || JWT_SECRET.includes('CHANGE_ME')) { |
Was this helpful? React with 👍 or 👎 to provide feedback.
|
|
||
| // Routes | ||
| app.use('/api/auth', authRoutes); | ||
| app.use('/api/auth', authLimiter, authRoutes); |
There was a problem hiding this comment.
🔴 Auth rate limiter (max 5 / 15 min) applies to GET /api/auth/me, breaking normal usage
The authLimiter with max: 5 is applied to all routes under /api/auth (backend/src/server.js:54), including GET /api/auth/me. The frontend calls getCurrentUser() (which hits /api/auth/me) on every page load via the useEffect in AuthContext.tsx:20. After just 5 page loads/refreshes within 15 minutes, the rate limiter returns 429, which triggers the catch block in AuthContext.tsx:22-26 that removes authToken from localStorage — effectively logging the user out. The authLimiter should only be applied to the login endpoint, not the entire /api/auth prefix.
Prompt for agents
The authLimiter rate limiter (max 5 requests per 15 minutes) is mounted on the entire /api/auth prefix in backend/src/server.js:54. This means it also affects GET /api/auth/me, which the frontend calls on every page load (frontend/src/contexts/AuthContext.tsx:20). After 5 page loads, the user gets rate-limited and effectively logged out.
The fix should apply the authLimiter only to the login endpoint. Two approaches:
1. In server.js, change the mounting so authRoutes gets the normal global limiter, and apply authLimiter specifically. For example, add the authLimiter inside the routes/auth.js file only on the POST /login route.
2. Alternatively, in server.js, mount authRoutes without authLimiter, and add a separate route-specific mount: app.post('/api/auth/login', authLimiter) before the authRoutes mount (though this may cause ordering issues).
The cleanest approach is probably option 1: import the rate limiter in routes/auth.js and apply it only to router.post('/login', authLimiter, ...) handler.
Was this helpful? React with 👍 or 👎 to provide feedback.
Summary
Addresses the top 3 findings from an OWASP Top 10 security review of the timesheet app:
1. Broken Authentication (Critical): Replaced the insecure
x-user-emailheader-based authentication with JWT Bearer token verification. The login endpoint now issues signed JWTs (24h expiry), and all protected routes verify tokens via theAuthorization: Bearer <token>header instead of trusting a raw email header. The middleware no longer auto-creates users on every request — it only verifies that the token's email exists in the database.2. Hardcoded JWT Secret (High): Updated
.env.exampleto remove the guessable default secret and added a generation command. Added a runtime warning when the secret is weak or missing.3. Auth Rate Limiting + Error Handler (High): Added a dedicated rate limiter on
/api/authroutes (5 requests per 15 minutes per IP) to prevent brute-force and email enumeration. Updated the error handler to suppress internal error details whenNODE_ENV=production.Frontend updated to store/send JWT tokens via
localStorage('authToken')andAuthorizationheader. All 160 tests updated and passing.Review & Testing Checklist for Human
POST /api/auth/loginwith a valid email format still gets a token. Evaluate whether this is acceptable for your threat model or if password/OTP should be added.x-user-emailheader will stop working. Verify no other services depend on the old auth mechanism.tokenfield on the login response may not be explicitly typed (check if the API response types in the frontend are loose/any).JWT_SECRETis read once at module load time (const JWT_SECRET = process.env.JWT_SECRET). Ensure the env var is set before the server starts; it cannot be changed at runtime.Notes
jsonwebtokenpackage was already a dependency — no new packages were added.Link to Devin session: https://partner-workshops.devinenterprise.com/sessions/10807ffdd5b740768d9bc784aecba943