Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
72 changes: 56 additions & 16 deletions internal/commands/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package commands
import (
"encoding/json"
"fmt"
"net/url"
"regexp"
"strings"

Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 err
Expand All @@ -201,20 +214,47 @@ 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 /<account-id>/ 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 /<account-id> 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 stray leading slash 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.TrimPrefix(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 == "" || !strings.EqualFold(u.Host, base.Host) {
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 /<account-id>.
path := u.EscapedPath()
if m := accountSegmentPattern.FindStringSubmatch(path); m != nil {
path = m[1]
}
if u.RawQuery != "" {
path += "?" + u.RawQuery
}
return path, nil
}

// Strip leading slashthe SDK prefixes the account path.
input = strings.TrimPrefix(input, "/")

return input
// Relative path — return without the leading slash; the SDK prefixes the
// account path (keeping it here would double-slash and, on Windows,
// MSYS/Git Bash converts /path to C:\...\path).
return candidate, nil
}

// apiSummary generates a summary from the API response.
Expand Down
42 changes: 41 additions & 1 deletion internal/commands/api_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ import (
"github.com/stretchr/testify/require"
)

const testBaseURL = "https://3.basecampapi.com"

func TestParsePath(t *testing.T) {
tests := []struct {
input string
Expand All @@ -17,13 +19,51 @@ 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.
Comment on lines +25 to +27

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The accepted-behavior here looks intentional rather than a gap: this PR added the assertion together with an explanatory comment — // Same host but no account segment — accepted, path used as-is. — so the description is the stale part, not the code.

The contract in the description (https://<host>/<numeric-account-id>/...) describes the original shape-based design. Over the course of review this became a host-based check (EqualFold(u.Host, base.Host)), which relocated the security property from 'does it have the account-id shape?' to 'is the host the one we're authenticated against?'. The account segment is irrelevant to the token-leak goal — only the host determines where the credential is sent.

Keeping the lenient acceptance is also the more consistent choice: api get projects.json and api get https://<configured-host>/projects.json are the same effective request (same host, path /projects.json, SDK re-prefixes the account). Rejecting the second while accepting the first would be arbitrary, and tightening the code would reject a harmless, equivalent input.

Recommend resolving this by updating the description to 'relative paths, or absolute URLs on the configured Basecamp host (foreign hosts rejected)' rather than changing the code. Leaving this for @jeremy to make the call.

{"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"},
}

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
// and a leading-slash smuggling attempt.
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
}

for _, input := range bad {
t.Run(input, func(t *testing.T) {
got, err := parsePath(input, testBaseURL)
require.Error(t, err)
assert.Empty(t, got)
})
}
}
Expand Down
Loading