From 48011c6b502362818e357c840bbcce0031ed8d32 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Sun, 23 Aug 2026 14:09:11 +0200 Subject: [PATCH 1/3] Fix regex API term extraction discarding quotes (#147) longestLiteralSequence() now accumulates " as a valid literal character instead of breaking the sequence on it. extractApiTerm() escapes and wraps the extracted term in an outer pair of quotes (GitHub's documented escaping syntax) whenever it contains a quote, instead of sending an unwrapped term that GitHub would otherwise strip the quotes from or treat too broadly. Verified against the live GitHub API: /"react":\s*"[~^]?[0-9]/ now sends "\"react\"" (227 candidates) instead of the previous bare react term (20288 candidates, mostly noise never surfacing the right files within the API's 1000-result best-match cap). Closes #147 --- src/regex.test.ts | 31 +++++++++++++++++++++++++++++++ src/regex.ts | 41 +++++++++++++++++++++++++++++++---------- 2 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/regex.test.ts b/src/regex.test.ts index 8add72b..9f0b034 100644 --- a/src/regex.test.ts +++ b/src/regex.test.ts @@ -240,6 +240,37 @@ describe("buildApiQuery — special escape handling in longestLiteralSequence", }); }); +describe("buildApiQuery — quote handling in extracted terms (issue #147)", () => { + it('/"react":\\s*"[~^]?[0-9]/ → escaped literal \'"\\"react\\""\' (quotes kept, not dropped)', () => { + // Regression: the previous behaviour dropped the `"` characters entirely, + // yielding the bare term "react" — verified via the GitHub API to return + // 20288 results (mostly noise: react-native, @types/react, docs, imports) + // instead of the ~227 candidates the quoted literal returns, all of which + // are then narrowed further by the local regex filter. + const r = buildApiQuery('/"react":\\s*"[~^]?[0-9]/'); + expect(r.apiQuery).toBe('"\\"react\\""'); + expect(r.regexFilter).not.toBeNull(); + expect(r.warn).toBeUndefined(); + }); + + it('/"axios"/ → escaped literal \'"\\"axios\\""\'', () => { + const r = buildApiQuery('/"axios"/'); + expect(r.apiQuery).toBe('"\\"axios\\""'); + }); + + it("/from.*['\\\"]axios/ → axios (quotes inside a character class are unaffected)", () => { + // Regression guard: quotes inside [...] must still be skipped as before — + // only quotes appearing as literal (non-class) pattern characters change. + const r = buildApiQuery("/from.*['\\\"]axios/"); + expect(r.apiQuery).toBe("axios"); + }); + + it("/TODO|FIXME|HACK/ → unquoted OR join is unaffected when no branch has quotes", () => { + const r = buildApiQuery("/TODO|FIXME|HACK/"); + expect(r.apiQuery).toBe("TODO OR FIXME OR HACK"); + }); +}); + describe("buildApiQuery — warn cases", () => { it("/[~^]?[0-9]+\\.[0-9]+/ → empty term + warn", () => { const r = buildApiQuery("/[~^]?[0-9]+\\.[0-9]+/"); diff --git a/src/regex.ts b/src/regex.ts index d209c1e..e57443b 100644 --- a/src/regex.ts +++ b/src/regex.ts @@ -111,6 +111,18 @@ function extractRegexToken(q: string): RegexToken | null { return { raw, pattern, flags, index: tokenStart }; } +/** + * Escape a literal API term for GitHub's exact-phrase query syntax when it + * contains a `"` character — e.g. `"react"` → `"\"react\""`. + * See GitHub's "Searching for quotes and backslashes" documentation. + * Terms without a `"` are returned unchanged (no behaviour change for the + * common case). + */ +function escapeApiTerm(term: string): string { + if (!term.includes('"')) return term; + return `"${term.replace(/"/g, '\\"')}"`; +} + /** * Derive a literal API search term from a regex pattern. * @@ -119,6 +131,11 @@ function extractRegexToken(q: string): RegexToken | null { * nested inside `(...)` or `[...]`) → join branches with ` OR `. * 2. Otherwise → extract all unescaped literal sequences, pick the longest one. * 3. If the best term is shorter than 3 characters → return `warn`. + * + * A `"` character is a valid literal (not a regex metacharacter) and is kept + * in the extracted term; it is then escaped and the whole term wrapped in an + * outer pair of quotes so the GitHub API treats it as a literal quote rather + * than stripping it — see issue #147. */ function extractApiTerm(pattern: string): { term: string; warn?: string } { // 1. Top-level alternation detection. @@ -130,7 +147,7 @@ function extractApiTerm(pattern: string): { term: string; warn?: string } { // "< 3 chars → warn + empty term" rule still applies. const branchTerms = branches.map((b) => longestLiteralSequence(b)); if (branchTerms.every((t) => t.length >= 3)) { - return { term: branchTerms.join(" OR ") }; + return { term: branchTerms.map(escapeApiTerm).join(" OR ") }; } } @@ -144,7 +161,7 @@ function extractApiTerm(pattern: string): { term: string; warn?: string } { "Use --regex-hint to specify the term to send to the GitHub API.", }; } - return { term }; + return { term: escapeApiTerm(term) }; } /** @@ -202,9 +219,12 @@ function splitTopLevelAlternation(pattern: string): string[] { * Extract the longest contiguous sequence of characters useful as a GitHub * search term from a regex pattern fragment. * - * Only `[a-zA-Z0-9_-]` characters are accumulated — punctuation and special - * characters that are valid regex literals (e.g. `\(`) are intentionally - * excluded because they produce poor search terms. + * `[a-zA-Z0-9_"-]` characters are accumulated — `"` is a valid literal (not a + * regex metacharacter) and is kept so terms like `"react"` survive extraction + * instead of being broken into the far too broad bare word `react` — see + * issue #147. Other punctuation and special characters that are valid regex + * literals (e.g. `\(`) are intentionally excluded because they produce poor + * search terms. * Character classes `[...]` are skipped entirely. * Uses `>=` when updating `best` so that later (more specific) sequences of * equal length are preferred over earlier structural ones (e.g. `old-lib` @@ -238,14 +258,15 @@ function longestLiteralSequence(pattern: string): string { // Handle escape sequences. if (ch === "\\") { const next = pattern[i + 1] ?? ""; - // Only accumulate if the escaped char is a word character or hyphen - // AND is not a common regex escape or backreference (\b, \d, \s, \w, - // \p, \u, \x, \1–9, …) or control-character escape (\n, \r, \t, \f, \v). + // Only accumulate if the escaped char is a word character, hyphen or + // double quote, AND is not a common regex escape or backreference + // (\b, \d, \s, \w, \p, \u, \x, \1–9, …) or control-character escape + // (\n, \r, \t, \f, \v). // Note: \a and \e are NOT in this list — in JS without u/v they are // identity escapes that simply match the literal letter ('a' or 'e'), // so they should be accumulated, not broken on. // \c = control escape (\cA–\cZ), \k = named back-reference (\k). - const isWordLike = /[a-zA-Z0-9_-]/.test(next); + const isWordLike = /[a-zA-Z0-9_"-]/.test(next); const isSpecialEscape = /[bBdDsSwWpPuUxX0-9nrtfvck]/.test(next); if (isWordLike && !isSpecialEscape) { current += next; @@ -260,7 +281,7 @@ function longestLiteralSequence(pattern: string): string { } // Only accumulate characters that make a useful GitHub search term. - if (/[a-zA-Z0-9_-]/.test(ch)) { + if (/[a-zA-Z0-9_"-]/.test(ch)) { current += ch; } else { if (current.length >= best.length) best = current; From 9fa0084cdc88ed27ddd438c239f5ccd0a217fce7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Sun, 23 Aug 2026 15:56:15 +0200 Subject: [PATCH 2/3] Address CodeQL: escape backslashes before quotes in escapeApiTerm CodeQL flagged escapeApiTerm() for incomplete string escaping: it escaped double-quote characters but not backslashes, per GitHub's documented escape syntax (which only recognises \\ and \" as escapes). A literal backslash in the term was left unescaped, which could be misread once wrapped in the outer quoted phrase. Escape backslashes first, then quotes, matching standard escaping order and GitHub's own documented rules. longestLiteralSequence() never actually emits a raw backslash in practice (it only ever appends the character *after* a backslash, never the backslash itself), so this is a pure defense-in-depth hardening fix with no observable behaviour change for any existing input -- confirmed by the full test suite staying green. escapeApiTerm() is now exported for direct unit testing of the backslash-then-quote escaping order. See https://github.com/fulll/github-code-search/pull/152#discussion_r3838495331 --- src/regex.test.ts | 29 ++++++++++++++++++++++++++++- src/regex.ts | 13 +++++++++---- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/regex.test.ts b/src/regex.test.ts index 9f0b034..a0ab54f 100644 --- a/src/regex.test.ts +++ b/src/regex.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "bun:test"; -import { buildApiQuery, isRegexQuery } from "./regex.ts"; +import { buildApiQuery, escapeApiTerm, isRegexQuery } from "./regex.ts"; // ─── isRegexQuery ───────────────────────────────────────────────────────────── @@ -271,6 +271,33 @@ describe("buildApiQuery — quote handling in extracted terms (issue #147)", () }); }); +describe("escapeApiTerm — backslash escaping (CodeQL: incomplete string escaping)", () => { + it("returns the term unchanged when it has no quote", () => { + expect(escapeApiTerm("axios")).toBe("axios"); + }); + + it("returns a term with a bare backslash and no quote unchanged", () => { + // No quote present → short-circuits before any escaping is needed. + expect(escapeApiTerm("foo\\bar")).toBe("foo\\bar"); + }); + + it('escapes a lone quote: "react" → \\"react\\"', () => { + expect(escapeApiTerm('"react"')).toBe('"\\"react\\""'); + }); + + it("escapes backslashes before escaping quotes, so a backslash never swallows the following escaped quote", () => { + // Regression: term containing a literal backslash immediately followed by + // a quote. Escaping quotes without first escaping backslashes would leave + // the backslash unescaped, producing an invalid/ambiguous sequence for + // GitHub's parser (which only recognises \\ and \" as escapes). + const term = '"a\\b"'; + const result = escapeApiTerm(term); + // Every literal backslash in the input must itself be escaped to \\, + // and every literal quote must be escaped to \", all wrapped in "...". + expect(result).toBe('"\\"a\\\\b\\""'); + }); +}); + describe("buildApiQuery — warn cases", () => { it("/[~^]?[0-9]+\\.[0-9]+/ → empty term + warn", () => { const r = buildApiQuery("/[~^]?[0-9]+\\.[0-9]+/"); diff --git a/src/regex.ts b/src/regex.ts index e57443b..66874ac 100644 --- a/src/regex.ts +++ b/src/regex.ts @@ -114,13 +114,18 @@ function extractRegexToken(q: string): RegexToken | null { /** * Escape a literal API term for GitHub's exact-phrase query syntax when it * contains a `"` character — e.g. `"react"` → `"\"react\""`. - * See GitHub's "Searching for quotes and backslashes" documentation. + * See GitHub's "Searching for quotes and backslashes" documentation, which + * defines `\\` and `\"` as the only two recognised escape sequences. Backslashes + * are escaped first so a literal `\` immediately preceding a `"` cannot be + * misread as escaping that quote once wrapped — see CodeQL "Incomplete string + * escaping or encoding" (js/incomplete-sanitization). * Terms without a `"` are returned unchanged (no behaviour change for the - * common case). + * common case). Exported for unit testing. */ -function escapeApiTerm(term: string): string { +export function escapeApiTerm(term: string): string { if (!term.includes('"')) return term; - return `"${term.replace(/"/g, '\\"')}"`; + const escaped = term.replace(/\\/g, "\\\\").replace(/"/g, '\\"'); + return `"${escaped}"`; } /** From c539258bd204b5013249cb87d283be1eff3b69fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9bastien=20HOUZ=C3=89?= Date: Sun, 23 Aug 2026 16:26:00 +0200 Subject: [PATCH 3/3] Clarify test description: escapeApiTerm also wraps the term in outer quotes Addresses review feedback: the test name implied the escaped output was just \"react\", but escapeApiTerm() also wraps the term in outer quotes, so the actual expectation is "\"react\"". No assertion or behaviour change. See https://github.com/fulll/github-code-search/pull/152#discussion_r3838703066 --- src/regex.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/regex.test.ts b/src/regex.test.ts index a0ab54f..8746b59 100644 --- a/src/regex.test.ts +++ b/src/regex.test.ts @@ -281,7 +281,7 @@ describe("escapeApiTerm — backslash escaping (CodeQL: incomplete string escapi expect(escapeApiTerm("foo\\bar")).toBe("foo\\bar"); }); - it('escapes a lone quote: "react" → \\"react\\"', () => { + it('escapes a lone quote and wraps the term: "react" → "\\"react\\""', () => { expect(escapeApiTerm('"react"')).toBe('"\\"react\\""'); });