runtime: System.IO.Packaging.Package should have a way to handle common errors with malformed packages

@karelz How can I get this proposal approved? I’m willing to implement it - many users of the Open XML SDK would appreciate this

Regarding Uri class:

• It is fairly complicated code. It is sensitive code and it was source of quite a few security issues since Uri class was born. As a result, our goal is to drive it towards maintainability (using more of safe code and simpler code).
• There were dozens of bug fixes (usually required by many customers) over the years in .NET Framework and in .NET Core to have stricter Uri parsing, compliant with RFC instead of lax parsing (as suggested here).
  • Note: Some of them broke existing customers, for which we usually had opt-out switches. We had 2 customers approaching us in last couple of years, asking us to bring some of the opt-out switches back into .NET Core (they both had large databases with Uri which they apparently couldn’t easily migrate to be compliant - similar to this case). After analysis, we concluded it would make the code frozen and we couldn’t innovate or improve perf in the code base, therefore we rejected the idea.

In summary, I would strongly urge against making Uri more lax in parsing (pre-.NET 4.5 times), or complicate the code significantly. On the other hand, there is slowly growing demand for non-compliant Uri representation and I wonder if we could achieve that via other ways – either the suggested IUriFactory, or support custom parsing, and/or custom error handling / fallback parsing in the Uri class itself as an advanced extension mechanism. Higher-level libraries (e.g. System.IO.Packaging) would have to then pass through the extension mechanism to allow such customization by the callers (OpenXML in this case). @MihaZupan do you have any thoughts on this?

Thanks @ericstj. I’ve added a tool into the OpenXml library to handle sanitizing it (https://github.com/OfficeDev/Open-XML-SDK/pull/793). Downside is that this requires rewriting the document, which may have only been read only before. However, it is interesting that the format itself appears to handle malformed Uris just fine and don’t have the restrictions that System.Uri has. ie Word/Excel/etc happily generate files with these values.

The roundtripping here is so that the values can be rewritten so the document can be read without modifying anything. OriginalString can’t be overwritten to output the original, malformed Uri, so a derived type could be used to ensure the roundtripping. This would allow the OpenXml library to transparently handle the change so people don’t have to modify (potentially old) LOB apps to handle this case.

Having a way to parse a Uri with a relaxed mode similar to pre-.NET 4.5 would be ideal, so hopefully @dotnet/ncl can give some feedback for that.

@JeremyKuhne I’ve taken another look at this since OpenXml users have been hitting it. After playing around with usage, I’ve updated my proposal to the following new APIs for System.IO.Packaging. I’m happy to take this throw the API proposal process, but haven’t done that before, so any guidance would be helpful.

namespace System.IO.Packaging
{
    public abstract partial class Package
    {
        protected Package(PackageSettings settings) { }
        public static System.IO.Packaging.Package Open(System.IO.Stream stream, PackageSettings settings) { throw null; }
        public static System.IO.Packaging.Package Open(string path, PackageSettings settings) { throw null; }
    }

    public partial class PackageSettings
    {
        public FileMode PackageMode { get { throw null; } set { throw null; } }
        public FileAccess PackageAccess { get { throw null; } set { throw null; } }
        public FileShare PackageShare { get { throw null; } set { throw null; } }
        public IUriFactory UriFactory { get { throw null; } set { throw null; } }
    }

    public interface IUriFactory
    {
        System.Uri CreateUri(string url, System.UriKind kind);
        string GetOriginalString(System.Uri uri);
    }

    public class DefaultUriFactory : IUriFactory
    {
        public static IUriFactory Instance => throw null;

        protected DefaultUriFactory() { }

        public virtual Uri CreateUri(string url, UriKind kind) => throw null;

        public virtual string GetOriginalString(Uri uri) => throw null;
    }
}

The PackageSettings class is to minimize the number of overloads of Open(...) that need to be added and allow for simpler additions in the future.

This would allow someone to intercept the creation and writing of the Uri instances so that they can do whatever they want with it. For example, here are some test cases using it:

[Fact]
public void MalformedHyperlinkThrows()
{
    using (var package = Package.Open("malformed_hyperlink.xlsx"))
    {
        var part = package.GetPart(new Uri("/xl/worksheets/sheet1.xml", UriKind.Relative));
        Assert.Throws<UriFormatException>(() => part.GetRelationships());
    }
}

[Fact]
public void MalformedHyperlinkHandle()
{
    const string ExpectedUri = "mailto:mailto@one@";
    var settings = new PackageSettings
    {
        UriFactory = new MalformedUriFactory()
    };

    using (var package = Package.Open("malformed_hyperlink.xlsx", settings))
    {
        var part = package.GetPart(new Uri("/xl/worksheets/sheet1.xml", UriKind.Relative));
        var relationship = Assert.Single(part.GetRelationships());

        var malformed = Assert.IsType<MalformedUri>(relationship.TargetUri);
        Assert.Equal(ExpectedUri, malformed.Uri);
    }
}

[Fact]
public void MalformedHyperlinkRoundtrip()
{
    const string InvalidUri = "mailto:mailto@one@";

    var settings = new PackageSettings
    {
        UriFactory = new MalformedUriFactory(),
        PackageMode = FileMode.OpenOrCreate,
        PackageAccess = FileAccess.ReadWrite
    };

    using (var stream = new MemoryStream())
    {
        using (var package = Package.Open(stream, settings))
        {
            Assert.Empty(package.GetRelationships());
            package.CreateRelationship(new MalformedUri(InvalidUri), TargetMode.External, "relationship");
        }

        using (var package = Package.Open(stream, settings))
        {
            var relationship = Assert.Single(package.GetRelationships());
            var malformed = Assert.IsType<MalformedUri>(relationship.TargetUri);
            Assert.Equal(InvalidUri, malformed.Uri);
        }
    }
}

private class MalformedUriFactory : IUriFactory
{
    public Uri CreateUri(string url, UriKind kind)
    {
        if (Uri.TryCreate(url, kind, out var result))
        {
            return result;
        }

        return new MalformedUri(url);
    }

    public string GetOriginalString(Uri uri)
    {
        if (uri is MalformedUri malformed)
        {
            return malformed.Uri;
        }

        return uri.OriginalString;
    }


internal class MalformedUri : Uri
{
    public MalformedUri(string uriString)
        : base("http://unknown")
    {
        Uri = uriString;
    }

    public string Uri { get; }

    public override string ToString() => Uri;
}

@carlossanlop Any thoughts on this API? I haven’t done an API review, but happy to help move something like this along. As more users are moving to .NET Core/.NET 5, this will become a problem for many LOB apps that rely on DocumentFormat.OpenXml.