Fixing a detect-non-literal-regexp False Positive

The short version

Semgrep’s detect-non-literal-regexp flagged new RegExp(c.path.replace(...)) in an E2E test, but that branch is a dead branch never reached at runtime. Changing allowUrl on the PageCase type from optional to required, and deleting the ?? new RegExp(...) fallback expression outright, cleared the finding without changing behaviour at all.

What it looks like

The E2E test that verifies the permission matrix per role (authorization-matrix.spec.ts) contained this fallback.

// before the fix
type PageCase = {
  label: string;
  path: string;
  allow: readonly RoleKey[];
  /** URL that must hold when reached by an allowed role (omitted: stays on path). */
  allowUrl?: RegExp;
};

// before the fix
await expect(page).toHaveURL(c.allowUrl ?? new RegExp(c.path.replace(/[/]/g, "\\/")));

Because of this code, SAST (Semgrep) on the main branch went red as a blocking detect-non-literal-regexp finding. The rule detects a string passed to new RegExp(...) that looks dynamic.

Why

The flagged new RegExp(c.path.replace(...)) is a fallback branch evaluated only when c.allowUrl is omitted. But every entry defined in PAGE_CASES already declared allowUrl without exception at the time of this fix. There is no case where c.allowUrl is undefined, so the ?? new RegExp(...) side was a dead branch never evaluated at runtime.

Being static analysis, Semgrep’s detect-non-literal-regexp does not track whether a branch is reachable at runtime. Looking only at the new RegExp(...) syntax, it can determine neither that the c.path being passed is a compile-time literal from the static PAGE_CASES array inside the test, nor that the branch itself is unreachable. So it flagged a place that is syntactically a non-literal regex construction but harmless in practice.

Fixing it

We changed the code and removed the “non-literal new RegExp” that Semgrep was tracking.

// after the fix
type PageCase = {
  label: string;
  path: string;
  allow: readonly RoleKey[];
  /**
   * Regex the URL must satisfy when reached by an allowed role (required). Each case declares it.
   * This used to be optional, with expectPageOutcome falling back dynamically to `new RegExp(c.path)`,
   * but since every PAGE_CASES entry has allowUrl that fallback is unreachable (a dead branch),
   * and the non-literal `new RegExp(...)` produced a Semgrep detect-non-literal-regexp false positive.
   */
  allowUrl: RegExp;
};

// after the fix
await expect(page).toHaveURL(c.allowUrl);

Making allowUrl a required type makes the ?? new RegExp(...) fallback expression unnecessary, so it can be deleted. Neither Semgrep’s rule set nor the semgrep ci configuration was changed at all. No suppression comment (# nosemgrep or similar) was used either. The resolution goes in the direction of deleting the code the rule pointed at rather than weakening the rule.

Preventing a repeat

The temptation to fix it by suppression was there, but suppression only “silences this one finding”, leaving the code-side risk of a forgotten allowUrl declaration in place. Changing allowUrl?: RegExp to required, as here, means that the moment someone tries to add a PAGE_CASES entry missing allowUrl, TypeScript’s own type check fails. Rather than filing Semgrep’s finding under “the limits of static analysis (it cannot track reachability)”, fixing the loose typing the finding had landed on got us both the cleared finding and protection against a future missing declaration.