C1-RJ-F1 — isolate repair-job append descriptors - #50
Conversation
Finding C1-RJ-F1 (C1 Repair Job Authority, P2, REPAIR_NOW). The exported append<T>() helper handed an ordinary Object.prototype- inheriting PropertyDescriptor literal to the captured Object.defineProperty. When a hostile getter or Proxy trap installs Object.prototype.get and/or .set, ToPropertyDescriptor walks the prototype chain, observes the inherited accessor keys beside the descriptor's own value/writable keys, rejects the mixed descriptor, and throws TypeError -- breaking the module's documented never- throws / fail-closed / total contract on every append path: the validated-list path (readList over authorizedPaths / authorizedCommandClasses), the invalidFields refusal-reporting path, and the imported permit-id builder that issueExecutionPermit uses at the ALLOW_ONCE mint. This is a totality/reliability failure (fail-stop, not fail-open); no authority escalation. Capture Object.setPrototypeOf at module load beside the existing Object intrinsics and give the descriptor a null prototype before the captured Object.defineProperty consumes it. Descriptor flags, index semantics, element order, invalidFields ordering, refusal semantics, permit identity/authority, the exported append signature, and the public API are all unchanged. The single change also closes the execution-permit consumer with no edit to that file. Regression tests cover ambient get / set / get+set, mid-evaluation poison, the validated-list and invalidFields paths, direct append descriptor semantics, element ordering, realm restoration through a throwing body, and the issueExecutionPermit consumer. Sibling descriptor-family sites remain separate carried obligations. Co-Authored-By: Claude Opus 4.8 <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 provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe repair-job append path now avoids inherited accessor interference by using a null-prototype descriptor. Tests cover poisoned prototypes during repair-job evaluation and permit issuance, including ordering, descriptors, authorization, and cleanup. ChangesPrototype Poisoning Hardening
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to This change isolates repair-job property descriptors and adds regression coverage without changing the public API or authority model; 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 |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Bravo. 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". |
Finding
Affected base
8cfe1f191699fd30226b811c4743777cc2d099fce3b8c4716a554f055262316ce111efa70691444dVerified impact
append<T>()handed an ordinaryObject.prototype-inheritingPropertyDescriptorliteral to the captured
Object.defineProperty. WithObject.prototype.getand/or
.setpresent,ToPropertyDescriptorwalks the prototype chain, observesthe inherited accessor keys beside the descriptor's own
value/writablekeys,and throws
TypeError: Invalid property descriptor…, breaking the module'sdocumented never-throws / fail-closed / total contract.
Verified reachable paths
readListvalid-input path (authorizedPaths/authorizedCommandClasses).invalidFieldsrefusal-reporting path.issueExecutionPermitALLOW_ONCE consumer (importsappendfromrepair-job.ts).Reproduced BEFORE (throws) under: ambient get, ambient set, get+set, mid-evaluation
get, mid-evaluation set, and the execution-permit consumer. Confirmed AFTER: every
path returns normally, never throwing.
Fix
Capture
Object.setPrototypeOfat module load beside the existing Objectintrinsics; give the descriptor a
nullprototype before the capturedObject.definePropertyconsumes it.Scope
src/domain/repair-job.tsonly.tests/domain/repair-job-invariants.test.ts(added),tests/domain/execution-permit.test.ts(consumer regression).Guarantees
appendAPI preserved (name + signature unchanged).execution-permit.tsproduction file unchanged — the one repair-job change closes the consumer.agent-invocation.ts,agent-invocation-report.ts,review-ingestion.ts) untouched — separate carried obligations.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests