fix(control-plane): confine legacy $ref, and enforce the uri rule ConfigSource declares - #221
Conversation
…figSource declares Two findings Copilot raised on #73 that were never answered. Both are live on main. 1. tools/validate_control_plane_examples.py — unconfined $ref path join `(schema_path.parent / legacy_ref).resolve()` took `legacy_ref` straight from schema content. `$ref` is content, so it is attacker-controlled the moment anyone can open a pull request, and this validator runs in CI: `make validate` depends on `validate-control-plane-examples`, and validate-ops-history.yml runs `make validate`. Demonstrated against unpatched main: a planted `../../../..` ref made the validator read a file outside the repository entirely, and it exited 0. The boundary is the `schemas/` tree, not the wrapper's own directory. Both shapes in use are legitimate and had to keep working: ReleaseSet.json -> "../ReleaseSet.json" (up into schemas/) BootReleaseSet.json -> "./boot-release-set.schema.json" (sibling) Confining to the wrapper's directory — the first thing I tried — rejects the former and breaks the gate. The tighter rule was not the safer one, just wrong. 2. schemas/ConfigSource.json — a declared rule nothing enforced `uri`'s description has always said "Required for git, http, and oci kinds", while the schema typed it `["string","null"]` with no condition. A git source with no location validated clean. That is a declared-unenforced rule: the reader believes a guarantee the contract never made. Now an if/then, so the schema says what the prose says. inline and bundle keep their null uri. Both fail when reverted: - reverted validator: "ESCAPED — read the outside file and exited 0"; restored: "CONFINED — refused the escaping $ref". - reverted schema: accepts a git ConfigSource with no uri; restored: "'uri' is a required property". `make validate` green (OK: validate). ajv compile of ConfigSource valid.
There was a problem hiding this comment.
Pull request overview
This PR tightens the normative control-plane validation and schema contracts by (1) confining legacy wrapper $ref resolution to the repository schemas/ tree to prevent CI runner path traversal, and (2) enforcing the already-declared requirement that ConfigSource.uri must be present for git/http/oci sources.
Changes:
- Add
resolve_legacy_ref()to confine wrapper$reftargets toschemas/and fail closed on escapes. - Enforce
ConfigSource.urias required (and non-null, non-empty) whensourceKindisgit,http, oroci.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tools/validate_control_plane_examples.py | Adds confined $ref resolution to prevent schema-controlled path traversal during CI validation. |
| schemas/ConfigSource.json | Adds conditional schema logic to require uri for git/http/oci sources, matching the documented contract. |
Comments suppressed due to low confidence (1)
tools/validate_control_plane_examples.py:44
- After confinement, the code still allows
$refto resolve toschemas/itself (or another non-file path inside the tree). That will fail later with a low-levelIsADirectoryError/FileNotFoundErrorrather than a clear schema error. Consider validating that the resolved target is a file before attempting to read it, and raiseValueErrorwith context.
base = SCHEMAS.resolve()
target = (schema_path.parent / legacy_ref).resolve()
if target != base and base not in target.parents:
raise ValueError(
f"{schema_path.name}: $ref {legacy_ref!r} resolves to {target}, outside {base}; "
| if "\x00" in legacy_ref: | ||
| raise ValueError(f"{schema_path.name}: $ref contains NUL") |
There was a problem hiding this comment.
Correct — fixed in 96a1b5d.
Confirmed by execution against the version you reviewed:
['./a.json'] -> TypeError: unsupported operand type(s) for /: 'PosixPath' and 'list'
{'x': 1} -> TypeError: unsupported operand type(s) for /: 'PosixPath' and 'dict'
42 -> TypeError: argument of type 'int' is not iterable
True -> TypeError: argument of type 'bool' is not iterable
The point generalizes usefully: $ref is JSON content, so its type is attacker-controlled as well as its value. I guarded the value (traversal, NUL) and left the type unguarded, which is a half-check — and a TypeError raised from inside a path helper is exactly the "crashed somewhere" outcome this validator's exit-code discipline exists to avoid.
Now an explicit isinstance raising ValueError naming the offending type, consistent with the escape and NUL branches beside it:
['./a.json'] -> ValueError: ReleaseSet.json: $ref must be a string, got list
42 -> ValueError: ReleaseSet.json: $ref must be a string, got int
Both legitimate shapes still resolve (./boot-release-set.schema.json sibling, ../ReleaseSet.json parent), make validate is green, and the traversal repro still reports CONFINED.
Copilot on this PR. `$ref` is JSON content, so its TYPE is attacker-controlled as well as its value. A list or dict ref died on `unsupported operand type(s) for /: 'PosixPath' and 'list'`, and an int on `argument of type 'int' is not iterable` — TypeErrors raised from inside a path helper, where what the operator needs to read is "this schema's $ref is malformed". Now an explicit isinstance check raising ValueError with the offending type, consistent with the escape and NUL branches beside it.
Keeper pass — suppressed-channel finding evaluatedCopilot left one finding on this PR that is not in Finding ( Verdict: real, reproduced, not a merge blocker. I ran
The containment property holds in both directions — including the Note also that the inline thread on Follow-up (not blocking): add Second item, on the schema change
|
Clears the sourceos-spec Copilot backlog. Two of the five unanswered threads were live defects; the other three are answered on-thread with reasons rather than fixed.
1. Unconfined
$ref— CI-side path traversaltools/validate_control_plane_examples.pyresolved the wrapper's legacy$refwith no confinement:$refis schema content, so it is attacker-controlled the moment anyone can open a pull request.Asking what calls it, because a finding in unreachable code is not worth a fix:
make validatedepends onvalidate-control-plane-examples.github/workflows/validate-ops-history.ymlrunsmake validateSo it runs in CI. Demonstrated against unpatched
main— planting a../../../..ref intoReleaseSet.json:The boundary is
schemas/, not the wrapper's directoryWorth recording, because the obvious fix is wrong. My first attempt confined to
schema_path.parent— a "legacy ref names a sibling" rule. That broke the gate, because two wrappers legitimately point up:$refReleaseSet.json../ReleaseSet.jsonschemas/ReleaseSet.jsonFingerprint.json../Fingerprint.jsonschemas/Fingerprint.jsonBootReleaseSet.json./boot-release-set.schema.jsonThe tighter rule was not the safer one, it was just the wrong one. Confinement is to the schema tree.
2.
ConfigSource.uri— a declared rule nothing enforcedThe
uridescription has always read "Required for git, http, and oci kinds". The schema typed it["string","null"]with no condition, so a git source with no location validated clean. A description stating a requirement the schema does not check is a declared-unenforced rule: the reader believes a guarantee the contract never made.Now an
if/then. Behaviour, verified against every kind:Evidence
ESCAPED — read the outside file, exited 0CONFINED — refused the escaping $refuri'uri' is a required propertymake validateOK: validateajv compile ConfigSource.json --spec=draft2020The other three threads — rejected, with reasons, on-thread
The ADR findings on #12/#13 ask to renumber
ADR-evidence-fabric-boundaries-v0-1.mdto "the next available number (currently 0006)" to match convention. The premise does not hold:docs/adr/uses0001eight times,0012three times, and0002/0006/0009/0010twice each. Following the advice would have created another collision — and0006is now itself used twice.There is a real issue nearby that Copilot did not name:
0006-evidence-fabric-boundaries.mdnow exists with the conventional header, covering the same decision asADR-evidence-fabric-boundaries-v0-1.md, with different content. Two ADRs for one decision. Flagged on the thread rather than resolved here — picking the canonical one is an editorial call, not a cleanup, and loose files in this estate are usually deliberate.