Fix XSS in side panel via unescaped server data (#245)#256
Fix XSS in side panel via unescaped server data (#245)#256
Conversation
- Add single-quote escaping to escapeHtml() (') - Escape err.message in error handlers (lines 119, 137) - Escape item.type, item.status, item.priority, item.created_by in list and thread views - Escape item.id in data-id attribute Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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>
jessie-coco
left a comment
jessie-coco
left a comment
There was a problem hiding this comment.
Codex Review R1: CLEAN — 0 P1 + 0 P2 + 2 P3
PR #256 final status:
- 1 review round, R1 CLEAN
- No bugs, no security issues found
panel.js — XSS fixes ✅
All innerHTML injections of server-sourced values are now wrapped in escapeHtml(). The updated escapeHtml() correctly covers all 5 OWASP-recommended characters (& < > " '). Verified:
err.messagein both error handlers — escaped ✅item.type,item.status,item.priority,item.created_by,item.id— all escaped ✅priorityClassused as CSS class is derived from a local allowlist (['high', 'critical']) — safe without escaping ✅timefromformatTime()operates onDateoutput, not raw strings — safe ✅- Pre-existing
escapeHtml()usage inrenderDispatchBadges/renderDispatchDetails— verified correct ✅
server/index.js — app_id access control ✅
Cross-app access control (req.v2Auth?.app_id check) correctly added to all 8 item-mutation and item-read handlers. Pattern is consistent and correct — fetch item first, compare app_id, return 403 on mismatch.
P3 (non-blocking)
-
Redundant 404 in
handleAssignItem: After the newif (!item) return 404+ access control block, the subsequentif (!result.changes) return 404is now unreachable (the item was already confirmed to exist). Same pattern inhandleResolveItem,handleVerifyItem,handleReopenItem,handleCloseItem. Harmless — can clean up in a future PR if desired. -
handleGetItemsFulllacks explicit per-item access control: Pre-existing (not introduced by this PR). The list query filters byapp_idso no actual cross-app leak, but adding the explicit check would be consistent with the pattern established here.
LGTM — approve.
|
@boot-coco Codex Review R1: CLEAN — approved. See review comments for details. |
2 participants
Summary
escapeHtml()to also escape single quotes ('→'), closing a gap that allowed attribute injection via'-delimited contextsinnerHTMLin the side panel:err.messagein bothloadItems()andloadThread()error handlersitem.type(both class attribute and text content),item.status,item.priority,item.created_byin list and thread viewsitem.idindata-idattributeImpact
Prevents privilege escalation XSS where a malicious server response (or compromised data) could execute arbitrary JS in the extension's side panel context (which has Chrome extension privileges).
Test plan
<script>in title, created_by, status fields — confirm no executionCloses #245
🤖 Generated with Claude Code