[Fix] Ensure malformed UUID URLs return 404 #682

[stktyagi]

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.
• I have updated the documentation.

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 NoReverseMatch errors.
Added regression test to verify malformed admin URLs correctly return 404, preventing bug recurrence.

[coderabbitai]

Walkthrough

This change addresses a URL validation vulnerability where overly permissive path parameter patterns were accepting malformed URLs, causing 500 errors instead of 404s. The fix introduces a UUID regex constant across multiple modules to enforce strict UUID-like format validation on path parameters (pk). URL patterns in admin views, API endpoints, WebSocket routes, and utilities are updated to use this tighter pattern, preventing unintended URL matches while maintaining backward compatibility through regex patterns.

Sequence Diagram(s)

No sequence diagrams are applicable to these changes. The modifications are primarily URL pattern refinements and test additions that do not involve new component interactions or sequential control flow changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

The changes apply a consistent, repetitive pattern: introducing UUID regex constants and updating existing URL patterns to use them across multiple files. While the scope spans several modules, each change follows the same straightforward logic without dense algorithms or complex interdependencies. Test additions validate the expected behavior simply. The effort is low because the changes are homogeneous applications of the same fix pattern.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The PR title clearly and specifically describes the main fix: ensuring malformed UUID URLs return 404 instead of 500 errors, directly addressing issue #682.
Description check	✅ Passed	The PR description covers all essential aspects: the UUID regex fix, test updates for invalid URLs, and a regression test. The checklist shows guidelines read and testing completed, though documentation updates were skipped.
Linked Issues check	✅ Passed	The PR successfully implements all coding requirements from issue #682: strict UUID regex patterns replace permissive ones across URL routes, malformed URLs now return 404, and tests verify this behavior.
Out of Scope Changes check	✅ Passed	All changes are directly related to fixing issue #682: UUID pattern tightening in admin, utils, routing, and tests. The new model bindings (Template, Vpn, Organization, etc.) are necessary dependencies for the admin URL constraints.

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

✨ Finishing touches

• 📝 Generate docstrings

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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 to get_extra_context(object_id) assumptions.

You explicitly allow __fk__ in the strict overrides (Line 200-213), but change_view() passes object_id into get_extra_context(), which currently treats any truthy pk as a real UUID (reverse download URL + DB lookup). If /__fk__/change/ is used by UUIDAdmin/related tooling, this likely breaks.

Also, safe_urls currently 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-duplicating uuid_regex.

This will prevent malformed pk segments 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 small openwisp_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 pk URLs 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_regex is duplicated across modules/tests—prefer a shared constant.

openwisp_controller/config/tests/test_controller.py (1)

273-279: Good: avoids reverse() for invalid pk; consider deriving from the valid reverse() result to reduce brittleness.

Instead of hardcoding "/controller/.../{pk}/", you can reverse() the valid URL and then replace/append to create the malformed path—keeps tests resilient to URL prefixing while still avoiding NoReverseMatch.

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_url construction to mirror the exact path shape from issue #682 (right now it’s valid_history_url + "history/...", i.e., “history/history/...”).

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0600954 and b764e67.

📒 Files selected for processing (6)

• openwisp_controller/config/admin.py
• openwisp_controller/config/tests/pytest.py
• openwisp_controller/config/tests/test_admin.py
• openwisp_controller/config/tests/test_controller.py
• openwisp_controller/config/utils.py
• openwisp_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