Tar: Ensure trailing separator in sanitized path for extraction #87534
Conversation
carlossanlop
requested review from
tmds,
jeffhandley,
GrabYourPitchforks,
ViktorHofer,
ericstj and
jozkee
|
Tagging subscribers to this area: @dotnet/area-system-formats-tar Issue Detailsnull
|
| var longFolderNames = new string[] | ||
| { | ||
| // Long path with many short segment names and a filename | ||
| "folderfolder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder/folder", |
There was a problem hiding this comment.
Can you generate these rather than paste in big strings? It's self documenting. Tests do this various ways eg string.Create, Enumerable.Repeat, maybe there's some Utility class. The micro benchmarks have another way, etc
| [InlineData(TarEntryFormat.Gnu)] | ||
| public async Task ExtractToDirectory_ExactRootDirMatch_HardLinks_Throws_Async(TarEntryFormat format) | ||
| { | ||
| await ExtractToDirectory_ExactRootDirMatch_Links_Throws_Async(format, TarEntryType.HardLink, inverted: false); |
There was a problem hiding this comment.
It might be cleaner to have one method call with a foreach over new bool[] { true, false }
| string linkPath = Path.Join(entryFolderPath, linkFileName); | ||
| string linkTargetPath = Path.Join(destinationFolderPath, linkTargetFileName); | ||
|
|
||
| Directory.CreateDirectory(entryFolderPath); |
There was a problem hiding this comment.
Is it worth extracting CreateDirectories(string[]) and using across your tests?
| string entryFolderName = "folder"; | ||
| string destinationFolderName = "folderSibling"; | ||
|
|
||
| using TempDirectory root = new TempDirectory(); |
There was a problem hiding this comment.
Possibly creating temp directory is cleaner in a single GlobalSetup?
Just looking for ways to make these tests shorter and cleaner by removing repeated stuff
| fileName = PathInternal.DirectorySeparatorCharAsString; | ||
| } | ||
|
|
||
| string sanitizedPath = Path.Join(Path.GetDirectoryName(normalizedPath), ArchivingUtils.SanitizeEntryFilePath(fileName)); |
There was a problem hiding this comment.
| string sanitizedPath = Path.Join(Path.GetDirectoryName(normalizedPath), ArchivingUtils.SanitizeEntryFilePath(fileName)); | |
| string sanitizedPath = Path.Join(Path.GetDirectoryName(normalizedPath), ArchivingUtils.SanitizeEntryFilePath(fileName)); |
Fwiw I think "GetSanitizedEntryFilePath" reads better than SanitizeEntryFilePath as it's idempotent.
(Or SanitizePath?)
| @@ -365,9 +365,17 @@ internal Task ExtractRelativeToDirectoryAsync(string destinationDirectoryPath, b | |||
| // If the path can be extracted in the specified destination directory, returns the full path with sanitized file name. Otherwise, returns null. | |||
| private static string? GetSanitizedFullPath(string destinationDirectoryFullPath, string path) | |||
| { | |||
| destinationDirectoryFullPath = PathInternal.EnsureTrailingSeparator(destinationDirectoryFullPath); | |||
There was a problem hiding this comment.
Just before this method gets called, there's a call that removes the directory separator:
Probably that can be removed?
Can we also add a call to PathInternal.EnsureTrailingSeparator higher up the call stack when called from TarFile.ExtractToDirectory to avoid allocating a string here each time an entry gets extracted?
| fileName = PathInternal.DirectorySeparatorCharAsString; | ||
| } | ||
|
|
||
| string sanitizedPath = Path.Join(Path.GetDirectoryName(normalizedPath), ArchivingUtils.SanitizeEntryFilePath(fileName)); |
There was a problem hiding this comment.
We're only calling ArchivingUtils.SanitizeEntryFilePath on the fileName, probably it makes sense to call it on path (which is the path that was stored in the entry) at the start of this method?
I think we no longer need to get the fileName then, which will simplify this method.
|
@danmoseley @tmds If you have no objections, I'd like to get this merged now to make sure it goes into the Preview6 snap. I opened issue #87835 as a reminder. |
ghost
locked as resolved and limited conversation to collaborators
No description provided.