security: close config trust-boundary gaps and gate completion loader#481
Conversation
![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: d7613bb6c8
ℹ️ 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".
There was a problem hiding this comment.
Pull request overview
This PR tightens the config trust boundary so untrusted repo/local configs can’t silently redirect cache writes or amplify LLM spend, and it aligns shell completion config-loading with the same trust model while hardening completion descriptions against terminal control characters.
Changes:
- Trust-gate additional “authority” config keys (
cache_dir,cache_enabled,llm_model,llm_max_concurrent,llm_token_budget) and validatellm_endpointscheme on config-file load. - Gate completion’s repo/local profile loading behind the trust store and sanitize completion descriptions to strip control chars.
- Enforce secure (
httpsfor non-localhost)llm_endpointat CLI startup and add targeted config tests.
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.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/config/config.go | Trust-gates additional keys and adds llm_endpoint scheme validation helper. |
| internal/config/config_test.go | Adds tests for rejecting authority keys from untrusted local/repo configs and rejecting unsafe llm_endpoint schemes. |
| internal/completion/completer.go | Sanitizes completion descriptions to prevent control-character terminal injection. |
| internal/completion/cache.go | Loads repo/local profiles for completion only if the config path is trusted. |
| internal/cli/root.go | Enforces secure llm_endpoint alongside base_url before running most commands. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
d858f7f to
9e7bb58
Compare
jeremy
force-pushed
the
security/04-config-trust-gating
branch
from
d7613bb to
3f44637
Compare
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
9e7bb58 to
3f9352d
Compare
jeremy
force-pushed
the
security/04-config-trust-gating
branch
from
3f44637 to
242f699
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 242f699ed6
ℹ️ 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/03-oauth-hardening
branch
from
3f9352d to
396ecac
Compare
jeremy
force-pushed
the
security/04-config-trust-gating
branch
from
242f699 to
97ebed6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97ebed69f6
ℹ️ 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/04-config-trust-gating
branch
from
97ebed6 to
0dcd57f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0dcd57facf
ℹ️ 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/03-oauth-hardening
branch
from
6a3a964 to
8add014
Compare
jeremy
force-pushed
the
security/04-config-trust-gating
branch
from
8d121df to
7fd2e91
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
jeremy
force-pushed
the
security/03-oauth-hardening
branch
from
04096c1 to
77e1dbe
Compare
jeremy
force-pushed
the
security/04-config-trust-gating
branch
from
7fd2e91 to
2ae8dcc
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
API-controlled strings (names, summaries, notices, headlines, affordance
commands) reached styled/markdown sinks and the --watch TUI without ANSI
stripping, allowing terminal/OSC injection. Sanitize WithSummary/WithNotice/
WithDiagnostic at the source, the timeline watch TUI fields (creator name
stripped before the empty check so an all-escape name still falls back to a
placeholder), and presenter format helpers, with defense-in-depth at the
render sinks. Stripping is centralized in RenderTemplate + RenderHeadline so
schema headlines ({{.name}}/{{.subject}}/{{.content}}) and affordance commands
are all covered, including the identity-label headline fallback.
Assert the exact sanitized string instead of mirroring ansi.Strip, which is a weaker contract than richtext.SanitizeTerminal and ties the expectation to the third-party stripper's behavior.
Stripping keys let a hostile key like "\x1b[31mtitle" collapse onto a legitimate "title", silently replacing or hiding that field in TTY --jq output. Keys changed by sanitization are now visibly escaped via strconv.Quote, so clean keys always keep their slot and no entry is dropped. Also update a stale test comment that referenced ansi.Strip.
The escaped form of a hostile key could still collide with a literal key that mimics escape notation (e.g. a real ESC-prefixed title vs the text "\x1b[31mtitle"), nondeterministically dropping one entry. Clean keys now claim their names first, and escaped keys re-quote with delimiters until unique, in sorted order for deterministic output.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 76a9c7f8a4
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3f77a3ae10
ℹ️ 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".
Request IDs and string ids print to line-oriented sinks, but SanitizeTerminal preserves newlines/tabs (which inject lines or columns into ids output) and deletes bare CR (which joins adjacent words). A shared sanitizeSingleLine helper normalizes CR/CRLF to newlines first, then strips and collapses all whitespace to single spaces.
- Scheme-validate the discovery authorization_endpoint (https, or http on loopback) before it's dispatched to the OS browser launcher, so a hostile discovery doc can't hand the OS a file:// (or other) URL. - Enforce RequireSecureURL on the DCR registration_endpoint. - Set CheckRedirect on the OAuth HTTP client so token exchange/refresh POST bodies (auth code, refresh_token) aren't replayed to a redirect target. Idempotent GET/HEAD (OAuth discovery) may still follow redirects, so we don't break discovery or force an unintended Launchpad fallback.
- Trust-gate cache_dir, cache_enabled, llm_model, llm_max_concurrent and llm_token_budget so an untrusted local/repo config can't redirect cache writes or amplify paid-LLM usage; only honored from trusted sources. 'config set' now also warns when these keys are written to an untrusted local config so the value isn't silently ignored on the next load. - Scheme/host-validate llm_endpoint on accept (rejects file://, hostless and malformed forms) and re-validate at the root.go enforcement point so env/ profile-sourced endpoints can't slip a non-http(s) scheme past RequireSecureURL, which only blocks http:// for non-localhost; this prevents leaking llm_api_key in cleartext. - Gate the completion profile loader behind the TrustStore and strip control characters from completion descriptions.
Close config trust-boundary gaps and gate the completion loader
(Threat: T2 malicious repo/parent-dir
.basecamp/config.json; cleartext key leak.)The existing
untrustedguard already gatesbase_url/profiles/llm_provider/llm_endpoint/llm_api_key. This closes the siblings that slipped through:cache_dir/cache_enabled— an untrusted repo config could redirect every cache write (completion, resilience, TUI workspace, recents, traces) to any user-writable path. Now trust-gated; accepted values arefilepath.Cleaned.llm_model— trust-gated (silent paid-model substitution).llm_max_concurrent/llm_token_budget— trust-gated (paid-LLM cost amplification).llm_endpoint— scheme-validated on accept (rejectsfile://etc.). HTTPS enforcement (so anhttp://endpoint can't leakllm_api_keyin cleartext) is applied at consumption viasummarize.ValidateEndpointinworkspace.NewSession— not at startup inroot.go— so a badllm_endpointcan't brick unrelated commands that never use it.completion/cache.go,completer.go) — repo/local profile loading now goes through the sameTrustStore.IsTrustedcheck as the main config; completion descriptions are stripped of control chars.Tests
config_test.goadds reject-from-local / accept-from-global / llm_endpoint-scheme cases.Part 4/6 of the stacked security series. Base:
security/03-oauth-hardening. (Sharesroot.gowith part 5 — hence the stack ordering.)📚 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.