Security review (2026-05-11): SEC-15 high PII leak on public pilot list, fixed inline#162
Merged
Conversation
SEC-15 (High): GET /api/comp/:comp_id/pilot used optionalAuth and only gated on comp.test, so anonymous callers received the full pilot row for every registered pilot in every public comp — including admin-entered email, the linked Better Auth account email, and driver_contact (the emergency-contact phone number). The route still returns names, classes, teams, glider, and national-body IDs (intentionally public on comp results pages), but the three PII fields are now redacted to null for any caller who is not a comp_admin of that comp. Implementation: new `serializeCompPilotPublic` wraps the existing `serializeCompPilot` and nulls out email / linked_email / driver_contact. The GET handler resolves comp_admin membership once and dispatches between the two serialisers. Four new regression tests cover: anonymous caller (worst case — seeded linked_email path), authenticated-non-admin caller, admin caller (sanity check that PII still surfaces for admin flows), and the existing admin happy-path test. Also appends the 2026-05-11 review round to docs/security-review.md: walks every prior SEC-NN finding against current code, closes the "per-tool MCP auth audit" and "wrangler.toml binding cross-environment audit" scope gaps from the previous round, and records a new scope gap to systematically audit all remaining optionalAuth endpoints for similar PII-on-public-read patterns. bun audit: 0 vulnerabilities. typecheck:all + test:all pass (229 competition-api tests, +3 from this PR). https://claude.ai/code/session_01HEtzn7C9m7QaRyGbUXwfCW
|
Preview Deployment |
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.
Summary
Whole-repo security re-review for 2026-05-11.
bun auditclean. SEC-10 / SEC-11 / SEC-12 fixes from the prior round all hold. One new High finding surfaced and was fixed inline:GET /api/comp/:comp_id/pilotusedoptionalAuthand only gated oncomp.test, so anonymous callers received the full pilot row for every registered pilot in every public comp — including admin-enteredemail, the linked Better Auth account email (linked_email), anddriver_contact(the emergency-contact phone). The route still returns names, classes, teams, glider, and national-body IDs (intentionally public on comp results pages), but the three PII fields are now redacted tonullfor any caller who is not acomp_adminof that comp.Two prior-round scope gaps closed without code changes (auditing surfaced no issues):
web/workers/mcp-api/src/tools/*.tsforwardsapiKeyviacompApi(env, apiKey, …)/compApiRaw(env, apiKey, …). No tool forges identity or short-circuits to admin.New scope gap recorded for the next round: systematically audit every remaining
optionalAuthroute for PII-on-public-read patterns (same class as SEC-15).Full per-finding status table, walk-through, and methodology appended to
docs/security-review.md.SEC-15 details
What was the bypass. The route declared
optionalAuth(notrequireAuth), and the only auth check was an early-return for test comps. For all non-test comps, anonymous callers received the fullserializeCompPilotoutput including:email— admin-entered per-pilot emaillinked_email— Better Auth account email of the linked user (viaLEFT JOIN "user" u ON p.user_id = u.id)driver_contact— free-form emergency-contact phone / WhatsApp handleThe frontend even surfaced
linked_emailas a hover tooltip on the link-status badge for every viewer.How the fix closes it. A new
serializeCompPilotPublichelper wrapsserializeCompPilotand nulls out the three PII fields. The GET handler resolvescomp_adminmembership once and dispatches between the two serialisers. The response shape is unchanged (same key set), so the frontendCompPilotinterface and its null-handling fall-back paths both work without modification.Regression tests that prove it stays closed (new, in
web/workers/competition-api/test/pilot-crud.test.ts):email = linked_email = driver_contact = null, butname/civl_id/linkedpopulated. Seeds apilotrow withuser-3so theLEFT JOIN "user"populateslinked_emailserver-side and the public path is forced to zero it.emailanddriver_contactstill populated end-to-end.user-2) gets the same redacted view as anonymous — being signed in does not grant admin.All 229 competition-api tests pass (+3 from this PR).
Test plan
bun run typecheck:all— all 6 workspaces passbun run test:all— competition-api 229/229, mcp-api 21/21, engine + root suites passbun audit— 0 vulnerabilitiescurl -s https://glidecomp.com/api/comp/<some-public-comp>/pilot | jq '.pilots[0] | {email, linked_email, driver_contact}'should be all-nullX-Glidecomp-Internal-Usershould still yield 401)https://claude.ai/code/session_01HEtzn7C9m7QaRyGbUXwfCW
Generated by Claude Code