Skip to content

fix(browser): reject credentialed navigation URLs#82490

Closed
lidge-jun wants to merge 1 commit into
openclaw:mainfrom
lidge-jun:codex/55365-browser-url-userinfo
Closed

fix(browser): reject credentialed navigation URLs#82490
lidge-jun wants to merge 1 commit into
openclaw:mainfrom
lidge-jun:codex/55365-browser-url-userinfo

Conversation

@lidge-jun

@lidge-jun lidge-jun commented May 16, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Reject HTTP(S) browser navigation URLs that contain embedded username/password credentials before the browser tool sends them to a local browser client or remote browser proxy.
  • Enforce the same credentialed-URL block in the browser navigation guard before Playwright/Chrome MCP navigation reaches Chromium.
  • Redact credential-like userinfo from invalid URL diagnostics so error text does not echo secrets.

Closes #55365

Real behavior proof (required for external PRs)

Behavior addressed: browser open/navigate requests no longer pass HTTP(S) URLs with embedded credentials such as https://user:[email protected]/path through to the browser proxy, local browser client, Playwright, or Chromium. Invalid URL diagnostics also redact credential-like userinfo instead of echoing the raw secret-bearing input.

Real environment tested: macOS local workstation, Node v24.14.1, pnpm v11.1.0, OpenClaw built from source at PR head 28c0451.

Exact steps or command run after this patch:
node scripts/run-vitest.mjs extensions/browser/src/browser/navigation-guard.test.ts extensions/browser/src/browser-tool.test.ts extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts
node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.extensions.json extensions/browser/src/browser/navigation-url-userinfo.ts extensions/browser/src/browser/navigation-guard.ts extensions/browser/src/browser/navigation-guard.test.ts extensions/browser/src/browser-tool.ts extensions/browser/src/browser-tool.test.ts
git diff --check origin/main..HEAD

Evidence after fix:
$ node scripts/run-vitest.mjs extensions/browser/src/browser/navigation-guard.test.ts extensions/browser/src/browser-tool.test.ts extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts
Test Files 3 passed (3)
Tests 84 passed (84)

$ node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.extensions.json extensions/browser/src/browser/navigation-url-userinfo.ts extensions/browser/src/browser/navigation-guard.ts extensions/browser/src/browser/navigation-guard.test.ts extensions/browser/src/browser-tool.ts extensions/browser/src/browser-tool.test.ts
Found 0 warnings and 0 errors.
Finished in 3.0s on 5 files with 216 rules using 1 threads.

$ git diff --check origin/main..HEAD
(no output)

Observed result after fix: the focused tests prove credentialed network navigation URLs are rejected before DNS lookup, before direct browser open/navigate calls, and before gateway proxy dispatch. The same test set also proves invalid URL errors redact https://user:secret@ as https://[redacted]@ and do not include the secret.

What was not tested: live navigation against a real Chromium instance and remote browser-node proxy transport on Linux/Windows.

Verification

  • node scripts/run-vitest.mjs extensions/browser/src/browser/navigation-guard.test.ts extensions/browser/src/browser-tool.test.ts extensions/browser/src/browser/pw-tools-core.snapshot.navigate-guard.test.ts
  • node scripts/run-oxlint.mjs --tsconfig config/tsconfig/oxlint.extensions.json extensions/browser/src/browser/navigation-url-userinfo.ts extensions/browser/src/browser/navigation-guard.ts extensions/browser/src/browser/navigation-guard.test.ts extensions/browser/src/browser-tool.ts extensions/browser/src/browser-tool.test.ts
  • git diff --check origin/main..HEAD

@openclaw-barnacle openclaw-barnacle Bot added size: S proof: supplied External PR includes structured after-fix real behavior proof. labels May 16, 2026
@clawsweeper

clawsweeper Bot commented May 16, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed June 30, 2026, 9:24 PM ET / 01:24 UTC.

Summary
The PR adds browser plugin validation and tests to reject HTTP(S) navigation URLs with embedded username/password credentials and redact credential-like invalid URL diagnostics.

PR surface: Source +49, Tests +57. Total +106 across 5 files.

Reproducibility: yes. source-level. Current main forwards browser open/navigate URL input toward local or node-proxy dispatch without URL-userinfo rejection, and the PR helper can be shown from the diff to let malformed credential-like strings fall through before dispatch.

Review metrics: 1 noteworthy metric.

  • Navigation Input Contract: 1 URL authority form changed to fail-fast. HTTP(S) browser navigation URLs with username/password userinfo currently pass through and would become immediate errors after merge.

Root-cause cluster
Relationship: fixed_by_candidate
Canonical: #55365
Summary: The linked browser credentialed-navigation issue is the canonical report, and this PR is its open candidate fix; adjacent credential-URL items cover different surfaces.

Members:

Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🦪 silver shellfish
Patch quality: 🧂 unranked krab
Result: blocked until real behavior proof from a real setup is added.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • [P1] Reject malformed credential-like HTTP(S) strings before local or node-proxy dispatch.
  • [P1] Add redacted live Chromium or browser-proxy proof for credentialed open and navigate failures with secrets omitted.
  • Get maintainer acceptance that URL-embedded credentials are unsupported for browser page navigation.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body contains focused test/lint/diff-check output and explicitly says live Chromium and browser-node proxy transport were not tested, so redacted live output, logs, terminal screenshots, screenshots, recordings, or linked artifacts from the real open/navigate path are still needed before merge; after updating the PR body, ClawSweeper should re-review automatically or a maintainer can comment @clawsweeper re-review.

Mantis proof suggestion
A real browser/proxy proof run would materially help verify the fail-fast navigation behavior and redacted error output. A maintainer can ask Mantis to capture proof by posting this exact PR comment:

@openclaw-mantis visual task: verify browser open and navigate reject credentialed HTTP(S) URLs before navigation and redact secrets.

Risk before merge

  • [P1] Malformed credential-like HTTP(S) inputs such as https://user:secret@ can still be serialized into local or remote browser open/navigate request bodies before the server guard redacts the later invalid-URL error.
  • [P1] The PR deliberately changes valid HTTP(S) URLs with userinfo from pass-through to immediate browser navigation errors, so upgraded users relying on URL-embedded Basic auth need maintainer-approved policy or operator guidance.
  • [P1] The supplied evidence is focused Vitest, oxlint, and diff-check output only; it does not show a redacted live Chromium or browser-node proxy run of the changed user path.

Maintainer options:

  1. Fix malformed pre-dispatch rejection and prove it live (recommended)
    Update the helper and tests so malformed credential-like HTTP(S) strings fail before client/proxy dispatch, then add redacted live Chromium or browser-proxy proof for open and navigate.
  2. Accept URL-userinfo fail-fast policy
    Maintainers can explicitly accept that browser page navigation no longer supports URL-embedded credentials once the implementation and proof gaps are resolved.
  3. Pause for Basic-auth compatibility
    If URL-embedded Basic auth should remain supported, pause this PR and choose a documented compatibility path such as authenticated profiles, cookies, headers, or an explicit opt-in.

Next step before merge

  • [P1] Human follow-up is needed because the branch needs contributor real behavior proof, maintainer acceptance of the fail-fast URL-userinfo policy, and a small pre-dispatch fix.

Security
Needs attention: The diff reduces credential exposure for valid userinfo URLs, but malformed credential-like strings can still cross the browser client/proxy boundary before redaction.

Review findings

  • [P2] Reject malformed credential URLs before dispatch — extensions/browser/src/browser/navigation-url-userinfo.ts:18-22
Review details

Best possible solution:

Reject and redact both parsed and malformed credential-like HTTP(S) browser navigation inputs before any local/proxy/Chromium dispatch, then merge only after redacted live browser/proxy proof and maintainer acceptance of the fail-fast policy.

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

Yes, source-level. Current main forwards browser open/navigate URL input toward local or node-proxy dispatch without URL-userinfo rejection, and the PR helper can be shown from the diff to let malformed credential-like strings fall through before dispatch.

Is this the best way to solve the issue?

No, not yet. The browser plugin input/guard path is the right boundary, but the helper must reject malformed credential-like inputs before dispatch and the PR still needs live proof plus maintainer acceptance of the compatibility policy.

Full review comments:

  • [P2] Reject malformed credential URLs before dispatch — extensions/browser/src/browser/navigation-url-userinfo.ts:18-22
    assertNoBrowserNavigationUrlUserInfo swallows URL parse failures, so https://user:secret@ can still return from readTargetUrlParam and be serialized into /tabs/open or /navigate request bodies before the server guard redacts the later invalid URL error. Please reject credential-like HTTP(S) userinfo strings in the parse-failure path too.
    Confidence: 0.86

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against b3b51b0c9185.

Label changes

Label justifications:

  • P2: This is bounded browser security and UX hardening with limited blast radius, but the patch still has a concrete merge-blocking implementation gap.
  • merge-risk: 🚨 compatibility: The patch deliberately turns an existing HTTP(S) URL form with embedded credentials into a browser navigation error for upgraded users.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🦪 silver shellfish and patch quality is 🧂 unranked krab.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body contains focused test/lint/diff-check output and explicitly says live Chromium and browser-node proxy transport were not tested, so redacted live output, logs, terminal screenshots, screenshots, recordings, or linked artifacts from the real open/navigate path are still needed before merge; after updating the PR body, ClawSweeper should re-review automatically or a maintainer can comment @clawsweeper re-review.
Evidence reviewed

PR surface:

Source +49, Tests +57. Total +106 across 5 files.

View PR surface stats
Area Files Added Removed Net
Source 3 53 4 +49
Tests 2 57 0 +57
Docs 0 0 0 0
Config 0 0 0 0
Generated 0 0 0 0
Other 0 0 0 0
Total 5 110 4 +106

Security concerns:

  • [medium] Malformed userinfo can still cross the proxy boundary — extensions/browser/src/browser/navigation-url-userinfo.ts:18
    Because the pre-dispatch helper swallows parse failures, malformed credential-like HTTP(S) inputs can still be sent as raw browser open/navigate request bodies before the navigation guard redacts the surfaced error.
    Confidence: 0.86

What I checked:

Likely related people:

  • steipete: File history shows repeated browser tool, navigation guard, SSRF, and bundled browser plugin ownership refactors on the affected files. (role: browser plugin history contributor; confidence: high; commits: 55aaeb5085a0, 8eeb7f082975, 197510f69302; files: extensions/browser/src/browser-tool.ts, extensions/browser/src/browser/navigation-guard.ts, extensions/browser/src/browser/routes/tabs.ts)
  • vincentkoc: History shows work on URL alias support for open/navigate and recent touches around the affected browser tool path. (role: recent browser tool contributor; confidence: medium; commits: ed51796d9719, e085fa1a3ffd; files: extensions/browser/src/browser-tool.ts, extensions/browser/src/browser/client-actions-core.ts)
  • eleqtrizit: History shows strict browser hostname navigation hardening in the same navigation guard surface. (role: adjacent browser navigation security contributor; confidence: medium; commits: 121c452d666d; files: extensions/browser/src/browser/navigation-guard.ts)
  • obviyus: History shows a recent browser hostname SSRF policy change in the same guard file. (role: adjacent browser navigation policy contributor; confidence: medium; commits: bf1d49093ae3; files: extensions/browser/src/browser/navigation-guard.ts)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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
  • 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.

@lidge-jun
lidge-jun force-pushed the codex/55365-browser-url-userinfo branch 2 times, most recently from c120895 to fb45066 Compare May 17, 2026 01:53
@openclaw-barnacle openclaw-barnacle Bot added channel: whatsapp-web Channel integration: whatsapp-web extensions: memory-core Extension: memory-core labels May 17, 2026
@clawsweeper clawsweeper Bot added the P2 Normal backlog priority with limited blast radius. label May 17, 2026
@lidge-jun
lidge-jun force-pushed the codex/55365-browser-url-userinfo branch from fb45066 to 57441fa Compare May 17, 2026 02:36
@openclaw-barnacle openclaw-barnacle Bot added the commands Command implementations label May 17, 2026
@clawsweeper clawsweeper Bot added the impact:security Security boundary, credential, authz, sandbox, or sensitive-data risk. label May 17, 2026
@lidge-jun
lidge-jun force-pushed the codex/55365-browser-url-userinfo branch from 57441fa to df41c1a Compare May 17, 2026 02:47
@openclaw-barnacle openclaw-barnacle Bot removed channel: whatsapp-web Channel integration: whatsapp-web extensions: memory-core Extension: memory-core labels May 17, 2026
@clawsweeper clawsweeper Bot mentioned this pull request May 19, 2026
Closed
@RomneyDa

RomneyDa commented May 29, 2026

Copy link
Copy Markdown
Member

Heads up: this PR needs to be updated against current main before the new required Dependency Guard check can pass.

@lidge-jun
lidge-jun force-pushed the codex/55365-browser-url-userinfo branch from df41c1a to 0e1f69c Compare June 1, 2026 20:18
@openclaw-barnacle openclaw-barnacle Bot removed the commands Command implementations label Jun 1, 2026
@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. and removed impact:security Security boundary, credential, authz, sandbox, or sensitive-data risk. labels Jun 1, 2026
@clawsweeper clawsweeper Bot mentioned this pull request Jun 20, 2026
Open
20 tasks
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Jun 23, 2026
@steipete steipete self-assigned this Jul 9, 2026
@steipete steipete mentioned this pull request Jul 9, 2026
Merged
@steipete

steipete commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Thank you, @lidge-jun — this PR identified the right browser credential-leak boundary and provided the base for the landed fix.

I carried the change forward as #102952 and improved it to use one canonical parser before both host/Browser-node dispatch and server-side DNS/SSRF checks. The landed rewrite also redacts malformed credential-bearing input and live-verifies that explicit Browser HTTP credentials and authenticated CDP endpoint URLs still work.

Landed in #102952 as d5fb4903f1b13a4309d479f1011d995b1fc706ae.

I attempted the normal native maintainer takeover path first. GitHub reported maintainer_can_modify=true, but the fork branch ref did not accept/reflect the prepared maintainer update, so this PR could not be landed directly. I am closing it as superseded by the credited maintainer PR. For future fork PRs, please keep Allow edits by maintainers enabled; it was nominally enabled here, but the branch update path still failed in practice.

Thanks again for the contribution.

@steipete steipete closed this Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@steipete steipete

Labels

merge-risk: 🚨 compatibility 🚨 May break existing users, config, migrations, defaults, or upgrade paths. P2 Normal backlog priority with limited blast radius. proof: supplied External PR includes structured after-fix real behavior proof. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. size: S 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.

Browser tool should reject or redact URLs with embedded credentials instead of passing them through

3 participants

@lidge-jun @RomneyDa @steipete