Conversation
…op resetting dry-run on deploy
… and pet category on institutional path
…e quota on expire, safer FK
…restrict institution deletion
… submits, cap rate_limit_per_day
…osed on query error
…ire dry-run config
… R2 uploads daily
There was a problem hiding this comment.
Pull request overview
Adds an institutional lost & found “MVP” flow (token-gated reporting + partner audit tooling) and extends retention/cleanup to cover institutional expiry and abandoned uploads, alongside several security/operational hardening measures.
Changes:
- Introduce institutions schema + RPCs for institutional insert, audit expiry, and 30-day auto-expire (plus retention cleanup updates).
- Add institutional UI routes (
/report/inst,/i/[slug]/[audit_slug]) and admin CRUD + printable QR poster. - Harden uploads and analytics: HMAC-bound upload tokens, R2 orphan sweep, stricter Referrer-Policy, PostHog URL sanitization, and admin action guarding.
Reviewed changes
Copilot reviewed 50 out of 51 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| workers/data-retention/wrangler.toml | Adds worker config var for image base URL used by the orphan sweep. |
| workers/data-retention/src/index.ts | Splits retention work into independent jobs; adds institutional expiry + R2 orphan sweep. |
| tests/hurl/reset.sh | Resets new institutional tables between Hurl runs. |
| tests/hurl/institutional-url-token-not-in-html.hurl | Regression test ensuring QR bearer token is not rendered into HTML. |
| tests/hurl/institutional-rate-limit.hurl | Verifies per-institution daily rate limiting + monotonic counters. |
| tests/hurl/institutional-preflight.hurl | Tests preflight validation to avoid orphaning uploads. |
| tests/hurl/institutional-no-referrer-header.hurl | Ensures credential-bearing routes emit Referrer-Policy: no-referrer. |
| tests/hurl/institutional-invalid-token.hurl | Validates token/slug/category/image_url rejection paths. |
| tests/hurl/institutional-happy.hurl | End-to-end institutional submission success path. |
| tests/hurl/institutional-auto-expire.hurl | Tests the institutional auto-expire RPC (dry-run + live). |
| tests/hurl/institutional-audit-expire.hurl | Tests audit kill-switch expiry endpoint behavior and authorization. |
| tests/hurl/institutional-anon-no-credentials.hurl | Verifies anon cannot read secret institution credentials via PostgREST. |
| tests/hurl/admin-auth-gates-actions.hurl | Regression test for guarding admin form actions (not only GET loads). |
| supabase/seed.sql | Seeds test institutions + hashed tokens for automated specs. |
| supabase/migrations/20250101000013_institutional_retention.sql | Extends retention cleanup to cover expired institutional rows. |
| supabase/migrations/20250101000012_institutional_auto_expire.sql | Adds RPC to expire old institutional items (service-role only). |
| supabase/migrations/20250101000011_institutions.sql | Adds institutions table, public view, FK on items, rate-limit table, and RPCs. |
| src/routes/api/upload/+server.ts | Returns an HMAC upload_token alongside image URL to bind upload→insert. |
| src/routes/api/items/+server.ts | Adds institutional submission path, token-in-cookie support, and upload ownership verification. |
| src/routes/api/institution/preflight/+server.ts | Preflight endpoint to check token/category/quota before uploading an image. |
| src/routes/api/institution/expire/+server.ts | Audit kill-switch endpoint to expire an institutional item via bearer audit slug. |
| src/routes/(main)/report/inst/+page.svelte | Institutional report form UI (token hidden via cookie; preflight + upload token). |
| src/routes/(main)/report/inst/+page.server.ts | Validates ?t= token, stores it in HttpOnly cookie, redirects to tokenless URL. |
| src/routes/(main)/reply/[id]/+page.svelte | Adds PostHog capture for reply opened/sent events. |
| src/routes/(main)/item/[id]/resolve/+page.svelte | Adds PostHog capture for resolve success/failure. |
| src/routes/(main)/item/[id]/+page.svelte | Renders institutional pickup block; suppresses peer contact/resolve flows for institutional items. |
| src/routes/(main)/i/[slug]/[audit_slug]/+page.svelte | Audit UI to list items and trigger expiry (“Remove”). |
| src/routes/(main)/i/[slug]/[audit_slug]/+page.server.ts | Server-side audit auth and queries for active + limited inactive institutional items. |
| src/routes/(main)/admin/institutions/[id]/poster/+page.svelte | A4 print layout for QR poster (noindex + no-referrer). |
| src/routes/(main)/admin/institutions/[id]/poster/+page.server.ts | SSR QR generation (via qrcode) and token verification before rendering. |
| src/routes/(main)/admin/institutions/+page.svelte | Admin institutions list + create form; shows one-time plaintext token + URLs. |
| src/routes/(main)/admin/institutions/+page.server.ts | Service-role backed institutions CRUD (create action returns token + URLs). |
| src/routes/(main)/admin/+page.svelte | Adds PostHog capture when resolving items from admin UI. |
| src/routes/(main)/+page.svelte | Includes institution_id in item list queries for UI gating/styling. |
| src/lib/utils/upload-signing.ts | Implements HMAC signing/verification for uploaded R2 keys. |
| src/lib/utils/mock-data.ts | Adds institution_id to mock items. |
| src/lib/utils/institution-token.ts | Token/audit slug generation + SHA-256 hashing utility. |
| src/lib/utils/hours.ts | Validates and formats weekly opening-hours JSON. |
| src/lib/types/item.ts | Adds institution + hours-related types; extends Item with institution_id. |
| src/lib/posthog.ts | Sanitizes URLs sent to PostHog to strip bearer token query params. |
| src/lib/i18n/is.json | Adds institutional UI translations (Icelandic). |
| src/lib/i18n/en.json | Adds institutional UI translations (English). |
| src/lib/components/ResolveModal.svelte | Adds method property to resolve analytics event. |
| src/lib/components/ReportForm.svelte | Sends upload_token when submitting peer-to-peer items with images. |
| src/lib/components/Map.svelte | Adds institutional marker style (color + building glyph). |
| src/lib/components/ItemPreview.svelte | Adjusts preview UI and hides contact for institutional items. |
| src/hooks.server.ts | Adds global admin guard for all admin routes + conditional no-referrer header. |
| pnpm-lock.yaml | Locks added QR code dependencies and transitive packages. |
| package.json | Adds qrcode and @types/qrcode. |
| docs/testing/institutional-mvp-test-plan.md | Adds a detailed local/manual test plan for institutional MVP. |
| .github/workflows/deploy-prod.yml | Enforces explicit institutional expiry dry-run config and writes worker secret. |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const { data: rows, error } = await supabase | ||
| .from('items') | ||
| .select('image_url') | ||
| .not('image_url', 'is', null); |
There was a problem hiding this comment.
runOrphanUploadSweep loads referenced items.image_url values with a single PostgREST .select() call (no pagination). Supabase/PostgREST responses are typically capped (often 1k rows), so referenced may be incomplete and the sweep can delete images that are still referenced by older item rows beyond the first page. Please paginate through items (e.g., range() loop) or move this into an RPC that returns all referenced keys (or checks existence for a candidate batch) before deleting from R2.
| [vars] | ||
| IMAGE_BASE_URL = "https://img.fundid.is" | ||
|
|
There was a problem hiding this comment.
The worker uses IMAGE_BASE_URL to decide which items.image_url values are “ours”, but the app uses PUBLIC_IMAGE_BASE_URL for upload/URLs. Having two separate base-URL configs risks drift (and then the orphan sweep would treat all images as unreferenced and delete them). Consider reusing the same variable name/value across the app + worker (or at least documenting that they must match and wiring one from the other in deploy config).
| - [ ] `pnpm check` returns 0 errors | ||
| - [ ] Full Hurl suite green after `reset.sh` | ||
| - [ ] M1–M8 manual paths pass in Chrome + Safari (iOS if possible, for the submission flow) | ||
| - [ ] `INSTITUTIONAL_EXPIRE_DRY_RUN=true` confirmed in `workers/data-retention/src/index.ts` default |
There was a problem hiding this comment.
The ship gate checklist says INSTITUTIONAL_EXPIRE_DRY_RUN=true is the default in workers/data-retention/src/index.ts, but the worker code actually requires an explicit value of "true" or "false" and skips the institutional expiry run otherwise. Please update this checklist item to match the current behavior (and/or reference the GitHub Actions variable as the source of truth).
| - [ ] `INSTITUTIONAL_EXPIRE_DRY_RUN=true` confirmed in `workers/data-retention/src/index.ts` default | |
| - [ ] `INSTITUTIONAL_EXPIRE_DRY_RUN` is explicitly set to `"true"` or `"false"` (no worker-code default; `deploy-prod.yml` / Actions config is the source of truth) |
| const contactEmail = (form.get('contact_email') as string)?.trim(); | ||
| const rateLimit = parseInt(form.get('rate_limit_per_day') as string) || 20; | ||
|
|
There was a problem hiding this comment.
rateLimit is parsed as parseInt(...) || 20, which silently turns an explicit 0 (or any falsy value) into 20 and bypasses the intended validation. It would be safer to parse into a number, explicitly handle NaN, and then validate the actual user-provided value so invalid inputs don’t get coerced into the default.
| @@ -0,0 +1,284 @@ | |||
| <script lang="ts"> | |||
| import { getTranslate } from '@tolgee/svelte'; | |||
| import { goto } from '$app/navigation'; | |||
There was a problem hiding this comment.
goto is imported but never used in this component. Removing unused imports helps keep the file tidy and avoids build/lint noise.
| import { goto } from '$app/navigation'; |
… rate_limit coercion, update doc, drop unused import
Summary
/report/insttoken-gated form → auto-expire in 30 days, no claim-code flow/i/[slug]/[audit_slug]) with kill-switch to disable a run-away partner/admin/institutionsCRUD + A4 QR poster via SSRqrcodeINSTITUTIONAL_EXPIRE_DRY_RUNconfig) and a daily R2 orphan sweepHardening
24 rinse iterations on top of the feature commit. Highlights:
items.institution_idFKON DELETE RESTRICT, monotonic daily submission counter/api/uploadverified on insert — blocks cross-item photo spoofingRollout
INSTITUTIONAL_EXPIRE_DRY_RUN=truebefore taggingv1.8.0. Worker fails closed and skips expiry entirely if the value isn't explicitlytrueorfalse.falseonce confident, re-deploy or restart the workerTest plan
pnpm check— 0 errors