diff --git a/CHANGELOG.md b/CHANGELOG.md index 39f966e..e125061 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,7 +25,11 @@ The OpenAPI vendor-extension option from the issue thread is deferred until a co ### 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. +- 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. + +- A JSON Patch (RFC 6902) that does not touch a write-only secret no longer drops it. The JSON Patch post-image is built from the masked read view (which exposes `has_key`, not `key`), so a patch that left the secret alone previously produced a post-image without it and tripped conditional-required validation ("key is required when encryption is ..."). The write path now carries forward any `writeOnly` field the patch did not set (`key`, `preshared_key`, `private_key`, `tls_auth`, `pkcs12`), matching what the merge-patch path already did; a patch that explicitly sets a new secret is left intact. + +- Three validation rules relaxed to match what stock OpenWrt ships (found by the same audit): `wireless.devices` accepts an empty `country` (read back as null) and the `"00"` world regulatory domain (stock 6 GHz default); `wireless.interfaces` accepts `encryption='owe'` (Opportunistic Wireless Encryption, keyless, the stock 6 GHz default) without demanding a key; `network.devices` no longer requires `type`, so a `config device` options-override section (name + macaddr, no type, as `config_generate` emits on some targets) round-trips. ### Internal diff --git a/build/openapi.json b/build/openapi.json index 48140dd..b70d9f1 100644 --- a/build/openapi.json +++ b/build/openapi.json @@ -20749,8 +20749,7 @@ } }, "required": [ - "name", - "type" + "name" ], "allOf": [ { @@ -21173,8 +21172,8 @@ "string", "null" ], - "pattern": "^[A-Za-z]{2}$", - "description": "ISO 3166-1 alpha-2 regulatory country code" + "pattern": "^([A-Za-z]{2}|00)$", + "description": "ISO 3166-1 alpha-2 regulatory country code, or \"00\" for the world/global domain (stock 6 GHz default)" }, "txpower": { "type": [ @@ -21245,6 +21244,7 @@ "enum": [ "none", "wep", + "owe", "psk", "psk2", "psk-mixed", diff --git a/src/lib/handler.uc b/src/lib/handler.uc index 9fcbf04..2057bcc 100644 --- a/src/lib/handler.uc +++ b/src/lib/handler.uc @@ -397,7 +397,7 @@ function diff_apply_patch(c, p, id, old_opts, new_opts) { // so we schema-check that; merge-patch (RFC 7396) is partial, so we schema- // check only the delta. Returns either { ok: true, merged, schema_body } or // an error result that the caller short-circuits with. -function apply_patch_body(existing, existing_view, body, ctx, merge_fn) { +function apply_patch_body(existing, existing_view, body, ctx, merge_fn, resource) { if (ctx != null && ctx.json_patch == true) { let jp = jsonpatch.apply(existing_view, body); if (!jp.ok) { @@ -409,7 +409,21 @@ function apply_patch_body(existing, existing_view, body, ctx, merge_fn) { sprintf("[%d]", jp.op_index ?? 0), "invalid_format", jp.message)] }; } - return { ok: true, merged: jp.value, schema_body: jp.value }; + // Carry forward write-only secrets the masked read view hid, unless the + // patch explicitly set one. The merge-patch path does this inside each + // resource's merge_for_patch; the JSON Patch post-image is built from + // existing_view (which exposes has_key, not key), so without this a + // patch that does not touch the secret drops it and trips conditional- + // required validation (e.g. encryption=psk2 needs key). The post[k]==null + // guard keeps a patch-supplied new secret intact. + let post = jp.value; + let sp = (resource != null) ? resource.schema_properties : null; + if (type(sp) == "object") + for (let k in sp) + if (type(sp[k]) == "object" && sp[k].writeOnly + && post[k] == null && existing[k] != null) + post[k] = existing[k]; + return { ok: true, merged: post, schema_body: post }; } return { ok: true, merged: merge_fn(existing, existing_view, body), schema_body: body }; @@ -590,7 +604,7 @@ function make(resource, opts) { message: pc.body.message }; let merge_fn = resource.merge_for_patch ?? default_merge_for_patch; - let r = apply_patch_body(existing, existing_view, body, ctx, merge_fn); + let r = apply_patch_body(existing, existing_view, body, ctx, merge_fn, resource); if (!r.ok) return r; let errs = _validate_with_schema(resource, r.schema_body, r.merged, c, id); @@ -755,7 +769,7 @@ function make_singleton(resource, opts) { for (let k in b) merged[k] = b[k]; return merged; }; - let r = apply_patch_body(existing, existing_view, body, ctx, singleton_merge); + let r = apply_patch_body(existing, existing_view, body, ctx, singleton_merge, resource); if (!r.ok) return r; let errs = _validate_with_schema(resource, r.schema_body, r.merged, c, id); diff --git a/src/resources/network.devices.uc b/src/resources/network.devices.uc index 15328a5..d21b985 100644 --- a/src/resources/network.devices.uc +++ b/src/resources/network.devices.uc @@ -49,8 +49,14 @@ function validate(json) { if (json.name == null || json.name == "") push(errs, { field: "name", code: "required", message: "is required" }); - if (json.type == null || json.type == "") - push(errs, { field: "type", code: "required", message: "is required" }); + // type is optional: a `config device` section with only name + options + // (e.g. a macaddr/mtu override on an existing kernel device) is valid uci + // and is exactly what config_generate emits for per-device macaddr on some + // targets. Requiring type would be stricter than the platform. Validate the + // enum only when a type is actually present. + if (json.type != null && json.type != "" && !VALID_TYPES[json.type]) + push(errs, { field: "type", code: "not_in_enum", + message: "must be one of bridge, 8021q, 8021ad, macvlan, veth, tun, tap" }); if (json.type == "8021q" && (json.vid == null || json.vid == "")) push(errs, { field: "vid", code: "required", @@ -68,7 +74,7 @@ return { toUci: toUci, validate: validate, openapi_singular: "network device", - openapi_required: ["name", "type"], + openapi_required: ["name"], openapi_conditional: [ { if: { properties: { type: { const: "8021q" } }, required: ["type"] }, then: { required: ["vid"] } }, diff --git a/src/resources/wireless.devices.uc b/src/resources/wireless.devices.uc index 2187d98..5d77d35 100644 --- a/src/resources/wireless.devices.uc +++ b/src/resources/wireless.devices.uc @@ -15,7 +15,9 @@ function fromUci(section) { channel: (section.channel == "auto" || section.channel == null) ? section.channel : as_int(section.channel), htmode: section.htmode ?? null, - country: section.country ?? null, + // Stock x86 ships country='' (unset); coerce empty to null so the read + // view is clean and the country pattern check below never sees ''. + country: (section.country == null || section.country == "") ? null : section.country, txpower: as_int(section.txpower), disabled: normalize_bool(section.disabled, false), runtime: {}, @@ -72,8 +74,8 @@ return { description: "Channel number (0-196) or the string \"auto\"" }, htmode: { type: ["string", "null"], description: "HT/VHT/HE mode label (e.g. HT20, VHT80, HE160)" }, - country: { type: ["string", "null"], pattern: "^[A-Za-z]{2}$", - description: "ISO 3166-1 alpha-2 regulatory country code" }, + country: { type: ["string", "null"], pattern: "^([A-Za-z]{2}|00)$", + description: "ISO 3166-1 alpha-2 regulatory country code, or \"00\" for the world/global domain (stock 6 GHz default)" }, txpower: { type: ["integer", "null"], minimum: 0, maximum: 30, description: "TX power in dBm" }, disabled: { type: "boolean", default: false, diff --git a/src/resources/wireless.interfaces.uc b/src/resources/wireless.interfaces.uc index 9bcbedd..14f37c0 100644 --- a/src/resources/wireless.interfaces.uc +++ b/src/resources/wireless.interfaces.uc @@ -5,11 +5,14 @@ const VALID_MODES = { "ap": true, "sta": true, "adhoc": true, "wds": true, "monitor": true, "mesh": true, }; const VALID_ENCRYPTION = { - "none": true, "wep": true, + "none": true, "wep": true, "owe": true, "psk": true, "psk2": true, "psk-mixed": true, "sae": true, "sae-mixed": true, "wpa": true, "wpa2": true, "wpa3": true, "wpa3-mixed": true, }; +// Encryption modes that take no passphrase. owe (Opportunistic Wireless +// Encryption) is keyless and is the stock default on 6 GHz radios. +const NO_KEY_ENCRYPTION = { "none": true, "wep": true, "owe": true }; function lookup_ifname(conn, section_name) { let status = null; @@ -106,7 +109,7 @@ function validate(json) { push(errs, { field: "encryption", code: "not_in_enum", message: "unknown encryption value" }); - let needs_key = json.encryption != null && json.encryption != "none" && json.encryption != "wep"; + let needs_key = json.encryption != null && !NO_KEY_ENCRYPTION[json.encryption]; if (needs_key && (json.key == null || json.key == "")) push(errs, { field: "key", code: "required", message: sprintf("is required when encryption is %J", json.encryption) }); diff --git a/tests/unit/handler_test.uc b/tests/unit/handler_test.uc index d4fb207..0b7a92b 100644 --- a/tests/unit/handler_test.uc +++ b/tests/unit/handler_test.uc @@ -383,6 +383,38 @@ t.describe('handler.patch', () => { }); }); +t.describe('handler.patch JSON Patch write-only secret carry-forward', () => { + let wiface_mod = loadfile('src/resources/wireless.interfaces.uc')(); + let wiface = handler.make(wiface_mod, { + tx: { + acquire: function() { return {}; }, release: function() {}, + reload: function() { return null; }, check_services: function() { return null; }, + }, + }); + function jctx() { return { request_id: "01hx0000000000000000000000", json_patch: true }; } + function seeded() { + return ubus.stub({ uci: { wireless: { + w1: { '.type': 'wifi-iface', '.anonymous': false, + device: 'radio0', ssid: 'home', encryption: 'psk2', key: 'secretpw' }, + } } }); + } + + t.it('a JSON Patch that does not touch the masked key preserves it (no spurious key-required 422)', () => { + let c = seeded(); + let r = wiface.patch(c, jctx(), 'w1', [ { op: 'replace', path: '/ssid', value: 'home2' } ]); + t.assert_equal(r.status, 200); + t.assert_equal(c._state.uci.wireless.w1.ssid, 'home2'); + t.assert_equal(c._state.uci.wireless.w1.key, 'secretpw'); + }); + + t.it('a JSON Patch that sets a new key is not clobbered by carry-forward', () => { + let c = seeded(); + let r = wiface.patch(c, jctx(), 'w1', [ { op: 'add', path: '/key', value: 'newsecret' } ]); + t.assert_equal(r.status, 200); + t.assert_equal(c._state.uci.wireless.w1.key, 'newsecret'); + }); +}); + t.describe('handler.adopt', () => { function with_anon() { let c = with_zones(); diff --git a/tests/unit/network_wireless_test.uc b/tests/unit/network_wireless_test.uc index ae9268e..479fb90 100644 --- a/tests/unit/network_wireless_test.uc +++ b/tests/unit/network_wireless_test.uc @@ -47,6 +47,15 @@ t.describe('network.devices', () => { let te = filter(errs, function(e) { return e.field == "type"; }); t.assert_equal(te[0].code, 'not_in_enum'); }); + + t.it('validate accepts a type-less options-override section (stock macaddr override)', () => { + // config_generate emits anonymous `config device` sections with only + // name + macaddr on some targets; requiring type would be stricter + // than the platform. + let errs = full_validate(netdev, { name: 'wan', macaddr: '00:11:22:33:44:55' }, null); + let te = filter(errs, function(e) { return e.field == "type"; }); + t.assert_equal(length(te), 0); + }); }); t.describe('wireless.devices', () => { @@ -66,6 +75,18 @@ t.describe('wireless.devices', () => { let be = filter(errs, function(e) { return e.field == "band"; }); t.assert_equal(be[0].code, 'not_in_enum'); }); + + t.it('fromUci coerces empty country to null (stock x86 ships country="")', () => { + let r = widev.fromUci({ '.name': 'radio0', '.anonymous': false, '.type': 'wifi-device', + type: 'mac80211', band: '2g', country: '' }); + t.assert_equal(r.country, null); + }); + + t.it('schema accepts "00" world regulatory domain (stock 6g default)', () => { + let errs = full_validate(widev, { type: 'mac80211', band: '6g', country: '00' }, null); + let ce = filter(errs, function(e) { return e.field == "country"; }); + t.assert_equal(length(ce), 0); + }); }); t.describe('wireless.interfaces', () => { @@ -103,6 +124,14 @@ t.describe('wireless.interfaces', () => { t.assert_equal(length(errs), 0); }); + t.it('validate accepts owe encryption without a key (keyless; stock 6g default)', () => { + let errs = full_validate(wiface, { device: 'radio0', ssid: 'mesh6', encryption: 'owe' }, null); + let ke = filter(errs, function(e) { return e.field == "key"; }); + let ee = filter(errs, function(e) { return e.field == "encryption"; }); + t.assert_equal(length(ke), 0); + t.assert_equal(length(ee), 0); + }); + t.it('merge_for_patch carries forward the existing key when the body omits it', () => { let existing_section = { '.name': 'w1', '.anonymous': false, '.type': 'wifi-iface', device: 'radio0', ssid: 'home', encryption: 'psk2', key: 'secretpw' };