-
Notifications
You must be signed in to change notification settings - Fork 177
Fix SbomValidator.ValidateSbomAsync false positive when outputPath is directory #1131
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
…or to check both errors and exceptions Co-authored-by: JoseRenan <[email protected]>
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
1 similar comment
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
|
/azp run |
Co-authored-by: sfoslund <[email protected]>
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
|
This PR changes files in the API project. Does it change any of the API interfaces in any way? Please note that this includes the following types of changes:
Because any of these changes can potentially break a downstream consumer with customized interface implementations, these changes need to be treated as breaking changes. Please do one of the following: Option 1 - Publish this as a breaking change
Option 2 - Refactor the changes to be non-breaking
|
…ator initialization Co-authored-by: DaveTryon <[email protected]>
Co-authored-by: DaveTryon <[email protected]>
Co-authored-by: DaveTryon <[email protected]>
Co-authored-by: DaveTryon <[email protected]>
| telemetryRecorder.RecordException(testException2); | ||
|
|
||
| Assert.AreEqual(2, exceptions.Count); | ||
| Assert.IsTrue(exceptions.Contains(testException1)); |
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.
testException1 should be at index 0.
testException2 should be at index 1
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.
Fixed the assertions to check specific indices instead of just containment. Now verifying that testException1 is at index 0 and testException2 is at index 1, which provides a more precise test of the storage order. d53a3bc
…ment Co-authored-by: DaveTryon <[email protected]>
| { | ||
| public IList<FileValidationResult> Errors => throw new NotImplementedException(); | ||
|
|
||
| public IList<Exception> Exceptions => throw new NotImplementedException(); |
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.
Please remove the Exceptions property
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.
Removed the Exceptions property from the PinnedIRecorder class as requested. 3f1cae3
Co-authored-by: DaveTryon <[email protected]>
Problem
When
SbomValidator.ValidateSbomAsyncis called with a directory path as theoutputPathparameter, it incorrectly returnsIsSuccessful = trueeven though validation fails. This happens because:FileOutputWriter.WriteAsync()throws an exception when trying to create a FileStream with a directory pathrecorder.RecordException(e)SbomValidator.ValidateSbomAsynconly checksrecorder.Errorsto determine success, ignoring recorded exceptionsIsSuccessful = truedespite the validation actually failingSolution
Added an
Exceptionsproperty to theIRecorderinterface and updatedSbomValidatorto consider both errors and exceptions when determining validation success:Changes Made
Exceptionsproperty to expose recorded exceptionsExceptionsproperty to return the privateexceptionscollectionIsSuccess = trueIsSuccess = falseIsSuccess = false(key fix)IsSuccess = falseTesting
All existing tests pass (370 tests) plus 6 new tests specifically for this fix. The solution maintains backward compatibility while properly handling the exception scenario.
Example
Fixes #1093.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.