fix(security): CSV formula injection in evidence export + unauthenticated accept-policies action (GH-097, GH-266) - #3577
dennisofficial wants to merge 4 commits into
Conversation
|
|
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
@cubic-dev-ai review it |
@dennisofficial I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Heads up: you’re close to your flex budget. Increase your flex budget so reviews don’t pause.
Fix all with cubic | Re-trigger cubic
Summary
Fixes two P3 security findings from the internal Glass House sweep.
GH-97 — CSV formula injection in evidence-form export
toCsvRowinapps/api/src/evidence-forms/evidence-forms.service.tsquoted CSV values but did nothing about spreadsheet formula prefixes. Employee/contractor-submitted free text (complaintDetails,individualsInvolved,evidence, ...) is a barez.string().min(1), and Excel/Sheets strip the surrounding CSV quotes before evaluating a leading=,+,-,@, tab, or CR — so quoting alone was not a mitigation. Any reviewer withevidence:readexporting the CSV could execute attacker-controlled formulas.Fix:
neutralizeFormula()prefixes any value starting with=,+,-,@, tab, or CR with a single quote before quoting, so spreadsheet apps render it as text.GH-266 — Unauthenticated
use serveractionaccept-policiesapps/portal/src/actions/accept-policies.tsexportedacceptPolicy/acceptAllPolicies, which push a caller-suppliedmemberIdonto any policy'ssignedBywith no session, membership, or org checks. Repo-wide grep confirmed it is unreferenced dead code (no imports, only one comment reference), so Next.js exposes no action id today — but a single future import would flip it into a P0-shaped compliance-attestation forgery.Fix: deleted the dead action. The live, authenticated equivalent already exists at
apps/portal/src/app/api/portal/accept-policies/route.ts(session + member-belongs-to-user checks). Also updated the one stale comment inapps/app/src/trigger/tasks/task/policy-acknowledgment-digest-helpers.tsthat pointed at the deleted file.Verification
apps/api/src/evidence-forms/evidence-forms.service.spec.tscovering=,+,-,@neutralization, benign-value passthrough, embedded-quote escaping, and the reviewer-role gate.bunx jest src/evidence-forms→ 3 suites, 30 tests passed.apps/api(22 errors) andapps/app(15 errors) have pre-existing failures onorigin/mainin unrelated test files (trigger specs, cloud-tests/documents/integrations test mocks) — verified identical with these changes stashed.apps/portalhas no typecheck script; the deleted file was verified unreferenced repo-wide.Summary by cubic
Fixes two security findings from an internal sweep: CSV formula injection in evidence-form exports and an unauthenticated policy-acceptance server action.
=,+,-,@, tab, CR, or line feed (plus the full-width variants\uFF1D,\uFF0B,\uFF0D,\uFF20) by prefixing them with a single quote so spreadsheet apps render them as text (Add policy comments functionality #97).accept-policiesserver action that pushed a caller-suppliedmemberIdonto any policy'ssignedBywithout auth checks; the authenticated API route already covers this flow (chore: update dependencies and improve localization handling #266).Written for commit 88ce8ab. Summary will update on new commits.