[fix] Restrict device registration to an allowlist of fields#1440
Open
MichaelUray wants to merge 1 commit into
Open
[fix] Restrict device registration to an allowlist of fields#1440MichaelUray wants to merge 1 commit into
MichaelUray wants to merge 1 commit into
Conversation
DeviceRegisterView.init_object() built the new Device from every POST field that happened to be a model field, skipping only "organization" and "organization_id". A registrant with a valid organization shared secret could therefore mass-assign internal fields such as the primary key "id": choosing an arbitrary UUID and observing the response turns registration into a cross-organization device existence oracle. Replace the blocklist with an explicit allowlist of the fields a device legitimately provides at registration (name, mac_address, hardware_id, os, model, system, management_ip), plus key only when consistent registration is enabled. Every other field (id, last_ip, _is_deactivated, group, created, modified, ...) is now set by the server or left at its default. Signed-off-by: Michael Uray <michaeluray@users.noreply.github.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details🧰 Additional context used📓 Path-based instructions (2)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{py,html}📄 CodeRabbit inference engine (Custom checks)
Files:
🧠 Learnings (8)📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-02-17T19:13:10.088ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
📚 Learning: 2026-06-07T12:07:08.468ZApplied to files:
📚 Learning: 2026-06-07T12:07:24.608ZApplied to files:
📚 Learning: 2026-06-25T12:20:18.414ZApplied to files:
📚 Learning: 2026-06-07T12:07:25.164ZApplied to files:
📚 Learning: 2026-06-25T12:20:45.387ZApplied to files:
🔇 Additional comments (2)
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (6 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by step-3.7-flash · Input: 97.3K · Output: 18.8K · Cached: 1.2M |
|
Coverage is 98.476% — MichaelUray:fix/register-field-allowlist into openwisp:master. No base build found for |
|
@coderabbitai summary |
Contributor
✅ Action performedSummary regeneration triggered. |
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
DeviceRegisterView.init_object()builds the newDevicefrom every POSTfield that happens to be a model field, skipping only
organizationandorganization_id. A registrant with a valid organization shared secret cantherefore mass-assign internal fields — most notably the primary key
id.Because the PK has
default=uuid4, Django usesforce_insert, so a chosenidcannot silently overwrite another device (a collision raisesIntegrityError). But it still lets a registrant choose an arbitrary deviceUUID and, by observing whether the response is
201or a server error, probefor the cross-organization existence of a UUID. Other internal fields
(
_is_deactivated,group,created, …) are likewise assignable.Fix
Replace the blocklist with an explicit allowlist of the fields a device
legitimately provides at registration:
plus
keyonly when consistent registration is enabled. Every other field(
id,last_ip,_is_deactivated,group,created,modified, …) is nowset by the server or left at its default.
last_ipis already overwritten fromREMOTE_ADDR, andmanagement_ipremains accepted (covered bytest_register_with_management_ip), so existing behaviour is preserved.Testing
New test
test_register_ignores_disallowed_fields(a forgedidis ignored,the server assigns its own UUID). The full registration test set and
./run-qa-checkspass.