Feat/security hardening middleware suite#487
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Review limit reached
More reviews will be available in 40 minutes and 44 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR adds backend production security (env validation, Helmet headers, request logging, rate limiting, secure Mongo-backed sessions, centralized error handling, and guarded auth routes), security tests, and a frontend AuthContext with axios client and ProtectedRoute; pages and routing are updated to use the new auth flow. ChangesBackend Security and Authentication Infrastructure
Frontend Authentication & Protected Routes
Sequence DiagramsequenceDiagram
participant User
participant BrowserApp
participant ProtectedRoute
participant AuthContext
participant axiosClient
participant BackendAPI
User->>BrowserApp: Navigate to protected route (/track)
BrowserApp->>ProtectedRoute: guard check
ProtectedRoute->>AuthContext: isAuthenticated / isLoading
AuthContext->>BackendAPI: GET /api/auth/me
BackendAPI-->>AuthContext: user data / 401
alt authenticated
AuthContext-->>ProtectedRoute: isAuthenticated=true
ProtectedRoute-->>BrowserApp: render page
else unauthenticated
ProtectedRoute-->>BrowserApp: redirect /login (preserve location)
end
BrowserApp->>axiosClient: POST /api/auth/login (with credentials)
axiosClient->>BackendAPI: request
alt 200
BackendAPI-->>axiosClient: success + session cookie
axiosClient-->>AuthContext: update user state
else 401
BackendAPI-->>axiosClient: 401
axiosClient-->>BrowserApp: redirect /login
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (2)
backend/middleware/logger.js (1)
8-11: ⚖️ Poor tradeoffConsider privacy implications of logging IP addresses and user agents.
The current morgan format logs
:remote-addr(IP addresses) and:user-agentstrings, which may be considered PII under GDPR/CCPA depending on your jurisdiction and data handling practices. While HTTP request logging is standard for security monitoring and debugging, ensure that:
- Log retention policies are defined and documented
- Access to logs is appropriately restricted
- Compliance requirements are met for your deployment regions
If full IP logging is not required, consider using a truncated or hashed version of the IP address for security monitoring while reducing PII exposure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/middleware/logger.js` around lines 8 - 11, The current morgan logger (httpLogger) logs raw :remote-addr and :user-agent which may expose PII; update the logging pipeline to mask or hash IPs and omit or redact full user-agent strings before they reach morgan: either replace the format string used by morgan to remove :remote-addr and :user-agent and use a custom token (via morgan.token) that returns a truncated/hashed IP (and a shortened user-agent) or add a small middleware that computes a hashedIp/obfuscatedUA on req (using a stable hash/salted HMAC) and then use those tokens in the morgan format referencing morganStream; ensure httpLogger and morganStream still receive the sanitized tokens.backend/server.js (1)
32-37: ⚡ Quick winConsider restricting undefined origin access.
The CORS configuration allows requests without an
Originheader (line 33:!origincheck). While this is a common pattern for non-browser clients, server-to-server calls, and same-origin requests, it bypasses the origin whitelist entirely.For a production security hardening PR, consider whether undefined-origin requests are necessary:
- If the API should only serve browser clients, remove the
!origincheck- If server-to-server calls are needed, document this explicitly or use a separate endpoint/authentication method that doesn't rely on CORS
🔒 Stricter CORS (browser-only)
app.use(cors({ origin: function (origin, callback) { - if (!origin || allowedOrigins.indexOf(origin) !== -1) { + if (allowedOrigins.indexOf(origin) !== -1) { callback(null, true); } else{ callback(new Error('Blocked by CORS policy')); } }, credentials: true }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/server.js` around lines 32 - 37, The CORS origin handler (origin: function (origin, callback)) currently allows requests with no Origin header via the !origin branch, which bypasses the allowedOrigins check; update this handler to disallow undefined/null origin for browser-only usage (remove the !origin condition) or, if server-to-server traffic is required, implement an explicit bypass path (e.g., check a header or token or route those requests to a separate endpoint) and document that behavior; ensure the new logic still calls callback(null, true) only when origin is present and included in allowedOrigins and calls callback(new Error('Blocked by CORS policy')) otherwise.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/.env.example`:
- Line 8: The .env.example exposes COOKIE_DOMAIN and LOG_LEVEL but
envValidator.js currently only validates MONGO_URI, PORT, SESSION_SECRET,
CLIENT_URL, NODE_ENV; decide whether COOKIE_DOMAIN and LOG_LEVEL are optional or
required, then update accordingly: if optional, add inline comments next to
COOKIE_DOMAIN and LOG_LEVEL in backend/.env.example stating they are optional
and provide defaults; if required, add them to the validation checks in
backend/middleware/envValidator.js (the validator that enumerates/throws for
missing env vars) so COOKIE_DOMAIN and LOG_LEVEL are validated at startup and
include helpful error text mentioning the variable names.
In `@backend/middleware/security.js`:
- Line 9: The imgSrc CSP entry in middleware/security.js is overly permissive
because it uses the 'https:' wildcard; remove 'https:' and replace it with a
strict allowlist of trusted image host origins (e.g., an array like
TRUSTED_IMAGE_HOSTS or from an env var) and update the imgSrc directive to only
include "'self'", 'data:' plus those specific domains; ensure the allowlist is
documented and loaded from config/env so new domains can be added safely rather
than permitting all HTTPS origins.
- Line 7: The CSP currently allows inline styles via the styleSrc array
containing "'unsafe-inline'"; remove "'unsafe-inline'" from styleSrc in the CSP
configuration (in backend/middleware/security.js) and implement a safer
alternative: generate a per-request nonce (e.g., middleware that attaches
res.locals.cspNonce or req.cspNonce), add "nonce-<nonceValue>" to the styleSrc
directive at response time, and update any templates/components to use that
nonce on inline <style> or style tags (or move inline styles to external
stylesheets or use SHA hashes for known static inline styles). Ensure the nonce
generation function is secure (cryptographically random) and referenced in the
CSP middleware and template renderer where styles are injected.
In `@backend/routes/auth.js`:
- Around line 32-35: The login failure currently forwards strategy detail via
info?.message which leaks whether an email or password was wrong; update the
failure response in the auth route handler (the block that checks if (!user)) to
always return a generic error message (e.g. 'Invalid credentials') and remove
inclusion of info?.message so the res.status(401).json payload does not expose
authentication reasons.
- Around line 20-25: The /signup route responses are missing the standardized
success boolean; update the JSON payloads returned around the existingUser check
and after newUser.save() so they include success: false for the early return
(where existingUser is truthy) and success: true for the 201 creation response,
keeping the rest of the message fields unchanged; look for the existingUser
conditional and the res.status(201).json(...) after newUser.save() in auth.js to
apply the change.
In `@backend/server.js`:
- Around line 127-133: The SIGTERM handler currently only calls
mongoose.connection.close and must also stop accepting new HTTP requests by
closing the Express HTTP server; update the process.on('SIGTERM') block to call
server.close (the variable that holds the result of app.listen) and wait for its
callback or error before calling mongoose.connection.close, logging via logger
at each step and ensuring process.exit(0/1) is invoked after DB close or on
server close error; reference the process.on('SIGTERM') handler, the
server.close callback, mongoose.connection.close, and logger for where to add
the sequencing.
- Around line 43-44: The current app.use calls set bodyParser.json and
bodyParser.urlencoded limits to '10mb', which is risky; change these to a safer
default (e.g., '1mb') or make the limit configurable (ENV variable) and only
allow larger sizes for dedicated upload endpoints; update the
app.use(bodyParser.json(...)) and app.use(bodyParser.urlencoded(...)) usages to
use the reduced/configurable limit, and if large payloads are required
implement/route them to a multipart/form-data or streaming handler instead of
increasing the global body-parser limit.
- Line 4: Replace the incorrect default import for connect-mongo by using the
named MongoStore import (change the require from using .default to destructuring
{ MongoStore }) so the existing MongoStore.create(...) call works with
connect-mongo v6; then verify the session store options passed to
MongoStore.create (specifically the touchAfter option on the store
configuration) use the correct units/interval you intend (touchAfter expects
seconds) and adjust the numeric value if you meant a different interval.
- Around line 86-87: Your current mounts stack rate limiters so requests to
/api/auth hit both authLimiter and generalLimiter; change generalLimiter so it
excludes auth routes (e.g., wrap or configure it to skip when
req.path.startsWith('/auth') or when req.originalUrl startsWith '/api/auth'), or
mount generalLimiter on a narrower path (e.g., '/api' but with a skip middleware
that bypasses it for '/api/auth'), ensuring authLimiter alone governs auth
endpoints; also update verification scripts to capture HTTP status and
rate-limit headers/body (e.g., print response headers and status) to determine
which limiter triggered.
In `@backend/spec/auth-security.spec.cjs`:
- Around line 58-66: The test "should allow up to 10 requests within limit"
currently rotates the source IP (uses `192.168.1.${i}`) so it doesn't validate a
single client's quota; update the loop in that test to use a fixed IP string
(e.g., `192.168.1.100`) for all 10 requests sent to POST '/api/auth/login' via
`request(app)` with `validPayload`, and assert none return 429; keep the rest of
the test (the for-loop structure,
`request(app).post('/api/auth/login').set(...).send(validPayload)` and
`expect(response.status).not.toBe(429)`) intact.
- Around line 9-16: The test currently connects to the production-seeming DB via
mongoose.connect('mongodb://localhost:27017/githubTracker') and then
unconditionally calls mongoose.connection.db.dropDatabase() in afterAll; change
the test to use an isolated test database (e.g., use a TEST_MONGO_URL env var or
a test-only DB name like githubTracker_test) or switch to an in-memory MongoDB
(mongodb-memory-server) before requiring the app, and ensure afterAll still
drops only that isolated test DB via mongoose.connection.db.dropDatabase() or
stops the in-memory server; update the connect call (mongoose.connect) and test
setup/teardown (afterAll) accordingly so local non-test data cannot be wiped.
In `@src/api/axiosClient.ts`:
- Around line 23-26: The global 401 redirect in the Axios response interceptor
(the block that checks error.response?.status === 401) is too broad and should
only redirect for non-auth routes; update the interceptor in
src/api/axiosClient.ts to inspect error.config?.url and window.location.pathname
and only perform window.location.href = '/login' when the failing request URL is
not an auth endpoint (e.g., exclude '/api/auth/login', '/api/auth/signup',
'/api/auth/me' or any '/api/auth/*') and when window.location.pathname !==
'/login'; keep the existing behavior of throwing the error so AuthContext /
Login.tsx can handle auth-related 401s.
In `@src/context/AuthContext.tsx`:
- Around line 46-53: checkAuth currently calls axiosClient.get('/api/auth/me')
but the backend only exposes /signup, /login, and /logout, so session checks
fail; either implement a new GET /api/auth/me (or /api/auth/status) on the
server that returns { success, user } using the existing session/token logic, or
change checkAuth in AuthContext.tsx to use whichever existing mechanism you have
for session validation (e.g., verify stored token or call the appropriate
existing endpoint) and update the axiosClient call in checkAuth accordingly so
setUser/setIsAuthenticated can receive a real user object.
- Around line 98-102: The signup success check in the signup function
incorrectly relies solely on response.data.success and treats 201 responses as
failures; update the condition in the signup handler (the block that calls
setError) to treat HTTP 201 as a success (e.g., use response.status === 201 ||
response.data.success or a 2xx status check) and only call setError when neither
the status indicates success nor response.data.success is truthy, leaving
setError(null) for successful signups.
In `@src/pages/Contributors/Contributors.tsx`:
- Line 17: Remove the unused named import isAxiosError from the import statement
that currently reads import axiosClient, { isAxiosError } from
"../../api/axiosClient"; in Contributors.tsx; keep the default axiosClient
import only (i.e., import axiosClient from "../../api/axiosClient";) so the lint
error is resolved and no other code changes are needed.
In `@src/pages/Login/Login.tsx`:
- Around line 34-35: The success message string is inconsistent:
setMessage("Login successful!") is used but the UI checks for "Login successful"
(also at the other occurrence around lines 131-133), causing the success case to
miss the intended style and be treated as an error; fix by using a single
canonical message string in both places (either remove the trailing "!" or
include it in the comparisons) or, better, replace string-based styling with an
explicit status flag (e.g., setStatus("success") / setStatus("error") alongside
setMessage) and update the rendering logic to check that status (references:
setMessage, navigate("/track"), and the message comparison logic in the Login
component).
In `@src/pages/Signup/Signup.tsx`:
- Around line 86-87: The success message string passed to setMessage currently
reads "Signup successful! You can now login." but the styling logic (the
condition that checks the message for success) is looking for the word
"successfully"; update either the displayed message or the condition to match:
change the message in setMessage (in Signup component) to use "successfully" or
change the style-check predicate that inspects the message to look for
"successful" (ensure both occurrences around setMessage and the message-checking
code use the same word so the success style is applied correctly).
---
Nitpick comments:
In `@backend/middleware/logger.js`:
- Around line 8-11: The current morgan logger (httpLogger) logs raw :remote-addr
and :user-agent which may expose PII; update the logging pipeline to mask or
hash IPs and omit or redact full user-agent strings before they reach morgan:
either replace the format string used by morgan to remove :remote-addr and
:user-agent and use a custom token (via morgan.token) that returns a
truncated/hashed IP (and a shortened user-agent) or add a small middleware that
computes a hashedIp/obfuscatedUA on req (using a stable hash/salted HMAC) and
then use those tokens in the morgan format referencing morganStream; ensure
httpLogger and morganStream still receive the sanitized tokens.
In `@backend/server.js`:
- Around line 32-37: The CORS origin handler (origin: function (origin,
callback)) currently allows requests with no Origin header via the !origin
branch, which bypasses the allowedOrigins check; update this handler to disallow
undefined/null origin for browser-only usage (remove the !origin condition) or,
if server-to-server traffic is required, implement an explicit bypass path
(e.g., check a header or token or route those requests to a separate endpoint)
and document that behavior; ensure the new logic still calls callback(null,
true) only when origin is present and included in allowedOrigins and calls
callback(new Error('Blocked by CORS policy')) otherwise.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61c99446-dac5-468e-9a13-13aa255e78c9
📒 Files selected for processing (19)
backend/.env.examplebackend/middleware/auth.jsbackend/middleware/envValidator.jsbackend/middleware/errorHandler.jsbackend/middleware/logger.jsbackend/middleware/security.jsbackend/package.jsonbackend/routes/auth.jsbackend/server.jsbackend/spec/auth-security.spec.cjsbackend/spec/support/jasmine.mjssrc/Routes/Router.tsxsrc/api/axiosClient.tssrc/components/ProtectedRoute.tsxsrc/context/AuthContext.tsxsrc/main.tsxsrc/pages/Contributors/Contributors.tsxsrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsx
| const checkAuth = async () => { | ||
| try { | ||
| setIsLoading(true); | ||
| // Call a backend endpoint to verify session | ||
| const response = await axiosClient.get('/api/auth/me'); | ||
| if (response.data.success) { | ||
| setUser(response.data.user); | ||
| setIsAuthenticated(true); |
There was a problem hiding this comment.
checkAuth points to an auth-status endpoint that isn’t implemented upstream.
/api/auth/me is called here, but the backend auth routes shown only expose /signup, /login, and /logout. This will keep clearing auth state after mount/refresh and break session persistence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/context/AuthContext.tsx` around lines 46 - 53, checkAuth currently calls
axiosClient.get('/api/auth/me') but the backend only exposes /signup, /login,
and /logout, so session checks fail; either implement a new GET /api/auth/me (or
/api/auth/status) on the server that returns { success, user } using the
existing session/token logic, or change checkAuth in AuthContext.tsx to use
whichever existing mechanism you have for session validation (e.g., verify
stored token or call the appropriate existing endpoint) and update the
axiosClient call in checkAuth accordingly so setUser/setIsAuthenticated can
receive a real user object.
| if (response.data.success) { | ||
| setError(null); | ||
| } else { | ||
| setError(response.data.message || 'Signup failed'); | ||
| } |
There was a problem hiding this comment.
signup treats successful 201 responses as failures.
The current condition expects response.data.success, but signup responses are { message: ... } on 201. That means successful signups fall into the failure branch and set an error state.
💡 Suggested fix
- if (response.data.success) {
- setError(null);
- } else {
- setError(response.data.message || 'Signup failed');
- }
+ // Backend returns 201 + { message } for successful signup
+ if (response.status >= 200 && response.status < 300) {
+ setError(null);
+ } else {
+ setError(response.data.message || 'Signup failed');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (response.data.success) { | |
| setError(null); | |
| } else { | |
| setError(response.data.message || 'Signup failed'); | |
| } | |
| // Backend returns 201 + { message } for successful signup | |
| if (response.status >= 200 && response.status < 300) { | |
| setError(null); | |
| } else { | |
| setError(response.data.message || 'Signup failed'); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/context/AuthContext.tsx` around lines 98 - 102, The signup success check
in the signup function incorrectly relies solely on response.data.success and
treats 201 responses as failures; update the condition in the signup handler
(the block that calls setError) to treat HTTP 201 as a success (e.g., use
response.status === 201 || response.data.success or a 2xx status check) and only
call setError when neither the status indicates success nor
response.data.success is truthy, leaving setError(null) for successful signups.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/routes/auth.js (1)
58-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHarden logout flow: avoid state-changing GET and explicitly invalidate the session/cookie
backend/routes/auth.jsdefinesGET /logoutand the handler only callsreq.logout(...)—there’s noreq.session.destroy(...)and nores.clearCookie(...)in that route. Switch logout to a non-GET method (e.g.,POST) and explicitly destroy/clear the session cookie; if you change the verb, updatesrc/context/AuthContext.tsxwhich currently callsaxiosClient.get('/api/auth/logout').🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/routes/auth.js` around lines 58 - 71, Replace the state-changing GET logout route with a non-GET verb and explicitly invalidate session/cookie: change router.get("/logout", ...) to router.post("/logout", requireAuth, ...), keep calling req.logout(err => ...), then inside the success path call req.session.destroy(err2 => ...) and res.clearCookie('<session-cookie-name>') before sending the success JSON; ensure you handle errors from both req.logout and req.session.destroy and return 500 on failure. Also update the client call in src/context/AuthContext.tsx from axiosClient.get('/api/auth/logout') to axiosClient.post('/api/auth/logout').
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/routes/auth.js`:
- Around line 58-71: Replace the state-changing GET logout route with a non-GET
verb and explicitly invalidate session/cookie: change router.get("/logout", ...)
to router.post("/logout", requireAuth, ...), keep calling req.logout(err =>
...), then inside the success path call req.session.destroy(err2 => ...) and
res.clearCookie('<session-cookie-name>') before sending the success JSON; ensure
you handle errors from both req.logout and req.session.destroy and return 500 on
failure. Also update the client call in src/context/AuthContext.tsx from
axiosClient.get('/api/auth/logout') to axiosClient.post('/api/auth/logout').
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 205ffed1-06e8-48d7-90e6-148294db502e
📒 Files selected for processing (6)
backend/.env.examplebackend/middleware/security.jsbackend/routes/auth.jssrc/pages/Contributors/Contributors.tsxsrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/pages/Contributors/Contributors.tsx
- backend/middleware/security.js
- src/pages/Login/Login.tsx
- src/pages/Signup/Signup.tsx
|
@Likhi2005 - can you rewrite whole backend in NestJS? |
|
I don't have experience with NestJS yet, but since I already work with Node.js and Express, I'd definitely be interested in learning it and contributing to a NestJS migration. For this PR, I've implemented the security hardening features using the project's existing Express architecture and addressed the requirements of the issue. If possible, I'd appreciate it if this PR could be reviewed and considered for merge as a major security update. For the NestJS migration, I'd be happy to work on it separately if a new issue is created and assigned to me. Thank you! |
Use this directly in your PR description:
Related Issue
Description
Implemented a comprehensive production security hardening suite for both backend and frontend authentication architecture.
Backend Security Improvements
requireAuth)Frontend Authentication & Route Protection
AuthContextuseAuth()hookTesting & Validation
New Files Added
Backend
middleware/security.jsmiddleware/auth.jsmiddleware/errorHandler.jsmiddleware/envValidator.jsmiddleware/logger.jsFrontend
src/context/AuthContext.tsxsrc/components/ProtectedRoute.tsxsrc/api/axiosClient.tsHow Has This Been Tested?
Screenshots (if applicable)
OWASP ZAP Security Scan Results
Type of Change
Summary by CodeRabbit
New Features
Security
Tests