Convex authorization hardening — close the data-plane exposure (40 functions) - #70
Conversation
The Convex data plane is directly reachable, bypassing SvelteKit auth. Ten public functions were callable unauthenticated. Each remediated at the function layer with the repo's existing patterns; caller routes thread the secret in lockstep so anonymous UX flows keep working and only DIRECT Convex calls now reject. - server-secret gate (requireInternalSecret): analytics.incrementBatch, positions.register, templates.findBySlug, verify.getCredentialByHash, debates.updateArgumentScores, waitlist.join - requireAuth self/owner/member scope: users.getIdentityForAtlas + getIdentityForEngagement (arbitrary-userId mDL oracle -> self only), templates.patchMetadata (owner), organizations.getBySlug (member, matching getDashboard), campaigns.getCampaignByDebateId (matching getCampaignForDebate) - verify.getCredentialByHash: reject empty hash, use by_credentialHash index instead of a full-table scan - waitlist.join: collapse the created/exists membership oracle to opaque success - tsconfig: ignoreDeprecations 6.0 (TS 6.0.3 requires it for downlevelIteration)
…c functions Systematic pass over the S1 classification (431 functions -> 40 exposed; A1 closed the 10 pentest findings, this closes the other 29 + 1 REVIEW). Each remediated with the repo's existing patterns; caller routes thread the secret in lockstep. - server-secret (requireInternalSecret): authOps.validateSession, events.rsvp/checkin, positions.batchRegister/confirmSend, networks.checkMembership, users.upsert/count/ list registration paths, legislation, deliveries, campaigns — anonymous/machine routes thread getInternalSecret() - requireAuth self/owner/member scope: templates.listByOrg/findByContentHash, campaigns, authOps.backfillTokenIdentifier (derives userId), debates campaign-spawn - output sanitization (mirror getBySlugPublic): templates.list/getBySlug/search — drop userId/contentHash/reputationDelta/moderation/verification fields + K-floor counts - internalize (zero public-api callers): users.bindIdentityCommitment (mDL binder, already test-guarded against api. calls), cutover.listActiveCredentials (admin script updated to internal. ref), templates.getUserOrgId, revocations, submissions HELD for founder decision: debates.updateStatus — conditional org-role gate lets an org-less debate be resolved by any authed user, but its CRON evaluate caller + Tier-3 resolve caller make the fail-close a governance-policy call. Left unchanged, flagged.
…s.search The final adversarial review caught templates.search sanitizing with a DENYLIST (stripInternal) where its sibling list used an allowlist — still leaking recipientConfig (plaintext recipient emails), messageBody, deliveryConfig, cwcConfig, sources, reviewedBy (moderator userId), and (vector branch) draft/private templates. - add shared toPublicTemplate() allowlist projection (13 named fields + optional _score); list + both search branches route through it; delete stripInternal/ stripEmbeddings entirely - semantic + keyword search paths post-filter to status=published && isPublic (neither index enforces isPublic; vector index enforces no status either) - networks.checkMembership: _secret v.optional -> v.string() (handler requires it) - campaigns.getDeliveryMetrics: non-existent campaign throws the same message as non-member (close the existence oracle) - hooks: move backfillTokenIdentifier after convexToken assignment so its requireAuth resolves (was firing pre-assignment, always throwing — inert migration)
…ateStatus) updateStatus writes debate resolution outcomes (status, winningStance, winningArgumentIndex, aiResolution, aiPanelConsensus, appealDeadline) but its org editor/owner check was CONDITIONAL on the debate's template having an orgId — so an org-less (person-layer) debate could be resolved by any authenticated direct Convex caller. Person-layer debates are legitimate (templates can lack an org), so fail-closing them was not an option. - add required _secret + requireInternalSecret as the PRIMARY control (matches the 17 other secret-gated functions); all 4 server callers thread getInternalSecret() - remove the top-level requireAuth: the CRON evaluate/governance-resolve routes carry no user session, so it would throw 'Not authenticated' on those paths once live - keep the org editor/owner check as defense-in-depth, gated on an authenticated identity so it still enforces on the two user-session routes (resolve, settle) Direct callers without the secret are now rejected for ALL debates, org-less included.
Deploying communique-site with
|
| Latest commit: |
237503a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a94454a2.communique-site.pages.dev |
| Branch Preview URL: | https://convex-authz-hardening.communique-site.pages.dev |
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. 📝 WalkthroughWalkthroughThis PR adds internal-secret and authentication/authorization gating across numerous Convex functions (mutations, queries, actions), converts several previously-public queries/mutations to internal-only, refactors template public projections into a shared helper, and updates all corresponding SvelteKit server call sites to supply the internal secret. ChangesInternal auth hardening across Convex and SvelteKit
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant SvelteRoute as SvelteKit Server Route
participant ConvexFn as Convex Function
Client->>SvelteRoute: HTTP request
SvelteRoute->>SvelteRoute: getInternalSecret()
SvelteRoute->>ConvexFn: serverMutation/Query(args, _secret)
ConvexFn->>ConvexFn: requireInternalSecret(_secret)
ConvexFn-->>SvelteRoute: result
SvelteRoute-->>Client: response
sequenceDiagram
participant Hooks as hooks.server.ts
participant AuthOps as convex/authOps.ts
participant Users as convex/users.ts
Hooks->>AuthOps: validateSession(_secret, sessionId)
AuthOps->>AuthOps: requireInternalSecret(_secret)
AuthOps-->>Hooks: session/user data
Hooks->>Hooks: mint Convex JWT
Hooks->>Users: backfillTokenIdentifier() if missing
Users->>Users: requireAuth(ctx) to resolve userId
Users-->>Hooks: patched user
Related Issues: Not specified in the provided data. Related PRs: Not specified in the provided data. Suggested labels: security, backend, convex Suggested reviewers: Not specified in the provided data. 🐰 hopping through secrets, gate by gate, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🪓 Brutalist Review
All four critics independently converge on one headline issue: debates.updateStatus enforces org editor/owner role only inside if (identity), so authorization silently no-ops whenever a user-session route reaches Convex without a forwarded JWT (token-minting degradation, or the tier-3-only resolve route). Native Claude and the GLM client verified that serverMutation does forward the identity on the happy path, so this is a conditional fail-open (High) rather than the unconditional Critical bypass Agy claimed. Secondary agreement: getDeliveryMetrics mislabels a missing campaign as a membership failure (low, not a leak — identical string on both paths), and the templates projection refactor (stripEmbeddings→toPublicTemplate) tightened search/textSearch to require isPublic but left list filtering only status=published. Disagreements resolved by verification: the 'frontend store brick' (Agy) is overstated since api.templates.list feeds only /api/health; getBySlug/backfillTokenIdentifier/getCampaignByDebateId were confirmed non-bugs by both Claude critics. Newly grounded: pii-smoke.mjs E2E calls submitAction without the now-required _secret and will break CI.
Inline comments: 5 (4 🟠 high · 1 🟡 medium)
Per-CLI breakdown
✅ Claude (default, 389951ms)
Native Claude traced each flagged item to source and call sites. Verdict: core hardening is sound — getInternalSecret fails closed, every secret-gated function has its call site updated, getBySlug/backfillTokenIdentifier/getCampaignByDebateId 'concerns' do not reproduce. Real residual issues: updateStatus identity-optional authz (medium/high), getDeliveryMetrics wrong error message (low), dual-secret auth hot-path operability (medium), toPublicTemplate undefined K-floor (low), bindIdentityCommitment dead code (low).
✅ Codex (default, 260356ms)
Codex was blocked by a local sandbox failure and read PR #70 via the GitHub connector instead. Flagged updateStatus/settle authorization mismatch (high), templates.list leaking non-public published rows while search was tightened (high), backfillTokenIdentifier token-propagation coupling (medium), toPublicTemplate as an API-breaking allowlist (medium), organizations.getBySlug redefined to require membership (medium), upsertRegistration ambient-secret superuser write (medium), getDeliveryMetrics wrong error + N+1 (low/medium).
✅ agy (Gemini 3.5 Flash (Medium), 274609ms)
Agy called updateStatus a Critical fail-open bypass and the toPublicTemplate projection a Critical frontend-store brick + search field-mapping mismatch. It correctly found pii-smoke.mjs E2E breakage (submitAction missing _secret) and the getDeliveryMetrics 404-as-403 error. The frontend-store 'brick' claim is overstated — api.templates.list feeds only /api/health, not the store — so that severity is downgraded here.
✅ glm (Claude) (glm-5.1, 492099ms)
GLM-routed Claude client did deep call-site tracing and confirmed the load-bearing assumption that serverQuery/serverMutation forward locals.convexToken. Rated updateStatus fail-open High, bindIdentityCommitment trust-shift Medium, templates four-shape projection Medium, getDeliveryMetrics error Low. Explicitly cleared getCampaignByDebateId, organizations.getBySlug (authed-only callers), and backfillTokenIdentifier as false alarms; flagged listAwaitingGovernance as un-scoped.
Out-of-diff findings (12)
security
- 🟡 medium
convex/templates.ts— Codex [unanchored]: templates.list leaks non-public published templates while search paths were tightened - 🔵 low
convex/users.ts— Claude [sub-threshold]: bindIdentityCommitment: ownership check removed, now dead internalMutation over Sybil/merge primitive - 🔵 low
convex/users.ts— Codex [sub-threshold]: upsertRegistration became an ambient-secret superuser write over Shadow Atlas state - 🔵 low
convex/debates.ts— glm (Claude) [sub-threshold]: listAwaitingGovernance now requires auth but is not org-scoped
testing
- 🟡 medium
tests/e2e/pii-smoke.mjs— agy [unanchored]: pii-smoke E2E calls submitAction without the newly-required _secret — will fail in CI
perf
- 🔵 low
convex/campaigns.ts— Codex [unanchored]: getDeliveryMetrics remains an N+1 dashboard query
maintainability
- 🔵 low
convex/organizations.ts— Codex [unanchored]: organizations.getBySlug redefined from public lookup to membership-gated — name is now a trap - 🔵 low
src/hooks.server.ts— Claude [sub-threshold]: validateSession is now on the per-request auth hot path behind a dual-configured secret - ⚪ nit
tsconfig.json— agy [sub-threshold]: ignoreDeprecations silences TS deprecation warnings instead of fixing them
correctness
- 🔵 low
convex/campaigns.ts— Claude [sub-threshold]: getDeliveryMetrics throws a membership error when the campaign is simply missing - 🔵 low
convex/campaigns.ts— agy [sub-threshold]: Missing campaign masked as 403 membership failure - 🔵 low
convex/templates.ts— glm (Claude) [sub-threshold]: toPublicTemplate K-floor returns undefined (not null) for absent counters
Brutalist orchestrator schemaVersion=1 · context_id=7bdf398f-a27f-4e86-9355-9b2b3dbe189e
| if (debate.templateId) { | ||
| const template = await ctx.db.get(debate.templateId); | ||
| const templateOrgId = template?.orgId; | ||
| if (templateOrgId) { |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[glm (Claude) 🟠 high] security — Absence-of-identity-as-authorization is a copyable fail-open primitive
Pre-PR this handler ran requireAuth(ctx) unconditionally. The PR relaxed that to enforce role only when an identity is present, to accommodate the CRON path. The result is that the Convex function cannot tell an authorized operator (CRON, no identity) from a JWT-minting failure or a future buggy route — both present as null identity + valid _secret. The resolve route makes it concrete: it gates on tier-3 only, runs only unauthenticated queries before the mutation, and mintConvexToken returns '' when the key is unset (hook swallows the throw), so the call reaches Convex unauthenticated and the editor check no-ops. Beyond the immediate risk, this pattern gets copied — once identity-optional authz exists, the next maintainer reaches for it under deadline.
| .unique(); | ||
| if (!membership || (membership.role !== "owner" && membership.role !== "editor")) { | ||
| throw new Error("Only org editors/owners can change debate status"); | ||
| const identity = await ctx.auth.getUserIdentity(); |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Claude 🟠 high] security — updateStatus infers authorization from absence of identity — fail-open when JWT minting is degraded
updateStatus is gated only by requireInternalSecret. Org editor/owner enforcement runs only inside if (identity). When a user-session route calls this mutation but locals.convexToken is unset (JWT key rotation gap, env drift, mintConvexToken returning '' and the hook swallowing the throw), ctx.auth.getUserIdentity() is null and the whole role check silently no-ops while the mutation still executes on the valid _secret. The function cannot distinguish the legitimate CRON/operator path (no identity, gated by CRON_SECRET in the route) from a token-minting failure on a user route. This is fail-open authorization dressed as defense-in-depth. Fix: pass an explicit operator credential the function recognizes, and require editor/owner unconditionally otherwise; make the resolve route fail-closed by running one requireAuth'd query before calling updateStatus.
| if (!membership || (membership.role !== "owner" && membership.role !== "editor")) { | ||
| throw new Error("Only org editors/owners can change debate status"); | ||
| const identity = await ctx.auth.getUserIdentity(); | ||
| if (identity) { |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[Codex 🟠 high] security — settle/resolve routes rely on the identity-optional branch for editor/owner enforcement
The settle route authorizes callers via getCampaignByDebateId, which only checks membership (any role), not editor/owner; the resolve route gates on tier-3 only, not org membership at all. Neither route itself enforces editor/owner — they lean entirely on updateStatus's if (identity) branch. If identity is absent on the mutation call, any member (settle) or any tier-3 user (resolve) passes and the role check is skipped. Campaign settlement authority collapses from editor/owner to member under a single transport/auth-context mismatch. Split cron/operator status mutation from user status mutation, or pass an explicit actor and enforce role for user paths unconditionally.
| const membership = await ctx.db.query("orgMemberships") | ||
| .withIndex("by_userId_orgId", (q) => q.eq("userId", userId).eq("orgId", templateOrgId)) | ||
| .unique(); | ||
| if (!membership || (membership.role !== "owner" && membership.role !== "editor")) { |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟠 high
[agy 🟠 high] security — Role check nested under if(identity) skips enforcement when Convex call carries no JWT
The editor/owner membership check is nested under if (identity). If SvelteKit token minting fails transiently (hooks.server.ts catches the mint error and fails open) or propagation fails, serverMutation calls Convex without a JWT, getUserIdentity() returns null, and the entire identity-gated auth block is skipped while the mutation still runs on _secret. A low-privilege user could then drive an org-tied debate to resolved/appealed during token-generation degradation. (Note: native-critic tracing confirms serverMutation does forward locals.convexToken on the happy path, so this is conditional on JWT degradation, not an unconditional bypass — hence High, not Critical.)
|
|
||
| return { | ||
| templates: scored, | ||
| templates: scored.map((t) => toPublicTemplate(t, t._score)), |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[agy 🟡 medium] maintainability — search result field renames (camelCase _id/_score) risk breaking client search consumers
search now maps results through toPublicTemplate returning _id and _score (camelCase, projected allowlist). Client-side search/cache code (embedding-search.ts) was reported to expect id/verified_sends/unique_districts/similarity. If the consumer keys don't align, cache hits and similarity mapping silently degrade to id:undefined / similarity:0. NOTE: the companion claim that the frontend templates store (isTemplate validator) gets 'bricked' is OVERSTATED — api.templates.list is consumed only by /api/health, and the store's isTemplate runs on reactive add/update paths, not on list results. Treat this as a search-path contract change needing a shared PublicTemplate type + contract test, not a store crash.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
convex/authOps.ts (1)
486-486: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid
ctx: any; use ConvexQueryCtx/MutationCtx.The changed handler signatures type
ctxasany, discarding type safety across the DB/auth surface. Prefer the Convex-provided context types.♻️ Suggested typing
- handler: async (ctx: any, { _secret, sessionId }): Promise<ValidateSessionResult> => { + handler: async (ctx: QueryCtx, { _secret, sessionId }): Promise<ValidateSessionResult> => {- handler: async (ctx: any): Promise<null> => { + handler: async (ctx: MutationCtx): Promise<null> => {As per coding guidelines: "Strive for strong type safety. Avoid using
anywhenever possible in TypeScript".Also applies to: 533-533
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/authOps.ts` at line 486, The handler signatures in authOps are weakening type safety by using ctx: any. Update the affected handlers (including ValidateSession-related ones) to use the appropriate Convex context types, such as QueryCtx or MutationCtx, matching whether the handler reads or writes. Keep the existing handler logic intact, but replace the any annotation so the DB/auth surface stays strongly typed.Source: Coding guidelines
src/routes/s/[slug]/+page.server.ts (1)
152-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
getInternalSecret()for the remaining_secretcalls in this loader too.
getInternalSecret()throws on missing or short config, whileenv.INTERNAL_API_SECRETcan flow through asundefinedand turn a misconfiguration into a silent auth failure.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routes/s/`[slug]/+page.server.ts around lines 152 - 156, The loader still mixes direct env access with internal-secret handling, so update the remaining `_secret` usages in the `+page.server.ts` loader to consistently call `getInternalSecret()`. In the relevant server query calls around `serverQuery(...)`, replace any `env.INTERNAL_API_SECRET` usage with `getInternalSecret()` so missing or invalid config fails fast instead of silently passing `undefined`. Keep the fix localized to this loader and align it with the existing `getInternalSecret()` call already used nearby.convex/templates.ts (2)
1172-1181: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPrefer the
by_userId_orgIdindex over.filteringetUserOrgId.
.filter((q) => q.eq(q.field("userId"), userId))scans allorgMemberships. Theby_userId_orgIdindex can serve this with auserIdprefixeq.♻️ Proposed indexed lookup
- const membership = await ctx.db - .query("orgMemberships") - .filter((q) => q.eq(q.field("userId"), userId)) - .first(); + const membership = await ctx.db + .query("orgMemberships") + .withIndex("by_userId_orgId", (q) => q.eq("userId", userId)) + .first();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/templates.ts` around lines 1172 - 1181, The getUserOrgId internalQuery is doing a full orgMemberships scan via .filter instead of using the by_userId_orgId index. Update the handler to query orgMemberships with the by_userId_orgId index using the userId prefix eq, then return the first matching membership as orgId or null, keeping the existing getUserOrgId shape and behavior.
1139-1149: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse the
by_userId_contentHashindex instead of a full-table.filter.
.filter(...)scans everytemplatesrow before matching. The schema already declaresby_userId_contentHash, so this can be an indexed point lookup.♻️ Proposed indexed lookup
- const templates = await ctx.db - .query("templates") - .filter((q) => - q.and( - q.eq(q.field("userId"), authUserId), - q.eq(q.field("contentHash"), contentHash), - ), - ) - .first(); + const templates = await ctx.db + .query("templates") + .withIndex("by_userId_contentHash", (q) => + q.eq("userId", authUserId).eq("contentHash", contentHash), + ) + .first();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@convex/templates.ts` around lines 1139 - 1149, The template lookup in the handler still uses a full-table ctx.db.query("templates").filter(...) scan; switch it to the existing by_userId_contentHash index for a point lookup. Update the query in this handler to use the index with authUserId and contentHash as the lookup keys, then keep the existing .first() behavior and downstream logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@convex/debates.ts`:
- Line 1135: The `listAwaitingGovernance` query is only protected by
`requireAuth(ctx)`, so any signed-in user can read awaiting-governance debates.
Update this handler to enforce the intended governance/operator role check in
addition to authentication, using the existing auth helpers/patterns in
`convex/debates.ts`. If the broader access is intentional, make that decision
explicit in the access control logic and keep the query’s protection aligned
with the governance dashboard’s intended audience.
---
Nitpick comments:
In `@convex/authOps.ts`:
- Line 486: The handler signatures in authOps are weakening type safety by using
ctx: any. Update the affected handlers (including ValidateSession-related ones)
to use the appropriate Convex context types, such as QueryCtx or MutationCtx,
matching whether the handler reads or writes. Keep the existing handler logic
intact, but replace the any annotation so the DB/auth surface stays strongly
typed.
In `@convex/templates.ts`:
- Around line 1172-1181: The getUserOrgId internalQuery is doing a full
orgMemberships scan via .filter instead of using the by_userId_orgId index.
Update the handler to query orgMemberships with the by_userId_orgId index using
the userId prefix eq, then return the first matching membership as orgId or
null, keeping the existing getUserOrgId shape and behavior.
- Around line 1139-1149: The template lookup in the handler still uses a
full-table ctx.db.query("templates").filter(...) scan; switch it to the existing
by_userId_contentHash index for a point lookup. Update the query in this handler
to use the index with authUserId and contentHash as the lookup keys, then keep
the existing .first() behavior and downstream logic unchanged.
In `@src/routes/s/`[slug]/+page.server.ts:
- Around line 152-156: The loader still mixes direct env access with
internal-secret handling, so update the remaining `_secret` usages in the
`+page.server.ts` loader to consistently call `getInternalSecret()`. In the
relevant server query calls around `serverQuery(...)`, replace any
`env.INTERNAL_API_SECRET` usage with `getInternalSecret()` so missing or invalid
config fails fast instead of silently passing `undefined`. Keep the fix
localized to this loader and align it with the existing `getInternalSecret()`
call already used nearby.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 337d16bd-72ea-4f4c-a7b4-1287e84771ac
📒 Files selected for processing (40)
convex/analytics.tsconvex/authOps.tsconvex/campaigns.tsconvex/cutover.tsconvex/debates.tsconvex/events.tsconvex/legislation.tsconvex/networks.tsconvex/organizations.tsconvex/positions.tsconvex/revocations.tsconvex/schema.tsconvex/submissions.tsconvex/templates.tsconvex/users.tsconvex/verify.tsconvex/waitlist.tsscripts/cutover-v1-credentials.tssrc/hooks.server.tssrc/routes/api/admin/reconcile-registrations/+server.tssrc/routes/api/analytics/increment/+server.tssrc/routes/api/debates/[debateId]/evaluate/+server.tssrc/routes/api/debates/[debateId]/governance-resolve/+server.tssrc/routes/api/debates/[debateId]/resolve/+server.tssrc/routes/api/debates/[debateId]/settle/+server.tssrc/routes/api/deliveries/record/+server.tssrc/routes/api/e/[id]/checkin/+server.tssrc/routes/api/e/[id]/rsvp/+server.tssrc/routes/api/org/check-slug/+server.tssrc/routes/api/positions/batch-register/+server.tssrc/routes/api/positions/confirm-send/+server.tssrc/routes/api/positions/register/+server.tssrc/routes/api/templates/+server.tssrc/routes/api/v1/networks/[id]/stats/+server.tssrc/routes/api/waitlist/+server.tssrc/routes/c/[slug]/+page.server.tssrc/routes/embed/campaign/[slug]/+page.server.tssrc/routes/s/[slug]/+page.server.tssrc/routes/verify/[hash]/+page.server.tstsconfig.json
…ure explicit CodeRabbit flagged the requireAuth-only gate as needing an explicit decision. It is deliberate: the participatory-governance queue is community-visible by design, with an allowlist projection (no PII/internal fields) and K-floored participation counts as the privacy control — not operator-only. Documented in place; no behavior change.
|
There was a problem hiding this comment.
🪓 Brutalist Review
All four critics independently converge on one headline: debates.updateStatus makes the org editor/owner check conditional on if (identity), so it fails open for any identityless server call carrying only the shared _secret (agy called it Critical; native Claude, Codex, and glm confirmed it is not exploitable in current routes because convexToken is forwarded, but rate it High for the fail-open fragility). The second cross-cutting theme is the shared-secret design: _secret is passed as a Convex function argument (likely persisted in Convex logs — unverified but flagged by two critics) and now stands in for user/org/operator/retry-queue authority across ~40 functions, so one leak has a large blast radius (unauthenticated writers like incrementBatch). Critics disagree on bindIdentityCommitment (Codex High vs Claude/glm Low) — resolved to Medium because independent verification found zero callers, making it latent dead code rather than a live vuln. The PR is a net security improvement (default-deny sweep + a genuine backfillTokenIdentifier IDOR fix), but before merge it needs: updateStatus fail-closed with a named operator mode, confirmation Convex redacts _secret args, and cleanup of the orphaned bindIdentityCommitment.
Inline comments: 6 (1 🟠 high · 5 🟡 medium)
Per-CLI breakdown
✅ Claude (default, 512762ms)
Native Claude traced every critical path end-to-end (secret impl, both auth directions, all SvelteKit callers). Headline: updateStatus fail-open org check (High) and _secret-as-argument logging exposure (Medium). Verified many non-regressions and credited the backfillTokenIdentifier IDOR fix. Flagged templates.list missing isPublic filter and orphaned bindIdentityCommitment dead code.
✅ Codex (default, 261699ms)
Codex (sandbox blocked; reviewed via GitHub connector) focused on failure modes: updateStatus identity-optional bypass (High), bindIdentityCommitment lost ownership (High, judge-downgraded to Medium as dead code), upsertRegistration trusts KV retry input (Medium), validateSession auth coupled to secret config (Medium), getBySlug semantic break (Low), textSearch window drop (Low), non-constant-time comparison (Low).
✅ agy (Gemini 3.5 Flash (Medium), 140009ms)
Antigravity produced a dependency/sequence diagram of the SvelteKit->Convex auth bridge. Rated updateStatus fail-open as Critical (judge-downgraded to High — no live always-on bypass). Also flagged unindexed identityCommitment table scans (perf) and the bufferEq length side-channel (Low).
✅ glm (Claude) (glm-5.1, 538597ms)
glm-routed Claude client gave the most thorough verification: confirmed convexToken forwarding (so updateStatus not exploitable today but still High/fail-open), detailed the _secret logging blast radius (incrementBatch etc. have no rate limit if the secret leaks), the silent auth-outage footgun (Medium), and credited the backfillTokenIdentifier IDOR fix. Verified verify/rsvp/checkin/waitlist callers all thread _secret correctly.
Out-of-diff findings (5)
security
- 🔵 low
convex/_internalAuth.ts— Codex [unanchored]: Convex secret comparison is not byte-constant-time and short-circuits per branch - 🔵 low
convex/_internalAuth.ts— agy [unanchored]: Length timing side-channel in bufferEq leaks secret length
perf
- 🔵 low
convex/users.ts— agy [unanchored]: Unindexed table scan for identityCommitment dedup in bindIdentityCommitment/finalizeMdlVerification
correctness
- 🔵 low
convex/templates.ts— Codex [sub-threshold]: textSearch post-filters after taking a small candidate window
design
- 🔵 low
convex/organizations.ts— Codex [sub-threshold]: getBySlug keeps a public-looking name but is now member-only
Brutalist orchestrator schemaVersion=1 · context_id=c8206d49-f017-4647-9e79-94dcf9caa8e0
| if (!membership || (membership.role !== "owner" && membership.role !== "editor")) { | ||
| throw new Error("Only org editors/owners can change debate status"); | ||
| const identity = await ctx.auth.getUserIdentity(); | ||
| if (identity) { |
There was a problem hiding this comment.
🪓 Brutalist — 4 critics, rollup: 🟠 high
[agy 🟠 high] security — Authorization bypass path in debates.updateStatus when JWT minting fails
agy rated this Critical; downgraded to High on judge review because thorough tracing (native + glm) confirmed the identity is forwarded on the live user-session routes, so there is no always-on bypass. The valid core: the mutation enforces owner/editor only if a user identity is present. SvelteKit's settle/resolve endpoints require locals.user but delegate org-membership checks entirely to Convex. If mintConvexToken throws and leaves locals.convexToken undefined, the mutation executes with a null identity and the org check is bypassed — the design fails open rather than closed. Recommend requiring identity for org-tied debates unless an explicit operator flag (gated separately by CRON_SECRET) is set.
[Claude 🟠 high] security — debates.updateStatus: org-authz is conditional on caller-supplied identity (fail-open)
The org editor/owner check for org-tied debates is nested inside if (identity). When ctx.auth.getUserIdentity() returns null (any server-to-server call without a forwarded JWT), the entire membership check is silently skipped and only the org-agnostic _secret gate remains. Native review confirmed this is NOT exploitable today — serverMutation forwards locals.convexToken, so user-session routes (resolve/settle/governance-resolve) attach a JWT and the check runs, while the operator CRON route carries no identity and is CRON-gated. It stays High because the shared _secret encodes trusted-origin, not which-org/which-user: a transient JWT-mint failure (hooks.server.ts try/catch leaves locals.convexToken unset while locals.user stays populated) or a future route using a bare ConvexHttpClient silently gains cross-org debate-status mutation. Fail-open authorization dressed as defense-in-depth. Fix: require the org check for org-tied debates and make the no-identity path a named, separately-gated operator mode.
[glm (Claude) 🟠 high] security — updateStatus org check is best-effort; 'no identity' is the default for server calls
The whole org boundary for debate status rests on the if (identity) branch, and 'no identity' is the default for any server-to-server call carrying only _secret. Not exploitable in current routes (verified: convexToken is forwarded for user routes; evaluate is CRON-gated), but the moment a future route threads _secret without locals — or uses a bare ConvexHttpClient — it silently gains cross-org debate-status mutation (flip any org's debate to resolved/awaiting_governance/under_appeal). This is a copy-paste template other maintainers will replicate. Make the org check non-optional and add a named operator mode.
[Codex 🟠 high] security — updateStatus identity-optional org check turns route-level mistakes into status-takeover
requireAuth(ctx) was replaced with requireInternalSecret(args._secret), and the org editor/owner check only runs inside if (identity). Changed callers (resolve/settle/evaluate/governance-resolve) pass only _secret. If the Convex call is identityless — which the new comments explicitly allow for operator routes — org-tied debate status changes skip Convex org authorization entirely, and /api/debates/[debateId]/resolve only checks 'authenticated tier 3+' at the route, not org editor/owner. Impact: non-org users or a compromised/misconfigured server route path can resolve org-linked debates and set winners. 'Internal' is not an org-scoped authorization model.
| export const validateSession = query({ | ||
| args: { sessionId: v.string() }, | ||
| handler: async (ctx: any, { sessionId }): Promise<ValidateSessionResult> => { | ||
| args: { _secret: v.string(), sessionId: v.string() }, |
There was a problem hiding this comment.
🪓 Brutalist — 2 critics, rollup: 🟡 medium
[Claude 🟡 medium] security — Shared secret transported as a function argument (validateSession) — likely captured in Convex logs
Every gated function takes _secret: v.string() as a normal Convex call argument rather than a header. Convex persists function-call arguments in its logs/dashboard by default, so the long-lived, reused-everywhere INTERNAL_API_SECRET may be written to observability on every gated call — and validateSession is the hottest path (every authenticated request via hooks.server.ts), making it the most frequently logged secret in the deployment. Anyone with dashboard/read-logs access (ops, support, a leaked read-only token, a log-forwarding integration) could recover the master secret, which unlocks all ~40 gated functions. Contrast the inbound direction (secret-auth.ts), which correctly uses an x-internal-secret header. Mechanism unverified — confirm whether Convex redacts _secret args; if not, this is already leaking. Prefer internalQuery/internalMutation (as done for cutover/revocations/submissions) wherever the caller is Convex-internal.
[glm (Claude) 🟡 medium] security — _secret-as-argument on the highest-traffic path; blast radius includes unauthenticated writers
The _secret arg pattern makes the secret the entire perimeter for ~40 functions and travels on validateSession, the hottest call in the system. If Convex isn't redacting the arg (unverified — must be confirmed, not assumed), the secret leaks here first. Blast radius if leaked: incrementBatch is a public mutation with no auth, no rate limit, no per-caller bound (rate-limiting offloaded to SvelteKit), and the same is true for events.createRsvp, events.publicCheckIn, waitlist.join, debates.updateArgumentScores — one leaked secret enables unbounded direct multi-table writes and cost-DoS, bypassing every SvelteKit gate. Reserve the _secret-arg pattern for genuinely HTTP-reachable functions and verify arg redaction.
| return { ...result, page: result.page.map(stripEmbeddings) }; | ||
| return { | ||
| ...result, | ||
| page: result.page.map((template) => toPublicTemplate(template)), |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Claude 🟡 medium] security — templates.list is missing the isPublic filter its siblings enforce
The public list query filters only status="published" (via by_status) and projects through toPublicTemplate, but every sibling public surface also gates on isPublic: listPublic (if (!t.isPublic) return false), search and textSearch (.filter(t => t.status === 'published' && t.isPublic)). A template that is status:"published" but isPublic:false (org-internal published) is therefore enumerable by any unauthenticated caller via list, exposing slug/title/description/domain and K-floored metrics — the existence and content of a template the org marked private. Confidence caveat: only exploitable if the published+isPublic:false state actually occurs. If the state exists, add .filter(q => q.eq(q.field('isPublic'), true)) to match the siblings.
| * (returns the canonical userId). Otherwise patches the current user. | ||
| */ | ||
| export const bindIdentityCommitment = mutation({ | ||
| export const bindIdentityCommitment = internalMutation({ |
There was a problem hiding this comment.
🪓 Brutalist — 2 critics, rollup: 🟡 medium
[Codex 🟡 medium] security — bindIdentityCommitment lost its self-ownership invariant
Converted to internalMutation with the only requireAuth + args.userId === authUserId ownership check removed. The function still patches an arbitrary caller-supplied userId with identityCommitment, isVerified: true, verificationMethod: 'mdl', and verifiedAt. codex framed this High as raw authority to mark any account verified; judge review (corroborated by native + glm) verified it currently has ZERO callers, so it is dead code and not a live vuln — hence Medium (latent footgun). 'Internal' is not an ownership model: any future internal caller that forwards a request-controlled userId grants identity-binding / verification-tier escalation onto another account. Fix: delete it or restore the ownership assertion so nobody wires a new caller into the unguarded version.
[Claude 🔵 low] maintainability — bindIdentityCommitment is now orphaned dead code with its invariants silently migrated
Repo-wide search returns only the definition, a test asserting the route does NOT reference it, and archived docs describing a documented TOCTOU account-merge fix (F-R3-12) that lived here. There is no internal.users.bindIdentityCommitment caller anywhere, so dropping the ownership check is safe today — but the function is dead and its merge/reauth logic was re-implemented inline in finalizeMdlVerification without parity tests against the original fix. Leaving a security-sensitive, invariant-bearing function unreferenced is how a future 'let me just call this internal helper' reintroduces the TOCTOU the team already fixed once. Delete it or wire the intended caller.
| handler: async (ctx, args) => { | ||
| const { userId: authUserId } = await requireAuth(ctx); | ||
| if (args.userId !== authUserId) throw new Error('Unauthorized'); | ||
| requireInternalSecret(args._secret); |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] security — upsertRegistration now trusts retry/admin KV input for arbitrary users
The requireAuth + args.userId !== authUserId ownership check was removed and replaced with requireInternalSecret(args._secret). The changed caller (api/admin/reconcile-registrations/+server.ts) reads userId, identityCommitment, leafIndex, merkleRoot, and merklePath from KV retry JSON and writes them under the internal secret. Poisoned KV, a buggy producer, or a compromised CRON_SECRET can overwrite Shadow Atlas registration state for any user; Convex does not cross-check that the registration commitment matches the target user's current identity commitment. Impact: corrupted proof state, wrong leaf assignments, verification records detached from the actual user.
| } | ||
|
|
||
| const result = await serverQuery(api.authOps.validateSession, { sessionId }); | ||
| const result = await serverQuery(api.authOps.validateSession, { _secret: getInternalSecret(), sessionId }); |
There was a problem hiding this comment.
🪓 Brutalist — 1 critic, rollup: 🟡 medium
[Codex 🟡 medium] correctness — Session validation now depends on internal-secret deployment parity
getInternalSecret() is called inline on every request carrying an auth cookie. It throws 'INTERNAL_API_SECRET not configured' when the env is missing/short; that throw is caught at the surrounding try/catch, logged as a '(transient)' error, and the user is set to locals.user = null. A single missing or under-length secret in SvelteKit therefore logs every user out (cookies remain, but every session fails to validate and can't re-validate) — a deployment footgun. The direction is fail-closed (safe), but it is mislabeled 'transient' when it is a hard config outage, and it degrades silently to anonymous rather than erroring loudly. Fail fast at boot with a distinctive log and a failing health check instead.
1 participant
What
The Convex data plane (quirky-chinchilla-352.convex.cloud) is directly reachable and bypasses SvelteKit auth. A pentest confirmed 10 live findings (mDL/identity oracle, raw-doc PII leak, k-anon-defeating writes, credential table scan, metric/waitlist oracles). A full classification of all 431 public functions found 391 already safe (the entire paid /api/v1 org surface was already secret-gated + org-scoped) and 40 exposed — concentrated in the person layer, exactly where the pentest hit.
Approach — default-deny with the repo's own patterns
Each of the 40 remediated at the Convex function layer; caller routes thread the secret in lockstep so anonymous UX flows keep working while direct Convex calls reject.
requireInternalSecret, existing_internalAuth.ts): 17 anonymous-but-sensitive fns (waitlist, positions, analytics, events, session validation…)getBySlugPublic): 3 leaky reads (templates list/search/getBySlug — denylist helpers deleted)Commits (structured, one concern each)
cd561f24— the 10 confirmed findings3b0bf8f1— the 39-function default-deny sweep285ea2a3— integrated-review blocker (templates.search allowlist)1582b186— org-less debate resolution bypassVerification
svelte-check 0 errors; change-scoped tests green at every wave; 3-lens integrated adversarial review (caught + fixed the search denylist leak). Every fix carries a code-level negative control (unauth call rejects; legit path works).
Deploy note
Code-level only until
convex deploy. Requires INTERNAL_API_SECRET set in Convex prod (gates) AND CF Pages prod (route threading) — deploy both together.Summary by CodeRabbit
New Features
Bug Fixes