feat(security): per-tool approver allowlist for DEFER approvals (email-based, fail-closed) (#313)

docs/security/defer-decisions.md

@@ -185,9 +185,10 @@ context template.
 When the agent runs with `--with slack` and a deferred tool's `to` is
 `channel:slack:<channel>`, Forge **delivers the approval natively**: on a
 deferral the Slack adapter posts a **Block Kit** message with **Approve /
-Reject** buttons to that channel, and a click resolves the deferral —
-the tool proceeds or fails and the message updates with the outcome +
-approver.
+Reject** buttons to that channel. **Approve** resolves the deferral
+immediately; **Reject** opens a modal that prompts for a reason (recorded
+as the decision `note`). Either way the tool then proceeds or fails and
+the message updates with the outcome + approver.
 
 ```yaml
 security:
@@ -228,31 +229,86 @@ doesn't implement interactive approvals (`channels.ApprovalDeliverer`)
 can't be a target. Telegram / MS Teams interactive approvals are a
 follow-up (same interface).
 
-> ⚠️ **The approval authority is channel membership.** Any user who can
-> see the message and click a button resolves the deferred call. Forge
-> records **who** clicked (in the audit event) but does **not** yet check
-> that they are *authorized* to approve — so **the target channel's
-> membership IS the approval ACL.** Consequences:
+> ⚠️ **Without an `approvers` allowlist, the approval authority is channel
+> membership.** Any user who can see the message and click a button
+> resolves the deferred call. So unless you set `approvers` (below),
+> **the target channel's membership IS the approval ACL.** Consequences:
 >
 > - Route approvals to a **tightly-scoped private channel**, not a broad
 >   `#oncall` that includes guests, contractors, bots, or integrations.
 > - A **compromised member account** grants approval authority over every
 >   agent action routed there — treat channel membership as a privileged
 >   grant.
-> - There is no requester≠approver (four-eyes) or per-tool approver
->   restriction today. A per-tool, email-based approver allowlist is
->   tracked in #313.
+> - There is no requester≠approver (four-eyes) gate yet.
 >
-> **No rejection reason is captured from Slack today.** A button click
-> carries no free text, so a Slack `reject` records an empty `note`; if
-> you need a documented reason, resolve via `POST /tasks/{id}/decisions`
-> with a `note`, or wait for the follow-up that opens a reason modal.
+> **Reject captures a reason.** A button click carries no free text, so
+> clicking **Reject** opens a modal that prompts for a justification; the
+> typed reason is recorded as the decision `note` (and surfaces in the
+> `task_deferred_decision` audit event and the `defer: rejected by …`
+> tool error). If the modal can't be opened (a transient `views.open`
+> failure), Forge falls back to a reason-less reject so the decision is
+> never lost. **Approve** resolves immediately, with no modal.
 
 **If the target adapter isn't running**, Forge warns at startup (e.g.
 `security.defer` routes `cli_execute` to `channel:slack:…` but `slack` is
 not active — start with `--with slack`). The deferral still holds; the
 approval just won't be delivered until an approver POSTs directly.
 
+### Approver allowlist (`approvers`)
+
+To require that only specific people can approve — rather than anyone in
+the channel — set a per-tool (or default) **email** allowlist:
+
+```yaml
+security:
+  defer:
+    default_approvers: [sec-lead@corp.com]     # applied when a tool omits its own
+    tools:
+      cli_execute:
+        to: channel:slack:#sec-approvals
+        timeout: 5m
+        approvers: [alice@corp.com, oncall-lead@corp.com]   # only these may resolve
+```
+
+- **Email, not platform ids** — a portable identity that works the same
+  across Slack / Teams / a web console and matches Forge's OIDC model.
+- **The Slack adapter resolves the clicker's email** via `users.info`
+  (cached) — this needs the bot's **`users:read.email`** scope.
+- **Enforced in the shared runtime** at `POST /tasks/{id}/decisions`
+  (adapter-agnostic — a direct `curl` must send `approver_email` too), and
+  **fails closed**: an approver whose email isn't listed — or can't be
+  resolved (guest without email, missing scope) — is refused with `403`
+  and the **deferral stays pending** for a real approver. The refusal is
+  audited (`task_deferred_decision` with `authorized: false`).
+- Empty `approvers` (and empty `default_approvers`) → no allowlist; channel
+  membership is the ACL (see the warning above).
+
+> 🔑 **The allowlist authorizes an *asserted* email — its strength depends on
+> who asserts it.**
+>
+> - **Slack path** (the one #311 left open): the runtime resolves the real
+>   email from `users.info`, bound to the authenticated Slack identity. Here
+>   the allowlist genuinely restricts *which channel members* can approve.
+> - **Direct `POST /tasks/{id}/decisions`**: the endpoint trusts the
+>   `approver_email` the caller supplies. Whoever holds the runtime bearer
+>   token can assert any listed email — so for direct posts the real gate is
+>   the endpoint's **authentication**, and the allowlist is advisory on top.
+>   That's acceptable (the token holder is already trusted), but it means
+>   **`--no-auth` + an allowlist does not hold**: with no auth, anyone who can
+>   reach the port can assert any email. Never rely on the allowlist without
+>   the runner's auth middleware enabled.
+>
+> A **malformed** approver entry — a bare handle (`alice`), a missing `@`
+> (`alicecorp.com`), or a no-dot domain (`alice@corp`) — can never match and
+> silently locks that person out; fail-closed, not insecure. Forge warns at
+> startup for any `approvers`/`default_approvers` entry that isn't
+> email-shaped so the mistake is caught before it's needed. (This is a
+> structural check only — it can't catch a *valid-but-wrong* address like a
+> `corp`↔`crop` domain typo, which needs a directory to detect.)
+
+Group/role-based approver policies and a requester≠approver (four-eyes)
+gate are future work.
+
 > ⏱ **Approval window for channel-initiated conversations.** A conversation
 > that arrives *through* a channel adapter (Slack/Telegram → agent) is
 > served synchronously: the channel router holds the request open for

forge-cli/cmd/run.go

@@ -399,6 +399,15 @@ func runRun(cmd *cobra.Command, args []string) error {
 	for _, w := range deferTimeoutWarnings(cfg.Security.Defer, channels.SyncRequestTimeout) {
 		fmt.Fprintln(os.Stderr, "  Warning:    "+w)
 	}
+	// #315 review (finding 2): approver emails are free strings, so a
+	// malformed entry (a bare handle, a missing `@`, a no-dot domain) silently
+	// never matches and locks that person out — fail-closed, so not insecure,
+	// just a confusing lockout. Warn on any approver entry that isn't
+	// email-shaped so the mistake is caught at startup. (Structural only — a
+	// valid-but-wrong address like a `corp`↔`crop` typo can't be caught here.)
+	for _, w := range deferApproverWarnings(cfg.Security.Defer) {
+		fmt.Fprintln(os.Stderr, "  Warning:    "+w)
+	}
 
 	return runner.Run(ctx)
 }
@@ -464,6 +473,46 @@ func deferTimeoutWarnings(deferCfg types.DeferConfig, syncWait time.Duration) []
 	return warns
 }
 
+// deferApproverWarnings returns a warning for each DEFER approver entry
+// (per-tool `approvers` or `default_approvers`) that isn't email-shaped. The
+// allowlist matches on email (#313), so a non-email entry can never match and
+// silently locks that approver out — fail-closed, but a startup warning turns a
+// 2 a.m. lockout into a config typo caught at boot. Pure + testable.
+func deferApproverWarnings(deferCfg types.DeferConfig) []string {
+	if !deferCfg.Enabled {
+		return nil
+	}
+	var warns []string
+	check := func(where string, list []string) {
+		for _, a := range list {
+			if !looksLikeEmail(a) {
+				warns = append(warns, fmt.Sprintf(
+					"security.defer %s lists approver %q, which is not email-shaped — the approver allowlist matches on email, so this entry will never authorize anyone (missing @, bare handle, or no-dot domain)",
+					where, a))
+			}
+		}
+	}
+	for tool, tc := range deferCfg.Tools {
+		check(fmt.Sprintf("tool %q", tool), tc.Approvers)
+	}
+	check("default_approvers", deferCfg.DefaultApprovers)
+	sort.Strings(warns)
+	return warns
+}
+
+// looksLikeEmail is a deliberately loose check — one "@" with non-empty local
+// and domain parts, and a dot in the domain. Enough to catch the common typos
+// (missing @, bare username) without rejecting valid addresses.
+func looksLikeEmail(s string) bool {
+	s = strings.TrimSpace(s)
+	at := strings.IndexByte(s, '@')
+	if at <= 0 || at != strings.LastIndexByte(s, '@') || at == len(s)-1 {
+		return false
+	}
+	domain := s[at+1:]
+	return strings.Contains(domain, ".") && !strings.HasPrefix(domain, ".") && !strings.HasSuffix(domain, ".")
+}
+
 // parseDeferTarget parses a DEFER `to:` value of the form
 // "channel:<adapter>:<target>" (e.g. "channel:slack:#oncall") into its adapter
 // and target. Returns ok=false for any other shape.
@@ -480,9 +529,10 @@ func parseDeferTarget(to string) (adapter, target string, ok bool) {
 // whether the channel adapter runs in-process or as a separate process.
 func postDeferDecision(ctx context.Context, agentURL, token string, d corechannels.ApprovalDecision) error {
 	body, _ := json.Marshal(map[string]string{
-		"decision": d.Decision,
-		"approver": d.Approver,
-		"note":     d.Note,
+		"decision":       d.Decision,
+		"approver":       d.Approver,
+		"approver_email": d.ApproverEmail, // #313 — the runtime allowlist check keys on this
+		"note":           d.Note,
 	})
 	url := fmt.Sprintf("%s/tasks/%s/decisions", agentURL, d.TaskID)
 	req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body))

forge-cli/cmd/run_test.go

@@ -181,6 +181,62 @@ func TestDeferTimeoutWarnings(t *testing.T) {
 	})
 }
 
+// TestDeferApproverWarnings covers the config-hygiene warning (#315 finding 2):
+// a non-email approver entry (per-tool or default) can never match the
+// email-keyed allowlist, so it's flagged at startup.
+func TestDeferApproverWarnings(t *testing.T) {
+	cfg := types.DeferConfig{
+		Enabled: true,
+		Tools: map[string]types.DeferToolConfig{
+			"ok_tool":  {Approvers: []string{"alice@corp.com"}},        // valid → no warn
+			"bad_tool": {Approvers: []string{"bob", "carol@corp.com"}}, // "bob" → warn
+			"typo":     {Approvers: []string{"dave@corp"}},             // no dot in domain → warn
+		},
+		DefaultApprovers: []string{"@handle"}, // bare handle → warn
+	}
+
+	warns := deferApproverWarnings(cfg)
+	if len(warns) != 3 { // bob + dave@corp + @handle
+		t.Fatalf("expected 3 approver warnings, got %d: %v", len(warns), warns)
+	}
+	joined := strings.Join(warns, "\n")
+	for _, want := range []string{`"bob"`, `"dave@corp"`, `"@handle"`, "default_approvers"} {
+		if !strings.Contains(joined, want) {
+			t.Errorf("approver warning missing %q:\n%s", want, joined)
+		}
+	}
+	if strings.Contains(joined, "alice@corp.com") {
+		t.Errorf("must not warn for a valid email:\n%s", joined)
+	}
+
+	t.Run("disabled → no warnings", func(t *testing.T) {
+		off := cfg
+		off.Enabled = false
+		if w := deferApproverWarnings(off); w != nil {
+			t.Errorf("disabled defer must not warn, got %v", w)
+		}
+	})
+
+	t.Run("looksLikeEmail edge cases", func(t *testing.T) {
+		for in, want := range map[string]bool{
+			"a@b.com":     true,
+			"a.b@c.co.uk": true,
+			"bob":         false,
+			"a@b":         false, // no dot in domain
+			"@b.com":      false, // empty local
+			"a@":          false, // empty domain
+			"a@@b.com":    false, // two @
+			"a@.com":      false, // domain starts with dot
+			"a@b.":        false, // domain ends with dot
+			" a@b.com ":   true,  // trimmed
+		} {
+			if got := looksLikeEmail(in); got != want {
+				t.Errorf("looksLikeEmail(%q) = %v, want %v", in, got, want)
+			}
+		}
+	})
+}
+
 // TestSyncRequestTimeout_IsReferenced guards the single-source-of-truth: the
 // warning threshold uses the exported router constant.
 func TestSyncRequestTimeout_IsReferenced(t *testing.T) {