fix: add exact command allowlist matching#652
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds support for enabling only specific commands (exact matches) in the CLI command allowlist logic.
Changes:
- Introduces
--enable-commands-exactflag alongside existing--enable-commands. - Updates enable-command enforcement to allow either prefix-based matches or exact matches.
- Adds unit tests for exact-match command path logic.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/cmd/root.go | Adds the new root flag and wires it into enforcement + arg parsing for value-taking flags. |
| internal/cmd/enabled_commands.go | Extends enable-command enforcement to support exact allow rules. |
| internal/cmd/enabled_commands_test.go | Adds tests for commandPathMatchesExact. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EnableCommands string `help:"Comma-separated list of enabled command prefixes; dot paths allowed (restricts CLI)" default:"${enabled_commands}"` | ||
| EnableCommandsExact string `name:"enable-commands-exact" help:"Comma-separated list of exact enabled commands; dot paths allowed and parent commands do not enable children"` |
There was a problem hiding this comment.
Fixed in the latest branch update. EnableCommandsExact now has default:"${enabled_commands_exact}", and newParser wires enabled_commands_exact from GOG_ENABLE_COMMANDS_EXACT. I also added TestEnableCommandsExactEnvDefault to cover the env/default path.
| func enforceEnabledCommands(kctx *kong.Context, enabled string, enabledExact string) error { | ||
| enabled = strings.TrimSpace(enabled) | ||
| if enabled == "" { | ||
| enabledExact = strings.TrimSpace(enabledExact) | ||
| if enabled == "" && enabledExact == "" { | ||
| return nil | ||
| } | ||
|
|
||
| allow := parseEnabledCommands(enabled) | ||
| if len(allow) == 0 { | ||
| exactAllow := parseEnabledCommands(enabledExact) | ||
| if len(allow) == 0 && len(exactAllow) == 0 { | ||
| return nil | ||
| } | ||
| if allow["*"] || allow["all"] { | ||
| if allow["*"] || allow["all"] || exactAllow["*"] || exactAllow["all"] { | ||
| return nil | ||
| } | ||
|
|
||
| path := commandPath(kctx.Command()) | ||
| if len(path) == 0 { | ||
| return nil | ||
| } | ||
| if !commandPathMatches(allow, path) { | ||
| return usagef("command %q is not enabled (set --enable-commands to allow it)", strings.Join(path, " ")) | ||
| if commandPathMatches(allow, path) || commandPathMatchesExact(exactAllow, path) { | ||
| return nil | ||
| } | ||
| return nil | ||
|
|
||
| return usagef("command %q is not enabled (set --enable-commands or --enable-commands-exact to allow it)", strings.Join(path, " ")) | ||
| } |
There was a problem hiding this comment.
Fixed in the latest branch update. Added focused enforceEnabledCommands coverage for prefix-only allow, exact-only allow, combined allowlists, exact allow blocking a sibling command, and neither allowlist matching returning the usage error. Also added env/default coverage for GOG_ENABLE_COMMANDS_EXACT.
|
Codex review: needs changes before merge. Reviewed May 29, 2026, 12:53 PM ET / 16:53 UTC. Summary Reproducibility: not applicable. This PR adds a new strict allowlist mode rather than reporting a broken existing behavior. Source inspection and the PR body’s terminal output are enough to review the intended behavior. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Land the additive exact allowlist with regenerated command reference docs after maintainers confirm the prefix-or-exact matching model is the intended compatibility tradeoff. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR adds a new strict allowlist mode rather than reporting a broken existing behavior. Source inspection and the PR body’s terminal output are enough to review the intended behavior. Is this the best way to solve the issue? Mostly yes: an additive exact allowlist preserves existing prefix behavior while giving agents a stricter option. The remaining improvement is to regenerate the command reference and have maintainers explicitly accept the additive matching semantics. Full review comments:
Overall correctness: patch is correct AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 929e26b4a999. Label changesLabel justifications:
Evidence reviewedAcceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
clawsweeper
Bot
added
proof: sufficient
jason-allen-oneal
left a comment
jason-allen-oneal
left a comment
There was a problem hiding this comment.
Follow-up docs request addressed in cb5b071.
Added README coverage for --enable-commands-exact and GOG_ENABLE_COMMANDS_EXACT, and added docs/spec.md environment coverage for GOG_ENABLE_COMMANDS_EXACT while keeping the compatibility-preserving prefix behavior documented for --enable-commands.
Local proof from the docs follow-up:
ok github.com/steipete/gogcli/internal/cmd 0.387s
README.md:386:- `--enable-commands-exact <csv>`: allow only exact command paths. Parent paths do not allow children, so `gmail.search` allows `gog gmail search` without allowing sibling commands like `gog gmail send`.
README.md:394: --enable-commands-exact gmail.search,gmail.get,drive.ls,docs.cat \
README.md:401:For environment-configured agents or CI, set `GOG_ENABLE_COMMANDS_EXACT` to the
README.md:405:GOG_ENABLE_COMMANDS_EXACT=gmail.search,gmail.get,drive.ls,docs.cat \
docs/spec.md:156:- `GOG_ENABLE_COMMANDS_EXACT=calendar.events,gmail.search` (optional exact allowlist; dot paths allowed; parent paths do not allow children)
Summary
Adds
--enable-commands-exactfor strict command allowlisting.The existing
--enable-commandsbehavior remains unchanged and continues to allow command-prefix matches. This preserves compatibility for users who intentionally allow whole command families such asgmailordrive.The new exact allowlist mode lets agent and CI callers permit only specific command paths, such as
gmail.search, without also enabling sibling commands such asgmail.send.Security impact
Before this change, command allowlist matching was prefix-based. A broad allowlist entry such as
gmailenabled the full Gmail command family. That is useful for humans, but risky for agent deployments because the Gmail family includes read, write, and settings/admin-style commands.This change provides a non-breaking strict alternative:
--enable-commands gmailkeeps the existing prefix behavior.--enable-commands-exact gmail.searchallows onlygmail search.--enable-commands-exact gmail.searchblocks sibling Gmail commands.GOG_ENABLE_COMMANDS_EXACT=gmail.searchconfigures the same exact allowlist through the env/default path.Review follow-up
Addressed review feedback:
default:"${enabled_commands_exact}"to the new root flag.enabled_commands_exacttoGOG_ENABLE_COMMANDS_EXACTinnewParser.enforceEnabledCommandscovering prefix-only allow, exact-only allow, combined allowlists, blocked sibling commands, neither allowlist matching, and env/default wiring.--enable-commands-exactandGOG_ENABLE_COMMANDS_EXACT.docs/spec.mdenvironment coverage forGOG_ENABLE_COMMANDS_EXACT.Observed behavior proof
Proof was run locally on Kali from branch
fix/exact-command-allowlist.Observed output:
Interpretation:
GOG_ENABLE_COMMANDS_EXACTis wired into the same gate.gmail.searchpath is not blocked by the exact allowlist gate. It proceeds to normal auth/API handling, which proves the intended command path passed the gate.Docs proof
Docs-only follow-up was run locally after commit
cb5b071.Observed output:
Compatibility
This is non-breaking. Existing
--enable-commandsprefix behavior is unchanged.Note
This branch was prepared through the GitHub API, so it contains several small mechanical commits. The final diff is limited to
internal/cmd/enabled_commands.go,internal/cmd/enabled_commands_test.go,internal/cmd/root.go,README.md, anddocs/spec.md, and can be squash-merged into one commit.