Cleanup, and the decisions that were left open - #325
Conversation
**The second sanitizer is gone.** `sanitizeInput` stripped markup, truncated every string at 10,000 characters and rejected any prose containing SQL keywords — "select a track from the list" among them. Nothing called it: the request path runs `scrubMarkup`, which refuses executable markup and passes everything else through byte for byte. Only the tests called it, so the suite was describing behaviour the product did not have. They now target the real one. **`publicProcedure` requires a database.** Without it a public query with no connection fell through to its own `if (!db)` branch, which returns null — and the funnel page renders that as "nothing announced yet". A misconfigured deploy looked like an empty product instead of a broken one. **Cache invalidation actually evicts.** `deletePattern` anchors with `$`, so the wildcard-free entries matched no real key — and because the map HAS an entry, the namespace fallback was skipped, so nothing at all was evicted. Also adds the caller's own keys to events.checkIn, so a member who just scanned sees the change. **Security logging goes to stderr**, not to a file under the source tree that does not exist in the container, where the ENOENT was swallowed by an empty callback — the one message saying security logging had stopped was itself silently dropped. Dead exports removed with it. Decisions, all recorded in PLAN.md §5: - **D2** — registration is an application. `approved` to form a team, `checked_in` to submit: acceptance is a decision made weeks earlier and says nothing about whether somebody turned up, and judging is in person against a table number. Every participant is created `pending` and `pending` used to pass, so teams formed and projects were submitted with no review at all. - **D4** — points removed. Every club check-in carried the schema default and the attendee CSV exported that constant as a "Points" column: a fabricated number presented as data. - **D5** — a `not_accepted` audience, excluded from every other one so it can only be sent on purpose, with a drafted message prefilled into an untouched form. - **D7** — the security headers ship report-only until CSP_ENFORCE=true, with /api/csp-report collecting violations; the 238-line module nothing imported is deleted. `camera` is deliberately absent from Permissions-Policy: /scan reads QR codes through getUserMedia. - **D9** — /docs is linked from the admin nav. - **W22** — initiative decisions are emailed. People applied and heard nothing. Database: packages/db/ddl/2026-08-08-drop-points.sql (D4). Verified: typecheck, 420 tests, lint --max-warnings 0, build.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_1638eca8-9c8c-4215-a551-17eea6e017d9) |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Visit the preview URL for this PR (updated for commit dcb83da): https://hacklytics2027--pr-325-oiydu4kv.web.app (expires Sun, 16 Aug 2026 02:11:55 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 Sign: c48ba34db61581e25fe2978355160b5eefe0e83f |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| // ({"csp-report": {...}}) and the newer Reporting API array. Log whichever | ||
| // arrives rather than parsing both into one shape — this is a diagnostic, | ||
| // not a data pipeline. | ||
| console.warn("[CSP] violation report:", body.slice(0, MAX_REPORT_BYTES)); |
|
| Filename | Overview |
|---|---|
| sites/mainweb/app/api/csp-report/route.ts | Adds CSP report collection and a pre-read size check, but missing Content-Length is coerced to zero and leaves oversized unauthenticated bodies buffered. |
| packages/api/src/routers/hackathon/admin.ts | Adds guarded single and bulk checked-in transitions, preventing pending, rejected, and waitlisted participants from bypassing approval. |
| packages/api/src/routers/team.ts | Tightens team participation to admitted applicants and project submission to checked-in participants. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[POST /api/csp-report] --> B[Read Content-Length]
B --> C{Finite and <= 8 KiB?}
C -->|No| D[Return 413]
C -->|Yes, including absent header coerced to 0| E[Buffer complete body]
E --> F{String length <= 8192?}
F -->|No| D
F -->|Yes| G[Write CSP report to stderr]
Reviews (2): Last reviewed commit: "fix: gate check-in on acceptance, and ca..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Both points from Greptile on #325. **`checked_in` was reachable from any status.** D2 makes it an authorisation state — submitting a project requires it — so setting it on somebody still pending hands them the whole event with no review having happened. The attendees screen offers "Select all N matching", so one wrong click could do that to every applicant at once. The single-participant path now refuses and names the remedy: accept them first, then check them in. The batch path filters in the WHERE instead of refusing whole, because a 2000-row selection with a few unreviewed applicants in it should still admit everybody else — and it now reports how many rows it skipped, so a check-in run that quietly did less than it looked like cannot pass for a complete one. **The CSP report cap was documentation, not a limit.** `await request.text()` buffers the entire request before the length check runs, so the endpoint had already accepted whatever was sent. Content-Length is checked first, and an absent or unparseable one is refused: this endpoint is unauthenticated and a browser sending a violation report always declares a length. The post-read check stays, because Content-Length is the sender's claim rather than the truth. The transition guard is mutation-tested. Verified: typecheck, 423 tests, lint --max-warnings 0, build.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_54461c3c-065e-4ee9-89f4-e38c517c8090) |
|
Both correct. Fixed in dcb83da.
Guard is mutation-tested; three tests cover refusal, the message, and re-checking in somebody already inside (ordinary, not a transition). CSP report size — you are right that it was documentation rather than a limit: Gate: typecheck · 423 tests · lint |
| const declared = Number(request.headers.get("content-length")); | ||
|
|
||
| if (!Number.isFinite(declared) || declared > MAX_REPORT_BYTES) { |
There was a problem hiding this comment.
When an unauthenticated request omits Content-Length, headers.get() returns null and Number(null) becomes zero, so the request passes this guard and request.text() buffers the complete body before rejecting it. This retains the memory-exhaustion path that the pre-buffer limit is intended to close.
How this was verified: The current route passes Number(null) === 0 through the pre-read guard and then calls request.text() before checking the actual body.
* fix(hackathon): make check-in reachable, or nothing can be submitted #325 made submitting a project require `checked_in`. Nothing in the product could set it. `scanParticipantPass` — the badge scan at the door — recorded attendance for one event and deliberately left the roster alone, and no screen ever passed `checked_in` to `updateParticipantStatus`: the attendees table only ever sends approved, rejected or waitlisted. So the terminal reachable status was `approved`, and every submission returned "You need to check in at the event before submitting. Find a volunteer and have your badge scanned" — telling the attendee to do the exact thing they had just done. No project could be submitted for the entire event. The first scan is now the check-in: an approved participant is promoted and `checkedInAt` is stamped once, so the arrival time keeps pointing at when they arrived rather than at their most recent meal. Cache eviction stays narrow — the event, plus the one person scanned — because this runs at every door station all weekend. Also adds a manual check-in action to the attendees table. The scan needs an event to scan somebody into, and submission must not depend on the schedule having been built yet. The qr-checkin test that asserted the old behaviour is updated rather than deleted: it was right while the status was only a label, and the comment now records why that changed. Found by an adversarial review pass after #325 merged — two independent agents traced every writer of registrationStatus and every UI call site. Greptile reviewed the same diff and did not catch it, and neither did 423 passing tests, because every one of them sets the fixture status directly. Verified: typecheck, 425 tests, lint --max-warnings 0, build. The promotion is mutation-tested. * fix(hackathon): make the scan promotion atomic and conditional Both points from Greptile on #327 — the same family as the earlier ones. **A concurrent revocation could be overwritten.** The status was read at the top of the procedure and the write went out by participant id alone, so an organiser rejecting or waitlisting somebody between the two had that decision silently reversed — handing submission rights back to a person who had just been removed from the event. `registrationStatus = 'approved'` is now in the WHERE, so the promotion is a compare-and-set and the loser changes nothing. **A half-finished scan could not be retried.** The attendance insert committed before the promotion, so a failure in between left attendance recorded and the roster untouched — and the retry hits the duplicate guard ("already checked into"), which meant the promotion could never happen and that attendee could not submit for the rest of the event. Both writes are one transaction now, so a failure rolls the attendance back and a rescan is the fix. Verified: typecheck, 426 tests, lint --max-warnings 0, build. The compare-and-set is mutation-tested — dropping the status predicate fails the new test.
…le (#328) #325 deleted packages/api/src/middleware/http-security.ts and left its subpath in the package exports map, so `@query/api/middleware/http-security` — the specifier the manifest says is valid — resolves to a file that is not there. Nothing imports it, so nothing is broken today; this only removes the trap. Three entries were already dangling before this: ./middleware, ./client and ./trpc-server all point at files that do not exist. Left alone deliberately — they predate this work and removing them is a separate decision — but they are worth a look. Verified: typecheck, 426 tests, lint --max-warnings 0, build.
2 participants
Last in the stack.
Cleanup that was more than cosmetic
The second sanitizer is gone.
sanitizeInputstripped markup, truncated every string at 10,000 characters, and rejected any prose containing SQL keywords — "select a track from the list" among them. Nothing called it. The request path runsscrubMarkup, which refuses executable markup and passes everything else through byte for byte. Only the tests called it, so the suite was describing behaviour the product did not have; they now target the real one.publicProcedurerequires a database. Without it, a public query with no connection fell through to its ownif (!db)branch — and those returnnull, which the funnel page renders as "nothing announced yet". A misconfigured deploy looked like an empty product instead of a broken one.Cache invalidation actually evicts now.
deletePatternanchors with$, so"events:list"matched no real key (events:list:all,events:list:public) — and because the map had an entry, the namespace fallback was skipped, so nothing at all was evicted.events.checkInalso picks up the caller's ownevents:my*/events:stats*, so a member who just scanned sees the change instead of scanning again.Security logging goes to stderr. It appended to
packages/api/src/.security-errors.log— a source path that does not exist in the container — with the ENOENT swallowed by an empty callback. The one message saying security logging had stopped was itself silently dropped.The decisions (all recorded in PLAN.md §5)
approvedto form a team,checked_into submit. Every participant is createdpendingandpendingused to pass, so teams formed and projects were submitted with no review at all while organisers were shown an approve/reject screen that decided nothing but an email. Submitting needs the badge scan because judging is in person against a table number — a project that cannot be visited cannot be judged.not_acceptedaudience — rejected and waitlisted applicants, excluded from every other audience so it can only be sent on purpose, with a drafted message prefilled into an untouched form.CSP_ENFORCE=true, with/api/csp-reportcollecting violations; the 238-line module nothing imported is deleted.camerais deliberately absent fromPermissions-Policy—/scanreads QR codes throughgetUserMedia./docsis linked from the admin nav.Two runbook steps became load-bearing:
checked_in.Before enforcing the CSP
Run a real event-day path — Stripe payment,
/scancamera, QR print view — and read the[CSP]lines from the logs. The Stripe hosts in the policy are load-bearing; miss one and payments fail at the last step for everyone.Database
packages/db/ddl/2026-08-08-drop-points.sql(D4).Verification
typecheck · 420 tests · lint
--max-warnings 0· build. I also checked this branch does not revert anything merged earlier in the stack — the claim/lock/safeLogId/keepCompletedfixes from #318, #320 and #323 are all still present, and the only deletions are the three intended ones.Note
High Risk
Changes hackathon authorization, mass admin actions, auth env, and CSP/Stripe/camera paths—operational runbook steps (review before team window, badge scan before submit) and CSP enforcement need verification before production.
Overview
This PR closes several product decisions (D2, D4, D5, D7, D9, W22) and fixes infrastructure bugs where tests described behavior the live stack did not run.
Hackathon admission (D2) — Registration is treated as an application: only
approvedorchecked_incan form/join teams; project submission requireschecked_in. Admins cannot check inpending/rejectedapplicants (single and batch paths reportskipped). The submit UI warns before the form is filled in.Announcements (D5) — Adds a
not_acceptedaudience (rejected + waitlisted) with an admin UI template; other audiences stay exclusive.Points removed (D4) — API, UI, and schema drop gamification points; DDL
2026-08-08-drop-points.sqldrops the columns.Input & security — Removes unused
sanitizeInput/http-security.tsand thehellorouter. Request path usesscrubMarkup(reject executable markup; no SQL-keyword guessing or silent truncation).publicProcedurenow usesrequiresDbso a missing DB fails visibly. Rate limiting: violation decay fromlastViolation, idle bucket eviction fixed, array cap 2500, cache invalidation patterns use wildcards (events:list*, etc.). Security logs go to stderr;TRUSTED_PROXY_HOPSis set inapphosting.yaml.Auth & access — GitHub OAuth env vars in App Hosting; member directory
list/getByIdare admin-only. Initiative application/proposal decisions send email after commit.Frontend — Report-only CSP +
/api/csp-report; Judge Portal link for staff judges; Docs in admin nav; Firebase drops unuseddsgt-portalhosting target.Reviewed by Cursor Bugbot for commit dcb83da. Bugbot is set up for automated code reviews on this repo. Configure here.