Eight sites rely on inline codeql/lgtm suppressions that this repo documents as non-functional; two alerts are open behind them #16172

Activity

1. github-actions commented on Sep 10, 2026

   

   github-actionsbot

   on Sep 10, 2026 – with GitHub Actions

   Contributor

   More actions

   👋 Auto-triage could not confidently classify this issue (prose matched 4 skill areas; too ambiguous to label). Could a maintainer add the appropriate labels (frontend, backend, infrastructure, docs, testing and good-first-issue, intermediate, advanced)?
2. mrveiss commented on Sep 10, 2026

   

   mrveiss

   on Sep 10, 2026

   OwnerAuthor

   More actions

   Upgrading this: the second mechanism does not work either, and it has been believed to work since June.

   CODEQL_SUPPRESSION.md presents query-filters as the mechanism that does work — "Suppresses future alerts? Yes". Alert #1030 is a counter-example, and the dates are unambiguous:

   config exclusion for external_importer.py landed   717ec9863   2026-06-04
   alert 1030 raised on external_importer.py:331                  2026-07-25
                                                                  ^ seven weeks later
   

   .github/codeql/codeql-config.yml:26-35 excludes py/full-ssrf for exactly that file. CodeQL raised a py/full-ssrf alert on exactly that file anyway.

   Why the other excluded file looks fine, and why that is misleading

   autobot_shared/security/ssrf_guard.py is excluded by the same construct and has no open alerts — so the filter appears to work. It does not. Its own comment records what actually cleared them:

   Pre-existing on main (alerts 1042/1045, both dismissed as false positives)
   

   Dismissed, not filtered. Both excluded files ended up clean-or-not purely according to whether someone dismissed their alerts by hand. The filter has never demonstrably suppressed anything.

   Suspected cause, stated as a suspicion

   The exclusion blocks use a files: key nested inside query-filters. I have not confirmed against the CodeQL schema whether files: is a recognised field there — path scoping is normally expressed with paths-ignore at config top level, and query-filters normally take id/tags. If files: is not a recognised key it would be ignored silently, which matches the evidence exactly: three exclusion blocks, no observable effect, no error, no warning, for three months.

   I am not asserting the schema. What is measured is that the filter landed and the alert followed.

   What this does to the backlog number

   The coordinator measured 129 open alerts and established that 3 carry inline suppressions, leaving 126 genuine decisions. That correction assumed the config-level exclusions were doing their job. If they are not, the inflation is larger and differently shaped — every finding in a config-excluded path is being reported as an outstanding decision when a maintainer already recorded one.

   Currently 127 open (two dismissed under #16165). The three exclusion blocks cover external_importer.py, ssrf_guard.py and one py/path-injection path — so the direct inflation is small, but the mechanism being broken means no future exclusion will hold either, and that is the part that matters for "solve all CodeQL alerts" as a standing goal.

   Additional finding on the same block

   The external_importer.py exclusion's reason describes the git path — _validate_git_url(), _run_git(), ssh/https scheme allowlist, #MVA-2605. Alert 1030 is on line 331, the HTTP catalog fetch, guarded by a different function (_validate_catalog_url()) with different rules — it permits http and has no host allowlist.

   So even had the filter worked, its stated justification would not have covered the alert it suppressed. A reader checking whether the exclusion legitimately covers line 331 finds a reason about a different code path.

   Added criteria

   • Establish empirically whether files: inside query-filters is honoured — e.g. by a deliberate test exclusion on a known-alerting path — rather than by reading the config and assuming
   • If it is not honoured, the three existing blocks are re-expressed in a form that is, and CODEQL_SUPPRESSION.md's claim that query-filters suppresses future alerts is corrected
   • The external_importer.py exclusion's reason covers both outbound paths in that file, or is split so each path's justification names its own guard
   • A check fails if a config exclusion exists for a path that still has open alerts of that rule — the condition that went unnoticed for three months
3. mrveiss commented on Sep 10, 2026

   

   mrveiss

   on Sep 10, 2026

   OwnerAuthor

   More actions

   Measured, not inherited: inline suppressions are decorative — with the alert IDs to prove it

   codeql-config.yml:5-9 already claims this (citing #12307). Here it is demonstrated against the alert API, on the branch that owns alert state.

   Method — match each suppression's file:line and the rule it names against alerts for that same rule at that same file, rather than counting alerts per file. Per-file state is not per-alert: these files carry py/clear-text-**logging**-sensitive-data alerts too, and the suppressions name py/clear-text-**storage**-sensitive-data. Reading the file-level mix would have produced a confident wrong answer in either direction.

   suppression (file:line)	rule it names	alert for that rule	state
   migrate_secrets_ownership.py:331	py/clear-text-storage-sensitive-data	line 332	open
   validate_migration.py:551	same	line 552	open
   security_scan.py:391	same	line 392	open
   security/mtls-migrate.py:91	same	line 92	open

   The comment sits one line above the flagged expression in every case. The suppression is present, names the correct rule, and the alert is open.

   Confound closed: all open alerts live on refs/heads/main, so I verified the comments exist there, not only on Dev_new_gui — git show origin/main:<file> returns # codeql[py/clear-text-storage-sensitive-data] at each of those four lines. The suppression and the open alert are on the same branch, at the same line, for the same rule.

   Two sites that are inert for a different reason

   • export_service_keys.py:131 names py/clear-text-storage-sensitive-data and there is no alert of that rule at that file in any state. It suppresses something that never fired.
   • tts-worker.py.j2:646,647 name py/path-injection; that file's two py/path-injection alerts are at lines 354 and 355 and are dismissed. The dismissals came from the API — the mechanism that works — and the comments at 646/647 correspond to no alert at all.

   So of the eight live sites: four sit beside a live open alert, two suppress nothing that fires, and two were actually handled by dismissal elsewhere in the file.

   On the count — the issue is right and I was wrong to doubt it

   git grep -cE '#\s*(codeql|lgtm)\[' returns 15 matches, which is what a naive sweep reports. The split:

   where	count	live code?
   docs/developer/CODEQL_SUPPRESSION.md	5	no — documentation examples
   CHANGELOG.md	1	no
   .github/codeql/codeql-config.yml	1	no — the comment explaining they don't work
   live code	8	yes

   Eight is exactly right. I nearly filed a correction saying otherwise; the 15 is my grep's population, not this issue's.

   What this does and does not license

   It licenses removing the eight inline comments only alongside the working mechanism — an API dismissal with a reason, or a query-filters entry. Removing them alone changes nothing about alert state and loses the reviewer's recorded judgement about why the finding was acceptable, which is the only part of them with any value.

   It does not license concluding anything about query-filters — that is #13697 and remains undetermined for reasons recorded there.
4. mrveiss

   modified the milestones: Backlog, v0.16.0 on Sep 12, 2026
5. mrveiss commented on Oct 8, 2026

   

   mrveiss

   on Oct 8, 2026

   OwnerAuthor

   More actions

   Decisive evidence that the inline comments are inert

   This came up while unblocking code-quality on main (#18096), and it settles the "appear not to suppress" wording with a case that cannot be read two ways.

   autobot-frontend/src/components/settings/SettingsPanel.vue:236 carries:

   obj[finalKey] = value // codeql[js/prototype-pollution-utility]

   And the code-scanning API has an alert for that exact rule, on that exact line, in state dismissed with dismissed_reason: "false positive":

   autobot-frontend/src/components/settings/SettingsPanel.vue:236
     js/prototype-pollution-utility   false positive
   

   If the inline comment suppressed anything, the alert would never have been raised at all. It was raised, and then a human dismissed it. So the comment did not suppress it — the dismissal did. One confirmed instance is enough to establish the mechanism; this is not an inference from absence.

   The scale of the real mechanism, measured today:

   	count
   dismissed alerts repo-wide	393
   …sampled reasons	every one false positive
   codeql[...] comments in the tree	a handful (frontend + autobot_shared/security/ssrf_guard.py)

   393 adjudications against a handful of comments is the ratio you would expect if the comments were decorative and dismissal were doing all the work.

   Why "no open alert on that line" is NOT evidence the comment works

   Worth recording the trap, because it is how I first read this and it is wrong. For the other two commented sites there is no alert at the exact line, which looks like the comment working. It is not evidence either way: the rule may simply not have fired there, or the alert may sit on a neighbouring line after a code shift. The only probative case is the one above — an alert that was raised on a commented line. Absence of an alert is consistent with both "suppressed" and "never triggered", so it cannot distinguish them.

   Why this costs something concrete

   A comment that implies protection it does not provide is the same class as a sanitiser that sanitises nothing. It was load-bearing in my reasoning an hour ago: when f6b64b67d0 left 2 open alerts at useVoiceConversation.ts:152, my first fix was to move the codeql[...] comment onto the newly-anchored line — because the three comments already in that file read as a working suppression. That fix could not have worked, and I only found out by measuring.

   The fix that landed instead removes the need for any suppression there: script removal moved inside the existing while (prev !== clean) fixed-point loop, which is the canonical complete form for js/incomplete-multi-character-sanitization. useVoiceConversation.ts now carries zero codeql[...] comments. That is the pattern worth generalising for the remaining sites — make the code satisfy the rule, rather than annotate past it.

   Also relevant to the gate side, measured at pipeline-scripts/check_codeql_alert_ceiling.sh:38: it reads code-scanning/alerts?state=open with no ref, so it counts main's alerts regardless of which PR is running. A PR cannot clear the ceiling by fixing code — the check never looks at the PR. That is worth knowing before anyone tries to fix an alert "to make CI green".

   Cross-refs: #16165 (the open-alert backlog), #16166 (the same inert-suppression shape for nosemgrep), #13697 (query-filter exclusions not suppressing).
6. mrveiss

   added 2 commits that reference this issue on Oct 8, 2026

   fix(voice): close the script end-tag hole and sanitize to a fixed poi…

   ...

   8f7cf07

   fix(voice): remove markup by PARSING, which fixes prose the regex was…

   ...

   cc55e46