Skip to content

fix(security): scope assignment update/delete/sync writes by user_id — defense-in-depth (follow-up #123)#235

Open
Jose-Gael-Cruz-Lopez wants to merge 1 commit into
mainfrom
security/follow-up-224-sibling-write-scoping
Open

fix(security): scope assignment update/delete/sync writes by user_id — defense-in-depth (follow-up #123)#235
Jose-Gael-Cruz-Lopez wants to merge 1 commit into
mainfrom
security/follow-up-224-sibling-write-scoping

Conversation

@Jose-Gael-Cruz-Lopez

@Jose-Gael-Cruz-Lopez Jose-Gael-Cruz-Lopez commented Jun 14, 2026

Copy link
Copy Markdown
Member

Queued follow-up from the cross-PR review (not urgent — not exploitable today).

update_assignment, delete_assignment, and sync_to_google each do a user_id-scoped SELECT and 404 a non-owned id before writing, so an IDOR isn't reachable now. But their write/delete filters were id-only, leaning entirely on that read-guard. This scopes the writes by user_id too (matching export_to_google from #224), so the day someone refactors the guard they can't silently reopen the hole — "not exploitable today" → "can't become exploitable."

Tests assert the update/delete/sync write filters now carry user_id. Calendar suites green; ruff clean.

⚠️ Defense-in-depth, do not merge without your review (per your instruction to queue these).

Summary by CodeRabbit

  • Bug Fixes

    • Calendar assignment update operations now include additional user-level scoping
    • Calendar assignment delete operations now include additional user-level scoping
    • Calendar synchronization processes now include user-level scoping when writing results back to the database
  • Tests

    • Added comprehensive test coverage to validate user-level scoping on all calendar assignment write operations

…ser_id (defense-in-depth, #123)

Follow-up to #123/#224. update_assignment, delete_assignment, and sync_to_google
do a user_id-scoped SELECT and 404 a non-owned id before writing, so they are
not exploitable today — but their write/delete filters were id-only, relying
solely on that guard. Scope the writes by user_id too, so a future change to the
read-guard can't silently reopen the IDOR (matches export_to_google).

Tests assert the update/delete/sync write filters now carry user_id.
@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 14, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
frontend 0cb7589 Commit Preview URL

Branch Preview URL
Jun 14 2026, 03:35 AM

@coderabbitai

coderabbitai Bot commented Jun 14, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 56693632-4897-4bc2-9515-e29a0801e0ba

📥 Commits

Reviewing files that changed from the base of the PR and between 84aee68 and 0cb7589.

📒 Files selected for processing (2)
  • backend/routes/calendar.py
  • backend/tests/test_calendar_sibling_write_scoping.py

📝 Walkthrough

Walkthrough

Three write operations in backend/routes/calendar.pyPATCH /assignments/{id}, DELETE /assignments/{id}, and the google_event_id writeback in POST /sync — now include user_id in their SQL filter conditions alongside the assignment id. A new test module adds three regression tests confirming each write path enforces both filters.

Changes

IDOR Fix: user_id Scoping for Calendar Assignment Writes

Layer / File(s) Summary
user_id filter added to PATCH, DELETE, and sync writeback
backend/routes/calendar.py
update_assignment and delete_assignment each add user_id to their SQL WHERE clause; sync_to_google's google_event_id stamping update adds the same filter.
Regression tests for all three write paths
backend/tests/test_calendar_sibling_write_scoping.py
New TestSiblingWriteScoping class with tests for PATCH, DELETE, and sync writeback, each asserting the mocked table call receives both user_id and id in its filter payload.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related issues

Possibly related PRs

  • SaplingLearn/Sapling#224: Applies the same user_id-alongside-id filter pattern to the /export endpoint in calendar.py, with matching IDOR-focused test coverage — same file, same fix class, adjacent function.

Poem

🐇 Hop, hop, through the calendar rows,
No sneaky sibling can peek at what grows!
With user_id added to each WHERE clause tight,
The bunny keeps every assignment in sight.
IDOR be gone — only owners may write! ✨

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive The description provides a comprehensive explanation of the security rationale, current implementation, and the defense-in-depth enhancement. However, it does not follow the provided template structure with explicit sections for Changes Made, Testing, and Related Issues. Consider restructuring the description to match the provided template with explicit sections: Changes Made (bullet points), Related Issues (issue number), and Testing (checkboxes), for consistency and clarity.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main security-focused change: scoping assignment write operations by user_id for defense-in-depth protection.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security/follow-up-224-sibling-write-scoping

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.

1 participant