Skip to content

vuln-gate: a typo'd --level opens the gate (fail-open) — port the fixed logic from repo-template #529

Description

@mforce

.github/scripts/vuln-gate.mjs on main carries three defects. The first is a
fail-open: the gate reports success while a critical advisory is present.

This script came from the same lineage as mforce/repo-template,
which is now public and has all three fixed with mutation checks. Everything
below links to real upstream code — port the logic, do not re-derive it.


Part 1 — the defects (required)

P2 — an unknown --level makes the gate stop gating

gate() computes its floor with severityRank(level). That function
deliberately ranks an unrecognised string above critical, which is right
for a finding (a tool inventing a new severity must still block) and exactly
backwards for the threshold: an inflated floor is unreachable, so every advisory
reads as "below threshold".

Reproduced on main:

$ printf '{"auditReportVersion":2,"vulnerabilities":{"m":{"name":"m","severity":"critical","via":[{"source":1,"name":"m","severity":"critical","title":"x","url":"https://github.com/advisories/GHSA-93q8-gq69-wqmw"}]}}}' \
    | node .github/scripts/vuln-gate.mjs --ecosystem npm --level hihg --exceptions /nonexistent
[npm] no advisories at or above "hihg" (0 excepted, 1 below threshold).
exit=0

A critical advisory, a green run, no warning — from a typo.

A finding's severity comes from a tool we do not control, so unknown must sort
high. The threshold is one of our own strings, so unknown is a bad
invocation and must be refused.

Upstream fix: levelProblem —
validated in main() (exit 2) and thrown from
gate(), with the floor computed by
SEVERITIES.indexOf so the
wrong-function mistake cannot recur. Validating only in main() leaves the
landmine in the exported API for the next caller — that residual is
filed against collectify, do
not repeat it here.

P3 — a lapsed "ANY" exception never gets its "remove me" warning

vuln-gate.mjs:191:

const inScope = (e) => e.ecosystem === "any" || e.ecosystem.toLowerCase() === ecosystem;

Suppression (matches) lower-cases the scope; this does not. An exception
written "ecosystem": "ANY" correctly stops suppressing once it lapses, but is
never reported stale — it lingers as dead config nobody is told to delete, which
is the entire point of the staleness warning.

Upstream fix: one scopeOf helper
used by both, so the two cannot disagree again.

P3 — a corrupt exceptions file is swallowed silently

vuln-gate.mjs:298:

function loadExceptions(path) {
  try {
    return JSON.parse(readFileSync(path, "utf8")).exceptions ?? [];
  } catch {
    return []; // no file → empty allowlist, the normal case
  }
}

The catch cannot tell "no file" (normal) from "the file is corrupt". Returning
[] is the safe direction — nothing is suppressed — but silently: every entry
someone believes is muting an advisory is doing nothing, with no signal. It also
contradicts the documented "reported as a warning" contract.

Upstream fix: loadExceptions —
only ENOENT stays quiet; a parse failure, an unreadable file, or an
exceptions key that is not an array each warn and return []. Without the
array check a wrong-shaped file crashes with a raw TypeError from
exceptions.filter instead of a reason.

Definition of done

  • --level hihg exits 2; a critical advisory at the default level still exits 1.
  • The --level guard is end to end, spawning the real CLI —
    example, helper at
    runCli. Every unit here
    behaves correctly on its own; only the assembled exit code shows the hole,
    so a unit test of the predicate does not pin the regression. This was
    measured upstream: deleting the whole fix left the sibling repo's suite
    28/28 green.
  • Mutation-checked per docs/decisions/ guard rules: green baseline, each
    mutant red on its own named assertion, baseline green after restoring.

Part 2 — alignment with the template (optional, separate PRs)

Do not blind-copy the upstream file over this one. The two have drifted 175
lines and cluckwork's 26 tests are written against its own shape:

cluckwork repo-template
ecosystem dispatch ECOSYSTEMS hardcoded (vuln-gate.mjs:35) + a ternary at :339 a PARSERS registry; ECOSYSTEMS derived from it
adding an ecosystem edits in 3+ places one entry in PARSERS
threshold predicate none levelProblem (reason string or null)
loadExceptions private exported, takes an injectable warn

Port Part 1 as a minimal diff against cluckwork's existing shape. Adopting
the PARSERS registry is a legitimate follow-up, but it is a refactor and
belongs in its own PR — not bundled with a security fix.

Template files cluckwork does not have, if you want them (each its own PR, none
blocking this issue):

  • .gitattributes — * text=auto eol=lf. The highest-value one: cluckwork's
    .githooks/* are POSIX sh run via their shebang, and a CRLF checkout makes
    the kernel look for /bin/sh with a trailing CR, failing with a message that
    names neither the hook nor the cause.
  • actionlint + shellcheck in pre-commit — cluckwork's has neither, and the
    repo carries a lot of workflow and shell. Warns and continues when the tools
    are absent, so it is safe to add.
  • SECURITY.md, CONTRIBUTING.md, .github/pull_request_template.md,
    .editorconfig — absent here.
  • .github/CODEOWNERS — the only in-repo lever on "a self-merge to .github/
    rewrites what every gate means". Ships inert upstream on purpose: an entry
    naming an unresolvable user is silently ignored by GitHub, so a
    plausible-looking broken file is worse than an obviously empty one.

Activity

  1. changed the title [-]vuln-gate: a typo'd --level opens the gate (fail-open), plus two hygiene defects[/-] [+]vuln-gate: a typo'd --level opens the gate (fail-open) — port the fixed logic from repo-template[/+] on Aug 16, 2026
  2. mforce commented on Aug 31, 2026

    @mforce
    OwnerAuthor

    Filed the follow-up this issue named: #634 — replacing vuln-gate's four hand-dispatched ecosystem sites with the PARSERS registry.

    Recorded here because that sentence ("a legitimate follow-up, but it is a refactor and belongs in its own PR") had no issue behind it, and a closed issue is not a place a follow-up survives.

    For the record, the other residual in this body is already placed and #634 explicitly leaves it alone: the gate()-level --level validation is filed at collectify#117. Worth noting the precise behaviour, since "fail-open" in this issue's title refers to the main() path that was fixed here — an unvalidated level reaching the exported gate() yields floor = -1 at vuln-gate.mjs:198, so every finding ranks at or above it and the gate blocks everything. Wrong, but fail-closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions