Skip to content

Review and security fix: entity confirmation lateral deletion (A + C) - #15

Open
davidpomerenke wants to merge 2 commits into
mainfrom
claude/stoic-feynman-lr7i8a
Open

Review and security fix: entity confirmation lateral deletion (A + C)#15
davidpomerenke wants to merge 2 commits into
mainfrom
claude/stoic-feynman-lr7i8a

Conversation

@davidpomerenke

@davidpomerenke davidpomerenke commented Jun 18, 2026

Copy link
Copy Markdown
Contributor

Summary

  • REVIEW.md — new comprehensive code review documenting security issues, GDPR concerns, and architecture notes
  • SEC-1 (HIGH): lateral deletion fix — the DELETE /api/entity-confirmations endpoint previously allowed any user whose entity field matched the target entity to delete all confirmations for that entity, including those created by colleagues. Fixed to only allow deletion by the original creator or an admin.

Findings at a glance

# Severity Issue
SEC-1 High Entity confirmation DELETE allows any entity member to delete colleagues' confirmations — fixed in this PR
SEC-2 Medium Admin reads all survey responses without per-entity gating; responder_email exposed
SEC-3 Medium AI chat logs stored indefinitely — no retention policy (GDPR risk)
SEC-4 Low No rate limiting on AI chat tool execution
SEC-5 Low Regex-based SQL safety in AI chat (defence-in-depth gap)
✓ Correct Magic-link auth: 15-min expiry, single-use, timing-safe, domain+whitelist
✓ Correct All SQL parameterised; DB read-only role for AI chat

Security detail

Before (vulnerable)

if (userEntity && userEntity === entity) {
  // Any entity member could wipe all confirmations for their entity
  result = await query(`DELETE ... WHERE proper_title = $1 AND entity = $2`, ...)
} else {
  result = await query(`DELETE ... WHERE ... AND confirmed_by_user_id = $3`, ...)
}

After (fixed)

if (user.role === "admin") {
  result = await query(`DELETE ... WHERE proper_title = $1 AND entity = $2`, ...)
} else {
  // Regular users can only delete their own confirmations
  result = await query(`DELETE ... WHERE ... AND confirmed_by_user_id = $3`, ...)
}

Test plan

  • As a regular user at Entity X, confirm a report → verify you can delete your own confirmation
  • As a different regular user at Entity X, attempt to delete the first user's confirmation → expect it to be a no-op (returns { deleted: false })
  • As an admin, delete any confirmation → succeeds
  • Confirm the deleted flag in the response correctly reflects whether a row was removed
  • Verify REVIEW.md renders correctly on GitHub

🤖 Generated with Claude Code

Documents security issues: entity confirmation lateral deletion,
admin reads all survey responses without entity gating, AI chat log
indefinite retention (GDPR), no AI rate limiting. Also notes missing
CLAUDE.md and zero tests.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HNCpQeWsxkkQX4iN1q8yKQ
@vercel

vercel Bot commented Jun 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
un80-sg-reports-survey Ready Ready Preview, Comment Jun 18, 2026 6:21am

Request Review

Previously any authenticated user at Entity X could delete all
confirmations for Entity X, including ones created by colleagues.

Fix: only the creator of a confirmation (or an admin) may delete it.
Entity membership is no longer a sufficient permission — this matches
the principle of least privilege and prevents intra-entity lateral
tampering.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HNCpQeWsxkkQX4iN1q8yKQ
@davidpomerenke davidpomerenke changed the title A: Add REVIEW.md Review and security fix: entity confirmation lateral deletion (A + C) Jun 18, 2026
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