SLVS-1436 Escape rfc 3986 reserved chars in file names. #5668
Conversation
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
|
|
||
| public FileUri(string uriString) | ||
| { | ||
| uri = new Uri(uriString); | ||
| var unescapedUri = Uri.UnescapeDataString(uriString); |
There was a problem hiding this comment.
I'm kind of afraid of this method. Have you tested this end to end with weird filenames (reserved chars, other escape-able chars, whitespaces?)
There was a problem hiding this comment.
I have tested it with reserved chars and it works, yes. I don't think there is any reason to be suspicious about it. it just unescapes all encoded characters. The Uri.EscapeDataString was the one what we didn't want to use, due to escaping characters that we might not want.
There was a problem hiding this comment.
But you could also go ahead and test it. Maybe I missed some cases. :)
I just tested that having the reserved chars in javascript file name leads to analysis working for those files (issues are shown)
| return !charsToEscape.Any() | ||
| ? stringToEscape | ||
| : charsToEscape.Aggregate(stringToEscape, (current, charToEscape) => current.Replace(charToEscape.ToString(), Uri.HexEscape(charToEscape))); | ||
| } |
There was a problem hiding this comment.
Let's use k:=Rfc3986ReservedCharsToEncoding.Length, l:=charsToEscape.Length, n:=stringToEscape.Length
Line 59 already gives you O(n*k) complexity
The fact that you optimize k down to l on line 63 does not matter, especially since 0<=l<=k=5 and string replace not returning new string if there were no replacements made
Just create a StringBuilder and run replace in foreach(Rfc3986ReservedCharsToEncoding). That would be a lot simpler :-)
There was a problem hiding this comment.
Yes, line 59 gives you O(n) complexity, which will probably be 99% of the cases, because I don't expected that the Rfc3986ReservedChars to exist in file names too often.
There was a problem hiding this comment.
That's the thing, you could just remove that line and leave the rest of the code as is (charToEscape would be equal to all reserved characters), and it would actually reduce the number of iterations for the same input. Using a string builder is optional. If you think that more often than not it's going to be returning the same string, then we can use string replace aggregate to save on the StringBuilder creation. Though, it takes nanoseconds for new StringBuilder(string).ToString() to execute
| [DataRow("]", "%5D")] | ||
| [DataRow("#", "%2523")] | ||
| [DataRow("@", "%40")] | ||
| public void ToString_PercentEncodesReservedRfc3986Characters(string reservedChar, string expectedEncoding) |
There was a problem hiding this comment.
Maybe add similar tests, but in reverse: from file://... to FileUri.LocalPath, rather than from filePath to uri string
| @@ -152,4 +165,15 @@ public void Deserialize_ProducesCorrectUri() | |||
| fileUri.ToString().Should().Be("file:///C:/file%20with%20%204%20spaces%20and%20a%20back%60tick"); | |||
| fileUri.LocalPath.Should().Be(@"C:\file with 4 spaces and a back`tick"); | |||
| } | |||
|
|
|||
| [TestMethod] | |||
| public void Deserialize_ReservedRfc3986Characters_ProducesCorrectUri() | |||
There was a problem hiding this comment.
Maybe replace this test with a similar testcase based test I mentioned above
There was a problem hiding this comment.
I won't replace it, I will just add one more test. This calls the FileUriConverter, which is also important.
- remove check if the filename contains reserved char for performance reason - remove ? char from reserved list, because it is not supported in file names in windows and it also increases performance - use string builder to improve performance - add more unit tests
|
gabriela-trutan-sonarsource
requested a review
from georgii-borovinskikh-sonarsource
|
Summary:
|
gabriela-trutan-sonarsource
merged commit 3ff6fc4
into
feature/hardening-sept-2024
gabriela-trutan-sonarsource
deleted the
gt/escape-rfc-3986-chars-in-uri
branch
SLVS-1436