Skip to content

fix(reporter): dedupe lockfile poisoning markdown threats by URL#714

Closed
Carla232 wants to merge 8 commits into
safedep:mainfrom
Carla232:issue-231-lockfile-poisoning-dedupe
Closed

fix(reporter): dedupe lockfile poisoning markdown threats by URL#714
Carla232 wants to merge 8 commits into
safedep:mainfrom
Carla232:issue-231-lockfile-poisoning-dedupe

Conversation

@Carla232

@Carla232 Carla232 commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #231

Summary

  • consolidate lockfile poisoning threat entries by resolved URL
  • apply the URL-based consolidation to both the markdown summary and the CLI summary output
  • keep existing wording for non-lockfile threats and lockfile threats without a URL in the message
  • add unit tests for URL-based grouping and markdown/summary rendering behavior

Why

When multiple package entries resolve through the same private or non-registry URL, the report can repeat nearly identical lockfile poisoning findings. Grouping by URL makes the report shorter and easier to review, even when different package names point to the same resolved artifact.

Testing

  • go test ./pkg/reporter -run "Test(GroupLockfilePoisoningThreatsByURL|AddThreatsSectionConsolidatesLockfilePoisoningByURL|GroupLockfilePoisoningMessagesByURL)$" -count=1 -v
  • go test ./pkg/reporter -count=1
  • manual repro with glob and globx pointing to the same tarball URL: CLI summary contains the URL once
  • manual repro with --report-markdown-summary: markdown summary contains the URL once

Open in Devin Review

Signed-off-by: Carla232 <79741526+Carla232@users.noreply.github.com>
@safedep

safedep Bot commented Apr 21, 2026

Copy link
Copy Markdown

SafeDep Report Summary

Green Malicious Packages Badge Green Vulnerable Packages Badge Green Risky License Badge

No dependency changes detected. Nothing to scan.

View complete scan results →

This report is generated by SafeDep Github App

@KunalSin9h

Copy link
Copy Markdown
Member

@Carla232 thanks for contribution

@KunalSin9h

Copy link
Copy Markdown
Member

@Carla232 builds are failing..

Signed-off-by: Carla232 <79741526+Carla232@users.noreply.github.com>
@Carla232

Copy link
Copy Markdown
Contributor Author

@Carla232 builds are failing..

Thanks for the heads-up. I pushed a fix in commit e342908 (the previous build break is addressed).
The checks on this fork PR now show action_required (not a new code failure).
Could a maintainer please approve/re-run the workflows?

Signed-off-by: Carla232 <79741526+Carla232@users.noreply.github.com>
@Carla232

Copy link
Copy Markdown
Contributor Author

I pushed a follow-up fix in 7547b3a for the Go Linter failure (gci import grouping in the markdown summary test). I verified locally with the targeted tests and golangci-lint --new-from-patch, which now reports 0 issues.

Could you please approve/re-run the workflows?

Signed-off-by: Carla232 <79741526+Carla232@users.noreply.github.com>

Copy link
Copy Markdown
Contributor Author

I pushed another follow-up in ea7c8a5 for the run-test failure in pkg/scanner.

The failure was caused by a flaky test: the query-worker test was starting the full Malysis enricher, so the result worker and the test were both consuming from the same resultsChannel. The test now starts only the query worker it is meant to verify, which avoids that race.

Locally verified:

  • go test ./pkg/scanner -run "TestMalysisMalwareEnricherQueryWorker/MalysisMalwareEnricher/QueryWorker/should_return_error_for_requests_with_future_retry$" -count=1 -v
  • go test ./pkg/scanner -run "MalysisMalware" -count=1 -v
  • go test ./pkg/scanner -count=1

Could you please approve/re-run the workflows for the latest commit?

@abhisek abhisek requested a review from KunalSin9h April 28, 2026 03:50

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 3 additional findings.

Open in Devin Review

@KunalSin9h

Copy link
Copy Markdown
Member

@Carla232 i tested it loally (from your branch) by building vet.

Why the url is repeated, it should solve this?

Screenshot_2026-04-30_18-26-34

my package.json

{
  "name": "vet-test",
  "version": "1.0.0",
  "description": "",
  "main": "index.js",
  "scripts": {
    "test": "echo \"Error: no test specified\" && exit 1"
  },
  "keywords": [],
  "author": "",
  "license": "ISC",
  "type": "commonjs",
  "dependencies": {
    "glob": "https://github.com/isaacs/node-glob/archive/refs/tags/v10.3.0.tar.gz",
    "globx": "https://github.com/isaacs/node-glob/archive/refs/tags/v10.3.0.tar.gz"
  }
}

Signed-off-by: Carla232 <79741526+Carla232@users.noreply.github.com>

Copy link
Copy Markdown
Contributor Author

Thanks for testing this and for the screenshot. You were right: the previous change still allowed the same resolved URL to appear multiple times in the CLI summary output, especially when different package names point to the same tarball URL.

I pushed ddf97b4 to address that. The PR now groups lockfile poisoning signals strictly by resolved URL in both places:

  • CLI summary output
  • --report-markdown-summary

I also updated the PR description to reflect this broader scope.

Verified with the glob / globx repro case from your comment:

  • CLI summary now prints one consolidated line: 4 lockfile poisoning signals share URL https://github.com/isaacs/node-glob/archive/refs/tags/v10.3.0.tar.gz
  • markdown summary contains that URL once
  • go test ./pkg/reporter -run "Test(GroupLockfilePoisoningThreatsByURL|AddThreatsSectionConsolidatesLockfilePoisoningByURL|GroupLockfilePoisoningMessagesByURL)$" -count=1 -v
  • go test ./pkg/reporter -count=1

@KunalSin9h

Copy link
Copy Markdown
Member

thanks will test

@abhisek

abhisek commented May 9, 2026

Copy link
Copy Markdown
Member

@KunalSin9h Can you please review and take this forward?

@KunalSin9h

Copy link
Copy Markdown
Member

merged fix in #729.

@Carla232 thanks for contribution.

@KunalSin9h KunalSin9h closed this May 15, 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.

Avoid Duplicate Reporting of Lockfile Poisoning in Markdown Summary Reporter

3 participants