Skip to content

Spammer should preserve secure WebSocket URLs and report invalid targets without panicking #347

Description

@Kewe63

Summary

The spammer's direct WebSocket target parser has two related problems:

  1. secure WebSocket URLs beginning with wss:// are rewritten as malformed ws:// URLs instead of being preserved;
  2. invalid target values panic through Url::parse(...).unwrap() instead of returning a normal CLI error.

Both behaviors originate in the same helper, ws_url_from_str(), and should be fixed together.

Affected file

  • crates/spammer/src/main.rs

Observed behavior

The current target parser is:

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

Secure WebSocket URLs are corrupted

Only the literal ws:// prefix is recognized. A valid secure target such as:

wss://rpc.example:8546

is prefixed again and parsed from:

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

The resulting URL is:

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

rather than the requested:

wss://rpc.example:8546/

The connection therefore no longer targets the requested secure WebSocket endpoint.

Invalid targets panic

Url::parse() errors are unconditionally unwrapped. Invalid host input can therefore terminate the process with a panic instead of returning a user-facing configuration error.

An invalid IDNA hostname produces:

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

Expected behavior

  • Explicit ws:// URLs should remain unchanged.
  • Explicit wss:// URLs should remain unchanged.
  • Bare host:port targets should continue to receive the default ws:// scheme.
  • Invalid targets should return a descriptive error identifying the offending --targets entry.
  • Unsupported explicit schemes should be rejected instead of being prefixed into misleading WebSocket URLs.
  • The CLI should exit normally with an error, not panic.

Reproduction

I added two focused regression tests against current main:

#[test]
fn ws_url_from_str_preserves_secure_websocket_scheme() {
    let url = ws_url_from_str("wss://rpc.example:8546".to_string());
    assert_eq!(url.as_str(), "wss://rpc.example:8546/");
}

#[test]
fn ws_url_from_str_does_not_panic_on_invalid_target() {
    let result = std::panic::catch_unwind(|| {
        ws_url_from_str("\u{200d}.example:8546".to_string())
    });
    assert!(result.is_ok(), "invalid target caused a panic");
}

Command:

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

Result:

running 4 tests

test tests::ws_url_from_str_adds_ws_scheme_if_missing ... ok
test tests::ws_url_from_str_parses_endpoint ... ok
test tests::ws_url_from_str_preserves_secure_websocket_scheme ... FAILED
test tests::ws_url_from_str_does_not_panic_on_invalid_target ... FAILED

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

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

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

The real CLI entry point also reproduces the panic before any network connection is attempted:

spammer --silent ws --targets $'\u200d.example:8546'

Result:

thread 'main' panicked at crates/spammer/src/main.rs:246:26:
called `Result::unwrap()` on an `Err` value: IdnaError

Exit code:

101

Tested against commit:

97f8da0dc4faa703fe2d68ca007e40dab2c8a9ef

Root cause

The helper uses a string-prefix check that recognizes only ws://, rather than parsing and validating the target's scheme.

It then unwraps the URL parser result. Consequently:

  • wss:// is mistaken for a schemeless target and receives a second scheme;
  • parse failures escape as panics.

Both failures occur before the target reaches the WebSocket client.

Why this matters

Secure WebSocket endpoints cannot be used reliably through the direct spammer ws --targets path. Operators can believe TLS is configured while the parser has silently changed the endpoint.

Invalid command-line input should also not crash the process. A normal error would identify the bad target and allow scripts or orchestrators to handle the failure predictably.

Suggested fix

Change the direct-target parsing path to return Result and propagate errors to main().

The parser should:

  1. preserve explicit ws:// and wss:// URLs;
  2. add ws:// only when the input has no scheme;
  3. parse the resulting URL without unwrap();
  4. verify that the final scheme is ws or wss;
  5. include the original target in any validation error.

For example, the relevant signatures could become:

fn ws_url_from_str(target: String) -> Result<Url>
fn ws_urls_from_strings(targets: Vec<String>) -> Result<Vec<(String, Url)>>

The TargetCommand::Ws branch can then propagate the result with ?.

Potential regression tests

Add tests verifying that:

  • bare host:port receives ws://;
  • an explicit ws:// URL is preserved;
  • an explicit wss:// URL is preserved;
  • malformed/invalid host input returns an error without panicking;
  • unsupported explicit schemes such as http:// are rejected;
  • a list containing one invalid target reports that target and does not partially continue.

An actual CLI test should also verify a nonzero error exit without panic text for invalid --targets input.

Scope

This should be limited to direct target parsing in crates/spammer/src/main.rs and its regression tests.

The nodes-metadata path already deserializes execution.ws_url directly as Url and does not use ws_url_from_str().

Duplicate check

I searched open and closed issues and pull requests using combinations of:

  • spammer wss
  • WebSocket target
  • invalid target
  • IdnaError
  • wss://
  • ws_url_from_str
  • panic

No direct duplicate or existing implementation was found.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions