From dde1132a296c71f727160f17f4b0aba66a27b84b Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 2 Jul 2026 08:00:58 -0700 Subject: [PATCH] v0.42.55.0 fix(security): dotfile/skills/slug confinement + DCR consent default + schema-lint migration (#418 #419 #245 #1353 #1647 #171 #1385) (#2399) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(security): confine routing dotfiles, skills dir, slugs, and transcription exec Shared src/core/path-confine.ts consolidates the realpath-containment idiom (moved from sources-ops.ts) and adds isTrustedDotfile + isWriteTargetContained. - .gbrain-source (source-resolver) and .gbrain-mount (brain-resolver) walk-up dotfiles are now lstat trust-gated: a symlink, foreign-owned, or world-writable file is refused on multi-user hosts (#418), fail-closed on stat error. - resolveWorkspaceSkillsDir + every skills-dir tier (env, cwd_walk_up, repo_root, cwd_skills, install_path) route through realpath containment so a symlinked workspace/skills can't escape the declared workspace (#419). - resolveSourceId/resolveBrainId realpath both sides of the registered local_path / mount prefix match so a symlinked cwd can't misattribute source/brain. - validateSlug rejects NUL/control, bidi/RTL overrides, backslashes, and URL-encoded path separators at the shared putPage/updateSlug chokepoint; write-through confirms the file path stays within the source tree. - transcribeLargeFile uses execFileSync arg-arrays + fs.rmSync (no shell), so a path with shell metacharacters is never parsed by a shell (#245). Co-Authored-By: Claude Opus 4.8 (1M context) * fix(security): default dynamic-registration clients to authorization_code Self-registered DCR clients (the unauthenticated network registration path) previously defaulted to the client_credentials grant, which bypasses the /authorize consent screen. They now default to authorization_code; an explicit client_credentials request is rejected with invalid_client_metadata unless the operator opts in with the new --enable-dcr-insecure flag. A loud stderr WARNING prints at startup whenever DCR is enabled (#1353). Manual CLI/admin client registration is unchanged (operator-trusted). Co-Authored-By: Claude Opus 4.8 (1M context) * fix(security): schema-lint hardening migration (search_path + view security_invoker) Migration v120 brings existing brains to the same posture as fresh installs: - ALTER VIEW page_links SET (security_invoker = on) on Postgres so the view honors the caller's RLS instead of the owner's (the view-through-RLS bypass). - ALTER FUNCTION ... SET search_path on the gbrain-owned trigger/event functions (both engines, IF EXISTS so engine-only functions are skipped; body untouched, so the load-bearing auto_enable_rls event trigger is unchanged). Closes #171. - Broaden the BYPASSRLS preflight in the historical RLS migration gates to honor superuser and inherited-role BYPASSRLS, so a superuser-connected fresh install no longer aborts (#1385). Fresh-install function definitions in schema.sql / pglite-schema.ts are born-correct (regenerated schema-embedded.ts). scripts/check-search-path.sh is a new CI guard (wired into verify) that fails if a trigger function in the schema base files is added without SET search_path. Postgres-only assertions live in the bootstrap E2E; the PGLite path is covered by test/migration-v120.test.ts. Co-Authored-By: Claude Opus 4.8 (1M context) * v0.42.55.0 fix(security): dotfile/skills/slug confinement, DCR consent default, schema-lint migration Bump VERSION + package.json to 0.42.55.0 and add the CHANGELOG entry for the security-hardening wave (#418 #419 #245 #1353 #1647 #171 #1385). Co-Authored-By: Claude Opus 4.8 (1M context) * docs(security): note the DCR consent default in SECURITY.md (#1353) The "disable client_credentials, only allow authorization_code" guidance is now the built-in DCR default; document the new --enable-dcr-insecure escape hatch. Co-Authored-By: Claude Opus 4.8 (1M context) * docs(todos): add takes_search + code_def to the federated by-slug P1 (#2200) The v0.42.55.0 eng-review codex pass flagged takes_search (holder-allowlist only) and code_def (brain-wide raw SQL over content_chunks) as remaining same-class surfaces. Noted on the existing #2200 P1 follow-up, with the caveat that the #2399 close-list deliberately keeps #1371/#2200 open until this lands. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(security): correct plpgsql alias collision in #1385 BYPASSRLS gate (real-PG) The broadened BYPASSRLS preflight aliased `pg_roles r`, but several RLS DO-blocks already declare `r record` for their backfill FOR loop, so plpgsql resolved `r.oid`/`r.rolbypassrls` to the unassigned record variable → "record \"r\" is not assigned yet" on real Postgres (PGLite tolerated it; the DATABASE_URL-gated e2e jobs are the backstop). Renamed the subquery alias to `pr` at all 10 migrate.ts sites; also broadened the schema.sql base RLS gate the same way (with the `pr` alias) for #1385 consistency on superuser fresh installs, and regenerated schema-embedded.ts. Also fixes a PRE-EXISTING engine-parity bug (confirmed failing on clean origin/master): the relationalFanout shape compared `canonical_chunk_id`, a serial id that diverges between a fresh PGLite engine and a shared Postgres DB (setupDB TRUNCATEs without RESTART IDENTITY). Compare its presence, not the exact value. Validated on real Postgres (pgvector/pg16): migration v120 applies, the v35 RLS backfill runs, and engine-parity + postgres-bootstrap + jsonb-parity are green. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 20 ++ SECURITY.md | 12 + TODOS.md | 22 +- VERSION | 2 +- package.json | 3 +- scripts/check-search-path.sh | 41 ++++ scripts/run-verify-parallel.sh | 1 + src/cli.ts | 3 +- src/commands/serve-http.ts | 29 ++- src/commands/serve.ts | 9 +- src/core/brain-resolver.ts | 18 +- src/core/migrate.ts | 92 +++++++- src/core/oauth-provider.ts | 42 +++- src/core/path-confine.ts | 121 ++++++++++ src/core/pglite-schema.ts | 6 +- src/core/repo-root.ts | 30 ++- src/core/schema-embedded.ts | 14 +- src/core/source-resolver.ts | 27 ++- src/core/sources-ops.ts | 28 +-- src/core/transcription.ts | 38 ++-- src/core/utils.ts | 25 +++ src/core/write-through.ts | 17 +- src/schema.sql | 14 +- test/e2e/engine-parity.test.ts | 7 +- test/e2e/postgres-bootstrap.test.ts | 27 +++ test/migration-v120.test.ts | 48 ++++ test/oauth.test.ts | 44 +++- test/path-confine.test.ts | 319 +++++++++++++++++++++++++++ test/transcription-injection.test.ts | 65 ++++++ 29 files changed, 1018 insertions(+), 106 deletions(-) create mode 100755 scripts/check-search-path.sh create mode 100644 src/core/path-confine.ts create mode 100644 test/migration-v120.test.ts create mode 100644 test/path-confine.test.ts create mode 100644 test/transcription-injection.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 45dcd7e..ba53233 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,26 @@ All notable changes to GBrain will be documented in this file. +## [0.42.55.0] - 2026-06-24 + +**A security-hardening pass: routing dotfiles, the skills directory, page slugs, and large-file transcription are confined against multi-user-host and untrusted-input edge cases; dynamic OAuth client registration defaults to a consent-bearing grant; and a schema-lint migration brings existing brains to the fresh-install posture.** Several of these close community-reported gaps. Fresh installs were already covered; existing brains are brought to the same bar automatically on upgrade. If `gbrain doctor` flags anything afterward, its message names the object and the exact fix. + +### Fixed +- **Walk-up routing dotfiles are trust-gated.** On a shared multi-user host, a `.gbrain-source` / `.gbrain-mount` found by the ancestor-directory walk is accepted only when it's owned by you (or root) and is neither a symlink nor world-writable — otherwise it's skipped, fail-closed. The working-directory match that routes by registered path now resolves symlinks on both sides, so a redirected directory can't misattribute your source or brain. (#418, contributed by @garagon) +- **The skills directory is confined to its workspace.** Every skills-dir resolution tier now requires the resolved directory to stay within the declared workspace, so a redirected `skills` entry can't point the loader outside it. (#419, contributed by @garagon) +- **Page slugs reject unsafe characters at the write boundary.** The shared slug validator now rejects control bytes, bidirectional/RTL overrides, backslashes, and URL-encoded path separators on top of the existing traversal check, and the file-write path confirms the target stays inside the source's working tree. Ordinary slugs — including non-Latin and CJK — are unaffected. +- **Large-file transcription no longer builds shell command strings.** The segmentation path invokes `ffprobe`/`ffmpeg` with argument arrays and removes its temp directory through the filesystem API, so a media path is never parsed by a shell. (#245, contributed by @aliceagent) +- **Superuser-connected fresh installs no longer abort during migration.** The RLS preflight in the schema migrations recognizes superuser and inherited-role privileges, not just the role's own flag. (#1385) + +### Changed +- **Dynamic client registration defaults to the consent-bearing grant.** With Dynamic Client Registration enabled, a self-registered client now defaults to `authorization_code` (which goes through the approval screen) instead of `client_credentials`. Operators who need the machine-to-machine grant opt in with the new `--enable-dcr-insecure` flag, and a startup warning prints whenever registration is open. Registering clients via the CLI or admin API is unchanged. (#1353) + +### Added +- **Migration v120 — schema-lint hardening.** Brings existing brains to the fresh-install posture: the `page_links` view runs with the caller's privileges on Postgres, and the gbrain-owned trigger/event functions pin their schema search path on both engines. A new CI guard keeps new trigger functions from regressing. (#1647, #171) + +### To take advantage of v0.42.55.0 +`gbrain upgrade`, then `gbrain apply-migrations --yes` (or any command that opens the brain) to pick up migration v120 — no manual step, all on by default. Fresh installs already carry every change. The hardening applies automatically; `--enable-dcr-insecure` is the explicit escape hatch if you genuinely need the machine-to-machine OAuth grant. + ## [0.42.53.0] - 2026-06-23 **`gbrain sync` works again on managed Postgres brains: the durable-checkpoint pin write was encoding its value the wrong way, so every multi-source sync aborted at the very first checkpoint. Fixed, plus a repo-wide sweep of the same JSONB footgun and a new CI guard so it can't come back.** A recent release added a structural check on the sync checkpoint table; the pin write that runs before every drain bound its value as a string rather than a real array, so the check rejected it and the run bailed before importing anything. The bug was invisible on the embedded engine (its driver parses the value either way) and only bit managed Postgres. diff --git a/SECURITY.md b/SECURITY.md index 1099ccc..60def94 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -87,6 +87,18 @@ and the DCR `POST /register` path. Pre-v0.41.3 the CLI hard-coded operators to UPDATE `oauth_clients` rows by hand to make claude.ai work without `--enable-dcr`. That footgun is gone. +### DCR consent default (v0.42.55+) + +The "disable `client_credentials`, only allow `authorization_code`" guidance +above is now the built-in default for the DCR path, not just advice for custom +wrappers. With `--enable-dcr` on, a self-registered client defaults to the +`authorization_code` (browser-approval) grant, and an explicit +`client_credentials` request is rejected with `invalid_client_metadata`. +Operators who genuinely need the machine-to-machine grant on the registration +endpoint opt in with `--enable-dcr-insecure` (which implies `--enable-dcr`); a +startup WARNING prints whenever DCR is enabled, and a second when the insecure +grant is allowed. Pre-registering clients via the CLI / admin API is unchanged. + ### Token Management ```bash diff --git a/TODOS.md b/TODOS.md index 153805c..421c525 100644 --- a/TODOS.md +++ b/TODOS.md @@ -78,17 +78,23 @@ job) and sync. See CLAUDE.md "Pace Mode". `get_timeline` through the federated source scope and taught the engine methods to honor `sourceIds[]`. The adversarial review (Codex + Claude) flagged sibling read ops in the SAME class that still use scalar-only `ctx.sourceId ? {sourceId} : {}` and never thread - `ctx.auth.allowedSources`: `get_chunks`, `get_raw_data`, `get_versions`, and `resolve_slugs` - (the standalone op — `resolve_slugs` passes NO scope at all). A remote federated client - (grant set, dispatch-default `ctx.sourceId='default'`) reads these against `default` or - unscoped, not its grant. + `ctx.auth.allowedSources`: `get_chunks`, `get_raw_data`, `get_versions`, `resolve_slugs` + (the standalone op — `resolve_slugs` passes NO scope at all), plus (per the v0.42.55.0 + eng-review codex pass) `takes_search` (`operations.ts:1727` — holder-allowlist only, no + `sourceScopeOpts`) and `code_def` (`operations.ts:4155` — brain-wide raw SQL over + `content_chunks`; confirm whether brain-wide is intentional before scoping). A remote + federated client (grant set, dispatch-default `ctx.sourceId='default'`) reads these against + `default` or unscoped, not its grant. - **Why:** same cross-source correctness/isolation class #2200 targets; a federated client - can't read chunks/raw-data/versions for an authorized non-default source, and `resolve_slugs` - can fuzzy-resolve across all sources. + can't read chunks/raw-data/versions for an authorized non-default source, `resolve_slugs` + can fuzzy-resolve across all sources, and `takes_search`/`code_def` query without the grant. + The #2399 close-list deliberately did NOT blanket-close #1371/#2200 because of these residual + surfaces — close those issues only after this TODO lands. - **How to start:** mirror the #2200 pattern — route each handler through `sourceScopeOpts(ctx)` (or `linkReadScopeOpts` if a far endpoint exists), add `sourceIds?: string[]` to the engine - methods (`getChunks` / `getRawData` / `getVersions` / `resolveSlugs`) with `source_id = ANY($::text[])` - precedence, and add federated/isolation tests + engine-parity arms. + methods (`getChunks` / `getRawData` / `getVersions` / `resolveSlugs` / the takes-search + + code-def queries) with `source_id = ANY($::text[])` precedence, and add federated/isolation + tests + engine-parity arms. - **Depends on:** nothing; #2200 established the pattern and the `linkReadScopeOpts` helper. ## Spend-controls wave follow-ups (filed v0.42.45.0, #2139) diff --git a/VERSION b/VERSION index 161d361..fa271bf 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.42.53.0 +0.42.55.0 diff --git a/package.json b/package.json index e59e46a..31c0c88 100644 --- a/package.json +++ b/package.json @@ -65,6 +65,7 @@ "ci:select-e2e": "bun run scripts/select-e2e.ts", "typecheck": "tsc --noEmit", "check:jsonb": "scripts/check-jsonb-pattern.sh", + "check:search-path": "scripts/check-search-path.sh", "check:no-double-retry": "scripts/check-no-double-retry.sh", "check:batch-audit-site": "scripts/check-batch-audit-site.sh", "check:worker-lock-renewal-shape": "scripts/check-worker-lock-renewal-shape.sh", @@ -143,5 +144,5 @@ "bun": ">=1.3.10" }, "license": "MIT", - "version": "0.42.53.0" + "version": "0.42.55.0" } diff --git a/scripts/check-search-path.sh b/scripts/check-search-path.sh new file mode 100755 index 0000000..8158d94 --- /dev/null +++ b/scripts/check-search-path.sh @@ -0,0 +1,41 @@ +#!/usr/bin/env bash +# CI guard (#1647 / #171): every trigger function in the canonical schema base +# files MUST pin `SET search_path`. Without it, an unqualified reference inside +# the function body resolves through the caller's search_path, so a same-named +# object in a user-controlled schema could shadow it. Migration v120 ALTERs +# existing brains; this guard keeps fresh-install function definitions correct +# so a NEW trigger function can't reintroduce the gap. Mirrors the +# check-jsonb-pattern.sh guard philosophy (a written rule caused the disease; +# a guard cures it). +# +# Scope: schema base files only (src/schema.sql, src/core/pglite-schema.ts). +# Historical migration bodies in migrate.ts are append-only and not rescanned; +# the runtime doctor probe (pg_proc.proconfig) covers the live post-migration +# state on real brains. +# +# Usage: scripts/check-search-path.sh +# Exit: 0 when all trigger functions pin search_path, 1 otherwise. + +set -euo pipefail + +ROOT="$(git rev-parse --show-toplevel 2>/dev/null || pwd)" +cd "$ROOT" + +FILES="src/schema.sql src/core/pglite-schema.ts src/core/schema-embedded.ts" + +# A hardened header reads `... RETURNS trigger SET search_path = ... AS $tag$`. +# An UNHARDENED one reads `... RETURNS trigger AS $tag$` — match that form and +# (belt-and-suspenders) drop any line that already mentions search_path. +BAD="$(grep -nEi 'CREATE OR REPLACE FUNCTION [a-z_]+\(\) RETURNS trigger AS ' $FILES 2>/dev/null | grep -vi 'search_path' || true)" + +if [ -n "$BAD" ]; then + echo "ERROR: trigger function(s) missing SET search_path in schema base files:" + echo "$BAD" + echo + echo "Add 'SET search_path = pg_catalog, public' to the function header, e.g.:" + echo " CREATE OR REPLACE FUNCTION foo() RETURNS trigger SET search_path = pg_catalog, public AS \$\$" + echo "See #1647 / #171." + exit 1 +fi + +echo "OK: all trigger functions in schema base files pin search_path" diff --git a/scripts/run-verify-parallel.sh b/scripts/run-verify-parallel.sh index 5aed8b9..f03c560 100755 --- a/scripts/run-verify-parallel.sh +++ b/scripts/run-verify-parallel.sh @@ -38,6 +38,7 @@ CHECKS=( "check:proposal-pii" "check:test-names" "check:jsonb" + "check:search-path" "check:source-id-projection" "check:source-config-leak" "check:progress" diff --git a/src/cli.ts b/src/cli.ts index e1cb8db..14221ab 100755 --- a/src/cli.ts +++ b/src/cli.ts @@ -2325,7 +2325,8 @@ ADMIN serve MCP server (stdio) serve --http [--port N] HTTP MCP server with OAuth 2.1 --token-ttl N Access token TTL in seconds (default: 3600) - --enable-dcr Enable Dynamic Client Registration + --enable-dcr Enable Dynamic Client Registration (DCR clients default to authorization_code) + --enable-dcr-insecure Also allow the consent-bypassing client_credentials grant on DCR (implies --enable-dcr) --public-url URL Public issuer URL (required behind proxy/tunnel) connect --token Wire Claude Code to a remote gbrain (bearer token) [--install] [--json] Print the paste-ready command, or --install to run it diff --git a/src/commands/serve-http.ts b/src/commands/serve-http.ts index e700397..5a05394 100644 --- a/src/commands/serve-http.ts +++ b/src/commands/serve-http.ts @@ -259,6 +259,11 @@ interface ServeHttpOptions { port: number; tokenTtl: number; enableDcr: boolean; + /** + * #1353: allow the consent-bypassing client_credentials grant on the DCR path. + * Off by default; DCR clients default to authorization_code. Implies enableDcr. + */ + enableDcrInsecure?: boolean; /** * Public URL the server is reachable at (e.g., https://brain.example.com). * Used as the OAuth issuer in discovery metadata. Defaults to @@ -396,7 +401,7 @@ export function skillPublishStatus(publishSkills: boolean): { bannerValue: strin } export async function runServeHttp(engine: BrainEngine, options: ServeHttpOptions) { - const { port, tokenTtl, enableDcr, publicUrl, logFullParams } = options; + const { port, tokenTtl, enableDcr, enableDcrInsecure, publicUrl, logFullParams } = options; // v0.34.1 (#864, D11): default bind flipped from 0.0.0.0 to 127.0.0.1. // gbrain's primary use case is a personal-knowledge brain on a laptop; // the pre-v0.34 default exposed brains on every interface. Server @@ -479,8 +484,28 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption sql, tokenTtl, dcrDisabled: !enableDcr, + allowClientCredentialsDcr: enableDcrInsecure === true, }); + // #1353: loud stderr security WARN when DCR is enabled. DCR is an + // unauthenticated network registration endpoint; surface the posture change + // (and the extra blast radius of --enable-dcr-insecure) so it's visible in + // logs, not buried in the neutral "DCR: enabled" banner line. + if (enableDcr) { + console.error( + 'SECURITY WARNING: Dynamic Client Registration (--enable-dcr) is ON. ' + + 'Any network caller can self-register an OAuth client. DCR clients default ' + + 'to the authorization_code (consent-bearing) grant. See SECURITY.md.', + ); + if (enableDcrInsecure) { + console.error( + 'SECURITY WARNING: --enable-dcr-insecure is ON — self-registered DCR ' + + 'clients may request the client_credentials grant, which BYPASSES the ' + + '/authorize consent screen. Only use this on a trusted network.', + ); + } + } + // Sweep expired tokens on startup (non-blocking) try { const swept = await oauthProvider.sweepExpiredTokens(); @@ -2133,7 +2158,7 @@ export async function runServeHttp(engine: BrainEngine, options: ServeHttpOption ║ Engine: ${(config.engine || 'pglite').padEnd(40)}║ ║ Issuer: ${issuerUrl.origin.padEnd(40)}║ ║ Clients: ${String((clientCount[0] as any).count).padEnd(40)}║ -║ DCR: ${(enableDcr ? 'enabled' : 'disabled').padEnd(40)}║ +║ DCR: ${(enableDcr ? (enableDcrInsecure ? 'enabled (INSECURE: client_credentials)' : 'enabled') : 'disabled').padEnd(40)}║ ║ Skills: ${skillStatus.bannerValue.padEnd(40)}║ ║ Token TTL: ${(tokenTtl + 's').padEnd(40)}║ ╠══════════════════════════════════════════════════════╣ diff --git a/src/commands/serve.ts b/src/commands/serve.ts index 058c859..de7352d 100644 --- a/src/commands/serve.ts +++ b/src/commands/serve.ts @@ -85,7 +85,12 @@ export async function runServe( const ttlIdx = args.indexOf('--token-ttl'); const tokenTtl = ttlIdx >= 0 ? parseInt(args[ttlIdx + 1]) || 3600 : 3600; - const enableDcr = args.includes('--enable-dcr'); + // #1353: --enable-dcr-insecure opts into the consent-bypassing + // client_credentials grant on the DCR path. It implies --enable-dcr (you + // can't allow insecure DCR clients without DCR). Plain --enable-dcr keeps + // the secure default: DCR clients are authorization_code (consent-bearing). + const enableDcrInsecure = args.includes('--enable-dcr-insecure'); + const enableDcr = args.includes('--enable-dcr') || enableDcrInsecure; const publicUrlIdx = args.indexOf('--public-url'); const publicUrl = publicUrlIdx >= 0 ? args[publicUrlIdx + 1] : undefined; @@ -114,7 +119,7 @@ export async function runServe( const suppressBootstrapToken = args.includes('--suppress-bootstrap-token'); const { runServeHttp } = await import('./serve-http.ts'); - await runServeHttp(engine, { port, tokenTtl, enableDcr, publicUrl, logFullParams, bind, suppressBootstrapToken }); + await runServeHttp(engine, { port, tokenTtl, enableDcr, enableDcrInsecure, publicUrl, logFullParams, bind, suppressBootstrapToken }); return; } diff --git a/src/core/brain-resolver.ts b/src/core/brain-resolver.ts index 906b046..ac7ff28 100644 --- a/src/core/brain-resolver.ts +++ b/src/core/brain-resolver.ts @@ -19,9 +19,10 @@ * parent's brainId instead of re-running this resolver. */ -import { readFileSync, existsSync } from 'fs'; +import { readFileSync, lstatSync, type Stats } from 'fs'; import { join, dirname, resolve } from 'path'; import { HOST_BRAIN_ID, loadMounts, validateMountId, type MountEntry } from './brain-registry.ts'; +import { isTrustedDotfile, realpathOrResolve } from './path-confine.ts'; const DOTFILE = '.gbrain-mount'; /** Same regex as brain-registry. Kept in sync. */ @@ -36,7 +37,13 @@ function readDotfileWalk(startDir: string): string | null { let dir = resolve(startDir); for (let i = 0; i < 50; i++) { const candidate = join(dir, DOTFILE); - if (existsSync(candidate)) { + // lstatSync (NOT statSync) + isTrustedDotfile: refuse a symlink, foreign- + // owned, or world-writable `.gbrain-mount` planted by another user in a + // shared ancestor dir (same multi-user-host hijack as #418, applied to the + // brain axis). Any stat error → skip and keep walking (fail-closed). + let st: Stats | null = null; + try { st = lstatSync(candidate); } catch { st = null; } + if (st && isTrustedDotfile(st)) { try { const content = readFileSync(candidate, 'utf8').trim().split('\n')[0].trim(); if (content === HOST_BRAIN_ID) return content; @@ -54,11 +61,14 @@ function readDotfileWalk(startDir: string): string | null { /** Longest-prefix match: find the mount whose `path` contains `cwd`. */ function longestPathPrefixMount(mounts: MountEntry[], cwd: string): MountEntry | null { - const cwdResolved = resolve(cwd); + // realpath both sides so a symlinked CWD can't forge a prefix match against a + // mount path it doesn't really live under (codex #9, brain-axis mirror of the + // source-resolver fix). Falls back to lexical resolve() for a stale mount path. + const cwdResolved = realpathOrResolve(cwd); let best: { mount: MountEntry; pathLen: number } | null = null; for (const m of mounts) { if (m.enabled === false) continue; - const p = resolve(m.path); + const p = realpathOrResolve(m.path); if (cwdResolved === p || cwdResolved.startsWith(p + '/')) { if (!best || p.length > best.pathLen) { best = { mount: m, pathLen: p.length }; diff --git a/src/core/migrate.ts b/src/core/migrate.ts index acc57ae..61ee2da 100644 --- a/src/core/migrate.ts +++ b/src/core/migrate.ts @@ -862,7 +862,7 @@ export const MIGRATIONS: Migration[] = [ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; -- #1385: superuser + inherited-role BYPASSRLS, not just the role's own rolbypassrls IF NOT has_bypass THEN -- Fail the migration loudly instead of WARNING + version-bump. -- The runner unconditionally records schema_version on success, @@ -1151,7 +1151,7 @@ export const MIGRATIONS: Migration[] = [ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; -- #1385: superuser + inherited-role BYPASSRLS, not just the role's own rolbypassrls IF NOT has_bypass THEN RAISE EXCEPTION 'v29 cathedral_ii_code_edges_rls: role % does not have BYPASSRLS privilege — cannot enable RLS safely. Re-run as postgres (or another BYPASSRLS role). The migration will retry automatically on the next initSchema call.', current_user; END IF; @@ -1239,7 +1239,7 @@ export const MIGRATIONS: Migration[] = [ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; -- #1385: superuser + inherited-role BYPASSRLS, not just the role's own rolbypassrls IF has_bypass THEN ALTER TABLE takes ENABLE ROW LEVEL SECURITY; ALTER TABLE synthesis_evidence ENABLE ROW LEVEL SECURITY; @@ -1341,7 +1341,7 @@ export const MIGRATIONS: Migration[] = [ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; -- #1385: superuser + inherited-role BYPASSRLS, not just the role's own rolbypassrls IF has_bypass THEN ALTER TABLE dream_verdicts ENABLE ROW LEVEL SECURITY; END IF; @@ -1380,7 +1380,7 @@ export const MIGRATIONS: Migration[] = [ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; -- #1385: superuser + inherited-role BYPASSRLS, not just the role's own rolbypassrls IF NOT has_bypass THEN RAISE EXCEPTION 'v31 eval_capture_tables: role % does not have BYPASSRLS privilege — cannot enable RLS safely. Re-run as postgres (or another BYPASSRLS role). The migration will retry automatically on the next initSchema call.', current_user; END IF; @@ -1499,7 +1499,7 @@ export const MIGRATIONS: Migration[] = [ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; -- #1385: superuser + inherited-role BYPASSRLS, not just the role's own rolbypassrls IF has_bypass THEN ALTER TABLE oauth_clients ENABLE ROW LEVEL SECURITY; ALTER TABLE oauth_tokens ENABLE ROW LEVEL SECURITY; @@ -1720,7 +1720,7 @@ export const MIGRATIONS: Migration[] = [ has_bypass BOOLEAN; r record; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; -- #1385: superuser + inherited-role BYPASSRLS, not just the role's own rolbypassrls IF NOT has_bypass THEN -- Same posture as v24: raise to abort the migration so the runner -- leaves config.version unbumped and retries on the next call. @@ -2112,7 +2112,7 @@ export const MIGRATIONS: Migration[] = [ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; -- #1385: superuser + inherited-role BYPASSRLS, not just the role's own rolbypassrls IF has_bypass THEN ALTER TABLE drift_decisions ENABLE ROW LEVEL SECURITY; END IF; @@ -2365,7 +2365,7 @@ export const MIGRATIONS: Migration[] = [ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; -- #1385: superuser + inherited-role BYPASSRLS, not just the role's own rolbypassrls IF has_bypass THEN ALTER TABLE facts ENABLE ROW LEVEL SECURITY; END IF; @@ -4389,7 +4389,7 @@ export const MIGRATIONS: Migration[] = [ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; -- #1385: superuser + inherited-role BYPASSRLS, not just the role's own rolbypassrls IF has_bypass THEN ALTER TABLE take_domain_assignments ENABLE ROW LEVEL SECURITY; END IF; @@ -5367,6 +5367,78 @@ export const MIGRATIONS: Migration[] = [ END $$; `, }, + { + version: 120, + name: 'schema_lint_hardening_search_path_security_invoker', + // v0.42 schema-lint hardening wave (#1647 / #171). + // + // (b) security_invoker on the page_links view: pre-fix the view ran with + // the definer/owner's privileges, so the anon / PostgREST role could + // read `links` (which has RLS) THROUGH the view, bypassing RLS. This + // is the single ERROR-severity Supabase lint. Postgres-only — PGLite + // is embedded/single-user with no anon role and no PostgREST, so the + // view has no RLS-bypass surface there (and security_invoker carries + // no benefit). Guarded with IF EXISTS for very old brains. + // + // (a)/(#171) search_path on every gbrain-owned trigger/event function: + // an unqualified reference (e.g. `FROM timeline_entries`) resolves + // through the caller's search_path, so a same-named object in a + // user-controlled schema could shadow it. Pinning search_path closes + // that. ALTER FUNCTION (NOT CREATE OR REPLACE) leaves each body + // untouched — lowest drift risk, and critically safe for the + // load-bearing `auto_enable_rls` event-trigger function (codex #3). + // The IF EXISTS loop is engine-agnostic and skips functions a given + // brain never created (e.g. auto_enable_rls + the NOTIFY/chunk + // trigger functions are Postgres-only — codex #4). + // + // Regression guard is a doctor probe (pg_proc.proconfig) + scripts/ + // check-search-path.sh, NOT a migration verify-hook — hooks don't run on + // brains already stamped past this version (learning: migration-verify-hook- + // never-runs-on-stamped-brains). Fresh installs are born correct: the + // function defs in schema.sql / pglite-schema.ts carry SET search_path too. + idempotent: true, + sql: '', // engine-specific via sqlFor + sqlFor: { + postgres: ` + ALTER VIEW IF EXISTS page_links SET (security_invoker = on); + + DO $$ + DECLARE fn text; + BEGIN + FOREACH fn IN ARRAY ARRAY[ + 'bump_page_generation_fn','bump_page_generation_clock_fn', + 'update_chunk_search_vector','update_page_search_vector', + 'notify_minion_job_change','auto_enable_rls' + ] LOOP + IF EXISTS ( + SELECT 1 FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' AND p.proname = fn + ) THEN + EXECUTE format('ALTER FUNCTION public.%I() SET search_path = pg_catalog, public', fn); + END IF; + END LOOP; + END $$; + `, + pglite: ` + DO $$ + DECLARE fn text; + BEGIN + FOREACH fn IN ARRAY ARRAY[ + 'bump_page_generation_fn','bump_page_generation_clock_fn', + 'update_chunk_search_vector','update_page_search_vector', + 'notify_minion_job_change' + ] LOOP + IF EXISTS ( + SELECT 1 FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' AND p.proname = fn + ) THEN + EXECUTE format('ALTER FUNCTION public.%I() SET search_path = pg_catalog, public', fn); + END IF; + END LOOP; + END $$; + `, + }, + }, ]; export const LATEST_VERSION = MIGRATIONS.length > 0 diff --git a/src/core/oauth-provider.ts b/src/core/oauth-provider.ts index 8335853..52845ea 100644 --- a/src/core/oauth-provider.ts +++ b/src/core/oauth-provider.ts @@ -22,7 +22,7 @@ import type { import type { OAuthServerProvider, AuthorizationParams } from '@modelcontextprotocol/sdk/server/auth/provider.js'; import type { OAuthRegisteredClientsStore } from '@modelcontextprotocol/sdk/server/auth/clients.js'; import type { AuthInfo as SdkAuthInfo } from '@modelcontextprotocol/sdk/server/auth/types.js'; -import { InvalidTokenError } from '@modelcontextprotocol/sdk/server/auth/errors.js'; +import { InvalidTokenError, InvalidClientMetadataError } from '@modelcontextprotocol/sdk/server/auth/errors.js'; import { hashToken, generateToken, isUndefinedColumnError } from './utils.ts'; import { hasScope, assertAllowedScopes, parseScopeString, InvalidScopeError } from './scope.ts'; import type { AuthInfo as CoreAuthInfo } from './operations.ts'; @@ -183,6 +183,16 @@ interface GBrainOAuthProviderOptions { * before mcpAuthRouter ran). */ dcrDisabled?: boolean; + /** + * Allow the consent-bypassing `client_credentials` grant on the unauthenticated + * Dynamic Client Registration path. Default false (#1353): a self-registered + * DCR client defaults to `authorization_code` (which goes through /authorize + * consent), and an explicit `client_credentials` request is rejected. Operators + * who genuinely need machine-to-machine DCR clients opt in via + * `--enable-dcr-insecure`. Manual CLI / admin registration is unaffected + * (operator-trusted, registers grants directly). + */ + allowClientCredentialsDcr?: boolean; } // --------------------------------------------------------------------------- @@ -190,7 +200,7 @@ interface GBrainOAuthProviderOptions { // --------------------------------------------------------------------------- class GBrainClientsStore implements OAuthRegisteredClientsStore { - constructor(private sql: SqlQuery) {} + constructor(private sql: SqlQuery, private allowClientCredentialsDcr = false) {} async getClient(clientId: string): Promise { const rows = await this.sql` @@ -243,6 +253,24 @@ class GBrainClientsStore implements OAuthRegisteredClientsStore { // registration entry points share one allow-list. const authMethod = validateTokenEndpointAuthMethod(client.token_endpoint_auth_method); + // v0.42 (#1353): the DCR path is the unauthenticated network entry point. + // `client_credentials` skips /authorize consent entirely, so a self- + // registered DCR client must NOT get it by default. Default the grant to + // `authorization_code` (the consent-bearing flow) when unspecified, and + // reject an explicit `client_credentials` request unless the operator opted + // in via `--enable-dcr-insecure`. Manual CLI/admin registration bypasses + // this store method, so operators can still mint machine clients directly. + const grantTypes = (client.grant_types && client.grant_types.length > 0) + ? client.grant_types + : ['authorization_code']; + if (!this.allowClientCredentialsDcr && grantTypes.includes('client_credentials')) { + throw new InvalidClientMetadataError( + 'client_credentials grant is not permitted via dynamic client registration; ' + + 'restart the server with --enable-dcr-insecure to allow it, or register the ' + + 'client via the gbrain CLI / admin API.', + ); + } + const clientId = generateToken('gbrain_cl_'); // v0.34.1 (#909): RFC 7591 §2 — clients that authenticate at the token // endpoint via PKCE alone declare `token_endpoint_auth_method: "none"`. @@ -273,7 +301,7 @@ class GBrainClientsStore implements OAuthRegisteredClientsStore { client_id_issued_at, source_id, federated_read) VALUES (${clientId}, ${secretHash}, ${client.client_name || 'unnamed'}, ${pgArray((client.redirect_uris || []).map(String))}, - ${pgArray(client.grant_types || ['client_credentials'])}, + ${pgArray(grantTypes)}, ${client.scope || ''}, ${authMethod}, ${now}, ${'default'}, ${pgArray(['default'])}) `; @@ -286,7 +314,7 @@ class GBrainClientsStore implements OAuthRegisteredClientsStore { client_id_issued_at, source_id) VALUES (${clientId}, ${secretHash}, ${client.client_name || 'unnamed'}, ${pgArray((client.redirect_uris || []).map(String))}, - ${pgArray(client.grant_types || ['client_credentials'])}, + ${pgArray(grantTypes)}, ${client.scope || ''}, ${authMethod}, ${now}, ${'default'}) `; @@ -298,7 +326,7 @@ class GBrainClientsStore implements OAuthRegisteredClientsStore { client_id_issued_at) VALUES (${clientId}, ${secretHash}, ${client.client_name || 'unnamed'}, ${pgArray((client.redirect_uris || []).map(String))}, - ${pgArray(client.grant_types || ['client_credentials'])}, + ${pgArray(grantTypes)}, ${client.scope || ''}, ${authMethod}, ${now}) `; @@ -313,7 +341,7 @@ class GBrainClientsStore implements OAuthRegisteredClientsStore { client_id_issued_at) VALUES (${clientId}, ${secretHash}, ${client.client_name || 'unnamed'}, ${pgArray((client.redirect_uris || []).map(String))}, - ${pgArray(client.grant_types || ['client_credentials'])}, + ${pgArray(grantTypes)}, ${client.scope || ''}, ${authMethod}, ${now}) `; @@ -350,7 +378,7 @@ export class GBrainOAuthProvider implements OAuthServerProvider { constructor(options: GBrainOAuthProviderOptions) { this.sql = options.sql; - this._clientsStore = new GBrainClientsStore(this.sql); + this._clientsStore = new GBrainClientsStore(this.sql, options.allowClientCredentialsDcr === true); this.dcrDisabled = options.dcrDisabled === true; this.tokenTtl = options.tokenTtl || 3600; this.refreshTtl = options.refreshTtl || 30 * 24 * 3600; diff --git a/src/core/path-confine.ts b/src/core/path-confine.ts new file mode 100644 index 0000000..9f7eced --- /dev/null +++ b/src/core/path-confine.ts @@ -0,0 +1,121 @@ +/** + * Shared symlink-safe path-confinement + dotfile-trust helpers. + * + * Consolidates the realpath-containment idiom that previously lived only in + * `sources-ops.ts` (`isPathContained`) and `validateUploadPath` + * (`operations.ts`), and adds `isTrustedDotfile` — the multi-user-host trust + * gate for walk-up routing dotfiles (`.gbrain-source` / `.gbrain-mount`). + * + * Threat model (POSIX multi-user host): an attacker who can write into a + * shared ancestor directory of the victim's CWD (`/tmp`, `/var/tmp`, + * `/dev/shm`, shared NFS/SMB, CI runner volumes, container bind-mounts) can + * plant a routing dotfile that silently retargets the victim's reads/writes + * to the attacker's source/brain. The walk-up resolvers must therefore refuse + * a dotfile they can't prove the victim (or root) owns. (#418/#419) + * + * Fail-closed: any stat/realpath error → not trusted / not contained. The one + * documented exception is platforms without numeric uid (Windows), where the + * multi-user-POSIX threat model does not apply and `isTrustedDotfile` trusts + * by default so existing single-user setups keep working. + */ + +import { realpathSync, existsSync, type Stats } from 'fs'; +import { resolve as resolvePath, relative, isAbsolute, dirname, basename, join } from 'path'; + +/** + * Symlink-safe path confinement: realpath BOTH sides, then a separator-aware + * prefix check. A plain `startsWith()` on un-resolved paths would let a + * `parent/skills` symlink → `/etc` (or `$GBRAIN_HOME/clones/` → `/etc`) + * bypass the boundary; resolving first defeats that. + * + * Returns true iff `child` exists AND its realpath is `parent`'s realpath or a + * real subtree of it. Returns false if either path is unresolvable (missing / + * permission) or the resolved child escapes — fail-closed. + */ +export function isPathContained(child: string, parent: string): boolean { + let resolvedChild: string; + let resolvedParent: string; + try { + resolvedChild = realpathSync(child); + resolvedParent = realpathSync(parent); + } catch { + return false; // missing / unresolvable path → not contained + } + // Append a separator so /foo doesn't match /foobar. + const parentWithSep = resolvedParent.endsWith('/') ? resolvedParent : resolvedParent + '/'; + return resolvedChild === resolvedParent || resolvedChild.startsWith(parentWithSep); +} + +/** + * Trust gate for a walk-up routing dotfile, given its `lstatSync` Stats. + * + * The caller MUST pass an `lstatSync` result, never `statSync` — `lstat` does + * not follow symlinks, so a planted symlink redirect is visible here as + * `isSymbolicLink()` instead of being followed-then-trusted. + * + * Rejects three classes of untrusted file: + * 1. symlinks — an attacker-planted redirect to a file they control; + * 2. foreign-owned — `uid` is neither the caller's nor root's (an attacker + * can't `chown` a file to the victim, so foreign ownership means planted; + * root-owned is trusted — root is the system admin and can write anywhere + * regardless); + * 3. world-writable (`mode & 0o002`) — anyone can clobber it later, even when + * ownership is currently legitimate. + * + * On platforms without `process.getuid` (Windows) returns true: the + * multi-user-POSIX threat model does not apply and ownership is unknowable. + */ +export function isTrustedDotfile(stats: Stats): boolean { + // No numeric uid (Windows) → can't verify ownership; threat model N/A. + if (typeof process.getuid !== 'function') return true; + // A symlink is an attacker redirect — never trust. (Requires an lstat Stats.) + if (stats.isSymbolicLink()) return false; + const myUid = process.getuid(); + // Foreign-owned (not me, not root) → planted. Root-owned is trusted. + if (stats.uid !== myUid && stats.uid !== 0) return false; + // World-writable → anyone can clobber it later, even when ownership is legit. + if ((stats.mode & 0o002) !== 0) return false; + return true; +} + +/** + * Resolve a path through symlinks, falling back to lexical `resolve()` when the + * path doesn't exist (stale registration). Used by the registered-path prefix + * matchers so a symlinked CWD can't create a false prefix match against a + * registered `local_path` / mount path while still tolerating a registered path + * that no longer exists on disk. + */ +export function realpathOrResolve(p: string): string { + try { + return realpathSync(p); + } catch { + return resolvePath(p); + } +} + +/** + * Containment check for a write TARGET that may not exist yet (a new page file). + * `isPathContained` requires the child to already exist; this instead realpaths + * the deepest EXISTING ancestor of `target` (catching a symlinked intermediate + * directory that escapes the tree) and re-attaches the not-yet-created tail + * lexically, then confirms the result stays within `root`. + * + * Defense-in-depth for the write-through FS sink (#1647-slug / codex #6): + * `validateSlug` already rejects `..`/backslash/control/%2e in the slug, so this + * guards a pre-existing hostile row or a symlinked source-tree subdirectory. + */ +export function isWriteTargetContained(target: string, root: string): boolean { + const resolvedRoot = realpathOrResolve(root); + let existing = resolvePath(target); + const tail: string[] = []; + for (let i = 0; i < 4096 && !existsSync(existing); i++) { + tail.unshift(basename(existing)); + const parent = dirname(existing); + if (parent === existing) break; // filesystem root + existing = parent; + } + const base = realpathOrResolve(existing); + const finalPath = tail.length ? join(base, ...tail) : base; + const rel = relative(resolvedRoot, finalPath); + return rel === '' || (!rel.startsWith('..') && !isAbsolute(rel)); +} diff --git a/src/core/pglite-schema.ts b/src/core/pglite-schema.ts index 1a0bc6c..e733677 100644 --- a/src/core/pglite-schema.ts +++ b/src/core/pglite-schema.ts @@ -124,7 +124,7 @@ CREATE TABLE IF NOT EXISTS pages ( -- bookmark gate fires for any cache row stored before the new page existed. -- UPDATE: bumps only when content columns IS DISTINCT FROM (allow-list of -- 10 widened per D6) so read-time mutations don't invalidate every cache. -CREATE OR REPLACE FUNCTION bump_page_generation_fn() RETURNS trigger AS $func$ +CREATE OR REPLACE FUNCTION bump_page_generation_fn() RETURNS trigger SET search_path = pg_catalog, public AS $func$ BEGIN IF (TG_OP = 'INSERT') THEN NEW.generation := COALESCE((SELECT MAX(generation) FROM pages), 0) + 1; @@ -179,7 +179,7 @@ SELECT setval('page_generation_clock_seq', GREATEST( COALESCE((SELECT MAX(generation) FROM pages), 0) )); -CREATE OR REPLACE FUNCTION bump_page_generation_clock_fn() RETURNS trigger AS $func$ +CREATE OR REPLACE FUNCTION bump_page_generation_clock_fn() RETURNS trigger SET search_path = pg_catalog, public AS $func$ BEGIN PERFORM nextval('page_generation_clock_seq'); RETURN NULL; @@ -1016,7 +1016,7 @@ ALTER TABLE pages ADD COLUMN IF NOT EXISTS search_vector tsvector; CREATE INDEX IF NOT EXISTS idx_pages_search ON pages USING GIN(search_vector); -CREATE OR REPLACE FUNCTION update_page_search_vector() RETURNS trigger AS $$ +CREATE OR REPLACE FUNCTION update_page_search_vector() RETURNS trigger SET search_path = pg_catalog, public AS $$ DECLARE timeline_text TEXT; BEGIN diff --git a/src/core/repo-root.ts b/src/core/repo-root.ts index 9f6a922..29f7200 100644 --- a/src/core/repo-root.ts +++ b/src/core/repo-root.ts @@ -2,6 +2,7 @@ import { existsSync } from 'fs'; import { fileURLToPath } from 'url'; import { isAbsolute, join, resolve as resolvePath } from 'path'; import { RESOLVER_FILENAMES, hasResolverFile } from './resolver-filenames.ts'; +import { isPathContained } from './path-confine.ts'; /** * Walk up from `startDir` looking for a `skills/` directory that @@ -66,17 +67,23 @@ function resolveWorkspaceSkillsDir( sourceSubdir: SkillsDirSource, sourceRoot: SkillsDirSource, ): SkillsDirDetection | null { - // Preferred: workspace/skills with a resolver file inside it (gbrain-native). const subdir = join(workspace, 'skills'); + // Refuse a `skills/` that escapes the declared workspace via symlink (#419). + // isPathContained realpaths both ends, so `workspace/skills` → /etc is + // rejected, while a legit in-workspace symlink (`workspace/skills` → + // `workspace/_real-skills`) stays contained and is allowed. A non-contained + // candidate returns null so lower tiers can try, rather than trusting an escape. + const contained = isPathContained(subdir, workspace); + // Preferred: workspace/skills with a resolver file inside it (gbrain-native). if (hasResolverFile(subdir)) { - return { dir: subdir, source: sourceSubdir }; + return contained ? { dir: subdir, source: sourceSubdir } : null; } // Fallback: resolver file at workspace root (OpenClaw-native layout). // The skills/ subtree still governs file layout even when routing lives // at workspace root. Return the skills subdir so downstream file lookups // work; the resolver parser knows how to look one level up. if (hasResolverFile(workspace) && existsSync(subdir)) { - return { dir: subdir, source: sourceRoot }; + return contained ? { dir: subdir, source: sourceRoot } : null; } return null; } @@ -152,7 +159,10 @@ export function autoDetectSkillsDir( let dir = startDir; for (let i = 0; i < 10; i++) { const candidate = join(dir, 'skills'); - if (existsSync(candidate)) { + // Only accept a `skills/` contained within the ancestor it was found under + // (#419). An escaping symlink is skipped, and the walk continues upward + // rather than trusting a dir that resolves outside the boundary. + if (existsSync(candidate) && isPathContained(candidate, dir)) { return { dir: candidate, source: 'cwd_walk_up' }; } const parent = join(dir, '..'); @@ -177,7 +187,10 @@ export function autoDetectSkillsDir( // 3. gbrain repo walk from cwd. const repoRoot = findRepoRoot(startDir); if (repoRoot && isGbrainRepoRoot(repoRoot)) { - return { dir: join(repoRoot, 'skills'), source: 'repo_root' }; + const skillsDir = join(repoRoot, 'skills'); + if (isPathContained(skillsDir, repoRoot)) { + return { dir: skillsDir, source: 'repo_root' }; + } } // 4. ./skills fallback (with hasResolverFile gate). Functionally @@ -187,7 +200,7 @@ export function autoDetectSkillsDir( // In practice this tier never fires after 1b — cwd_walk_up matches // the same path first. Kept in the type union for back-compat. const cwdSkills = join(startDir, 'skills'); - if (hasResolverFile(cwdSkills)) { + if (hasResolverFile(cwdSkills) && isPathContained(cwdSkills, startDir)) { return { dir: cwdSkills, source: 'cwd_skills' }; } @@ -237,7 +250,10 @@ export function autoDetectSkillsDirReadOnly( const moduleDir = fileURLToPath(import.meta.url); const installRoot = findRepoRoot(moduleDir); if (installRoot && isGbrainRepoRoot(installRoot)) { - return { dir: join(installRoot, 'skills'), source: 'install_path' }; + const skillsDir = join(installRoot, 'skills'); + if (isPathContained(skillsDir, installRoot)) { + return { dir: skillsDir, source: 'install_path' }; + } } } catch { // fileURLToPath can throw on malformed import.meta.url (rare; some diff --git a/src/core/schema-embedded.ts b/src/core/schema-embedded.ts index 32beb37..e5ed576 100644 --- a/src/core/schema-embedded.ts +++ b/src/core/schema-embedded.ts @@ -164,7 +164,7 @@ CREATE TABLE IF NOT EXISTS pages ( -- content columns IS DISTINCT FROM (allow-list widened per D6 + codex #3 -- to include title/type/page_kind/corpus_generation/content_hash) so -- read-time mutations don't invalidate every cache row. -CREATE OR REPLACE FUNCTION bump_page_generation_fn() RETURNS trigger AS \$func\$ +CREATE OR REPLACE FUNCTION bump_page_generation_fn() RETURNS trigger SET search_path = pg_catalog, public AS \$func\$ BEGIN IF (TG_OP = 'INSERT') THEN NEW.generation := COALESCE((SELECT MAX(generation) FROM pages), 0) + 1; @@ -244,7 +244,7 @@ SELECT setval('page_generation_clock_seq', GREATEST( COALESCE((SELECT MAX(generation) FROM pages), 0) )); -CREATE OR REPLACE FUNCTION bump_page_generation_clock_fn() RETURNS trigger AS \$func\$ +CREATE OR REPLACE FUNCTION bump_page_generation_clock_fn() RETURNS trigger SET search_path = pg_catalog, public AS \$func\$ BEGIN PERFORM nextval('page_generation_clock_seq'); RETURN NULL; @@ -360,7 +360,7 @@ CREATE INDEX IF NOT EXISTS content_chunks_stale_idx -- NL queries ("how do we handle errors") rank doc-comment hits above body text. -- BEFORE INSERT OR UPDATE OF specific columns — only refires when those change, -- not on every chunk update (e.g., embedding refresh doesn't trigger rebuild). -CREATE OR REPLACE FUNCTION update_chunk_search_vector() RETURNS TRIGGER AS \$fn\$ +CREATE OR REPLACE FUNCTION update_chunk_search_vector() RETURNS TRIGGER SET search_path = pg_catalog, public AS \$fn\$ BEGIN NEW.search_vector := setweight(to_tsvector('english', COALESCE(NEW.doc_comment, '')), 'A') || @@ -819,7 +819,7 @@ ALTER TABLE pages ADD COLUMN IF NOT EXISTS search_vector tsvector; CREATE INDEX IF NOT EXISTS idx_pages_search ON pages USING GIN(search_vector); -- Function to rebuild search_vector for a page -CREATE OR REPLACE FUNCTION update_page_search_vector() RETURNS trigger AS \$\$ +CREATE OR REPLACE FUNCTION update_page_search_vector() RETURNS trigger SET search_path = pg_catalog, public AS \$\$ DECLARE timeline_text TEXT; BEGIN @@ -1358,7 +1358,7 @@ CREATE INDEX IF NOT EXISTS think_ab_results_recent_idx ON think_ab_results (source_id, ran_at DESC); -- NOTIFY trigger for real-time job events (Postgres only, not PGLite) -CREATE OR REPLACE FUNCTION notify_minion_job_change() RETURNS trigger AS \$\$ +CREATE OR REPLACE FUNCTION notify_minion_job_change() RETURNS trigger SET search_path = pg_catalog, public AS \$\$ BEGIN PERFORM pg_notify('minion_jobs', json_build_object( 'id', NEW.id, 'status', NEW.status, 'name', NEW.name, @@ -1383,7 +1383,9 @@ DO \$\$ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + -- #1385: recognize superuser + inherited-role BYPASSRLS, not just the role's + -- own rolbypassrls (alias \`pr\` avoids any plpgsql record-variable collision). + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; IF has_bypass THEN ALTER TABLE pages ENABLE ROW LEVEL SECURITY; ALTER TABLE content_chunks ENABLE ROW LEVEL SECURITY; diff --git a/src/core/source-resolver.ts b/src/core/source-resolver.ts index 64bb330..8b9f3ba 100644 --- a/src/core/source-resolver.ts +++ b/src/core/source-resolver.ts @@ -13,10 +13,11 @@ * commands (Steps 4/5), and the operation layer (Step 2+). */ -import { readFileSync, existsSync } from 'fs'; +import { readFileSync, lstatSync, type Stats } from 'fs'; import { join, dirname, resolve } from 'path'; import type { BrainEngine } from './engine.ts'; import { SOURCE_ID_RE, isValidSourceId } from './source-id.ts'; +import { isTrustedDotfile, realpathOrResolve } from './path-confine.ts'; const DOTFILE = '.gbrain-source'; // Canonical SOURCE_ID_RE imported from `source-id.ts` (single source of truth). @@ -34,7 +35,15 @@ function readDotfileWalk(startDir: string): string | null { // Guard against infinite loops on malformed paths. for (let i = 0; i < 50; i++) { const candidate = join(dir, DOTFILE); - if (existsSync(candidate)) { + // lstatSync (NOT statSync) so a planted symlink is seen here, not silently + // followed-then-trusted. Any stat error (ENOENT / permission) → skip this + // candidate and keep walking (fail-closed). On a multi-user host an + // attacker who can write a shared ancestor dir could otherwise plant a + // forged `.gbrain-source`; `isTrustedDotfile` refuses symlinks, + // foreign-owned, and world-writable files (#418). + let st: Stats | null = null; + try { st = lstatSync(candidate); } catch { st = null; } + if (st && isTrustedDotfile(st)) { try { const content = readFileSync(candidate, 'utf8').trim().split('\n')[0].trim(); // Silent-fallback tier per codex P1-F: invalid dotfile content @@ -105,10 +114,15 @@ export async function resolveSourceId( const registered = await engine.executeRaw<{ id: string; local_path: string }>( `SELECT id, local_path FROM sources WHERE local_path IS NOT NULL`, ); - const cwdResolved = resolve(cwd); + // realpath BOTH sides (not bare resolve) so a symlinked CWD can't forge a + // prefix match against a registered local_path it doesn't really live under + // (codex #9). realpathOrResolve falls back to lexical resolve() for a stale + // registration whose path no longer exists. Resolving both sides keeps a + // legitimately symlinked vault matching — only one-sided symlinks break. + const cwdResolved = realpathOrResolve(cwd); let best: { id: string; pathLen: number } | null = null; for (const r of registered) { - const p = resolve(r.local_path); + const p = realpathOrResolve(r.local_path); if (cwdResolved === p || cwdResolved.startsWith(p + '/')) { if (!best || p.length > best.pathLen) { best = { id: r.id, pathLen: p.length }; @@ -303,10 +317,11 @@ export async function resolveSourceWithTier( const registered = await engine.executeRaw<{ id: string; local_path: string }>( `SELECT id, local_path FROM sources WHERE local_path IS NOT NULL`, ); - const cwdResolved = resolve(cwd); + // realpath both sides — see the matching block in resolveSourceId (codex #9). + const cwdResolved = realpathOrResolve(cwd); let best: { id: string; path: string; pathLen: number } | null = null; for (const r of registered) { - const p = resolve(r.local_path); + const p = realpathOrResolve(r.local_path); if (cwdResolved === p || cwdResolved.startsWith(p + '/')) { if (!best || p.length > best.pathLen) { best = { id: r.id, path: p, pathLen: p.length }; diff --git a/src/core/sources-ops.ts b/src/core/sources-ops.ts index a7d95ee..69c31ba 100644 --- a/src/core/sources-ops.ts +++ b/src/core/sources-ops.ts @@ -37,8 +37,8 @@ */ import { existsSync, mkdirSync, renameSync, rmSync, lstatSync } from 'fs'; -import { realpathSync } from 'fs'; import { join, dirname, basename, resolve as resolvePath } from 'path'; +import { isPathContained } from './path-confine.ts'; import { randomBytes } from 'crypto'; import type { BrainEngine } from './engine.ts'; import { @@ -231,28 +231,10 @@ function makeTempCloneDir(id: string): string { return gbrainPath('clones', '.tmp', `${id}-${rand}`); } -/** - * Symlink-safe path confinement: realpath both sides, then lstat-walk to - * confirm `child` is a real subtree of `parent`. Mirrors validateUploadPath - * shape at src/core/operations.ts:61. String startsWith() would let - * $GBRAIN_HOME/clones/ → /etc bypass the confine. - * - * Returns true if `child` exists and is contained under `parent`. - * Returns false if the resolved path escapes, or either path is unresolvable. - */ -export function isPathContained(child: string, parent: string): boolean { - let resolvedChild: string; - let resolvedParent: string; - try { - resolvedChild = realpathSync(child); - resolvedParent = realpathSync(parent); - } catch { - return false; // missing path → not contained - } - // Append a separator to parent so /foo doesn't match /foobar. - const parentWithSep = resolvedParent.endsWith('/') ? resolvedParent : resolvedParent + '/'; - return resolvedChild === resolvedParent || resolvedChild.startsWith(parentWithSep); -} +// `isPathContained` moved to `src/core/path-confine.ts` (shared with the +// dotfile-trust + skills-dir confinement helpers). Re-exported here so existing +// importers (and recloneIfMissing below) keep working unchanged. +export { isPathContained }; /** * Did gbrain CREATE this clone (so re-clone/delete is safe)? Ownership, NOT diff --git a/src/core/transcription.ts b/src/core/transcription.ts index 7aed5f7..e87efdb 100644 --- a/src/core/transcription.ts +++ b/src/core/transcription.ts @@ -6,8 +6,8 @@ * For files >25MB: ffmpeg segmentation into <25MB chunks, transcribe each, concatenate. */ -import { statSync, readFileSync } from 'fs'; -import { basename, extname } from 'path'; +import { statSync, readFileSync, rmSync } from 'fs'; +import { basename, extname, join } from 'path'; // --------------------------------------------------------------------------- // Types @@ -169,14 +169,22 @@ async function transcribeLargeFile( ); } - // Segment into ~20MB chunks (with some overlap for better joining) - const { execSync } = await import('child_process'); - const tmpDir = execSync('mktemp -d').toString().trim(); + // Segment into ~20MB chunks (with some overlap for better joining). + // + // SECURITY (#245): use execFileSync with argument arrays — never a shell + // command string. audioPath is interpolated here, and a path containing + // shell metacharacters (`;`, `$()`, backticks, quotes) would be a command + // injection if passed through a shell. Argument arrays bypass the shell + // entirely, so audioPath is always a single literal argv element. + const { execFileSync } = await import('child_process'); + const tmpDir = execFileSync('mktemp', ['-d']).toString().trim(); try { // Get audio duration - const durationStr = execSync( - `ffprobe -v error -show_entries format=duration -of default=noprint_wrappers=1:nokey=1 "${audioPath}"`, + const durationStr = execFileSync( + 'ffprobe', + ['-v', 'error', '-show_entries', 'format=duration', + '-of', 'default=noprint_wrappers=1:nokey=1', audioPath], { encoding: 'utf-8' } ).trim(); const totalDuration = parseFloat(durationStr) || 0; @@ -188,8 +196,10 @@ async function transcribeLargeFile( // Split audio const ext = extname(audioPath); - execSync( - `ffmpeg -i "${audioPath}" -f segment -segment_time ${segmentSeconds} -c copy "${tmpDir}/segment_%03d${ext}"`, + execFileSync( + 'ffmpeg', + ['-i', audioPath, '-f', 'segment', '-segment_time', String(segmentSeconds), + '-c', 'copy', join(tmpDir, `segment_%03d${ext}`)], { stdio: 'pipe' } ); @@ -200,7 +210,7 @@ async function transcribeLargeFile( let timeOffset = 0; for (const seg of segments) { - const segPath = `${tmpDir}/${seg}`; + const segPath = join(tmpDir, seg); const result = await transcribeFile(segPath, provider, apiKey, config); // Offset timestamps result.segments = result.segments.map(s => ({ @@ -221,15 +231,15 @@ async function transcribeLargeFile( provider, }; } finally { - // Cleanup temp directory - try { execSync(`rm -rf "${tmpDir}"`); } catch {} + // Cleanup temp directory via fs.rmSync — never a shell remove command (#245). + try { rmSync(tmpDir, { recursive: true, force: true }); } catch {} } } async function checkFfmpeg(): Promise { try { - const { execSync } = await import('child_process'); - execSync('ffmpeg -version', { stdio: 'pipe' }); + const { execFileSync } = await import('child_process'); + execFileSync('ffmpeg', ['-version'], { stdio: 'pipe' }); return true; } catch { return false; diff --git a/src/core/utils.ts b/src/core/utils.ts index dd34843..0e885b2 100644 --- a/src/core/utils.ts +++ b/src/core/utils.ts @@ -19,11 +19,36 @@ export function generateToken(prefix: string): string { /** * Validate and normalize a slug. Slugs are lowercased repo-relative paths. * Rejects empty slugs, path traversal (..), and leading /. + * + * SECURITY (#1647-slug / codex #6): also rejects a small set of dangerous + * characters that survive the `..` check but can still produce a hostile + * filename at the write-through FS sink or hide the real target — NUL/control + * bytes, Unicode bidirectional/RTL overrides, backslashes, and URL-encoded + * path separators/traversal. None appear in legitimate slugs (lowercase + * alphanumerics, hyphens, dots, underscores, slashes, unicode letters, and CJK + * all still pass), so this is pure hardening, not a behavior change. This is the + * shared chokepoint for both `putPage` and `updateSlug` on both engines. */ export function validateSlug(slug: string): string { if (!slug || /(^|\/)\.\.($|\/)/.test(slug) || /^\//.test(slug)) { throw new Error(`Invalid slug: "${slug}". Slugs cannot be empty, start with /, or contain path traversal.`); } + // Control / NUL bytes (C0 + DEL + C1). + if (/[\x00-\x1f\x7f-\x9f]/.test(slug)) { + throw new Error(`Invalid slug: "${slug}". Slugs cannot contain control characters.`); + } + // Unicode bidirectional / RTL overrides (visual-spoofing of the real path). + if (/[\u202a-\u202e\u2066-\u2069]/.test(slug)) { + throw new Error(`Invalid slug: "${slug}". Slugs cannot contain bidirectional/RTL override characters.`); + } + // Backslash (Windows-style separator / escape). + if (slug.includes('\\')) { + throw new Error(`Invalid slug: "${slug}". Backslashes are not allowed in slugs.`); + } + // URL-encoded path separators / traversal (%2e=., %2f=/, %5c=\). + if (/%2e|%2f|%5c/i.test(slug)) { + throw new Error(`Invalid slug: "${slug}". URL-encoded path separators are not allowed in slugs.`); + } return slug.toLowerCase(); } diff --git a/src/core/write-through.ts b/src/core/write-through.ts index 96a1aad..5ca2b56 100644 --- a/src/core/write-through.ts +++ b/src/core/write-through.ts @@ -26,6 +26,7 @@ import { dirname, join } from 'path'; import { randomBytes } from 'crypto'; import type { BrainEngine } from './engine.ts'; import { serializePageToMarkdown, resolvePageFilePath } from './markdown.ts'; +import { isWriteTargetContained } from './path-confine.ts'; /** Minimal logger surface — structurally compatible with operations.ts `Logger`. */ export interface WriteThroughLogger { @@ -45,8 +46,10 @@ export interface WriteThroughResult { * — #2018: writing here would pollute that sibling's repo, so we skip. * - page_not_found_after_write: the DB row isn't readable back (the caller's * DB write failed or targeted a different source). + * - path_escapes_source_root: the computed file path resolves outside the + * source's working tree (hostile slug row / symlinked subtree) — refused. */ - skipped?: 'no_repo_configured' | 'repo_not_found' | 'source_repo_belongs_to_other_source' | 'page_not_found_after_write'; + skipped?: 'no_repo_configured' | 'repo_not_found' | 'source_repo_belongs_to_other_source' | 'page_not_found_after_write' | 'path_escapes_source_root'; /** Set when the render/write/rename itself threw (EACCES, ENOTDIR, disk full). */ error?: string; } @@ -83,6 +86,7 @@ export async function writePageThrough( // `local_path`, nesting this page there would pollute that sibling's // git repo (the reported bug). Skip instead. let filePath: string; + let writeRoot: string; const srcRows = await engine.executeRaw<{ local_path: string | null }>( `SELECT local_path FROM sources WHERE id = $1`, [sourceId], @@ -93,6 +97,7 @@ export async function writePageThrough( return { written: false, skipped: 'repo_not_found' }; } filePath = join(sourceLocalPath, `${slug}.md`); + writeRoot = sourceLocalPath; } else { const repoPath = await engine.getConfig('sync.repo_path'); if (!repoPath) { @@ -111,6 +116,16 @@ export async function writePageThrough( return { written: false, skipped: 'source_repo_belongs_to_other_source' }; } filePath = resolvePageFilePath(repoPath, slug, sourceId); + writeRoot = repoPath; + } + + // Defense-in-depth (#1647-slug / codex #6): confirm the computed file path + // stays within the source's working tree before any mkdir/write. validateSlug + // already rejects `..`/backslash/control/%2e in the slug at write time, so + // this guards a pre-existing hostile row or a symlinked intermediate dir + // under the source tree from escaping to an arbitrary filesystem location. + if (!isWriteTargetContained(filePath, writeRoot)) { + return { written: false, skipped: 'path_escapes_source_root' }; } const writtenPage = await engine.getPage(slug, { sourceId }); diff --git a/src/schema.sql b/src/schema.sql index dcf9455..463d274 100644 --- a/src/schema.sql +++ b/src/schema.sql @@ -160,7 +160,7 @@ CREATE TABLE IF NOT EXISTS pages ( -- content columns IS DISTINCT FROM (allow-list widened per D6 + codex #3 -- to include title/type/page_kind/corpus_generation/content_hash) so -- read-time mutations don't invalidate every cache row. -CREATE OR REPLACE FUNCTION bump_page_generation_fn() RETURNS trigger AS $func$ +CREATE OR REPLACE FUNCTION bump_page_generation_fn() RETURNS trigger SET search_path = pg_catalog, public AS $func$ BEGIN IF (TG_OP = 'INSERT') THEN NEW.generation := COALESCE((SELECT MAX(generation) FROM pages), 0) + 1; @@ -240,7 +240,7 @@ SELECT setval('page_generation_clock_seq', GREATEST( COALESCE((SELECT MAX(generation) FROM pages), 0) )); -CREATE OR REPLACE FUNCTION bump_page_generation_clock_fn() RETURNS trigger AS $func$ +CREATE OR REPLACE FUNCTION bump_page_generation_clock_fn() RETURNS trigger SET search_path = pg_catalog, public AS $func$ BEGIN PERFORM nextval('page_generation_clock_seq'); RETURN NULL; @@ -356,7 +356,7 @@ CREATE INDEX IF NOT EXISTS content_chunks_stale_idx -- NL queries ("how do we handle errors") rank doc-comment hits above body text. -- BEFORE INSERT OR UPDATE OF specific columns — only refires when those change, -- not on every chunk update (e.g., embedding refresh doesn't trigger rebuild). -CREATE OR REPLACE FUNCTION update_chunk_search_vector() RETURNS TRIGGER AS $fn$ +CREATE OR REPLACE FUNCTION update_chunk_search_vector() RETURNS TRIGGER SET search_path = pg_catalog, public AS $fn$ BEGIN NEW.search_vector := setweight(to_tsvector('english', COALESCE(NEW.doc_comment, '')), 'A') || @@ -815,7 +815,7 @@ ALTER TABLE pages ADD COLUMN IF NOT EXISTS search_vector tsvector; CREATE INDEX IF NOT EXISTS idx_pages_search ON pages USING GIN(search_vector); -- Function to rebuild search_vector for a page -CREATE OR REPLACE FUNCTION update_page_search_vector() RETURNS trigger AS $$ +CREATE OR REPLACE FUNCTION update_page_search_vector() RETURNS trigger SET search_path = pg_catalog, public AS $$ DECLARE timeline_text TEXT; BEGIN @@ -1354,7 +1354,7 @@ CREATE INDEX IF NOT EXISTS think_ab_results_recent_idx ON think_ab_results (source_id, ran_at DESC); -- NOTIFY trigger for real-time job events (Postgres only, not PGLite) -CREATE OR REPLACE FUNCTION notify_minion_job_change() RETURNS trigger AS $$ +CREATE OR REPLACE FUNCTION notify_minion_job_change() RETURNS trigger SET search_path = pg_catalog, public AS $$ BEGIN PERFORM pg_notify('minion_jobs', json_build_object( 'id', NEW.id, 'status', NEW.status, 'name', NEW.name, @@ -1379,7 +1379,9 @@ DO $$ DECLARE has_bypass BOOLEAN; BEGIN - SELECT rolbypassrls INTO has_bypass FROM pg_roles WHERE rolname = current_user; + -- #1385: recognize superuser + inherited-role BYPASSRLS, not just the role's + -- own rolbypassrls (alias `pr` avoids any plpgsql record-variable collision). + SELECT EXISTS (SELECT 1 FROM pg_roles pr WHERE pg_has_role(current_user, pr.oid, 'USAGE') AND (pr.rolbypassrls OR pr.rolsuper)) INTO has_bypass; IF has_bypass THEN ALTER TABLE pages ENABLE ROW LEVEL SECURITY; ALTER TABLE content_chunks ENABLE ROW LEVEL SECURITY; diff --git a/test/e2e/engine-parity.test.ts b/test/e2e/engine-parity.test.ts index 7b27424..f0de50e 100644 --- a/test/e2e/engine-parity.test.ts +++ b/test/e2e/engine-parity.test.ts @@ -588,7 +588,12 @@ describeBoth('Engine parity — relationalFanout', () => { }, 30_000); const shape = (rows: Awaited>) => - rows.map(r => `${r.source_id}:${r.slug}:${r.hop}:${r.edge_count}:${r.via_link_types.join(',')}:${r.path.join('>')}:${r.canonical_chunk_id ?? 'null'}`); + // canonical_chunk_id is a serial id — its absolute value diverges between a + // fresh PGLite engine and a shared Postgres DB whose content_chunks sequence + // advanced earlier (setupDB TRUNCATEs without RESTART IDENTITY). Compare its + // PRESENCE, not the exact id, so the parity check verifies graph structure + + // canonical-chunk resolution without depending on cross-engine sequence state. + rows.map(r => `${r.source_id}:${r.slug}:${r.hop}:${r.edge_count}:${r.via_link_types.join(',')}:${r.path.join('>')}:${r.canonical_chunk_id != null ? 'set' : 'null'}`); test('typed-edge fan-out is identical across engines', async () => { const opts = { direction: 'in' as const, linkTypes: ['invested_in'] }; diff --git a/test/e2e/postgres-bootstrap.test.ts b/test/e2e/postgres-bootstrap.test.ts index 1924829..32b31f3 100644 --- a/test/e2e/postgres-bootstrap.test.ts +++ b/test/e2e/postgres-bootstrap.test.ts @@ -90,4 +90,31 @@ describe.skipIf(skip)('PostgresEngine forward-reference bootstrap (E2E)', () => await engine.initSchema(); expect(await engine.getConfig('version')).toBe(String(LATEST_VERSION)); }); + + // Migration v120 — schema-lint hardening (#1647 / #171). Postgres-only + // assertions (security_invoker has no surface on embedded PGLite). + test('v120: page_links view runs with security_invoker=on (#1647b)', async () => { + await engine.initSchema(); + const rows = await engine.executeRaw<{ reloptions: string[] | null }>( + `SELECT c.reloptions FROM pg_class c JOIN pg_namespace n ON n.oid = c.relnamespace + WHERE n.nspname = 'public' AND c.relname = 'page_links' AND c.relkind = 'v'`, + ); + expect(rows.length).toBe(1); + expect(JSON.stringify(rows[0].reloptions ?? [])).toContain('security_invoker=on'); + }); + + test('v120: trigger + event-trigger functions pin search_path, incl auto_enable_rls (#1647a/#171)', async () => { + await engine.initSchema(); + const rows = await engine.executeRaw<{ proname: string; proconfig: unknown }>( + `SELECT p.proname, p.proconfig FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' + AND p.proname IN ('bump_page_generation_fn','bump_page_generation_clock_fn', + 'update_chunk_search_vector','update_page_search_vector', + 'notify_minion_job_change','auto_enable_rls')`, + ); + expect(rows.length).toBeGreaterThanOrEqual(5); + for (const r of rows) { + expect(JSON.stringify(r.proconfig ?? [])).toContain('search_path='); + } + }); }); diff --git a/test/migration-v120.test.ts b/test/migration-v120.test.ts new file mode 100644 index 0000000..b6fac0c --- /dev/null +++ b/test/migration-v120.test.ts @@ -0,0 +1,48 @@ +/** + * Migration v120 — schema-lint hardening (#1647 / #171). + * + * Validates the search_path pin lands on the PGLite trigger functions and that + * the migration is idempotent. (PGLite is Postgres 17.5, so this also + * empirically confirms `ALTER FUNCTION ... SET search_path` runs on PGLite — + * the engine-asymmetry concern from the eng-review codex pass.) The + * security_invoker + auto_enable_rls assertions are Postgres-only and live in + * the Postgres bootstrap E2E. + */ + +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { runMigrations } from '../src/core/migrate.ts'; + +describe('migration v120 — search_path hardening', () => { + let engine: PGLiteEngine; + + beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); // applies all migrations through LATEST_VERSION (incl. v120) + }); + + afterAll(async () => { + await engine.disconnect(); + }); + + test('PGLite trigger functions carry SET search_path after migrations', async () => { + const rows = await engine.executeRaw<{ proname: string; proconfig: unknown }>( + `SELECT p.proname, p.proconfig + FROM pg_proc p JOIN pg_namespace n ON n.oid = p.pronamespace + WHERE n.nspname = 'public' + AND p.proname IN ('bump_page_generation_fn','bump_page_generation_clock_fn','update_page_search_vector')`, + ); + expect(rows.length).toBe(3); + for (const r of rows) { + // proconfig is a text[] like {search_path=pg_catalog, public}; coerce to a + // string so the assertion is robust to driver array shape. + expect(JSON.stringify(r.proconfig ?? [])).toContain('search_path='); + } + }, 30000); + + test('re-running migrations after initSchema is idempotent (0 applied, no error)', async () => { + const res = await runMigrations(engine); + expect(res.applied).toBe(0); + }, 30000); +}); diff --git a/test/oauth.test.ts b/test/oauth.test.ts index 3c466ec..ee528ed 100644 --- a/test/oauth.test.ts +++ b/test/oauth.test.ts @@ -11,7 +11,7 @@ import { } from '../src/core/oauth-provider.ts'; import { hashToken, generateToken } from '../src/core/utils.ts'; import { PGLITE_SCHEMA_SQL } from '../src/core/pglite-schema.ts'; -import { InvalidTokenError } from '@modelcontextprotocol/sdk/server/auth/errors.js'; +import { InvalidTokenError, InvalidClientMetadataError } from '@modelcontextprotocol/sdk/server/auth/errors.js'; import type { AuthInfo as CoreAuthInfo } from '../src/core/operations.ts'; // --------------------------------------------------------------------------- @@ -1489,12 +1489,50 @@ describe('v0.41.3 DCR validator (T5)', () => { test('DCR accepts "client_secret_basic" — codex F3 regression', async () => { const reg = await provider.clientsStore.registerClient!({ client_name: 'dcr-basic-test', - grant_types: ['client_credentials'], + grant_types: ['authorization_code'], scope: 'read', - redirect_uris: [], + redirect_uris: ['https://example.test/cb'], token_endpoint_auth_method: 'client_secret_basic', } as any); expect(reg.client_id).toStartWith('gbrain_cl_'); expect(reg.client_secret).toStartWith('gbrain_cs_'); }); }); + +describe('#1353 DCR default-grant hardening', () => { + test('DCR rejects explicit client_credentials by default', async () => { + await expect( + provider.clientsStore.registerClient!({ + client_name: 'cc-default-test', + grant_types: ['client_credentials'], + scope: 'read', + redirect_uris: [], + token_endpoint_auth_method: 'client_secret_post', + } as any), + ).rejects.toThrow(InvalidClientMetadataError); + }); + + test('DCR defaults to authorization_code when grant_types unspecified', async () => { + const reg = await provider.clientsStore.registerClient!({ + client_name: 'no-grant-test', + scope: 'read', + redirect_uris: ['https://example.test/cb'], + token_endpoint_auth_method: 'none', + } as any); + const stored = await provider.clientsStore.getClient(reg.client_id); + expect(stored?.grant_types).toEqual(['authorization_code']); + }); + + test('--enable-dcr-insecure (allowClientCredentialsDcr) permits client_credentials', async () => { + const insecure = new GBrainOAuthProvider({ sql, allowClientCredentialsDcr: true }); + const reg = await insecure.clientsStore.registerClient!({ + client_name: 'cc-allowed-test', + grant_types: ['client_credentials'], + scope: 'read', + redirect_uris: [], + token_endpoint_auth_method: 'client_secret_post', + } as any); + const stored = await insecure.clientsStore.getClient(reg.client_id); + expect(stored?.grant_types).toEqual(['client_credentials']); + }); +}); diff --git a/test/path-confine.test.ts b/test/path-confine.test.ts new file mode 100644 index 0000000..960d2f8 --- /dev/null +++ b/test/path-confine.test.ts @@ -0,0 +1,319 @@ +/** + * Security tests for the shared path-confinement + dotfile-trust helpers + * (src/core/path-confine.ts) and their integration into the source / brain + * resolvers and the skills-dir auto-detector. + * + * Threat model: a multi-user POSIX host where an attacker can write into a + * shared ancestor directory of the victim's CWD. The hardening must refuse a + * routing dotfile (`.gbrain-source` / `.gbrain-mount`) the victim doesn't own, + * and refuse a `skills/` directory that escapes its declared workspace via + * symlink. (#418 / #419, codex #9) + */ + +import { describe, test, expect, beforeEach, afterEach } from 'bun:test'; +import { + mkdtempSync, mkdirSync, writeFileSync, rmSync, symlinkSync, chmodSync, + lstatSync, type Stats, +} from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { + isTrustedDotfile, isPathContained, realpathOrResolve, isWriteTargetContained, +} from '../src/core/path-confine.ts'; +import { validateSlug } from '../src/core/utils.ts'; +import { resolveSourceId } from '../src/core/source-resolver.ts'; +import { resolveBrainId } from '../src/core/brain-resolver.ts'; +import { HOST_BRAIN_ID } from '../src/core/brain-registry.ts'; +import { autoDetectSkillsDir } from '../src/core/repo-root.ts'; +import type { BrainEngine } from '../src/core/engine.ts'; + +// ── helpers ──────────────────────────────────────────────── + +const created: string[] = []; +function scratch(prefix = 'path-confine-'): string { + const dir = mkdtempSync(join(tmpdir(), prefix)); + created.push(dir); + return dir; +} +afterEach(() => { + while (created.length) { + const p = created.pop()!; + try { rmSync(p, { recursive: true, force: true }); } catch { /* ignore */ } + } +}); + +function fakeStats(opts: { uid: number; mode: number; symlink?: boolean }): Stats { + return { + uid: opts.uid, + mode: opts.mode, + isSymbolicLink: () => opts.symlink === true, + } as unknown as Stats; +} + +/** Stub engine for resolveSourceId: registers `ids`, no local_paths, no default. */ +function stubEngine(ids: string[]): BrainEngine { + return { + kind: 'pglite', + executeRaw: async (sql: string, params?: unknown[]): Promise => { + if (sql.includes('SELECT id FROM sources WHERE id = $1')) { + const t = params?.[0]; + return (ids.includes(t as string) ? [{ id: t } as unknown as T] : []); + } + if (sql.includes('SELECT id, local_path FROM sources')) return [] as unknown as T[]; + return [] as unknown as T[]; + }, + getConfig: async () => null, + } as unknown as BrainEngine; +} + +// ── isTrustedDotfile (synthetic Stats — precise branch coverage) ── + +describe('isTrustedDotfile', () => { + const realGetuid = process.getuid; + beforeEach(() => { (process as { getuid?: () => number }).getuid = () => 1000; }); + afterEach(() => { (process as { getuid?: () => number }).getuid = realGetuid; }); + + test('own, not world-writable, not symlink → trusted', () => { + expect(isTrustedDotfile(fakeStats({ uid: 1000, mode: 0o644 }))).toBe(true); + }); + test('foreign-owned → NOT trusted', () => { + expect(isTrustedDotfile(fakeStats({ uid: 2000, mode: 0o644 }))).toBe(false); + }); + test('root-owned → trusted (root-as-root allowance)', () => { + expect(isTrustedDotfile(fakeStats({ uid: 0, mode: 0o644 }))).toBe(true); + }); + test('world-writable, even when owned → NOT trusted', () => { + expect(isTrustedDotfile(fakeStats({ uid: 1000, mode: 0o666 }))).toBe(false); + }); + test('symlink → NOT trusted regardless of ownership', () => { + expect(isTrustedDotfile(fakeStats({ uid: 1000, mode: 0o644, symlink: true }))).toBe(false); + }); + test('no getuid (Windows) → trusted by default', () => { + (process as { getuid?: () => number }).getuid = undefined; + expect(isTrustedDotfile(fakeStats({ uid: 2000, mode: 0o666 }))).toBe(true); + }); +}); + +describe('isTrustedDotfile — real fs (lstat)', () => { + test('a real owned file is trusted; a symlink and a world-writable file are not', () => { + if (typeof process.getuid !== 'function') return; // POSIX-only + const dir = scratch(); + const ownFile = join(dir, 'own'); + writeFileSync(ownFile, 'x'); + expect(isTrustedDotfile(lstatSync(ownFile))).toBe(true); + + const link = join(dir, 'link'); + symlinkSync(ownFile, link); + expect(isTrustedDotfile(lstatSync(link))).toBe(false); + + const ww = join(dir, 'ww'); + writeFileSync(ww, 'x'); + chmodSync(ww, 0o666); + expect(isTrustedDotfile(lstatSync(ww))).toBe(false); + }); +}); + +// ── isPathContained + realpathOrResolve ──────────────────── + +describe('isPathContained', () => { + test('real subdir is contained', () => { + const dir = scratch(); + const sub = join(dir, 'sub'); + mkdirSync(sub); + expect(isPathContained(sub, dir)).toBe(true); + }); + test('symlink escaping the parent is NOT contained', () => { + const dir = scratch(); + const outside = scratch('pc-outside-'); + const link = join(dir, 'escape'); + symlinkSync(outside, link); + expect(isPathContained(link, dir)).toBe(false); + }); + test('symlink resolving INSIDE the parent IS contained', () => { + const dir = scratch(); + const real = join(dir, 'real'); + mkdirSync(real); + const link = join(dir, 'inlink'); + symlinkSync(real, link); + expect(isPathContained(link, dir)).toBe(true); + }); + test('missing path → not contained (fail-closed)', () => { + const dir = scratch(); + expect(isPathContained(join(dir, 'nope'), dir)).toBe(false); + }); + test('sibling prefix does not match (/foo vs /foobar)', () => { + const base = scratch(); + const foo = join(base, 'foo'); const foobar = join(base, 'foobar'); + mkdirSync(foo); mkdirSync(foobar); + expect(isPathContained(foobar, foo)).toBe(false); + }); +}); + +describe('realpathOrResolve', () => { + test('resolves a symlink to its real target', () => { + const dir = scratch(); + const real = join(dir, 'real'); mkdirSync(real); + const link = join(dir, 'link'); symlinkSync(real, link); + expect(realpathOrResolve(link)).toBe(realpathOrResolve(real)); + }); + test('nonexistent path → lexical resolve (does not throw)', () => { + const p = join(scratch(), 'does', 'not', 'exist'); + expect(realpathOrResolve(p)).toBe(p); + }); +}); + +// ── validateSlug dangerous-char hardening (#1647-slug / codex #6) ── + +describe('validateSlug — dangerous-char guard', () => { + test('allows legitimate slugs (lowercase, unicode, dot, underscore, CJK, nested)', () => { + for (const ok of ['notes/2026', 'münchen', 'my_notes', 'a-b/c-d', '会议/纪要', 'readme.v2', 'Mixed-Case']) { + expect(() => validateSlug(ok)).not.toThrow(); + } + }); + test('rejects path traversal and leading slash (existing behavior preserved)', () => { + for (const bad of ['../etc/passwd', 'a/../../b', '/abs/path', '']) { + expect(() => validateSlug(bad)).toThrow(); + } + }); + test('rejects NUL / control bytes', () => { + expect(() => validateSlug("a" + String.fromCharCode(0x00) + "b")).toThrow(/control/); + expect(() => validateSlug("a" + String.fromCharCode(0x1f) + "b")).toThrow(/control/); + }); + test('rejects Unicode bidirectional / RTL overrides', () => { + expect(() => validateSlug("a" + String.fromCharCode(0x202e) + "b")).toThrow(/bidirectional|RTL/); + expect(() => validateSlug("a" + String.fromCharCode(0x2066) + "b")).toThrow(/bidirectional|RTL/); + }); + test('rejects backslashes', () => { + expect(() => validateSlug('a\\b')).toThrow(/[Bb]ackslash/); + }); + test('rejects URL-encoded path separators / traversal', () => { + for (const bad of ['a%2e%2e/b', 'a%2fb', 'a%5cb', 'A%2E%2Eb']) { + expect(() => validateSlug(bad)).toThrow(/URL-encoded/); + } + }); +}); + +// ── isWriteTargetContained (write-through FS sink) ────────── + +describe('isWriteTargetContained', () => { + test('a new file directly under root is contained', () => { + const root = scratch(); + expect(isWriteTargetContained(join(root, 'page.md'), root)).toBe(true); + }); + test('a new file in a new nested dir under root is contained', () => { + const root = scratch(); + expect(isWriteTargetContained(join(root, 'sub', 'deep', 'page.md'), root)).toBe(true); + }); + test('a ../ escape is refused', () => { + const root = scratch(); + expect(isWriteTargetContained(join(root, '..', 'escape.md'), root)).toBe(false); + }); + test('a symlinked intermediate dir escaping the root is refused', () => { + if (typeof process.getuid !== 'function') return; + const root = scratch(); + const outside = scratch('wt-outside-'); + symlinkSync(outside, join(root, 'link')); // root/link → outside + // target lands under the escaping symlink → real path is outside root + expect(isWriteTargetContained(join(root, 'link', 'page.md'), root)).toBe(false); + }); + test('a symlinked intermediate dir staying inside the root is allowed', () => { + const root = scratch(); + mkdirSync(join(root, 'real')); + symlinkSync(join(root, 'real'), join(root, 'link')); + expect(isWriteTargetContained(join(root, 'link', 'page.md'), root)).toBe(true); + }); +}); + +// ── source-resolver integration (#418) ───────────────────── + +describe('resolveSourceId — .gbrain-source dotfile trust', () => { + test('trusted dotfile resolves to its id (control)', async () => { + const dir = scratch(); + writeFileSync(join(dir, '.gbrain-source'), 'evil\n'); + expect(await resolveSourceId(stubEngine(['evil', 'default']), null, dir)).toBe('evil'); + }); + test('symlinked dotfile is REFUSED → falls through to default', async () => { + if (typeof process.getuid !== 'function') return; + const dir = scratch(); + const target = join(dir, 'target'); writeFileSync(target, 'evil\n'); + symlinkSync(target, join(dir, '.gbrain-source')); + expect(await resolveSourceId(stubEngine(['evil', 'default']), null, dir)).toBe('default'); + }); + test('world-writable dotfile is REFUSED → falls through to default', async () => { + if (typeof process.getuid !== 'function') return; + const dir = scratch(); + const df = join(dir, '.gbrain-source'); writeFileSync(df, 'evil\n'); chmodSync(df, 0o666); + expect(await resolveSourceId(stubEngine(['evil', 'default']), null, dir)).toBe('default'); + }); +}); + +// ── brain-resolver integration (#418, brain axis) ────────── + +describe('resolveBrainId — .gbrain-mount dotfile trust', () => { + const noMounts = () => []; + test('trusted dotfile resolves to its id (control)', () => { + const dir = scratch(); + writeFileSync(join(dir, '.gbrain-mount'), 'evil-brain\n'); + expect(resolveBrainId(null, dir, noMounts)).toBe('evil-brain'); + }); + test('symlinked .gbrain-mount is REFUSED → falls through to host', () => { + if (typeof process.getuid !== 'function') return; + const dir = scratch(); + const target = join(dir, 'm'); writeFileSync(target, 'evil-brain\n'); + symlinkSync(target, join(dir, '.gbrain-mount')); + expect(resolveBrainId(null, dir, noMounts)).toBe(HOST_BRAIN_ID); + }); + test('world-writable .gbrain-mount is REFUSED → falls through to host', () => { + if (typeof process.getuid !== 'function') return; + const dir = scratch(); + const df = join(dir, '.gbrain-mount'); writeFileSync(df, 'evil-brain\n'); chmodSync(df, 0o666); + expect(resolveBrainId(null, dir, noMounts)).toBe(HOST_BRAIN_ID); + }); +}); + +// ── repo-root skills-dir confinement (#419) ──────────────── + +describe('autoDetectSkillsDir — skills/ symlink confinement', () => { + function seedSkills(dir: string): void { + mkdirSync(join(dir, 'skills'), { recursive: true }); + writeFileSync(join(dir, 'skills', 'RESOLVER.md'), '# RESOLVER\n'); + } + + test('OPENCLAW_WORKSPACE: escaping skills symlink is refused', () => { + if (typeof process.getuid !== 'function') return; + const ws = scratch('ws-'); const outside = scratch('outside-'); + seedSkills(outside); // real skills with RESOLVER.md, OUTSIDE the workspace + symlinkSync(join(outside, 'skills'), join(ws, 'skills')); // ws/skills → outside/skills + const found = autoDetectSkillsDir(scratch('cwd-'), { OPENCLAW_WORKSPACE: ws }); + expect(found.source).not.toBe('openclaw_workspace_env'); + expect(found.dir).not.toBe(join(ws, 'skills')); + }); + + test('OPENCLAW_WORKSPACE: in-workspace skills symlink is allowed', () => { + const ws = scratch('ws-'); + mkdirSync(join(ws, '_real'), { recursive: true }); + writeFileSync(join(ws, '_real', 'RESOLVER.md'), '# RESOLVER\n'); + symlinkSync(join(ws, '_real'), join(ws, 'skills')); // contained symlink + const found = autoDetectSkillsDir(scratch('cwd-'), { OPENCLAW_WORKSPACE: ws }); + expect(found.source).toBe('openclaw_workspace_env'); + expect(found.dir).toBe(join(ws, 'skills')); + }); + + test('cwd_walk_up: escaping skills symlink is refused', () => { + if (typeof process.getuid !== 'function') return; + const ws = scratch('ws-'); const outside = scratch('outside-'); + mkdirSync(join(outside, 'skills'), { recursive: true }); + symlinkSync(join(outside, 'skills'), join(ws, 'skills')); + const found = autoDetectSkillsDir(ws, {}); + expect(found.dir).toBeNull(); + }); + + test('cwd_walk_up: in-workspace skills symlink is allowed', () => { + const ws = scratch('ws-'); + mkdirSync(join(ws, '_real'), { recursive: true }); + symlinkSync(join(ws, '_real'), join(ws, 'skills')); + const found = autoDetectSkillsDir(ws, {}); + expect(found.source).toBe('cwd_walk_up'); + expect(found.dir).toBe(join(ws, 'skills')); + }); +}); diff --git a/test/transcription-injection.test.ts b/test/transcription-injection.test.ts new file mode 100644 index 0000000..ad05058 --- /dev/null +++ b/test/transcription-injection.test.ts @@ -0,0 +1,65 @@ +/** + * Security test for #245 — the large-file transcription path must not pass + * caller-controlled audio paths through a shell. transcribeLargeFile now uses + * execFileSync with argument arrays (+ fs.rmSync for cleanup), so a path + * containing shell metacharacters is a literal argv element, never parsed. + * + * transcription.ts is a dead-internally-but-PUBLISHED export (gbrain/transcription), + * so an external programmatic consumer can still call transcribe() with an + * attacker-influenced path — hence the harden over delete. + */ + +import { describe, test, expect } from 'bun:test'; +import { + mkdtempSync, writeFileSync, rmSync, existsSync, readFileSync, +} from 'fs'; +import { join } from 'path'; +import { tmpdir } from 'os'; +import { execFileSync } from 'child_process'; +import { transcribe } from '../src/core/transcription.ts'; + +function ffmpegAvailable(): boolean { + try { execFileSync('ffmpeg', ['-version'], { stdio: 'pipe' }); + execFileSync('ffprobe', ['-version'], { stdio: 'pipe' }); + return true; } catch { return false; } +} + +describe('transcription — command injection (#245)', () => { + // Source-level guard (deterministic, no ffmpeg dependency): the shell-string + // exec forms must be gone. Mirrors the repo's check-*.sh guard philosophy. + test('source uses execFileSync arg-arrays, never shell execSync', () => { + const src = readFileSync(new URL('../src/core/transcription.ts', import.meta.url), 'utf8'); + // `\bexecSync(` matches the dangerous shell-string call but NOT execFileSync. + expect(src).not.toMatch(/\bexecSync\s*\(/); // no shell command strings + expect(src).toMatch(/execFileSync\(/); // arg-array exec + expect(src).toMatch(/rmSync\(/); // fs cleanup, not shell remove + }); + + // Behavioral: a >25MB file whose NAME contains a $(...) payload. Under the old + // shell-string code the double-quoted interpolation command-substitutes the + // payload (creating the sentinel) before ffprobe runs; under execFileSync the + // payload is a literal filename, so no sentinel is ever created. + test('a $(...) payload in the audio path does not spawn a shell', async () => { + if (!ffmpegAvailable()) return; // path requires ffmpeg/ffprobe; skip otherwise + // Sentinel must be a slash-free name so it's a legal single filename segment; + // a shell `touch ` would create it in process.cwd(), so we check there. + const sentinelName = `gbrain-inj-sentinel-${process.pid}-${Date.now()}`; + const sentinelPath = join(process.cwd(), sentinelName); + if (existsSync(sentinelPath)) rmSync(sentinelPath, { force: true }); + const dir = mkdtempSync(join(tmpdir(), 'transcribe-inj-')); + // Payload touches the sentinel IF a shell ever parses the path. + const evil = join(dir, `clip$(touch ${sentinelName}).mp3`); + writeFileSync(evil, Buffer.alloc(26 * 1024 * 1024)); // >25MB → large-file path + try { + await transcribe(evil, { apiKey: 'dummy', provider: 'groq' }); + } catch { + // Expected: ffprobe/ffmpeg reject the garbage file, or transcribeFile has + // no real key. We only care that no shell ran. + } finally { + rmSync(dir, { recursive: true, force: true }); + } + const pwned = existsSync(sentinelPath); + if (pwned) rmSync(sentinelPath, { force: true }); + expect(pwned).toBe(false); + }, 30_000); +});