From 4f6735955d104896d0bdd4eb7fd8b0f7fa51657b Mon Sep 17 00:00:00 2001 From: Taylor Southwick Date: Mon, 8 Jan 2024 18:35:12 -0800 Subject: [PATCH] Check for seekable stream for malformed uri handling (#1644) - 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 --- CHANGELOG.md | 8 +- .../Features/PackageFeatureBase.cs | 11 +- .../Features/StreamPackageFeature.cs | 9 +- .../Packaging/WriteableStreamExtensions.cs | 2 +- .../Resources/ExceptionMessages.Designer.cs | 18 +++ .../Resources/ExceptionMessages.resx | 6 + ...umentFormat.OpenXml.Framework.Tests.csproj | 1 + .../IFile.cs | 4 + .../TemporaryFile.cs | 4 + .../TestAssets.cs | 21 ++-- .../OpenXmlDomTest/DocumentOpenTests.cs | 113 ++++++++++++++++++ 11 files changed, 183 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2786a9ad..107acaef8 100644 --- 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 index b3c5edec6..6c78b402b 100644 --- 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 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 index 92d48897e..c1316c54e 100644 --- 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 index 94bfc092c..577a74b5d 100644 --- 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() is { Stream.CanWrite: false } feature && + if (features.Get() is { Stream: { CanWrite: false, CanSeek: true } } feature && features.Get() 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 index 6eedab315..ddf7d06c4 100644 --- a/src/DocumentFormat.OpenXml.Framework/Resources/ExceptionMessages.Designer.cs +++ b/src/DocumentFormat.OpenXml.Framework/Resources/ExceptionMessages.Designer.cs @@ -294,6 +294,15 @@ internal static string ExternalRelationshipIsNotReferenced { } } + /// + /// Looks up a localized string similar to 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.. + /// + internal static string FailedToOpenPackage { + get { + return ResourceManager.GetString("FailedToOpenPackage", resourceCulture); + } + } + /// /// Looks up a localized string similar to Feature {0} is not available in this collection.. /// @@ -504,6 +513,15 @@ internal static string MainPartIsDifferent { } } + /// + /// Looks up a localized string similar to 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.. + /// + internal static string MalformedUri { + get { + return ResourceManager.GetString("MalformedUri", resourceCulture); + } + } + /// /// Looks up a localized string similar to An Audio / Video part shall not have implicit or explicit relationships to other parts defined by Open XML Standard.. /// diff --git a/src/DocumentFormat.OpenXml.Framework/Resources/ExceptionMessages.resx b/src/DocumentFormat.OpenXml.Framework/Resources/ExceptionMessages.resx index 3fa9aae68..51133b8dc 100644 --- a/src/DocumentFormat.OpenXml.Framework/Resources/ExceptionMessages.resx +++ b/src/DocumentFormat.OpenXml.Framework/Resources/ExceptionMessages.resx @@ -408,4 +408,10 @@ Namespace ids are only available for namespaces for Office 2021 and before. Please use prefixes or URLs instead. + + 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. + + + 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. + \ No newline at end of file diff --git a/test/DocumentFormat.OpenXml.Framework.Tests/DocumentFormat.OpenXml.Framework.Tests.csproj b/test/DocumentFormat.OpenXml.Framework.Tests/DocumentFormat.OpenXml.Framework.Tests.csproj index 4e6e2c276..dd185f6ac 100644 --- 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 @@ + diff --git a/test/DocumentFormat.OpenXml.Tests.Assets/IFile.cs b/test/DocumentFormat.OpenXml.Tests.Assets/IFile.cs index 2ab208580..89e2d8072 100644 --- 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 index 6a47fca6a..b713841a5 100644 --- 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 index 9aa6b0e99..fe24eddd7 100644 --- 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 index 2d761fe2c..6f15afbf9 100644 --- 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(() => PresentationDocument.Open(NonExistantFile, true, new OpenSettings())); Assert.Throws(() => 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(worksheetPart.RootElement); + var hyperlink = Assert.Single(worksheet.Descendants()); + + // 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(worksheetPart.RootElement); + var hyperlink = Assert.Single(worksheet.Descendants()); + + // 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(() => + { + 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(exception.InnerException); +#else + Assert.IsType(exception.InnerException); +#endif + } + + [Fact] + public void NonSeekableRewriteMalformedUriCompatMode() + { + // Arrange + using var stream = GetStream(TestFiles.Malformed_uri_xlsx); + + // Act/Assert + var exception = Assert.Throws(() => SpreadsheetDocument.Open(new NonSeekableStream(stream), isEditable: false, new OpenSettings { CompatibilityLevel = CompatibilityLevel.Version_2_20 })); + +#if NETFRAMEWORK + Assert.IsType(exception.InnerException); +#else + Assert.IsType(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(); + } } }