fix(security): clear CodeQL self-gate (complete sanitization, no double-escaping, atomic FS ops)#39
Conversation
…g, FS races) The CodeQL self-gate (security-extended, tier:block) was RED on main, blocking every PR. Fix all gated findings properly — no in-source suppression: - js/incomplete-multi-character-sanitization (transmitter visibleLength, rich validateRichHtml): a `/<[^>]+>/g` tag-strip left a dangling, unterminated `<script` behind. New shared stripHtmlTags() removes a trailing tag-like fragment too, in one pass, while preserving a lone literal `<` in text. - js/double-escaping (transmitter visibleLength, table.test): chained entity `.replace()`s could feed one decode into the next (`&lt;`). New shared decodeHtmlEntities() consumes each entity exactly once via a lookup table. - js/incomplete-sanitization (rich RICH_TAG_RE builder): the regex was built escaping only `-`. New shared escapeRegExp() escapes the full ES metachar set; install-skill's local escapeRegex is migrated onto it (DRY). - js/file-system-race (install appendMarked + ensureSessionStartHook, telegram blobFor): replace check-then-read/-write TOCTOU pairs with atomic ops — readTextIfExists() (single read, catch ENOENT) and readSmallFile() (one fd: open -> fstat -> read -> re-fstat guard), backing up the in-memory snapshot rather than re-reading the path. - js/file-access-to-http (scripts/*.ts, 10 findings): scope the gate to the shipped product surface via .github/codeql/codeql-config.yml paths-ignore: scripts — by-hand dev asset-generators, never shipped, no untrusted input. features/ keeps full coverage incl. this query. New shared helpers live in features/render/html.ts (stripHtmlTags, decodeHtmlEntities) and features/util/regex.ts (escapeRegExp), with unit tests (html-sanitize, read-small-file) plus visibleLength integration cases. bun test: 1031 pass / 0 fail. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2158becd4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // from that same in-memory copy. A separate existsSync()/readFileSync() per | ||
| // use would re-stat the path each time, racing with concurrent writers | ||
| // (js/file-system-race); a single read snapshots the bytes we then act on. | ||
| const original = readTextIfExists(settingsPath); |
There was a problem hiding this comment.
When ~/.claude/settings.json exists but cannot be read as a normal file (for example it is a directory, has bad permissions, or a transient read error occurs), readTextIfExists() rethrows before the existing parse/error guard runs. The previous implementation had the read inside the try and returned false, conservatively skipping the hook; this change now aborts install-skill after it may already have written other files. Move this read back under the guard or catch non-ENOENT errors here and return false.
Useful? React with 👍 / 👎.
1 participant
Why
The CodeQL self-gate (
security-extended,tier: blockin rig.yaml) was RED onmain, blocking every tg-cli PR — including the socket-leak crash-fix #37. Root cause: 18 gatedwarning-level findings (8 in product code, 10js/file-access-to-httpin by-hand dev scripts).What
All findings fixed properly — no in-source suppression of the product-code bugs.
js/incomplete-multi-character-sanitizationtransmitter.ts,rich.ts/<[^>]+>/gleft a dangling<script; new sharedstripHtmlTags()also strips a trailing tag-like fragment, one pass, preserving a lone literal<js/double-escapingtransmitter.ts,table.test.ts.replace()s could chain-decode (&lt;); new shareddecodeHtmlEntities()consumes each entity once via a lookup tablejs/incomplete-sanitizationrich.tsRICH_TAG_RE-; new sharedescapeRegExp()escapes the full ES metachar set (install-skill's localescapeRegexmigrated onto it)js/file-system-raceinstall.ts,telegram.tsreadTextIfExists()(single read, catch ENOENT) andreadSmallFile()(one fd: open→fstat→read→re-fstat guard);.bakwritten from the in-memory snapshotjs/file-access-to-http(×10)scripts/*.ts.github/codeql/codeql-config.ymlpaths-ignore: scripts— by-hand emoji/sticker generators, never shipped, no untrusted input.features/keeps full coverage incl. this queryNew shared helpers:
features/render/html.ts(stripHtmlTags,decodeHtmlEntities),features/util/regex.ts(escapeRegExp).Tests
bun test: 1031 pass / 0 fail. New:tests/html-sanitize.test.ts,tests/read-small-file.test.ts, plusvisibleLengthintegration cases pinning the dangling-tag and&lt;edge cases the findings flagged.Review
Pre-commit
review --staged -m kimi,glm,qwen,minimax,deepseek(3 rounds). The opencode-backed models (kimi/qwen/minimax/deepseek) were down (provider 500s / db-locked) throughout; GLM reviewed each round and every legitimate finding was addressed: hardenedstripHtmlTags(tag-like trailing only),readSmallFilegrow-guard (re-fstat), corrected the misleadingvisibleLengthcomment, strongerescapeRegExp/RICH_TAG_REtests, andpaths-ignoreover a query-level exclude (which would have blinded product code).🤖 Generated with Claude Code