Hacktoberfest 2026: die Issues, die Maintainer für den Oktober markiert haben – offen und einsteigerfreundlich. Hacktoberfest-Issues durchsuchen

JS: Some sanitizer guards no longer apply after `if (!check(x)) x = default` (false positives since 2.20.2)

Offen
  • Stimmt etwas nicht?
#22,848 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen

Maintainer antworten meist innerhalb von 1 Tag

Dieses Issue hat noch niemand übernommen.

Bewertung

Schwierigkeit
4/5
Geschätzter Aufwand
3-5 Tage
Anfängerfreundlichkeit
48/100
Issue-Typ
Bug
Klarheit
Größtenteils klar
Aktivitätsstatus
Aktiv
Tech-Stack
javascript
Bereich
security, testing-qa

Rechercherichtung

Start by reading the referenced JavaScript data-flow code in BarrierGuards.qll and sharedlib/Ssa.qll, then compare it with the older barrierGuardBlocksEdge implementation in Configuration.qll and the examples in the issue. Reproduce the reported behavior on the sample guards and inspect existing tests for barrier guards on phi inputs. Done means the default-assignment cases are no longer reported while the early-return and unguarded control cases retain their stated behavior.

Vom Indexierungsmodell aus dem Issue-Text verfasst.

Beschreibung

false-positive

Description of the false positive

Since CodeQL 2.20.2, a sanitizer guard no longer protects a variable when the branch where the check fails assigns a safe default instead of returning, as in if (!["en", "de"].includes(lang)) { lang = "en"; }. After the if, lang is either the default or a value that passed the check, so it is safe, but js/reflected-xss reports the use. The same check followed by an early return is not reported. CodeQL 2.20.1 and earlier did not report it; 2.20.2 and every later release I tried, up to the latest 2.27.2, do. The pattern is common for user-selected settings with a fallback (language, theme, sort key, page size).

It is not specific to one guard or query: a /^\w+$/.test(name) guard behaves the same way, and so does js/resource-exhaustion with an upper-bound check that clamps instead of returning (if (n > 1024) n = 1024;). There n is at most 1024 afterwards, or NaN or negative, which Buffer.alloc rejects with ERR_OUT_OF_RANGE. Assigning the default in an explicit else branch of if (["en", "de"].includes(lang)) is reported as well. Comparisons with constants are not affected: if (lang !== "en" && lang !== "de") lang = "en"; is still not reported, presumably because such conditions get SSA refinement nodes.

Code samples or links to source code

app.js:

const express = require("express");
const app = express();

app.get("/a", (req, res) => {
  let lang = req.query.lang;
  if (!["en", "de"].includes(lang)) {
    lang = "en";
  }
  res.send("<p>" + lang + "</p>"); // reported since the shared data-flow library; lang is "en" or "de" here
});

app.get("/b", (req, res) => {
  let name = req.query.name;
  if (!/^\w+$/.test(name)) {
    name = "guest";
  }
  res.send("<p>Hello " + name + "</p>"); // reported likewise; name matches /^\w+$/ or is "guest"
});

app.get("/c", (req, res) => {
  const lang = req.query.lang;
  if (!["en", "de"].includes(lang)) {
    return res.send("unsupported");
  }
  res.send("<p>" + lang + "</p>"); // not reported (same guard, early return)
});

app.get("/d", (req, res) => {
  res.send("<p>" + req.query.lang + "</p>"); // reported (control)
});

app.listen(3000);

size.js:

const http = require("http");
const url = require("url");

http.createServer((req, res) => {
  let n = parseInt(url.parse(req.url, true).query.n);
  if (n > 1024) {
    n = 1024;
  }
  Buffer.alloc(n); // js/resource-exhaustion: reported since the shared data-flow library; n <= 1024 or NaN here

  const m = parseInt(url.parse(req.url, true).query.m);
  if (m > 1024) {
    return res.end("too large");
  }
  Buffer.alloc(m); // not reported (same guard, early return)
  res.end("ok");
}).listen(8080);
CodeQL js/reflected-xss on app.js js/resource-exhaustion on size.js
2.16.2, 2.20.1 line 29 none
2.20.2, 2.27.2 lines 9, 17, 29 line 9

The pattern occurs in real code. Two examples, each scanned as a whole repository with both versions, where 2.27.2 reports alerts that 2.20.1 does not:

  • HCDP/hcdp_api (API of the Hawaii Climate Data Portal): if(!mesonetLocations.includes(location)) { location = "hawaii"; }, where mesonetLocations is ["hawaii", "american_samoa"] (config.ts), and then path.join(rawDataRoot, "sff", ${location}_sff.csv). js/path-injection reports raw.ts:148 and raw.ts:161, and the same check at L193-L195 leads to a third alert at util.ts:46.
  • flukexp/PiperTTS-API-Wrapper: if (!voices.includes(voice)) { voice = DEFAULT_VOICE; }, where voices are the model file names in a local directory, before voice is used in an exec command line (L86-L95). js/command-line-injection reports L95. The POST handler in the same file performs the same check as a conditional expression (L134) and is not reported.

2.20.2 is the release in which JavaScript moved to the shared data-flow library (codeql/javascript-all 2.3.0, whose change note says users may observe differences in the alerts).

As far as I can tell, the old library handled this case in barrierGuardBlocksEdge, which blocked the flow into a phi node from an input whose use is controlled by the guard. The note in the current BarrierGuards.qll says that this predicate was not carried over because "dataflow2 does not support barrier edges". Barrier guards on phi inputs are handled for other languages (for example #16677 for C++), but the JavaScript integration of the shared SSA library in sharedlib/Ssa.qll is only used for use-use flow after a post-update. For comparison, Java and Ruby handle the same pattern with 2.27.2: java/ssrf does not report if (!ALLOWED.contains(url)) { url = "https://a.example/"; } followed by new URL(url).openConnection(), and rb/command-line-injection does not report lang = "en" unless ["en", "de"].include?(lang) followed by system("echo " + lang).

Is this a known consequence of not carrying over barrierGuardBlocksEdge? If so, is there a plan to support guards on phi inputs for JavaScript, as in the other languages?

Vorherrschende Sprache
CodeQL
Sterne
10.2k
Forks
2.1k
Ø Merge
2 T. 18 Std.
Gemergte PRs (30 T.)
142

Entwicklungsumgebung

In Codespaces öffnen

Startet den Dev-Container des Projekts im Browser, mit Ihrem eigenen GitHub-Konto.

Erste Schritte

  1. Lesen Sie das ganze Issue und danach den Beitragsleitfaden des Projekts.
  2. Schreiben Sie ins Issue, dass Sie es übernehmen — das erspart doppelte Arbeit.
  3. Forken Sie das Repository und arbeiten Sie in einem Branch.
  4. Öffnen Sie einen Pull Request, der die Issue-Nummer nennt.

Mehr aus github/codeql

Alle Issues in github/codeql

Ähnliche Issues

Weitere Issues zu Security

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.