Repository navigation
fix(gui): require one-use config write intent for standalone pairing - #6605
Conversation
Bind standalone pairing to a random in-memory verifier and an owner-written digest commitment, consumed before grant creation. Preserve headless CLI delivery and existing pairing/Child safeguards; fence mixed CLI/server contracts at v2. Document path restrictions and cleanup limits, and exercise concurrent consumption and recovery. Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (20)
✨ Finishing Touches🧪 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 |
|
There was a problem hiding this comment.
Readiness triage for exact head b8ee1f323de397d49664fb8685fe600088f3ad8f, targeting dev e53c8077e40b6f540bf1e5d5791a55282466bfac.
Please keep this draft pending completed required CI and a recorded maintainer review of the authentication/configuration-write boundary. The local independent-agent review described in Verification is author-supplied evidence, not a GitHub maintainer approval. @lidge-jun: this needs explicit boundary-review ownership under MAINTAINERS.md. The narrow sponsorship-path matcher does not cover these changed pairing files; hygiene success therefore does not establish that review or waive it.
Cross-platform CI 37248706711 is still in progress at this checkpoint. Its full Windows/macOS test jobs are skipped; the separate keyring/npm-global jobs do not establish native pairing/ACL/handle behavior. Your description correctly leaves native Windows acceptance and installed-service/packaged-client lifecycle unverified. Retain those limitations and resolve the required coverage with the reviewing maintainer before readiness.
The reported candidate/base aggregate failures and isolated timeout recheck do not amount to an aggregate pass. Preserve the named results and the scoped-validation exception rather than replacing them with a blanket tested claim.
This is coordination/readiness triage only, not technical or security approval. I have not executed local tests, pairing/enrollment, new scans, workflow approvals or dispatches, and have not changed your branch.
Update — 2026-10-05: Cross-platform CI 37248706711 has completed successfully on the unchanged head b8ee1f323de397d49664fb8685fe600088f3ad8f. All four Linux shards and selected gates/smokes passed; full Windows/macOS jobs remain skipped. Your follow-up correctly preserves the aggregate/native/lifecycle limitations and pending maintainer boundary-review acceptance. This updates the CI checkpoint only: the required review/coverage decisions remain outstanding, and this is still not an approving review.
|
Addressed the readiness triage in the PR description. The requested maintainer boundary-review owner is lidge-jun; ownership acceptance and a recorded authentication/configuration-write review are pending. The independent local-agent review remains author-supplied evidence. Neither that review, hygiene success, the sponsorship-path matcher nor this response constitutes maintainer security approval. The PR remains Draft until required exact-head CI completes and the reviewing maintainer records the boundary review and the required native/installed/packaged coverage or an explicitly accepted scope exception. Full Windows/macOS test jobs are skipped in the current run. Keyring/npm-global smokes do not establish native pairing, ACL, reparse-point or handle behavior; native Windows pairing and installed-service/packaged-client lifecycle remain unverified. The description preserves the exact candidate/base aggregate results (3,489 pass/106 fail/33 skip/1 error versus 3,431 pass/133 fail/33 skip), the 105 shared failure names, and the isolated recheck's 30 pass/4 timeout failures/4 late errors. It makes no aggregate-pass claim. The 272-test focused pairing/regression run and static/build gates remain scoped evidence. Cross-platform CI 37248706711 is still running on b8ee1f3 at this checkpoint; monitoring continues. No branch or Draft-state change was made for this response. |
There was a problem hiding this comment.
Readiness follow-up for new head cb3867d7a57c89cdb699c9a88502e7a3b7a77a9f, base dev e53c8077e40b6f540bf1e5d5791a55282466bfac.
The new description records actual Windows/NTFS helper-only evidence and a synchronized multi-process regression, while correctly distinguishing helper consumption from multiple delivered GUI grants or a route replay-cache bypass. Those are author-reported results, not executions I repeated. Please qualify the older blanket Windows ACL/handle-unverified paragraph so it does not contradict the newly documented helper coverage; full HTTP/CLI pairing and installed/packaged lifecycle acceptance still remain unverified.
Existing hosted hygiene run 37253012358 failed empty_catch in src/lib/gui-pair-intent.ts and tests/gui/gui-pair-delivery.test.ts. Address the deterministic gate with deliberate error handling; do not clear it by inventing a sponsorship/skip exception. The new Cross-platform CI 37253015110 and React Doctor 37253015116 are pending, and the earlier b8ee1f32 green checks do not cover this follow-up. Keep Draft and the required project-maintainer boundary/coverage acceptance hold. No technical/security patch approval, new scan, repeated test or workflow authorization was performed.
Author-response checkpoint — 2026-10-05 02:50 UTC intake: I read comment 5987248149 and the updated description for successor c4af02a03e30ac7c1a860b88829893a717195820. The older Windows-unverified paragraph is now explicitly historical, and the description adds the isolated Windows source CLI/HTTP result (1 pass, 52 assertions) alongside the 13 helper checks. Those results are author-reported, not executions or boundary validation I repeated. The description reports deliberate cleanup handling and a passing local hygiene evaluator; current hosted Cross-platform CI 37255485385 is still pending and React Doctor 37255485442 queued. The earlier gate failure is historical, not independently certified fixed by this review. The description clarification request is addressed; required maintainer boundary review and installed/packaged acceptance remain outstanding. This review remains COMMENTED at its original cb3867d7 attachment, not a current-head technical/security approval. No scan, repeated test, workflow authorization or merge was performed.
✅ READY
Hygiene✅ Deterministic PR hygiene checks passed. |
|
Addressed the Windows follow-up review in The empty cleanup catches now have explicit failure/control-flow handling. The unchanged local hygiene evaluator reports no failures; no sponsorship/skip exception or gate weakening was used. Actual Windows/NTFS validation covers 13 helper checks, including one winner among eight simultaneous consumers, plus the real isolated source CLI/HTTP pairing test (1 pass, 52 assertions). The latter exercises grant creation/redemption and replay/origin/credential/CSRF refusals, stopping before SSH and leaving installed services/accounts untouched. Final focused cleanup regressions and typecheck also pass. The description records exact source hashes, commands and limits. New-head CI is pending; the prior-head hygiene failure remains historical until the queued checks update. Draft and the required maintainer boundary/coverage acceptance hold remain. Source-level Windows tests do not establish packaged-client or installed-service lifecycle acceptance. |
…e budget after the dev refresh
… to keep gui-and-management-api.md within budget
On Windows, consuming a standalone pairing intent now requires that the intent directory and the record are owned by the serving account (or by Administrators when that is the token's default owner and the role is enabled) and carry a protected DACL with exactly one ordinary Full Control allow entry for the serving account. The check reads the raw security descriptor through the trusted PowerShell executable with a fixed encoded script and environment-only paths, runs only after the record matched its proof and under the consume lock, then re-checks directory and record identity before unlinking. Anything that cannot be verified refuses; redemption never repairs permissions or ownership. Publication keeps its required hardening. Regression cases live in the existing gui-pair-delivery test file; structure and remote-link guides document the rule, the refusal, and the remaining limits.
…and keep verifier output ASCII Get-Acl returns a .NET-canonicalized descriptor (same-principal entries merge, entries that grant nothing drop; another principal's entry and the owner are never changed), so the structure doc, guides, comments and test names now state the effective-DACL policy instead of promising raw on-disk bytes. The native regression also adds a container inherit-only entry for another principal, which must refuse. The verifier script switches console output to ASCII before writing, so a UTF-8 console code page cannot prefix the line protocol with a byte-order mark. The native test first runs the real verifier on a clean intent and reports its protocol output if it is refused.
… account's Full Control Native CI showed that a hardened intent directory on a Windows runner carries explicit, protected Full Control entries for SYSTEM and BUILTIN\Administrators next to the serving account's entry, which the one-entry rule refused. The verifier now requires the serving account's own Full Control allow entry and also accepts one plain allow entry each for SYSTEM and Administrators. LocalSystem and an elevated administrator can already take ownership of any object, and a filtered administrator token holds Administrators only as deny-only, so those entries add no authority. Any other grantee, duplicate, deny, callback or inherited entry still refuses, and ownership checks are unchanged.
|
Maintainer decision and security review record for head Decision. The boundary review of this PR raised a question about how Windows redemption establishes write authority. The maintainers chose to require fail-closed owner and ACL verification at redemption on Windows. Maintainers pushed that change to this branch (maintainer edits are enabled):
Regression cases live in Independent security review (gpt-6.1-sol): PASS at CI at this head.
No local tests were run for these maintainer commits, per the maintainers' instruction for this round; the evidence above is hosted CI. @luvs01: please review the maintainer commits. The PR stays in draft until you mark it ready. |
|
Maintainer integration (lidge-jun, dev only) — 2026-10-05 bug triage round 2, lane B6.
|
Summary
Standalone
POST /api/gui/pairing-grantscurrently accepts a process-bound HMAC derived from readable runtime state. A process with the runtime secret and local HTTP access can therefore mint a code and redeem a paired GUI session without having write authority over the configuration directory. That session can satisfy the existing pairing prerequisite for Child enrollment. This change adds the missing configuration-write predicate at the grant-delivery boundary.ocx gui pair --origin <origin>creates a random 256-bit verifier. It exclusively publishes only a SHA-256 commitment beneath the service's configuration home, then sends the verifier with the existing signed request. The verifier remains in CLI memory.v2, so mixed-version CLI/server pairing is refused for both standalone and Hub commands; update and restart them together.Existing grant redemption, CSRF, expiry, role-change invalidation, listener/peer checks and the pre-SSH paired-join guard remain in place. This PR only adds the missing write-intent protection and its validation; it does not replace those safeguards.
Threat model: this separates runtime-secret read access plus local HTTP access from configuration-directory write authority. It is neither proof of direct human consent nor protection against unrestricted same-user malware with write access to that directory. It is defense in depth for split-authority local deployments.
Verification
Current cleanup/hygiene follow-up (
c4af02a)Published head:
c4af02a03e30ac7c1a860b88829893a717195820, tree0d8075ce5a0b9ff62d2b7aa693f485e7698e3710; parentcb3867d7a57c89cdb699c9a88502e7a3b7a77a9f. The integration base remainse53c8077e40b6f540bf1e5d5791a55282466bfac.The preceding head's hygiene and target gates rejected two empty cleanup catches (
empty_catch). Lock release now has an explicit failure result without overriding the consume result; child-stdin cleanup explicitly continues to the remaining children and still awaits all exits. No policy waiver or check suppression was added.assessHygieneevaluator reports no failures for all 16 PR-relative patches.84230eed16ac1ce28ff6eb1b0a700dde6be85856; no live provider or installed-service state was used.bun test tests/gui/gui-pair-delivery.test.ts --test-name-pattern 'separate processes|proof refusal' --timeout 30000: 2 pass / 0 fail / 26 filtered, 16 assertions.bun run typecheckandgit diff --check: pass.bun test --isolate tests/gui/gui-pair-http.test.ts1 pass / 0 fail, 52 assertions. The real source CLI starts an isolated loopback server, runtime-secret-only and fabricated-intent requests are refused, the CLI obtains and redeems one grant, replay/origin/credential/CSRF controls refuse as expected, and enrollment stops before SSH without durable links. Native ACL/principal behavior is not mocked in this subprocess test. The existing installed app/service, real accounts and providers remain untouched; this is not packaged-client or full SSH lifecycle acceptance.prepack/npm packpath, and installed the resulting 2.78.0 tarball into a non-global, disposable prefix with its own HOME/config/cache. Tarball SHA-256:f2ed02860ef92f3cd0080e9ec9d732d3a6898f229d266b422f52808bf6600d04(13,749,321 bytes; 1,892 packaged files). Eight critical source/launcher/generated-manifest/GUI files matched the checkout hashes. The repository's 52-assertion HTTP fixture was retargeted only to the installed package and relocated helper imports: server via the package's Bun 1.4.0 and packaged CLI source, pairing client via Node v24.18.0 and the packagedbin/ocx.mjslauncher. 1 pass / 0 fail, 52 assertions. The harness ran with Bun 1.4.2 from the repository cwd. This verifies the npm CLI/runtime path, not a Tauri desktop package, global command-shim discovery, Windows service lifecycle, other-user access or full SSH enrollment. No real installation or account was modified; npm's script-approval configuration was not changed.Previous Windows consumption follow-up (
cb3867d)Published head:
cb3867d7a57c89cdb699c9a88502e7a3b7a77a9f, tree5f93a142412d88d4238f5b59991ab24f5ea5be4d; parentb8ee1f323de397d49664fb8685fe600088f3ad8f. The integration base remainse53c8077e40b6f540bf1e5d5791a55282466bfac.The original helper's eight-process consumption check was strengthened with a readiness barrier. On actual Windows/NTFS with Bun 1.4.2, the published original helper returned success to four consumers, then two in a second reproduction. These were helper-only synthetic tests, not proof of multiple GUI grants or a bypass of the route's capability replay cache. Consumption now acquires an atomic per-capability directory lock before opening the record and only releases its own identity-matching empty lock. The synchronized repository regression uses real Windows ACL runners.
6a73079e71c104ed9a4fe075eb762c0ec1fdb293. With no platform/principal/ACL/runner injection, tests cover cold publication (623 ms), actual protected DACLs and absence of broad allow entries, NTFS identity, replay/recreation, corrupted/oversized records, hard links, junctions, wrong-proof recovery, existing lock preservation and eight simultaneous consumers with exactly one success. No real account, provider, installed service or credential store was used.bun test --isolate tests/gui/gui-pair-delivery.test.ts tests/gui/gui-pair-client.test.ts: 44 pass / 4 POSIX-only skips / 0 fail, 190 assertions. After making the new test accumulate readiness bytes and reap partially started child groups, only the affectedseparate processestest was rerun: 1 pass / 27 filtered / 0 fail, 12 assertions. The production module did not change after these checks.bun run typecheck,bun run structure:checkandgit diff --check: pass. The initial structure check caught a 602/600 line-budget overflow; the same contract section was condensed to 600 lines and the check then passed. No budget was raised.bun run privacy:scanpassed on the sparse checkout containing all five changed files. The whole-repository file-size command could not read omitted.coderabbit.yaml; it is not a passing full scan. Changed-file evaluation using the unmodified official ratchet's baseline/evaluator passed for all five paths. Full repository/privacy/ratchet and documentation-build coverage for this follow-up is left to the new hosted run; no scanner or guard was weakened.Earlier Linux verification (
b8ee1f32)Base: current
deve53c8077e40b6f540bf1e5d5791a55282466bfac.Candidate:
b8ee1f323de397d49664fb8685fe600088f3ad8f(tree22760b621f76b2ba8cf0fd1bc5d1e20ec916e5d4).Local execution used repository-pinned Bun 1.4.0 on Linux, temporary test homes and the repository test wrapper. Inherited HTTP proxy variables were cleared for network tests; live installation credentials were excluded by the wrapper.
devreceived201instead of the required403. With this patch it receives403andlocal_pairing_intent_required; the real source CLI, with ignored stdin and a server without terminal output, successfully publishes intent, receives a code and redeems it. Replay, foreign-origin, alternate-credential, CSRF and pre-SSH join controls remain covered.bun run typecheck,bun run lint:gui:if-changed(full GUI lint),bun run privacy:scan,bun run structure:check,bun run skill:surface:check, andgit diff --check upstream/dev HEADpassed.cd docs-site && bun install --frozen-lockfile && bun run buildpassed: 561 pages and 78,088 internal links checked. Build tooling used writable temporary/cache/config directories with Astro telemetry disabled.Focused command (with the inherited proxy variables removed):
bun run test --parallel=1 \ tests/gui/gui-pair-delivery.test.ts tests/gui/gui-pair-client.test.ts \ tests/gui/gui-pair-capability.test.ts tests/gui/gui-pair-http.test.ts \ tests/server/link-join-route.test.ts tests/server/link-management-routes.test.ts \ tests/server/server-management-auth.test.ts tests/server/management-route-registry.test.ts \ tests/test-layout.test.ts tests/test-layout-tooling.test.ts \ tests/ci-workflows/file-size-ratchet.test.ts tests/cli/hub-invite.test.ts \ tests/server/server-live.test.tsMaintainer boundary review and Draft retention
Requested maintainer boundary-review owner: @lidge-jun, under MAINTAINERS.md. Ownership acceptance and a recorded review of the authentication/configuration-write boundary are pending. The independent local-agent review is author-supplied evidence and supplies no GitHub maintainer approval. Ingwannu's readiness triage is coordination only; no technical or security approval is claimed.
Hygiene success and the narrow sponsorship-path matcher do not establish or waive boundary review for these pairing files. This PR remains Draft until required exact-head CI completes and the reviewing maintainer records the boundary review and required native/installed/packaged coverage or an explicit accepted scope exception. At that earlier Linux checkpoint, native Windows pairing/ACL/handle behavior had not been verified. The current Windows helper and isolated source CLI/HTTP checks above add evidence for that scope; installed-service/packaged-client lifecycle and maintainer acceptance remain outstanding. Full Windows/macOS test jobs in that earlier hosted run were skipped; separate keyring/npm-global smokes do not validate those pairing contracts.
Exact-head hosted checks completed successfully: Cross-platform CI 37248706711 passed, including all four Linux test shards and the aggregate
cijob; React Doctor 37248706725 also passed. Gates, API usage, storage policy, docs, Docker and keyring/npm-global smokes completed successfully. Full Windows/macOS test jobs were skipped. These results belong tob8ee1f323de397d49664fb8685fe600088f3ad8f; they do not erase the failing local aggregate record below or supply maintainer approval. The PR remains Draft pending the recorded maintainer boundary review and required native/installed/packaged coverage disposition.Aggregate validation remains failing
bun run test:changedselected the actual upstream merge base and 170 files: 3,489 pass, 33 skip, 106 fail, 1 error (3,628 tests). The identical existing-file selection on unchanged upstream, with the same wrapper, Bun and parallelism, produced 3,431 pass, 33 skip, 133 fail (3,597 tests across 169 files; the new intent file is absent).105 of the candidate's 106 failed test names also failed in the original unchanged-upstream aggregate. The remaining case is investigated below; all 106 original failures are not claimed as baseline failures. Its earlier isolated full-file recheck passed that case but still had 30 pass, 4 timeout failures and 4 late errors. The focused pairing checks and static/build gates passed, but the local aggregate remains failing and no passing local aggregate or full local repository-suite result is claimed. Hosted results are recorded separately above. Maintainer boundary review and coverage acceptance remain readiness gates.
Investigation of the unmatched Responses failure
Exact test:
plaintext v2 agent messages at the Responses server boundary > fails closed when the headerless prefix probe reads a failed body, intests/server/plaintext-v2-agent-messages-server.test.ts:329. The candidate aggregate timed out at 5,025.47 ms against a 5,000 ms deadline. Its outstanding request later failed line 342,expect(response.status).toBe(502), with received 200. The original upstream aggregate passed this case at 1,040.46 ms.A bounded follow-up compared this exact case on the unchanged base and candidate, using the same Bun, wrapper, fresh temporary homes and cleared proxy variables:
bun run test --parallel=1 tests/server/plaintext-v2-agent-messages-server.test.ts -t 'fails closed when the headerless prefix probe reads a failed body'.upstreamWebsocket: falsemade both original assertions pass under the original deadline, in 67.03 ms on the base and 64.44 ms on the candidate. Neither trace contained a connect call.The existing fixture overrides
globalThis.fetchbut leaves the canonical backend's streaming WebSocket transport enabled on Bun 1.4.0. The real WebSocket dial precedes the mocked HTTP fallback. After a timeout, the fixture releases its lease and replaces global fetch without cancelling/awaiting the old handler; that handler can later read a following case's mock. The test, helper and inspected Responses/transport implementation files are unchanged by this patch.Assessment: no concrete introduced regression was found; evidence supports an existing network-sensitive fixture and timeout/mock-teardown race. No unrelated runtime change or permanent test workaround was added. The historical aggregate lacks per-dial phase traces, so its exact DNS/upgrade timing and the specific later mock supplying its 200 cannot be proved retrospectively. This investigation neither recovers the local aggregate nor explains every other local failure. Required maintainer boundary review and native/installed/packaged coverage holds remain in place.
Limitations and recovery
OPENCODEX_HOME, under its owning account. CLI and server must use the same home and operation contract..consuminglock. It refuses only that capability; a fresh command obtains a different capability. Online cleanup does not reclaim locks merely by age, since a delayed owner may still be active. Do not manually remove pairing state while commands or the service are running.Checklist
Co-authored-by: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>