Add emulator-backed rules and integration test suites - #63
Open
ameyypawar wants to merge 8 commits into
Open
Conversation
updateUserRole() had no caller check. Because 'use server' exports compile to addressable HTTP endpoints and the isAdmin() gate in the admin page is client-side only, any caller could invoke the action with their own uid and 'admin' and gain manage_users, moderate_forums and delete_content. Server actions had no way to identify their caller, so this adds the primitive rather than a per-call token argument: an httpOnly session cookie minted from a verified Firebase ID token, plus getSessionUser/requireAuth/ requireAdmin helpers that read it. The role is always loaded from Firestore, never taken from the token or an argument, so it cannot be forged. updateUserRole now calls requireAdmin(), refuses self-escalation, and writes an audit record to roleChanges - previously there was no trace of who changed whose role. Scoped to updateUserRole only. deleteQuestion, updateQuestion, updateComment, addEvent and the vote functions will reuse the same helpers in #19, #21, #22 and #30; guarding them here would risk breaking live writes for browsers that hold no session cookie yet. Fixes #17
- Community.icon is required: all six COMMUNITIES entries already supply one, so the optional type was allowing an unreachable undefined-component case in community/[communityId]/page.tsx. - Event gains createdAt?: string and author: UserProfile: both are already written by eventService.addEvent (server timestamp + the full UserProfile), the type just never reflected it. Propagates a matching `author` fix into searchService.searchEvents and the unused mockEvents fixtures, both of which build Event literals by hand. - SettingsContent read user.metadata, which doesn't exist on the app's UserProfile (that's a Firebase Auth User field, not what useAuth() returns) - "Member Since" silently showed N/A for every user. Fixed to read UserProfile.createdAt. There's no stored equivalent for "Last Sign In", so that row is removed rather than inventing a field.
cert() parses the PEM eagerly and throws DECODER routines::unsupported on a synthetic test key, so there's no way to satisfy the production credential path in a test. When FIRESTORE_EMULATOR_HOST or FIREBASE_AUTH_EMULATOR_HOST is set, initialize with just a projectId (from GCLOUD_PROJECT or FIREBASE_PROJECT_ID) and no credential instead. loadAdminCredential() and the production path are untouched. Side benefit: this also lets a contributor without production Firebase credentials run the app locally against emulators.
Vitest (^3.2.7, needs >=3.2 for test.projects) with a single `unit` project: tests/unit/**/*.test.ts, node environment, no setupFiles, no env, @ -> ./src alias. resolve.alias lives at the config root rather than inside the project so the `rules`/`integration` projects Phase 2 adds can inherit it without this file being reshaped (inline projects need `extends: true` to actually inherit root options - verified empirically, the docs undersell how load-bearing that flag is). `npm test` runs `vitest run --project unit`; added `engines.node`. Extracted four pure modules out of components/pages so they're testable without a browser or a live Firestore connection, each imports nothing but @/lib/types and (for urlUtils) zod: - lib/utils/commentUtils.ts: buildCommentTree, moved out of app/qna/[id]/page.tsx verbatim. - lib/utils/questionListUtils.ts: filterQuestions/sortQuestions, moved out of QuestionList.tsx. Fixes #40: sortQuestions now sorts a copy instead of calling Array.prototype.sort() on the array that was also the `questions` React state, which silently reordered it in place whenever no filter was active. MyForumsList.tsx had the identical shape (search filter + activity-desc sort) and now shares the same two functions instead of its own copy. - lib/utils/tagUtils.ts: addTagNormalized. Fixes #38: QuestionForm compared the *raw* tag input against the (lowercased) stored list, so "React" typed after "react" passed the duplicate check and got added anyway. Normalizing once up front before the dedupe check fixes it. QuestionEditForm already normalized correctly but is rewired to the same helper so the two forms can't drift again. - lib/utils/urlUtils.ts: isSafeHttpUrl + safeHttpUrl (Zod refinement). Fixes #20: empirically confirmed (against this project's installed Zod 3.24.2) that z.string().url() accepts javascript:, data:, vbscript:, and ftp: URLs - it only checks for *an* absolute URL, not a safe scheme. Allowlisting http/https via the WHATWG URL parser also closes the case/leading-whitespace/embedded-newline bypasses for free, since the parser normalizes all of those before exposing `.protocol`. Rewired EventForm's posterImageUrl and rsvpLink, the only two `z.string().url()` call sites in the codebase (grepped). 41 unit tests across 5 files cover all of the above, including a side-by-side test asserting bare z.string().url() still accepts javascript: so the refinement doesn't quietly get "simplified" away.
There was no ESLint config and no eslint dependency at all - "lint": "next lint" had nothing to run, and next lint is deprecated in Next 15 and removed in 16. Adds eslint ^9.39.5, eslint-config-next@15.5.23 (pinned to match the installed Next version), and @eslint/eslintrc for FlatCompat. eslint.config.mjs extends next/core-web-vitals only, with ignores in the config itself (flat config doesn't read .eslintignore). "lint" now runs "eslint .". Measured before gating: a first `eslint .` run reported 36 errors, all react/no-unescaped-entities (unescaped apostrophes/quotes in JSX text) across ~15 files unrelated to this change, plus one pre-existing react-hooks/exhaustive-deps warning. Demoted react/no-unescaped-entities to 'warn' rather than fixing unrelated file content in a test- infrastructure change; --max-warnings is left unset. `eslint .` now exits 0 with 33 warnings (32 react/no-unescaped-entities that were errors before, plus the pre-existing exhaustive-deps one). Also deletes src/app/auth/page copy.tsx and page copy 2.tsx - the App Router only registers the exact filename page.tsx, so these two were unrouted dead code (confirmed unimported anywhere) that would otherwise sit there getting type-checked and linted indefinitely. Removing them dropped 4 of the 36 errors above.
…55) Adds two new Vitest projects alongside the existing unit suite, both running exclusively against the Firestore/Auth emulators: - rules: exercises the default-deny firestore.rules shipped in #56 at the real paths the app uses (questions, events, users, roleChanges, the legacy top-level comments collection from #55, the comments subcollection, and the 4-segment votes/questions/{qid}/{uid} path), denying both unauthenticated and authenticated callers, and get/list separately. - integration: end-to-end session and updateUserRole coverage against real Auth + Firestore, proving the #17 contract that role and permission decisions always come from the caller's Firestore document, never the session cookie's own claims - including a live role-flip test and a revoked-session-cookie test (both verified against the emulator). Structurally guarded (tests/helpers/assertEmulator.ts) against ever targeting a non-demo-* project id, so this can never reach the real v-threads project; verified by deliberately pointing the integration project at a non-demo id and confirming it refuses to run before touching either emulator. @firebase/rules-unit-testing is pinned to ^4 rather than the newer ^5 line, which requires firebase ^12 - this app is on firebase ^11, and bumping that major version is out of scope here. Also notes (not fixed, out of scope for this phase): updateUserRole's self-escalation guard can never actually throw under any input, since requireAdmin() already restricts callers to role 'admin' - the maximum ROLE_RANK value - before that check runs.
requireAdmin() guarantees caller.role is 'admin', which is ROLE_RANK's maximum, so ROLE_RANK[role] > ROLE_RANK[caller.role] is false for every target role. The branch cannot fire today. Found while writing the integration test for it, which failed asserting the guard's own message and got 'Admin privileges required.' instead. Kept as defence in depth in case requireAdmin() is ever relaxed, but comment it so nobody mistakes it for active protection.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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.
Adds the emulator-backed half of the test suite: 30 rules tests proving Firestore denies client access, and 17 integration tests proving the authorization guard from #17 actually fires.
Stacked on #61 (unit suite, type errors) and #62 (#17's authz fix). Merge order: #61 → #62 → this. Once those land, this PR's diff reduces to the test files alone.
Why this exists
Before this, the two most important security properties in the codebase were verified by hand, once:
firestore.rulesgenuinely denies client access (Ship firestore.rules and move privileged writes to the Admin SDK #16)updateUserRolegenuinely rejects a non-admin caller (Enforce admin authorization server-side in updateUserRole() #17)A manual probe proves something was true on one afternoon. These suites prove it on every run.
Rules suite — 30 tests
Runs
@firebase/rules-unit-testingagainst the committedfirestore.rules, at the real paths the code uses rather than invented ones:questions,events,users,roleChanges—getandsetdeniedstudent-1— also denied. This is the test that matters: "default-deny" is easy to misread as "signed-in users can read", and that assumption is exactly what a future rules change would breakquestions/{qid}/comments/{cid}— the real subcollection path fromcommentService.ts, not a top-levelcommentsguesscommentscollection as well, because it genuinely exists in production as legacy data (Remove orphaned top-level comments collection with plaintext emails #55) even though no current code touches itvotes/questions/{qid}/{uid}— the real 4-segment path fromvoteService.ts. Confirmed the library handles a 4-segment path without complaintlistas well asgetonquestions, since they are separate verbs and denying one does not deny the otherIntegration suite — 17 tests
The design decision worth reviewing: the cookie jar never fabricates a cookie value.
signInAs(uid, { role })creates a real Auth-emulator user, writes a realusers/{uid}document, mints a real custom token, exchanges it for a real ID token via the emulator's REST endpoint, then calls the actualPOSThandler fromsrc/app/api/auth/session/route.tsso the realverifyIdTokenandcreateSessionCookierun.Only the HTTP cookie transport is faked, via a
next/headersalias. Token minting, signature verification, issuer and audience checks, revocation checking and the Firestore role load are all genuine. A test that hand-wrote a cookie string would prove nothing about verification — which is the whole thing under test.Coverage is described in #62. The load-bearing case: mint a cookie while admin, change the Firestore role to
user, andrequireAdmin()must throw — proving the role comes from Firestore rather than the bearer token.Three structural locks against touching production
The live project
v-threadsholds real student data, so "don't point tests at production" is enforced rather than documented.tests/helpers/assertEmulator.tsruns before anysrc/import and:FIRESTORE_EMULATOR_HOSTis set — which alone pins the Firestore transport to localhost regardless of credentialsdemo-.v-threadsfails this by nameFIREBASE_SERVICE_ACCOUNT_JSON/_PATH/GOOGLE_APPLICATION_CREDENTIALSfrom the environmentLock 2 was exercised deliberately: pointing the integration project at
v-threadsmakes the suite refuse to run — it aborts at setup in 281ms, before any emulator call, and exits 1.Commands
npm run testdeliberately stays JVM-free so the majority of contributions get a signal without installing anything. The globs are disjoint, so it is structurally incapable of picking up an emulator test.Both emulator scripts use
firebase emulators:exec, which owns startup, readiness detection and teardown including on failure — chosen over backgrounding plus asleep, which is the usual source of emulator flake in CI.Prerequisite worth documenting
The Firestore emulator needs JDK 21 or newer —
firebase-toolsrejects 17 outright. macOS ships ajavastub that looks present and fails on invocation, producing a confusing error. This needs to land inCONTRIBUTING.mdwhen that arrives.Deviations from the original design, with reasons
@firebase/rules-unit-testing@^4rather than^5. v5 requiresfirebase@^12; this app depends onfirebase@^11.8.1as a real client-SDK dependency used throughoutsrc/. Bumping that major to satisfy a test library would risk the live app. v4.0.1 declaresfirebase: ^11.0.0as a peer and installed clean with no--legacy-peer-deps.fileParallelismsits at the roottestconfig, not per-project — Vitest 3.2.7's project config type excludes it, confirmed bytscrather than assumed. Harmless, since each script runs exactly one project.@alias explicitly rather than relying onextends: trueto merge alias objects, since whether that merges or replaces was not established.Finding surfaced by writing a test
updateUserRole's self-escalation branch is unreachable — see #62 for the detail. It was found because the test asserted the guard's own error message and got a different one. That is the suite paying for itself before it merged.Verification
npm run test→ 41 passed, with no emulator runningnpm run test:rules→ 30 passednpm run test:integration→ 17 passednpx tsc --noEmit→ 0 errors, with the new test files included in the typechecknpm run build→ succeeds, 17 routesNot done
Not a fresh-clone run (
rm -rf node_modules && npm install).firebase-toolsresolves as a local devDependency rather than a hoisted global, and the scripts rely on npm'snode_modules/.binPATH, but the full cold path is untested. Verified on macOS only.