Skip to content

fix(chrome): isolate and clean up compatibility preloads - #386

Merged
steipete merged 1 commit into
openclaw:mainfrom
SebTardif:fix/chrome-compat-unique-tmp-patch
Sep 22, 2026
Merged

fix(chrome): isolate and clean up compatibility preloads#386
steipete merged 1 commit into
openclaw:mainfrom
SebTardif:fix/chrome-compat-unique-tmp-patch

Conversation

@SebTardif

@SebTardif SebTardif commented Sep 20, 2026 •
edited by steipete
Loading

Copy link
Copy Markdown
Contributor

The Chrome compatibility fallback wrote JavaScript to a predictable shared-temp filename before loading it through NODE_OPTIONS. A pre-created symlink could overwrite an unrelated file or substitute code.

Create each fallback exclusively in a unique private directory: mode 0700/0600 on POSIX, and a verified current-user-only ACL on Windows. Carry cleanup ownership with the compatibility result so transport shutdown and failed setup remove the temporary directory. Packaged preload files remain caller-independent and are never deleted.

Validation: the synthetic symlink regression overwrites its victim before the fix and preserves it afterward. Tests verify permissions, loading the generated module, transport cleanup wiring, and removal after write failure. AWS Crabbox pnpm check and the full suite passed (2,020 tests; 27 skipped); independent P0–P2 review is clean. No real Chrome attachment was made. Contributor credit and changelog thanks are preserved for @SebTardif.

@clawsweeper

clawsweeper Bot commented Sep 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

ClawSweeper review complete

ClawSweeper finished reviewing this revision. The review result is being finalized.

View the workflow run.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 20, 2026
@clawsweeper

clawsweeper Bot commented Sep 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 22, 2026, 2:02 AM ET / 06:02 UTC (Revision 2).

ClawSweeper review

What this changes

The PR creates Chrome compatibility scripts in private temporary directories and removes them when transport setup fails or the transport closes.

Merge readiness

⛔ Blocked before merge - 2 items remain

This remains a useful, focused security fix: current main and v0.13.13 retain the vulnerable fallback write. No introduced correctness defect was found, but the earlier runtime evidence covers a narrower revision.

Priority: P2
Reviewed head: 2b68e3fd1f45cc7851bc7bd4a6645f0c6a2c8aa8

Review scores

Measure Result What it means
Overall readiness 🦐 gold shrimp (3/6) The implementation is focused and has no identified blocking defect, but current behavior proof does not cover the expanded lifecycle.
Proof confidence 🦐 gold shrimp (3/6) Needs stronger real behavior proof before merge: Earlier Windows resolver evidence establishes unique allocation, but this revision adds Windows ACL creation and transport-owned cleanup. The current body reports tests rather than an observed production-path run covering those additions; a redacted filesystem/transport transcript is sufficient, without requiring a real Chrome attachment. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs stronger real behavior proof before merge: Earlier Windows resolver evidence establishes unique allocation, but this revision adds Windows ACL creation and transport-owned cleanup. The current body reports tests rather than an observed production-path run covering those additions; a redacted filesystem/transport transcript is sufficient, without requiring a real Chrome attachment. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 7 items Pinned introduced change: The verified base-to-head delta changes four files: the fallback writer, transport cleanup wiring, regression tests, and the unreleased changelog. It adds no dependencies or package execution hooks.
Current main still needs the repair: Without packaged candidates, current main writes rendered JavaScript to a predictable filename directly beneath the temporary directory, without exclusive creation. A pre-existing symlink is therefore followed before that path is supplied to Node.
Latest release comparison: The supplied latest release, v0.13.13, contains the same predictable, non-exclusive fallback write.
Findings None None.
Security None None.

How this fits together

MCPorter launches Chrome DevTools MCP servers with a compatibility script imported through Node’s startup options. When the packaged script is missing, this subsystem creates the fallback file and transfers its cleanup ownership to the server transport.

flowchart TD
  A[Chrome server command] --> B[Compatibility eligibility]
  B --> C{Packaged script available?}
  C -->|Yes| D[Packaged script]
  C -->|No| E[Private temporary script]
  D --> F[Node startup import]
  E --> F
  F --> G[Server transport]
  G --> H[Remove owned temporary directory]
Loading

Before merge

  • Add real behavior proof - Needs stronger real behavior proof before merge: Earlier Windows resolver evidence establishes unique allocation, but this revision adds Windows ACL creation and transport-owned cleanup. The current body reports tests rather than an observed production-path run covering those additions; a redacted filesystem/transport transcript is sufficient, without requiring a real Chrome attachment. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Complete next step (P2) - Add real behavior proof for the current fallback and cleanup lifecycle. Terminal output or logs are acceptable; screenshots or recordings are preferred when they show the result clearly. Redact credentials, private paths, addresses, and endpoints. Updating the PR body should trigger review automatically; otherwise ask a maintainer to comment @clawsweeper re-review.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Production and test delta Production +46/-21; tests +42/-4 Production growth supplies private allocation and lifecycle ownership, with focused filesystem regression coverage.

Technical review

Best possible solution:

Keep private, exclusive fallback creation coupled to transport ownership, while preserving packaged-script preference and existing launch options.

Do we have a high-confidence way to reproduce the issue?

Yes: current main writes through a pre-created symlink at the predictable fallback filename when packaged candidates are absent; the added regression exercises that mechanism. This review did not execute it.

Is this the best way to solve the issue?

Yes: private allocation, exclusive creation, and ownership-based cleanup narrowly repair the existing fallback without changing its generated script or adding configuration.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning medium; reviewed against b67907939bd7.

Labels

Label changes:

  • add rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Earlier Windows resolver evidence establishes unique allocation, but this revision adds Windows ACL creation and transport-owned cleanup. The current body reports tests rather than an observed production-path run covering those additions; a redacted filesystem/transport transcript is sufficient, without requiring a real Chrome attachment. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • remove proof: sufficient: Current real behavior proof status is insufficient, not sufficient.
  • remove rating: 🐚 platinum hermit: Current PR rating is rating: 🦐 gold shrimp, so this older rating label is no longer current.
  • remove status: 👀 ready for maintainer look: Current PR status label is status: 📣 needs proof.

Label justifications:

  • P2: This addresses a concrete executable-file vulnerability in the limited fallback path used when packaged build output is unavailable.
  • rating: 🦐 gold shrimp: Overall readiness is 🦐 gold shrimp; proof is 🦐 gold shrimp and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs stronger real behavior proof before merge: Earlier Windows resolver evidence establishes unique allocation, but this revision adds Windows ACL creation and transport-owned cleanup. The current body reports tests rather than an observed production-path run covering those additions; a redacted filesystem/transport transcript is sufficient, without requiring a real Chrome attachment. No stored-data contract changes. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Pinned introduced change: The verified base-to-head delta changes four files: the fallback writer, transport cleanup wiring, regression tests, and the unreleased changelog. It adds no dependencies or package execution hooks. (src/chrome-devtools-compat.ts:81, 2b68e3fd1f45)
  • Current main still needs the repair: Without packaged candidates, current main writes rendered JavaScript to a predictable filename directly beneath the temporary directory, without exclusive creation. A pre-existing symlink is therefore followed before that path is supplied to Node. (src/chrome-devtools-compat.ts:74, b67907939bd7)
  • Latest release comparison: The supplied latest release, v0.13.13, contains the same predictable, non-exclusive fallback write. (src/chrome-devtools-compat.ts:74, e4cb002200e4)
  • Security and lifecycle inspection: The fallback uses exclusive file creation inside a unique directory. Windows reuses the existing helper that creates and verifies a current-user-only ACL; POSIX uses mkdtemp. Transport cleanup invokes the owned close callback in finally, including failed construction, while packaged files have no deletion callback. No persisted data model or public package export changes. (src/runtime/transport.ts:226, 2b68e3fd1f45)
  • Earlier proof has narrower coverage: The previous completed review records a Windows 11 production-resolver transcript showing distinct temporary directories at b9ab9a2. Reading that revision through GitHub confirms it had neither the Windows ACL helper nor cleanup ownership. Its positive allocation evidence is retained, but it does not demonstrate those newly added behaviors. (src/chrome-devtools-compat.ts:73, b9ab9a2a3fa5)
  • Current validation evidence: The complete supplied body, captured under sourceRevision 4ec098d789d56a38689c2c6218068cd3307b4cb03910b202b7b39b5f117f4e1c, reports a synthetic before/after symlink regression, permission/import/cleanup tests, and successful pnpm check and 2,020 tests. These are useful supplemental checks, but no current-revision production lifecycle transcript is supplied. Tests were inspected rather than executed because this review must remain read-only. (tests/chrome-devtools-compat.test.ts:80, 2b68e3fd1f45)

Likely related people:

  • unknown: The claimed source-line change could not be verified from bounded local history. (role: source history unknown; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add current-revision production-path output showing private fallback creation, successful Node loading, and removal after transport shutdown or failed setup, including the new Windows ACL path.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

History

Review history (1 earlier review cycle)
  • reviewed 2026-09-20T20:14:06.041Z sha b9ab9a2 :: needs maintainer review before merge. :: none

@steipete
steipete force-pushed the fix/chrome-compat-unique-tmp-patch branch from b9ab9a2 to 2b68e3f Compare September 22, 2026 05:57
@steipete steipete changed the title fix(chrome): write auto-connect fallback patch in a unique temp dir fix(chrome): isolate and clean up compatibility preloads Sep 22, 2026
@clawsweeper clawsweeper Bot added rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. and removed proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Sep 22, 2026
@steipete
steipete merged commit 139b2e5 into openclaw:main Sep 22, 2026
9 of 10 checks passed
@seathatflowsinourveins seathatflowsinourveins mentioned this pull request Sep 26, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

P2 Normal priority bug or improvement with limited blast radius. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@SebTardif @steipete