diff --git a/internal/commands/api.go b/internal/commands/api.go index a2223e6a..6e58d382 100644 --- a/internal/commands/api.go +++ b/internal/commands/api.go @@ -3,6 +3,7 @@ package commands import ( "encoding/json" "fmt" + "net/url" "regexp" "strings" @@ -47,7 +48,10 @@ func newAPIGetCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL) + if err != nil { + return err + } resp, err := app.Account().Get(cmd.Context(), path) if err != nil { return convertSDKError(err) @@ -85,7 +89,10 @@ func newAPIPostCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL) + if err != nil { + return err + } // Parse JSON data var body any @@ -134,7 +141,10 @@ func newAPIPutCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL) + if err != nil { + return err + } // Parse JSON data var body any @@ -176,7 +186,10 @@ func newAPIDeleteCmd() *cobra.Command { return err } - path := parsePath(args[0]) + path, err := parsePath(args[0], app.Config.BaseURL) + if err != nil { + return err + } resp, err := app.Account().Delete(cmd.Context(), path) if err != nil { return convertSDKError(err) @@ -201,20 +214,70 @@ func apiPathArgs(cmd *cobra.Command, args []string) error { return nil } -// parsePath extracts and normalizes the API path. -// Handles full URLs and relative paths. The leading slash is stripped because -// the SDK's accountPath and buildURL both add one — keeping it here would -// double-slash and, on Windows, MSYS/Git Bash converts /path to C:\...\path. -func parsePath(input string) string { - urlPattern := regexp.MustCompile(`^https?://[^/]+/[0-9]+(/.*)`) - if matches := urlPattern.FindStringSubmatch(input); len(matches) > 1 { - return matches[1] +// accountSegmentPattern matches a leading // segment in an API +// path so it can be dropped — the SDK re-prefixes the configured account. +var accountSegmentPattern = regexp.MustCompile(`^/[0-9]+(/.*)$`) + +// parsePath normalizes the user-supplied API path against the configured base +// URL. It accepts relative paths ("projects.json", "/projects.json") and +// absolute Basecamp URLs whose host matches baseURL — from which the path is +// extracted and a leading / segment dropped (the SDK re-prefixes +// the configured account). Absolute URLs on ANY other host are rejected so the +// bearer token is never attached to a request bound for a foreign host. +// +// All leading slashes and a mixed-case scheme are normalized first so neither +// "//https://evil/…" nor "HTTPS://evil/…" can smuggle an absolute URL past the +// host check (URL schemes are case-insensitive per RFC 3986 §3.1). +func parsePath(input, baseURL string) (string, error) { + candidate := strings.TrimLeft(input, "/") + lower := strings.ToLower(candidate) + if strings.HasPrefix(lower, "http://") || strings.HasPrefix(lower, "https://") { + u, err := url.Parse(candidate) + if err != nil || u.Host == "" { + return "", output.ErrUsage("invalid API URL: " + input) + } + base, baseErr := url.Parse(baseURL) + if baseErr != nil || base.Host == "" || !sameHostPort(u, base) { + return "", output.ErrUsage("API path must be relative or a Basecamp URL on the configured host; refusing to send credentials to " + input) + } + // Same host: use the path (+ query), dropping a leading /. + path := u.EscapedPath() + if m := accountSegmentPattern.FindStringSubmatch(path); m != nil { + path = m[1] + } + if u.RawQuery != "" { + path += "?" + u.RawQuery + } + return path, nil } - // Strip leading slash — the SDK prefixes the account path. - input = strings.TrimPrefix(input, "/") + // Relative path — return without leading slashes; the SDK prefixes the + // account path (keeping one here would double-slash and, on Windows, + // MSYS/Git Bash converts /path to C:\...\path). + return candidate, nil +} - return input +// sameHostPort reports whether u and base address the same host: hostnames +// compared case-insensitively, ports compared after normalizing an omitted +// port to the scheme default. A matching hostname on a different port is +// still a different origin and must not receive credentials. +func sameHostPort(u, base *url.URL) bool { + if !strings.EqualFold(u.Hostname(), base.Hostname()) { + return false + } + return effectivePort(u) == effectivePort(base) +} + +// effectivePort returns the URL's explicit port, or the scheme's default +// (80 for http, 443 for https) when omitted. +func effectivePort(u *url.URL) string { + if port := u.Port(); port != "" { + return port + } + if strings.EqualFold(u.Scheme, "http") { + return "80" + } + return "443" } // apiSummary generates a summary from the API response. diff --git a/internal/commands/api_test.go b/internal/commands/api_test.go index 103ec1f0..d87c8085 100644 --- a/internal/commands/api_test.go +++ b/internal/commands/api_test.go @@ -12,6 +12,8 @@ import ( "github.com/basecamp/basecamp-cli/internal/output" ) +const testBaseURL = "https://3.basecampapi.com" + func TestParsePath(t *testing.T) { tests := []struct { input string @@ -21,13 +23,58 @@ func TestParsePath(t *testing.T) { {"/projects.json", "projects.json"}, {"buckets/123/todos/456.json", "buckets/123/todos/456.json"}, {"/buckets/123/todos/456.json", "buckets/123/todos/456.json"}, + // Same-host absolute URLs: extract the path, dropping the account segment. {"https://3.basecampapi.com/999/projects.json", "/projects.json"}, {"https://3.basecampapi.com/12345/buckets/1/todos/2.json", "/buckets/1/todos/2.json"}, + // Same host but no account segment — accepted, path used as-is. + {"https://3.basecampapi.com/projects.json", "/projects.json"}, + // Query strings are preserved. + {"https://3.basecampapi.com/999/projects.json?page=2", "/projects.json?page=2"}, + // Schemes are case-insensitive (RFC 3986): a same-host uppercase scheme + // still extracts the path. + {"HTTPS://3.basecampapi.com/999/projects.json", "/projects.json"}, + // An explicit default port is still the configured host. + {"https://3.basecampapi.com:443/999/projects.json", "/projects.json"}, } for _, tt := range tests { t.Run(tt.input, func(t *testing.T) { - assert.Equal(t, tt.want, parsePath(tt.input)) + got, err := parsePath(tt.input, testBaseURL) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestParsePathRejectsForeignHosts guards against bearer-token exfiltration: an +// absolute URL whose host differs from the configured base URL must be rejected +// before it reaches the SDK, which would otherwise attach the Authorization +// header and send credentials to the foreign host. Covers mixed-case schemes, +// leading-slash smuggling attempts, and a same-hostname non-default port. +func TestParsePathRejectsForeignHosts(t *testing.T) { + bad := []string{ + "https://evil.com/projects.json", + "http://evil.com/projects.json", + "https://evil.com/", + "https://evil.com", + "https://evil.example/999/projects.json", // foreign host, numeric form + "http://127.0.0.1:9999/projects.json", + "HTTPS://evil.example/projects.json", // uppercase scheme + "HtTpS://evil.example/projects.json", // mixed-case scheme + "/https://evil.example/projects.json", // leading-slash smuggling + "/HTTPS://evil.example/projects.json", // leading slash + uppercase + "//https://evil.example/projects.json", // double-slash smuggling + "///https://evil.example/x", // triple-slash smuggling + "https://3.basecampapi.com:8443/projects.json", // same hostname, non-default port + "https://3.basecampapi.com:443.evil.com/projects.json", // port-lookalike host smuggling + "https://3.basecampapi.com:abc/x", // non-numeric port + } + + for _, input := range bad { + t.Run(input, func(t *testing.T) { + got, err := parsePath(input, testBaseURL) + require.Error(t, err) + assert.Empty(t, got) }) } }