fix: address CodeQL real-bug findings (6 surgical changes)#170
Merged
Conversation
Six targeted fixes for findings from the just-enabled CodeQL scan (#166). Each fix is independently verifiable; tests added for the two with subtle correctness implications. 1. apps/web/src/lib/github.ts — drop `.replace(/-/g, "-")` from `formatTagName`. Identity replacement; dead code from a prior refactor. 2. packages/annotator/src/annotator.ts — `yamlEscape()` now escapes backslashes before double quotes. Previously, a `caseName` like `X \ Y` would emit `"X \ Y"` (raw backslash) which YAML readers interpret as an escape sequence start. CWE-116 (incomplete sanitization). Test added. 3. packages/fetcher/src/fetcher.ts — `parseReleasePoints` regex anchored to the literal path structure (`/download/releasepoints/ us/pl/.../xml_usc...zip`) and constrains each segment to non-slash/non-quote chars. Removes the `[^"]*` prefix that gave CodeQL js/polynomial-redos backtracking exposure. ReDoS-resistance test added (40k-char malformed input must return in <100ms). 4. scripts/bulk-import.ts — replace predictable `/tmp/usc-title-N` paths with `mkdtemp(tmpdir() + 'usc-title-')` (random per-call dir). Also drops the `statSync(tmpZip)` → `readFile(tmpZip)` pair (TOCTOU race) since the buffer's `.length` is sufficient. Single finally block now handles cleanup. 5. scripts/lib/import-helpers.ts — same temp-dir hardening as #4 for the historical import path (`downloadAndExtractXml`). Verified locally: pnpm build, test (118+74+... = 269 passing — two new tests), typecheck (0 errors), lint all green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Six targeted fixes for findings from the CodeQL scan enabled in #166. Each is independent and small. Tests added for the two with subtle correctness implications.
apps/web/src/lib/github.ts:160js/identity-replacement.replace(/-/g, "-").packages/annotator/src/annotator.ts:64,65,68(×3)js/incomplete-sanitization(CWE-116)yamlEscape()that escapes backslashes before double quotes. Previously, acaseNamelikeX \ Ywould emit a raw backslash that YAML readers interpret as an escape-sequence start. Test added.packages/fetcher/src/fetcher.ts:128js/polynomial-redos/download/releasepoints/us/pl/.../xml_usc…zipand replace[^"]*segments with[^"/]+. Test added (40k-char malformed input returns in <100ms).scripts/bulk-import.ts:53–54, 62, 66js/insecure-temporary-file,js/file-system-race/tmp/usc-title-Npaths withmkdtemp(tmpdir() + 'usc-title-'). DropstatSync→readFilepair (TOCTOU); derive size fromheader.length.scripts/lib/import-helpers.ts:157–158, 178js/insecure-temporary-fileFindings NOT addressed here
js/http-to-file-access(×3) — the pipeline's purpose is fetch-remote-XML → write-file. Will dismiss in CodeQL UI as "won't fix - by design" with this PR linked as context.js/insecure-temporary-fileinpackages/pipeline/src/orchestrate.ts:208,243— these are product output writes (statute Markdown + annotation YAML) to a configuredoutputDir, not temp files. CodeQL's pattern matcher misclassifies them; path traversal is already guarded byisSafePath. Will dismiss "won't fix - false positive".github.ts(sanitizeContent/sanitizeExcerpt) — deferred to a follow-up PR (PR F) that swaps insanitize-htmllibrary; bundling here would require both a runtime-dep addition and a golden-output regression suite that's worth its own review.Test plan
pnpm install,pnpm build,pnpm test(120 fetcher + ... = 269 tests; +2 new),pnpm typecheck(clean),pnpm lint(clean) all green locally🤖 Generated with Claude Code