From 7dde8cac82eb97b0504a21d13f6cc142442a656d Mon Sep 17 00:00:00 2001 From: Jakob Wennberg <149234542+jakobwennberg@users.noreply.github.com> Date: Mon, 27 Jul 2026 14:02:25 +0200 Subject: [PATCH] fix(security): resolve the CodeQL backlog, three fixes and three documented false positives (#1225) Triage of all 9 CodeQL alerts surfaced on main by #1223. None were introduced by that PR. Fixed: the compliance-review artifact now unpacks to runner.temp instead of over the trusted checkout (actions/artifact-poisoning, critical); MCP LIKE patterns escape backslash first, which was a real correctness bug returning wrong rows for any search containing a backslash (js/incomplete-sanitization, 2 sites); and the mcp-oauth consent form action is HTML-escaped (js/reflected-xss, not exploitable because WHATWG URL already percent-encodes " < >, but & is not in that encode set). Dismissed as false positives with reasoning recorded at each site and in DECISIONS.md: sie-export escapeQuotes, where doubling backslashes would violate SIE 4B, corrupt files in conformant readers and skew #KSUMMA under BFL 7-year retention; hashApiKey, where SHA-256 is correct for a 256-bit CSPRNG token and changing it would invalidate every live gnubok_sk_ key; and the DuplicateBookingDialog href, which is a DB UUID behind a literal path prefix. Regression tests cover both behavioural fixes, including the escape ordering. --- .../workflows/swedish-compliance-review.yml | 18 ++++++++-- DECISIONS.md | 4 +++ .../authorize/__tests__/route.test.ts | 36 +++++++++++++++++++ app/api/mcp-oauth/authorize/route.ts | 17 ++++++++- .../__tests__/query-journal.test.ts | 32 +++++++++++++++++ extensions/general/mcp-server/server.ts | 22 ++++++++++-- lib/auth/api-keys.ts | 17 +++++++++ lib/reports/sie-export.ts | 22 +++++++++++- 8 files changed, 161 insertions(+), 7 deletions(-) diff --git a/.github/workflows/swedish-compliance-review.yml b/.github/workflows/swedish-compliance-review.yml index ea822f70..def828f7 100644 --- a/.github/workflows/swedish-compliance-review.yml +++ b/.github/workflows/swedish-compliance-review.yml @@ -43,13 +43,25 @@ jobs: uses: actions/download-artifact@3e5f45b2cfb9172054b4087a40e8e0b5a5461e7c # v8 with: name: compliance-input + # Unpacked OUTSIDE the checkout, not into the workspace root. + # + # This artifact is built from a fork's PR head, so its contents are + # attacker-influenced. Extracted over the workspace, an entry named + # `scripts/swedish-compliance-review.mjs` would overwrite the trusted + # script this job is about to run, with the AWS secrets and a write + # token already in scope. Stage 1 only ever writes three fixed + # filenames, and its workflow definition comes from the base repo even + # for fork PRs, so that is not reachable today: this keeps it + # unreachable if stage 1 ever grows a filename derived from PR + # content. Flagged by CodeQL as actions/artifact-poisoning. + path: ${{ runner.temp }}/compliance-input run-id: ${{ github.event.workflow_run.id }} github-token: ${{ secrets.GITHUB_TOKEN }} - name: Resolve PR number id: pr run: | set -euo pipefail - NUM=$(cat pr-number.txt) + NUM=$(cat "$RUNNER_TEMP/compliance-input/pr-number.txt") # Guard: pr-number.txt must be a plain integer (artifact is untrusted input). if ! [[ "$NUM" =~ ^[0-9]+$ ]]; then echo "Refusing to continue: pr-number.txt is not a number" >&2 @@ -81,8 +93,8 @@ jobs: AWS_REGION: ${{ secrets.AWS_REGION || 'eu-north-1' }} REVIEW_MODEL: eu.anthropic.claude-sonnet-5 # Two-stage mode: read the diff from the artifact instead of git-diffing. - DIFF_FILE: diff.patch - FILES_FILE: files.txt + DIFF_FILE: ${{ runner.temp }}/compliance-input/diff.patch + FILES_FILE: ${{ runner.temp }}/compliance-input/files.txt run: node scripts/swedish-compliance-review.mjs - name: Assert review produced output # This job once produced no compliance signal for 10 consecutive PR diff --git a/DECISIONS.md b/DECISIONS.md index 5700c82c..a804541c 100644 --- a/DECISIONS.md +++ b/DECISIONS.md @@ -584,3 +584,7 @@ One line per decision: `[YYYY-MM-DD] : `. Appended by agents and [2026-07-27] Docker layer cache tag is now per-architecture (buildcache-amd64 / buildcache-arm64): with native runners each job builds one platform, so a shared tag would leave the two racing to overwrite a cache manifest describing layers the other cannot use. [2026-07-27] Assistant screen-reader announcement fires on turn boundaries, not on the streaming text: a live region over token deltas re-announces on every delta, so the finished answer is announced once (capped at 400 chars) instead of the stream being narrated. [2026-07-27] Did NOT self-host the dicebear avatars in the PR7 polish pass despite it being on the plan: the Notionists set is third-party artwork with its own licence terms, and vendoring it into an AGPL-3.0 repo is a licence decision for the founder, not a polish item. +[2026-07-27] SIE export keeps escaping ONLY quotes, against CodeQL js/incomplete-sanitization: SIE 4B defines the backslash purely as a marker before a quotation mark, defines no \\ sequence, and excludes that marker from the #KSUMMA control total, so doubling backslashes would invent a sequence the format lacks, land as a literal double backslash in conformant readers (Fortnox/Visma/BL) and skew the checksum, in a file kept under BFL 7-year retention. +[2026-07-27] hashApiKey stays SHA-256 against CodeQL js/insufficient-password-hash: the input is 32 CSPRNG bytes, not a user-chosen password, so no KDF work factor is meaningful against 256 bits; the hash is also the primary-key lookup on every MCP request, and changing it would invalidate every live gnubok_sk_ key since the hash IS the stored credential. +[2026-07-27] mcp-oauth consent form action is HTML-escaped even though the CodeQL js/reflected-xss finding is not exploitable (WHATWG URL parsing already percent-encodes " < > in the query component): & is not in that encode set so the attribute was emitting invalid raw ampersands, and resting the page on an unstated parser-normalisation invariant is one refactor away from being wrong. +[2026-07-27] Compliance-review artifact unpacks to runner.temp instead of the workspace root: extracting fork-influenced content over the trusted checkout, with AWS secrets in scope, was safe only because stage 1 happens to write fixed filenames; moving it makes overwrite unreachable by construction. diff --git a/app/api/mcp-oauth/authorize/__tests__/route.test.ts b/app/api/mcp-oauth/authorize/__tests__/route.test.ts index 051c57d4..5283e47c 100644 --- a/app/api/mcp-oauth/authorize/__tests__/route.test.ts +++ b/app/api/mcp-oauth/authorize/__tests__/route.test.ts @@ -110,6 +110,42 @@ describe('GET /api/mcp-oauth/authorize: CSP', () => { expect(csp).not.toContain('env=prod') }) + it('HTML-escapes the reflected query string in the form action', async () => { + // The consent form posts back to the same URL, so url.search is echoed into + // an HTML attribute, and only redirect_uri/client_id/scope are validated: + // any extra parameter reaches that attribute. + // + // Two layers, and it is worth being precise about which does what. WHATWG + // URL parsing already percent-encodes " < > in the query component, so an + // injected tag arrives inert and CodeQL's js/reflected-xss report is not a + // live exploit. But `&` is NOT in that encode set, so without escaping the + // attribute carries raw ampersands, which is invalid HTML and leaves the + // page one refactor (a raw header, a non-WHATWG parser) away from a real + // breakout. This asserts the escaping layer, independent of the parser. + const request = new Request( + buildAuthorizeUrl({ + response_type: 'code', + redirect_uri: 'https://claude.com/api/oauth/callback', + code_challenge: 'abc', + code_challenge_method: 'S256', + scope: 'mcp', + }) + '&evil=%22%3E%3Cscript%3Ealert(1)%3C%2Fscript%3E' + ) + const response = await GET(request) + expect(response.status).toBe(200) + + const html = await response.text() + const action = html.match(/
') + }) + it('renders both read and write rows when client passes only the legacy `mcp` scope marker', async () => { // Claude's connector sends scope=mcp today. The consent UI must render // every scope group so the user can opt into write/approval rows if they diff --git a/app/api/mcp-oauth/authorize/route.ts b/app/api/mcp-oauth/authorize/route.ts index 802f45ff..d65666ed 100644 --- a/app/api/mcp-oauth/authorize/route.ts +++ b/app/api/mcp-oauth/authorize/route.ts @@ -556,7 +556,7 @@ export async function GET(request: Request) { - + @@ -832,6 +832,21 @@ function scopeRow(scope: ApiKeyScope, checked: boolean, kind: 'read' | 'write'): ` } +/** + * Every interpolation into the consent-page template goes through this, + * including the form's own action attribute (url.pathname + url.search). + * + * On that one: only redirect_uri/client_id/scope are validated upstream, so any + * extra query parameter a caller appends is reflected into the attribute. + * CodeQL reports it as js/reflected-xss. It was not a live exploit, because + * WHATWG URL parsing already percent-encodes " < > in the query component and + * an injected tag therefore arrives inert. It is escaped anyway for two + * reasons: & is NOT in that encode set, so the unescaped form emitted raw + * ampersands in an attribute (invalid HTML), and the safety of the page + * otherwise rests on a parser normalisation invariant that nothing in this file + * states or tests. Escaping & as & is correct here: the browser decodes it + * back on submit, so the query string round-trips intact. + */ function escapeHtml(str: string): string { return str .replace(/&/g, '&') diff --git a/extensions/general/mcp-server/__tests__/query-journal.test.ts b/extensions/general/mcp-server/__tests__/query-journal.test.ts index ab02c3bc..a69b5fd7 100644 --- a/extensions/general/mcp-server/__tests__/query-journal.test.ts +++ b/extensions/general/mcp-server/__tests__/query-journal.test.ts @@ -517,6 +517,38 @@ describe('gnubok_query_journal: free-text search', () => { expect(ilikeCalls[0].pattern).toBe('%2\\_441\\%foo%') }) + it('escapes a literal backslash so it does not swallow the next character', async () => { + // `\` is LIKE's own escape character. Before this was handled, a search for + // `a\b` reached Postgres as `%a\b%`, where `\b` means "literal b", so the + // filter silently matched rows containing `ab` and missed the ones the user + // actually asked for. Flagged by CodeQL as js/incomplete-sanitization. + const tool = tools.find((t) => t.name === 'gnubok_query_journal')! + const { supabase, ilikeCalls } = makeQueueMock([ + { data: [], count: 0 }, + { data: [], count: 0 }, + ]) + + await tool.execute({ text: 'a\\b', limit: 50 }, 'company-1', 'user-1', supabase) + + expect(new Set(ilikeCalls.map((c) => c.pattern)).size).toBe(1) + expect(ilikeCalls[0].pattern).toBe('%a\\\\b%') + }) + + it('escapes backslash before the wildcard rules, not after', async () => { + // Order matters: escaping `\` last would also double the backslashes the + // % / _ rules just introduced, turning `50%` into `50\\%` (a literal + // backslash followed by a wildcard) instead of `50\%` (a literal percent). + const tool = tools.find((t) => t.name === 'gnubok_query_journal')! + const { supabase, ilikeCalls } = makeQueueMock([ + { data: [], count: 0 }, + { data: [], count: 0 }, + ]) + + await tool.execute({ text: '50%', limit: 50 }, 'company-1', 'user-1', supabase) + + expect(ilikeCalls[0].pattern).toBe('%50\\%%') + }) + it('does NOT flag truncated when an overlap row is hit by both legs and merged set fits limit', async () => { // Greptile / Compliance V2.3 regression: previously, dbMatched = sum of // leg counts and a row matching both legs would inflate the count and diff --git a/extensions/general/mcp-server/server.ts b/extensions/general/mcp-server/server.ts index 3f52f03e..7a003a74 100644 --- a/extensions/general/mcp-server/server.ts +++ b/extensions/general/mcp-server/server.ts @@ -6589,7 +6589,16 @@ export const tools: McpTool[] = [ // searches the ENTRY description only (documented in the // schema); the two-leg line+entry union query_journal runs is // overkill for a write filter. - const escaped = text.replace(/[%]/g, '\\%').replace(/_/g, '\\_') + // + // Backslash is escaped FIRST, and the order matters: `\` is LIKE's + // own escape character, so an unescaped one in the search term + // swallows the character after it (searching `a\b` matched rows + // containing `ab`). Escaping it last would instead double the + // backslashes the % / _ rules just added. + const escaped = text + .replace(/\\/g, '\\\\') + .replace(/%/g, '\\%') + .replace(/_/g, '\\_') e = e.ilike('description', `%${escaped}%`) } return e @@ -7262,7 +7271,16 @@ export const tools: McpTool[] = [ // separator. The .ilike() path passes the pattern as a parameterised // filter operand where `,` is a literal: stripping would mangle // searches for real commas in line descriptions. - const escaped = text.replace(/[%]/g, '\\%').replace(/_/g, '\\_') + // + // Backslash is escaped FIRST, and the order matters: `\` is LIKE's own + // escape character, so an unescaped one in the search term swallows the + // character after it (searching `a\b` matched rows containing `ab`). + // Escaping it last would instead double the backslashes the % / _ rules + // just added. + const escaped = text + .replace(/\\/g, '\\\\') + .replace(/%/g, '\\%') + .replace(/_/g, '\\_') const pattern = `%${escaped}%` // Fetch up to 2× limit per leg to reduce global-ordering loss when diff --git a/lib/auth/api-keys.ts b/lib/auth/api-keys.ts index 47b88775..e1d03f37 100644 --- a/lib/auth/api-keys.ts +++ b/lib/auth/api-keys.ts @@ -378,6 +378,23 @@ export function generateApiKey(mode: ApiKeyMode = 'live'): { key: string; hash: return { key, hash, prefix } } +/** + * SHA-256, deliberately, and NOT a slow KDF like bcrypt/argon2. + * + * CodeQL flags this as js/insufficient-password-hash. That rule exists for + * user-chosen passwords, which are low-entropy and brute-forceable, so the + * defence is to make each guess expensive. This input is not a password: keys + * come from generateApiKey as 32 CSPRNG bytes (`gnubok_sk_`), and no + * work factor moves the needle on a 256-bit random secret. + * + * A slow KDF would also be actively worse here: this runs on the hot path of + * every MCP request, where the hash is the primary-key lookup used to find the + * row, so per-request cost is real latency for zero security gain. + * + * Do NOT "fix" this by changing the algorithm. The hash IS the stored + * credential, so a different function invalidates every live `gnubok_sk_` key, + * breaking existing MCP connections with no migration path. + */ export function hashApiKey(key: string): string { return crypto.createHash('sha256').update(key).digest('hex') } diff --git a/lib/reports/sie-export.ts b/lib/reports/sie-export.ts index cd53124b..30577c87 100644 --- a/lib/reports/sie-export.ts +++ b/lib/reports/sie-export.ts @@ -307,7 +307,27 @@ function formatAmount(amount: number): string { } /** - * Escape double quotes in SIE strings + * Escape double quotes in SIE strings. + * + * Quotes only. A literal backslash is deliberately NOT doubled, and it must stay + * that way. SIE 4B defines the backslash purely as a marker placed before a + * quotation mark and defines no `\\` sequence at all: "Quotation marks in export + * fields are to be preceded by a backslash (ASCII 92)", and for the checksum, + * "Quotation marks within fields are marked with a 'backslash'. However, only + * the quotation marks are to be included in the calculation of the control + * total" -- the marker is excluded from #KSUMMA, which is only coherent if it is + * not itself data. + * + * Emitting `\\` for a literal backslash would invent a sequence the format does + * not define, land as a doubled backslash in every reader that implements the + * spec's single rule (Fortnox, Visma, BL), and skew #KSUMMA. That corrupts a + * file kept under BFL 7-year retention. + * + * Round-tripping is already correct: `a\"b` exports as `a\\"b`, and the parser's + * /\\"/g rule (lib/import/sie-parser.ts) recovers `a\"b`. + * + * CodeQL flags this as js/incomplete-sanitization; it is a false positive here, + * because the rule assumes a grammar in which backslash escapes itself. */ function escapeQuotes(str: string): string { return str.replace(/"/g, '\\"')