Skip to content

ci(security): focused gate, static guards, and broader revocation reach (WS9)#693

Merged
buremba merged 3 commits into
mainfrom
sec/9-ci-guards
May 13, 2026
Merged

ci(security): focused gate, static guards, and broader revocation reach (WS9)#693
buremba merged 3 commits into
mainfrom
sec/9-ci-guards

Conversation

@buremba
Copy link
Copy Markdown
Member

@buremba buremba commented May 13, 2026

Final piece of the WS1–WS8 security sweep. Bundles three things into a single PR per spec:

PIECE A — security-tests.yml workflow

Dedicated GitHub Actions workflow that runs the security suites in isolation, so a regression on any of them surfaces under a clearly-named required check. Three jobs:

  • static-guardsscripts/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. Backed by a Postgres + pgvector service container.
  • vitest-security-tests — webhook-signatures (Slack HMAC contract) 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 guards

scripts/check-security-patterns.sh greps the tree for three banned patterns:

  1. window.confirm / window.alert / window.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 it with a strict attribute-ref validator.

One pre-existing hit in packages/server/src/tools/get_content.ts was annotated security-allowedscopedQuery is an internally-built SQL fragment and every appended WHERE clause uses $N placeholders.

No new ESLint rule — the existing biome lint setup doesn't gate this category, and a fresh linter would be a heavier lift than the grep guard.

PIECE C — WS5 revocation reach

verifySettingsSession already enforces jti revocation internally, so every async call site 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 on a revoked jti.
  • gateway/routes/internal/middleware.ts (authenticateWorker) — consults the singleton store and returns 401.
  • 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; same-process revokes hit the cache immediately, remote revokes propagate within CACHE_TTL_MS.

New test: revoked-token-store.test.tsauthenticateWorker (internal middleware) — revocation reach — pins that a revoked jti returns 401 from the internal middleware path that previously bypassed the store.

Drive-by cleanup so CI passes

These bugs were already breaking main (CI on a2db0473 is red); fixing them here unblocks the new security workflow:

Verification

  • make build-packages — clean.
  • bun run typecheck — clean.
  • bun run lint / bun run format:check — clean.
  • scripts/check-security-patterns.sh — exits 0.
  • All 182 listed security tests pass locally (bun test with ENCRYPTION_KEY set) plus 7 vitest webhook-signature tests.

@codecov-commenter
Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment on lines +38 to +50
name: Static security guards
runs-on: ubuntu-latest
timeout-minutes: 3
steps:
- uses: actions/checkout@v4
with:
# full history so `git grep` sees every tracked file
fetch-depth: 1

- name: Banned-pattern scan
run: ./scripts/check-security-patterns.sh

bun-security-tests:
Comment on lines +51 to +123
name: Security tests (bun:test)
runs-on: ubuntu-latest
timeout-minutes: 15
services:
postgres:
image: pgvector/pgvector:pg16
env:
POSTGRES_USER: postgres
POSTGRES_PASSWORD: postgres
POSTGRES_DB: lobu_test
ports:
- 5433:5432
options: >-
--health-cmd "pg_isready -U postgres"
--health-interval 5s
--health-timeout 5s
--health-retries 10
env:
DATABASE_URL: postgres://postgres:postgres@127.0.0.1:5433/lobu_test
# Deterministic key for tests that exercise encrypt/decrypt + jti.
ENCRYPTION_KEY: 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
steps:
- uses: actions/checkout@v4

- uses: ./.github/actions/setup-submodule
with:
deploy-key: ${{ secrets.OWLETTO_WEB_DEPLOY_KEY }}

- uses: oven-sh/setup-bun@v2
with:
bun-version: 1.3.5

- name: Install dependencies
run: bun install

- name: Build core (server tests import from @lobu/core dist)
run: |
cd packages/core && bun run build && cd ../..

- name: Verify Postgres + pgvector
run: |
for i in {1..20}; do
if pg_isready -h 127.0.0.1 -p 5433 -U postgres; then break; fi
sleep 1
done
PGPASSWORD=postgres psql -h 127.0.0.1 -p 5433 -U postgres -d lobu_test \
-c "CREATE EXTENSION IF NOT EXISTS vector"

- name: core security tests (encryption, worker-auth)
run: |
bun test \
packages/core/src/__tests__/encryption-key-validation.test.ts \
packages/core/src/__tests__/encryption.test.ts \
packages/core/src/__tests__/worker-auth.test.ts

- name: server unit security tests (nix, secret-proxy, egress, sandbox, ssrf)
run: |
bun test \
packages/server/src/__tests__/unit/nix-package-attr-ref.test.ts \
packages/server/src/__tests__/unit/secret-proxy-lifecycle.test.ts \
packages/server/src/__tests__/unit/egress-judge-timeout.test.ts \
packages/server/src/__tests__/unit/sandbox/manifest-enforcement.test.ts \
packages/server/src/__tests__/unit/http/proxy-ssrf-hardening.test.ts

- name: server gateway security tests (revocation, proxy, secret-proxy, egress)
run: |
bun test \
packages/server/src/gateway/__tests__/revoked-token-store.test.ts \
packages/server/src/gateway/__tests__/proxy-hardening.test.ts \
packages/server/src/gateway/__tests__/secret-proxy-harden.test.ts \
packages/server/src/gateway/__tests__/egress-judge-circuit-breaker.test.ts

vitest-security-tests:
Comment on lines +124 to +183
name: Security tests (vitest)
runs-on: ubuntu-latest
timeout-minutes: 15
services:
postgres:
image: pgvector/pgvector:pg16
env:
POSTGRES_USER: postgres
POSTGRES_PASSWORD: postgres
POSTGRES_DB: lobu_test
ports:
- 5433:5432
options: >-
--health-cmd "pg_isready -U postgres"
--health-interval 5s
--health-timeout 5s
--health-retries 10
env:
DATABASE_URL: postgres://postgres:postgres@127.0.0.1:5433/lobu_test
ENCRYPTION_KEY: 0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef
steps:
- uses: actions/checkout@v4

- uses: ./.github/actions/setup-submodule
with:
deploy-key: ${{ secrets.OWLETTO_WEB_DEPLOY_KEY }}

- uses: oven-sh/setup-bun@v2
with:
bun-version: 1.3.5

# vitest in this repo runs under Node (isolated-vm etc.) — pin Node 22
# so any native addon prebuild matches.
- uses: actions/setup-node@v4
with:
node-version: '22'

- name: Install dependencies
run: bun install

- name: Build core + connector-sdk
run: |
cd packages/core && bun run build && cd ../..
cd packages/connector-sdk && bun run build && cd ../..

- name: Verify Postgres + pgvector
run: |
for i in {1..20}; do
if pg_isready -h 127.0.0.1 -p 5433 -U postgres; then break; fi
sleep 1
done
PGPASSWORD=postgres psql -h 127.0.0.1 -p 5433 -U postgres -d lobu_test \
-c "CREATE EXTENSION IF NOT EXISTS vector"

- name: webhook-signatures (WS1)
working-directory: packages/server
run: |
node ../../node_modules/.bin/vitest run \
src/__tests__/webhook-signatures.test.ts \
--reporter=default
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e996ba4f7f

ℹ️ 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".

Comment on lines +59 to +60
git grep -nE '["`][^"`]*\bSELECT\b[^"`]*["`][[:space:]]*\+|\+[[:space:]]*["`][^"`]*\bWHERE\b' -- \
'packages/**/*.ts' \
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Fix static SQL guard before making it required

This new grep runs unconditionally in security-tests.yml, but it already fails on the current tree: I ran ./scripts/check-security-patterns.sh and it reports the existing execute-data-sources.ts SQL builders plus get_content.ts despite the nearby allowlist comment. That means every push/PR will get a red Static security guards check until the pre-existing patterns are either allowlisted in a way the filter actually recognizes or the scan is narrowed.

Useful? React with 👍 / 👎.

buremba added 3 commits May 13, 2026 15:19
…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).
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).
@buremba buremba force-pushed the sec/9-ci-guards branch from 907ad3d to 4a4ccb8 Compare May 13, 2026 14:19
@buremba buremba merged commit 5aa579d into main May 13, 2026
15 of 18 checks passed
@buremba buremba deleted the sec/9-ci-guards branch May 13, 2026 14:20
buremba added a commit that referenced this pull request May 13, 2026
PR #694 restored the helper, but the merge of #693 left an earlier copy
intact — esbuild errors out on `Multiple exports with the same name
"getClientIp"` in src/gateway/utils/rate-limiter.ts, breaking
`make build-packages` on main (typecheck + integration CI jobs cascade
from there). Drop the older un-documented copy; keep the JSDoc'd one.

No behavior change — the two implementations were functionally identical.
buremba added a commit that referenced this pull request May 13, 2026
PR #694 restored the helper, but the merge of #693 left an earlier copy
intact — esbuild errors out on `Multiple exports with the same name
"getClientIp"` in src/gateway/utils/rate-limiter.ts, breaking
`make build-packages` on main (typecheck + integration CI jobs cascade
from there). Drop the older un-documented copy; keep the JSDoc'd one.

No behavior change — the two implementations were functionally identical.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants