Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
8 changes: 4 additions & 4 deletions build/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -20749,8 +20749,7 @@
}
},
"required": [
"name",
"type"
"name"
],
"allOf": [
{
Expand Down Expand Up @@ -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": [
Expand Down Expand Up @@ -21245,6 +21244,7 @@
"enum": [
"none",
"wep",
"owe",
"psk",
"psk2",
"psk-mixed",
Expand Down
22 changes: 18 additions & 4 deletions src/lib/handler.uc
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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 };
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
12 changes: 9 additions & 3 deletions src/resources/network.devices.uc
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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"] } },
Expand Down
8 changes: 5 additions & 3 deletions src/resources/wireless.devices.uc
Original file line number Diff line number Diff line change
Expand Up @@ -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: {},
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 5 additions & 2 deletions src/resources/wireless.interfaces.uc
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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) });
Expand Down
32 changes: 32 additions & 0 deletions tests/unit/handler_test.uc
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
29 changes: 29 additions & 0 deletions tests/unit/network_wireless_test.uc
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand Down Expand Up @@ -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' };
Expand Down