Skip to content

fix: close the CodeQL alerts in shipped code - #24

Merged
dgesteves merged 1 commit into
mainfrom
fix/codeql-alerts
Nov 2, 2026
Merged

dgesteves merged 1 commit into
mainfrom
fix/codeql-alerts

Conversation

@dgesteves

Copy link
Copy Markdown
Owner

Default setup had left 11 CodeQL alerts open on code Sigil ships, since the first commits on 2026-11-01. Ten are fixed here with tests. The eleventh is a false positive; reasoning below.

Alerts Where Fix
#7-#11 prototype-polluting assignment compile/merge.ts JSON pointers come from capability payloads (an MCP server's name, a hook's event) and from the lockfile, so they are untrusted. A __proto__, constructor or prototype segment is now refused with a MergeError before anything is written, which sigil add reports as a conflict. Reads (get, revert, pruneContainers, mergeStatus) treat such a pointer as missing, and own-property checks replace in and === undefined. A key like toString can still be used as a plain name.
#16 polynomial ReDoS compile/merge.ts Trailing newlines are trimmed with a loop instead of /\n+$/.
#4, #5 incomplete multi-character sanitization site/src/lib/server/markdown.ts Heading text comes from marked's text renderer, with inline HTML dropped, instead of regex-stripping tags from rendered HTML. slugify now receives plain text. The method page's anchors are unchanged (tested against the real METHODOLOGY).
#6 incomplete sanitization site/src/lib/oracle/intent.ts Technology words are regex-escaped in full; a ( used to throw.
#15 URL substring check Observatory.tsx The fetch log compares the parsed hostname.

Controls run: with the old merge.ts, every new test fails. Trimming 200k newlines took 57 s, and the prototype writes polluted Object.prototype badly enough to break unrelated tests later in the run. With the fix, all 314 package tests pass. The old regex escaping throws on C(.

#12 (shell command from environment, lab/exec.ts) is a false positive. All four flows in the SARIF end in child_process.spawn without a shell (docker cp and rm -rf -- inside the sandbox), so argv is passed verbatim and a path cannot inject a command. The one script that embeds a path already quotes it (shellQuote in lab/tree.ts). It should be dismissed as a false positive rather than worked around in code.

Also verified: typecheck, lint (the 4 warnings are pre-existing in lab/), and 11 site tests.

compile/merge.ts: pointers come from capability payloads and the
lockfile, so a "__proto__", "constructor" or "prototype" segment is now
refused before anything is written (it could change every object in the
CLI process), and reads treat it as missing. Trailing newlines are
trimmed with a loop: /\n+$/ took 57 s on 200k newlines.

site: heading text comes from marked's text renderer instead of
stripping tags with a regex, the Oracle escapes every regex
metacharacter in technology words, and the fetch log compares the
parsed hostname instead of searching the URL.
@vercel

vercel Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated
sigil-lab Ignored Ignored Preview Nov 2, 2026 9:54am UTC

@dgesteves
dgesteves merged commit 4909df0 into main Nov 2, 2026
8 checks passed
@dgesteves
dgesteves deleted the fix/codeql-alerts branch November 2, 2026 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant