Skip to content

Commit 0fe4595

Browse files
veteranbvveteranbv
committed
fix(gmail): skip guard account inference without guards; cover Discovery sends
Two follow-ups from Codex review: - The pre-Run per-account check called requireAccount even when no_send_accounts was empty, so a plain dry-run could read the keyring through default-account inference with no guard to evaluate. Skip account resolution entirely unless a per-account guard exists. - The Discovery path (gog api call gmail v1 ...messages.send/drafts.send) ran dryRunExit before checkDiscoveryGmailNoSend, so the same dry-run blind spot existed there. Hoist the guard above the dry-run exit with the same skip and error-tolerance rules, and keep a post-auth checkAccountNoSend for the resolved account, mirroring the first-class send commands.
1 parent ef0e135 commit 0fe4595

5 files changed

Lines changed: 80 additions & 10 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
## 0.34.1 - Unreleased
44

55
- MCP: add optional global and per-account capability ceilings in `config.json`, with narrow persistent write authorization, runtime-only restriction, and fail-closed selector validation. (#913) — thanks @mcaldas.
6-
- Gmail: enforce per-account `config no-send` guards before the dry-run exit so `--dry-run` reports the block instead of `would gmail.send`, matching the `--gmail-no-send` and `gmail_no_send` layers. (#915)
6+
- Gmail: enforce per-account `config no-send` guards before the dry-run exit so `--dry-run` reports the block instead of `would gmail.send`, matching the `--gmail-no-send` and `gmail_no_send` layers; the same pre-dry-run enforcement now covers Discovery `api call` Gmail send methods, and account resolution is skipped entirely when no per-account guards are configured. (#915)
77

88
## 0.34.0 - 2026-07-11
99

‎internal/cmd/api.go‎

Lines changed: 27 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,11 @@ func (c *APICallCmd) Run(ctx context.Context, flags *RootFlags) error {
118118
if !read && !c.AllowWrite {
119119
return usagef("method %s uses %s; pass --allow-write to opt in", method.ID, method.Spec.HttpMethod)
120120
}
121+
// Gmail send guards run before the dry-run exit so --dry-run reports
122+
// the block, matching the first-class Gmail send commands.
123+
if guardErr := checkDiscoveryGmailNoSend(ctx, flags, method.ID); guardErr != nil {
124+
return guardErr
125+
}
121126
plan := map[string]any{"api": c.API, "version": c.Version, "method": method.ID, "http_method": method.Spec.HttpMethod, "url": requestURL, "has_body": len(body) > 0}
122127
if dryRunErr := dryRunExit(ctx, flags, "api.call", plan); dryRunErr != nil {
123128
return dryRunErr
@@ -138,8 +143,12 @@ func (c *APICallCmd) Run(ctx context.Context, flags *RootFlags) error {
138143
if err != nil {
139144
return err
140145
}
141-
if guardErr := checkDiscoveryGmailNoSend(ctx, flags, account, method.ID); guardErr != nil {
142-
return guardErr
146+
// Defense-in-depth for the auth-resolved account, mirroring the
147+
// first-class send commands.
148+
if isDiscoveryGmailSendMethod(method.ID) {
149+
if guardErr := checkAccountNoSend(ctx, account); guardErr != nil {
150+
return guardErr
151+
}
143152
}
144153
scopes, err := discoveryScopes(method.Spec.Scopes, c.Scope)
145154
if err != nil {
@@ -249,8 +258,12 @@ func discoveryScopeScore(scope string) int {
249258
}
250259
}
251260

252-
func checkDiscoveryGmailNoSend(ctx context.Context, flags *RootFlags, account, methodID string) error {
253-
if methodID != "gmail.users.messages.send" && methodID != "gmail.users.drafts.send" {
261+
func isDiscoveryGmailSendMethod(methodID string) bool {
262+
return methodID == "gmail.users.messages.send" || methodID == "gmail.users.drafts.send"
263+
}
264+
265+
func checkDiscoveryGmailNoSend(ctx context.Context, flags *RootFlags, methodID string) error {
266+
if !isDiscoveryGmailSendMethod(methodID) {
254267
return nil
255268
}
256269
if flags != nil && flags.GmailNoSend {
@@ -269,7 +282,16 @@ func checkDiscoveryGmailNoSend(ctx context.Context, flags *RootFlags, account, m
269282
return usage("Gmail sending is blocked by config gmail_no_send")
270283
}
271284

272-
return checkAccountNoSend(ctx, account)
285+
// Same rules as enforceGmailNoSend: only resolve an account when a
286+
// per-account guard could match (default-account inference reads the
287+
// keyring), and leave resolution failures to the command itself.
288+
if len(cfg.NoSendAccounts) == 0 {
289+
return nil
290+
}
291+
if account, accountErr := requireAccount(flags); accountErr == nil {
292+
return checkAccountNoSend(ctx, account)
293+
}
294+
return nil
273295
}
274296

275297
func readDiscoveryBody(value string) (json.RawMessage, error) {

‎internal/cmd/api_test.go‎

Lines changed: 29 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,8 @@ import (
1010
"strings"
1111
"testing"
1212

13+
"github.com/steipete/gogcli/internal/app"
14+
"github.com/steipete/gogcli/internal/config"
1315
"github.com/steipete/gogcli/internal/googleapi"
1416
"github.com/steipete/gogcli/internal/outfmt"
1517
)
@@ -116,12 +118,38 @@ func TestValidateDiscoveryRedirect(t *testing.T) {
116118
}
117119

118120
func TestDiscoveryGmailSendHonorsNoSendFlag(t *testing.T) {
119-
err := checkDiscoveryGmailNoSend(context.Background(), &RootFlags{GmailNoSend: true}, "[email protected]", "gmail.users.messages.send")
121+
err := checkDiscoveryGmailNoSend(context.Background(), &RootFlags{GmailNoSend: true}, "gmail.users.messages.send")
120122
if err == nil || !strings.Contains(err.Error(), "--gmail-no-send") {
121123
t.Fatalf("error = %v, want no-send policy", err)
122124
}
123125
}
124126

127+
func TestDiscoveryGmailSendHonorsPerAccountNoSend(t *testing.T) {
128+
t.Parallel()
129+
130+
store := config.NewConfigStore(config.Layout{ConfigDir: t.TempDir()})
131+
if err := store.Write(config.File{NoSendAccounts: map[string]bool{"[email protected]": true}}); err != nil {
132+
t.Fatalf("WriteConfig: %v", err)
133+
}
134+
ctx := app.WithRuntime(context.Background(), &app.Runtime{Config: store})
135+
flags := &RootFlags{Account: "[email protected]"}
136+
137+
err := checkDiscoveryGmailNoSend(ctx, flags, "gmail.users.messages.send")
138+
if err == nil || !strings.Contains(err.Error(), "no-send") {
139+
t.Fatalf("error = %v, want per-account no-send", err)
140+
}
141+
if err := checkDiscoveryGmailNoSend(ctx, flags, "gmail.users.drafts.send"); err == nil {
142+
t.Fatalf("drafts.send: expected per-account no-send error")
143+
}
144+
// Non-send methods and non-guarded accounts are unaffected.
145+
if err := checkDiscoveryGmailNoSend(ctx, flags, "gmail.users.labels.list"); err != nil {
146+
t.Fatalf("non-send method: %v", err)
147+
}
148+
if err := checkDiscoveryGmailNoSend(ctx, &RootFlags{Account: "[email protected]"}, "gmail.users.messages.send"); err != nil {
149+
t.Fatalf("non-guarded account: %v", err)
150+
}
151+
}
152+
125153
func TestWriteDiscoveryResponsePreservesMedia(t *testing.T) {
126154
var stdout bytes.Buffer
127155
ctx := newCmdRuntimeOutputContext(t, &stdout, &bytes.Buffer{})

‎internal/cmd/gmail_no_send.go‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -43,9 +43,14 @@ func enforceGmailNoSend(kctx *kong.Context, flags *RootFlags, runtime *app.Runti
4343
// Per-account guard, enforced at the same layer as the flag and the
4444
// global config key so it also holds under --dry-run, which exits the
4545
// command before the post-auth checkAccountNoSend call is reached.
46-
// Account resolution failures are not errors here: commands own that
47-
// failure mode, and checkAccountNoSend still covers real sends after
48-
// auth resolves the account.
46+
// Skip entirely when no per-account guards exist so a plain dry-run
47+
// never resolves an account (default-account inference reads the
48+
// keyring). Account resolution failures are not errors here: commands
49+
// own that failure mode, and checkAccountNoSend still covers real
50+
// sends after auth resolves the account.
51+
if len(cfg.NoSendAccounts) == 0 {
52+
return nil
53+
}
4954
if account, accountErr := requireAccount(flags); accountErr == nil {
5055
blocked, blockedErr := runtime.Config.IsNoSendAccount(account)
5156
if blockedErr != nil {

‎internal/cmd/root_more_test.go‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -449,6 +449,21 @@ func TestConfigNoSendAccountDoesNotOverblockDryRun(t *testing.T) {
449449
}
450450
}
451451

452+
func TestGmailSendDryRunWithoutGuardsSkipsAccountResolution(t *testing.T) {
453+
t.Parallel()
454+
455+
// No per-account guards configured: dry-run must succeed without any
456+
// account being resolvable (resolution would otherwise read the
457+
// keyring before the dry-run exit).
458+
store := config.NewConfigStore(config.Layout{ConfigDir: t.TempDir()})
459+
runtime := &app.Runtime{Config: store}
460+
args := []string{"gmail", "send", "--to", "[email protected]", "--subject", "S", "--body", "B", "--dry-run"}
461+
result := executeWithTestRuntime(t, args, runtime)
462+
if result.err != nil {
463+
t.Fatalf("expected success for %v, got %v\nstderr=%q", args, result.err, result.stderr)
464+
}
465+
}
466+
452467
func TestConfigIndependentCommandsDoNotRequireHome(t *testing.T) {
453468
t.Setenv("HOME", "")
454469
t.Setenv("USERPROFILE", "")

0 commit comments

Comments
 (0)