Conversation
…te scope OAuth access tokens carry an mcp:read|write|admin scope that the consent screen presents as a real permission boundary, but scope was enforced only at the MCP tool wrapper. The REST routes those tools call are reachable directly with the same Bearer token; the prior pass gated /api/execute/* and the workflow-mutation routes but missed every other OAuth-reachable mutation, because resolveOrganizationId and resolveCreatorContext dropped the token scope from their return shape. Surface scope through both helpers (OAuth callers carry the token scope; api-key and cookie-session callers leave it undefined, which scopeSatisfies treats as full access, so the dashboard and kh_ keys are unaffected), then apply the existing requireScope(scope, SCOPE_MCP_WRITE) guard to the remaining state-changing handlers: - integrations create/update/delete/test (credential CRUD) - user wallet-token add/remove - address-book and tag create/update/delete - project create/update/delete - organization settings + execution-digest update - web3 ABI fetch and gas estimate (outbound RPC/explorer work) A class re-derivation over every mutation route confirms no other OAuth-reachable mutation remains ungated; keys/[keyId] revoke also gets the guard as defense-in-depth even though its session+MFA gate already blocks OAuth. The control hinges on authenticated OAuth tokens always carrying a string scope; document that invariant in oauth-auth and pin it with a regression test so a future token-shape change cannot silently reopen the gap. Verified on a local dev server: an mcp:read token is now rejected (403 insufficient_scope) at every gated route while mcp:write tokens, kh_ API keys, cookie sessions, and all GET reads are unaffected.
…xecution /api/execute/node spread the caller's free-form config verbatim into the step input, preserving config.web3Connection. The web3 step cores forward web3Connection to resolveSignerForNode, whose 'eoa' branch short-circuits to the org's Turnkey EOA and bypasses the org Safe's Zodiac Roles policy on org-custodied writes. A holder of an mcp:write key could thus route an unconstrained write through the EOA, sidestepping the Role allowances that bound a compromised or over-permissioned key. Strip web3Connection (alongside the already-stripped network, integrationId and _context) before it reaches the step input, forcing the persisted node's signer mode (the org-policy path: Safe + active Role). The dedicated transfer/contract-call/check-and-execute routes never forward web3Connection, so this keeps /api/execute/node no weaker than its siblings. Also strip the reserved keys from the persisted/redacted execution-audit input so the record matches what the step actually received.
…binding The sandbox grandchild's fetch ran the SSRF blocklist against a DNS lookup, then handed the original hostname to a fresh fetch, letting undici re-resolve at connect time. A short-TTL rebinding record (public on the guard lookup, private at connect) could dial a cluster-internal target. Resolve once and dial only blocklist-validated IPs: route the pinned fetch through node:http/https with a custom lookup that re-validates every candidate address and connects to the validated IP, while keeping the real hostname for TLS SNI, certificate validation and the Host header. Every redirect hop is pinned and re-validated too (the existing per-hop SSRF check is preserved). The grandchild has no undici, so this mirrors lib/safe-fetch.ts validatingConnect with node builtins. Requests Accept-Encoding: identity with a bounded gzip fallback (no bespoke deflate/brotli decode) and releases the socket on every redirect-reject path.
…argets The webhook step, the http-request system action and the blockscout step take a fully attacker-controlled URL and relied on safeFetch, whose enforcement defaulted to shadow mode (allow + log) unless SAFE_FETCH_ENFORCE=true. Any environment that omits that env var shipped an unauthenticated SSRF primitive (e.g. fetch http://169.254.169.254/... to read cloud metadata). Add an unconditional assertUrlIsPublic() pre-check before safeFetch on every user-URL sink (webhook, http-request, blockscout); it ignores the enforce flag and always blocks loopback/link-local/metadata/private/reserved hosts and non-http schemes, and hard-fails regardless of failOnError. Flip the safeFetch shadow-mode default to fail-closed (enforce unless SAFE_FETCH_ENFORCE=false) so a missing env var fails safe; deployed envs already set it true. Document the local-dev opt-out in .env.example.
…step The web3 sign-typed-data step signs a caller/config-supplied EIP-712 payload with the org's creator EOA (the default funded wallet) unconditionally, with no domain or primaryType allowlist and no Turnkey policy on creator sub-orgs. A marketplace template can embed a hardcoded Permit; the deploy path copies the node config verbatim into the victim's org, the executor passes it straight to Turnkey, and the victim's wallet signs it -- a supply-chain token-drain via a signed off-chain authorization. Reject fund-moving primaryTypes (Permit, PermitSingle/Batch, PermitTransferFrom family, ERC20Permit, DAIPermit, TransferWithAuthorization, ReceiveWithAuthorization, CancelAuthorization, DelegateBySig, Delegation, and any type whose name contains permit) before any wallet/signer resolution, and reject a present-but-unsupported domain.chainId. The gate lives in the web3 step only; the x402/MPP and agentic signers use a separate, policy-gated sub-org and entrypoint and are unaffected.
The main-app sandbox client v8.deserialize'd the sandbox server's HTTP response
(lib/sandbox/client.ts), and the sandbox server v8.deserialize'd the inbound
/run request body before any structural check (sandbox/src/index.ts).
v8.deserialize is an unsafe sink on untrusted bytes (host-object gadgets,
prototype-pollution wire formats), and these are the two cross-process HTTP hops
at the sandbox boundary -- the last v8 sites after the in-pod child->parent paths
were already moved to an fd-3 tagged-JSON frame.
Cut both directions over to safe codecs, in lockstep:
- Request (client -> server): JSON.stringify({ code, timeout }) with
Content-Type application/json; the server JSON.parses it and keeps the
existing object / "code"-present validation.
- Response (server -> client): the server encodes the (already fd-3-laundered)
outcome with a new exported encodeSandboxResult -- the structured-clone-
preserving tagged-JSON encoder, the exact inverse of the existing
decodeSandboxResult and prototype-pollution-safe; the client decodes with
decodeSandboxResult and validates the ChildOutcome shape. No node:v8 remains
in either boundary file.
Deploy note: this is a hard cutover and the sandbox service deploys from a
separate pipeline than the main app, so deploy the sandbox service first to
minimise the brief Code-node window where a new client could meet an old
(v8-only) server.
Wire the security-behavioral-scan endpoint to a Kubernetes CronJob in both prod and staging (every 5 minutes), mirroring the existing reaper job and reusing deploy/scripts/reaper.sh. Auth: the endpoint validates via authenticateInternalService (async) scoped to caller "scheduler" -- the legacy-bearer path resolves the CronJob's X-Service-Key (SCHEDULER_SERVICE_API_KEY) to that caller, and least-privilege rejects the other internal callers. Widen EXECUTION_LOOKBACK_MS to 10 minutes so consecutive 5-minute scans overlap and scheduler jitter cannot drop a new-account-first-workflow event; matched to the 10-minute window of the downstream Loki alert. The scan body is wrapped so a failure of the scan itself (e.g. a DB error) emits a self-guarded security.behavioral.scan_error signal and a 500 instead of silently dropping detection to zero behind a green CronJob (reaper.sh reports a non-2xx as a successful job).
Add the security-behavioral-scan CronJob to the PR-environment Helm template (every minute, faster than prod's 5-minute cadence) so a PR deployment exercises the scheduled scan end-to-end.
These two routes are read-shaped (estimate gas, fetch a contract ABI) and were gated on mcp:write. Gate on mcp:read instead so a legitimate mcp:read OAuth token can reach them, while still rejecting public/empty/invalid-scope tokens. Api-key and cookie-session callers remain full-access (scope undefined).
Drives the real route handlers with real minted OAuth tokens and asserts the scope-gate outcome across the three OAuth-reachable auth-helper families (resolveOrganizationId, resolveCreatorContext, getDualAuthContext) plus the two read-gated routes. Confirms mcp:read passes gas/estimate and fetch-abi while non-read scopes are rejected, and mcp:read is blocked on write routes.
…3-oauth-scope-mcpreadwriteadmin-is-enforced-only-at-the # Conflicts: # app/api/keys/[keyId]/route.ts
Both the step-input path and the persisted audit-input path stripped the same four reserved config keys (network, integrationId, web3Connection, _context) via duplicated destructures. A future reserved field added to one site but not the other would let the audit record and the step input diverge on a security-sensitive boundary. Collapse both into one helper.
The existing web3Connection test only asserted on the step input (the stepFn mock). The audit-input strip -- the keys that reach the execution record via checkAndReserveExecution -- had no coverage, even though redactInput passes unknown keys through verbatim, so a regression there would silently persist web3Connection into the audit record. Assert the reserved keys are absent from the reserved execution input.
…t log directExecutions.input is a forensic-only, write-only audit column: it is never returned by the status endpoint, never read by analytics/billing/ spending-cap, and never replayed. For that use case the most useful record is what the caller actually sent -- a smuggled web3Connection='eoa' is a signer-bypass attempt worth seeing. Previously the node route stripped web3Connection from the audit entirely (losing the attempt) while the sibling routes (transfer, contract-call, check-and-execute) persisted it at the top level of the raw body (where a reader could mistake it for a value that influenced the org-custodied write). Neither is faithful. Add withRejectedSignerOverride: it relocates a caller-supplied web3Connection out of the top level and into _rejectedConfig, so the audit log shows the attempt without implying it took effect. Applied to all five audit-write sites (node x2, transfer, contract-call, check-and-execute). swap is a 501 stub and the protocol catch-all writes no audit row, so both are out of scope. Unit-test the helper directly (incl. the sibling raw-body case) and assert node's audit input carries _rejectedConfig while the step input never does.
Mirror the webhook sink: emit a logUserError on an SsrfBlockedError so all three user-URL sinks produce queryable telemetry when an internal/metadata target is blocked, instead of silently returning an error.
…OnError A malformed endpoint makes assertUrlIsPublic's URL parse throw a TypeError. Treat it as a configuration error and hard-fail it, rather than letting it fall through to the failOnError soft-fail path where an aggregator workflow could swallow it as a null-data success. DNS resolution failures stay soft-failable.
The opt-out only relaxes safeFetch's own check; the always-on assertUrlIsPublic guard on the webhook/http-request/blockscout sinks still rejects internal targets regardless of the flag. Document that reaching localhost through those sinks is intentionally unsupported, even in shadow mode.
reaper.sh used curl -sS without -f, so a 4xx/5xx (e.g. an HMAC-secret mismatch returning 401 before the handler runs) was delivered with exit 0 and left the CronJob green while the job's work was silently undone. Capture the response, parse the http_code from the -w trailer, and exit non-zero on any non-2xx (or unparseable status) so Kubernetes records a failed job. Applies to every cron that runs this script. Update the scan route's catch-block comment and its test comment, which claimed reaper.sh treats a non-2xx as a successful job -- no longer true; the self-failure signal is now defense-in-depth that names why detection went dark rather than the sole failure signal.
The auth comment overstated the guarantee. In v1 all internal callers share one HMAC secret and the caller is bound into the signed string, so the caller==scheduler check gives audit attribution and blocks honest misrouting -- it does not cryptographically stop a compromised internal service from signing as scheduler. Comment-only; no behavior change.
The HMAC signing string lives in both shell (deploy/scripts/reaper.sh) and TypeScript (lib/internal-service-auth.ts). Every existing test mocks the verifier and the TS helper re-implements the format, so nothing pinned the shell producer -- a drift in reaper.sh (pathname, field order, body digest, caller) would ship a silent 401 in prod uncaught. Run the real reaper.sh against a stub curl (no network), capture the X-KH-* headers, and assert the unmocked verifier accepts them as caller scheduler; plus a tamper case so the positive isn't trivially green. Skips when sh/openssl are unavailable so it can't wedge a minimal runner.
The overlapping scan windows re-emit the same execution from two consecutive scans (~10x in PR envs where the job runs every minute), fragmenting it across many Sentry events. Fingerprint on the executionId so the duplicates collapse into a single Sentry issue while each event keeps its full tags/extra for triage; Loki already dedupes the page. Lock the fingerprint in the response-shape test.
…pe-mcpreadwriteadmin-is-enforced-only-at-the fix(oauth): gate remaining OAuth-reachable mutation routes on mcp:write scope
Apply biome formatting to the new contract test and the fingerprint assertion, and lift the header/timestamp/signature regex literals to module scope (lint/performance/useTopLevelRegex).
…rity-behavioral-scan feat(security): schedule the behavioral detection scan cron
The JSON request + tagged-JSON response wire replaced base64(v8) in a hard
cutover with no version signal, so a deploy-time skew between the app and the
sandbox service surfaced as a confusing "malformed body"/parse error. Add a
shared SANDBOX_WIRE_VERSION ("json-v1"): the client sends it as the
X-Sandbox-Wire request header and validates it on the response; the server
requires it on /run (415 with a clear message otherwise) and echoes it on the
200. Either direction of a skew now fails fast with an explicit version-mismatch
message. This is not dual-accept of the old base64(v8) wire -- doing so would
reintroduce the v8.deserialize gadget this PR removes.
…-typed-data-step-signs-arbitrary-eip-712-with fix(web3): block fund-moving EIP-712 primaryTypes in sign-typed-data step
The response-size cap used `Number.parseInt(env ?? "", 10) || DEFAULT`, which accepts a negative override as truthy: SANDBOX_MAX_RESPONSE_BYTES=-1 made the cap negative, so `received > cap` was true on the first byte and every sandbox response was rejected. The pre-existing SANDBOX_HTTP_SLACK_MS and SANDBOX_MAX_RETRIES parses had the same shape (a negative slack fires the budget timer immediately; a negative retry count skips the loop and returns "client error: undefined"). Route all three through a parseIntEnv helper that falls back on a non-finite or below-min value, mirroring the server's getMaxBodyBytes guard.
…oding The fidelity fix used Object.defineProperty for every object key on every encode, in both encoders, solely to preserve a rare own "__proto__" key. Only "__proto__" hits the prototype setter on a plain assignment, so restrict defineProperty to that key and use cheap assignment for all others. Preserves the round-trip fidelity (and the parity test still passes) without the per-key descriptor cost on the hot result-encoding path.
…tion The fail-fast wire-version header was X-Sandbox-Wire, a one-off namespace. The repo standardizes service-to-service headers under X-KH-* (X-KH-Caller, X-KH-Signature, X-KH-Key-Version, ...), so rename to X-KH-Sandbox-Wire to match that convention. Header semantics and the json-v1 value are unchanged.
Since the server now relays the child frame unrevived, the main-app client is the only place the result subtree is decoded. A forged frame with thousands of nested tags would recurse decodeSandboxNode into a RangeError in the main app. Thread a depth counter through decodeSandboxNode/decodeSandboxObject and throw a clear "nesting too deep" error past SANDBOX_MAX_DEPTH (256), well below the JS stack limit; legitimate results nest shallowly. The bare `.map(decodeSandboxNode)` call sites are wrapped so the array index is not mis-passed as the depth.
suisuss
added
the
metrics-db-reviewed
The standalone @keeperhub/sandbox package builds with its own tsc (separate from
the app's pnpm type-check), which flagged that the close handler reads
result.outcome.errorMessage while SandboxRunResult's relay:false arm was typed as
the full ChildOutcome union. Every synthetic (non-relay) result is an error, so
narrow that arm to the ok:false outcome (Extract<ChildOutcome, { ok: false }>).
…8deserialize-trusts-child-stdout-user-code-can fix(sandbox): decode the sandbox HTTP boundary with safe JSON, not v8
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.