webui: fix firewall zone service resets

Resetting a zone's services (or interfaces) wiped the entire zone: action, interfaces, address-sets, everything but the name. The keyed zone GET decodes into a wrapper expecting the node bare, but the server nests it under the full parent path: {"infix-firewall:firewall": {"zone": [ ... ]}} The decode silently matched nothing, so the reset handler rebuilt the zone from an empty struct and PUT only the zone name. The same mismatch also broke network preservation when saving a zone. Clear leaf-lists by deleting each instance instead of rebuilding the zone with PUT, so a reset cannot touch anything else. Fix the wrapper shape, document the nesting convention on the RESTCONF client Get, and log a warning when a non-empty response decodes to a zero-valued wrapper. When saving zones, omit empty leaf-lists and a defaulted action. Refresh the firewall page after resets and update the handler tests to mock the real response shape. Signed-off-by: Joachim Wiberg <troglobit@gmail.com>
2026-07-22 01:13:00 +02:00 · 2026-07-04 09:32:01 +02:00
parent 56ea7faa7c
commit 7291b812a6
4 changed files with 205 additions and 79 deletions
@@ -26,11 +26,6 @@ type cfgFwWrapper struct {
 	Firewall *firewallJSON `json:"infix-firewall:firewall,omitempty"`
 }

-// cfgFwZoneWrapper is used when reading a single zone by path.
-type cfgFwZoneWrapper struct {
-	Zone []zoneJSON `json:"infix-firewall:zone"`
-}
-
 // ─── Template display rows ────────────────────────────────────────────────────

 type cfgZoneRow struct {
@@ -63,6 +58,31 @@ type cfgAddrSetRow struct {
 	EntriesTxt string // one entry per line for the textarea
 }

+func zoneConfigBody(cur zoneJSON) map[string]any {
+	zone := map[string]any{
+		"name": cur.Name,
+	}
+	if cur.Action != "" {
+		zone["action"] = cur.Action
+	}
+	if cur.Description != "" {
+		zone["description"] = cur.Description
+	}
+	if len(cur.Interface) > 0 {
+		zone["interface"] = cur.Interface
+	}
+	if len(cur.Network) > 0 {
+		zone["network"] = cur.Network
+	}
+	if len(cur.AddressSet) > 0 {
+		zone["address-set"] = cur.AddressSet
+	}
+	if len(cur.Service) > 0 {
+		zone["service"] = cur.Service
+	}
+	return zone
+}
+
 // toSet builds a membership map for template "index" lookups.
 func toSet(ss []string) map[string]bool {
 	set := make(map[string]bool, len(ss))
@@ -72,11 +92,6 @@ func toSet(ss []string) map[string]bool {
 	return set
 }

-// cfgFwSvcWrapper is used when reading a single service by path.
-type cfgFwSvcWrapper struct {
-	Service []fwServiceJSON `json:"infix-firewall:service"`
-}
-
 // ─── Template data ────────────────────────────────────────────────────────────

 type cfgFirewallPageData struct {
@@ -342,15 +357,15 @@ func (h *ConfigureFirewallHandler) SaveZone(w http.ResponseWriter, r *http.Reque
 	}
 	name := r.PathValue("name")

-	var wrap cfgFwZoneWrapper
+	var wrap firewallWrapper // keyed GETs nest the zone under its full parent path
 	if err := h.RC.Get(r.Context(), fwConfigPath+"/zone="+url.PathEscape(name), &wrap); err != nil {
 		log.Printf("configure firewall zone save %q: GET: %v", name, err)
 		renderSaveError(w, err)
 		return
 	}
 	cur := zoneJSON{Name: name}
-	if len(wrap.Zone) > 0 {
-		cur = wrap.Zone[0]
+	if len(wrap.Firewall.Zone) > 0 {
+		cur = wrap.Firewall.Zone[0]
 	}

 	cur.Action = r.FormValue("action")
@@ -374,20 +389,7 @@ func (h *ConfigureFirewallHandler) SaveZone(w http.ResponseWriter, r *http.Reque
 	}
 	cur.AddressSet = sets

-	zone := map[string]any{
-		"name":        cur.Name,
-		"action":      cur.Action,
-		"interface":   cur.Interface,
-		"service":     cur.Service,
-		"address-set": cur.AddressSet,
-	}
-	if cur.Description != "" {
-		zone["description"] = cur.Description
-	}
-	if len(cur.Network) > 0 {
-		zone["network"] = cur.Network
-	}
-	body := map[string]any{"infix-firewall:zone": []map[string]any{zone}}
+	body := map[string]any{"infix-firewall:zone": []map[string]any{zoneConfigBody(cur)}}
 	if err := h.RC.Put(r.Context(), fwConfigPath+"/zone="+url.PathEscape(name), body); err != nil {
 		log.Printf("configure firewall zone save %q: PUT: %v", name, err)
 		renderSaveError(w, err)
@@ -396,55 +398,38 @@ func (h *ConfigureFirewallHandler) SaveZone(w http.ResponseWriter, r *http.Reque
 	renderSavedRedirect(w, "Zone saved", "/configure/firewall")
 }

-// ResetZoneLeafList clears a leaf-list (interface or service) on a zone by
-// re-PUTting the zone container without that field. RFC 8040 leaf-list
-// DELETE requires per-entry key predicates, so a bulk clear has to go
-// through the parent.
+// ResetZoneLeafList clears a leaf-list (interface or service) on a zone.
+// RFC 8040 leaf-list DELETE requires per-entry key predicates, so each
+// instance is deleted individually.  Deliberately avoids rebuilding the
+// zone with PUT: the reset must not be able to touch anything else.
 func (h *ConfigureFirewallHandler) resetZoneLeafList(w http.ResponseWriter, r *http.Request, leaf string) {
 	name := r.PathValue("name")
-	var wrap cfgFwZoneWrapper
+	var wrap firewallWrapper // keyed GETs nest the zone under its full parent path
 	if err := h.RC.Get(r.Context(), fwConfigPath+"/zone="+url.PathEscape(name), &wrap); err != nil {
 		log.Printf("configure firewall zone reset %s/%s: GET: %v", name, leaf, err)
 		renderSaveError(w, err)
 		return
 	}
-	cur := zoneJSON{Name: name}
-	if len(wrap.Zone) > 0 {
-		cur = wrap.Zone[0]
-	}
-	switch leaf {
-	case "interface":
-		cur.Interface = nil
-	case "service":
-		cur.Service = nil
+	var values []string
+	if len(wrap.Firewall.Zone) > 0 {
+		switch leaf {
+		case "interface":
+			values = wrap.Firewall.Zone[0].Interface
+		case "service":
+			values = wrap.Firewall.Zone[0].Service
+		}
 	}

-	zone := map[string]any{
-		"name":   cur.Name,
-		"action": cur.Action,
+	for _, val := range values {
+		path := fmt.Sprintf("%s/zone=%s/%s=%s", fwConfigPath,
+			restconf.EscapeKey(name), leaf, restconf.EscapeKey(val))
+		if err := h.RC.Delete(r.Context(), path); err != nil && !restconf.IsNotFound(err) {
+			log.Printf("configure firewall zone reset %s/%s=%s: %v", name, leaf, val, err)
+			renderSaveError(w, err)
+			return
+		}
 	}
-	if cur.Description != "" {
-		zone["description"] = cur.Description
-	}
-	if len(cur.Interface) > 0 {
-		zone["interface"] = cur.Interface
-	}
-	if len(cur.Network) > 0 {
-		zone["network"] = cur.Network
-	}
-	if len(cur.AddressSet) > 0 {
-		zone["address-set"] = cur.AddressSet
-	}
-	if len(cur.Service) > 0 {
-		zone["service"] = cur.Service
-	}
-	body := map[string]any{"infix-firewall:zone": []map[string]any{zone}}
-	if err := h.RC.Put(r.Context(), fwConfigPath+"/zone="+url.PathEscape(name), body); err != nil {
-		log.Printf("configure firewall zone reset %s/%s: PUT: %v", name, leaf, err)
-		renderSaveError(w, err)
-		return
-	}
-	renderSaved(w, "Reset to default")
+	renderSavedRedirect(w, "Reset to default", "/configure/firewall")
 }

 // ResetZoneInterfaces clears the zone's interface leaf-list.
@@ -82,9 +82,10 @@ func TestConfigureFirewallOverview_AddressSets(t *testing.T) {

 type recordingFetcher struct {
 	*testutil.MockFetcher
-	putCalls int
-	lastPath string
-	lastBody any
+	putCalls    int
+	lastPath    string
+	lastBody    any
+	deletePaths []string
 }

 func (r *recordingFetcher) Put(_ context.Context, path string, body any) error {
@@ -94,15 +95,29 @@ func (r *recordingFetcher) Put(_ context.Context, path string, body any) error {
 	return nil
 }

+func (r *recordingFetcher) Delete(_ context.Context, path string) error {
+	r.deletePaths = append(r.deletePaths, path)
+	return nil
+}
+
+// zoneGetResponse mimics the server's response shape for a keyed zone GET:
+// the zone is nested under its full parent path, not returned bare.
+func zoneGetResponse(zone map[string]any) map[string]any {
+	return map[string]any{
+		"infix-firewall:firewall": map[string]any{
+			"zone": []map[string]any{zone},
+		},
+	}
+}
+
 func TestConfigureFirewallSaveZoneAllowsInterfacesWithAddressSets(t *testing.T) {
 	mock := &recordingFetcher{MockFetcher: testutil.NewMockFetcher()}
-	mock.SetResponse(candidatePath+"/infix-firewall:firewall/zone=public", map[string]any{
-		"infix-firewall:zone": []map[string]any{{
-			"name":      "public",
-			"action":    "drop",
-			"interface": []string{"eth0"},
-		}},
-	})
+	mock.SetResponse(candidatePath+"/infix-firewall:firewall/zone=public", zoneGetResponse(map[string]any{
+		"name":      "public",
+		"action":    "drop",
+		"interface": []string{"eth0"},
+		"network":   []string{"10.0.0.0/24"},
+	}))

 	h := &ConfigureFirewallHandler{
 		Template: minimalCfgFwTmpl,
@@ -153,6 +168,9 @@ func TestConfigureFirewallSaveZoneAllowsInterfacesWithAddressSets(t *testing.T)
 	if got, want := zone["address-set"], []string{"allowed"}; !reflect.DeepEqual(got, want) {
 		t.Fatalf("want address-sets %#v got %#v", want, got)
 	}
+	if got, want := zone["network"], []string{"10.0.0.0/24"}; !reflect.DeepEqual(got, want) {
+		t.Fatalf("want networks preserved %#v got %#v", want, got)
+	}
 	var trig map[string]string
 	if err := json.Unmarshal([]byte(w.Header().Get("HX-Trigger")), &trig); err != nil {
 		t.Fatalf("unmarshal HX-Trigger: %v", err)
@@ -161,3 +179,99 @@ func TestConfigureFirewallSaveZoneAllowsInterfacesWithAddressSets(t *testing.T)
 		t.Fatalf("unexpected success message %q", got)
 	}
 }
+
+func TestConfigureFirewallSaveZoneClearsAllServices(t *testing.T) {
+	mock := &recordingFetcher{MockFetcher: testutil.NewMockFetcher()}
+	mock.SetResponse(candidatePath+"/infix-firewall:firewall/zone=public", zoneGetResponse(map[string]any{
+		"name":      "public",
+		"action":    "drop",
+		"interface": []string{"eth0"},
+		"service":   []string{"ssh", "http"},
+	}))
+
+	h := &ConfigureFirewallHandler{
+		Template: minimalCfgFwTmpl,
+		RC:       mock,
+		Schema:   schema.NewCache(mock, t.TempDir()),
+	}
+
+	form := url.Values{
+		"action":      {"drop"},
+		"description": {"Public zone"},
+		"interfaces":  {"eth0"},
+	}
+	req := httptest.NewRequest(http.MethodPost, "/configure/firewall/zones/public", strings.NewReader(form.Encode()))
+	req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
+	req.SetPathValue("name", "public")
+	ctx := restconf.ContextWithCredentials(req.Context(), restconf.Credentials{
+		Username: "admin",
+		Password: "admin",
+	})
+	ctx = security.WithToken(ctx, "test-csrf-token")
+	req = req.WithContext(ctx)
+
+	w := httptest.NewRecorder()
+	h.SaveZone(w, req)
+
+	if mock.putCalls != 1 {
+		t.Fatalf("want 1 PUT call got %d", mock.putCalls)
+	}
+	body, ok := mock.lastBody.(map[string]any)
+	if !ok {
+		t.Fatalf("unexpected PUT body type %T", mock.lastBody)
+	}
+	zones, ok := body["infix-firewall:zone"].([]map[string]any)
+	if !ok || len(zones) != 1 {
+		t.Fatalf("unexpected PUT zone payload %#v", body["infix-firewall:zone"])
+	}
+	if _, ok := zones[0]["service"]; ok {
+		t.Fatalf("expected cleared services to be omitted from payload, got %#v", zones[0]["service"])
+	}
+}
+
+func TestConfigureFirewallResetZoneServicesOnlyDeletesServices(t *testing.T) {
+	mock := &recordingFetcher{MockFetcher: testutil.NewMockFetcher()}
+	mock.SetResponse(candidatePath+"/infix-firewall:firewall/zone=public", zoneGetResponse(map[string]any{
+		"name":        "public",
+		"action":      "drop",
+		"interface":   []string{"eth0"},
+		"address-set": []string{"allowed"},
+		"service":     []string{"ssh", "dhcpv6-client"},
+	}))
+
+	h := &ConfigureFirewallHandler{
+		Template: minimalCfgFwTmpl,
+		RC:       mock,
+		Schema:   schema.NewCache(mock, t.TempDir()),
+	}
+
+	req := httptest.NewRequest(http.MethodDelete, "/configure/firewall/zones/public/services", nil)
+	req.SetPathValue("name", "public")
+	ctx := restconf.ContextWithCredentials(req.Context(), restconf.Credentials{
+		Username: "admin",
+		Password: "admin",
+	})
+	ctx = security.WithToken(ctx, "test-csrf-token")
+	req = req.WithContext(ctx)
+
+	w := httptest.NewRecorder()
+	h.ResetZoneServices(w, req)
+
+	if w.Code != http.StatusNoContent {
+		t.Fatalf("want 204 got %d; body: %s", w.Code, w.Body.String())
+	}
+	if got, want := w.Header().Get("HX-Location"), `{"path":"/configure/firewall","target":"#content"}`; got != want {
+		t.Fatalf("want HX-Location %q got %q", want, got)
+	}
+	if mock.putCalls != 0 {
+		t.Fatalf("reset must not rewrite the zone, got %d PUT call(s) with body %#v",
+			mock.putCalls, mock.lastBody)
+	}
+	want := []string{
+		candidatePath + "/infix-firewall:firewall/zone=public/service=ssh",
+		candidatePath + "/infix-firewall:firewall/zone=public/service=dhcpv6-client",
+	}
+	if !reflect.DeepEqual(mock.deletePaths, want) {
+		t.Fatalf("want DELETE paths %#v got %#v", want, mock.deletePaths)
+	}
+}
@@ -9,8 +9,10 @@ import (
 	"encoding/json"
 	"fmt"
 	"io"
+	"log"
 	"net/http"
 	"net/url"
+	"reflect"
 	"strings"
 	"time"
 )
@@ -79,6 +81,12 @@ func (c *Client) doRequest(ctx context.Context, method, path string) (*http.Resp

 // Get fetches a RESTCONF resource, decoding the JSON response into target.
 // User credentials are taken from the request context (set by auth middleware).
+// Get sends a GET request and decodes the JSON response into target.
+//
+// NOTE: deep-path GETs (keyed list entries, nested containers) return the
+// requested node nested under its full parent path, not bare as the RFC
+// 8040 examples may suggest.  Wrapper structs must model the nesting from
+// the top-level container down, or the decode silently matches nothing.
 func (c *Client) Get(ctx context.Context, path string, target any) error {
 	resp, err := c.doRequest(ctx, http.MethodGet, path)
 	if err != nil {
@@ -88,7 +96,20 @@ func (c *Client) Get(ctx context.Context, path string, target any) error {
 	if resp.StatusCode != http.StatusOK {
 		return parseError(resp)
 	}
-	return json.NewDecoder(resp.Body).Decode(target)
+	data, err := io.ReadAll(resp.Body)
+	if err != nil {
+		return err
+	}
+	if err := json.Unmarshal(data, target); err != nil {
+		return err
+	}
+	// Tripwire for the nesting trap above: content arrived but nothing
+	// in the wrapper matched it.
+	if v := reflect.ValueOf(target); len(data) > 2 &&
+		v.Kind() == reflect.Pointer && v.Elem().IsZero() {
+		log.Printf("restconf: GET %s decoded to zero %T, wrapper shape mismatch?", path, target)
+	}
+	return nil
 }

 // Post sends a POST request to a RESTCONF RPC endpoint.