diff --git a/.github/workflows/security-gate.yml b/.github/workflows/security-gate.yml new file mode 100644 index 0000000..8e87de6 --- /dev/null +++ b/.github/workflows/security-gate.yml @@ -0,0 +1,105 @@ +# Blocking security gate. +# +# Separate from threatcrush-scan.yml on purpose: that file is managed by the +# sh1pt Actions Fleet and carries a content hash, so local edits are liable to +# be overwritten on the next pack update. It reports findings; this decides +# whether they stop the merge. +# +# This started out as a reviewed-baseline gate, because ThreatCrush 0.3.0 +# reported 56 findings here and all 56 were false positives — six of them +# high-severity, so `--fail-on high` would have blocked every pull request. +# Those were rule bugs rather than anything in this repository, and they are +# fixed in 0.4.0 (profullstack/threatcrush#76): static `innerHTML` assignments, +# short escaper aliases like `esc()`, `searchParams.set` counted as untrusted +# input, and test fixtures read as live credentials. This repository now has +# zero high-severity findings, so the plain gate does the job and ~200 lines of +# baseline machinery went away with it. +# +# The remaining findings are medium and deliberately do not block: they are +# `innerHTML` sinks in multi-line templates whose interpolations are escaped on +# a different line than the assignment, which a line-oriented scanner cannot +# see. +# +# Know what this does not catch. Severity depends on whether the scanner can +# see the taint source near the sink, so the same XSS is rated differently +# depending on how the code is arranged: +# +# innerHTML = '' + new URL(location).searchParams.get('q') + '' high, blocks +# innerHTML = '' + q + '' // q is a parameter medium, does not +# +# Both were measured. The gate therefore catches a vulnerability written in +# one place and misses one whose source sits in another function. Raising it to +# `--fail-on medium` would close that gap and currently costs 31 false +# positives; the honest position is that this gate is a floor, not a proof. +name: security gate + +on: + push: + branches: [main] + pull_request: + +permissions: + contents: read + +jobs: + gate: + name: no high-severity findings + runs-on: ubuntu-latest + timeout-minutes: 15 + + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + node-version: "20" + + # Same retry shape as the scan workflow: a registry blip is not a + # security signal and must not read like one. + - name: Install ThreatCrush + run: | + for attempt in 1 2 3; do + if npm install -g "@profullstack/threatcrush@latest"; then + exit 0 + fi + delay=$((attempt * 10)) + echo "::warning::ThreatCrush install attempt ${attempt}/3 failed; retrying in ${delay}s" + sleep "${delay}" + done + echo "::error::ThreatCrush install failed after 3 attempts" + exit 1 + + - name: Record the version that ran + run: threatcrush --version + + # Fails closed in both directions. `--fail-on high` exits 1 when a + # high-or-critical finding exists; any other non-zero exit is a scan that + # did not complete, and an unexamined diff is not a clean one — so the + # step fails either way rather than treating "no output" as "no findings". + - name: Gate on high-severity findings + run: | + set -o pipefail + code=0 + threatcrush scan . --fail-on high --format json --output threatcrush.json || code=$? + + if [ ! -s threatcrush.json ]; then + echo "::error::ThreatCrush produced no findings file (exit ${code}) — this diff was NOT scanned" + exit 1 + fi + + total=$(python3 -c "import json;d=json.load(open('threatcrush.json'));print(len(d['findings'] if isinstance(d,dict) else d))") + echo "ThreatCrush: ${total} finding(s), exit ${code}." >> "$GITHUB_STEP_SUMMARY" + + if [ "$code" -ne 0 ]; then + echo "::error::high-severity finding(s) present — see the Security tab or the artifact below" + exit "$code" + fi + + - name: Upload findings + if: always() + uses: actions/upload-artifact@v4 + with: + name: threatcrush-findings + path: threatcrush.json + retention-days: 30 + if-no-files-found: warn diff --git a/README.md b/README.md index bd2862d..39ab535 100644 --- a/README.md +++ b/README.md @@ -479,6 +479,70 @@ bun test # deterministic unit tests (indicators, parsing) bun run typecheck # tsc --noEmit ``` +## Security scanning + +Every pull request is scanned by ThreatCrush. Two workflows are involved and +they do different jobs: + +- **`threatcrush-scan.yml`** reports findings and uploads SARIF to the Security + tab. It is managed by the sh1pt Actions Fleet and carries a content hash, so + do not edit it locally — a pack update will overwrite it. +- **`security-gate.yml`** decides whether findings stop the merge. It runs + `threatcrush scan . --fail-on high`, so a high-or-critical finding fails the + build. + +It fails closed in both directions: a scan that produces no findings file is +reported as *not scanned* rather than as clean, because an unexamined diff is +not a clean one. + +### Why the gate is `high` and not `medium` + +The scan reported **56 findings here, all 56 false positives**, six of them +high-severity — which made any `--fail-on` setting unusable, since it would +have blocked every pull request. Triaging them showed the fault was in the +rules rather than in this repository, and the fixes shipped in ThreatCrush +0.4.0 ([threatcrush#76](https://github.com/profullstack/threatcrush/pull/76)): + +- static `innerHTML` assignments reported as XSS +- the escaper guard matching `escapeHtml(` but not `esc(`, so the code that + escapes most rigorously was reported most often +- `searchParams` counted as untrusted input even when *writing* an outbound + URL, which fired the SSRF rule on constant hosts +- credentials in test fixtures treated as live + +That took this repository to **zero high-severity findings**, so the plain gate +now works and the reviewed-baseline machinery it replaced (~200 lines) is gone. + +The findings that remain are medium and deliberately do not block. They are +`innerHTML` sinks inside multi-line templates whose interpolations *are* +escaped, just on a different line from the assignment — which a line-oriented +scanner cannot see. + +### What this gate does not catch + +Severity depends on whether the scanner can see the taint source near the sink, +so the *same* vulnerability is rated differently depending on how the code is +arranged. Both of these were measured against this repository: + +| shape | severity | blocks? | +| --- | --- | --- | +| `innerHTML = '' + new URL(location).searchParams.get('q') + ''` | high | yes | +| `innerHTML = '' + q + ''`, where `q` is a parameter | medium | **no** | + +So the gate stops a vulnerability written in one place and misses one whose +source sits in another function. `--fail-on medium` would close the gap and +today costs 31 false positives, which is why it is not set. Treat this as a +floor, not a proof — it is not a substitute for review. + +To silence a finding you have established is safe, use the scanner's own +directive on the line above it, with the rule named so a *different* rule +firing there still surfaces: + +```js +// threatcrush-disable-next-line js-unescaped-html-sink +el.innerHTML = template; +``` + ## Compliance Every ranking includes: