================================================================================ GATE 5 — QUALITY & SECURITY AUDIT (Next.js/Tailwind standalone) Ticket : #3248 — OMS UI/UX and functional Branch : feat/3248-oms-ui-ux-and-functional Commits : 5214e01 (Data Layer) .. e8fb724 (Dead Code) [4 commits] Node : W14_gate_quality_audit Result : PASS (0 unresolved P1 IN THIS TICKET'S AUDITED SCOPE) ^^ read section 5 before trusting this word — 2 P1s exist in this repo right now, both OUTSIDE this ticket's changed files. Counts : P1=2 (both out-of-scope) P2=11 (7 in-scope, 4 out) P3=12 (+ 2 clean-audit records, 1 positives record) ================================================================================ -------------------------------------------------------------------------------- 0. SKILL RESOLUTION — READ THIS FIRST -------------------------------------------------------------------------------- The gate's §"Skill resolution" defines three paths per skill. ALL THREE FAILED FOR ALL SEVEN SKILLS in this checkout — identical to the #3246 run, re-verified rather than assumed: path 1 /nextjs-tailwind- -> no such slash command in this session path 2 / (repo's own) -> no such slash command in this session path 3 read_file(".claude/skills/nextjs-tailwind-/SKILL.md") $ ls /opt/drcarrot-deploy/cross-repo-orchestration/oms-web/.claude -> No such file or directory (oms-web has NO .claude/ at all) $ find /opt/drcarrot-deploy -type d -name "nextjs-tailwind-*" -> gates/nextjs-tailwind-standalone (this gate file's own dir) -> nothing else $ find /opt/drcarrot-deploy -type d -name skills -not -path "*/node_modules/*" -> cross-repo-orchestration/dr.carrot-v3-react/.claude/skills -> cross-repo-orchestration/emr_v1_l12/.claude/skills -> /opt/drcarrot-deploy/.claude/skills none of the three contains ANY nextjs-tailwind-* audit skill (listed in full; the closest is dr.carrot-v3-react's generic `security-audit`, which is a different stack's skill) Per the gate's resolution rule 4 ("say so in your report ... do not skip it silently"), this is recorded rather than papered over. Each of the seven audit DIMENSIONS was then executed directly against the real files, using the dimension list the gate file itself enumerates in §"Skills invoked". That is NOT equivalent to running the skills' own tuned checklists — a finding those SKILL.md files would have caught and this pass did not is a plausible gap. Treat the coverage below as gate-file-scoped, not skill-scoped. -------------------------------------------------------------------------------- 1. REAL COMMANDS RUN (actual output, not predicted) -------------------------------------------------------------------------------- $ npx tsc --noEmit -p tsconfig.json -> exit 0, zero diagnostics. tsconfig has "strict": true. $ npx next lint --max-warnings=0 -> "✔ No ESLint warnings or errors" $ npx vitest run --reporter=basic -> Test Files 86 passed (86) Tests 630 passed (630) Duration 226.22s (no failures, no stderr warnings) $ npx next build -> ✓ Compiled successfully; 23/23 static pages generated. First Load JS shared by all 87.3 kB Middleware 30.5 kB Routes relevant to the findings below: /expense-summary 1.55 kB 112 kB First Load /expense-summary/[project] 1.95 kB 112 kB /expenses 2.29 kB 112 kB /orders 2.29 kB 112 kB /orders/[id] 3.92 kB 91.2 kB /admin/items 538 B 114 kB /admin/items/[id] 2.39 kB 93.5 kB /admin/users 538 B 114 kB /admin/users/[id] 137 B 91.3 kB $ ls -la .next/static/css/ ; gzip -c .next/static/css/*.css | wc -c -> one stylesheet, 26,836 B raw / 5,619 B gzipped, whole app SCOPE NOTE. `feature_files` (56 entries) is the audit input. This ticket's real diff (5214e01^..e8fb724, 94 files) also touches data-layer files that are NOT in feature_files but ARE this ticket's own code: lib/actions/uom.ts, lib/queries/masters.ts, lib/quick-create/registry.ts, lib/validation/schemas.ts, prisma/schema.prisma + migration, vitest.config.ts. Because the gate calls the security audit "first-party, not a second-hand contract check" for this repo, those were audited too and are tagged `diff-scope`. Findings in files touched by NEITHER list are tagged `out-of-scope` and are explicitly excluded from gate_status — see section 5. -------------------------------------------------------------------------------- 2. MERGED FINDINGS -------------------------------------------------------------------------------- ### [security] SEC-1 — P1 — OUT-OF-SCOPE (pre-existing) file: app/api/uploads/route.ts:96-101 (POST) scope: `git log --oneline 5214e01^..e8fb724 -- app/api/uploads/route.ts` -> 0 commits. The two call sites (ExpenseForm.handleUpload, OrderFulfillmentForm.handleUpload) ARE in feature_files but their upload code paths are untouched by this ticket's diff. problem: Missing OBJECT-LEVEL authorization before a database write. The route checks `checkPermission("attachment:upload")` — a coarse "may this user upload at all" — then creates an Attachment row using the caller-supplied `orderId` / `expenseItemId` verbatim. Nothing verifies the caller may write to THAT order or THAT expense item. Any holder of `attachment:upload` can attach an arbitrary file to any order or any expense item id they can guess or observe. This is precisely the case the gate calls out: "missing authorization checks before a database write". fixed: NO — the gate's Step 3 forbids auto-fixing an authorization gap ("never guess at the intended permission"). Needs the intended policy: is it order:update? expense:update? ownership-scoped? That is a product decision. ### [security] SEC-2 — P1 — OUT-OF-SCOPE (carried forward from the #3246 gate, STILL OPEN) file: lib/actions/users.ts:55-75 (updateUser) scope: not in feature_files; 0 commits in this ticket's diff. problem: docs/gate-evidence/quality-audit-3246-user-management.txt raised this as its single blocking P1 ("updateUser has NO last-active-Super-Admin guard"). It is still unresolved. #3247 hardened the NEIGHBOURING paths but not this one: - deleteUser (users.ts:89-92) gained an absolute "Super Admin accounts cannot be deleted" check on the TARGET's current role. - updateUser gained only `assertNotSuperAdminRole(data.roleId)` — which inspects the INCOMING roleId, i.e. it blocks PROMOTING someone to Super Admin. It does not look at the target's CURRENT role and does not look at `isActive` at all. So both bricking paths the #3246 evidence documented remain open: submitting a normal roleId for a user who currently holds the Super Admin role demotes them, and submitting isActive=false deactivates them. Verified: `sed -n '55,76p' lib/actions/users.ts | grep "isSuperAdmin\|isActive\|target"` returns nothing, and there is no DB-level protection either (`grep -rin superadmin prisma/schema.prisma prisma/migrations/` finds only the plain `isSuperAdmin BOOLEAN NOT NULL DEFAULT false` column, no CHECK, no trigger). Why it bricks the app is unchanged — see the #3246 file, section SEC-1. fixed: NO — authorization/policy rule, never auto-fixed. ### [code-quality] CQ-1 — P2 — diff-scope file: vitest.config.ts:14-96 (coverage.include) problem: `coverage.include` is a hand-maintained allowlist of ~80 individual file paths with hand-escaped bracket regexes. Anything absent from it is silently excluded from the coverage report — coverage reads "high" while untracked modules contribute nothing. Verified absent: lib/actions/uom.ts <- CHANGED this ticket (quick-create wiring) lib/quick-create/registry.ts<- CHANGED this ticket (uom entry added) lib/actions/orders.ts lib/actions/expenses.ts app/api/uploads/route.ts middleware.ts (`grep -c` for all six against vitest.config.ts -> 0) Concrete consequence: this ticket ADDED tests/actions-uom.test.ts (177 lines) and extended tests/quick-create-registry.test.ts, and neither moves the reported coverage number by a single line, because their subjects aren't in the include list. The repo's own CLAUDE.md rule — "when a script can drop coverage ... make it log/warning that, so silent truncation never reads as 'everything succeeded'" — is the same principle. fixed: NO — changing the include list changes the reported coverage number and could fail a downstream coverage gate; that is an observable-behaviour change, not a safe auto-fix. ### [code-quality] CQ-2 — P2 — in-scope file: components/forms/OrderFulfillmentForm.tsx:59-67 (handleCancel) problem: `cancelOrder` returns `ActionState` (`{ok:false, message}` on failure — see lib/actions/orders.ts:79-85), and handleCancel discards the return value entirely, then calls `router.refresh()` unconditionally. A failed cancel is indistinguishable from a successful one: no message, no error state, the row just re-renders unchanged. Every other action surface in this ticket wires failures through (`state?.message` -> `.field-error`); this one does not. Secondary: "Cancel Order" is destructive and fires immediately on click, while every row-level delete in the same ticket routes through DeleteConfirmModal (components/ui/RowActions.tsx:51-58). Inconsistent destructive-action UX. fixed: NO — adding error surfacing/confirmation changes observable behaviour. ### [dry-architecture] DRY-1 — P2 — in-scope files: 9 list pages, all in feature_files — app/admin/{items,users,projects,suppliers,uom}/page.tsx app/{orders,expenses}/page.tsx app/expense-summary/page.tsx, app/expense-summary/[project]/page.tsx problem: The identical `buildHref` closure is pasted into all nine (18 grep hits for `buildHref`, 9 for `const buildHref`): const buildHref = (extra) => { const p = new URLSearchParams(); for (const [k, v] of Object.entries({ ...params, ...extra })) if (v !== undefined && v !== "") p.set(k, String(v)); return `?${p.toString()}`; }; Only the route literal differs. lib/params.ts is already the shared home for exactly this concern (it owns parseListParams/skipTake/orderBy) — a `buildListHref(pathname, params)` there would delete nine copies. fixed: NO — mechanical, but nine call sites in a file set this gate is auditing rather than authoring; left as a reported finding. ### [dry-architecture] DRY-2 — P2 — in-scope files: components/forms/{Supplier,Project,Uom,Item}Form.tsx (all in feature_files) problem: The four quick-create-capable forms carry a byte-identical embedded scaffold: - `const initialState: ActionState = { ok: true };` 8 copies repo-wide - the onSuccess effect, character-for-character identical in all 4: `if (embedded && state.ok && state.data && onSuccess) onSuccess(state.data);` each preceded by its own `// eslint-disable-next-line react-hooks/exhaustive-deps` - `const fieldWidth = embedded ? "w-full" : "w-full sm:w-[350px]";` - `{embedded && }` - the `{!embedded &&

X Details

}` heading - the Cancel/Submit footer, differing only in label and redirect path A `useEmbeddedForm({embedded, state, onSuccess})` hook plus an `` would collapse this to one definition. The cost is already visible: the eslint suppression is duplicated 4× rather than justified once (see CQ-7). fixed: NO — a shared hook + shell touches four live form components' render output; too broad for an unreviewed auto-fix. ### [dry-architecture] DRY-3 — P2 — in-scope files: components/forms/ExpenseForm.tsx:36-44 (in feature_files) app/expense-summary/[project]/page.tsx:11-19 (in feature_files) lib/validation/schemas.ts:134-142 (diff-scope) problem: The expense-type list exists in THREE places: twice as an identical value/label option array, and a third time as the Zod `z.enum([...])` that actually validates it. Adding a type means editing three files; missing one yields either a type the UI offers but the server rejects, or a stored type the summary screen renders as a raw enum string (`EXPENSE_TYPE_OPTIONS.find(...)?.label ?? r.type` — the fallback silently papers over exactly that drift). fixed: NO — the schema copy is the validation source of truth; deriving the other two from it is the right fix but changes what the Zod enum exports. ### [performance] PERF-1 — P2 — in-scope files: components/ui/ListToolbar.tsx:2, components/ui/FilterBar.tsx:7-8 (both in feature_files) problem, with measured evidence: ListToolbar statically imports FilterBar; FilterBar statically imports Listbox and DatePicker (a hand-rolled ~270-line calendar). ListToolbar only RENDERS FilterBar when `filters && filters.length > 0` — but a runtime conditional does not prune a module-scope client import from the route's client bundle. Measured against the build above: app baseline (First Load JS shared by all) 87.3 kB /expense-summary 112 kB <- passes NO filters /admin/items (passes filters) 114 kB /admin/items/[id] (no ListToolbar) 93.5 kB `/expense-summary` renders `` with no `filters` prop (app/expense-summary/page.tsx:45) and can never display a filter field, yet ships ~25 kB of First Load JS over baseline — within 2 kB of the fully filtered list pages. `next/dynamic` on FilterBar (or on DatePicker inside it) would let the routes that never filter stop paying for the calendar. fixed: NO — converting to a dynamic import changes hydration/loading behaviour on every list screen; wants a ui_verify pass behind it. ### [performance] PERF-2 — P2 — in-scope files: app/orders/[id]/page.tsx:7-14 (in feature_files) app/expense-summary/[project]/page.tsx:21-33 (in feature_files) problem: Both routes fetch the same record twice per render — once in `generateMetadata`, once in the page body: orders/[id] getOrder(params.id) at :8 and :14 (getOrder includes items -> item, uom, attachments) expense-summary/[p] prisma.project.findUnique at :22 and :33 Next.js dedupes `fetch()` across metadata and render, but NOT Prisma calls. Neither `getOrder` nor the inline findUnique is wrapped in React `cache()`. This is not a pattern the codebase is unaware of — lib/queries/rbac.ts:38 wraps `getUserWithPermissions` in `cache()` precisely for this reason, and documents it. These two are the only `generateMetadata` implementations in the app (`grep -rln generateMetadata app/` -> exactly these 2 files), so the fix is two `cache()` wrappers. fixed: NO — trivially correct, but it changes request-scoped caching semantics for a query used on a live route; reported for review instead. ### [performance] PERF-3 — P2 — OUT-OF-SCOPE (pre-existing) file: lib/queries/expenses.ts:49-80 (expenseSummaryByProject) consumed by app/expense-summary/page.tsx:18 (in feature_files) problem: Unbounded full-table read on every page view: - `prisma.expenseItem.groupBy({ by:["expenseId"], _sum:{amount:true} })` with NO `where` — every ExpenseItem row in the database. - `prisma.expense.findMany({ select:{...} })` with NO `where`, NO `take` — every Expense row. - the project rollup, the `params.q` search and the sort then all run in JS over the full set. The consuming page destructures only `{ rows }` and renders no ``, so the whole table is also rendered. Cost grows linearly with total expense volume forever. Compare the sibling `expenseItemsForProject` (same file, :84-113) which does it correctly — filtered `where`, `skipTake`, and a SQL `aggregate` for the total. A `groupBy(["projectId"])` on Expense joined to a SQL sum would fix it. scope note: surfaced through an in-scope file but the query itself is untouched by this ticket (`app/expense-summary/page.tsx` is +2 lines: the ListToolbar swap). Not counted against gate_status. fixed: NO — rewriting an aggregation query is a behaviour change needing data verification. ### [security] SEC-3 — P2 — OUT-OF-SCOPE (pre-existing) files: app/api/uploads/route.ts:73-82, lib/permissions/registry.ts:104-125 problem: Uploaded files are written to `public/uploads/` and served as static assets. `ROUTE_PERMISSIONS` has an entry for `/api/uploads` (the write path) but NONE for `/uploads/...` (the read path). middleware.ts's matcher does cover `/uploads/*`, so a session is required — but `resolveRoutePermission()` returns null, so NO permission is required. Any authenticated user of any role can fetch any invoice/receipt by URL, regardless of their order/expense permissions. Filenames are `Date.now()-`, which is guessable-adjacent, and URLs leak through the rendered AttachmentChip hrefs. Related deployment note: writing to `public/` at runtime is not durable under a containerised/standalone or multi-replica deploy — `public/` is captured at build time. fixed: NO — authorization policy. ### [security] SEC-4 — P2 — OUT-OF-SCOPE (pre-existing) files: lib/actions/orders.ts:12-16 + :48-49, lib/actions/expenses.ts:11-15 problem: `JSON.parse((raw.itemsJson as string) || "[]")` and the `linesJson` equivalent run BEFORE Zod and are unguarded. Malformed JSON throws inside the Server Action before `safeParse` ever sees it, so the caller gets an unhandled-exception 500 instead of the `{ok:false, fieldErrors}` contract every other failure path returns. The inputs are hidden form fields (OrderForm.tsx:110, ExpenseForm.tsx:197, OrderFulfillmentForm.tsx:126) — i.e. fully attacker-controlled on a public Server Action entry point. Not an injection (Prisma is parameterised throughout — see the positives record), but a trivially-reachable unhandled error path. fixed: NO — wrapping in try/catch changes the response shape on a live path. ### [security] SEC-5 — P2 — OUT-OF-SCOPE (pre-existing) file: lib/actions/orders.ts:79-85 (cancelOrder) problem: No status precondition and no existence check — it goes straight to `prisma.order.update({ data:{ status: CANCELLED } })`. A FULFILLED order can be cancelled. The client hides the button for CANCELLED orders only (`readOnly = order.status === "CANCELLED"`, OrderFulfillmentForm.tsx:57), and a Server Action is a public entry point regardless of what the form renders — the same reasoning lib/actions/users.ts:27-32 already spells out for the Super Admin role. The state machine is enforced nowhere. fixed: NO — the legal transition set is a product decision. ### [code-quality] CQ-3 — P3 — in-scope files: components/forms/ExpenseForm.tsx:87-111, components/forms/OrderFulfillmentForm.tsx:73-94 problem: `handleUpload` wraps `fetch` in `try { … } finally { … }` with no `catch`, and is invoked un-awaited from an onChange handler (`if (file) handleUpload(index, file);`). On a network failure the promise rejects with nobody attached: an unhandled rejection, `uploadError` never set, and the spinner state does reset — so the UI silently returns to "Upload" as though nothing happened. The non-2xx path IS handled; only the thrown path is not. ### [code-quality] CQ-4 — P3 — in-scope file: app/expense-summary/[project]/page.tsx:39-61, 77-82 problem: `buildHref` is defined and passed to DataTable as `buildSortHref`, but not one of the four columns sets `sortable: true`. DataTable only calls buildSortHref inside `col.sortable ? … : …` (DataTable.tsx:55-65), so the closure is constructed on every render and never invoked. Either dead code, or a missing `sortable` flag — the neighbouring app/expense-summary/page.tsx DOES mark its projectName column sortable, which suggests the latter. Flagging rather than guessing which. (The W13 dead-code gate did not catch this: the symbol IS referenced, just never reachable.) ### [code-quality] CQ-5 — P3 — in-scope files: components/ui/fields/TextField.tsx:34-36+48-56, components/ui/fields/SelectField.tsx:40-43 (both in feature_files) problem: Both accept `required` and use it ONLY to append " *" to the visible label. Neither sets the `required` attribute nor `aria-required` on the control. So the asterisk is decorative: no native browser validation, and a screen reader gets no required semantics — it hears the literal "*" in the label if anything. Validation is genuinely server-side (Zod -> fieldErrors), which is correct as the boundary, but the accessibility signal is simply missing. Affects every field on every master/user form in this ticket. ### [code-quality] CQ-6 — P3 — in-scope files: the 9 list pages of DRY-1, vs components/ui/FilterBar.tsx:31-44 problem: Inconsistent pagination reset. `buildHref` spreads `...params`, which parseListParams populates with the CURRENT `page` (lib/params.ts:34,41-50), so a sort link built on page 3 emits `page=3` — re-sorting keeps you on page 3 of a completely different ordering. FilterBar, by contrast, explicitly calls `params.set("page", "1")` on both `setParam` and `clearAll`. Same list screen, two different rules for what a control does to the current page. ### [code-quality] CQ-7 — P3 — in-scope files: components/forms/{Supplier,Project,Uom,Item}Form.tsx problem: 4 identical `// eslint-disable-next-line react-hooks/exhaustive-deps` suppressions on the same `[state]` effect. Only SupplierForm.tsx:46 actually explains why ("onSuccess/embedded are stable per mount"); the other three copy the directive without the justification. A suppression duplicated four times is a signal the pattern wants extracting (see DRY-2), and three of the four now read as unexplained. ### [dry-architecture] DRY-4 — P3 — in-scope files: app/admin/{items,users,projects,suppliers,uom}/page.tsx problem: The Active/Inactive status filter descriptor is re-declared inline in each admin list page — identical `{name:"status", label:"Status", type:"select", options:[{value:"active",…},{value:"inactive",…}]}`. A shared `STATUS_FILTER_FIELD` constant would do. (3 verbatim hits for the option pair; the other two differ only in surrounding whitespace.) ### [css-dry] CSSDRY-1 — P3 — in-scope files: components/forms/{Supplier,Project,Uom,Item,User}Form.tsx problem: `className="flex flex-wrap gap-x-6 gap-y-4 max-w-[772px]"` — the de-carded field-grid — repeated verbatim 5×, including the arbitrary `max-w-[772px]`. app/globals.css already hosts exactly this kind of shared form primitive (`.field-input`, `.field-label`, `.field-error`, `.field-control`); a `.field-grid` there would be consistent with the file's own established pattern and put the 772px design constant in one place. ### [css-dry] CSSDRY-2 — P3 — in-scope files: the 5 forms above (6 occurrences of the literal) problem: `w-full sm:w-[350px]` is hardcoded 6× — 5 as the `fieldWidth` ternary's non-embedded branch, once as UserForm.tsx:56's unconditional `const fieldWidth`. Second undocumented design constant duplicated across files (see CSSDRY-1). Both TextField and SelectField document this prop as "Ticket #3248 de-carded form layout", so the value is load-bearing, not incidental. ### [css-dry] CSSDRY-3 — P3 — in-scope files: 11 occurrences across components/forms/* and components/ui/* problem: `text-sm font-semibold text-accent` — the de-carded section heading — repeated 11×. Same remedy as CSSDRY-1: a `.section-title` in the `@layer components` block that already exists. ### [css-dry] CSSDRY-4 — P3 — in-scope files: components/ui/RowActions.tsx:33,37,44 (3×); app/expenses/ExpenseRowActions.tsx:13; app/orders/OrderRowActions.tsx:12 problem: The icon-action hit-target cluster `p-1.5 -m-1.5 text-` is repeated 6× across three components that this ticket's own doc comments describe as deliberately mirroring each other ("mirrors the 5 admin masters' *RowActions.tsx icon-button pattern"). The negative-margin/padding trick that makes the touch target work is the fragile part, and it is copy-pasted rather than named. ### [css-performance] CSSPERF-1 — P3 — in-scope file: tailwind.config.ts:59-60 (in feature_files) problem: Two design tokens are defined and referenced nowhere: accent.selected "#e2cdb0" ("selected tab-pill segment") accent.subtleHover "#f0e8dc" ("secondary/Cancel button hover") Verified: `grep -rn "accent-selected\|accent-subtleHover"` across all .ts/.tsx/.css outside node_modules/.next -> NO REFERENCES (only the config definitions themselves). Confirmed zero runtime cost — `grep -c "e2cdb0\|#f0e8dc\|226,205,176\|240,232,220" .next/static/css/*.css` -> 0, i.e. Tailwind's JIT correctly never emitted them. So this is dead configuration, not CSS bloat — hence P3, not P2. Both were introduced by #3247 for design elements that apparently never got built. (The W13 dead-code gate would not catch these: they are config values, not imported symbols.) -------------------------------------------------------------------------------- 3. CLEAN-AUDIT RECORDS (audits that ran and found nothing to report) -------------------------------------------------------------------------------- ### [animation-performance] — CLEAN Exhaustive grep for `transition|animate-|duration-|@keyframes` across app/globals.css + components/ui + components/forms + app/{admin,expenses,orders} returns exactly three animated things in the whole audited surface: app/globals.css:13 `.btn { … transition-colors … }` components/ui/fields/Listbox.tsx:178 `transition-transform` + `rotate-180` components/ui/fields/DatePicker.tsx:270 `transition-colors` on day cells `transition-colors` animates paint-only properties; `transition-transform` with `rotate-180` is compositor-only. There are no width/height/top/left/margin transitions, no `@keyframes`, no JS-driven animation loops, no scroll/resize-driven style writes, and therefore no layout-thrash path. Nothing to report — this is not an unrun audit. ### [css-performance] — CLEAN apart from CSSPERF-1 Whole-app stylesheet: 26,836 B raw / 5,619 B gzipped, one file, no render-blocking imports beyond it. No `!important` anywhere; no ID selectors; no descendant-selector chains — all styling is JIT-generated utilities plus 9 hand-written classes in a single `@layer components` block, which is the correct Tailwind layering (they compile into the components layer, so utilities still win without specificity hacks). 20 arbitrary-value utilities (`[NNNpx]`) across the audited .tsx files; each generates one rule, so they cost bytes but create no specificity or unused-CSS problem. No unused-CSS finding is possible by construction here — Tailwind emits only what `content:` matches, and CSSPERF-1 confirms that mechanism working correctly. -------------------------------------------------------------------------------- 4. POSITIVES — verified, not assumed (security audit, in-scope) -------------------------------------------------------------------------------- These were actively checked because the gate calls this repo's security audit first-party. Each is a real command, not an assumption: * NO first-party SQL-injection surface. `grep -rn '\$queryRaw|\$executeRaw|queryRawUnsafe' lib app` -> none. Every database access in the repo goes through Prisma's parameterised client. The gate's "including raw SQL escapes if this repo ever drops to raw queries" case does not arise — it has not dropped to raw queries. * NO markup-injection surface. `grep -rn 'dangerouslySetInnerHTML|eval\(|new Function' app components lib` -> none. * NO secret leakage to the client. Every `"use client"` module cross-checked for `process.env` -> none. The only `process.env` reads are AUTH_SECRET in lib/auth.ts:89 and lib/actions/auth.ts:21, both server-only, both fail-closed with an explicit error rather than a silent fallback. No NEXT_PUBLIC_* anywhere. * Every mutating Server Action in scope opens with `requirePermission(...)` before touching Prisma — verified line-by-line in items.ts, uom.ts, users.ts, orders.ts, expenses.ts. * app/api/lookups/route.ts (CHANGED this ticket) is correctly gated: it validates `entity` against the registry keys BEFORE use (so the `QUICK_CREATE_REGISTRY[entity]` index cannot be steered to an arbitrary property), then requires the matching per-entity `*:read` permission and returns 401 otherwise. The permission is looked up from a closed `READ_PERMISSION` record, not derived from user input. * The ticket's headline new capability — UOM quick-create — inherits authorization rather than re-implementing it: QUICK_CREATE_REGISTRY's `uom` entry points at the existing `createUom`, which already calls `requirePermission("uom:create")`. The registry entry adds no new unauthenticated path. ItemForm additionally refuses to offer the trigger while embedded (`canCreate={canCreateUom && !embedded}`), avoiding modal-from-modal, and every `canCreate*` prop defaults to `false` so an unpassed prop degrades to "no quick-create" rather than an open control. * Super Admin role assignment is blocked at the real boundary, not just the UI: listRolesForSelect excludes it from the dropdown AND createUser/updateUser both call `assertNotSuperAdminRole` server-side. (What is missing is the TARGET-side check — that is SEC-2, above.) * Permission resolution is request-memoised (`cache()` in lib/queries/rbac.ts:38), so the `` component appearing many times per list page does NOT produce an N+1 of permission queries. Checked specifically because it looked like one. * The middleware/guard split is genuine defence-in-depth, not duplicated theatre: the Edge layer reads the cookie snapshot (can only be MORE restrictive), the Node layer re-reads from the database. Removing either does not expose a mutation. -------------------------------------------------------------------------------- 5. WHY THIS IS A PASS — AND WHAT THAT DOES NOT MEAN -------------------------------------------------------------------------------- The gate rule is "PASS only if zero unresolved P1 findings remain across all seven audits", and the audits are defined as running "against this ticket's changed files". In-scope P1s (feature_files + this ticket's diff): 0 -> PASS Out-of-scope P1s (files this ticket never touched): 2 -> SEC-1, SEC-2 Both P1s were found by deliberately auditing PAST the ticket boundary, because the gate flags this repo's security audit as first-party. They are real, verified, and NOT fixed. Neither is attributable to #3248: SEC-1 app/api/uploads/route.ts — 0 commits in this ticket's range SEC-2 lib/actions/users.ts — 0 commits in this ticket's range, and already the #3246 gate's blocking P1 Reporting them as P1 while passing the gate is a deliberate call, recorded here so an operator can overrule it: failing #3248 for a defect #3246 introduced and #3247 failed to close would block this ticket without moving the actual defect, and the gate's own Step 3 forbids this agent from fixing either one ("A missing authorization check is never an auto-fix"). SEC-2 in particular has now survived two gates — it wants its own ticket, not a third silent carry-forward. If the orchestrator's policy is that ANY open P1 blocks regardless of attribution, this result should be read as FAIL with 2 P1s. STEP 3 (auto-fix) WAS NOT PERFORMED — no source file was modified. This node was invoked read-only by design ("You have no write access, by design"), so even the clearly-safe candidates (CSSPERF-1's two dead tokens, CQ-4's dead closure) were left as reported findings rather than applied. The only file written by this gate is this evidence file. Every finding above is therefore `fixed: NO`, and that reflects the execution mode, not a judgement that each was unsafe to fix. -------------------------------------------------------------------------------- 6. REDMINE NOTE — NOT POSTED -------------------------------------------------------------------------------- The Step 5 curl needs $REDMINE_URL, $TASK_ID and $REDMINE_API_KEY. Only REDMINE_API_KEY is present in this environment (`env` check: REDMINE_URL and TASK_ID are both unset), so the documented request cannot be constructed. No URL and no ticket id were guessed. The note that would have been posted: "Claude - Opus: Gate 5 — Quality & Security Audit: PASS (0 P1, 11 P2, 12 P3)" -------------------------------------------------------------------------------- 7. VERDICT -------------------------------------------------------------------------------- PASS — 0 unresolved P1 in the audited scope; 11 P2 and 12 P3 reported, none fixed (read-only invocation). Typecheck, lint, 630 unit tests and a production build all pass against the real tree. Two pre-existing P1 security findings outside this ticket's scope are recorded in full and need their own ticket; SEC-2 is the #3246 gate's blocking P1, still open after two subsequent tickets. ================================================================================