fix(matrix): guard transport new URL() against malformed input by ZengWen-DT · Pull Request #109759 · openclaw/openclaw

ZengWen-DT · 2026-07-17T07:17:35Z

What Problem This Solves

Fixes an issue where Matrix transport callers would receive an uncaught TypeError: Invalid URL when given malformed homeserver URLs, redirect Location headers, or absolute API endpoints, instead of a controlled application error.

Why This Change Was Made

Matrix transport now parses user-controlled URLs through a shared parseMatrixUrl helper that wraps new URL() in try/catch and throws a stable Error("Invalid URL") with the native parse failure retained as cause, matching the defensive pattern already used in the same file by withoutMatrixStateAfterSyncParam. Valid homeserver, redirect, and endpoint handling is unchanged. Risk note: this only hardens URL parsing at transport entry points; it does not change SSRF policy decisions once a URL parses successfully.

User Impact

Matrix channel setup and API calls now fail with a predictable typed error for malformed URLs, without echoing credential-bearing homeserver strings into diagnostics or logs.

Evidence

Before (upstream/main):

malformed homeserver error name: TypeError
malformed homeserver error message: Invalid URL
malformed guarded fetch URL error name: TypeError
malformed guarded fetch URL error message: Invalid URL

Premise (sibling pattern in the same file):
withoutMatrixStateAfterSyncParam already wraps new URL(rawUrl) in try/catch without reflecting the supplied URL.

After (this branch):

malformed homeserver error name: Error
malformed homeserver error message: Invalid URL
malformed homeserver message includes secret: false
malformed guarded fetch URL error name: Error
malformed guarded fetch URL error message: Invalid URL
malformed redirect location error name: Error
malformed redirect location error message: Invalid URL
malformed redirect location message includes secret: false

Harness (real exported functions, no mocks; redirect case uses a real loopback HTTP server):

node --import tsx -e "
import http from 'node:http';
import { performMatrixRequest, createMatrixGuardedFetch } from './extensions/matrix/src/matrix/sdk/transport.ts';
# ... homeserver, guarded fetch, and redirect probes as run locally
"

Focused tests:

node scripts/run-vitest.mjs run extensions/matrix/src/matrix/sdk/transport.test.ts -t "malformed"
# 5 passed

What was not tested: live Matrix homeserver setup with a real Synapse/Dendrite instance; the local harness proves transport entry points now fail closed with a stable non-disclosing error instead of a URL constructor TypeError, including on redirect Location headers from a real HTTP server.

AI-assisted: Cursor Composer; proof run manually on this machine.

Wrap user-controlled URL parsing in Matrix transport with a shared helper that throws a stable Invalid URL error instead of leaking URL constructor TypeErrors from malformed homeservers, redirect locations, and endpoints. Co-authored-by: Cursor <[email protected]>

clawsweeper · 2026-07-17T07:28:03Z

Codex review: needs changes before merge. Reviewed July 17, 2026, 3:24 AM ET / 07:24 UTC.

Summary
Wraps Matrix homeserver, endpoint, guarded-fetch, and redirect URL construction in a shared error-normalizing helper and adds malformed-input regression tests.

PR surface: Source +8, Tests +70. Total +78 across 2 files.

Reproducibility: yes. from source and the submitted before/after harness: malformed Matrix URLs reach these constructors and produce parser errors. The remaining disclosure is source-reproducible because the patch retains Node's documented input-bearing exception as cause.

Review metrics: none identified.

Merge readiness
Overall: 🦪 silver shellfish
Proof: 🐚 platinum hermit
Patch quality: 🦪 silver shellfish
Result: blocked by patch quality or review findings.

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

Rank-up moves:

Remove or sanitize the native URL parser cause.
Assert that full error and cause inspection omits the credential fixture.

Risk before merge

[P1] Merging the wrapper as written could create a false expectation that malformed credential-bearing Matrix URLs are safe to log, while the chained native error still retains the rejected input.

Maintainer options:

Sanitize the error chain (recommended)
Remove or sanitize the native URL exception and add regression coverage proving complete error inspection cannot reveal the malformed URL or embedded credentials.

Copy recommended automerge instruction

@clawsweeper automerge

Special instructions:
Remove credential-bearing URL parser state from the thrown Matrix error and add focused tests proving neither the outer error, its cause, nor inspected diagnostic output contains the malformed URL secret.

Next step before merge

[P2] The blocker has a narrow mechanical repair in the two touched Matrix transport files and does not require product direction.

Security
Needs attention: The new wrapper retains an invalid-URL exception that can contain the credential-bearing input and be recursively rendered by standard diagnostics.

Review findings

[P1] Remove the credential-bearing URL parse cause — extensions/matrix/src/matrix/sdk/transport.ts:52

Review details

Best possible solution:

Throw a stable Matrix URL error without retaining raw parser state, or preserve only sanitized metadata such as a safe error code, then test the complete inspected error and cause chain for secret absence across homeserver and redirect inputs.

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

Yes from source and the submitted before/after harness: malformed Matrix URLs reach these constructors and produce parser errors. The remaining disclosure is source-reproducible because the patch retains Node's documented input-bearing exception as cause.

Is this the best way to solve the issue?

No as written. Centralizing defensive URL parsing is the right boundary, but retaining the unsanitized native cause conflicts with the stated non-disclosure objective.

Full review comments:

[P1] Remove the credential-bearing URL parse cause — extensions/matrix/src/matrix/sdk/transport.ts:52
Node's ERR_INVALID_URL typically stores the rejected URL in input, and util.inspect() recursively renders an attached cause. Keeping the native exception here therefore leaves homeserver or redirect credentials inspectable despite the sanitized outer message; drop or sanitize the cause and test the complete diagnostic representation.
Confidence: 0.99

Overall correctness: patch is incorrect
Overall confidence: 0.96

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

add P2: This is a bounded Matrix transport hardening fix with limited blast radius, but its stated diagnostic-safety property remains incomplete.
add merge-risk: 🚨 security-boundary: The proposed error chain can retain credential-bearing URL input despite presenting a sanitized outer message.
add proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies before/after terminal output from real exported transport functions and a real loopback redirect server, convincingly showing the changed outer error behavior; the implementation review separately finds that the retained cause remains unsafe.
add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🐚 platinum hermit and patch quality is 🦪 silver shellfish.
add status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body supplies before/after terminal output from real exported transport functions and a real loopback redirect server, convincingly showing the changed outer error behavior; the implementation review separately finds that the retained cause remains unsafe.

Label justifications:

P2: This is a bounded Matrix transport hardening fix with limited blast radius, but its stated diagnostic-safety property remains incomplete.
merge-risk: 🚨 security-boundary: The proposed error chain can retain credential-bearing URL input despite presenting a sanitized outer message.
rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🐚 platinum hermit and patch quality is 🦪 silver shellfish.
status: ⏳ waiting on author: ClawSweeper has contributor-facing work open and is waiting for author action. Sufficient (terminal): The PR body supplies before/after terminal output from real exported transport functions and a real loopback redirect server, convincingly showing the changed outer error behavior; the implementation review separately finds that the retained cause remains unsafe.
proof: sufficient: Contributor real behavior proof is sufficient. The PR body supplies before/after terminal output from real exported transport functions and a real loopback redirect server, convincingly showing the changed outer error behavior; the implementation review separately finds that the retained cause remains unsafe.

Evidence reviewed

PR surface:

Source +8, Tests +70. Total +78 across 2 files.

View PR surface stats

Area	Files	Added	Removed	Net
Source	1	12	4	+8
Tests	1	70	0	+70
Docs	0	0	0	0
Config	0	0	0	0
Generated	0	0	0	0
Other	0	0	0	0
Total	2	82	4	+78

Security concerns:

[medium] Malformed URL remains available through cause — extensions/matrix/src/matrix/sdk/transport.ts:52
The outer message is generic, but the native parser error typically exposes the rejected URL through input; retaining it as cause undermines the claimed secret-redaction boundary.
Confidence: 0.99

Acceptance criteria:

[P1] node scripts/run-vitest.mjs extensions/matrix/src/matrix/sdk/transport.test.ts.
[P1] Run the PR's exported-function loopback harness and inspect the complete thrown error object and cause chain for secret absence.

What I checked:

Sensitive native cause retained: The new helper throws a generic outer error but attaches the original URL-constructor exception as cause. (extensions/matrix/src/matrix/sdk/transport.ts:52, 801631d67cc0)
Node invalid-URL contract: Node documents that ERR_INVALID_URL typically exposes the rejected URL through its input property. (nodejs.org)
Cause inspection behavior: Node documents that util.inspect() recursively serializes an error's cause, making the retained invalid URL and any embedded credentials observable in common diagnostics. (nodejs.org)
Incomplete secrecy assertion: The added credential regression checks the outer error message but does not inspect the retained cause or complete diagnostic representation. (extensions/matrix/src/matrix/sdk/transport.test.ts:512, 801631d67cc0)

Likely related people:

slepybear: Recent released work credited this contributor for Matrix security behavior, making them a useful reviewer for the plugin-owned error boundary. (github.com) (role: recent Matrix security contributor; confidence: medium; files: extensions/matrix/src/matrix/sdk/transport.ts)
gumadeiras: Recent released Matrix command-path work credits this contributor, providing adjacent current-history familiarity with the Matrix plugin. (github.com) (role: recent Matrix contributor; confidence: low; files: extensions/matrix/src/matrix/sdk/transport.ts, extensions/matrix/src/matrix/sdk/transport.test.ts)
mjamiv: Recent repository discussion shows detailed work on shared SSRF and guarded-fetch behavior, which is adjacent to this transport's URL and network safety boundary. (github.com) (role: adjacent network-safety contributor; confidence: low; files: extensions/matrix/src/matrix/sdk/transport.ts, src/infra/net/ssrf.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.

openclaw-barnacle Bot added channel: matrix Channel integration: matrix size: S labels Jul 17, 2026

LZY3538 mentioned this pull request Jul 17, 2026

fix(plugin-sdk): guard provider catalog live URL parsing against malformed responses #109986

Merged

ZengWen-DT closed this Jul 18, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(matrix): guard transport new URL() against malformed input#109759

fix(matrix): guard transport new URL() against malformed input#109759
ZengWen-DT wants to merge 1 commit into
openclaw:mainfrom
ZengWen-DT:fix/matrix-transport-guard-malformed-url

ZengWen-DT commented Jul 17, 2026

Uh oh!

clawsweeper Bot commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

ZengWen-DT commented Jul 17, 2026

What Problem This Solves

Why This Change Was Made

User Impact

Evidence

Uh oh!

clawsweeper Bot commented Jul 17, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant