[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
📝 WalkthroughWalkthroughAdds a module-level UUID regex constant ( Sequence Diagram(s)Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧹 Recent nitpick comments
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2026-01-12T22:27:48.342ZApplied to files:
📚 Learning: 2026-01-15T15:05:49.557ZApplied to files:
📚 Learning: 2026-01-15T15:07:17.354ZApplied to files:
🧬 Code graph analysis (1)openwisp_controller/config/tests/test_admin.py (2)
🪛 Ruff (0.14.13)openwisp_controller/config/tests/test_admin.py1424-1424: Loop control variable Rename unused (B007) ⏰ 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)
🔇 Additional comments (2)
✏️ Tip: You can disable this entire section by setting 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}
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.
| [ | ||
| re_path( | ||
| r"^ws/controller/device/(?P<pk>[^/]+)/$", | ||
| rf"^ws/controller/device/(?P<pk>{uuid_regex})/$", |
There was a problem hiding this comment.
Django has uuid path converter that we should be able to reuse here:
https://docs.djangoproject.com/en/5.2/topics/http/urls/#path-converters
I am not sure why this was added here in the first place, can't we just reuse the main url router defined in the application?
| reverse("controller:device_checksum", args=[pk]), {"key": d.key} | ||
| ) | ||
| bad_path = f"/controller/device/checksum/{pk}/" | ||
| response = self.client.get(bad_path, {"key": d.key}) |
There was a problem hiding this comment.
This change is hardcoding the path instead of using reverse, which adds maintenance overhead, plus I don't understand what's the real difference with the previous version, it does not look like something we should really do.
| reverse("controller:vpn_checksum", args=[pk]), {"key": v.key} | ||
| ) | ||
| bad_path = f"/controller/vpn/checksum/{pk}/" | ||
| response = self.client.get(bad_path, {"key": v.key}) |
| reverse("controller:device_report_status", args=[pk]), {"key": d.key} | ||
| ) | ||
| bad_path = f"/controller/device/report-status/{pk}/" | ||
| response = self.client.post(bad_path, {"key": d.key}) |
| reverse("controller:device_update_info", args=[pk]), params | ||
| ) | ||
| bad_path = f"/controller/device/update-info/{pk}/" | ||
| response = self.client.post(bad_path, params) |
| # custom app URLs | ||
| re_path( | ||
| r"^download/(?P<pk>[^/]+)/$", | ||
| rf"^download/(?P<pk>{uuid_regex})/$", |
There was a problem hiding this comment.
we should migrate to uuid path converters shipped in django:
https://docs.djangoproject.com/en/5.2/topics/http/urls/#path-converters
| rf"^(?P<object_id>({uuid_regex}|__fk__))/change/$", | ||
| self.admin_site.admin_view(self.change_view), | ||
| name=f"{url_prefix}_change", | ||
| ), |
There was a problem hiding this comment.
why do we really need to do this?
| ), | ||
| re_path( | ||
| "controller/device/report-status/(?P<pk>[^/]+)/$", | ||
| rf"controller/device/report-status/(?P<pk>{uuid_regex})/$", |
There was a problem hiding this comment.
same here for tha path converters
|
Thanks @nemesifier im on it
LMK if you want any changes in the above points |
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.