Second-pass review — defects and improvements
Read date: 2026-08-19. Found by reading source directly, not by running tests and not by trusting repo docs. Separate from docs/e2e/PRE-RUN-FINDINGS.md (15 items found while writing the E2E suites) — nothing here duplicates that file. Read both.
Same caveat applies: these are code-read findings. Each needs a run to become a defect report with a repro. Nothing here is filed as a work item — that is a backlog call.
S — Security. The upload endpoint is the weak spot
All four of these are the same ~40 lines: src/modules/upload/. It is generic scaffold code that predates the permit domain, and it is now the path certificates and e-signature evidence flow through, which is what makes it worth reading twice.
| # | Finding | Where |
|---|---|---|
| S1 | Stored-XSS vector: Content-Type is taken from the client. putObject is called with 'Content-Type': file.type — a value the uploader controls — and the object is then handed back as a presigned GET URL. Upload text/html, get a URL on the storage origin that the browser renders as a document. The file extension is also taken from the client filename (file.name.split('.') last segment). Fix is an allowlist: sniff the real type, don't echo the declared one. | adapters/storage/minio-storage.ts:60 |
| S2 | No size limit, and the whole file is buffered into memory. Buffer.from(await file.arrayBuffer()) with t.File() carrying no maxSize. One large POST from any authenticated account is an OOM on a single-process Bun server. Elysia's t.File({ maxSize }) covers it in one line. | commands/upload.model.ts:6, minio-storage.ts:53 |
| S3 | subFolder is client-supplied and unsanitized. It goes from request body to object key with only //→/ and trailing-slash cleanup. Any authenticated caller picks the destination prefix — including another module's prefix. Not filesystem traversal (object storage has no .. semantics), but it does mean the key namespace has no server-side owner. | upload.service.ts:11 → minio-storage.ts:57-59 |
| S4 | The returned fileUrl expires in 60 seconds. presignedGetObject(..., 60). Correct as a short-lived handle; a defect the moment any caller persists fileUrl rather than filePath. Worth an audit of every certificate/evidence row: if a stored URL is a presigned one, those images are already dead. | minio-storage.ts:66 |
S5–S7 — Auth surface
| # | Finding | Where |
|---|---|---|
| S5 | Rate limiting exists but is applied to exactly one endpoint. hitRateLimit (a clean Redis fixed-window, correctly written) is used only by GET /qr/:token. Login, password reset and register are unlimited: unlimited credential stuffing, and sendResetPassword is an unmetered outbound-email trigger keyed on an attacker-supplied address. The util already exists — this is wiring, not new code. | libs/utils/rate-limit.util.ts, only consumer permit/queries/qr-verify/…controller.ts:14 |
| S6 | Public signup is open unless an env var is set. disableSignUp: USER_DISABLE_SIGNUP?.toUpperCase() === 'TRUE' — unset means enabled. Anyone can create an account on a plant-safety system. The blast radius is capped (permitRole is input: false and the guard denies a null role, which is the right design), so a self-registered account can reach nothing role-gated — but it is an unbounded row-creation endpoint and it defaults open. Default should be closed, with the env var opting in. | libs/plugins/better-auth/user-auth.plugin.ts:26 |
| S7 | The QR token never expires and cannot be revoked. HMAC-SHA256 over the permit id, timingSafeEqual compared — the crypto is right. But there is no expiry and no nonce, so a token photographed once is a permanent public read of that permit's live status, entrant count and latest gas reading, and re-issuing a QR does not invalidate the old one. Live-status-not-snapshot is deliberate and correct; permanence probably is not. Add an issuance epoch to the payload. | permit/lib/qr-token.util.ts |
Q — Quality gates. What green currently means
| # | Finding |
|---|---|
| Q1 | The API has no typecheck. Already filed (feat-012) and honestly documented in init.sh rather than faked — good. Restating it because it is the largest hole in the workspace's definition of done: a type error lands green on the one repo both frontends' contracts depend on. Fix is a local typescript devDependency (so bunx stops resolving a version this tsconfig cannot satisfy), not a tsconfig rewrite. |
| Q2 | The Contractor app has essentially no component or page tests. 27 test files, of which 17 are utils/ and 2 are wizard schemas. Exactly one file mounts a component (components/input/tests/Switch.test.ts) and no file mounts a page. The sibling Safety/Inspector app has 69 files and 22 that mount, covering every page directory. Same stack, same team, same patterns available — the asymmetry is unexplained, and the Contractor app is the one users self-serve into. |
| Q3 | The audit-log unit test verifies rows that never touch Postgres. This is why P0 #1 in PRE-RUN-FINDINGS.md went unseen. Broader point: audit-log.spec.ts is the only test of the product's tamper-evidence claim, and it tests the pure function rather than the stored artefact. The regression test that matters writes a row, reads it back, and verifies the chain from storage. |
| Q5 | The Safety/Inspector repo could not be committed to at all. lint-staged.config.mjs carried a TypeScript return-type annotation — (): string => 'bunx vitest run' — in a .mjs file. Node cannot parse it, so lint-staged found no valid config, the husky pre-commit hook exited 1, and every commit in that repo failed. The sibling Contractor repo has the identical file without the annotation. Found on 2026-08-19 while taking a restore point, not by reading; it is why 265 files had accumulated uncommitted. Fixed (annotation dropped, hook verified green by an actual commit). |
| Q4 | Three working trees are uncommitted, across many sessions. ~30 modified files in the API alone — the feat-007 contract hardening from 2026-08-17. Both frontends' green baselines were verified against those working trees, not against HEAD. A fresh clone silently regresses both frontends today. This is the highest-severity process item on the page and it is a two-minute fix. |
C — Contract and consistency
| # | Finding |
|---|---|
| C1 | The API throws hardcoded Thai at clients, with no errorCode. NotFoundError('ไม่พบ bucket ที่ระบุ'), BadRequestError('ไม่มีสิทธิ์ในการอัปโหลดไฟล์') in the storage adapters. Both violate CONTEXT.md §2 — clients localize off errorCode and never render message — and neither passes one, so a client that obeys the invariant has nothing to show. Note this is the mirror of finding 6 in PRE-RUN-FINDINGS.md (frontends rendering backend English); the same invariant is broken from both ends. |
| C2 | latestSafetyReading: t.Nullable(t.Any()) in the public QR response. The one endpoint with no auth is also the one with an untyped payload, so it is absent from the generated contract and check-contract-sync.mjs cannot see it. Type it. |
| C3 | CONTEXT.md §4 promises infrastructure the compose file does not provide. Line 118 says the backend "needs Postgres + Redis + MinIO (docker-compose.yml in the repo)" and then tells you to docker compose up -d — but that compose file defines only the api service, and references env vars that are not set. Redis is a hard load-time dependency (redisPlugin is an eager new Redis() at module scope, not lazy), so this is not a soft failure: a reader following our own setup doc gets a connection error that reads like a code bug. The E2E pass corrected this same instruction in the test plan and did not correct it here. This is a defect in my own document. |
P — Product questions surfaced by the code
Not defects. Each is a decision nobody has recorded, and each will otherwise be settled by accident.
- S7 above is really a product question: how long should a printed permit QR stay scannable? The answer sets the token design.
- Open registration (S6): is self-signup a product feature awaiting a role-assignment screen, or scaffold that should be off?
create.service.tsshows provisioning issafety_officer-gated, which suggests the latter. - Certificate evidence retention (S4): is
filePaththe stored reference andfileUrlderived per-request? If so, say it inCONTEXT.md, because the current response shape invites the wrong one.
What I would do first, in order
- Commit the three working trees (Q4). Everything else is measured against a baseline that does not exist in version control. Two minutes.
- The audit chain fix — P0 #1 in
PRE-RUN-FINDINGS.md, plus the storage round-trip test (Q3). This is the product's central integrity claim and it does not currently hold. - Upload hardening (S1–S3). One file, an allowlist and a
maxSize; S1 is the only finding on this page that is remotely exploitable by an outsider. - Rate-limit login and password reset (S5). The util is written; this is three lines at two call sites.
- The API typecheck (Q1). Cheapest permanent raise of what "green" means for the repo both frontends depend on.
Everything else is a backlog conversation, not an emergency.
Triage for the 2026-08-19 fix pass
Agents were spawned to fix the mechanical findings. These were deliberately withheld from them, because each needs a decision only the product owner can make, and an agent handed one will invent an answer:
| Item | The decision you need to make |
|---|---|
| S7 | How long should a printed permit QR stay scannable? That answer designs the token — an issuance epoch, a TTL, a revocation list, or genuinely permanent. |
| Finding 11 | Should risk-map pins be coloured by permit status rather than type? Today status appears only in the tap-through modal, and pin position is a hash of free-text location, so it carries no spatial meaning either. |
| Finding 12 | Should the entrant register be an in/out log, or stay "who is inside now"? Today check-out is observed as a row disappearing and no out-timestamp is retained. |
| Finding 13 | Should a wizard draft resume on reload? Today it does not, and continuing creates a second draft. |
| Finding 15 | Audit-log date filtering: bare YYYY-MM-DD against UTC storage with +7 display. Pick the boundary semantics. |
| S6 (partly) | Signup is being defaulted closed as a security default. If self-signup is a real product feature awaiting a role-assignment screen, say so and it comes back. |
| Q4 | Restore-point commits were taken in all three repos so the agent pass is revertable. Reviewing and squashing that history is still yours. |
Finding 7 needs no fix — the wizard stubs are unbuilt screens, and it is already recorded in the plan and in CT-WIZARD.md so nobody files it as broken.
New findings surfaced by the fix pass (2026-08-19)
Fixing the list turned up things the list did not contain. Recorded here; none is fixed.
| # | Finding |
|---|---|
| N1 | t.File({ maxSize }) and t.File({ type }) are advisory no-ops on Elysia 1.4.28's multipart path. Proven, not assumed — the rejections carry the wrong errorCode, showing the declared constraints never fired. The explicit server-side check is the only real gate. This matters well beyond the upload fix: any other place in this codebase that treats a t.File option as enforcement is unguarded. Worth a sweep. |
| N2 | Certificate attachment is non-functional end to end, silently. The contractor app assigns the 60-second presigned URL as fileRef while the durable path sits unused one line away — and it does not matter, because the API discards the field entirely (no such column, no such body field, Elysia strips unknown keys). The user attaches a file, sees success, and nothing is stored. This is finding S4's "the moment a caller persists fileUrl" hypothesis found live, and worse than predicted. Client half is being fixed; the server half is an API gap. |
| N3 | The tsconfig was never the blocker on the API typecheck. The standing assumption — recorded in init.sh and in my own review as Q1 — was that bunx tsc failed on moduleResolution: node10 / baseUrl. TS 5.9 accepts both. What actually blocks the gate is four pre-existing type errors in application code. The gate was absent for a misdiagnosed reason. |
| N4 | A blocked registration now answers 500 with better-auth's own English and no errorCode. A side effect of defaulting signup closed: the refusal path is better-auth's, not ours, so it violates the §2 invariant and a compliant client has nothing to render. It is now the default path, which makes it worth fixing. |
| N5 | STORAGE_UNAVAILABLE is returned as a 400. It reuses BadRequestError. A dead storage bucket is a server fault; 503 is the honest code. Your call. |
| N6 | Local .env MinIO credentials do not match the running container (minioadmin vs minioadminpassword), so the upload happy path returns STORAGE_UNAVAILABLE on this machine. Pre-existing and environmental, not code. .env was left alone. |
| N7 | Correction — a reported "GET /permits is broken today, returns 422" is FALSE. Verified live against the running server on the post-fix code: both GET /api/v1/permits and GET /api/v1/permits/:id return 200 with createdBy populated, and the responses carry the new validationSummary, proving the server is running current code. The reported fix (add a Prisma include for createdBy/approvedBy/closedBy) is impossible — prisma/models/permit.prisma:21,29,36 declares all three as Json? scalar columns, not relations. They are denormalised author snapshots and need no include; Prisma rejects include on a scalar. The probe that produced the 422 must have bypassed the real read path. |
| N8 | The real remaining type error, correctly diagnosed. list.http.controller.ts(9,13) is a mismatch between Prisma's JsonValue | null for those three Json columns and PermitModel.entity, which declares them required. Not a missing include, and not literal-widening (paginate() already types message as the literal 'success'). The latent risk is real but not live: the DB schema permits createdBy = null while the published contract marks it required, so such a row would 422 on response validation. Unreachable through the API today — create.service.ts:28 is the only create path and always writes it. Fixing this means either loosening the published contract or a non-null migration, so it is a contract decision, deliberately left to you, not an agent edit. This is why the typecheck gate stays unwired and feat-012 stays in-progress. |
| N9 | useUpload.upload() fabricates success. smart-work-permit-contractor-frontend/src/composables/useUpload.ts:41-47 catches every upload failure, toasts a hardcoded Thai Google-Cloud-Storage billing string in an app with no GCS, and returns a fake result — { fileUrl: '/assets/images/logo.png', filePath: '' }. So a failed upload reports success and hands the caller the app's own logo as the user's file. Same shape as finding 6 but outside the handleLoading sweep that fixed it; the caller now drops the empty filePath, but the composable still lies. Not fixed. |
| N10 | check-contract-sync.mjs has a hole I did not document: it validates the contractor app's EApiErrorCode only. The Safety/Inspector app's error-code coverage is not machine-checked at all, so "contract-sync: OK" is not evidence that repo can localize a backend code. Recorded in CONTEXT.md §2; the check itself should be extended to both. |