From 6b78ebcacb2062c3a5ef9d8f86d52b322fef6cd4 Mon Sep 17 00:00:00 2001
From: Christian Manivong <christian.manivong@gmail.com>
Date: Thu, 16 Jul 2026 12:05:07 +0200
Subject: [PATCH] fix: push_mac_acl() must never set macfilter='disable'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

OpenWrt's wifi-scripts validator rejects any macfilter value other than
"allow"/"deny" outright — confirmed on real hardware, setting
macfilter='disable' puts netifd in a permanent restart crash loop with the
radio stuck down. The only way to disable filtering is to delete the option
entirely. Also guards against pushing an empty whitelist (macfilter='allow'
with zero MACs blocks every client outright) by treating it as equivalent
to "off".
---
 napalm_openwrt/wireless_mixin.py | 19 ++++++++++++++++---
 tests/unit/test_driver.py        | 24 ++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 5 deletions(-)

diff --git a/napalm_openwrt/wireless_mixin.py b/napalm_openwrt/wireless_mixin.py
index 33a702a..a6c240f 100644
--- a/napalm_openwrt/wireless_mixin.py
+++ b/napalm_openwrt/wireless_mixin.py
@@ -570,14 +570,22 @@ class OpenWrtWirelessMixin:
         Full-rebuild, not diff — always deletes the existing maclist before
         re-adding, so the result is idempotent regardless of prior state.
 
+        OpenWrt's wifi-scripts validator rejects any ``macfilter`` value other
+        than ``"allow"``/``"deny"`` outright (confirmed on real hardware:
+        setting ``macfilter='disable'`` puts netifd in a permanent restart
+        crash loop with the radio stuck down) — there is no "off" value; the
+        only way to disable filtering is to omit the option entirely.
+
+        A whitelist ("allow") with zero MACs blocks every client outright, so
+        it is treated as equivalent to "off" instead of being pushed as-is.
+
         :param ssid_name: SSID name to match against ``option ssid`` on each wifi-iface section.
-        :param mode: ``"off"`` | ``"whitelist"`` | ``"blacklist"`` — mapped to UCI
-            ``macfilter`` ``"disable"``/``"allow"``/``"deny"``.
+        :param mode: ``"off"`` | ``"whitelist"`` | ``"blacklist"``.
         :param macs: MAC addresses to set as the maclist. Only the entries for the
             active mode's list are ever passed in — the caller resolves whitelist
             vs. blacklist before calling.
         """
-        uci_mode = {"off": "disable", "whitelist": "allow", "blacklist": "deny"}[mode]
+        effective_mode = "off" if (mode == "whitelist" and not macs) else mode
         sections = self._send_command(
             "uci show wireless | grep -oE '^wireless\\.[^.]+' | sort -u"
         ).split()
@@ -585,6 +593,11 @@ class OpenWrtWirelessMixin:
             ssid_val = self._send_command(f"uci -q get {sec}.ssid 2>/dev/null || true").strip()
             if ssid_val != ssid_name:
                 continue
+            if effective_mode == "off":
+                self._send_command(f"uci -q delete {sec}.macfilter || true")
+                self._send_command(f"uci -q delete {sec}.maclist || true")
+                continue
+            uci_mode = "allow" if effective_mode == "whitelist" else "deny"
             self._send_command(f"uci set {sec}.macfilter='{uci_mode}'")
             self._send_command(f"uci delete {sec}.maclist 2>/dev/null || true")
             for mac in macs:
diff --git a/tests/unit/test_driver.py b/tests/unit/test_driver.py
index 7a36c9d..691e16f 100644
--- a/tests/unit/test_driver.py
+++ b/tests/unit/test_driver.py
@@ -1197,14 +1197,34 @@ class TestPushMacAcl:
         driver.push_mac_acl("CorpWiFi", "blacklist", ["AA:BB:CC:DD:EE:01"])
         assert any("wireless.@wifi-iface[0].macfilter='deny'" in c for c in issued)
 
-    def test_off_sets_macfilter_disable_and_clears_maclist(self, driver):
+    def test_off_deletes_macfilter_option_instead_of_setting_disable(self, driver):
+        """OpenWrt's validator rejects macfilter='disable' outright (confirmed on
+        real hardware — it puts netifd in a permanent restart crash loop with
+        the radio stuck down). "off" must delete the option, never set it."""
         send, issued = self._make_send()
         driver._send_command = send
         driver.push_mac_acl("CorpWiFi", "off", [])
-        assert any("wireless.@wifi-iface[0].macfilter='disable'" in c for c in issued)
+        assert not any("macfilter='disable'" in c for c in issued)
+        assert any("delete wireless.@wifi-iface[0].macfilter" in c for c in issued)
         assert any("delete wireless.@wifi-iface[0].maclist" in c for c in issued)
         assert not any("add_list wireless.@wifi-iface[0].maclist" in c for c in issued)
 
+    def test_whitelist_with_zero_macs_treated_as_off(self, driver):
+        """An empty whitelist blocks every client outright — must not be pushed
+        as macfilter='allow' with an empty list."""
+        send, issued = self._make_send()
+        driver._send_command = send
+        driver.push_mac_acl("CorpWiFi", "whitelist", [])
+        assert not any("macfilter='allow'" in c for c in issued)
+        assert any("delete wireless.@wifi-iface[0].macfilter" in c for c in issued)
+
+    def test_blacklist_with_zero_macs_still_pushed(self, driver):
+        """An empty blacklist is safe (blocks nobody) — no guard needed."""
+        send, issued = self._make_send()
+        driver._send_command = send
+        driver.push_mac_acl("CorpWiFi", "blacklist", [])
+        assert any("macfilter='deny'" in c for c in issued)
+
     def test_maclist_entries_added(self, driver):
         send, issued = self._make_send()
         driver._send_command = send