feat(extension): [CLI-1653] dynamic, out-of-process extensions over gRPC

[mattdolan-snyk]

Description

Adds a way to load CLI extensions dynamically — from a prebuilt binary discovered at runtime — instead of only compiling them into the CLI. This unblocks developing an extension without rebuilding the host on every change, and lays the groundwork for downloadable / third-party extensions.

Extensions run out of process and talk to the host over gRPC via hashicorp/go-plugin (the Terraform/Vault model), so a crashing extension can't take down the CLI and no extension code is loaded into the host's address space. Despite the boundary, an extension is written like an in-process workflow: extension.Handler is a type alias for workflow.Callback, and it receives a real workflow.InvocationContext.

What an extension gets:

• Config — its declared flags, plus settings the host resolves.
• Authenticated network — GetNetworkAccess().GetHttpClient() returns a client the host authenticates for it, via a per-invocation loopback proxy. The user's credentials stay in the host process; the extension never holds them, and they can only reach the configured Snyk API (not arbitrary hosts).
• Calling other workflows — GetEngine().Invoke(id, …) runs a built-in or another extension's workflow on the host, in its full context.
• Analytics — AddExtension*Value / AddError feed the host's batch.

Developer loop: go build your extension → point a host at it (app.WithExtensionPaths(...), or a --plugin-path flag / internal_extension_paths config key) → iterate with no host rebuild. Getting-started guide: docs/dynamic-extensions.md. Two runnable examples: examples/extensions/whoami (minimal, calls a host workflow) and pkg/extension/testdata/exampleplugin (config, authenticated API call, sibling invoke + analytics).

Safety / scope: this is the trusted-code posture — isolation for robustness, not a sandbox (a loaded binary runs with the user's OS privileges). A dynamic extension cannot silently override a bundled workflow (opt in with internal_extension_allow_override). The enterprise trust layer (signature/registry verification + a default-deny allowlist) is intentionally a later phase; the loader's dialer is the single chokepoint where verify-before-launch slots in.

Blast radius: the workflow.Engine, Register/Invoke, InvocationContext, and content-typed Data contracts are unchanged; built-in (compile-time) extensions behave exactly as before. The only core addition is workflow.ResolveInvokeOptions, a helper that lets an Invoke be forwarded across the boundary. Protobuf codegen is wired into go generate / make generate.

Design + internals: docs/dynamic-extensions-design.md · diagrams: docs/dynamic-extensions-architecture.md.

Architecture (blue = pre-existing, green = added here)

flowchart TB
    classDef existing fill:#dbeafe,stroke:#3b82f6,color:#0b1f44;
    classDef new fill:#d4f8d4,stroke:#2ea043,color:#06210d;

    subgraph HOST["Host CLI process"]
        APP["app.CreateAppEngineWithOptions"]:::existing
        ENG["workflow.Engine<br/>registry + Invoke dispatch"]:::existing
        LWF["localworkflows.Init<br/>built-in extensions (compile-time import)"]:::existing
        BW["built-in workflow callback<br/>e.g. flw://output"]:::existing
        IC["InvocationContext<br/>Config · NetworkAccess · Analytics · UI · Logger"]:::existing

        LOADER["extension.Loader<br/>(a workflow.ExtensionInit)"]:::new
        PROXY["proxy callback<br/>1 per discovered workflow"]:::new
        AP["auth proxy<br/>loopback, option C"]:::new
        HCB["HostCallback server<br/>sibling Invoke · analytics"]:::new
    end

    subgraph EXT["Extension process — separate binary (subprocess)"]
        SERVE["extension.Serve"]:::new
        EH["Handler = workflow.Callback"]:::new
        PIC["plugin InvocationContext<br/>remoteEngine · remoteAnalytics · NetworkAccess"]:::new
    end

    API["Snyk API"]:::existing

    APP --> ENG
    APP --> LWF
    LWF -. "AddExtensionInitializer / Register" .-> ENG
    ENG --> BW
    BW --> IC

    APP --> LOADER
    LOADER -. "launch + Discover, then Register proxy" .-> ENG
    ENG -->|"Invoke(id)"| PROXY
    PROXY -->|"gRPC: Execute<br/>(config snapshot, input,<br/>proxy URL+secret, broker id)"| SERVE
    SERVE --> EH --> PIC

    PIC -->|"plain HTTP + secret"| AP
    AP -->|"authenticated HTTPS<br/>(host injects credentials)"| API

    PIC -->|"gRPC broker (bidirectional)"| HCB
    HCB -->|"Engine.Invoke(sibling)"| ENG
    HCB -->|"record"| IC

Loading

Checklist

• Tests added and all succeed — TDD throughout; go test ./pkg/extension/... ./pkg/app/... ./pkg/workflow/... passes, including subprocess end-to-end tests (load-without-rebuild, authenticated API call with host-injected token, sibling invoke + analytics over the broker) and a 24-way concurrency test that passes under -race. (Full-repo make test not run here.)
• Regenerated mocks, etc. (make generate) — protobuf now regenerates via go generate (buf + protoc plugins pinned in the tool block); committed files were regenerated through it.
• Linted (make lint) — gofmt, goimports, and go vet are clean; golangci-lint needs a CI run (its binary targeted an older Go than this module locally).
• Test your changes work for the CLI — not yet exercised against the CLI repo.

🤖 Generated with Claude Code

---

Note

High Risk
Touches authentication (loopback proxy, credential export rules) and cross-process invocation of host workflows—security-sensitive paths with broad new surface area, though heavily tested.

Overview
Adds dynamic extensions: standalone binaries loaded at runtime via hashicorp/go-plugin and gRPC, registered on the existing workflow.Engine like built-ins—no host rebuild for extension iteration.

New pkg/extension covers discovery/Execute, a per-invocation loopback auth proxy so extensions call the Snyk API with host-injected credentials (not raw tokens), and HostCallback bridging sibling Invoke, analytics, and config overrides with guards (declared-flag-only overrides, credential key blocking, invocation depth cap).

app wires WithExtensionPaths, reads paths at Init() (post-flag-parse), dedupes paths, and CreateAppEngineWithCloser to tear down subprocesses. workflow.ResolveInvokeOptions is the small core helper for forwarding Invoke options across the boundary.

Docs (dynamic-extensions*.md), make build-examples, and the whoami example round out the developer loop; dependencies add gRPC/protobuf/buf tooling.

^{Reviewed by Cursor Bugbot for commit c807005. Bugbot is set up for automated code reviews on this repo. Configure here.}

[snyk-io]

⚠️ Snyk checks are incomplete.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
⚠️	Open Source Security	0	0	0	0	See details
⚠️	Licenses	0	0	0	0	See details
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[snyk-io]

⚠️ Snyk checks are incomplete.

Status	Scan Engine	Critical	High	Medium	Low	Total (0)
⚠️	Open Source Security	0	0	0	0	See details
⚠️	Licenses	0	0	0	0	See details
✅	Code Security	0	0	0	0	0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

[basti-snyk]

Should-fix (concurrency / resource leak): The host launches `go c.broker.AcceptAndServe(brokerID, …)` on every invocation with a non-nil invocation (and makeProxy always sets it), but the plugin only dials the broker at serve.go:124 — after three early returns (NotFound, input-decode InvalidArgument at serve.go:109, identifier-parse InvalidArgument at serve.go:114). On any of those error paths the plugin returns without dialing, so this AcceptAndServe goroutine + its grpc.Server block until the plugin client is killed. Ordinary bad input reaches the decode path, so a long-lived host (snyk-ls) leaks one goroutine + listener per errored Execute for the session lifetime. Bound the accept to the Execute RPC (per-call context / stop the broker server when Execute returns) rather than relying on the plugin to always dial.\n\n— AI review

[mattdolan-snyk]

Good catch on the mechanism, though the specific leak path here is already closed: the extension's own Execute (serve.go) now dials the host broker as the very first thing it does, before any error return (unknown workflow, bad input, bad identifier) — see the earlier fix in this PR at d674285. So a plugin that reaches Execute at all will always dial back.

The residual gap you're pointing at — the RPC to the extension failing/timing out before it ever reaches Execute (so nothing on the plugin side can dial) — is real and not addressed. Bounding AcceptAndServe to the surrounding Execute call's own lifetime (stop the broker server unconditionally once c.client.Execute returns, rather than trusting the plugin) would close that too, but touches go-plugin broker internals I'd want to test more carefully before landing. Leaving as a known residual gap for now rather than rushing it — happy to pick up as a follow-up if this needs closing sooner.

[basti-snyk]

Should-fix (lifecycle / subprocess leak): CreateAppEngineWithOptions builds loader := extension.NewLoader(...), registers loader.Init, then discards the reference. Loader.Close() (loader.go:231) — the only path that runs the accumulated client.Kill cleanups — is unreachable in production (no non-test caller). A long-lived host that recreates engines per session spawns one exec.Command subprocess per extension at Init() and never reaps them; orphaned processes + gRPC conns accumulate for the host lifetime. Either return/store the loader and call Close() on shutdown (or register plugin.CleanupClients), or delete Close() if you truly rely only on go-plugin parent-death detection so it is not a false promise.\n\n— AI review

[mattdolan-snyk]

Fixed in 165f2fa: the loader initializer now closes over its *extension.Loader instance, and CreateAppEngineWithCloser returns a close func wired to it so callers that create engines dynamically can reap the subprocesses on shutdown. CreateAppEngineWithOptions is unchanged for existing callers that don't need it.

[basti-snyk]

Should-fix (regression — CLI flag silently ignored): Loader registration is gated on config.GetStringSlice(ConfigurationKeyPaths) evaluated at engine construction. The documented usage (loader.go:20-24) is a repeatable --plugin-path flag bound to this key, but CLIs normally create the engine, then parse flags, then Init(). At construction the flag is unparsed → slice empty → loader.Init is never registered, and even after flags parse nothing loads. The previous placeholder lived inside Init() (late); this moved the decision early. Register a loader initializer unconditionally and read the paths when it runs, so flag-supplied paths take effect. (Only env/opt values present at creation currently work.)\n\n— AI review

[mattdolan-snyk]

Fixed in 165f2fa (same commit as the Close() fix, since both were the same root cause): the loader initializer is now registered unconditionally and reads extension.ConfigurationKeyPaths when it actually runs at Init() time, not at engine construction. Added a regression test (TestCreateAppEngine_ExtensionPathSetAfterConstruction_StillLoads) that sets the path after CreateAppEngineWithOptions returns, mirroring the real CLI startup order.

[basti-snyk]

Should-fix (credential-isolation bypass — undermines the core premise): The host forwards config to the extension by iterating over flag names the extension itself declared via Discover, with no denylist: req.config[flag.GetName()] = config.GetString(flag.GetName()). Sensitive keys live in the same config namespace (snyk_token, api, snyk_cfg_api, snyk_oauth_token — see pkg/configuration/constants.go). A malicious/compromised extension that declares a flag named snyk_token/api/snyk_oauth_token makes the host read the real credential and serialize it into the ExecuteRequest sent to the extension process — defeating the "extension never holds credentials" guarantee the auth-proxy (option C) exists to provide. Add a reserved/sensitive-key denylist (or forbid extension flag names that collide with host config keys) on this loop.\n\n— AI review

[mattdolan-snyk]

Confirmed and fixed in aab491a: makeProxy now checks each declared flag name against a denylist of reserved host credential keys (snyk_token, snyk_oauth_token, the legacy api token alias, OAuth client-id/client-secret) before exporting its value, and skips (with a warning log) any collision instead. Added TestLoader_DoesNotExportSensitiveConfigValue covering the exact scenario you described.

[basti-snyk]

Should-fix (unbounded cross-process recursion + trust boundary): Invoke calls s.engine.Invoke(id, …) for any identifier the extension supplies, with no allowlist and no self-invocation/depth guard. Two issues: (1) an extension can drive any registered host workflow in the host's full context (in-process host workflows use the real credentials directly), not just siblings as the comments imply; (2) if the extension invokes its own flw://x, the host re-enters the same proxy → new auth proxy + broker stream + Execute → recurses, amplifying subprocesses/goroutines/listeners across the boundary until FD/goroutine/OOM exhaustion (vs a bounded in-process stack). Scope to an allowlist/capability grant and add a depth or cycle guard. Separately, this host-side broker gRPC server has no panic-recovery interceptor, so a host workflow that panics on plugin-supplied input crashes the whole host — add a recover() unary interceptor.\n\n— AI review

[mattdolan-snyk]

Split into two fixes in 5ea06d2:

1. Recursion/amplification: added a round-trip depth guard. Each hop is tracked via a context value threaded through workflow.WithContext on the host side (no proto change needed — every hop in the chain is host-process-local except the two gRPC calls, which already carry a context both ways). Invoke now rejects the call once depth exceeds a fixed max (10) instead of letting a self-referential chain run until the host exhausts file descriptors/goroutines/memory.
2. Panic isolation: added a recover() unary interceptor on the host-side HostCallback gRPC server, so a host workflow panicking on plugin-supplied input returns a gRPC error instead of crashing the host process.

On the broader point — extension can drive any registered host workflow, not just declared siblings — leaving that as-is rather than adding an allowlist: it matches how in-process workflows already call each other via engine.Invoke (no allowlist there either), and constraining it further felt like a separate design decision worth its own discussion rather than bundling into this fix. Happy to pick that up if you think it needs to be scoped down.

[basti-snyk]

Should-fix (violates the "broken extension must never block CLI startup" invariant, loader.go:105-107): loadOne dials and discovers with context.Background() (l.dialer(context.Background(), …) at :118 and conn.Discover(context.Background()) here), and grpcDialer ignores the passed context. go-plugin bounds only the handshake; once a plugin handshakes and then hangs inside Discover, this call has no deadline and Loader.Init blocks the CLI indefinitely — exactly the failure the stated invariant forbids. Add a bounded context and propagate a real ctx into the dialer.\n\n— AI review

[mattdolan-snyk]

Fixed in aab491a (same commit as the credential-denylist fix): loadOne now bounds dial + Discover to a 10s timeout instead of context.Background(), and grpcDialer derives go-plugin's StartTimeout from that same deadline instead of ignoring the passed context.

[basti-snyk]

Should-fix (nil deref / interface contract violation): GetRuntimeInfo() unconditionally returns nil. Host consumers deref it without a nil check — e.g. report_analytics_workflow.go does invocationCtx.GetRuntimeInfo().GetName()/.GetVersion(). In-process this is populated; run the same workflow as an extension and it panics inside the subprocess. Populate RuntimeInfo from the request (or from host-forwarded values) instead of returning nil.\n\n— AI review

[mattdolan-snyk]

Fixed in c92c5b8: ExecuteRequest now carries the host's runtime info name/version, and the plugin side wraps it into a real (possibly empty, but never nil) runtimeinfo.RuntimeInfo — matching in-process behavior exactly when the host has one set, and failing safe instead of panicking when it doesn't.

[basti-snyk]

Should-fix (silent wrong behaviour for sibling invokes): InvokeWithConfig(id, _ configuration.Configuration) drops its config argument and calls invoke(id, nil); InvokeRequest carries no config, so any per-invocation override is silently discarded. An extension mirroring the in-repo cfg.Set(FLAG_EXPERIMENTAL, true); GetEngine().InvokeWithConfig(WORKFLOWID_WHOAMI, cfg) pattern gets the override dropped → sibling fails ("experimental feature") or returns wrong results with no signal. Relatedly GetWorkflow always returns (nil,false) and GetWorkflows returns nil (proxies.go:120-121), so guard-then-invoke patterns silently skip valid host workflows. Forward config across the boundary and back the lookup methods with the host engine.\n\n— AI review

[mattdolan-snyk]

The InvokeWithConfig dropped-config half was already fixed earlier in this PR (commit a84dd20 — config overrides are now diffed and forwarded across the boundary via InvokeRequest.config, applied by hostCallbackServer.Invoke on top of the host's own config).

The GetWorkflow/GetWorkflows stub half is still open — deliberately not doing it here. Backing it truthfully needs a new HostCallback RPC (there's no existing way to ask the host "is this identifier registered" or "list your workflows" across the boundary), which is a bigger, separate change. Tracking it as follow-up rather than rushing a proto addition into this pass.

[basti-snyk]

AI review summary — dynamic out-of-process gRPC extensions

Reviewed origin/main...HEAD (36 files, +4819/-15) with four independent reviewers (semantic, adversarial, security scan, high-effort code review). Strong consensus. No approval — comment-only sweep. CI: no required checks reported.

What's solid: credential isolation holds — the raw token never crosses into the plugin; auth is injected host-side by the loopback proxy's transport, the per-invocation secret uses crypto/rand + subtle.ConstantTimeCompare, and the token header is stripped before forwarding upstream. Handshake magic cookie, default no-override of bundled workflows, plugin-side handler panic isolation, and e2e/dialer/convert/auth-proxy tests are all present.

Should-fix (posted inline)

1. Broker AcceptAndServe goroutine/server leak on plugin early-return paths — plugin.go:117.
2. Loader.Close() orphaned → extension subprocesses never reaped in a long-lived host — app.go:435 / loader.go:231.
3. --plugin-path read at construction, not Init() → CLI-flag paths silently ignored — app.go:434.
4. Credential-isolation bypass via flag-name collision — extension declaring a flag named snyk_token/api/snyk_oauth_token gets the real credential serialized into ExecuteRequest — loader.go:182.
5. Host callback invokes any workflow + no depth/cycle guard (unbounded cross-process recursion) and no panic-recovery interceptor on the host-side broker server (plugin-triggered host panic crashes host) — hostcallback.go:49.
6. Discover/dial use context.Background() (no timeout) → a hung extension blocks CLI startup, violating the stated "broken extension must never block startup" invariant — loader.go:118,126.
7. GetRuntimeInfo() returns nil → panics extension handlers that read runtime info (analytics) — invocationcontext.go:128.
8. InvokeWithConfig drops its config arg; GetWorkflow/GetWorkflows always empty → sibling invocation silently wrong — proxies.go:106,120-121.

Suggestions (non-blocking)

• Lossy JSON payload round-trip — non-[]byte/string payloads decode to map[string]interface{}/float64; int64 > 2^53 loses precision; only the map case is tested (convert.go:108,128).
• specsToFlagSet only reconstructs bool/int — stringSlice/duration/count flags collapse to plain string; def, _ := strconv.Atoi(...) swallows malformed-default errors (serve.go:201).
• Enable AutoMTLS on the go-plugin client/server — currently unix-socket (Linux/mac) / loopback (Windows) with no crypto auth on the host↔plugin + broker channels (loader.go:244, serve.go:155).
• Auth-proxy http.Server sets no ReadHeaderTimeout (gosec G112) — loopback + secret-guarded, low risk (authproxy.go:71).
• Proxy token added as a global header on the extension's NetworkAccess — rides along if extension retargets the URL; token is loopback-only so low impact (invocationcontext.go:92).
• Per-invocation auth-proxy churn — a fresh listener + goroutine + secret per invocation even for no-network workflows; one proxy per loaded binary would suffice (loader.go:186).
• Dead local analytics write — remoteAnalytics writes into a discarded local analytics.New() never flushed on the plugin side (proxies.go:33).
• WithLogger(nil) would nil-panic later; guard with zerolog.Nop() (loader.go:66). exec.Command(path) does no stat/executable check — failures collapse into a generic warn-and-skip (loader.go:247).
• Blast-radius docs: an extension gets full authenticated Snyk API access (no method/path allowlist) and can drive the full host workflow set — worth documenting for a third-party plugin system.

Supply chain: new deps (grpc@1.81.1, protobuf@1.36.11, hashicorp/go-plugin@1.8.0, go-hclog@1.6.3) scanned clean; tool block adds build-time bufbuild/buf (build-only). Only SCA vulns are pre-existing in go-git/v5@5.19.0 (no fix). No in-diff secrets (test-only dummy tokens).

— AI review

[mattdolan-snyk]

Addressing the 3 findings from the review guide:

• Functional Option Loss — fixed in a84dd20. InvokeRequest gained a config map; remoteEngine.Invoke now diffs the resolved configuration against its base and forwards only the overridden keys, which hostCallbackServer.Invoke applies on top of the host's own config before running the sibling. WithContext/WithInstrumentationCollector intentionally stay host-only — a Go context.Context and an analytics.InstrumentationCollector can't be serialized across the process boundary.
• Redundant Subprocess Spawning — fixed in 8840dc4. Loader.Init is now idempotent: a second call is a no-op instead of re-spawning every configured binary.
• Subprocess Lifecycle Leak — not fixed here. CreateAppEngineWithOptions returns workflow.Engine only, so there's no clean way to hand back the Loader for Close() without changing that public signature. Leaving as a known limitation for now; happy to discuss an API change (e.g. an engine shutdown hook, or a variant that also returns a closer) in a follow-up if that's wanted.

[mattdolan-snyk]

Fixed the config-override type-safety issue in dcb1282: configOverrides now skips slice-typed values (e.g. AUTHENTICATION_SUBDOMAINS) instead of stringifying them, which would have corrupted the host's GetStringSlice reads once hostCallbackServer.Invoke applied the override with cfg.Set(key, aSingleString). Added TestConfigOverrides_SkipsSliceTypedValues covering it.

The "Subprocess Resource Leak" finding here is the same Loader.Close() issue already fixed earlier in this PR (165f2fa) — this review ran against the pre-fix commit.

[mattdolan-snyk]

Two points from this review pass:

• Subprocess Lifecycle Management — by design, not a gap: CreateAppEngineWithOptions/CreateAppEngine are unchanged (additive API, existing callers like pkg/devtools/cmd.go keep today's behavior). CreateAppEngineWithCloser is opt-in for callers that create/destroy engines repeatedly in a long-lived process. Callers that need the closer should switch to it; that's a call for each call site's owner, not something this PR can force retroactively without changing existing signatures.
• Configuration Loss Risk (path-format dedup) — correct, and a real limitation: utils.Dedupe only catches byte-identical strings, so ./foo and /abs/path/foo (same binary, different spelling) still double-launch. Leaving as-is for now — normalizing (e.g. filepath.Abs) has its own sharp edges (changes semantics if a relative path is intentional, differs by CWD at call time), and this is a narrower version of a case that was already somewhat possible before this PR's dedup fix too. Open to tightening it if it turns out to bite in practice.

[basti-snyk]

Critical (credential exfiltration — new since last review): The inbound HostCallback.Invoke path applies extension-supplied config overrides onto a clone of the host's real, token-bearing configuration with no key filtering — cfg := s.config.Clone() then cfg.Set(key, value) for every entry in req.GetConfig(). This is the reverse of the outbound path (loader.go sensitiveConfigKeys), which deliberately refuses to export credential-colliding keys so the extension never holds the token.

The gap is exploitable: the auth middleware decides whether to attach the token by prefix-matching the request URL against config.GetString(API_URL) from this same cloned config (pkg/networking/middleware/auth_header.go ShouldRequireAuthentication). A malicious/compromised extension can call the callback with config[API_URL]="https://attacker.example" (or AUTHENTICATION_ADDITIONAL_URLS) and then invoke any host workflow that makes an authenticated API call — the host runs the sibling with the real credentials and sends them to the attacker's host. That defeats the auth-proxy isolation this whole feature is built around. Verified independently by three reviewers and traced to the middleware source.

Compounding: s.engine.Invoke(id, ...) (line 104) runs any workflow the extension names — there is still no allow-list restricting a plugin to a declared/sibling set (raised in the prior review).

Fix: host-side, reject/strip override keys in sensitiveConfigKeys and the network-routing keys (configuration.API_URL, AUTHENTICATION_ADDITIONAL_URLS, proxy / TLS-skip keys) before cfg.Set; ideally allow only keys declared by the invoked workflow's flags. Add a test asserting an extension-supplied API_URL override does not change the host sibling's effective endpoint/credentials.

— AI review

[mattdolan-snyk]

Confirmed and fixed in 28f2a67 — you're right, this defeated the whole isolation design. hostCallbackServer.Invoke now only applies an override for a key the invoked workflow itself declared as configurable (via its ConfigurationOptions/flags at Register time), mirroring how the outbound (host-to-extension) direction already only exports a workflow's own declared FlagSpec keys. An override for API_URL, AUTHENTICATION_ADDITIONAL_URLS, or any other key the target workflow didn't register is refused outright — no fallback, no partial application. Added TestHostCallbackServer_RejectsConfigOverrideForUndeclaredKey covering exactly the API_URL exfiltration path you traced.

On the allow-list point (any workflow, no sibling restriction): still the same open design question from the prior review round — left as-is pending a product/security decision on the intended blast radius, not re-litigating that here.

[basti-snyk]

Should-fix (residual subprocess leak + unsynchronized loader): The prior review flagged that Loader.Close() was unreachable; this revision adds a closer and CreateAppEngineWithCloser, but the most-used constructor CreateAppEngineWithOptions still discards it (engine, _ := createAppEngine(opts...)). Any existing caller (CLI, snyk-ls) that adopts --plugin-path through this constructor and creates engines per-session / repeatedly leaks the launched extension subprocesses, their gRPC conns, goroutines, and per-invocation auth-proxy listeners for the host lifetime, with no signal. Consider registering the loader's Close on an engine shutdown hook so the default path is safe, rather than relying on callers to switch to the closer variant.

Separately, the loader variable in createAppEngine is written by the extension-initializer closure (runs inside engine.Init()) and read by the returned closer with no synchronization — Loader.mu guards the struct internals but not this app-level pointer. A daemon that runs closer on a shutdown goroutine while Init() is still executing races and may observe a nil/partial loader, skipping Close() and leaking subprocesses.

— AI review

[mattdolan-snyk]

Two separate points here:

• CreateAppEngineWithOptions not getting the closer — same as my reply on the previous round: by design, additive API, existing callers keep today's behavior. Not re-litigating.
• Unsynchronized loader variable — real bug, fixed in c807005. The extension initializer writes loader from inside engine.Init(); the closer reads it. Both were plain closured-variable accesses with no synchronization, so a shutdown handler calling closer() concurrently with a still-running Init() (e.g. slow-loading extension) could observe a torn/nil read and skip Close(), leaking the subprocess. Now guarded by a mutex. Added a race-detector regression test (TestCreateAppEngineWithCloser_ConcurrentInitAndCloseIsRaceFree) exercising Init() and closer() concurrently — clean under go test -race.

[basti-snyk]

AI re-review — dynamic out-of-process gRPC extensions

New commits landed since the last AI review (which was against 4c017b28, 2026-07-08). Re-reviewed origin/main...HEAD @ d61fdaeb (37 files, +5531/-16) with four independent reviewers (semantic, adversarial, security scan, high-effort code review). No approval — comment-only sweep.

Prior findings now addressed (verified) ✅

• Outbound credential-collision — sensitiveConfigKeys now refuses to export auth/token keys on flag-name collision.
• --plugin-path read lazily inside the initializer (not at construction); paths de-duplicated.
• Host-callback recursion bounded (maxInvocationDepth, depth carried in context) and a panic-recovery interceptor added.
• GetRuntimeInfo() now mirrors host RuntimeInfo instead of returning nil.
• InvokeWithConfig now forwards overrides across the boundary.
• Loader.Init idempotent; Init on a closed loader rejected; WithLogger(nil) guarded; malformed int flag defaults logged; auth-proxy ReadHeaderTimeout set (G112).

New this review

🔴 Critical — inbound host-callback config-override credential exfiltration (pkg/extension/hostcallback.go:96-104, posted inline). The fix that made InvokeWithConfig forward overrides also lets an untrusted extension set any config key (incl. API_URL / AUTHENTICATION_ADDITIONAL_URLS) on a clone of the host's real token-bearing config before a sibling host workflow runs — redirecting authenticated host requests to an attacker. Confirmed by 3 of 4 reviewers and traced to auth_header.go ShouldRequireAuthentication. Fix = host-side denylist/allow-list on the override loop + a test.

🟠 Should-fix — residual subprocess leak + unsynchronized loader (pkg/app/app.go:417, posted inline). A closer was added, but the default constructor CreateAppEngineWithOptions still discards it → per-session/repeated-engine hosts leak extension subprocesses; the loader variable is also read/written across goroutines without synchronization.

Still open from the prior review (not re-posted inline to avoid duplicates)

• No workflow allow-list on host callbacks (hostcallback.go:104) — an extension can invoke any registered host workflow; compounds the Critical above.
• GetWorkflow/GetWorkflows on the remote engine always return empty (proxies.go) — extension code that guards-then-invokes silently no-ops out-of-process though Invoke would succeed.
• No AutoMTLS on the go-plugin client/server (loader.go, serve.go) — host↔extension + callback channel authenticated only by the public magic cookie; a co-resident local process could drive host workflows. Security agent elevates this given the callback channel carries real credentials.
• conn.Execute has no host-imposed timeout (loader.go) — the launch/Discover path is bounded, but a top-level Invoke with no deadline lets a hung extension block the host indefinitely.
• Suggestions unchanged: lossy JSON payload round-trip / slice-typed flag values across the boundary; per-invocation auth-proxy churn; proxy secret attached as a global header; authproxy.stop() uses abrupt Close().

Supply chain: new deps (grpc@1.81.1, protobuf@1.36.11, hashicorp/go-plugin@1.8.0, go-hclog@1.6.3, build-only bufbuild/buf) scanned clean. Only SCA vulns are pre-existing in go-git/v5@5.19.0 (no fix). Snyk Code error-level hits are outside this diff. No real secrets in-diff (test fixtures only). go vet + go test -race ./pkg/extension/ pass.

— AI review

[snyk-pr-review-bot]

PR Reviewer Guide 🔍

🧪 PR contains tests

🔒 No security concerns identified

⚡ No major issues detected

📚 Repository Context Analyzed This review considered 89 relevant code sections from 11 files (average relevance: 0.94) docs/extension-architecture-guide.md (2 segments, lines 1-209) pkg/extension/proto/extension.pb.go (2 segments, lines 815-953) CONTRIBUTING.md (2 segments, lines 170-300) pkg/workflow/types.go (lines 1-115) pkg/configuration/constants.go (lines 3-96) pkg/local_workflows/connectivity_check_extension/connectivity_check_workflow.go (lines 1-97) pkg/local_workflows/ignore_workflow/ignore_workflow.go (lines 1-98) ... and 4 more file(s)

[cursor]

Cursor Bugbot has reviewed your changes and found 1 potential issue.

^{Reviewed by Cursor Bugbot for commit c807005. Configure here.}

[cursor]

Inbound overrides skip credential denylist

Medium Severity

Sibling HostCallback.Invoke applies extension-supplied config overrides only against the invoked workflow’s declared flags, not the host’s sensitiveConfigKeys denylist used on outbound snapshots. If a host workflow ever registers a flag whose name matches a credential key, an extension could push that value onto a clone of the host configuration for that invocation.

Additional Locations (1)

• pkg/extension/loader.go#L23-L36

 

^{Reviewed by Cursor Bugbot for commit c807005. Configure here.}

[basti-snyk]

Critical (credential redirect — the new inbound override filter is bypassable): This revision adds invokableConfigKeys to filter extension-supplied config overrides, but it keys the defense on an attacker-controlled allowlist: the permitted keys are exactly the flags the invoked workflow declared, and a dynamically-loaded extension controls its own workflows' declared flags. Discover returns FlagSpecs → specsToFlagSet (serve.go) turns every spec into a pflag with no rejection of reserved key names → registerWorkflow registers it as-is. So a malicious extension can populate the allowlist with a sensitive routing key.

Exploit (self-invoke): the extension declares a workflow flag named snyk_api (= configuration.API_URL) and/or internal_additional_auth_urls (= AUTHENTICATION_ADDITIONAL_URLS), then calls HostCallback.Invoke(id=<its own wf>, config={"snyk_api":"https://attacker.example"}). invokableConfigKeys returns {"snyk_api":true}, the check here passes, cfg.Set("snyk_api", …) + WithConfig(cfg) poison the invocation, and startAuthProxy reads API_URL and forwards the request — with the host's real token injected host-side — to the attacker's host. This is exactly the "attach the user's real token to a host the extension chose" threat the comment above describes; the allowlist does not stop it.

Fix: gate the inbound path on an unconditional denylist (credentials and auth-routing keys: snyk_token, snyk_oauth_token, snyk_api/API_URL, internal_additional_auth_urls, internal_auth_subdomain, plus the full outbound sensitiveConfigKeys set), rejecting them regardless of what the workflow declared — and reject those reserved names in specsToFlagSet/registerWorkflow so they never enter any workflow's ConfigurationOptions. Note: the outbound sensitiveConfigKeys denylist (loader.go) currently omits the URL-routing keys, so it is no backstop. (The case-folding path is fine — both sides lowercase and viper normalizes keys — so consistency there is not the issue; the allowlist-poisoning is.)

— AI review

[basti-snyk]

Should-fix (mutex makes the pointer race-free but does not serialize the Init-vs-close lifecycle it claims to guard): The comment above says loaderMu defends against "a shutdown handler racing a still-running Init()", but the lock is dropped between the read at :467 and the store at :486, so check-then-act is not atomic and the leak windows remain:

• Init-after-close leak: closer() (:491) takes the lock, reads loader == nil (initializer hasn't stored yet), unlocks, closes nothing. The initializer then stores newLoader and runs newLoader.Init(engine) (:488), spawning extension subprocess(es) after shutdown — nothing ever calls Close() on them.
• Double-init leak: two concurrent initializer runs both read existing == nil before either stores, both NewLoader + Init (double-spawn), and only the last-stored loader is ever Close()d — the earlier one's subprocesses leak.

go test -race (and TestCreateAppEngineWithCloser_ConcurrentInitAndCloseIsRaceFree) verifies data-race freedom but cannot catch either logical leak, so the new test gives false confidence. Fix: hold loaderMu across the whole check-create-store, and add a closed bool under the same mutex so the initializer refuses to start a loader once closer has run (and closer closes whichever loader is set). The root-cause-correct home for the shutdown guard is Loader.Init itself. If Init and shutdown are in fact always single-threaded, drop the mutex and document that contract instead of half-implementing serialization. (Separately, the default constructor CreateAppEngineWithOptions still discards this closer — raised in the prior review — so the loader is never closed on that path regardless.)

— AI review

[basti-snyk]

AI re-review — dynamic out-of-process gRPC extensions

3 new commits landed since the last AI review (which was against d61fdaeb). Re-reviewed origin/main...HEAD @ c80700511d (delta touches hostcallback.go, loader.go, app.go + tests) with four independent reviewers (semantic, adversarial, security scan, high-effort code review). No approval — comment-only sweep.

Prior findings now addressed (verified) ✅

• Inbound config-override credential leak — a key filter (invokableConfigKeys) was added on HostCallback.Invoke, fail-closed on unknown workflows. (But see Critical below — the filter is bypassable.)
• Extension subprocess leaked on Discover failure — loadOne now calls cleanup() on the error path (loader.go); client.Kill is idempotent so the extra call from Close() is safe.
• Data race on the shared loader var — now mutex-guarded (app.go). (Race-safe, but the lifecycle leak remains — see Should-fix.)

New blocking findings (posted inline)

1. Critical — the new inbound override filter is bypassable → token redirect. invokableConfigKeys allowlists the keys the invoked workflow declared, but a dynamically-loaded extension controls its own workflows' declared flags (Discover → specsToFlagSet → registerWorkflow, no reserved-name rejection). Declaring a flag named snyk_api/internal_additional_auth_urls, then sending it as an override, poisons API_URL so the host auth-proxy forwards the extension's request — with the real token injected host-side — to an attacker-chosen host. Fix = unconditional denylist on the inbound path (+ reject reserved names at spec→flag conversion). hostcallback.go:108.
2. Should-fix — the app.go mutex removes the data race but not the Init-vs-close subprocess-leak race its own comment cites. Check-then-act isn't atomic: closer() can observe loader==nil while the initializer then spawns subprocesses post-shutdown (Init-after-close), and concurrent Init can double-spawn with only the last loader closed. -race can't catch either. Fix = hold the lock across check-create-store + a closed guard (root-cause home is Loader.Init). app.go:466.

Suggestions (non-blocking)

• loadOne early-kill covers only Discover failure, not the "Discover succeeds but every registerWorkflow fails / zero workflows registered" case — same leak rationale applies (loader.go).
• s.config.Clone() runs unconditionally inside the override block even when no key is allowed (applied stays false, clone discarded) — clone lazily on first allowed key (hostcallback.go:109).
• Behavior nuance: when overrides are present but all keys are rejected, no WithConfig is appended, so the sibling runs against the engine base config clone rather than the caller invocation's config the old code passed — only bites when those diverge (hostcallback.go:118).

Security scan: new deps (go-plugin@1.8.0, grpc@1.81.1, protobuf@1.36.11) scanned clean; no in-diff secrets (test-only dummy tokens); the auth-proxy design (crypto/rand per-invocation secret, ConstantTimeCompare, loopback-only, token stripped upstream) holds. Only SCA findings are pre-existing in go-git/v5 (not bumped here).

— AI review

[danskmt]

The inner function can be moved to a separate function, so this current method (createAppEngine) doesn't grow too much and keeps it easy to read

[danskmt]

Note that createAppEngine always creates that close function - you probably expect to use the close function here or return it in same way you do for CreateAppEngineWithCloser