Check for seekable stream for malformed uri handling (#1644)

twsouthwick · web-flow · commit 4f6735955d10 · 2024-01-08T18:35:12.000-08:00
- We must be able to seek to create a temporary file requried for uri handling - We should throw the same exception we did in v2.x - If compat mode is on, this will throw at open time (i.e. it will eagerly load everything) Fixes #1636
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,17 +1,21 @@
 # Changelog
+
 All notable changes to this project will be documented in this file.
 
 The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).
 
-## [3.1.0]
+## [3.0.1]
 
 ### Fixed
+
 - Fixed issue where document type would not be correct unless content type was checked first (#1625)
+- Added check to only seek on packages where it is supported (#1644)
+- If a malformed URI is encountered, the exception is now the same as v2.x (`OpenXmlPackageException` with an inner `UriFormatException`) (#1644)
 
 ## [3.0.0] - 2023-11-15
 
-## Added
+### Added
 
 - `DocumentFormat.OpenXml.Office.PowerPoint.Y2023.M02.Main` namespace
 - `DocumentFormat.OpenXml.Office.PowerPoint.Y2022.M03.Main` namespace
diff --git a/src/DocumentFormat.OpenXml.Framework/Features/PackageFeatureBase.cs b/src/DocumentFormat.OpenXml.Framework/Features/PackageFeatureBase.cs
@@ -243,7 +243,16 @@ protected override void Delete(string id)
             => Feature.Package.GetPart(Uri).DeleteRelationship(id);
 
         protected override IEnumerable<PackageRelationship> GetRelationships()
-            => Feature.Package.GetPart(Uri).GetRelationships();
+        {
+            try
+            {
+                return Feature.Package.GetPart(Uri).GetRelationships();
+            }
+            catch (UriFormatException ex)
+            {
+                throw new OpenXmlPackageException(ExceptionMessages.MalformedUri, ex);
+            }
+        }
     }
 
     private abstract class RelationshipCollection : IRelationshipCollection
diff --git a/src/DocumentFormat.OpenXml.Framework/Features/StreamPackageFeature.cs b/src/DocumentFormat.OpenXml.Framework/Features/StreamPackageFeature.cs
@@ -111,7 +111,14 @@ private void InitializePackage(FileMode? mode = default, FileAccess? access = de
             SetStream(GetStream(mode.Value, access.Value));
         }
 
-        _package = Package.Open(_stream, mode.Value, access.Value);
+        try
+        {
+            _package = Package.Open(_stream, mode.Value, access.Value);
+        }
+        catch (ArgumentException ex)
+        {
+            throw new OpenXmlPackageException(ExceptionMessages.FailedToOpenPackage, ex);
+        }
     }
 
     protected override Package Package => _package;
diff --git a/src/DocumentFormat.OpenXml.Framework/Packaging/WriteableStreamExtensions.cs b/src/DocumentFormat.OpenXml.Framework/Packaging/WriteableStreamExtensions.cs
@@ -15,7 +15,7 @@ internal static class WriteableStreamExtensions
     [System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Disposable is registered with package")]
     public static bool EnableWriteableStream(this IFeatureCollection features)
     {
-        if (features.Get<IPackageStreamFeature>() is { Stream.CanWrite: false } feature &&
+        if (features.Get<IPackageStreamFeature>() is { Stream: { CanWrite: false, CanSeek: true } } feature &&
             features.Get<IPackageFeature>() is { } packageFeature &&
             packageFeature.Capabilities.HasFlagFast(PackageCapabilities.Reload))
         {
diff --git a/src/DocumentFormat.OpenXml.Framework/Resources/ExceptionMessages.Designer.cs b/src/DocumentFormat.OpenXml.Framework/Resources/ExceptionMessages.Designer.cs
diff --git a/src/DocumentFormat.OpenXml.Framework/Resources/ExceptionMessages.resx b/src/DocumentFormat.OpenXml.Framework/Resources/ExceptionMessages.resx
@@ -408,4 +408,10 @@
   <data name="NamespaceIdNotAvailable" xml:space="preserve">
     <value>Namespace ids are only available for namespaces for Office 2021 and before. Please use prefixes or URLs instead.</value>
   </data>
+  <data name="MalformedUri" xml:space="preserve">
+    <value>The package contains malformed URI relationships. Please ensure the package is opened with a stream (that is seekable) or a file path to have the SDK automatically handle these scenarios.</value>
+  </data>
+  <data name="FailedToOpenPackage" xml:space="preserve">
+    <value>Package could not be opened for stream. See inner exception for details and be aware that there are behavior differences in stream support between .NET Framework and Core.</value>
+  </data>
 </root>
diff --git a/test/DocumentFormat.OpenXml.Framework.Tests/DocumentFormat.OpenXml.Framework.Tests.csproj b/test/DocumentFormat.OpenXml.Framework.Tests/DocumentFormat.OpenXml.Framework.Tests.csproj
@@ -21,6 +21,7 @@
   <ItemGroup>
     <ProjectReference Include="..\..\src\DocumentFormat.OpenXml.Framework\DocumentFormat.OpenXml.Framework.csproj" />
     <ProjectReference Include="..\..\src\DocumentFormat.OpenXml\DocumentFormat.OpenXml.csproj" />
+    <ProjectReference Include="..\DocumentFormat.OpenXml.Tests.Assets\DocumentFormat.OpenXml.Tests.Assets.csproj" />
   </ItemGroup>
 
 </Project>
diff --git a/test/DocumentFormat.OpenXml.Tests.Assets/IFile.cs b/test/DocumentFormat.OpenXml.Tests.Assets/IFile.cs
@@ -11,5 +11,9 @@ public interface IFile : IDisposable
         string Path { get; }
 
         Stream Open();
+
+        bool IsEditable { get; }
+
+        FileAccess Access { get; }
     }
 }
diff --git a/test/DocumentFormat.OpenXml.Tests.Assets/TemporaryFile.cs b/test/DocumentFormat.OpenXml.Tests.Assets/TemporaryFile.cs
@@ -16,6 +16,10 @@ private TemporaryFile(string path)
 
         public string Path { get; }
 
+        public bool IsEditable => true;
+
+        public FileAccess Access => FileAccess.ReadWrite;
+
         public Stream Open() => File.Open(Path, FileMode.OpenOrCreate, FileAccess.ReadWrite);
 
         public void Dispose()
diff --git a/test/DocumentFormat.OpenXml.Tests.Assets/TestAssets.cs b/test/DocumentFormat.OpenXml.Tests.Assets/TestAssets.cs
@@ -98,6 +98,10 @@ public MemoryFile(Stream stream, string path)
 
             public string Path { get; }
 
+            public bool IsEditable => _stream.CanWrite;
+
+            public FileAccess Access => _stream.CanWrite ? FileAccess.ReadWrite : FileAccess.Read;
+
             public Stream Open() => _stream;
 
             public void Dispose()
@@ -108,23 +112,22 @@ public void Dispose()
 
         private class CopiedFile : IFile
         {
-            private readonly FileAccess _access;
-
             public CopiedFile(Stream stream, string extension, FileAccess access)
             {
                 Path = System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"{Guid.NewGuid()}{extension}");
+                Access = access;
 
-                _access = access;
-
-                using (var fs = File.OpenWrite(Path))
-                {
-                    stream.CopyTo(fs);
-                }
+                using var fs = File.OpenWrite(Path);
+                stream.CopyTo(fs);
             }
 
             public string Path { get; }
 
-            public Stream Open() => File.Open(Path, FileMode.Open, _access);
+            public bool IsEditable => Access == FileAccess.ReadWrite;
+
+            public FileAccess Access { get; }
+
+            public Stream Open() => File.Open(Path, FileMode.Open, Access);
 
             public void Dispose()
             {
diff --git a/test/DocumentFormat.OpenXml.Tests/OpenXmlDomTest/DocumentOpenTests.cs b/test/DocumentFormat.OpenXml.Tests/OpenXmlDomTest/DocumentOpenTests.cs
@@ -1,14 +1,18 @@
 ﻿// Copyright (c) Microsoft. All rights reserved.
 // Licensed under the MIT license. See LICENSE file in the project root for full license information.
 
+using DocumentFormat.OpenXml.Framework;
 using DocumentFormat.OpenXml.Packaging;
 using DocumentFormat.OpenXml.Spreadsheet;
 using DocumentFormat.OpenXml.Validation;
 using System;
 using System.Collections.Generic;
 using System.IO;
+using System.Linq;
 using Xunit;
 
+using static DocumentFormat.OpenXml.Tests.TestAssets;
+
 namespace DocumentFormat.OpenXml.Tests
 {
     public class DocumentOpenTests
@@ -35,5 +39,114 @@ public void ThrowIfFileCannotBeFound()
             Assert.Throws<FileNotFoundException>(() => PresentationDocument.Open(NonExistantFile, true, new OpenSettings()));
             Assert.Throws<FileNotFoundException>(() => PresentationDocument.Open(NonExistantFile, false, new OpenSettings()));
         }
+
+        [Theory]
+        [InlineData(FileAccess.Read)]
+        [InlineData(FileAccess.ReadWrite)]
+        public void RewriteMalformedUriLong(FileAccess access)
+        {
+            // Arrange
+            const string ExpectedMalformedUri = "mailto:test@test.com;%20test2@test.com;%252test3@test.com;%20test3@test.com;%20test4@test.com;%20test5@test.com?subject=Unsubscribe%20Request&body=Please%20unsubscribe%20me%20from%20all%20future%20communications";
+            const string Id = "rId1";
+
+            using var file = OpenFile(TestFiles.Malformed_uri_long_xlsx, access);
+            using var stream = file.Open();
+            using var doc = SpreadsheetDocument.Open(stream, isEditable: file.IsEditable);
+
+            // Act
+            var worksheetPart = doc.WorkbookPart.WorksheetParts.Single();
+            var hyperlinkRelationship = worksheetPart.HyperlinkRelationships.Single();
+            var worksheet = Assert.IsType<Worksheet>(worksheetPart.RootElement);
+            var hyperlink = Assert.Single(worksheet.Descendants<Hyperlink>());
+
+            // Assert
+            Assert.Equal(Id, hyperlinkRelationship.Id);
+            Assert.Equal(Id, hyperlink.Id);
+            Assert.Equal(ExpectedMalformedUri, hyperlinkRelationship.Uri.ToString());
+        }
+
+        [Theory]
+        [InlineData(FileAccess.Read)]
+        [InlineData(FileAccess.ReadWrite)]
+        public void RewriteMalformedUri(FileAccess access)
+        {
+            // Arrange
+            const string Id = "rId1";
+
+            using var file = OpenFile(TestFiles.Malformed_uri_xlsx, access);
+            using var stream = file.Open();
+            using var doc = SpreadsheetDocument.Open(stream, isEditable: file.IsEditable);
+
+            // Act
+            var worksheetPart = doc.WorkbookPart.WorksheetParts.Single();
+            var hyperlinkRelationship = worksheetPart.HyperlinkRelationships.Single();
+            var worksheet = Assert.IsType<Worksheet>(worksheetPart.RootElement);
+            var hyperlink = Assert.Single(worksheet.Descendants<Hyperlink>());
+
+            // Assert
+            Assert.Equal(Id, hyperlink.Id);
+            Assert.Equal(Id, hyperlinkRelationship.Id);
+            Assert.Equal("mailto:one@", hyperlinkRelationship.Uri.ToString());
+        }
+
+        [Fact]
+        public void NonSeekableRewriteMalformedUri()
+        {
+            // Arrange
+            using var stream = GetStream(TestFiles.Malformed_uri_xlsx);
+
+            var exception = Assert.Throws<OpenXmlPackageException>(() =>
+            {
+                using var doc = SpreadsheetDocument.Open(new NonSeekableStream(stream), isEditable: false);
+
+                // Act/Assert
+                var worksheetPart = doc.WorkbookPart.WorksheetParts.Single();
+                var link = worksheetPart.HyperlinkRelationships.Single();
+            });
+
+#if NETFRAMEWORK
+            Assert.IsType<ArgumentException>(exception.InnerException);
+#else
+            Assert.IsType<UriFormatException>(exception.InnerException);
+#endif
+        }
+
+        [Fact]
+        public void NonSeekableRewriteMalformedUriCompatMode()
+        {
+            // Arrange
+            using var stream = GetStream(TestFiles.Malformed_uri_xlsx);
+
+            // Act/Assert
+            var exception = Assert.Throws<OpenXmlPackageException>(() => SpreadsheetDocument.Open(new NonSeekableStream(stream), isEditable: false, new OpenSettings { CompatibilityLevel = CompatibilityLevel.Version_2_20 }));
+
+#if NETFRAMEWORK
+            Assert.IsType<ArgumentException>(exception.InnerException);
+#else
+            Assert.IsType<UriFormatException>(exception.InnerException);
+#endif
+        }
+
+        private sealed class NonSeekableStream : DelegatingStream
+        {
+            public NonSeekableStream(Stream innerStream)
+                : base(innerStream)
+            {
+            }
+
+            public override bool CanSeek => false;
+
+            public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException();
+
+            public override long Position
+            {
+                get => throw new NotSupportedException();
+                set => throw new NotSupportedException();
+            }
+
+            public override long Length => throw new NotSupportedException();
+
+            public override void SetLength(long value) => throw new NotSupportedException();
+        }
     }
 }

Original file line number	Diff line number	Diff line change
`@@ -111,7 +111,14 @@ private void InitializePackage(FileMode? mode = default, FileAccess? access = de`
`111`	`111`	`SetStream(GetStream(mode.Value, access.Value));`
`112`	`112`	`}`
`113`	`113`
`114`		`- _package = Package.Open(_stream, mode.Value, access.Value);`
	`114`	`+ try`
	`115`	`+ {`
	`116`	`+ _package = Package.Open(_stream, mode.Value, access.Value);`
	`117`	`+ }`
	`118`	`+ catch (ArgumentException ex)`
	`119`	`+ {`
	`120`	`+ throw new OpenXmlPackageException(ExceptionMessages.FailedToOpenPackage, ex);`
	`121`	`+ }`
`115`	`122`	`}`
`116`	`123`
`117`	`124`	`protected override Package Package => _package;`
Original file line number	Diff line number	Diff line change
`@@ -15,7 +15,7 @@ internal static class WriteableStreamExtensions`
`15`	`15`	`[System.Diagnostics.CodeAnalysis.SuppressMessage("Reliability", "CA2000:Dispose objects before losing scope", Justification = "Disposable is registered with package")]`
`16`	`16`	`public static bool EnableWriteableStream(this IFeatureCollection features)`
`17`	`17`	`{`
`18`		`- if (features.Get<IPackageStreamFeature>() is { Stream.CanWrite: false } feature &&`
	`18`	`+ if (features.Get<IPackageStreamFeature>() is { Stream: { CanWrite: false, CanSeek: true } } feature &&`
`19`	`19`	`features.Get<IPackageFeature>() is { } packageFeature &&`
`20`	`20`	`packageFeature.Capabilities.HasFlagFast(PackageCapabilities.Reload))`
`21`	`21`	`{`
Original file line number	Diff line number	Diff line change
`@@ -11,5 +11,9 @@ public interface IFile : IDisposable`
`11`	`11`	`string Path { get; }`
`12`	`12`
`13`	`13`	`Stream Open();`
	`14`	`+`
	`15`	`+ bool IsEditable { get; }`
	`16`	`+`
	`17`	`+ FileAccess Access { get; }`
`14`	`18`	`}`
`15`	`19`	`}`