refactor(gateway): hygiene batch — delete dead handler, tighten boundaries, add caller-level chat tests

[ilblackdragon]

Summary

Five follow-ups from the ironclaw#2599 tracking comment, bundled because each is small, touches the same file family, and #5 depends on #4's effect (origin gate now precedes the WS upgrade extractor — which is what lets oneshot unit-test the origin rejection).

1. Delete src/channels/web/handlers/static_files.rs (163 LOC dead code)

Every pub fn in this file had a canonical live version the router actually wires:

Dead	Canonical
health_handler	platform/static_files::health_handler
project_{redirect,index,file}_handler	platform/static_files::*
logs_events_handler	features/logs::logs_events_handler
gateway_status_handler	features/status::gateway_status_handler

Left behind from stage 4b. #[allow(dead_code)] on the pub mod static_files; declaration was a breadcrumb. Nothing imports from handlers::static_files, safe git rm.

2. is_local_origin case-insensitive

features/chat/mod.rs::is_local_origin used exact-case matching. Browsers lowercase Origin in practice, but RFC 7230 §5.4 permits uppercase hostnames and HTTP is case-insensitive in the scheme. The helper now lowercases before parsing. Regression test test_is_local_origin_accepts_uppercase pins http://LOCALHOST, HTTP://localhost, HTTP://127.0.0.1.

3. extensions_install_handler validates req.name via ExtensionName::new

The sibling URL-path handlers (activate, remove, setup, setup_submit) all validate at the boundary; install was the last untyped entry point. The JSON-body name now parses through ExtensionName::new before reaching registry lookup, filesystem paths under ~/.ironclaw/extensions/, or any extension-manager call.

Behavior change: names that previously reached the install pipeline and failed deep now return 400 Invalid extension name: ... at the boundary. test_extensions_install_handler_rejects_malformed_name pins .., ../traversal, slash/name, BadCase, has space.

4. Refactor chat_ws_handler so the Origin gate runs before the WS upgrade extractor

Swap ws: WebSocketUpgrade → ws: Result<WebSocketUpgrade, WebSocketUpgradeRejection>. Two real effects:

• Security signal is more accurate. Bad-origin requests now always return 403 Forbidden regardless of whether they included upgrade headers. Previously a probe without upgrade headers got 426 Upgrade Required — which told the caller "you're allowed here, just add these headers" instead of "you're not allowed."
• Unit testing unblocked. tower::ServiceExt::oneshot can't synthesize hyper's OnUpgrade extension, so the previous signature made origin-rejection branches untestable via unit tests. The new signature hands the handler the raw extraction result so origin gating fires first; oneshot now reaches all three origin paths.

5. Four caller-level chat handler tests

Per .claude/rules/testing.md ("Test Through the Caller, Not Just the Helper"):

• chat_send_handler — 3 tests: forwards to msg_tx (side effect verified via receiver), returns 503 without channel, rate-limits after 30/60s threshold.
• chat_ws_handler — 3 tests: rejects missing Origin → 403, rejects remote Origin → 403, accepts localhost Origin (asserts 426 as positive signal the gate passed — oneshot can't complete the upgrade, real 101 covered by tests/ws_gateway_integration.rs).
• chat_threads_handler — 1 test: returns in-memory threads when DB absent (fallback branch).
• chat_new_thread_handler — 1 test: persists to both session AND conversation store (both side effects verified).

Quality gate

• cargo fmt --all
• cargo clippy --all --benches --tests --examples --all-features — zero warnings
• cargo check -p ironclaw --no-default-features --features libsql --tests — clean
• cargo test -p ironclaw --lib channels::web — 456 passed (up 10 from staging baseline)
• cargo test -p ironclaw --test multi_tenant_integration — 40 passed
• cargo test -p ironclaw --test ws_gateway_integration — 11 passed (all real-TCP WS paths still green with the refactored handler)
• python3 scripts/check_gateway_boundaries.py + test — clean, 16/16
• bash scripts/pre-commit-safety.sh — clean

Follow-ups status after this merges

From the #2599 tracking comment:

• ✅ Test helper relocation (refactor(gateway): promote cross-slice GatewayState test builders — ironclaw#2599 stage-6 prereq #2704)
• ✅ server.rs deletion (refactor(gateway): delete server.rs shim + relocate tests to slices — ironclaw#2599 stage 6 #2706)
• ✅ is_local_origin case sensitivity (this PR)
• ✅ Four caller-level chat handler tests (this PR)
• ✅ extensions_install_handler ExtensionName::new validation (this PR)
• 🔲 GatewayStatusResponse duplicate engine_v2 fields (coordinated backend+frontend)
• 🔲 chat_history_handler holds session lock across DB I/O
• 🔲 Extension.name typing (String → ExtensionName — needs hyphen-fold harmonization with pairing storage)

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Refactors parts of the web gateway to remove dead code, tighten input/origin validation boundaries, and add caller-level tests for chat and extensions handlers (aligned with the gateway boundary/testing guidance from ironclaw#2599).

Changes:

• Removed unused legacy static-file handler module and its module export.
• Hardened boundary validation: is_local_origin becomes case-insensitive; extensions_install_handler validates req.name via ExtensionName::new.
• Refactored chat_ws_handler to run the Origin gate before the WS-upgrade extractor and added caller-level tests for chat handlers.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/channels/web/handlers/static_files.rs	Deleted dead/unused handlers module.
src/channels/web/handlers/mod.rs	Removed static_files module export.
src/channels/web/features/extensions/mod.rs	Validate install request name via ExtensionName::new; added regression test for malformed names.
src/channels/web/features/chat/mod.rs	Origin gate precedes WS upgrade extraction; is_local_origin lowercases; added caller-level tests for chat handlers.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

chat_ws_handler converts WebSocketUpgradeRejection into just (StatusCode, String), which drops the rejection response headers/body (e.g., 426 Upgrade Required typically includes WS-specific headers). Consider returning Ok(rej.into_response()) on rejection (or changing the error type) so axum’s full rejection response is preserved.

[ilblackdragon]

Fixed in 03d2d66. Changed the handler's return type to plain axum::response::Response and return rej.into_response() verbatim on upgrade failure — axum's 426 response (with its RFC 7231 §6.5.15 Upgrade header) and 400 variants now pass through untouched. The two origin-rejection early returns also go through .into_response() so the response shape stays consistent. The three caller-level origin tests keep asserting on the status codes (403 / 403 / 426), and the tests/ws_gateway_integration.rs real-TCP suite still passes.

[Copilot]

This test awaits rx.recv().await without a timeout; if the handler stops sending (regression), the test can hang the entire suite instead of failing. Use a short tokio::time::timeout around recv() (similar to other tests in this module).

Suggested change

	let received = rx
	.recv()
	.await
	let received = tokio::time::timeout(std::time::Duration::from_secs(1), rx.recv())
	.await
	.expect("timed out waiting for msg_tx to receive the accepted message")

[ilblackdragon]

Fixed in 03d2d66. Wrapped the rx.recv() in a 500ms tokio::time::timeout with an explicit .expect("accepted send must enqueue a message promptly") so a regression that returns 202 without sending fails the test instead of hanging the suite.

[Copilot]

The loop drains the channel via rx.recv().await with no timeout. If a future change causes chat_send_handler to return 202 without actually sending, this will hang rather than fail. Wrap the receive in a small tokio::time::timeout and assert it completed.

Suggested change

	let _ = rx.recv().await;
	tokio::time::timeout(tokio::time::Duration::from_millis(100), rx.recv())
	.await
	.expect("accepted send must enqueue a message promptly")
	.expect("channel closed before queued message was received");

[ilblackdragon]

Fixed in 03d2d66. Each of the 30 drain iterations now goes through tokio::time::timeout(Duration::from_millis(100), rx.recv()) with an explicit expect, so a no-send regression fails fast instead of hanging the loop.

[gemini-code-assist]

Code Review

This pull request refactors the WebSocket handler to improve origin validation and error reporting, updates origin checks to be case-insensitive, and introduces input validation for extension installation names to prevent path traversal. It also removes the static_files module and adds comprehensive unit tests for chat handlers and extension installation. Feedback includes preserving protocol-required headers in WebSocket upgrade rejections by returning the response directly and improving the robustness of origin string normalization by trimming whitespace before case conversion.

[gemini-code-assist]

By extracting only the status code from the WebSocketUpgradeRejection and returning a new (StatusCode, String) error, the handler discards any headers present in the original rejection response. For 426 Upgrade Required responses, RFC 7231 requires an Upgrade header field to indicate the supported protocols, which Axum's rejection response typically includes. Consider changing the handler's return type to axum::response::Response (and converting other error paths via .into_response()) so that the rejection response can be returned directly, preserving its headers.

[ilblackdragon]

Fixed in 03d2d66 — handler return type is now plain axum::response::Response, the WebSocketUpgradeRejection passes through via rej.into_response(), and the early origin-rejection arms route through .into_response() as well. RFC 7231 §6.5.15 Upgrade header on 426 (and diagnostics on the 400 variants) are preserved verbatim. Copilot independently flagged the same finding on a sibling thread; both resolved by the same change.

[gemini-code-assist]

The current implementation is susceptible to false negatives if the Origin header contains leading or trailing whitespace (e.g., " http://localhost"), as the strip_prefix and subsequent matches! check are whitespace-sensitive. To adhere to the project convention of avoiding unnecessary allocations for invalid or empty inputs, the string should be trimmed and checked for emptiness before calling to_ascii_lowercase(), which is the preferred method for case-insensitive comparisons in this repository.

    let origin = origin.trim();
    if origin.is_empty() {
        return false;
    }
    let origin_lc = origin.to_ascii_lowercase();

References

1. When canonicalizing a string, perform cheaper validation checks (like length, emptiness, and character set) on a trimmed slice before performing more expensive operations like replace that allocate a new string.
2. For case-insensitive comparisons, use to_ascii_lowercase() as it is the project convention, especially when the text is known to be ASCII.

[ilblackdragon]

Respectfully declining this one.

Trimming origin before parsing would flip the behavior for whitespace-padded inputs from reject to accept. Current flow for e.g. " http://localhost":

1. to_ascii_lowercase() → " http://localhost" (whitespace preserved).
2. strip_prefix("http://") fails on the leading space.
3. host = "" falls through unwrap_or("").
4. matches!(host, "localhost" | ...) → false, returns 403.

With the suggested trim():

1. trim() → "http://localhost".
2. strip_prefix + match → host = "localhost" → returns true, upgrade accepted.

Browsers don't send padded Origin headers in practice (RFC 6454 specifies the serialized form without padding), so callers who send them are either broken clients or probes. Keeping the "reject on odd shape" default is the safer posture — especially on a CSRF gate where the cost of a false reject is a weird-but-fixable client bug and the cost of a false accept is cross-site WS hijacking.

The allocation optimization angle of the suggestion also doesn't buy much — "".to_ascii_lowercase() is essentially free, so the is-empty early-return saves nothing measurable in the hot path. Leaving this as-is.