Skip to content

test(scanner): source lockfiles from vulsio/integration#2531

Merged
shino merged 9 commits intomasterfrom
fix/scorecard-fixture-dedup
Apr 30, 2026
Merged

test(scanner): source lockfiles from vulsio/integration#2531
shino merged 9 commits intomasterfrom
fix/scorecard-fixture-dedup

Conversation

@shino
Copy link
Copy Markdown
Collaborator

@shino shino commented Apr 28, 2026

Summary

  • Delete the 30 lockfile fixtures under scanner/testdata/fixtures/ that duplicate integration/data/lockfile/ and have TestAnalyzeLibrary_Golden read from the integration repo directly.
  • Fetch the integration repo in CI via a separate actions/checkout step pinned to a commit SHA (no submodules: true on the main checkout — that would let a fork PR redirect .gitmodules and run attacker code under go test).
  • Replace the single installed.json lockfileEntry with installed-pear/installed.json and installed-packagist/installed.json to match the directory split shipped via the companion integration PR; refresh the corresponding two golden files.
  • GNUmakefile: add composer-vendor-pear and composer-vendor-packagist to the make diff LIBS list so the new server entries flow through the int(-redis)-config.toml diff target.
  • TestAnalyzeLibrary_Golden / TestAnalyzeLibrary_PomOnline: t.Fatalf (not skip) when the integration data directory is missing, so a botched checkout in CI surfaces as a test failure rather than a silent green.

Why

  • OpenSSF Scorecard alert #81 was triggered by old packages declared inside scanner/testdata/fixtures/go.mod (OPA v0.35.0, docker v20.10.11, etc.) being scanned as real dependencies. Those files were just copies of fixtures already present in vulsio/integration — they had been duplicated so CI could go test without enabling submodules.
  • Removing the duplicates and reading the fixtures from the upstream repo gets the dependency scanners off our back without weakening the supply-chain posture: the SHA pin lives in the workflow file (not .gitmodules), so a fork PR cannot redirect it without a reviewable diff.

Security note

Three patterns for fetching the integration data and their threat models:

Approach Fork PR attack surface
submodules: true on actions/checkout Edit .gitmodules URL → arbitrary code in go test
git clone <hardcoded-URL> step in workflow Edit workflow URL → same
actions/checkout with repository: and ref: <SHA> (this PR) Edit workflow repository:/ref: → same

All three are equivalent in worst-case scope (runner-only RCE for fork PRs, GITHUB_TOKEN is read-only and secrets are gated). actions/checkout was chosen because it's the standard pattern, supports SHA pin via ref:, and concentrates the trust anchor in the workflow file (not .gitmodules). The integration submodule entry stays for local convenience and make diff.

Both actions/checkout steps also set persist-credentials: false so the checkout token is not left on disk for the rest of the job.

Companion PR

  • chore(lockfile): add pear/packagist installed.json fixture variants vulsio/integration#33 (merged 2026-04-28 at 6dfd74510f5944e7c973e40d7844020d53dbb3a7) added installed-pear/installed.json, moved the existing installed.json to installed-packagist/, and updated int(-redis)-config.toml to the two composer-vendor-* entries.
  • The workflow ref: SHA in .github/workflows/test.yml and the integration/ submodule pointer have been bumped to that merge commit, so CI on this branch resolves both installed-* goldens against the merged layout.

Test plan

  • go test ./scanner -run TestAnalyzeLibrary_Golden -v — 36/36 PASS locally with submodule pointing at the integration merge commit
  • No remaining references to scanner/testdata/fixtures/ outside analyze_golden_test.go (detector/vuls2/testdata/fixtures/enrich is unrelated)
  • CI green on this branch after the ref SHA bump
  • After merge: Scorecard schedule run closes alert Add option for it get cve detail from cve.sqlite3. #81

OpenSSF Scorecard alert #81 was triggered by old packages declared in
scanner/testdata/fixtures/go.mod (OPA v0.35.0, docker v20.10.11, etc.)
being scanned as real dependencies of vuls. Those fixtures were just
copies of files already in vulsio/integration; they had been
duplicated here to avoid pulling submodules in CI, which would let a
fork PR redirect .gitmodules to attacker code that "go test" would
then run.

Delete the 30 duplicate files and have TestAnalyzeLibrary_Golden read
straight from integration/data/lockfile/. CI now fetches that repo
via a separate actions/checkout step pinned by commit SHA. The pin
lives in the workflow file (not .gitmodules), so a fork PR cannot
redirect it without a reviewable diff. The submodule entry stays for
local development and "make diff".

After vulsio/integration merges the matching installed.json change,
bump the ref SHA in .github/workflows/test.yml and the submodule
pointer to that merge commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@shino shino changed the title fix(scanner/fixture): source lockfiles from vulsio/integration test(scanner): source lockfiles from vulsio/integration Apr 28, 2026
shino and others added 3 commits April 28, 2026 17:17
Match the integration repo's directory split (vulsio/integration#33):
the merged installed.json fixture is replaced by two distinct fixtures
under installed-pear/ and installed-packagist/, exercising the minimal
PEAR-style and the full Packagist-style Composer 2.x package shapes
separately. Update TestAnalyzeLibrary_Golden's lockfileEntry list and
regenerate the golden files; drop the now-unused installed.json.json.

The submodule pointer and the integration ref in
.github/workflows/test.yml will be bumped together once the companion
integration PR lands.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ff LIBS

The two pseudo servers added in vulsio/integration#33 (replacing the
old [servers.composer-vendor] that was never wired into LIBS) need to
be listed here so make diff actually exercises the installed.json
parser. Closes a pre-existing gap as a side effect: composer-vendor
has been an orphan entry in int(-redis)-config.toml since it was added.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…plit

vulsio/integration#33 has merged at 6dfd74510f5944e7c973e40d7844020d53dbb3a7.
Update both sources of truth for the integration test data:

- .github/workflows/test.yml: actions/checkout ref pin
- integration/ submodule pointer

CI on this PR can now resolve installed-pear/installed.json and
installed-packagist/installed.json against the upstream merge commit.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@shino shino marked this pull request as ready for review April 28, 2026 08:59
@shino shino requested a review from Copilot April 28, 2026 08:59
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes duplicated lockfile fixtures from scanner/testdata/fixtures and updates the golden tests (and CI) to source lockfile/binary fixtures from the vulsio/integration repository instead, addressing dependency-scanner noise from stale fixture dependencies.

Changes:

  • Update TestAnalyzeLibrary_Golden (and the pom online test) to read fixtures from ../integration/data/lockfile, skipping if the integration repo isn’t available.
  • Add a dedicated actions/checkout step in CI to fetch vulsio/integration pinned to a commit SHA (without enabling submodules).
  • Split the composer installed.json fixture into installed-pear/installed.json and installed-packagist/installed.json, and refresh/add the corresponding golden outputs; delete the now-redundant local fixtures.

Reviewed changes

Copilot reviewed 16 out of 36 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
scanner/analyze_golden_test.go Switch golden tests to load fixtures from the integration repo; update fixture list for the composer installed.json split.
.github/workflows/test.yml Add a pinned actions/checkout step to fetch vulsio/integration into integration/ for tests.
GNUmakefile Update integration-test LIBS list to include composer vendor split identifiers.
scanner/testdata/golden/installed-pear_installed.json.json Update expected lockfilePath for the pear composer vendor installed.json moved into a subdirectory.
scanner/testdata/golden/installed-packagist_installed.json.json Add golden output for the new packagist composer vendor installed.json location.
scanner/testdata/fixtures/requirements.txt Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/pubspec.lock Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/pom.xml Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/poetry-v2/poetry.lock Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/poetry-v1/poetry.lock Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/packages.lock.json Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/packages.config Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/mix.lock Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/installed.json Remove old composer vendor fixture (replaced by installed-pear/ and installed-packagist/ in integration).
scanner/testdata/fixtures/gradle.lockfile Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/go.mod Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/datacollector.deps.json Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/conan-v2/conan.lock Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/conan-v1/conan.lock Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/bun.lock Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/Podfile.lock Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/Pipfile.lock Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/Package.resolved Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/Gemfile.lock Remove duplicated local fixture (now sourced from integration).
scanner/testdata/fixtures/Directory.Packages.props Remove duplicated local fixture (now sourced from integration).
Files not reviewed (1)
  • scanner/testdata/fixtures/npm-v1/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread .github/workflows/test.yml
Comment thread scanner/analyze_golden_test.go
Comment thread scanner/analyze_golden_test.go Outdated
shino and others added 3 commits April 28, 2026 18:38
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Per Copilot review: os.Stat returning a non-not-exist error (e.g.,
permission, IO) should fail the test, not silently skip — those
indicate real issues that masking would hide. Distinguish via
errors.Is(err, fs.ErrNotExist).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI broke after the previous suggestion changed the ref to
refs/pull/33/merge: that ref is server-side only while the PR is
open, so once vulsio/integration#33 was merged the runner got
"fatal: couldn't find remote ref refs/pull/33/merge".

Pin to the actual merge commit SHA (6dfd74510f5944e7c973e40d7844020d53dbb3a7).
A SHA is also the correct anchor for the security argument: a mutable
ref defeats the point of pinning, since whoever can move the ref can
swap in arbitrary code that go test would execute.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 36 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • scanner/testdata/fixtures/npm-v1/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread scanner/analyze_golden_test.go
Comment thread scanner/analyze_golden_test.go
Per Copilot review: t.Skipf can mask CI misconfiguration (e.g. someone
removes the integration checkout step or changes the path) and silently
disable golden-test coverage. Drop the skip path entirely — both CI and
local runs now hard-fail with a message pointing at the right remediation
(git submodule update --init locally, or check the workflow). Removes the
errors/io/fs imports that were only there for the skip branch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 36 changed files in this pull request and generated 1 comment.

Files not reviewed (1)
  • scanner/testdata/fixtures/npm-v1/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread scanner/analyze_golden_test.go
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 16 out of 36 changed files in this pull request and generated no new comments.

Files not reviewed (1)
  • scanner/testdata/fixtures/npm-v1/package-lock.json: Language not supported

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@shino shino requested a review from MaineK00n April 30, 2026 02:06
@shino shino merged commit 18abe61 into master Apr 30, 2026
11 checks passed
@shino shino deleted the fix/scorecard-fixture-dedup branch April 30, 2026 07:01
@shino shino self-assigned this Apr 30, 2026
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.

3 participants