From 9b0f6cf809240fa3402018edfa422957dfabb032 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20Salavisa?= Date: Mon, 24 Aug 2026 20:31:32 +0000 Subject: [PATCH] fix(plugins): accept same-origin GET on session-authenticated webhooks (#2984) Co-authored-by: Kandev Agent Co-authored-by: Claude Opus 5 (1M context) --- apps/backend/internal/plugins/handlers.go | 71 ++++++++- .../plugins/handlers_webhook_auth_test.go | 146 ++++++++++++++++-- docs/public/plugins-authoring.md | 33 +++- .../requirements/voice-extraction-host.md | 6 +- 4 files changed, 231 insertions(+), 25 deletions(-) diff --git a/apps/backend/internal/plugins/handlers.go b/apps/backend/internal/plugins/handlers.go index 37e3dac58a..e81d0db456 100644 --- a/apps/backend/internal/plugins/handlers.go +++ b/apps/backend/internal/plugins/handlers.go @@ -455,18 +455,73 @@ func webhookCallerAuthorized(ctx *gin.Context, public bool) bool { ctx.AbortWithStatusJSON(http.StatusUnauthorized, gin.H{"error": "authentication required"}) return false } - if identity.SessionID != "" { - origin := ctx.GetHeader("Origin") - if origin == "" || !commonhttpmw.AllowedOrigin(origin, ctx.Request.Host) { - ctx.AbortWithStatusJSON(http.StatusForbidden, gin.H{ - "error": "an accepted Origin is required for session-authenticated webhooks", - }) - return false - } + if identity.SessionID != "" && !webhookSameOriginRequest(ctx) { + ctx.AbortWithStatusJSON(http.StatusForbidden, gin.H{ + "error": "a same-origin request is required for session-authenticated webhooks", + }) + return false } return true } +// webhookSameOriginRequest reports whether a session-authenticated webhook +// call is a genuine same-origin request rather than cross-site forgery. +// +// A session identity rides on an ambient cookie, so a page on another site can +// make the browser attach it; a PAT or the synthetic auth-disabled identity +// cannot be borrowed that way, which is why only sessions reach this check. +// The signal is the request's origin, never its method: Kandev does not +// enforce webhooks[].method and cannot see whether a plugin's GET handler has +// side effects, so exempting safe verbs would reopen exactly the hole this +// gate closes. +// +// The two signals, in order: +// +// - Origin present: decided by httpmw.AllowedOrigin, the shared origin trust +// policy. Every cross-origin request carries Origin, including the SPA's +// own fetch in a split-origin or desktop install (frontend and backend on +// different ports, see apps/web/lib/plugins/host-api.ts), which the browser +// labels Sec-Fetch-Site: same-site. Origin therefore has to decide alone +// here; also demanding same-origin fetch metadata would break those installs. +// +// - Origin absent: decided by Sec-Fetch-Site. Browsers do not send Origin on +// a same-origin GET or HEAD (per Fetch, it is attached to cross-origin +// requests and to same-origin requests whose method is neither GET nor +// HEAD), so an absent Origin is the normal state of a plugin panel polling +// its own webhook, not evidence of a cross-origin caller. Requiring Origin +// unconditionally refused every such poll on any auth-enabled instance. +// +// same-origin and none (a user-initiated request with no initiator: address +// bar, bookmark) are accepted; cross-site and same-site are refused. Refusing +// cross-site closes the one ambient-credential vector left on this route: the +// SameSite=Lax session cookie is not sent on a cross-site subresource request +// or POST, but it *is* sent on a cross-site top-level GET navigation, which +// carries no Origin either. +// +// Neither header is refused. A session cookie with no origin signal at all is +// indistinguishable from a cross-site top-level GET navigation made by a +// browser that predates Fetch Metadata (or behind something stripping it), +// which SameSite=Lax does attach the cookie to, so accepting it would reopen +// the CSRF path this gate exists to close. That costs nothing anyone has +// today: such a request is already refused before this change, so refusing it +// still is not a regression, and the deliberate non-browser caller (curl, a +// CLI, a server-side integration) has a credential built for exactly this, +// the PAT, which is not ambient and is not gated here at all. +func webhookSameOriginRequest(ctx *gin.Context) bool { + if origin := ctx.GetHeader("Origin"); origin != "" { + return commonhttpmw.AllowedOrigin(origin, ctx.Request.Host) + } + // Fetch Metadata values are lowercase per spec; match leniently, which + // cannot admit a value outside the accepted set. An absent header falls to + // the default and is refused along with cross-site and same-site. + switch strings.ToLower(strings.TrimSpace(ctx.GetHeader("Sec-Fetch-Site"))) { + case "same-origin", "none": + return true + default: + return false + } +} + // webhookStatusForResponse validates a plugin-supplied WebhookResponse.Status // against the HTTP status code range net/http's WriteHeader accepts // ([100, 599] — RFC 9110 informational/success/redirection/client diff --git a/apps/backend/internal/plugins/handlers_webhook_auth_test.go b/apps/backend/internal/plugins/handlers_webhook_auth_test.go index b740e215c4..c1e2b323a8 100644 --- a/apps/backend/internal/plugins/handlers_webhook_auth_test.go +++ b/apps/backend/internal/plugins/handlers_webhook_auth_test.go @@ -134,12 +134,23 @@ func TestWebhookAuthGate_AuthenticatedNonPublicReachesRelay(t *testing.T) { } } +// browserSignals is the pair of headers the session CSRF gate reads. A zero +// value models a request carrying neither: a non-browser client replaying a +// session cookie, or a browser predating Fetch Metadata. +type browserSignals struct { + origin string + secFetchSite string +} + func doWebhookIdentityRequest( - router http.Handler, method, path string, identity authn.Identity, origin string, + router http.Handler, method, path string, identity authn.Identity, signals browserSignals, ) *httptest.ResponseRecorder { req := httptest.NewRequest(method, path, strings.NewReader("{}")) - if origin != "" { - req.Header.Set("Origin", origin) + if signals.origin != "" { + req.Header.Set("Origin", signals.origin) + } + if signals.secFetchSite != "" { + req.Header.Set("Sec-Fetch-Site", signals.secFetchSite) } req = req.WithContext(authn.WithIdentity(req.Context(), identity)) rec := httptest.NewRecorder() @@ -147,23 +158,132 @@ func doWebhookIdentityRequest( return rec } -func TestWebhookAuthGate_SessionRequiresAcceptedOrigin(t *testing.T) { +// TestWebhookAuthGate_SessionCSRFSignals is the table over +// (method, Origin, Sec-Fetch-Site, identity) for the session CSRF gate. +// +// The gate exists because a session identity is carried by an ambient cookie, +// so a cross-site page can make the browser attach it. It must therefore +// admit a genuine same-origin request and refuse a cross-site one. The +// discriminating signal is *origin*, never the verb: a plugin's GET webhook +// may have side effects and the host cannot verify that it does not, so +// exempting safe methods would reopen the hole this gate closes. +// +// Sec-Fetch-Site is the signal for the case Origin cannot answer. Browsers do +// not send Origin on a same-origin GET/HEAD (Fetch, "append a request Origin +// header": Origin is attached to cross-origin requests and to same-origin +// requests whose method is neither GET nor HEAD), so an absent Origin is the +// *normal* state of the SPA polling its own plugin webhook, not evidence of a +// cross-origin caller. +func TestWebhookAuthGate_SessionCSRFSignals(t *testing.T) { router, svc := newTestRouter(t) installedWebhookPlugin(t, svc, "kandev-plugin-session-origin", "key1", false) - identity := authn.Identity{UserID: "user-1", Role: authn.RoleMember, SessionID: "session-1"} + session := authn.Identity{UserID: "user-1", Role: authn.RoleMember, SessionID: "session-1"} + pat := authn.Identity{UserID: "user-1", Role: authn.RoleMember, TokenID: "token-1"} path := "/api/plugins/kandev-plugin-session-origin/webhooks/key1" + const ( + blocked = http.StatusForbidden + relayed = http.StatusServiceUnavailable // reached the relay; see reachedRelay + ) + for _, tc := range []struct { - name string - origin string - want int + name string + method string + identity authn.Identity + signals browserSignals + want int }{ - {name: "missing", want: http.StatusForbidden}, - {name: "foreign", origin: "https://attacker.example", want: http.StatusForbidden}, - {name: "same host", origin: "https://example.com", want: http.StatusServiceUnavailable}, + // The case broken in production: a plugin panel polling its own + // webhook same-origin with GET. No Origin is sent, and before this + // gate learned to read Sec-Fetch-Site every such poll was refused 403 + // on any auth-enabled instance. + { + name: "same-origin GET without Origin", method: http.MethodGet, identity: session, + signals: browserSignals{secFetchSite: "same-origin"}, want: relayed, + }, + { + name: "same-origin POST without Origin", method: http.MethodPost, identity: session, + signals: browserSignals{secFetchSite: "same-origin"}, want: relayed, + }, + // Header values are lowercase per the Fetch Metadata spec; matching + // case-insensitively costs nothing and cannot admit a new value. + { + name: "same-origin spelled with mixed case", method: http.MethodGet, identity: session, + signals: browserSignals{secFetchSite: "Same-Origin"}, want: relayed, + }, + // Sec-Fetch-Site: none is a user-initiated request with no initiator + // (address bar, bookmark). A cross-site page cannot cause a request + // to be labelled none, so it is not a CSRF vector. + { + name: "user-initiated navigation", method: http.MethodGet, identity: session, + signals: browserSignals{secFetchSite: "none"}, want: relayed, + }, + // A cross-site top-level GET navigation sends no Origin, and the + // SameSite=Lax session cookie *is* attached to it — this is the one + // ambient-credential vector left on this route, and the gate refuses + // it on the fetch-metadata signal alone. + { + name: "cross-site without Origin", method: http.MethodGet, identity: session, + signals: browserSignals{secFetchSite: "cross-site"}, want: blocked, + }, + { + name: "cross-site POST without Origin", method: http.MethodPost, identity: session, + signals: browserSignals{secFetchSite: "cross-site"}, want: blocked, + }, + // same-site (a sibling subdomain, or the same host on another port) + // is refused too: SameSite=Lax does not stop it, and no legitimate + // caller reaches this route as same-site without also sending Origin + // (see the split-origin case below). + { + name: "same-site without Origin", method: http.MethodGet, identity: session, + signals: browserSignals{secFetchSite: "same-site"}, want: blocked, + }, + // Origin present keeps deciding on its own: a cross-origin fetch + // always carries Origin, and httpmw.AllowedOrigin is the single + // shared origin trust policy. + { + name: "foreign Origin", method: http.MethodPost, identity: session, + signals: browserSignals{origin: "https://attacker.example"}, want: blocked, + }, + { + name: "foreign Origin claiming same-origin", method: http.MethodGet, identity: session, + signals: browserSignals{origin: "https://attacker.example", secFetchSite: "same-origin"}, + want: blocked, + }, + { + name: "accepted Origin", method: http.MethodPost, identity: session, + signals: browserSignals{origin: "https://example.com"}, want: relayed, + }, + // The split-origin deployment host-api.ts documents: the SPA on one + // port fetching the backend on another. That is cross-origin, so the + // browser sends Origin (which AllowedOrigin accepts — it ignores + // ports) and labels it same-site. Deciding on Origin first is what + // keeps this working; demanding same-origin whenever the header is + // present would break every split-origin and desktop install. + { + name: "accepted Origin on another port", method: http.MethodGet, identity: session, + signals: browserSignals{origin: "https://example.com:1420", secFetchSite: "same-site"}, + want: relayed, + }, + // Neither signal: refused. An ambient session cookie with no origin + // evidence is indistinguishable from a cross-site top-level GET + // navigation by a browser predating Fetch Metadata, which + // SameSite=Lax does attach the cookie to. A deliberate non-browser + // caller uses a PAT, which is not gated at all (next case). + { + name: "no browser signals at all", method: http.MethodGet, identity: session, + signals: browserSignals{}, want: blocked, + }, + // The gate is scoped to ambient-credential identities. A PAT is sent + // deliberately by its holder, so a cross-site page cannot borrow it + // and the fetch-metadata signal is irrelevant. + { + name: "PAT identity is not gated", method: http.MethodGet, identity: pat, + signals: browserSignals{secFetchSite: "cross-site"}, want: relayed, + }, } { t.Run(tc.name, func(t *testing.T) { - rec := doWebhookIdentityRequest(router, http.MethodGet, path, identity, tc.origin) + rec := doWebhookIdentityRequest(router, tc.method, path, tc.identity, tc.signals) if rec.Code != tc.want { t.Fatalf("status = %d, want %d, body=%s", rec.Code, tc.want, rec.Body.String()) } @@ -180,7 +300,7 @@ func TestWebhookAuthGate_NonBrowserIdentitiesDoNotRequireOrigin(t *testing.T) { {UserID: "default", Role: authn.RoleAdmin, Synthetic: true}, } for _, identity := range identities { - rec := doWebhookIdentityRequest(router, http.MethodPost, path, identity, "") + rec := doWebhookIdentityRequest(router, http.MethodPost, path, identity, browserSignals{}) if !reachedRelay(rec.Code) { t.Fatalf("identity %+v: status = %d, want 503, body=%s", identity, rec.Code, rec.Body.String()) } diff --git a/docs/public/plugins-authoring.md b/docs/public/plugins-authoring.md index 2469df6455..ec2bc162b4 100644 --- a/docs/public/plugins-authoring.md +++ b/docs/public/plugins-authoring.md @@ -174,6 +174,10 @@ curated React, UI, and app-store surface. for browser UI and billable operations. Kandev does not enforce `webhooks[].method`; public integrations must still validate the provider signature and replay/timestamp rules before side effects. +- An authenticated webhook is reachable from your panel with any method, + `GET` included: `host.api.fetch` is same-origin (or carries an accepted + `Origin` in a split-origin install), which is what the host checks. You do + not need to force a read onto `POST`. - capabilities.auth is the highest-risk capability. A webhook response may assert a verified external identity with X-Kandev-Auth-Login; only assert an email the IdP verified as owned by the subject. See [ADR 0050](../decisions/0050-plugin-external-auth-capability.md). @@ -1984,6 +1988,25 @@ For plugin-UI operations, declare `access: authenticated`; those routes may raise `max_body_bytes` to 16 MiB. Kandev verifies the current user but strips its session cookie and PAT before relaying the remaining headers. +On an instance with authentication enabled, a caller identified by the session +cookie must additionally look same-origin, because that cookie is ambient and a +page on another site could otherwise make the browser send it. The host accepts +the request in exactly two cases: + +- it carries an accepted `Origin`; or +- it carries no `Origin` and `Sec-Fetch-Site` is `same-origin` or `none`. + +Everything else is rejected, including `Sec-Fetch-Site` of `cross-site` or +`same-site` and a request carrying neither header. Browsers omit `Origin` on +same-origin `GET` and `HEAD` requests, so the second rule is what lets a panel +poll its own webhook with `GET`. In practice `host.api.fetch` from an active +`ui.bundle` satisfies it with no work on your part, whatever method you use. + +Callers identified by a PAT are not subject to this check at all, because a PAT +is not ambient. That is the supported way to call an authenticated webhook from +outside a browser (a script, a CLI, your own backend); a replayed session cookie +carries no origin signal and is refused. + ```yaml webhooks: - key: "provider-events" @@ -2192,9 +2215,13 @@ repackaging. - **Trusting webhook metadata:** `webhooks[].method` is informational. Public routes are not authenticated by Kandev, so validate method, signature, timestamp, replay protection, and body before side effects. -- **Authenticated webhook access:** a valid Kandev session or PAT is all the - host checks. Any signed-in user reaches an authenticated webhook, so enforce - your own per-user or per-role rules when the endpoint needs them. +- **Authenticated webhook access:** a valid Kandev session or PAT, plus a + same-origin request for the session case, is all the host checks. Any + signed-in user reaches an authenticated webhook, so enforce your own + per-user or per-role rules when the endpoint needs them. It is not an + authorization boundary, and it is not a CSRF boundary for your own state + either: the host proves the request came from its own origin, not that the + user meant to trigger whatever your handler does. - **Bundling React:** use host.React, host.jsx, and host.ui; a second React or Radix copy breaks shared contexts and portals. - **Shipping the wrong binary name:** every declared executable must be under diff --git a/docs/specs/plugins/requirements/voice-extraction-host.md b/docs/specs/plugins/requirements/voice-extraction-host.md index 8669f506a6..2ba5c34744 100644 --- a/docs/specs/plugins/requirements/voice-extraction-host.md +++ b/docs/specs/plugins/requirements/voice-extraction-host.md @@ -156,7 +156,11 @@ rpc HandleWebhook(WebhookRequest) returns (WebhookResponse) ``` `access` is enforced by the host before `HandleWebhook` runs. For `authenticated`, the normal Kandev -identity must be present and browser cookie authentication additionally requires an accepted Origin; +identity must be present, and a session-cookie identity additionally requires a same-origin request. +Exactly two signals accept it: an accepted Origin, or (when the browser sends no Origin, as it does +not on same-origin GET/HEAD) a `Sec-Fetch-Site` of `same-origin` or `none`. Everything else is +rejected, including `cross-site`, `same-site`, and a request carrying neither header. PAT and +synthetic identities are not ambient and are not subject to that check; authentication-disabled installations use the synthetic default user. Public webhooks retain their current external-call behavior. Request cancellation propagates through the existing RPC context.