Skip to content

fix: delete_entry IDOR — uniform response for single-entry deletes (#193)#234

Merged
cmeans-claude-dev[bot] merged 1 commit into
mainfrom
fix/delete-entry-idor
Apr 10, 2026
Merged

fix: delete_entry IDOR — uniform response for single-entry deletes (#193)#234
cmeans-claude-dev[bot] merged 1 commit into
mainfrom
fix/delete-entry-idor

Conversation

@cmeans-claude-dev
Copy link
Copy Markdown
Contributor

@cmeans-claude-dev cmeans-claude-dev Bot commented Apr 9, 2026

Summary

  • Single-entry delete_entry(entry_id=...) now returns "status": "acknowledged" with no trashed count — prevents entry existence enumeration across tenants
  • Includes "note": "If the entry was not found, no action was taken." — honest without revealing why
  • Bulk deletes (tags, source) unchanged — counts are already owner-scoped

Closes #193

QA

Prerequisites

  • pip install -e ".[dev]"
  • Deploy to test instance on alternate port (AWARENESS_PORT=8421)

Manual tests (via MCP tools)

    • Delete existing entry returns acknowledged
    remember(source="test", tags=["qa"], description="idor test")
    

    Note the ID, then:

    delete_entry(entry_id="<id>")
    

    Expected: {"status": "acknowledged", "entry_id": "...", "recoverable_days": 30, "note": "..."}

    • Delete nonexistent entry returns same shape
    delete_entry(entry_id="00000000-0000-0000-0000-000000000000")
    

    Expected: identical response shape — acknowledged, no trashed field

    • Bulk delete by tags still shows count
    delete_entry(tags=["qa"], confirm=true)
    

    Expected: {"status": "ok", "trashed": <N>, ...} — bulk path unchanged

🤖 Generated with Claude Code

)

Single-entry delete by ID now returns "status": "acknowledged" with no
trashed count, preventing entry existence enumeration across tenants.
Includes a note field: "If the entry was not found, no action was taken."

Bulk deletes (tags, source) retain their counts since they're already
scoped by owner_id.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cmeans-claude-dev cmeans-claude-dev Bot added the bug Something isn't working label Apr 9, 2026
@github-actions github-actions Bot added the Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA label Apr 9, 2026
@cmeans-claude-dev cmeans-claude-dev Bot added the Dev Active Developer is actively working on this PR; QA should not start label Apr 9, 2026
@github-actions github-actions Bot removed the Awaiting CI Dev complete, waiting for CI/Codecov to pass before QA label Apr 9, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 9, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@cmeans-claude-dev cmeans-claude-dev Bot added Ready for QA Dev work complete — QA can begin review and removed Dev Active Developer is actively working on this PR; QA should not start labels Apr 9, 2026
@cmeans cmeans added QA Active QA is actively reviewing; Dev should not push changes and removed Ready for QA Dev work complete — QA can begin review labels Apr 9, 2026
@cmeans
Copy link
Copy Markdown
Owner

cmeans commented Apr 9, 2026

Adding QA Active — starting code review and manual testing of IDOR fix.

Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

QA Review — PR #234: delete_entry IDOR fix

Code Review

Changes reviewed: tools.py, test_server.py, CHANGELOG.md (3 files, +13/−7)

The fix is minimal and correct:

  • Single-entry delete_entry(entry_id=...) now discards the soft_delete_by_id return value and returns a uniform "acknowledged" response regardless of whether the entry existed
  • No trashed count is exposed, eliminating the IDOR vector (attacker could previously enumerate entry existence by observing trashed: 0 vs trashed: 1)
  • Bulk paths (tags, source) are unchanged — their counts are safe since they're already owner-scoped via _owner_id()
  • SQL layer (soft_delete_by_id.sql) properly scopes by owner_id AND deleted IS NULL — no cross-tenant access possible even at the store level
  • Store method still returns bool — the change is purely at the tool layer, which is the right boundary

No findings.

Automated Tests

Suite Result
pytest 763/763 passed (21.8s)
ruff Clean
mypy Clean

Manual MCP Tests (via docker-compose.qa.yaml stack)

# Test Result
1 Delete existing entry returns acknowledged PASS
2 Delete nonexistent entry returns identical shape PASS
3 Bulk delete by tags still shows count PASS

All 3/3 manual tests pass. PR checkboxes updated.

Verdict

Zero findings. Clean fix at the right abstraction layer. Ready for signoff.

@cmeans
Copy link
Copy Markdown
Owner

cmeans commented Apr 10, 2026

All checks pass, zero findings on code review and manual testing. Adding Ready for QA Signoff.

@cmeans cmeans added Ready for QA Signoff QA passed — ready for maintainer final review and merge and removed QA Active QA is actively reviewing; Dev should not push changes labels Apr 10, 2026
Copy link
Copy Markdown
Owner

@cmeans cmeans left a comment

Choose a reason for hiding this comment

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

LGTM

@cmeans cmeans added QA Approved Manual QA testing completed and passed and removed Ready for QA Signoff QA passed — ready for maintainer final review and merge labels Apr 10, 2026
@cmeans-claude-dev cmeans-claude-dev Bot merged commit dac8786 into main Apr 10, 2026
51 checks passed
@cmeans-claude-dev cmeans-claude-dev Bot deleted the fix/delete-entry-idor branch April 10, 2026 00:03
cmeans pushed a commit that referenced this pull request Apr 10, 2026
6 bug-fix PRs since v0.16.1:
- #226 LazyStore thread safety (#164)
- #227 SQL template injection hardening (#165)
- #232 Stateless HTTP mode (#180)
- #233 GET /mcp intercept + request logger (#178)
- #234 delete_entry IDOR fix (#193)
- #236 RLS-safe opt-in cleanup (#179, #183)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@cmeans-claude-dev cmeans-claude-dev Bot mentioned this pull request Apr 10, 2026
3 tasks
cmeans-claude-dev Bot added a commit that referenced this pull request Apr 10, 2026
6 bug-fix PRs since v0.16.1:
- #226 LazyStore thread safety (#164)
- #227 SQL template injection hardening (#165)
- #232 Stateless HTTP mode (#180)
- #233 GET /mcp intercept + request logger (#178)
- #234 delete_entry IDOR fix (#193)
- #236 RLS-safe opt-in cleanup (#179, #183)

Co-authored-by: cmeans-claude-dev[bot] <3223881+cmeans-claude-dev[bot]@users.noreply.github.com>
Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working QA Approved Manual QA testing completed and passed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: delete_entry leaks entry existence via response shape (IDOR)

1 participant