security: close config trust-boundary gaps and gate completion loader#481
security: close config trust-boundary gaps and gate completion loader#481jeremy wants to merge 1 commit into
Conversation
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.
d858f7f to
9e7bb58
Compare
d7613bb to
3f44637
Compare
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
9e7bb58 to
3f9352d
Compare
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".
3f9352d to
396ecac
Compare
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".
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".
6a3a964 to
8add014
Compare
0dcd57f to
253f4f1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 253f4f1f45
ℹ️ 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".
8add014 to
159b0ef
Compare
253f4f1 to
0635112
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0635112246
ℹ️ 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".
159b0ef to
472188d
Compare
0635112 to
ffb8fe1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffb8fe1cfb
ℹ️ 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".
- 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.
472188d to
e97f2a9
Compare
ffb8fe1 to
97fd65d
Compare
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.) andRequireSecureURLenforced inroot.goalongsidebase_url, so anhttp://endpoint can't leakllm_api_keyin cleartext.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.