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

[initializ-mk]

Closes #313.

Problem

Native DEFER approvals (#310/#311) record who clicked Approve/Reject but never check whether that user is authorized — so channel membership was the entire approval ACL. Anyone in the target channel (guests, contractors, bots) could resolve a security-sensitive deferred tool call.

Change — email-based, adapter-agnostic, fail-closed

Per the issue's design: key the allowlist on email (portable across Slack/Teams/console, matches the OIDC model), enforce it in the shared runtime (not the adapter), and fail closed.

• Config — per-tool approvers (+ optional default_approvers):

  security:
    defer:
      default_approvers: [sec-lead@corp.com]
      tools:
        cli_execute:
          to: channel:slack:#sec-approvals
          timeout: 5m
          approvers: [alice@corp.com, oncall-lead@corp.com]

  Empty → no allowlist (channel membership is the ACL, as before).
• Engine — Spec.Approvers (normalized lowercase); Handle.IsApprover(email) (empty list → anyone; non-empty → listed email, case-insensitive; empty email → fail closed). Resolution.ApproverEmail threads it through.
• Decisions endpoint — the single chokepoint for both Slack and direct curl. Accepts approver_email; when the deferral has an allowlist, an unlisted/empty email is refused 403 and the deferral stays pending for a real approver. The refusal is audited (task_deferred_decision, authorized: false). Enforcement is adapter-agnostic.
• channels.ApprovalDecision.ApproverEmail — adapters populate it.
• Slack — resolves the clicker's email via users.info (cached; needs the users:read.email scope) into ApproverEmail. Best-effort: on failure the email stays empty and the runtime fails closed.
• run.go — postDeferDecision forwards approver_email.

Acceptance

• approvers (+ default_approvers) configurable per DEFER tool.
• Slack resolves approver email via users.info (cached) → ApproverEmail.
• A non-allowlisted click is refused; the deferral stays pending; an allowlisted one resolves it.
• Fail-closed when the email can't be resolved.
• The allowlist check is in the shared runtime (decisions endpoint), not the Slack adapter.
• Audit records the approver email + the allow/deny outcome.

Tests

• Handle.IsApprover (empty allows anyone; listed case/space-insensitive; unlisted refused; empty-email fail-closed).
• Decisions endpoint: listed resolves (case-insensitive), unlisted 403 + stays pending, empty email fails closed, no allowlist allows anyone.
• resolveDeferSpec normalization + default_approvers fallback.
• Slack users.info resolve + cache + no-email/missing-scope errors; handleInteractive attaches the resolved email.

Full forge-core / forge-plugins / forge-cli suites pass; lint clean.

Docs

defer-decisions.md — new Approver allowlist section (config, users:read.email scope, fail-closed, adapter-agnostic), and the membership-ACL warning is softened to "unless you set approvers".

Out of scope (per issue)

Group/role-based approver policies; requester≠approver (four-eyes) gate; Slack reject-reason modal.

---

Update — reject-reason modal (folded in)

Also lands the reject-reason capture that was previously listed as out-of-scope, since it's the same code path.

• Reject now opens a Slack modal (views.open, via the click's trigger_id) that prompts for a justification. The deferral stays pending until the modal is submitted; Approve still resolves immediately.
• On submit, the view_submission (callback forge_defer_reject_modal) carries the typed reason → resolved as a reject with the reason as the decision note, flowing into the task_deferred_decision audit event and the defer: rejected by … tool error.
• The task id + message locator round-trip through the modal's private_metadata; resolution reuses the shared resolveDecision path (email resolution → allowlist enforcement → message update).
• Fallback: a transient views.open failure falls back to a reason-less reject (logged) so a decision is never lost.
• No new Slack scope (views.open uses the existing bot token).

Tests: modal shape, view_submission parse (reason + metadata, foreign-callback / bad-metadata ignore), reject-opens-modal-without-resolving, and submission-resolves-with-note+email. Docs updated (the old "no rejection reason captured" note is replaced).

Remaining out of scope: group/role approver policies; requester≠approver (four-eyes) gate.

[initializ-mk]

Review: per-tool approver allowlist for DEFER (email-based, fail-closed)

This is the actual authorization gate that closes the "channel membership was the entire ACL" hole from #311 — exactly the design #313 specified: email-keyed (portable, matches OIDC), enforced in the shared runtime (decisions endpoint, not the adapter), fail-closed. I traced the security-critical paths and they're all correct:

• IsApprover is sound: empty allowlist → anyone (preserves pre-#313); non-empty → trim+lowercase input, empty email → false (fail-closed), else slices.Contains. Case-insensitivity is properly backed — normalizeApprovers lowercases/trims/drops-blanks the config list, so both sides of the compare are normalized.
• Single chokepoint: the endpoint checks IsApprover AFTER Peek and BEFORE Resolve, and on failure audits authorized: false/unauthorized_approver, returns 403, and leaves the deferral pending so a real approver can still act. That stays-pending behavior is exactly right.
• The endpoint IS authenticated — verified the auth middleware wraps the whole mux (handler = s.authMiddleware(handler)), and the REST handlers register on that same mux, so a direct POST needs the runtime bearer token. The allowlist is a second authorization layer on top of auth.
• Slack resolution fails closed: resolveUserEmail errors on missing scope / no-email / API failure → caller leaves ApproverEmail empty → runtime refuses against a configured allowlist. Cached, correct.

Test coverage is thorough (the IsApprover table incl. empty-email fail-closed; endpoint listed-resolves / unlisted-403-stays-pending / empty-email-fail-closed / no-allowlist; resolveDeferSpec normalization + default_approvers fallback; Slack resolve+cache+no-email). CI fully green.

One docs finding (inline) + two optional minors:

2. Minor: no forge validate hygiene on approver entries

Approver emails are free strings; a typo (alice@crop.com) silently never matches, locking that person out — fail-closed so not insecure, just a confusing 2 a.m. lockout. A forge validate warning for an approvers/default_approvers entry lacking @ would mirror the config-hygiene precedent from #303/#297. Optional.

3. Nit (audit consistency): the denial event uses plain Emit

The 403 denial audits via auditLogger.Emit (no seq/trace/correlation), while the approve/reject events go through the correlation-carrying path. Defensible — the decision POST is its own HTTP request outside the deferred task's invocation scope (same rationale as the schedule-fire events that intentionally use plain Emit), and both event types carry TaskID, the real join key for a cross-request deferral. Noting the asymmetry is intentional, not a gap.

Verdict

Merge-ready. The gate is correct where it counts: fail-closed IsApprover, normalized both-sides comparison, single authenticated chokepoint, stays-pending on refusal, Slack binding the email to a verified identity, audit recording both allow and deny. Finding 1 (the caller-asserted-email docs paragraph) is the one I'd fold in. Cleanly resolves #313 and completes the #310/#311 approval story.

[initializ-mk]

Minor (threat-model clarity): state that the email is caller-asserted — its strength differs by path.

The allowlist authorizes an asserted email, and its trustworthiness comes entirely from who asserts it:

• Slack path: the runtime resolves the real email from users.info, bound to the authenticated Slack identity. This is the strong path — the one that was vulnerable in feat(channels): native Slack interactive approvals for DEFER (R4c) (#310) #311 — so the gate genuinely restricts which channel members can approve.
• Direct-POST path: whoever holds the runtime bearer token can POST any approver_email, including a listed one. Acceptable (the token holder is already trusted), but it means the direct path's real gate is the endpoint's authentication, and the allowlist is advisory there.

This section documents the config, the users:read.email scope, and fail-closed, but never states this distinction — an operator could assume the email is always verified. Suggest a paragraph: "The Slack adapter resolves the approver's email from their authenticated Slack identity; a direct POST /tasks/{id}/decisions trusts the approver_email the (authenticated) caller supplies — so the endpoint's bearer auth is the gate for direct posts." Worth a corollary line: don't run --no-auth with an allowlist and expect it to hold (no auth ⇒ anyone reaching the port asserts any email).

[initializ-mk]

Verified this enforcement is correct: IsApprover is checked after Peek (deferral exists) and before Resolve, a refusal returns 403 + audits authorized:false + leaves the deferral pending (no Resolve call) so a real approver can still act, and the endpoint is behind the auth middleware (the whole mux is wrapped). No change requested here — flagging it as the load-bearing line so a future refactor doesn't reorder the check past Resolve or drop the stays-pending property. The caller-asserted-email nuance is the docs comment on defer-decisions.md.

[initializ-mk]

Re-review: reject-reason modal (2cad19a)

The modal flow is correct and well-built. I verified the three things that matter for a new interactive path:

1. The submission path is NOT an allowlist bypass (top concern): handleRejectSubmission → parseRejectSubmission → the shared resolveDecision, which resolves the submitter's email via users.info and routes through the same decisions-endpoint IsApprover gate as Approve. A reject-with-reason is authorized identically — the modal can't sidestep #313's allowlist. The email is resolved from the submitter's in.User.ID, and Slack only shows the modal to the clicker, so submitter = rejecter; fail-closed either way.
2. private_metadata round-trip is trustworthy: task id + message locator are Slack-held between views.open and view_submission (the user can't tamper), and even if they could, the allowlist gate + the endpoint's Peek (task-must-exist) still apply. No harmful spoofing vector.
3. The 3s view_submission ack budget is met: the envelope is acked up front in the readLoop (conn.WriteJSON(ack)) BEFORE handleInteractive, so the modal closes on that ack and the users.info + decisions-POST + chat.update calls run after — they don't race the deadline. The reason input is a required input block, so Slack enforces non-empty client-side, sidestepping the need for a response_action: errors ack.

The reason-less fallback (if views.open fails, reject without a reason rather than lose it) is the right failure direction, and the shared resolveDecision refactor cleanly unifies approve / reject-with-reason / fallback. Tests cover the new paths including the fail-closed parse cases (foreign callback_id, bad metadata → ignore). This also closes the empty-note gap flagged back in #311 — as a proper feature rather than a caveat.

Minor observation (not a finding): handleInteractive runs synchronously in the readLoop, so its up-to-three HTTP calls block the next envelope's ack. Negligible for approval frequency, but under a burst a later interaction's own 3s ack could be delayed — a goroutine dispatch would decouple it if that ever matters.

Still open from the #315 review

This commit is additive scope, not a response to the earlier findings (no replies on those threads):

• Finding 1 (the one I'd fold in): the caller-asserted-email docs paragraph — that Slack binds the email to a verified identity while a direct authenticated POST trusts the caller's asserted approver_email, so the endpoint's bearer auth is the gate for direct posts. Still not in the docs.
• Finding 2 (optional): forge validate hygiene for typo'd approver emails (entry lacking @).

Verdict

The reject-modal is sound and merge-ready on its own merits — allowlist-enforced, ack-safe, fail-closed. The PR overall is still lightly gated on finding 1's docs paragraph, which this commit didn't touch. (CI: all pass except Integration Tests, still running at review time.)

[initializ-mk]

Addressed the two open review findings in eb2bf44 (finding 3 was noted as intentional — left as-is):

Finding 1 — caller-asserted-email threat-model note (docs). Added a callout to the Approver allowlist section in defer-decisions.md spelling out that the allowlist authorizes an asserted email whose strength depends on who asserts it:

• Slack path — the runtime resolves the real email from users.info, bound to the authenticated Slack identity (the strong gate feat(channels): native Slack interactive approvals for DEFER (R4c) (#310) #311 lacked); the allowlist genuinely restricts which channel members can approve.
• Direct POST /tasks/{id}/decisions — the endpoint trusts the caller-supplied approver_email, so the real gate there is the endpoint's bearer auth and the allowlist is advisory on top. Included the corollary you flagged: --no-auth + an allowlist does not hold (no auth ⇒ anyone reaching the port asserts any email).

Finding 2 — forge validate hygiene (optional, done). deferApproverWarnings now warns at startup for any approvers/default_approvers entry that isn't email-shaped (looksLikeEmail: exactly one @, non-empty local, dotted domain), mirroring the existing channel-target and timeout startup warnings. A typo like alice@crop.com or a bare handle is fail-closed (never matches → lockout), so this turns a 2 a.m. lockout into a boot-time warning. Covered by TestDeferApproverWarnings + looksLikeEmail edge cases.

Finding 3 — plain Emit on the denial event. Left as-is per your note that the asymmetry is intentional (the decision POST is its own HTTP request outside the deferred task's invocation scope, and both events carry TaskID as the join key).

CI green.

[initializ-mk]

Re-review of eb2bf44 — both #315 findings resolved; one small prose inaccuracy the fix introduces

Finding 1 (caller-asserted-email docs) — resolved excellently. The callout cleanly splits the two paths: Slack resolves the real email from an authenticated identity (the strong gate #311 lacked), while a direct POST trusts the caller-supplied approver_email so its real gate is bearer auth with the allowlist "advisory on top." It includes the --no-auth-doesn't-hold corollary and ties in the typo-lockout note. Comprehensive — clearer than the suggestion.

Finding 2 (validate hygiene) — correct guard, well-tested, consistent. deferApproverWarnings + looksLikeEmail warns at startup for non-email approver entries, mirroring the deferChannelTargetWarnings/deferTimeoutWarnings precedent. The looksLikeEmail edge-case test is solid.

One minor inaccuracy (prose, not code)

The illustrative example alice@crop.com — used in the commit message, the run.go comment, and the user-facing docs — describes something the guard does NOT catch. alice@crop.com is structurally a valid email (one @, dotted domain), so looksLikeEmail returns true and no warning fires. The test author clearly understood this: the actual warn cases are bob (bare handle), dave@corp (no dot), @handle (empty local) — all structural non-emails — and the test explicitly asserts alice@corp.com doesn't warn. So the code and tests are correct and self-consistent; only the prose example is wrong.

The honest framing is that the guard catches malformed/non-email entries (missing @, bare handle, no-dot domain), not domain typos — and it can't catch crop↔corp without a directory. The alice@crop.com example oversells it as "typo detection." Suggest swapping to a structural example (alice, or alicecorp.com) in the three prose spots so operators don't expect domain-typo detection the guard doesn't provide. See the inline note on the docs line.

Verdict

Both #315 findings resolved, the code is correct and well-tested — merge-ready. The only residual is that three-place prose example: a wording fix, not a code change, but worth correcting since it's in operator-facing docs and slightly misrepresents the guard's coverage. (CI: Lint + doc-link pass; Test running at review time.)

[initializ-mk]

Minor (accuracy): alice@crop.com is used here (and in the run.go comment + commit message) as the example of what the startup warning catches — but it doesn't. alice@crop.com is structurally a valid email (one @, dotted domain), so looksLikeEmail returns true and no warning fires (your own test asserts alice@corp.com — same shape — produces no warning).

The guard catches malformed entries — missing @, a bare handle, a no-dot domain (bob, @handle, dave@corp, the actual warn cases in the test) — not intra-email domain typos, which it can't distinguish without a directory. Swap the example to a structural one (e.g. alice, or alicecorp.com) so the docs don't imply domain-typo detection. Same edit in the run.go comment and, ideally, note it in the commit trailer.

[initializ-mk]

Fixed in 2783b66. You're right — alice@crop.com is structurally valid, so it's exactly what the guard does not fire on (the test even asserts alice@corp.com produces no warning). Reframed all three spots (docs callout, run.go comment, warning message) around malformed / not email-shaped entries — bare handle (alice), missing @ (alicecorp.com), no-dot domain (alice@corp) — and added an explicit note that a valid-but-wrong domain typo can't be caught structurally. Code and tests were already correct; this was prose-only.

[initializ-mk]

Closing confirmation — 2783b66 corrects the example in all three places

The prose-accuracy finding is fully resolved:

• Docs callout — alice@crop.com swapped for three genuinely-structural non-emails (alice, alicecorp.com, alice@corp), reframed as "malformed," plus the scope boundary I'd recommended: "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."
• run.go comment — same correction and the same explicit "structural only" caveat.
• The warning string itself — reworded from "is not an email … (check for a typo)" to "is not email-shaped … (missing @, bare handle, or no-dot domain)," so the runtime message an operator sees now names the actual failure modes rather than implying typo-detection.

That last one is the nicest touch — the docs, the code comment, the warning text, and the test cases now all agree on exactly what the guard does and doesn't catch. Pure docs/string change, no logic touched.

Verdict

All #315 findings resolved — merge-ready. Nothing outstanding from my side once Lint/Test go green. This also closes the full #310→#311→#313/#315 approval arc: native Slack delivery → email-based fail-closed authorization gate → reject-with-reason modal → threat-model docs → config-hygiene warning, now accurate down to the warning string. Nice work across the series.