Skip to content

security: rate limit invalid token requests - #423

Open
Rodrick0209 wants to merge 2 commits into
devfrom
security/student-token-rate-limit-audit
Open

Rodrick0209 wants to merge 2 commits into
devfrom
security/student-token-rate-limit-audit

Conversation

@Rodrick0209

Copy link
Copy Markdown

Summary

  • Added employee-scoped rate limiting for repeated invalid student-code requests to POST /api/students/token.
  • Created an alternative rate limiter specifically for invalid codes, because the existing shared rate limiter counts all requests and could block valid student scans during high event traffic.
  • Ensured valid student-token requests do not consume the invalid-code request limit.
  • Added audit logging for repeated invalid requests without logging student codes or issued JWTs.
  • Added automated tests covering rate limiting, employee isolation, and normal authorized requests.

Validation

  • Lint/typecheck checks pass where applicable
  • Tests pass where applicable
  • Production build passes where applicable
  • Security/secret checks pass

Release impact

  • release:patch — backwards-compatible fix
  • release:minor — backwards-compatible functionality
  • release:major — intentional breaking change
  • Exempt — Dependabot or documentation/CI/metadata-only change

Deployment / migration notes

None

Manual actions

None

Issues

Closes #393

@Rodrick0209 Rodrick0209 self-assigned this Oct 13, 2026
@Rodrick0209 Rodrick0209 linked an issue Oct 13, 2026 that may be closed by this pull request
1 task done

@dinisjcorreia dinisjcorreia 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.

What this change does:
Adds employee-scoped throttling for invalid student preview-token requests. Valid requests stay outside the counter.

Must fix:

  1. No audit record is emitted (src/app/api/students/token/route.ts:63-65)

    • Problem: Issue #393 requires repeated invalid requests to be observable without exposing student codes or JWTs. This branch records only an in-memory count; it writes no audit event.
    • Fix: Log the rate-limit event and/or invalid attempts through the project logger with employee identity and safe metadata only. Add a focused assertion.
    • If we skip it: Abuse cannot be investigated or detected after the fact.
  2. Concurrent invalid requests bypass the five-request limit (src/app/api/students/token/route.ts:59-64)

    • Problem: checkInvalidCodeLimit only reads state. Requests overlap at await jwtStudent, so a burst can all observe fewer than five failures, all query the student service, then increment later.
    • Fix: Make reservation/check atomic across the async lookup, while releasing a reservation for valid results, and add a concurrent regression test.
    • If we skip it: An authenticated employee can send a parallel burst of arbitrary codes, defeating the new bound.

Should fix:

  1. Duplicate identical test (src/app/api/students/token/route.test.ts:118-176)
    • Problem: Same rate-limit test appears twice. It adds no coverage and makes future edits drift.
    • Fix: Keep one copy.
    • If we skip it: Test suite has redundant maintenance cost.

Verdict: fix 1 and 2 first.
Lean: -29 lines possible.
Not checked: I did not run PR code locally because this checkout is detached without an upstream; CI is green (8/8).

@david-fs-valente

Copy link
Copy Markdown
Collaborator

Review: changes requested

Blocking

  • src/app/api/students/token/route.ts:24 — check-then-act race. checkInvalidCodeLimit runs before await jwtStudent() and recordInvalidCode after it, so N concurrent invalid requests from one employee all pass the check. Reserve the slot before the await and refund on success.
  • src/app/api/students/token/route.test.ts:117 — the test "rate limits repeated requests with unknown student codes" is duplicated verbatim (~line 148). Remove one copy.
  • PR body claims audit logging for repeated invalid requests, but the diff contains no logging call. Add it or correct the description.

Should fix

  • route.ts:6 — in-memory Map is per-process, so replicas or restarts reset the limit. AGENTS.md requires the shared limiter in production.
  • route.ts:6 — buckets are never evicted; the map grows per employee. Reuse createRateLimiter from src/lib/rateLimit.ts (it sweeps) instead of re-implementing fixed-window logic.
  • A jwtStudent throw skips recordInvalidCode, so DB errors never count. Confirm this is intended.
  • Test gaps: concurrent requests, window expiry and Retry-After value (fake timers), missing employee.id 403 path, a successful request not resetting the invalid count. vi.resetModules() is what isolates the module-level Map; add a comment.

Question

  • After 5 invalid codes the 429 blocks valid scans from the same employee for 60s, which contradicts the stated goal of not blocking valid scans during event traffic. What is the intended behavior?

Closes #393 should wait until the race and logging are fixed.

@dinisjcorreia dinisjcorreia 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.

One blocking regression: concurrent valid scans can receive 429 responses.

Limitations: Isolated validation is blocked by the lockfile dependency URL at cdn.sheetjs.com, which is outside the managed network allowlist.

return NextResponse.json({ error: "Forbidden" }, { status: 403 });
}

const limit = invalidCodeLimiter.reserve(employeeId);

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.

reserve runs before the student lookup, so it also consumes all five slots for in-flight valid scans. Six simultaneous valid requests for one employee make the sixth return 429 without calling jwtStudent; the first five only release their reservations after their lookups resolve. This breaks the invalid-code-only limiter behavior and is not covered by the sequential valid-scan test. Change the approach so valid concurrent scans are not rejected, while retaining atomic invalid-attempt enforcement.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Security] Rate-limit student preview-token issuance

3 participants