Skip to content

fix: reject access to documents not owned by current user#135

Closed
greatjourney589 wants to merge 3 commits into
MkDev11:mainfrom
greatjourney589:fix/issue-133-cross-tenant-document-access
Closed

fix: reject access to documents not owned by current user#135
greatjourney589 wants to merge 3 commits into
MkDev11:mainfrom
greatjourney589:fix/issue-133-cross-tenant-document-access

Conversation

@greatjourney589
Copy link
Copy Markdown

@greatjourney589 greatjourney589 commented May 22, 2026

Summary

Fixes a cross-tenant document access vulnerability in the SDK document
endpoint. The handler looked up a document by document_id without checking
that the document belonged to the current user, so any authenticated user
could read another tenant's document by supplying its ID.

This adds an ownership check via DocumentService.accessible(document_id, current_user.id) before the document is fetched, returning an error result
when the current user does not own the document. current_user is imported
from api.apps to support the check.

Related Issues

Fixes #133

Type of Change

  • Bug fix
  • New feature
  • Enhancement
  • Refactor
  • Documentation
  • Other (describe):

Testing

  • pnpm build passes
  • Manual browser smoke test (for UI changes)
  • N/A — fix delivered as a patch against the upstream API; verified the
    ownership check rejects access to documents not owned by the requesting
    user and still allows owned documents.

Checklist

  • Self-reviewed the diff
  • Follows existing code patterns and naming
  • No unrelated changes included
  • Documentation updated if behavior changed

Summary by CodeRabbit

  • Bug Fixes
    • Fixed a security issue allowing cross-account document access. Document endpoints now validate requester ownership and return an error when access is not permitted, preventing unauthorized document retrieval.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 22, 2026

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: 821cd8c2-a226-475f-998c-668f5ab4536e

📥 Commits

Reviewing files that changed from the base of the PR and between 2567de2 and 100e993.

📒 Files selected for processing (1)
  • fixes/issue-133-cross-tenant-document-access.patch

📝 Walkthrough

Walkthrough

Imports current_user and adds an authorization guard: DocumentService.accessible(document_id, current_user.id) is checked after validating document_id; unauthorized requests return "Document not found!" and the document is not loaded or streamed.

Changes

Cross-tenant document access fix

Layer / File(s) Summary
Import and authorization guard for document download
api/apps/sdk/doc.py
current_user is imported and DocumentService.accessible(document_id, current_user.id) is checked after document_id validation; if the check fails the handler returns "Document not found!", otherwise it proceeds to DocumentService.query(...) and existing not-found handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 I hopped through code to mend the seam,
Where files once slipped like a secret stream.
I checked each key with a careful eye,
Closed the gap where strangers pry.
Now tenants' pages safely dream.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: adding an ownership check to reject unauthorized document access.
Linked Issues check ✅ Passed The PR successfully implements the core fix for issue #133: it adds DocumentService.accessible() check before document access, imports current_user, and returns a generic error message to prevent information leakage.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the cross-tenant document access vulnerability documented in issue #133; no unrelated modifications are present.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

greatjourney589 and others added 2 commits May 22, 2026 10:07
Per issue MkDev11#133's Expected behavior, the rejection should mirror the
get(doc_id) endpoint and return "Document not found!" so an unauthorized
caller cannot distinguish an existing-but-not-owned document from a
non-existent one (cross-tenant ID enumeration). DocumentService.accessible
already returns False uniformly for both cases, so this is the correct,
non-leaking message.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@MkDev11
Copy link
Copy Markdown
Owner

MkDev11 commented May 24, 2026

I am going to close this PR as not applicable

@MkDev11 MkDev11 closed this May 24, 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.

[Bug]: Cross-tenant document download via GET /api/v1/documents/<document_id> (IDOR / Broken Object-Level Authorization)

3 participants