security: tighten config-dir perms and validate plugin scope argv#482
Conversation
github-actions
Bot
added
commands
There was a problem hiding this comment.
Pull request overview
This PR strengthens local security boundaries by ensuring the global config directory is created with owner-only permissions (protecting data like credentials.json) and by validating untrusted plugin scope values before passing them to claude plugin uninstall.
Changes:
- Create
config.GlobalConfigDir()with0700(instead of0755) when writing the update-notice cache and the skill refresh sentinel. - Add a best-effort early permission tightening for existing global config dirs created by older versions.
- Whitelist valid Claude plugin scopes (
user|project|local|global) before using them in uninstall argv; add a unit test for the whitelist.
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 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/commands/wizard_agents.go | Whitelists plugin --scope values read from untrusted plugin metadata before uninstalling. |
| internal/commands/wizard_agents_test.go | Adds TestValidPluginScope to lock in the scope whitelist behavior. |
| internal/commands/update_notice.go | Creates the update cache directory with 0700 under global config. |
| internal/commands/skill.go | Creates the version-sentinel directory with 0700 under global config. |
| internal/cli/root.go | Adds a best-effort chmod to tighten perms on existing global config dirs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
jeremy
force-pushed
the
security/04-config-trust-gating
branch
from
d7613bb to
3f44637
Compare
jeremy
force-pushed
the
security/05-perms-and-exec-args
branch
from
e4a7a62 to
a3ff017
Compare
jeremy
force-pushed
the
security/04-config-trust-gating
branch
from
3f44637 to
242f699
Compare
jeremy
force-pushed
the
security/05-perms-and-exec-args
branch
from
a3ff017 to
c1d95a4
Compare
jeremy
force-pushed
the
security/04-config-trust-gating
branch
from
242f699 to
97ebed6
Compare
jeremy
force-pushed
the
security/05-perms-and-exec-args
branch
from
c1d95a4 to
fc26287
Compare
![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: fc262879ca
ℹ️ 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/05-perms-and-exec-args
branch
from
fc26287 to
7ae26db
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ae26db91e
ℹ️ 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
0dcd57f to
253f4f1
Compare
jeremy
force-pushed
the
security/05-perms-and-exec-args
branch
from
7ae26db to
36ba2ba
Compare
jeremy
force-pushed
the
security/04-config-trust-gating
branch
from
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: d120dcc9a0
ℹ️ 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: b1b5b680c7
ℹ️ 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".
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: 4c4fa1e981
ℹ️ 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: cfb5f3a024
ℹ️ 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: 4ca54128d7
ℹ️ 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.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5535c3f16
ℹ️ 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: 1dd902f31e
ℹ️ 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".
- Create the global config dir at 0700 (it can hold credentials.json) in the
skill-refresh and update-notice bootstrap paths, and harden it once early in
root.go so an existing 0755 dir from an older version is tightened. The
hardening is an atomic fd-based open(O_NOFOLLOW) -> fstat -> fchmod sequence
that verifies a self-owned real directory on the open fd, so symlink swaps
and writable ancestors can't redirect the chmod (no Lstat->Chmod TOCTOU),
and a config dir under a sticky world-writable parent like /tmp is hardened
like any other.
- Whitelist the plugin --scope value {user,project,local,global} read from
installed_plugins.json before passing it to 'claude plugin uninstall',
preventing a '-'-leading value from injecting a flag into the argv.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31060a441a
ℹ️ 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".
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| if err := syscall.Fstat(targetFd, &st); err != nil { | ||
| return | ||
| } | ||
| if st.Mode&syscall.S_IFMT != syscall.S_IFDIR || st.Uid != uint32(euid) { //nolint:gosec // G115: Geteuid() returns a valid non-negative uid that fits in uint32 |
| if st.Mode&0o022 != 0 && st.Mode&unix.S_ISVTX == 0 { | ||
| return false | ||
| } | ||
| if st.Uid != uint32(euid) && st.Uid != 0 { //nolint:gosec // G115: Geteuid() returns a valid non-negative uid that fits in uint32 |
| if !ok { | ||
| return false | ||
| } | ||
| return st.Uid == uint32(uid) //nolint:gosec // G115: Geteuid() returns a valid non-negative uid that fits in uint32 |
| func uninstallUnscoped(ctx context.Context, claudePath, key string) bool { | ||
| n := 0 | ||
| for i := 0; i < 10; i++ { | ||
| c := exec.CommandContext(ctx, claudePath, "plugin", "uninstall", key) //nolint:gosec // G204: claudePath from FindClaudeBinary |
Tighten config-dir perms and validate plugin
--scopeargv(Threats: T3 other local users on a shared machine; T6 untrusted data into exec args.)
~/.config/basecamp/at0755, and it runs before the auth store, leaving a dir that can holdcredentials.jsonworld-listable. Changed both bootstrapMkdirAlls to0o700and added a one-timeos.Chmod(GlobalConfigDir(), 0o700)inroot.goto tighten dirs created by older versions. (Intentionally-world-readable~/.agents/~/.claude/skillsdirs left at0755.)scoperead frominstalled_plugins.jsonwas appended after--scopeinclaude plugin uninstallunvalidated; a--leading value injects a flag. Now whitelisted to{user, project, local}.Tests
TestValidPluginScope.Part 5/6 of the stacked security series. Base:
security/04-config-trust-gating.📚 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.