From 9f3f5b43bddeeb3eefa764b4e76e2074071a0bb4 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sun, 12 Jul 2026 09:03:06 +0300 Subject: [PATCH] fix(search): F1 term-cap stack-depth guard + restore/extend coverage, drop dead code (#529) F1: cap parsed terms at MAX_PARSED_TERMS (64) in parseSearchQuery so a huge pasted query no longer nests the combined tsquery deep enough to blow Postgres' stack depth limit (HTTP 500); +@MaxLength(10000) on SearchDTO.query as defense-in-depth. F2/F3: restore titleOnly text_content leak-guard and parentPageId subtree-scoping coverage in the lexical int-spec. F4: positive ancestor-path ordering + non-empty snippet asserts. F5: remove dead SearchLookupTier enum, unused buildTsQuery + pg-tsquery require (and its tests), and the discarded parentPageId select in fetchDetails. F6: rewrite the stale #443 DTO comment (parentPageId/titleOnly read by the engine; substring ignored). Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/server/src/core/search/dto/search.dto.ts | 18 ++- .../core/search/search-query-parser.spec.ts | 55 ++++++- .../src/core/search/search-query-parser.ts | 18 +++ .../src/core/search/search.service.spec.ts | 58 +------ apps/server/src/core/search/search.service.ts | 29 ---- .../integration/search-lexical.int-spec.ts | 148 ++++++++++++++++++ 6 files changed, 235 insertions(+), 91 deletions(-) diff --git a/apps/server/src/core/search/dto/search.dto.ts b/apps/server/src/core/search/dto/search.dto.ts index 945c1dd4..65441567 100644 --- a/apps/server/src/core/search/dto/search.dto.ts +++ b/apps/server/src/core/search/dto/search.dto.ts @@ -5,11 +5,17 @@ import { IsNumber, IsOptional, IsString, + MaxLength, } from 'class-validator'; export class SearchDTO { + // Defense-in-depth cap on the raw query length. The real stack-depth bound is + // the parser's MAX_PARSED_TERMS term cap (see search-query-parser.ts); this + // just rejects absurd payloads early. 10k chars still comfortably holds any + // legitimate query. @IsNotEmpty() @IsString() + @MaxLength(10000) query: string; // #529 A3 — match mode. `auto` (default) routes identifier-like terms @@ -41,15 +47,19 @@ export class SearchDTO { // --- Opt-in agent-lookup mode (#443). ------------------------------------ // These fields are ADDITIVE and default-off: a web client that sends none of - // them gets byte-identical FTS behaviour and result shape. They are only read - // by the substring/path/snippet code path in SearchService.searchPage. + // them gets byte-identical FTS behaviour and result shape. In the unified #529 + // engine, `parentPageId` and `titleOnly` are read by SearchService.searchPage + // (subtree scoping and title-only matching, respectively). `substring` is NOT + // read by the native driver — it is accepted-but-ignored, kept only for + // back-compat with the upstream lookup request shape. // // NOTE (standalone stdio vs stock upstream): stock upstream validates this DTO // with `whitelist: true`, so an older server silently strips these unknown // fields and the request degrades gracefully to the plain FTS behaviour. - // Enables the hybrid substring branch (title + text_content LIKE) merged with - // the existing FTS branch, plus tiered ranking, path and windowed snippet. + // Accepted-but-ignored by the #529 native driver (kept for upstream lookup + // back-compat). The unified engine ALWAYS runs the hybrid FTS + substring/ + // trigram branches with tiered ranking, so this flag no longer toggles anything. @IsOptional() @IsBoolean() substring?: boolean; diff --git a/apps/server/src/core/search/search-query-parser.spec.ts b/apps/server/src/core/search/search-query-parser.spec.ts index 098be4e2..23b5a353 100644 --- a/apps/server/src/core/search/search-query-parser.spec.ts +++ b/apps/server/src/core/search/search-query-parser.spec.ts @@ -1,4 +1,8 @@ -import { parseSearchQuery, hasPositiveRecall } from './search-query-parser'; +import { + parseSearchQuery, + hasPositiveRecall, + MAX_PARSED_TERMS, +} from './search-query-parser'; describe('parseSearchQuery — tokenization & operators (A2)', () => { it('splits on whitespace into positive terms (OR recall)', () => { @@ -113,3 +117,52 @@ describe('parseSearchQuery — reasons & recall', () => { expect(parseSearchQuery('a b').mode).toBe('or'); }); }); + +describe('parseSearchQuery — term cap (stack-depth guard)', () => { + it('caps the total parsed terms at MAX_PARSED_TERMS without throwing', () => { + // A pasted text block: far more words than the cap. The parser must bound the + // SQL tsquery nesting depth (else Postgres blows its stack → HTTP 500). + const words = Array.from({ length: 5000 }, (_, i) => `w${i}`); + let p!: ReturnType; + expect(() => { + p = parseSearchQuery(words.join(' ')); + }).not.toThrow(); + const total = p.positive.length + p.required.length + p.excluded.length; + expect(total).toBe(MAX_PARSED_TERMS); + // Stable order: the FIRST cap terms are kept. + expect(p.positive.slice(0, 3).map((t) => t.text)).toEqual(['w0', 'w1', 'w2']); + expect(p.positive[MAX_PARSED_TERMS - 1].text).toBe(`w${MAX_PARSED_TERMS - 1}`); + }); + + it('counts positive + required + excluded together toward the cap', () => { + // Interleave operators so overflow can fall on any bucket. Total must still + // never exceed the cap, and the first cap terms (in stable order) win. + const tokens: string[] = []; + for (let i = 0; i < 200; i++) { + const op = i % 3 === 0 ? '+' : i % 3 === 1 ? '-' : ''; + tokens.push(`${op}t${i}`); + } + const p = parseSearchQuery(tokens.join(' ')); + const total = p.positive.length + p.required.length + p.excluded.length; + expect(total).toBe(MAX_PARSED_TERMS); + // t0 (+, required) and t1 (-, excluded) and t2 (bare, positive) are all within + // the first cap tokens → each bucket got its leading terms. + expect(p.required[0].text).toBe('t0'); + expect(p.excluded[0].text).toBe('t1'); + expect(p.positive[0].text).toBe('t2'); + }); + + it('leaves a normal (<= cap) query completely unchanged', () => { + const p = parseSearchQuery('+кофейня -архив "воздушный шар" ресторан 10.31.41'); + expect(p.required.map((t) => t.text)).toEqual(['кофейня']); + expect(p.excluded.map((t) => t.text)).toEqual(['архив']); + expect(p.positive.map((t) => t.text)).toEqual([ + 'воздушный шар', + 'ресторан', + '10.31.41', + ]); + // Phrase / substring branch handling survives under the cap. + expect(p.positive[0].branch).toBe('phrase'); + expect(p.positive[2].branch).toBe('substring'); + }); +}); diff --git a/apps/server/src/core/search/search-query-parser.ts b/apps/server/src/core/search/search-query-parser.ts index 1d325a34..05def6d9 100644 --- a/apps/server/src/core/search/search-query-parser.ts +++ b/apps/server/src/core/search/search-query-parser.ts @@ -24,6 +24,17 @@ export type SearchMatchMode = 'auto' | 'word' | 'prefix' | 'substring'; export type SearchBooleanMode = 'or' | 'and'; +// Hard cap on the total number of parsed terms (positive + required + excluded). +// SearchService folds each FTS term into a LEFT-NESTED SQL tsquery expression +// `(((t1)||(t2))||(t3))…`, embedded several times per query — so nesting depth +// grows with the term count. A pasted text block (thousands of words) would nest +// deep enough to blow Postgres' `stack depth limit` → an ERROR → HTTP 500 for the +// caller. Capping in the parser bounds that depth for EVERY consumer (web / MCP / +// share), since they all route through here. 64 comfortably covers any real query +// while keeping the SQL nesting shallow. Overflow terms (beyond the first 64, in +// stable order) are dropped rather than throwing. +export const MAX_PARSED_TERMS = 64; + // How a single term is matched against the index. // - 'fts' : full-text lexeme, exact (no trailing prefix). // - 'ftsPrefix' : full-text lexeme with a `:*` prefix match. @@ -194,6 +205,13 @@ export function parseSearchQuery( } if (!usableText) continue; + // Stack-depth guard: stop after MAX_PARSED_TERMS surviving terms (positive + + // required + excluded, combined) so the SQL tsquery nesting stays shallow. + // The first 64 terms are kept in stable order; the rest are dropped. + if (positive.length + required.length + excluded.length >= MAX_PARSED_TERMS) { + break; + } + const term: ParsedTerm = { text: usableText, branch }; if (tok.op === '+') required.push(term); diff --git a/apps/server/src/core/search/search.service.spec.ts b/apps/server/src/core/search/search.service.spec.ts index 1def74c0..efd4d2b8 100644 --- a/apps/server/src/core/search/search.service.spec.ts +++ b/apps/server/src/core/search/search.service.spec.ts @@ -1,4 +1,4 @@ -import { SearchService, buildTsQuery } from './search.service'; +import { SearchService } from './search.service'; describe('SearchService', () => { it('should be defined', () => { @@ -99,59 +99,3 @@ describe('SearchService.searchSuggestions — onlyTemplates filter', () => { expect(isTemplateWhereCall(pageBuilder)).toBeUndefined(); }); }); - -// Unit tests for `buildTsQuery` (extracted from search.service.ts). It turns a raw -// user query into a prefix tsquery string fed to `to_tsquery('english', ...)`. -// -// REAL BUG (Gitea #139, item 10): the previous inline `tsquery(query.trim() + '*')` -// let to_tsquery operator characters through, so adversarial inputs could produce a -// fragment that to_tsquery rejects -> 500. The extraction sanitizes the input -// (strip everything but letters/numbers/whitespace) so these inputs degrade to a -// safe, neutral query with NO throw, while normal queries keep working. -describe('buildTsQuery', () => { - it('builds a prefix query for a normal single word', () => { - expect(buildTsQuery('hello')).toBe('hello:*'); - }); - - it('joins multiple words with AND and a trailing prefix match', () => { - expect(buildTsQuery('foo bar')).toBe('foo&bar:*'); - }); - - it('preserves accented and non-Latin words', () => { - expect(buildTsQuery('héllo café')).toBe('héllo&café:*'); - expect(buildTsQuery('日本語')).toBe('日本語:*'); - }); - - it('neutralizes to_tsquery operator inputs without throwing', () => { - // Each of these previously risked an invalid to_tsquery -> 500. They must now - // produce a safe (here empty) query and never throw. - for (const input of ['&', '!', '*', '<->', '\\']) { - expect(() => buildTsQuery(input)).not.toThrow(); - expect(buildTsQuery(input)).toBe(''); - } - }); - - it('handles stopword-only input safely', () => { - // pg-tsquery still tokenizes stopwords; to_tsquery reduces them to nothing. - // The important contract is: no throw, and a deterministic string. - expect(() => buildTsQuery('the a of')).not.toThrow(); - expect(buildTsQuery('the a of')).toBe('the&a&of:*'); - }); - - it('returns empty string for empty / whitespace-only / null-ish input', () => { - expect(buildTsQuery('')).toBe(''); - expect(buildTsQuery(' ')).toBe(''); - expect(buildTsQuery(undefined as unknown as string)).toBe(''); - }); - - it('handles a very long input without throwing', () => { - const long = 'a'.repeat(10000); - expect(() => buildTsQuery(long)).not.toThrow(); - expect(buildTsQuery(long)).toBe(`${long}:*`); - }); - - it('strips punctuation embedded in otherwise valid words', () => { - expect(buildTsQuery('c++ code')).toBe('c&code:*'); - expect(buildTsQuery('a-b-c')).toBe('a&b&c:*'); - }); -}); diff --git a/apps/server/src/core/search/search.service.ts b/apps/server/src/core/search/search.service.ts index 5e866e23..ffd5d72b 100644 --- a/apps/server/src/core/search/search.service.ts +++ b/apps/server/src/core/search/search.service.ts @@ -20,9 +20,6 @@ import { hasPositiveRecall, } from './search-query-parser'; -// eslint-disable-next-line @typescript-eslint/no-require-imports -const tsquery = require('pg-tsquery')(); - // The FTS text-search configuration used on BOTH the stored side (pages.tsv via // its trigger, see migration 20260707T130000) and the query side here. #529 // acceptance #13 invariant: column config and query config always change as a @@ -34,24 +31,6 @@ const TS_CONFIG = 'ru_en'; // substring tier scales never need normalizing — that is the whole point. const RRF_K = 60; -// Legacy prefix-tsquery builder (kept for the /suggest path + back-compat unit -// tests). The #529 engine below no longer uses it — it parses the query into an -// AST instead — but `buildTsQuery` remains exported and behaviour-identical. -// -// Strips everything that is not a letter/number/space BEFORE handing text to -// pg-tsquery so adversarial to_tsquery operators degrade to a neutral query -// instead of a 500. -export function buildTsQuery(raw: string): string { - const cleaned = (raw ?? '') - .normalize('NFC') - .replace(/[^\p{L}\p{N}\s]+/gu, ' ') - .replace(/\s+/g, ' ') - .trim(); - - if (!cleaned) return ''; - return tsquery(cleaned + '*'); -} - // Escape the LIKE metacharacters (`%`, `_`, `\`) so every character is matched // LITERALLY by a `col LIKE '%' || q || '%'` predicate. Without this a query of // `%` or `_` would match every row. @@ -67,13 +46,6 @@ export function escapeLikePattern(raw: string): string { .replace(/_/g, '\\_'); } -// Substring tier (highest first), used to rank the substring branch before RRF. -export enum SearchLookupTier { - TITLE_EXACT = 3, - TITLE_SUBSTRING = 2, - TEXT = 1, -} - // Env-tunable fusion window: the top-N candidates (by RRF) that pagination can // reach. The tail beyond it is unreachable (truncatedAtCap:true). Default 500. function getCandidateCap(): number { @@ -533,7 +505,6 @@ export class SearchService { 'pages.slugId as slugId', 'pages.title as title', 'pages.icon as icon', - 'pages.parentPageId as parentPageId', 'pages.creatorId as creatorId', 'pages.spaceId as spaceId', 'pages.createdAt as createdAt', diff --git a/apps/server/test/integration/search-lexical.int-spec.ts b/apps/server/test/integration/search-lexical.int-spec.ts index fcffc151..b553c6db 100644 --- a/apps/server/test/integration/search-lexical.int-spec.ts +++ b/apps/server/test/integration/search-lexical.int-spec.ts @@ -461,4 +461,152 @@ describe('SearchService #529 lexical overhaul [integration]', () => { // matchedTerms already echoed the required term; still true. expect(hit.matchedTerms).toContain('кофейня'); }); + + // F1 — stack-depth guard: a pasted text block (thousands of FTS terms) used to + // nest the combined tsquery so deep that Postgres raised `stack depth limit + // exceeded` → HTTP 500 for the caller. The parser now caps terms at + // MAX_PARSED_TERMS, so a huge query returns 200 and the leading (in-cap) term + // still matches. Mutation: remove the cap → this reddens (500 / stack depth). + it('F1 a huge multi-term query returns 200, not a stack-depth 500', async () => { + const page = await insertPage({ + title: 'qfonemarker заголовок', + textContent: 'тело страницы', + }); + // Purely-alphabetic filler words → FTS branch (the branch that nests in + // combineTsq). A digit-bearing token would route to substring and not nest. + const alphaWord = (i: number): string => { + let s = ''; + let n = i + 1; + while (n > 0) { + s = String.fromCharCode(97 + (n % 26)) + s; + n = Math.floor(n / 26); + } + return 'q' + s; + }; + const words = [ + 'qfonemarker', + ...Array.from({ length: 5000 }, (_, i) => alphaWord(i)), + ]; + const res = await search(buildService(), { + query: words.join(' '), + spaceId, + }); + // Bounded nesting → no 500; the leading in-cap term still recalls the page. + expect(res.items.map((i: any) => i.id)).toContain(page); + }); + + // F2 — titleOnly leak-guard (restores coverage the deleted lookup spec gave). + // A term present ONLY in text_content must NOT match under titleOnly, and a + // title hit must carry NO body snippet. Uses match:'substring' because titleOnly + // gates the substring branch (the FTS `pages.tsv` already spans title+body). + it('F2 titleOnly does not match text_content and yields no body snippet', async () => { + // Marker lives only in the body, never in the title. + const bodyOnly = await insertPage({ + title: 'нейтральный заголовок f2', + textContent: 'секрет titleonlybody конец', + }); + const res = await search(buildService(), { + query: 'titleonlybody', + match: 'substring', + spaceId, + titleOnly: true, + }); + // titleOnly must NOT leak a body-substring match. + expect(res.items.map((i: any) => i.id)).not.toContain(bodyOnly); + + // Sanity: WITHOUT titleOnly the same term DOES find it via the body. + const resOpen = await search(buildService(), { + query: 'titleonlybody', + match: 'substring', + spaceId, + }); + expect(resOpen.items.map((i: any) => i.id)).toContain(bodyOnly); + + // A page that hits on its TITLE: the snippet must be empty (no body leaks in). + const titleHit = await insertPage({ + title: 'titleonlytitle страница', + textContent: 'какой-то текст тела здесь для сниппета', + }); + const res2 = await search(buildService(), { + query: 'titleonlytitle', + match: 'substring', + spaceId, + titleOnly: true, + }); + const hit = res2.items.find((i: any) => i.id === titleHit); + expect(hit).toBeDefined(); + expect(hit.snippet).toBe(''); + }); + + // F3 — parentPageId subtree scoping (restores coverage the deleted lookup spec + // gave). Two sibling subtrees share one term; scoping to ONE root returns only + // that subtree's hits INCLUDING the root/parent page itself, and NONE of the + // sibling subtree. Mutation: drop the ANY(descendantIds) filter → this reddens. + it('F3 parentPageId scopes to one subtree incl. the parent, excludes siblings', async () => { + const rootA = await insertPage({ + title: 'subtreeA корень', + textContent: 'f3marker в корне A', + }); + const childA = await insertPage({ + title: 'subtreeA потомок', + textContent: 'f3marker в потомке A', + parentPageId: rootA, + }); + const rootB = await insertPage({ + title: 'subtreeB корень', + textContent: 'f3marker в корне B', + }); + const childB = await insertPage({ + title: 'subtreeB потомок', + textContent: 'f3marker в потомке B', + parentPageId: rootB, + }); + const res = await search(buildService(), { + query: 'f3marker', + spaceId, + parentPageId: rootA, + }); + const ids = res.items.map((i: any) => i.id); + expect(ids).toContain(rootA); // the parent/root page itself + expect(ids).toContain(childA); + expect(ids).not.toContain(rootB); // sibling subtree cut off + expect(ids).not.toContain(childB); + }); + + // F4 — positive path + snippet content asserts (the #11c test only asserts what + // must NOT be in path). (a) a nested hit's path is root→parent ordered and a + // root-level hit's path is []; (b) a text-body hit returns a NON-empty snippet. + it('F4 path is root→parent ordered ([] at root) and body hits carry a snippet', async () => { + const root = await insertPage({ title: 'F4Root' }); + const parent = await insertPage({ title: 'F4Parent', parentPageId: root }); + const leaf = await insertPage({ + title: 'f4leaf лист', + parentPageId: parent, + textContent: 'f4leafmarker тело с содержимым для сниппета', + }); + const res = await search(buildService(), { + query: 'f4leafmarker', + spaceId, + }); + const hit = res.items.find((i: any) => i.id === leaf); + expect(hit).toBeDefined(); + // Positive ancestry: root → direct parent, hit's own title excluded. + expect(hit.path).toEqual(['F4Root', 'F4Parent']); + // Snippet content: a text-body hit returns a non-empty snippet with the term. + expect(hit.snippet.length).toBeGreaterThan(0); + expect(hit.snippet).toContain('f4leafmarker'); + + // A root-level hit (no parent) → empty path. + const rootHit = await insertPage({ + title: 'f4rootlevel уникальный', + textContent: 'f4rootmarker в теле', + }); + const res2 = await search(buildService(), { + query: 'f4rootmarker', + spaceId, + }); + const hit2 = res2.items.find((i: any) => i.id === rootHit); + expect(hit2).toBeDefined(); + expect(hit2.path).toEqual([]); + }); });