From 1eb430a2df9f842a754dd6af9910f049ccac65a1 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Mon, 8 Jun 2026 21:19:25 -0700 Subject: [PATCH] v0.42.37.0 fix(security,ingest): source-isolation grant enforcement + non-string frontmatter guard + papercuts (#1999) * fix(security): scope cross-source reads to the caller grant; close get_page exact-path leak One shared resolveRequestedScope() routes every source-scoped read op (query, code_callers/callees, search_by_image, code_blast/flow, get_page) through a single fail-closed trust+grant ladder: a remote caller's __all__ collapses to its granted sources (never the whole brain) and an explicit out-of-grant source_id is rejected. get_page's exact-match path now honors a federated grant via getPage(sourceIds[]) in both engines. Legacy bearer tokens carry their stored permissions.source_id grant (bounded, never widened). Also retries getConfig on transient connection loss. Closes #1924, #1371, #1393, #1336, #1603. * fix(ingest): non-string frontmatter no longer aborts lint/sync; embed/hook/catalog papercuts Parser coerces a non-string title to a string and falls back to inference for slug/type (never fabricating a "123" slug), with a lint NON_STRING_FIELD finding surfacing the malformed frontmatter; a defensive guard in content-sanity stops a non-string title from crashing the whole lint/sync run brain-wide. Plus: embed --catch-up no longer arms the overflowed 32-bit budget timer (and surfaces unembeddable chunks); the frontmatter pre-commit hook ships a correct .md/.mdx regex; and the skill catalog parses YAML block-scalar descriptions. Closes #1883, #1658, #1556, #1948, #1946, #1840, #1711. * v0.42.37.0 fix(security,ingest): source-isolation grant enforcement + non-string frontmatter guard + papercuts Co-Authored-By: Claude Opus 4.8 (1M context) * docs: add NON_STRING_FIELD frontmatter validation class to docs for v0.42.37.0 The v0.42.37.0 non-string-frontmatter fix added an eighth validation class (NON_STRING_FIELD / lint code frontmatter-non-string-field). Update the two current-state docs that enumerate the validation classes: - skills/frontmatter-guard/SKILL.md (seven->eight + table row) - docs/integrations/pre-commit.md (seven->eight + table row) Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 23 +++ VERSION | 2 +- docs/integrations/pre-commit.md | 3 +- package.json | 2 +- skills/frontmatter-guard/SKILL.md | 3 +- src/commands/embed.ts | 39 ++++- src/commands/frontmatter-install-hook.ts | 2 +- src/commands/lint.ts | 1 + src/core/content-sanity.ts | 6 +- src/core/markdown.ts | 23 +++ src/core/operations.ts | 190 +++++++++++++++------ src/core/pglite-engine.ts | 10 +- src/core/postgres-engine.ts | 37 +++- src/core/skill-catalog.ts | 46 ++++- src/core/types.ts | 6 + src/mcp/http-transport.ts | 54 +++++- test/frontmatter-install-hook.test.ts | 19 +++ test/frontmatter-non-string-fields.test.ts | 83 +++++++++ test/get-page-federated-scope.test.ts | 91 ++++++++++ test/legacy-token-federated-scope.test.ts | 45 +++++ test/skill-catalog-block-scalar.test.ts | 49 ++++++ test/source-scope-resolver.test.ts | 123 +++++++++++++ 22 files changed, 768 insertions(+), 89 deletions(-) create mode 100644 test/frontmatter-non-string-fields.test.ts create mode 100644 test/get-page-federated-scope.test.ts create mode 100644 test/legacy-token-federated-scope.test.ts create mode 100644 test/skill-catalog-block-scalar.test.ts create mode 100644 test/source-scope-resolver.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 60a26a647..3a1f1ba82 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,29 @@ All notable changes to GBrain will be documented in this file. +## [0.42.37.0] - 2026-06-08 + +**Cross-source reads now honor the caller's grant everywhere, a single bad frontmatter value no longer wedges a whole `lint`/`sync` run, and a handful of long-standing papercuts are gone.** A triage of the open issue backlog pulled the highest-impact bugs into one wave. + +The headline is a source-isolation hardening pass. Every read that can be scoped to a source now resolves through one shared, fail-closed trust+grant check, so a remote client only ever sees the sources it was granted — whether it asks for one source, all sources, or reads a page by exact slug. Reads route the same way across query, the code-intel traversals, image search, and `get_page`. Legacy bearer tokens now carry the source grant an operator already stored on them, instead of being pinned to `default`. + +On ingestion, a non-string frontmatter value (a bare number or date in `title:`, `slug:`, or `type:`) used to throw partway through and abort the entire run — so one malformed file could stop a whole brain from linting or syncing. Now those values are coerced to a usable string (a bare date `2024-06-01` becomes a real slug, not a crash), and `gbrain lint` flags the un-quoted field by name so you can clean it up. + +Plus: `gbrain embed --catch-up` runs to completion instead of stopping after the first batch (and tells you when chunks genuinely can't be embedded); the frontmatter pre-commit hook actually matches `.md`/`.mdx` files now instead of silently doing nothing; the skill catalog shows the real description for skills that write it as a YAML block scalar; and `getConfig` retries through a transient connection blip instead of silently falling back to defaults. + +### Fixed +- **Source-scoped reads honor the caller's grant across every read op (gbrain#1924, #1371, #1393).** One shared resolver replaces the per-op scope logic: a remote caller's "all sources" request is bounded to its grant, an out-of-grant source is refused, and `get_page`'s exact-slug path is scoped like every other read (both engines). +- **Legacy bearer tokens carry their stored source grant (gbrain#1336).** Tokens with an operator-set source grant read across exactly those sources instead of being limited to `default`. +- **Non-string frontmatter no longer aborts `lint`/`sync` (gbrain#1883, #1658, #1556, #1948).** Title/slug/type are coerced to usable strings instead of throwing mid-run, and `gbrain lint` reports the un-quoted field by name. +- **`embed --catch-up` runs to completion (gbrain#1946).** The mode no longer stops after one batch, and surfaces chunks that can't be embedded instead of looking like a clean finish. +- **Frontmatter pre-commit hook matches `.md`/`.mdx` files (gbrain#1840).** The installed hook was a silent no-op; it now validates staged markdown on commit. +- **Skill catalog shows block-scalar descriptions (gbrain#1711).** Skills written with `description: |` show their real text instead of a stray indicator. +- **`getConfig` retries on a transient connection blip (gbrain#1603)** instead of silently falling through to defaults (which surfaced as the wrong search mode / empty output on remote Postgres). + +### To take advantage of v0.42.37.0 + +`gbrain upgrade`. No configuration needed. If `gbrain lint` now flags a `frontmatter-non-string-field` on a page, quote the value in that page's frontmatter (e.g. `title: "123"`). Reinstall the pre-commit hook with `gbrain frontmatter install-hook` to pick up the fixed matcher. + ## [0.42.36.0] - 2026-06-08 **A huge `gbrain sync` that keeps getting killed now converges instead of restarting from zero.** On a high-write source — hundreds of thousands of files, a generator committing faster than each sync can drain — a full sync that ran past its launching session's timeout (SIGTERM) would lose 100% of its progress and re-import the entire backlog on the next run, forever. The bookmark never advanced, the source went quietly stale for hours while the importer burned CPU the whole time, and competing hourly launches stole each other's lock and raced. This release makes a large sync **resumable, durable, and single-flight** so it banks what it imports and picks up where it left off. diff --git a/VERSION b/VERSION index c4ec2f4a4..6f19cb7c6 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.42.36.0 +0.42.37.0 diff --git a/docs/integrations/pre-commit.md b/docs/integrations/pre-commit.md index f683161b2..fc0453fdf 100644 --- a/docs/integrations/pre-commit.md +++ b/docs/integrations/pre-commit.md @@ -7,7 +7,7 @@ brain source's repo that runs `gbrain frontmatter validate` against staged ## What the hook catches -The same seven validation classes the `frontmatter-guard` skill and +The same eight validation classes the `frontmatter-guard` skill and `gbrain doctor`'s `frontmatter_integrity` subcheck report: | Code | What it catches | @@ -18,6 +18,7 @@ The same seven validation classes the `frontmatter-guard` skill and | `SLUG_MISMATCH` | `slug:` in frontmatter doesn't match path-derived slug | | `NULL_BYTES` | Binary corruption (`\x00`) anywhere in the content | | `NESTED_QUOTES` | `title: "outer "inner" outer"` shape that breaks YAML | +| `NON_STRING_FIELD` | `title`/`type`/`slug` is an unquoted non-string scalar (`title: 123`) | | `EMPTY_FRONTMATTER` | `---` ... `---` with nothing meaningful between | ## Install diff --git a/package.json b/package.json index 332d060ab..fbd6e4e2d 100644 --- a/package.json +++ b/package.json @@ -143,5 +143,5 @@ "bun": ">=1.3.10" }, "license": "MIT", - "version": "0.42.36.0" + "version": "0.42.37.0" } diff --git a/skills/frontmatter-guard/SKILL.md b/skills/frontmatter-guard/SKILL.md index 2f9e43ff8..6c0744945 100644 --- a/skills/frontmatter-guard/SKILL.md +++ b/skills/frontmatter-guard/SKILL.md @@ -24,7 +24,7 @@ mutating: true ## Contract This skill guarantees: -- Every brain page is scanned against the seven canonical frontmatter validation classes +- Every brain page is scanned against the eight canonical frontmatter validation classes - Mechanical errors (nested quotes, missing closing `---`, null bytes, slug mismatch) are auto-repairable on demand with `.bak` backups - Validation logic is shared with `gbrain doctor`'s `frontmatter_integrity` subcheck — single source of truth - Reports per source (gbrain is multi-source since v0.18.0); never silently audits the wrong root @@ -50,6 +50,7 @@ Without a guard, these accumulate silently until `gbrain sync` chokes or search | `SLUG_MISMATCH` | Frontmatter `slug:` differs from path-derived slug | Yes (removes the field) | | `NULL_BYTES` | Binary corruption (`\x00`) | Yes | | `NESTED_QUOTES` | `title: "outer "inner" outer"` shape | Yes | +| `NON_STRING_FIELD` | `title`/`type`/`slug` is an unquoted non-string scalar (e.g. `title: 123`, `slug: 2024-06-01`) | No (quote the value) | | `EMPTY_FRONTMATTER` | Open + close present but nothing between | No (needs human) | ## Phases diff --git a/src/commands/embed.ts b/src/commands/embed.ts index 08685dc69..3afefab03 100644 --- a/src/commands/embed.ts +++ b/src/commands/embed.ts @@ -629,15 +629,20 @@ async function embedAllStale( const CONCURRENCY = parseInt(process.env.GBRAIN_EMBED_CONCURRENCY || '20', 10); // D3 + D3a + D8: wall-clock budget. 30 min default; env override. - // v0.41.18.0 (A13): --catch-up removes the wall-clock cap entirely so the - // handler runs until countStaleChunks() returns 0. Use Number.MAX_SAFE_INTEGER - // (effectively unbounded) instead of the 30-min default. The AbortController - // still wraps for SIGINT propagation; just the timer never fires. - const BUDGET_MS = staleOpts?.catchUp - ? Number.MAX_SAFE_INTEGER + // #1946: --catch-up removes the wall-clock cap. The prior code set BUDGET_MS = + // Number.MAX_SAFE_INTEGER and passed it to setTimeout — but setTimeout's delay + // is a 32-bit signed int, so MAX_SAFE_INTEGER (9e15) overflows and the timer + // fires almost immediately, aborting catch-up after a single batch. The fix is + // to NOT arm the timer in catch-up at all: the keyset pass below terminates on + // its own (the (page_id, chunk_index) cursor advances monotonically), and + // SIGINT / worker-abort still propagate via externalSignal. + const BUDGET_MS: number | null = staleOpts?.catchUp + ? null : parseInt(process.env.GBRAIN_EMBED_TIME_BUDGET_MS || `${30 * 60 * 1000}`, 10); const budgetController = new AbortController(); - const budgetTimer = setTimeout(() => budgetController.abort(), BUDGET_MS); + const budgetTimer = BUDGET_MS != null + ? setTimeout(() => budgetController.abort(), BUDGET_MS) + : undefined; const budgetSignal = budgetController.signal; // #1737: the effective signal fires when EITHER the internal wall-clock // budget OR the caller's abort (worker timeout / lock loss / SIGTERM) fires. @@ -660,6 +665,10 @@ async function embedAllStale( let afterUpdatedAt: string | null = null; let totalChunksLoaded = 0; let budgetExitNotified = false; + // #1946 (OV2a): track chunks that errored out so a catch-up pass that finishes + // with stale chunks still remaining (un-embeddable for a non-transient reason) + // surfaces that loudly instead of looking like a clean run. + let embedFailures = 0; try { // eslint-disable-next-line no-constant-condition @@ -746,6 +755,7 @@ async function embedAllStale( // Budget/abort-fired cancellations are expected on the way out; don't // spam per-page "Error embedding" lines when we're shutting down. if (effectiveSignal.aborted) return; + embedFailures++; serr(`\n Error embedding ${slug}: ${e instanceof Error ? e.message : e}`); } totalProcessedPages++; @@ -772,10 +782,23 @@ async function embedAllStale( if (batch.length < PAGE_SIZE) break; } } finally { - clearTimeout(budgetTimer); + if (budgetTimer) clearTimeout(budgetTimer); } slog(`Embedded ${result.embedded} chunks across ${totalProcessedPages} pages`); + + // #1946 (OV2a): a catch-up pass that completed without being aborted but left + // chunks unembedded means those chunks are stuck (a non-transient embed + // failure), not that we ran out of time. Surface it loudly so it doesn't read + // as a clean run — re-running won't help until the underlying failure is fixed. + if (staleOpts?.catchUp && !effectiveSignal.aborted && embedFailures > 0) { + const remaining = await engine.countStaleChunks( + signature ? { signature, ...(sourceId ? { sourceId } : {}) } : (sourceId ? { sourceId } : undefined), + ); + if (remaining > 0) { + serr(`\n [embed] catch-up finished but ${remaining} chunk(s) remain stale after ${embedFailures} embed failure(s). These are not embeddable as-is; re-running won't clear them until the underlying error is resolved.`); + } + } } /** diff --git a/src/commands/frontmatter-install-hook.ts b/src/commands/frontmatter-install-hook.ts index 1477d28c1..6224a982f 100644 --- a/src/commands/frontmatter-install-hook.ts +++ b/src/commands/frontmatter-install-hook.ts @@ -39,7 +39,7 @@ if ! command -v gbrain >/dev/null 2>&1; then exit 0 fi -staged=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\\\\.mdx?$' || true) +staged=$(git diff --cached --name-only --diff-filter=ACM | grep -E '\\.mdx?$' || true) [ -z "$staged" ] && exit 0 failed=0 diff --git a/src/commands/lint.ts b/src/commands/lint.ts index 21b3cac55..c9bfdba8d 100644 --- a/src/commands/lint.ts +++ b/src/commands/lint.ts @@ -45,6 +45,7 @@ const FRONTMATTER_RULE_NAMES: Record = { SLUG_MISMATCH: 'frontmatter-slug-mismatch', NULL_BYTES: 'frontmatter-null-bytes', NESTED_QUOTES: 'frontmatter-nested-quotes', + NON_STRING_FIELD: 'frontmatter-non-string-field', EMPTY_FRONTMATTER: 'frontmatter-empty', }; diff --git a/src/core/content-sanity.ts b/src/core/content-sanity.ts index ca5260162..17a3616c9 100644 --- a/src/core/content-sanity.ts +++ b/src/core/content-sanity.ts @@ -376,9 +376,9 @@ export function assessContentSanity(opts: { // doesn't repeat the lowercase per literal. const bodyHead = body.slice(0, SCAN_HEAD_BYTES); const bodyHeadLower = bodyHead.toLowerCase(); - // Defensive coercion (issue #1939): this is a pure exported fn; lint.ts and - // import-file both pass `parsed.title`, which a malformed YAML date/number - // title could make non-string. Never throw on a bad title. + // Defensive coercion (issue #1939 / #1883 / #1658): this is a pure exported fn; + // lint.ts and import-file both pass `parsed.title`, which a malformed YAML + // date/number title could make non-string. Never throw on a bad title. const title = String(opts.title ?? ''); const titleLower = title.toLowerCase(); diff --git a/src/core/markdown.ts b/src/core/markdown.ts index 9a6359b6b..79cef3df0 100644 --- a/src/core/markdown.ts +++ b/src/core/markdown.ts @@ -10,6 +10,7 @@ export type ParseValidationCode = | 'SLUG_MISMATCH' | 'NULL_BYTES' | 'NESTED_QUOTES' + | 'NON_STRING_FIELD' | 'EMPTY_FRONTMATTER'; export interface ParseValidationError { @@ -124,6 +125,13 @@ export function parseMarkdown( const { compiled_truth, timeline } = splitBody(body); + // #1948/#1939: frontmatter values can be non-strings (YAML coerces `title: 123` + // → number, a bare date → Date). The `as string` cast used to lie: a truthy + // non-string flowed downstream typed as string and crashed the first + // `.toLowerCase()` (content-sanity), aborting the whole lint/sync run. + // coerceFrontmatterString turns a scalar/date into a usable string (a date slug + // `2024-06-01` is legitimate); the NON_STRING_FIELD lint finding below still + // surfaces the un-quoted field so it can be cleaned up. const type = coerceFrontmatterString(frontmatter.type) || ( opts?.activePack ? inferTypeFromPack(filePath, opts.activePack) : inferType(filePath) ); @@ -308,6 +316,21 @@ function collectValidationErrors( }); } } + + // 8. NON_STRING_FIELD (#1948) — title/type/slug declared as a non-string YAML + // scalar (e.g. `title: 123`, `slug: 2024`). The parser coerces title to a + // string and falls back to inference for type/slug, but lint surfaces the + // malformed frontmatter so it gets fixed rather than silently rewritten. + // Pre-fix the slug validator above `typeof`-skipped these, hiding them. + for (const field of ['title', 'type', 'slug'] as const) { + const v = ctx.parsedFrontmatter[field]; + if (v != null && typeof v !== 'string') { + errors.push({ + code: 'NON_STRING_FIELD', + message: `Frontmatter "${field}" should be a string but is ${typeof v} (${JSON.stringify(v)}); quote the value (e.g. ${field}: "${String(v)}").`, + }); + } + } } /** diff --git a/src/core/operations.ts b/src/core/operations.ts index 8739a2232..aa6f41e41 100644 --- a/src/core/operations.ts +++ b/src/core/operations.ts @@ -425,6 +425,91 @@ export function sourceScopeOpts(ctx: OperationContext): { sourceId?: string; sou return {}; } +/** + * Resolve a per-call requested source scope against the caller's trust + grant. + * FAIL-CLOSED: anything not strictly `ctx.remote === false` is untrusted. + * + * This is the SINGLE resolver for every read op that accepts a per-call + * `source_id` / `all_sources` parameter (query, code_callers, code_callees, + * get_page, search_by_image, code_blast, code_flow). Inlining the `__all__` + * branch per handler is the bug class that leaked cross-source reads (#1924, + * #1371): a remote client could pass `source_id: '__all__'` to opt out of its + * grant, or pass an explicit out-of-grant `source_id` that was never checked. + * + * - `__all__` / `all_sources`: + * trusted local (remote === false) → `{}` (spans the whole brain) + * remote → the caller's grant (sourceScopeOpts) + * - explicit `source_id`: + * remote + federated grant that doesn't include it → permission_denied + * otherwise → `{ sourceId }` + * - neither → the caller's grant (sourceScopeOpts). + * + * `code_traversal_cache_clear` is intentionally NOT a caller — it is localOnly + * and carries its own destructive D8 all_sources guard. + */ +export function resolveRequestedScope( + ctx: OperationContext, + sourceIdParam: string | undefined, + allSourcesParam = false, +): { sourceId?: string; sourceIds?: string[] } { + const wantsAll = allSourcesParam || sourceIdParam === '__all__'; + if (wantsAll) { + return ctx.remote === false ? {} : sourceScopeOpts(ctx); + } + if (sourceIdParam !== undefined) { + const allowed = ctx.auth?.allowedSources; + if (ctx.remote !== false && allowed && allowed.length > 0 && !allowed.includes(sourceIdParam)) { + throw new OperationError( + 'permission_denied', + `source '${sourceIdParam}' is outside your granted sources`, + 'Request access to this source, or omit source_id to search within your grant.', + ); + } + return { sourceId: sourceIdParam }; + } + return sourceScopeOpts(ctx); +} + +/** + * Code-intel adapter for `resolveRequestedScope`. Graph traversal + * (code_callers/code_callees/code_blast/code_flow) is single-source by design — + * the engine APIs and the traversal cache key take ONE `sourceId` string, not a + * federated array. So this collapses the resolver's output to `{allSources, + * sourceId}`, fail-closed: + * + * - resolver → one source (scalar or single-element grant) → that source + * - resolver → multi-source grant (federated remote client) → reject: ask the + * caller to specify which granted source (we must not silently span all) + * - resolver → empty scope → `allSources` ONLY for trusted local callers; a + * remote caller with no source in scope is denied, never widened to all. + */ +export function resolveCodeIntelScope( + ctx: OperationContext, + sourceIdParam: string | undefined, + allSourcesParam = false, +): { allSources: boolean; sourceId?: string } { + const scope = resolveRequestedScope(ctx, sourceIdParam, allSourcesParam); + if (scope.sourceId) return { allSources: false, sourceId: scope.sourceId }; + if (scope.sourceIds && scope.sourceIds.length === 1) { + return { allSources: false, sourceId: scope.sourceIds[0] }; + } + if (scope.sourceIds && scope.sourceIds.length > 1) { + throw new OperationError( + 'invalid_params', + 'Code traversal runs against a single source. Specify source_id (one of your granted sources).', + 'Pass source_id=.', + ); + } + // Empty scope: span everything only for trusted local callers; a remote caller + // that reached here has no source in scope and must NOT get cross-source results. + if (ctx.remote === false) return { allSources: true, sourceId: undefined }; + throw new OperationError( + 'permission_denied', + 'No source in scope for this request.', + 'Specify source_id, or check your granted sources.', + ); +} + /** * T4/D5 — resolve a per-call search-mode override. Honored ONLY for trusted/ * local callers (ctx.remote === false) so a remote OAuth client can't escalate @@ -524,19 +609,14 @@ const get_page: Operation = { const slug = p.slug as string; const fuzzy = (p.fuzzy as boolean) || false; const includeDeleted = (p.include_deleted as boolean) === true; - // v0.31.8 (D20): thread ctx.sourceId through read-side ops. Only pass - // sourceId when it's set on ctx — when unset (local CLI default chain - // resolves to no source), the engine two-branch query falls through to - // the cross-source view, preserving pre-v0.31.8 behavior. MCP callers - // (stdio + HTTP) populate ctx.sourceId via the transport layer. - const sourceOpts = ctx.sourceId ? { sourceId: ctx.sourceId } : {}; - // v0.41.13 #1436: fuzzy resolveSlugs ALSO needs source scope — pre-fix - // it was unscoped, so a remote `get_page` with `fuzzy: true` could - // return candidates from sources outside ctx.auth.allowedSources / - // ctx.sourceId. sourceScopeOpts(ctx) is the canonical precedence - // ladder (federated array > scalar > nothing) shared with every other - // read-side handler. - const fuzzyScope = sourceScopeOpts(ctx); + // #1393: route BOTH the exact-match read and the fuzzy resolveSlugs through + // the canonical precedence ladder (federated array > scalar > nothing). The + // exact path previously used scalar `ctx.sourceId` only, so a remote client + // with a federated `allowedSources` grant (and no single ctx.sourceId) got + // an UNSCOPED exact lookup — a cross-source read of any page by slug. getPage + // now honors sourceIds[] (both engines), so the same scope closes both paths. + const sourceOpts = sourceScopeOpts(ctx); + const fuzzyScope = sourceOpts; let page = await ctx.engine.getPage(slug, { includeDeleted, ...sourceOpts }); let resolved_slug: string | undefined; @@ -1398,7 +1478,7 @@ const query: Operation = { source_id: { type: 'string', description: - "v0.34: scope search to a single source. Defaults to OperationContext.sourceId (set from CLI --source / GBRAIN_SOURCE / .gbrain-source dotfile). Pass '__all__' to force cross-source search in multi-source brains.", + "v0.34: scope search to a single source. Defaults to OperationContext.sourceId (set from CLI --source / GBRAIN_SOURCE / .gbrain-source dotfile). Pass '__all__' to span every source for trusted local callers; for remote callers '__all__' spans only your granted sources.", }, cross_modal: { type: 'string', @@ -1448,15 +1528,14 @@ const query: Operation = { typeof p.embedding_column === 'string' && p.embedding_column.length > 0 ? (p.embedding_column as string) : undefined; - // Explicit per-call source_id must win over ctx.sourceId. The special - // __all__ value opts out of source filtering for local cross-source search. + // Explicit per-call source_id must win over ctx.sourceId. `__all__` spans + // every source for trusted local callers, but only the caller's granted + // sources for remote callers (resolveRequestedScope is the single + // trust+grant resolver shared by every source-scoped read op). This scope + // is spread into BOTH the image-similarity searchVector path and the text + // hybridSearch path below, so both honor the same grant. const sourceIdParam = typeof p.source_id === 'string' ? p.source_id : undefined; - const querySourceScope = - sourceIdParam !== undefined - ? sourceIdParam === '__all__' - ? {} - : { sourceId: sourceIdParam } - : sourceScopeOpts(ctx); + const querySourceScope = resolveRequestedScope(ctx, sourceIdParam); // v0.27.1: image-similarity branch. Bypasses hybridSearch (which is // text-only); embeds the image via embedMultimodal and runs a direct @@ -3790,21 +3869,17 @@ const code_callers: Operation = { params: { symbol: { type: 'string', required: true, description: 'Symbol to find callers of (bare or qualified name).' }, limit: { type: 'number', description: 'Max edges returned. Default 100.' }, - source_id: { type: 'string', description: "Scope to a single source. Defaults to ctx.sourceId; pass '__all__' to force cross-source." }, - all_sources: { type: 'boolean', description: 'Force cross-source search (equivalent to source_id=__all__).' }, + source_id: { type: 'string', description: "Scope to a single source. Defaults to ctx.sourceId; '__all__' spans every source for trusted local callers, your granted sources for remote callers." }, + all_sources: { type: 'boolean', description: 'Span sources (equivalent to source_id=__all__): every source locally, your grant remotely.' }, }, scope: 'read', handler: async (ctx, p) => { const symbol = p.symbol as string; const limit = (p.limit as number) ?? 100; - const allSourcesParam = p.all_sources === true; const sourceIdParam = typeof p.source_id === 'string' ? p.source_id : undefined; - const allSources = allSourcesParam || sourceIdParam === '__all__'; - const sourceId = allSources - ? undefined - : sourceIdParam !== undefined - ? sourceIdParam - : ctx.sourceId; + // Single trust+grant resolver: remote callers can't span sources outside + // their grant, and `__all__` collapses to their grant (not the whole brain). + const { allSources, sourceId } = resolveCodeIntelScope(ctx, sourceIdParam, p.all_sources === true); const edges = await ctx.engine.getCallersOf(symbol, { limit, allSources, @@ -3825,21 +3900,16 @@ const code_callees: Operation = { params: { symbol: { type: 'string', required: true, description: 'Symbol to find callees of (bare or qualified name).' }, limit: { type: 'number', description: 'Max edges returned. Default 100.' }, - source_id: { type: 'string', description: "Scope to a single source. Defaults to ctx.sourceId; pass '__all__' to force cross-source." }, - all_sources: { type: 'boolean', description: 'Force cross-source search.' }, + source_id: { type: 'string', description: "Scope to a single source. Defaults to ctx.sourceId; '__all__' spans every source for trusted local callers, your granted sources for remote callers." }, + all_sources: { type: 'boolean', description: 'Span sources: every source locally, your grant remotely.' }, }, scope: 'read', handler: async (ctx, p) => { const symbol = p.symbol as string; const limit = (p.limit as number) ?? 100; - const allSourcesParam = p.all_sources === true; const sourceIdParam = typeof p.source_id === 'string' ? p.source_id : undefined; - const allSources = allSourcesParam || sourceIdParam === '__all__'; - const sourceId = allSources - ? undefined - : sourceIdParam !== undefined - ? sourceIdParam - : ctx.sourceId; + // Single trust+grant resolver (see code_callers). + const { allSources, sourceId } = resolveCodeIntelScope(ctx, sourceIdParam, p.all_sources === true); const edges = await ctx.engine.getCalleesOf(symbol, { limit, allSources, @@ -3910,6 +3980,7 @@ const code_blast: Operation = { depth: { type: 'number', description: 'Hop cap (default 5, max 8)' }, max_nodes: { type: 'number', description: 'Result-set cap (default 200)' }, exact: { type: 'boolean', description: 'Skip bare-name disambiguation; treat symbol as exact qualified name' }, + source_id: { type: 'string', description: 'Source to traverse. Defaults to ctx.sourceId; federated clients with multiple granted sources must specify one.' }, }, scope: 'read', handler: async (ctx, p) => { @@ -3919,14 +3990,20 @@ const code_blast: Operation = { const depth = Math.min((p.depth as number) ?? 5, 8); const max_nodes = Math.min((p.max_nodes as number) ?? 200, 200); const exact = (p.exact as boolean) ?? false; + // Single trust+grant resolver: a remote federated client can't traverse a + // source outside its grant (pre-fix this scoped by bare ctx.sourceId only). + // Falls back to ctx.sourceId (a required string) for the trusted-local case, + // exactly preserving pre-fix local behavior. + const { sourceId: scopedSourceId } = resolveCodeIntelScope(ctx, typeof p.source_id === 'string' ? p.source_id : undefined); + const sourceId = scopedSourceId ?? ctx.sourceId; return getCachedOrCompute( ctx.engine, - { symbol_qualified: symbol, depth, source_id: ctx.sourceId }, + { symbol_qualified: symbol, depth, source_id: sourceId }, () => runRecursiveWalk(ctx.engine, symbol, { direction: 'callers', depth, maxNodes: max_nodes, - sourceId: ctx.sourceId, + sourceId, exact, }), ); @@ -3942,6 +4019,7 @@ const code_flow: Operation = { depth: { type: 'number', description: 'Hop cap (default 8, max 12)' }, max_nodes: { type: 'number', description: 'Result-set cap (default 200)' }, exact: { type: 'boolean', description: 'Skip bare-name disambiguation' }, + source_id: { type: 'string', description: 'Source to traverse. Defaults to ctx.sourceId; federated clients with multiple granted sources must specify one.' }, }, scope: 'read', handler: async (ctx, p) => { @@ -3951,14 +4029,17 @@ const code_flow: Operation = { const depth = Math.min((p.depth as number) ?? 8, 12); const max_nodes = Math.min((p.max_nodes as number) ?? 200, 200); const exact = (p.exact as boolean) ?? false; + // Single trust+grant resolver (see code_blast). + const { sourceId: scopedSourceId } = resolveCodeIntelScope(ctx, typeof p.source_id === 'string' ? p.source_id : undefined); + const sourceId = scopedSourceId ?? ctx.sourceId; return getCachedOrCompute( ctx.engine, - { symbol_qualified: symbol + ':flow', depth, source_id: ctx.sourceId }, + { symbol_qualified: symbol + ':flow', depth, source_id: sourceId }, () => runRecursiveWalk(ctx.engine, symbol, { direction: 'callees', depth, maxNodes: max_nodes, - sourceId: ctx.sourceId, + sourceId, exact, }), ); @@ -3979,6 +4060,9 @@ const code_traversal_cache_clear: Operation = { scope: 'admin', localOnly: true, handler: async (ctx, p) => { + // INTENTIONAL exemption from resolveRequestedScope: this is a localOnly + // admin/destructive op with its own D8 all_sources guard. The read-side + // trust+grant resolver does not apply here (no remote caller reaches it). const { clearTraversalCache } = await import('./code-intel/traversal-cache.ts'); const sourceId = (p.source_id as string | undefined) ?? ctx.sourceId; const allSources = (p.all_sources as boolean) ?? false; @@ -4011,7 +4095,7 @@ const search_by_image: Operation = { query: { type: 'string', description: 'Optional text refinement; runs hybrid intersect via D13 weighted RRF.' }, limit: { type: 'number', description: 'Max results (default 20)' }, offset: { type: 'number', description: 'Skip first N results (for pagination)' }, - source_id: { type: 'string', description: "Scope to a single source. Defaults to ctx.sourceId. '__all__' opts out." }, + source_id: { type: 'string', description: "Scope to a single source. Defaults to ctx.sourceId. '__all__' spans every source for trusted local callers, your granted sources for remote callers." }, }, scope: 'read', // NOT localOnly: remote MCP callers can pass image_url or image_data @@ -4061,13 +4145,12 @@ const search_by_image: Operation = { { maxBytes: cap }, ); - // Resolve source-scope (D5 canonical thread). - const resolvedSourceId = - sourceIdParam !== undefined - ? sourceIdParam === '__all__' - ? undefined - : sourceIdParam - : ctx.sourceId; + // Resolve source-scope through the single trust+grant resolver. Pre-fix + // this branch computed resolvedSourceId then spread sourceScopeOpts(ctx) + // after it (double-application: the spread silently won, and `__all__` + // didn't opt out for local callers with ctx.sourceId set). One resolver, + // one spread — `__all__` spans the brain only for trusted local callers. + const imageSourceScope = resolveRequestedScope(ctx, sourceIdParam); const { searchByImage } = await import('./search/by-image.ts'); const results = await searchByImage( @@ -4077,8 +4160,7 @@ const search_by_image: Operation = { limit: (p.limit as number) || 20, offset: (p.offset as number) || 0, query: queryRefinement, - sourceId: resolvedSourceId, - ...sourceScopeOpts(ctx), + ...imageSourceScope, }, ); diff --git a/src/core/pglite-engine.ts b/src/core/pglite-engine.ts index ca727cd5f..0a94630e6 100644 --- a/src/core/pglite-engine.ts +++ b/src/core/pglite-engine.ts @@ -830,13 +830,19 @@ export class PGLiteEngine implements BrainEngine { } // Pages CRUD - async getPage(slug: string, opts?: { sourceId?: string; includeDeleted?: boolean }): Promise { + async getPage(slug: string, opts?: { sourceId?: string; sourceIds?: string[]; includeDeleted?: boolean }): Promise { // v0.26.5: hide soft-deleted by default; opt-in via opts.includeDeleted. const includeDeleted = opts?.includeDeleted === true; const sourceId = opts?.sourceId; + const sourceIds = opts?.sourceIds; const where: string[] = ['slug = $1']; const params: unknown[] = [slug]; - if (sourceId) { + // #1393: federated grant (sourceIds[]) wins over scalar sourceId so the + // exact-match read honors allowedSources, not just one source. + if (sourceIds && sourceIds.length > 0) { + params.push(sourceIds); + where.push(`source_id = ANY($${params.length}::text[])`); + } else if (sourceId) { params.push(sourceId); where.push(`source_id = $${params.length}`); } diff --git a/src/core/postgres-engine.ts b/src/core/postgres-engine.ts index ba0218a61..0c130cd32 100644 --- a/src/core/postgres-engine.ts +++ b/src/core/postgres-engine.ts @@ -895,13 +895,21 @@ export class PostgresEngine implements BrainEngine { } // Pages CRUD - async getPage(slug: string, opts?: { sourceId?: string; includeDeleted?: boolean }): Promise { + async getPage(slug: string, opts?: { sourceId?: string; sourceIds?: string[]; includeDeleted?: boolean }): Promise { const sql = this.sql; const includeDeleted = opts?.includeDeleted === true; const sourceId = opts?.sourceId; - // v0.26.5: default hides soft-deleted rows. Compose with optional sourceId + const sourceIds = opts?.sourceIds; + // v0.26.5: default hides soft-deleted rows. Compose with optional source // filter via fragment chaining (postgres.js supports sql`` composition). - const sourceCondition = sourceId ? sql`AND source_id = ${sourceId}` : sql``; + // #1393: a federated grant (sourceIds[]) takes precedence over scalar + // sourceId so the exact-match read honors allowedSources, not just one source. + const sourceCondition = + sourceIds && sourceIds.length > 0 + ? sql`AND source_id = ANY(${sourceIds}::text[])` + : sourceId + ? sql`AND source_id = ${sourceId}` + : sql``; const deletedCondition = includeDeleted ? sql`` : sql`AND deleted_at IS NULL`; const rows = await sql` SELECT id, source_id, slug, type, title, compiled_truth, timeline, frontmatter, content_hash, created_at, updated_at, deleted_at, @@ -4847,9 +4855,26 @@ export class PostgresEngine implements BrainEngine { // Config async getConfig(key: string): Promise { - const sql = this.sql; - const rows = await sql`SELECT value FROM config WHERE key = ${key}`; - return rows.length > 0 ? (rows[0].value as string) : null; + // #1603: a transient pooler drop on this read used to throw / fall through + // to defaults silently — which on remote Postgres surfaces as the wrong + // search mode/knobs and empty-stdout queries. Retry-with-reconnect using the + // same tuned opts as the bulk writers. No auditSite: this is a single-row + // read, not a bulk write, so it must not emit batch-retry audit rows. + // `this.sql` is a getter, so each attempt sees the pool rebuilt by reconnect. + const opts = this.getBulkRetryOpts(); + return withRetry( + async () => { + const rows = await this.sql`SELECT value FROM config WHERE key = ${key}`; + return rows.length > 0 ? (rows[0].value as string) : null; + }, + { + maxRetries: opts.maxRetries, + delayMs: opts.delayMs, + delayMaxMs: opts.delayMaxMs, + jitter: BULK_RETRY_OPTS.jitter, + reconnect: (ctx) => this.reconnect(ctx), + }, + ); } async setConfig(key: string, value: string): Promise { diff --git a/src/core/skill-catalog.ts b/src/core/skill-catalog.ts index 237283521..31d6fe9e0 100644 --- a/src/core/skill-catalog.ts +++ b/src/core/skill-catalog.ts @@ -326,12 +326,48 @@ function availableBrainTools(ctx: OperationContext): string[] { // Frontmatter projection + description // --------------------------------------------------------------------------- -/** Parse a single-line `description:` from raw frontmatter (best-effort). */ +/** + * Parse a `description:` from raw frontmatter (best-effort). + * + * #1711: handles YAML block scalars. `description: |` (literal) and + * `description: >` (folded), with optional chomping/indent indicators + * (`|-`, `>+`, `|2`), are parsed by reading the following indented lines and + * folding them into a single line for the one-line catalog. Pre-fix the regex + * captured the bare `|`/`>` indicator as the description, so block-scalar skills + * showed a literal "|" in the catalog instead of their text. + */ function parseDescriptionField(raw: string): string | undefined { - const m = raw.match(/^description:\s*["']?(.+?)["']?\s*$/m); - if (!m) return undefined; - const v = m[1].trim(); - return v.length > 0 ? v : undefined; + const lines = raw.split('\n'); + for (let i = 0; i < lines.length; i++) { + const m = lines[i].match(/^description:[ \t]*(.*)$/); + if (!m) continue; + const inline = m[1].trim(); + + // Block scalar indicator (`|`, `>`, with optional chomp `+`/`-` and indent digit). + if (/^[|>][+-]?\d*$/.test(inline)) { + const block: string[] = []; + let baseIndent: number | null = null; + for (let j = i + 1; j < lines.length; j++) { + const line = lines[j]; + if (line.trim().length === 0) { block.push(''); continue; } + const indent = line.length - line.trimStart().length; + if (baseIndent === null) { + if (indent === 0) break; // no indented continuation + baseIndent = indent; + } + if (indent < baseIndent) break; // dedent ends the block + block.push(line.slice(baseIndent)); + } + // Catalog descriptions are one line; collapse literal/folded whitespace. + const folded = block.join(' ').replace(/\s+/g, ' ').trim(); + return folded.length > 0 ? folded : undefined; + } + + // Inline scalar: strip a matching pair of surrounding quotes. + const unq = inline.replace(/^(['"])([\s\S]*)\1$/, '$2').trim(); + return unq.length > 0 ? unq : undefined; + } + return undefined; } /** Strip the leading `---\n...\n---` fence; return the prose body. */ diff --git a/src/core/types.ts b/src/core/types.ts index 16488b5ff..300e6880b 100644 --- a/src/core/types.ts +++ b/src/core/types.ts @@ -330,6 +330,12 @@ export interface PageFilters { export interface GetPageOpts { /** Filter to a specific source. When omitted, getPage returns the first slug match across sources (pre-existing semantics). */ sourceId?: string; + /** + * Filter to a federated set of sources (the caller's allowedSources grant). + * Takes precedence over `sourceId` when non-empty. Closes the #1393 leak: the + * get_page exact path must honor a federated grant, not just scalar sourceId. + */ + sourceIds?: string[]; /** Include soft-deleted pages. Default false. See PageFilters.includeDeleted. */ includeDeleted?: boolean; } diff --git a/src/mcp/http-transport.ts b/src/mcp/http-transport.ts index 78a13f038..c1ccb3152 100644 --- a/src/mcp/http-transport.ts +++ b/src/mcp/http-transport.ts @@ -29,6 +29,7 @@ import { createHash } from 'crypto'; import type { BrainEngine } from '../core/engine.ts'; import { buildToolDefs } from './tool-defs.ts'; import { operations } from '../core/operations.ts'; +import type { AuthInfo } from '../core/operations.ts'; import { VERSION } from '../version.ts'; import { dispatchToolCall } from './dispatch.ts'; import { buildDefaultLimiters, type RateLimiter } from './rate-limit.ts'; @@ -74,6 +75,36 @@ interface AuthResult { * for narrower scoping. */ sourceId?: string; + /** + * #1336: AuthInfo carrying the legacy token's stored federated_read grant + * (`permissions.source_id` array). Threaded so `sourceScopeOpts` can scope + * read ops to the operator-granted sources instead of just scalar `sourceId`. + * Bounded to the stored grant — never widened to "all". + */ + auth?: AuthInfo; +} + +/** + * #1336: derive a legacy bearer token's source scope from its stored + * `permissions.source_id`. An ARRAY value is a federated_read grant → + * `allowedSources` (scoped reads across exactly those sources). A STRING value + * scopes the scalar floor. Anything else → 'default' (preserves pre-v0.34 + * behavior). NEVER widened to "all": an empty/garbage value keeps the 'default' + * floor and no federated grant. + */ +export function parseLegacyTokenScope(rawSource: unknown): { sourceId: string; allowedSources?: string[] } { + if (Array.isArray(rawSource)) { + const allowedSources = (rawSource as unknown[]).filter(s => typeof s === 'string' && s.length > 0) as string[]; + if (allowedSources.length > 0) { + // Scalar floor: the first granted source (write authority); reads span the array. + return { sourceId: allowedSources[0], allowedSources }; + } + return { sourceId: 'default' }; + } + if (typeof rawSource === 'string' && rawSource.length > 0) { + return { sourceId: rawSource }; + } + return { sourceId: 'default' }; } /** Read up to `cap` bytes off req.body. Returns null if cap exceeded. */ @@ -193,21 +224,29 @@ export async function startHttpTransport(opts: HttpTransportOptions) { .catch(() => { /* fire-and-forget */ }); // v0.28: extract per-token takes-holder allow-list. Fail-safe default // is ['world'] — a token with no permissions row sees public claims only. - const perms = (row as { permissions?: { takes_holders?: unknown } }).permissions; + const perms = (row as { permissions?: { takes_holders?: unknown; source_id?: unknown } }).permissions; const allowList = Array.isArray(perms?.takes_holders) ? (perms!.takes_holders as unknown[]).filter(h => typeof h === 'string') as string[] : ['world']; + // #1336: honor the operator-set source grant stored on the token. + const { sourceId, allowedSources } = parseLegacyTokenScope(perms?.source_id); + const auth: AuthInfo = { + token, + clientId: rowId, + clientName: rowName, + scopes: [], + sourceId, + ...(allowedSources ? { allowedSources } : {}), + }; return { ok: true, tokenId: rowId, tokenName: rowName, takesHoldersAllowList: allowList, // v0.34.1 (#861, D13): legacy bearer tokens default to 'default' - // source. Preserves the pre-v0.34 effective behavior of the - // serve-http fallback chain that was removed for OAuth clients - // (migration v60 backfills oauth_clients.source_id). This path - // is for the older v0.22.7 access_tokens transport. - sourceId: 'default', + // source unless the token carries an explicit grant (#1336 above). + sourceId, + auth, }; } catch { return { ok: false }; @@ -362,6 +401,9 @@ export async function startHttpTransport(opts: HttpTransportOptions) { remote: true, takesHoldersAllowList: auth.takesHoldersAllowList, sourceId: auth.sourceId, + // #1336: thread the token's federated_read grant so read ops scope + // to the operator-granted sources via sourceScopeOpts. + auth: auth.auth, }); const status = result.isError ? 'error' : 'success'; logRequest(auth.tokenName!, `tools/call:${toolName}`, status, Date.now() - startedMs); diff --git a/test/frontmatter-install-hook.test.ts b/test/frontmatter-install-hook.test.ts index 4f11177a0..baf928fe8 100644 --- a/test/frontmatter-install-hook.test.ts +++ b/test/frontmatter-install-hook.test.ts @@ -53,6 +53,25 @@ describe('frontmatter install-hook (B13)', () => { } }); + test('#1840 — generated hook matches .md/.mdx (single-backslash regex, not over-escaped)', () => { + installHook(tmp, false); + const content = readFileSync(join(tmp, '.githooks', 'pre-commit'), 'utf8'); + // The shell must see `grep -E '\.mdx?$'`. Pre-fix it emitted `'\\.mdx?$'` + // (literal backslash), so the hook matched nothing and silently no-opped. + expect(content).toContain("grep -E '\\.mdx?$'"); + expect(content).not.toContain("grep -E '\\\\.mdx?$'"); + + // Prove the emitted pattern actually selects markdown files. Extract the + // exact pattern between the single quotes and run it through ripgrep-free + // JS regex parity (POSIX ERE `\.mdx?$` == JS `/\.mdx?$/`). + const m = content.match(/grep -E '([^']+)'/); + expect(m).not.toBeNull(); + const re = new RegExp(m![1]); + expect(re.test('notes/thing.md')).toBe(true); + expect(re.test('notes/thing.mdx')).toBe(true); + expect(re.test('notes/thing.txt')).toBe(false); + }); + test('installHook refuses to clobber existing hook without --force', () => { const hooksDir = join(tmp, '.githooks'); mkdirSync(hooksDir, { recursive: true }); diff --git a/test/frontmatter-non-string-fields.test.ts b/test/frontmatter-non-string-fields.test.ts new file mode 100644 index 000000000..2370f41bc --- /dev/null +++ b/test/frontmatter-non-string-fields.test.ts @@ -0,0 +1,83 @@ +/** + * Non-string frontmatter title/type/slug (#1883, #1658, #1556, #1948). + * + * A YAML scalar like `title: 123` parses to a NUMBER. Pre-fix the parser cast it + * `as string`, so a non-string flowed downstream typed as string and crashed the + * first `.toLowerCase()` in content-sanity — aborting the whole lint/sync run + * brain-wide (root trigger behind the never-converging-sync reports #1794/#1939). + * + * Fix: coerce title to a string at the parser; for slug/type fall back to + * inference (never fabricate a "123" slug); guard content-sanity defensively; + * and surface the malformed frontmatter via a lint NON_STRING_FIELD finding. + */ +import { describe, test, expect } from 'bun:test'; +import { parseMarkdown } from '../src/core/markdown.ts'; +import { assessContentSanity } from '../src/core/content-sanity.ts'; + +const fm = (body: string) => `---\n${body}\n---\nbody text here\n`; + +describe('parseMarkdown coerces/guards non-string frontmatter', () => { + test('numeric title is coerced to a string (intent preserved)', () => { + const p = parseMarkdown(fm('title: 123'), 'notes/thing.md'); + expect(typeof p.title).toBe('string'); + expect(p.title).toBe('123'); + }); + + test('boolean title is coerced to a string', () => { + const p = parseMarkdown(fm('title: false'), 'notes/thing.md'); + expect(p.title).toBe('false'); + }); + + test('missing title falls back to inferred title (string)', () => { + const p = parseMarkdown(fm('type: note'), 'notes/My Thing.md'); + expect(typeof p.title).toBe('string'); + expect(p.title.length).toBeGreaterThan(0); + }); + + test('non-string slug is coerced to a usable string (date slugs are legitimate)', () => { + // YAML parses `2024-06-01` as a Date; coerceFrontmatterString → "2024-06-01". + const p = parseMarkdown(fm('slug: 2024-06-01'), 'notes/real-slug.md'); + expect(typeof p.slug).toBe('string'); + expect(p.slug).toBe('2024-06-01'); + }); + + test('numeric type is coerced to a string (never crashes downstream)', () => { + const p = parseMarkdown(fm('type: 5\ntitle: ok'), 'notes/thing.md'); + expect(typeof p.type).toBe('string'); + }); + + test('valid string fields pass through unchanged', () => { + const p = parseMarkdown(fm('title: Real Title\ntype: concept\nslug: my-slug'), 'x.md'); + expect(p.title).toBe('Real Title'); + expect(p.type).toBe('concept'); + expect(p.slug).toBe('my-slug'); + }); +}); + +describe('lint surfaces non-string frontmatter (NON_STRING_FIELD)', () => { + test('numeric title produces a NON_STRING_FIELD validation error', () => { + const p = parseMarkdown(fm('title: 123'), 'notes/thing.md', { validate: true }); + const codes = (p.errors ?? []).map(e => e.code); + expect(codes).toContain('NON_STRING_FIELD'); + }); + + test('all-string frontmatter produces no NON_STRING_FIELD error', () => { + const p = parseMarkdown(fm('title: Fine\ntype: note'), 'notes/thing.md', { validate: true }); + const codes = (p.errors ?? []).map(e => e.code); + expect(codes).not.toContain('NON_STRING_FIELD'); + }); +}); + +describe('assessContentSanity never throws on a non-string title (belt-and-suspenders)', () => { + test('numeric title does not crash the sanity pass', () => { + expect(() => + assessContentSanity({ compiled_truth: 'hello world', timeline: '', title: 123 as unknown as string }), + ).not.toThrow(); + }); + + test('undefined title does not crash the sanity pass', () => { + expect(() => + assessContentSanity({ compiled_truth: 'hello world', timeline: '', title: undefined as unknown as string }), + ).not.toThrow(); + }); +}); diff --git a/test/get-page-federated-scope.test.ts b/test/get-page-federated-scope.test.ts new file mode 100644 index 000000000..a21bc89c0 --- /dev/null +++ b/test/get-page-federated-scope.test.ts @@ -0,0 +1,91 @@ +/** + * #1393 — get_page exact-match path honors the federated source grant. + * + * Pre-fix the exact path used scalar `ctx.sourceId` only: + * const sourceOpts = ctx.sourceId ? { sourceId: ctx.sourceId } : {}; + * A remote OAuth client with a federated `allowedSources` grant (and no single + * ctx.sourceId) therefore got an UNSCOPED exact lookup — a cross-source read of + * any page by slug. The fuzzy path was already scoped (#1436); this closes the + * exact path by (a) routing it through sourceScopeOpts and (b) teaching + * engine.getPage to honor a `sourceIds[]` array (both engines). + */ +import { describe, test, expect, beforeAll, afterAll, beforeEach } from 'bun:test'; +import { PGLiteEngine } from '../src/core/pglite-engine.ts'; +import { resetPgliteState } from './helpers/reset-pglite.ts'; +import { operations, OperationError, type OperationContext } from '../src/core/operations.ts'; + +let engine: PGLiteEngine; +const get_page = operations.find(o => o.name === 'get_page')!; + +function ctxOf(overrides: Partial = {}): OperationContext { + return { + engine: engine as any, + config: {} as any, + logger: console as any, + dryRun: false, + remote: true, + sourceId: 'default', + ...overrides, + }; +} + +beforeAll(async () => { + engine = new PGLiteEngine(); + await engine.connect({}); + await engine.initSchema(); +}, 60_000); + +afterAll(async () => { + if (engine) await engine.disconnect(); +}, 60_000); + +beforeEach(async () => { + await resetPgliteState(engine); + await engine.executeRaw(`INSERT INTO sources (id, name, local_path) VALUES ('alpha', 'alpha', '/tmp/alpha') ON CONFLICT (id) DO NOTHING`); + await engine.executeRaw(`INSERT INTO sources (id, name, local_path) VALUES ('beta', 'beta', '/tmp/beta') ON CONFLICT (id) DO NOTHING`); + // Distinct slugs per source so an exact lookup can leak across the boundary. + await engine.putPage('secret/beta-doc', { + type: 'note', title: 'Beta secret', compiled_truth: 'beta-only content', frontmatter: {}, + }, { sourceId: 'beta' }); + await engine.putPage('shared/alpha-doc', { + type: 'note', title: 'Alpha doc', compiled_truth: 'alpha content', frontmatter: {}, + }, { sourceId: 'alpha' }); +}); + +describe('engine.getPage honors sourceIds[] (federated grant)', () => { + test('sourceIds[] matching the page returns it', async () => { + const page = await engine.getPage('secret/beta-doc', { sourceIds: ['alpha', 'beta'] }); + expect(page?.title).toBe('Beta secret'); + }); + + test('sourceIds[] NOT containing the page returns null', async () => { + const page = await engine.getPage('secret/beta-doc', { sourceIds: ['alpha'] }); + expect(page).toBeNull(); + }); + + test('sourceIds[] takes precedence over scalar sourceId', async () => { + // scalar says alpha, array says beta-only — array wins, page found. + const page = await engine.getPage('secret/beta-doc', { sourceId: 'alpha', sourceIds: ['beta'] }); + expect(page?.title).toBe('Beta secret'); + }); +}); + +describe('get_page handler closes the cross-source exact-read leak', () => { + test('remote client granted only [alpha] CANNOT read a beta-only slug', async () => { + const ctx = ctxOf({ remote: true, auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['alpha'] } as any }); + // Pre-fix this returned the beta page (leak). Now it is scoped out → 404. + await expect(get_page.handler(ctx, { slug: 'secret/beta-doc' })).rejects.toBeInstanceOf(OperationError); + }); + + test('remote client granted [alpha, beta] CAN read the beta slug', async () => { + const ctx = ctxOf({ remote: true, auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['alpha', 'beta'] } as any }); + const page: any = await get_page.handler(ctx, { slug: 'secret/beta-doc' }); + expect(page.title).toBe('Beta secret'); + }); + + test('remote client granted only [alpha] CAN read its own alpha slug', async () => { + const ctx = ctxOf({ remote: true, auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['alpha'] } as any }); + const page: any = await get_page.handler(ctx, { slug: 'shared/alpha-doc' }); + expect(page.title).toBe('Alpha doc'); + }); +}); diff --git a/test/legacy-token-federated-scope.test.ts b/test/legacy-token-federated-scope.test.ts new file mode 100644 index 000000000..3cf1547b9 --- /dev/null +++ b/test/legacy-token-federated-scope.test.ts @@ -0,0 +1,45 @@ +/** + * #1336 — legacy bearer tokens honor their stored federated_read grant. + * + * Pre-fix the legacy access_tokens path hardcoded `sourceId: 'default'` and never + * populated `allowedSources`, so a token whose `permissions.source_id` granted + * multiple sources could not read across them (and MCP reads silently returned + * empty in non-default brains). The grant is now parsed and threaded. + * + * Bounded by design: never widened to "all" — an empty/garbage value keeps the + * 'default' floor and no federated grant. + */ +import { describe, test, expect } from 'bun:test'; +import { parseLegacyTokenScope } from '../src/mcp/http-transport.ts'; + +describe('parseLegacyTokenScope', () => { + test('array grant → allowedSources (federated read) with first as scalar floor', () => { + expect(parseLegacyTokenScope(['dept-x', 'shared'])).toEqual({ sourceId: 'dept-x', allowedSources: ['dept-x', 'shared'] }); + }); + + test('single-element array → that source as both floor and grant', () => { + expect(parseLegacyTokenScope(['only'])).toEqual({ sourceId: 'only', allowedSources: ['only'] }); + }); + + test('string grant → scalar source, no federated array', () => { + expect(parseLegacyTokenScope('team-a')).toEqual({ sourceId: 'team-a' }); + }); + + test('absent grant → default floor, never widened', () => { + expect(parseLegacyTokenScope(undefined)).toEqual({ sourceId: 'default' }); + expect(parseLegacyTokenScope(null)).toEqual({ sourceId: 'default' }); + }); + + test('empty array → default floor, no grant (NOT "all")', () => { + expect(parseLegacyTokenScope([])).toEqual({ sourceId: 'default' }); + }); + + test('garbage (number / empty string) → default floor', () => { + expect(parseLegacyTokenScope(123)).toEqual({ sourceId: 'default' }); + expect(parseLegacyTokenScope('')).toEqual({ sourceId: 'default' }); + }); + + test('array with non-string junk is filtered to valid sources', () => { + expect(parseLegacyTokenScope(['a', 5, '', 'b'])).toEqual({ sourceId: 'a', allowedSources: ['a', 'b'] }); + }); +}); diff --git a/test/skill-catalog-block-scalar.test.ts b/test/skill-catalog-block-scalar.test.ts new file mode 100644 index 000000000..4f53ebc85 --- /dev/null +++ b/test/skill-catalog-block-scalar.test.ts @@ -0,0 +1,49 @@ +/** + * #1711 — skill catalog parses YAML block-scalar `description:` fields. + * + * Pre-fix `parseDescriptionField` matched `description: |` with a greedy regex + * and captured the bare block indicator (`|` / `>`) as the description, so a + * skill written with `description: |` showed a literal "|" in the catalog + * instead of its text. + */ +import { describe, test, expect } from 'bun:test'; +import { oneLineDescription } from '../src/core/skill-catalog.ts'; + +describe('oneLineDescription — block scalars', () => { + test('literal block scalar (|) folds indented lines into the description', () => { + const raw = ['name: demo', 'description: |', ' First line of the description.', ' Second line continues it.'].join('\n'); + const out = oneLineDescription(raw, 'body fallback'); + expect(out).toBe('First line of the description. Second line continues it.'); + expect(out).not.toContain('|'); + }); + + test('folded block scalar (>) is parsed too', () => { + const raw = ['description: >', ' Folded description', ' across two lines.'].join('\n'); + expect(oneLineDescription(raw, 'fallback')).toBe('Folded description across two lines.'); + }); + + test('chomping/indent indicators (|-, >+, |2) are recognized', () => { + const raw = ['description: |-', ' Trimmed block scalar.'].join('\n'); + expect(oneLineDescription(raw, 'fallback')).toBe('Trimmed block scalar.'); + }); + + test('block scalar with no indented continuation falls back to body prose', () => { + const raw = ['description: |', 'name: next-key'].join('\n'); + expect(oneLineDescription(raw, 'Body prose line')).toBe('Body prose line'); + }); +}); + +describe('oneLineDescription — inline scalars still work', () => { + test('plain inline description', () => { + expect(oneLineDescription('description: A plain one-liner', 'fallback')).toBe('A plain one-liner'); + }); + + test('quoted inline description strips surrounding quotes', () => { + expect(oneLineDescription('description: "Quoted desc"', 'fallback')).toBe('Quoted desc'); + expect(oneLineDescription("description: 'Single quoted'", 'fallback')).toBe('Single quoted'); + }); + + test('absent description falls back to first prose line', () => { + expect(oneLineDescription('name: x', 'The first prose line.')).toBe('The first prose line.'); + }); +}); diff --git a/test/source-scope-resolver.test.ts b/test/source-scope-resolver.test.ts new file mode 100644 index 000000000..e5b9d3671 --- /dev/null +++ b/test/source-scope-resolver.test.ts @@ -0,0 +1,123 @@ +/** + * Source-isolation trust+grant resolver (#1924, #1371, #1393). + * + * The cross-source leak class: a remote OAuth client scoped to one source could + * pass `source_id: "__all__"` (or an explicit out-of-grant source_id) to read + * sources it was never granted. Every source-scoped read op now routes through + * ONE resolver. These tests pin the trust+grant matrix at the unit level so a + * future per-handler "optimization" that re-inlines the `__all__` branch fails + * loudly here. + */ +import { describe, test, expect } from 'bun:test'; +import { + resolveRequestedScope, + resolveCodeIntelScope, + OperationError, + type OperationContext, +} from '../src/core/operations.ts'; + +function ctxOf(overrides: Partial = {}): OperationContext { + return { + engine: {} as any, + config: {} as any, + logger: console as any, + dryRun: false, + remote: true, + sourceId: 'default', + ...overrides, + }; +} + +describe('resolveRequestedScope — __all__ / all_sources', () => { + test('trusted local + __all__ spans every source (empty scope)', () => { + const scope = resolveRequestedScope(ctxOf({ remote: false, sourceId: 'a' }), '__all__'); + expect(scope).toEqual({}); + }); + + test('remote + __all__ collapses to the caller grant, NOT the whole brain', () => { + const ctx = ctxOf({ remote: true, sourceId: 'a', auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['a', 'b'] } as any }); + const scope = resolveRequestedScope(ctx, '__all__'); + expect(scope).toEqual({ sourceIds: ['a', 'b'] }); + }); + + test('remote + __all__ with single-source grant scopes to that one source', () => { + const ctx = ctxOf({ remote: true, sourceId: 'a', auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['a'] } as any }); + expect(resolveRequestedScope(ctx, '__all__')).toEqual({ sourceIds: ['a'] }); + }); + + test('remote + __all__ with no federated grant falls back to scalar sourceId (never empty)', () => { + const ctx = ctxOf({ remote: true, sourceId: 'a' }); + expect(resolveRequestedScope(ctx, '__all__')).toEqual({ sourceId: 'a' }); + }); + + test('all_sources=true is treated identically to __all__', () => { + const ctx = ctxOf({ remote: true, auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['x'] } as any }); + expect(resolveRequestedScope(ctx, undefined, true)).toEqual({ sourceIds: ['x'] }); + }); +}); + +describe('resolveRequestedScope — explicit source_id', () => { + test('remote + explicit source_id OUTSIDE the grant is rejected', () => { + const ctx = ctxOf({ remote: true, auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['a'] } as any }); + expect(() => resolveRequestedScope(ctx, 'b')).toThrow(OperationError); + try { + resolveRequestedScope(ctx, 'b'); + } catch (e) { + expect((e as OperationError).code).toBe('permission_denied'); + } + }); + + test('remote + explicit source_id INSIDE the grant is allowed', () => { + const ctx = ctxOf({ remote: true, auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['a', 'b'] } as any }); + expect(resolveRequestedScope(ctx, 'b')).toEqual({ sourceId: 'b' }); + }); + + test('trusted local + explicit source_id is allowed even with no grant', () => { + expect(resolveRequestedScope(ctxOf({ remote: false }), 'anything')).toEqual({ sourceId: 'anything' }); + }); + + test('remote with no federated grant array can pass an explicit source_id (scalar-floor model)', () => { + // allowedSources undefined → no federated restriction to enforce; the scalar + // sourceId path governs. (Empty [] is treated the same as undefined.) + expect(resolveRequestedScope(ctxOf({ remote: true }), 'z')).toEqual({ sourceId: 'z' }); + const emptyGrant = ctxOf({ remote: true, auth: { token: 't', clientId: 'c', scopes: [], allowedSources: [] } as any }); + expect(resolveRequestedScope(emptyGrant, 'z')).toEqual({ sourceId: 'z' }); + }); +}); + +describe('resolveRequestedScope — default (no param)', () => { + test('falls back to the canonical sourceScopeOpts ladder (federated array wins)', () => { + const ctx = ctxOf({ remote: true, sourceId: 'a', auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['a', 'b'] } as any }); + expect(resolveRequestedScope(ctx, undefined)).toEqual({ sourceIds: ['a', 'b'] }); + }); + + test('falls back to scalar sourceId when no federated grant', () => { + expect(resolveRequestedScope(ctxOf({ remote: true, sourceId: 'a' }), undefined)).toEqual({ sourceId: 'a' }); + }); +}); + +describe('resolveCodeIntelScope — single-source code traversal', () => { + test('scalar sourceId → that source, allSources false', () => { + expect(resolveCodeIntelScope(ctxOf({ remote: true, sourceId: 'a' }), undefined)).toEqual({ allSources: false, sourceId: 'a' }); + }); + + test('single-element federated grant → that one source', () => { + const ctx = ctxOf({ remote: true, auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['only'] } as any }); + expect(resolveCodeIntelScope(ctx, '__all__')).toEqual({ allSources: false, sourceId: 'only' }); + }); + + test('multi-source federated grant → rejected (must specify one)', () => { + const ctx = ctxOf({ remote: true, sourceId: 'a', auth: { token: 't', clientId: 'c', scopes: [], allowedSources: ['a', 'b'] } as any }); + expect(() => resolveCodeIntelScope(ctx, '__all__')).toThrow(OperationError); + }); + + test('trusted local + all → allSources true (spans the brain)', () => { + // ctx.sourceId is empty so the resolver yields {} → trusted-local allSources. + expect(resolveCodeIntelScope(ctxOf({ remote: false, sourceId: '' }), '__all__')).toEqual({ allSources: true, sourceId: undefined }); + }); + + test('remote with no source in scope is denied, never widened to all', () => { + const ctx = ctxOf({ remote: true, sourceId: '' }); + expect(() => resolveCodeIntelScope(ctx, '__all__')).toThrow(OperationError); + }); +});