Repository navigation
security: rate limit invalid token requests - #423
Rodrick0209 wants to merge 2 commits into
Conversation
dinisjcorreia
left a comment
There was a problem hiding this comment.
What this change does:
Adds employee-scoped throttling for invalid student preview-token requests. Valid requests stay outside the counter.
Must fix:
-
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.
-
Concurrent invalid requests bypass the five-request limit (
src/app/api/students/token/route.ts:59-64)- Problem:
checkInvalidCodeLimitonly reads state. Requests overlap atawait 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.
- Problem:
Should fix:
- 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).
Review: changes requestedBlocking
Should fix
Question
|
dinisjcorreia
left a comment
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
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.
Summary
POST /api/students/token.Validation
Release impact
release:patch— backwards-compatible fixrelease:minor— backwards-compatible functionalityrelease:major— intentional breaking changeDeployment / migration notes
None
Manual actions
None
Issues
Closes #393