search

microsoft / sarif-sdk

.NET code and supporting files for working with the 'Static Analysis Results Interchange Format' (SARIF, see https://github.com/oasis-tcs/sarif-spec)
Other
189 stars 88 forks source link

Report `WRN997_InvalidTarget` if the path of the scan target contains invalid character. Scan file with URL-decodable name without throwing exceptions. #2783

Closed LingZhou-gh closed 4 months ago

LingZhou-gh commented 5 months ago

When the path of the scan target contains invalid character, the exception 'ArgumentException: Illegal characters in path' was raised for 100++ rule checks, as reported in a bug.

This fix check the path of the scan target before invoking any rule skimmer. If the path contains invalid character or URL encoded character, return WRN997_InvalidTarget.

Unit tests are added/updated.

====More detailed description=== In Microsoft.CodeAnalysis.Sarif.RuntimeConditions enum, there are at least 2 different target-related condition: Fatal one (Error): NoValidAnalysisTargets Non-fatal (Warning): TargetNotValidToAnalyze If we have 2 files to scan, one is a normal file, another is a file with weird filename (invalid char or contains %2F) We won't see NoValidAnalysisTargets, as we do have a valid target.

But when file with weird name continues to flow through the code, it might cause trouble somewhere. 1) if the filename contains URL encoded character, such as "%2F", when URI is made out of the filename. The filename changed!, which means it won't be located later. Eg: Incoming-email-not-synchronized-%2D-%22Ignoring-item%22.md will be changed to Incoming-email-not-synchronized---"Ignoring-item".md

2) if the filename contains invalid character as defined by System.IO.Path.GetInvalidPathChars, it could throw exception as documented by Path.GetExtension Method (System.IO) | Microsoft Learn

LingZhou-gh commented 5 months ago

@stacywray is added to the review. #Closed

LingZhou-gh commented 5 months ago

@stacywray is added to the review. #Closed

LingZhou-gh commented 5 months ago

@stacywray is added to the review. #Closed

michaelcfanning commented 5 months ago 
    public void AnalyzeCommand_EncodedPaths()

this change isn't right. i don't think.

we authored this test because these aren't legal file paths, they are relative paths. #Resolved

Refers to: src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs:835 in 7340d37. [](commit_id = 7340d376bb832c77894b6bd2509cfa3b3dd79d27, deletion_comment = False)

LingZhou-gh commented 5 months ago 
    public void AnalyzeCommand_EncodedPaths()

The test case title is "..._EncodedPaths"....

In reply to: 1939774900

Refers to: src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs:835 in 7340d37. [](commit_id = 7340d376bb832c77894b6bd2509cfa3b3dd79d27, deletion_comment = False)

LingZhou-gh commented 4 months ago 
    public void AnalyzeCommand_EncodedPaths()

With new commit, this test case can keep unchanged. Add a new test case instead.

In reply to: 1942193638

Refers to: src/Test.UnitTests.Sarif.Driver/Sdk/AnalyzeCommandBaseTests.cs:835 in 7340d37. [](commit_id = 7340d376bb832c77894b6bd2509cfa3b3dd79d27, deletion_comment = False)