-
-
Notifications
You must be signed in to change notification settings - Fork 226
ci: re-enable List vulnerable packages #4756
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
| schedule: | ||
| - cron: "0 0 * * *" # once a day | ||
| pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: revert change from 3d0eb8b
| run: dotnet restore Sentry.sln --nologo | ||
|
|
||
| # The dotnet list package command doesn't change its exit code on detection, so tee to a file and scan it | ||
| # The dotnet package list command doesn't change its exit code on detection, so tee to a file and scan it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note:
- .NET 9 SDK and earlier:
dotnet list package - the "noun first" form was introduced in .NET 10 SDK
- see also dotnet package list
| shell: bash | ||
| run: | | ||
| dotnet list ${{ github.workspace }}/Sentry.sln package --vulnerable --include-transitive | tee vulnerable.txt | ||
| dotnet package list --project Sentry.sln --vulnerable --include-transitive --no-restore | tee vulnerable.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note: add --no-restore since the preceding step executes dotnet restore
This also changes the output:
- Determining projects to restore...
- All projects are up-to-date for restore.
The following sources were used:
https://api.nuget.org/v3/index.json
..
The given project `Sentry.Benchmarks` has no vulnerable packages given the current sources.
The given project `Sentry.Samples.Android` has no vulnerable packages given the current sources.
..
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4756 +/- ##
==========================================
- Coverage 73.85% 73.84% -0.02%
==========================================
Files 485 485
Lines 17689 17689
Branches 3496 3496
==========================================
- Hits 13064 13062 -2
- Misses 3765 3766 +1
- Partials 860 861 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@sentry review |
| dotnet package list --project Sentry.sln --vulnerable --include-transitive --no-restore | tee vulnerable.txt | ||
| # https://github.com/getsentry/sentry-dotnet/issues/2814 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change from dotnet list package (verb-first syntax) to dotnet package list (noun-first syntax) assumes .NET 10 SDK is available in the runtime environment. The noun-first form was introduced in .NET 10. Verify that the ./.github/actions/environment setup step installs .NET 10 SDK or later, otherwise this command will fail on systems running .NET 9 SDK or earlier. Consider adding a comment documenting the .NET 10+ requirement if this is intentional, or provide a fallback mechanism for older SDK versions.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/workflows/vulnerabilities.yml#L33-L34
Potential issue: The change from `dotnet list package` (verb-first syntax) to `dotnet
package list` (noun-first syntax) assumes .NET 10 SDK is available in the runtime
environment. The noun-first form was introduced in .NET 10. Verify that the
`./.github/actions/environment` setup step installs .NET 10 SDK or later, otherwise this
command will fail on systems running .NET 9 SDK or earlier. Consider adding a comment
documenting the .NET 10+ requirement if this is intentional, or provide a fallback
mechanism for older SDK versions.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2854978
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The actions/setup-dotnet steps use /global.json, where we require the .NET SDK version 10.0.100.
| schedule: | ||
| - cron: "0 0 * * *" # once a day | ||
| pull_request: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The workflow is being re-enabled after being commented out due to a .NET 8 issue (see NuGet/Home#12954). Verify that the underlying issue is resolved in the current .NET SDK version(s) being used. If the issue still exists but this workflow is being enabled anyway, document the expected behavior or workaround in a comment.
Severity: MEDIUM
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: .github/workflows/vulnerabilities.yml#L5-L7
Potential issue: The workflow is being re-enabled after being commented out due to a
.NET 8 issue (see https://github.com/NuGet/Home/issues/12954). Verify that the
underlying issue is resolved in the current .NET SDK version(s) being used. If the issue
still exists but this workflow is being enabled anyway, document the expected behavior
or workaround in a comment.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference_id: 2854978
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See workflow run List vulnerable packages / List vulnerable packages (pull_request) succeeding,
and listing all vulnerable packages.
Noticed during #4715 (comment) that we previously disabled this workflow.
Do not fail workflow yet, since we still have vulnerable dependencies, that we need to review and update.
A related issue is: #4616
Changes
pull_requestand on dailyschedule)--no-restore, since we do an explicitdotnet restorein the preceding step