From 7291b812a6fd9566a6e37cccf0776b14e486b25c Mon Sep 17 00:00:00 2001 From: Joachim Wiberg Date: Thu, 2 Jul 2026 17:10:31 +0200 Subject: [PATCH] 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 --- doc/ChangeLog.md | 10 +- .../internal/handlers/configure_firewall.go | 117 +++++++-------- .../handlers/configure_firewall_test.go | 134 ++++++++++++++++-- src/webui/internal/restconf/client.go | 23 ++- 4 files changed, 205 insertions(+), 79 deletions(-) diff --git a/doc/ChangeLog.md b/doc/ChangeLog.md index ddc5bf72..9df059a7 100644 --- a/doc/ChangeLog.md +++ b/doc/ChangeLog.md @@ -3,14 +3,19 @@ Change Log All notable changes to the project are documented in this file. -[UNRELEASED][] --------------- +[v26.08.0][UNRELEASED] +------------------------- ### Changes - Add support for firewall address-set (ipset): named sets of IP addresses and networks, usable as zone sources for per-IP access control, issue #1189 +### Fixes + +- Fix annoying "cannot deselect all services" or reset to YANG default in the + web interface's firewall configuration page + [v26.06.0][] - 2026-07-01 ------------------------- @@ -2214,6 +2219,7 @@ Supported YANG models in addition to those used by sysrepo and netopeer: [buildroot]: https://buildroot.org/ [UNRELEASED]: https://github.com/kernelkit/infix/compare/v26.06.0...HEAD +[v26.08.0]: https://github.com/kernelkit/infix/compare/v26.06.0...v26.08.0 [v26.06.0]: https://github.com/kernelkit/infix/compare/v26.05.0...v26.06.0 [v26.05.0]: https://github.com/kernelkit/infix/compare/v26.04.0...v26.05.0 [v26.04.0]: https://github.com/kernelkit/infix/compare/v26.03.0...v26.04.0 diff --git a/src/webui/internal/handlers/configure_firewall.go b/src/webui/internal/handlers/configure_firewall.go index 72cc1f8b..a69e09c8 100644 --- a/src/webui/internal/handlers/configure_firewall.go +++ b/src/webui/internal/handlers/configure_firewall.go @@ -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. diff --git a/src/webui/internal/handlers/configure_firewall_test.go b/src/webui/internal/handlers/configure_firewall_test.go index 518fecd2..4d4b44db 100644 --- a/src/webui/internal/handlers/configure_firewall_test.go +++ b/src/webui/internal/handlers/configure_firewall_test.go @@ -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) + } +} diff --git a/src/webui/internal/restconf/client.go b/src/webui/internal/restconf/client.go index 0f647090..50cc14aa 100644 --- a/src/webui/internal/restconf/client.go +++ b/src/webui/internal/restconf/client.go @@ -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.