diff --git a/CHANGELOG.md b/CHANGELOG.md index 49b1b61..4855faa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,11 @@ Dates are the npm publish dates. The same notes are at https://shellroute.com/changelog. +## v0.2.1 + +- Proxy credentials are taken exactly as written: a password with `/`, `#`, `?`, `%`, spaces or `@` no longer needs percent-encoding. Entries saved by 0.2.0 keep working. +- When a proxy rejects the login, or the shell splits an unquoted URL, the error says what to do about it. + ## v0.2.0 (2026-10-21) Bring your own proxy. Route a command or the interactive shell through a proxy you already have, with no Shellroute account. diff --git a/docs/bring-your-own-proxy.md b/docs/bring-your-own-proxy.md index 8a9e4f5..e329438 100644 --- a/docs/bring-your-own-proxy.md +++ b/docs/bring-your-own-proxy.md @@ -41,9 +41,10 @@ shellroute upstream test https://USER:PASSWORD@HOST:443 | `socks5h://host:port` | the same as `socks5://` | - The port is required. -- Credentials go in the URL: `scheme://user:pass@host:port`. URL-encode special characters: `@` becomes `%40`, `:` becomes `%3A`, `/` becomes `%2F`, `#` becomes `%23`. +- Credentials go in the URL: `scheme://user:password@host:port`. Username and password are taken exactly as written, any character. Wrap the URL in single quotes when it contains spaces or shell characters such as `$`, `!`, `&`, `;` or `#`: `shellroute upstream add your-proxy-name 'http://USER:PASSWORD@HOST:PORT'`. +- Entries saved by versions before 0.2.1 hold percent-encoded passwords and keep working. - IPv6 hosts go in brackets: `http://[2001:db8::1]:8080`. -- Your provider's documentation gives the hostname, port and username format. Many providers select the country or a sticky session through the username; paste it into the URL as given, URL-encoded. +- Your provider's documentation gives the hostname, port and username format. Many providers select the country or a sticky session through the username; paste it into the URL as given. Saved names are 3 to 32 characters: lowercase letters, digits, `_` or `-`. A name can never look like a two-letter country code, so `shellroute run your-proxy-name` and `shellroute run DE` never collide. @@ -102,6 +103,7 @@ A `4xx` from the proxy for one target is forwarded to the client and does not co - `unknown upstream "your-proxy-name"`: nothing is saved under that alias. `shellroute upstream list` shows what is; `shellroute upstream add your-proxy-name ` saves it. - `proxy URL needs a port`: add `:8080`, `:1080` or whatever your provider gives. -- `proxy URL is not valid`: URL-encode special characters in the username or password (see above). +- `proxy URL is not valid`: the host part is wrong. The format is `scheme://user:password@host:port`. If the shell split the URL at a space or a symbol, wrap it in single quotes. +- the proxy rejects the username or password: compare them with what your provider gave you. A password percent-encoded for an older version is now taken as written; type the real characters. - `upstream test` passes but a program is not routed: the program does not read the proxy environment variables. See the [compatibility matrix](compatibility.md). - `upstream test` reports an unexpected country: the exit is the provider's choice. Check the username or port format in the provider's documentation. Geo-IP databases also misplace some addresses. diff --git a/internal/cli/run_exit_test.go b/internal/cli/run_exit_test.go index 93b3677..3fd94e2 100644 --- a/internal/cli/run_exit_test.go +++ b/internal/cli/run_exit_test.go @@ -10,6 +10,8 @@ import ( "syscall" "testing" "time" + + "github.com/shellroute/shellroute-cli/internal/proxy" ) // exitAsChild and exitFromSignal end the process, so each case runs in a @@ -57,6 +59,16 @@ func TestHelperProcess(t *testing.T) { os.Exit(98) } os.Exit(0) + case "run-own-url": + // `shellroute run --proxy -- sh -c 'exit 0'` through a fake proxy the + // parent test started; the exit IP probe goes to the parent's echo server. + proxy.ExitInfoEndpoints = []string{os.Getenv("SR_TEST_EXIT_IP_URL")} + rootCmd.SetArgs([]string{"run", "--proxy", os.Getenv("SR_TEST_PROXY_URL"), "--no-stat", + "--", "sh", "-c", "exit 0"}) + if err := rootCmd.Execute(); err != nil { + fmt.Println(err) + os.Exit(98) + } case "run-own-env": // SHELLROUTE_PROXY selects the route; the command must not see it. rootCmd.SetArgs([]string{"run", "--no-check", "--no-stat", diff --git a/internal/cli/run_upstream.go b/internal/cli/run_upstream.go index c034fc7..7c13d1d 100644 --- a/internal/cli/run_upstream.go +++ b/internal/cli/run_upstream.go @@ -76,7 +76,11 @@ func resolveRunTarget(cfg *config.Config, args []string, dashIdx int, proxyFlag, if proxyFlag != "" { if first != "" { - return runTarget{}, nil, errors.New("--proxy cannot be combined with a country or upstream name") + if _, saved := cfg.FindUpstream(first); isCountryWord(first) || saved { + return runTarget{}, nil, errors.New("--proxy cannot be combined with a country or upstream name") + } + // Any other word before -- is the tail of a URL the shell split. + return runTarget{}, nil, fmt.Errorf("--proxy: %v", proxy.ErrURLFormat) } up, err := proxy.ParseUpstreamURL(proxyFlag) if err != nil { @@ -100,7 +104,7 @@ func resolveRunTarget(cfg *config.Config, args []string, dashIdx int, proxyFlag, if !ok { return runTarget{}, nil, fmt.Errorf("unknown upstream %q. Save it with `shellroute upstream add %s ` or list saved ones with `shellroute upstream list`", first, first) } - up, err := proxy.ParseUpstreamURL(entry.URL) + up, err := proxy.ParseSavedUpstreamURL(entry.URL, entry.Literal) if err != nil { return runTarget{}, nil, fmt.Errorf("saved upstream %q has an invalid URL (%v). Remove it and add it again", first, err) } @@ -128,7 +132,7 @@ func resolveUpstreamRef(cfg *config.Config, ref string) (runTarget, error) { return runTarget{}, errors.New("upstream required") } if entry, ok := cfg.FindUpstream(ref); ok { - up, err := proxy.ParseUpstreamURL(entry.URL) + up, err := proxy.ParseSavedUpstreamURL(entry.URL, entry.Literal) if err != nil { return runTarget{}, fmt.Errorf("saved upstream %q has an invalid URL (%v). Remove it and add it again", ref, err) } @@ -188,7 +192,7 @@ func viaLine(sess *session.Session) string { // reportOwnStartError explains why the user's own proxy could not be used. func reportOwnStartError(t runTarget, err error) error { if proxy.TunnelFailed(err) { - display.Error("Cannot connect through %s: %s", t.describe(), proxy.Cause(err)) + display.Error("Cannot connect through %s: %s", t.describe(), proxy.Explain(err)) } else { display.Error("Cannot start the local proxy. Try again.") } diff --git a/internal/cli/upstream.go b/internal/cli/upstream.go index a96bd40..838dcbb 100644 --- a/internal/cli/upstream.go +++ b/internal/cli/upstream.go @@ -29,15 +29,17 @@ alias wherever a country code goes: Aliases are 3 to 32 characters: lowercase letters, digits, _ or -. URL schemes: http, https (TLS to the proxy), socks5, socks5h. The port is -required. Credentials go in the URL, URL-encoded. The child command only ever -sees the local proxy on 127.0.0.1; your proxy's password never reaches it. -Where the proxy exits is observed when a route starts, not declared.`, +required. Username and password go in the URL exactly as your provider gave +them; wrap the URL in single quotes when they contain spaces or symbols. The +child command only ever sees the local proxy on 127.0.0.1; your proxy's +password never reaches it. Where the proxy exits is observed when a route +starts, not declared.`, } var upstreamAddCmd = &cobra.Command{ Use: "add ", Short: "Save your own proxy under a name", - Args: cobra.ExactArgs(2), + Args: exactArgsQuoted(2, "shellroute upstream add your-proxy-name 'scheme://user:password@host:port'"), RunE: runUpstreamAdd, } @@ -58,10 +60,21 @@ var upstreamRemoveCmd = &cobra.Command{ var upstreamTestCmd = &cobra.Command{ Use: "test ", Short: "Open one tunnel through a proxy and report its exit IP", - Args: cobra.ExactArgs(1), + Args: exactArgsQuoted(1, "shellroute upstream test 'scheme://user:password@host:port'"), RunE: runUpstreamTest, } +// exactArgsQuoted is cobra.ExactArgs(n) that, given too many arguments, +// explains the likely cause: the shell split an unquoted URL. +func exactArgsQuoted(n int, example string) cobra.PositionalArgs { + return func(cmd *cobra.Command, args []string) error { + if len(args) > n { + return fmt.Errorf("too many arguments. %s: %s", proxy.QuoteHint, example) + } + return cobra.ExactArgs(n)(cmd, args) + } +} + func init() { upstreamAddCmd.Flags().StringVar(&upstreamAddNote, "note", "", "Free-text note") upstreamCmd.AddCommand(upstreamAddCmd, upstreamListCmd, upstreamRemoveCmd, upstreamTestCmd) @@ -79,7 +92,7 @@ func runUpstreamAdd(cmd *cobra.Command, args []string) error { if err != nil { return err } - replaced := cfg.AddUpstream(config.Upstream{Name: name, URL: strings.TrimSpace(rawURL), Note: upstreamAddNote}) + replaced := cfg.AddUpstream(config.Upstream{Name: name, URL: strings.TrimSpace(rawURL), Note: upstreamAddNote, Literal: true}) if err := config.Save(cfg); err != nil { return err } @@ -165,7 +178,7 @@ func runUpstreamTest(cmd *cobra.Command, args []string) error { display.Success("via %s · exit IP %s%s · %d ms", t.describe(), res.ExitIP, where, res.Latency.Milliseconds()) return nil case proxy.TunnelFailed(err): - display.Error("Cannot connect through %s: %s", t.describe(), proxy.Cause(err)) + display.Error("Cannot connect through %s: %s", t.describe(), proxy.Explain(err)) return errors.New("upstream unreachable") default: display.Warn("Tunnel open via %s, but the exit IP could not be read: %s", t.describe(), proxy.Cause(err)) diff --git a/internal/cli/upstream_literal_test.go b/internal/cli/upstream_literal_test.go new file mode 100644 index 0000000..bb468c4 --- /dev/null +++ b/internal/cli/upstream_literal_test.go @@ -0,0 +1,180 @@ +package cli + +import ( + "io" + "os" + "strings" + "testing" + + "github.com/shellroute/shellroute-cli/internal/config" + "github.com/shellroute/shellroute-cli/internal/proxy/proxytest" +) + +// literalPassword has every character the old rules made people encode. +const literalPassword = "p@ss/w#rd?100% x" + +const exitIPBody = `{"ip": "203.0.113.5", "country": "LT"}` + +// captureStderr returns what f wrote to stderr. +func captureStderr(t *testing.T, f func()) string { + t.Helper() + r, w, err := os.Pipe() + if err != nil { + t.Fatal(err) + } + old := os.Stderr + os.Stderr = w + f() + w.Close() + os.Stderr = old + b, _ := io.ReadAll(r) + return string(b) +} + +func TestUpstreamTest_LiteralPasswordHTTP(t *testing.T) { + withOwnConfig(t) + p := proxytest.StartHTTPProxy(t, "alice", literalPassword) + withExitIPEndpoints(t, ipEcho(t, exitIPBody).URL) + if _, err := execCLI(t, "upstream", "add", "work", "http://alice:"+literalPassword+"@"+p.Addr); err != nil { + t.Fatalf("add: %v", err) + } + if _, err := execCLI(t, "upstream", "test", "work"); err != nil { + t.Fatalf("test: %v", err) + } + if p.Requests.Load() == 0 || p.Rejected.Load() != 0 { + t.Errorf("proxy saw %d authenticated and %d rejected requests", p.Requests.Load(), p.Rejected.Load()) + } +} + +func TestUpstreamTest_LiteralPasswordSOCKS5ByURL(t *testing.T) { + withOwnConfig(t) + s := proxytest.StartSOCKS5(t, "alice", literalPassword) + withExitIPEndpoints(t, ipEcho(t, exitIPBody).URL) + if _, err := execCLI(t, "upstream", "test", "socks5://alice:"+literalPassword+"@"+s.Addr); err != nil { + t.Fatalf("test: %v", err) + } + if s.Connects.Load() == 0 || s.Rejected.Load() != 0 { + t.Errorf("proxy saw %d connects and %d rejected logins", s.Connects.Load(), s.Rejected.Load()) + } +} + +func TestUpstreamTest_LegacyEncodedEntryStillWorks(t *testing.T) { + withOwnConfig(t) + p := proxytest.StartHTTPProxy(t, "alice", "p@ss") + withExitIPEndpoints(t, ipEcho(t, exitIPBody).URL) + cfg, err := loadConfig() + if err != nil { + t.Fatal(err) + } + // Written by 0.2.0: percent-encoded, no literal marker. + cfg.AddUpstream(config.Upstream{Name: "old", URL: "http://alice:p%40ss@" + p.Addr}) + if err := config.Save(cfg); err != nil { + t.Fatal(err) + } + if _, err := execCLI(t, "upstream", "test", "old"); err != nil { + t.Fatalf("legacy entry: %v", err) + } + if p.Requests.Load() == 0 { + t.Error("the decoded password should have authenticated") + } +} + +func TestUpstreamAdd_SavesLiteralMarker(t *testing.T) { + withOwnConfig(t) + p := proxytest.StartHTTPProxy(t, "alice", "p%40ss") + withExitIPEndpoints(t, ipEcho(t, exitIPBody).URL) + if _, err := execCLI(t, "upstream", "add", "new", "http://alice:p%40ss@"+p.Addr); err != nil { + t.Fatal(err) + } + cfg, err := loadConfig() + if err != nil { + t.Fatal(err) + } + entry, ok := cfg.FindUpstream("new") + if !ok || !entry.Literal { + t.Fatalf("saved entry = %+v, want Literal true", entry) + } + if _, err := execCLI(t, "upstream", "test", "new"); err != nil { + t.Fatalf("literal %%40 password: %v", err) + } + if p.Requests.Load() == 0 { + t.Error("the literal password should have authenticated") + } +} + +func TestUpstreamAdd_TooManyArgsHintsQuoting(t *testing.T) { + withOwnConfig(t) + _, err := execCLI(t, "upstream", "add", "work", "http://u:pa", "ss@h:1") + if err == nil || !strings.Contains(err.Error(), "single quotes") { + t.Errorf("err = %v, want the quoting hint", err) + } + _, err = execCLI(t, "upstream", "test", "http://u:pa", "ss@h:1") + if err == nil || !strings.Contains(err.Error(), "single quotes") { + t.Errorf("test: err = %v, want the quoting hint", err) + } +} + +func TestRun_ProxyFlagSplitByShellHintsQuoting(t *testing.T) { + withOwnConfig(t) + _, err := execCLI(t, "run", "--proxy", "http://u:pa", "ss@h:1", "--", "echo") + if err == nil || !strings.Contains(err.Error(), "single quotes") { + t.Errorf("err = %v, want the quoting hint", err) + } +} + +func TestUpstreamTest_RejectedCredentialsShowHints(t *testing.T) { + withOwnConfig(t) + p := proxytest.StartHTTPProxy(t, "alice", "right") + withExitIPEndpoints(t, "http://127.0.0.1:1/ip") + if _, err := execCLI(t, "upstream", "add", "work", "http://alice:wr%41ng@"+p.Addr); err != nil { + t.Fatal(err) + } + out := captureStderr(t, func() { + if _, err := execCLI(t, "upstream", "test", "work"); err == nil { + t.Error("expected a failure") + } + }) + for _, want := range []string{"rejected credentials", "single quotes", "percent-encoded"} { + if !strings.Contains(out, want) { + t.Errorf("stderr lacks %q:\n%s", want, out) + } + } + if strings.Contains(out, "wr%41ng") { + t.Errorf("stderr shows the password:\n%s", out) + } +} + +func TestRunOwn_LiteralPasswordEndToEnd(t *testing.T) { + for _, scheme := range []string{"http", "socks5"} { + t.Run(scheme, func(t *testing.T) { + var addr string + var authenticated func() int32 + if scheme == "http" { + p := proxytest.StartHTTPProxy(t, "alice", literalPassword) + addr, authenticated = p.Addr, p.Requests.Load + } else { + s := proxytest.StartSOCKS5(t, "alice", literalPassword) + addr, authenticated = s.Addr, s.Connects.Load + } + echo := ipEcho(t, exitIPBody) + res := runHelper(t, false, + "SR_TEST_HELPER=run-own-url", + "SHELLROUTE_HOME="+t.TempDir(), + "SHELLROUTE_API_KEY=", + "SHELLROUTE_PROXY=", + "SHELLROUTE_API_URL=http://127.0.0.1:1", + "SR_TEST_PROXY_URL="+scheme+"://alice:"+literalPassword+"@"+addr, + "SR_TEST_EXIT_IP_URL="+echo.URL, + ) + if res.sig != 0 || res.code != 0 { + t.Fatalf("exit %d, signal %v:\n%s", res.code, res.sig, res.out) + } + if authenticated() == 0 { + t.Error("the proxy never saw an authenticated request") + } + if strings.Contains(res.out, literalPassword) { + t.Errorf("output shows the password:\n%s", res.out) + } + }) + } +} diff --git a/internal/config/upstream.go b/internal/config/upstream.go index 6cb47e4..d78ed63 100644 --- a/internal/config/upstream.go +++ b/internal/config/upstream.go @@ -13,6 +13,10 @@ type Upstream struct { Name string `toml:"name"` URL string `toml:"url"` Note string `toml:"note,omitempty"` + // Literal marks an entry saved by 0.2.1 or later: the username and + // password in URL are exactly as typed. Older entries hold them + // percent-encoded and are decoded when read. + Literal bool `toml:"literal,omitempty"` } // Upstream names are 3 to 32 characters of lowercase letters, digits, diff --git a/internal/proxy/upstream.go b/internal/proxy/upstream.go index 22662b1..eabb1d2 100644 --- a/internal/proxy/upstream.go +++ b/internal/proxy/upstream.go @@ -72,6 +72,7 @@ type DialError struct { Status int Msg string Err error + Hint string // what the user can do about it; set for own-proxy credential failures } func (e *DialError) Error() string { @@ -121,6 +122,16 @@ func Cause(err error) string { } } +// Explain is Cause followed by the hint attached to the error, if any. +func Explain(err error) string { + cause := Cause(err) + var de *DialError + if errors.As(err, &de) && de.Hint != "" { + return cause + ". " + de.Hint + } + return cause +} + // netCause names the network reason behind a dial failure. func netCause(err error) string { if err == nil { diff --git a/internal/proxy/upstream_http.go b/internal/proxy/upstream_http.go index 5c43463..7bfb522 100644 --- a/internal/proxy/upstream_http.go +++ b/internal/proxy/upstream_http.go @@ -18,6 +18,7 @@ type HTTPUpstream struct { useTLS bool tlsConfig *tls.Config // nil: verify against system roots auth string // base64 "user:pass", "" when the proxy takes no credentials + credHint string // attached to a credentials rejection; set by ParseUpstreamURL label string hosted bool // interpret the hosted gateway's extra replies timeout time.Duration @@ -117,7 +118,11 @@ func (u *HTTPUpstream) DialTarget(ctx context.Context, hostPort string) (net.Con return nil, &DialError{Kind: KindDepleted, Status: resp.StatusCode} case resp.StatusCode == http.StatusProxyAuthRequired: conn.Close() - return nil, &DialError{Kind: KindCredentials, Status: resp.StatusCode} + de := &DialError{Kind: KindCredentials, Status: resp.StatusCode, Hint: u.credHint} + if u.auth == "" { + de.Msg = "proxy requires a username and password" + } + return nil, de case u.hosted && resp.Header.Get("X-Route-Dead") == "true": conn.Close() return nil, &DialError{Kind: KindRouteDead, Status: resp.StatusCode} diff --git a/internal/proxy/upstream_socks5.go b/internal/proxy/upstream_socks5.go index a65d3fe..0a9888e 100644 --- a/internal/proxy/upstream_socks5.go +++ b/internal/proxy/upstream_socks5.go @@ -29,6 +29,7 @@ type SOCKS5Upstream struct { username string password string label string + credHint string // attached to a credentials rejection; set by ParseUpstreamURL } // NewSOCKS5Upstream returns an upstream for a SOCKS5 proxy. label is shown in @@ -86,7 +87,7 @@ func (u *SOCKS5Upstream) handshake(conn net.Conn, host string, port int) error { case socksNoAuth: case socksUserPass: if !wantAuth { - return &DialError{Kind: KindCredentials, Msg: "proxy requires a username and password"} + return &DialError{Kind: KindCredentials, Msg: "proxy requires a username and password", Hint: u.credHint} } if err := u.authenticate(conn); err != nil { return err @@ -95,7 +96,7 @@ func (u *SOCKS5Upstream) handshake(conn net.Conn, host string, port int) error { if wantAuth { return &DialError{Kind: KindHandshake, Msg: "proxy accepts neither anonymous nor username/password authentication"} } - return &DialError{Kind: KindCredentials, Msg: "proxy requires a username and password"} + return &DialError{Kind: KindCredentials, Msg: "proxy requires a username and password", Hint: u.credHint} default: return &DialError{Kind: KindHandshake, Msg: fmt.Sprintf("proxy chose unsupported authentication method %d", reply[1])} } @@ -177,7 +178,7 @@ func (u *SOCKS5Upstream) authenticate(conn net.Conn) error { return handshakeError("proxy did not answer the authentication request", err) } if reply[1] != 0x00 { - return &DialError{Kind: KindCredentials, Status: int(reply[1])} + return &DialError{Kind: KindCredentials, Status: int(reply[1]), Hint: u.credHint} } return nil } diff --git a/internal/proxy/upstream_test.go b/internal/proxy/upstream_test.go index 1b4821c..35c6545 100644 --- a/internal/proxy/upstream_test.go +++ b/internal/proxy/upstream_test.go @@ -406,95 +406,6 @@ func TestCause_PlainErrorPassesThrough(t *testing.T) { } } -// --- URL parsing --- - -func TestParseUpstreamURL(t *testing.T) { - cases := []struct { - raw string - wantType string - wantErr string - }{ - {"http://user:pass@proxy.example:8080", "*proxy.HTTPUpstream", ""}, - {"HTTPS://proxy.example:443", "*proxy.HTTPUpstream", ""}, - {"socks5://proxy.example:1080", "*proxy.SOCKS5Upstream", ""}, - {"socks5h://u:p@[::1]:1080", "*proxy.SOCKS5Upstream", ""}, - {"", "", "empty"}, - {"proxy.example:8080", "", "needs a scheme"}, - {"ftp://proxy.example:21", "", "unsupported proxy scheme"}, - {"http://proxy.example", "", "needs a port"}, - {"http://:8080", "", "needs a host"}, - {"http://proxy.example:8080/path", "", "must not have a path"}, - {"http://proxy.example:8080?x=1", "", "must not have a path"}, - {"http://user:pa ss@host:1", "", "not valid"}, - } - for _, tc := range cases { - t.Run(tc.raw, func(t *testing.T) { - up, err := ParseUpstreamURL(tc.raw) - if tc.wantErr != "" { - if err == nil || !strings.Contains(err.Error(), tc.wantErr) { - t.Fatalf("err = %v, want containing %q", err, tc.wantErr) - } - return - } - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if got := typeName(up); got != tc.wantType { - t.Errorf("type = %s, want %s", got, tc.wantType) - } - }) - } -} - -func TestParseUpstreamURL_DecodesCredentialsAndRedactsLabel(t *testing.T) { - up, err := ParseUpstreamURL("http://al%40ice:p%3Ass@proxy.example:8080") - if err != nil { - t.Fatal(err) - } - h := up.(*HTTPUpstream) - if h.addr != "proxy.example:8080" { - t.Errorf("addr = %q", h.addr) - } - if h.ProxyAuthorization() != "Basic YWxAaWNlOnA6c3M=" { // base64("al@ice:p:ss") - t.Errorf("auth = %q", h.ProxyAuthorization()) - } - if strings.Contains(up.Describe(), "ss") || !strings.Contains(up.Describe(), "***") { - t.Errorf("Describe leaks the password: %q", up.Describe()) - } -} - -func TestParseUpstreamURL_ErrorsNeverContainPassword(t *testing.T) { - for _, raw := range []string{ - "ftp://user:topsecret@h:1", - "http://user:topsecret@h", - "http://user:topsecret@h:1/path", - } { - _, err := ParseUpstreamURL(raw) - if err == nil { - t.Fatalf("%s: expected error", raw) - } - if strings.Contains(err.Error(), "topsecret") { - t.Errorf("error leaks password: %v", err) - } - } -} - -func TestRedactURL(t *testing.T) { - cases := map[string]string{ - "http://user:secret@h:8080": "http://user:***@h:8080", - "socks5://h:1080": "socks5://h:1080", - "http://user@h:8080": "http://user@h:8080", - "https://u:p@[::1]:443": "https://u:***@[::1]:443", - "not a url": "(invalid proxy URL)", - "http://user:se%40cret@h:80": "http://user:***@h:80", - } - for in, want := range cases { - if got := RedactURL(in); got != want { - t.Errorf("RedactURL(%q) = %q, want %q", in, got, want) - } - } -} - // --- Origin-form rewrite for SOCKS5 plain HTTP --- func TestOriginForm(t *testing.T) { diff --git a/internal/proxy/upstream_url.go b/internal/proxy/upstream_url.go index 54e6877..6e76dc3 100644 --- a/internal/proxy/upstream_url.go +++ b/internal/proxy/upstream_url.go @@ -5,75 +5,179 @@ import ( "fmt" "net" "net/url" + "regexp" "strings" ) -// ParseUpstreamURL builds an Upstream from a proxy URL such as -// http://user:pass@host:8080, https://host:443 or socks5://user:pass@host:1080. -// The port is required. Credentials go in the userinfo part, URL-encoded. -// Returned errors never contain the password. +// Hints shown with a failure, so nobody has to know the URL rules upfront. +// They never contain any part of the URL. +const ( + // QuoteHint explains the one thing the CLI cannot fix itself: the shell + // splitting or expanding an unquoted URL before the CLI sees it. + QuoteHint = "If the password has spaces or symbols such as $ or !, wrap the URL in single quotes" + // encodedHint is attached when the proxy rejects credentials that contain + // a %XX sequence: they may have been percent-encoded for an older version. + encodedHint = "If you percent-encoded the password, write the real characters instead" + // noCredentialsHint is attached when a proxy asked for credentials the URL + // did not carry. + noCredentialsHint = "Add them to the URL: scheme://user:password@host:port" + // socksCredentialLength is RFC 1929's limit, checked before anything is sent. + socksCredentialLength = "SOCKS5 username and password must be at most 255 bytes" +) + +// ErrURLFormat is returned when the host part of a proxy URL does not parse. +// Credentials are taken as written, so a bad host part is the only way to get +// here, typically after the shell cut an unquoted URL. +var ErrURLFormat = errors.New("proxy URL is not valid: expected scheme://user:password@host:port. " + QuoteHint) + +// schemeRe matches a URL scheme (RFC 3986). Anything else before "://" is +// never echoed back: it could be a misplaced password. +var schemeRe = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9+.-]*$`) + +// percentRe matches one percent-encoded byte. +var percentRe = regexp.MustCompile(`%[0-9A-Fa-f]{2}`) + +// ParseUpstreamURL builds an Upstream from scheme://[user[:password]@]host:port +// as typed now. The username and password are taken byte for byte, whatever +// they contain; the text before the last "@" is the credentials, split once +// at the first ":". The port is required. Returned errors never contain the +// password. func ParseUpstreamURL(raw string) (Upstream, error) { + return parseUpstreamURL(raw, false) +} + +// ParseSavedUpstreamURL builds an Upstream from a saved entry. Entries saved +// before 0.2.1 (literal false) hold percent-encoded credentials and are +// decoded; entries saved since (literal true) are taken as written. +func ParseSavedUpstreamURL(raw string, literal bool) (Upstream, error) { + return parseUpstreamURL(raw, !literal) +} + +func parseUpstreamURL(raw string, encoded bool) (Upstream, error) { raw = strings.TrimSpace(raw) if raw == "" { return nil, errors.New("proxy URL is empty") } - if !strings.Contains(raw, "://") { + scheme, rest, ok := strings.Cut(raw, "://") + if !ok { return nil, errors.New("proxy URL needs a scheme: http, https, socks5 or socks5h") } - u, err := url.Parse(raw) - if err != nil { - return nil, errors.New("proxy URL is not valid; URL-encode special characters in the username and password") - } - scheme := strings.ToLower(u.Scheme) + scheme = strings.ToLower(scheme) switch scheme { case "http", "https", "socks5", "socks5h": default: - return nil, fmt.Errorf("unsupported proxy scheme %q: use http, https, socks5 or socks5h", u.Scheme) + if schemeRe.MatchString(scheme) { + return nil, fmt.Errorf("unsupported proxy scheme %q: use http, https, socks5 or socks5h", scheme) + } + return nil, errors.New("unsupported proxy scheme: use http, https, socks5 or socks5h") + } + + creds, hostport, hasCreds := cutLast(rest, "@") + var user, pass string + if hasCreds { + user, pass, _ = strings.Cut(creds, ":") + if encoded { + user, pass = legacyDecode(user), legacyDecode(pass) + } + } + + host, port, err := parseHostPort(scheme, hostport) + if err != nil { + return nil, err } - host := u.Hostname() - port := u.Port() + + socks := scheme == "socks5" || scheme == "socks5h" + if socks && (len(user) > 255 || len(pass) > 255) { + return nil, errors.New(socksCredentialLength) + } + + addr := net.JoinHostPort(host, port) + label := RedactURL(raw) + hint := credentialHint(hasCreds, !encoded && percentRe.MatchString(creds)) + if socks { + up := NewSOCKS5Upstream(addr, user, pass, label) + up.credHint = hint + return up, nil + } + up := NewHTTPUpstream(addr, scheme == "https", user, pass, label) + up.credHint = hint + return up, nil +} + +// parseHostPort validates the part after the credentials: a host and a port, +// nothing else. +func parseHostPort(scheme, hostport string) (host, port string, err error) { + u, err := url.Parse(scheme + "://" + hostport) + if err != nil { + return "", "", ErrURLFormat + } + host, port = u.Hostname(), u.Port() if host == "" { - return nil, errors.New("proxy URL needs a host") + return "", "", errors.New("proxy URL needs a host") } if port == "" { - return nil, errors.New("proxy URL needs a port, for example http://proxy.example.com:8080") + return "", "", errors.New("proxy URL needs a port, for example http://proxy.example.com:8080") } if (u.Path != "" && u.Path != "/") || u.RawQuery != "" || u.Fragment != "" { - return nil, errors.New("proxy URL must not have a path, query or fragment") + return "", "", errors.New("proxy URL must not have a path, query or fragment") } + return host, port, nil +} - user := u.User.Username() - pass, _ := u.User.Password() - addr := net.JoinHostPort(host, port) - label := RedactURL(raw) +// credentialHint is attached to a credentials rejection from this upstream. +func credentialHint(hasCreds, looksEncoded bool) string { + if !hasCreds { + return noCredentialsHint + } + if looksEncoded { + return QuoteHint + ". " + encodedHint + } + return QuoteHint +} - switch scheme { - case "http": - return NewHTTPUpstream(addr, false, user, pass, label), nil - case "https": - return NewHTTPUpstream(addr, true, user, pass, label), nil - default: - return NewSOCKS5Upstream(addr, user, pass, label), nil +// legacyDecode undoes the percent-encoding that versions before 0.2.1 asked +// for. A value that does not decode is kept as written. +func legacyDecode(v string) string { + if d, err := url.PathUnescape(v); err == nil { + return d } + return v +} + +// cutLast splits s at the last occurrence of sep. +func cutLast(s, sep string) (before, after string, found bool) { + i := strings.LastIndex(s, sep) + if i < 0 { + return "", s, false + } + return s[:i], s[i+len(sep):], true } // RedactURL returns scheme://user:***@host:port for a proxy URL, hiding the -// password. A URL that does not parse is replaced entirely so nothing leaks. +// password. It splits the text the same way as ParseUpstreamURL, so no byte +// of the password can leak whatever the password contains. Text that is not +// shaped like a proxy URL, including one cut short before its host part, is +// replaced entirely. func RedactURL(raw string) string { - u, err := url.Parse(strings.TrimSpace(raw)) - if err != nil || u.Scheme == "" || u.Host == "" { + scheme, rest, ok := strings.Cut(strings.TrimSpace(raw), "://") + if !ok || !schemeRe.MatchString(scheme) { + return "(invalid proxy URL)" + } + creds, hostport, hasCreds := cutLast(rest, "@") + if _, _, err := parseHostPort(strings.ToLower(scheme), hostport); err != nil { return "(invalid proxy URL)" } var b strings.Builder - b.WriteString(strings.ToLower(u.Scheme)) + b.WriteString(strings.ToLower(scheme)) b.WriteString("://") - if u.User != nil { - b.WriteString(url.User(u.User.Username()).String()) - if _, has := u.User.Password(); has { + if hasCreds { + user, _, hasPass := strings.Cut(creds, ":") + b.WriteString(user) + if hasPass { b.WriteString(":***") } b.WriteString("@") } - b.WriteString(u.Host) + b.WriteString(hostport) return b.String() } diff --git a/internal/proxy/upstream_url_test.go b/internal/proxy/upstream_url_test.go new file mode 100644 index 0000000..cddfd20 --- /dev/null +++ b/internal/proxy/upstream_url_test.go @@ -0,0 +1,320 @@ +package proxy + +import ( + "context" + "encoding/base64" + "strings" + "testing" + + "github.com/shellroute/shellroute-cli/internal/proxy/proxytest" +) + +// literalPasswords must reach the proxy byte for byte. +var literalPasswords = []string{ + "p@ss", "p:ss", "p/ss", "p#ss", "p?ss", "pa ss", `p"ss`, "pässwörd", + "100%", "%40", "%zz", "p@ss/w#rd?100% x", +} + +// credentials returns the username and password an upstream will send. +func credentials(t *testing.T, up Upstream) (string, string) { + t.Helper() + switch u := up.(type) { + case *HTTPUpstream: + if u.auth == "" { + return "", "" + } + raw, err := base64.StdEncoding.DecodeString(u.auth) + if err != nil { + t.Fatalf("auth is not base64: %v", err) + } + user, pass, _ := strings.Cut(string(raw), ":") + return user, pass + case *SOCKS5Upstream: + return u.username, u.password + } + t.Fatalf("unexpected upstream %T", up) + return "", "" +} + +func upstreamAddr(up Upstream) string { + switch u := up.(type) { + case *HTTPUpstream: + return u.addr + case *SOCKS5Upstream: + return u.addr + } + return "" +} + +func hintOf(up Upstream) string { + switch u := up.(type) { + case *HTTPUpstream: + return u.credHint + case *SOCKS5Upstream: + return u.credHint + } + return "" +} + +func TestParseUpstreamURL_CredentialsAreLiteral(t *testing.T) { + for _, scheme := range []string{"http", "https", "socks5", "socks5h"} { + for _, pass := range literalPasswords { + raw := scheme + "://user:" + pass + "@proxy.example:1080" + t.Run(raw, func(t *testing.T) { + up, err := ParseUpstreamURL(raw) + if err != nil { + t.Fatalf("parse: %v", err) + } + user, got := credentials(t, up) + if user != "user" || got != pass { + t.Errorf("credentials = %q / %q, want user / %q", user, got, pass) + } + if addr := upstreamAddr(up); addr != "proxy.example:1080" { + t.Errorf("addr = %q", addr) + } + want := scheme + "://user:***@proxy.example:1080" + if d := up.Describe(); d != want || strings.Contains(d, pass) { + t.Errorf("Describe = %q, want %q", d, want) + } + if r := RedactURL(raw); r != want || strings.Contains(r, pass) { + t.Errorf("RedactURL = %q, want %q", r, want) + } + }) + } + } +} + +func TestParseUpstreamURL_Shapes(t *testing.T) { + cases := []struct{ raw, user, pass, addr, label string }{ + {"http://user@h:1", "user", "", "h:1", "http://user@h:1"}, + {"http://user:@h:1", "user", "", "h:1", "http://user:***@h:1"}, + {"http://h:1", "", "", "h:1", "http://h:1"}, + {"http://u:p@[2001:db8::1]:8080", "u", "p", "[2001:db8::1]:8080", "http://u:***@[2001:db8::1]:8080"}, + {"http://a@b:c@d@h:1", "a@b", "c@d", "h:1", "http://a@b:***@h:1"}, + {" HTTP://u:p@h:1 ", "u", "p", "h:1", "http://u:***@h:1"}, + {"http://u:" + strings.Repeat("x", 300) + "@h:1", "u", strings.Repeat("x", 300), "h:1", "http://u:***@h:1"}, + } + for _, tc := range cases { + t.Run(tc.raw, func(t *testing.T) { + up, err := ParseUpstreamURL(tc.raw) + if err != nil { + t.Fatalf("parse: %v", err) + } + user, pass := credentials(t, up) + if user != tc.user || pass != tc.pass { + t.Errorf("credentials = %q / %q, want %q / %q", user, pass, tc.user, tc.pass) + } + if got := upstreamAddr(up); got != tc.addr { + t.Errorf("addr = %q, want %q", got, tc.addr) + } + if got := up.Describe(); got != tc.label { + t.Errorf("Describe = %q, want %q", got, tc.label) + } + }) + } +} + +func TestParseUpstreamURL_Errors(t *testing.T) { + cases := map[string]string{ + "": "empty", + "proxy.example:8080": "needs a scheme", + "ftp://u:topsecret@h:21": `unsupported proxy scheme "ftp"`, + "u:topsecret@http://h:1": "unsupported proxy scheme: use", + "http://u:topsecret@h": "needs a port", + "http://u:topsecret@:8080": "needs a host", + "http://u:topsecret@": "needs a host", + "http://u:topsecret@h:1/x": "must not have a path", + "http://u:topsecret@h:1?x=1": "must not have a path", + "http://u:topsecret@h:1#x": "must not have a path", + "http://u:topsecret@h:1 x": "single quotes", + "http://u:topsecret": "single quotes", + "socks5://u:" + strings.Repeat("x", 256) + "@h:1": "255 bytes", + } + for raw, want := range cases { + _, err := ParseUpstreamURL(raw) + if err == nil || !strings.Contains(err.Error(), want) { + t.Errorf("%q: err = %v, want containing %q", raw, err, want) + continue + } + if strings.Contains(err.Error(), "topsecret") || strings.Contains(err.Error(), "xxxx") { + t.Errorf("%q: error echoes the credentials: %v", raw, err) + } + } +} + +func TestRedactURL_NeverLeaksAnyPasswordShape(t *testing.T) { + for _, pass := range append(literalPasswords, "top/secret@evil", "s://ecret") { + for _, raw := range []string{ + "http://user:" + pass + "@h:1", + "socks5://user:" + pass + "@[::1]:1080", + "http://user:" + pass, // host part missing + "user:" + pass + "@http://h:1", + } { + if got := RedactURL(raw); strings.Contains(got, pass) { + t.Errorf("RedactURL(%q) = %q leaks the password", raw, got) + } + } + } +} + +func TestParseSavedUpstreamURL_LegacyEntriesAreDecoded(t *testing.T) { + raw := "http://al%40ice:p%3Ass@proxy.example:8080" + up, err := ParseSavedUpstreamURL(raw, false) + if err != nil { + t.Fatal(err) + } + if user, pass := credentials(t, up); user != "al@ice" || pass != "p:ss" { + t.Errorf("legacy entry decoded to %q / %q", user, pass) + } + up, err = ParseSavedUpstreamURL(raw, true) + if err != nil { + t.Fatal(err) + } + if user, pass := credentials(t, up); user != "al%40ice" || pass != "p%3Ass" { + t.Errorf("literal entry changed to %q / %q", user, pass) + } + up, err = ParseSavedUpstreamURL("http://u:100%@h:1", false) + if err != nil { + t.Fatal(err) + } + if _, pass := credentials(t, up); pass != "100%" { + t.Errorf("undecodable legacy password became %q", pass) + } +} + +func TestParseUpstreamURL_CredentialHints(t *testing.T) { + cases := map[string]string{ + "http://u:p@h:1": QuoteHint, + "http://u:p%41@h:1": encodedHint, + "http://h:1": noCredentialsHint, + } + for raw, want := range cases { + up, err := ParseUpstreamURL(raw) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(hintOf(up), want) { + t.Errorf("%q: hint = %q, want containing %q", raw, hintOf(up), want) + } + } + up, _ := ParseSavedUpstreamURL("http://u:p%41@h:1", false) + if strings.Contains(hintOf(up), encodedHint) { + t.Error("a legacy entry was percent-encoded on purpose; no encoding hint") + } +} + +func TestExplain_CredentialsRejectedCarriesHint(t *testing.T) { + p := proxytest.StartHTTPProxy(t, "alice", "right") + up, err := ParseUpstreamURL("http://alice:wr%41ng@" + p.Addr) + if err != nil { + t.Fatal(err) + } + _, err = up.DialTarget(context.Background(), "203.0.113.1:443") + if err == nil { + t.Fatal("expected a credentials rejection") + } + got := Explain(err) + for _, want := range []string{"proxy rejected credentials", QuoteHint, encodedHint} { + if !strings.Contains(got, want) { + t.Errorf("Explain = %q, want containing %q", got, want) + } + } + if strings.Contains(got, "wr%41ng") || strings.Contains(got, "wrAng") { + t.Errorf("Explain leaks the password: %q", got) + } + + s := proxytest.StartSOCKS5(t, "alice", "right") + up, _ = ParseUpstreamURL("socks5://" + s.Addr) + _, err = up.DialTarget(context.Background(), "203.0.113.1:443") + if err == nil || !strings.Contains(Explain(err), noCredentialsHint) { + t.Errorf("SOCKS5 without credentials: Explain = %q", Explain(err)) + } +} + +func TestParseUpstreamURL(t *testing.T) { + cases := []struct { + raw string + wantType string + wantErr string + }{ + {"http://user:pass@proxy.example:8080", "*proxy.HTTPUpstream", ""}, + {"HTTPS://proxy.example:443", "*proxy.HTTPUpstream", ""}, + {"socks5://proxy.example:1080", "*proxy.SOCKS5Upstream", ""}, + {"socks5h://u:p@[::1]:1080", "*proxy.SOCKS5Upstream", ""}, + {"", "", "empty"}, + {"proxy.example:8080", "", "needs a scheme"}, + {"ftp://proxy.example:21", "", "unsupported proxy scheme"}, + {"http://proxy.example", "", "needs a port"}, + {"http://:8080", "", "needs a host"}, + {"http://proxy.example:8080/path", "", "must not have a path"}, + {"http://proxy.example:8080?x=1", "", "must not have a path"}, + {"http://user:pa ss@host:1", "*proxy.HTTPUpstream", ""}, + {"http://host:1 x", "", "not valid"}, + } + for _, tc := range cases { + t.Run(tc.raw, func(t *testing.T) { + up, err := ParseUpstreamURL(tc.raw) + if tc.wantErr != "" { + if err == nil || !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("err = %v, want containing %q", err, tc.wantErr) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := typeName(up); got != tc.wantType { + t.Errorf("type = %s, want %s", got, tc.wantType) + } + }) + } +} + +func TestParseSavedUpstreamURL_LegacyDecodesCredentialsAndRedactsLabel(t *testing.T) { + up, err := ParseSavedUpstreamURL("http://al%40ice:p%3Ass@proxy.example:8080", false) + if err != nil { + t.Fatal(err) + } + h := up.(*HTTPUpstream) + if h.addr != "proxy.example:8080" { + t.Errorf("addr = %q", h.addr) + } + if h.ProxyAuthorization() != "Basic YWxAaWNlOnA6c3M=" { // base64("al@ice:p:ss") + t.Errorf("auth = %q", h.ProxyAuthorization()) + } + if strings.Contains(up.Describe(), "ss") || !strings.Contains(up.Describe(), "***") { + t.Errorf("Describe leaks the password: %q", up.Describe()) + } +} + +func TestParseUpstreamURL_ErrorsNeverContainPassword(t *testing.T) { + for _, raw := range []string{ + "ftp://user:topsecret@h:1", + "http://user:topsecret@h", + "http://user:topsecret@h:1/path", + } { + _, err := ParseUpstreamURL(raw) + if err == nil { + t.Fatalf("%s: expected error", raw) + } + if strings.Contains(err.Error(), "topsecret") { + t.Errorf("error leaks password: %v", err) + } + } +} + +func TestRedactURL(t *testing.T) { + cases := map[string]string{ + "http://user:secret@h:8080": "http://user:***@h:8080", + "socks5://h:1080": "socks5://h:1080", + "http://user@h:8080": "http://user@h:8080", + "https://u:p@[::1]:443": "https://u:***@[::1]:443", + "not a url": "(invalid proxy URL)", + "http://user:se%40cret@h:80": "http://user:***@h:80", + } + for in, want := range cases { + if got := RedactURL(in); got != want { + t.Errorf("RedactURL(%q) = %q, want %q", in, got, want) + } + } +} diff --git a/internal/session/control_own.go b/internal/session/control_own.go index 470e0b0..cd0ba21 100644 --- a/internal/session/control_own.go +++ b/internal/session/control_own.go @@ -17,7 +17,7 @@ func (c *Controller) httpConnectOwn(w http.ResponseWriter, name string) { fmt.Fprintf(w, "ERROR Unknown upstream %q. Save it with: shellroute upstream add %s \n", name, name) return } - up, err := proxy.ParseUpstreamURL(entry.URL) + up, err := proxy.ParseSavedUpstreamURL(entry.URL, entry.Literal) if err != nil { w.WriteHeader(400) fmt.Fprintf(w, "ERROR Saved upstream %q has an invalid URL. Remove it and add it again.\n", name) @@ -32,7 +32,7 @@ func (c *Controller) httpConnectOwn(w http.ResponseWriter, name string) { sess, err := StartOwn(context.Background(), up, name, 0, StartOpts{}) if err != nil { if proxy.TunnelFailed(err) { - fmt.Fprintf(w, "ERROR Cannot connect through %s: %s\n", name, proxy.Cause(err)) + fmt.Fprintf(w, "ERROR Cannot connect through %s: %s\n", name, proxy.Explain(err)) } else { fmt.Fprintln(w, "ERROR Cannot start the local proxy. Try again.") } diff --git a/internal/session/control_own_literal_test.go b/internal/session/control_own_literal_test.go new file mode 100644 index 0000000..ffe8fcf --- /dev/null +++ b/internal/session/control_own_literal_test.go @@ -0,0 +1,31 @@ +package session + +import ( + "strings" + "testing" + + "github.com/shellroute/shellroute-cli/internal/api" + "github.com/shellroute/shellroute-cli/internal/config" + "github.com/shellroute/shellroute-cli/internal/proxy/proxytest" +) + +func TestConnectOwn_LiteralPasswordEntry(t *testing.T) { + t.Setenv("SHELLROUTE_HOME", t.TempDir()) + p := proxytest.StartHTTPProxy(t, "u", "p@ss/w#rd?100% x") + cfg := &config.Config{} + cfg.AddUpstream(config.Upstream{Name: "work", URL: "http://u:p@ss/w#rd?100% x@" + p.Addr, Literal: true}) + ctrl := NewController(api.New("http://127.0.0.1:1", ""), cfg) + withExitIPEndpoints(t, echoIP(t, `{"ip": "203.0.113.4", "country": "DE"}`).URL) + t.Cleanup(func() { ctrl.StopAndDisconnect() }) + + code, body := connectOwn(t, ctrl, "work") + if code != 200 { + t.Fatalf("code %d: %s", code, body) + } + if p.Requests.Load() == 0 || p.Rejected.Load() != 0 { + t.Errorf("proxy saw %d authenticated and %d rejected requests", p.Requests.Load(), p.Rejected.Load()) + } + if strings.Contains(body, "w#rd") { + t.Errorf("exports contain the password:\n%s", body) + } +}