Skip to content

[fix] Fixed admin subnet export multitenancy security issue#220

Merged
nemesifier merged 1 commit into
masterfrom
security-export-subnet
Jun 15, 2026
Merged

[fix] Fixed admin subnet export multitenancy security issue#220
nemesifier merged 1 commit into
masterfrom
security-export-subnet

Conversation

@nemesifier

Copy link
Copy Markdown
Member

Checklist

  • I have read the OpenWISP Contributing Guidelines.
  • I have manually tested the changes proposed in this pull request.
  • I have written new test cases for new code and/or updated existing tests for changes to existing code.
  • N/A I have updated the documentation.

Reference to Existing Issue

Addresses GHSA-x287-5c68-36wp.

Description of Changes

Before this patch, if a specific subnet ID was known, any staff user with permissions to operate on subnet objects was allowed to export the subnet contents, regardless of whether they managed the organization of the subnet or not. This patch fixes it.

Before this patch, if a specific subnet ID was known,
any staff user with permissions to operate on subnet
objects was allowed to export the subnet contents,
regardless of whether they managed the organization
of the subnet or not. This patch fixes it.
@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8103de3f-5986-4787-9c7b-5dbb454021aa

📥 Commits

Reviewing files that changed from the base of the PR and between 922b9e1 and a4b2724.

📒 Files selected for processing (2)
  • openwisp_ipam/admin.py
  • openwisp_ipam/tests/test_multitenant.py
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
  • GitHub Check: Python==3.12 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.1.0
  • GitHub Check: Python==3.13 | django~=5.2.0
  • GitHub Check: Python==3.11 | django~=5.2.0
  • GitHub Check: Python==3.10 | django~=4.2.0
  • GitHub Check: Python==3.10 | django~=5.2.0
  • GitHub Check: Python==3.12 | django~=5.1.0
  • GitHub Check: Python==3.13 | django~=5.1.0
  • GitHub Check: Python==3.10 | django~=5.1.0
  • GitHub Check: Python==3.11 | django~=4.2.0
  • GitHub Check: Python==3.12 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{py,html}

📄 CodeRabbit inference engine (Custom checks)

For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework

Files:

  • openwisp_ipam/tests/test_multitenant.py
  • openwisp_ipam/admin.py
🪛 ast-grep (0.43.0)
openwisp_ipam/admin.py

[error] 149-149: Lack of sanitization of user data
Context: HttpResponse(content_type="text/csv")
Note: [CWE-20].

(http-response-from-request)

🔇 Additional comments (2)
openwisp_ipam/admin.py (1)

13-13: LGTM!

Also applies to: 147-154

openwisp_ipam/tests/test_multitenant.py (1)

99-110: LGTM!


📝 Walkthrough

Walkthrough

The subnet CSV export admin view in admin.py is updated to enforce multitenancy: it now imports get_object_or_404 and resolves the requested subnet using self.get_queryset(request) scoped to the current user, raising a 404 if the subnet is not accessible. Previously, the export used the raw subnet_id directly. A new test test_export_subnet_multitenancy in test_multitenant.py logs in as an org1-scoped administrator and asserts that exporting an org2 subnet returns HTTP 404.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title follows the required format [type] with 'fix' prefix and clearly describes the security issue addressed in the changeset regarding multitenancy authorization.
Description check ✅ Passed The description includes the required sections (checklist, issue reference, and detailed changes), though documentation update is marked as N/A rather than completed.
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.
Bug Fixes ✅ Passed The PR properly fixes the multitenancy authorization bug by using get_object_or_404(self.get_queryset(request), pk=subnet_id), addressing the root cause. The regression test test_export_subnet_mult...

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

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch security-export-subnet

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.

@github-project-automation github-project-automation Bot moved this from In progress to Reviewer approved in OpenWISP Priorities for next releases Jun 15, 2026
@nemesifier
nemesifier merged commit a4b2724 into master Jun 15, 2026
21 checks passed
@nemesifier
nemesifier deleted the security-export-subnet branch June 15, 2026 19:28
@github-project-automation github-project-automation Bot moved this from Reviewer approved to Done in OpenWISP Priorities for next releases Jun 15, 2026
@nemesifier

Copy link
Copy Markdown
Member Author

/backport 1.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Development

Successfully merging this pull request may close these issues.

1 participant