fix(security): close the remaining access-control and credential find… - #97
Open
tanvishdesai wants to merge 1 commit into
Open
fix(security): close the remaining access-control and credential find…#97tanvishdesai wants to merge 1 commit into
tanvishdesai wants to merge 1 commit into
Conversation
…ings Rebased onto main after edf3916 (questions room ownership) and b2bac7b (transcription auth) landed the same fixes for two of the five findings; those hunks are dropped and this branch reuses the checkRoomOwnership util they introduced. Three findings remain unaddressed on main, plus one hardening gap. GET /api/responses only checked room MEMBERSHIP, and scoped the query to one student only if the caller chose to pass studentId. Omitting it returned every participant's answers plus the populated answer key, live or long after the session. Members already have purpose-built routes that withhold what they must not see (GET /responses/room/:roomId/student/:studentId, GET /questions), so this route is now restricted to the room's owning teacher rather than re-implementing a third copy of the stripping rules. Samagama-SSO accounts were provisioned with a shared literal password that is public in this repository, so anyone knowing such an account's email could log in as it. New accounts get random bytes instead; the literal is refused outright in login() because the old hashes are still in the database. JWT_SECRET fell back to a published literal in two places; it is now read once, exported, and missing it is a startup failure rather than a silent total auth bypass. This required loading dotenv before the imports that read env at module scope — it ran after them, so a .env-supplied secret was never actually seen. RESEARCH_API_KEY loses its fallback too, but stays optional: unset closes the export lane rather than opening it on a published default. /api/transcription/transcribe additionally gets a dedicated rate limit: it is now authenticated, but a single teacher session can still drive unbounded CPU on the whisper service through the deliberately loose blanket apiLimiter. Route-level tests cover all of it, including the two fixes already on main — they were unit-tested at the util, not through the routes that call it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
Confirmed this is a real, currently-live bug on main — not just a nit. Backend is ESM ("type": "module" in backend/package.json), so all import statements in index.js are hoisted and run before any of the file's own top-level code. On main today:
Net effect: even with JWT_SECRET correctly set in .env, the server silently signs/verifies tokens with the published fallback secret instead — a full auth bypass, since anyone can forge a valid JWT using that known literal. This PR fixes it by making import 'dotenv/config' the very first line of index.js (before any other import), exporting JWT_SECRET from auth.js for index.js to reuse, and removing the fallback entirely so a missing secret is a startup crash instead of a silent bypass. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
…ings
Rebased onto main after edf3916 (questions room ownership) and b2bac7b (transcription auth) landed the same fixes for two of the five findings; those hunks are dropped and this branch reuses the checkRoomOwnership util they introduced. Three findings remain unaddressed on main, plus one hardening gap.
GET /api/responses only checked room MEMBERSHIP, and scoped the query to one student only if the caller chose to pass studentId. Omitting it returned every participant's answers plus the populated answer key, live or long after the session. Members already have purpose-built routes that withhold what they must not see (GET /responses/room/:roomId/student/:studentId, GET /questions), so this route is now restricted to the room's owning teacher rather than re-implementing a third copy of the stripping rules.
Samagama-SSO accounts were provisioned with a shared literal password that is public in this repository, so anyone knowing such an account's email could log in as it. New accounts get random bytes instead; the literal is refused outright in login() because the old hashes are still in the database.
JWT_SECRET fell back to a published literal in two places; it is now read once, exported, and missing it is a startup failure rather than a silent total auth bypass. This required loading dotenv before the imports that read env at module scope — it ran after them, so a .env-supplied secret was never actually seen. RESEARCH_API_KEY loses its fallback too, but stays optional: unset closes the export lane rather than opening it on a published default.
/api/transcription/transcribe additionally gets a dedicated rate limit: it is now authenticated, but a single teacher session can still drive unbounded CPU on the whisper service through the deliberately loose blanket apiLimiter.
Route-level tests cover all of it, including the two fixes already on main — they were unit-tested at the util, not through the routes that call it.