Skip to content

[fix] Ensure malformed UUID URLs return 404 #682 #1154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
stktyagi wants to merge 25 commits into
base: master
Choose a base branch
from
Open

[fix] Ensure malformed UUID URLs return 404 #682 #1154

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
ed5eb49
[Fix] Ensure malformed UUID URLs return 404 #682
stktyagi Oct 25, 2025
b764e67
Merge branch 'master' into issues/682-NoReverseMatch-error
stktyagi Jan 10, 2026
0348df0
Merge branch 'master' into issues/682-NoReverseMatch-error
stktyagi Jan 22, 2026
8a96299
[Fix] Fix admin URL reversing in malformed admin URL tests #682
xoaryaa Jan 22, 2026
7e30f8c
[tests] Fix NoReverseMatch in malformed URL tests #682
stktyagi Jan 22, 2026
22b354f
Merge branch 'master' into issues/682-NoReverseMatch-error
stktyagi Feb 2, 2026
b860b75
[Fix] Add basic formatting #682
xoaryaa Feb 3, 2026
fd3641d
Fix malformed UUID admin URLs with custom converter
xoaryaa Feb 2, 2026
083c900
[Fix] Follow-up fixes for UUID admin URL converter #682
stktyagi Feb 3, 2026
8977a73
Merge branch 'master' into issues/682-NoReverseMatch-error
nemesifier Feb 6, 2026
d3fbb24
Merge branch 'master' into issues/682-NoReverseMatch-error
nemesifier Feb 12, 2026
b6239c9
[fix] Replace regex URLs with custom Path Converters #682
stktyagi Feb 14, 2026
40789e3
[fix] Fix admin URL test failures in CI #682
stktyagi Feb 14, 2026
4d79d4f
[fix] Fix NoReverseMatch in strict admin URL regression test #682
stktyagi Feb 14, 2026
b700608
[fix] Use correct reverse path #682
stktyagi Feb 14, 2026
d3ded63
[fix] Improve test range #682
stktyagi Feb 14, 2026
96955e3
[fix] Fix websocket route with path converters #682
stktyagi Feb 14, 2026
9189fa6
[fix] Add tests and docstrings for UUID converters #682
stktyagi Feb 14, 2026
5828d87
[fix] Remove redundant comments #682
stktyagi Feb 14, 2026
54d2df9
Merge branch 'master' into issues/682-NoReverseMatch-error
stktyagi Feb 24, 2026
465f19d
[fix] Moved imports to the top #682
stktyagi Feb 24, 2026
12565e9
Merge branch 'master' into issues/682-NoReverseMatch-error
stktyagi Feb 25, 2026
056609b
[fix] Keep minimal change for bug fix #682
stktyagi Feb 25, 2026
c9ef2a3
[fix] Added tests for converters #682
stktyagi Feb 25, 2026
e8ce4c8
Merge branch 'master' into issues/682-NoReverseMatch-error
stktyagi Mar 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 34 additions & 4 deletions openwisp_controller/config/admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@

logger = logging.getLogger(__name__)
prefix = "config/"
uuid_regex = (
r"[0-9a-fA-F]{8}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{12}"
)
Config = load_model("config", "Config")
Device = load_model("config", "Device")
DeviceGroup = load_model("config", "DeviceGroup")
Expand Down Expand Up @@ -166,9 +169,19 @@ def change_view(self, request, object_id, form_url="", extra_context=None):
def get_urls(self):
options = getattr(self.model, "_meta")
url_prefix = "{0}_{1}".format(options.app_label, options.model_name)
return [
default_urls = super().get_urls()
safe_urls = []
for url in default_urls:
if url.name and not (
url.name.endswith("_change")
or url.name.endswith("_history")
or url.name.endswith("_delete")
Copy link
Copy Markdown
Member

@nemesifier nemesifier Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see these being added back below. Why is this being done, what problems does this solve?
At minimum we need an explanatory comment.

Copy link
Copy Markdown
Member Author

@stktyagi stktyagi Feb 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Copy Markdown
Member

@nemesifier nemesifier Feb 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@stktyagi stktyagi Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

):
safe_urls.append(url)
strict_urls = [
# custom app URLs
re_path(
r"^download/(?P<pk>[^/]+)/$",
rf"^download/(?P<pk>{uuid_regex})/$",
Comment thread
stktyagi marked this conversation as resolved.
Outdated
self.admin_site.admin_view(self.download_view),
name="{0}_download".format(url_prefix),
),
Expand All @@ -178,11 +191,28 @@ def get_urls(self):
name="{0}_preview".format(url_prefix),
),
re_path(
r"^(?P<pk>[^/]+)/context\.json$",
rf"^(?P<pk>{uuid_regex})/context\.json$",
self.admin_site.admin_view(self.context_view),
name="{0}_context".format(url_prefix),
),
] + super().get_urls()
# strict overrides for the default admin views
re_path(
rf"^(?P<object_id>({uuid_regex}|__fk__))/history/$",
self.admin_site.admin_view(self.history_view),
name=f"{url_prefix}_history",
),
re_path(
rf"^(?P<object_id>({uuid_regex}|__fk__))/delete/$",
self.admin_site.admin_view(self.delete_view),
name=f"{url_prefix}_delete",
),
re_path(
rf"^(?P<object_id>({uuid_regex}|__fk__))/change/$",
self.admin_site.admin_view(self.change_view),
name=f"{url_prefix}_change",
),
Comment thread
stktyagi marked this conversation as resolved.
Outdated
]
return strict_urls + safe_urls

def _get_config_model(self):
model = self.model
Expand Down
6 changes: 5 additions & 1 deletion openwisp_controller/config/tests/pytest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,10 @@

Device = load_model("config", "Device")

uuid_regex = (
r"[0-9a-fA-F]{8}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{12}"
)


@pytest.mark.asyncio
@pytest.mark.django_db(transaction=True)
Expand All @@ -25,7 +29,7 @@ class TestDeviceConsumer(CreateDeviceMixin):
URLRouter(
[
re_path(
r"^ws/controller/device/(?P<pk>[^/]+)/$",
rf"^ws/controller/device/(?P<pk>{uuid_regex})/$",
Comment thread
stktyagi marked this conversation as resolved.
Outdated
BaseDeviceConsumer.as_asgi(),
)
]
Expand Down
46 changes: 46 additions & 0 deletions openwisp_controller/config/tests/test_admin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1405,6 +1405,52 @@ def test_change_device_malformed_uuid(self):
response = self.client.get(path)
self.assertEqual(response.status_code, 404)

def test_malformed_admin_urls_return_404(self):
Copy link
Copy Markdown
Member

@nemesifier nemesifier Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

@stktyagi stktyagi Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll modify this test to only assert that the status code isnt 500 for the download url for device, template and vpn.

device = self._create_device()
template = self._create_template()
vpn = self._create_vpn()
test_cases = [
("device", device.pk, "config_device"),
("template", template.pk, "config_template"),
("vpn", vpn.pk, "config_vpn"),
]
junk_path = "some/junk/path/here/"
original_bug_junk = "history/1564/undefinedadmin/img/icon-deletelink.svg"

for model_name, valid_pk, model_admin_name in test_cases:
with self.subTest(model=model_name):
change_url_name = f"admin:{model_admin_name}_change"
valid_change_url = reverse(change_url_name, args=[valid_pk])
malformed_change_url = f"{valid_change_url}{junk_path}"
response_change = self.client.get(malformed_change_url, follow=False)

self.assertEqual(
response_change.status_code,
404,
(f'Malformed "change" URL for {model_name} did not return 404. '),
)
history_url_name = f"admin:{model_admin_name}_history"
valid_history_url = reverse(history_url_name, args=[valid_pk])
malformed_history_url = f"{valid_history_url}{junk_path}"
response_history = self.client.get(malformed_history_url, follow=False)

self.assertEqual(
response_history.status_code,
404,
(f'Malformed "history" URL for {model_name} did not return 404. '),
)
original_bug_url = f"{valid_history_url}{original_bug_junk}"
response_original_bug = self.client.get(original_bug_url, follow=False)

self.assertEqual(
response_original_bug.status_code,
404,
(
f"Original bug URL for {model_name} did not return 404. "
"This may be a regression of bug #682."
),
)

def test_uuid_field_in_change(self):
t = Template.objects.first()
d = self._create_device()
Expand Down
30 changes: 12 additions & 18 deletions openwisp_controller/config/tests/test_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,9 +273,8 @@ def test_device_checksum_requested_signal_is_emitted(self):
def test_device_checksum_bad_uuid(self):
d = self._create_device_config()
pk = "{}-wrong".format(d.pk)
response = self.client.get(
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})
Comment thread
stktyagi marked this conversation as resolved.
Outdated
self.assertEqual(response.status_code, 404)

def test_device_config_download_requested_signal_is_emitted(self):
Expand Down Expand Up @@ -339,9 +338,8 @@ def test_deactivated_device_download_config(self):
def test_device_download_config_bad_uuid(self):
d = self._create_device_config()
pk = "{}-wrong".format(d.pk)
response = self.client.get(
reverse("controller:device_download_config", args=[pk]), {"key": d.key}
)
bad_path = f"/controller/device/download-config/{pk}/"
response = self.client.get(bad_path, {"key": d.key})
self.assertEqual(response.status_code, 404)

def test_vpn_checksum_requested_signal_is_emitted(self):
Expand Down Expand Up @@ -399,9 +397,8 @@ def test_vpn_checksum(self):
def test_vpn_checksum_bad_uuid(self):
v = self._create_vpn()
pk = "{}-wrong".format(v.pk)
response = self.client.get(
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})
Comment thread
stktyagi marked this conversation as resolved.
Outdated
self.assertEqual(response.status_code, 404)

@capture_any_output()
Expand Down Expand Up @@ -516,9 +513,8 @@ def test_vpn_download_config(self):
def test_vpn_download_config_bad_uuid(self):
v = self._create_vpn()
pk = "{}-wrong".format(v.pk)
response = self.client.get(
reverse("controller:vpn_download_config", args=[pk]), {"key": v.key}
)
bad_path = f"/controller/vpn/download-config/{pk}/"
response = self.client.get(bad_path, {"key": v.key})
self.assertEqual(response.status_code, 404)

@capture_any_output()
Expand Down Expand Up @@ -974,9 +970,8 @@ def test_deactivated_device_report_status(self):
def test_device_report_status_bad_uuid(self):
d = self._create_device_config()
pk = "{}-wrong".format(d.pk)
response = self.client.post(
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})
Comment thread
stktyagi marked this conversation as resolved.
Outdated
self.assertEqual(response.status_code, 404)

@capture_any_output()
Expand Down Expand Up @@ -1091,9 +1086,8 @@ def test_device_update_info_bad_uuid(self):
"os": "OpenWrt 18.06-SNAPSHOT r7312-e60be11330",
"system": "Atheros AR9344 rev 3",
}
response = self.client.post(
reverse("controller:device_update_info", args=[pk]), params
)
bad_path = f"/controller/device/update-info/{pk}/"
response = self.client.post(bad_path, params)
Comment thread
stktyagi marked this conversation as resolved.
Outdated
self.assertEqual(response.status_code, 404)

def test_device_update_info_400(self):
Expand Down
23 changes: 13 additions & 10 deletions openwisp_controller/config/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@
from openwisp_notifications.utils import _get_object_link

logger = logging.getLogger(__name__)
uuid_regex = (
r"[0-9a-fA-F]{8}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{12}"
)


def get_object_or_404(model, **kwargs):
Expand Down Expand Up @@ -117,22 +120,22 @@ def get_controller_urls(views_module):
"""
urls = [
re_path(
"controller/device/checksum/(?P<pk>[^/]+)/$",
rf"controller/device/checksum/(?P<pk>{uuid_regex})/$",
views_module.device_checksum,
name="device_checksum",
),
re_path(
"controller/device/download-config/(?P<pk>[^/]+)/$",
rf"controller/device/download-config/(?P<pk>{uuid_regex})/$",
views_module.device_download_config,
name="device_download_config",
),
re_path(
"controller/device/update-info/(?P<pk>[^/]+)/$",
rf"controller/device/update-info/(?P<pk>{uuid_regex})/$",
views_module.device_update_info,
name="device_update_info",
),
re_path(
"controller/device/report-status/(?P<pk>[^/]+)/$",
rf"controller/device/report-status/(?P<pk>{uuid_regex})/$",
Comment thread
stktyagi marked this conversation as resolved.
Outdated
views_module.device_report_status,
name="device_report_status",
),
Expand All @@ -142,33 +145,33 @@ def get_controller_urls(views_module):
name="device_register",
),
re_path(
"controller/vpn/checksum/(?P<pk>[^/]+)/$",
rf"controller/vpn/checksum/(?P<pk>{uuid_regex})/$",
views_module.vpn_checksum,
name="vpn_checksum",
),
re_path(
"controller/vpn/download-config/(?P<pk>[^/]+)/$",
rf"controller/vpn/download-config/(?P<pk>{uuid_regex})/$",
views_module.vpn_download_config,
name="vpn_download_config",
),
# legacy URLs
re_path(
"controller/checksum/(?P<pk>[^/]+)/$",
rf"controller/checksum/(?P<pk>{uuid_regex})/$",
views_module.device_checksum,
name="checksum_legacy",
),
re_path(
"controller/download-config/(?P<pk>[^/]+)/$",
rf"controller/download-config/(?P<pk>{uuid_regex})/$",
views_module.device_download_config,
name="download_config_legacy",
),
re_path(
"controller/update-info/(?P<pk>[^/]+)/$",
rf"controller/update-info/(?P<pk>{uuid_regex})/$",
views_module.device_update_info,
name="update_info_legacy",
),
re_path(
"controller/report-status/(?P<pk>[^/]+)/$",
rf"controller/report-status/(?P<pk>{uuid_regex})/$",
views_module.device_report_status,
name="report_status_legacy",
),
Expand Down
6 changes: 5 additions & 1 deletion openwisp_controller/connection/channels/routing.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,15 @@

from . import consumers as ow_consumer

uuid_regex = (
r"[0-9a-fA-F]{8}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{4}-?[0-9a-fA-F]{12}"
)


def get_routes(consumer=ow_consumer):
return [
re_path(
r"^ws/controller/device/(?P<pk>[^/]+)/command$",
rf"^ws/controller/device/(?P<pk>{uuid_regex})/command$",
consumer.CommandConsumer.as_asgi(),
)
]
Loading