Uh oh!
There was an error while loading. Please reload this page.
fix(security): scope calendar export by user_id — close cross-user IDOR (#123) - #224
Merged
Merged
Conversation
…DOR (#123) export_to_google selected each assignment by id alone, so an authenticated caller could pass another user's assignment UUIDs to read and decrypt their private notes, push them into the caller's Google Calendar, and stamp google_event_id onto the victim's row (data corruption). require_self only validated the body's user_id, not the ids. Scope both the select and the write-back update by user_id (matching the sync/ update/delete siblings): a non-owned id now returns no row and is skipped. Test exercises the exact cross-user path — attacker authenticated as self, targeting the victim's assignment id — and asserts nothing is decrypted, exported, or stamped. Fails on pre-fix code (exported_count==1).
📝 WalkthroughWalkthroughThe PR fixes a critical IDOR vulnerability in the calendar export endpoint by adding ChangesCalendar Export Cross-User IDOR Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend | 488a201 | Commit Preview URL Branch Preview URL | Jun 13 2026, 04:56 AM |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/test_calendar_export_idor.py (1)
96-99: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAssert write-back remains user-scoped in the owner-path test.
This test validates export success but not the
update(..., filters={id,user_id})contract, so the write-back scoping could regress unnoticed.Suggested test hardening
assert r.status_code == 200 assert r.json()["exported_count"] == 1 service.events.return_value.insert.assert_called_once() + t.return_value.update.assert_called_once()+ _, update_kwargs = t.return_value.update.call_args+ assert update_kwargs["filters"] == {+ "id": "eq.my_assignment",+ "user_id": f"eq.{VICTIM}",+ }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/test_calendar_export_idor.py` around lines 96 - 99, The test at backend/tests/test_calendar_export_idor.py only asserts insert was called but doesn't verify the write-back used user-scoped filters; add an assertion that service.events.return_value.update was called once with filters including both "id" and "user_id" (or that the call args/kwargs contain filters={"id": <expected>, "user_id": <expected>}), alongside the existing insert assertion so the test enforces the update(..., filters={id,user_id}) contract and prevents regression of owner-scoping.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/tests/test_calendar_export_idor.py`:
- Around line 96-99: The test at backend/tests/test_calendar_export_idor.py only
asserts insert was called but doesn't verify the write-back used user-scoped
filters; add an assertion that service.events.return_value.update was called
once with filters including both "id" and "user_id" (or that the call
args/kwargs contain filters={"id": <expected>, "user_id": <expected>}),
alongside the existing insert assertion so the test enforces the update(...,
filters={id,user_id}) contract and prevents regression of owner-scoping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ada72b7-83e3-4348-9906-49f997d6cfed
📒 Files selected for processing (2)
backend/routes/calendar.pybackend/tests/test_calendar_export_idor.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes#123. P0 / CRITICAL.
Vulnerability
export_to_google(routes/calendar.py) selected each requested assignment byidwith nouser_idscope — read+decrypt another user's privatenotes, push to the attacker's calendar, stampgoogle_event_idon the victim's row.Fix
Per-site
user_idscoping on the export select and write-back.Note on sibling endpoints
update_assignment/delete_assignment/sync_to_googleare not vulnerable — each does auser_id-scoped SELECT and returns 404 before its (currently id-only) write, so a non-owned id never reaches the mutation. Their write filters are scoped-by-read-guard, not scoped-by-filter; tightening those write filters as defense-in-depth is a tracked follow-up, not part of this PR.Negative test (cross-user, fails pre-fix)
tests/test_calendar_export_idor.py: attacker authenticated as self targets victim's assignment id →exported_count==0, no decrypt/insert/update; control test confirms owner can still export. Fails pre-fix (exported_count==1).