Skip to content

Add OSV-Scanner-based security workflow#388

Open
vikrantpuppala wants to merge 1 commit into
mainfrom
vp/security-scan
Open

Add OSV-Scanner-based security workflow#388
vikrantpuppala wants to merge 1 commit into
mainfrom
vp/security-scan

Conversation

@vikrantpuppala

Copy link
Copy Markdown
Collaborator

Summary

  • Adds .github/workflows/securityScan.yml — single workflow, single job, three triggers (PR / weekly cron / manual). PR runs fail on CVSS ≥ 7 only; weekly runs report all findings and email the team.
  • Adds osv-scanner.toml — empty suppressions file (populate iteratively as real false positives or dev-only findings surface).
  • Reuses the existing ./.github/actions/setup-jfrog composite action — no duplicate OIDC-token logic.

Mirrors the JDBC driver's workflow (databricks-jdbc#1460), adapted for Node.js: reads package-lock.json natively via OSV-Scanner (no separate SBOM tool needed).

Day-one results

The workflow is not yet wired into branch protection, so its first PR-time runs are advisory. A dry-run against current main surfaces:

  • 22 HIGH (CVSS ≥ 7) — concentrated in form-data, basic-ftp, flatted, minimatch, thrift, ws, serialize-javascript, cross-spawn, path-to-regexp, braces, picomatch, @75lb/deep-merge
  • 15 MED, 5 LOW

Important: OSV scans both runtime and devDependencies (it treats everything in package-lock.json equally). Many of the day-one findings are dev-only (the mocha/nyc/eslint toolchain — flatted, serialize-javascript, nanoid, js-yaml, etc.) and don't reach dist/. The team has two options for those:

  1. Bump the offending dev-dep (cleanest — usually a npm update away).
  2. Add a documented [[IgnoredVulns]] entry to osv-scanner.toml justifying why the CVE is dev-only and doesn't affect shipped artifacts.

A follow-up PR can either bump deps or curate the suppression list once we triage what's runtime vs. dev.

Test plan

  • Dry-run OSV-Scanner v2.3.8 locally against package-lock.json — produces expected findings
  • YAML validates
  • First CI run on this PR exercises the PR path (will fail by design — the 22 HIGHs above)
  • Manual workflow_dispatch after merge exercises the weekly path
  • Secrets (SMTP_USERNAME, SMTP_PASSWORD, EMAIL_RECIPIENTS) wired in repo settings before the first scheduled run

This pull request was AI-assisted by Isaac.

Single workflow, single job, three triggers:
  - pull_request to main: fails on CVSS >= 7 findings only
    (HIGH/CRITICAL block merges; MED/LOW visible but non-blocking)
  - cron weekly (Sunday 00:00 UTC): reports ALL findings via email
  - workflow_dispatch: behaves like cron

Mirrors the JDBC driver's security workflow (databricks-jdbc#1460)
adapted for Node.js:
  - Reads package-lock.json natively via OSV-Scanner --lockfile (no
    separate SBOM tool needed)
  - Reuses the existing ./.github/actions/setup-jfrog composite action
    for parity with main.yml (the workflow functionally doesn't need
    JFrog since OSV reads the lockfile directly, but keeping the
    composite action preserves the established pattern)
  - Suppressions in osv-scanner.toml ([[IgnoredVulns]] schema)

The workflow is not yet wired into branch protection. Day-one scan
against current main surfaces 22 HIGH / 15 MED / 5 LOW (42 total).
Many are in dev dependencies (mocha/nyc/eslint chains). The team can
either bump the offending deps or add documented [[IgnoredVulns]]
entries for dev-only findings that don't reach `dist/`.

Co-authored-by: Isaac
Signed-off-by: Vikrant Puppala <vikrant.puppala@databricks.com>
@gopalldb

Copy link
Copy Markdown
Collaborator

⚠️ [MAJOR] — File-level | bug | logical_claude_agent

▎ .max_severity // "0" only substitutes when max_severity is null/false, NOT when it is an empty string. OSV-Scanner emits max_severity as "" for advisories lacking a CVSS vector (common for GHSA-only advisories). Such a finding gets severity="" -> tonumber? // 0 -> 0, so a genuinely HIGH vuln never trips the CVSS>=7 PR gate (fail-open). Validator: confirm OSV v2.3.8 emits max_severity="" for scoreless advisories and that the JDBC bouncycastle-style GHSA findings carry no numeric max_severity.

▎ 💡 Suggested Fix: Treat an empty-string max_severity the same as a scoreless finding but do NOT let it silently pass the gate. Either coalesce empty string as well as null (so the finding surfaces with a visible non-numeric marker rather than 0), or explicitly route scoreless advisories (empty max_severity, or ids matching MAL-*/GHSA-only-no-CVSS) into the fail path so malware advisories can't sail through the CVSS>=7 gate. E.g. change the mapping so a missing/empty score becomes a sentinel that the gate counts as blocking, or add a separate scoreless_count that also fails the PR.

⚠️ [MAJOR] — File-level | bug | logical_claude_agent

▎ Collect findings runs set -uo pipefail (no -e). If /tmp/osv-out.json is malformed JSON, the ALL_FINDINGS jq exits 5, ALL_FINDINGS='', then TOTAL_FINDINGS='' and HIGH_COUNT='' are written to GITHUB_OUTPUT as empty strings. Confirmed locally: [ "$TOTAL_FINDINGS" -gt 0 ] on line 142 errors with 'integer expression expected'. Downstream high_count != '0' becomes '' != '0' (true). Validator: trace empty-output propagation to the gate steps.

▎ 💡 Suggested Fix: Validate the jq parse before deriving counts: check the first jq's exit status (or run jq empty /tmp/osv-out.json first) and fail the step explicitly if OSV output is not valid JSON, instead of letting empty strings flow into GITHUB_OUTPUT. Also guard the numeric comparisons by defaulting the counts to 0 when empty (e.g. TOTAL_FINDINGS=${TOTAL_FINDINGS:-0}).

📄 .github/workflows/securityScan.yml

⚠️ [MAJOR] || true masks all osv-scanner failures; only a zero-byte guard backs it, which partial writes defeat
Line 89 | bug | logical_claude_agent

▎ || true swallows every osv-scanner failure, and the only backstop ([ ! -s ]) misses a partial write. Line 89's || true was intended to tolerate osv-scanner's exit-1-on-any-finding, but it also masks genuine failures: a network error reaching OSV.dev, a malformed osv-scanner.toml, a wrong-arch/corrupt binary, or a crash mid-write to --output-file. The sole thing distinguishing those from a clean scan is the line-91 [ ! -s /tmp/osv-out.json ] guard, which only rejects a zero-byte file. A truncated-but-non-empty output file (reproduced: a 140-byte partial JSON) passes the guard, then the Collect-findings jq (line 110) fails to parse it and empty counts propagate to the gates -- an errored scan becomes indistinguishable from a clean one (fail-open on the scheduled path) or emits misleading counts. (Note: the sibling concern that scan source --lockfile --config --format=json --output-file is invalid v2.3.8 CLI is not a bug -- that syntax is valid and was verified against the v2.3.8 binary.)

▎ 💡 Suggested Fix: Capture osv-scanner's exit code instead of || true, and distinguish 'findings present' (osv exits 1) from a real failure (other non-zero exits, e.g. 127/128+). After the scan, validate the output is parseable JSON (jq empty /tmp/osv-out.json) and fail the step on a parse error or unexpected exit code, so masked/partial-scan failures surface instead of flowing into the count logic as garbage.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants