Skip to content

Feat/security hardening middleware suite#487

Open
Likhi2005 wants to merge 4 commits into
GitMetricsLab:mainfrom
Likhi2005:feat/security-hardening-middleware-suite
Open

Feat/security hardening middleware suite#487
Likhi2005 wants to merge 4 commits into
GitMetricsLab:mainfrom
Likhi2005:feat/security-hardening-middleware-suite

Conversation

@Likhi2005
Copy link
Copy Markdown
Contributor

@Likhi2005 Likhi2005 commented May 24, 2026

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

  • Added centralized security middleware architecture
  • Added Helmet.js security headers
  • Added authentication middleware (requireAuth)
  • Added centralized error handling middleware
  • Added environment variable validation during startup
  • Added request logging middleware
  • Added secure session cookie configuration
  • Added MongoDB-backed session store
  • Added rate limiting for authentication routes
  • Improved CORS configuration using whitelist validation
  • Standardized API response format

Frontend Authentication & Route Protection

  • Added centralized AuthContext
  • Added reusable useAuth() hook
  • Added protected route handling
  • Added centralized axios client with interceptors
  • Added session persistence support
  • Refactored Login and Signup pages
  • Updated Navbar authentication state handling
  • Protected sensitive frontend routes

Testing & Validation

  • Added backend security tests
  • Tested authentication middleware
  • Tested rate limiting behavior
  • Verified security headers
  • Verified protected route redirection
  • Tested session persistence and logout flow

New Files Added

Backend

  • middleware/security.js
  • middleware/auth.js
  • middleware/errorHandler.js
  • middleware/envValidator.js
  • middleware/logger.js

Frontend

  • src/context/AuthContext.tsx
  • src/components/ProtectedRoute.tsx
  • src/api/axiosClient.ts

How Has This Been Tested?

  • Tested authentication flow locally
  • Verified protected routes redirect unauthenticated users
  • Tested API rate limiting with repeated auth requests
  • Verified Helmet security headers in responses
  • Tested session persistence after page refresh
  • Verified secure cookie configuration
  • Ran backend security test cases successfully
  • Verified existing routes still function correctly

Screenshots (if applicable)

OWASP ZAP Security Scan Results

OWASP ZAP Scan Results
Logout without authentication (should fail)


Type of Change

  • Bug fix
  • New feature
  • Code style update
  • Breaking change
  • Documentation update

Summary by CodeRabbit

  • New Features

    • Full auth flows: signup, login, logout, current-user endpoint, and client-side auth context with route protection and loading state.
    • Health check endpoint and graceful server shutdown.
  • Security

    • Environment validation, strict security headers, rate limiting, and hardened session cookie handling.
    • Request logging and stricter body limits.
  • Tests

    • Added end-to-end security/authentication tests covering CORS, headers, rate limits, and session behavior.

Review Change Stack

@netlify
Copy link
Copy Markdown

netlify Bot commented May 24, 2026

Deploy Preview for github-spy ready!

Name Link
🔨 Latest commit 5ce876d
🔍 Latest deploy log https://app.netlify.com/projects/github-spy/deploys/6a165cddcb2edf00084f8765
😎 Deploy Preview https://deploy-preview-487--github-spy.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 24, 2026

Warning

Review limit reached

@Likhi2005, we couldn't start this review because you've reached your PR review rate limit.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f8f5ee0f-3477-4c02-b903-0b5060bd6fd7

📥 Commits

Reviewing files that changed from the base of the PR and between ab7d384 and 5ce876d.

📒 Files selected for processing (1)
  • src/context/AuthContext.tsx
📝 Walkthrough

Walkthrough

This 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.

Changes

Backend Security and Authentication Infrastructure

Layer / File(s) Summary
Security middleware & environment setup
backend/.env.example, backend/middleware/auth.js, backend/middleware/envValidator.js, backend/middleware/errorHandler.js, backend/middleware/logger.js, backend/middleware/security.js, backend/package.json
Adds example env file; requireAuth guard; validateEnv() that exits on missing vars; centralized errorHandler and asyncHandler; Morgan-based request logger; Helmet security headers (CSP, HSTS, frameguard, noSniff, referrerPolicy); and new runtime deps (connect-mongo, express-rate-limit, helmet, morgan).
Server setup & route integration
backend/server.js, backend/routes/auth.js
Executes env validation at startup; applies security and logging middleware; configures MongoStore-backed sessions with tightened cookie settings; adds auth-aware rate limiters and /api/health; wires 404 and global error handler; refactors /signup, /login, /logout, and adds GET /me to return structured JSON and use asyncHandler/requireAuth.
Dependencies & Security tests
backend/spec/auth-security.spec.cjs, backend/spec/support/jasmine.mjs
Adds Supertest/Jasmine test suite asserting CORS, rate limiting (429), security headers, session cookie attributes, authentication guards, centralized response format (success/message), and NODE_ENV-dependent stack visibility; adds Jasmine spec config.

Frontend Authentication & Protected Routes

Layer / File(s) Summary
AuthContext, axios client, ProtectedRoute
src/context/AuthContext.tsx, src/api/axiosClient.ts, src/components/ProtectedRoute.tsx
Introduces AuthContext exposing login, signup, logout, checkAuth and user/auth state; axiosClient configured with base URL, 10s timeout, credentials, request logging and 401 redirect; ProtectedRoute renders spinner while loading, redirects unauthenticated users to /login, and renders children when authenticated.
App wiring & page conversions
src/main.tsx, src/Routes/Router.tsx, src/pages/Login/Login.tsx, src/pages/Signup/Signup.tsx, src/pages/Contributors/Contributors.tsx
Wraps app with AuthProvider; protects /track and /activity; updates Login/Signup pages to call useAuth() methods and handle backend messages; replaces direct axios calls with axiosClient in Contributors.

Sequence Diagram

sequenceDiagram
  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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

level:advanced, type:feature

Poem

🐰 A rabbit hops through guarded gates,

Cookies tight and headers straight,
With AuthContext snug and routes protected,
The tracker’s data’s now inspected.
Hop, hop—secure and elated!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: implementing a security hardening middleware suite for the application.
Description check ✅ Passed The description comprehensively covers all required template sections with detailed information about changes, testing, and type of change, following the template structure.
Linked Issues check ✅ Passed All objectives from issue #322 are met: CORS whitelist [#322], rate limiting [#322], Helmet security headers [#322], secure session cookies [#322], centralized auth middleware [#322], and environment validation [#322].
Out of Scope Changes check ✅ Passed All changes directly support the security hardening objectives; frontend route protection and auth context are integral to implementing complete end-to-end security.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 17

🧹 Nitpick comments (2)
backend/middleware/logger.js (1)

8-11: ⚖️ Poor tradeoff

Consider privacy implications of logging IP addresses and user agents.

The current morgan format logs :remote-addr (IP addresses) and :user-agent strings, 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:

  1. Log retention policies are defined and documented
  2. Access to logs is appropriately restricted
  3. 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 win

Consider restricting undefined origin access.

The CORS configuration allows requests without an Origin header (line 33: !origin check). 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 !origin check
  • 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

📥 Commits

Reviewing files that changed from the base of the PR and between 6c6bc3e and b0844df.

📒 Files selected for processing (19)
  • backend/.env.example
  • backend/middleware/auth.js
  • backend/middleware/envValidator.js
  • backend/middleware/errorHandler.js
  • backend/middleware/logger.js
  • backend/middleware/security.js
  • backend/package.json
  • backend/routes/auth.js
  • backend/server.js
  • backend/spec/auth-security.spec.cjs
  • backend/spec/support/jasmine.mjs
  • src/Routes/Router.tsx
  • src/api/axiosClient.ts
  • src/components/ProtectedRoute.tsx
  • src/context/AuthContext.tsx
  • src/main.tsx
  • src/pages/Contributors/Contributors.tsx
  • src/pages/Login/Login.tsx
  • src/pages/Signup/Signup.tsx

Comment thread backend/.env.example Outdated
Comment thread backend/middleware/security.js Outdated
Comment thread backend/middleware/security.js Outdated
Comment thread backend/routes/auth.js Outdated
Comment thread backend/routes/auth.js
Comment on lines +46 to +53
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);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

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.

Comment thread src/context/AuthContext.tsx Outdated
Comment on lines +98 to +102
if (response.data.success) {
setError(null);
} else {
setError(response.data.message || 'Signup failed');
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

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.

Suggested change
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.

Comment thread src/pages/Contributors/Contributors.tsx
Comment thread src/pages/Login/Login.tsx
Comment thread src/pages/Signup/Signup.tsx
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Harden logout flow: avoid state-changing GET and explicitly invalidate the session/cookie

backend/routes/auth.js defines GET /logout and the handler only calls req.logout(...)—there’s no req.session.destroy(...) and no res.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, update src/context/AuthContext.tsx which currently calls axiosClient.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

📥 Commits

Reviewing files that changed from the base of the PR and between b0844df and ab7d384.

📒 Files selected for processing (6)
  • backend/.env.example
  • backend/middleware/security.js
  • backend/routes/auth.js
  • src/pages/Contributors/Contributors.tsx
  • src/pages/Login/Login.tsx
  • src/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

@mehul-m-prajapati
Copy link
Copy Markdown
Collaborator

@Likhi2005 - can you rewrite whole backend in NestJS?

@Likhi2005
Copy link
Copy Markdown
Contributor Author

Hi @mehul-m-prajapati,

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

🚀 Feature: Production Security Hardening: Centralized Security Middleware Suite

2 participants