Skip to content

Safe naming and authN checks - #155

Open
GauravPawar101 wants to merge 3 commits into
ekzhang:mainfrom
GauravPawar101:fix/CI
Open

GauravPawar101 wants to merge 3 commits into
ekzhang:mainfrom
GauravPawar101:fix/CI

Conversation

@GauravPawar101

@GauravPawar101 GauravPawar101 commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Two hardening fixes to the session WebSocket endpoint, plus incidental build/CI fixes.

  1. {name} path parameter is now validated before it is used. Session names are interpolated into an upstream URL when this server proxies a connection to a mesh peer (proxy_redirect → format!("ws://{host}/api/s/{name}")) and are also used as a storage key in the mesh (mesh.get_owner(name)). The value arrived from the wire as an unvalidated String, so a percent-encoded %2F, %2E%2E, %20, etc. decoded into a name containing path metacharacters that changed the meaning of the proxied URL and the key namespace.
  2. Session persistence is no longer triggerable by an unauthenticated client. handle_socket called session.sync_now() before reading the client's auth message, so any anonymous socket connection allocated a UID from the persisted counter and woke the storage sync task — forcing a snapshot + write to Redis.

No change for legitimate clients: session names are generated server-side as rand_alphanumeric(10) (grpc.rs:51), which is always within the new validator's alphabet.

Changes

SafeName — a validated identifier newtype

New module crates/sshx-core/src/name.rs, re-exported from sshx_core (pub use name::{InvalidName, SafeName}):

  • Whitelist, not blacklist. Session names come from rand_alphanumeric, so FromStr accepts only ASCII alphanumerics. Everything that could change the meaning of an interpolated URL path — /, ., %, \, ?, #, whitespace, control bytes, NUL, non-ASCII — is rejected outright rather than escaped.
  • Bounded input. Non-empty and at most MAX_LEN = 64 bytes. Far above the length of any generated name (10, and up to 22 for the server secret); the bound exists only to cap work on attacker-controlled input.
  • Invariants recorded, not assumed. The SafeName doc comment states the property callers rely on — interpolating a SafeName into a URL path is always correct, so callers never need to escape it. This is what makes proxy_redirect safe without changing that function.
  • Validation enforced by the deserializer. Deserialize is hand-written to go through from_str via a visitor, so the whitelist applies to every deserializer, not just explicit parse calls. This is the load-bearing part: it is what lets axum::extract::Path<SafeName> reject a bad name on its own, because axum deserializes path parameters — a malformed name fails extraction with 400 Bad Request before the handler, the mesh lookup, or the upstream proxy is ever reached.
  • Specific rejection reasons. InvalidName { Empty, TooLong, NotAlphanumeric } implements Display and std::error::Error, so logs carry the reason instead of a bare failure.
  • Ergonomics. Display, Deref<Target = str>, and as_str() keep it usable wherever a &str was used before.

Authenticated-before-persist ordering

In crates/sshx-server/src/web/socket.rs:

  • get_session_ws extracts Path<SafeName> instead of Path<String>.
  • session.sync_now() moves from before the Hello send / auth read to after the client has been authenticated and user_scope has registered the user. An unauthenticated or wrongly-authenticated connection now returns early via WsServer::InvalidAuth without ever marking the session for sync. As a side benefit, the snapshot that does get written now includes the user that was just allocated, rather than a bare counter increment.

Integration test

test_session_name_rejects_path_traversal in crates/sshx-server/tests/simple.rs performs real WebSocket handshakes against a fresh TestServer. Real handshakes are used deliberately: a plain GET cannot distinguish the two outcomes, since WebSocketUpgrade also rejects a non-upgrade request with 400.

  • Control case: a legitimate name (aB3xY9zQ1mK) must still complete its handshake.
  • Rejected with 400: a%2Fb, a%2F..%2F..%2Fevil, %2E%2E%2Fadmin, %2E%2E, a%20b, a+b. These stay percent-encoded on the wire (the http crate does not normalize paths) and are decoded to /, ., and space by the server before validation, which is exactly the case that has to be refused.

Unit tests

generated_names_are_always_safe samples rand_alphanumeric at several lengths and asserts every result parses as SafeName. This is a guard on the invariant the whole design rests on: if the generator's alphabet ever widens, the test fails instead of the validator silently becoming the thing that's wrong. The remaining tests cover acceptance, the list of rejected metacharacters, and the exact length boundary.

Compatibility

The only newly rejected inputs are ones that could never have been valid session names, so no legitimate client is affected. The observable difference for a malformed name is 400 at extraction instead of an upgrade followed by a 4404 close frame.

@GauravPawar101

Copy link
Copy Markdown
Contributor Author

@ekzhang I was not able to update description due to some reason. Just did it, lmk if any issues or discrepancies

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant