diff --git a/CHANGELOG.md b/CHANGELOG.md index 1549349..39f966e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,10 @@ Both surfaces enumerate the same internal `KNOWN_PATHS` const; the accessor (rat The OpenAPI vendor-extension option from the issue thread is deferred until a code-generated client surfaces with a concrete need; premature spec annotations without consumer evidence is what 2.2.2 / 2.2.3 corrected. +### Fixed + +- PATCH no longer deletes uci options the resource does not model. Previously every write verb shared one `diff_apply` that deleted any existing uci option not re-emitted by `toUci`, so a partial PATCH (e.g. changing `unbound` verbosity, or any field on `firewall.rules`) silently wiped hand-set or stock options the curated model omits (`dns64_prefix`, `icmp_type`, `synflood_protect`, cert `key_type`/`ec_curve`, `ttylogin`, and many more). PATCH now preserves them (RFC 7396 merge-patch: only options inside the resource's own modeled footprint are touched). PUT keeps full-replace semantics: it normalizes the section to the modeled set, so unmodeled options are intentionally dropped on a PUT. An audit of all curated resources against their stock OpenWrt configs found this affected 13 resources. The companion `reject_422` audit (validation stricter than the platform) came back clean for the optional packages; the only residual reject cases are in `wireless` (stock empty `country` / `encryption='owe'` on 6 GHz) and `network.devices` (type-less macaddr-override sections), tracked separately. + ### Internal - New integration test `tests/integration/44_stock_config_test.sh` round-trips every curated CRUD resource and singleton whose package ships in the bare OpenWrt 25.12.4 image (firewall, network, dhcp, dropbear, system): GET the section, adopt if unmanaged, PUT-self (or PATCH-self for singletons), then GET again and assert the persistable shape did not drift. A 422 or a diff surfaces a regression where uapi rejects (or silently mutates) what the platform itself ships. Resources from optional packages (snmpd, lldpd, vnstat, mwan3, etc.) are deferred to a follow-up that wires their install at VM-setup time. First run forced four schema relaxations to bring API rules in line with the platform: `igmp` accepted by `firewall.rules`/`firewall.redirects`; `*`/`any` wildcards accepted by `firewall.rules` zone refs; `dhcp.servers` no longer requires the referenced network interface to exist (stock ships `dhcp.wan` against an absent `network.wan` on x86); `is_valid_cidr_any` accepts IPv6 CIDR (used by `mwan3.rules` once mwan3 coverage lands). diff --git a/docs/architecture.md b/docs/architecture.md index f79cb5b..baf6deb 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -176,6 +176,28 @@ inherits fromUci's string-form view of integer-typed fields, so type-checking the merge would falsely 422 on integer-untouched patches). The full merged body still goes through `resource.validate()` for cross-field logic. +### Unmodeled uci options: PUT replaces, PATCH preserves + +A resource module curates a subset of its uci section's options; `toUci` emits +only those. The write path treats options outside that modeled set +differently per verb: + +- **PUT (replace)** normalizes the section to the modeled set. `diff_apply` + iterates the raw existing section and deletes every option not re-emitted by + `toUci`, including ones the resource does not model. PUT means "this body is + the whole resource," so uapi owns the section. +- **PATCH (partial update)** preserves unmodeled options. `diff_apply_patch` + computes the modeled footprint as `toUci(fromUci(existing))` and only deletes + within it (a modeled field the patch cleared). Raw uci options outside the + footprint (which `toUci` cannot emit) are never enumerated, so a PATCH that + touches one field leaves a hand-set or stock option like firewall `icmp_type` + / unbound `dns64_prefix` untouched. This matches RFC 7396 merge-patch and + avoids the silent data loss a naive delete-everything-absent would cause. + +A consequence: the GET-then-PATCH-self round trip is lossless even for options +uapi does not model, but GET-then-PUT-self is not (PUT discards them). Tooling +that wants to preserve unmodeled state must use PATCH. + Field errors are deduped by `(field, code)`. Schema errors win over validator errors for the same (field, code) tuple. The 422 body carries the full set; clients fix everything in one round trip. diff --git a/src/lib/handler.uc b/src/lib/handler.uc index 71652d9..9fcbf04 100644 --- a/src/lib/handler.uc +++ b/src/lib/handler.uc @@ -364,10 +364,10 @@ function translate_tx(ctx, result) { sprintf("transaction returned unknown kind %J", result.kind)); } -// Replace any uci options not in new_opts; set the rest. The two-loop shape -// is required because uci_set on an existing key updates; we have to -// explicitly delete the keys the new body omitted. The leading-dot keys -// (.name/.type/.anonymous) are pseudo and must not be touched. +// PUT/replace semantics: the body is the whole resource, so any uci option +// not in new_opts is removed. Iterates the RAW existing section, so options +// the resource does not model are normalized away too (uapi owns the section +// on replace). The leading-dot keys (.name/.type/.anonymous) are pseudo. function diff_apply(c, p, id, existing, new_opts) { for (let k in existing) { if (substr(k, 0, 1) == ".") continue; @@ -377,6 +377,21 @@ function diff_apply(c, p, id, existing, new_opts) { for (let k in new_opts) c.uci_set(p, id, k, new_opts[k]); } +// PATCH semantics: a partial update must not wipe options the resource does +// not model. old_opts is toUci(fromUci(existing)) -- exactly the modeled keys +// the section currently carries. We only delete within that footprint (a key +// the patch cleared); raw uci options outside it (toUci cannot emit them) are +// left untouched. Without this, a PATCH that touches one field would delete +// every stock/operator option the resource happens not to model. +function diff_apply_patch(c, p, id, old_opts, new_opts) { + for (let k in old_opts) { + if (substr(k, 0, 1) == ".") continue; + if (exists(new_opts, k)) continue; + c.uci_delete(p, id, k); + } + for (let k in new_opts) c.uci_set(p, id, k, new_opts[k]); +} + // Reduce a PATCH body against existing state to a post-image plus a schema- // validation target. JSON Patch (RFC 6902) synthesises the full post-image, // so we schema-check that; merge-patch (RFC 7396) is partial, so we schema- @@ -587,7 +602,7 @@ function make(resource, opts) { return { ok: false, kind: "validation", errors: uf_errs }; let new_opts = resource.toUci(r.merged); - diff_apply(c, p, id, existing, new_opts); + diff_apply_patch(c, p, id, resource.toUci(existing_view), new_opts); let view = { ...new_opts }; view['.name'] = id; view['.anonymous'] = false; @@ -748,7 +763,7 @@ function make_singleton(resource, opts) { return { ok: false, kind: "validation", errors: errs }; let new_opts = resource.toUci(r.merged); - diff_apply(c, p, id, existing, new_opts); + diff_apply_patch(c, p, id, resource.toUci(existing_view), new_opts); let view = { ...new_opts }; view['.name'] = id; view['.anonymous'] = !!existing['.anonymous']; diff --git a/tests/integration/44_stock_config_test.sh b/tests/integration/44_stock_config_test.sh index bcb9e1f..9e1cf16 100755 --- a/tests/integration/44_stock_config_test.sh +++ b/tests/integration/44_stock_config_test.sh @@ -147,4 +147,25 @@ for s in $SINGLETONS; do echo " ok: $s" done +echo "=== PATCH preserves uci options uapi does not model (RFC-hybrid) ===" +# Regression guard for the diff_apply silent-drop class: a partial PATCH must +# not delete options the resource doesn't model. `localservice` is a real +# dnsmasq option uapi does not curate; set it, PATCH a modeled field, and +# confirm it survives. The drift check above can't catch this (fromUci never +# surfaces unmodeled keys), so it needs a direct uci read. +$SSH "uci set dhcp.@dnsmasq[0].localservice='1'; uci commit dhcp" +patch_code=$(curl -sS -H "$ADMIN" -H 'Content-Type: application/json' \ + -o /dev/null -w '%{http_code}' -X PATCH "$URL/dhcp/dnsmasq" -d '{"domain":"uapitest"}') +[ "$patch_code" = "200" ] || fail "PATCH /dhcp/dnsmasq returned $patch_code" +# `uci -q get` exits non-zero when the key is absent; `|| true` keeps the +# assignment from tripping `set -e` so the explicit check below reports clearly. +preserved=$($SSH "uci -q get dhcp.@dnsmasq[0].localservice" 2>/dev/null || true) +[ "$preserved" = "1" ] || fail "PATCH wiped unmodeled uci option localservice (silent_drop regression): got '$preserved'" +echo " ok: unmodeled option survived PATCH" + +# Note: the PUT-side counterpart (full replace DOES drop unmodeled options) is +# covered by the unit test handler_test.uc "normalizes away unmodeled uci +# options" and is already exercised by the round-trip loop above; no separate +# integration assertion here. + echo "stock-config round-trip ok" diff --git a/tests/unit/handler_test.uc b/tests/unit/handler_test.uc index 7dbdfed..d4fb207 100644 --- a/tests/unit/handler_test.uc +++ b/tests/unit/handler_test.uc @@ -303,6 +303,18 @@ t.describe('handler.replace', () => { t.assert_deep_equal(r.body.match.dest_port, []); t.assert_deep_equal(r.body.match.proto, []); }); + + t.it('normalizes away unmodeled uci options (PUT = full replace, uapi owns the section)', () => { + // Counterpart to the PATCH-preserve test: PUT is a whole-resource + // replace, so an option uapi does not model is intentionally dropped. + let c = with_existing(); + c._state.uci.firewall.r_existing.icmp_type = ['echo-request']; + let r = rules.replace(c, ctx(), 'r_existing', { + target: 'DROP', match: { src_zone: 'wan' }, + }); + t.assert_equal(r.status, 200); + t.assert_equal(c._state.uci.firewall.r_existing.icmp_type, null); + }); }); t.describe('handler.patch', () => { @@ -349,6 +361,26 @@ t.describe('handler.patch', () => { t.assert_equal(r.body.id, 'r_existing'); t.assert_equal(r.body.target, 'REJECT'); }); + + t.it('preserves uci options the resource does not model (PATCH is partial)', () => { + // icmp_type and limit are real stock firewall-rule options uapi does + // not model. A partial PATCH must not delete them (RFC-7396 merge). + let c = with_existing(); + c._state.uci.firewall.r_existing.icmp_type = ['echo-request']; + c._state.uci.firewall.r_existing.limit = '1000/sec'; + let r = rules.patch(c, ctx(), 'r_existing', { target: 'REJECT' }); + t.assert_equal(r.status, 200); + t.assert_deep_equal(c._state.uci.firewall.r_existing.icmp_type, ['echo-request']); + t.assert_equal(c._state.uci.firewall.r_existing.limit, '1000/sec'); + }); + + t.it('still deletes a modeled field the patch clears (footprint delete)', () => { + let c = with_existing(); + let r = rules.patch(c, ctx(), 'r_existing', { match: { dest_port: [] } }); + t.assert_equal(r.status, 200); + t.assert_deep_equal(r.body.match.dest_port, []); + t.assert_equal(c._state.uci.firewall.r_existing.dest_port, null); + }); }); t.describe('handler.adopt', () => {