Fix dropped NuGet vulnerability warnings (NU1902/NU1903) from MSBuild Exec output#1535
Fix dropped NuGet vulnerability warnings (NU1902/NU1903) from MSBuild Exec output#1535Dipanshusinghh wants to merge 2 commits into
Conversation
b4c99cc to
5e64447
Compare
7ec9942 to
98fbf20
Compare
What is the use case for dropping diagnostics with exe/placeholder names? I mean, why would one want to drop those? |
☀️ Quality MonitorTests Coverage for New Code 〰️ Line Coverage: 100.00% — perfect 🎉 Coverage for Whole Project 〰️ Line Coverage: 94.56% Style Bugs API Problems 🚫 Revapi: No warnings Vulnerabilities 🛡️ OWASP Dependency Check: No vulnerabilities Software Metrics 🌀 Cyclomatic Complexity: 3510 (total) 📌 Reference ResultsDelta reports computed against the reference results of 735001a in workflow run 27370332799. 🚦 Quality GatesOverall Status: ❌ FAILURE✅ Passed Gates
❌ Failed Gates
Created by Quality Monitor v4.15.0 (#82d77af). More details are shown in the GitHub Checks Result. |
uhafner
left a comment
There was a problem hiding this comment.
Thanks for your PR. It looks like this parser is actually a mess which is hard to maintain. It covers too many cases and workarounds. Maybe we need to split it in the future so we get a dedicate parser for each regexp group.
| private boolean isToolName(final String fileName) { | ||
| if (StringUtils.isBlank(fileName) || "-".equals(fileName) || "unknown.file".equals(fileName)) { | ||
| return true; | ||
| // Checks if fileName is a tool name. Returns "-" if bare, null if dropped, or original fileName if real. |
There was a problem hiding this comment.
Please convert to a Javadoc comment.
Also please do not use null as a value in my code if possible. You can return a Optional.empty in that case.
It also makes sense to global replace in this parser "-" with something like NO_SOURCE_FILE.
| } | ||
|
|
||
| var baseFileName = FilenameUtils.getName(fileName).trim(); | ||
| var baseFileName = java.util.Objects.requireNonNull(FilenameUtils.getName(fileName)); |
There was a problem hiding this comment.
When you check in the beginning, no need to check here.
| return true; | ||
| // Checks if fileName is a tool name. Returns "-" if bare, null if dropped, or original fileName if real. | ||
| @CheckForNull | ||
| private String checkToolName(final String fileName) { |
There was a problem hiding this comment.
| private String checkToolName(final String fileName) { | |
| private String checkToolName(@CheckForNull final String fileName) { |
| // Checks if fileName is a tool name. Returns "-" if bare, null if dropped, or original fileName if real. | ||
| @CheckForNull | ||
| private String checkToolName(final String fileName) { | ||
| if ("-".equals(fileName) || "unknown.file".equals(fileName)) { |
There was a problem hiding this comment.
Wouldn't it make sense to check for null here?
Maybe it helps to look into the tests and the associated issues. I am not using this parser on my own so I cannot remember when we introduced that. |
98fbf20 to
6607240
Compare
When MSBuild's Exec task runs an external tool, it prefixes any output lines that lack a file name with the task name in upper-case (e.g. EXEC). isToolName() was treating these the same as executable names (.exe) and discarding the whole warning via Optional.empty(). This caused NuGet vulnerability warnings (NU1902, NU1903, etc.) to be silently dropped and never appear in the Jenkins Warnings plugin. Fix: split the isToolName branch. Executable names (.exe) and pseudo-file placeholders like <command line option> are still dropped. Bare MSBuild task names (EXEC, NMAKE, CSC, cl, rs, ...) now produce an issue with fileName set to '-' so the warning still shows up in the report. Fixes jenkinsci#1512
6607240 to
057f67b
Compare
|
Please do not use force push after a review as this discards the history. |
| private String normalizeFileName(final String fileName, @CheckForNull final String projectDir) { | ||
| var concatenated = FilenameUtils.concat(projectDir, fileName); | ||
| var normalized = canResolveRelativeFileName(fileName, projectDir) | ||
| ? StringUtils.defaultString(concatenated, fileName) : fileName; |
|
@uhafner |
The tests still fail. |
Fixes #1512
MSBuild's Exec task prefixes output lines with the task name (e.g. EXEC)
when there's no file path. The parser treated this the same as an .exe
name and dropped it, so NuGet warnings (NU1902/NU1903) never showed up
in the Warnings plugin.
Split isToolName into two cases - exe/placeholder names get dropped as
before, bare task names (EXEC, NMAKE, CSC etc.) now create an issue with
fileName "-" so the warning is visible.
Added issue1512.txt with the warnings from the bug report and a new test,
plus updated the existing 56613 tests since they now expect these warnings
to come through.