From 10057f5d4e8f3781b9275fb79de4666b34f055d3 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Fri, 29 May 2026 22:55:22 -0700 Subject: [PATCH] Reject foreign-host URLs in api command to prevent token leak MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit parsePath now validates absolute URLs against the configured base URL host: a path is accepted only if it's relative or an absolute Basecamp URL on the same host (path extracted, account segment dropped — the SDK re-prefixes the configured account). Absolute URLs on any other host are rejected before any network call, so the SDK never attaches the Authorization: Bearer header to a foreign host. A stray leading slash ('/https://evil/…') and mixed-case schemes ('HTTPS://…') are normalized first so neither can smuggle an absolute URL past the host check. Error propagated through get/post/put/delete; api_test.go covers same-host extraction, query preservation, and foreign-host rejection. --- internal/commands/api.go | 115 +++++++++++++++++++++++++++++----- internal/commands/api_test.go | 92 ++++++++++++++++++++++++++- 2 files changed, 190 insertions(+), 17 deletions(-) diff --git a/internal/commands/api.go b/internal/commands/api.go index a2223e6a..959df762 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, app.Config.AccountID) + 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, app.Config.AccountID) + 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, app.Config.AccountID) + 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, app.Config.AccountID) + if err != nil { + return err + } resp, err := app.Account().Delete(cmd.Context(), path) if err != nil { return convertSDKError(err) @@ -201,20 +214,92 @@ 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. The +// trailing path is optional so a bare "/" (no further segments) +// also matches, leaving m[2] empty. +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. +// +// A leading account segment must match the configured accountID: the SDK +// re-prefixes the configured account, so silently stripping a foreign account +// id would retarget the request (including DELETEs) at the wrong account. +// When accountID is empty (no configured account to compare against), the +// segment is stripped as before. +// +// 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, accountID 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 / + // — but only when it matches the configured account. + path := u.EscapedPath() + if m := accountSegmentPattern.FindStringSubmatch(path); m != nil { + if accountID != "" && m[1] != accountID { + return "", output.ErrUsage(fmt.Sprintf("URL is for account %s; refusing to retarget to configured account %s", m[1], accountID)) + } + // A bare "/" leaves m[2] empty; the request targets the + // account root, so normalize to "/" rather than an empty path. + if path = m[2]; path == "" { + path = "/" + } + } + // A same-host URL with no path at all (e.g. "https://host" or a + // query-only "https://host?foo=bar") leaves path empty since the + // account-segment pattern can't match ""; normalize to the root. + if path == "" { + path = "/" + } + 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 +} + +// 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) +} - return input +// 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..901bc363 100644 --- a/internal/commands/api_test.go +++ b/internal/commands/api_test.go @@ -12,6 +12,11 @@ import ( "github.com/basecamp/basecamp-cli/internal/output" ) +const ( + testBaseURL = "https://3.basecampapi.com" + testAccountID = "12345" +) + func TestParsePath(t *testing.T) { tests := []struct { input string @@ -21,13 +26,96 @@ 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"}, - {"https://3.basecampapi.com/999/projects.json", "/projects.json"}, + // Same-host absolute URLs: extract the path, dropping the account + // segment when it matches the configured account. + {"https://3.basecampapi.com/12345/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"}, + // Same-host URL with no path at all normalizes to the account root. + {"https://3.basecampapi.com", "/"}, + // Same-host query-only URL normalizes to root and keeps the query. + {"https://3.basecampapi.com?foo=bar", "/?foo=bar"}, + // A bare "/" with no trailing segment still matches and + // strips to the account root path rather than leaving it re-prefixed. + {"https://3.basecampapi.com/12345", "/"}, + // Bare "/" plus a query strips to root and keeps the query. + {"https://3.basecampapi.com/12345?foo=bar", "/?foo=bar"}, + // Query strings are preserved. + {"https://3.basecampapi.com/12345/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/12345/projects.json", "/projects.json"}, + // An explicit default port is still the configured host. + {"https://3.basecampapi.com:443/12345/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, testAccountID) + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +// TestParsePathRejectsForeignAccount guards against silent account +// retargeting: the SDK re-prefixes the configured account, so stripping a +// different account's id from a pasted URL would aim the request (including +// DELETEs) at the configured account instead of the one the URL names. +func TestParsePathRejectsForeignAccount(t *testing.T) { + for _, input := range []string{ + "https://3.basecampapi.com/999/projects.json", + "https://3.basecampapi.com/999/buckets/1/todos/2.json", + "https://3.basecampapi.com/999/projects.json?page=2", + // A bare foreign "/" (no trailing path) must still be + // rejected — the broadened pattern must not open a retargeting hole. + "https://3.basecampapi.com/999", + } { + t.Run(input, func(t *testing.T) { + got, err := parsePath(input, testBaseURL, testAccountID) + require.Error(t, err) + assert.Contains(t, err.Error(), "refusing to retarget") + assert.Empty(t, got) + }) + } + + t.Run("empty configured account strips the segment as before", func(t *testing.T) { + got, err := parsePath("https://3.basecampapi.com/999/projects.json", testBaseURL, "") + require.NoError(t, err) + assert.Equal(t, "/projects.json", 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, testAccountID) + require.Error(t, err) + assert.Empty(t, got) }) } }