Backend hardening: rate-limit ICR, fix XSS/data-export gaps, consolidate LLM providers, drop 49MB dead weight - #158
Open
tanvishdesai wants to merge 9 commits into
Conversation
Database/*.json was a 2.2M-line, ~49MB commit (ceb6691, "mongo database in json format") that added synthetic/generated seed data unreferenced by any code path in backend/, frontend/, or ai-services/. It has sat dead in every clone since. Delete it and gitignore the path so it can't silently recur.
…lity startServer() previously built every route AND mounted Vite dev middleware AND called app.listen() in one inseparable function, so there was no way to get a handle on a working, fully-routed app without also starting a real listening server. Split route registration into an exported createApp() (connects DB, builds the Express app, registers every route, returns it — no Vite, no listen). startServer() now calls createApp() then does the Vite mount / listen. Guard the module's auto-start (startServer() at the bottom) so it only fires when this file is the process entry point, not when createApp() is imported by tests. No route logic changed; verified the dev server still boots and serves correctly after the split.
…onstants.ts CLAUDE.md flags max-level, certification, and score-band thresholds as duplicated across many files with no single source of truth. Created shared/constants.ts (MAX_LEVEL, CERTIFICATION_LEVEL, SCORE_BAND_STRONG, SCORE_BAND_SATISFACTORY) and migrated every genuinely-identical backend usage to it: - MAX_LEVEL=93 replaces all `93` / `Math.min(93, ...)` literals in index.ts and gemini.ts (confirmed 93 is the real cap by cross-checking curriculumMap.ts's 93-level registry — CLAUDE.md's "59" reference is stale docs, 59 is just one curriculum level's own id, never a cap). - CERTIFICATION_LEVEL=5 replaces `currentLevel >= 5` occurrences. Left `currentLevel >= 16` (a level-distribution chart bucket) alone — different threshold, same numeral family, not the same concept. - SCORE_BAND_STRONG=80 replaces gemini.ts's deterministic advance-a-level check. Deliberately did NOT force index.ts's percentage-based sub-level banding (findLevel: 80/50, three-tier) or its per-topic concept-mastery bands (70/60/50) into these constants — on inspection those are distinct, already-diverged threshold sets that only coincidentally share some numerals with the "80/60" pair CLAUDE.md's audit note describes. Merging them would misrepresent them as one shared value and risk silently changing grading behavior. Left inline with a comment explaining why, rather than a band-aid unification. Frontend has ~8 more call sites with the same literals but runs entirely on the mock interceptor today (not this backend) — scoped out of this change, tracked as a fast follow-up.
The .gitignore edit adding Database/ was made alongside the Database/ removal but didn't get staged in that commit (git add -A with an explicit pathspec silently missed it) - it's been sitting as an uncommitted change since. Committing it now so it actually takes effect.
/api/icr/evaluate-cloud and /api/icr/evaluate-pdf were reachable by any authenticated account (any role) with no rate limit and no payload size cap. evaluate-cloud forwards the request to a paid external API (Google Vision / MiniMax / OCR.space) using server-held keys, and evaluate-pdf spawns a 60s local subprocess per call — both were direct, unbounded cost/CPU exposure. - Added icrRateLimiter (same express-rate-limit pattern as the existing authRateLimiter), mounted on both routes: 20 requests / 15 min. - Added the same 8MB decoded-size cap /api/icr/filter already enforces, applied at the actual point of decode (matching that endpoint's pattern) on both of evaluate-pdf's temp-file writes and evaluate-cloud's base64 body. - Fixed evaluate-pdf's temp file cleanup: fs.unlinkSync only ran on the success path in both the no-classId fast path and the classId bulk path, so any exception thrown after the file was written (subprocess failure, a downstream dbStore call failing mid-evaluation) leaked the file into ai-services/scratch permanently. Both paths now clean up in a finally block regardless of outcome.
GET /api/students defaulted to a 1000-row page size, but ?all=1 set limit=0, which the query builder only forwards `if (limit > 0)` - meaning `all=1` actually meant "no limit sent to the query at all", pulling the full 86k+ row table (names, school, level history; Aadhaar already masked) in a single response for any authenticated user with broad-enough role scope (state/district/block admins have no schoolScope filter). Introduced HARD_MAX_ROWS=10000, applied even when all=1 is requested. This keeps `all=1`'s actual purpose intact (bypass the default 1000-row page for legitimate bulk reads) without the unbounded-dump behavior; true full-table access still works via the endpoint's existing `offset` pagination in a loop.
studentName/studentId are admin/teacher-entered data, interpolated unescaped into HTML passed to printPage.setContent() - a real, network-capable headless Chrome instance running server-side. A crafted name (e.g. containing an <img onerror=...> or <script> tag) would execute inside that Chrome instance, not just corrupt the PDF visually - a real SSRF/internal-recon vector, not a cosmetic bug. Added a small local escapeHtml() (no existing escaping utility or dependency covers this) and applied it to both interpolation points.
backend/src/gemini.ts was Gemini-only with no fallback, while ai-services/scripts/_api.py already runs Groq (llama-3.1-8b-instant) as primary with Gemini as fallback on the Python side - two different providers for the same class of work (grading/report generation), depending on which half of the system you're looking at. Replaced generateContentWithRetry (Gemini-only) with three functions: - callGroq(): Groq's OpenAI-compatible chat-completions endpoint, same model and same retry/backoff shape (429/5xx retry, 401 non-retryable) as the existing Python implementation. - callGemini(): the previous retry/model-fallback logic, kept as-is, now the fallback path instead of the only path. - callLLM(): tries Groq first (if GROQ_API_KEY is set), falls back to Gemini on any Groq failure (missing/bad key, retries exhausted, network error). Both evaluateAIDiagnostic and evaluateAIWorksheet now call this instead of hitting Gemini directly. Dropped Gemini's structured responseSchema (Groq's JSON mode doesn't support per-field schemas) in favor of describing the expected JSON shape in the prompt text for both providers - each call site already had a deterministic fallback for missing/malformed fields, so this doesn't reduce robustness, just makes both providers go through the same code path. Also deleted generateAIPersonalizedWorksheet: exported from this file and imported in index.ts, but never called anywhere in the codebase. Migrating it to the new callLLM() for zero callers wasn't worth doing; removed it and its dead import instead. Removed DEFAULT_GEMINI_MODEL alongside it once nothing referenced it anymore. Documented GROQ_API_KEY/GEMINI_API_KEY in backend/.env.example (already in the root .env.example for ai-services, but easy to miss when only setting up the backend - index.ts already loads the root .env directly, so no code change needed for the key to actually be picked up).
No test framework existed anywhere in this monorepo before this (confirmed - only tsc --noEmit). Added Node's built-in test runner (node:test + node:assert) - zero new dependencies, already compatible with tsx. New "test" script: tsx --test src/*.test.ts. Isolation from real dev data: db.ts resolves its file-DB path as path.resolve(process.cwd(), 'data'), so test-helpers.ts's isolateTestDb() just mkdtemps a temp dir and chdirs into it before any import that transitively pulls in db.ts - no code change needed in db.ts itself. Empirically verified (via a throwaway probe) that Node's test runner isolates each test *file* into its own process, so this is safe without any explicit --test-isolation flag. Coverage: - paperGenerator.test.ts: escapeHtml() - all 5 HTML-significant chars, a benign name round-trips unchanged, an onerror-image injection attempt is neutralized. - gemini.test.ts: callLLM()'s Groq-primary/Gemini-fallback logic via a mocked global.fetch - Groq success never touches Gemini, Groq 401 is non-retryable and falls through, Groq 429 retries exactly 3 times before giving up; evaluateAIDiagnostic still returns a valid deterministic result when both providers are unavailable. - icr-rate-limit.test.ts / icr-size-cap.test.ts: real HTTP requests against createApp() wrapped in an ephemeral http server - confirms the ICR rate limiter actually trips, and both evaluate-cloud and evaluate-pdf (both code paths) actually reject oversized payloads. - students-export.test.ts: pulled the /api/students limit-resolution math out into an exported resolveStudentsQueryLimit() pure function (index.ts) so the real 10,000-row ceiling is provable directly, instead of only provable by seeding 10,000+ rows into a test database; plus an isolated-DB smoke test that ?all=1 still returns a full small dataset untruncated. Writing icr-size-cap.test.ts surfaced a real bug: evaluate-cloud checked the provider's API key (returning 503 if unset) BEFORE checking the decoded payload size, so in any environment without a configured cloud key the size cap was unreachable - an oversized request got a 503 instead of a 413, and would still have gotten a 503 in front of a would-be 413 in front of a properly-configured deployment too, just with an extra ~2ms of wasted decode/getCloudKey work first. Reordered so the size check runs immediately after basic input validation, before any downstream lookup work.
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
1 participant
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.
Summary
A batch of independently-verified fixes from an audit of the backend, delivered as one PR with a real test suite (there wasn't one before). Every finding below was confirmed by reading the actual code paths, not inferred from symptoms — file:line references are in the commit messages.
POST /api/icr/evaluate-cloudand/api/icr/evaluate-pdfwere reachable by any authenticated account (any role) with no throttling and no payload cap.evaluate-cloudforwards to a paid external API (Google Vision / MiniMax / OCR.space) using server-held keys, andevaluate-pdfspawns a 60s local subprocess per call — both were direct, unbounded cost/CPU exposure. Added the sameexpress-rate-limitpattern already used for login (20 req/15min), and the same 8MB decoded-size cap/api/icr/filteralready enforced, now applied consistently across all three ICR upload endpoints. Also fixed a temp-file leak:evaluate-pdfonly cleaned up its scratch file on the success path, so any exception after the file was written left it on disk permanently.paperGenerator.tsinterpolatedstudentName/studentIdunescaped into HTML fed toprintPage.setContent()— a real, network-capable headless Chrome instance. A crafted name could execute inside that Chrome instance server-side, not just corrupt the PDF visually. AddedescapeHtml()and applied it at both interpolation points; verified live against the real Puppeteer pipeline with an<img onerror>payload as the student name (no injection, no crash, PDF renders the literal text).?all=1export on/api/students.all=1setlimit=0, which the query builder only forwardedif (limit > 0)— meaningall=1actually meant "no limit passed to the query at all," pulling the full 86k+ row table in one response for any account with broad-enough role scope. Introduced a 10,000-row hard ceiling that applies even whenall=1is requested, preservingall=1's actual purpose (bypass the default 1000-row page) without the unbounded-dump behavior.ai-services/scripts/_api.pyalready runs Groq (llama-3.1-8b-instant) as primary with Gemini as fallback;backend/src/gemini.tswas Gemini-only with no fallback at all. Rewrotegemini.ts's LLM calls to match the Python side's provider order (Groq primary, Gemini fallback on missing/bad key or exhausted retries), so both halves of the system behave consistently and depend on one, cheaper primary provider instead of two. Also deletedgenerateAIPersonalizedWorksheet— exported and imported, but called by nothing anywhere in the codebase.Database/*.json(commitceb6691, "mongo database in json format") added ~49MB of unreferenced synthetic seed data — confirmed via grep that no code inbackend/,frontend/, orai-services/reads it, and confirmed the content itself is synthetic/generated (fake names, sequential fake ObjectIds, already-masked Aadhaar numbers), not a real PII leak. Removed it and gitignored the path so it can't silently recur.MAX_LEVEL,CERTIFICATION_LEVEL,SCORE_BAND_STRONG) intoshared/constants.ts, backend-only for now. Also corrected a stale assumption along the way:59isn't the level cap (it's just curriculum level feat: Add Intervention Dashboard #59's own id) —93is, confirmed againstcurriculumMap.ts's 93-node registry. Deliberately did not force-merge threshold values that only coincidentally share a numeral but aren't the same concept (e.g. a 3-tier 80/50 sub-level band vs. per-topic 70/60/50 concept-mastery bands) — left those as literals with a comment explaining why, rather than a band-aid unification that would misrepresent them as one shared value.One planned item was reverted after live testing showed it would break a real feature: admin-gating
GET /api/icr/cloud-config(to match itsPOSTcounterpart) turned out to be consumed by the actual scanning UI (IcrTwoStageScan.tsx) for every role, not just admins — it only exposes booleans ("is a provider configured?"), the sensitive write path (POST, which sets actual API keys) was already correctly admin-gated. Left as-is; noted in the PR for visibility rather than silently dropped.Test plan
No test framework existed in this monorepo before this PR (only
tsc --noEmit). Added Node's built-in test runner (node:test, zero new dependencies) —npm run test --workspace @fln/backend, 15/15 passing:paperGenerator.test.ts—escapeHtml()against all 5 HTML-significant characters, a benign name round-trips unchanged, anonerror-image injection attempt is neutralizedgemini.test.ts— Groq-primary/Gemini-fallback logic via a mockedfetch: Groq success never touches Gemini, Groq 401 is non-retryable and falls through, 429 retries exactly 3 times before giving up; deterministic grading still works when both providers are unavailableicr-rate-limit.test.ts/icr-size-cap.test.ts— real HTTP requests against the app (via an exportedcreateApp(), split out ofstartServer()specifically to make this possible) wrapped in an ephemeral server: confirms the rate limiter actually trips and bothevaluate-cloudandevaluate-pdf(both of its code paths) actually reject oversized payloads. Writing this test caught a real ordering bug — the size check ran after the API-key lookup, so it was unreachable whenever no cloud key was configured; fixed.students-export.test.ts— the?all=1limit math was pulled into an exported pure function (resolveStudentsQueryLimit) so the real 10,000-row ceiling is directly provable, instead of only provable by seeding 10,000+ rows into a test DB; plus an isolated-DB integration check thatall=1still returns a full small dataset untruncatednpm run lintclean (tsc --noEmit, no new errors)200fromGET /api/icr/cloud-config(confirming the revert above is correct); full live Puppeteer PDF generation with a malicious student name, no crash, no injected markup in the render🤖 Generated with Claude Code