test(server): harden gateway, MCP, REST, orchestration, connections, auth, proxy#684
Merged
Conversation
45 deterministic bun:test tests covering missing/expired auth (401), cross-org agent and connection isolation (403/404), Slack OAuth callback with missing/expired/replayed state, stale-timestamp rejection, /lobu prefix routing, agentId input validation, and connection webhook 404 for unknown connections. Documents the unauthenticated /internal/connections endpoint as a known security gap.
…tests 56 new bun:test cases covering: watcher-run-race regression (classifyQueue never emits connector lanes), spawn-error/crash cleanup, stale worker reconciliation, maxDeployments concurrency limit, workspace dir creation + agentId validation, WORKER_ENV_* prefix-stripping, nix package injection prevention, killWorker double-exit safety, grant sync cache, deployment name determinism, and backoff correctness.
…ation Add 76 deterministic bun:test tests covering: - isSecretField heuristic (true/false for 17 field names) - isTelegramConfig / isSlackConfig narrowing type guards - InteractionService URL scheme guard (rejects javascript:, data:, file:) - InteractionService platform field on all 4 emitted event types - registerInteractionBridge: events with non-matching connectionId are ignored - registerInteractionBridge cleanup: all 4 event-type listeners removed - Duplicate event id idempotency (5-minute dedup window blocks second emit) - Two bridges on different connections don't cross-contaminate - Telegram polling-mode selection (auto/webhook/polling x publicGatewayUrl) - parseSlackTeamJoinEvent edge cases (bot, deleted, missing fields, bad JSON) - registerSlackPlatformHandlers DM vs group channel detection - Unique event ids per postStatusMessage / postLinkButton - beforeCreateHook ordering guarantee - postOauthLink delegates with linkType=oauth
New test files: - mcp-proxy-edge-cases.test.ts: SSRF guard (7 IPv4 private CIDRs + IPv6 loopback), cross-agent JWT isolation, tool-registry collision routing, wildcard grant coverage, onToolBlocked callback metadata, body-size 413, SSE-framed JSON-RPC parsing, in-memory session TTL/isolation, executeToolDirect happy+error paths, requiresToolApproval annotation logic - mcp-tool-call.test.ts: callMcpTool happy path, isError wrapping, 403/404/502 forwarding, empty-content fallback, fetch-throws, header correctness, askUserQuestion/uploadUserFile/getChannelHistory error propagation - tool-policy-edge-cases.test.ts: isDirectPackageInstallCommand (detected + non-detected + documented conservative over-detections), wildcard prefix patterns in buildToolPolicy, normalizeToolList coercion, deny-over-allow priority, case-insensitive allow/deny matching
New test files: - mcp-proxy-edge-cases.test.ts: SSRF guard (7 IPv4 private CIDRs + IPv6 loopback contract), cross-agent JWT isolation, tool-registry collision routing, wildcard grant (/mcp/id/tools/*), onToolBlocked metadata, body-size 413, SSE-framed JSON-RPC parsing, in-memory session TTL, executeToolDirect happy+error, requiresToolApproval annotation logic - mcp-tool-call.test.ts: callMcpTool happy path, isError wrapping, 403/404/502 forwarding, empty-content fallback, fetch-throws, header correctness, AskUser/UploadFile/GetChannelHistory error propagation - tool-policy-edge-cases.test.ts: isDirectPackageInstallCommand (detected + non-detected + conservative over-detections documented), wildcard prefix patterns, normalizeToolList coercion, deny-over-allow priority, case-insensitive matching
71 new deterministic bun tests across 6 describe blocks: - isBlockedIpAddress: private IPv4 ranges, IPv6 loopback/ULA/multicast, IPv4-mapped ::ffff: (dotted + hex forms), zone-ID stripping; documents NAT64 gap (64:ff9b::/96 not currently decoded) - Domain-blocking edge cases: wildcard boundary (evilexample.com vs .example.com), exact-vs-subdomain, case-insensitive, blocklist in unrestricted mode, IP-literal CONNECT (0.0.0.0, 127.0.0.1, 192.168.1.1), IPv6-bracket CONNECT returns 400 (bug documented), DNS rebinding mix - CRLF injection: verifies escapeHeaderValue() prevents \r\n in judge reason from creating extra HTTP header lines; documents that CRLF is collapsed to a space in the status-line reason - VerdictCache: method/path/CONNECT key independence, policyHash invalidation, extraPolicy change → hash change, cross-agent hash isolation - CircuitBreaker: success resets failure counter, re-trips after reset, per-policy isolation, unknown-hash default (closed), half-open exclusivity - PolicyStore.resolve: exact > wildcard, longer wildcard > shorter, agentId isolation, no-bundle, implicit 'default' judge, missing judge → undefined (fail closed), clear(), root-domain wildcard, case-insensitive, cross-agent policyHash independence - EgressJudge: default Haiku model, per-rule model override, circuit-open short-circuit, cache metadata on cached decision, single-failure source distinction (judge-error vs circuit-open)
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
This was referenced May 13, 2026
![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: b7306f6ca7
ℹ️ 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".
|
|
||
| test("wrong-key token → null (key rotation / leakage scenario)", () => { | ||
| // Encrypt with ALT_KEY, then switch back to TEST_KEY: AES-GCM tag fails. | ||
| process.env.ENCRYPTION_KEY = ALT_KEY; |
There was a problem hiding this comment.
When this wrong-key test runs after any earlier encrypt/decrypt call, @lobu/core has already memoized the first ENCRYPTION_KEY, so assigning process.env.ENCRYPTION_KEY = ALT_KEY here does not actually change the key used by encrypt(). The token is still produced with the cached test key and verifySettingsToken() will accept it instead of returning null, making the new suite order-dependent/CI-failing unless the test calls __resetEncryptionKeyCacheForTests() around key changes.
Useful? React with 👍 / 👎.
Comment on lines
+175
to
+176
| process.env.ENCRYPTION_KEY = | ||
| "fedcba9876543210fedcba9876543210fedcba9876543210fedcba9876543210"; |
There was a problem hiding this comment.
freshToken() encrypts with the current key and primes the memoized key in @lobu/core; changing process.env.ENCRYPTION_KEY afterward does not affect verifyWorkerToken() in this process. In this scenario the middleware still decrypts the token with the cached original key and returns 200, so the new key-rotation test fails unless the encryption cache is reset before/after mutating the env var.
Useful? React with 👍 / 👎.
Comment on lines
+267
to
+268
| * registered for that connection. The `platform` field on the event payload | ||
| * is metadata, not a routing key — isolation comes from `connectionId`. |
There was a problem hiding this comment.
These tests now document platform as mere metadata and assert that connectionId alone provides isolation, but the repo-level platform-isolation rule says every platform renderer must filter on the event's explicit platform field. With the behavior encoded this way, a same-connection event carrying the wrong platform would still be considered valid by the test suite instead of being rejected, so this should add/expect a same-connection platform-mismatch no-op rather than codifying connection-only routing.
Useful? React with 👍 / 👎.
buremba
added a commit
that referenced
this pull request
May 13, 2026
…test - bun-security-tests job: switch from cd-into-core to `make build-packages` so transitive @lobu/connector-sdk + agent-worker dist are present when the gateway tests import them. - packages/server/vitest.config.ts: exclude src/auth/__tests__/oauth-utils.test.ts (a bun:test unit file landed via #684 in a vitest-scanned directory; vitest can't resolve the bun:test specifier and the test doesn't need a DB anyway).
buremba
added a commit
that referenced
this pull request
May 13, 2026
…ch (WS9)
Final piece of the WS1–WS8 security sweep. Adds a dedicated CI job and
regression-prevention guards, plus extends the WS5 token-revocation check
to call sites that previously bypassed it.
PIECE A — security-tests workflow
- New .github/workflows/security-tests.yml runs the security suites in
isolation so a regression on them shows up under a clear name rather
than as collateral in the broader CI job. Three jobs:
- static-guards: runs scripts/check-security-patterns.sh
- bun-security-tests: encryption, worker-auth, nix-attr-ref,
secret-proxy lifecycle/harden, egress-judge timeout/circuit,
manifest-enforcement, proxy SSRF hardening, revoked-token-store,
proxy-hardening
- vitest-security-tests: webhook-signatures (Slack HMAC) under
Node + Postgres
Out of scope per repo memory (vitest CI gap): the ~30 stale vitest
integration files. Documented in the workflow header.
PIECE B — static regression guards
- scripts/check-security-patterns.sh greps the tree for:
1. window.confirm/alert/prompt outside node_modules (banned by
packages/web/DESIGN_GUIDELINES.md — inline confirms only).
2. SQL string-concatenation onto literal SELECT/WHERE fragments.
Findings can be suppressed with a same-line
`// security-allowed: <reason>` comment.
3. The loose Nix charset regex [A-Za-z0-9._-]+ re-introduced inside
packages/server/src/gateway/orchestration/. WS3 replaced this
with a strict attribute-ref validator.
- One pre-existing hit in packages/server/src/tools/get_content.ts was
annotated security-allowed (scopedQuery is an internally-built SQL
fragment, every WHERE clause uses $N placeholders).
PIECE C — WS5 revocation reach
verifySettingsSession already enforces jti revocation internally, so
every call site that awaits it inherits the check. For verifyWorkerToken,
several non-middleware call sites still bypassed the revoked-token store:
- gateway/auth/mcp/proxy.ts (authenticateRequest): now async, checks
isRevoked() before returning a session.
- gateway/auth/mcp/config-service.ts (buildWorkerMcpConfig): rejects a
revoked jti and returns an empty MCP config.
- gateway/gateway/index.ts (authenticateWorker): now async, denies the
SSE/response/session-context endpoints when the jti is revoked.
- gateway/routes/internal/middleware.ts (authenticateWorker): consults
the singleton store and returns 401 on a revoked jti.
- gateway/routes/public/agent.ts (requireAgentOwnership): checks jti
before authorizing the worker-token branch.
- gateway/proxy/http-proxy.ts (validateProxyAuth): uses a new sync
fast-path RevokedTokenStore.isRevokedCached(jti) — the CONNECT path
must stay sync, and same-process revokes hit the cache immediately.
New test: revoked-token-store.test.ts "authenticateWorker (internal
middleware) — revocation reach" — pins that a revoked jti returns 401
from the internal middleware path that was previously bypassed.
Drive-by cleanup so the build passes:
- restored getClientIp export in gateway/utils/rate-limiter.ts (removed
in #684, broke secret-proxy.ts imports on main).
- fixed pre-existing TS errors in packages/core/src/__tests__/
utils-json.test.ts and command-registry.test.ts introduced by #685.
- fixed pre-existing biome noEmptyBlockStatements in
command-registry.test.ts.
- updated the stale NAT64 gap test in proxy-hardening.test.ts — the
64:ff9b::/96 prefix is now decoded and blocked, so the assertion
flips from .toBe(false) to .toBe(true).
buremba
added a commit
that referenced
this pull request
May 13, 2026
…ch (WS9) (#693) * ci(security): focused gate, static guards, and broader revocation reach (WS9) Final piece of the WS1–WS8 security sweep. Adds a dedicated CI job and regression-prevention guards, plus extends the WS5 token-revocation check to call sites that previously bypassed it. PIECE A — security-tests workflow - New .github/workflows/security-tests.yml runs the security suites in isolation so a regression on them shows up under a clear name rather than as collateral in the broader CI job. Three jobs: - static-guards: runs scripts/check-security-patterns.sh - bun-security-tests: encryption, worker-auth, nix-attr-ref, secret-proxy lifecycle/harden, egress-judge timeout/circuit, manifest-enforcement, proxy SSRF hardening, revoked-token-store, proxy-hardening - vitest-security-tests: webhook-signatures (Slack HMAC) under Node + Postgres Out of scope per repo memory (vitest CI gap): the ~30 stale vitest integration files. Documented in the workflow header. PIECE B — static regression guards - scripts/check-security-patterns.sh greps the tree for: 1. window.confirm/alert/prompt outside node_modules (banned by packages/web/DESIGN_GUIDELINES.md — inline confirms only). 2. SQL string-concatenation onto literal SELECT/WHERE fragments. Findings can be suppressed with a same-line `// security-allowed: <reason>` comment. 3. The loose Nix charset regex [A-Za-z0-9._-]+ re-introduced inside packages/server/src/gateway/orchestration/. WS3 replaced this with a strict attribute-ref validator. - One pre-existing hit in packages/server/src/tools/get_content.ts was annotated security-allowed (scopedQuery is an internally-built SQL fragment, every WHERE clause uses $N placeholders). PIECE C — WS5 revocation reach verifySettingsSession already enforces jti revocation internally, so every call site that awaits it inherits the check. For verifyWorkerToken, several non-middleware call sites still bypassed the revoked-token store: - gateway/auth/mcp/proxy.ts (authenticateRequest): now async, checks isRevoked() before returning a session. - gateway/auth/mcp/config-service.ts (buildWorkerMcpConfig): rejects a revoked jti and returns an empty MCP config. - gateway/gateway/index.ts (authenticateWorker): now async, denies the SSE/response/session-context endpoints when the jti is revoked. - gateway/routes/internal/middleware.ts (authenticateWorker): consults the singleton store and returns 401 on a revoked jti. - gateway/routes/public/agent.ts (requireAgentOwnership): checks jti before authorizing the worker-token branch. - gateway/proxy/http-proxy.ts (validateProxyAuth): uses a new sync fast-path RevokedTokenStore.isRevokedCached(jti) — the CONNECT path must stay sync, and same-process revokes hit the cache immediately. New test: revoked-token-store.test.ts "authenticateWorker (internal middleware) — revocation reach" — pins that a revoked jti returns 401 from the internal middleware path that was previously bypassed. Drive-by cleanup so the build passes: - restored getClientIp export in gateway/utils/rate-limiter.ts (removed in #684, broke secret-proxy.ts imports on main). - fixed pre-existing TS errors in packages/core/src/__tests__/ utils-json.test.ts and command-registry.test.ts introduced by #685. - fixed pre-existing biome noEmptyBlockStatements in command-registry.test.ts. - updated the stale NAT64 gap test in proxy-hardening.test.ts — the 64:ff9b::/96 prefix is now decoded and blocked, so the assertion flips from .toBe(false) to .toBe(true). * ci(security): block-level allowlist comments + 3-line lookback in guard The static guard's grep-only filter required an inline 'security-allowed:' comment, which is unreadable on multi-line template-literal concatenations in execute-data-sources.ts. Switch to a 3-line lookback so the marker can live on a preceding comment line, and annotate the legitimate validated- table-name CTE constructions in execute-data-sources.ts that the guard flagged on its first CI run (these were called out as safe in WS2's audit: table identifiers come from QUERYABLE_TABLE_NAMES whitelist, orgP is a $N placeholder). * ci: bump to re-trigger workflows
This was referenced May 18, 2026
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
2 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.
Tests-only. Bundles 5 hardening passes from parallel teammates into one PR by layer.
Coverage added
test/harden-proxy-egress(cherry-pick)test/harden-mcptest/harden-rest-api/lobuprefixtest/harden-orchestrationtest/harden-connectionstest/harden-authNon-test changes
None — purely new
__tests__/*.test.tsfiles.Suspected real bugs (filed as issues, not fixed here)
GET /internal/connectionsrequires no auth — anonymous enumeration of every connection. (rest-api)allowwhen tool annotations can't be fetched (e.g. SSRF block). (mcp)64:ff9b::/96not decoded → SSRF reach into private space. (proxy)connectionIdfrom an InteractionService event. (connections)OpenClawProgressProcessor.processEventnull-derefs on malformedmessage_update. (worker)TOKEN_EXPIRATION_MSguard inapi-auth-middleware(real window is 2h). (auth)Brittle existing tests called out (recommend follow-up)
secret-proxy.test.ts— one assertion isexpect(true).toBe(true)(dead test).connections/__tests__/multi-tenant-integration.test.ts— allvitest .todo()stubs, never runs.mcp-proxy.test.ts"caches tools" / "retries on Server not initialized" — count exact fetch calls, brittle.agent-routes.test.ts— only covers happy path.config.test.ts— assertsgetAuthConfigreturns a Promise and nothing else.🤖 Generated with Claude Code