D1-44-F2 — isolate Cockpit snapshot from prototype poisoning - #46
Conversation
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 (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change hardens accepted cockpit snapshots against inherited accessor poisoning. Records use null prototypes, arrays use own inert ChangesSnapshot hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This localized repair is merge-ready after normal checks and review; no actionable merge-blocking risk remains. 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 Review ONLY PR #46 at exact CURRENT HEAD: This PR repairs exactly one verified finding: D1-44-F2 Review mandate: Adversarial IMPLEMENTATION review only. Determine whether the 2-file repair fails to preserve the stated D1-44-F2 invariant or introduces a concrete regression caused by this patch. Required finding bar: A reported defect must be:
Do NOT perform a broad architecture review. Reviewer output is evidence/claim only, not authority. Do not modify the PR. |
|
@coderabbitai review Review ONLY PR #46 at exact CURRENT HEAD: Review mandate: DIFF / REGRESSION review only. Scope exactly: src/cockpit/read-model.ts Look only for concrete regressions or missed call sites caused by this patch. Do NOT perform broad security theorising. Any actionable finding must include a concrete CURRENT-head failure. Reviewer output is evidence/claim only, not authority. Do not modify the PR. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== HEAD =='
git rev-parse HEAD
git show -s --format='commit=%H%nparents=%P%nsubject=%s' HEAD
echo '== Scoped files changed by HEAD =='
git diff-tree --no-commit-id --name-only -r HEAD
echo '== Scoped diff against first parent =='
git diff --no-ext-diff --unified=80 HEAD^ HEAD -- \
src/cockpit/read-model.ts \
tests/cockpit/read-model-invariants.test.ts
echo '== Implementation outline =='
ast-grep outline src/cockpit/read-model.ts --items all
echo '== Test outline =='
ast-grep outline tests/cockpit/read-model-invariants.test.ts --items all
echo '== Relevant scoped implementation symbols and call contexts =='
rg -n -C 8 \
-e 'readCockpitSnapshot\s*\(' \
-e 'toJSON' \
-e 'Object\.freeze' \
-e 'Object\.create' \
src/cockpit/read-model.ts \
tests/cockpit/read-model-invariants.test.tsLength of output: 69262 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== readCockpitSnapshot direct call sites =='
rg -n -C 12 '\breadCockpitSnapshot\s*\(' --glob '*.{ts,tsx,js,mjs,cjs}' .
echo '== Imports/re-exports for the read model =='
rg -n -C 8 \
-e "from ['\"].*cockpit.*['\"]" \
-e 'export .*readCockpitSnapshot' \
--glob '*.{ts,tsx,js,mjs,cjs}' \
src tests
echo '== CockpitSnapshot consumer operations that can depend on prototype methods =='
rg -n -C 6 \
-e '\.snapshot\??\.(hasOwnProperty|toString|valueOf|constructor)\b' \
-e '\b(snapshot|repository|provenance|pullRequests|evidence|findings|repairJobs)\.(hasOwnProperty|toString|valueOf|constructor)\b' \
-e 'Object\.getPrototypeOf\s*\(' \
--glob '*.{ts,tsx,js,mjs,cjs}' \
src tests
echo '== Public cockpit index and TypeScript configuration =='
if [ -f src/cockpit/index.ts ]; then
cat -n src/cockpit/index.ts
fi
fd -HI -t f '^(tsconfig.*\.json|package\.json)$' . -x sh -c 'echo "== $1 =="; sed -n "1,240p" "$1"' sh {}Length of output: 60879 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Cockpit public export =='
cat -n src/cockpit/index.ts
echo '== TypeScript and package configuration =='
fd -HI -t f '^(tsconfig.*\.json|package\.json)$' . -x sh -c '
echo "== $1 =="
sed -n "1,240p" "$1"
' sh {}
echo '== Direct prototype-sensitive operations in production source =='
rg -n -C 4 \
-e '\.(hasOwnProperty|toString|valueOf|constructor)\s*(\(|\b)' \
-e 'Object\.getPrototypeOf\s*\(' \
--glob '*.{ts,tsx,js,mjs,cjs}' \
src \
|| true
echo '== Current repair diff whitespace check =='
git diff --check HEAD^ HEAD -- \
src/cockpit/read-model.ts \
tests/cockpit/read-model-invariants.test.tsLength of output: 4309
I reviewed only commit The repair applies the isolation to all accepted snapshot record and list nodes. The public Cockpit exports are unchanged. I found no production call site that depends on inherited methods of returned record nodes. I found no concrete regression or missed call site caused by this patch. ✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. 🎉 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". |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a055e39d6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
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".
| objectDefineProperty(list, 'toJSON', { | ||
| value: undefined, | ||
| enumerable: false, | ||
| writable: false, | ||
| configurable: false, |
There was a problem hiding this comment.
When an input getter runs before freezeList and defines Object.prototype.get or Object.prototype.set, this descriptor literal inherits that accessor. Object.defineProperty then observes both the inherited accessor and the own value/writable fields and throws a TypeError; for example, a valid pull-request title getter that installs Object.prototype.get makes readCockpitSnapshot throw instead of returning a result. Build this descriptor without a prototype so hostile validation side effects cannot affect descriptor conversion.
Useful? React with 👍 / 👎.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! 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". |
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Purpose
This stacked validation PR repairs exactly one independently verified blocking finding from parent PR #44.
Finding:
D1-44-F2Classification on affected parent HEAD:
CURRENTSeverity:
P2Repair candidate status:
FIXEDProtected parent
Parent PR:
#44
Parent branch:
cockpit/d1-read-model-contractExact affected parent HEAD:
bdb16a488aba9488ca424c1556cc5151c10ddc54This child does not mutate the protected parent directly.
Repair identity
Repair commit:
0a055e39d639c68e418c57b83ba5f3fc44dcd344Repair tree SHA / ArtifactIdentity:
c1477ea1789ab8448da4af1b2f86ee39374e1b50Validated patch SHA-256:
1A74BB5AAC4A508B2831F305EA456BBBBE046708E69CBA4BB71A048BD93C998DPatch size:
9501 bytesVerified defect
A hostile getter executed during
readCockpitSnapshotvalidation can mutate:Object.prototype.toJSONThe original D1 implementation could still accept the snapshot while the returned snapshot graph inherited hostile
toJSONbehavior.Consequences reproduced against the affected parent included:
JSON.stringify(snapshot)throwingThis violated D1's accepted-snapshot plain-JSON round-trip / output-isolation invariant.
Protected invariant
An accepted Cockpit snapshot must remain plain-JSON serializable and round-trip safe even when hostile input mutates
Object.prototypeduring validation or after the snapshot is returned.The repair protects D1's returned snapshot graph only.
It does not claim to protect the entire JavaScript realm.
Exact repair scope
Exactly 2 changed files:
src/cockpit/read-model.tstests/cockpit/read-model-invariants.test.tsDiff:
+171 / -8No other files.
No package changes.
No dependency changes.
No CI changes.
No documentation changes.
No domain changes.
No adapter changes.
No Autoflow changes.
No public API changes.
No exported-symbol changes.
No PR #9 changes.
Repair shape
The repair:
toJSON: undefinedshadow to those lists before freezingArray.prototype -> Object.prototypeto hostile inheritedtoJSONThe outer
{ snapshot, invalidFields }read-result wrapper is intentionally unchanged because the current D1 round-trip contract applies to the acceptedCockpitSnapshot, not the wrapper.Independent validation
Fresh independent validator:
PASS
Finding status on candidate:
FIXEDNew findings:
NONEThe independent validator verified:
toJSONtoJSONtoJSONtoJSONshadow is non-enumerable and inertValidation
Focused D1-44-F2 regressions:
2 passedCockpit invariant suite:
18 passedCockpit suite:
45 passed / 3 filesFull suite:
930 passed / 18 filesTypecheck:
PASSLint:
PASSBuild:
PASSgit diff --check:PASSSeparate non-blocking observation
Claim A — Proxy-shortened list length:
Classification:
CURRENTSeverity:
P3Blocking:
NOIt is intentionally NOT repaired in this PR.
Autoflow / PR #9
Autoflow PR #9 remains independent active work.
This repair does not modify WorkflowState or any PR #9 file.
PR #9 remains untouched.
Authority boundary
This PR must be created as DRAFT.
It is a quarantined validation child.
Do not merge it directly to
main.If this child passes exact-head CI, independent GitHub review, policy/evidence checks, and later Ready/post-Ready gates, human integration must merge it upward into:
cockpit/d1-read-model-contractusing CREATE A MERGE COMMIT.
No Ready authority is granted by PR creation.
No merge authority is granted by PR creation.
Human merge authority is preserved.
Reviewer findings are claims/evidence, not authority.
Every new finding must be verified against exact CURRENT HEAD and formally classified before any repair decision.
Summary by CodeRabbit
Bug Fixes
Tests