diff --git a/crates/trusted-server-cli/src/commands/dev/proxy/browser.rs b/crates/trusted-server-cli/src/commands/dev/proxy/browser.rs index 4af9b2d3d..e61f92a01 100644 --- a/crates/trusted-server-cli/src/commands/dev/proxy/browser.rs +++ b/crates/trusted-server-cli/src/commands/dev/proxy/browser.rs @@ -18,7 +18,7 @@ use crate::output; /// is `on` or `off`. A missing third line is tolerated when reading (treated as /// `on` if a URL is present, else `off`) for forward-compatibility with the /// earlier two-line format. -const SAFARI_RESTORE_FILE: &str = "safari-proxy-restore"; +pub(super) const SAFARI_RESTORE_FILE: &str = "safari-proxy-restore"; /// Generates a PAC script that proxies only `https://` requests for matched FROM hosts. /// diff --git a/crates/trusted-server-cli/src/commands/dev/proxy/config.rs b/crates/trusted-server-cli/src/commands/dev/proxy/config.rs index d831735c2..86ffc9d0f 100644 --- a/crates/trusted-server-cli/src/commands/dev/proxy/config.rs +++ b/crates/trusted-server-cli/src/commands/dev/proxy/config.rs @@ -349,6 +349,7 @@ fn resolve_basic_auth(args: &ProxyArgs) -> Result, ConfigError #[cfg(test)] mod tests { + use clap::Parser as _; use hyper::header::HeaderValue; use rustls::pki_types::ServerName; @@ -358,31 +359,25 @@ mod tests { AddressPolicy, OriginKey, ReferenceIdentity, Transport, VerifyMode, }; - fn base_args() -> crate::commands::dev::proxy::ProxyArgs { - // Construct via clap so defaults match the real surface. - use clap::Parser; - #[derive(clap::Parser)] - struct W { - #[command(flatten)] - a: crate::commands::dev::proxy::ProxyArgs, - } - W::parse_from(["ts"]).a + fn parse_args(argv: &[&str]) -> crate::commands::dev::proxy::ProxyArgs { + try_parse_args(argv).expect("should parse proxy args") } - fn parse_args(argv: &[&str]) -> crate::commands::dev::proxy::ProxyArgs { - use clap::Parser; + fn try_parse_args( + argv: &[&str], + ) -> Result { #[derive(clap::Parser)] struct W { #[command(flatten)] a: crate::commands::dev::proxy::ProxyArgs, } - W::parse_from(argv).a + W::try_parse_from(argv).map(|w| w.a) } #[test] fn clap_parses_rewrite_host_as_a_bool() { assert!( - !parse_args(&["ts"]).rewrite_host, + !parse_args(&["ts", "--from", "a.example.com", "--to", "b.example.com"]).rewrite_host, "absent --rewrite-host is false" ); assert!( @@ -391,11 +386,25 @@ mod tests { ); } + #[test] + fn clap_applies_the_real_listen_default() { + let args = parse_args(&["ts", "--rewrite-host"]); + assert_eq!( + args.listen, + crate::commands::dev::proxy::DEFAULT_LISTEN, + "should apply the real clap --listen default" + ); + } + #[test] fn single_rule_from_to_keeps_from_host_by_default() { - let mut args = base_args(); - args.from = Some("www.example-publisher.com".into()); - args.to = Some("to.edgecompute.app".into()); + let args = parse_args(&[ + "ts", + "--from", + "www.example-publisher.com", + "--to", + "to.edgecompute.app", + ]); let cfg = resolve(&args).expect("should resolve"); let rule = cfg .rules @@ -411,9 +420,12 @@ mod tests { #[test] fn rewrite_host_uses_to() { - let mut args = base_args(); - args.map = vec!["www.example-publisher.com=to.edgecompute.app".into()]; - args.rewrite_host = true; + let args = parse_args(&[ + "ts", + "--map", + "www.example-publisher.com=to.edgecompute.app", + "--rewrite-host", + ]); let cfg = resolve(&args).expect("should resolve"); assert_eq!( rewrite_for( @@ -429,10 +441,13 @@ mod tests { #[test] fn resolve_pins_host_to_ip() { - let mut args = base_args(); - args.map = vec!["www.example-publisher.com=ts.edgecompute.app".into()]; - // Mixed case to confirm the host key is lowercased. - args.resolve = vec!["TS.EdgeCompute.app:192.0.2.10".into()]; + let args = parse_args(&[ + "ts", + "--map", + "www.example-publisher.com=ts.edgecompute.app", + "--resolve", + "TS.EdgeCompute.app:192.0.2.10", // Mixed case to confirm the host key is lowercased. + ]); let cfg = resolve(&args).expect("should resolve"); assert_eq!( cfg.resolve.get("ts.edgecompute.app"), @@ -443,10 +458,13 @@ mod tests { #[test] fn resolve_accepts_ipv6_target() { - let mut args = base_args(); - args.map = vec!["a.example.com=b.edgecompute.app".into()]; - // Split-on-first-colon must keep the colon-bearing IPv6 address intact. - args.resolve = vec!["b.edgecompute.app:::1".into()]; + let args = parse_args(&[ + "ts", + "--map", + "a.example.com=b.edgecompute.app", + "--resolve", + "b.edgecompute.app:::1", // Split-on-first-colon must keep the colon-bearing IPv6 address intact. + ]); let cfg = resolve(&args).expect("should resolve"); assert_eq!( cfg.resolve.get("b.edgecompute.app"), @@ -457,11 +475,15 @@ mod tests { #[test] fn resolve_host_not_matching_any_rule_warns_but_succeeds() { - let mut args = base_args(); - args.map = vec!["a.example.com=b.edgecompute.app".into()]; - // A pin for a host that is no rule's TO is a likely typo: it should warn - // (not error) and still be recorded. - args.resolve = vec!["typo.edgecompute.app:192.0.2.10".into()]; + let args = parse_args(&[ + "ts", + "--map", + "a.example.com=b.edgecompute.app", + // A pin for a host that is no rule's TO is a likely typo: it should warn + // (not error) and still be recorded. + "--resolve", + "typo.edgecompute.app:192.0.2.10", + ]); let cfg = resolve(&args).expect("an unmatched --resolve host should warn, not error"); assert!( cfg.resolve.contains_key("typo.edgecompute.app"), @@ -471,9 +493,13 @@ mod tests { #[test] fn resolve_rejects_malformed_value() { - let mut args = base_args(); - args.map = vec!["a.example.com=b.edgecompute.app".into()]; - args.resolve = vec!["b.edgecompute.app:not-an-ip".into()]; + let args = parse_args(&[ + "ts", + "--map", + "a.example.com=b.edgecompute.app", + "--resolve", + "b.edgecompute.app:not-an-ip", + ]); let err = resolve(&args).expect_err("a non-IP --resolve target should error"); assert!( matches!(err.current_context(), ConfigError::Resolve { .. }), @@ -483,8 +509,7 @@ mod tests { #[test] fn map_value_must_be_from_equals_to() { - let mut args = base_args(); - args.map = vec!["not-a-map".into()]; + let args = parse_args(&["ts", "--map", "not-a-map"]); assert!(resolve(&args).is_err(), "malformed --map errors"); } @@ -492,11 +517,16 @@ mod tests { fn basic_auth_on_non_loopback_listen_is_rejected() { // Injected Basic auth on a non-loopback bind would expose the upstream // credentials to any reachable network client. - let mut args = base_args(); - args.map = vec!["a.example.com=b.edgecompute.app".into()]; - args.listen = "0.0.0.0:18080".into(); - args.allow_non_loopback = true; - args.basic_auth = Some("dev:secret".into()); + let args = parse_args(&[ + "ts", + "--map", + "a.example.com=b.edgecompute.app", + "--listen", + "0.0.0.0:18080", + "--allow-non-loopback", + "--basic-auth", + "dev:secret", + ]); let err = resolve(&args).expect_err("non-loopback listen with --basic-auth should be rejected"); assert!( @@ -508,7 +538,14 @@ mod tests { ); // The same non-loopback bind without credentials is allowed. - args.basic_auth = None; + let args = parse_args(&[ + "ts", + "--map", + "a.example.com=b.edgecompute.app", + "--listen", + "0.0.0.0:18080", + "--allow-non-loopback", + ]); assert!( resolve(&args).is_ok(), "non-loopback without --basic-auth is allowed" @@ -518,8 +555,7 @@ mod tests { #[test] fn invalid_from_host_is_rejected() { // A FROM with characters that would break the PAC JS / Host header. - let mut args = base_args(); - args.map = vec!["bad\"host=to.edgecompute.app".into()]; + let args = parse_args(&["ts", "--map", "bad\"host=to.edgecompute.app"]); let err = resolve(&args).expect_err("a malformed FROM host should error"); assert!( matches!(err.current_context(), ConfigError::InvalidFrom { .. }), @@ -529,14 +565,25 @@ mod tests { #[test] fn non_loopback_listen_requires_flag() { - let mut args = base_args(); - args.map = vec!["a.example.com=b.edgecompute.app".into()]; - args.listen = "0.0.0.0:18080".into(); + let args = parse_args(&[ + "ts", + "--map", + "a.example.com=b.edgecompute.app", + "--listen", + "0.0.0.0:18080", + ]); assert!( resolve(&args).is_err(), "non-loopback without flag is rejected" ); - args.allow_non_loopback = true; + let args = parse_args(&[ + "ts", + "--map", + "a.example.com=b.edgecompute.app", + "--listen", + "0.0.0.0:18080", + "--allow-non-loopback", + ]); assert!(resolve(&args).is_ok(), "non-loopback allowed with flag"); } @@ -559,11 +606,15 @@ mod tests { #[test] fn resolve_precomputes_typed_rule_identity_and_headers() { - let mut args = base_args(); - args.map = vec!["www.example.com=TO.Example.com:8443".into()]; - args.rewrite_host = true; - args.insecure = true; - args.resolve = vec!["to.example.com:192.0.2.10".into()]; + let args = parse_args(&[ + "ts", + "--map", + "www.example.com=TO.Example.com:8443", + "--rewrite-host", + "--insecure", + "--resolve", + "to.example.com:192.0.2.10", + ]); let cfg = resolve(&args).expect("should resolve"); let rule = cfg @@ -602,10 +653,7 @@ mod tests { #[test] fn resolve_keeps_ip_reference_identities_http1_only() { - let mut args = base_args(); - args.map = vec!["www.example.com=127.0.0.1".into()]; - args.rewrite_host = true; - + let args = parse_args(&["ts", "--map", "www.example.com=127.0.0.1", "--rewrite-host"]); let cfg = resolve(&args).expect("should resolve"); let rule = cfg .rules @@ -655,9 +703,13 @@ mod tests { let dir = tempfile::tempdir().expect("should create temp dir"); let missing = dir.path().join("no-such-file.txt"); - let mut args = base_args(); - args.map = vec!["a.example.com=b.edgecompute.app".into()]; - args.basic_auth_file = Some(missing.to_string_lossy().into_owned()); + let args = parse_args(&[ + "ts", + "--map", + "a.example.com=b.edgecompute.app", + "--basic-auth-file", + &missing.to_string_lossy(), + ]); let err = resolve(&args).expect_err("should fail when file is missing"); assert!( @@ -666,9 +718,24 @@ mod tests { ); } + #[test] + fn bare_invocation_is_rejected_at_parse_time() { + // `arg_required_else_help` makes a fully-bare `ts` fail to parse at all, + // before `resolve` (and its `NoRule` check) ever runs. + let error = try_parse_args(&["ts"]) + .expect_err("a fully-bare invocation should short-circuit to help"); + assert_eq!( + error.kind(), + clap::error::ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand, + "should short-circuit to help rather than reaching resolve" + ); + } + #[test] fn no_rule_passed_is_a_no_rule_error() { - let args = base_args(); + // An invocation with some other flag but no rule still reaches + // `resolve`: `arg_required_else_help` only rejects a fully-bare `ts`. + let args = parse_args(&["ts", "--insecure"]); let err = resolve(&args).expect_err("should error when no rule is passed"); assert!( matches!(err.current_context(), ConfigError::NoRule), diff --git a/crates/trusted-server-cli/src/commands/dev/proxy/mod.rs b/crates/trusted-server-cli/src/commands/dev/proxy/mod.rs index 1bebd872d..8bb28118a 100644 --- a/crates/trusted-server-cli/src/commands/dev/proxy/mod.rs +++ b/crates/trusted-server-cli/src/commands/dev/proxy/mod.rs @@ -76,8 +76,13 @@ async fn finish_interrupted_run( let _ = tokio::time::timeout(std::time::Duration::from_secs(2), drain_manager).await; } +/// Default `--listen` address, shared with the `config` tests so they cannot +/// silently drift from the real default. +pub(crate) const DEFAULT_LISTEN: &str = "127.0.0.1:18080"; + /// `ts dev proxy [OPTIONS]` โ€” see the design spec ยง4. #[derive(Debug, clap::Args)] +#[command(arg_required_else_help = true)] pub struct ProxyArgs { /// Rewrite rule `FROM=TO` (repeatable). #[arg(long = "map", value_name = "FROM=TO")] @@ -94,7 +99,7 @@ pub struct ProxyArgs { pub to: Option, /// Proxy listen address. Non-loopback requires `--allow-non-loopback`. - #[arg(long, value_name = "ADDR", default_value = "127.0.0.1:18080")] + #[arg(long, value_name = "ADDR", default_value = DEFAULT_LISTEN)] pub listen: String, /// Permit binding a non-loopback `--listen` (disables blind tunnel/forward). @@ -247,14 +252,19 @@ pub fn run(args: &ProxyArgs) -> core::result::Result<(), error_stack::Report::try_parse_from(["ts", "--insecure", "--ca-dir", &ca_dir]) + .expect("should parse proxy args") + .a; + + let err = run(&args).expect_err("should fail without a rewrite rule"); + + assert!( + matches!(err.current_context(), ProxyError::Config), + "should fail with a config error" + ); + assert!( + restore_path.exists(), + "should fail before attempting to restore the system proxy" + ); + } } diff --git a/crates/trusted-server-cli/src/run.rs b/crates/trusted-server-cli/src/run.rs index 44ddbafe7..0468c600c 100644 --- a/crates/trusted-server-cli/src/run.rs +++ b/crates/trusted-server-cli/src/run.rs @@ -1073,4 +1073,32 @@ mod tests { "error should explain unsupported option" ); } + + #[test] + #[cfg(target_os = "macos")] + fn dev_proxy_bare_invocation_shows_help_before_running() { + let error = Args::try_parse_from(["ts", "dev", "proxy"]) + .expect_err("a bare `ts dev proxy` should short-circuit to help, not run"); + assert_eq!( + error.kind(), + clap::error::ErrorKind::DisplayHelpOnMissingArgumentOrSubcommand, + "should print help instead of touching system proxy state or attempting sudo" + ); + } + + #[test] + #[cfg(target_os = "macos")] + fn dev_proxy_ca_subcommands_still_parse_under_arg_required_else_help() { + for action in ["path", "install", "uninstall", "regenerate"] { + parse(&["ts", "dev", "proxy", "ca", action]); + } + } + + #[test] + #[cfg(target_os = "macos")] + fn dev_proxy_partial_rule_parses_instead_of_showing_help() { + // An explicit but incomplete rule (`--from` with no `--to`) must reach + // `run` and surface the concise no-rule error there, not clap help. + parse(&["ts", "dev", "proxy", "--from", "a.example.com"]); + } } diff --git a/crates/trusted-server-cli/tests/support/mod.rs b/crates/trusted-server-cli/tests/support/mod.rs index 8e8e3e66f..6c464b015 100644 --- a/crates/trusted-server-cli/tests/support/mod.rs +++ b/crates/trusted-server-cli/tests/support/mod.rs @@ -206,7 +206,7 @@ fn resolve(argv: &[&str]) -> config::ResolvedConfig { #[command(flatten)] args: trusted_server_cli::commands::dev::proxy::ProxyArgs, } - let parsed = Wrapper::parse_from(argv); + let parsed = Wrapper::try_parse_from(argv).expect("should parse proxy args"); config::resolve(&parsed.args).expect("should resolve test config") } diff --git a/docs/guide/ts-dev-proxy.md b/docs/guide/ts-dev-proxy.md index b99603b9e..789393e92 100644 --- a/docs/guide/ts-dev-proxy.md +++ b/docs/guide/ts-dev-proxy.md @@ -77,8 +77,9 @@ shorthand, or one or more `--map FROM=TO` rules: ts dev proxy -f www.example-publisher.com -t trusted-server-example.edgecompute.app ``` -With no `--map`/`-f`/`-t`, the proxy exits with -`no rewrite rule: pass --map FROM=TO (or -f/--from with -t/--to)`. +A bare `ts dev proxy` prints help and exits before proxy startup. An +invocation with explicit options but no complete rewrite rule fails with a +`no rewrite rule` error before touching system proxy state. Connection options โ€” `--rewrite-host`, `--basic-auth`/`--basic-auth-file`, `--insecure`, and `--upstream-plaintext` โ€” apply to every mapping, not per-rule.