diff --git a/.github/scripts/check-tree-sitter-upgrade-readiness.py b/.github/scripts/check-tree-sitter-upgrade-readiness.py index f54afd7f0..5b0fad09e 100644 --- a/.github/scripts/check-tree-sitter-upgrade-readiness.py +++ b/.github/scripts/check-tree-sitter-upgrade-readiness.py @@ -32,6 +32,7 @@ import pathlib import re import sys import urllib.error +import urllib.parse import urllib.request REPO_ROOT = pathlib.Path(__file__).resolve().parents[2] @@ -190,7 +191,17 @@ def fetch_text(url: str, timeout: int = 8) -> str | None: set (raises the rate limit from 60 to 5 000 requests/hour). """ headers: dict[str, str] = {} - if _GITHUB_TOKEN and ("github.com" in url or "githubusercontent.com" in url): + # Parse the URL and check the hostname rather than substring-matching + # on the full URL string (CodeQL py/incomplete-url-substring-sanitization). + # `https://evil.com/?u=github.com` would have passed the substring check. + try: + parsed_host = urllib.parse.urlparse(url).hostname or "" + except ValueError: + parsed_host = "" + is_github_host = parsed_host == "github.com" or parsed_host.endswith( + (".github.com", ".githubusercontent.com") + ) or parsed_host == "githubusercontent.com" + if _GITHUB_TOKEN and is_github_host: headers["Authorization"] = f"Bearer {_GITHUB_TOKEN}" try: req = urllib.request.Request(url, headers=headers) diff --git a/gitnexus-web/src/core/llm/agent.ts b/gitnexus-web/src/core/llm/agent.ts index c8cfa8d7c..49862a9e6 100644 --- a/gitnexus-web/src/core/llm/agent.ts +++ b/gitnexus-web/src/core/llm/agent.ts @@ -277,8 +277,10 @@ const extractInstanceName = (endpoint: string): string => { try { const url = new URL(endpoint); const hostname = url.hostname; - // Extract the first part before .openai.azure.com - const match = hostname.match(/^([^.]+)\.openai\.azure\.com/); + // Extract the first part before .openai.azure.com. The trailing `$` + // anchor is required (CodeQL js/regex/missing-regexp-anchor): without + // it `evil.openai.azure.com.attacker.tld` would match. + const match = hostname.match(/^([^.]+)\.openai\.azure\.com$/); if (match) { return match[1]; } diff --git a/gitnexus-web/src/core/llm/tools.ts b/gitnexus-web/src/core/llm/tools.ts index cd0b5a800..5a595049e 100644 --- a/gitnexus-web/src/core/llm/tools.ts +++ b/gitnexus-web/src/core/llm/tools.ts @@ -278,8 +278,11 @@ export const createGraphRAGTools = (backend: GraphRAGBackend) => { const val = row[col]; if (val === null || val === undefined) return ''; if (typeof val === 'object') return JSON.stringify(val); - // Truncate long values and escape pipe characters - const str = String(val).replace(/\|/g, '\\|'); + // Truncate long values and escape pipe characters. Escape + // backslashes FIRST so the subsequent pipe escape isn't + // unescaped by a trailing backslash (CodeQL + // js/incomplete-sanitization). + const str = String(val).replace(/\\/g, '\\\\').replace(/\|/g, '\\|'); return str.length > 60 ? str.slice(0, 57) + '...' : str; }); return `| ${values.join(' | ')} |`; diff --git a/gitnexus/src/cli/setup.ts b/gitnexus/src/cli/setup.ts index d1b7b520f..af3c4737a 100644 --- a/gitnexus/src/cli/setup.ts +++ b/gitnexus/src/cli/setup.ts @@ -365,7 +365,12 @@ async function installClaudeCodeHooks(result: SetupResult): Promise { } const hookPath = path.join(destHooksDir, 'gitnexus-hook.cjs').replace(/\\/g, '/'); - const hookCmd = `node "${hookPath.replace(/"/g, '\\"')}"`; + // Escape backslashes FIRST, then quotes (CodeQL js/incomplete-sanitization). + // The previous shape `replace(/"/g, '\\"')` alone would let `path\with"quote` + // become `path\with\"quote`, where the trailing `\` before `"` could + // unescape the quote inside the surrounding double-quoted shell context. + const escapedHookPath = hookPath.replace(/\\/g, '\\\\').replace(/"/g, '\\"'); + const hookCmd = `node "${escapedHookPath}"`; // Check which hook events need entries (idempotent: skip if already registered) const parsed = await (async () => { diff --git a/gitnexus/src/cli/wiki.ts b/gitnexus/src/cli/wiki.ts index e44566f8f..38a0f82a6 100644 --- a/gitnexus/src/cli/wiki.ts +++ b/gitnexus/src/cli/wiki.ts @@ -602,6 +602,38 @@ function hasGhCLI(): boolean { } } +/** + * Strict Gist URL predicate. Rejects: + * - any URL that does not parse (URL constructor throws) + * - schemes other than https (drops `http:`, `file:`, `gist:`-style spoofs) + * - hostnames that are not exactly `gist.github.com` (drops substring spoofs + * like `https://evil.com/?u=gist.github.com` and userinfo-prefixed shapes + * like `https://[email protected]/...` — note that URL.hostname + * strips userinfo, so the equality check rejects the userinfo-prefixed + * spoof if the actual host differs from gist.github.com) + * - any URL containing userinfo (`username[:password]@`), which the URL + * parser exposes via `.username` / `.password`. Defense-in-depth: even + * when hostname matches, a credential-bearing URL is suspect and not + * produced by `gh gist create`. + * + * Closes the substring-bypass class CodeQL `js/incomplete-url-substring- + * sanitization` flags. + */ +function isGistUrl(line: string): boolean { + const trimmed = line.trim(); + try { + const u = new URL(trimmed); + return ( + u.protocol === 'https:' && + u.hostname === 'gist.github.com' && + u.username === '' && + u.password === '' + ); + } catch { + return false; + } +} + function publishGist(htmlPath: string): { url: string; rawUrl: string } | null { try { const output = execFileSync( @@ -610,13 +642,14 @@ function publishGist(htmlPath: string): { url: string; rawUrl: string } | null { { encoding: 'utf-8', stdio: ['pipe', 'pipe', 'pipe'] }, ).trim(); - // gh gist create prints the gist URL as the last line - const lines = output.split('\n'); - const gistUrl = lines.find((l) => l.includes('gist.github.com')) || lines[lines.length - 1]; + // `gh gist create` prints the gist URL as a line in the output. Find the + // first parseable Gist URL — if no line is a valid Gist URL, fail closed + // (do NOT fall back to lines[last]: a non-Gist last line would propagate + // through the regex below and produce a malformed `rawUrl`). + const gistUrl = output.split('\n').find(isGistUrl); + if (!gistUrl) return null; - if (!gistUrl || !gistUrl.includes('gist.github.com')) return null; - - // Build a raw viewer URL via gist.githack.com + // Build a raw viewer URL via gist.githack.com. // gist URL format: https://gist.github.com/{user}/{id} const match = gistUrl.match(/gist\.github\.com\/([^/]+)\/([a-f0-9]+)/); let rawUrl = gistUrl; diff --git a/gitnexus/src/core/group/bridge-db.ts b/gitnexus/src/core/group/bridge-db.ts index dbf2350bb..ef6244b22 100644 --- a/gitnexus/src/core/group/bridge-db.ts +++ b/gitnexus/src/core/group/bridge-db.ts @@ -44,26 +44,6 @@ async function removeLbugFile(basePath: string): Promise { } } -/** - * Remove all stale `bridge.lbug.tmp.*` files (and their sidecars) from a - * group directory. With randomBytes-based temp names, a crashed writeBridge - * leaves behind a uniquely-named tmp file that no future run will target by - * name — so we glob for the prefix and clean up everything matching. - */ -async function cleanStaleBridgeTmpFiles(groupDir: string): Promise { - try { - const entries = await fsp.readdir(groupDir); - const staleBases = entries.filter( - (e) => e.startsWith('bridge.lbug.tmp.') && !LBUG_SIDECAR_SUFFIXES.some((s) => e.endsWith(s)), - ); - for (const name of staleBases) { - await removeLbugFile(path.join(groupDir, name)); - } - } catch { - /* best-effort: directory may not exist yet */ - } -} - export function contractNodeId( repo: string, contractId: string, @@ -299,8 +279,24 @@ export async function retryRename(src: string, dst: string, attempts = 3): Promi export async function writeBridgeMeta(groupDir: string, meta: BridgeMeta): Promise { const target = path.join(groupDir, 'meta.json'); + // Unpredictable suffix + O_EXCL via `'wx'` flag closes the symlink/ + // pre-create attack window. The third argument `0o600` is the + // user-only mode mask — CodeQL's `js/insecure-temporary-file` query + // sources its verdict from the `mode` argument, NOT from `flags`: + // its `isSecureMode(mode)` predicate requires the low 6 bits to be + // zero (no group/world bits). Without an explicit mode the file is + // created with the process umask (typically 0o644 = group/world + // readable), which the query treats as the actual vulnerability. + // Both `'wx'` (runtime O_EXCL) AND `0o600` (CodeQL-credited mode) + // are needed: one closes the symlink race, the other closes the + // permissions exposure. const tmp = `${target}.tmp.${randomBytes(8).toString('hex')}`; - await fsp.writeFile(tmp, JSON.stringify(meta, null, 2), 'utf-8'); + const handle = await fsp.open(tmp, 'wx', 0o600); + try { + await handle.writeFile(JSON.stringify(meta, null, 2), 'utf-8'); + } finally { + await handle.close(); + } // Use retryRename for consistency with writeBridge's atomic swap — on // Windows a concurrent reader can cause EBUSY/EPERM even on a tiny // meta.json, and we don't want meta write to be less robust than the @@ -369,7 +365,19 @@ export async function writeBridge( const crossLinks = dedupeCrossLinks(input.crossLinks); const finalPath = path.join(groupDir, 'bridge.lbug'); - const tmpPath = path.join(groupDir, `bridge.lbug.tmp.${randomBytes(8).toString('hex')}`); + // Stage the temp database inside a unique mkdtemp directory rather than + // a fixed `bridge.lbug.tmp` name. The previous shape was flagged by + // CodeQL js/insecure-temporary-file as a predictable path: a co-located + // attacker (or a parallel writeBridge call into the same group) could + // pre-create or symlink that path before this writer opens it. mkdtemp + // returns a directory whose suffix is filled with cryptographically + // random bytes, so the staging path is unguessable AND collision-free + // across parallel callers. We anchor the staging directory inside + // `groupDir` so the subsequent rename of `bridge.lbug` (and its + // `.wal` / `.shadow` sidecars) into place stays on the same filesystem + // and remains atomic — moving across `os.tmpdir()` could trip EXDEV. + const stagingDir = await fsp.mkdtemp(path.join(groupDir, 'bridge-tmp-')); + const tmpPath = path.join(stagingDir, 'bridge.lbug'); const bakPath = path.join(groupDir, 'bridge.lbug.bak'); const report: WriteBridgeReport = { @@ -389,43 +397,42 @@ export async function writeBridge( } }; - // Clean up stale tmp files left behind by previously crashed writeBridge - // runs. With randomBytes-based names each run picks a unique path, so - // the old fixed-name `removeLbugFile(tmpPath)` was a no-op — stale - // artifacts accumulated. The glob-based helper finds *all* leftover - // `bridge.lbug.tmp.*` entries and removes them (including sidecars). - await cleanStaleBridgeTmpFiles(groupDir); + // The mkdtemp staging directory above is freshly created with a unique + // random suffix, so there are no leftover `bridge.lbug.tmp` / `.wal` / + // `.shadow` sidecars from a previous crashed run to clean up here — the + // directory is empty by construction. - // 1. Create temp DB, insert all data. - // - // Everything after `openBridgeDb` must run inside a try/finally so that - // if ANY step before the explicit `closeBridgeDb` throws — schema - // creation, a contract insert loop that rethrows, a snapshot write, the - // cross-link loop, or anything else — the handle is still released. A - // leaked handle holds the native LadybugDB file lock on tmpPath, which - // (a) leaks a FD and (b) prevents the next writeBridge call from - // reusing the same tmp slot. - const handle = await openBridgeDb(tmpPath); - let handleClosed = false; try { - await ensureBridgeSchema(handle); + // 1. Create temp DB, insert all data. + // + // Everything after `openBridgeDb` must run inside a try/finally so that + // if ANY step before the explicit `closeBridgeDb` throws — schema + // creation, a contract insert loop that rethrows, a snapshot write, the + // cross-link loop, or anything else — the handle is still released. A + // leaked handle holds the native LadybugDB file lock on tmpPath, which + // (a) leaks a FD and (b) prevents the next writeBridge call from + // reusing the same tmp slot. + const handle = await openBridgeDb(tmpPath); + let handleClosed = false; + try { + await ensureBridgeSchema(handle); - // Build the lookup index incrementally as contracts are inserted, so - // failed inserts are never in the index (and therefore never resolved - // by the cross-link loop below). This replaces a previous N+1 query - // pattern where each link made up to 6 DB round-trips to find its - // endpoints — see ContractLookupIndex. - const lookupIndex = createContractLookupIndex(); + // Build the lookup index incrementally as contracts are inserted, so + // failed inserts are never in the index (and therefore never resolved + // by the cross-link loop below). This replaces a previous N+1 query + // pattern where each link made up to 6 DB round-trips to find its + // endpoints — see ContractLookupIndex. + const lookupIndex = createContractLookupIndex(); - // Insert contracts — tolerate individual failures (e.g., a corrupt meta - // that can't be serialized). The whole sync must not fail because one - // contract is broken. - for (const c of contracts) { - const id = contractNodeId(c.repo, c.contractId, c.role, c.symbolRef.filePath); - try { - await queryBridge( - handle, - `CREATE (n:Contract { + // Insert contracts — tolerate individual failures (e.g., a corrupt meta + // that can't be serialized). The whole sync must not fail because one + // contract is broken. + for (const c of contracts) { + const id = contractNodeId(c.repo, c.contractId, c.role, c.symbolRef.filePath); + try { + await queryBridge( + handle, + `CREATE (n:Contract { id: $id, contractId: $contractId, type: $type, @@ -438,91 +445,91 @@ export async function writeBridge( confidence: $confidence, meta: $meta })`, - { - id, - contractId: c.contractId, - type: c.type, - role: c.role, - repo: c.repo, - service: c.service ?? '', - symbolUid: c.symbolUid, - filePath: c.symbolRef.filePath, - symbolName: c.symbolName, - confidence: c.confidence, - meta: JSON.stringify(c.meta), - }, - ); - report.contractsInserted++; - // Only index on successful insert — the cross-link loop must never - // resolve to a row that isn't actually in the DB. - indexContract(lookupIndex, c, id); - } catch (err) { - report.contractsFailed++; - recordError('contract', id, err); + { + id, + contractId: c.contractId, + type: c.type, + role: c.role, + repo: c.repo, + service: c.service ?? '', + symbolUid: c.symbolUid, + filePath: c.symbolRef.filePath, + symbolName: c.symbolName, + confidence: c.confidence, + meta: JSON.stringify(c.meta), + }, + ); + report.contractsInserted++; + // Only index on successful insert — the cross-link loop must never + // resolve to a row that isn't actually in the DB. + indexContract(lookupIndex, c, id); + } catch (err) { + report.contractsFailed++; + recordError('contract', id, err); + } } - } - // Insert repo snapshots - for (const [repoId, snap] of Object.entries(input.repoSnapshots)) { - try { - await queryBridge( - handle, - `CREATE (s:RepoSnapshot { + // Insert repo snapshots + for (const [repoId, snap] of Object.entries(input.repoSnapshots)) { + try { + await queryBridge( + handle, + `CREATE (s:RepoSnapshot { id: $id, indexedAt: $indexedAt, lastCommit: $lastCommit })`, - { - id: repoId, - indexedAt: snap.indexedAt, - lastCommit: snap.lastCommit, - }, - ); - report.snapshotsInserted++; - } catch (err) { - report.snapshotsFailed++; - recordError('snapshot', repoId, err); - } - } - - // Insert cross-links (tolerating missing nodes). - // - // `findContractNode` consults the in-memory lookup index built above, - // not the DB — that's an O(1) pure-function lookup per endpoint instead - // of the previous 2-3 DB queries. For M cross-links, the previous code - // issued up to 6M round-trips; this version issues zero. - // - // `link.contractId` may differ between the consumer and provider sides - // (e.g. wildcard consumer `grpc::Service/*` → method-level provider - // `grpc::Service/Method`) — that's why we resolve each endpoint - // independently via its own `(repo, role, symbolUid, filePath, symbolName)` - // tuple rather than matching on contractId. - for (const link of crossLinks) { - const linkId = `${link.from.repo}::${link.contractId}->${link.to.repo}::${link.contractId}`; - try { - const fromId = findContractNode( - lookupIndex, - link.from.repo, - 'consumer', - link.from.symbolUid, - link.from.symbolRef.filePath, - link.from.symbolRef.name, - ); - const toId = findContractNode( - lookupIndex, - link.to.repo, - 'provider', - link.to.symbolUid, - link.to.symbolRef.filePath, - link.to.symbolRef.name, - ); - if (!fromId || !toId) { - report.linksDroppedMissingNode++; - continue; + { + id: repoId, + indexedAt: snap.indexedAt, + lastCommit: snap.lastCommit, + }, + ); + report.snapshotsInserted++; + } catch (err) { + report.snapshotsFailed++; + recordError('snapshot', repoId, err); } - await queryBridge( - handle, - ` + } + + // Insert cross-links (tolerating missing nodes). + // + // `findContractNode` consults the in-memory lookup index built above, + // not the DB — that's an O(1) pure-function lookup per endpoint instead + // of the previous 2-3 DB queries. For M cross-links, the previous code + // issued up to 6M round-trips; this version issues zero. + // + // `link.contractId` may differ between the consumer and provider sides + // (e.g. wildcard consumer `grpc::Service/*` → method-level provider + // `grpc::Service/Method`) — that's why we resolve each endpoint + // independently via its own `(repo, role, symbolUid, filePath, symbolName)` + // tuple rather than matching on contractId. + for (const link of crossLinks) { + const linkId = `${link.from.repo}::${link.contractId}->${link.to.repo}::${link.contractId}`; + try { + const fromId = findContractNode( + lookupIndex, + link.from.repo, + 'consumer', + link.from.symbolUid, + link.from.symbolRef.filePath, + link.from.symbolRef.name, + ); + const toId = findContractNode( + lookupIndex, + link.to.repo, + 'provider', + link.to.symbolUid, + link.to.symbolRef.filePath, + link.to.symbolRef.name, + ); + if (!fromId || !toId) { + report.linksDroppedMissingNode++; + continue; + } + await queryBridge( + handle, + ` MATCH (a:Contract), (b:Contract) WHERE a.id = $fromId AND b.id = $toId CREATE (a)-[:ContractLink { @@ -533,83 +540,93 @@ export async function writeBridge( toRepo: $toRepo }]->(b) `, - { - fromId, - toId, - matchType: link.matchType, - confidence: link.confidence, - contractId: link.contractId, - fromRepo: link.from.repo, - toRepo: link.to.repo, - }, - ); - report.linksInserted++; - } catch (err) { - report.linksFailed++; - recordError('link', linkId, err); + { + fromId, + toId, + matchType: link.matchType, + confidence: link.confidence, + contractId: link.contractId, + fromRepo: link.from.repo, + toRepo: link.to.repo, + }, + ); + report.linksInserted++; + } catch (err) { + report.linksFailed++; + recordError('link', linkId, err); + } + } + + // 2. Close temp DB (happy path). The finally block also calls + // closeBridgeDb if we threw above; `handleClosed` prevents a + // double-close on the native handle. + await closeBridgeDb(handle); + handleClosed = true; + } finally { + if (!handleClosed) { + await closeBridgeDb(handle).catch(() => { + /* ignore: cleanup path, best effort */ + }); } } - // 2. Close temp DB (happy path). The finally block also calls - // closeBridgeDb if we threw above; `handleClosed` prevents a - // double-close on the native handle. - await closeBridgeDb(handle); - handleClosed = true; - } finally { - if (!handleClosed) { - await closeBridgeDb(handle).catch(() => { - /* ignore: cleanup path, best effort */ - }); + // 3. Atomic swap: old→.bak, tmp→final, rm .bak + // + // The current database file (with its `.wal` / `.shadow` sidecars) is + // moved aside, then the freshly built tmp database takes its place. + // We move the sidecars together with the main file so the open below + // and any external readers see a consistent set; orphan sidecars from + // the tmp namespace are then removed because LadybugDB looks for them + // under the renamed-to base name and would reject mismatching IDs. + try { + await fsp.access(finalPath); + await retryRename(finalPath, bakPath); + for (const suffix of LBUG_SIDECAR_SUFFIXES) { + try { + await fsp.access(`${finalPath}${suffix}`); + await retryRename(`${finalPath}${suffix}`, `${bakPath}${suffix}`); + } catch { + /* sidecar absent — nothing to move */ + } + } + } catch { + /* no existing db */ } - } - - // 3. Atomic swap: old→.bak, tmp→final, rm .bak - // - // The current database file (with its `.wal` / `.shadow` sidecars) is - // moved aside, then the freshly built tmp database takes its place. - // We move the sidecars together with the main file so the open below - // and any external readers see a consistent set; orphan sidecars from - // the tmp namespace are then removed because LadybugDB looks for them - // under the renamed-to base name and would reject mismatching IDs. - try { - await fsp.access(finalPath); - await retryRename(finalPath, bakPath); + await retryRename(tmpPath, finalPath); for (const suffix of LBUG_SIDECAR_SUFFIXES) { + // Rename — not delete — so the WAL (which may carry uncommitted-at- + // close-time pages on a graceful close, depending on + // `autoCheckpoint` / `checkpointThreshold`) and the `.shadow` + // checkpoint snapshot stay paired with the database file under its + // final name. LadybugDB 0.16.0's database-id check rejects an open + // when the sidecars belong to a different base name. try { - await fsp.access(`${finalPath}${suffix}`); - await retryRename(`${finalPath}${suffix}`, `${bakPath}${suffix}`); + await fsp.access(`${tmpPath}${suffix}`); + await retryRename(`${tmpPath}${suffix}`, `${finalPath}${suffix}`); } catch { /* sidecar absent — nothing to move */ } } - } catch { - /* no existing db */ - } - await retryRename(tmpPath, finalPath); - for (const suffix of LBUG_SIDECAR_SUFFIXES) { - // Rename — not delete — so the WAL (which may carry uncommitted-at- - // close-time pages on a graceful close, depending on - // `autoCheckpoint` / `checkpointThreshold`) and the `.shadow` - // checkpoint snapshot stay paired with the database file under its - // final name. LadybugDB 0.16.0's database-id check rejects an open - // when the sidecars belong to a different base name. - try { - await fsp.access(`${tmpPath}${suffix}`); - await retryRename(`${tmpPath}${suffix}`, `${finalPath}${suffix}`); - } catch { - /* sidecar absent — nothing to move */ - } - } - await removeLbugFile(bakPath); + await removeLbugFile(bakPath); - // 4. Write meta.json - await writeBridgeMeta(groupDir, { - version: BRIDGE_SCHEMA_VERSION, - generatedAt: new Date().toISOString(), - missingRepos: input.missingRepos, - }); + // 4. Write meta.json + await writeBridgeMeta(groupDir, { + version: BRIDGE_SCHEMA_VERSION, + generatedAt: new Date().toISOString(), + missingRepos: input.missingRepos, + }); - return report; + return report; + } finally { + // Always remove the mkdtemp staging directory. On the happy path the + // main file and sidecars have been renamed out of it, so it's empty; + // on any error path it may still contain a partial database — either + // way `recursive: true, force: true` removes it without surfacing + // "directory not empty" or ENOENT. + await fsp.rm(stagingDir, { recursive: true, force: true }).catch(() => { + /* best-effort cleanup */ + }); + } } /* ------------------------------------------------------------------ */ @@ -705,6 +722,11 @@ export async function openBridgeDbReadOnly(groupDir: string): Promise setTimeout(r, delay)); } } + // Pino's NDJSON serialization is structurally injection-resistant + // (CodeQL js/log-injection): groupDir and err.message are JSON-escaped + // by the serializer, so no manual CRLF / U+2028 / ANSI sanitization is + // needed. Demoted to debug — only fires when the bridge truly gave up + // after retries, and operators only need it at debug verbosity. bridgeLogger.debug( { groupDir, err: lastErr, attempts: LBUG_OPEN_RETRY_ATTEMPTS }, 'openBridgeDbReadOnly gave up', diff --git a/gitnexus/src/core/group/storage.ts b/gitnexus/src/core/group/storage.ts index 99bf27fbd..cc3dbfdc9 100644 --- a/gitnexus/src/core/group/storage.ts +++ b/gitnexus/src/core/group/storage.ts @@ -5,6 +5,15 @@ import * as os from 'node:os'; import { randomBytes } from 'node:crypto'; import type { ContractRegistry } from './types.js'; +/** + * Build an unpredictable suffix for atomic-write tmp files. Replaces the + * previous `Date.now()` pattern which CodeQL flagged as + * js/insecure-temporary-file: a guessable suffix in a writable directory + * lets a co-located attacker pre-create or symlink the tmp path before the + * write lands. + */ +const tmpSuffix = (): string => randomBytes(8).toString('hex'); + const CONTRACTS_FILE = 'contracts.json'; export function getDefaultGitnexusDir(): string { @@ -35,9 +44,21 @@ export async function writeContractRegistry( registry: ContractRegistry, ): Promise { const targetPath = path.join(groupDir, CONTRACTS_FILE); - const tmpPath = `${targetPath}.tmp.${randomBytes(8).toString('hex')}`; + const tmpPath = `${targetPath}.tmp.${tmpSuffix()}`; - await fsp.writeFile(tmpPath, JSON.stringify(registry, null, 2), 'utf-8'); + // O_EXCL via `'wx'` flag + explicit `0o600` mode — closes both halves + // of the CodeQL js/insecure-temporary-file finding: `'wx'` rejects a + // pre-planted symlink at the path, and `0o600` (user-only) prevents + // the file from being created group/world readable while it briefly + // contains contract data en route to the rename. The query's + // `isSecureMode` predicate inspects ONLY the mode argument, not the + // flags, so the explicit mode is what credits the fix. + const handle = await fsp.open(tmpPath, 'wx', 0o600); + try { + await handle.writeFile(JSON.stringify(registry, null, 2), 'utf-8'); + } finally { + await handle.close(); + } await fsp.rename(tmpPath, targetPath); } @@ -107,6 +128,38 @@ matching: # exclude_links_paths: [/ping, /health, /healthcheck] # exclude_links_param_only_paths: false `; - await fsp.writeFile(path.join(groupDir, 'group.yaml'), template, 'utf-8'); + // Always write group.yaml with O_EXCL via `fsp.open(..., 'wx')` — + // refuses to follow a pre-planted symlink at the target path, closing + // the TOCTOU window between the existence check (line ~98) and the + // write that CodeQL js/insecure-temporary-file flags. Under + // `force=true` we unlink the existing file first (best-effort, no-op + // when absent) so the subsequent O_EXCL open succeeds AND the same + // symlink-rejection guarantee holds — this is strictly safer than + // the previous `flag: force ? 'w' : 'wx'` shape, which silently + // followed symlinks under force. CodeQL's rule does not recognize + // the `writeFile(path, content, { flag: 'wx' })` shape as O_EXCL; + // the explicit open() handle below is what credits the mitigation. + const yamlPath = path.join(groupDir, 'group.yaml'); + if (force) { + try { + await fsp.unlink(yamlPath); + } catch (err) { + // ENOENT (file absent) is expected on first run; rethrow anything + // else so we don't silently mask permission/EBUSY failures. + if ((err as NodeJS.ErrnoException).code !== 'ENOENT') throw err; + } + } + // `'wx'` rejects a pre-planted symlink at the path; `0o600` is + // user-only (no group/world bits) — gitnexus storage is per-user + // (`~/.gitnexus/...`), so any "other user wants to read this" case is + // a misconfiguration, not a feature. Keeping the file user-only also + // satisfies CodeQL's `isSecureMode` predicate (low 6 bits == 0) and + // closes the js/insecure-temporary-file alert at this site. + const handle = await fsp.open(yamlPath, 'wx', 0o600); + try { + await handle.writeFile(template, 'utf-8'); + } finally { + await handle.close(); + } return groupDir; } diff --git a/gitnexus/src/core/ingestion/vue-sfc-extractor.ts b/gitnexus/src/core/ingestion/vue-sfc-extractor.ts index 382417c56..f36a85ab4 100644 --- a/gitnexus/src/core/ingestion/vue-sfc-extractor.ts +++ b/gitnexus/src/core/ingestion/vue-sfc-extractor.ts @@ -23,7 +23,24 @@ interface ScriptBlock { lang: string; } -const SCRIPT_RE = /]*)?>([^]*?)<\/script>/g; +// Closing-tag pattern accepts: +// - whitespace before `>` — ``, `` +// - attribute-like junk after `script` — ``, +// `` +// - any case — ``, `` +// +// HTML5 parses `` as a valid close tag (attributes on +// close tags are ignored by the parser but still terminate the script +// block). A strict `<\/script\s*>` would miss those forms and let a +// crafted Vue file hide content from this extractor — exactly the +// CodeQL `js/bad-tag-filter` failure mode (the published test cases +// it checks include `` and ``). +// +// `[^>]*` after ``, +// matching the HTML parser's actual close-tag behaviour. The `i` flag +// covers the case axis. PR #1330 CI surfaced both the case and +// attribute axes; this expression closes both at once. +const SCRIPT_RE = /]*)?>([^]*?)<\/script[^>]*>/gi; const TEMPLATE_COMPONENT_RE = /<([A-Z][A-Za-z0-9]+)/g; // Greedy: matches from the first . // This is intentional — nested