fix(gmail): enforce per-account no-send guard before dry-run exit#916
Conversation
The per-account no-send guard was only checked post-auth inside each send command's Run(), which --dry-run exits before reaching. A dry run against a guarded account reported 'would gmail.send' while the flag and global-config layers both block under the same conditions. Enforce the per-account guard in enforceGmailNoSend alongside the flag and global config key. The account resolves without auth via requireAccount; when no account is resolvable the command keeps ownership of that failure mode, and the post-auth checkAccountNoSend call remains as defense-in-depth for real sends. Fixes openclaw#915
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 69f31a012a
ℹ️ 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".
The early no-send hook ran before Execute installed cli.configStoreResolver, so alias resolution inside requireAccount returned an internal resolver error and the per-account guard was skipped for --account <alias>. Install a config-backed resolver before the enforcement hooks; the context-backed resolver installed later is equivalent (both reduce to configureRuntimeConfig + runtime.Config). Flagged by Codex review on openclaw#916.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ef0e13566b
ℹ️ 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".
…ery 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.
|
Codex review: needs maintainer review before merge. Reviewed July 13, 2026, 3:20 AM ET / 07:20 UTC. Summary Reproducibility: yes. Current main exits through dry-run before the per-account check, and the submitted isolated baseline binary reproduces the incorrect Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Land the pre-dry-run policy enforcement with its post-auth defense-in-depth checks and focused regression coverage once required checks pass. Do we have a high-confidence way to reproduce the issue? Yes. Current main exits through dry-run before the per-account check, and the submitted isolated baseline binary reproduces the incorrect Is this the best way to solve the issue? Yes. Hoisting the existing per-account policy to the same pre-Run layer as its global siblings is the narrowest consistent fix, while retaining the resolved-account check after authentication as defense in depth. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5800e0ef0d9f. Label changesLabel justifications:
Evidence reviewedWhat 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
Review history (5 earlier review cycles)
|
|
@clawsweeper re-review PR body upgraded with live-behavior proof: 24/24-case matrix run against real binaries (baseline |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
CHANGELOG.md is release-owned for normal PRs; the user-facing release context stays in the PR body for the landing workflow to consume.
|
@clawsweeper re-review Dropped the PR-owned CHANGELOG entry in 55ee3a4 per the release-note policy — the user-facing release context stays in the PR body for the landing workflow. Only change since the last review; code, tests, and docs/spec are untouched. |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Co-authored-by: Henry Sowell <[email protected]>
Co-authored-by: Henry Sowell <[email protected]>
Co-authored-by: Henry Sowell <[email protected]>
|
Landed in e4239f8c57a5. Maintainer proof on exact PR head
Thank you @veteranbv for the report, fix, and thorough initial proof. |
Fixes #915.
What Problem This Solves
The per-account no-send guard (
gog config no-send set <email>) is enforced only inside each send command'sRun(), after auth resolution — and every send path callsdryRunExitbefore that point.--dry-run, the safe way to probe a guard, reportedwould gmail.sendfor an account whose sends are blocked. The--gmail-no-sendflag and globalgmail_no_sendconfig layers block under--dry-run; the per-account layer silently did not.Why This Change Was Made
An operator or agent verifying its guardrails with
--dry-runis told the guard is absent when it isn't — the exact wrong answer for a safety feature. Real (non-dry-run) sends were never at risk:checkAccountNoSendblocks before any API call on the real path. The failure is the safety signal.The fix enforces the per-account guard in
enforceGmailNoSend, at the same pre-Run()layer as its two siblings, and applies the same pre-dry-run enforcement to the Discovery path (gog api call gmail v1 gmail.users.messages.send/...drafts.send). Per Codex review follow-ups: aliases resolve correctly (a config-backed store resolver is installed before the enforcement hooks), and account resolution is skipped entirely when no per-account guards exist, so a plain dry-run never reads the keyring. Post-authcheckAccountNoSendremains as defense-in-depth everywhere.User Impact
--dry-runon any Gmail send path (first-class or Discovery) now reports the per-account block instead ofwould gmail.send/would api.call.Evidence
Baseline
gog-mainbuilt from upstreammain@5800e0ef;gog-prbuilt from this PR @0fe4595bdf16508e8eae99295757b706af996f06. All runs fully isolated:env -iwith throwawayHOME,mktempconfig dirs, synthetic accounts only, no credentials. Guard config:{"account_aliases":{"work":"[email protected]"},"no_send_accounts":{"[email protected]":true}}Claims proved live (24/24 checks):
send,drafts send,forward,reply,autoreply) and the Discoverygmail.users.messages.sendcall reportwould ...on main for a guarded account under--dry-run, andGmail sending is blocked for [email protected] (config no-send)on this PR.--account work→[email protected]) blocked on this PR.drafts createfor the guarded account dry-runs normally; empty-guards config with no--accountdry-runs normally — all identical on both binaries.--gmail-no-sendand globalgmail_no_sendblock identically on both binaries.Full result matrix (24 cases, JSON)
{"case":"guarded-email send --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.send","pass":true} {"case":"guarded-alias send --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.send","pass":true} {"case":"guarded drafts send --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.drafts.send","pass":true} {"case":"guarded forward --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.forward","pass":true} {"case":"guarded reply --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.reply","pass":true} {"case":"guarded autoreply --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would gmail.autoreply","pass":true} {"case":"guarded discovery messages.send --dry-run","binary":"gog-main","expect_contains":"Dry run:","first_line":"Dry run: would api.call","pass":true} {"case":"other-account send --dry-run (allowed)","binary":"gog-main","expect_contains":"Dry run: would gmail.send","first_line":"Dry run: would gmail.send","pass":true} {"case":"guarded drafts create --dry-run (allowed)","binary":"gog-main","expect_contains":"Dry run: would gmail.drafts.create","first_line":"Dry run: would gmail.drafts.create","pass":true} {"case":"no-guards no-account send --dry-run (allowed)","binary":"gog-main","expect_contains":"Dry run: would gmail.send","first_line":"Dry run: would gmail.send","pass":true} {"case":"flag --gmail-no-send --dry-run (blocked)","binary":"gog-main","expect_contains":"blocked by --gmail-no-send","first_line":"Gmail sending is blocked by --gmail-no-send","pass":true} {"case":"global gmail_no_send --dry-run (blocked)","binary":"gog-main","expect_contains":"blocked by config gmail_no_send","first_line":"Gmail sending is blocked by config gmail_no_send","pass":true} {"case":"guarded-email send --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true} {"case":"guarded-alias send --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true} {"case":"guarded drafts send --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true} {"case":"guarded forward --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true} {"case":"guarded reply --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true} {"case":"guarded autoreply --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true} {"case":"guarded discovery messages.send --dry-run","binary":"gog-pr","expect_contains":"Gmail sending is blocked for [email protected] (config no-send)","first_line":"Gmail sending is blocked for [email protected] (config no-send)","pass":true} {"case":"other-account send --dry-run (allowed)","binary":"gog-pr","expect_contains":"Dry run: would gmail.send","first_line":"Dry run: would gmail.send","pass":true} {"case":"guarded drafts create --dry-run (allowed)","binary":"gog-pr","expect_contains":"Dry run: would gmail.drafts.create","first_line":"Dry run: would gmail.drafts.create","pass":true} {"case":"no-guards no-account send --dry-run (allowed)","binary":"gog-pr","expect_contains":"Dry run: would gmail.send","first_line":"Dry run: would gmail.send","pass":true} {"case":"flag --gmail-no-send --dry-run (blocked)","binary":"gog-pr","expect_contains":"blocked by --gmail-no-send","first_line":"Gmail sending is blocked by --gmail-no-send","pass":true} {"case":"global gmail_no_send --dry-run (blocked)","binary":"gog-pr","expect_contains":"blocked by config gmail_no_send","first_line":"Gmail sending is blocked by config gmail_no_send","pass":true}Scope note: the live matrix intentionally exercises only dry-run and pre-auth paths (no credentialed runs, nothing sent). The real-send guard behavior is unchanged and covered by the existing and new unit tests.
Hygiene, pinned to
0fe4595b:git diff --check origin/main...HEAD— cleanmake fmt— zero files changedmake lint— passgo test ./internal/cmd/ -run 'NoSend|DiscoveryGmailSend|GmailSendDryRun' -v— 10 passed, 0 failedgo test ./internal/cmd/— pass; upstream CI green across all seven checksTest plan
TestConfigNoSendAccountBlocksBeforeDryRun— guarded send paths + alias blocked under--dry-run(fails on unpatched main, passes with fix)TestConfigNoSendAccountDoesNotOverblockDryRun— other accounts and non-send commands unaffectedTestGmailSendDryRunWithoutGuardsSkipsAccountResolution— no guards → no account resolution on dry-runTestDiscoveryGmailSendHonorsPerAccountNoSend— Discovery messages.send/drafts.send blocked per-account; non-send methods and other accounts unaffectedTestDiscoveryGmailSendHonorsNoSendFlag— updated for the parameter-less guard signaturemake fmt/make lint/make test— cleanUser-facing change:
--dry-runon a send command now reports the per-account block instead ofDry run: would gmail.send. No new flags. CHANGELOG and docs/spec.md updated.