[INFRA-778] fix(security): scope ExportIssuesEndpoint.get to the requesting user's own exports - #9707
[INFRA-778] fix(security): scope ExportIssuesEndpoint.get to the requesting user's own exports#9707mguptahub wants to merge 2 commits into
Conversation
…esting user's own exports ExportIssuesEndpoint.get filtered ExporterHistory by workspace__slug only, with no initiated_by filter, so any workspace ADMIN/MEMBER could list every other member's export history. ExporterHistorySerializer includes url (a presigned S3 link, 7-day expiry, no auth required to use) and token. Since an export defaults to the initiator's own projects (including fully private ones) when no project list is supplied, any workspace member could read another member's private project data by listing exports and using the disclosed url — no crafted request needed, and revoking the user's access doesn't revoke the download. Add initiated_by=request.user to the queryset filter, per the advisory's own suggested fix. 2 new tests, fail-before verified. Co-authored-by: Plane AI <noreply@plane.so>
|
Linked to Plane Work Item(s) This comment was auto-generated by Plane |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe export history endpoint now returns only issue exports initiated by the requesting user. Contract tests verify cross-member visibility and presigned URL access. ChangesExport history access scoping
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to The change limits export-history results to the requesting user while preserving access to that user's own exports; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description provides a detailed vulnerability summary, fix, scope boundaries, regression tests, and validation results. It does not use all template headings, but it contains the required substantive information.
✨ Finishing Touches 💡 1📝 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 |
|
/code-review: clean, no findings. Confirmed this is the only route serving ExporterHistory data (no parallel endpoint reintroduces the leak) and the test correctly exercises both negative and positive cases. |
There was a problem hiding this comment.
Pull request overview
Scopes ExportIssuesEndpoint.get export-history listing to the requesting user to prevent cross-member disclosure of export metadata (including presigned download URLs/tokens), and adds contract-level regression coverage to ensure the vulnerability stays closed.
Changes:
- Restrict
ExporterHistoryGET queryset byinitiated_by=request.userinExportIssuesEndpoint.get. - Add contract regression tests verifying a member cannot see another member’s export history, while still being able to see their own.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apps/api/plane/app/views/exporter/base.py | Adds initiated_by=request.user to the export-history queryset to prevent cross-member exposure. |
| apps/api/plane/tests/contract/app/test_export_history_scope.py | New contract regression tests covering cross-member visibility and positive control for own exports. |
Suppressed comments (2)
apps/api/plane/tests/contract/app/test_export_history_scope.py:77
- Use
status.HTTP_200_OKinstead of the literal200to match the established pattern in other contract tests and make the assertion self-explanatory.
assert response.status_code == 200, response.data
apps/api/plane/tests/contract/app/test_export_history_scope.py:85
- Use
status.HTTP_200_OKinstead of the literal200to match the established pattern in other contract tests and make the assertion self-explanatory.
assert response.status_code == 200, response.data
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Address Copilot finding on PR #9707: this repo's contract tests consistently assert HTTP codes via rest_framework.status constants. Co-authored-by: Plane AI <noreply@plane.so>
|
Re the /code-review finding on reusing session_client/create_bot_user instead of local _make_user/_client_for helpers: checked before acting on it. Neither fixture actually fits this test's need — session_client is force_authenticate-tied to the single create_user fixture (can't produce a second, distinct authenticated user), and create_bot_user creates a bot-flagged user, not a regular member. The local _make_user/_client_for pattern here matches the existing precedent already in test_project_member_is_active_authz.py (pre-existing file, not from this PR), which has the identical local _make_user helper for the same reason. Leaving as-is rather than introducing a new shared fixture unrequested — that would be a conftest.py change affecting other test files, better done as its own deliberate cleanup than folded into this security fix. Noting it as a legitimate future refactor candidate, not fixing reactively here. |
Summary
ExportIssuesEndpoint.getfilteredExporterHistorybyworkspace__slugonly, with noinitiated_byfilter — any workspace ADMIN/MEMBER could list every other member's export history.ExporterHistorySerializerincludesurl(a presigned S3 download link, 7-day expiry, no authentication required to use) andtoken.projectlist is supplied in the create request, any workspace member could read another member's private project data simply by listing exports and using the disclosedurl— no crafted request needed, and revoking the original user's access doesn't revoke the download.initiated_by=request.userto the queryset filter, per the advisory's own suggested fix.url/tokenfrom the serializer in favor of an authenticated per-request redirect endpoint, and shortening the 7-day presigned-URL expiry.Test plan
apps/api/plane/tests/contract/app/test_export_history_scope.py: a workspace member cannot see another member's export history via GET; a member still sees and can retrieve their own (including theurlfield)git stash— the cross-member-visibility test fails against pre-fix coderuff check/ruff format --checkcleanSummary by CodeRabbit
Bug Fixes
Tests