Complete disc image metadata support: fix ISO tests, avoid per-file exceptions

gfs · Copilot · gfs · commit b4552e8cd82f · 2026-07-31T06:49:33.000-07:00
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 &lt;223556219+Copilot@users.noreply.github.com&gt;
Copilot-Session: 64923784-69ff-4396-9d02-8320536ce3e2
diff --git a/README.md b/README.md
@@ -127,6 +127,47 @@ public string? ParentPath { get; }
 public DateTime CreateTime { get; }
 public DateTime ModifyTime { get; }
 public DateTime AccessTime { get; }
+public FileEntryMetadata? Metadata { get; set; }
+```
+</details>
+
+<details>
+<summary>File Metadata</summary>
+<br/>
+When the source archive or disc image records it, `FileEntry.Metadata` holds additional attributes for the entry. Every property is nullable, and `Metadata` itself is `null` when the format carries no metadata at all, so you can distinguish "not recorded" from a real value.
+
+```csharp
+public long? Mode { get; set; }                     // Unix permission bits
+public bool? IsExecutable { get; }                  // Derived from Mode
+public bool? IsSetUid { get; }                      // Derived from Mode
+public bool? IsSetGid { get; }                      // Derived from Mode
+public long? Uid { get; set; }                      // Unix owner id
+public long? Gid { get; set; }                      // Unix group id
+public FileAttributes? FileAttributes { get; set; } // Windows/DOS file attributes
+public string? SecurityDescriptorSddl { get; set; } // Windows security descriptor, SDDL form
+```
+
+Which properties are populated depends on the format:
+
+| Source | Populated |
+| --- | --- |
+| TAR, AR/DEB | `Mode`, `Uid`, `Gid` |
+| Ext, XFS, Btrfs, HFS+ (inside VHD/VHDX/VMDK/DMG) | `Mode`, `Uid`, `Gid` |
+| ISO 9660 with RockRidge extensions | `Mode`, `Uid`, `Gid`, `FileAttributes` |
+| ISO 9660 without RockRidge extensions | `FileAttributes` |
+| FAT (inside a disc image) | `FileAttributes` |
+| NTFS (inside a disc image) | `FileAttributes`, `SecurityDescriptorSddl` |
+| WIM | `FileAttributes`, and `SecurityDescriptorSddl` when the image records one |
+| UDF | None; `Metadata` is `null` |
+
+```csharp
+foreach (var file in extractor.Extract("path/to/image.vhdx"))
+{
+    if (file.Metadata?.SecurityDescriptorSddl is { } sddl)
+    {
+        Console.WriteLine($"{file.FullPath}: {sddl}");
+    }
+}
 ```
 </details>
 
diff --git a/RecursiveExtractor.Tests/ExtractorTests/FileMetadataTests.cs b/RecursiveExtractor.Tests/ExtractorTests/FileMetadataTests.cs
@@ -1,8 +1,10 @@
-﻿// Copyright (c) Microsoft Corporation. Licensed under the MIT License.
+// Copyright (c) Microsoft Corporation. Licensed under the MIT License.
 
 using Microsoft.CST.RecursiveExtractor;
+using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using System.Runtime.InteropServices;
 using System.Threading.Tasks;
 using Xunit;
 
@@ -144,23 +146,28 @@ public void FileEntry_MetadataDefaultsToNull()
     }
 
     [Fact]
-    public async Task IsoEntries_MetadataIsNullWithoutRockRidge()
+    public async Task IsoEntries_HaveDosAttributesButNoUnixMetadataWithoutRockRidge()
     {
-        // TestData.iso does not have RockRidge extensions, so Unix metadata is not available
+        // TestData.iso has no RockRidge extensions, so Unix metadata is unavailable.
+        // CDReader still implements IDosFileSystem, so file attributes are reported.
         var extractor = new Extractor();
         var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "TestData.iso");
         var results = await extractor.ExtractAsync(path, new ExtractorOptions() { Recurse = false }).ToListAsync();
 
         Assert.NotEmpty(results);
         foreach (var entry in results)
         {
-            // Without RockRidge extensions, metadata should be null
-            Assert.Null(entry.Metadata);
+            Assert.NotNull(entry.Metadata);
+            Assert.Equal(FileAttributes.ReadOnly, entry.Metadata!.FileAttributes);
+            Assert.Null(entry.Metadata.Mode);
+            Assert.Null(entry.Metadata.Uid);
+            Assert.Null(entry.Metadata.Gid);
+            Assert.Null(entry.Metadata.SecurityDescriptorSddl);
         }
     }
 
     [Fact]
-    public void IsoEntries_MetadataIsNullWithoutRockRidge_Sync()
+    public void IsoEntries_HaveDosAttributesButNoUnixMetadataWithoutRockRidge_Sync()
     {
         var extractor = new Extractor();
         var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "TestData.iso");
@@ -169,7 +176,12 @@ public void IsoEntries_MetadataIsNullWithoutRockRidge_Sync()
         Assert.NotEmpty(results);
         foreach (var entry in results)
         {
-            Assert.Null(entry.Metadata);
+            Assert.NotNull(entry.Metadata);
+            Assert.Equal(FileAttributes.ReadOnly, entry.Metadata!.FileAttributes);
+            Assert.Null(entry.Metadata.Mode);
+            Assert.Null(entry.Metadata.Uid);
+            Assert.Null(entry.Metadata.Gid);
+            Assert.Null(entry.Metadata.SecurityDescriptorSddl);
         }
     }
 
@@ -181,30 +193,67 @@ public async Task IsoRockRidgeEntries_HaveMetadata()
         var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "TestDataRockRidge.iso");
         var results = await extractor.ExtractAsync(path, new ExtractorOptions() { Recurse = false }).ToListAsync();
 
-        Assert.NotEmpty(results);
+        AssertRockRidgeMetadata(results);
+    }
+
+    [Fact]
+    public void IsoRockRidgeEntries_HaveMetadata_Sync()
+    {
+        var extractor = new Extractor();
+        var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "TestDataRockRidge.iso");
+        var results = extractor.Extract(path, new ExtractorOptions() { Recurse = false }).ToList();
+
+        AssertRockRidgeMetadata(results);
+    }
+
+    private static void AssertRockRidgeMetadata(IList<FileEntry> results)
+    {
+        Assert.Equal(2, results.Count);
         foreach (var entry in results)
         {
             Assert.NotNull(entry.Metadata);
-            Assert.NotNull(entry.Metadata!.Mode);
-            Assert.NotNull(entry.Metadata.Uid);
-            Assert.NotNull(entry.Metadata.Gid);
+            Assert.Equal(1001, entry.Metadata!.Uid);
+            Assert.Equal(1001, entry.Metadata.Gid);
+            Assert.NotNull(entry.Metadata.FileAttributes);
         }
+
+        // testfile.txt is 0755 (493 decimal), subdir/nested.txt is 0644 (420 decimal)
+        var topLevel = results.Single(x => x.Name == "testfile.txt");
+        Assert.Equal(493, topLevel.Metadata!.Mode);
+        Assert.True(topLevel.Metadata.IsExecutable);
+
+        var nested = results.Single(x => x.Name == "nested.txt");
+        Assert.Equal(420, nested.Metadata!.Mode);
+        Assert.False(nested.Metadata.IsExecutable);
     }
 
     [Fact]
-    public void IsoRockRidgeEntries_HaveMetadata_Sync()
+    public async Task UdfEntries_HaveNoMetadata()
     {
+        // UdfReader implements none of the Unix/DOS/Windows file system interfaces,
+        // so no metadata is available for pure UDF images.
         var extractor = new Extractor();
-        var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "TestDataRockRidge.iso");
+        var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "UdfTest.iso");
+        var results = await extractor.ExtractAsync(path, new ExtractorOptions() { Recurse = false }).ToListAsync();
+
+        Assert.NotEmpty(results);
+        foreach (var entry in results)
+        {
+            Assert.Null(entry.Metadata);
+        }
+    }
+
+    [Fact]
+    public void UdfEntries_HaveNoMetadata_Sync()
+    {
+        var extractor = new Extractor();
+        var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "UdfTest.iso");
         var results = extractor.Extract(path, new ExtractorOptions() { Recurse = false }).ToList();
 
         Assert.NotEmpty(results);
         foreach (var entry in results)
         {
-            Assert.NotNull(entry.Metadata);
-            Assert.NotNull(entry.Metadata!.Mode);
-            Assert.NotNull(entry.Metadata.Uid);
-            Assert.NotNull(entry.Metadata.Gid);
+            Assert.Null(entry.Metadata);
         }
     }
 
@@ -216,14 +265,30 @@ public async Task VhdxNtfsEntries_HaveWindowsMetadata()
         var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "TestData.vhdx");
         var results = await extractor.ExtractAsync(path, new ExtractorOptions() { Recurse = false }).ToListAsync();
 
+        AssertNtfsMetadata(results);
+    }
+
+    [Fact]
+    public void VhdxNtfsEntries_HaveWindowsMetadata_Sync()
+    {
+        var extractor = new Extractor();
+        var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "TestData.vhdx");
+        var results = extractor.Extract(path, new ExtractorOptions() { Recurse = false }).ToList();
+
+        AssertNtfsMetadata(results);
+    }
+
+    private static void AssertNtfsMetadata(IList<FileEntry> results)
+    {
         Assert.NotEmpty(results);
         foreach (var entry in results)
         {
             Assert.NotNull(entry.Metadata);
             // NTFS provides Windows file attributes
-            Assert.NotNull(entry.Metadata!.FileAttributes);
+            Assert.Equal(FileAttributes.Archive, entry.Metadata!.FileAttributes);
             // NTFS provides security descriptors
             Assert.NotNull(entry.Metadata.SecurityDescriptorSddl);
+            Assert.Contains("O:", entry.Metadata.SecurityDescriptorSddl); // Owner present
             Assert.Contains("D:", entry.Metadata.SecurityDescriptorSddl); // DACL present
             // NTFS does not provide Unix metadata
             Assert.Null(entry.Metadata.Mode);
@@ -233,19 +298,48 @@ public async Task VhdxNtfsEntries_HaveWindowsMetadata()
     }
 
     [Fact]
-    public void VhdxNtfsEntries_HaveWindowsMetadata_Sync()
+    public async Task WimEntries_HaveWindowsFileAttributes()
     {
+        // WIM extraction is only supported on Windows
+        if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+        {
+            return;
+        }
+
         var extractor = new Extractor();
-        var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "TestData.vhdx");
+        var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "TestData.wim");
+        var results = await extractor.ExtractAsync(path, new ExtractorOptions() { Recurse = false }).ToListAsync();
+
+        AssertWimMetadata(results);
+    }
+
+    [Fact]
+    public void WimEntries_HaveWindowsFileAttributes_Sync()
+    {
+        if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+        {
+            return;
+        }
+
+        var extractor = new Extractor();
+        var path = Path.Combine(Directory.GetCurrentDirectory(), "TestData", "TestDataArchives", "TestData.wim");
         var results = extractor.Extract(path, new ExtractorOptions() { Recurse = false }).ToList();
 
+        AssertWimMetadata(results);
+    }
+
+    private static void AssertWimMetadata(IList<FileEntry> results)
+    {
         Assert.NotEmpty(results);
         foreach (var entry in results)
         {
             Assert.NotNull(entry.Metadata);
-            Assert.NotNull(entry.Metadata!.FileAttributes);
-            Assert.NotNull(entry.Metadata.SecurityDescriptorSddl);
+            Assert.Equal(FileAttributes.Archive, entry.Metadata!.FileAttributes);
             Assert.Null(entry.Metadata.Mode);
+            Assert.Null(entry.Metadata.Uid);
+            Assert.Null(entry.Metadata.Gid);
+            // This WIM records no security descriptors, so an empty SDDL is normalized to null
+            Assert.Null(entry.Metadata.SecurityDescriptorSddl);
         }
     }
 }
diff --git a/RecursiveExtractor/Extractors/DiscCommon.cs b/RecursiveExtractor/Extractors/DiscCommon.cs
@@ -1,4 +1,5 @@
 ﻿using DiscUtils;
+using DiscUtils.Iso9660;
 using System;
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
@@ -16,12 +17,14 @@ public static class DiscCommon
 
         /// <summary>
         /// Tries to extract file metadata from a DiscUtils file system entry.
-        /// For file systems implementing <see cref="IUnixFileSystem"/> (Ext, Xfs, Btrfs, HfsPlus),
+        /// For file systems implementing <see cref="IUnixFileSystem"/> (such as Ext, Xfs, Btrfs, HfsPlus,
+        /// and ISO 9660 images via <c>CDReader</c> when RockRidge extensions are present),
         /// returns permissions, UID, and GID.
-        /// For file systems implementing <see cref="IDosFileSystem"/> (NTFS, FAT, WIM),
+        /// For file systems implementing <see cref="IDosFileSystem"/> (such as NTFS, FAT, WIM, and ISO 9660),
         /// returns Windows file attributes.
-        /// For file systems implementing <see cref="IWindowsFileSystem"/> (NTFS, WIM),
-        /// also returns the security descriptor in SDDL format.
+        /// For file systems implementing <see cref="IWindowsFileSystem"/> (such as NTFS and WIM),
+        /// also returns the security descriptor in SDDL format when the file system provides one.
+        /// The lists above are not exhaustive; support is determined by the interfaces the file system implements.
         /// Returns null for file systems that support none of these interfaces.
         /// </summary>
         /// <param name="fs">The opened disc file system</param>
@@ -31,7 +34,7 @@ public static class DiscCommon
         {
             FileEntryMetadata? metadata = null;
 
-            if (fs is IUnixFileSystem unixFs)
+            if (fs is IUnixFileSystem unixFs && SupportsUnixMetadata(fs))
             {
                 try
                 {
@@ -68,11 +71,12 @@ public static class DiscCommon
                 try
                 {
                     var securityDescriptor = windowsFs.GetSecurity(filePath);
-                    if (securityDescriptor != null)
+                    var sddl = securityDescriptor?.GetSddlForm(
+                        DiscUtils.Core.WindowsSecurity.AccessControl.AccessControlSections.All);
+                    if (!string.IsNullOrEmpty(sddl))
                     {
                         metadata ??= new FileEntryMetadata();
-                        metadata.SecurityDescriptorSddl = securityDescriptor.GetSddlForm(
-                            DiscUtils.Core.WindowsSecurity.AccessControl.AccessControlSections.All);
+                        metadata.SecurityDescriptorSddl = sddl;
                     }
                 }
                 catch (Exception e)
@@ -84,6 +88,15 @@ public static class DiscCommon
             return metadata;
         }
 
+        /// <summary>
+        /// Determines whether a file system that implements <see cref="IUnixFileSystem"/> can actually
+        /// return Unix metadata. <c>CDReader</c> implements the interface unconditionally but only exposes
+        /// Unix information when the active ISO 9660 variant is RockRidge, and throws for every other
+        /// variant. Checking up front avoids throwing and catching an exception for every file in an image.
+        /// </summary>
+        private static bool SupportsUnixMetadata(DiscFileSystem fs)
+            => fs is not CDReader cdReader || cdReader.ActiveVariant == Iso9660Variant.RockRidge;
+
         /// <summary>
         /// Pre-collects metadata for all files while the file system is still open.
         /// Used by extractors (e.g., ISO) where the file system is disposed before files are processed.
diff --git a/RecursiveExtractor/FileEntryMetadata.cs b/RecursiveExtractor/FileEntryMetadata.cs
@@ -48,15 +48,15 @@ public class FileEntryMetadata
 
         /// <summary>
         /// The Windows file attributes (e.g., ReadOnly, Hidden, System, Archive).
-        /// Available for NTFS, FAT, and WIM file systems.
+        /// Available for disc image file systems that expose DOS attributes, such as NTFS, FAT, WIM, and ISO 9660.
         /// Null if not available from the archive format.
         /// </summary>
         public FileAttributes? FileAttributes { get; set; }
 
         /// <summary>
-        /// The NTFS security descriptor in SDDL (Security Descriptor Definition Language) format.
-        /// Available for NTFS and WIM file systems that implement <c>IWindowsFileSystem</c>.
-        /// Null if not available from the archive format.
+        /// The Windows security descriptor in SDDL (Security Descriptor Definition Language) format.
+        /// Available for disc image file systems that expose Windows security descriptors, such as NTFS and WIM.
+        /// Null if not available from the archive format, or if the file system did not record one for this file.
         /// </summary>
         public string? SecurityDescriptorSddl { get; set; }
     }
