feat(mcp): Forge-driven Slack delivery of MCP consent prompts (#343)#345
Conversation
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Review — Forge-driven Slack delivery of MCP consent prompts (#343)
Approve with comments. The core architecture is right and the managed invariant holds. One Medium availability gap, two Low notes. Merge-ordering: stacked on #344 (base: feat/mcp-standalone-consent-332) — merge #344 first, then retarget this to main.
Verified solid
- Managed invariant preserved:
FetchAuthorizeURLsends{server, subject}with the same Bearer-agent-identity +Org-Id/Workspace-Idheaders as the token endpoint and treats the returned URL as opaque — the code + refresh token land at the platform, Forge never sees them. Delivery-leg ⊥ callback-leg holds. - Fallback safety is real (
mcp_authgate.go:74-82):EventMCPAuthRequiredis emitted beforedeliver(...), anddeliverlogs a delivery failure as a Warn without returning an error — a Slack outage or unreachable subject never fails or hangs the parked call. - Egress allowlist now merges both
token_endpointandauthorize_endpointhosts; the authorize fetch is allowlist-routed. refresolution in the managed provider matches the token-endpoint path, so the platform can correlate the two requests.- Cancel routing is keyed on the exact
consentCancelActionID, routed first, so it can't collide with DEFER approvals or the Connect URL button. Socket Mode ⇒ workspace-authenticated transport, no signature gap. Cancel goes throughPOST /mcp/consentwithrunner.AuthToken(), mirroring DEFER.
Findings
- [Medium] standalone + Slack: a per-subject delivery failure leaves the user with no link — the single-slot Slack deliverer displaces the standalone A2A-artifact link deliverer, and the audit backstop has no reader in standalone. Inline on the wiring.
- [Low] Cancel click doesn't verify the clicker is the subject — latent today (v1 is DM-only), but a cross-user DoS once origin-thread delivery is enabled. Inline.
- [Low / defense-in-depth] platform-returned
authorize_urlis delivered to a clickable button unvalidated — anhttps://assertion is cheap insurance. Inline.
| consentAdapter.SetConsentCanceler(func(ctx context.Context, subject, server string) error { | ||
| return postMCPConsent(ctx, agentURL, runner.AuthToken(), subject, server, false) | ||
| }) | ||
| runner.SetConsentDeliverer(func(ctx context.Context, subject, server, taskID string, deadline time.Time) error { |
There was a problem hiding this comment.
[Medium] In standalone + Slack, a per-subject delivery failure leaves the user with no link.
SetConsentDeliverer is a single slot, and this wires the Slack deliverer before Run(). enableStandaloneConsent only sets its A2A-artifact link deliverer if r.consentDeliverer == nil, so when Slack is active the standalone A2A-artifact link deliverer is displaced.
Now the common case where the requesting user's email isn't in the Slack workspace (users.lookupByEmail fails): deliver logs a Warn and moves on (correctly non-fatal) — but
- the task artifact (
setAuthRequired) shows only bare text "Authorization required: connect X", no link (the link lived only in the displaced standalone deliverer), and - the
mcp_auth_requiredaudit event carriesserver/subject/deadlinebut noauthorize_url, and in standalone mode nothing reads it.
So a supported config (standalone + Slack) dead-ends the user on any per-subject Slack miss. The documented "falls back to the A2A artifact" only holds when no channel is active — not when the active channel fails for a subject.
Suggest: always publish the A2A-artifact link (durable record on the task) and treat Slack as an additive push, or chain to the artifact on DeliverConsent error. That also gives managed mode a richer backstop than an authorize_url-less audit event.
| if p.consentCanceler == nil { | ||
| return true, fmt.Errorf("consent cancel click for %s/%s but no canceler wired", subject, server) | ||
| } | ||
| if cErr := p.consentCanceler(ctx, subject, server); cErr != nil { |
There was a problem hiding this comment.
[Low] The Cancel handler doesn't verify the clicker is the subject.
handleConsentCancel cancels the parked call for the {subject, server} in the button value without checking interaction.User.ID against the subject. Latent today — v1 never populates Origin, so every prompt is a DM the subject alone sees. But consentTarget already supports origin-thread delivery, and once that's enabled, any member of a shared channel could click Cancel and fail another user's parked consent (a DoS). Worth a clicker-identity check (in.User.ID maps to subject) before wiring origin-thread delivery.
| if out.AuthorizeURL == "" { | ||
| return "", fmt.Errorf("%w: platform authorize response carried no authorize_url", ErrProtocolError) | ||
| } | ||
| return out.AuthorizeURL, nil |
There was a problem hiding this comment.
[Low / defense-in-depth] The platform-returned authorize_url is delivered unvalidated.
This value flows straight to a clickable Slack button (buildConsentPayload → url). The platform is trusted, so this is low — but an https:// scheme assertion here (and optionally a host allowlist) is cheap insurance against a misconfigured or compromised platform turning the bot into a phishing relay. Mirrors the open-redirect reasoning applied to /mcp/oauth/start in #344, where the redirect target was checked to be server-built.
… check (#345 review) [Medium] Standalone + Slack dead-ended the user on a per-subject Slack failure: SetConsentDeliverer is a single slot, so the Slack deliverer displaced the standalone A2A-artifact link deliverer — and on a users.lookupByEmail miss the artifact showed only bare text with no link. Fix: the login link is now ALWAYS published on the task's auth-required artifact (Runner.PublishConsentArtifact, a durable record) and Slack is an additive push on top. Both the standalone deliverer and the cmd/run.go Slack closure publish the artifact first, so a Slack failure still leaves the user a clickable link. Also gives managed mode a richer backstop than an authorize_url-less audit event. [Low] handleConsentCancel now verifies the clicker is the subject (resolves the clicker's email via users.info, compares to the parked subject; fails open only when the email can't be resolved — a DM-only deployment has no cross-user exposure). Latent today (v1 is DM-only) but fails closed once origin-thread delivery lands, preventing a cross-user cancel DoS in a shared channel. [Low] FetchAuthorizeURL now asserts the platform-returned authorize_url is an absolute https URL before it flows to a clickable Slack button — cheap defense-in-depth against a misconfigured/compromised platform (mirrors the open-redirect check on /mcp/oauth/start). Docs updated: the A2A artifact is always published; Slack is additive.
|
All three addressed in 🟡 [Medium] Standalone + Slack dead-ends the user on a per-subject Slack miss — fixed. Took your first suggestion: the login link is now always published on the A2A 🟢 [Low] Cancel clicker-identity — guarded. 🟢 [Low] Unvalidated Local: Merge-ordering unchanged: #344 → retarget this to |
initializ-mk
left a comment
initializ-mk
left a comment
There was a problem hiding this comment.
Re-review — fix commit 1916fd4 ✅
All three findings resolved, each via the stronger option; base retargeted to main; CI green (Test/Lint/Integration/Build). Approve.
🟡 [Medium] Durable link — fixed the unconditional-artifact way
New nil-safe PublishConsentArtifact(...), and cmd/run.go calls it first, always, then the Slack push:
runner.PublishConsentArtifact(taskID, subject, server, link, deadline) // durable, always
return consentAdapter.DeliverConsent(ctx, ...) // additive pushA Slack miss (email not in workspace / missing scope) no longer dead-ends the user — the clickable link is on the task artifact regardless. standaloneConsentDeliverer was refactored onto the same helper (one writer, no duplication), and this also upgrades managed mode's backstop. This is the stronger fix over chain-on-error. Docs updated to match.
🟢 [Low] Cancel clicker-identity guard — fixed with a sound fail-open
parseConsentCancel now surfaces in.User.ID; handleConsentCancel resolves the clicker's email (the existing #313 users.info path) and rejects a non-subject click with a user-facing message. The fail-open-on-resolve-error is well-reasoned: the guard only bites in the origin-thread/shared-channel case where the users:read.email scope is present and resolution succeeds; a DM-only deployment has no cross-user exposure anyway. Both directions tested through the real handleBlockAction dispatch.
One to revisit when origin-thread delivery actually ships: a transient users.info failure would then fail open and admit a non-subject cancel — worth flipping to fail-closed at that point. Not blocking while that path is unwired.
🟢 [Low] authorize_url https validation — fixed as suggested
FetchAuthorizeURL now rejects any non-absolute-https platform URL, referencing the #344 open-redirect check. Correctly scoped: the standalone button opens the agent's own /mcp/oauth/start (self, https) so the raw IdP URL is only followed server-side — the managed fetch is the right and only place this guard is needed.
Clean series. The whole #317 delegated-consent arc (#330 gate → #331 callback → #332 standalone → #343 Slack) now hangs together with the managed invariant intact and a durable, always-present fallback under it.
When a type: user MCP call parks on the auth-required gate, Forge now presents
a "Connect <server>" login link to the requesting user over its existing Slack
connection, in both standalone and managed modes. Delivery is decoupled from
token custody — the same Slack code serves both.
forge-core:
- channels.ConsentDeliverer capability (DeliverConsent + SetConsentCanceler),
ConsentPrompt{Subject,Server,AuthorizeURL,Deadline,Origin}, ChannelOrigin,
ConsentCanceler — mirrors ApprovalDeliverer.
- PlatformConfig.AuthorizeEndpoint + mcp.FetchAuthorizeURL: the managed
consent-URL contract — POST {server, subject} (Bearer agent identity +
tenancy headers) → {authorize_url}. The platform builds the URL with its own
client_id/redirect_uri/state and hosts the callback, so the code + refresh
token never reach Forge.
forge-plugins/slack:
- consent.go: DeliverConsent DMs the subject (users.lookupByEmail →
conversations.open → chat.postMessage) or replies in the Slack origin thread;
Block Kit Connect URL button + optional Cancel; Cancel routes through the
existing block-action dispatch to the wired canceler. Reuses the socket-mode
connection + bot token (adds im:write scope).
forge-cli:
- Runner.AuthorizeURL resolves the link via a provider — standalone builds it
(#332), managed fetches it from the platform, an embedder may inject its own
(SetAuthorizeURLProvider takes precedence). enableManagedConsentProvider
auto-wires the managed HTTP provider; the authorize_endpoint host is merged
into the egress allowlist.
- cmd/run.go wires SetConsentDeliverer + SetConsentCanceler to the first active
consent-capable adapter (mirrors the DEFER block); postMCPConsent helper for
the Cancel path. No adapter → falls back to the A2A artifact / audit event.
Tests: Slack payload shape, DM-by-email + origin-thread delivery, cancel
routing; FetchAuthorizeURL contract; managed provider fetch + embedder
precedence. Docs: managed consent delivery + authorize_endpoint in
docs/mcp/configuration.md.
Stacked on #344 (uses the SetAuthorizeURLProvider seam).
… check (#345 review) [Medium] Standalone + Slack dead-ended the user on a per-subject Slack failure: SetConsentDeliverer is a single slot, so the Slack deliverer displaced the standalone A2A-artifact link deliverer — and on a users.lookupByEmail miss the artifact showed only bare text with no link. Fix: the login link is now ALWAYS published on the task's auth-required artifact (Runner.PublishConsentArtifact, a durable record) and Slack is an additive push on top. Both the standalone deliverer and the cmd/run.go Slack closure publish the artifact first, so a Slack failure still leaves the user a clickable link. Also gives managed mode a richer backstop than an authorize_url-less audit event. [Low] handleConsentCancel now verifies the clicker is the subject (resolves the clicker's email via users.info, compares to the parked subject; fails open only when the email can't be resolved — a DM-only deployment has no cross-user exposure). Latent today (v1 is DM-only) but fails closed once origin-thread delivery lands, preventing a cross-user cancel DoS in a shared channel. [Low] FetchAuthorizeURL now asserts the platform-returned authorize_url is an absolute https URL before it flows to a clickable Slack button — cheap defense-in-depth against a misconfigured/compromised platform (mirrors the open-redirect check on /mcp/oauth/start). Docs updated: the A2A artifact is always published; Slack is additive.
3024144 to
193b123
Compare
1 participant
Implements #343 — Forge-driven Slack delivery of MCP consent prompts. When a
type: userMCP call parks on the auth-required gate (#330), Forge presents a "Connect <server>" login link to the requesting user over its own Slack connection — in both standalone and managed modes.The key principle — delivery leg ⊥ callback leg
Who delivers the link is independent of who receives the code / holds the refresh token (decided by the URL's
redirect_uri/client_id). So the same Slack code serves both modes; the only difference is whereConsentPrompt.AuthorizeURLcomes from:buildStandaloneConsentLink)/mcp/oauth/callback→ in-memorySubjectStoreFetchAuthorizeURL→platform.authorize_endpoint)How managed mode fetches the URL (answering the design question raised in review)
New
platform.authorize_endpoint: Forge POSTs{"server": <ref>, "subject": <email>}(same Bearer agent-identity +Org-Id/Workspace-Idheaders astoken_endpoint) and the platform returns{"authorize_url": "https://…"}, built with its ownclient_id/redirect_uri/state. Forge treats it as opaque and only delivers it. The authorizationcodeand refresh token land at the platform (managed invariant preserved).Layering (mirrors DEFER)
ConsentDeliverercapability (DeliverConsent+SetConsentCanceler),ConsentPrompt,ChannelOrigin,ConsentCanceler.PlatformConfig.AuthorizeEndpoint+FetchAuthorizeURL(the managed contract, mirrorsdoPlatformTokenRequest).consent.go: DM the subject (users.lookupByEmail→conversations.open→chat.postMessage) or reply in the Slack origin thread; Block Kit Connect URL button + optional Cancel; Cancel routes through the existing block-action dispatch. Reuses the socket-mode connection + bot token; adds only theim:writescope.Runner.AuthorizeURL(provider resolves: embedder override › managed › standalone);enableManagedConsentProviderauto-wires the HTTP provider;authorize_endpointhost merged into the egress allowlist.cmd/run.gowires the deliverer + canceler to the first active consent-capable adapter (mirrors the DEFER block);postMCPConsentfor the Cancel path.Fallback
No consent-capable channel active →
SetConsentDelivererisn't called → the link falls back to the A2Aauth-requiredartifact (standalone) /mcp_auth_requiredaudit event (platform-read).mcp_auth_requiredis emitted before delivery, so a Slack outage never strands the parked call.Tests
FetchAuthorizeURL: request shape ({server, subject}+ Bearer), success, empty-subjectErrNoToken, non-200, missingauthorize_url.Runner.AuthorizeURL: managed fetch from platform, embedder-precedence, standalone unwired-in-managed.All
forge-core/{channels,mcp,validate},forge-plugins/channels/slack,forge-cli/{runtime,cmd}suites green; lint 0 issues.Follow-ups (not in this PR)
ChannelOriginfield is in place).ConsentDeliverer.