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.
This commit is contained in:
Jakob Wennberg
@@ -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
|
||||
|
||||
@@ -584,3 +584,7 @@ One line per decision: `[YYYY-MM-DD] <decision>: <why>`. 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.
|
||||
|
||||
@@ -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(/<form method="POST" action="([^"]*)"/)?.[1]
|
||||
expect(action).toBeDefined()
|
||||
|
||||
// Separators are entity-encoded: proof escapeHtml ran over the whole thing.
|
||||
expect(action).toContain('&evil=')
|
||||
expect(action).not.toMatch(/&(?!amp;|quot;|lt;|gt;)/)
|
||||
// The attribute is never closed early, so no raw markup escapes into the page.
|
||||
expect(html).not.toContain('"><script>')
|
||||
expect(html).not.toContain('<script>alert(1)</script>')
|
||||
})
|
||||
|
||||
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
|
||||
|
||||
@@ -556,7 +556,7 @@ export async function GET(request: Request) {
|
||||
<span class="account-name">${escapeHtml(companyName)}</span>
|
||||
</div>
|
||||
|
||||
<form method="POST" action="${url.pathname}${url.search}" id="consent-form">
|
||||
<form method="POST" action="${escapeHtml(url.pathname + url.search)}" id="consent-form">
|
||||
<input type="hidden" name="scope_binding" value="${escapeHtml(scopeBindingValue)}">
|
||||
<input type="hidden" name="scope_binding_sig" value="${escapeHtml(scopeBindingSignature)}">
|
||||
|
||||
@@ -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, '&')
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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_<base64url>`), 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')
|
||||
}
|
||||
|
||||
@@ -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, '\\"')
|
||||
|
||||
Reference in New Issue
Block a user