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) <noreply@anthropic.com>
This commit is contained in:
@@ -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;
|
||||
|
||||
@@ -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<typeof parseSearchQuery>;
|
||||
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');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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:*');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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',
|
||||
|
||||
@@ -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([]);
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user