Skip to content

fix(security): Phase 0 β€” close IDOR, source leak, broken update, filter injection - #122

Merged
niyazmft merged 2 commits into
mainfrom
fix/phase0-security
Sep 10, 2026
Merged

niyazmft merged 2 commits into
mainfrom
fix/phase0-security

Conversation

@niyazmft

Copy link
Copy Markdown
Owner

Phase 0 β€” Security & Data Integrity

Closes the four P0 findings from the 4-agent senior-engineer code audit (see reports/code-audit.md).

Fixes

  • P0-1 β€” Subscription IDOR (Critical): updateSubscription/deleteSubscription now fetch the record and verify user_id === req.user.uid before mutating (403 on mismatch). Previously any authenticated user could modify/delete any other user's subscription by ID. Mirrors the existing savingsGoalController ownership pattern.
  • P0-2 β€” Frontend source served publicly (Critical): removed express.static(frontend/src) from app.js. The entire Vue source tree (components, services, stores) was being served verbatim. Only dist/ is now served.
  • P0-3 β€” Transaction update drops payload (High): updateTransaction was calling updateRecord(tableId, id, updatedData) but the service signature is updateRecord(tableId, data) β€” the validated fields were silently dropped and the PATCH body was the raw id string. Now sends { Id: id, ...updatedData }.
  • P0-4 β€” Filter injection + DoS (High): startDate/endDate are now validated with validateAndFormatDate before interpolation into NocoDB filter strings (transactionService + all 3 report endpoints). duration in the cash-flow forecast is clamped to 1–365 to prevent the unbounded per-day simulation CPU DoS.

Tests added (90 passing, +0 failures)

  • subscriptionController.test.js β€” IDOR: cross-user update/delete β†’ 403, service not called
  • transactionController.test.js β€” update sends object payload (not raw id), cross-user β†’ 403
  • transactionService.test.js β€” filter-injection guard, invalid endDate β†’ 400, validated dates in where clause
  • cashFlowController.test.js β€” duration default 30, clamp to 365, clamp negative to 1

Verification

  • pnpm test β€” 90 passing
  • eslint backend/**/*.js β€” clean
  • app.js loads without error

Notes

  • Read-only audit; no unrelated changes.
  • The docs commit (docs: document intentional design choices) is on main separately.

…er injection

- P0-1: Add ownership checks to subscription update/delete (IDOR)
- P0-2: Stop serving frontend/src as static files (source disclosure)
- P0-3: Fix transaction update payload (was sending raw id, dropping fields)
- P0-4: Validate date/duration query params before NocoDB filter interpolation

Adds regression tests for subscription IDOR, transaction update payload,
date filter injection guard, and forecast duration clamp.
@niyazmft
niyazmft merged commit e2e9640 into main Sep 10, 2026
3 of 4 checks passed
@niyazmft
niyazmft deleted the fix/phase0-security branch September 10, 2026 20:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant