fix(gmail): enforce per-account no-send guard before dry-run exit (#916)

veteranbv · web-flow · commit e4239f8c57a5 · 2026-07-13T00:34:14.000-07:00
Enforce per-account Gmail no-send guards before dry-run exits for first-class and Discovery send paths. Preserve alias and account resolution, avoid keyring access when no active per-account guard exists, and retain post-auth defense in depth. Fixes #915. Co-authored-by: Henry Sowell <henrysowell@gmail.com>
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,7 @@
 
 ## 0.34.1 - Unreleased
 
+- Gmail: enforce per-account no-send guards before dry-run exits for first-class and Discovery send paths while preserving no-guard keyring avoidance. (#915, #916) — thanks @veteranbv.
 - 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.
 
 ## 0.34.0 - 2026-07-11
diff --git a/internal/cmd/api.go b/internal/cmd/api.go
@@ -118,6 +118,11 @@ func (c *APICallCmd) Run(ctx context.Context, flags *RootFlags) error {
 	if !read && !c.AllowWrite {
 		return usagef("method %s uses %s; pass --allow-write to opt in", method.ID, method.Spec.HttpMethod)
 	}
+	// Gmail send guards run before the dry-run exit so --dry-run reports
+	// the block, matching the first-class Gmail send commands.
+	if guardErr := checkDiscoveryGmailNoSend(ctx, flags, method.ID); guardErr != nil {
+		return guardErr
+	}
 	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}
 	if dryRunErr := dryRunExit(ctx, flags, "api.call", plan); dryRunErr != nil {
 		return dryRunErr
@@ -138,8 +143,12 @@ func (c *APICallCmd) Run(ctx context.Context, flags *RootFlags) error {
 	if err != nil {
 		return err
 	}
-	if guardErr := checkDiscoveryGmailNoSend(ctx, flags, account, method.ID); guardErr != nil {
-		return guardErr
+	// Defense-in-depth for the auth-resolved account, mirroring the
+	// first-class send commands.
+	if isDiscoveryGmailSendMethod(method.ID) {
+		if guardErr := checkAccountNoSend(ctx, account); guardErr != nil {
+			return guardErr
+		}
 	}
 	scopes, err := discoveryScopes(method.Spec.Scopes, c.Scope)
 	if err != nil {
@@ -249,8 +258,12 @@ func discoveryScopeScore(scope string) int {
 	}
 }
 
-func checkDiscoveryGmailNoSend(ctx context.Context, flags *RootFlags, account, methodID string) error {
-	if methodID != "gmail.users.messages.send" && methodID != "gmail.users.drafts.send" {
+func isDiscoveryGmailSendMethod(methodID string) bool {
+	return methodID == "gmail.users.messages.send" || methodID == "gmail.users.drafts.send"
+}
+
+func checkDiscoveryGmailNoSend(ctx context.Context, flags *RootFlags, methodID string) error {
+	if !isDiscoveryGmailSendMethod(methodID) {
 		return nil
 	}
 	if flags != nil && flags.GmailNoSend {
@@ -269,7 +282,16 @@ func checkDiscoveryGmailNoSend(ctx context.Context, flags *RootFlags, account, m
 		return usage("Gmail sending is blocked by config gmail_no_send")
 	}
 
-	return checkAccountNoSend(ctx, account)
+	// Same rules as enforceGmailNoSend: only resolve an account when a
+	// per-account guard could match (default-account inference reads the
+	// keyring), and leave resolution failures to the command itself.
+	if !hasActiveNoSendAccount(cfg.NoSendAccounts) {
+		return nil
+	}
+	if account, accountErr := requireAccount(flags); accountErr == nil {
+		return checkAccountNoSend(ctx, account)
+	}
+	return nil
 }
 
 func readDiscoveryBody(value string) (json.RawMessage, error) {
diff --git a/internal/cmd/api_test.go b/internal/cmd/api_test.go
@@ -10,8 +10,11 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/steipete/gogcli/internal/app"
+	"github.com/steipete/gogcli/internal/config"
 	"github.com/steipete/gogcli/internal/googleapi"
 	"github.com/steipete/gogcli/internal/outfmt"
+	"github.com/steipete/gogcli/internal/secrets"
 )
 
 func TestAPICallRequiresWriteOptIn(t *testing.T) {
@@ -116,12 +119,58 @@ func TestValidateDiscoveryRedirect(t *testing.T) {
 }
 
 func TestDiscoveryGmailSendHonorsNoSendFlag(t *testing.T) {
-	err := checkDiscoveryGmailNoSend(context.Background(), &RootFlags{GmailNoSend: true}, "user@example.com", "gmail.users.messages.send")
+	err := checkDiscoveryGmailNoSend(context.Background(), &RootFlags{GmailNoSend: true}, "gmail.users.messages.send")
 	if err == nil || !strings.Contains(err.Error(), "--gmail-no-send") {
 		t.Fatalf("error = %v, want no-send policy", err)
 	}
 }
 
+func TestDiscoveryGmailSendHonorsPerAccountNoSend(t *testing.T) {
+	t.Parallel()
+
+	store := config.NewConfigStore(config.Layout{ConfigDir: t.TempDir()})
+	if err := store.Write(config.File{NoSendAccounts: map[string]bool{"blocked@example.com": true}}); err != nil {
+		t.Fatalf("WriteConfig: %v", err)
+	}
+	ctx := app.WithRuntime(context.Background(), &app.Runtime{Config: store})
+	flags := &RootFlags{Account: "blocked@example.com"}
+
+	err := checkDiscoveryGmailNoSend(ctx, flags, "gmail.users.messages.send")
+	if err == nil || !strings.Contains(err.Error(), "no-send") {
+		t.Fatalf("error = %v, want per-account no-send", err)
+	}
+	if err := checkDiscoveryGmailNoSend(ctx, flags, "gmail.users.drafts.send"); err == nil {
+		t.Fatalf("drafts.send: expected per-account no-send error")
+	}
+	// Non-send methods and non-guarded accounts are unaffected.
+	if err := checkDiscoveryGmailNoSend(ctx, flags, "gmail.users.labels.list"); err != nil {
+		t.Fatalf("non-send method: %v", err)
+	}
+	if err := checkDiscoveryGmailNoSend(ctx, &RootFlags{Account: "other@example.com"}, "gmail.users.messages.send"); err != nil {
+		t.Fatalf("non-guarded account: %v", err)
+	}
+}
+
+func TestDiscoveryGmailSendWithInactiveGuardsSkipsAccountResolution(t *testing.T) {
+	t.Parallel()
+
+	store := config.NewConfigStore(config.Layout{ConfigDir: t.TempDir()})
+	if err := store.Write(config.File{NoSendAccounts: map[string]bool{"inactive@example.com": false}}); err != nil {
+		t.Fatalf("WriteConfig: %v", err)
+	}
+	ctx := app.WithRuntime(context.Background(), &app.Runtime{Config: store})
+	flags := &RootFlags{authOperations: app.AuthOperations{
+		OpenSecretsStore: func() (secrets.Store, error) {
+			t.Fatal("inactive no-send entries must not trigger account resolution")
+			return nil, errors.New("unexpected account resolution")
+		},
+	}}
+
+	if err := checkDiscoveryGmailNoSend(ctx, flags, "gmail.users.messages.send"); err != nil {
+		t.Fatalf("inactive no-send entry: %v", err)
+	}
+}
+
 func TestWriteDiscoveryResponsePreservesMedia(t *testing.T) {
 	var stdout bytes.Buffer
 	ctx := newCmdRuntimeOutputContext(t, &stdout, &bytes.Buffer{})
diff --git a/internal/cmd/gmail_no_send.go b/internal/cmd/gmail_no_send.go
@@ -40,9 +40,38 @@ func enforceGmailNoSend(kctx *kong.Context, flags *RootFlags, runtime *app.Runti
 	if cfg.GmailNoSend {
 		return usage("Gmail sending is blocked by config gmail_no_send")
 	}
+	// Per-account guard, enforced at the same layer as the flag and the
+	// global config key so it also holds under --dry-run, which exits the
+	// command before the post-auth checkAccountNoSend call is reached.
+	// Skip entirely when no per-account guards exist so a plain dry-run
+	// never resolves an account (default-account inference reads the
+	// keyring). Account resolution failures are not errors here: commands
+	// own that failure mode, and checkAccountNoSend still covers real
+	// sends after auth resolves the account.
+	if !hasActiveNoSendAccount(cfg.NoSendAccounts) {
+		return nil
+	}
+	if account, accountErr := requireAccount(flags); accountErr == nil {
+		blocked, blockedErr := runtime.Config.IsNoSendAccount(account)
+		if blockedErr != nil {
+			return blockedErr
+		}
+		if blocked {
+			return usagef("Gmail sending is blocked for %s (config no-send)", strings.TrimSpace(account))
+		}
+	}
 	return nil
 }
 
+func hasActiveNoSendAccount(accounts map[string]bool) bool {
+	for _, blocked := range accounts {
+		if blocked {
+			return true
+		}
+	}
+	return false
+}
+
 func checkAccountNoSend(ctx context.Context, account string) error {
 	store, err := commandConfigStore(ctx)
 	if err != nil {
diff --git a/internal/cmd/root.go b/internal/cmd/root.go
@@ -167,6 +167,18 @@ func executeWithRuntime(args []string, runtime *app.Runtime) (err error) {
 	cli.authMode = googleapi.ParseAuthMode(os.Getenv("GOG_AUTH_MODE"))
 	applyExplicitOutputModePrecedence(kctx, &cli.RootFlags)
 
+	// Make config-backed account and alias resolution available to the
+	// pre-Run enforcement hooks below (enforceGmailNoSend resolves the
+	// target account). The context-backed resolver installed later
+	// replaces this with an equivalent one; both reduce to
+	// configureRuntimeConfig + runtime.Config.
+	cli.configStoreResolver = func() (*config.ConfigStore, error) {
+		if cfgErr := configureRuntimeConfig(runtime); cfgErr != nil {
+			return nil, cfgErr
+		}
+		return runtime.Config, nil
+	}
+
 	if err = enforceBakedSafetyProfile(kctx); err != nil {
 		return reportEarlyError(runtimeIO.Err, err)
 	}
diff --git a/internal/cmd/root_more_test.go b/internal/cmd/root_more_test.go
@@ -14,6 +14,7 @@ import (
 
 	"github.com/steipete/gogcli/internal/app"
 	"github.com/steipete/gogcli/internal/config"
+	"github.com/steipete/gogcli/internal/secrets"
 )
 
 func setTestConfigHome(t *testing.T) {
@@ -396,6 +397,93 @@ func TestConfigGmailNoSendBlocksBeforeAuth(t *testing.T) {
 	}
 }
 
+func TestConfigNoSendAccountBlocksBeforeDryRun(t *testing.T) {
+	t.Parallel()
+
+	store := config.NewConfigStore(config.Layout{ConfigDir: t.TempDir()})
+	if err := store.Write(config.File{
+		AccountAliases: map[string]string{"work": "blocked@example.com"},
+		NoSendAccounts: map[string]bool{"blocked@example.com": true},
+	}); err != nil {
+		t.Fatalf("WriteConfig: %v", err)
+	}
+	runtime := &app.Runtime{Config: store}
+	tests := [][]string{
+		{"gmail", "send", "--account", "blocked@example.com", "--to", "a@example.com", "--subject", "S", "--body", "B", "--dry-run"},
+		{"gmail", "autoreply", "from:a@example.com", "--account", "blocked@example.com", "--subject", "S", "--body", "B", "--dry-run"},
+		{"gmail", "forward", "msg-1", "--account", "blocked@example.com", "--to", "a@example.com", "--dry-run"},
+		{"gmail", "drafts", "send", "draft-1", "--account", "blocked@example.com", "--dry-run"},
+		// Alias resolving to a guarded account is blocked too.
+		{"gmail", "send", "--account", "work", "--to", "a@example.com", "--subject", "S", "--body", "B", "--dry-run"},
+	}
+	for _, args := range tests {
+		result := executeWithTestRuntime(t, args, runtime)
+		err := result.err
+		if err == nil {
+			t.Fatalf("expected error for %v", args)
+		}
+		if !strings.Contains(err.Error(), "no-send") {
+			t.Fatalf("unexpected error for %v: %v", args, err)
+		}
+	}
+}
+
+func TestConfigNoSendAccountDoesNotOverblockDryRun(t *testing.T) {
+	t.Parallel()
+
+	store := config.NewConfigStore(config.Layout{ConfigDir: t.TempDir()})
+	if err := store.Write(config.File{NoSendAccounts: map[string]bool{"blocked@example.com": true}}); err != nil {
+		t.Fatalf("WriteConfig: %v", err)
+	}
+	runtime := &app.Runtime{Config: store}
+	tests := [][]string{
+		// Send from a different account is not blocked.
+		{"gmail", "send", "--account", "other@example.com", "--to", "a@example.com", "--subject", "S", "--body", "B", "--dry-run"},
+		// Non-send commands for the guarded account are not blocked.
+		{"gmail", "drafts", "create", "--account", "blocked@example.com", "--to", "a@example.com", "--subject", "S", "--body", "B", "--dry-run"},
+	}
+	for _, args := range tests {
+		result := executeWithTestRuntime(t, args, runtime)
+		if result.err != nil {
+			t.Fatalf("expected success for %v, got %v\nstderr=%q", args, result.err, result.stderr)
+		}
+	}
+}
+
+func TestGmailSendDryRunWithoutGuardsSkipsAccountResolution(t *testing.T) {
+	t.Parallel()
+
+	// No per-account guards configured: dry-run must succeed without any
+	// account being resolvable (resolution would otherwise read the
+	// keyring before the dry-run exit).
+	store := config.NewConfigStore(config.Layout{ConfigDir: t.TempDir()})
+	runtime := &app.Runtime{Config: store}
+	args := []string{"gmail", "send", "--to", "a@example.com", "--subject", "S", "--body", "B", "--dry-run"}
+	result := executeWithTestRuntime(t, args, runtime)
+	if result.err != nil {
+		t.Fatalf("expected success for %v, got %v\nstderr=%q", args, result.err, result.stderr)
+	}
+}
+
+func TestGmailSendDryRunWithInactiveGuardsSkipsAccountResolution(t *testing.T) {
+	t.Parallel()
+
+	store := config.NewConfigStore(config.Layout{ConfigDir: t.TempDir()})
+	if err := store.Write(config.File{NoSendAccounts: map[string]bool{"inactive@example.com": false}}); err != nil {
+		t.Fatalf("WriteConfig: %v", err)
+	}
+	runtime := &app.Runtime{Config: store}
+	runtime.Auth.OpenSecretsStore = func() (secrets.Store, error) {
+		t.Fatal("inactive no-send entries must not trigger account resolution")
+		return nil, errors.New("unexpected account resolution")
+	}
+	args := []string{"gmail", "send", "--to", "a@example.com", "--subject", "S", "--body", "B", "--dry-run"}
+	result := executeWithTestRuntime(t, args, runtime)
+	if result.err != nil {
+		t.Fatalf("expected success for %v, got %v\nstderr=%q", args, result.err, result.stderr)
+	}
+}
+
 func TestConfigIndependentCommandsDoNotRequireHome(t *testing.T) {
 	t.Setenv("HOME", "")
 	t.Setenv("USERPROFILE", "")
