fix(spammer): validate WebSocket targets

[Kewe63]

Summary

Fixes #347

• Preserve explicit wss:// targets in the spammer's direct WebSocket target path.
• Continue adding ws:// to bare host:port targets.
• Reject unsupported explicit schemes instead of rewriting them into misleading WebSocket URLs.
• Propagate URL parsing failures as normal CLI errors instead of panicking through unwrap().
• Add focused regression coverage for secure WebSocket targets and invalid target handling.

---

Problem

ws_url_from_str() previously recognized only the literal ws:// prefix:

let url_str = if !ip_port.starts_with("ws://") {
    format!("ws://{ip_port}")
} else {
    ip_port
};
Url::parse(&url_str).unwrap()

This caused two related failures.

First, an explicit secure WebSocket target such as:

wss://rpc.example:8546

was prefixed again and parsed as:

ws://wss://rpc.example:8546

which normalized to the wrong endpoint:

ws://wss//rpc.example:8546

Second, invalid target values reached Url::parse(...).unwrap() and terminated the process with a panic. An invalid IDNA hostname produced:

called `Result::unwrap()` on an `Err` value: IdnaError

---

Changes

The direct-target parsing path now:

1. preserves inputs with an explicit URL scheme for validation;
2. adds ws:// only to schemeless targets such as 127.0.0.1:8546;
3. parses the resulting URL without unwrap();
4. accepts only ws and wss schemes;
5. reports the original --targets entry in parsing and scheme errors;
6. propagates errors through ws_urls_from_strings() and main().

The nodes-metadata path is unchanged because it already deserializes execution.ws_url directly as Url and does not call ws_url_from_str().

---

Regression Coverage

Added tests verifying that:

• bare host:port targets still receive ws://;
• explicit ws:// targets remain valid;
• explicit wss:// targets are preserved;
• invalid IDNA targets return a contextual error without panicking;
• unsupported explicit schemes such as http:// are rejected.

Existing direct-target list and nodes-metadata tests remain in place.

---

RED Verification

Before the production fix, the focused regression tests failed on current main:

secure WebSocket target:
  left: "ws://wss//rpc.example:8546"
 right: "wss://rpc.example:8546/"

invalid target:
called `Result::unwrap()` on an `Err` value: IdnaError

test result: FAILED. 2 passed; 2 failed; 8 filtered out

The real CLI entry point also panicked with exit code 101 before attempting a network connection.

---

How to Test

Focused target parsing tests:

cargo +1.94.0 test \
  -p spammer \
  --bin spammer \
  ws_url_from_str_ \
  -- --nocapture

Result:

5 passed; 0 failed

Complete spammer package:

cargo +1.94.0 test -p spammer

Results:

library tests: 72 passed; 0 failed
binary tests: 13 passed; 0 failed
doc tests: 0 failed

Additional checks:

cargo +1.94.0 clippy -p spammer --all-targets -- -D warnings
cargo +1.94.0 fmt -p spammer -- --check
git diff --check

All checks passed.

The fixed binary was also invoked with the same invalid IDNA target. It now exits normally with code 1 and reports:

Error: Invalid --targets entry "\u{200d}.example:8546"

Caused by:
    invalid international domain name

No panic text is emitted.

Local verification used Rust 1.94.0 because the local pinned 1.93.0 installation has a cargo-clippy component conflict. CI should provide the authoritative pinned-toolchain result.

---

Scope and Risk

Risk is low. The change is limited to direct spammer WebSocket target parsing in:

crates/spammer/src/main.rs

It does not change:

• WebSocket client connection or retry behavior;
• transaction generation or sending;
• nodes metadata parsing;
• consensus or protocol behavior;
• dependency versions.

The existing bare host:port and explicit ws:// forms remain supported.

---

Duplicate Check

Open and closed issues and pull requests were searched using the issue number, helper name, affected schemes, panic text, and target-parsing terms. No duplicate PR or existing implementation was found.

---

Checklist

• Bug reproduced on current main
• Regression tests confirmed failing before the fix
• wss:// targets are preserved
• Invalid targets return errors without panicking
• Unsupported explicit schemes are rejected
• Focused and complete package tests pass
• Formatting and Clippy checks pass
• Real CLI error path verified
• No unrelated files changed
• Independent read-only review found no blocking issues
• Follows Conventional Commits

---

Impact

Type: 🐛 Bug fix

Fixes: #347

[osr21]

Disclosure: I'm not affiliated with Circle — an external community contributor, not a maintainer, with no write access. This review is advisory only and carries no merge authority; please defer to Circle maintainers.

---

The bug is real, and your reported output is exact. I can't run Rust here, so I verified the normalization against the WHATWG URL Standard — which the url crate implements — using another conforming implementation:

"ws://wss://rpc.example:8546"  -> ws://wss//rpc.example:8546
"wss://rpc.example:8546"       -> wss://rpc.example:8546/
"WSS://rpc.example:8546"       -> wss://rpc.example:8546/
"ws://<ZWJ>.example:8546"      -> THROWS (invalid URL)

ws://wss//rpc.example:8546 matches the string in your issue character-for-character, including the collapsed colon. The double-prefix corruption and the IDNA failure both reproduce at spec level.

The scheme heuristic holds up

contains("://") is a heuristic rather than a real scheme parse, so I probed where it misfires. Every path I could find degrades safely:

• localhost:8546/path://x → treated as schemed → parses with scheme localhost → caught by your matches! check → contextual error.
• http:rpc.example (scheme, no //) → treated as schemeless → ws://http:rpc.example → parse error → contextual error.
• "" → ws:// → invalid empty host → contextual error.
• [::1]:8546 → no :// → ws://[::1]:8546/ → correct.

So the loose check never produces a panic or a silently wrong endpoint — the two properties the PR exists to guarantee. Worth stating explicitly, because contains("://") is the line a reviewer is most likely to squint at, and it's fine as written. Notably it also avoids the trap a Url::parse-based scheme probe would hit, where localhost:8546 parses as scheme localhost and a bare host:port gets misclassified.

This coverage is genuinely CI-enforced

Worth confirming, since it isn't true everywhere in this repo. spammer is a workspace member via crates/*, and the unit test job runs:

cargo nextest run --locked --workspace --exclude arc-test-integration

so all five tests execute on every PR. cargo clippy --all-targets --all-features -- -D warnings covers the test code too.

Pre-empting one objection: the workspace sets unwrap_used = "deny" and your new tests call .unwrap(). That's fine — clippy.toml sets allow-unwrap-in-tests = true. Not a problem, but a reviewer skimming the diff may well flag it.

Your toolchain caveat is also well-founded: rust-toolchain.toml pins 1.93.0 and the workspace declares rust-version = "1.93", and setup-rust-toolchain honours that file, so CI will be authoritative as you say. Nothing in the diff needs anything newer — ?, matches! and wrap_err_with are all long-stable — so 1.93 shouldn't surprise you. Context is already imported at the top of the file, so wrap_err_with resolves.

The change I'd actually suggest — make the fix self-enforcing

This is the one worth taking, and it's a single line.

main.rs opens with:

#![allow(
    clippy::arithmetic_side_effects,
    clippy::cast_possible_truncation,
    clippy::unwrap_used
)]

That blanket allow is why Url::parse(&url_str).unwrap() survived in production code despite the workspace denying unwrap_used. I checked what remains after your change — scanning everything above mod tests:

pr348 production unwrap/expect/panic before tests:  (NONE remaining)
main  production unwrap/expect/panic before tests:  246: Url::parse(&url_str).unwrap()

Yours was the last one. So dropping just clippy::unwrap_used from that allow list would let the workspace lint enforce the invariant you've established, and the tests keep passing untouched because allow-unwrap-in-tests already covers them. Leave the other two allows alone.

Without it, the fix is a point repair and the next unwrap() added to this file lands silently. With it, the file can't regress. Same PR, same file, one line.

Two minor notes

1. The ws subcommand's after_long_help still advertises only ws://:

spammer ws --targets ws://127.0.0.1:8546,ws://127.0.0.1:9546

Adding a wss:// example would make the newly-supported form discoverable — otherwise the capability exists but nothing points at it.

2. ws_urls_from_strings collects into Result<Vec<_>>, so it short-circuits on the first bad entry. With a comma-separated list, a user fixes one target and immediately rediscovers the next. Accumulating and reporting all invalid entries at once would be friendlier. Entirely optional, and arguably out of scope.

---

Careful, well-evidenced work — the RED-before-GREEN verification, the explicit statement that the nodes-metadata path is untouched, and the scope note all made this quick to check. Approving on the strength of source review and the spec-level reproduction; I could not compile or run the crate here, so CI remains authoritative on the pinned toolchain.