From e920e4b16ad4556f4f5ad5fa754d8edb25689c2f Mon Sep 17 00:00:00 2001 From: lr00rl Date: Tue, 14 Jul 2026 01:37:02 -0700 Subject: [PATCH] fix: close three gaps in the plugin gateway boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Restrict /api/plugins/invoke to diagnostic actions. Invoke is a raw runtime channel gated only by plugin:admin: it takes an arbitrary action, consults no per-method scopes, and binds no operator target. That was tolerable when it was the only way to reach a staged artifact, but /api/plugins/call is now the real gateway and is the only path that enforces the manifest's declared scopes, binds an operator target to a single invocation, and (for host-risk work) requires a plan and an approval. Left open, invoke would let a plugin:admin operator call a plugin's declared methods while bypassing their scopes — and once any plugin ships an execute action it would bypass the plan/approval/one-time-capability binding entirely and apply host changes unreviewed. The action list is now closed to describe and health; everything else is refused before the artifact runs. Derive the plugin-asset CSP only from the configured public URL. The policy fell back to an origin built from the request's Host header when the public URL was unset, so the policy confining plugin code could be named by the request it was confining. Asset serving now fails closed without a public URL, and the request-derived fallback is gone. Redact the operator target from guard failures. An operator target may carry its secret in the URL path, and url.Parse errors echo the URL they failed on — text that reaches both the audit record and the API response, while the audit record must never contain it. The reason is now surfaced without the value. A substring scrub is not enough on its own: *url.Error renders the URL quoted and escaped, so the inner reason is unwrapped and both raw and escaped forms are scrubbed. Tests: go vet clean; internal/plugin and internal/server green under -race. --- internal/server/server_plugin_assets.go | 35 ++++---- internal/server/server_plugin_assets_test.go | 21 +++-- internal/server/server_plugin_invoke.go | 65 +++++++++++++-- internal/server/server_plugin_invoke_test.go | 85 ++++++++++++++++++++ 4 files changed, 177 insertions(+), 29 deletions(-) diff --git a/internal/server/server_plugin_assets.go b/internal/server/server_plugin_assets.go index 7c4d1a1..1bc4073 100644 --- a/internal/server/server_plugin_assets.go +++ b/internal/server/server_plugin_assets.go @@ -69,6 +69,15 @@ func (s *Server) resolvePluginAssetRequest(r *http.Request) (resolvedPluginAsset func (s *Server) servePluginAsset(w http.ResponseWriter, r *http.Request, asset resolvedPluginAsset) { loaded, assetPath := asset.loaded, asset.assetPath + // The confining policy is derived from the configured public URL. Without it there + // is no trustworthy origin to name as a script source — and the only alternative, + // the caller's Host header, would let the request shape the policy meant to confine + // it. Fail closed rather than serve executable plugin code under a weaker policy. + if canonicalPluginAssetOrigin(s.publicURL) == "" { + writeError(w, http.StatusServiceUnavailable, + errors.New("plugin assets are not available (server public URL unset)")) + return + } if strings.EqualFold(filepath.Ext(assetPath), ".html") && !asset.isEntrypoint { http.NotFound(w, r) return @@ -91,7 +100,7 @@ func (s *Server) servePluginAsset(w http.ResponseWriter, r *http.Request, asset w.Header().Set("Content-Type", contentType) w.Header().Set("X-Content-Type-Options", "nosniff") - w.Header().Set("Content-Security-Policy", pluginAssetCSP(s.publicURL, pluginAssetRequestOrigin(r))) + w.Header().Set("Content-Security-Policy", pluginAssetCSP(s.publicURL)) if !asset.isEntrypoint { // The sandboxed entrypoint has an opaque origin, so its external scripts // and styles use credentialless CORS. These resources remain bound to an @@ -109,31 +118,25 @@ func (s *Server) servePluginAsset(w http.ResponseWriter, r *http.Request, asset http.ServeContent(w, r, filepath.Base(assetPath), time.Time{}, bytes.NewReader(data)) } -func pluginAssetCSP(publicURL, requestOrigin string) string { +// pluginAssetCSP builds the per-route policy for a sandboxed plugin document. The +// allowed origin is taken only from the server's configured public URL: a request's +// Host header is caller-controlled, and letting it name a script source would make +// the policy that confines plugin code depend on the request it is confining. +// Serving is refused outright when the public URL is unset (see servePluginAsset), +// so there is no request-derived fallback to reach. +func pluginAssetCSP(publicURL string) string { assetSources := "'self'" - origin := canonicalPluginAssetOrigin(publicURL) - if origin == "" { - origin = canonicalPluginAssetOrigin(requestOrigin) - } - if origin != "" { + if origin := canonicalPluginAssetOrigin(publicURL); origin != "" { // Sandboxed plugin documents intentionally have an opaque origin. Their // signed external assets therefore need the configured control-plane // origin in addition to 'self', which does not match from an opaque origin. assetSources += " " + origin } return "default-src 'none'; base-uri 'none'; form-action 'none'; frame-ancestors 'self'; " + - "object-src 'none'; style-src " + assetSources + "; script-src " + assetSources + "; " + + "object-src 'none'; frame-src 'none'; style-src " + assetSources + "; script-src " + assetSources + "; " + "img-src " + assetSources + " data:; font-src " + assetSources + "; connect-src 'none'" } -func pluginAssetRequestOrigin(r *http.Request) string { - scheme := "http" - if r.TLS != nil { - scheme = "https" - } - return scheme + "://" + r.Host -} - func canonicalPluginAssetOrigin(raw string) string { if raw == "" { return "" diff --git a/internal/server/server_plugin_assets_test.go b/internal/server/server_plugin_assets_test.go index b8bf724..ef08404 100644 --- a/internal/server/server_plugin_assets_test.go +++ b/internal/server/server_plugin_assets_test.go @@ -317,7 +317,7 @@ func TestPluginAssetHeadersCacheAndPathValidation(t *testing.T) { } func TestPluginAssetCSPOnlyAcceptsCanonicalHTTPOrigins(t *testing.T) { - valid := pluginAssetCSP("https://lattice.example.test:8443", "http://ignored.test") + valid := pluginAssetCSP("https://lattice.example.test:8443") if !strings.Contains(valid, "script-src 'self' https://lattice.example.test:8443") || !strings.Contains(valid, "img-src 'self' https://lattice.example.test:8443 data:") { t.Fatalf("valid public origin missing from CSP: %q", valid) @@ -330,14 +330,25 @@ func TestPluginAssetCSPOnlyAcceptsCanonicalHTTPOrigins(t *testing.T) { "https://lattice.example.test/#fragment", "https://lattice.example.test;script-src.example", } { - csp := pluginAssetCSP(invalid, "") + csp := pluginAssetCSP(invalid) if strings.Contains(csp, invalid) || !strings.Contains(csp, "script-src 'self';") { t.Fatalf("invalid public URL %q affected CSP: %q", invalid, csp) } } +} - fallback := pluginAssetCSP("", "http://127.0.0.1:8088") - if !strings.Contains(fallback, "script-src 'self' http://127.0.0.1:8088") { - t.Fatalf("request origin fallback missing from CSP: %q", fallback) +// The policy that confines plugin code must never be derived from the request being +// confined: a caller-controlled Host header could otherwise name a script source. +// Asset serving fails closed when the public URL is unset, so no request-derived +// origin can reach the CSP. +func TestPluginAssetCSPIgnoresRequestOrigin(t *testing.T) { + csp := pluginAssetCSP("") + if !strings.Contains(csp, "script-src 'self';") { + t.Fatalf("unset public URL must yield a bare 'self' script-src: %q", csp) + } + for _, host := range []string{"127.0.0.1:8088", "evil.test", "lattice.example.test"} { + if strings.Contains(csp, host) { + t.Fatalf("request-derived host %q leaked into CSP: %q", host, csp) + } } } diff --git a/internal/server/server_plugin_invoke.go b/internal/server/server_plugin_invoke.go index a4c4ef7..7191015 100644 --- a/internal/server/server_plugin_invoke.go +++ b/internal/server/server_plugin_invoke.go @@ -6,6 +6,8 @@ import ( "errors" "fmt" "net/http" + "net/url" + "strconv" "strings" "time" @@ -351,13 +353,37 @@ func extractOperatorTargets(payload json.RawMessage, fields []string) ([]string, } target = strings.TrimSpace(target) if err := outbound.GuardOperatorURL(target); err != nil { - return nil, fmt.Errorf("operator target field %q is invalid: %w", field, err) + return nil, fmt.Errorf("operator target field %q is invalid: %s", field, redactOperatorTarget(err, target)) } targets = append(targets, target) } return uniqueStrings(targets), nil } +// redactOperatorTarget keeps a guard failure's reason but strips the secret-bearing +// target out of it. An operator target may carry its secret in the URL path, and this +// text reaches both the audit record and the API response — the audit record must +// never contain it. +func redactOperatorTarget(err error, target string) string { + message := err.Error() + // *url.Error renders as `parse "": ` with the URL quoted and escaped, + // so scrubbing the raw target cannot remove it. Keep only the reason, which never + // carries the URL. + var urlErr *url.Error + if errors.As(err, &urlErr) && urlErr.Err != nil { + message = urlErr.Err.Error() + } + if target == "" { + return message + } + // Any other guard that echoes the target does so either raw or Go-escaped. + escaped := strconv.Quote(target) + for _, form := range []string{target, escaped, escaped[1 : len(escaped)-1]} { + message = strings.ReplaceAll(message, form, "[redacted]") + } + return message +} + func (s *Server) recordPluginCallAudit(p principal, pluginID, service, method string, scopes []string, decision, reason string) { scope := "plugin" if len(scopes) > 0 { @@ -419,13 +445,27 @@ func pluginGatewayScopeRequiresUnrestrictedAllowlist(scope string) bool { } } -// handlePluginInvoke runs one action on an ACTIVE plugin via the runtime (the -// Tier-2 system runner execs the artifact's {action,payload}->{ok,result} -// protocol). This is the minimal seed of the design-10 dashboard->plugin gateway: -// it makes plugin EXECUTION reachable (the system runner otherwise stages the -// artifact but nothing triggers it). Gated by plugin:admin + audited. A plugin -// that is not armed, or whose runner cannot invoke (noop), returns an error -// rather than silently doing nothing. +// diagnosticPluginActions is the closed set of actions reachable through the raw +// invoke channel. Everything with an effect on domain state must go through +// /api/plugins/call, which is the only path that enforces the manifest's +// per-method scopes, binds operator targets to a single invocation, and (for +// host-risk work) requires a plan and an approval. +// +// This list must stay closed. `call` and `plan` would bypass per-method scopes; +// `execute` would bypass the whole plan/approval/one-time-capability binding, so +// an operator holding only plugin:admin could apply host changes unreviewed. +var diagnosticPluginActions = map[string]bool{ + "describe": true, + "health": true, +} + +// handlePluginInvoke runs one DIAGNOSTIC action on an ACTIVE plugin via the +// runtime (the Tier-2 system runner execs the artifact's {action,payload}-> +// {ok,result} protocol). It exists so an operator can interrogate a staged +// artifact directly; it is not a gateway. Gated by plugin:admin, restricted to +// diagnosticPluginActions, and audited. A plugin that is not armed, or whose +// runner cannot invoke (noop), returns an error rather than silently doing +// nothing. func (s *Server) handlePluginInvoke(w http.ResponseWriter, r *http.Request, p principal) { if r.Method != http.MethodPost { writeError(w, http.StatusMethodNotAllowed, errors.New("method not allowed")) @@ -446,6 +486,15 @@ func (s *Server) handlePluginInvoke(w http.ResponseWriter, r *http.Request, p pr writeError(w, http.StatusBadRequest, errors.New("id and action are required")) return } + if !diagnosticPluginActions[req.Action] { + s.recordPrincipalAudit(p, model.AuditEvent{ + ID: id.New("audit"), Action: "plugin.invoke", Scope: "plugin:admin", Decision: "deny", + Reason: "action is not a diagnostic action; use /api/plugins/call", + Metadata: map[string]string{"plugin_id": req.ID, "plugin_action": req.Action}, + }) + writeError(w, http.StatusForbidden, errors.New("only diagnostic actions may be invoked directly; use /api/plugins/call")) + return + } if s.pluginRuntime == nil { writeError(w, http.StatusServiceUnavailable, errors.New("plugin runtime unavailable")) return diff --git a/internal/server/server_plugin_invoke_test.go b/internal/server/server_plugin_invoke_test.go index 25b98e0..b83ca4c 100644 --- a/internal/server/server_plugin_invoke_test.go +++ b/internal/server/server_plugin_invoke_test.go @@ -1,6 +1,7 @@ package server import ( + "bytes" "context" "encoding/json" "io" @@ -437,3 +438,87 @@ func TestExtractOperatorTargetsRequiresDeclaredPayloadField(t *testing.T) { } } } + +// The raw invoke channel is gated only by plugin:admin. It must therefore never reach +// an action with an effect on domain state: `call` and `plan` would bypass the +// manifest's per-method scopes and operator-target binding, and `execute` would bypass +// the plan/approval/one-time-capability binding entirely. +func TestPluginInvokeRefusesNonDiagnosticActions(t *testing.T) { + pluginRoot := t.TempDir() + bundle := filepath.Join(pluginRoot, "test.exec") + if err := os.MkdirAll(bundle, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(bundle, "manifest.json"), + []byte(`{"id":"test.exec","name":"Exec Test","type":"system","capabilities":["node:read"]}`), 0o644); err != nil { + t.Fatal(err) + } + // The artifact would happily answer anything; the host must refuse before it runs. + script := "#!/bin/sh\nread line\necho '{\"ok\":true,\"message\":\"executed\",\"result\":{\"ran\":true}}'\n" + if err := os.WriteFile(filepath.Join(bundle, "artifact"), []byte(script), 0o755); err != nil { + t.Fatal(err) + } + + st, err := store.Open("") + if err != nil { + t.Fatal(err) + } + srv, err := New(Options{ + Store: st, AdminPassword: testAdminPass, DisableRenewalScheduler: true, + PluginDir: pluginRoot, + PluginRuntimeDir: t.TempDir(), + }) + if err != nil { + t.Fatalf("New: %v", err) + } + handler := srv.Handler() + cookies, csrf := loginSession(t, handler) + + for _, status := range []string{"installed", "active"} { + resp := doJSON(t, handler, http.MethodPost, "/api/plugins/lifecycle", + `{"id":"test.exec","status":"`+status+`"}`, cookies, csrf) + if resp.StatusCode != http.StatusOK { + t.Fatalf("lifecycle %s: %d", status, resp.StatusCode) + } + resp.Body.Close() + } + + for _, action := range []string{"call", "plan", "execute", "migrate", "anything"} { + resp := doJSON(t, handler, http.MethodPost, "/api/plugins/invoke", + `{"id":"test.exec","action":"`+action+`"}`, cookies, csrf) + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + if resp.StatusCode != http.StatusForbidden { + t.Fatalf("invoke %q: want 403, got %d (%s)", action, resp.StatusCode, body) + } + if bytes.Contains(body, []byte("executed")) { + t.Fatalf("invoke %q reached the artifact: %s", action, body) + } + } + + // Diagnostics remain reachable. + for _, action := range []string{"describe", "health"} { + resp := doJSON(t, handler, http.MethodPost, "/api/plugins/invoke", + `{"id":"test.exec","action":"`+action+`"}`, cookies, csrf) + resp.Body.Close() + if resp.StatusCode != http.StatusOK { + t.Fatalf("invoke %q: want 200, got %d", action, resp.StatusCode) + } + } +} + +// An operator target may carry its secret in the URL path. url.Parse errors echo the +// URL they failed on, and that text reaches both the audit record and the API +// response, so the guard's reason must be surfaced without the value. +func TestOperatorTargetErrorRedactsSecret(t *testing.T) { + const secret = "https://sub.example.test/aVerySecretToken123/api" + payload := json.RawMessage(`{"base_url":"` + secret + "\x7f" + `"}`) + + _, err := extractOperatorTargets(payload, []string{"base_url"}) + if err == nil { + t.Fatal("want an error for a malformed operator target") + } + if strings.Contains(err.Error(), "aVerySecretToken123") { + t.Fatalf("operator target secret leaked into the error: %q", err) + } +}