Repository navigation
fix(sarif): report a subdirectory's lockfile relative to the source root - #1403
Open
sonukapoor wants to merge 3 commits into
Open
sonukapoor wants to merge 3 commits into
sonukapoor wants to merge 3 commits into
Conversation
Every SARIF location is emitted with uriBaseId %SRCROOT%, but the URI was the lockfile's basename. A scan of a subdirectory therefore reported its findings against a lockfile at the repository root, so Code Scanning put the alerts on a file that does not contain the vulnerable packages, or on one that does not exist. Measure the lockfile from the launch directory instead. That is the checkout root on every path that uploads to Code Scanning, since our own Action runs each scan step from the workspace and passes the target as an argument, so a scan of the root still emits the bare filename. A lockfile above the launch directory cannot be source-root-relative and falls back to the basename. This deliberately differs from the HTML report, which measures from the scanned project because it is read wherever it is pasted (#1271).
…Windows drive On Windows a lockfile on a different drive from the launch directory makes path.relative return an absolute path, which the parent-directory check did not catch, so the location came out as D:/proj/package-lock.json. That is not source-root-relative and is worse than the basename that shipped.
…e root The function had no direct tests, only mocks of it, which is how a basename-only location shipped. Covers the launch directory, a subdirectory, a relative path, a lockfile above the launch directory, the no-path default, and under Windows path rules the slash conversion and the cross-drive case.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SARIF locations are emitted relative to
%SRCROOT%, but the URI was the lockfile's bare filename. So a scan of a subdirectory reported every finding against a lockfile at the repository root, and Code Scanning put the alerts on a file without those dependencies, or on one that does not exist.The lockfile is now measured from the launch directory, which is the checkout root on every path that uploads to Code Scanning: our Action runs each scan from the workspace and passes the target as an argument. A scan run at the project root still emits the bare filename, so single-project setups see no change. A lockfile above the launch directory, or on another Windows drive, falls back to the bare filename rather than emitting an invalid location.
This deliberately differs from the HTML report, which measures from the scanned project because it is read wherever it is pasted (#1271). Each artefact measures from wherever it is consumed.
Scanning
examples/direct-fixablefrom the repository root: before, all 26 results were located atpackage-lock.json; now all 26 are atexamples/direct-fixable/package-lock.json. The same scan run from inside the project is unchanged, and the findings are identical in both.Worth a line in the release notes: some adopters work around this by prefixing the folder onto SARIF paths themselves before uploading. Once they upgrade, that step would double the prefix, so it should be removed.
Closes #1380