C1-A03 — Reject inherited repair list elements - #19
Conversation
`readList` obtained each authorization-list entry with an ordinary indexed read, which walks the prototype chain. At a sparse hole that resolved whatever a custom array prototype — or `Array.prototype` itself — carried at that numeric key, so a value the operator never supplied could enter the trusted `RepairJobAuthorization` snapshot as an authorized path or command class and reach `ALLOW_ONCE` with an `ExecutionPermit` bound to the fabricated operand. Entries are now obtained through `readOwnElement`, which gates the read behind the module's already-captured `Object.hasOwn` and reports absence with a module-private sentinel rather than collapsing it into `undefined`, so the list refuses a missing element itself instead of relying on the element reader. A sparse hole rejects the whole list: never skipped, defaulted, or filled from the prototype chain. Dense own lists are unaffected. The guarantee is documented at the strength the code proves. It holds for any array whose own-property introspection is truthful; a Proxy defines the observable result of both the own check and the read, so one that misreports ownership can still pass an inherited value through. That widens nothing — such a caller can supply the same value as a dense own element — and the comment and architecture text now say so rather than claiming an atomic observation. The sentinel is a bare object literal, so it adds no call into a mutable global and keeps the module's captured-intrinsic discipline. Merge stays OPERATOR_REQUIRED, auto-merge stays DENY, and C1-A01 and C1-A02 are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughAuthorization-list validation now requires own indexed properties. Sparse holes, inherited values, and throwing ownership or element reads are rejected. Tests cover arrays, prototypes, access traps, Proxies, module evaluation, and existing merge barriers. ChangesAuthorization list validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The authorization-list change is localized and all reported validation checks pass; no actionable merge-blocking risk remains beyond normal checks and review. Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Finding
C1-A03 — P2 CURRENT at protected-parent HEAD:
146734b751371bc0db38ea0f37dd62ccb68d078fProtected parent:
PR #14 — Cockpit C1
This is an isolated stacked validation PR targeting:
cockpit/c1-job-authorityIt does NOT target
main.Original defect
readListobtained authorization-list entries with ordinary indexed reads.For sparse arrays, an indexed read can walk the prototype chain.
A well-formed inherited numeric value from either a custom array prototype or
Array.prototypecould therefore enter trusted repair-job authorization state.Confirmed reachable effects on the vulnerable parent included:
authorizedPaths;source.editchanging from DENY toALLOW_ONCE;ExecutionPermitbeing issued for the fabricated path;authorizedCommandClassesand gaining command-class authority.The defect did not grant merge or auto-merge authority.
Repair
Authorization-list elements are now read through an own-element gate using the
module's already-captured
Object.hasOwn.For ordinary arrays whose own-property introspection is truthful:
Array.prototypenumeric pollution cannot fill a hole with authorization;The internal absence sentinel is a module-private object literal, avoiding a new
mutable-global
Symbol(...)dependency.The documentation explicitly records the hostile-Proxy boundary:
a Proxy defines the observable result of both the own-property check and the
subsequent read. C1 does not claim provenance beyond the supplied object's
observable own-property report.
A lying Proxy does not widen the caller's authority because a caller capable of
supplying that Proxy can supply the same value directly as a dense own element.
Scope
Changed files only:
src/domain/repair-job.tsdocs/architecture/C1-repair-job-authority.mdtests/domain/job-authorization-invariants.test.tsNo other production file changed.
C1-A01 remains FIXED and unchanged.
C1-A02 remains FIXED and unchanged.
No merge execution.
No auto-merge authority.
No GitHub mutation authority.
No filesystem/process/provider authority.
No authentication.
No replay store.
No workflow-state machinery.
No PR #10 transport work.
No unrelated refactor.
Independent validation
A separate validator that did not implement the repair independently:
Array.prototypenumeric pollution;ALLOW_ONCEwith a permit;Final independent result:
PASS
Validation
Actual final results at commit:
14b4fb3674c6409bc29ca2061221e0a5b2030b7anpm run typechecknpm run lintnpm testnpm run buildnpm auditgit diff --checkPassing tests are evidence, not absolute proof.
Quarantine
This PR is a stacked validation PR.
It must pass:
before it may reach
READY_FOR_MERGE.MERGE IS OPERATOR-ONLY.
No AI agent is authorized to merge this repair PR.
No auto-merge.
A PASS permits only an operator decision to merge this exact repair PR into the
protected parent.
After any operator merge, the protected parent PR's new HEAD must be re-audited.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation