Skip to content
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

Fix path resolution for relative SARIF artifact locations relative to %SRCROOT% #615

Merged
merged 2 commits into from
Apr 3, 2024

Conversation

chrishuynhc
Copy link
Collaborator

Overview

Issue

This change addresses an issue found when trying to manually open a SARIF log containing a relative URI for artifactLocation such as:

"artifactLocation": {
     "uri": "Foo/Bar.cs",
     "uriBaseId": "%SRCROOT%"
}

in which the SARIF log was not successfully processed with the following error:

Processing the SARIF log file `<SARIF log path>`.
Failed to process log file. Reason: Invalid URI: The format of the URI could not be determined.
The SARIF log file `<SARIF log path>` processed.

The issue was caused by the path resolution mechanism returning a relative path as the "resolved path" which caused an exception to be thrown when trying to convert that relative resolved path to an absolute Uri (System.UriFormatException - Invalid URI: The format of the URI could not be determined).

To dive deeper, the reason the relative path was returned was because _fileSystem.FileExists was utilized to verify the resolved path existed on the file system. For more context, _fileSystem.FileExists is simply a wrapper around File.Exists. If File.Exists is passed a relative path, which is possible in this scenario, it is interpreted as relative to the current working directory and passes the check. As a result, the relative path is assigned as the "resolved path" where as we expect resolved path to be a full path. This relative resolved path then causes issues later in the flow.

Fix

The fix modifies src/Sarif.Viewer.VisualStudio.Core/CodeAnalysisResultManager.cs to return a fully resolved and normalized path by verifying that the path is rooted. If it is rooted, great, simply return it. If not, make sure to combine (Path.Combine) the relative path with the base path before returning.

The GetCommonSuffix utility, which is used later in the path resolution flow, was also modified to normalize both paths before doing a comparison. Although this might cause redundancy if a path is already normalized, it removes the responsibility from the caller to handle path normalization.

@EasyRhinoMSFT EasyRhinoMSFT reopened this Mar 29, 2024
@EasyRhinoMSFT EasyRhinoMSFT reopened this Mar 29, 2024
@EasyRhinoMSFT EasyRhinoMSFT reopened this Apr 3, 2024
@EasyRhinoMSFT EasyRhinoMSFT merged commit 0033272 into main Apr 3, 2024
8 checks passed
@EasyRhinoMSFT EasyRhinoMSFT deleted the user/chhuynh/fixResolvedPath branch April 3, 2024 19:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants