Follow-ups from the #348 security and code review (non-blocking)

[leggetter]

Why

Two review passes over #348 (a security review and a code review) found seven release-blocking defects, all fixed in that PR. They also found a set of real-but-not-blocking issues that would otherwise exist only in a chat transcript. This is where they live.

None of these block v3.0.0. Each has a concrete failure scenario.

Correctness risks

• Fallthrough default: arms perform mutations. pkg/gateway/mcp/tool_connections.go maps any uncased action to disable; tool_event.go to mute; tool_request.go to retry; pkg/outpost/mcp/tool_destinations.go to delete; tool_tenants.go to portal. Adding a read action to an ActionSet and forgetting the case silently performs a mutation instead of erroring. Dispatch validates the action name against the set, so this is not reachable today — it is a trap for the next person adding an action. An explicit default: returning "unhandled action" costs nothing.

• CLI and MCP send different queries for the connection disabled filter. pkg/cmd/connection_list.go sends disabled=true|false, with a comment recording behaviour verified against the API. pkg/gateway/mcp/tool_connections.go sends disabled_at[any]=true, and sends nothing for disabled: false. Two consequences: an agent asking "which connections are disabled?" issues a different query from the CLI, and there is no way to ask MCP for only active connections, which is the CLI's default. Verify disabled_at[any] against the live API first — the existing test asserts against a stub that echoes whatever it is asked, so it proves the key is sent, not that it is understood. If the API ignores an unknown key, the tool returns everything while looking filtered.

• outpostQuery leaves index gaps. pkg/hookdeck/outpost.go skips empty list entries but indexes on the original position, so ["a","","b"] becomes id[0]=a&id[2]=b. Confirm the API tolerates the gap.

• CLI outpost publish lacks the tenant guard the MCP tool has. TenantExistsForPublish is called only from pkg/outpost/mcp/tool_publish.go. The CLI publishes straight through, so --api-key for the wrong project prints ✔ Event accepted for an event that will never be delivered. Partly mitigated by the "No destinations matched this topic" line, but the two surfaces answer the same mistake differently.

Maintainability

• Outpost's spec list is duplicated three times — pkg/outpost/mcp/tools.go and twice in tool_help.go — and they already differ in how they handle publish. Gateway solves this with a single resourceSpecs(). Adding an Outpost tool and updating two of three lists leaves help silently stale.

• FillProjectDisplayNameIfNeeded re-runs on every tool call and never caches failure. pkg/mcpcore/server.go calls it per invocation; it returns early only once a name resolves. With a project-scoped key whose validated project differs from the active one, the lookup fails and nothing is cached, so every tool call pays two extra API round trips indefinitely. This also leaks into the unit suite: newTestClient("https://api.hookdeck.com", ...) in pkg/gateway/mcp/server_test.go sets a ProjectID with no name, so those tests make real HTTPS calls to production with a fake key. Cache the failure, and point the tests at their stub.

• outpost config set fetches the current config on every run (pkg/cmd/outpost_config.go) but only uses it in the --dry-run branch — a wasted GET on every real write.

Low-severity hardening

• Debug logging writes response bodies unredacted. pkg/hookdeck/client.go logs full response bodies at debug level; request headers and bodies are redacted. Pre-existing, but this release adds GET /tenants/{id}/token, whose body is a 24-hour tenant credential. hookdeck outpost tenant token acme --log-level debug, or an MCP server started with --log-level debug, writes that JWT to stderr, which MCP clients typically persist to a log file. Same for the portal URL.

• Predictable cache path is symlink-clobberable on Linux. pkg/cmd/outposttypes/types.go builds a fully predictable path under os.TempDir() and writes with os.WriteFile, which follows an existing symlink. On Linux another local user can pre-create it as a symlink to a file the victim can write. macOS is unaffected (os.TempDir() is per-user).

Needs someone outside this repo

• Does the transformation sandbox have network egress? gateway_transformations run is deliberately Write: false, so a read-only MCP session can execute arbitrary JavaScript server-side. That was checked to have no persistent side effects (no execution record, no id returned), and the reasoning holds. But if the sandbox can make outbound requests, a read-only session could exfiltrate anything it can read via the API. Not assessable from this repo — worth confirming with whoever owns the sandbox.

Related

• v3.0.0: Outpost support, Gateway MCP write mode, and the gateway_ tool rename #348 — the PR these came from
• Read-only MCP mode filters actions but still advertises write-only properties #363 — write-only properties advertised in read-only mode, raised in the same review
• Acceptance tests leak sources and destinations: ~36k of each in the CI Gateway project #362 — acceptance tests leaking sources and destinations