Extract file metadata for DiscUtils-based disc image formats#200
Conversation
Co-authored-by: gfs <[email protected]>
- Add TryGetFileMetadata() and CollectMetadata() helpers to DiscCommon for extracting Unix permissions, UID, and GID from IUnixFileSystem-compatible file systems (Ext, Xfs, Btrfs, HfsPlus, ISO 9660 with RockRidge) - Apply metadata extraction in DiscCommon (VHD/VHDX/VMDK), IsoExtractor, UdfExtractor, and WimExtractor - Add TestDataRockRidge.iso test fixture with RockRidge extensions - Add tests for ISO metadata extraction (both with and without RockRidge) - Restore nuget.config to original configuration Co-authored-by: gfs <[email protected]>
Co-authored-by: gfs <[email protected]>
- Add FileAttributes and SecurityDescriptorSddl properties to FileEntryMetadata - Update DiscCommon.TryGetFileMetadata() to handle IDosFileSystem (file attributes) and IWindowsFileSystem (SDDL security descriptors) in addition to IUnixFileSystem - Update DiscCommon.CollectMetadata() to recognize Windows file systems - Add VHDX/NTFS metadata tests verifying FileAttributes and SecurityDescriptorSddl - Restore nuget.config to original configuration Co-authored-by: gfs <[email protected]>
Co-authored-by: gfs <[email protected]>
There was a problem hiding this comment.
Pull request overview
Adds DiscUtils-backed filesystem metadata extraction so disc image formats (ISO/UDF/VHDX/WIM) can populate FileEntry.Metadata similarly to existing archive extractors, including Windows-specific NTFS/FAT metadata.
Changes:
- Extend
FileEntryMetadatawith WindowsFileAttributesand NTFSSecurityDescriptorSddl. - Add
DiscCommon.TryGetFileMetadata()/CollectMetadata()and wire metadata into DiscUtils-based extractors. - Add/expand tests and fixtures for ISO RockRidge Unix metadata and VHDX (NTFS) Windows metadata.
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| nuget.config | Normalizes XML header/format (restored feed configuration). |
| RecursiveExtractor/FileEntryMetadata.cs | Adds Windows file attributes + SDDL fields to metadata model. |
| RecursiveExtractor/Extractors/DiscCommon.cs | Implements DiscUtils metadata extraction and pre-collection helper. |
| RecursiveExtractor/Extractors/WimExtractor.cs | Sets metadata per extracted WIM entry via DiscCommon. |
| RecursiveExtractor/Extractors/IsoExtractor.cs | Pre-collects metadata while ISO FS is open and assigns to entries. |
| RecursiveExtractor/Extractors/UdfExtractor.cs | Pre-collects metadata while UDF FS is open and assigns to entries. |
| RecursiveExtractor.Tests/RecursiveExtractor.Tests.csproj | Includes new RockRidge ISO test fixture in outputs. |
| RecursiveExtractor.Tests/ExtractorTests/FileMetadataTests.cs | Adds coverage for ISO (with/without RockRidge) and VHDX NTFS metadata. |
| RecursiveExtractor.Tests/ExtractorTests/ExpectedNumFilesTests.cs | Adds expected file counts for the new RockRidge ISO fixture. |
…Extractor and ZipExtractor (#187) Reliability improvements for Tar and Zip extractors to improve behavior around failure to extract individual entries. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: gfs <[email protected]> Co-authored-by: Copilot <[email protected]>
ZipSlipSanitize only stripped .. sequences but did not handle absolute paths or Windows drive roots in archive entry names. Since
Path.Combine(base, absolutePath) discards the base argument when the second path is absolute, ExtractToDirectory could write files outside
the intended output directory. A secondary typo ($"{sep},{sep}" instead of $"{sep}{sep}") also prevented double-separator cleanup from ever
running.
Changes:
- Normalize path separators to OS-native, strip drive-letter roots (only when followed by a separator), strip leading separators, and
remove ../. as whole path segments (not substrings, preserving names like file..txt)
- Apply ZipSlipSanitize before SanitizePath in the FileEntry constructor so traversal segments are removed while path structure is intact
- Add defense-in-depth resolved-path containment check in all ExtractToDirectory code paths (sync, parallel, async)
- Trim trailing separator from resolved output directory before appending one, preventing double-separator mismatch when the output is a
root path
- Make drive-root stripping recursive to prevent nested-root bypass (e.g. C:\C:\...)
- Add comprehensive SanitizePathTests covering absolute Unix/Windows paths, drive roots, .. traversal, combined attacks, colon-containing
filenames, . segments, double-separator collapsing, and FileEntry integration
-----------------
Co-authored-by: gfs <[email protected]>
Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: Copilot <[email protected]>
* Update Microsoft.Bcl.AsyncInterfaces to version 10.0.6 * Update System.Linq.AsyncEnumerable package version
…nReader/OpenArchive surface (#205) * Initial plan * Upgrade SharpCompress to 0.47.4 and migrate breaking API calls Agent-Logs-Url: https://github.com/microsoft/RecursiveExtractor/sessions/4a1fc24b-3c85-4d07-ae1d-e352607abda7 Co-authored-by: gfs <[email protected]> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: gfs <[email protected]>
* Plan: bump dependency versions to latest * Bump dependencies to latest; * Use SharpCompress 0.49.1; fix PAX-header test counts --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: Copilot App <[email protected]> Copilot-Session: 64923784-69ff-4396-9d02-8320536ce3e2
…xceptions The two IsoEntries_MetadataIsNull* tests were written when only IUnixFileSystem was handled and were never updated when IDosFileSystem support was added, so they asserted null metadata for plain ISOs that now report FileAttributes. They failed and left the PR incomplete. - Fix the ISO tests to assert the real behavior and tighten the RockRidge and NTFS assertions to check concrete values instead of just non-null. - Add UDF and WIM metadata tests, covering the previously untested WimExtractor and UdfExtractor paths. - Skip the Unix branch for non-RockRidge ISO images. CDReader implements IUnixFileSystem unconditionally but throws for every file when the active variant is not RockRidge, which meant one thrown/caught exception per file. - Normalize an empty security descriptor to null so SecurityDescriptorSddl is either absent or meaningful (WIM images report empty SDDL). - Clarify the XML docs per review feedback and document FileEntryMetadata and the per-format support matrix in the README. Co-authored-by: Copilot App <[email protected]> Copilot-Session: 64923784-69ff-4396-9d02-8320536ce3e2
There was a problem hiding this comment.
Review details
Suppressed comments (3)
RecursiveExtractor/Extractor.cs:574
- Same as the non-parallel path: on Windows the output-directory containment check should be case-insensitive to avoid false positives when path casing differs after normalization.
var targetPath = Path.Combine(outputDirectory, entry.GetSanitizedPath());
var fullTargetPath = Path.GetFullPath(targetPath);
if (!fullTargetPath.StartsWith(parallelFullOutputDir, StringComparison.Ordinal))
{
Logger.Warn("Skipping entry that would escape output directory: {0}", entry.FullPath);
return;
RecursiveExtractor/Extractor.cs:672
- Same containment check issue in the async extraction path: use OrdinalIgnoreCase on Windows so in-directory targets aren't accidentally skipped due to casing differences.
var targetPath = Path.Combine(outputDirectory, entry.GetSanitizedPath());
var fullTargetPath = Path.GetFullPath(targetPath);
if (!fullTargetPath.StartsWith(asyncFullOutputDir, StringComparison.Ordinal))
{
Logger.Warn("Skipping entry that would escape output directory: {0}", entry.FullPath);
continue;
RecursiveExtractor/Extractor.cs:505
- On Windows, using a case-sensitive StartsWith comparison can incorrectly treat valid in-directory paths as escaping the output directory if the drive letter or path casing differs (common with symlinks/UNC normalization). Use a Windows-specific case-insensitive comparison for this containment check.
This issue also appears in the following locations of the same file:
- line 569
- line 667
var targetPath = Path.Combine(outputDirectory, entry.GetSanitizedPath());
var fullTargetPath = Path.GetFullPath(targetPath);
if (!fullTargetPath.StartsWith(fullOutputDir, StringComparison.Ordinal))
{
Logger.Warn("Skipping entry that would escape output directory: {0}", entry.FullPath);
- Files reviewed: 27/28 changed files
- Comments generated: 0 new
- Review effort level: Lite
…SO RockRidge README: add ZIP, RAR and 7z to the file metadata support table. The table read as exhaustive while omitting three formats that already populate Mode, so it implied they carried no metadata. DiscCommon.CollectMetadata: read each entry under its own try/catch. It was called inside the try that guards reader construction and entry enumeration, so an unexpected failure while reading metadata would have marked the entire ISO or UDF image as a failed archive. Collection is now documented and implemented as non-throwing, and one bad entry no longer costs metadata for the rest. DiscCommon.CollectIsoMetadata: recover RockRidge metadata from images that carry both Joliet and RockRidge. CDReader is opened with Joliet given priority, and DiscUtils only parses SUSP records for the variant it activates, so on an image built with both (mkisofs -J -R) Mode, Uid and Gid were silently dropped. When the active variant is not RockRidge, a second reader that skips Joliet is opened and its Unix metadata merged by path. Plain ISOs pay one extra volume descriptor scan and nothing per file, since the fallback stops as soon as the second reader reports a non-RockRidge variant. Adds TestDataJolietRockRidge.iso (built with pycdlib, rock_ridge 1.09 + joliet 3, mirroring the layout of TestDataRockRidge.iso) with sync and async coverage, and wires it into the expected file count theories. Co-authored-by: Copilot App <[email protected]> Copilot-Session: 1d67ebf0-3614-404e-ac29-0c60ee93469a
|
Reviewed this and pushed a follow-up commit (f61c96d) rather than sending it back for another round. Three things: Hybrid Joliet + RockRidge ISOs were silently losing Unix metadata. Added
The README support table was missing formats that already populate metadata. ZIP ( Things I looked at and deliberately left alone: plain ISOs now attach a non-null 676 lib tests and 37 CLI tests pass locally on net9.0, and the library still builds on every TFM including netstandard2.0. Nice work on the write-up, by the way — documenting exactly why the branch went red made this much faster to pick up. |
Extends the file metadata work from #196 to the DiscUtils-based disc image formats, and finishes the parts of this PR that were left incomplete.
What's new
FileEntryMetadata.FileAttributes— Windows/DOS file attributes.FileEntryMetadata.SecurityDescriptorSddl— Windows security descriptor in SDDL form.DiscCommon.TryGetFileMetadata()reads metadata from any file system implementingIUnixFileSystem(mode/uid/gid),IDosFileSystem(attributes), orIWindowsFileSystem(SDDL), so VHD/VHDX/VMDK/DMG, ISO 9660, UDF, and WIM all pick up whatever the underlying file system exposes.DiscCommon.CollectMetadata()pre-collects metadata for the ISO/UDF extractors, which dispose the reader before entries are consumed.Support by format, verified by reflecting over the DiscUtils file system types:
Mode,Uid,GidMode,Uid,Gid,FileAttributesFileAttributesFileAttributesFileAttributes,SecurityDescriptorSddlFileAttributes, plusSecurityDescriptorSddlwhen recordedMetadatastaysnullWhy this PR was stalled
IsoEntries_MetadataIsNullWithoutRockRidgeand its_Synctwin were written in the first commit, when onlyIUnixFileSystemwas handled. The follow-up commit addedIDosFileSystemsupport, andCDReaderimplements it — so plain ISO entries started reportingFileAttributes = ReadOnlyand those two tests began failing on an assertion ofnullmetadata. They were never updated, so the branch was left red and the PR stayed a draft.Fixes in this pass
0755/0644, uid/gid1001,FileAttributes.Archive, owner and DACL present in the SDDL) instead of just non-null.UdfExtractorandWimExtractorpaths that previously had no metadata coverage.CDReaderimplementsIUnixFileSystemunconditionally but throwsInvalidOperationExceptionfor every entry when the active variant isn't RockRidge.TryGetFileMetadatanow checksActiveVariantup front. SinceCDReaderis constructed with Joliet priority (Joliet>RockRidge>Iso9660) and SUSP records are only parsed for the RockRidge variant, this is an exact check, not a heuristic.null, soSecurityDescriptorSddlis either absent or meaningful. WIM images report an empty SDDL rather than no descriptor.CDReader, note the RockRidge requirement, and state that the lists aren't exhaustive.FileEntryMetadataand the support matrix in the README, and addedMetadatato the documentedFileEntrysurface.main(SharpCompress 0.49.1 and theOpenReader/OpenArchivemigration landed after this branch was cut).nuget.configis byte-identical tomain; the earlier BOM/newline drift is gone.Validation
662 tests × net8.0/net9.0/net10.0/net48 and 37 CLI tests × net8.0/net9.0/net10.0, all passing.
Fixes #197