Skip to content

Add verify-login compatibility route for 2FA#4331

Open
SueJianjian wants to merge 3 commits into
anurag3407:mainfrom
SueJianjian:codex/add-verify-login-twofactor-route
Open

Add verify-login compatibility route for 2FA#4331
SueJianjian wants to merge 3 commits into
anurag3407:mainfrom
SueJianjian:codex/add-verify-login-twofactor-route

Conversation

@SueJianjian

@SueJianjian SueJianjian commented Jun 25, 2026

Copy link
Copy Markdown

User description

Summary

  • add a backwards-compatible /api/auth/2fa/verify-login route for older frontend login flows
  • validate the legacy payload shape and dispatch to TOTP or backup-code verification
  • add regression coverage for the new route registration and verification branches

Testing

  • node --test src/routes/tests/twoFactor.test.js
  • node --test src/schemas/tests/schemas.test.js

Closes #4279


CodeAnt-AI Description

Add a legacy 2FA login route and improve security scan reporting

What Changed

  • Added a backwards-compatible 2FA login verification route for older sign-in flows, including support for TOTP codes and backup codes
  • The login verification request now accepts an optional email and handles invalid or expired codes with clear messages
  • Updated the security scan workflow to handle missing Snyk credentials, skip dependency review on forked pull requests, and produce clearer scan summaries and PR comments
  • Security scan artifacts and reports are now collected and labeled in a way that is easier to review from Actions

Impact

✅ Fewer login breakages for older frontends
✅ Clearer 2FA sign-in errors
✅ More reliable dependency scan results on forks and private repos

💡 Usage Guide

Checking Your Pull Request

Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.

Talking to CodeAnt AI

Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:

@codeant-ai ask: Your question here

This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.

Example

@codeant-ai ask: Can you suggest a safer alternative to storing this secret?

Preserve Org Learnings with CodeAnt

You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:

@codeant-ai: Your feedback here

This helps CodeAnt AI learn and adapt to your team's coding style and standards.

Example

@codeant-ai: Do not flag unused imports.

Retrigger review

Ask CodeAnt AI to review the PR again, by typing:

@codeant-ai: review

Check Your Repository Health

To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.

Summary by CodeRabbit

  • New Features

    • Added a backward-compatible two-factor login verification flow for backup codes and one-time codes.
    • Improved security scan handling in the CI pipeline, including clearer pass/skip/failure reporting.
  • Bug Fixes

    • Tightened error handling for invalid backup-code verification.
    • Prevented some security checks from running in unsupported cases, such as forked pull requests or missing tokens.
  • Tests

    • Added coverage for the new two-factor verification flow and validation rules.

@vercel

vercel Bot commented Jun 25, 2026

Copy link
Copy Markdown

@SueJianjian is attempting to deploy a commit to the Anurag Mishra's projects Team on Vercel.

A member of the Team first needs to authorize it.

@codeant-ai

codeant-ai Bot commented Jun 25, 2026

Copy link
Copy Markdown

CodeAnt AI is reviewing your PR.

@codeant-ai

codeant-ai Bot commented Jun 25, 2026

Copy link
Copy Markdown

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@coderabbitai

coderabbitai Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

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

More reviews will be available in 20 minutes and 19 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.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: de351690-35c2-4b08-a9c1-1228c42f7534

📥 Commits

Reviewing files that changed from the base of the PR and between 0353ac0 and 5e1c631.

📒 Files selected for processing (1)
  • .github/workflows/security-scan.yml
📝 Walkthrough

Walkthrough

The PR adds a backend 2FA login verification route with schema validation and tests, and updates the security-scan workflow trigger filters, scan job conditions, artifact handling, and summary reporting.

Changes

Security Scan Workflow

Layer / File(s) Summary
Trigger and scan jobs
.github/workflows/security-scan.yml
push and pull_request runs are limited to main, master, and develop; npm audit adds backend caching, records an exit code output, fails conditionally on high or critical findings, and Snyk/dependency review only run when their conditions are met.
SARIF upload and summary
.github/workflows/security-scan.yml
The OSV SARIF upload uses an explicit file-existence condition, and the summary job rewrites the PR comment/status text while keeping the final npm-audit failure gate.

Two-Factor Login Verification

Layer / File(s) Summary
Verify-login schema
backend/src/schemas/twoFactor.schema.js, backend/src/schemas/__tests__/schemas.test.js
verifyLoginSchema accepts email, token, and useBackup, and the schema tests cover valid token-only, backup-login, and invalid-email inputs.
Verify-login route and helper
backend/src/routes/twoFactor.js, backend/src/routes/__tests__/twoFactor.test.js
verifyLoginChallenge selects TOTP or backup-code verification from req.body.useBackup, adds POST /api/auth/2fa/verify-login, and the route tests cover registration, success, and 401 failure behavior.

Sequence Diagram(s)

sequenceDiagram
  participant Route as POST /api/auth/2fa/verify-login
  participant verifyToken
  participant verifyLimiter
  participant Schema as validate(verifyLoginSchema)
  participant verifyLoginChallenge
  participant Totp as deps.verifyTotp
  participant Backup as deps.verifyBackupCode
  participant res

  Route->>verifyToken: authenticate request
  verifyToken->>verifyLimiter: apply rate limit
  verifyLimiter->>Schema: validate { email, token, useBackup }
  Schema->>verifyLoginChallenge: call handler
  alt useBackup is true
    verifyLoginChallenge->>Backup: verifyBackupCode(uid, token)
  else useBackup is false
    verifyLoginChallenge->>Totp: verifyTotp(uid, token)
  end
  verifyLoginChallenge-->>res: { success: true }
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested labels

level:advanced, quality:clean

Poem

I hopped through 2FA with a token in paw,
Then backed up the backup with a tidy new law.
The scans chirped along with a Snyk little song,
While comments and SARIF marched brave and strong.
🐰✨ A carrot to code, and the checks all came along!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The security-scan workflow edits are unrelated to fixing the broken 2FA login flow and appear out of scope. Move the security-scan workflow changes into a separate PR so this patch stays focused on the 2FA verify-login fix.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main change: adding a 2FA verify-login compatibility route.
Linked Issues check ✅ Passed The PR adds the missing /api/auth/2fa/verify-login route and supports email, token, and optional backup-code login flows as requested.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ 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.

@codeant-ai codeant-ai Bot added the size:L This PR changes 100-499 lines, ignoring generated files label Jun 25, 2026
@codeant-ai

codeant-ai Bot commented Jun 25, 2026

Copy link
Copy Markdown

CodeAnt AI finished reviewing your PR.

@codeant-ai

codeant-ai Bot commented Jun 26, 2026

Copy link
Copy Markdown

CodeAnt AI is running Incremental review

@codeant-ai

codeant-ai Bot commented Jun 26, 2026

Copy link
Copy Markdown

Thanks for using CodeAnt! 🎉

We're free for open-source projects. if you're enjoying it, help us grow by sharing.

Share on X ·
Reddit ·
LinkedIn

@codeant-ai codeant-ai Bot added size:L This PR changes 100-499 lines, ignoring generated files and removed size:L This PR changes 100-499 lines, ignoring generated files labels Jun 26, 2026
@codeant-ai

codeant-ai Bot commented Jun 26, 2026

Copy link
Copy Markdown

CodeAnt AI Incremental review completed.

@coderabbitai coderabbitai Bot left a comment

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.

Actionable comments posted: 4

🧹 Nitpick comments (3)
backend/src/routes/__tests__/twoFactor.test.js (1)

73-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the default 401 branch too.

Lines 73-100 only lock down the backup-code failure path. Add the matching TOTP failure assertion so the default "Invalid or expired code" contract on verifyLoginChallenge can't regress unnoticed.

Suggested test addition
+  test('throws the TOTP error message on invalid code', async () => {
+    const req = {
+      user: { uid: 'user-000' },
+      body: { token: '000000' },
+    };
+    const res = {
+      json() {
+        assert.fail('res.json should not be called on invalid TOTP code');
+      },
+    };
+
+    await assert.rejects(
+      verifyLoginChallenge(req, res, {
+        async verifyTotp() {
+          return false;
+        },
+        async verifyBackupCode() {
+          return true;
+        },
+      }),
+      (error) => {
+        assert.ok(error instanceof ApiError);
+        assert.equal(error.statusCode, 401);
+        assert.equal(error.message, 'Invalid or expired code');
+        return 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/src/routes/__tests__/twoFactor.test.js` around lines 73 - 100, The
current verifyLoginChallenge test only covers the invalid backup-code path, so
add a matching failure case for the normal TOTP branch to lock in the default
401 behavior. Extend the existing twoFactor.test.js coverage around
verifyLoginChallenge by asserting that when useBackup is false and verifyTotp()
returns false, the promise rejects with ApiError statusCode 401 and message
"Invalid or expired code", keeping the existing backup-code assertion intact.
.github/workflows/security-scan.yml (2)

46-49: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value

Exit-code capture depends on implicit pipefail.

AUDIT_EXIT=$? after npm audit ... | tee ... captures the pipeline status, i.e. tee's exit code, unless pipefail is active. It works because the GitHub Actions default bash shell runs with -eo pipefail, but if that default ever changes the high/critical gate at Line 62 would silently pass. Consider making the intent explicit.

🛡️ Make pipefail explicit
       run: |
+        set -o pipefail
         npm audit --json > npm-audit-report.json || true
         npm audit --audit-level=high 2>&1 | tee npm-audit-summary.txt || AUDIT_EXIT=$?
         echo "exit_code=${AUDIT_EXIT:-0}" >> "$GITHUB_OUTPUT"
🤖 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 @.github/workflows/security-scan.yml around lines 46 - 49, The exit-code
capture in the security scan workflow relies on implicit shell behavior around
the piped `npm audit` and `tee` command. Make the intent explicit in the
workflow step that sets `AUDIT_EXIT` by enabling `pipefail` (or otherwise
ensuring the pipeline returns `npm audit`’s status) so the subsequent gate using
`exit_code` is robust even if the default GitHub Actions shell behavior changes.

75-77: 🔒 Security & Privacy | 🔵 Trivial | ⚖️ Poor tradeoff

Optional security posture: pin actions and disable credential persistence.

Static analysis flags actions/checkout (here and at Line 128) for persisting credentials into the runner, and reports the third-party actions in this file are referenced by mutable tags (e.g. @v4, @master) rather than commit SHAs. If your supply-chain policy requires it, add persist-credentials: false to checkout steps that don't push, and pin actions to full-length commit hashes.

🔒 Example for checkout
       - name: Checkout repository
         if: env.SNYK_TOKEN != ''
         uses: actions/checkout@v4
+        with:
+          persist-credentials: false
🤖 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 @.github/workflows/security-scan.yml around lines 75 - 77, The workflow uses
mutable action tags and the Checkout repository step still persists credentials
by default. Update the `actions/checkout` usages in this workflow to set
`persist-credentials: false` where no pushes are needed, and pin all third-party
actions referenced here (including `actions/checkout` and any others in the same
workflow) to full commit SHAs instead of tags like `@v4` or `@master`.

Source: Linters/SAST tools

🤖 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 @.github/workflows/security-scan.yml:
- Around line 172-183: The Generate summary step is vulnerable because
github.ref_name is interpolated directly into the shell script, allowing command
injection from attacker-controlled refs. Move github.ref_name into an env
variable for this step and use the quoted shell variable inside the run block,
keeping the existing github.event_name usage as-is. Update the Generate summary
block in security-scan.yml so the ref name is never expanded inline in the
script.
- Line 125: The fork protection check in the workflow job condition is using the
base repository flag, so it does not correctly block pull requests from external
forks. Update the conditional on the relevant job in security-scan.yml to use
the pull request head repository fork boolean instead, keeping the rest of the
`github.event_name == 'pull_request'` logic intact so the guard reliably skips
forked PRs.
- Around line 118-120: The Snyk workflow currently only skips individual steps,
so the job still ends as success and never produces a true skipped result. Add a
job-level condition on the snyk job itself, using a repo/org variable or a
preceding job output to gate execution when the token is unavailable, and remove
reliance on the “Skip Snyk scan when token is unavailable” step for status. This
should make needs.snyk.result resolve to skipped so the summary table and PR
comment branches that check for Skipped become reachable and accurate.

In `@backend/src/schemas/twoFactor.schema.js`:
- Around line 43-45: `twoFactor.schema.js` still uses Zod 3-only
`required_error` on the `token` field, which breaks the intended `/verify-login`
validation messages in Zod 4. Update the `token` schema in the relevant object
to use the unified `error` callback syntax instead of `required_error`, and
preserve the two cases by returning the missing-field message when `issue.input
=== undefined` and a generic invalid-token message otherwise.

---

Nitpick comments:
In @.github/workflows/security-scan.yml:
- Around line 46-49: The exit-code capture in the security scan workflow relies
on implicit shell behavior around the piped `npm audit` and `tee` command. Make
the intent explicit in the workflow step that sets `AUDIT_EXIT` by enabling
`pipefail` (or otherwise ensuring the pipeline returns `npm audit`’s status) so
the subsequent gate using `exit_code` is robust even if the default GitHub
Actions shell behavior changes.
- Around line 75-77: The workflow uses mutable action tags and the Checkout
repository step still persists credentials by default. Update the
`actions/checkout` usages in this workflow to set `persist-credentials: false`
where no pushes are needed, and pin all third-party actions referenced here
(including `actions/checkout` and any others in the same workflow) to full
commit SHAs instead of tags like `@v4` or `@master`.

In `@backend/src/routes/__tests__/twoFactor.test.js`:
- Around line 73-100: The current verifyLoginChallenge test only covers the
invalid backup-code path, so add a matching failure case for the normal TOTP
branch to lock in the default 401 behavior. Extend the existing
twoFactor.test.js coverage around verifyLoginChallenge by asserting that when
useBackup is false and verifyTotp() returns false, the promise rejects with
ApiError statusCode 401 and message "Invalid or expired code", keeping the
existing backup-code assertion intact.
🪄 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: 5b45e631-dd4c-4b09-8e5c-a15477187703

📥 Commits

Reviewing files that changed from the base of the PR and between 45fbc55 and 0353ac0.

📒 Files selected for processing (5)
  • .github/workflows/security-scan.yml
  • backend/src/routes/__tests__/twoFactor.test.js
  • backend/src/routes/twoFactor.js
  • backend/src/schemas/__tests__/schemas.test.js
  • backend/src/schemas/twoFactor.schema.js

Comment on lines +118 to +120
- name: Skip Snyk scan when token is unavailable
if: env.SNYK_TOKEN == ''
run: echo "SNYK_TOKEN is not configured for this repository. Skipping Snyk scan."

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.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Snyk job never reports skipped; summary will show "Passed" when the token is unset.

The snyk job has no job-level if, so when SNYK_TOKEN is empty the real steps skip individually but this echo step still runs and the job completes with result success. Consequently needs.snyk.result is never 'skipped', making the 'Skipped' branches in the summary table (Line 179) and PR comment (Lines 197-198) unreachable — the report will read "Passed" even though Snyk never ran. Gate the whole job instead so it is actually skipped.

🔧 Skip at the job level so the result is `skipped`
   snyk:
     name: Snyk Vulnerability Scan
     runs-on: ubuntu-latest
+    if: ${{ vars.SNYK_ENABLED == 'true' }}  # or another job-level gate; secrets can't be read in job-level if
     env:
       SNYK_TOKEN: ${{ secrets.SNYK_TOKEN }}

Note: secrets cannot be referenced directly in a job-level if, so route the toggle through a repo/org variable (or a preceding job output) if you want a true skipped result.

🤖 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 @.github/workflows/security-scan.yml around lines 118 - 120, The Snyk
workflow currently only skips individual steps, so the job still ends as success
and never produces a true skipped result. Add a job-level condition on the snyk
job itself, using a repo/org variable or a preceding job output to gate
execution when the token is unavailable, and remove reliance on the “Skip Snyk
scan when token is unavailable” step for status. This should make
needs.snyk.result resolve to skipped so the summary table and PR comment
branches that check for Skipped become reachable and accurate.

name: GitHub Dependency Review
runs-on: ubuntu-latest
if: github.event_name == 'pull_request'
if: github.event_name == 'pull_request' && !github.event.repository.fork

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.

🎯 Functional Correctness | 🟠 Major

Fork guard condition is ineffective for PRs from forks

-    if: github.event_name == 'pull_request' && !github.event.repository.fork
+    if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository

Alternatively, use !github.event.pull_request.head.repo.fork for a direct boolean check.

github.event.repository refers to the base repository, which is not flagged as a fork, so the check passes for PRs originating from external forks. This allows the job to run in environments with restricted GITHUB_TOKEN permissions or missing secrets.

📝 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: github.event_name == 'pull_request' && !github.event.repository.fork
if: github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name == github.repository
🤖 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 @.github/workflows/security-scan.yml at line 125, The fork protection check
in the workflow job condition is using the base repository flag, so it does not
correctly block pull requests from external forks. Update the conditional on the
relevant job in security-scan.yml to use the pull request head repository fork
boolean instead, keeping the rest of the `github.event_name == 'pull_request'`
logic intact so the guard reliably skips forked PRs.

Comment on lines 172 to +183
- name: Generate summary
run: |
echo "## 🔐 Dependency Security Scan Summary" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "| Scanner | Status |" >> $GITHUB_STEP_SUMMARY
echo "|---------------|--------|" >> $GITHUB_STEP_SUMMARY
echo "| npm audit | ${{ needs.npm-audit.result == 'success' && 'Passed' || 'Failed' }} |" >> $GITHUB_STEP_SUMMARY
echo "| Snyk | ${{ needs.snyk.result == 'success' && 'Passed' || '⚠️ Issues found' }} |" >> $GITHUB_STEP_SUMMARY
echo "| OSV Scanner | ${{ needs.osv-scan.result == 'success' && 'Passed' || '⚠️ Issues found' }} |" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "_Scan triggered by: \`${{ github.event_name }}\` on \`${{ github.ref_name }}\`_" >> $GITHUB_STEP_SUMMARY
echo "_Run time: $(date -u '+%Y-%m-%d %H:%M UTC')_" >> $GITHUB_STEP_SUMMARY
echo "## Dependency Security Scan Summary" >> "$GITHUB_STEP_SUMMARY"
echo "" >> "$GITHUB_STEP_SUMMARY"
echo "| Scanner | Status |" >> "$GITHUB_STEP_SUMMARY"
echo "|---|---|" >> "$GITHUB_STEP_SUMMARY"
echo "| npm audit | ${{ needs.npm-audit.result == 'success' && 'Passed' || 'Failed' }} |" >> "$GITHUB_STEP_SUMMARY"
echo "| Snyk | ${{ needs.snyk.result == 'success' && 'Passed' || needs.snyk.result == 'skipped' && 'Skipped' || 'Issues found' }} |" >> "$GITHUB_STEP_SUMMARY"
echo "| OSV Scanner | ${{ needs.osv-scan.result == 'success' && 'Passed' || 'Issues found' }} |" >> "$GITHUB_STEP_SUMMARY"
echo "" >> "$GITHUB_STEP_SUMMARY"
echo "_Scan triggered by: \`${{ github.event_name }}\` on \`${{ github.ref_name }}\`_" >> "$GITHUB_STEP_SUMMARY"
echo "_Run time: $(date -u '+%Y-%m-%d %H:%M UTC')_" >> "$GITHUB_STEP_SUMMARY"

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.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Template injection via github.ref_name in the run block.

${{ github.ref_name }} is expanded into the shell script before execution. Branch/tag names are attacker-controllable (git refs permit characters such as `, $, (, ), ;, |), so a crafted ref name can inject commands into this step and access the job's GITHUB_TOKEN/secrets. Pass the value through an env variable and reference it as a quoted shell variable. (github.event_name is from a fixed set and is safe.)

🔒 Use an env intermediary
       - name: Generate summary
+        env:
+          EVENT_NAME: ${{ github.event_name }}
+          REF_NAME: ${{ github.ref_name }}
         run: |
           echo "## Dependency Security Scan Summary" >> "$GITHUB_STEP_SUMMARY"
           ...
-          echo "_Scan triggered by: \`${{ github.event_name }}\` on \`${{ github.ref_name }}\`_" >> "$GITHUB_STEP_SUMMARY"
+          echo "_Scan triggered by: \`${EVENT_NAME}\` on \`${REF_NAME}\`_" >> "$GITHUB_STEP_SUMMARY"
           echo "_Run time: $(date -u '+%Y-%m-%d %H:%M UTC')_" >> "$GITHUB_STEP_SUMMARY"
📝 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
- name: Generate summary
run: |
echo "## 🔐 Dependency Security Scan Summary" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "| Scanner | Status |" >> $GITHUB_STEP_SUMMARY
echo "|---------------|--------|" >> $GITHUB_STEP_SUMMARY
echo "| npm audit | ${{ needs.npm-audit.result == 'success' && '✅ Passed' || '❌ Failed' }} |" >> $GITHUB_STEP_SUMMARY
echo "| Snyk | ${{ needs.snyk.result == 'success' && '✅ Passed' || '⚠️ Issues found' }} |" >> $GITHUB_STEP_SUMMARY
echo "| OSV Scanner | ${{ needs.osv-scan.result == 'success' && '✅ Passed' || '⚠️ Issues found' }} |" >> $GITHUB_STEP_SUMMARY
echo "" >> $GITHUB_STEP_SUMMARY
echo "_Scan triggered by: \`${{ github.event_name }}\` on \`${{ github.ref_name }}\`_" >> $GITHUB_STEP_SUMMARY
echo "_Run time: $(date -u '+%Y-%m-%d %H:%M UTC')_" >> $GITHUB_STEP_SUMMARY
echo "## Dependency Security Scan Summary" >> "$GITHUB_STEP_SUMMARY"
echo "" >> "$GITHUB_STEP_SUMMARY"
echo "| Scanner | Status |" >> "$GITHUB_STEP_SUMMARY"
echo "|---|---|" >> "$GITHUB_STEP_SUMMARY"
echo "| npm audit | ${{ needs.npm-audit.result == 'success' && 'Passed' || 'Failed' }} |" >> "$GITHUB_STEP_SUMMARY"
echo "| Snyk | ${{ needs.snyk.result == 'success' && 'Passed' || needs.snyk.result == 'skipped' && 'Skipped' || 'Issues found' }} |" >> "$GITHUB_STEP_SUMMARY"
echo "| OSV Scanner | ${{ needs.osv-scan.result == 'success' && 'Passed' || 'Issues found' }} |" >> "$GITHUB_STEP_SUMMARY"
echo "" >> "$GITHUB_STEP_SUMMARY"
echo "_Scan triggered by: \`${{ github.event_name }}\` on \`${{ github.ref_name }}\`_" >> "$GITHUB_STEP_SUMMARY"
echo "_Run time: $(date -u '+%Y-%m-%d %H:%M UTC')_" >> "$GITHUB_STEP_SUMMARY"
- name: Generate summary
env:
EVENT_NAME: ${{ github.event_name }}
REF_NAME: ${{ github.ref_name }}
run: |
echo "## Dependency Security Scan Summary" >> "$GITHUB_STEP_SUMMARY"
echo "" >> "$GITHUB_STEP_SUMMARY"
echo "| Scanner | Status |" >> "$GITHUB_STEP_SUMMARY"
echo "|---|---|" >> "$GITHUB_STEP_SUMMARY"
echo "| npm audit | ${{ needs.npm-audit.result == 'success' && 'Passed' || 'Failed' }} |" >> "$GITHUB_STEP_SUMMARY"
echo "| Snyk | ${{ needs.snyk.result == 'success' && 'Passed' || needs.snyk.result == 'skipped' && 'Skipped' || 'Issues found' }} |" >> "$GITHUB_STEP_SUMMARY"
echo "| OSV Scanner | ${{ needs.osv-scan.result == 'success' && 'Passed' || 'Issues found' }} |" >> "$GITHUB_STEP_SUMMARY"
echo "" >> "$GITHUB_STEP_SUMMARY"
echo "_Scan triggered by: \`${EVENT_NAME}\` on \`${REF_NAME}\`_" >> "$GITHUB_STEP_SUMMARY"
echo "_Run time: $(date -u '+%Y-%m-%d %H:%M UTC')_" >> "$GITHUB_STEP_SUMMARY"
🧰 Tools
🪛 zizmor (1.26.1)

[error] 182-182: code injection via template expansion (template-injection): may expand into attacker-controllable code

(template-injection)

🤖 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 @.github/workflows/security-scan.yml around lines 172 - 183, The Generate
summary step is vulnerable because github.ref_name is interpolated directly into
the shell script, allowing command injection from attacker-controlled refs. Move
github.ref_name into an env variable for this step and use the quoted shell
variable inside the run block, keeping the existing github.event_name usage
as-is. Update the Generate summary block in security-scan.yml so the ref name is
never expanded inline in the script.

Source: Linters/SAST tools

Comment on lines +43 to +45
token: z
.string({ required_error: 'token is required' })
.min(1, 'token cannot be empty'),

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.

🎯 Functional Correctness | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

In Zod 4.4.3, does z.string({ required_error: "token is required" })still customize the missing-field error, or should schemas use the unifiederror option instead?

💡 Result:

In Zod 4.4.3, you should use the unified error option rather than the legacy required_error parameter [1][2]. Zod 4 has officially dropped support for the required_error and invalid_type_error parameters that were available in Zod 3 [2]. In Zod 4, these have been replaced by a single, unified error parameter which accepts either a string or a function to customize error messages [3][1][2]. To customize the error message for a missing field (previously handled by required_error), you should use the function syntax for the error option, allowing you to check if the input is undefined [4][1][2]. Example usage in Zod 4: z.string({ error: (issue) => issue.input === undefined? "token is required": "Invalid input" }); This approach is the recommended and standard way to handle error customization in Zod 4 [4][1][2]. Using the old required_error parameter will not function as intended as it has been removed from the API [2].

Citations:


Update required_error to unified error format for Zod 4 compatibility

required_error and invalid_type_error have been removed in Zod 4. Using them now results in generic error messages or ignored configuration for the /verify-login endpoint.

Replace the required_error option with the unified error configuration:

Current snippet
  token: z
    .string({ required_error: 'token is required' })
    .min(1, 'token cannot be empty'),
Proposed Zod 4 fix
  token: z
    .string({ error: (issue) => issue.input === undefined ? 'token is required' : 'Invalid token' })
    .min(1, 'token cannot be empty'),

Use the function syntax for error to distinguish between a missing field (input === undefined) and an invalid value type.

🤖 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/src/schemas/twoFactor.schema.js` around lines 43 - 45,
`twoFactor.schema.js` still uses Zod 3-only `required_error` on the `token`
field, which breaks the intended `/verify-login` validation messages in Zod 4.
Update the `token` schema in the relevant object to use the unified `error`
callback syntax instead of `required_error`, and preserve the two cases by
returning the missing-field message when `issue.input === undefined` and a
generic invalid-token message otherwise.

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

Labels

size:L This PR changes 100-499 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Frontend calls /auth/2fa/verify-login but no such backend route exists -- 2FA login flow broken

1 participant