diff --git a/.agents/skills/create-kandev-plugin/SKILL.md b/.agents/skills/create-kandev-plugin/SKILL.md index a1571861de..fbc75a9b3c 100644 --- a/.agents/skills/create-kandev-plugin/SKILL.md +++ b/.agents/skills/create-kandev-plugin/SKILL.md @@ -229,12 +229,18 @@ request. Do not silently substitute a directory in the Kandev monorepo. subscriptions, and side effects; Kandev revokes registered routes, slots, handlers, styles, and navigation separately. Use `/mobile-parity` for interaction design and `/e2e` for user-visible flows. -7. Treat webhook routes as public ingress. Follow +7. API v2 webhook routes require a real Kandev caller identity by default. API + v1 keeps omitted access public for compatibility, so new plugins must use + API v2. Follow `docs/public/plugins-authoring.md` for the current body-size and route - limits. Kandev rejects undeclared keys, but it does not authenticate callers - or enforce the manifest's informational `method` field. Validate both before - side effects, return status codes from 100 through 599, and avoid reflecting - unsafe headers or bodies. + limits. Kandev rejects undeclared keys and, unless the manifest declares + `webhooks[].public: true`, rejects anonymous callers with 401 before your + handler runs. Only mark a webhook `public` when it is genuinely third-party + ingress (a provider callback, an SSO initiate/callback pair) that your own + handler authenticates — Kandev does not enforce the manifest's informational + `method` field or verify that a public webhook's own auth is correct. + Validate method and signature before side effects, return status codes from + 100 through 599, and avoid reflecting unsafe headers or bodies. 8. Keep package paths and platform declarations synchronized. Build every executable declared in `runtime.executables`; include `.exe` for Windows. diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 53a60f4dc2..9d97611f7e 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -180,7 +180,7 @@ Standalone agentctl is launched in its own process group so terminal Ctrl+C is h - **WS**: clients carry their identity; dispatched actions and subscriptions are scoped; workspace-carrying events route via `Hub.BroadcastToWorkspace`. A new `hub.Broadcast` (global) call site needs a `//ws:global` justification comment. - **Task overview vs. session detail:** Keep cross-task state in the bounded, persisted `TaskStatusSummary` projection and `task.status_summary.updated`; keep unbounded transcript and execution data session-scoped. The [bounded task-status spec](../../docs/specs/platform/bounded-task-status-delivery.md) and [accepted ADR](../../docs/decisions/2026-08-01-separate-task-summary-session-stream-traffic.md) define the shared contract. - **Port-proxy subtree capability:** with auth enforced, `/port-proxy/:sessionId/:port/*` needs a credential on *every* request, but browser subresource fetches cannot always carry one (the `` fetch never sends cookies; sandboxed iframes and `?token=`-opened previews drop them elsewhere). `PortProxyHandler` mints a short-lived (15 min, sliding), path-scoped, HMAC-signed capability after the document authenticates and propagates it as a cookie scoped to the proxy subtree plus a `kandev_cap` query parameter appended to rewritten asset URLs. `requireConnectionAuth` (`internal/gateway/websocket/access.go`) accepts either form only for the exact session:port it was minted for and restores the issuing identity, so `CheckSessionAccess` still gates on the real owner. Synthetic identities (auth disabled) skip minting — proxied bodies stay byte-identical to the pre-auth behavior. -- **Self-authenticating webhooks** (automation, office channels, plugin webhooks) and `/health`, `/api/v1/features`, `/api/v1/app-state` stay public — the allowlist lives in `auth/httpmw/middleware.go` with a pinning test. +- **Self-authenticating webhooks** (automation and office channels) and `/health`, `/api/v1/features`, `/api/v1/app-state` stay public — the allowlist lives in `auth/httpmw/middleware.go` with a pinning test. Plugin webhooks are not allowlisted: `auth/httpmw` structurally defers only GET/POST relay paths because it cannot read manifests, and `internal/plugins.Controller.webhook` enforces each manifest's `access` policy. ## Execution Flow diff --git a/apps/backend/cmd/plugin-fixture/fixture-package/manifest.yaml b/apps/backend/cmd/plugin-fixture/fixture-package/manifest.yaml index 08ca9a01bc..5eba8e4b53 100644 --- a/apps/backend/cmd/plugin-fixture/fixture-package/manifest.yaml +++ b/apps/backend/cmd/plugin-fixture/fixture-package/manifest.yaml @@ -5,7 +5,7 @@ # the host platform into server/, and packs the whole tree with # cmd/plugin-pack into apps/backend/.build/kandev-plugin-e2e-1.0.0.tar.gz. id: "kandev-plugin-e2e" -api_version: 1 +api_version: 2 version: "1.0.0" display_name: "Kandev E2E Fixture Plugin" description: "SDK-based plugin backend used by backend integration tests and Playwright e2e tests" @@ -85,6 +85,10 @@ webhooks: - key: "test-hook" description: "Records inbound webhooks for e2e polling" method: "POST" + - key: "public-hook" + description: "Public counterpart of test-hook for exercising the anonymous auth-gate opt-in" + method: "POST" + access: public ui: bundle: "/ui/bundle.js" diff --git a/apps/backend/cmd/plugin-fixture/fixture_package_test.go b/apps/backend/cmd/plugin-fixture/fixture_package_test.go index 194ede8e74..1033bec0ae 100644 --- a/apps/backend/cmd/plugin-fixture/fixture_package_test.go +++ b/apps/backend/cmd/plugin-fixture/fixture_package_test.go @@ -21,7 +21,7 @@ func TestFixtureManifest_ParsesAndValidates(t *testing.T) { require.NoError(t, m.Validate()) require.Equal(t, "kandev-plugin-e2e", m.ID) - require.Equal(t, 1, m.APIVersion) + require.Equal(t, manifest.CurrentAPIVersion, m.APIVersion) require.Equal(t, "1.0.0", m.Version) require.True(t, m.IsManaged()) require.Equal(t, "https://github.com/kdlbs/kandev-plugin-template", m.RepoURL) @@ -41,9 +41,12 @@ func TestFixtureManifest_ParsesAndValidates(t *testing.T) { require.Len(t, m.AgentTools, 1) require.Equal(t, "test_echo", m.AgentTools[0].Name) - require.Len(t, m.Webhooks, 1) + require.Len(t, m.Webhooks, 2) require.Equal(t, "test-hook", m.Webhooks[0].Key) require.Equal(t, "POST", m.Webhooks[0].Method) + require.Equal(t, manifest.WebhookAccessAuthenticated, m.Webhooks[0].EffectiveAccess(m.APIVersion), "test-hook exercises the private (auth-gated) webhook path") + require.Equal(t, "public-hook", m.Webhooks[1].Key) + require.Equal(t, manifest.WebhookAccessPublic, m.Webhooks[1].EffectiveAccess(m.APIVersion), "public-hook exercises the anonymous auth-gate opt-in") } func TestFixtureManifest_DeclaresHostPlatformExecutable(t *testing.T) { diff --git a/apps/backend/internal/auth/httpmw/middleware.go b/apps/backend/internal/auth/httpmw/middleware.go index 4cddeb12ac..9ab00ca116 100644 --- a/apps/backend/internal/auth/httpmw/middleware.go +++ b/apps/backend/internal/auth/httpmw/middleware.go @@ -297,6 +297,9 @@ func BearerToken(r *http.Request) string { // isPublicPath is the unauthenticated allowlist. Everything here is either // pre-session bootstrap, a credential-issuing endpoint, or a // self-authenticating webhook (own secret/HMAC validated by its handler). +// Plugin webhooks are NOT in this allowlist — whether a given plugin webhook +// is anonymous-callable depends on its manifest (webhooks[].public), which +// only plugins.Controller.webhook can read; see isPluginWebhookPath below. func isPublicPath(method, path string) bool { switch path { case "/health": @@ -347,9 +350,6 @@ func isPublicPath(method, path string) bool { // GitHub App webhook delivery; HMAC (X-Hub-Signature-256) verified by // the handler, not request identity. return true - case strings.HasPrefix(path, "/api/plugins/") && strings.Contains(path, "/webhooks/"): - // Relayed to the plugin subprocess, which owns signature validation. - return true case strings.HasPrefix(path, "/api/v1/e2e") || strings.HasPrefix(path, "/api/v1/_test/"): // Test-harness routes; only mounted under KANDEV_E2E_MOCK. Never // registered on production binaries. @@ -375,6 +375,12 @@ func isDeferredPath(c *gin.Context, path string) bool { // External MCP enforces PAT auth in its own group middleware // (externalMCPAuthMiddleware) so agent clients get MCP-shaped errors. return true + case isPluginWebhookRelayMethod(c.Request.Method) && isPluginWebhookPath(path): + // Whether this specific webhook is anonymous-callable depends on the + // plugin's manifest (webhooks[].public), which only + // plugins.Controller.webhook can read — it enforces the auth gate + // itself (webhookCallerAuthorized), mirroring the /mcp precedent above. + return true case strings.HasPrefix(path, "/api/v1/office/") && BearerToken(c.Request) != "": // Sandbox office agents call back with an agent JWT (KANDEV_API_KEY); // officeagents.AgentAuthMiddleware validates it. Bearer-less office @@ -388,3 +394,23 @@ func isDeferredPath(c *gin.Context, path string) bool { } return false } + +// isPluginWebhookRelayMethod reports whether method is registered on the plugin +// webhook relay route. Other methods should not bypass the global auth +// challenge just because the path has the relay shape. +func isPluginWebhookRelayMethod(method string) bool { + return method == http.MethodGet || method == http.MethodPost +} + +// isPluginWebhookPath structurally matches /api/plugins//webhooks/ +// with both dynamic segments non-empty — precisely what plugins.RegisterRoutes +// registers for the relay endpoint (POST/GET /:id/webhooks/:key). Splitting +// on "/" (rather than the old strings.Contains(path, "/webhooks/")) avoids +// over-matching unrelated routes that merely contain the substring, such as +// /api/plugins/p1/user-state/task/t1/note/webhooks/k or /api/plugins/webhooks/x. +func isPluginWebhookPath(path string) bool { + parts := strings.Split(path, "/") + return len(parts) == 6 && + parts[0] == "" && parts[1] == "api" && parts[2] == "plugins" && + parts[3] != "" && parts[4] == "webhooks" && parts[5] != "" +} diff --git a/apps/backend/internal/auth/httpmw/middleware_test.go b/apps/backend/internal/auth/httpmw/middleware_test.go index 31de23a9ab..760d1e7e27 100644 --- a/apps/backend/internal/auth/httpmw/middleware_test.go +++ b/apps/backend/internal/auth/httpmw/middleware_test.go @@ -212,7 +212,27 @@ func TestEnabledModeAllowlistMatrix(t *testing.T) { {name: "auth me", method: http.MethodGet, path: "/api/v1/auth/me"}, {name: "automation webhook", method: http.MethodPost, path: "/api/v1/automations/webhook/abc"}, {name: "office channel inbound", method: http.MethodPost, path: "/api/v1/office/channels/ch1/inbound"}, - {name: "plugin webhook", method: http.MethodPost, path: "/api/plugins/p1/webhooks/key1"}, + { + // Deferred, not public: the middleware lets GET/POST + // /api/plugins//webhooks/ requests through structurally + // (isPluginWebhookPath) because it cannot read the plugin manifest to know + // whether this specific webhook is public. This row is therefore a WEAKENED + // pin: it only proves the path still passes the middleware, not that + // anonymous callers reach the subprocess. The real 401-vs-relay policy is + // enforced and tested in internal/plugins: see handlers_webhook_auth_test.go. + name: "plugin webhook (deferred, not a policy pin)", method: http.MethodPost, path: "/api/plugins/p1/webhooks/key1", + }, + { + name: "plugin webhook GET (deferred, not a policy pin)", method: http.MethodGet, + path: "/api/plugins/p1/webhooks/key1", + }, + { + // Unsupported methods are not registered webhook relay routes, so they should + // not bypass the global auth challenge just because the path has the relay + // shape. + name: "plugin webhook unsupported method", method: http.MethodPut, + path: "/api/plugins/p1/webhooks/key1", blocked: true, + }, {name: "ws upgrade deferred", method: http.MethodGet, path: "/ws"}, {name: "terminal deferred", method: http.MethodGet, path: "/terminal/target"}, {name: "vscode proxy deferred", method: http.MethodGet, path: "/vscode/s1/index.html"}, @@ -264,6 +284,19 @@ func TestEnabledModeAllowlistMatrix(t *testing.T) { name: "plugin user-state unauthenticated", method: http.MethodGet, path: "/api/plugins/p1/user-state/task/task1/note", blocked: true, }, + { + // AC6: the old strings.Contains(path, "/webhooks/") match let this + // through as a "plugin webhook"; the structural isPluginWebhookPath + // match (exactly ["", "api", "plugins", id, "webhooks", key]) rejects + // it as unrelated to the webhook relay. + name: "plugin user-state path containing webhooks substring", method: http.MethodGet, + path: "/api/plugins/p1/user-state/task/t1/note/webhooks/k", blocked: true, + }, + { + // AC6: missing plugin id segment — not a real /:id/webhooks/:key route. + name: "plugin webhooks with no id segment", method: http.MethodPost, + path: "/api/plugins/webhooks/x", blocked: true, + }, {name: "debug", method: http.MethodGet, path: "/debug/vars", blocked: true}, { name: "github connections requires auth", method: http.MethodGet, diff --git a/apps/backend/internal/backendapp/auth.go b/apps/backend/internal/backendapp/auth.go index 77d3c6f5d5..266db228ae 100644 --- a/apps/backend/internal/backendapp/auth.go +++ b/apps/backend/internal/backendapp/auth.go @@ -28,6 +28,13 @@ type pluginSSOBridge struct { auth *auth.Service } +// SessionCookieName returns the name of Kandev's own session cookie, used by +// the plugin webhook relay to strip it from headers forwarded to a plugin +// subprocess. +func (b pluginSSOBridge) SessionCookieName() string { + return b.auth.CookieName() +} + func (b pluginSSOBridge) LoginExternal(c *gin.Context, provider, subject, email, displayName string) error { _, token, err := b.auth.AuthenticateExternal(c.Request.Context(), auth.ExternalIdentity{ Provider: provider, diff --git a/apps/backend/internal/backendapp/auth_plugin_bridge_test.go b/apps/backend/internal/backendapp/auth_plugin_bridge_test.go new file mode 100644 index 0000000000..93093b558f --- /dev/null +++ b/apps/backend/internal/backendapp/auth_plugin_bridge_test.go @@ -0,0 +1,23 @@ +package backendapp + +import "testing" + +// TestPluginSSOBridgeSessionCookieName pins the production end of the seam +// internal/plugins.Service.sessionCookieName() reads: the webhook relay strips +// Kandev's session cookie from the headers it forwards to a plugin subprocess, +// and stripSessionCookie skips that work entirely when the name is empty. A +// bridge returning "" would therefore hand every authenticated caller's session +// cookie to the plugin while every flattenHeaders unit test — which passes the +// name in directly — still passed. +func TestPluginSSOBridgeSessionCookieName(t *testing.T) { + svc := newSSOTestAuthService(t) + bridge := pluginSSOBridge{auth: svc} + + got := bridge.SessionCookieName() + if got == "" { + t.Fatal("SessionCookieName() is empty; the webhook relay would forward the caller's session cookie to the plugin subprocess") + } + if want := svc.CookieName(); got != want { + t.Fatalf("SessionCookieName() = %q, want the auth service's cookie name %q", got, want) + } +} diff --git a/apps/backend/internal/plugins/auth_login.go b/apps/backend/internal/plugins/auth_login.go index 852111009d..f4598c2491 100644 --- a/apps/backend/internal/plugins/auth_login.go +++ b/apps/backend/internal/plugins/auth_login.go @@ -40,6 +40,11 @@ type AuthLoginBridge interface { // cookie on c. It returns an error when authentication is not enabled or // the identity cannot be established. LoginExternal(c *gin.Context, provider, subject, email, displayName string) error + + // SessionCookieName returns the name of Kandev's own session cookie, so + // the webhook relay (flattenHeaders) can strip it from the headers + // forwarded to a plugin subprocess. + SessionCookieName() string } // takeAuthLoginDirective removes the reserved SSO login directive header from a diff --git a/apps/backend/internal/plugins/auth_login_test.go b/apps/backend/internal/plugins/auth_login_test.go index cb59abd4e3..b769b83330 100644 --- a/apps/backend/internal/plugins/auth_login_test.go +++ b/apps/backend/internal/plugins/auth_login_test.go @@ -24,6 +24,8 @@ type fakeBridge struct { setCookie bool } +func (b *fakeBridge) SessionCookieName() string { return "kandev_session" } + func (b *fakeBridge) LoginExternal(c *gin.Context, provider, subject, email, displayName string) error { b.called = true b.provider, b.subject, b.email, b.display = provider, subject, email, displayName diff --git a/apps/backend/internal/plugins/handlers.go b/apps/backend/internal/plugins/handlers.go index 13bf82d6a7..37e3dac58a 100644 --- a/apps/backend/internal/plugins/handlers.go +++ b/apps/backend/internal/plugins/handlers.go @@ -13,7 +13,9 @@ import ( "github.com/gin-gonic/gin" "go.uber.org/zap" + "github.com/kandev/kandev/internal/auth" "github.com/kandev/kandev/internal/auth/authn" + commonhttpmw "github.com/kandev/kandev/internal/common/httpmw" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/plugins/manifest" "github.com/kandev/kandev/internal/plugins/pkgtar" @@ -51,14 +53,19 @@ type actionInvoker interface { ) (*pluginsdk.PluginActionResponse, error) } +type webhookInvoker interface { + InvokeWebhook(context.Context, string, *pluginsdk.WebhookRequest) (*pluginsdk.WebhookResponse, error) +} + // Controller holds the plugin HTTP handlers: operator-facing management // (install/list/get/config/uninstall/enable/disable), the bundle/UI // static-file serving (from the extracted package on disk), and the // external webhook relay (HTTP -> Host RPC over the live subprocess). type Controller struct { - svc *Service - log *logger.Logger - actionInvoker actionInvoker + svc *Service + log *logger.Logger + actionInvoker actionInvoker + webhookInvoker webhookInvoker } // RegisterRoutes wires the plugin HTTP surface. deliverer is accepted for @@ -66,7 +73,7 @@ type Controller struct { // alongside this call) — no handler in this file calls it directly, since // Service already notifies it on every install/status change. func RegisterRoutes(router *gin.Engine, svc *Service, _ Deliverer, log *logger.Logger) { - ctrl := &Controller{svc: svc, log: log, actionInvoker: svc} + ctrl := &Controller{svc: svc, log: log, actionInvoker: svc, webhookInvoker: svc} api := router.Group("/api/plugins") api.POST("/install", authn.RequireAdmin(), ctrl.install) @@ -391,45 +398,73 @@ func serveInstalledFile(ctx *gin.Context, root, relPath, contentType string) { // Service.InvokeWebhook, then writes back the plugin's WebhookResponse // verbatim. func (c *Controller) webhook(ctx *gin.Context) { - id := ctx.Param("id") - record, err := c.svc.Get(id) - if err != nil { - c.writeLookupError(ctx, err) + id, key := ctx.Param("id"), ctx.Param("key") + record, lookupErr := c.svc.Get(id) + declaration, declared := findWebhookDeclaration(record, key) + public := declared && declaration.EffectiveAccess(record.APIVersion) == manifest.WebhookAccessPublic + if !webhookCallerAuthorized(ctx, public) { return } - - key := ctx.Param("key") - declaration, ok := findWebhookDeclaration(record, key) - if !ok { + if lookupErr != nil { + c.writeLookupError(ctx, lookupErr) + return + } + if !declared { ctx.JSON(http.StatusNotFound, gin.H{"error": fmt.Sprintf("plugin %q has no webhook %q", id, key)}) return } - if declaration.EffectiveAccess() == manifest.WebhookAccessAuthenticated { - if _, authenticated := authn.FromGin(ctx); !authenticated { - ctx.JSON(http.StatusUnauthorized, gin.H{actionErrorField: authenticationRequiredMessage}) - return - } - } body, err := readCappedWebhookBody(ctx, declaration.EffectiveMaxBodyBytes()) if err != nil { - return // readCappedWebhookBody already wrote the error response + return } req := &pluginsdk.WebhookRequest{ WebhookKey: key, Method: ctx.Request.Method, Query: ctx.Request.URL.RawQuery, - Headers: flattenWebhookHeaders(ctx.Request.Header), + Headers: flattenHeaders(ctx.Request.Header, c.svc.sessionCookieName(), public), Body: body, } - resp, err := c.svc.InvokeWebhook(ctx.Request.Context(), id, req) + leasedRecord, release, err := c.svc.beginPluginDispatch(id, dispatchGeneration(record)) if err != nil { ctx.JSON(http.StatusServiceUnavailable, gin.H{"error": err.Error()}) return } - c.writeWebhookResponse(ctx, record, resp) + defer release() + + resp, err := c.webhookInvoker.InvokeWebhook(ctx.Request.Context(), id, req) + if err != nil { + ctx.JSON(http.StatusServiceUnavailable, gin.H{"error": err.Error()}) + return + } + c.writeWebhookResponse(ctx, leasedRecord, resp) +} + +// webhookCallerAuthorized requires a caller identity unless the declaration +// explicitly opts into public access. It runs before lookup errors to avoid +// revealing installed plugin IDs or declared webhook keys to anonymous callers. +func webhookCallerAuthorized(ctx *gin.Context, public bool) bool { + if public { + return true + } + identity, ok := authn.FromGin(ctx) + if !ok { + ctx.Header("WWW-Authenticate", "Bearer") + 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 + } + } + return true } // webhookStatusForResponse validates a plugin-supplied WebhookResponse.Status @@ -511,9 +546,10 @@ func (c *Controller) applyAuthLogin(ctx *gin.Context, record *store.Record, raw return bridge.LoginExternal(ctx, a.Provider, a.Subject, a.Email, a.DisplayName) } -// manifestDeclaresWebhookKey reports whether record's manifest declares a -// webhooks[] entry with the given key. func findWebhookDeclaration(record *store.Record, key string) (manifest.Webhook, bool) { + if record == nil { + return manifest.Webhook{}, false + } for _, wh := range record.Webhooks { if wh.Key == key { return wh, true @@ -522,11 +558,6 @@ func findWebhookDeclaration(record *store.Record, key string) (manifest.Webhook, return manifest.Webhook{}, false } -func manifestDeclaresWebhookKey(record *store.Record, key string) bool { - _, ok := findWebhookDeclaration(record, key) - return ok -} - // readCappedWebhookBody reads ctx.Request.Body bounded at // maxWebhookBodyBytes via http.MaxBytesReader, writing the 413 response // itself (and returning a non-nil error as a sentinel to the caller) when @@ -553,17 +584,93 @@ func readCappedWebhookBody(ctx *gin.Context, maxBytes int64) ([]byte, error) { // single-valued map[string]string WebhookRequest.Headers expects, // per §3 of docs/plans/plugins/GRPC-CONTRACT.md: multi-valued headers are // joined by ", ". -func flattenHeaders(h http.Header) map[string]string { +func flattenHeaders(h http.Header, sessionCookieName string, public bool) map[string]string { out := make(map[string]string, len(h)) for k, v := range h { - out[k] = strings.Join(v, ", ") + switch http.CanonicalHeaderKey(k) { + case "Cookie": + if !public { + continue + } + stripped, keep := stripSessionCookies(v, sessionCookieName) + if !keep { + continue + } + out[k] = stripped + case "Authorization": + if !public { + continue + } + if containsKandevPATCredential(v) { + continue + } + out[k] = strings.Join(v, ", ") + default: + out[k] = strings.Join(v, ", ") + } } return out } -func flattenWebhookHeaders(h http.Header) map[string]string { - safe := h.Clone() - safe.Del("Authorization") - safe.Del("Cookie") - return flattenHeaders(safe) +func stripSessionCookies(headers []string, sessionCookieName string) (string, bool) { + kept := make([]string, 0, len(headers)) + for _, header := range headers { + stripped, keep := stripSessionCookie(header, sessionCookieName) + if keep { + kept = append(kept, stripped) + } + } + if len(kept) == 0 { + return "", false + } + return strings.Join(kept, "; "), true +} + +func containsKandevPATCredential(values []string) bool { + for _, value := range values { + if isKandevPATCredential(value) || strings.Contains(value, auth.PATPrefix) { + return true + } + } + return false +} + +// isKandevPATCredential reports whether an Authorization header value carries +// a kandev_pat_* token, with or without a bearer scheme prefix. RFC 9110 makes +// the auth scheme case-insensitive, so "bearer kandev_pat_..." has to be +// stripped just like "Bearer kandev_pat_...": httpmw.BearerToken would not have +// authenticated such a request, but the credential still must not be relayed to +// a plugin subprocess (which a public webhook would otherwise receive it as). +func isKandevPATCredential(value string) bool { + const bearerPrefix = "Bearer " + token := strings.TrimSpace(value) + if len(token) > len(bearerPrefix) && strings.EqualFold(token[:len(bearerPrefix)], bearerPrefix) { + token = strings.TrimSpace(token[len(bearerPrefix):]) + } + return strings.HasPrefix(token, auth.PATPrefix) +} + +// stripSessionCookie removes the sessionCookieName cookie from a Cookie +// header value ("a=1; b=2"). keep is false when no cookies remain, so the +// caller can omit the header entirely rather than send an empty one. +func stripSessionCookie(header, sessionCookieName string) (stripped string, keep bool) { + if sessionCookieName == "" { + return header, true + } + parts := strings.Split(header, ";") + kept := make([]string, 0, len(parts)) + for _, p := range parts { + trimmed := strings.TrimSpace(p) + name, _, found := strings.Cut(trimmed, "=") + if found && name == sessionCookieName { + continue + } + if trimmed != "" { + kept = append(kept, trimmed) + } + } + if len(kept) == 0 { + return "", false + } + return strings.Join(kept, "; "), true } diff --git a/apps/backend/internal/plugins/handlers_test.go b/apps/backend/internal/plugins/handlers_test.go index 6d295d67d2..a3931208aa 100644 --- a/apps/backend/internal/plugins/handlers_test.go +++ b/apps/backend/internal/plugins/handlers_test.go @@ -96,6 +96,19 @@ func doRequest(router *gin.Engine, method, path string, body string, headers map return rec } +// doAuthedRequest attaches a resolved caller identity, standing in for +// httpmw.Middleware without wiring it into focused handler tests. +func doAuthedRequest(router *gin.Engine, method, path string, body string, headers map[string]string) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, path, strings.NewReader(body)) + req = req.WithContext(authn.WithIdentity(req.Context(), authn.Identity{UserID: "user_1", Role: authn.RoleMember})) + for k, v := range headers { + req.Header.Set(k, v) + } + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + return rec +} + func doMultipartInstall(t *testing.T, router *gin.Engine, pkg *bytes.Buffer) *httptest.ResponseRecorder { t.Helper() var body bytes.Buffer @@ -465,7 +478,7 @@ runtime: func TestWebhookHandlerUnknownPluginReturns404(t *testing.T) { router, _ := newTestRouter(t) - rec := doRequest(router, http.MethodPost, "/api/plugins/missing/webhooks/key1", "{}", nil) + rec := doAuthedRequest(router, http.MethodPost, "/api/plugins/missing/webhooks/key1", "{}", nil) if rec.Code != http.StatusNotFound { t.Fatalf("status = %d, want 404, body=%s", rec.Code, rec.Body.String()) } @@ -481,7 +494,7 @@ func TestWebhookHandlerUndeclaredKeyReturns404(t *testing.T) { t.Fatalf("Install: %v", err) } - rec := doRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-slack/webhooks/undeclared-key", "{}", nil) + rec := doAuthedRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-slack/webhooks/undeclared-key", "{}", nil) if rec.Code != http.StatusNotFound { t.Fatalf("status = %d, want 404, body=%s", rec.Code, rec.Body.String()) } @@ -497,7 +510,7 @@ func TestWebhookHandlerOversizedBodyReturns413(t *testing.T) { } oversized := strings.Repeat("a", int(maxWebhookBodyBytes+1)) - rec := doRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-slack/webhooks/key1", oversized, nil) + rec := doAuthedRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-slack/webhooks/key1", oversized, nil) if rec.Code != http.StatusRequestEntityTooLarge { t.Fatalf("status = %d, want 413, body=%s", rec.Code, rec.Body.String()) } @@ -527,13 +540,13 @@ func TestReadCappedWebhookBodyUsesDeclaredLimit(t *testing.T) { func TestWebhookRelayHeadersStripHostCredentials(t *testing.T) { headers := http.Header{ - "Authorization": []string{"Bearer kandev-pat"}, + "Authorization": []string{"Bearer kandev_pat_secret"}, "Cookie": []string{"kandev_session=secret"}, "Content-Type": []string{"audio/webm"}, "X-Plugin-Key": []string{"plugin-secret"}, } - got := flattenWebhookHeaders(headers) + got := flattenHeaders(headers, "kandev_session", false) if _, ok := got["Authorization"]; ok { t.Fatal("Authorization header was forwarded to plugin") @@ -555,7 +568,7 @@ func TestWebhookHandlerNotRunningReturns503(t *testing.T) { t.Fatalf("Disable: %v", err) } - rec := doRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-slack/webhooks/key1", "{}", nil) + rec := doAuthedRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-slack/webhooks/key1", "{}", nil) if rec.Code != http.StatusServiceUnavailable { t.Fatalf("status = %d, want 503, body=%s", rec.Code, rec.Body.String()) } diff --git a/apps/backend/internal/plugins/handlers_webhook_auth_test.go b/apps/backend/internal/plugins/handlers_webhook_auth_test.go new file mode 100644 index 0000000000..b740e215c4 --- /dev/null +++ b/apps/backend/internal/plugins/handlers_webhook_auth_test.go @@ -0,0 +1,393 @@ +// handlers_webhook_auth_test.go covers the plugin webhook relay's auth gate +// (AC1-AC5, AC9 of the "require auth for plugin webhooks unless the manifest +// declares them public" plan). It lives in its own file per +// apps/backend/AGENTS.md's revive 800-effective-line file cap: +// handlers_test.go is already close to that limit. +// +// Test technique: PluginRuntime.Get returns a concrete *pluginsdk.RemotePlugin +// (see internal/plugins/types.go), which fakeRuntime (service_test.go) can +// only stand in for as a nil pointer when "running" — calling HandleWebhook +// on it would panic (nil pointer dereference), so these tests never leave a +// plugin running. Instead they reuse the same technique +// TestWebhookHandlerNotRunningReturns503 (handlers_test.go) already +// establishes: with the plugin installed but not running, a request that +// passes the auth gate reaches Service.InvokeWebhook and gets a clean 503 +// "plugin ... is not running" — proof the gate let it through to the relay +// path without needing a live subprocess. A request the gate blocks never +// gets that far and returns 401 instead. 401 vs 503 is therefore the +// discriminator these tests assert on, exactly matching "HandleWebhook is +// never invoked" (AC1) / "reaches the subprocess" (AC2, AC3, AC4) without +// risking the nil-pointer panic. +package plugins + +import ( + "bytes" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + + goruntime "runtime" + + "github.com/kandev/kandev/internal/auth/authn" + "github.com/kandev/kandev/internal/plugins/pkgtar/pkgtartest" +) + +// installedWebhookPlugin installs a plugin declaring exactly one webhook +// (key, with the given access policy), then disables it so the runtime never +// reports it running — see the file doc comment for why. +func installedWebhookPlugin(t *testing.T, svc *Service, id, key string, public bool) { + t.Helper() + platformKey := goruntime.GOOS + "-" + goruntime.GOARCH + manifestYAML := fmt.Sprintf(` +id: %s +api_version: 2 +version: "1.0.0" +display_name: Test Plugin +webhooks: + - key: %s + method: POST + access: %s +runtime: + type: binary + executables: + %s: server/plugin +`, id, key, webhookAccessForTest(public), platformKey) + var buf bytes.Buffer + files := map[string][]byte{ + "manifest.yaml": []byte(manifestYAML), + "server/plugin": []byte("#!/bin/sh\necho fake\n"), + } + if err := pkgtartest.WritePackage(&buf, files); err != nil { + t.Fatalf("WritePackage: %v", err) + } + if _, err := svc.Install(t.Context(), &buf); err != nil { + t.Fatalf("Install(%q): %v", id, err) + } + if err := svc.Disable(id); err != nil { + t.Fatalf("Disable(%q): %v", id, err) + } +} + +func webhookAccessForTest(public bool) string { + if public { + return "public" + } + return "authenticated" +} + +// reachedRelay reports whether a webhook response proves the request reached +// Service.InvokeWebhook (503 "not running") as opposed to being blocked by +// the auth gate (401). +func reachedRelay(code int) bool { return code == http.StatusServiceUnavailable } + +// TestWebhookAuthGate_AnonymousNonPublicBlocked pins AC1: an anonymous +// caller against a webhook without public: true gets 401 with +// WWW-Authenticate: Bearer, and never reaches the relay. +func TestWebhookAuthGate_AnonymousNonPublicBlocked(t *testing.T) { + router, svc := newTestRouter(t) + installedWebhookPlugin(t, svc, "kandev-plugin-private", "key1", false) + + rec := doRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-private/webhooks/key1", "{}", nil) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401, body=%s", rec.Code, rec.Body.String()) + } + if got := rec.Header().Get("WWW-Authenticate"); got != "Bearer" { + t.Fatalf("WWW-Authenticate = %q, want %q", got, "Bearer") + } + if reachedRelay(rec.Code) { + t.Fatal("request reached the relay path; HandleWebhook must never be invoked for an anonymous, non-public webhook") + } +} + +// TestWebhookAuthGate_AnonymousPublicReachesRelay pins AC2: an anonymous +// caller against a webhook declaring public: true passes the gate and +// reaches the relay (503 "not running", not 401 "authentication required"). +func TestWebhookAuthGate_AnonymousPublicReachesRelay(t *testing.T) { + router, svc := newTestRouter(t) + installedWebhookPlugin(t, svc, "kandev-plugin-public", "key1", true) + + rec := doRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-public/webhooks/key1", "{}", nil) + if rec.Code == http.StatusUnauthorized { + t.Fatalf("public webhook was blocked by the auth gate: %d body=%s", rec.Code, rec.Body.String()) + } + if !reachedRelay(rec.Code) { + t.Fatalf("status = %d, want 503 (reached relay), body=%s", rec.Code, rec.Body.String()) + } +} + +// TestWebhookAuthGate_AuthenticatedNonPublicReachesRelay pins AC3: a caller +// carrying a real request identity (session/PAT, modeled here via the same +// context-injection doAuthedRequest uses) reaches the relay for a non-public +// webhook. +func TestWebhookAuthGate_AuthenticatedNonPublicReachesRelay(t *testing.T) { + router, svc := newTestRouter(t) + installedWebhookPlugin(t, svc, "kandev-plugin-private2", "key1", false) + + rec := doAuthedRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-private2/webhooks/key1", "{}", nil) + if rec.Code == http.StatusUnauthorized { + t.Fatalf("authenticated caller was blocked by the auth gate: %d body=%s", rec.Code, rec.Body.String()) + } + if !reachedRelay(rec.Code) { + t.Fatalf("status = %d, want 503 (reached relay), body=%s", rec.Code, rec.Body.String()) + } +} + +func doWebhookIdentityRequest( + router http.Handler, method, path string, identity authn.Identity, origin string, +) *httptest.ResponseRecorder { + req := httptest.NewRequest(method, path, strings.NewReader("{}")) + if origin != "" { + req.Header.Set("Origin", origin) + } + req = req.WithContext(authn.WithIdentity(req.Context(), identity)) + rec := httptest.NewRecorder() + router.ServeHTTP(rec, req) + return rec +} + +func TestWebhookAuthGate_SessionRequiresAcceptedOrigin(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"} + path := "/api/plugins/kandev-plugin-session-origin/webhooks/key1" + + for _, tc := range []struct { + name string + origin string + 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}, + } { + t.Run(tc.name, func(t *testing.T) { + rec := doWebhookIdentityRequest(router, http.MethodGet, path, identity, tc.origin) + if rec.Code != tc.want { + t.Fatalf("status = %d, want %d, body=%s", rec.Code, tc.want, rec.Body.String()) + } + }) + } +} + +func TestWebhookAuthGate_NonBrowserIdentitiesDoNotRequireOrigin(t *testing.T) { + router, svc := newTestRouter(t) + installedWebhookPlugin(t, svc, "kandev-plugin-non-browser", "key1", false) + path := "/api/plugins/kandev-plugin-non-browser/webhooks/key1" + identities := []authn.Identity{ + {UserID: "user-1", Role: authn.RoleMember, TokenID: "token-1"}, + {UserID: "default", Role: authn.RoleAdmin, Synthetic: true}, + } + for _, identity := range identities { + rec := doWebhookIdentityRequest(router, http.MethodPost, path, identity, "") + if !reachedRelay(rec.Code) { + t.Fatalf("identity %+v: status = %d, want 503, body=%s", identity, rec.Code, rec.Body.String()) + } + } +} + +// TestWebhookAuthGate_AuthDisabledReachesRelayRegardlessOfPublic pins AC4: +// with auth disabled, httpmw.Middleware injects a synthetic identity onto +// every request, so both public and non-public webhooks reach the relay — +// this is the no-regression pin for the default (auth-off) configuration. +// This handler test wires no auth middleware at all (see newTestRouter), so +// "auth disabled" is modeled the same way httpmw.Middleware's disabled path +// behaves: a request identity is always present. doAuthedRequest supplies +// that identity; a plain doRequest (used above for the anonymous AC1/AC2 +// cases) supplies none, matching what httpmw.Middleware does only once auth +// is enabled. +func TestWebhookAuthGate_AuthDisabledReachesRelayRegardlessOfPublic(t *testing.T) { + router, svc := newTestRouter(t) + installedWebhookPlugin(t, svc, "kandev-plugin-priv3", "key1", false) + installedWebhookPlugin(t, svc, "kandev-plugin-pub3", "key1", true) + + for _, id := range []string{"kandev-plugin-priv3", "kandev-plugin-pub3"} { + rec := doAuthedRequest(router, http.MethodPost, "/api/plugins/"+id+"/webhooks/key1", "{}", nil) + if !reachedRelay(rec.Code) { + t.Fatalf("%s: status = %d, want 503 (reached relay), body=%s", id, rec.Code, rec.Body.String()) + } + } +} + +// TestWebhookAuthGate_AnonymousUnknownPluginReturns401 pins AC5's first case: +// an anonymous caller against an unknown plugin ID gets 401, not 404 — a +// distinct status here would let an anonymous caller enumerate installed +// plugins. +func TestWebhookAuthGate_AnonymousUnknownPluginReturns401(t *testing.T) { + router, _ := newTestRouter(t) + rec := doRequest(router, http.MethodPost, "/api/plugins/does-not-exist/webhooks/key1", "{}", nil) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401, body=%s", rec.Code, rec.Body.String()) + } +} + +// TestWebhookAuthGate_AnonymousUndeclaredKeyReturns401 pins AC5's second +// case: an anonymous caller against an installed plugin but an undeclared +// webhook key gets 401, not 404. +func TestWebhookAuthGate_AnonymousUndeclaredKeyReturns401(t *testing.T) { + router, svc := newTestRouter(t) + installedWebhookPlugin(t, svc, "kandev-plugin-declared", "key1", false) + + rec := doRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-declared/webhooks/undeclared", "{}", nil) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401, body=%s", rec.Code, rec.Body.String()) + } +} + +// TestWebhookAuthGate_AnonymousNonPublicKeyReturns401 pins AC5's third case: +// an anonymous caller against a declared-but-not-public key gets 401, the +// same status as the unknown-plugin and undeclared-key cases above, so an +// anonymous caller cannot distinguish "no such plugin," "no such webhook," +// and "webhook exists but requires auth." +func TestWebhookAuthGate_AnonymousNonPublicKeyReturns401(t *testing.T) { + router, svc := newTestRouter(t) + installedWebhookPlugin(t, svc, "kandev-plugin-declared2", "key1", false) + + rec := doRequest(router, http.MethodPost, "/api/plugins/kandev-plugin-declared2/webhooks/key1", "{}", nil) + if rec.Code != http.StatusUnauthorized { + t.Fatalf("status = %d, want 401, body=%s", rec.Code, rec.Body.String()) + } +} + +// --- AC9: credential stripping --- + +// TestFlattenHeaders_StripsKandevSessionCookie pins AC9: the Kandev session +// cookie is dropped from the Cookie header before relaying to a plugin +// subprocess, while a non-Kandev cookie in the same header survives. +func TestFlattenHeaders_StripsKandevSessionCookie(t *testing.T) { + h := http.Header{} + h.Set("Cookie", "kandev_session=secret-token; other=keep-me") + + out := flattenHeaders(h, "kandev_session", true) + if got := out["Cookie"]; got != "other=keep-me" { + t.Fatalf("Cookie = %q, want only the surviving non-Kandev cookie", got) + } +} + +// TestFlattenHeaders_OmitsCookieHeaderWhenNothingSurvives pins that the +// Cookie header is dropped entirely (not sent as an empty string) once the +// only cookie present was the Kandev session cookie. +func TestFlattenHeaders_OmitsCookieHeaderWhenNothingSurvives(t *testing.T) { + h := http.Header{} + h.Set("Cookie", "kandev_session=secret-token") + + out := flattenHeaders(h, "kandev_session", true) + if _, present := out["Cookie"]; present { + t.Fatalf("Cookie header present = %v, want omitted entirely", out["Cookie"]) + } +} + +// TestFlattenHeaders_StripsPATBearerButKeepsOtherBearers pins AC9: a +// kandev_pat_* Authorization bearer is dropped, while a non-PAT bearer (a +// provider's own token, which a public webhook may need to verify) survives. +func TestFlattenHeaders_StripsPATBearerButKeepsOtherBearers(t *testing.T) { + pat := http.Header{} + pat.Set("Authorization", "Bearer kandev_pat_abc123_secret") + if out := flattenHeaders(pat, "", true); out["Authorization"] != "" { + t.Fatalf("Authorization = %q, want stripped PAT bearer", out["Authorization"]) + } + + provider := http.Header{} + provider.Set("Authorization", "Bearer provider-issued-token") + if out := flattenHeaders(provider, "", true); out["Authorization"] != "Bearer provider-issued-token" { + t.Fatalf("Authorization = %q, want the non-PAT bearer relayed unchanged", out["Authorization"]) + } +} + +// TestFlattenHeaders_StripsPATBearerCaseInsensitively pins that the bearer +// scheme is matched case-insensitively (RFC 9110): a caller sending +// "bearer kandev_pat_..." is not authenticated by httpmw.BearerToken, so on a +// public webhook the request is relayed — the PAT must still be stripped +// rather than handed to the plugin subprocess. +func TestFlattenHeaders_StripsPATBearerCaseInsensitively(t *testing.T) { + for _, scheme := range []string{"bearer", "BEARER", "BeArEr"} { + h := http.Header{} + h.Set("Authorization", scheme+" kandev_pat_abc123_secret") + if out := flattenHeaders(h, "", true); out["Authorization"] != "" { + t.Fatalf("scheme %q: Authorization = %q, want stripped PAT bearer", scheme, out["Authorization"]) + } + } + + // A bare PAT with no scheme at all is stripped too. + bare := http.Header{} + bare.Set("Authorization", "kandev_pat_abc123_secret") + if out := flattenHeaders(bare, "", true); out["Authorization"] != "" { + t.Fatalf("Authorization = %q, want stripped bare PAT", out["Authorization"]) + } +} + +// TestFlattenHeaders_StripsSessionCookieFromRepeatedHeaders proves that a client +// cannot hide the Kandev session by splitting cookies over repeated fields. +func TestFlattenHeaders_StripsSessionCookieFromRepeatedHeaders(t *testing.T) { + h := http.Header{} + h.Add("Cookie", "other=keep-me") + h.Add("Cookie", "kandev_session=secret-token") + + out := flattenHeaders(h, "kandev_session", true) + if got := out["Cookie"]; got != "other=keep-me" { + t.Fatalf("Cookie = %q, want only the non-Kandev cookie", got) + } +} + +// TestFlattenHeaders_StripsPATFromRepeatedAuthorizationHeaders proves that a +// Kandev PAT cannot be hidden behind a provider bearer in another field. +func TestFlattenHeaders_StripsPATFromRepeatedAuthorizationHeaders(t *testing.T) { + h := http.Header{} + h.Add("Authorization", "Bearer provider-issued-token") + h.Add("Authorization", "Bearer kandev_pat_abc123_secret") + + if out := flattenHeaders(h, "", true); out["Authorization"] != "" { + t.Fatalf("Authorization = %q, want omitted when any field carries a PAT", out["Authorization"]) + } +} + +// TestServiceSessionCookieName_ReflectsWiredBridge pins the seam the +// flattenHeaders tests above cannot reach: they pass the cookie name in +// directly, so they stay green no matter what Service.sessionCookieName() +// actually resolves to at runtime. Because stripSessionCookie no-ops on an +// empty name, a wired bridge reporting "" would silently forward every +// authenticated caller's kandev_session cookie to the plugin subprocess. +func TestServiceSessionCookieName_ReflectsWiredBridge(t *testing.T) { + _, svc := newTestRouter(t) + + if got := svc.sessionCookieName(); got != "" { + t.Fatalf("sessionCookieName() = %q with no bridge wired, want %q", got, "") + } + + svc.SetAuthLoginBridge(&fakeBridge{}) + if got := svc.sessionCookieName(); got != "kandev_session" { + t.Fatalf("sessionCookieName() = %q after wiring the bridge, want the bridge's own name", got) + } +} + +// TestFlattenHeaders_NoSessionCookieNameSkipsStripping pins that an empty +// sessionCookieName (no auth bridge wired — auth disabled entirely, so no +// session cookie is ever minted) relays the Cookie header unchanged. +func TestFlattenHeaders_NoSessionCookieNameSkipsStripping(t *testing.T) { + h := http.Header{} + h.Set("Cookie", "a=1; b=2") + + out := flattenHeaders(h, "", true) + if out["Cookie"] != "a=1; b=2" { + t.Fatalf("Cookie = %q, want unchanged when no session cookie name is configured", out["Cookie"]) + } +} + +func TestFlattenHeaders_AuthenticatedWebhookDropsAmbientCredentials(t *testing.T) { + h := http.Header{} + h.Set("Authorization", "Bearer reverse-proxy-token") + h.Set("Cookie", "CF_Authorization=ambient; _oauth2_proxy=ambient") + h.Set("Content-Type", "application/json") + + out := flattenHeaders(h, "kandev_session", false) + if _, present := out["Authorization"]; present { + t.Fatalf("Authorization = %q, want omitted for authenticated webhook", out["Authorization"]) + } + if _, present := out["Cookie"]; present { + t.Fatalf("Cookie = %q, want omitted for authenticated webhook", out["Cookie"]) + } + if got := out["Content-Type"]; got != "application/json" { + t.Fatalf("Content-Type = %q, want application/json", got) + } +} diff --git a/apps/backend/internal/plugins/handlers_webhook_lifecycle_test.go b/apps/backend/internal/plugins/handlers_webhook_lifecycle_test.go new file mode 100644 index 0000000000..3c020456c9 --- /dev/null +++ b/apps/backend/internal/plugins/handlers_webhook_lifecycle_test.go @@ -0,0 +1,238 @@ +package plugins + +import ( + "bytes" + "context" + "fmt" + "io" + "net/http" + "net/http/httptest" + "runtime" + "sync" + "testing" + "time" + + "github.com/gin-gonic/gin" + + "github.com/kandev/kandev/internal/plugins/pkgtar/pkgtartest" + "github.com/kandev/kandev/pkg/pluginsdk" +) + +type blockingWebhookInvoker struct { + entered chan struct{} + proceed chan struct{} + response *pluginsdk.WebhookResponse +} + +type recordingWebhookInvoker struct { + calls int +} + +func (i *recordingWebhookInvoker) InvokeWebhook( + _ context.Context, _ string, _ *pluginsdk.WebhookRequest, +) (*pluginsdk.WebhookResponse, error) { + i.calls++ + return &pluginsdk.WebhookResponse{Status: http.StatusOK}, nil +} + +type gatedRequestBody struct { + entered chan struct{} + proceed chan struct{} + once sync.Once + data []byte + offset int +} + +func (b *gatedRequestBody) Read(p []byte) (int, error) { + b.once.Do(func() { + close(b.entered) + <-b.proceed + }) + if b.offset == len(b.data) { + return 0, io.EOF + } + n := copy(p, b.data[b.offset:]) + b.offset += n + return n, nil +} + +func (*gatedRequestBody) Close() error { return nil } + +func (i *blockingWebhookInvoker) InvokeWebhook( + _ context.Context, _ string, _ *pluginsdk.WebhookRequest, +) (*pluginsdk.WebhookResponse, error) { + close(i.entered) + <-i.proceed + return i.response, nil +} + +type blockingAuthLoginBridge struct { + entered chan struct{} + proceed chan struct{} +} + +func (b *blockingAuthLoginBridge) SessionCookieName() string { return "kandev_session" } + +func (b *blockingAuthLoginBridge) LoginExternal( + c *gin.Context, _, _, _, _ string, +) error { + close(b.entered) + <-b.proceed + c.SetCookie("kandev_session", "minted-by-generation-a", 3600, "/", "", false, true) + return nil +} + +func lifecycleWebhookPackage(t *testing.T, id, version, access string, authCapability bool) *bytes.Buffer { + t.Helper() + capabilities := "" + if authCapability { + capabilities = "capabilities:\n auth: true\n" + } + manifestYAML := fmt.Sprintf(` +id: %s +api_version: 2 +version: %q +display_name: Lifecycle Plugin +%swebhooks: + - key: callback + access: %s +runtime: + type: binary + executables: + %s-%s: server/plugin +`, id, version, capabilities, access, runtime.GOOS, runtime.GOARCH) + var buf bytes.Buffer + if err := pkgtartest.WritePackage(&buf, map[string][]byte{ + "manifest.yaml": []byte(manifestYAML), + "server/plugin": []byte("#!/bin/sh\necho fake\n"), + }); err != nil { + t.Fatalf("WritePackage: %v", err) + } + return &buf +} + +func TestWebhookDispatchLeaseCoversRPCAndAuthLoginResponse(t *testing.T) { + const id = "kandev-plugin-generation" + svc, _, _ := newTestService(t) + if _, err := svc.Install(t.Context(), lifecycleWebhookPackage(t, id, "1.0.0", "public", true)); err != nil { + t.Fatalf("Install generation A: %v", err) + } + + invoker := &blockingWebhookInvoker{ + entered: make(chan struct{}), + proceed: make(chan struct{}), + response: &pluginsdk.WebhookResponse{Status: http.StatusFound, Headers: map[string]string{ + authLoginHeader: authAssertionHeader(), + }}, + } + bridge := &blockingAuthLoginBridge{entered: make(chan struct{}), proceed: make(chan struct{})} + svc.SetAuthLoginBridge(bridge) + gin.SetMode(gin.TestMode) + router := gin.New() + ctrl := &Controller{svc: svc, log: testLogger(t), webhookInvoker: invoker} + router.GET("/api/plugins/:id/webhooks/:key", ctrl.webhook) + + response := make(chan *httptest.ResponseRecorder, 1) + go func() { + response <- doRequest(router, http.MethodGet, "/api/plugins/"+id+"/webhooks/callback", "", nil) + }() + select { + case <-invoker.entered: + case <-time.After(5 * time.Second): + t.Fatal("generation A webhook did not enter the plugin RPC") + } + + replacement := lifecycleWebhookPackage(t, id, "2.0.0", "authenticated", false) + upgrade := make(chan error, 1) + go func() { + _, err := svc.Install(t.Context(), replacement) + upgrade <- err + }() + select { + case err := <-upgrade: + t.Fatalf("generation B installed while generation A RPC was active: %v", err) + case <-time.After(100 * time.Millisecond): + } + + close(invoker.proceed) + select { + case <-bridge.entered: + case <-time.After(5 * time.Second): + t.Fatal("generation A response did not reach the auth login bridge") + } + select { + case err := <-upgrade: + t.Fatalf("generation B installed while generation A auth response was active: %v", err) + case <-time.After(100 * time.Millisecond): + } + + close(bridge.proceed) + select { + case rec := <-response: + if rec.Code != http.StatusFound { + t.Fatalf("webhook status = %d, want 302, body=%s", rec.Code, rec.Body.String()) + } + case <-time.After(5 * time.Second): + t.Fatal("generation A webhook did not finish") + } + select { + case err := <-upgrade: + if err != nil { + t.Fatalf("Install generation B: %v", err) + } + case <-time.After(5 * time.Second): + t.Fatal("generation B remained blocked after the response completed") + } + current, err := svc.Get(id) + if err != nil || current.Version != "2.0.0" || current.Capabilities.Auth { + t.Fatalf("current generation = %+v, err=%v, want non-auth generation 2.0.0", current, err) + } +} + +func TestWebhookGenerationChangeAfterAuthorizationDoesNotInvokeReplacement(t *testing.T) { + const id = "kandev-plugin-generation-race" + svc, _, _ := newTestService(t) + if _, err := svc.Install(t.Context(), lifecycleWebhookPackage(t, id, "1.0.0", "public", false)); err != nil { + t.Fatalf("Install public generation A: %v", err) + } + + invoker := &recordingWebhookInvoker{} + gin.SetMode(gin.TestMode) + router := gin.New() + ctrl := &Controller{svc: svc, log: testLogger(t), webhookInvoker: invoker} + router.POST("/api/plugins/:id/webhooks/:key", ctrl.webhook) + body := &gatedRequestBody{ + entered: make(chan struct{}), + proceed: make(chan struct{}), + data: []byte("{}"), + } + req := httptest.NewRequest(http.MethodPost, "/api/plugins/"+id+"/webhooks/callback", nil) + req.Body = body + rec := httptest.NewRecorder() + done := make(chan struct{}) + go func() { + router.ServeHTTP(rec, req) + close(done) + }() + select { + case <-body.entered: + case <-time.After(5 * time.Second): + t.Fatal("generation A request did not reach body read after authorization") + } + + if _, err := svc.Install(t.Context(), lifecycleWebhookPackage(t, id, "2.0.0", "authenticated", false)); err != nil { + t.Fatalf("Install authenticated generation B: %v", err) + } + close(body.proceed) + select { + case <-done: + case <-time.After(5 * time.Second): + t.Fatal("generation A request did not finish after body release") + } + if rec.Code != http.StatusServiceUnavailable { + t.Fatalf("status = %d, want 503 for stale generation, body=%s", rec.Code, rec.Body.String()) + } + if invoker.calls != 0 { + t.Fatalf("replacement invocations = %d, want 0", invoker.calls) + } +} diff --git a/apps/backend/internal/plugins/manifest/manifest.go b/apps/backend/internal/plugins/manifest/manifest.go index ee20e52af2..6cf0045425 100644 --- a/apps/backend/internal/plugins/manifest/manifest.go +++ b/apps/backend/internal/plugins/manifest/manifest.go @@ -14,6 +14,15 @@ import ( // declare. Individual actions must still declare their own smaller bound. const MaxActionBodyBytes = 1 << 20 +const ( + // LegacyAPIVersion preserves the original webhook behavior: an omitted + // access field means public. + LegacyAPIVersion = 1 + // CurrentAPIVersion makes omitted webhook access authenticated. Authors can + // still opt individual integration callbacks into public access. + CurrentAPIVersion = 2 +) + // Manifest is the plugin registration manifest. type Manifest struct { ID string `yaml:"id" json:"id"` @@ -149,7 +158,9 @@ type AuthProvider struct { Initiate string `yaml:"initiate" json:"initiate"` } -// Webhook is a proxied external webhook endpoint the plugin declares. +// Webhook is a proxied external webhook endpoint the plugin declares. The +// manifest api_version selects the default when Access is omitted: v1 remains +// public for compatibility, while v2 defaults to authenticated. type Webhook struct { Key string `yaml:"key" json:"key"` Description string `yaml:"description,omitempty" json:"description,omitempty"` @@ -165,9 +176,12 @@ const ( MaximumWebhookMaxBodyBytes int64 = 16 << 20 ) -func (w Webhook) EffectiveAccess() string { +func (w Webhook) EffectiveAccess(apiVersion int) string { if w.Access == "" { - return WebhookAccessPublic + if apiVersion == LegacyAPIVersion { + return WebhookAccessPublic + } + return WebhookAccessAuthenticated } return w.Access } diff --git a/apps/backend/internal/plugins/manifest/manifest_test.go b/apps/backend/internal/plugins/manifest/manifest_test.go index aeac76de2b..2491ead1a4 100644 --- a/apps/backend/internal/plugins/manifest/manifest_test.go +++ b/apps/backend/internal/plugins/manifest/manifest_test.go @@ -3,6 +3,8 @@ package manifest import ( "strings" "testing" + + "gopkg.in/yaml.v3" ) const validManifestYAML = ` @@ -81,6 +83,48 @@ func TestParse_UserStateCapabilityDefaultsFalse(t *testing.T) { } } +// TestParse_WebhookAccessRoundTrips pins that webhooks[].access parses +// (AC10): the auth gate reads this field to decide whether a webhook is +// reachable without a caller identity. +func TestParse_WebhookAccessRoundTrips(t *testing.T) { + input := strings.Replace(validManifestYAML, `method: "POST"`, "method: \"POST\"\n access: public", 1) + m, err := Parse([]byte(input)) + if err != nil { + t.Fatalf("Parse() unexpected error: %v", err) + } + encoded, err := yaml.Marshal(m) + if err != nil { + t.Fatalf("Marshal() unexpected error: %v", err) + } + m, err = Parse(encoded) + if err != nil { + t.Fatalf("Parse(round trip) unexpected error: %v", err) + } + if len(m.Webhooks) != 1 || m.Webhooks[0].EffectiveAccess(m.APIVersion) != WebhookAccessPublic { + t.Fatalf("Webhooks[0].EffectiveAccess() = %+v, want a single public webhook", m.Webhooks) + } +} + +func TestWebhookAccessDefaultIsVersioned(t *testing.T) { + webhook := Webhook{Key: "events"} + if got := webhook.EffectiveAccess(LegacyAPIVersion); got != WebhookAccessPublic { + t.Fatalf("v1 EffectiveAccess() = %q, want public", got) + } + if got := webhook.EffectiveAccess(CurrentAPIVersion); got != WebhookAccessAuthenticated { + t.Fatalf("v2 EffectiveAccess() = %q, want authenticated", got) + } +} + +func TestValidateAcceptsSupportedAPIVersions(t *testing.T) { + for _, version := range []int{LegacyAPIVersion, CurrentAPIVersion} { + m := validManifest(t) + m.APIVersion = version + if err := m.Validate(); err != nil { + t.Fatalf("api_version %d: Validate() unexpected error: %v", version, err) + } + } +} + func TestValidate_ValidManifestPasses(t *testing.T) { m, err := Parse([]byte(validManifestYAML)) if err != nil { @@ -529,7 +573,7 @@ func TestValidate_RejectsInvalidManifests(t *testing.T) { }, { name: "unsupported api_version", - mutate: func(m *Manifest) { m.APIVersion = 2 }, + mutate: func(m *Manifest) { m.APIVersion = CurrentAPIVersion + 1 }, wantErr: "api_version", }, { diff --git a/apps/backend/internal/plugins/manifest/validate.go b/apps/backend/internal/plugins/manifest/validate.go index 3c4c1965a0..1494330dc5 100644 --- a/apps/backend/internal/plugins/manifest/validate.go +++ b/apps/backend/internal/plugins/manifest/validate.go @@ -23,9 +23,6 @@ var idPattern = regexp.MustCompile(`^[a-z0-9][a-z0-9._-]*$`) var actionKeyPattern = regexp.MustCompile(`^[a-z0-9][a-z0-9._-]*$`) var referenceIdentityPattern = regexp.MustCompile(`^[a-z0-9][a-z0-9._:-]{0,127}$`) -// supportedAPIVersion is the only api_version this kandev build accepts. -const supportedAPIVersion = 1 - // validCategories are the allowed values for Manifest.Categories entries. var validCategories = map[string]bool{ "connector": true, @@ -260,8 +257,11 @@ func (m *Manifest) validateIdentity() []error { } else if strings.HasSuffix(m.ID, dotConfigSuffix) { errs = append(errs, fmt.Errorf("invalid plugin id %q: must not end in %q", m.ID, dotConfigSuffix)) } - if m.APIVersion != supportedAPIVersion { - errs = append(errs, fmt.Errorf("unsupported api_version %d: only %d is supported", m.APIVersion, supportedAPIVersion)) + if m.APIVersion < LegacyAPIVersion || m.APIVersion > CurrentAPIVersion { + errs = append(errs, fmt.Errorf( + "unsupported api_version %d: supported versions are %d through %d", + m.APIVersion, LegacyAPIVersion, CurrentAPIVersion, + )) } return errs } @@ -525,13 +525,13 @@ func (m *Manifest) validateWebhooks() []error { seen := make(map[string]bool, len(m.Webhooks)) var errs []error for _, wh := range m.Webhooks { - if access := wh.EffectiveAccess(); access != WebhookAccessPublic && access != WebhookAccessAuthenticated { + if access := wh.EffectiveAccess(m.APIVersion); access != WebhookAccessPublic && access != WebhookAccessAuthenticated { errs = append(errs, fmt.Errorf("webhook %q access %q is invalid", wh.Key, wh.Access)) } if wh.MaxBodyBytes < 0 || wh.MaxBodyBytes > MaximumWebhookMaxBodyBytes { errs = append(errs, fmt.Errorf("webhook %q max_body_bytes must be between 1 and %d when set", wh.Key, MaximumWebhookMaxBodyBytes)) } - if wh.EffectiveAccess() == WebhookAccessPublic && wh.MaxBodyBytes > DefaultWebhookMaxBodyBytes { + if wh.EffectiveAccess(m.APIVersion) == WebhookAccessPublic && wh.MaxBodyBytes > DefaultWebhookMaxBodyBytes { errs = append(errs, fmt.Errorf("webhook %q must use authenticated access when max_body_bytes exceeds %d", wh.Key, DefaultWebhookMaxBodyBytes)) } if seen[wh.Key] { diff --git a/apps/backend/internal/plugins/provider.go b/apps/backend/internal/plugins/provider.go index b245dc9010..0528892d46 100644 --- a/apps/backend/internal/plugins/provider.go +++ b/apps/backend/internal/plugins/provider.go @@ -72,6 +72,7 @@ func Provide(cfg *config.Config, dbPool *db.Pool, secrets SecretVault, eventBus } svc := NewService(pluginStore, registry, eventBus, log) + svc.warnLoadedWebhookAccessIssues() svc.SetState(stateStore) svc.SetUserState(userStateStore) svc.SetSecrets(secrets) diff --git a/apps/backend/internal/plugins/service.go b/apps/backend/internal/plugins/service.go index dbd6ee0e34..5d692ce759 100644 --- a/apps/backend/internal/plugins/service.go +++ b/apps/backend/internal/plugins/service.go @@ -482,6 +482,18 @@ func (s *Service) authLoginBridge() AuthLoginBridge { return s.authLogin } +// sessionCookieName returns the name of Kandev's own session cookie via the +// wired auth bridge, or "" when no bridge is wired (auth disabled entirely, +// so no session cookie is ever minted). Used by the webhook relay to strip +// that cookie before forwarding headers to a plugin subprocess. +func (s *Service) sessionCookieName() string { + bridge := s.authLoginBridge() + if bridge == nil { + return "" + } + return bridge.SessionCookieName() +} + // SetKandevVersion wires the currently running kandev build version, // enabling Install to enforce a package's manifest.min_kandev_version // (checkMinKandevVersion): a package requiring a newer kandev is rejected diff --git a/apps/backend/internal/plugins/service_install.go b/apps/backend/internal/plugins/service_install.go index 850b967fce..78324d89b5 100644 --- a/apps/backend/internal/plugins/service_install.go +++ b/apps/backend/internal/plugins/service_install.go @@ -54,6 +54,7 @@ func (s *Service) Install(ctx context.Context, r io.Reader) (*store.Record, erro _ = os.RemoveAll(result.InstallPath) return nil, err } + s.warnWebhookAccessIssues(*result.Manifest) s.agentToolInstallMu.Lock() catalogLocked := true defer func() { diff --git a/apps/backend/internal/plugins/service_sync.go b/apps/backend/internal/plugins/service_sync.go index 4505d9c6d5..01cdac78b6 100644 --- a/apps/backend/internal/plugins/service_sync.go +++ b/apps/backend/internal/plugins/service_sync.go @@ -208,6 +208,7 @@ func (s *Service) registerSideload(id, version string) error { return fmt.Errorf("persist sideloaded record: %w", err) } s.registry.Add(rec) + s.warnWebhookAccessIssues(rec.Manifest) return nil } diff --git a/apps/backend/internal/plugins/sso.go b/apps/backend/internal/plugins/sso.go index e2c990ad3f..5d01f8f89b 100644 --- a/apps/backend/internal/plugins/sso.go +++ b/apps/backend/internal/plugins/sso.go @@ -1,5 +1,7 @@ package plugins +import "github.com/kandev/kandev/internal/plugins/manifest" + // SSOProvider is a login option surfaced on the pre-auth login screen, // contributed by an active auth-capable plugin's manifest auth_providers. // InitiateURL is the plugin webhook the login button navigates to, which @@ -13,7 +15,13 @@ type SSOProvider struct { // SSOProviders returns the external login options from every active, // auth-capable plugin that declares auth_providers. A provider whose Initiate // does not match one of the plugin's declared webhook keys is skipped (its -// button would 404), mirroring the webhook handler's own key check. +// button would 404), mirroring the webhook handler's own key check. A +// provider whose Initiate webhook does not have `access: public` is also +// skipped: the login button is the visitor's very first request, with no +// session or PAT to present, so a non-public initiate webhook would 401 the +// moment it's clicked. This is a clean skip rather than a manifest +// validation error, so an unpatched SSO plugin degrades to "no login button" +// instead of an error state (see docs/decisions for the rationale). func (s *Service) SSOProviders() []SSOProvider { var out []SSOProvider for _, rec := range s.List() { @@ -21,7 +29,9 @@ func (s *Service) SSOProviders() []SSOProvider { continue } for _, p := range rec.AuthProviders { - if p.ID == "" || p.Initiate == "" || !manifestDeclaresWebhookKey(rec, p.Initiate) { + wh, declared := findWebhookDeclaration(rec, p.Initiate) + if p.ID == "" || p.Initiate == "" || !declared || + wh.EffectiveAccess(rec.APIVersion) != manifest.WebhookAccessPublic { continue } out = append(out, SSOProvider{ diff --git a/apps/backend/internal/plugins/sso_test.go b/apps/backend/internal/plugins/sso_test.go index b9f1ca0279..40478a66c4 100644 --- a/apps/backend/internal/plugins/sso_test.go +++ b/apps/backend/internal/plugins/sso_test.go @@ -7,14 +7,24 @@ import ( "github.com/kandev/kandev/internal/plugins/store" ) +// webhookAccess assigns access declarations to a test record's webhooks. +type webhookAccess map[string]string + func ssoRecord(id string, status string, auth bool, webhooks []string, providers []manifest.AuthProvider) *store.Record { + return ssoRecordWithAccess(id, status, auth, webhooks, nil, providers) +} + +func ssoRecordWithAccess( + id, status string, auth bool, webhooks []string, access webhookAccess, providers []manifest.AuthProvider, +) *store.Record { whs := make([]manifest.Webhook, 0, len(webhooks)) for _, k := range webhooks { - whs = append(whs, manifest.Webhook{Key: k}) + whs = append(whs, manifest.Webhook{Key: k, Access: access[k]}) } return &store.Record{ Status: status, Manifest: manifest.Manifest{ + APIVersion: manifest.CurrentAPIVersion, ID: id, Capabilities: manifest.Capabilities{Auth: auth}, Webhooks: whs, @@ -23,19 +33,35 @@ func ssoRecord(id string, status string, auth bool, webhooks []string, providers } } +func TestSSOProvidersPreservesLegacyPublicDefault(t *testing.T) { + record := ssoRecordWithAccess("legacy", StatusActive, true, []string{"initiate"}, nil, + []manifest.AuthProvider{{ID: "legacy", DisplayName: "Legacy", Initiate: "initiate"}}) + record.APIVersion = manifest.LegacyAPIVersion + reg := NewRegistry() + reg.Add(record) + + svc := &Service{registry: reg} + if got := svc.SSOProviders(); len(got) != 1 { + t.Fatalf("SSOProviders() = %d providers, want 1 for v1 public default: %+v", len(got), got) + } +} + func TestSSOProviders(t *testing.T) { reg := NewRegistry() - // Valid: active, auth-capable, provider names a declared webhook. - reg.Add(ssoRecord("google", StatusActive, true, []string{"initiate", "callback"}, + // Valid: active, auth-capable, provider names a declared, public webhook. + reg.Add(ssoRecordWithAccess("google", StatusActive, true, []string{"initiate", "callback"}, + webhookAccess{"initiate": manifest.WebhookAccessPublic, "callback": manifest.WebhookAccessPublic}, []manifest.AuthProvider{{ID: "google", DisplayName: "Google", Initiate: "initiate"}})) // Skipped: provider names an undeclared webhook (button would 404). reg.Add(ssoRecord("broken", StatusActive, true, nil, []manifest.AuthProvider{{ID: "x", DisplayName: "X", Initiate: "missing"}})) // Skipped: declares providers but lacks the auth capability. - reg.Add(ssoRecord("nocap", StatusActive, false, []string{"initiate"}, + reg.Add(ssoRecordWithAccess("nocap", StatusActive, false, []string{"initiate"}, + webhookAccess{"initiate": manifest.WebhookAccessPublic}, []manifest.AuthProvider{{ID: "y", DisplayName: "Y", Initiate: "initiate"}})) // Skipped: inactive plugin. - reg.Add(ssoRecord("inactive", store.StatusDisabled, true, []string{"initiate"}, + reg.Add(ssoRecordWithAccess("inactive", store.StatusDisabled, true, []string{"initiate"}, + webhookAccess{"initiate": manifest.WebhookAccessPublic}, []manifest.AuthProvider{{ID: "z", DisplayName: "Z", Initiate: "initiate"}})) svc := &Service{registry: reg} @@ -50,3 +76,28 @@ func TestSSOProviders(t *testing.T) { t.Fatalf("unexpected provider: %+v", p) } } + +// TestSSOProvidersSkipsNonPublicInitiateWebhook pins AC8: a provider whose +// initiate key is not `access: public` is omitted (the login +// button's very first request has no session or PAT to present, so a +// non-public initiate would 401 immediately), and the same provider is +// included once the manifest marks that key public. +func TestSSOProvidersSkipsNonPublicInitiateWebhook(t *testing.T) { + reg := NewRegistry() + reg.Add(ssoRecordWithAccess("google", StatusActive, true, []string{"initiate"}, nil, + []manifest.AuthProvider{{ID: "google", DisplayName: "Google", Initiate: "initiate"}})) + + svc := &Service{registry: reg} + if got := svc.SSOProviders(); len(got) != 0 { + t.Fatalf("SSOProviders() = %d providers, want 0 (initiate not public): %+v", len(got), got) + } + + reg2 := NewRegistry() + reg2.Add(ssoRecordWithAccess("google", StatusActive, true, []string{"initiate"}, + webhookAccess{"initiate": manifest.WebhookAccessPublic}, + []manifest.AuthProvider{{ID: "google", DisplayName: "Google", Initiate: "initiate"}})) + svc2 := &Service{registry: reg2} + if got := svc2.SSOProviders(); len(got) != 1 { + t.Fatalf("SSOProviders() = %d providers, want 1 (initiate public): %+v", len(got), got) + } +} diff --git a/apps/backend/internal/plugins/webhook_compat.go b/apps/backend/internal/plugins/webhook_compat.go new file mode 100644 index 0000000000..34e6f742a8 --- /dev/null +++ b/apps/backend/internal/plugins/webhook_compat.go @@ -0,0 +1,75 @@ +package plugins + +import ( + "go.uber.org/zap" + + "github.com/kandev/kandev/internal/plugins/manifest" +) + +func undeclaredLegacyWebhookKeys(m manifest.Manifest) []string { + if m.APIVersion != manifest.LegacyAPIVersion { + return nil + } + keys := make([]string, 0, len(m.Webhooks)) + for _, webhook := range m.Webhooks { + if webhook.Access == "" { + keys = append(keys, webhook.Key) + } + } + return keys +} + +func (s *Service) warnUndeclaredLegacyWebhookAccess(m manifest.Manifest) { + keys := undeclaredLegacyWebhookKeys(m) + if len(keys) == 0 { + return + } + s.log.Warn( + "plugins: api_version 1 uses the deprecated public webhook access default", + zap.String("plugin_id", m.ID), + zap.Strings("webhook_keys", keys), + zap.String("migration", "set access explicitly or upgrade the manifest to api_version 2"), + ) +} + +func nonPublicSSOInitiateProviderIDs(m manifest.Manifest) []string { + if !m.Capabilities.Auth { + return nil + } + accessByKey := make(map[string]string, len(m.Webhooks)) + for _, webhook := range m.Webhooks { + accessByKey[webhook.Key] = webhook.EffectiveAccess(m.APIVersion) + } + var providerIDs []string + for _, provider := range m.AuthProviders { + access, declared := accessByKey[provider.Initiate] + if provider.ID != "" && declared && access != manifest.WebhookAccessPublic { + providerIDs = append(providerIDs, provider.ID) + } + } + return providerIDs +} + +func (s *Service) warnNonPublicSSOInitiates(m manifest.Manifest) { + providerIDs := nonPublicSSOInitiateProviderIDs(m) + if len(providerIDs) == 0 { + return + } + s.log.Warn( + "plugins: SSO providers require a public initiate webhook", + zap.String("plugin_id", m.ID), + zap.Strings("auth_provider_ids", providerIDs), + zap.String("migration", "set access: public on each provider's initiate webhook"), + ) +} + +func (s *Service) warnWebhookAccessIssues(m manifest.Manifest) { + s.warnUndeclaredLegacyWebhookAccess(m) + s.warnNonPublicSSOInitiates(m) +} + +func (s *Service) warnLoadedWebhookAccessIssues() { + for _, record := range s.List() { + s.warnWebhookAccessIssues(record.Manifest) + } +} diff --git a/apps/backend/internal/plugins/webhook_compat_test.go b/apps/backend/internal/plugins/webhook_compat_test.go new file mode 100644 index 0000000000..86faaeea68 --- /dev/null +++ b/apps/backend/internal/plugins/webhook_compat_test.go @@ -0,0 +1,131 @@ +package plugins + +import ( + "reflect" + "testing" + + "go.uber.org/zap" + "go.uber.org/zap/zaptest/observer" + + "github.com/kandev/kandev/internal/common/logger" + "github.com/kandev/kandev/internal/plugins/manifest" +) + +func TestUndeclaredLegacyWebhookKeys(t *testing.T) { + tests := []struct { + name string + manifest manifest.Manifest + want []string + }{ + { + name: "v1 omitted access uses compatibility default", + manifest: manifest.Manifest{APIVersion: manifest.LegacyAPIVersion, Webhooks: []manifest.Webhook{ + {Key: "events"}, + {Key: "callback", Access: manifest.WebhookAccessPublic}, + {Key: "upload", Access: manifest.WebhookAccessAuthenticated}, + {Key: "commands"}, + }}, + want: []string{"events", "commands"}, + }, + { + name: "v2 omission is intentional authenticated default", + manifest: manifest.Manifest{APIVersion: manifest.CurrentAPIVersion, Webhooks: []manifest.Webhook{ + {Key: "events"}, + }}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := undeclaredLegacyWebhookKeys(tt.manifest); !reflect.DeepEqual(got, tt.want) { + t.Fatalf("undeclaredLegacyWebhookKeys() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestLegacyWebhookWarningNamesPluginAndKeys(t *testing.T) { + core, observed := observer.New(zap.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("NewFromZap: %v", err) + } + svc := &Service{log: log} + svc.warnUndeclaredLegacyWebhookAccess(manifest.Manifest{ + ID: "kandev-plugin-legacy", + APIVersion: manifest.LegacyAPIVersion, + Webhooks: []manifest.Webhook{{Key: "events"}, {Key: "commands"}}, + }) + + entries := observed.All() + if len(entries) != 1 { + t.Fatalf("warning count = %d, want 1", len(entries)) + } + fields := entries[0].ContextMap() + if fields["plugin_id"] != "kandev-plugin-legacy" || + !reflect.DeepEqual(fields["webhook_keys"], []any{"events", "commands"}) { + t.Fatalf("warning fields = %#v, want plugin id and both webhook keys", fields) + } +} + +func TestNonPublicSSOInitiateProviderIDs(t *testing.T) { + m := manifest.Manifest{ + APIVersion: manifest.CurrentAPIVersion, + Capabilities: manifest.Capabilities{Auth: true}, + Webhooks: []manifest.Webhook{ + {Key: "google", Access: manifest.WebhookAccessPublic}, + {Key: "oidc"}, + {Key: "saml", Access: manifest.WebhookAccessAuthenticated}, + }, + AuthProviders: []manifest.AuthProvider{ + {ID: "google", Initiate: "google"}, + {ID: "oidc", Initiate: "oidc"}, + {ID: "saml", Initiate: "saml"}, + {ID: "missing", Initiate: "missing"}, + }, + } + + want := []string{"oidc", "saml"} + if got := nonPublicSSOInitiateProviderIDs(m); !reflect.DeepEqual(got, want) { + t.Fatalf("nonPublicSSOInitiateProviderIDs() = %v, want %v", got, want) + } + + m.APIVersion = manifest.LegacyAPIVersion + if got := nonPublicSSOInitiateProviderIDs(m); !reflect.DeepEqual(got, []string{"saml"}) { + t.Fatalf("legacy nonPublicSSOInitiateProviderIDs() = %v, want [saml]", got) + } + + m.Capabilities.Auth = false + if got := nonPublicSSOInitiateProviderIDs(m); got != nil { + t.Fatalf("non-auth plugin providers = %v, want nil", got) + } +} + +func TestNonPublicSSOInitiateWarningNamesPluginAndProviders(t *testing.T) { + core, observed := observer.New(zap.WarnLevel) + log, err := logger.NewFromZap(zap.New(core)) + if err != nil { + t.Fatalf("NewFromZap: %v", err) + } + svc := &Service{log: log} + svc.warnNonPublicSSOInitiates(manifest.Manifest{ + ID: "kandev-plugin-sso", + APIVersion: manifest.CurrentAPIVersion, + Capabilities: manifest.Capabilities{Auth: true}, + Webhooks: []manifest.Webhook{{Key: "oidc"}, {Key: "saml"}}, + AuthProviders: []manifest.AuthProvider{ + {ID: "oidc", Initiate: "oidc"}, + {ID: "saml", Initiate: "saml"}, + }, + }) + + entries := observed.All() + if len(entries) != 1 { + t.Fatalf("warning count = %d, want 1", len(entries)) + } + fields := entries[0].ContextMap() + if fields["plugin_id"] != "kandev-plugin-sso" || + !reflect.DeepEqual(fields["auth_provider_ids"], []any{"oidc", "saml"}) { + t.Fatalf("warning fields = %#v, want plugin id and both provider ids", fields) + } +} diff --git a/apps/web/components/settings/plugins/plugin-manifest-card.tsx b/apps/web/components/settings/plugins/plugin-manifest-card.tsx index ab2c11c8ee..624f4fcc2a 100644 --- a/apps/web/components/settings/plugins/plugin-manifest-card.tsx +++ b/apps/web/components/settings/plugins/plugin-manifest-card.tsx @@ -8,6 +8,7 @@ import { Card, CardContent, CardHeader, CardTitle } from "@kandev/ui/card"; import { formatDateTime } from "@/lib/i18n/formats"; import type { PluginRecord } from "@/lib/types/plugins"; import { SETTINGS_TYPOGRAPHY } from "@/components/settings/settings-typography"; +import { isPublicWebhook } from "./plugin-manifest-model"; /** * Read-only view of the plugin's manifest: identity, capabilities, declared @@ -52,7 +53,12 @@ export function PluginManifestCard({ plugin }: { plugin: PluginRecord }) { ({ key: w.key, text: w.key }))} + items={(plugin.webhooks ?? []).map((w) => ({ + key: w.key, + text: isPublicWebhook(plugin.api_version, w.access) + ? t("plugins:manifestWebhookPublic", { key: w.key }) + : w.key, + }))} /> {showRaw && ( diff --git a/apps/web/components/settings/plugins/plugin-manifest-model.test.ts b/apps/web/components/settings/plugins/plugin-manifest-model.test.ts new file mode 100644 index 0000000000..1ea7773262 --- /dev/null +++ b/apps/web/components/settings/plugins/plugin-manifest-model.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, it } from "vitest"; +import { isPublicWebhook } from "./plugin-manifest-model"; + +describe("isPublicWebhook", () => { + it("preserves the v1 public default", () => { + expect(isPublicWebhook(1, undefined)).toBe(true); + }); + + it("uses the v2 authenticated default", () => { + expect(isPublicWebhook(2, undefined)).toBe(false); + }); + + it("honors explicit access in every version", () => { + expect(isPublicWebhook(1, "authenticated")).toBe(false); + expect(isPublicWebhook(2, "public")).toBe(true); + }); +}); diff --git a/apps/web/components/settings/plugins/plugin-manifest-model.ts b/apps/web/components/settings/plugins/plugin-manifest-model.ts new file mode 100644 index 0000000000..745ee4b037 --- /dev/null +++ b/apps/web/components/settings/plugins/plugin-manifest-model.ts @@ -0,0 +1,6 @@ +import type { PluginWebhook } from "@/lib/types/plugins"; + +export function isPublicWebhook(apiVersion: number, access: PluginWebhook["access"]): boolean { + if (access !== undefined) return access === "public"; + return apiVersion === 1; +} diff --git a/apps/web/lib/types/plugins.ts b/apps/web/lib/types/plugins.ts index 83614e12a3..74d3078815 100644 --- a/apps/web/lib/types/plugins.ts +++ b/apps/web/lib/types/plugins.ts @@ -20,6 +20,11 @@ export interface PluginWebhook { key: string; description?: string; method?: string; + /** API v1 defaults to "public" for compatibility. API v2 defaults to + * "authenticated", requiring a caller identity (session or PAT). Explicit + * values override either default. See docs/public/plugins-manifest.md. */ + access?: "public" | "authenticated"; + max_body_bytes?: number; } export interface PluginUIPage { diff --git a/apps/web/src/locales/en/plugins.json b/apps/web/src/locales/en/plugins.json index c4aabc00a8..380386c0bc 100644 --- a/apps/web/src/locales/en/plugins.json +++ b/apps/web/src/locales/en/plugins.json @@ -47,6 +47,7 @@ "manifestInstalled": "Installed", "manifestSigned": "Signed", "manifestVersion": "Version", + "manifestWebhookPublic": "{{key}} (public)", "manifestWebhooks": "Webhooks", "marketplaceSources": "Marketplace sources", "marketplaceSourcesDescription": "Add a team or corporate registry to browse its plugins alongside the official catalog. Each source serves an {{document}} document.", diff --git a/apps/web/src/locales/pseudo/plugins.json b/apps/web/src/locales/pseudo/plugins.json index 1363ad542e..77d28ae47c 100644 --- a/apps/web/src/locales/pseudo/plugins.json +++ b/apps/web/src/locales/pseudo/plugins.json @@ -47,6 +47,7 @@ "manifestInstalled": "Ĩńśţàĺĺēď", "manifestSigned": "Śĩĝńēď", "manifestVersion": "Vēŕśĩōń", + "manifestWebhookPublic": "{{key}} (ƥũƀĺĩć)", "manifestWebhooks": "Ŵēƀĥōōķś", "marketplaceSources": "Ḿàŕķēţƥĺàćē śōũŕćēś", "marketplaceSourcesDescription": "Àďď à ţēàḿ ōŕ ćōŕƥōŕàţē ŕēĝĩśţŕŷ ţō ƀŕōŵśē ĩţś ƥĺũĝĩńś àĺōńĝśĩďē ţĥē ōƒƒĩćĩàĺ ćàţàĺōĝ. Ēàćĥ śōũŕćē śēŕvēś àń {{document}} ďōćũḿēńţ.", diff --git a/apps/web/src/locales/pt-pt/plugins.json b/apps/web/src/locales/pt-pt/plugins.json index 98cf2348f0..c251ff9f7e 100644 --- a/apps/web/src/locales/pt-pt/plugins.json +++ b/apps/web/src/locales/pt-pt/plugins.json @@ -47,6 +47,7 @@ "manifestInstalled": "Instalado", "manifestSigned": "Assinado", "manifestVersion": "Versão", + "manifestWebhookPublic": "{{key}} (público)", "manifestWebhooks": "Webhooks", "marketplaceSources": "Origens do marketplace", "marketplaceSourcesDescription": "Adicione um registo de equipa ou empresarial para explorar os seus plugins juntamente com o catálogo oficial. Cada origem disponibiliza um documento {{document}}.", diff --git a/apps/web/src/locales/zh-cn/plugins.json b/apps/web/src/locales/zh-cn/plugins.json index 3e9dde8cbc..586753d4c4 100644 --- a/apps/web/src/locales/zh-cn/plugins.json +++ b/apps/web/src/locales/zh-cn/plugins.json @@ -47,6 +47,7 @@ "manifestInstalled": "已安装", "manifestSigned": "已签名", "manifestVersion": "版本", + "manifestWebhookPublic": "{{key}}(公开)", "manifestWebhooks": "Webhooks", "marketplaceSources": "市场来源", "marketplaceSourcesDescription": "添加团队或企业注册表,以便与官方目录一起浏览其插件。每个来源提供一份 {{document}} 文档。", diff --git a/apps/web/src/locales/zh-hk/plugins.json b/apps/web/src/locales/zh-hk/plugins.json index a16e280275..9d44a9dd34 100644 --- a/apps/web/src/locales/zh-hk/plugins.json +++ b/apps/web/src/locales/zh-hk/plugins.json @@ -47,6 +47,7 @@ "manifestInstalled": "已安裝", "manifestSigned": "已簽名", "manifestVersion": "版本", + "manifestWebhookPublic": "{{key}}(公開)", "manifestWebhooks": "Webhooks", "marketplaceSources": "市場來源", "marketplaceSourcesDescription": "新增團隊或企業註冊表,以便與官方目錄一起瀏覽其外掛程式。每個來源提供一份 {{document}} 文檔。", diff --git a/apps/web/src/locales/zh-tw/plugins.json b/apps/web/src/locales/zh-tw/plugins.json index 0a174fc164..ec3f50507b 100644 --- a/apps/web/src/locales/zh-tw/plugins.json +++ b/apps/web/src/locales/zh-tw/plugins.json @@ -47,6 +47,7 @@ "manifestInstalled": "已安裝", "manifestSigned": "已簽名", "manifestVersion": "版本", + "manifestWebhookPublic": "{{key}}(公開)", "manifestWebhooks": "Webhooks", "marketplaceSources": "市場來源", "marketplaceSourcesDescription": "新增團隊或企業登入檔,以便與官方目錄一起瀏覽其外掛。每個來源提供一份 {{document}} 檔案。", diff --git a/docs/decisions/0050-plugin-external-auth-capability.md b/docs/decisions/0050-plugin-external-auth-capability.md index 3ee9fe9ed1..cc44eb2382 100644 --- a/docs/decisions/0050-plugin-external-auth-capability.md +++ b/docs/decisions/0050-plugin-external-auth-capability.md @@ -1,6 +1,6 @@ # 0050 — Plugins provide OIDC/SAML login via a capability-gated, host-minted session -- Status: accepted +- Status: accepted (amended 2026-08-12) - Date: 2026-07-26 - Area: backend, security, protocol - Related: [0043 — Plugin host data API](0043-plugin-host-data-api.md), @@ -19,8 +19,10 @@ delivered as an out-of-tree plugin rather than baked into the host. The plugin capability model (ADR 0043) is a set of capability-gated `Host` gRPC RPCs the plugin *calls*, plus host→plugin events and a proxied inbound webhook -(`/api/plugins/{id}/webhooks/{key}`, already on the auth public allowlist). -None of these can establish an authenticated browser session. +(`/api/plugins/{id}/webhooks/{key}`, reachable by an anonymous visitor when its +manifest entry declares `webhooks[].public: true` — see the 2026-08-12 +amendment below). None of these can establish an authenticated browser +session. The architectural constraint that shaped this decision: the session cookie (name derived from the request host — base name `kandev_session`) must be set @@ -87,6 +89,16 @@ handing the raw session token to the plugin is strictly weaker. scope (SSO requires `ModeEnabled`); the OIDC/SAML plugin implementations live in their own repos (e.g. `kandev-plugin-google-oidc`). +**Amendment (2026-08-12):** `2026-08-12-plugin-webhook-auth-gate.md` closed an +exposure where every plugin webhook, including ones with no external +authentication of their own, was reachable anonymously by default. The +`initiate` webhook this ADR depends on (and the IdP callback key it +redirects to) must now declare `webhooks[].public: true` in the manifest, or +`SSOProviders()` silently omits that provider from the login screen rather +than surface a button that 401s. Existing SSO plugin manifests (e.g. +`kandev-plugin-google-oidc`) need that flag added before their login button +works again on an auth-enabled install. + ## Alternatives considered - **A `Host` RPC that returns the session token to the plugin.** Rejected: the diff --git a/docs/decisions/2026-07-24-opt-in-authentication.md b/docs/decisions/2026-07-24-opt-in-authentication.md index 651037250b..2d30475b62 100644 --- a/docs/decisions/2026-07-24-opt-in-authentication.md +++ b/docs/decisions/2026-07-24-opt-in-authentication.md @@ -1,6 +1,6 @@ # ADR-2026-07-24-opt-in-authentication: Opt-in Authentication and Per-User Workspace Scoping -**Status:** accepted +**Status:** accepted (amended 2026-08-12) **Date:** 2026-07-24 **Area:** backend, frontend, protocol, security @@ -59,9 +59,19 @@ a login screen or any behavioral change. justification. 6. **Explicit public allowlist** in `auth/httpmw`: readiness probe, SPA shell/static, bootstrap reads (`features`, `app-state`), credential - endpoints, and self-authenticating webhooks. `/mcp` enforces PATs in its - own group middleware; office agent JWTs pass through the global layer to - `AgentAuthMiddleware`. + endpoints, and self-authenticating webhooks (automation, office channels). + `/mcp` enforces PATs in its own group middleware; office agent JWTs pass + through the global layer to `AgentAuthMiddleware`. Plugin webhooks + (`/api/plugins/*/webhooks/*`) are NOT in this allowlist — see the + 2026-08-12 plugin-webhook-auth-gate ADR amendment below. + +**Amendment (2026-08-12):** point 6 originally allowlisted plugin webhooks +too, on the premise that "the plugin subprocess owns signature validation." +That premise was unenforced: nothing in the manifest schema or the plugin SDK +required a plugin to authenticate its own webhook. See +`2026-08-12-plugin-webhook-auth-gate.md` for the fix — plugin webhooks now +require a real caller identity by default; a manifest opts a specific webhook +out via `webhooks[].public: true`. ## Consequences diff --git a/docs/decisions/2026-08-01-per-user-plugin-storage.md b/docs/decisions/2026-08-01-per-user-plugin-storage.md index 6f68ba54cb..e468f06df6 100644 --- a/docs/decisions/2026-08-01-per-user-plugin-storage.md +++ b/docs/decisions/2026-08-01-per-user-plugin-storage.md @@ -1,6 +1,6 @@ # ADR-2026-08-01-per-user-plugin-storage: Host-Provided Per-User Plugin Storage -**Status:** accepted +**Status:** accepted (amended 2026-08-12) **Date:** 2026-08-01 **Area:** backend, frontend, security @@ -9,11 +9,20 @@ The plugin contract's only persistence is `plugin_state` (`apps/backend/internal/plugins/state/store.go`), keyed `(plugin_id, scope, scope_id, state_key)` with no user dimension, written only -by a plugin's own gRPC-connected Go backend via the Host `SetState` RPC. The -only frontend-reachable plugin HTTP route is the public, unauthenticated -`/api/plugins/:id/webhooks/:key` proxy, explicitly bypassed in +by a plugin's own gRPC-connected Go backend via the Host `SetState` RPC. At +the time of this ADR, the only frontend-reachable plugin HTTP route was the +unauthenticated `/api/plugins/:id/webhooks/:key` proxy, explicitly bypassed in `internal/auth/httpmw/middleware.go`'s allowlist. +**Amendment (2026-08-12):** that allowlist entry was removed — see +`2026-08-12-plugin-webhook-auth-gate.md` — so the webhook proxy is +authenticated by default too, unless its manifest entry opts out with +`webhooks[].public: true`. This does not change the rest of this ADR's +reasoning: what motivated `host.storage` was the *browser session's own +user identity* being reachable at all from a plugin's frontend bundle, which +the webhook proxy still does not provide (it relays a request, not a +per-user storage API). + A UI-only plugin (a native JS bundle with no Go backend) that needs to persist data scoped to the calling browser session's user — a scratchpad, a checklist, a per-user filter — has no host-provided way to do so. The driving case is a diff --git a/docs/decisions/2026-08-12-plugin-webhook-auth-gate.md b/docs/decisions/2026-08-12-plugin-webhook-auth-gate.md new file mode 100644 index 0000000000..ac654a3d41 --- /dev/null +++ b/docs/decisions/2026-08-12-plugin-webhook-auth-gate.md @@ -0,0 +1,155 @@ +# ADR-2026-08-12: Require Auth for Plugin Webhooks Unless the Manifest Declares Them Public + +**Status:** accepted +**Date:** 2026-08-12 +**Area:** backend, frontend, security + +## Context + +`isPublicPath` in `internal/auth/httpmw/middleware.go` allowlisted every +`/api/plugins/*/webhooks/*` path as unauthenticated, on the stated premise +that "the plugin subprocess owns signature validation." That premise was +enforced nowhere: `manifest.Webhook` had exactly three fields (`key`, +`description`, `method`), `validateWebhooks` checked only duplicate keys, and +nothing in `pkg/pluginsdk` required a plugin's `HandleWebhook` to verify its +caller. QA confirmed the consequence on an auth-enabled instance: an +anonymous `POST /api/plugins/kandev-plugin-notes/webhooks/enhance` passed the +middleware, reached the plugin subprocess, and got as far as +`host.InvokeUtilityAgent`, stopping only because no agent profile was bound. +With a profile bound, the same anonymous request returned a real, billed LLM +completion. Any plugin webhook reaching `agent_invoke` or another +spend-worthy capability was an open proxy on any reachable instance. + +This was pre-existing — neither the route nor the allowlist entry came from +any single feature branch. What changed reachability was a later branch +whose guided-setup buttons walk users to a bound utility-agent profile, +moving the exposure from latent to live on installs that follow the new +setup flow. + +Two anonymous callers of the webhook proxy are load-bearing and documented +elsewhere, ruling out simply dropping plugin webhooks from the allowlist: + +- **SSO.** `0050-plugin-external-auth-capability.md` — the webhook response + is the only plugin→browser HTTP path, so it is how a first session is + minted for a visitor with no credentials yet. +- **Third-party callbacks.** `docs/specs/plugins/spec.md` pins Jira POSTing + to `/api/plugins/kandev-plugin-jira/webhooks/jira-webhooks` as an + acceptance criterion; the plugin registry lists `kandev-plugin-slack` and + `kandev-plugin-github-status`, which take the same shape of callback. + +## Decision + +`manifest.Webhook` gains a `public bool` field (`webhooks[].public`, default +`false`). `internal/auth/httpmw` no longer allowlists plugin webhooks +unconditionally: it structurally defers GET/POST `/api/plugins//webhooks/` +(`isPluginWebhookPath`, replacing an over-matching `strings.Contains` check) +because the middleware has no access to the plugin registry to decide policy +per-webhook. `internal/plugins.Controller.webhook` is the one place holding +the manifest, so it enforces the actual gate +(`webhookCallerAuthorized`): a webhook whose manifest entry declares +`public: true` is relayed to anyone; anything else requires a real request +identity — a resolved session/PAT, or the synthetic identity +`httpmw.Middleware` injects while authentication is disabled. Identity +*presence* is a complete proxy for "authenticated or auth is off," so the +plugins package needs no dependency on `internal/auth` to decide. + +The gate runs before any lookup-error or 404 is written. For a caller with no +identity, an unknown plugin ID, an undeclared webhook key, and a +declared-non-public key all return the same `401 {"error":"authentication +required"}` — never a 404 — so an anonymous caller cannot use response-code +differences to enumerate installed plugins. + +`SSOProviders()` gains a matching skip: a provider whose `initiate` webhook +is declared but not `public` is omitted from the login screen rather than +surfaced as a button that immediately 401s (a visitor clicking a login +button has no session or PAT to present). This is a clean skip, not a +manifest validation error — the same tolerance `SSOProviders()` already had +for an `initiate` key naming no declared webhook — so an unpatched SSO +plugin degrades to "no login button" rather than an error state on upgrade. + +The relay's header forwarding also changes: `flattenHeaders` now strips +Kandev's own session cookie and any `kandev_pat_*` Authorization bearer +before building the `WebhookRequest`. This matters because a non-public +webhook is now routinely reached by an authenticated browser +(`host.api.fetch` sends `credentials: "include"`) or PAT caller — the plugin +subprocess should see only what it needs to verify itself (a provider's own +bearer, a signature header), never the caller's Kandev credential. A +non-Kandev cookie or non-PAT bearer still relays unchanged, so a `public` +webhook can still verify a provider's own token. + +A cookie-authenticated webhook request must include an accepted `Origin`. +Kandev uses the shared `internal/common/httpmw.AllowedOrigin` policy. PAT calls +and the synthetic identity used when authentication is off do not need an +Origin header. + +Header forwarding depends on access: + +- An authenticated webhook receives no `Authorization` or `Cookie` header. + This prevents ambient reverse-proxy and browser credentials from reaching the + plugin process. +- A public webhook can receive provider credentials. Kandev still removes its + own session cookie and every `kandev_pat_*` credential. + +Webhook dispatch uses the existing plugin generation read lease. The lease +covers the plugin RPC and response handling. Disable, uninstall, configuration +restart, and upgrade wait for the response to finish. Thus, an access decision +and an SSO login directive cannot cross into a replacement plugin generation. + +`SSOProviders()` only returns a provider when its initiate webhook is effectively +public. API v1 omission remains public. API v2 requires `access: public` for the +initiate webhook and its callback. +The host logs one load-time warning when a declared initiate webhook is not +public. This gives the operator a diagnostic without logging on each boot read. + +## Consequences + +- The allowlist's stated premise ("the plugin subprocess owns signature + validation") is now something the manifest actually asserts per webhook, + instead of a blanket, unverifiable claim covering every plugin webhook. +- Production plugins taking third-party callbacks (`kandev-plugin-slack`, + `kandev-plugin-github-status`, `kandev-plugin-google-oidc`) need a manifest + update adding `public: true` to their callback/initiate webhook keys before + they work again on an auth-enabled install; `kandev-plugin-notes`' `enhance` + webhook is deliberately left unflagged, since it is only ever called from + the logged-in Kandev UI. +- An auth-enabled install running an unpatched SSO plugin loses its login + button (a clean `SSOProviders()` skip) until that plugin's manifest ships + the flag; mitigated by the setup-wizard local admin always being available. +- `internal/plugins` now imports `internal/auth` for `auth.PATPrefix` — a + same-direction import verified acyclic (nothing under `internal/auth` + imports `internal/plugins`) and preferred over duplicating a + security-relevant token format. +- The `auth/httpmw` pinning test for the plugin-webhook path is weaker by + construction: under deferral, the middleware passes the path through either + way, so it can no longer prove the auth policy on its own. The real pin is + `internal/plugins/handlers_webhook_auth_test.go`. +- A `public: true` webhook is exactly as exposed as every plugin webhook was + before this change — the flag makes that exposure explicit and reviewable, + it does not remove it. Kandev can only enforce that *a* caller identity is + present; it cannot verify a `public: true` webhook actually authenticates + its own callers. No host-side rate limiting is added by this change. +- This amends `2026-07-24-opt-in-authentication.md` (point 6, the public + allowlist), `0050-plugin-external-auth-capability.md` (the "already on the + auth public allowlist" claim), and `2026-08-01-per-user-plugin-storage.md` + (the "public, unauthenticated" webhook proxy claim). + +## Alternatives Considered + +1. **Registry-aware middleware.** Inject a plugin-webhook policy lookup into + `httpmw.Middleware` so `isPublicPath` could consult the manifest directly. + Rejected: couples the auth middleware to the plugin service, duplicates + the manifest lookup the handler already performs, and breaks + `isPublicPath`'s pure-function `(method, path)` testability. +2. **Drop plugin webhooks from the allowlist entirely, require a PAT for + every caller.** Rejected: breaks the documented SSO first-session path (no + PAT can exist before a session does) and every third-party callback + (Slack, Jira, GitHub cannot present a Kandev PAT). +3. **Per-plugin shared secret verified by the host.** Rejected: leaves the + platform default insecure (every plugin would need to opt into the + secret), repeats the check outside any single enforcement point, and + cannot express the SSO first-session case (no secret exists yet either). +4. **`api_version`-gated grandfathering** (unflagged webhooks on manifests + still declaring `api_version: 1` stay public). Rejected: keeps the + exposure open for exactly the plugins that have it today, defeating the + purpose of the change; the flag addition is additive YAML the parser + already tolerates, so there was no compatibility reason to gate it. diff --git a/docs/decisions/INDEX.md b/docs/decisions/INDEX.md index 91336b3d62..6d9678336c 100644 --- a/docs/decisions/INDEX.md +++ b/docs/decisions/INDEX.md @@ -191,6 +191,7 @@ Read individual ADRs for full context. Create new ones via `/record decision` or | 2026-08-20-acp-client-non-underscore-extension-methods | [Route Non-Underscore Inbound Client Methods to the Extension Handler](2026-08-20-acp-client-non-underscore-extension-methods.md) | accepted | backend, protocol | 2026-08-20 | | 2026-08-22-persistent-fork-approval-labels | [Persist Fork Approval Labels Across Pushes](2026-08-22-persistent-fork-approval-labels.md) | accepted | infra, workflow, security | 2026-08-22 | | 2026-08-22-plugin-version-retention | [Keep Exactly One Superseded Plugin Version](2026-08-22-plugin-version-retention.md) | accepted | backend | 2026-08-22 | +| 2026-08-12-plugin-webhook-auth-gate | [Require Auth for Plugin Webhooks Unless the Manifest Declares Them Public](2026-08-12-plugin-webhook-auth-gate.md) | accepted | backend, frontend, security | 2026-08-12 | | 2026-08-22-system-oriented-specifications | [Organize specifications by system](2026-08-22-system-oriented-specifications.md) | accepted | workflow, infra | 2026-08-22 | | 2026-08-22-pr-walkthrough-r2-hosting | [Host PR walkthrough HTML in Cloudflare R2](2026-08-22-pr-walkthrough-r2-hosting.md) | accepted | infra, workflow, security | 2026-08-22 | | 2026-08-22-agent-owned-pr-walkthrough-rendering | [Keep PR walkthrough rendering agent owned and provider neutral](2026-08-22-agent-owned-pr-walkthrough-rendering.md) | superseded by 2026-08-22-pr-walkthrough-filesystem-runner | workflow, infra, security | 2026-08-22 | diff --git a/docs/plans/plugin-voice-extraction-host/plan.md b/docs/plans/plugin-voice-extraction-host/plan.md index fd014a6219..e8e8c5750f 100644 --- a/docs/plans/plugin-voice-extraction-host/plan.md +++ b/docs/plans/plugin-voice-extraction-host/plan.md @@ -23,7 +23,8 @@ only a later parity task may remove core code. validation, and submit lifecycles differ. - Plugin UI continues to use `host.api.fetch("webhooks/")` and the existing `HandleWebhook` RPC. `webhooks[].access` selects public or authenticated invocation and `max_body_bytes` sets a bounded - per-key limit up to 16 MiB; old manifests retain public/4 MiB defaults. + per-key limit up to 16 MiB. API v1 manifests retain public/4 MiB defaults. API v2 defaults to + authenticated access and a 4 MiB limit. - Plugin localization is a separate prerequisite. It must be resolved before core Voice Mode is removed, but this package adds no host i18n API. @@ -66,8 +67,9 @@ only a later parity task may remove core code. ## Backend Authenticated Webhook Architecture - Extend manifest `Webhook` with `Access` and `MaxBodyBytes`. Validate `access` as - `public|authenticated`, default it to `public`, default the limit to 4 MiB, reject non-positive or - >16 MiB values, and preserve those values through install/registry serialization. + `public|authenticated`, use the API v1 public default and API v2 authenticated default, default the + limit to 4 MiB, reject non-positive or >16 MiB values, and preserve those values through + install/registry serialization. - Keep the existing webhook URL and `HandleWebhook` RPC. In the handler, resolve the declared key before reading the body, apply its effective limit, and require normal Kandev identity plus accepted Origin for authenticated cookie requests. Authentication-disabled mode uses the synthetic user. @@ -79,9 +81,9 @@ only a later parity task may remove core code. ## Localization And Compatibility -- Add optional manifest fields without changing plugin `api_version: 1`. Document that consumers must - declare the first supporting `min_kandev_version`; older hosts fail installation instead of falling - back to webhooks. +- Keep API v1 omissions public and introduce API v2 with an authenticated default. Document that + consumers must declare the first supporting `min_kandev_version`; older hosts fail installation + instead of falling back to webhooks. - Update `docs/specs/plugins/spec.md`, `docs/plans/plugins/PLUGIN-API.md`, `docs/public/plugins-authoring.md`, and `docs/public/plugins-manifest.md` in the same contract task. diff --git a/docs/public/authentication.md b/docs/public/authentication.md index 8cb3cc622d..c9dbb9ff10 100644 --- a/docs/public/authentication.md +++ b/docs/public/authentication.md @@ -63,7 +63,9 @@ External MCP clients (Claude Code, Cursor connecting to `/mcp`) must be configur ## Endpoints that stay public -`/health` (readiness probes), the login/setup/invite pages, `GET /api/v1/features`, and self-authenticating webhook receivers (automation webhooks with `X-Webhook-Secret`, office channel HMAC webhooks, plugin webhooks). Everything else requires a session or token. +`/health` (readiness probes), the login/setup/invite pages, `GET /api/v1/features`, and self-authenticating webhook receivers (automation webhooks with `X-Webhook-Secret`, office channel HMAC webhooks). Everything else requires a session or token. + +Plugin webhooks (`/api/plugins/{id}/webhooks/{key}`) are **not** public by default: a plugin's manifest must explicitly declare `webhooks[].public: true` for that specific webhook to accept anonymous requests. Unflagged webhooks require a session or PAT like any other endpoint. See [Plugin manifest reference](plugins-manifest.md). ## Multiple instances on one host diff --git a/docs/public/plugins-authoring.md b/docs/public/plugins-authoring.md index 6bc480b5ee..ef4c6ac11b 100644 --- a/docs/public/plugins-authoring.md +++ b/docs/public/plugins-authoring.md @@ -945,6 +945,11 @@ plugin never handles the raw token, and any `Set-Cookie` you return is dropped. Requires authentication enabled; emitting the header without `capabilities.auth` returns 403. +Declare both the initiate webhook and the callback webhook with `access: public`. +Kandev checks the initiate key before it shows the login button. The manifest +has no callback field, so your plugin must declare the callback key separately. +Kandev logs a warning when a declared initiate webhook is not public. + **You MUST only assert an `email` the IdP has verified as owned by `subject`.** Kandev auto-links that email to (or provisions) an account, so an unverified or user-settable email claim is an account-takeover vector. Kandev refuses to @@ -2122,6 +2127,9 @@ 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. - **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/auth/spec.md b/docs/specs/auth/spec.md index 0441debfd2..481f4c4c04 100644 --- a/docs/specs/auth/spec.md +++ b/docs/specs/auth/spec.md @@ -85,7 +85,7 @@ the local single-user install with a login screen it never asked for. payload (`/api/v1/app-state` — returns only `{features, auth}` for anonymous visitors), `/api/v1/features`, credential endpoints (login/setup/invite-accept), and self-authenticating webhooks (automation - `X-Webhook-Secret`, office channel HMAC, plugin webhooks). The GitHub + `X-Webhook-Secret`, office channel HMAC). The GitHub credential broker (`/api/v1/github/credentials/resolve`, GET readiness + POST resolve) is likewise public: containers and remote executors hold no session cookie or PAT by design, and the opaque, task-scoped lease in the @@ -94,6 +94,14 @@ the local single-user install with a login screen it never asked for. (`/api/v1/github/app/registrations/{id}/webhook`) is public for the same reason as the other webhooks: its own HMAC (`X-Hub-Signature-256`) is verified by the handler. +- **Plugin webhooks (`/api/plugins/{id}/webhooks/{key}`) are NOT + self-authenticating by default.** The global middleware cannot read the + plugin registry, so it structurally defers GET/POST requests for the path; `plugins.Controller` + enforces the actual gate: a webhook requires a real caller identity + (session/PAT, or the synthetic identity while auth is disabled) unless its + manifest declares `webhooks[].public: true`. An anonymous caller sees 401 + for an unknown plugin ID, an undeclared key, and a declared-non-public key + alike, so a caller with no identity cannot enumerate installed plugins. - **Session challenges are distinct from provider authentication failures.** The browser clears its Kandev identity and opens `/login` only when a 401 is a Kandev session challenge. A third-party integration may also return 401 diff --git a/docs/specs/plugins/spec.md b/docs/specs/plugins/spec.md index 52123a0d77..41d995d92c 100644 --- a/docs/specs/plugins/spec.md +++ b/docs/specs/plugins/spec.md @@ -153,6 +153,7 @@ webhooks: - key: "slack-events" description: "Slack Events API webhook" method: "POST" + access: public config_schema: type: object @@ -388,10 +389,10 @@ Wildcard subscriptions: `task.*`, `agent.*`, `.*` (any subject prefix). POST /api/plugins/{plugin_id}/webhooks/{webhook_key} ``` -This remains kandev's one plugin-facing **HTTP** endpoint (external systems like Slack -or Jira cannot speak gRPC). Kandev validates the plugin is active and the webhook key -is declared, converts the HTTP request into a `WebhookRequest` gRPC message, and calls -the plugin's `Plugin.HandleWebhook` RPC: +This is kandev's only plugin-facing **HTTP** endpoint. Anonymous callers need +`access: public`; otherwise kandev returns 401 without revealing plugin/key. Kandev +validates plugin/key, strips session/PAT, converts to `WebhookRequest`, and calls +`Plugin.HandleWebhook`: ```proto message WebhookRequest { diff --git a/docs/specs/spec-lint.json b/docs/specs/spec-lint.json index 4d573e3204..f746185ba5 100644 --- a/docs/specs/spec-lint.json +++ b/docs/specs/spec-lint.json @@ -33,7 +33,7 @@ "docs/specs/office-agent-tier-routing/spec.md": 58448, "docs/specs/platform/diagnostic-logging.md": 40700, "docs/specs/plugin-nav-sidebar-footer/spec.md": 63150, - "docs/specs/plugins/spec.md": 77820, + "docs/specs/plugins/spec.md": 77806, "docs/specs/subagent-context-persistence/spec.md": 160552, "docs/specs/system-page/storage-maintenance.md": 54320, "docs/specs/ui/ci-pr-automation.md": 57643