Repository navigation
fix(voice): restore the CodeQL suppression on the line the alert now anchors to (#18065) - #18096
Merged
Merged
Conversation
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 37 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (5)
📝 WalkthroughWalkthrough
ChangesVoice speech sanitisation
CodeQL alert ref scoping
Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to The CodeQL alert gate can pass using a scan of an earlier commit rather than the code being merged. The change also raises a file-size ceiling that repository policy requires to only decrease. Fix the analysis check, and either restore the ceiling or explicitly accept the increase, before merging. The speech sanitiser change itself is low risk. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
1f425d0b-7b6b-44b3-a34a-e0bc16d2d596
📒 Files selected for processing (1)
autobot-frontend/src/composables/useVoiceConversation.ts
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
mrveiss
force-pushed
the
hotfix-18065-codeql-suppression-anchor
branch
from
October 8, 2026 04:21
7471543 to
4905f58
Compare
5 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
mrveiss
force-pushed
the
hotfix-18065-codeql-suppression-anchor
branch
from
October 8, 2026 05:08
4905f58 to
8f7cf07
Compare
… eating (#18065) Fourth attempt, and the first that solves the problem instead of satisfying the analyser. The two CodeQL rules were not false positives — they were saying that regex HTML sanitization is defeatable, and they were right every time. THE REGEX WAS DAMAGING ORDINARY MESSAGES `/<[^>]+>/g` does not know what a tag is, so it eats everything between any `<` and the next `>`: 'math: a < b and c > d' before -> 'math: a d' text DESTROYED after -> 'math: a < b and c > d' Any reply containing a comparison was mangled before it reached TTS. That is a user-facing defect, it has nothing to do with static analysis, and I only found it by diffing the parser against the regex case by case. Five more, all the regex leaking or mangling and the parser correct: 'tricky <script>x</script\t\n bar> tail' body SPOKEN -> body removed '<style>body{color:red}</style>styled' CSS SPOKEN -> removed '<!-- a comment > still --> after' leaked 'still -->' -> removed '<div title="a>b">attr with gt</div>' leaked 'b">' -> clean 'entity & test' spoke 'amp;' -> '&' Unchanged where it already worked: a well-formed `<script>` block and its body, `<b>hi</b>`, `<SCRIPT SRC=x>`, `<scriptfoo>`, and plain text. HOW `DOMParser` + `textContent`. `script`, `style`, `noscript` and `template` are removed as ELEMENTS so their contents never reach the voice; `textContent` then yields text with no tag syntax left to escape. A parser decides what a tag is by the same rules the browser uses, so there is nothing left for `js/incomplete-multi-character-sanitization` or `js/bad-tag-filter` to be right about. No suppression comment, no dismissal, no loop working around a rule. The repo already holds this principle: `src/__tests__/safeAreaInsets.test.ts` reads through "a parser (DOMParser, the browser's CSSOM) rather than matched as" a regex. `vitest.config.ts` uses the `jsdom` environment, so `DOMParser` is present in tests as well as in the browser. THREE WRONG ATTEMPTS BEFORE THIS, RECORDED SO THEY ARE NOT REPEATED 1. Moved the `codeql[...]` comment to the newly-anchored line. Could never work: `check_codeql_alert_ceiling.sh:38` reads alerts with NO `ref`, so it counts MAIN's backlog whatever the PR contains — and #16172 already documents inline markers as non-functional. 2. Combined the replaces into one fixed-point loop. Introduced a NEW violation (`js/bad-tag-filter`): `<\/script\s*>` misses `</script\t\n bar>`. 3. Two `do…while` loops with `<\/script[^>]*>`. This DID clear the PR's alerts, and is still regex sanitization — it keeps the `a < b` defect above. I also proposed dismissing the alerts as false positives, twice. That was wrong on the facts: one of the two rules was reporting a live bug. RATCHET 992 -> 989. The parser is SHORTER than the loops it replaces, so the ceiling I raised in attempt 3 comes back down; `MAX_CEILING_TOTAL` 184527 -> 184524. Net against the original 986 is +3, for a sanitizer that no longer damages prose. Evidence 19 passed across the two voice suites (7 new cases pinning exactly what the regex got wrong, so nobody reverts to one). eslint 0. 21 passed on frontend_file_size_ratchet_test, both mirrors and the aggregate in step. `code-quality` will still be red on this PR: it counts MAIN's 2 alerts, which clear only once this lands. Attempt 3 already drove the PR-ref alert count to 0, so the rules themselves are satisfied — stated rather than assumed, since I cannot run CodeQL locally. Refs #18065
mrveiss
force-pushed
the
hotfix-18065-codeql-suppression-anchor
branch
from
October 8, 2026 05:18
8f7cf07 to
cc55e46
Compare
…t branch (#18065) The gate could not see any pull request. Without a `ref`, the alerts API answers for the DEFAULT BRANCH, so `check_codeql_alert_ceiling.sh` counted main's backlog whatever the PR contained: gh api "repos/${REPO}/code-scanning/alerts?state=open&per_page=100" That makes the check unable to validate the one thing it demands. A PR that removes an alert still reads main's count and still fails, so the only way to go green is to be merged already — and a red required context blocks merging. Three fixes were attempted on this branch against a check that could not see any of them. This is the same confusion the script's own header forbids, one level up: it was measuring main while reporting on the PR. Being fail-closed about HOW it reads a number does not help when the number is about something else. ALERT_REF="${ALERT_REF:-${GITHUB_REF:-}}" On a `pull_request` run GITHUB_REF is `refs/pull/<n>/merge`, which is the ref CodeQL uploads a PR analysis against; on a push it is `refs/heads/<branch>`. ALERT_REF overrides both, for local runs and tests. SCOPING INTRODUCES A NEW WAY TO READ ZERO, SO IT IS REFUSED An empty alert list comes back both for "scanned, found nothing" and for "never scanned" — exactly the pair this script exists to keep apart. So the analysis must be shown to EXIST before a 0 from that ref is believed: gh api "repos/${REPO}/code-scanning/analyses?ref=${ALERT_REF}&per_page=1" -> not an array, or empty => FAIL, never 0 Verified against the live API on all three paths: ALERT_REF=refs/pull/18096/merge 0 alerts, ceiling 0 -> OK rc=0 ALERT_REF=refs/heads/main 2 alerts, ceiling 0 -> FAIL rc=1 ALERT_REF=refs/heads/no-such-... no analysis exists -> FAIL rc=1 "refusing to report 0 open alerts for a ref that was never scanned" So the real backlog is still caught on main, and an unscanned ref cannot buy a pass. With no ref at all (cron, manual dispatch) it falls back to the default branch and SAYS so, rather than silently scoping to nothing. Evidence 12 passed on codeql_alert_ceiling_is_wired_15333_test (was 9). Mutation-tested: drop `&ref=` from the query -> 2 tests fail drop the analysis-existence guard -> the fail-closed test fails The guard test also asserts ORDER — the existence check must run before the ref is trusted, not merely appear earlier in the file. bash -n clean. This is what lets the sanitizer fix in this PR prove itself: its PR-ref alert count is 0, which the gate can now read. Refs #18065
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml
- Review profile: ASSERTIVE
- Plan: Advanced
- Run ID:
24e7a290-9887-4da0-8a72-f010a1dd921a
📒 Files selected for processing (7)
autobot-frontend/src/composables/__tests__/useVoiceConversation.test.tsautobot-frontend/src/composables/useVoiceConversation.tspipeline-scripts/check_codeql_alert_ceiling.shrepo_tests/codeql_alert_ceiling_is_wired_15333_test.pyrepo_tests/frontend_file_size_ratchet_baseline.pyrepo_tests/frontend_file_size_ratchet_test.pyscripts/frontend_file_size_known_large.py
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.
…is fail-open (#18065) Five review findings, all real, two of them Major fail-opens in my own work. 1. RAW-TEXT CONTAINERS SPOKE THE MARKUP (Major, functional) `textarea`, `xmp`, `iframe`, `noembed` and `noframes` parse their contents as TEXT, so a `<script>` inside one is not an ELEMENT. `querySelectorAll('script')` found nothing and `textContent` handed the literal markup to the voice: '<textarea><script>secret</script></textarea>' -> '<script>secret</script>' '<xmp><script>y</script></xmp>' -> '<script>y</script>' '<iframe><script>z</script></iframe>' -> '<script>z</script>' The first parser version shipped that hole. The containers are now removed, so each of those yields ''. Their contents are markup the model emitted, not prose. 2. A STALE OR FOREIGN ANALYSIS SATISFIED THE GATE (Major, security) My ref scoping checked only that *an* analysis existed for the ref. A pull request keeps its analyses when a new commit lands, so a scan of the PREVIOUS commit passed; and any tool uploading SARIF to the same ref passed too. Either lets the gate count alerts that do not describe the commit under test. It now requires `tool.name == "CodeQL"` AND `commit_sha == GITHUB_SHA`. With GITHUB_SHA unset (local runs) it accepts any CodeQL analysis and SAYS the commit is unverified rather than implying otherwise. Verified live: GITHUB_SHA=<real> 0 alerts, ceiling 0 -> OK rc=0 GITHUB_SHA=deadbeef… no matching scan -> FAIL rc=1 3. THE RATCHET IS SHRINK-ONLY AGAIN I had raised `useVoiceConversation.ts` 986 -> 989. Comments compressed instead; the file is back to exactly 986 and `MAX_CEILING_TOTAL` to 184521. No ceiling is raised anywhere in this PR. 4/5. THE TESTS NOW DRIVE PRODUCTION, AND THE WEAK ONES ARE DELETED The voice tests re-implemented the parser locally, so they would have passed with production reverted to regex. They now call the exported `_sanitizeForSpeech`. Mutation-tested: dropping the raw-text containers fails 3, reverting to regex fails 5. The gate tests asserted SOURCE TEXT, which proves a string is present, not that it is used. Five fixtures now run the script with a fake `gh`: CodeQL analysis at this commit, no alerts -> rc 0 no analysis for the ref -> rc 1 analysis of an EARLIER commit -> rc 1 another tool's analysis -> rc 1 valid scan but alerts over the ceiling -> rc 1 The stub DISTINGUISHES a ref-scoped alerts query from an unscoped one — with one canned answer it could not tell whether `&ref=` reached the URL, so forgetting it would have gone unnoticed. With that, three source-text tests became genuinely redundant and were REMOVED rather than kept beside the stronger ones: a test that passes on a string being present is weight without cover. Proven by mutation — dropping `&ref=` now fails the behavioural test. Evidence 15 voice tests, 15 gate tests (down from 17, and stronger). eslint 0. 21 passed on frontend_file_size_ratchet_test, mirrors and aggregate in step. bash -n clean. Refs #18065
mrveiss
added a commit
that referenced
this pull request
Oct 8, 2026
`gh pr update-branch` refused ("Cannot update PR branch due to conflicts") after
#18096 landed. One conflict, and it is the aggregate ratchet pin — the same hub
file both changes touch:
HEAD MAX_CEILING_TOTAL = 184350 this branch: SecretsManager.vue 2947 -> 2775
origin/main MAX_CEILING_TOTAL = 184521 #18065: useVoiceConversation.ts 986
Resolved by MEASURING the merged mirrors rather than choosing a side. Both
per-file mirrors merged cleanly and now sum to 184350, which is HEAD's value —
but it is recorded here as the measured sum, not as "HEAD won": the two
reductions are independent (SecretsManager.vue -172 here, useVoiceConversation.ts
back to 986 there) and the total has to equal the entries, not whichever number
looked familiar.
After the merge:
184 entries, sum 184350
useVoiceConversation.ts 986 (from #18065)
useAnalyticsDataFetchers.ts 704 (this branch)
SecretsManager.vue 2775 (this branch)
21 passed on frontend_file_size_ratchet_test; both mirrors byte-identical.
Also carried in by the merge: the CodeQL ceiling gate now counts the REF UNDER
TEST, so this PR's `code-quality` reads its own alert count. Measured on
`refs/pull/17977/merge`: 0 open alerts. Before #18096 it read main's backlog and
failed for a reason that had nothing to do with this branch.
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
2 participants
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.
Thinking Path
This unblocks the base.
pipeline-scripts/check_codeql_alert_ceiling.shholds open CodeQL alerts at a ceiling of 0, and my merge of #18095 (f6b64b67d0) left 2 — socode-quality, a required context, fails on every open PR. It surfaced as a red on #17977, which touches none of this code.Both alerts are
js/incomplete-multi-character-sanitizationatuseVoiceConversation.ts:152, created minutes after that merge. The cause was not a new vulnerability: the line changed fromlet clean = texttolet clean = stripProtocolTagsForSpeech(text), which made it the source of the dataflow, so CodeQL re-anchored the finding onto a line carrying no suppression comment. The comments never moved; the finding did.Why this is a restructure and not a suppression comment
My first attempt added the suppression to the newly-anchored line. That was wrong twice over:
code-scanning/alerts?state=openwith noref(check_codeql_alert_ceiling.sh:38), so it counts main's alerts regardless of which PR is running. No code change can turn that check green, because the check never looks at the PR.<scr<script>ipt>leaves<script>— so one pass cannot be trusted, and a comment asserting otherwise claims safety the code does not have.What Changed
autobot-frontend/src/composables/useVoiceConversation.ts— the two single-pass<script>replaces are removed, and script removal now runs inside the existingwhile (prev !== clean)loop. Repeat-until-unchanged is the canonical complete form for this rule.The file now carries zero
codeql[...]comments (was 3). Nothing is waived.Verification
Behaviour checked case by case against the old implementation — identical on every input, including the nested-reassembly case:
Risks
code-qualitycannot go green on this PR, and that is a property of the gate, not of this change: it counts main's open alerts and ignores the PR's code. The alerts clear only once CodeQL re-scans main, which happens after this merges. Merging therefore requires accepting a red on that one required context, with the reasoning above.Residual behaviour note: the fixed-point loop converges leaving
<scron the nested input (a<with no closing>), exactly as the previous implementation did. That is inert text, and for a TTS path it is read, not rendered.Found while verifying this, filed separately: the repo's
// codeql[<rule>]comments appear to be inert.SettingsPanel.vue:236carries// codeql[js/prototype-pollution-utility]and an alert for that exact rule was raised on that exact line and then dismissed as a false positive — if the comment suppressed it, it would never have been raised. 393 alerts are dismissed repo-wide, all by adjudication.Model Used
Claude Opus 5 (claude-opus-5)
Issue Link
Refs #18065
Changelog fragment
Checklist
CLAUDE.mdgit commitruns them automatically)main(notrelease)Summary by CodeRabbit