security: reject foreign-host URLs in api to prevent token leak#478
Conversation
github-actions
Bot
added
commands
Review carefully before merging. Consider a major version bump. |
There was a problem hiding this comment.
Pull request overview
Hardens the basecamp api subcommand against a credential-exfiltration vector. Previously, parsePath silently passed any non-canonical absolute URL through to the SDK, which would then attach the user's Authorization: Bearer header and send it to the foreign host. parsePath now returns an error for absolute http(s):// URLs that aren't of the form https://<host>/<numeric-account-id>/..., and all four api verb handlers propagate that error before any network call.
Changes:
- Change
parsePathsignature from(string)to(string, error)and reject absolute URLs that don't match the canonical Basecamp account-id form. - Wire error propagation through the get/post/put/delete RunE handlers.
- Add
TestParsePathRejectsForeignHostsand updateTestParsePathfor the new return signature.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/commands/api.go | parsePath now returns an error and rejects non-canonical absolute URLs; all four handlers propagate the error. |
| internal/commands/api_test.go | Updates existing test to new signature; adds table-driven test asserting rejection of foreign/non-canonical URLs. |
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
b6d427d to
2eacce7
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2eacce7710
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
2eacce7 to
447cbf3
Compare
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
447cbf3 to
2734359
Compare
robzolkos
force-pushed
the
security/01-api-token-leak
branch
from
2734359 to
396f100
Compare
robzolkos
force-pushed
the
security/01-api-token-leak
branch
from
396f100 to
20854f7
Compare
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
20854f7 to
d932099
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d932099ec2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
d932099 to
dfb4c74
Compare
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
dfb4c74 to
fc7bafa
Compare
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
fc7bafa to
e56fa0c
Compare
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
e56fa0c to
a97718e
Compare
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.
jeremy
force-pushed
the
security/01-api-token-leak
branch
from
a97718e to
10057f5
Compare
Reject foreign-host URLs in
apito prevent bearer-token leakSeverity: High — credential exfiltration primitive. (Threat: T1 malicious/compromised API target.)
parsePathonly extracted the path from absolute URLs whose first segment was a numeric account ID. Any other absolute URL (e.g.https://evil.example/projects.json) failed the regex, fell through the no-opTrimPrefix, and was returned verbatim toapp.Account().Get/Post/Put/Delete— the SDK then attached theAuthorization: Bearerheader and sent the request to the foreign host.Fix
parsePathnow returns(string, error)and accepts an absolutehttp(s)://URL only when its host matches the API base host we're authenticated against (case-insensitive). URLs on any other host are rejected before any request is made, so the OAuth token can never be sent to a foreign host. Same-host URLs are used as given: the path (plus query) is passed through with no account-segment shape requirement — a leading/<account-id>segment, if present, is dropped since the SDK re-prefixes the configured account. Error propagated through all fourapihandlers.Tests
api_test.goadds a table for relative / leading-slash / valid-account / foreign-host inputs (TestParsePathRejectsForeignHosts). Legitimatehttps://3.basecampapi.com/999/...still extracts correctly.Part 1/6 of a stacked security-hardening series (audit remediation). This one is independent and safe to land alone; the rest stack on top in order. Base:
main.📚 Stack (merge bottom-up)
apito prevent token leak #478 — reject foreign-host URLs inapi(basemain)Each is independent except #482 depends on #481 (shared
root.go). #478 can land first/alone.