Skip to content

fix: add app_id scope check to V1 API handlers (#244)#257

Open
boot-coco wants to merge 1 commit intodevelopfrom
fix/244-v1-api-access-control
Open

fix: add app_id scope check to V1 API handlers (#244)#257
boot-coco wants to merge 1 commit intodevelopfrom
fix/244-v1-api-access-control

Conversation

@boot-coco
Copy link
Copy Markdown
Contributor

Summary

  • Fixes broken access control in V1 API endpoints (issue V1 API endpoints missing app_id scope check (broken access control) #244): all 8 V1 item handlers now verify that the requested item belongs to the authenticated user's app_id before allowing access
  • Uses the same req.v2Auth?.app_id && item.app_id !== req.v2Auth.app_id check already present in V2 endpoints, returning 403 on mismatch
  • For handlers that previously skipped the item fetch (assign, resolve, verify, reopen, close), an explicit getItem() call was added before the scope check

Affected handlers

handleGetItem, handleAddMessage, handleAssignItem, handleResolveItem, handleVerifyItem, handleReopenItem, handleCloseItem, handleRespondToItem

Test plan

  • All 514 existing unit tests pass
  • Verify V1 endpoints return 403 when accessing an item from a different app
  • Verify V1 endpoints still work normally when accessing items within the same app
  • Verify unauthenticated requests (no v2Auth) are unaffected (guard uses optional chaining)

🤖 Generated with Claude Code

V1 item handlers (handleGetItem, handleAddMessage, handleAssignItem,
handleResolveItem, handleVerifyItem, handleReopenItem, handleCloseItem,
handleRespondToItem) did not verify that the requested item belongs to
the authenticated user's app_id. This allowed any authenticated user to
read, modify, or close items belonging to other apps.

Add the same app_id scoping check used by V2 endpoints:
  if (req.v2Auth?.app_id && item.app_id !== req.v2Auth.app_id)
    → 403 Forbidden

For handlers that previously skipped the item fetch (assign, resolve,
verify, reopen, close), an explicit getItem() call is added before the
scope check.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@jessie-coco jessie-coco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codex Review R1: CLEAN — 0 P1 + 0 P2 + 1 P3

Scope check pattern matches V2 endpoints exactly (req.v2Auth?.app_id && item.app_id !== req.v2Auth.app_id). All 8 V1 item handlers are covered. Consistent and correct.

Checked dimensions:

  • Correctness ✅ — Logic mirrors V2 pattern. Guard placement is correct (after 404 check, before mutation).
  • Security ✅ — Fixes broken access control for V1 endpoints. 403 on app_id mismatch.
  • Edge cases ✅ — JWT with null app_id (no default app) skips check gracefully via optional chaining. Same behavior as V2.
  • Integration ✅ — No callers broken. V1 routes already have v2Auth middleware, so req.v2Auth is always set when handlers execute.

P3 (non-blocking): In handleAssignItem, handleResolveItem, handleVerifyItem, handleReopenItem, handleCloseItem — the new getItem() + 404 check makes the subsequent if (!result.changes) return 404 unreachable (item was just confirmed to exist). Dead code, harmless. Could be cleaned up later if desired.

Not in scope (noted for awareness): handleGetItems, handleCreateItem, and handleGetItemsFull still use resolveAppId(req) from URL params rather than auth-resolved app_id. V2 equivalents use req.v2Auth.app_id. This is a separate issue from #244.

R1 CLEAN. Approved.

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.

2 participants