fix(auth): remove unverified client-asserted OAuth identity branch + throttle callback

[PierreBrisorgueil]

Summary

• Critical pre-auth account-takeover removed: the OAuth callback contained a branch (req.body.strategy === false) that minted a TOKEN session cookie from a client-controlled identity without ever verifying a provider token. Any caller that posted a crafted strategy=false body with an arbitrary email could log in as any user with zero provider involvement.
• Fix: the short-circuit branch is deleted; the callback now unconditionally falls through to Passport's authenticate(), which performs the actual provider-token verification before a session is issued.
• Throttling: authLimiter (existing rate-limiter middleware) is applied to both the callback route and the OAuth initiation route, preventing brute-force enumeration of the former attack surface.
• Regression test: a failing-first test covering the strategy=false body payload is included, asserting the callback rejects without a valid provider token.

Why

The removed branch was an unguarded escape hatch that let any HTTP client bypass the OAuth provider entirely and impersonate any account.

Scope

• Module(s) impacted: modules/auth
• Cross-module impact: none
• Risk level: high

Validation

• npm run lint
• npm test
• Manual checks done (if applicable)

Guardrails check

• No secrets or credentials introduced (.env*, secrets/**, keys, tokens)
• No risky rename/move of core stack paths
• Changes remain merge-friendly for downstream projects
• Tests added or updated when behavior changed

Notes for reviewers

• Security considerations: this closes a critical pre-auth account-takeover. The removed strategy===false branch had no legitimate use — it was a vestigial fast-path that bypassed the provider entirely.
• Mergeability considerations: no schema or API changes; backward-compatible.
• Follow-up tasks: none for this PR.

Closes #3849
Part of the security audit epic #3848

Summary by CodeRabbit

Release Notes

• Security Fixes

  • OAuth authentication now exclusively relies on the OAuth provider as the source of authenticated identity, removing the previous fallback mechanism that accepted client-asserted identity.

• Improvements

  • Added rate limiting protection to OAuth strategy and callback endpoints.

[coderabbitai]

Warning

Review limit reached

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

More reviews will be available in 47 minutes and 26 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e30f25c7-9453-4e05-bf6e-0cb23e4a1315

📥 Commits

Reviewing files that changed from the base of the PR and between e4ee5d9 and d1ce8ec.

📒 Files selected for processing (2)

• modules/auth/controllers/auth.controller.js
• modules/auth/tests/auth.integration.tests.js

Walkthrough

Removes the req.body.strategy === false branch in oauthCallback that allowed unauthenticated clients to assert an OAuth identity via request body fields and receive a minted session token. All OAuth routes now enforce authLimiter, and the integration tests are updated with a security regression test covering the removed exploit path.

Changes

OAuth Callback Security Fix

Layer / File(s)	Summary
Remove client-asserted identity branch in oauthCallback modules/auth/controllers/auth.controller.js	Deletes the req.body.strategy === false code path (~37 lines) that accepted client-supplied key/value body fields to look up and mint a session token without any server-side provider verification. Identity is now exclusively derived from passport.authenticate(...).
Apply authLimiter to OAuth routes modules/auth/routes/auth.routes.js	Adds authLimiter middleware to GET /api/auth/:strategy and both GET/POST /api/auth/:strategy/callback route definitions, which previously had no rate limiting.
Security regression test modules/auth/tests/auth.integration.tests.js	Removes the two tests that exercised the now-deleted client-side callback branch and replaces them with a test that seeds a victim user, submits a client-asserted identity POST, and asserts no TOKEN cookie is issued and the status is ≥ 400.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• pierreb-devkit/Node#3496: Modifies the same oauthCallback handler's handling of req.body-driven strategy/key logic, directly overlapping with the removed branch.
• pierreb-devkit/Node#3268: Also changes how req.body.strategy/req.body.key is handled in oauthCallback, adding an allowlist guard for the same client-supplied field that this PR removes entirely.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the two main changes: removing the unverified client-asserted OAuth identity branch and adding rate-limiting to the callback.
Description check	✅ Passed	The description follows the template structure comprehensively, covering summary, scope, validation, guardrails, and security considerations with appropriate detail and verification checkmarks.
Linked Issues check	✅ Passed	All objectives from issue #3849 are addressed: the unsafe strategy === false branch is removed from the controller, authLimiter is applied to both callback routes, and a regression test confirms rejection of crafted payloads.
Out of Scope Changes check	✅ Passed	All changes are tightly scoped to the stated objectives: controller logic removal, route middleware application, and test coverage. No unrelated modifications are present.
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
• Commit unit tests in branch fix/oauth-callback-client-identity

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.48%. Comparing base (c31a5f0) to head (d1ce8ec).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #3866   +/-   ##
=======================================
  Coverage   92.48%   92.48%           
=======================================
  Files         165      165           
  Lines        5400     5387   -13     
  Branches     1735     1730    -5     
=======================================
- Hits         4994     4982   -12     
+ Misses        326      325    -1     
  Partials       80       80           

Flag	Coverage Δ
integration	60.10% <100.00%> (-0.08%)	⬇️
unit	73.60% <0.00%> (+0.17%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

---

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c31a5f0...d1ce8ec. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

This PR hardens the OAuth flow in modules/auth by removing a client-controlled OAuth callback bypass that could mint a session without provider verification, and by applying existing rate-limiting to the OAuth initiation/callback routes.

Changes:

• Removes the req.body.strategy === false OAuth callback branch that trusted client-asserted identity data.
• Applies authLimiter to /api/auth/:strategy and /api/auth/:strategy/callback (GET + POST).
• Replaces the prior “client-side OAuth callback” integration test with a regression test ensuring no TOKEN cookie is minted from a forged callback body.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
modules/auth/controllers/auth.controller.js	Deletes the insecure client-asserted identity branch and always delegates identity verification to Passport.
modules/auth/routes/auth.routes.js	Adds authLimiter to OAuth initiation and callback routes to reduce brute-force/enumeration risk.
modules/auth/tests/auth.integration.tests.js	Updates integration tests to cover the removed bypass with a security regression assertion.

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@modules/auth/tests/auth.integration.tests.js`:
- Around line 579-600: The UserService.create() call that creates the victim
user is currently outside the try/finally block, which means if an old user with
the same email exists from a previous test run, the create will fail with no
cleanup opportunity, causing flaky test failures. Move the victim user creation
inside the try block after its opening brace, and before creating the victim,
add a pre-cleanup step that attempts to remove any existing user with the
victimEmail (wrapped in a try-catch to ignore errors if the user doesn't exist).
This ensures the test starts with a clean state regardless of previous test
outcomes.

🪄 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: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: abc1e8e2-65db-47cd-809e-60061bd3466e

📥 Commits

Reviewing files that changed from the base of the PR and between c31a5f0 and e4ee5d9.

📒 Files selected for processing (3)

• modules/auth/controllers/auth.controller.js
• modules/auth/routes/auth.routes.js
• modules/auth/tests/auth.integration.tests.js