From 5cf3e045496efad220eb9c4bf66243a62c35fb88 Mon Sep 17 00:00:00 2001 From: Marcin Zieba Date: Thu, 2 Jul 2026 20:25:32 +0200 Subject: [PATCH 01/17] fix(security): escape untrusted LibreNMS data and gate verify endpoints Escape untrusted LibreNMS/VLAN values in the interface table render (XSS), validate the POSTed server_key and escape interface_name_field on the sync redirect, gate the single-interface / IP / module / VLAN verify endpoints on object-view permission before resolving the device, guard the CSRF-token lookups in the verify/persist JS handlers, and give the NetBox-only modal checkboxes and the promote modal accessible names. --- .../js/librenms_sync.js | 155 ++++- netbox_librenms_plugin/tables/interfaces.py | 89 +-- .../_interface_sync.html | 13 +- .../_interface_sync_content.html | 6 +- .../htmx/device_validation_details.html | 4 + .../tests/test_coverage_sync_interfaces.py | 132 +++++ .../tests/test_interface_sync_button_type.py | 53 ++ .../tests/test_ip_verify.py | 66 +++ .../tests/test_permissions.py | 22 + .../test_validation_template_server_key.py | 37 ++ .../tests/test_verify_views.py | 543 ++++++++++++++++++ .../tests/test_view_wiring.py | 67 ++- 12 files changed, 1118 insertions(+), 69 deletions(-) create mode 100644 netbox_librenms_plugin/tests/test_interface_sync_button_type.py create mode 100644 netbox_librenms_plugin/tests/test_validation_template_server_key.py diff --git a/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js b/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js index 9a9b0a0207..78a7685888 100644 --- a/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js +++ b/netbox_librenms_plugin/static/netbox_librenms_plugin/js/librenms_sync.js @@ -18,6 +18,17 @@ const TOMSELECT_INIT_DELAY_MS = 100; const COUNTDOWN_UPDATE_INTERVAL_MS = 1000; +/** + * Return the CSRF token value, or null when the hidden input is missing/empty. + * Callers MUST bail (running any needed UI cleanup) on null instead of reading + * `.value` off a missing element, which throws a TypeError and breaks the handler. + * @returns {string|null} + */ +function getCsrfToken() { + const input = document.querySelector('[name=csrfmiddlewaretoken]'); + return input && input.value ? input.value : null; +} + /** * Show a Bootstrap modal, using native Bootstrap Modal when available, * falling back to manual DOM manipulation otherwise. @@ -291,34 +302,59 @@ function initializeTableCheckboxes(tableId) { const table = document.getElementById(tableId); if (!table) return; + // Query the CURRENT checkboxes live inside every handler instead of closing over a snapshot. + // The master initializer re-runs on each htmx:afterSwap, but the dataset guards below keep the + // toggle/shift handlers from re-binding on a SURVIVING toggle; a NodeList captured once + // would then go stale, so select-all / shift-range would iterate detached checkboxes and miss + // the rows a later row-level swap injected. + const liveCheckboxes = () => Array.from(table.querySelectorAll('td input[name="select"]')); const toggleAll = table.querySelector('th input.toggle'); - const checkboxes = table.querySelectorAll('td input[name="select"]'); - let lastChecked = null; + // Persist the shift-range anchor on the TABLE element, not in a per-call closure. This + // initializer re-runs on every htmx:afterSwap: checkboxes bound in an earlier run keep their + // handlers (the dataset guard skips re-binding), so a closure-scoped anchor would leave old + // rows referencing a stale `lastChecked` while rows added by a later swap use a fresh one — + // shift-clicking between the two then uses disconnected anchors and selects nothing. One + // anchor on the shared table node keeps every row's handler in sync across swaps. + const getAnchor = () => table._lnmsLastChecked || null; + const setAnchor = (cb) => { + table._lnmsLastChecked = cb; + }; - if (toggleAll) { + // Guard against stacked handlers: register each listener at most once per element (a dataset + // flag marks it done) since the master initializer re-runs on every htmx:afterSwap. + if (toggleAll && toggleAll.dataset.tableToggleInitialized !== 'true') { + toggleAll.dataset.tableToggleInitialized = 'true'; toggleAll.addEventListener('change', function () { - checkboxes.forEach(checkbox => { + liveCheckboxes().forEach(checkbox => { checkbox.checked = toggleAll.checked; }); }); } - checkboxes.forEach(checkbox => { + liveCheckboxes().forEach(checkbox => { + if (checkbox.dataset.tableClickInitialized === 'true') return; + checkbox.dataset.tableClickInitialized = 'true'; checkbox.addEventListener('click', function (e) { - if (!lastChecked) { - lastChecked = checkbox; + const anchor = getAnchor(); + if (!anchor) { + setAnchor(checkbox); return; } if (e.shiftKey) { - const start = Array.from(checkboxes).indexOf(checkbox); - const end = Array.from(checkboxes).indexOf(lastChecked); - Array.from(checkboxes).slice(Math.min(start, end), Math.max(start, end) + 1).forEach(cb => { - cb.checked = lastChecked.checked; - }); + const current = liveCheckboxes(); + const start = current.indexOf(checkbox); + const end = current.indexOf(anchor); + // Skip the range when the prior anchor was swapped out (indexOf -1) rather than + // slicing a bogus range off the live list. + if (start !== -1 && end !== -1) { + current.slice(Math.min(start, end), Math.max(start, end) + 1).forEach(cb => { + cb.checked = anchor.checked; + }); + } } - lastChecked = checkbox; + setAnchor(checkbox); }); }); } @@ -589,11 +625,17 @@ function verifyVlanInGroup(select, deviceId, vid, vlanType, groupId) { const modal = document.getElementById('vlanDetailModal'); const capturedSafeName = modal?.dataset.currentSafeName; + const csrfToken = getCsrfToken(); + if (!csrfToken) { + _vlanVerifyEnd(saveBtn); // don't leave the Save button stuck disabled + return; + } + fetch('/plugins/librenms_plugin/verify-vlan-group/', { method: 'POST', headers: { 'Content-Type': 'application/json', - 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value + 'X-CSRFToken': csrfToken }, body: JSON.stringify({ device_id: deviceId, @@ -774,11 +816,24 @@ function initializeVlanModalSave() { // Persist overrides in server cache so other table pages pick them up if (applyToAll && Object.keys(vidGroupMap).length > 0) { const deviceId = modalEl.dataset.currentDeviceId; + const csrfToken = getCsrfToken(); + if (!csrfToken) { + // Can't persist without a CSRF token; surface it via the same error UI the + // fetch .catch uses rather than letting a `.value`-on-null TypeError abort silently. + let alertEl = modalEl.querySelector('.vlan-override-error'); + if (!alertEl) { + alertEl = document.createElement('div'); + alertEl.className = 'vlan-override-error alert alert-danger mt-2'; + modalEl.querySelector('.modal-body')?.appendChild(alertEl); + } + alertEl.textContent = 'Failed to save VLAN group overrides: CSRF token not found.'; + return; + } fetch('/plugins/librenms_plugin/save-vlan-group-overrides/', { method: 'POST', headers: { 'Content-Type': 'application/json', - 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value + 'X-CSRFToken': csrfToken }, body: JSON.stringify({ device_id: deviceId, @@ -916,11 +971,14 @@ function handleVRFChange(select, value) { } const deviceId = deviceInfo.id; + const csrfToken = getCsrfToken(); + if (!csrfToken) return; // missing token → abort rather than throw on `.value` + fetch('/plugins/librenms_plugin/verify-ipaddress/', { method: 'POST', headers: { 'Content-Type': 'application/json', - 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value + 'X-CSRFToken': csrfToken }, body: JSON.stringify({ device_id: deviceId, @@ -958,11 +1016,14 @@ function handleVRFChange(select, value) { * @param {string} value - Selected device ID */ function handleInterfaceChange(select, value) { + const csrfToken = getCsrfToken(); + if (!csrfToken) return; // missing token → abort rather than throw on `.value` + fetch('/plugins/librenms_plugin/verify-interface/', { method: 'POST', headers: { 'Content-Type': 'application/json', - 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value + 'X-CSRFToken': csrfToken }, body: JSON.stringify({ device_id: value, @@ -1004,11 +1065,14 @@ function handleInterfaceChange(select, value) { * @param {string} value - Selected device ID */ function handleCableChange(select, value) { + const csrfToken = getCsrfToken(); + if (!csrfToken) return; // missing token → abort rather than throw on `.value` + fetch('/plugins/librenms_plugin/verify-cable/', { method: 'POST', headers: { 'Content-Type': 'application/json', - 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value + 'X-CSRFToken': csrfToken }, body: JSON.stringify({ device_id: value, @@ -1058,11 +1122,14 @@ function handleModuleChange(select, value) { const controller = new AbortController(); select._moduleVerifyController = controller; + const csrfToken = getCsrfToken(); + if (!csrfToken) return; // missing token → abort rather than throw on `.value` + fetch('/plugins/librenms_plugin/verify-module/', { method: 'POST', headers: { 'Content-Type': 'application/json', - 'X-CSRFToken': document.querySelector('[name=csrfmiddlewaretoken]').value + 'X-CSRFToken': csrfToken }, body: JSON.stringify({ device_id: value, @@ -1159,15 +1226,22 @@ function initializeBulkEditApply() { function initializeCheckboxListeners() { const interfaceTable = document.getElementById('librenms-interface-table'); if (!interfaceTable) return; - const checkboxes = interfaceTable.querySelectorAll('input[name="select"]'); - checkboxes.forEach(checkbox => { + // Query live inside the handlers: the bulkToggle guard below keeps the toggle handler from + // re-binding on a surviving toggle across htmx:afterSwap, so a captured NodeList would + // go stale and select-all would skip rows added by later row-level swaps. + const liveCheckboxes = () => interfaceTable.querySelectorAll('input[name="select"]'); + // Idempotent across htmx:afterSwap re-runs — register the change handler once per checkbox. + liveCheckboxes().forEach(checkbox => { + if (checkbox.dataset.bulkChangeInitialized === 'true') return; + checkbox.dataset.bulkChangeInitialized = 'true'; checkbox.addEventListener('change', updateBulkActionButton); }); const toggleAll = interfaceTable.querySelector('input.toggle'); - if (toggleAll) { + if (toggleAll && toggleAll.dataset.bulkToggleInitialized !== 'true') { + toggleAll.dataset.bulkToggleInitialized = 'true'; toggleAll.addEventListener('change', function () { - checkboxes.forEach(checkbox => { + liveCheckboxes().forEach(checkbox => { checkbox.checked = toggleAll.checked; }); updateBulkActionButton(); @@ -1629,6 +1703,14 @@ function initializeSyncFormSpinners() { * The form is separate from the table (to avoid nested forms), so we copy the * selected checkbox values into hidden inputs just before the form is submitted. * Guard against duplicate listeners on repeated HTMX swaps via a data attribute. + * + * NOTE: this submit-phase injection only reliably covers the NATIVE (no-htmx) submit + * fallback. When htmx drives the POST, its own submit listener can be registered on the + * form BEFORE this one (fresh page load: htmx's DOMContentLoaded processNode runs before + * initializeScripts), so it serializes the form first and these hidden inputs arrive too + * late. The htmx path is therefore injected at htmx:configRequest (see the + * DOMContentLoaded handler), which fires after serialization and replaces any + * select/device_selection values this handler managed to add. */ function handleInstallSelectedSubmit() { // Remove any previously-injected hidden inputs to avoid duplicates @@ -1943,6 +2025,33 @@ document.addEventListener('DOMContentLoaded', function () { if (csrfToken) { event.detail.headers['X-CSRFToken'] = csrfToken.value; } + // Install Selected: the checked rows live in the table OUTSIDE the form, and htmx's + // own submit listener (attached to the form at ITS DOMContentLoaded processNode, + // which on a fresh page load runs before initializeScripts registers the + // submit-phase injector on the same element — listener ORDER, not event phase, + // decides) serializes the form BEFORE the hidden inputs are injected. The first + // click after a full page load then POSTs no 'select' values and the view warns + // "No modules selected." while wiping the selection. configRequest fires AFTER + // htmx serialization, exactly to let listeners amend the outgoing parameters, so + // injecting here is ordering-independent. Replace (not append to) any + // select/device_selection values the submit-phase injector already serialized so + // rows are never posted twice. + if (event.detail.elt && event.detail.elt.id === 'install-selected-form') { + const params = event.detail.parameters; + Array.from(params.keys()) + .filter((k) => k === 'select' || k.startsWith('device_selection_')) + .forEach((k) => params.delete(k)); + const table = document.getElementById('librenms-module-table'); + if (table) { + table.querySelectorAll('input[name="select"]:checked').forEach((cb) => { + params.append('select', cb.value); + const selectedDevice = table.querySelector(`#device_selection_${cb.value}`); + if (selectedDevice) { + params.append(`device_selection_${cb.value}`, selectedDevice.value); + } + }); + } + } }); }); diff --git a/netbox_librenms_plugin/tables/interfaces.py b/netbox_librenms_plugin/tables/interfaces.py index d99d2a7bfe..34dbc65794 100644 --- a/netbox_librenms_plugin/tables/interfaces.py +++ b/netbox_librenms_plugin/tables/interfaces.py @@ -52,7 +52,12 @@ def __init__(self, *args, device=None, interface_name_field=None, vlan_groups=No self.device = device self.interface_name_field = interface_name_field or get_interface_name_field() self.vlan_groups = vlan_groups or [] - self.server_key = server_key + # Default the key so render_librenms_id's get_librenms_device_id(self.server_key) lookup + # falls back to the "default" server entry; a None key would miss {"default": 42} values. + self.server_key = server_key or "default" + # Lazily-built {(librenms_type, librenms_speed): mapping} cache so render_type doesn't run + # 1-2 InterfaceTypeMapping queries for every interface row (the table is small and static). + self._interface_type_mapping_cache = None # Update column accessors after initialization for column in ["selection", "name"]: @@ -178,23 +183,31 @@ def render_vlans(self, value, record): else: css = get_tagged_vlan_css_class(vid, netbox_tagged_vids, exists_in_netbox, missing_vlans, group_matches) warning = get_missing_vlan_warning(vid, missing_vlans) - inline_parts.append(f'{vid}({vlan_type}){warning}') + # Escape the LibreNMS-sourced vid/vlan_type (XSS, issue #105 class). css is an + # internal class name; warning is the static icon HTML from get_missing_vlan_warning, + # so it is marked safe rather than escaped. + inline_parts.append( + format_html('{}({}){}', css, vid, vlan_type, mark_safe(warning)) + ) - summary = ", ".join(inline_parts) + # inline_parts are already escaped SafeStrings; join them and keep the result safe. + summary = mark_safe(", ".join(str(part) for part in inline_parts)) if len(all_vlans) > MAX_INLINE: extra = len(all_vlans) - MAX_INLINE - summary += f' +{extra} more' + summary = format_html('{} +{} more', summary, extra) - # Build tooltip showing auto-selected VLAN group per VLAN + # Build tooltip showing auto-selected VLAN group per VLAN. Escape the LibreNMS-sourced + # vid/vlan_type and group_name; the " " separator is a literal newline entity for the + # title attribute, so join the escaped lines and mark the whole tooltip safe. tooltip_lines = [] for vlan_type, vid in all_vlans: if vid in missing_vlans: - tooltip_lines.append(f"VLAN {vid}({vlan_type}) → ⚠ Not in NetBox") + tooltip_lines.append(format_html("VLAN {}({}) → ⚠ Not in NetBox", vid, vlan_type)) else: group_info = vlan_group_map.get(vid, {}) group_name = group_info.get("group_name", "Global") - tooltip_lines.append(f"VLAN {vid}({vlan_type}) → {escape(group_name)}") - tooltip_text = " ".join(tooltip_lines) + tooltip_lines.append(format_html("VLAN {}({}) → {}", vid, vlan_type, group_name)) + tooltip_text = mark_safe(" ".join(str(line) for line in tooltip_lines)) # Build hidden inputs for per-VLAN group selections (submitted with form) hidden_inputs = [] @@ -282,8 +295,8 @@ def render_vlans(self, value, record): return format_html( '{}{}{}', - mark_safe(tooltip_text), - mark_safe(summary), + tooltip_text, + summary, edit_btn, hidden_inputs_html, ) @@ -355,29 +368,31 @@ def render_mtu(self, value, record): def render_librenms_id(self, value, record): """Render the 'librenms_id' field with appropriate styling based on comparison with NetBox.""" + # Same XSS guard as _render_field: value/netbox_librenms_id originate outside NetBox, so + # use format_html to auto-escape both the body and the title attribute (issue #105). if not record.get("exists_in_netbox"): - return mark_safe(f'{value}') + return format_html('{}', value) netbox_interface = record.get("netbox_interface") if not netbox_interface: - return mark_safe(f'{value}') + return format_html('{}', value) netbox_librenms_id = get_librenms_device_id(netbox_interface, self.server_key, auto_save=False) if netbox_librenms_id is None: - return mark_safe( - f'{value}' + return format_html( + '{}', value ) # Compare the IDs if str(value) != str(netbox_librenms_id): # IDs do not match - return mark_safe( - f'{value}' + return format_html( + '{}', netbox_librenms_id, value ) else: # IDs match - return mark_safe(f'{value}') + return format_html('{}', value) def _compare_mac_addresses(self, librenms_mac, netbox_interface): """ @@ -399,17 +414,20 @@ def _compare_mac_addresses(self, librenms_mac, netbox_interface): def _render_field(self, value, record, librenms_key, netbox_key): """Render a field value with appropriate styling based on the comparison with NetBox.""" + # value is an untrusted LibreNMS field (ifName, description, MAC, …). Use format_html so + # it is auto-escaped — a device reporting e.g. ifName="" must + # not render as live HTML (stored XSS, issue #105). The class names stay literal. if not record.get("exists_in_netbox"): - return mark_safe(f'{value}') + return format_html('{}', value) netbox_interface = record.get("netbox_interface") if not netbox_interface: - return mark_safe(f'{value}') + return format_html('{}', value) if librenms_key == "ifPhysAddress": mac_matches = self._compare_mac_addresses(value, netbox_interface) css_class = "text-success" if mac_matches else "text-warning" - return mark_safe(f'{value}') + return format_html('{}', css_class, value) netbox_value = getattr(netbox_interface, netbox_key, None) librenms_value = record.get(librenms_key) @@ -418,9 +436,9 @@ def _render_field(self, value, record, librenms_key, netbox_key): librenms_value = convert_speed_to_kbps(librenms_value) if librenms_value != netbox_value: - return mark_safe(f'{value}') + return format_html('{}', value) - return mark_safe(f'{value}') + return format_html('{}', value) def render_type(self, value, record): """Render interface type with appropriate styling based on comparison with NetBox""" @@ -445,18 +463,23 @@ def render_type(self, value, record): return format_html('{}', combined_display) def get_interface_mapping(self, librenms_type, speed): - """Get interface type mapping based on type and speed""" - - # First try exact match with type and speed - mapping = InterfaceTypeMapping.objects.filter(librenms_type=librenms_type, librenms_speed=speed).first() - - # If no match found, fall back to type-only match - if not mapping: - mapping = InterfaceTypeMapping.objects.filter( - librenms_type=librenms_type, librenms_speed__isnull=True - ).first() + """Get interface type mapping based on type and speed. - return mapping + Resolves from a single in-memory snapshot of the (small, static) + InterfaceTypeMapping table, built on first use, so a table render doesn't + issue 1-2 queries per interface row. + """ + if getattr(self, "_interface_type_mapping_cache", None) is None: + cache = {} + # Keep the FIRST mapping per key to match the previous .filter().first() semantics. + for m in InterfaceTypeMapping.objects.all(): + cache.setdefault((m.librenms_type, m.librenms_speed), m) + self._interface_type_mapping_cache = cache + + # Exact (type, speed) match, then the type-only (speed is NULL) fallback. + return self._interface_type_mapping_cache.get((librenms_type, speed)) or self._interface_type_mapping_cache.get( + (librenms_type, None) + ) def render_mapping_tooltip(self, value, speed, mapping): """Render tooltip for interface type mapping""" diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html index fa1d3c0d73..7bb4aeca25 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync.html @@ -7,19 +7,24 @@

Interface Sync

{% csrf_token %} + {# Carry the active server_key so the refresh hits the right LibreNMS server/cache (else a non-default server tab uses the fallback). #} + {% if server_key %}{% endif %} {% if has_librenms_id %} {% with model_name=object|meta:"model_name" %} {% if model_name == "device" %} - {% elif model_name == "virtualmachine" %} - diff --git a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html index c8a51bdcad..5620026c9b 100644 --- a/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html +++ b/netbox_librenms_plugin/templates/netbox_librenms_plugin/_interface_sync_content.html @@ -305,7 +305,8 @@
hx-swap="none" hx-include="#use-sysname-toggle, #strip-domain-toggle"> {% csrf_token %} + {# server_key keeps the action scoped to the active LibreNMS server (issue #106). #} + {% if validation.device_type_mismatch %} @@ -546,6 +548,8 @@
hx-swap="none" hx-include="#use-sysname-toggle, #strip-domain-toggle"> {% csrf_token %} + {# server_key keeps the action scoped to the active LibreNMS server (issue #106). #} + {% if validation.device_type_mismatch %} diff --git a/netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py b/netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py index e3c91942b9..7406c76163 100644 --- a/netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py +++ b/netbox_librenms_plugin/tests/test_coverage_sync_interfaces.py @@ -163,6 +163,46 @@ def test_cache_hit_returns_ports(self): assert result == ports + def test_malformed_cached_ports_treated_as_miss(self): + """A stale/malformed cache entry (ports not a list of dicts, e.g.""" + from netbox_librenms_plugin.views.sync.interfaces import SyncInterfacesView + + view = object.__new__(SyncInterfacesView) + view.get_cache_key = MagicMock(return_value="k") + req = _make_request() + mock_obj = MagicMock(pk=1) + + for bad in ({"ports": None}, {"ports": "oops"}, {"ports": [None]}, ["not-a-dict"]): + with ( + patch("netbox_librenms_plugin.views.sync.interfaces.cache") as mock_cache, + patch("netbox_librenms_plugin.views.sync.interfaces.messages") as mock_msgs, + ): + mock_cache.get.return_value = bad + result = view.get_cached_ports_data(req, mock_obj, "default") + + assert result is None, f"malformed {bad!r} should be a miss" + mock_msgs.warning.assert_called_once() + + def test_dict_without_ports_key_is_noop_not_miss(self): + """A cached dict that simply lacks a 'ports' key is a harmless empty no-op (historical behavior), not a 'refresh first' abort — only PRESENT-but-malformed ports fail closed.""" + from netbox_librenms_plugin.views.sync.interfaces import SyncInterfacesView + + view = object.__new__(SyncInterfacesView) + view.get_cache_key = MagicMock(return_value="k") + req = _make_request() + mock_obj = MagicMock(pk=1) + + for empty in ({}, {"librenms_id": 5}, {"ports": []}): + with ( + patch("netbox_librenms_plugin.views.sync.interfaces.cache") as mock_cache, + patch("netbox_librenms_plugin.views.sync.interfaces.messages") as mock_msgs, + ): + mock_cache.get.return_value = empty + result = view.get_cached_ports_data(req, mock_obj, "default") + + assert result == [], f"{empty!r} should be an empty no-op, not a miss" + mock_msgs.warning.assert_not_called() + def test_no_server_key_uses_librenms_api(self): from netbox_librenms_plugin.views.sync.interfaces import SyncInterfacesView @@ -316,6 +356,57 @@ def test_vm_post_success(self): mock_redirect.assert_called_once() +class TestSyncInterfacesViewServerKeyAndRedirect: + """Issues #107/#108/#109: the POST server_key must be validated against configured servers before it scopes cache/CF lookups, and interface_name_field must be URL-escaped in the post-sync redirect.""" + + def _make_view(self, configured_servers): + from netbox_librenms_plugin.views.sync.interfaces import SyncInterfacesView + + view = object.__new__(SyncInterfacesView) + view.require_all_permissions = MagicMock(return_value=None) + view.get_required_permissions_for_object_type = MagicMock(return_value=[]) + mock_api = MagicMock(server_key="default") + mock_api.get_available_servers.return_value = configured_servers + return view, mock_api + + def _run_no_selection(self, view, mock_api, post_data, name_field="ifName"): + """Drive post() down the no-selection redirect path (server_key + redirect_url are set before that), returning the redirect call mock.""" + with ( + patch("netbox_librenms_plugin.views.sync.interfaces.get_object_or_404", return_value=MagicMock(pk=1)), + patch("netbox_librenms_plugin.views.sync.interfaces.get_interface_name_field", return_value=name_field), + patch("netbox_librenms_plugin.views.sync.interfaces.messages"), + patch("netbox_librenms_plugin.views.sync.interfaces.redirect") as mock_redirect, + patch("netbox_librenms_plugin.views.sync.interfaces.reverse", return_value="/sync/"), + # The POSTed key is validated via the LibreNMSAPI classmethod (not the instance property) + # so a misconfigured default client is never built during validation — mock the classmethod. + patch( + "netbox_librenms_plugin.librenms_api.LibreNMSAPI.get_available_servers", + return_value=mock_api.get_available_servers.return_value, + ), + patch.object(type(view), "librenms_api", new_callable=lambda: property(lambda s: mock_api)), + ): + view.post(_make_request(post_data=post_data), "device", 1) + return mock_redirect + + def test_unconfigured_server_key_falls_back_to_active(self): + view, mock_api = self._make_view({"default": "Default", "secondary": "Secondary"}) + self._run_no_selection(view, mock_api, {"server_key": "evil-server"}) + # The forged key is not configured, so it is dropped in favour of the active server. + assert view._post_server_key == "default" + + def test_configured_server_key_is_honoured(self): + view, mock_api = self._make_view({"default": "Default", "secondary": "Secondary"}) + self._run_no_selection(view, mock_api, {"server_key": "secondary"}) + assert view._post_server_key == "secondary" + + def test_interface_name_field_is_url_escaped_in_redirect(self): + view, mock_api = self._make_view({"default": "Default"}) + mock_redirect = self._run_no_selection(view, mock_api, {}, name_field="ifName&injected=1") + url = mock_redirect.call_args.args[0] + assert "ifName%26injected%3D1" in url + assert "ifName&injected=1" not in url + + # =========================================================================== # SyncInterfacesView.sync_interface — Device paths # =========================================================================== @@ -1308,3 +1399,44 @@ class _DNE(Exception): assert data["deleted_count"] == 1 assert "error" in data["message"] mock_interface_ok.delete.assert_called_once() + + +class TestSyncInterfacesViewServerKeyValidation: + """A POSTed valid non-default server_key must be validated via the LibreNMSAPI classmethod. + + Touching the ``self.librenms_api`` instance property builds the default/selected client, so a + misconfigured default would 500 a sync the user legitimately requested on a working non-default + server. The POSTed key only scopes cache + librenms_id CF reads, so validation must not build. + """ + + def test_valid_server_key_does_not_build_default_client(self): + from netbox_librenms_plugin.views.sync.interfaces import SyncInterfacesView + + view = object.__new__(SyncInterfacesView) + view.require_all_permissions = MagicMock(return_value=None) + view.get_required_permissions_for_object_type = MagicMock(return_value=[]) + view.get_object = MagicMock(return_value=MagicMock(pk=7)) + # No selection → redirect right after server_key is resolved (keeps the test focused). + view.get_selected_interfaces = MagicMock(return_value=None) + + def _boom(self): + raise RuntimeError("default LibreNMS client build must not happen during key validation") + + request = _make_request(post_data={"server_key": "production"}) + + with ( + patch( + "netbox_librenms_plugin.librenms_api.LibreNMSAPI.get_available_servers", + return_value={"production": {"url": "u", "token": "t"}}, + ), + patch.object(type(view), "librenms_api", property(_boom)), + patch("netbox_librenms_plugin.views.sync.interfaces.get_interface_name_field", return_value="ifName"), + patch("netbox_librenms_plugin.views.sync.interfaces.reverse", return_value="/sync/"), + patch("netbox_librenms_plugin.views.sync.interfaces.redirect", side_effect=lambda url: url), + ): + result = view.post(request, object_type="device", object_id=7) + + # Fixed: classmethod validates "production" without touching the property. Unfixed: + # self.librenms_api.get_available_servers() builds the misconfigured default → RuntimeError. + assert view._post_server_key == "production" + assert "server_key=production" in result diff --git a/netbox_librenms_plugin/tests/test_interface_sync_button_type.py b/netbox_librenms_plugin/tests/test_interface_sync_button_type.py new file mode 100644 index 0000000000..1c9ad42b58 --- /dev/null +++ b/netbox_librenms_plugin/tests/test_interface_sync_button_type.py @@ -0,0 +1,53 @@ +"""Issue #116 (CodeRabbit): the HTMX "Refresh Interfaces" buttons in _interface_sync.html +must declare type="button". They live inside a and drive their POST via +hx-post; the HTML default of type="submit" would also fire a native form submit on click. + +The device branch is rendered for real against a real Device (the ``meta`` filter resolves +model_name to "device"); a source-structure check covers the VM branch too without heavy VM +scaffolding. +""" + +import pathlib +import re + +import pytest +from django.contrib.auth.models import AnonymousUser +from django.template.loader import get_template, render_to_string +from django.test import RequestFactory + +TEMPLATE = "netbox_librenms_plugin/_interface_sync.html" + + +def test_every_button_declares_type(): + """Every