fix(orchestrator): block Nix-expression injection in nix-shell -p#682
Merged
Conversation
Skill-declared nixPackages were passed directly to `nix-shell -p <arg>`,
which evaluates each <arg> as a Nix expression. A skill could smuggle a
side-effecting expression — e.g. `pkgs.fetchurl; builtins.exec ...`,
`import ./evil.nix`, or `a && b` — past the previous charset-only
validator and run code at evaluation time, before the worker process
even started.
Replace the raw pass-through with strict validation: each entry must be
either a leaf identifier matching `^[a-z0-9][a-z0-9-]*$` or a
`<known-namespace>.<leaf>` attr path (only known per-language sets like
`python3Packages`, `nodePackages`, …; matches the shapes real skills
already use). Validated names are re-emitted as explicit `pkgs.<...>`
references inside a single `nix-shell -E` expression
(`let pkgs = import <nixpkgs> {}; in pkgs.mkShell { buildInputs = [ … ]; }`),
so nix-shell never parses caller-controlled text.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a03fd5ab50
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "luaPackages", | ||
| ]); | ||
|
|
||
| const NIX_LEAF_RE = /^[a-z0-9][a-z0-9-]*$/; |
There was a problem hiding this comment.
This stricter leaf regex rejects valid nixpkgs attributes that contain underscores, so any agent configured with such a package now fails before spawning with Invalid nix package name. This is not just theoretical: a repo-wide search shows the personal-finance example declares nix_packages = ["poppler_utils", "csvtk"] in examples/personal-finance/lobu.toml:26, and poppler_utils was accepted by the previous validator and is a valid nixpkgs attr. The injection fix can still keep the safe pkgs.<attr> re-emission while allowing _ in the leaf identifier.
Useful? React with 👍 / 👎.
NIX_LEAF_RE was /^[a-z0-9][a-z0-9-]*$/ which rejected valid nixpkgs attributes like `poppler_utils` (used in examples/personal-finance/lobu.toml). Underscore is a legal character in Nix attribute names and is safe here because the injection-guard metacharacter regex already blocks ; | & etc., and the attr is re-emitted as pkgs.<attr> — the raw string is never handed to nix. Updated regex: /^[a-z0-9_][a-z0-9_-]*$/ (mirrors NIX_ATTR_LEAF_RE which was already correct for namespace-qualified leaves). Added positive test cases: poppler_utils, csvtk, cairo_2. Added negative cases confirming semicolons are still blocked even when combined with underscore-containing names (pkgs;builtins.exec, foo_bar;builtins.exec). Fixes Codex P2 finding on PR #682 (sec/3-nix-injection).
buremba
added a commit
that referenced
this pull request
May 13, 2026
…y, token revocation, egress timeout, sandbox, encryption-key (#692) * fix(sandbox): re-enforce SDK manifest, org-slug guard, and allowCrossOrg on __sdk_dispatch host side (#680) A malicious guest script can call the __sdk_dispatch global directly with an un-manifested method, a poisoned orgPath ('__proto__'/'constructor'), or a cross-org path the manifest never advertised. Re-check all three on the host: - reject any path not in the manifest's dispatchable set, regardless of mode (dry-run only skips writes for modes that have it - it is not an auth gate) - reject org slugs that are __proto__/constructor/prototype, non-string, or when 'org' isn't an own property of the target SDK, before target.org(slug) - reject any non-empty orgPath when allowCrossOrg is false Adds packages/server/src/__tests__/unit/sandbox/manifest-enforcement.test.ts. * test(connections): pin webhook signature verification contract (#678) Inbound chat-platform webhooks (/slack/events, /api/v1/webhooks/:id) are authenticated by the Chat SDK adapter for each platform, not the HTTP route. Verified the adapters genuinely perform the documented scheme: - slack → HMAC-SHA256 over `v0:{ts}:{rawBody}` vs `x-slack-signature`, 401 on mismatch (@chat-adapter/slack `verifySignature`) - telegram → constant-time compare of `x-telegram-bot-api-secret-token` - discord → Ed25519 verify of `x-signature-ed25519`/`-timestamp` - whatsapp → HMAC-SHA256 over the body vs `x-hub-signature-256` - teams → Bot Framework bearer-JWT validation in `bridgeAdapter` Adds a regression test that drives the real Slack adapter — forged signature → 401, missing signature → 401, stale-but-signed replay → 401, correctly-signed url_verification challenge → 200 — plus route-level tests that the /slack/events edge keeps its freshness-window check and delegates fresh requests to ChatInstanceManager.handleSlackAppWebhook. Documents where verification happens in slack.ts and handleWebhook(). * fix(egress-judge): hard per-call timeout that fails closed and feeds the breaker (#677) The LLM egress judge was awaited synchronously by the HTTP proxy with no timeout independent of the model client's own transport timeout, so a hung judge call could stall an outbound request indefinitely. Add a per-call timeout (default 8s, configurable via EGRESS_JUDGE_TIMEOUT_MS or EgressJudgeOptions.judgeTimeoutMs) that, on expiry, fails the verdict closed (deny) and counts as a circuit-breaker failure — so a run of timeouts opens the breaker and subsequent calls short-circuit without invoking the model. Verdict cache untouched. * fix(orchestrator): block Nix-expression injection in nix-shell -p (#682) * fix(orchestrator): block Nix-expression injection in nix-shell -p Skill-declared nixPackages were passed directly to `nix-shell -p <arg>`, which evaluates each <arg> as a Nix expression. A skill could smuggle a side-effecting expression — e.g. `pkgs.fetchurl; builtins.exec ...`, `import ./evil.nix`, or `a && b` — past the previous charset-only validator and run code at evaluation time, before the worker process even started. Replace the raw pass-through with strict validation: each entry must be either a leaf identifier matching `^[a-z0-9][a-z0-9-]*$` or a `<known-namespace>.<leaf>` attr path (only known per-language sets like `python3Packages`, `nodePackages`, …; matches the shapes real skills already use). Validated names are re-emitted as explicit `pkgs.<...>` references inside a single `nix-shell -E` expression (`let pkgs = import <nixpkgs> {}; in pkgs.mkShell { buildInputs = [ … ]; }`), so nix-shell never parses caller-controlled text. * fix(orchestrator): allow underscores in Nix package leaf names NIX_LEAF_RE was /^[a-z0-9][a-z0-9-]*$/ which rejected valid nixpkgs attributes like `poppler_utils` (used in examples/personal-finance/lobu.toml). Underscore is a legal character in Nix attribute names and is safe here because the injection-guard metacharacter regex already blocks ; | & etc., and the attr is re-emitted as pkgs.<attr> — the raw string is never handed to nix. Updated regex: /^[a-z0-9_][a-z0-9_-]*$/ (mirrors NIX_ATTR_LEAF_RE which was already correct for namespace-qualified leaves). Added positive test cases: poppler_utils, csvtk, cairo_2. Added negative cases confirming semicolons are still blocked even when combined with underscore-containing names (pkgs;builtins.exec, foo_bar;builtins.exec). Fixes Codex P2 finding on PR #682 (sec/3-nix-injection). * fix(proxy): harden worker egress proxy against SSRF via IPv6 normalization (#681) * fix(proxy): harden worker egress proxy against SSRF via IPv6 normalization Route every resolved IP and CONNECT/forward target literal through a single normalizeIpLiteral() funnel before the blocklist check: handles IPv4-mapped IPv6 (dotted and hex), NAT64 (64:ff9b::/96), compressed forms, and strips zone IDs; normalized-to-IPv4 results are re-checked against the IPv4 blocklist, and anything that looks like an IP literal but won't parse fails closed. Validate the CONNECT port is an integer in 1..65535. Pin connections to the already-validated address (no re-resolve) and document that 3xx redirects re-enter the proxy and are independently re-validated. Adds a vitest regression suite. * fix(proxy): canonicalize IPv6 before NAT64 prefix check The previous startsWith("64:ff9b::") check only matched compressed IPv6 spellings, so a fully-expanded form such as `64:ff9b:0:0:0:0:a9fe:a9fe` (= 169.254.169.254 — AWS metadata endpoint) passed net.isIP() === 6, missed the NAT64 decode, and fell through to the bare IPv6 blocklist which does not know about embedded IPv4 addresses. Fix: add expandIpv6ToHextets() which normalises any valid IPv6 string into 8 unsigned 16-bit hextets (handling :: compression and dotted-quad suffixes per RFC 4291 §2.2). The NAT64 check now verifies hextets [0..5] === [0x0064, 0xff9b, 0, 0, 0, 0] then extracts the embedded IPv4 from hextets [6..7], so both compressed and expanded spellings produce the same decoded IPv4 and the existing private-IP blocklist catches it. Identified in Codex review of PR #681. Tests: adds three new cases covering compressed-form loopback (already blocked), expanded-form link-local (the regression — now blocked), and expanded-form public address (still allowed). * feat(auth): jti-based token revocation kill switch (WS5) (#683) * feat(auth): jti-based token revocation kill switch Worker tokens and settings session cookies are pure cryptographic material — a leaked token used to be valid until expiry with no way to invalidate it. Both token types now carry a random `jti` minted at issue time, and a new `revoked_tokens` Postgres table (mirroring `grant-store.ts`) maps revoked jtis to their original expiry. - `generateWorkerToken` mints `jti: randomUUID()`; `WorkerTokenData` gains `jti?: string`. - `setSettingsSessionCookie` mints `jti` when absent. A new local `SettingsSession` type widens the decoded payload so the field surfaces through verification without changing the shared payload interface. - New `RevokedTokenStore` (Postgres-backed, in-memory TTL cache, lazy GC sweep, lazy `CREATE TABLE IF NOT EXISTS`). - `createApiAuthMiddleware` calls `isRevoked(jti)` after both the settings-cookie path and the worker-token path; revoked tokens return 401. - New `lobu token revoke <jti>` CLI subcommand writes the row via `DATABASE_URL`. Effective within one cache TTL (~60s). - Vitest/bun regression test covers store revoke/isRevoked/sweep plus the middleware accept/reject paths. * fix(auth): enforce jti revocation inside verifySettingsSession, not just middleware Codex P1 finding (PR #683 sec/5-token-revocation): RevokedTokenStore.isRevoked(jti) was only checked inside createApiAuthMiddleware. Routes that call verifySettingsSession or verifySettingsSessionOrToken directly bypass that middleware and therefore accepted revoked cookies until token expiry. Previously-bypassed routes: - packages/server/src/gateway/routes/public/agent-history.ts:275 - packages/server/src/gateway/routes/public/agent-config.ts:354,375,409 - packages/server/src/gateway/routes/public/connections.ts:473,508 Fix: - verifySettingsSession, verifySettingsToken, and verifySettingsSessionOrToken are now async and perform the isRevoked check before returning the session payload. - A setRevokedTokenStore() injector (mirroring setAuthProvider()) allows tests to supply a custom store without touching the process-wide singleton. - The redundant post-check in createApiAuthMiddleware is removed; the middleware now delegates to the shared helper for settings session revocation. - All call sites updated to await the now-async functions. - Three new tests for verifySettingsSession: live jti passes, revoked jti returns null, injected store takes precedence over singleton. * fix(secret-proxy): 24h orphan TTL + per-source failed-resolution throttle (#679) Reduce the placeholder->secret mapping default TTL from ~7 days to 24h (configurable via SECRET_PLACEHOLDER_TTL_MS) so an orphaned mapping left behind by a worker crash / mid-day agent deletion stops being live within a day; teardown cascade and lazy GC are unchanged. Add an in-memory per-source (bound agentId, else client IP) cap on *failed* placeholder resolutions: after 20 misses in 5min the source hard-fails and we log the throttle once instead of per-attempt -- a successful lookup clears the bucket, so legitimate high-throughput valid traffic is never throttled. The cross-agent placeholder-binding 403 is untouched. * fix(core): harden ENCRYPTION_KEY parsing and make worker-token clock skew configurable (#676)
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
1 participant
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.
Problem
Worker spawn passes skill-declared
nixConfig.packagesstraight intonix-shell -p <arg>. The previous validator only enforced the charset[A-Za-z0-9._-]+, which still permits Nix attribute expressions (pkgs.fetchurl,import ./evil.nix,a && b, …).nix-shell -pevaluates each argument as a Nix expression, so a side-effecting expression runs at evaluation time — before the worker process even starts, and bypassing every downstream sandboxing layer (proxy, IPAddressDeny, capability drops).The malicious skill needs no special trust:
nixPackagesis declared in a skill'sSKILL.mdand merged into the agent's Nix env at spawn without a per-skill approval prompt (AGENTS.md§Integration auth).Fix
Stop passing skill strings into
nix-shell -pat all.^[a-z0-9][a-z0-9-]*$) or a<known-namespace>.<leaf>attr path. The namespace must be one of the known per-language package sets (python3Packages,nodePackages,perlPackages, …) — matches the shapes real skills already use (chromium,jq,ripgrep,pandoc,python3Packages.requests).; & |…) for a clearer error.pkgs.<...>attribute reference inside a singlenix-shell -Eexpression:let pkgs = import <nixpkgs> {}; in pkgs.mkShell { buildInputs = [ pkgs.ripgrep pkgs.python3Packages.requests ]; }nix-shell never sees caller-controlled text.
Test
packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts(bun:test, matches the surrounding unit-test convention):ripgrep,jq,python3,chromium,python3Packages.requests,nodePackages.typescript."pkgs.x; touch /tmp/pwn","a && b","import ./evil.nix","pkgs.fetchurl","builtins.exec", nestedpython3Packages.foo.bar, empty string, uppercase leaves.8 tests, all pass.
Validation
make build-packages— greenbun run typecheck— greenbun test packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts— 8 pass