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
193 stars 91 forks source link

File Path Encoding Bugfix #2796

Closed scottoneil-ms closed 2 months ago

scottoneil-ms commented 7 months ago

This change proposes to make it a convention that we use Uri.OriginalPath for file IO operations (not Uri.GetFilePath()), and eliminates calls that could introduce URI-encoding into file paths in ways that can not be thereby reversed. Ling has correctly pointed out that this means we would not support file:// URI inputs. If that's not acceptable, we might need to work this a little bit more.

Part of this work was a comprehensive review of calls to Uri.GetFilePath(), looking to divert calls that seem intended to lead to actual file IO APIs, as opposed to logging output and such. All found callsites are addressed, except one in Multitool. That site may need a little more work to address because of how it interacts with some of its Unit Tests. This can be done in a future change.

michaelcfanning commented 7 months ago 
    public long? SizeInBytes

this code really should be commented if it raises exceptions for invalid file paths. you didn't create this technical debt but i can no longer remain silent about it.

Refers to: src/Sarif/EnumeratedArtifact.cs:147 in a7f7257. [](commit_id = a7f7257c7d9b0753c93513329c9e043b21af5f4c, deletion_comment = False)

rwoll commented 2 months ago

Thank you for your PR. As part of regular tidying and triage/prioritization, closing due to staleness.