fix(browser): make ref uploads request-owned#105151
Conversation
![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: fb03f04d39
ℹ️ 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".
| inputRef, | ||
| element, | ||
| paths: resolvedPaths, | ||
| ssrfPolicy: ctx.state().resolved.ssrfPolicy, |
There was a problem hiding this comment.
When an OpenClaw-managed profile is launched with --proxy-server while strict SSRF is in effect, browserNavigationPolicyForProfile adds browserProxyMode: "explicit-browser-proxy" so assertBrowserNavigationAllowed fails closed; this hook now wraps setInputFiles/ref uploads in navigation guards but passes only ssrfPolicy. An upload from an otherwise allowed page that auto-submits or navigates, for example to a public IP-literal URL, is therefore checked as direct browser traffic and can bypass the explicit-proxy block. Please pass the full browserNavigationPolicyForProfile(ctx, profileCtx) into both guarded upload calls.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
proof: sufficient
|
Codex review: found issues before merge. Reviewed July 12, 2026, 4:41 AM ET / 08:41 UTC. Summary PR surface: Source +261, Tests +439. Total +700 across 9 files. Reproducibility: yes. from source with high confidence: use a managed profile with an explicit browser proxy under strict SSRF policy and trigger navigation during direct-input or ref-based file assignment; the route drops Review metrics: none identified. Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review findings
Review detailsBest possible solution: Preserve the request-owned upload lifecycle, but pass Do we have a high-confidence way to reproduce the issue? Yes, from source with high confidence: use a managed profile with an explicit browser proxy under strict SSRF policy and trigger navigation during direct-input or ref-based file assignment; the route drops Is this the best way to solve the issue? No on the exact head. Request-owned chooser handling is the appropriate boundary, but the route must preserve the complete existing profile navigation policy rather than constructing a partial option bag. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 68553bed8437. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +261, Tests +439. Total +700 across 9 files. View PR surface stats
Security concerns:
What I checked:
Likely related people:
What the crustacean ranks mean
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. How this review workflow works
|
|
Maintainer verification on exact head
This closes the chooser race without removing any upload mode or forcing users onto a less capable path. |
|
Merged via squash.
|
* fix(browser): make ref uploads request-owned * style(browser): satisfy upload owner const lint
1 participant
Closes #38844
What Problem This Solves
Managed Browser
uploadrequests that supplied bothpathsand a triggerrefreturned before Playwright captured the chooser and assigned the files. Failures were detached and suppressed, and a stale waiter could consume a later unrelated chooser.Why This Change Was Made
The ref form now has one request-owned Playwright lifecycle: resolve one page, attach one chooser listener, guarded click, strict use-time path revalidation, guarded file assignment, and deterministic cleanup under one deadline and request signal. Ownership is connection-scoped because abort cleanup disconnects the shared Playwright connection.
The paths-only delayed-arm API remains detached. Direct
inputRefandelementuploads, Chrome MCP routing, and managed upload-root validation remain supported.User Impact
paths + refwaits until exactly one chooser receives the validated files or returns the specific failure.Evidence
uploadwith both paths and the current ref, the page observedselected:report.json, the agent returned the required pass token, and the Gateway remained alive.git diff --check: passed.Exact-head CI is pending. The final live proof needs a fresh credential because the prior OpenAI service-account key was exposed in an internal process-inspection transcript and will not be reused.