Skip to content

fix(#242): harden access control and atomicity from code review#248

Merged
NickMonrad merged 1 commit into
feature/timeline-ux-reworkfrom
nickmonrad/timeline-ux-code-review
May 29, 2026
Merged

fix(#242): harden access control and atomicity from code review#248
NickMonrad merged 1 commit into
feature/timeline-ux-reworkfrom
nickmonrad/timeline-ux-code-review

Conversation

@NickMonrad

Copy link
Copy Markdown
Owner

Summary

Fixes three findings from a code review of the timeline UX rework branch (#242): two cross-tenant access-control holes (IDOR) and a non-atomic count re-sync. Targets feature/timeline-ux-rework so the fixes land before #242 merges to main.

Related issue

Follow-up (deferred) items tracked in #247. Code-reviewed against #242.

Changes

  • resourceTypes DELETE /:id (IDOR, HIGH): scope the delete to the owned project via deleteMany({ where: { id, projectId } }) and return 404 when nothing matches. Previously deleted by global PK, letting any user delete another tenant's resource type.
  • timeline PUT /:featureId (IDOR, MEDIUM): verify the feature belongs to the owned project (feature.findFirst({ id, epic: { projectId } })) and 404 before the timelineEntry.upsert, preventing cross-tenant timeline overwrites.
  • namedResources POST/DELETE (reliability): wrap the capacity-plan exit, create/delete, and resourceType.count re-sync in a single prisma.$transaction, passing the tx client to exitCapacityPlanForManualScheduling, eliminating the count-desync window under concurrent requests.
  • Tests: added cross-tenant 404 tests for the DELETE and PUT routes; updated the named-resource tests + mocks (setup.ts) for the transactional flow.

E2E Tests

No client/UI behaviour changed — these are server-side authorization and atomicity fixes. No Playwright tests added or modified; e2e/TESTS.md unchanged.

Tests added/modified:

  • server/src/test/resourceTypes.test.ts — added cross-tenant DELETE 404 test + project-scoped happy path; updated the capacity-plan-exit DELETE test to assert in-transaction behaviour.
  • server/src/test/timeline.test.ts — added cross-tenant PUT 404 test (asserts upsert never called); updated happy-path mock.

Testing

  • npx tsc --noEmit passes in /server
  • Relevant server suites pass (resourceTypes, timeline, namedResourceAssignments, auth — 44 tests)
  • npx tsc --noEmit in /client — n/a (no client changes)
  • npm run test:e2e — n/a (no UI changes)
Test Files  4 passed (4)
     Tests  44 passed (44)

Notes

Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com

- resourceTypes DELETE: scope deleteMany to projectId and 404 when no row
  matches, preventing cross-tenant resource type deletion (IDOR)
- timeline PUT /:featureId: verify the feature belongs to the owned project
  before upserting, preventing cross-tenant timeline overwrites (IDOR)
- namedResources POST/DELETE: wrap capacity-plan exit, create/delete and
  count re-sync in a single transaction to avoid count desync under
  concurrent requests

Adds cross-tenant 404 tests for the resourceTypes DELETE and timeline PUT
routes and updates mocks for the transactional named-resource flow.

Note: the IPv6 rate-limit hardening (ipKeyGenerator) is deferred — the
installed express-rate-limit@7.5.1 does not export that helper.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@NickMonrad NickMonrad added bug Something isn't working reliability Reliability and error handling security Security vulnerability or concern labels May 29, 2026
@NickMonrad NickMonrad merged commit 203ee63 into feature/timeline-ux-rework May 29, 2026
@NickMonrad NickMonrad deleted the nickmonrad/timeline-ux-code-review branch May 29, 2026 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working reliability Reliability and error handling security Security vulnerability or concern

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant