Skip to content

Fix critical auth vulnerabilities: rate limiting, session fixation, CORS, cookie hardening#377

Closed
advikdivekar wants to merge 1 commit into
GitMetricsLab:mainfrom
advikdivekar:security/auth-hardening
Closed

Fix critical auth vulnerabilities: rate limiting, session fixation, CORS, cookie hardening#377
advikdivekar wants to merge 1 commit into
GitMetricsLab:mainfrom
advikdivekar:security/auth-hardening

Conversation

@advikdivekar
Copy link
Copy Markdown
Contributor

@advikdivekar advikdivekar commented May 21, 2026

What is the problem

The backend authentication layer had four exploitable security vulnerabilities:

  1. No rate limiting on /api/auth/login and /api/auth/signup — unlimited brute-force and credential-stuffing with no server-side resistance.
  2. Session fixationpassport.authenticate stored the authenticated user into the pre-existing session ID. An attacker who plants a known session ID in the victim's browser becomes authenticated after the victim logs in.
  3. Wildcard CORS (cors('*')) — any origin could read API responses, permanently exposing all current and future endpoints with no review step.
  4. Insecure session cookieexpress-session was configured without httpOnly, Secure, or SameSite, making the session ID readable via JavaScript and transmittable cross-site.

Two secondary issues fixed in the same pass: the bcrypt hash was leaking in the login response via req.user, and distinct error messages ('Email is invalid' vs 'Invalid password') enabled user enumeration.

What was changed

File Change
backend/server.js Replaced cors('*') with an explicit ALLOWED_ORIGINS allowlist; added express-rate-limit (10 req / 15 min / IP) on /api/auth/login and /api/auth/signup; added httpOnly, secure, sameSite, and maxAge to the session cookie config
backend/routes/auth.js Refactored login to callback form of passport.authenticate; calls req.session.regenerate() before req.logIn() to issue a fresh session ID; returns only { id, username, email } — never req.user; stripped err.message from all 500 responses
backend/config/passportConfig.js Unified both auth failure messages to 'Invalid credentials' to prevent user enumeration; added .select('-password') to deserializeUser so the hash is never loaded into req.user on subsequent requests
backend/package.json Added express-rate-limit ^7.5.1 as a production dependency
backend/.env.sample Documented NODE_ENV and ALLOWED_ORIGINS; replaced placeholder secret hint
backend/Dockerfile.prod Added ENV NODE_ENV=production so production cookie flags (Secure, SameSite=Strict) activate automatically

Why this approach

Each fix targets the root cause, not just the symptom:

  • Rate limiting is applied at the Express middleware level before the route handler, so it cannot be bypassed by any code path in the route.
  • Session regeneration happens before req.logIn(), which is the only safe ordering. After regeneration the old session is destroyed server-side and the new session is empty; logIn then writes the user ID into the fresh session. Doing it after would leave a window where the old session holds the authenticated state.
  • CORS allowlist is driven by an environment variable so dev, staging, and production each declare only their own frontend origin with zero code changes.
  • Cookie flags are environment-aware: Secure and SameSite=Strict in production, lax in development so local HTTP still works without disabling cookies entirely.

How to test

  1. Rate limiting — Send 11 consecutive POST /api/auth/login requests with wrong credentials from the same IP. The 11th must return 429 Too Many Requests with the message "Too many attempts, please try again after 15 minutes." A successful login on attempt 5 should reset the counter.

  2. Session fixation — Record the connect.sid cookie before login (any unauthenticated request). Log in with valid credentials. The Set-Cookie header in the login response must carry a different connect.sid value.

  3. Password hash not leaked — The POST /api/auth/login success response must contain exactly { message, user: { id, username, email } }. No password field at any level.

  4. CORS — From a browser tab at http://evil.com, run fetch('http://localhost:5000/api/auth/login', { method: 'POST', ... }). The browser must throw a CORS error. From http://localhost:5173 the same request succeeds.

  5. Cookie flags — After login, open DevTools → Application → Cookies. connect.sid must show HttpOnly ✓. In production (NODE_ENV=production) it must also show Secure ✓ and SameSite: Strict. Running document.cookie in the console must return "".

  6. User enumeration closedPOST /api/auth/login with a non-existent email and with an existing email but wrong password must both return { "message": "Invalid credentials" } — identical body, identical status code.

Edge cases covered

  • CORS !origin guard allows server-to-server requests (health checks, internal services) through without needing them on the allowlist.
  • ALLOWED_ORIGINS falls back to http://localhost:5173 when the env var is absent, so no config change is required for existing local dev setups.
  • skipSuccessfulRequests: true on the rate limiter means a user who provides correct credentials on attempt 3 does not consume further quota; only failures count.
  • sameSite: 'lax' in non-production keeps OAuth redirect flows and local development working while still blocking third-party POST CSRF.
  • secure: false in dev prevents cookie loss when the dev server runs on plain HTTP.
  • Both regenerateErr and loginErr are forwarded to the Express error handler via next() so they surface as 500s rather than silent hangs.
  • Internal error details (err.message) are removed from all 500 responses (signup, login error path, logout) to prevent leaking MongoDB internals.

Verification

  • Root cause fully resolved
  • All edge cases handled
  • No regressions introduced
  • Existing tests pass (test app in spec/ does not include rate limiter, so test suite is unaffected by the new middleware)
  • Code matches project style and conventions

Labels: type:security level:advanced gssoc:approved

Closes #372
Closes #373
Closes #374
Closes #375

Please assign this PR to me under GSSoC 2026.

Summary by CodeRabbit

Release Notes

  • Bug Fixes

    • Enhanced API security with origin-based access restrictions to prevent unauthorized cross-origin requests.
    • Added rate limiting to authentication endpoints to protect against brute-force attacks.
    • Improved session security with updated cookie handling and session regeneration.
    • Standardized authentication error messages for better security practices.
  • Chores

    • Updated production environment configuration.
    • Added rate-limiting dependency.

Review Change Stack

…flags

- Add express-rate-limit (10 req/15 min/IP) on /api/auth/login and /api/auth/signup
- Regenerate session ID after successful login to prevent session fixation
- Strip password hash from deserializeUser and login response
- Unify auth error messages to prevent user enumeration
- Replace wildcard CORS with explicit ALLOWED_ORIGINS allowlist
- Add httpOnly, Secure, SameSite, maxAge flags to session cookie
- Strip internal error details from 500 responses
- Set NODE_ENV=production in production Dockerfile
- Document ALLOWED_ORIGINS and NODE_ENV in .env.sample

Closes GitMetricsLab#372, GitMetricsLab#373, GitMetricsLab#374, GitMetricsLab#375
@netlify
Copy link
Copy Markdown

netlify Bot commented May 21, 2026

Deploy Preview for github-spy ready!

Name Link
🔨 Latest commit afeec41
🔍 Latest deploy log https://app.netlify.com/projects/github-spy/deploys/6a0f5d08c9b6350008d0a5af
😎 Deploy Preview https://deploy-preview-377--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 21, 2026

Caution

Review failed

Pull request was closed or merged during review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 5b210c8f-d98f-402b-8797-d995759d4a46

📥 Commits

Reviewing files that changed from the base of the PR and between 9d34c19 and afeec41.

📒 Files selected for processing (6)
  • backend/.env.sample
  • backend/Dockerfile.prod
  • backend/config/passportConfig.js
  • backend/package.json
  • backend/routes/auth.js
  • backend/server.js

📝 Walkthrough

Walkthrough

This PR hardens the backend authentication and API security by addressing four linked vulnerabilities: wildcard CORS, missing session cookie security flags, absent rate limiting on auth endpoints, and session fixation/password hash leaks. Environment variables, Passport strategy, session handling, rate limiting, CORS restrictions, and authentication routes are updated in concert.

Changes

Backend Security Hardening

Layer / File(s) Summary
Environment Configuration and Production Deployment
backend/.env.sample, backend/Dockerfile.prod
NODE_ENV, ALLOWED_ORIGINS, and SESSION_SECRET are documented in .env.sample; production Dockerfile now sets NODE_ENV=production to enable secure cookie and CORS defaults at runtime.
Passport Strategy and Session Persistence
backend/config/passportConfig.js
Local strategy refactored to query by email, return a unified "Invalid credentials" message for missing users or wrong passwords, and exclude the password hash from deserialized user objects stored in sessions.
Rate Limiting Infrastructure
backend/package.json, backend/server.js
express-rate-limit dependency added; authLimiter middleware created and mounted on /api/auth/login and /api/auth/signup with a 10-requests-per-15-minutes window per IP, skipping successful requests.
CORS Allowlist Configuration
backend/server.js
CORS middleware replaces wildcard policy with an allowlist parsed from ALLOWED_ORIGINS environment variable; still permits requests without an Origin header; credentials enabled.
Session Security and Authentication Route Hardening
backend/server.js, backend/routes/auth.js
Session cookies set with httpOnly, secure (conditional on NODE_ENV), sameSite (conditional on NODE_ENV), and 24-hour maxAge. Signup and logout return generic error messages; login now regenerates the session and returns only safe fields (id, username, email).

Sequence Diagram

sequenceDiagram
  participant Client
  participant RateLimit as Rate Limiter
  participant CORS
  participant LoginRoute as /api/auth/login
  participant Passport
  participant Session as Session Regenerate
  participant UserDB as User Database
  
  Client->>RateLimit: POST /api/auth/login
  RateLimit->>CORS: Check if within limit
  CORS->>CORS: Verify origin in allowlist
  CORS->>LoginRoute: Request approved
  LoginRoute->>Passport: passport.authenticate('local')
  Passport->>UserDB: Find user by email
  alt User found
    Passport->>Passport: Verify bcrypt password
    alt Password matches
      Passport->>LoginRoute: User valid
      LoginRoute->>Session: req.session.regenerate()
      Session->>Client: Issue new httpOnly, secure, sameSite cookie
      LoginRoute->>LoginRoute: req.logIn(user)
      LoginRoute->>Client: 200 {id, username, email}
    else Password mismatch
      Passport->>LoginRoute: Invalid credentials
      LoginRoute->>Client: 401 {message: "Invalid credentials"}
    end
  else User not found
    Passport->>LoginRoute: Invalid credentials
    LoginRoute->>Client: 401 {message: "Invalid credentials"}
  end
Loading

Estimated Code Review Effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 A rabbit's ode to security's spring,
where passwords hide and cookies cling,
rate limits tame the brute-force bite,
sessions bloom with httpOnly light, 🔒
and CORS guards the frontend's right!

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main security fixes: rate limiting, session fixation, CORS, and cookie hardening—all core changes in this PR.
Description check ✅ Passed The PR description is comprehensive, exceeding template requirements with detailed problem statement, file-by-file changes, rationale, testing instructions, and edge case handling.
Linked Issues check ✅ Passed All four linked issues (#372, #373, #374, #375) have their core requirements met: rate limiting [#372], cookie hardening [#373], CORS allowlist [#374], and session fixation + hash exclusion [#375].
Out of Scope Changes check ✅ Passed All changes are directly aligned with the four security issues. No extraneous refactoring, feature additions, or unrelated modifications detected.
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
⚔️ Resolve merge conflicts
  • Resolve merge conflict in branch security/auth-hardening

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

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

🎉 Thank you @advikdivekar for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines

@advikdivekar
Copy link
Copy Markdown
Contributor Author

Superseded by four focused PRs — one per security issue — to make review and merging easier.

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