fix(core): harden ENCRYPTION_KEY parsing + configurable worker-token clock skew#676
Merged
Merged
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…skew configurable
buremba
force-pushed
the
sec/8-encryption-key
branch
from
81f19c9 to
6a3d964
Compare
buremba
added a commit
that referenced
this pull request
May 13, 2026
Resolves conflict in packages/core/src/utils/encryption.ts: keep both the strict canonical-base64/hex round-trip validation (from #676) and the cached-key perf optimization (from main).
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.
WS8 of the hardening sweep.
Changes
packages/core/src/utils/encryption.ts:getEncryptionKey()now requires a canonical base64 (^[A-Za-z0-9+/]+={0,2}$, length % 4 === 0, decoded buffer re-encodes to the input) or hex (^[0-9a-fA-F]+$, even length, round-trips) string that decodes to exactly 32 bytes. PreviouslyBuffer.from(key, "base64")would silently drop invalid chars, so a typo'd key could yield a short/garbled key with no error. On-disk AES-GCM format unchanged.packages/core/src/worker/auth.ts: the 30s worker-token clock-skew tolerance is now overridable viaWORKER_TOKEN_CLOCK_SKEW_MS(default 30_000). Minimal, isolated change.packages/core/src/__tests__/encryption-key-validation.test.ts— junk key throws; base64 decoding to <32 bytes throws; non-canonical base64 throws; valid 32-byte base64 round-trips; valid 64-char hex (lower + upper) round-trips.Validation
make build-packages+bun run typecheckpass;bun testfor encryption, encryption-key-validation, worker-auth all green.