Skip to content

Missing ownership checks in /staged-files/clear-all and /commit-direct allow cross-user access to staged files #82

Description

@Ananya-vastare

🐛 Bug Description

routes/staged_files.py has two broken-access-control (IDOR) issues where an endpoint trusts client-supplied data instead of the identity returned by require_api_key, allowing one authenticated user to act on another user's staged files:

  1. POST /staged-files/clear-all determines whose files to delete from the X-Telegram-Id request header instead of the authenticated telegram_id from require_api_key. Any authenticated user can clear another user's staged files by setting this header to a victim's telegram ID.
  2. POST /commit-direct fetches staged files via get_staged_files_by_ids(payload.file_ids) with no check that the returned files belong to the authenticated user. A malicious file_ids payload lets a caller commit another user's staged file contents into their own repo (using their own GitHub token/repo/branch), and marks the victim's files as committed via mark_files_committed.

🔁 Steps to Reproduce

  1. As User A, obtain a valid API key/session and note User B's telegram_id.
  2. Clear-all case: Send POST /staged-files/clear-all with header X-Telegram-Id: <User B's telegram_id> and a valid auth token for User A. Observe User B's staged files are cleared.
  3. Commit-direct case: Discover or guess a valid staged file id belonging to User B (e.g. via GET /staged-files/{telegram_id} if that ID space is enumerable, or leaked in logs/UI). Send POST /commit-direct as User A with file_ids containing User B's file ID and telegram_id set to User A's own ID.
  4. Observe the commit succeeds against User A's repo using User B's staged file content, and User B's staged file is marked committed.

✅ Expected Behavior

  • clear-all should only ever clear staged files belonging to the authenticated user (from require_api_key), regardless of any header value supplied.
  • commit-direct should only commit staged files that belong to the authenticated user; any requested file_ids not owned by that user should be excluded (and the request should 404 if none remain), preventing cross-user data leakage.

❌ Actual Behavior

  • clear-all deletes staged files for whatever telegram_id is passed in the X-Telegram-Id header, not the authenticated user.
  • commit-direct commits and marks-as-committed any staged file matching the supplied file_ids, with no ownership/tenant check, exposing another user's staged diff content and disrupting their pending commit state.

🌍 Environment

Field Value
OS N/A (backend/API issue)
VS Code version N/A
GitPhone extension version N/A
Python version (if backend issue) 3.11 (FastAPI backend, routes/staged_files.py)

📋 Additional Context

  • Root cause pattern: sync_state_route, list_staged_files, and commit_direct's outer check all correctly compare the authenticated telegram_id (from require_api_key) against the requested one — but clear_all_route skips this entirely, and commit_direct's inner get_staged_files_by_ids call has no equivalent scoping for the individual file IDs.
  • Suggested fix for clear-all: drop the X-Telegram-Id header and Request param; use telegram_id: str = Depends(require_api_key) directly, as the other routes do.
  • Suggested fix for commit-direct: filter staged_files to only those where f["telegram_id"] == telegram_id before proceeding (ideally by scoping the query itself in get_staged_files_by_ids/supabase_service.py rather than filtering client-side after fetch).
  • Severity: clear-all is medium (data-loss/DoS against another user); commit-direct is high (cross-tenant data exfiltration + victim state corruption).

Metadata

Metadata

Labels

ECSoC26Required on all scored PRsbugSomething isn't workinggood first issueGood for newcomersgood-issueBonus +10 XP (admin only)

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions