From c853ac4d7ce1fdee37a261122f531e130a26d52d Mon Sep 17 00:00:00 2001 From: Will Portnoy Date: Tue, 27 Oct 2026 14:20:39 -0700 Subject: [PATCH 1/5] Add mocked regressions for network schema loading Cover opening and validation with an in-memory schema source. Four network-schema assertions intentionally fail before the product fix; local-schema and no-schema controls pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38cfa7db-d690-4e7f-a2ff-a38a5e8015b5 --- src/Model.Tests/Model.Tests.csproj | 17 +++ src/Model.Tests/SchemaLocationTests.cs | 158 +++++++++++++++++++++++++ src/Model/Model.csproj | 1 + src/Model/SchemaCache.cs | 15 ++- src/Model/proxy.cs | 12 +- src/xmlnotepad.sln | 26 ++++ 6 files changed, 225 insertions(+), 4 deletions(-) create mode 100644 src/Model.Tests/Model.Tests.csproj create mode 100644 src/Model.Tests/SchemaLocationTests.cs diff --git a/src/Model.Tests/Model.Tests.csproj b/src/Model.Tests/Model.Tests.csproj new file mode 100644 index 00000000..5092158c --- /dev/null +++ b/src/Model.Tests/Model.Tests.csproj @@ -0,0 +1,17 @@ + + + + net8.0 + enable + false + + + + + + + + + + + diff --git a/src/Model.Tests/SchemaLocationTests.cs b/src/Model.Tests/SchemaLocationTests.cs new file mode 100644 index 00000000..9e9f624b --- /dev/null +++ b/src/Model.Tests/SchemaLocationTests.cs @@ -0,0 +1,158 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using System.Net; +using System.Text; +using System.Xml; +using XmlNotepad; + +// Settings.Instance and the delayed-action dispatcher are process-global. +[assembly: DoNotParallelize] + +namespace XmlNotepad.Model.Tests +{ + [TestClass] + public sealed class SchemaLocationTests : IServiceProvider + { + private const string NetworkSchemaLocation = @"\\mock-schema.invalid\schemas\schema.xsd"; + private MockSchemaSource _source = null!; + private Settings _settings = null!; + private XmlCache _model = null!; + private string _documentFile = null!; + + [TestInitialize] + public void Initialize() + { + _source = new MockSchemaSource(); + _settings = new Settings + { + StartupPath = AppContext.BaseDirectory, + Resolver = _source + }; + _settings.SetDefaults(); + var actions = new DelayedActions(action => action()); + _settings.DelayedActions = actions; + var cache = new SchemaCache(this, _source); + _model = new XmlCache(this, cache, actions); + _documentFile = Path.Combine(AppContext.BaseDirectory, Guid.NewGuid().ToString("N") + ".xml"); + } + + [TestCleanup] + public void Cleanup() + { + _model?.Dispose(); + _settings?.Dispose(); + } + + public object? GetService(Type serviceType) + { + return serviceType == typeof(Settings) ? _settings : null; + } + + [TestMethod] + [DataRow("noNamespaceSchemaLocation")] + [DataRow("schemaLocation")] + public void OpeningDocumentDoesNotRequestNetworkSchema(string schemaAttribute) + { + LoadXml(CreateXml(schemaAttribute, NetworkSchemaLocation)); + + Assert.IsEmpty(_source.Requests, + "Opening XML must not implicitly request a schema from a network share. " + + "Mock source requests: " + string.Join(", ", _source.Requests)); + } + + [TestMethod] + [DataRow("noNamespaceSchemaLocation")] + [DataRow("schemaLocation")] + public void ValidatingDocumentDoesNotRequestNetworkSchema(string schemaAttribute) + { + var document = new XmlDocument { XmlResolver = _source }; + document.LoadXml(CreateXml(schemaAttribute, NetworkSchemaLocation)); + _model.Load(document); + + _model.ValidateModel(new ValidationErrorHandler()); + + Assert.IsEmpty(_source.Requests, + "Validation must not implicitly request a schema from a network share. " + + "Mock source requests: " + string.Join(", ", _source.Requests)); + } + + [TestMethod] + public void OpeningDocumentLoadsLocalSchemaFromMockSource() + { + LoadXml(CreateXml("noNamespaceSchemaLocation", "schema.xsd")); + + Assert.HasCount(1, _source.Requests); + Assert.AreEqual(new Uri(new Uri(_documentFile), "schema.xsd"), _source.Requests[0]); + Assert.AreEqual(1, _model.Document.Schemas.Count); + } + + [TestMethod] + public void OpeningDocumentWithoutSchemaLocationDoesNotRequestSchema() + { + LoadXml(""); + + Assert.IsEmpty(_source.Requests); + } + + private void LoadXml(string xml) + { + using (var text = new StringReader(xml)) + using (var reader = XmlReader.Create(text, _model.GetReaderSettings(), new Uri(_documentFile).AbsoluteUri)) + { + _model.Load(reader, _documentFile); + } + Assert.AreEqual("root", _model.Document.DocumentElement?.LocalName); + } + + private static string CreateXml(string schemaAttribute, string schemaLocation) + { + string namespaceAttribute = ""; + if (schemaAttribute == "schemaLocation") + { + namespaceAttribute = " xmlns=\"urn:mock-schema\""; + schemaLocation = "urn:mock-schema " + schemaLocation; + } + return ""; + } + + private sealed class MockSchemaSource : XmlResolver + { + public List Requests { get; } = new List(); + + public override ICredentials? Credentials + { + set => throw new NotSupportedException("The mock source never uses credentials."); + } + + public override object GetEntity(Uri absoluteUri, string? role, Type? ofObjectToReturn) + { + Requests.Add(absoluteUri); + if (absoluteUri.IsUnc || !absoluteUri.IsFile) + { + throw new IOException("The mock source recorded the network request without performing I/O."); + } + if (ofObjectToReturn != null && ofObjectToReturn != typeof(Stream)) + { + throw new NotSupportedException("The mock source only provides streams."); + } + + const string schema = "" + + ""; + return new MemoryStream(Encoding.UTF8.GetBytes(schema)); + } + } + + private sealed class ValidationErrorHandler : ErrorHandler + { + public override void HandleError(Severity sev, string reason, string filename, int line, int col, object data) + { + Trace.WriteLine(sev + ": " + reason); + } + } + } +} diff --git a/src/Model/Model.csproj b/src/Model/Model.csproj index b7edffa5..2a8f5a61 100644 --- a/src/Model/Model.csproj +++ b/src/Model/Model.csproj @@ -22,6 +22,7 @@ + True True diff --git a/src/Model/SchemaCache.cs b/src/Model/SchemaCache.cs index 57afa517..3a1bd50d 100644 --- a/src/Model/SchemaCache.cs +++ b/src/Model/SchemaCache.cs @@ -172,11 +172,15 @@ public class SchemaCache : IXmlSerializable private XmlResolver resolver; private List globalElementCache; - public SchemaCache(IServiceProvider site) + public SchemaCache(IServiceProvider site) : this(site, null) + { + } + + public SchemaCache(IServiceProvider site, XmlResolver sourceResolver) { this.site = site; this.pfn = new PersistentFileNames(Settings.Instance.StartupPath); - this.resolver = new SchemaResolver(this.site, this); + this.resolver = new SchemaResolver(this.site, this, sourceResolver); } void FireOnChanged() @@ -820,7 +824,12 @@ public class SchemaResolver : XmlProxyResolver SchemaCache cache; ValidationEventHandler handler; - public SchemaResolver(IServiceProvider site, SchemaCache cache) : base(site) + public SchemaResolver(IServiceProvider site, SchemaCache cache) : this(site, cache, null) + { + } + + internal SchemaResolver(IServiceProvider site, SchemaCache cache, XmlResolver sourceResolver) + : base(site, sourceResolver) { this.cache = cache; } diff --git a/src/Model/proxy.cs b/src/Model/proxy.cs index 695e80ff..7d1acdf2 100644 --- a/src/Model/proxy.cs +++ b/src/Model/proxy.cs @@ -16,10 +16,16 @@ namespace XmlNotepad public class XmlProxyResolver : XmlUrlResolver { private readonly WebProxyService _ps; + private readonly XmlResolver _sourceResolver; - public XmlProxyResolver(IServiceProvider site) + public XmlProxyResolver(IServiceProvider site) : this(site, null) + { + } + + protected XmlProxyResolver(IServiceProvider site, XmlResolver sourceResolver) { _ps = site.GetService(typeof(WebProxyService)) as WebProxyService; + _sourceResolver = sourceResolver; Proxy = HttpWebRequest.DefaultWebProxy; } @@ -29,6 +35,10 @@ public override object GetEntity(Uri absoluteUri, string role, Type ofObjectToRe { throw new ArgumentNullException("absoluteUri"); } + if (_sourceResolver != null) + { + return _sourceResolver.GetEntity(absoluteUri, role, ofObjectToReturn); + } if ((absoluteUri.Scheme == "http" || absoluteUri.Scheme == "https") && (ofObjectToReturn == null || ofObjectToReturn == typeof(Stream))) { diff --git a/src/xmlnotepad.sln b/src/xmlnotepad.sln index 51e1c929..324bfb92 100644 --- a/src/xmlnotepad.sln +++ b/src/xmlnotepad.sln @@ -41,6 +41,8 @@ Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "WindowsInput", "WindowsInpu EndProject Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "UpdateVersions", "UpdateVersions\UpdateVersions.csproj", "{DEEBB5D0-2B54-4A9D-9DC3-CD3060E7A533}" EndProject +Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "Model.Tests", "Model.Tests\Model.Tests.csproj", "{62171386-7CA3-4DDE-B81E-440BF8D222FC}" +EndProject Global GlobalSection(SolutionConfigurationPlatforms) = preSolution Debug|Any CPU = Debug|Any CPU @@ -241,6 +243,30 @@ Global {DEEBB5D0-2B54-4A9D-9DC3-CD3060E7A533}.Release|x64.Build.0 = Release|Any CPU {DEEBB5D0-2B54-4A9D-9DC3-CD3060E7A533}.Release|x86.ActiveCfg = Release|Any CPU {DEEBB5D0-2B54-4A9D-9DC3-CD3060E7A533}.Release|x86.Build.0 = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|Any CPU.ActiveCfg = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|Any CPU.Build.0 = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|ARM.ActiveCfg = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|ARM.Build.0 = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|ARM64.ActiveCfg = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|ARM64.Build.0 = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|Mixed Platforms.ActiveCfg = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|Mixed Platforms.Build.0 = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|x64.ActiveCfg = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|x64.Build.0 = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|x86.ActiveCfg = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Debug|x86.Build.0 = Debug|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|Any CPU.ActiveCfg = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|Any CPU.Build.0 = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|ARM.ActiveCfg = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|ARM.Build.0 = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|ARM64.ActiveCfg = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|ARM64.Build.0 = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|Mixed Platforms.ActiveCfg = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|Mixed Platforms.Build.0 = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|x64.ActiveCfg = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|x64.Build.0 = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|x86.ActiveCfg = Release|Any CPU + {62171386-7CA3-4DDE-B81E-440BF8D222FC}.Release|x86.Build.0 = Release|Any CPU EndGlobalSection GlobalSection(SolutionProperties) = preSolution HideSolutionNode = FALSE From cd7aa08813ea000bb04fa196851612b1b4359eea Mon Sep 17 00:00:00 2001 From: Will Portnoy Date: Fri, 30 Oct 2026 10:12:32 -0700 Subject: [PATCH 2/5] Block network-file access in external XML resource resolution Reject UNC aliases, mapped network drives, and unknown drive roots before synchronous or asynchronous resource I/O. Cover schemas, external DTDs/entities, XInclude, XSLT imports and document(), and cached-file metadata checks. Preserve local and HTTP(S) sources and explicitly selected input files. Remove automatic Windows target credentials and expand mocked regressions across .NET Framework 4.8 and .NET 8. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38cfa7db-d690-4e7f-a2ff-a38a5e8015b5 --- src/Model.Tests/ExternalResourceTests.cs | 467 +++++++++++++++++++++++ src/Model.Tests/Model.Tests.csproj | 3 +- src/Model.Tests/SchemaLocationTests.cs | 158 -------- src/Model/SchemaCache.cs | 16 +- src/Model/Settings.cs | 4 + src/Model/Strings.Designer.cs | 9 + src/Model/Strings.resx | 4 + src/Model/XmlCache.cs | 17 +- src/Model/XmlHelpers.cs | 13 +- src/Model/XmlResourceResolver.cs | 107 ++++++ src/Model/proxy.cs | 11 + src/XmlNotepad/AsyncXslt.cs | 25 +- src/XmlNotepad/XsltControl.cs | 4 +- 13 files changed, 652 insertions(+), 186 deletions(-) create mode 100644 src/Model.Tests/ExternalResourceTests.cs delete mode 100644 src/Model.Tests/SchemaLocationTests.cs create mode 100644 src/Model/XmlResourceResolver.cs diff --git a/src/Model.Tests/ExternalResourceTests.cs b/src/Model.Tests/ExternalResourceTests.cs new file mode 100644 index 00000000..afafc05c --- /dev/null +++ b/src/Model.Tests/ExternalResourceTests.cs @@ -0,0 +1,467 @@ +using Microsoft.VisualStudio.TestTools.UnitTesting; +using System; +using System.Collections.Generic; +using System.Diagnostics; +using System.IO; +using System.Net; +using System.Text; +using System.Threading.Tasks; +using System.Xml; +using System.Xml.Xsl; +using XmlNotepad; + +// Settings.Instance and the delayed-action dispatcher are process-global. +[assembly: DoNotParallelize] + +namespace XmlNotepad.Model.Tests +{ + [TestClass] + public sealed class ExternalResourceTests : IServiceProvider + { + private const string NetworkSchemaLocation = @"\\mock-schema.invalid\schemas\schema.xsd"; + private const string NetworkFileUri = "file://mock-schema.invalid/schemas/resource.xml"; + private const string LocalSchema = "" + + ""; + private MockSchemaSource _source = null!; + private TestProxyResolver _resolver = null!; + private Settings _settings = null!; + private XmlCache _model = null!; + private string _documentFile = null!; + + [TestInitialize] + public void Initialize() + { + _source = new MockSchemaSource(); + _resolver = new TestProxyResolver(this, _source); + _settings = new Settings + { + StartupPath = AppContext.BaseDirectory, + Resolver = _resolver + }; + _settings.SetDefaults(); + var actions = new DelayedActions(action => action()); + _settings.DelayedActions = actions; + var cache = new SchemaCache(this, _source); + _model = new XmlCache(this, cache, actions); + _documentFile = Path.Combine(AppContext.BaseDirectory, Guid.NewGuid().ToString("N") + ".xml"); + } + + [TestCleanup] + public void Cleanup() + { + _model?.Dispose(); + _settings?.Dispose(); + } + + public object? GetService(Type serviceType) + { + return serviceType == typeof(Settings) ? _settings : null; + } + + [TestMethod] + [DataRow("noNamespaceSchemaLocation", NetworkSchemaLocation)] + [DataRow("schemaLocation", NetworkSchemaLocation)] + [DataRow("noNamespaceSchemaLocation", NetworkFileUri)] + [DataRow("schemaLocation", NetworkFileUri)] + [DataRow("noNamespaceSchemaLocation", "file:///%5C%5Cmock-schema.invalid/schemas/schema.xsd")] + [DataRow("schemaLocation", "file:///%5C%5Cmock-schema.invalid/schemas/schema.xsd")] + public void OpeningDocumentDoesNotRequestNetworkSchema(string schemaAttribute, string schemaLocation) + { + LoadXml(CreateXml(schemaAttribute, schemaLocation)); + + Assert.IsEmpty(_source.Requests, + "Opening XML must not implicitly request a schema from a network share. " + + "Mock source requests: " + string.Join(", ", _source.Requests)); + } + + [TestMethod] + [DataRow("noNamespaceSchemaLocation", NetworkSchemaLocation)] + [DataRow("schemaLocation", NetworkSchemaLocation)] + [DataRow("noNamespaceSchemaLocation", NetworkFileUri)] + [DataRow("schemaLocation", NetworkFileUri)] + public void ValidatingDocumentDoesNotRequestNetworkSchema(string schemaAttribute, string schemaLocation) + { + var document = new XmlDocument { XmlResolver = _resolver }; + document.LoadXml(CreateXml(schemaAttribute, schemaLocation)); + _model.Load(document); + + _model.ValidateModel(new ValidationErrorHandler()); + + Assert.IsEmpty(_source.Requests, + "Validation must not implicitly request a schema from a network share. " + + "Mock source requests: " + string.Join(", ", _source.Requests)); + } + + [TestMethod] + public void OpeningDocumentLoadsLocalSchemaFromMockSource() + { + var schemaUri = new Uri(new Uri(_documentFile), "schema.xsd"); + _source.Documents.Add(schemaUri, LocalSchema); + LoadXml(CreateXml("noNamespaceSchemaLocation", "schema.xsd")); + + Assert.HasCount(1, _source.Requests); + Assert.AreEqual(schemaUri, _source.Requests[0]); + Assert.AreEqual(1, _model.Document.Schemas.Count); + } + + [TestMethod] + [DataRow("http://mock-schema.invalid/schema.xsd")] + [DataRow("https://mock-schema.invalid/schema.xsd")] + public void OpeningDocumentLoadsHttpSchemaFromMockSource(string location) + { + var schemaUri = new Uri(location); + _source.Documents.Add(schemaUri, LocalSchema); + + LoadXml(CreateXml("noNamespaceSchemaLocation", location)); + + Assert.HasCount(1, _source.Requests); + Assert.AreEqual(schemaUri, _source.Requests[0]); + Assert.AreEqual(1, _model.Document.Schemas.Count); + } + + [TestMethod] + [DataRow("include")] + [DataRow("import")] + public void OpeningDocumentDoesNotRequestNetworkSchemaDependency(string directive) + { + var schemaUri = new Uri(new Uri(_documentFile), "schema.xsd"); + string targetNamespace = directive == "import" ? " namespace=\"urn:external\"" : ""; + _source.Documents.Add(schemaUri, + "" + + "" + + ""); + + LoadXml(CreateXml("noNamespaceSchemaLocation", "schema.xsd")); + + Assert.HasCount(1, _source.Requests, + "Only the local parent schema may reach the source. Requests: " + + string.Join(", ", _source.Requests)); + Assert.AreEqual(schemaUri, _source.Requests[0]); + } + + [TestMethod] + public void OpeningDocumentWithoutSchemaLocationDoesNotRequestSchema() + { + LoadXml(""); + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + [DataRow(false)] + [DataRow(true)] + public void ParsingDtdDoesNotRequestNetworkResource(bool externalSubset) + { + _settings["IgnoreDTD"] = false; + string declaration = externalSubset + ? "" + : "]>"; + string xml = declaration + (externalSubset ? "" : "&external;"); + using (var text = new StringReader(xml)) + using (var reader = XmlHelpers.ReadXml(text, _resolver)) + { + var document = new XmlDocument(); + Assert.Throws(() => document.Load(reader)); + } + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + public void ParsingDtdLoadsLocalEntityFromMockSource() + { + _settings["IgnoreDTD"] = false; + var entityUri = new Uri(new Uri(_documentFile), "entity.txt"); + _source.Documents.Add(entityUri, "allowed"); + string xml = "]>&external;"; + using (var text = new StringReader(xml)) + using (var reader = XmlHelpers.ReadXml(text, _resolver)) + { + var document = new XmlDocument(); + document.Load(reader); + Assert.AreEqual("allowed", document.InnerText); + } + + Assert.HasCount(1, _source.Requests); + } + + [TestMethod] + public void ExpandingIncludesDoesNotRequestNetworkResource() + { + LoadXml("" + + ""); + + Assert.Throws(() => _model.ExpandIncludes()); + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + public void ExpandingIncludesLoadsLocalResourceFromMockSource() + { + var includeUri = new Uri(new Uri(_documentFile), "include.xml"); + _source.Documents.Add(includeUri, "allowed"); + LoadXml("" + + ""); + + _model.ExpandIncludes(); + + Assert.AreEqual("allowed", _model.Document.DocumentElement?.InnerText); + Assert.HasCount(1, _source.Requests); + } + + [TestMethod] + [DataRow("include")] + [DataRow("import")] + public void LoadingStylesheetDoesNotRequestNetworkResource(string directive) + { + string stylesheet = "" + + ""; + using (var text = new StringReader(stylesheet)) + using (var reader = XmlReader.Create(text)) + { + var transform = new XslCompiledTransform(); + Assert.Throws(() => transform.Load(reader, XsltSettings.Default, _resolver)); + } + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + public void TransformingDoesNotRequestNetworkDocument() + { + var transform = CreateDocumentTransform(NetworkFileUri); + using (var text = new StringReader("")) + using (var reader = XmlReader.Create(text)) + using (var output = new StringWriter()) + using (var writer = XmlWriter.Create(output, transform.OutputSettings)) + { + Assert.Throws(() => transform.Transform(reader, null, writer, _resolver)); + } + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + public void TransformingLoadsLocalDocumentFromMockSource() + { + var documentUri = new Uri(new Uri(_documentFile), "external.xml"); + _source.Documents.Add(documentUri, "allowed"); + var transform = CreateDocumentTransform(documentUri.AbsoluteUri); + using (var text = new StringReader("")) + using (var reader = XmlReader.Create(text)) + using (var output = new StringWriter()) + { + using (var writer = XmlWriter.Create(output, transform.OutputSettings)) + { + transform.Transform(reader, null, writer, _resolver); + } + StringAssert.Contains(output.ToString(), "allowed"); + } + + Assert.HasCount(1, _source.Requests); + } + + [TestMethod] + public async Task ResolvingNetworkResourceAsyncDoesNotReachSource() + { + await Assert.ThrowsAsync(() => + _resolver.GetEntityAsync(new Uri(NetworkFileUri), null, typeof(Stream))); + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + [DataRow("file:////mock-schema.invalid/schemas/resource.xml")] + [DataRow("file:///%5C%5C%3F%5CUNC%5Cmock-schema.invalid%5Cschemas%5Cresource.xml")] + public void ResolvingNetworkFileAliasDoesNotReachSource(string location) + { + Assert.Throws(() => + _resolver.GetEntity(new Uri(location), null, typeof(Stream))); + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + [DataRow(DriveType.Network)] + [DataRow(DriveType.Unknown)] + [DataRow(DriveType.NoRootDirectory)] + public void ResolvingRestrictedDriveDoesNotReachSource(DriveType driveType) + { + var uri = new Uri(_documentFile); + _source.Documents.Add(uri, ""); + var resolver = new ClassifiedDriveResolver(_source, driveType); + + Assert.Throws(() => resolver.GetEntity(uri, null, typeof(Stream))); + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + public async Task ResolvingMappedDriveAsyncDoesNotReachSource() + { + var uri = new Uri(_documentFile); + _source.Documents.Add(uri, ""); + var resolver = new ClassifiedDriveResolver(_source, DriveType.Network); + + await Assert.ThrowsAsync(() => resolver.GetEntityAsync(uri, null, typeof(Stream))); + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + public async Task ResolvingLocalResourceAsyncUsesMockSource() + { + var uri = new Uri(_documentFile); + _source.Documents.Add(uri, "allowed"); + var resolver = new ClassifiedDriveResolver(_source, DriveType.Fixed); + using (var stream = (Stream)await resolver.GetEntityAsync(uri, null, typeof(Stream))) + using (var reader = new StreamReader(stream)) + { + Assert.AreEqual("allowed", await reader.ReadToEndAsync()); + } + + Assert.HasCount(1, _source.Requests); + } + + [TestMethod] + public void ReaderGuardsCustomResolverBeforeReadingExternalEntity() + { + _settings["IgnoreDTD"] = false; + string xml = "]>&external;"; + using (var text = new StringReader(xml)) + using (var reader = XmlHelpers.ReadXml(text, _source)) + { + var document = new XmlDocument(); + Assert.Throws(() => document.Load(reader)); + } + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + public void LoadingSelectedLocalFilePreservesRelativeEntityBaseUri() + { + _settings["IgnoreDTD"] = false; + string filename = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N") + ".xml"); + var entityUri = new Uri(new Uri(filename), "entity.txt"); + _source.Documents.Add(entityUri, "allowed"); + try + { + File.WriteAllText(filename, + "]>&external;"); + + _model.Load(filename); + + Assert.AreEqual("allowed", _model.Document.DocumentElement?.InnerText); + Assert.HasCount(1, _source.Requests); + Assert.AreEqual(entityUri, _source.Requests[0]); + } + finally + { + _model.Clear(); + File.Delete(filename); + } + } + + private XslCompiledTransform CreateDocumentTransform(string location) + { + string stylesheet = "" + + ""; + var transform = new XslCompiledTransform(); + using (var text = new StringReader(stylesheet)) + using (var reader = XmlReader.Create(text)) + { + transform.Load(reader, new XsltSettings(true, false), _resolver); + } + return transform; + } + + private void LoadXml(string xml) + { + using (var text = new StringReader(xml)) + using (var reader = XmlReader.Create(text, _model.GetReaderSettings(), new Uri(_documentFile).AbsoluteUri)) + { + _model.Load(reader, _documentFile); + } + Assert.AreEqual("root", _model.Document.DocumentElement?.LocalName); + } + + private static string CreateXml(string schemaAttribute, string schemaLocation) + { + string namespaceAttribute = ""; + if (schemaAttribute == "schemaLocation") + { + namespaceAttribute = " xmlns=\"urn:mock-schema\""; + schemaLocation = "urn:mock-schema " + schemaLocation; + } + return ""; + } + + private sealed class MockSchemaSource : XmlResolver + { + public List Requests { get; } = new List(); + public Dictionary Documents { get; } = new Dictionary(); + + public override ICredentials? Credentials + { + set => throw new NotSupportedException("The mock source never uses credentials."); + } + + public override object GetEntity(Uri absoluteUri, string? role, Type? ofObjectToReturn) + { + Requests.Add(absoluteUri); + if (ofObjectToReturn != null && ofObjectToReturn != typeof(Stream)) + { + throw new NotSupportedException("The mock source only provides streams."); + } + + if (!Documents.TryGetValue(absoluteUri, out string? document)) + { + throw new XmlException("The mock source recorded the request without performing I/O."); + } + return new MemoryStream(Encoding.UTF8.GetBytes(document)); + } + + public override Task GetEntityAsync(Uri absoluteUri, string? role, Type? ofObjectToReturn) + { + return Task.FromResult(GetEntity(absoluteUri, role, ofObjectToReturn)); + } + } + + private sealed class TestProxyResolver : XmlProxyResolver + { + public TestProxyResolver(IServiceProvider site, XmlResolver source) : base(site, source) + { + } + } + + private sealed class ClassifiedDriveResolver : XmlResourceResolver + { + private readonly DriveType _driveType; + + public ClassifiedDriveResolver(XmlResolver source, DriveType driveType) : base(source) + { + _driveType = driveType; + } + + protected override DriveType GetDriveType(string root) + { + return _driveType; + } + } + + private sealed class ValidationErrorHandler : ErrorHandler + { + public override void HandleError(Severity sev, string reason, string filename, int line, int col, object data) + { + Trace.WriteLine(sev + ": " + reason); + } + } + } +} diff --git a/src/Model.Tests/Model.Tests.csproj b/src/Model.Tests/Model.Tests.csproj index 5092158c..88c02deb 100644 --- a/src/Model.Tests/Model.Tests.csproj +++ b/src/Model.Tests/Model.Tests.csproj @@ -1,7 +1,8 @@  - net8.0 + net8.0;net48 + 8.0 enable false diff --git a/src/Model.Tests/SchemaLocationTests.cs b/src/Model.Tests/SchemaLocationTests.cs deleted file mode 100644 index 9e9f624b..00000000 --- a/src/Model.Tests/SchemaLocationTests.cs +++ /dev/null @@ -1,158 +0,0 @@ -using Microsoft.VisualStudio.TestTools.UnitTesting; -using System; -using System.Collections.Generic; -using System.Diagnostics; -using System.IO; -using System.Net; -using System.Text; -using System.Xml; -using XmlNotepad; - -// Settings.Instance and the delayed-action dispatcher are process-global. -[assembly: DoNotParallelize] - -namespace XmlNotepad.Model.Tests -{ - [TestClass] - public sealed class SchemaLocationTests : IServiceProvider - { - private const string NetworkSchemaLocation = @"\\mock-schema.invalid\schemas\schema.xsd"; - private MockSchemaSource _source = null!; - private Settings _settings = null!; - private XmlCache _model = null!; - private string _documentFile = null!; - - [TestInitialize] - public void Initialize() - { - _source = new MockSchemaSource(); - _settings = new Settings - { - StartupPath = AppContext.BaseDirectory, - Resolver = _source - }; - _settings.SetDefaults(); - var actions = new DelayedActions(action => action()); - _settings.DelayedActions = actions; - var cache = new SchemaCache(this, _source); - _model = new XmlCache(this, cache, actions); - _documentFile = Path.Combine(AppContext.BaseDirectory, Guid.NewGuid().ToString("N") + ".xml"); - } - - [TestCleanup] - public void Cleanup() - { - _model?.Dispose(); - _settings?.Dispose(); - } - - public object? GetService(Type serviceType) - { - return serviceType == typeof(Settings) ? _settings : null; - } - - [TestMethod] - [DataRow("noNamespaceSchemaLocation")] - [DataRow("schemaLocation")] - public void OpeningDocumentDoesNotRequestNetworkSchema(string schemaAttribute) - { - LoadXml(CreateXml(schemaAttribute, NetworkSchemaLocation)); - - Assert.IsEmpty(_source.Requests, - "Opening XML must not implicitly request a schema from a network share. " + - "Mock source requests: " + string.Join(", ", _source.Requests)); - } - - [TestMethod] - [DataRow("noNamespaceSchemaLocation")] - [DataRow("schemaLocation")] - public void ValidatingDocumentDoesNotRequestNetworkSchema(string schemaAttribute) - { - var document = new XmlDocument { XmlResolver = _source }; - document.LoadXml(CreateXml(schemaAttribute, NetworkSchemaLocation)); - _model.Load(document); - - _model.ValidateModel(new ValidationErrorHandler()); - - Assert.IsEmpty(_source.Requests, - "Validation must not implicitly request a schema from a network share. " + - "Mock source requests: " + string.Join(", ", _source.Requests)); - } - - [TestMethod] - public void OpeningDocumentLoadsLocalSchemaFromMockSource() - { - LoadXml(CreateXml("noNamespaceSchemaLocation", "schema.xsd")); - - Assert.HasCount(1, _source.Requests); - Assert.AreEqual(new Uri(new Uri(_documentFile), "schema.xsd"), _source.Requests[0]); - Assert.AreEqual(1, _model.Document.Schemas.Count); - } - - [TestMethod] - public void OpeningDocumentWithoutSchemaLocationDoesNotRequestSchema() - { - LoadXml(""); - - Assert.IsEmpty(_source.Requests); - } - - private void LoadXml(string xml) - { - using (var text = new StringReader(xml)) - using (var reader = XmlReader.Create(text, _model.GetReaderSettings(), new Uri(_documentFile).AbsoluteUri)) - { - _model.Load(reader, _documentFile); - } - Assert.AreEqual("root", _model.Document.DocumentElement?.LocalName); - } - - private static string CreateXml(string schemaAttribute, string schemaLocation) - { - string namespaceAttribute = ""; - if (schemaAttribute == "schemaLocation") - { - namespaceAttribute = " xmlns=\"urn:mock-schema\""; - schemaLocation = "urn:mock-schema " + schemaLocation; - } - return ""; - } - - private sealed class MockSchemaSource : XmlResolver - { - public List Requests { get; } = new List(); - - public override ICredentials? Credentials - { - set => throw new NotSupportedException("The mock source never uses credentials."); - } - - public override object GetEntity(Uri absoluteUri, string? role, Type? ofObjectToReturn) - { - Requests.Add(absoluteUri); - if (absoluteUri.IsUnc || !absoluteUri.IsFile) - { - throw new IOException("The mock source recorded the network request without performing I/O."); - } - if (ofObjectToReturn != null && ofObjectToReturn != typeof(Stream)) - { - throw new NotSupportedException("The mock source only provides streams."); - } - - const string schema = "" + - ""; - return new MemoryStream(Encoding.UTF8.GetBytes(schema)); - } - } - - private sealed class ValidationErrorHandler : ErrorHandler - { - public override void HandleError(Severity sev, string reason, string filename, int line, int col, object data) - { - Trace.WriteLine(sev + ": " + reason); - } - } - } -} diff --git a/src/Model/SchemaCache.cs b/src/Model/SchemaCache.cs index 3a1bd50d..d06aef7c 100644 --- a/src/Model/SchemaCache.cs +++ b/src/Model/SchemaCache.cs @@ -38,10 +38,7 @@ public Uri Location { _location = value; _schema = null; - if (_location.IsFile) - { - _fileName = _location.LocalPath; - } + _fileName = _location.IsFile ? _location.LocalPath : null; } } @@ -52,6 +49,10 @@ public bool HasUpToDateSchema if (_schema == null) return false; if (_fileName != null) { + if (XmlResourceResolver.IsRestrictedFileUri(_location)) + { + return false; + } DateTime lastWriteTime = File.GetLastWriteTime(_fileName); if (lastWriteTime > this._lastModified) { @@ -69,8 +70,12 @@ public XmlSchema Schema { if (_schema != value) { + if (value != null && _fileName != null) + { + XmlResourceResolver.ValidateUri(_location); + } _schema = value; - if (_fileName != null) + if (value != null && _fileName != null) { this._lastModified = File.GetLastWriteTime(_fileName); } @@ -842,6 +847,7 @@ public ValidationEventHandler Handler public override object GetEntity(Uri absoluteUri, string role, Type ofObjectToReturn) { + XmlResourceResolver.ValidateUri(absoluteUri); CacheEntry ce = cache.FindSchemaByUri(absoluteUri); if (ce != null && ce.HasUpToDateSchema) return ce.Schema; diff --git a/src/Model/Settings.cs b/src/Model/Settings.cs index c34ac24c..2b981535 100644 --- a/src/Model/Settings.cs +++ b/src/Model/Settings.cs @@ -1047,6 +1047,10 @@ public string GetPersistentFileName(Uri uri) result = uri.OriginalString; int len = 0; string path = uri.AbsolutePath; + if (XmlResourceResolver.IsRestrictedFileUri(uri)) + { + return uri.OriginalString; + } if (uri.IsFile && !File.Exists(uri.LocalPath)) // sanity check! return null; diff --git a/src/Model/Strings.Designer.cs b/src/Model/Strings.Designer.cs index 64746f0e..80d1d930 100644 --- a/src/Model/Strings.Designer.cs +++ b/src/Model/Strings.Designer.cs @@ -105,6 +105,15 @@ internal static string InvalidCharacter { } } + /// + /// Looks up a localized string similar to External access to file location '{0}' is blocked. Use a local file or an HTTP(S) URL.. + /// + internal static string ExternalFileResourceBlocked { + get { + return ResourceManager.GetString("ExternalFileResourceBlocked", resourceCulture); + } + } + /// /// Looks up a localized string similar to Error loading schema '{0}' ///{1}. diff --git a/src/Model/Strings.resx b/src/Model/Strings.resx index da85af0d..fd13312d 100644 --- a/src/Model/Strings.resx +++ b/src/Model/Strings.resx @@ -115,6 +115,10 @@ {1} Message box contents, arg0=filename, arg1=Exception.Message. + + External access to file location '{0}' is blocked. Use a local file or an HTTP(S) URL. + Exception message, arg0=resource URI. + Unexpected token '{0}' at line {1} column {2} Message box, arg0=token, arg1=number, arg2=number diff --git a/src/Model/XmlCache.cs b/src/Model/XmlCache.cs index f70da590..7b844398 100644 --- a/src/Model/XmlCache.cs +++ b/src/Model/XmlCache.cs @@ -200,9 +200,22 @@ public void Load(string file) } var handler = new ValidationEventHandler(OnValidationEvent); - using (XmlReader reader = XmlHelpers.ReadXml(file, null, handler)) + if (uri.IsFile) { - this.Load(reader, file); + // The document itself was explicitly selected; only its references use the guarded resolver. + using (var stream = File.OpenRead(uri.LocalPath)) + using (var reader = XmlReader.Create(stream, + XmlHelpers.CreateXmlSettings(Settings.Instance.Resolver, handler), uri.AbsoluteUri)) + { + this.Load(reader, file); + } + } + else + { + using (var reader = XmlHelpers.ReadXml(file, Settings.Instance.Resolver, handler)) + { + this.Load(reader, file); + } } } diff --git a/src/Model/XmlHelpers.cs b/src/Model/XmlHelpers.cs index ef530943..ff578522 100644 --- a/src/Model/XmlHelpers.cs +++ b/src/Model/XmlHelpers.cs @@ -1,7 +1,6 @@ using Microsoft.Xml; using System.Collections.Generic; using System.IO; -using System.Net; using System.Xml; using System.Xml.Schema; @@ -231,17 +230,7 @@ public static XmlReaderSettings CreateXmlSettings(XmlResolver resolver = null, V var ignoreDtd = Settings.Instance.GetBoolean("IgnoreDTD"); rs.DtdProcessing = ignoreDtd ? DtdProcessing.Ignore : DtdProcessing.Parse; rs.NameTable = new NameTable(); - if (resolver != null) - { - rs.XmlResolver = resolver; - } - else if (!ignoreDtd) - { - rs.XmlResolver = new XmlUrlResolver - { - Credentials = CredentialCache.DefaultCredentials - }; - } + rs.XmlResolver = resolver == null ? new XmlResourceResolver() : new XmlResourceResolver(resolver); if (handler != null) { rs.ValidationEventHandler += handler; diff --git a/src/Model/XmlResourceResolver.cs b/src/Model/XmlResourceResolver.cs new file mode 100644 index 00000000..1a64d6fb --- /dev/null +++ b/src/Model/XmlResourceResolver.cs @@ -0,0 +1,107 @@ +using System; +using System.IO; +using System.Net; +using System.Threading.Tasks; +using System.Xml; + +namespace XmlNotepad +{ + public class XmlResourceResolver : XmlResolver + { + private readonly XmlResolver _resolver; + + public XmlResourceResolver() : this(new XmlUrlResolver()) + { + } + + public XmlResourceResolver(XmlResolver resolver) + { + _resolver = resolver ?? throw new ArgumentNullException(nameof(resolver)); + } + + public override ICredentials Credentials + { + set => _resolver.Credentials = value; + } + + public override Uri ResolveUri(Uri baseUri, string relativeUri) + { + return _resolver.ResolveUri(baseUri, relativeUri); + } + + public override bool SupportsType(Uri absoluteUri, Type type) + { + return _resolver.SupportsType(absoluteUri, type); + } + + public override object GetEntity(Uri absoluteUri, string role, Type ofObjectToReturn) + { + ValidateUri(absoluteUri, GetDriveType); + return _resolver.GetEntity(absoluteUri, role, ofObjectToReturn); + } + + public override Task GetEntityAsync(Uri absoluteUri, string role, Type ofObjectToReturn) + { + ValidateUri(absoluteUri, GetDriveType); + return _resolver.GetEntityAsync(absoluteUri, role, ofObjectToReturn); + } + + public static void ValidateUri(Uri absoluteUri) + { + ValidateUri(absoluteUri, GetSystemDriveType); + } + + protected virtual DriveType GetDriveType(string root) + { + return GetSystemDriveType(root); + } + + internal static bool IsRestrictedFileUri(Uri absoluteUri) + { + return IsRestrictedFileUri(absoluteUri, GetSystemDriveType); + } + + private static void ValidateUri(Uri absoluteUri, Func getDriveType) + { + if (absoluteUri == null) + { + throw new ArgumentNullException(nameof(absoluteUri)); + } + if (IsRestrictedFileUri(absoluteUri, getDriveType)) + { + throw new XmlException(string.Format(Strings.ExternalFileResourceBlocked, absoluteUri)); + } + } + + private static bool IsRestrictedFileUri(Uri absoluteUri, Func getDriveType) + { + if (!absoluteUri.IsFile) + { + return false; + } + + string path = absoluteUri.LocalPath; + if (absoluteUri.IsUnc || path.StartsWith(@"\\", StringComparison.Ordinal) || + path.StartsWith("//", StringComparison.Ordinal)) + { + return true; + } + + string fullPath = Path.GetFullPath(path); + if (fullPath.StartsWith(@"\\", StringComparison.Ordinal) || + fullPath.StartsWith("//", StringComparison.Ordinal)) + { + return true; + } + // A drive-letter URI can still refer to a mapped network share. + DriveType driveType = getDriveType(Path.GetPathRoot(fullPath)); + return driveType == DriveType.Network || driveType == DriveType.Unknown || + driveType == DriveType.NoRootDirectory; + } + + private static DriveType GetSystemDriveType(string root) + { + return new DriveInfo(root).DriveType; + } + } +} diff --git a/src/Model/proxy.cs b/src/Model/proxy.cs index 7d1acdf2..c45c8df7 100644 --- a/src/Model/proxy.cs +++ b/src/Model/proxy.cs @@ -35,6 +35,7 @@ public override object GetEntity(Uri absoluteUri, string role, Type ofObjectToRe { throw new ArgumentNullException("absoluteUri"); } + XmlResourceResolver.ValidateUri(absoluteUri); if (_sourceResolver != null) { return _sourceResolver.GetEntity(absoluteUri, role, ofObjectToReturn); @@ -68,6 +69,16 @@ public override object GetEntity(Uri absoluteUri, string role, Type ofObjectToRe } } + public override System.Threading.Tasks.Task GetEntityAsync(Uri absoluteUri, string role, Type ofObjectToReturn) + { + XmlResourceResolver.ValidateUri(absoluteUri); + if (_sourceResolver != null) + { + return _sourceResolver.GetEntityAsync(absoluteUri, role, ofObjectToReturn); + } + return base.GetEntityAsync(absoluteUri, role, ofObjectToReturn); + } + Stream GetResponse(Uri uri) { Debug.WriteLine($"Loading Uri {uri}"); diff --git a/src/XmlNotepad/AsyncXslt.cs b/src/XmlNotepad/AsyncXslt.cs index 6a7f97f9..5b895538 100644 --- a/src/XmlNotepad/AsyncXslt.cs +++ b/src/XmlNotepad/AsyncXslt.cs @@ -31,7 +31,7 @@ internal class AsyncXsltContext public bool hasDefaultXsltOutput; public bool enableScripts; public bool disableOutputFile; - public XmlUrlResolver resolver; + public XmlResolver resolver; public CancellationTokenSource token = new CancellationTokenSource(); public string defaultSSResource; public bool running; @@ -249,6 +249,7 @@ async System.Threading.Tasks.Task InternalRunTransform() else { resolved = new Uri(_context.baseUri, this._context.xsltfilename); + XmlResourceResolver.ValidateUri(resolved); if (resolved != this._xsltUri || IsModified()) { try @@ -264,7 +265,10 @@ async System.Threading.Tasks.Task InternalRunTransform() // the XSLT DOM is also handy to have around for GetOutputMethod this._xsltdoc = new XmlDocument(); - this._xsltdoc.Load(resolved.AbsoluteUri); + using (XmlReader r = XmlHelpers.ReadXml(resolved.AbsoluteUri, this._context.resolver)) + { + this._xsltdoc.Load(r); + } } catch (Exception) { @@ -321,7 +325,10 @@ async System.Threading.Tasks.Task InternalRunTransform() { using (StringWriter writer = new StringWriter()) { - transform.Transform(xmlReader, null, writer); + using (XmlWriter output = XmlWriter.Create(writer, transform.OutputSettings)) + { + transform.Transform(xmlReader, null, output, this._context.resolver); + } this._xsltUri = resolved; this._context.output = writer.ToString(); } @@ -339,7 +346,10 @@ async System.Threading.Tasks.Task InternalRunTransform() // cache to an inmemory stream so we can strip the BOM. using (MemoryStream ms = new MemoryStream()) { - transform.Transform(xmlReader, null, ms); + using (XmlWriter output = XmlWriter.Create(ms, transform.OutputSettings)) + { + transform.Transform(xmlReader, null, output, this._context.resolver); + } ms.Seek(0, SeekOrigin.Begin); EncodingHelpers.WriteFileWithoutBOM(ms, outpath); } @@ -356,7 +366,10 @@ async System.Threading.Tasks.Task InternalRunTransform() this._context.writer = wrapper; Stopwatch watch = new Stopwatch(); watch.Start(); - transform.Transform(xmlReader, null, wrapper); + using (XmlWriter output = XmlWriter.Create(wrapper, transform.OutputSettings)) + { + transform.Transform(xmlReader, null, output, this._context.resolver); + } watch.Stop(); this._context.info.XsltMilliseconds = watch.ElapsedMilliseconds; Debug.WriteLine("Transform in {0} milliseconds", watch.ElapsedMilliseconds); @@ -577,7 +590,7 @@ XslCompiledTransform GetDefaultStylesheet() using (XmlReader reader = XmlHelpers.ReadXml(new StringReader(html))) { XslCompiledTransform t = new XslCompiledTransform(); - t.Load(reader); + t.Load(reader, XsltSettings.Default, _context.resolver); _defaultss = t; } // the XSLT DOM is also handy to have around for GetOutputMethod diff --git a/src/XmlNotepad/XsltControl.cs b/src/XmlNotepad/XsltControl.cs index 8503ebb5..5ca9c61a 100644 --- a/src/XmlNotepad/XsltControl.cs +++ b/src/XmlNotepad/XsltControl.cs @@ -32,7 +32,7 @@ public partial class XsltControl : UserControl private Uri _baseUri; private PerformanceInfo _info = null; private ISite _site; - private XmlUrlResolver _resolver; + private XmlResolver _resolver; private Settings _settings; private bool _webInitialized; private bool _webView2Initialized; @@ -50,7 +50,7 @@ public partial class XsltControl : UserControl public XsltControl() { InitializeComponent(); - _resolver = new XmlUrlResolver(); + _resolver = new XmlResourceResolver(); _delayedActions = new DelayedActions(); } From f24f9c0000b88c9e10fe5763b215d75c51d94b36 Mon Sep 17 00:00:00 2001 From: Will Portnoy Date: Fri, 30 Oct 2026 13:32:43 -0700 Subject: [PATCH 3/5] Close XML resource resolution review gaps Classify lexical drive roots before filesystem-backed normalization. Guard schema-set preprocessing and stylesheet DOM reloads. Preserve explicit comparison inputs and the XmlTextReader contract while guarding their references. Add focused regressions and run the headless model suite in both desktop CI configurations. All 43 cases pass on .NET Framework 4.8 and .NET 8 in Debug and Release. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38cfa7db-d690-4e7f-a2ff-a38a5e8015b5 --- .github/workflows/dotnet-desktop.yml | 5 + src/Application/XmlDiffWrapper.cs | 13 ++- src/Model.Tests/ExternalResourceTests.cs | 117 ++++++++++++++++++++++- src/Model/SchemaCache.cs | 6 +- src/Model/XmlCache.cs | 18 +--- src/Model/XmlHelpers.cs | 48 ++++++++++ src/Model/XmlResourceResolver.cs | 17 +++- src/XmlNotepad/AsyncXslt.cs | 9 +- src/XmlNotepad/XsdInstanceGenerator.cs | 2 +- src/XmlNotepad/XsltViewer.cs | 9 +- 10 files changed, 205 insertions(+), 39 deletions(-) diff --git a/.github/workflows/dotnet-desktop.yml b/.github/workflows/dotnet-desktop.yml index af5c6bc9..3f373e51 100644 --- a/.github/workflows/dotnet-desktop.yml +++ b/.github/workflows/dotnet-desktop.yml @@ -47,3 +47,8 @@ jobs: msbuild ./src/XmlNotepad.sln /p:Configuration=$env:Configuration "/p:Platform=Any CPU" /verbosity:minimal env: Configuration: ${{ matrix.configuration }} + + - name: Run headless model regression tests + run: dotnet test .\src\Model.Tests\Model.Tests.csproj --configuration $env:Configuration --no-build --verbosity normal + env: + Configuration: ${{ matrix.configuration }} diff --git a/src/Application/XmlDiffWrapper.cs b/src/Application/XmlDiffWrapper.cs index 8defef3a..7442db8c 100644 --- a/src/Application/XmlDiffWrapper.cs +++ b/src/Application/XmlDiffWrapper.cs @@ -70,14 +70,14 @@ public void DoCompare(Form owner, XmlCache model, string otherXmlFile, XmlDiffOp // diffView.Load call. XmlDocument original = new XmlDocument(); XmlReaderSettings settings = model.GetReaderSettings(); - using (XmlReader reader = XmlReader.Create(filename, settings)) + using (XmlReader reader = XmlHelpers.ReadDocument(filename, settings)) { original.Load(reader); } XmlDocument doc = new XmlDocument(); settings = model.GetReaderSettings(); - using (XmlReader reader = XmlReader.Create(otherXmlFile, settings)) + using (XmlReader reader = XmlHelpers.ReadDocument(otherXmlFile, settings)) { doc.Load(reader); } @@ -111,10 +111,13 @@ public void DoCompare(Form owner, XmlCache model, string otherXmlFile, XmlDiffOp using (XmlReader diffGram = XmlReader.Create(diffFile, settings)) { XmlDiffView diffView = new XmlDiffView(); - using (var reader = new XmlTextReader(filename)) + var location = new Uri(filename); + // XmlDiffView requires XmlTextReader; only the selected root uses the unguarded transport. + using (var input = (Stream)new XmlUrlResolver().GetEntity(location, null, typeof(Stream))) + using (var reader = new XmlTextReader(location.AbsoluteUri, input)) { - var ignoreDtd = Settings.Instance.GetBoolean("IgnoreDTD"); - reader.DtdProcessing = ignoreDtd ? DtdProcessing.Ignore : DtdProcessing.Parse; + reader.DtdProcessing = settings.DtdProcessing; + reader.XmlResolver = new XmlResourceResolver(model.Settings.Resolver ?? new XmlUrlResolver()); diffView.Load(reader, diffGram); using (TextWriter htmlWriter = new StreamWriter(tempFile, false, Encoding.UTF8)) { diff --git a/src/Model.Tests/ExternalResourceTests.cs b/src/Model.Tests/ExternalResourceTests.cs index afafc05c..9ab346fb 100644 --- a/src/Model.Tests/ExternalResourceTests.cs +++ b/src/Model.Tests/ExternalResourceTests.cs @@ -7,6 +7,7 @@ using System.Text; using System.Threading.Tasks; using System.Xml; +using System.Xml.Schema; using System.Xml.Xsl; using XmlNotepad; @@ -25,6 +26,7 @@ public sealed class ExternalResourceTests : IServiceProvider private MockSchemaSource _source = null!; private TestProxyResolver _resolver = null!; private Settings _settings = null!; + private SchemaCache _cache = null!; private XmlCache _model = null!; private string _documentFile = null!; @@ -41,8 +43,8 @@ public void Initialize() _settings.SetDefaults(); var actions = new DelayedActions(action => action()); _settings.DelayedActions = actions; - var cache = new SchemaCache(this, _source); - _model = new XmlCache(this, cache, actions); + _cache = new SchemaCache(this, _source); + _model = new XmlCache(this, _cache, actions); _documentFile = Path.Combine(AppContext.BaseDirectory, Guid.NewGuid().ToString("N") + ".xml"); } @@ -298,6 +300,115 @@ public void ResolvingRestrictedDriveDoesNotReachSource(DriveType driveType) Assert.IsEmpty(_source.Requests); } + [TestMethod] + public void RestrictedDriveIsClassifiedBeforeNormalizingShortNameTail() + { + var uri = new Uri(new Uri(_documentFile), "SHORT~1|invalid/schema.xsd"); + var resolver = new ClassifiedDriveResolver(_source, DriveType.Network); + + Assert.Throws(() => resolver.GetEntity(uri, null, typeof(Stream))); + + Assert.HasCount(1, resolver.Roots); + Assert.AreEqual(Path.GetPathRoot(_documentFile), resolver.Roots[0]); + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + public void RootSuggestionsResolveDeferredImportsThroughSchemaResolver() + { + var childUri = new Uri("mock://schemas/child.xsd"); + const string parentSchema = + "" + + ""; + XmlSchema parent; + using (var text = new StringReader(parentSchema)) + using (var reader = XmlReader.Create(text, new XmlReaderSettings(), + "https://mock-schema.invalid/parent.xsd")) + { + parent = XmlSchema.Read(reader, null)!; + } + parent.SourceUri = "https://mock-schema.invalid/parent.xsd"; + _cache.Add(parent); + _source.Documents.Add(childUri, + "" + + "" + + ""); + _source.Requests.Clear(); + + var roots = new List(_cache.GetPossibleTopLevelElements()); + + Assert.HasCount(1, roots); + Assert.AreEqual(new XmlQualifiedName("root", "urn:parent"), roots[0].QualifiedName); + Assert.HasCount(1, _source.Requests); + Assert.AreEqual(childUri, _source.Requests[0]); + } + + [TestMethod] + public void LoadingResourceDocumentBlocksNetworkSource() + { + var uri = new Uri(NetworkFileUri); + _source.Documents.Add(uri, ""); + + Assert.Throws(() => XmlHelpers.LoadXmlResource(uri, _resolver)); + + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + [DataRow("stylesheet.xsl")] + [DataRow("https://mock-schema.invalid/stylesheet.xsl")] + public void LoadingResourceDocumentUsesAllowedMockSource(string location) + { + var uri = new Uri(new Uri(_documentFile), location); + _source.Documents.Add(uri, ""); + + var document = XmlHelpers.LoadXmlResource(uri, _resolver); + + Assert.AreEqual("html", document.DocumentElement?.GetAttribute("method")); + Assert.HasCount(1, _source.Requests); + } + + [TestMethod] + [DataRow(NetworkSchemaLocation)] + [DataRow(NetworkFileUri)] + public void ReadingExplicitlySelectedNetworkDocumentDoesNotResolveItsRoot(string filename) + { + Uri? opened = null; + var input = new MemoryStream(Encoding.UTF8.GetBytes("selected")); + using (var reader = XmlHelpers.ReadDocument(filename, _model.GetReaderSettings(), location => + { + opened = location; + return input; + })) + { + var document = new XmlDocument(); + document.Load(reader); + Assert.AreEqual("selected", document.InnerText); + } + + Assert.AreEqual(new Uri(filename), opened); + Assert.IsFalse(input.CanRead); + Assert.IsEmpty(_source.Requests); + } + + [TestMethod] + public void ReadingSelectedNetworkDocumentStillBlocksItsExternalEntities() + { + _settings["IgnoreDTD"] = false; + const string xml = "]>&external;"; + using (var reader = XmlHelpers.ReadDocument(NetworkFileUri, _model.GetReaderSettings(), + location => new MemoryStream(Encoding.UTF8.GetBytes(xml)))) + { + var document = new XmlDocument(); + Assert.Throws(() => document.Load(reader)); + } + + Assert.IsEmpty(_source.Requests); + } + [TestMethod] public async Task ResolvingMappedDriveAsyncDoesNotReachSource() { @@ -444,6 +555,7 @@ public TestProxyResolver(IServiceProvider site, XmlResolver source) : base(site, private sealed class ClassifiedDriveResolver : XmlResourceResolver { private readonly DriveType _driveType; + public List Roots { get; } = new List(); public ClassifiedDriveResolver(XmlResolver source, DriveType driveType) : base(source) { @@ -452,6 +564,7 @@ public ClassifiedDriveResolver(XmlResolver source, DriveType driveType) : base(s protected override DriveType GetDriveType(string root) { + Roots.Add(root); return _driveType; } } diff --git a/src/Model/SchemaCache.cs b/src/Model/SchemaCache.cs index d06aef7c..f2dbc25f 100644 --- a/src/Model/SchemaCache.cs +++ b/src/Model/SchemaCache.cs @@ -779,8 +779,9 @@ public IEnumerable GetPossibleTopLevelElements() { if (globalElementCache == null || globalElementCache.Count == 0) { - globalElementCache = new List(); + var elements = new List(); XmlSchemaSet set = new XmlSchemaSet(); + set.XmlResolver = this.Resolver; HashSet knownTargetNamespaces = new HashSet(); foreach (var entry in this.GetSchemas()) { @@ -809,9 +810,10 @@ public IEnumerable GetPossibleTopLevelElements() { if (o is XmlSchemaElement e) { - globalElementCache.Add(e); + elements.Add(e); } } + globalElementCache = elements; } return globalElementCache; } diff --git a/src/Model/XmlCache.cs b/src/Model/XmlCache.cs index 7b844398..3d480fdb 100644 --- a/src/Model/XmlCache.cs +++ b/src/Model/XmlCache.cs @@ -200,22 +200,10 @@ public void Load(string file) } var handler = new ValidationEventHandler(OnValidationEvent); - if (uri.IsFile) + using (var reader = XmlHelpers.ReadDocument(file, + XmlHelpers.CreateXmlSettings(Settings.Instance.Resolver, handler))) { - // The document itself was explicitly selected; only its references use the guarded resolver. - using (var stream = File.OpenRead(uri.LocalPath)) - using (var reader = XmlReader.Create(stream, - XmlHelpers.CreateXmlSettings(Settings.Instance.Resolver, handler), uri.AbsoluteUri)) - { - this.Load(reader, file); - } - } - else - { - using (var reader = XmlHelpers.ReadXml(file, Settings.Instance.Resolver, handler)) - { - this.Load(reader, file); - } + this.Load(reader, file); } } diff --git a/src/Model/XmlHelpers.cs b/src/Model/XmlHelpers.cs index ff578522..9a8de2fc 100644 --- a/src/Model/XmlHelpers.cs +++ b/src/Model/XmlHelpers.cs @@ -1,4 +1,5 @@ using Microsoft.Xml; +using System; using System.Collections.Generic; using System.IO; using System.Xml; @@ -244,6 +245,53 @@ public static XmlReader ReadXml(string path, XmlResolver resolver = null, Valida return XmlReader.Create(path, rs); } + public static XmlDocument LoadXmlResource(Uri location, XmlResolver resolver) + { + XmlResourceResolver.ValidateUri(location); + var document = new XmlDocument(); + using (var reader = ReadXml(location.AbsoluteUri, resolver)) + { + document.Load(reader); + } + return document; + } + + public static XmlReader ReadDocument(string filename, XmlReaderSettings settings, + Func openFile = null) + { + if (settings == null) + { + throw new ArgumentNullException(nameof(settings)); + } + var location = new Uri(filename, UriKind.RelativeOrAbsolute); + if (!location.IsAbsoluteUri) + { + location = new Uri(new Uri(Directory.GetCurrentDirectory() + Path.DirectorySeparatorChar), location); + } + if (!location.IsFile) + { + return XmlReader.Create(location.AbsoluteUri, settings); + } + + // Only the explicitly selected input bypasses resource resolution, not its references. + Stream input = openFile == null ? File.OpenRead(location.LocalPath) : openFile(location); + if (input == null) + { + throw new InvalidOperationException("The selected document source did not return a stream."); + } + try + { + var readerSettings = settings.Clone(); + readerSettings.CloseInput = true; + return XmlReader.Create(input, readerSettings, location.AbsoluteUri); + } + catch + { + input.Dispose(); + throw; + } + } + public static XmlReader ReadXml(Stream stm, XmlResolver resolver = null, ValidationEventHandler handler = null) { var rs = CreateXmlSettings(resolver, handler); diff --git a/src/Model/XmlResourceResolver.cs b/src/Model/XmlResourceResolver.cs index 1a64d6fb..90c5aacc 100644 --- a/src/Model/XmlResourceResolver.cs +++ b/src/Model/XmlResourceResolver.cs @@ -87,14 +87,21 @@ private static bool IsRestrictedFileUri(Uri absoluteUri, Func return true; } - string fullPath = Path.GetFullPath(path); - if (fullPath.StartsWith(@"\\", StringComparison.Ordinal) || - fullPath.StartsWith("//", StringComparison.Ordinal)) + string root = Path.DirectorySeparatorChar.ToString(); + if (Path.DirectorySeparatorChar == '\\') + { + if (path.Length < 3 || path[1] != ':' || (path[2] != '\\' && path[2] != '/')) + { + return true; + } + root = path.Substring(0, 2) + "\\"; + } + else if (path.Length == 0 || path[0] != Path.DirectorySeparatorChar) { return true; } - // A drive-letter URI can still refer to a mapped network share. - DriveType driveType = getDriveType(Path.GetPathRoot(fullPath)); + // Framework path normalization can probe short names, so classify the lexical root first. + DriveType driveType = getDriveType(root); return driveType == DriveType.Network || driveType == DriveType.Unknown || driveType == DriveType.NoRootDirectory; } diff --git a/src/XmlNotepad/AsyncXslt.cs b/src/XmlNotepad/AsyncXslt.cs index 5b895538..2f9ab6a7 100644 --- a/src/XmlNotepad/AsyncXslt.cs +++ b/src/XmlNotepad/AsyncXslt.cs @@ -264,11 +264,7 @@ async System.Threading.Tasks.Task InternalRunTransform() } // the XSLT DOM is also handy to have around for GetOutputMethod - this._xsltdoc = new XmlDocument(); - using (XmlReader r = XmlHelpers.ReadXml(resolved.AbsoluteUri, this._context.resolver)) - { - this._xsltdoc.Load(r); - } + this._xsltdoc = XmlHelpers.LoadXmlResource(resolved, this._context.resolver); } catch (Exception) { @@ -424,8 +420,7 @@ public string GetDefaultOutputExtension(string customFileName = null) { string path = customFileName; var resolved = new Uri(_context.baseUri, path); - this._xsltdoc = new XmlDocument(); - this._xsltdoc.Load(resolved.AbsoluteUri); + this._xsltdoc = XmlHelpers.LoadXmlResource(resolved, _context.resolver); } var method = GetOutputMethod(this._xsltdoc); if (method.ToLower() == "html") diff --git a/src/XmlNotepad/XsdInstanceGenerator.cs b/src/XmlNotepad/XsdInstanceGenerator.cs index d78a9c43..0d7acb94 100644 --- a/src/XmlNotepad/XsdInstanceGenerator.cs +++ b/src/XmlNotepad/XsdInstanceGenerator.cs @@ -46,8 +46,8 @@ public XsdInstanceGenerator(XmlSchema schema, IXmlNamespaceResolver mgr, XmlReso } this.xmlResolver = xmlResolver; this.schemaSet = new XmlSchemaSet(); - this.schemaSet.Add(schema); Init(mgr); + this.schemaSet.Add(schema); } private void Init(IXmlNamespaceResolver mgr) diff --git a/src/XmlNotepad/XsltViewer.cs b/src/XmlNotepad/XsltViewer.cs index 5f422875..27d83506 100644 --- a/src/XmlNotepad/XsltViewer.cs +++ b/src/XmlNotepad/XsltViewer.cs @@ -103,10 +103,15 @@ private void OnOutputFileNameKeyDown(object sender, KeyEventArgs e) } } - private bool IsValidPath(string path) + private bool IsValidPath(string path, bool externalResource = false) { try { + if (externalResource) + { + XmlResourceResolver.ValidateUri(this.xsltControl.ResolveRelativePath(path)); + return true; + } Uri uri = new Uri(path, UriKind.RelativeOrAbsolute); if (uri.IsAbsoluteUri && uri.Scheme == Uri.UriSchemeFile) { @@ -130,7 +135,7 @@ public async void DisplayXsltResults() string xpath = this.SourceFileName.Text.Trim().Trim('"'); if (!string.IsNullOrEmpty(xpath) && this._xsltFiles != null) { - if (!IsValidPath(xpath)) + if (!IsValidPath(xpath, externalResource: true)) { return; } From 6b402ab0bad4448b6fc61acff998c8e589bf0a9e Mon Sep 17 00:00:00 2001 From: Will Portnoy Date: Sat, 31 Oct 2026 18:41:27 -0700 Subject: [PATCH 4/5] Use explicit temporary filename APIs in resource tests Use Path.GetRandomFileName for the in-memory document base and Path.GetTempFileName for the owned file fixture, clarifying relative-path and file-ownership intent without CodeQL suppressions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38cfa7db-d690-4e7f-a2ff-a38a5e8015b5 --- src/Model.Tests/ExternalResourceTests.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Model.Tests/ExternalResourceTests.cs b/src/Model.Tests/ExternalResourceTests.cs index 9ab346fb..a55903d4 100644 --- a/src/Model.Tests/ExternalResourceTests.cs +++ b/src/Model.Tests/ExternalResourceTests.cs @@ -45,7 +45,7 @@ public void Initialize() _settings.DelayedActions = actions; _cache = new SchemaCache(this, _source); _model = new XmlCache(this, _cache, actions); - _documentFile = Path.Combine(AppContext.BaseDirectory, Guid.NewGuid().ToString("N") + ".xml"); + _documentFile = Path.Combine(AppContext.BaseDirectory, Path.GetRandomFileName()); } [TestCleanup] @@ -456,7 +456,7 @@ public void ReaderGuardsCustomResolverBeforeReadingExternalEntity() public void LoadingSelectedLocalFilePreservesRelativeEntityBaseUri() { _settings["IgnoreDTD"] = false; - string filename = Path.Combine(Path.GetTempPath(), Guid.NewGuid().ToString("N") + ".xml"); + string filename = Path.GetTempFileName(); var entityUri = new Uri(new Uri(filename), "entity.txt"); _source.Documents.Add(entityUri, "allowed"); try From f8441e17b777cae990fa6216224c89b9233a5202 Mon Sep 17 00:00:00 2001 From: Will Portnoy Date: Sat, 31 Oct 2026 18:58:38 -0700 Subject: [PATCH 5/5] Reserve virtual document names without path combination Use the framework-owned temporary-file path for the in-memory document base, remove its placeholder before loading to avoid file watching, and clean up the owned path if initialization fails. This removes the remaining CodeQL path-combine note without suppression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38cfa7db-d690-4e7f-a2ff-a38a5e8015b5 --- src/Model.Tests/ExternalResourceTests.cs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Model.Tests/ExternalResourceTests.cs b/src/Model.Tests/ExternalResourceTests.cs index a55903d4..47929468 100644 --- a/src/Model.Tests/ExternalResourceTests.cs +++ b/src/Model.Tests/ExternalResourceTests.cs @@ -45,13 +45,18 @@ public void Initialize() _settings.DelayedActions = actions; _cache = new SchemaCache(this, _source); _model = new XmlCache(this, _cache, actions); - _documentFile = Path.Combine(AppContext.BaseDirectory, Path.GetRandomFileName()); + _documentFile = Path.GetTempFileName(); + File.Delete(_documentFile); // The in-memory input must not enable file watching. } [TestCleanup] public void Cleanup() { _model?.Dispose(); + if (_documentFile != null) + { + File.Delete(_documentFile); + } _settings?.Dispose(); }