@qwen-code /review

✅ Local build verification (macOS) — merge reference

Verified PR head 10e9ae6df in a clean isolated worktree built from origin/main (85fef8527) with a fresh npm ci. This fills the macOS coverage gap (the PR table marks macOS not tested) and independently confirms @doudouOUC's approval.

Env: macOS 26.5.1 (arm64) · Node v22.22.2 · npm 10.9.7

Results

Mutation test — the new tests are not vacuous

Removing the new try/catch block from the source (keeping the tests) fails exactly 6 tests and nothing else — 2 malformed-URL cases (https://, http://[::1, which exercise the catch) and 4 credential cases (user:secret@, user@, :secret@, %75ser@). Restoring → 24/24 green. This proves the tests actually exercise the new validation path.

Adversarial robustness (WHATWG new URL() on Node 22, independent of the suite)

No credential leak found. The two accepts are correct (they carry no credentials).

Why both the regex and new URL() are needed

new URL() alone would accept http:/example.com, http:example.com, and ftp://example.com (it normalizes/accepts them). The ^https?:// regex is what rejects those; new URL() adds userinfo detection + malformed-authority rejection. Keeping both (with the new explanatory comment) is correct defense-in-depth.

Review threads — all resolved in the current head

• ✅ Distinct catch message "The 'url' is malformed and could not be parsed." (no longer duplicates the scheme message)
• ✅ Explanatory comment added above the regex
• ✅ Malformed cases merged into the existing scheme it.each (no duplicated ftp:///http:… cases)
• ✅ catch branch now has test coverage (https://, http://[::1)

Minor (non-blocking)

The PR description's Evidence block is from a superseded commit (d464833, "22 passed"); the current head 10e9ae6df has 24 tests. Worth refreshing the description before merge — code is correct.

Verdict

LGTM — safe to merge. Surgical, well-tested, no regressions in sibling credentialed-URL subsystems (proxy / git clone / npm registry / redaction paths are separate and unaffected by this validation).

_{Local build verification by the maintainer; not an automated bot review.}

@qwen-code /triage

Thanks for the PR @VectorPeak!

Template looks good ✓ — all required sections present and filled in.

On direction: this is a straightforward credential-security hardening. URLs with embedded userinfo (user:pass@host) can leak through logging, error messages, and confirmation prompts. Fixes #5782, squarely in category/security / scope/credential-security. Clear alignment with the project's security posture — no direction concerns.

On approach: the scope is tight — 9 lines of validation logic + 31 lines of tests, 2 files. No unrelated changes, no scope creep. The approach of parsing with new URL() after the regex scheme check is correct defense-in-depth: the regex catches bad schemes (ftp://, http:) that new URL() silently normalizes, while new URL() catches userinfo and malformed authority that the regex misses. Clean and minimal.

Moving on to code review. 🔍

— Qwen Code · qwen3.7-max

Code Review

Independent proposal: Before reading the diff, my approach was: after the existing ^https?:// regex, add new URL() parsing in a try/catch, check username/password properties, reject with a distinct error. Add tests for user:pass, user-only, password-only, and URL-encoded userinfo.

Diff comparison: The PR's approach matches my proposal exactly — and goes slightly further by also adding malformed-URL test cases (https://, http://[::1) to the catch branch. The implementation is correct and minimal:

• The regex + new URL() layering is proper defense-in-depth — regex catches bad schemes that new URL() silently normalizes; new URL() catches userinfo and malformed authority.
• The new comment above the regex (// Regex rejects non-http(s) schemes and malformed authority that new URL() normalizes away.) is a good why comment explaining the dual-check rationale.
• Error messages are distinct per failure mode — scheme error, credential error, and malformed error are all different strings.
• No over-abstraction, no drive-by refactors, no scope creep. 2 files, focused change.

No blockers found. ✅

Real-Scenario Testing

Before (installed build — main without PR #5783)

=== BEFORE (main without PR #5783) ===
[PASS] https://example.com/page -> ACCEPT
[PASS] http://example.com -> ACCEPT
[PASS] HTTPS://example.com -> ACCEPT
[FAIL] https://user:secret@example.com/page -> ACCEPT
[FAIL] http://user@example.com/page -> ACCEPT
[FAIL] https://:secret@example.com/page -> ACCEPT
[FAIL] https://%75ser@example.com/page -> ACCEPT
[PASS] ftp://example.com -> REJECT: The 'url' must be a valid URL starting with http:// or https://.
[PASS] http:example.com -> REJECT: The 'url' must be a valid URL starting with http:// or https://.
[FAIL] https:// -> ACCEPT
[FAIL] http://[::1 -> ACCEPT

5/11 passed

The bug is clearly visible: all 4 credential URLs and both malformed URLs are silently accepted.

After (this PR — built from PR head)

=== AFTER (PR #5783 - userinfo URL validation) ===
[PASS] https://example.com/page -> ACCEPT
[PASS] http://example.com -> ACCEPT
[PASS] HTTPS://example.com -> ACCEPT
[PASS] https://user:secret@example.com/page -> REJECT: The 'url' must not include credentials.
[PASS] http://user@example.com/page -> REJECT: The 'url' must not include credentials.
[PASS] https://:secret@example.com/page -> REJECT: The 'url' must not include credentials.
[PASS] https://%75ser@example.com/page -> REJECT: The 'url' must not include credentials.
[PASS] ftp://example.com -> REJECT: The 'url' must be a valid URL starting with http:// or https://.
[PASS] http:example.com -> REJECT: The 'url' must be a valid URL starting with http:// or https://.
[PASS] https:// -> REJECT: The 'url' is malformed and could not be parsed.
[PASS] http://[::1 -> REJECT: The 'url' is malformed and could not be parsed.

11/11 passed

All credential URLs rejected, malformed URLs caught, normal URLs unaffected.

Unit Tests

cd packages/core && npx vitest run src/tools/web-fetch.test.ts
✓ src/tools/web-fetch.test.ts (25 tests) 94ms
Test Files  1 passed (1)
Tests  25 passed (25)

=== BEFORE (main without PR #5783) ===
[FAIL] https://user:secret@example.com/page -> ACCEPT
[FAIL] http://user@example.com/page -> ACCEPT
[FAIL] https://:secret@example.com/page -> ACCEPT
[FAIL] https://%75ser@example.com/page -> ACCEPT
[FAIL] https:// -> ACCEPT
[FAIL] http://[::1 -> ACCEPT
5/11 passed

=== AFTER (PR #5783 - userinfo URL validation) ===
[PASS] https://user:secret@example.com/page -> REJECT: The 'url' must not include credentials.
[PASS] http://user@example.com/page -> REJECT: The 'url' must not include credentials.
[PASS] https://:secret@example.com/page -> REJECT: The 'url' must not include credentials.
[PASS] https://%75ser@example.com/page -> REJECT: The 'url' must not include credentials.
[PASS] https:// -> REJECT: The 'url' is malformed and could not be parsed.
[PASS] http://[::1 -> REJECT: The 'url' is malformed and could not be parsed.
11/11 passed

✓ src/tools/web-fetch.test.ts (25 tests) 94ms
Tests  25 passed (25)

— Qwen Code · qwen3.7-max

This is a clean, surgical security fix. The before/after evidence speaks for itself — 6 previously-accepted dangerous inputs (credential URLs, malformed URLs) are now properly rejected, with zero regressions on valid inputs.

Going back to my independent proposal from the code review: the PR's approach matched it exactly and went slightly further with malformed-URL coverage. I don't see a simpler path — 9 lines of validation logic is about as minimal as this gets while still being correct.

The code is straightforward, the why-comment earns its place, error messages are distinct per failure mode, and the test suite is thorough without being bloated. If I were maintaining this in six months, I'd thank the author.

Approving. ✅

— Qwen Code · qwen3.7-max

Hi @wenshao, sorry for the bump. Just checking in — is there anything else needed on this PR, or would it be possible to proceed with the merge? Happy to address any remaining feedback. Thanks for your time!