From 6a01d8121daba4a9b989d1ff65a1ac4a2213c579 Mon Sep 17 00:00:00 2001 From: Jack Murray Date: Tue, 27 Aug 2024 16:54:12 +0100 Subject: [PATCH 1/8] Add a new method to look up the app manifest from the deployment manifest --- .../DataFormatSigners/ClickOnceSigner.cs | 39 ++++++++++++++++++- src/Sign.Core/Resources.Designer.cs | 9 +++++ src/Sign.Core/Resources.resx | 3 ++ src/Sign.Core/xlf/Resources.cs.xlf | 5 +++ src/Sign.Core/xlf/Resources.de.xlf | 5 +++ src/Sign.Core/xlf/Resources.es.xlf | 5 +++ src/Sign.Core/xlf/Resources.fr.xlf | 5 +++ src/Sign.Core/xlf/Resources.it.xlf | 5 +++ src/Sign.Core/xlf/Resources.ja.xlf | 5 +++ src/Sign.Core/xlf/Resources.ko.xlf | 5 +++ src/Sign.Core/xlf/Resources.pl.xlf | 5 +++ src/Sign.Core/xlf/Resources.pt-BR.xlf | 5 +++ src/Sign.Core/xlf/Resources.ru.xlf | 5 +++ src/Sign.Core/xlf/Resources.tr.xlf | 5 +++ src/Sign.Core/xlf/Resources.zh-Hans.xlf | 5 +++ src/Sign.Core/xlf/Resources.zh-Hant.xlf | 5 +++ 16 files changed, 115 insertions(+), 1 deletion(-) diff --git a/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs b/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs index 4dec8194..1854b073 100644 --- a/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs +++ b/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs @@ -5,6 +5,7 @@ using System.Globalization; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; +using System.Xml; using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.FileSystemGlobbing.Abstractions; using Microsoft.Extensions.Logging; @@ -273,5 +274,41 @@ public void CopySigningDependencies(FileInfo deploymentManifestFile, DirectoryIn } } } + + /// + /// Try and find the application manifest (.manifest) file from a clickonce application manifest (.application / .vsto + /// There might not be one, if the user is attempting to only re-sign the deployment manifest without touching other files. + /// + /// + /// A FileInfo pointing to the Application manifest, or null if it couldn't be found. + /// + private FileInfo? GetApplicationManifestForDeploymentManifest(FileInfo deploymentManifest) + { + var xml = new XmlDocument(); + xml.Load(deploymentManifest.FullName); + // there should only be a single result here, if the file is a valid clickonce manifest. + var dependentAssemblies = xml.GetElementsByTagName("dependentAssembly"); + if (dependentAssemblies.Count != 1) + { + Logger.LogDebug(Resources.ApplicationManifestNotFound); + return null; + } + + var node = dependentAssemblies.Item(0); + if (node is null || node.Attributes is null) + { + Logger.LogDebug(Resources.ApplicationManifestNotFound); + return null; + } + + var applicationManifestFileNameAttribute = node.Attributes["codebase"]; + if (applicationManifestFileNameAttribute is null || string.IsNullOrEmpty(applicationManifestFileNameAttribute.Value)) + { + Logger.LogDebug(Resources.ApplicationManifestNotFound); + return null; + } + + return new FileInfo(Path.Combine(deploymentManifest.Directory!.FullName, applicationManifestFileNameAttribute.Value)); + } } -} \ No newline at end of file +} diff --git a/src/Sign.Core/Resources.Designer.cs b/src/Sign.Core/Resources.Designer.cs index 2cd9c9a6..8fd38aa6 100644 --- a/src/Sign.Core/Resources.Designer.cs +++ b/src/Sign.Core/Resources.Designer.cs @@ -60,6 +60,15 @@ internal Resources() { } } + /// + /// Looks up a localized string similar to Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest.. + /// + internal static string ApplicationManifestNotFound { + get { + return ResourceManager.GetString("ApplicationManifestNotFound", resourceCulture); + } + } + /// /// Looks up a localized string similar to Signing SignTool job with {count} files.. /// diff --git a/src/Sign.Core/Resources.resx b/src/Sign.Core/Resources.resx index 4ca2c0db..e245e5a1 100644 --- a/src/Sign.Core/Resources.resx +++ b/src/Sign.Core/Resources.resx @@ -275,4 +275,7 @@ The algorithm selected is not supported. + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + \ No newline at end of file diff --git a/src/Sign.Core/xlf/Resources.cs.xlf b/src/Sign.Core/xlf/Resources.cs.xlf index 10eedd92..91d1048e 100644 --- a/src/Sign.Core/xlf/Resources.cs.xlf +++ b/src/Sign.Core/xlf/Resources.cs.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. Podepisování úlohy SignTool s tímto počtem souborů: {count}. diff --git a/src/Sign.Core/xlf/Resources.de.xlf b/src/Sign.Core/xlf/Resources.de.xlf index 36fe225f..438f85b9 100644 --- a/src/Sign.Core/xlf/Resources.de.xlf +++ b/src/Sign.Core/xlf/Resources.de.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. Der SignTool-Auftrag wird mit {count} Dateien signiert. diff --git a/src/Sign.Core/xlf/Resources.es.xlf b/src/Sign.Core/xlf/Resources.es.xlf index 5f568dfa..449e1c8c 100644 --- a/src/Sign.Core/xlf/Resources.es.xlf +++ b/src/Sign.Core/xlf/Resources.es.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. Firmando el trabajo de SignTool con {count} archivos. diff --git a/src/Sign.Core/xlf/Resources.fr.xlf b/src/Sign.Core/xlf/Resources.fr.xlf index 69dcaead..16a6fdc7 100644 --- a/src/Sign.Core/xlf/Resources.fr.xlf +++ b/src/Sign.Core/xlf/Resources.fr.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. Signature du travail SignTool avec {count} fichiers. diff --git a/src/Sign.Core/xlf/Resources.it.xlf b/src/Sign.Core/xlf/Resources.it.xlf index 203f3a25..950c4155 100644 --- a/src/Sign.Core/xlf/Resources.it.xlf +++ b/src/Sign.Core/xlf/Resources.it.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. Firma del processo SignTool con {count} file. diff --git a/src/Sign.Core/xlf/Resources.ja.xlf b/src/Sign.Core/xlf/Resources.ja.xlf index db44e64d..d55bf2c1 100644 --- a/src/Sign.Core/xlf/Resources.ja.xlf +++ b/src/Sign.Core/xlf/Resources.ja.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. {count} 個のファイルを使用して SignTool ジョブに署名しています。 diff --git a/src/Sign.Core/xlf/Resources.ko.xlf b/src/Sign.Core/xlf/Resources.ko.xlf index cf25df6f..fe2390d1 100644 --- a/src/Sign.Core/xlf/Resources.ko.xlf +++ b/src/Sign.Core/xlf/Resources.ko.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. {count} 파일로 SignTool 작업에 서명하는 중입니다. diff --git a/src/Sign.Core/xlf/Resources.pl.xlf b/src/Sign.Core/xlf/Resources.pl.xlf index c9db68da..012cdd71 100644 --- a/src/Sign.Core/xlf/Resources.pl.xlf +++ b/src/Sign.Core/xlf/Resources.pl.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. Podpisywanie zadania SignTool przy użyciu {count} plików. diff --git a/src/Sign.Core/xlf/Resources.pt-BR.xlf b/src/Sign.Core/xlf/Resources.pt-BR.xlf index a405b988..077a9235 100644 --- a/src/Sign.Core/xlf/Resources.pt-BR.xlf +++ b/src/Sign.Core/xlf/Resources.pt-BR.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. Autenticando o trabalho SignTool com {count} arquivos. diff --git a/src/Sign.Core/xlf/Resources.ru.xlf b/src/Sign.Core/xlf/Resources.ru.xlf index 13aaacb1..0ad097de 100644 --- a/src/Sign.Core/xlf/Resources.ru.xlf +++ b/src/Sign.Core/xlf/Resources.ru.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. Задание подписывания SignTool с несколькими файлами ({count}). diff --git a/src/Sign.Core/xlf/Resources.tr.xlf b/src/Sign.Core/xlf/Resources.tr.xlf index aac32f14..d935eb59 100644 --- a/src/Sign.Core/xlf/Resources.tr.xlf +++ b/src/Sign.Core/xlf/Resources.tr.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. SignTool işi {count} dosya ile imzalanıyor. diff --git a/src/Sign.Core/xlf/Resources.zh-Hans.xlf b/src/Sign.Core/xlf/Resources.zh-Hans.xlf index cf8ecb11..405fe481 100644 --- a/src/Sign.Core/xlf/Resources.zh-Hans.xlf +++ b/src/Sign.Core/xlf/Resources.zh-Hans.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. 正在对包含 {count} 个文件的 SignTool 作业进行签名。 diff --git a/src/Sign.Core/xlf/Resources.zh-Hant.xlf b/src/Sign.Core/xlf/Resources.zh-Hant.xlf index 1fb97a2e..e39345aa 100644 --- a/src/Sign.Core/xlf/Resources.zh-Hant.xlf +++ b/src/Sign.Core/xlf/Resources.zh-Hant.xlf @@ -2,6 +2,11 @@ + + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + + Signing SignTool job with {count} files. 正在簽署具有 {count} 個檔案的 SignTool 工作。 From 929a797d65702e836db036feb19108b6b69a081e Mon Sep 17 00:00:00 2001 From: Jack Murray Date: Tue, 27 Aug 2024 17:14:16 +0100 Subject: [PATCH 2/8] detect the app manifest by looking it up, rather than assuming we have 0/1 files with a .manifest extension --- src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs b/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs index 1854b073..8e8e77b9 100644 --- a/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs +++ b/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs @@ -127,8 +127,8 @@ await Parallel.ForEachAsync(files, _parallelOptions, async (file, state) => // Inner files are now signed // now look for the manifest file and sign that if we have one - - FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => ".manifest".Equals(f.Extension, StringComparison.OrdinalIgnoreCase)); + var appManifestFromDeploymentManifest = GetApplicationManifestForDeploymentManifest(file); + FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => f.Name.Equals(appManifestFromDeploymentManifest?.Name, StringComparison.OrdinalIgnoreCase)); string fileArgs = $@"-update ""{manifestFile}"" {args}"; From 31708b484f60f6e350ff515c5414ef76d92e1726 Mon Sep 17 00:00:00 2001 From: Jack Murray Date: Tue, 27 Aug 2024 17:27:06 +0100 Subject: [PATCH 3/8] Add some dummy content for the deployment manifest so that it can be correctly parsed so that the existing tests pass --- .../ClickOnceSignerTests.cs | 22 ++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs b/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs index c906f771..fd7772b7 100644 --- a/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs +++ b/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs @@ -14,6 +14,22 @@ public sealed class ClickOnceSignerTests : IDisposable private readonly DirectoryService _directoryService = new(Mock.Of>()); private readonly ClickOnceSigner _signer; + private const string DeploymentManifestValidContent = @" + + + + + + + + + + + + + +"; + public ClickOnceSignerTests() { _signer = new ClickOnceSigner( @@ -215,7 +231,7 @@ public async Task SignAsync_WhenFilesIsClickOnceFile_Signs(string publisherName) FileInfo applicationFile = AddFile( containerSpy, temporaryDirectory.Directory, - string.Empty, + DeploymentManifestValidContent, "MyApp.application"); FileInfo dllDeployFile = AddFile( containerSpy, @@ -364,7 +380,7 @@ public async Task SignAsync_WhenFilesIsClickOnceFileWithoutContent_Signs() FileInfo applicationFile = AddFile( containerSpy, temporaryDirectory.Directory, - string.Empty, + DeploymentManifestValidContent, "MyApp.application"); SignOptions options = new( @@ -578,4 +594,4 @@ private static X509Certificate2 CreateCertificate() return request.CreateSelfSigned(now.AddMinutes(-5), now.AddMinutes(5)); } } -} \ No newline at end of file +} From c2cfe06ee9fc51f75d21c121c44dbf0df6585bf7 Mon Sep 17 00:00:00 2001 From: Jack Murray Date: Wed, 28 Aug 2024 10:08:16 +0100 Subject: [PATCH 4/8] handle codebase values that are uris --- .../DataFormatSigners/ClickOnceSigner.cs | 20 +++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-) diff --git a/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs b/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs index 8e8e77b9..2873ed69 100644 --- a/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs +++ b/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs @@ -128,7 +128,7 @@ await Parallel.ForEachAsync(files, _parallelOptions, async (file, state) => // Inner files are now signed // now look for the manifest file and sign that if we have one var appManifestFromDeploymentManifest = GetApplicationManifestForDeploymentManifest(file); - FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => f.Name.Equals(appManifestFromDeploymentManifest?.Name, StringComparison.OrdinalIgnoreCase)); + FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => f.Name.Equals(appManifestFromDeploymentManifest, StringComparison.OrdinalIgnoreCase)); string fileArgs = $@"-update ""{manifestFile}"" {args}"; @@ -278,11 +278,13 @@ public void CopySigningDependencies(FileInfo deploymentManifestFile, DirectoryIn /// /// Try and find the application manifest (.manifest) file from a clickonce application manifest (.application / .vsto /// There might not be one, if the user is attempting to only re-sign the deployment manifest without touching other files. + /// This is necessary because there might be multiple *.manifest files present, e.g. if a DLL that's part of the clickonce + /// package ships its own assembly manifest which isn't a clickonce application manifest. /// /// - /// A FileInfo pointing to the Application manifest, or null if it couldn't be found. + /// A string containing the file name of the Application manifest, or null if it couldn't be found. /// - private FileInfo? GetApplicationManifestForDeploymentManifest(FileInfo deploymentManifest) + private string? GetApplicationManifestForDeploymentManifest(FileInfo deploymentManifest) { var xml = new XmlDocument(); xml.Load(deploymentManifest.FullName); @@ -308,7 +310,17 @@ public void CopySigningDependencies(FileInfo deploymentManifestFile, DirectoryIn return null; } - return new FileInfo(Path.Combine(deploymentManifest.Directory!.FullName, applicationManifestFileNameAttribute.Value)); + // the codebase attribute can be a relative file path (e.g. Application Files\MyApp_1_0_0_0\MyApp.dll.manifest) or + // a URI (e.g. https://my.cdn.com/clickonce/MyApp/ApplicationFiles/MyApp_1_0_0_0/MyApp.dll.manifest) so we need to + // handle both cases and extract just the file name part. + // + // we only try and parse absolute uris, because a relative uri can just be treated like a file path for our purposes + if (Uri.TryCreate(applicationManifestFileNameAttribute.Value, UriKind.Absolute, out var uri)) + { + Path.GetFileName(uri.LocalPath); // works for http(s) and file:// uris + } + + return Path.GetFileName(applicationManifestFileNameAttribute.Value); } } } From 11499e034ed9e2dd912568e5a5b62919d68d0005 Mon Sep 17 00:00:00 2001 From: Jack Murray Date: Wed, 28 Aug 2024 10:08:36 +0100 Subject: [PATCH 5/8] fix file path and version string --- .../SignatureProviders/ClickOnceSignerTests.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs b/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs index fd7772b7..3d4e9a6a 100644 --- a/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs +++ b/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs @@ -16,15 +16,15 @@ public sealed class ClickOnceSignerTests : IDisposable private const string DeploymentManifestValidContent = @" - + - - + + From 1b46d6e098a73676e4cd4786bc4b756f6d0e7b78 Mon Sep 17 00:00:00 2001 From: Jack Murray Date: Wed, 28 Aug 2024 10:08:55 +0100 Subject: [PATCH 6/8] add test to verify that we detect/sign the correct manifest file --- .../ClickOnceSignerTests.cs | 141 ++++++++++++++++++ 1 file changed, 141 insertions(+) diff --git a/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs b/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs index 3d4e9a6a..bf665eee 100644 --- a/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs +++ b/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs @@ -563,6 +563,147 @@ public void CopySigningDependencies_CopiesCorrectFiles() } } + [Fact] + public async Task SignAsync_WhenFilesIsClickOnce_DetectsCorrectManifest() + { + const string commonName = "Test certificate (DO NOT TRUST)"; + + using (TemporaryDirectory temporaryDirectory = new(_directoryService)) + { + FileInfo clickOnceFile = new( + Path.Combine( + temporaryDirectory.Directory.FullName, + $"{Path.GetRandomFileName()}.clickonce")); + + ContainerSpy containerSpy = new(clickOnceFile); + + FileInfo applicationFile = AddFile( + containerSpy, + temporaryDirectory.Directory, + DeploymentManifestValidContent, + "MyApp.application"); + // This is an incomplete manifest --- just enough to satisfy SignAsync(...)'s requirements. + FileInfo manifestFile = AddFile( + containerSpy, + temporaryDirectory.Directory, + @$" + + +", + "MyApp_1_0_0_0", "MyApp.dll.manifest"); + // A second, unrelated manifest file, which we want to ignore and not sign. + FileInfo secondManifestFile = AddFile( + containerSpy, + temporaryDirectory.Directory, + @$" + + +", + "MyApp_1_0_0_0", "Some.Dependency.dll.manifest"); + + + SignOptions options = new( + "ApplicationName", + "PublisherName", + "Description", + new Uri("https://description.test"), + HashAlgorithmName.SHA256, + HashAlgorithmName.SHA256, + new Uri("http://timestamp.test"), + matcher: null, + antiMatcher: null); + + using (X509Certificate2 certificate = CreateCertificate()) + using (RSA privateKey = certificate.GetRSAPrivateKey()!) + { + Mock signatureAlgorithmProvider = new(); + Mock certificateProvider = new(); + + certificateProvider.Setup(x => x.GetCertificateAsync(It.IsAny())) + .ReturnsAsync(certificate); + + signatureAlgorithmProvider.Setup(x => x.GetRsaAsync(It.IsAny())) + .ReturnsAsync(privateKey); + + Mock serviceProvider = new(); + AggregatingSignerSpy aggregatingSignerSpy = new(); + + serviceProvider.Setup(x => x.GetService(It.IsAny())) + .Returns(aggregatingSignerSpy); + + Mock mageCli = new(); + string expectedArgs = $"-update \"{manifestFile.FullName}\" -a sha256RSA -n \"{options.ApplicationName}\""; + mageCli.Setup(x => x.RunAsync( + It.Is(args => string.Equals(expectedArgs, args, StringComparison.Ordinal)))) + .ReturnsAsync(0); + + string publisher; + + if (string.IsNullOrEmpty(options.PublisherName)) + { + publisher = certificate.SubjectName.Name; + } + else + { + publisher = options.PublisherName; + } + + expectedArgs = $"-update \"{applicationFile.FullName}\" -a sha256RSA -n \"{options.ApplicationName}\" -pub \"{publisher}\" -appm \"{manifestFile.FullName}\" -SupportURL https://description.test/"; + mageCli.Setup(x => x.RunAsync( + It.Is(args => string.Equals(expectedArgs, args, StringComparison.Ordinal)))) + .ReturnsAsync(0); + + Mock manifestSigner = new(); + Mock fileMatcher = new(); + + manifestSigner.Setup( + x => x.Sign( + It.Is(fi => fi.Name == manifestFile.Name), + It.Is(c => ReferenceEquals(certificate, c)), + It.Is(rsa => ReferenceEquals(privateKey, rsa)), + It.Is(o => ReferenceEquals(options, o)))); + + manifestSigner.Setup( + x => x.Sign( + It.Is(fi => fi.Name == applicationFile.Name), + It.Is(c => ReferenceEquals(certificate, c)), + It.Is(rsa => ReferenceEquals(privateKey, rsa)), + It.Is(o => ReferenceEquals(options, o)))); + + ILogger logger = Mock.Of>(); + ClickOnceSigner signer = new( + signatureAlgorithmProvider.Object, + certificateProvider.Object, + serviceProvider.Object, + mageCli.Object, + manifestSigner.Object, + logger, + fileMatcher.Object); + + await signer.SignAsync(new[] { applicationFile }, options); + + // Verify that files have been renamed back. + foreach (FileInfo file in containerSpy.Files) + { + file.Refresh(); + + Assert.True(file.Exists); + } + + mageCli.VerifyAll(); + manifestSigner.VerifyAll(); + + // make sure we never tried to sign the second manifest file + manifestSigner.Verify( + x => x.Sign( + It.Is(fi => fi.Name == secondManifestFile.Name), + It.Is(c => ReferenceEquals(certificate, c)), + It.Is(rsa => ReferenceEquals(privateKey, rsa)), + It.Is(o => ReferenceEquals(options, o))), Times.Never()); + } + } + } + private static FileInfo AddFile( ContainerSpy containerSpy, DirectoryInfo directory, From 0bf23a67ff1d467437d5305b189e48bef1a24ce3 Mon Sep 17 00:00:00 2001 From: Damon Tivel Date: Wed, 18 Sep 2024 14:27:40 -0700 Subject: [PATCH 7/8] Apply feedback --- .../DataFormatSigners/ClickOnceSigner.cs | 61 +++++++++++-------- src/Sign.Core/Resources.resx | 1 + src/Sign.Core/xlf/Resources.cs.xlf | 2 +- src/Sign.Core/xlf/Resources.de.xlf | 2 +- src/Sign.Core/xlf/Resources.es.xlf | 2 +- src/Sign.Core/xlf/Resources.fr.xlf | 2 +- src/Sign.Core/xlf/Resources.it.xlf | 2 +- src/Sign.Core/xlf/Resources.ja.xlf | 2 +- src/Sign.Core/xlf/Resources.ko.xlf | 2 +- src/Sign.Core/xlf/Resources.pl.xlf | 2 +- src/Sign.Core/xlf/Resources.pt-BR.xlf | 2 +- src/Sign.Core/xlf/Resources.ru.xlf | 2 +- src/Sign.Core/xlf/Resources.tr.xlf | 2 +- src/Sign.Core/xlf/Resources.zh-Hans.xlf | 2 +- src/Sign.Core/xlf/Resources.zh-Hant.xlf | 2 +- 15 files changed, 50 insertions(+), 38 deletions(-) diff --git a/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs b/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs index 2873ed69..c25b040f 100644 --- a/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs +++ b/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs @@ -2,6 +2,7 @@ // The .NET Foundation licenses this file to you under the MIT license. // See the LICENSE.txt file in the project root for more information. +using System.Diagnostics.CodeAnalysis; using System.Globalization; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; @@ -127,14 +128,17 @@ await Parallel.ForEachAsync(files, _parallelOptions, async (file, state) => // Inner files are now signed // now look for the manifest file and sign that if we have one - var appManifestFromDeploymentManifest = GetApplicationManifestForDeploymentManifest(file); - FileInfo? manifestFile = filteredFiles.SingleOrDefault(f => f.Name.Equals(appManifestFromDeploymentManifest, StringComparison.OrdinalIgnoreCase)); + FileInfo? applicationManifestFile = null; + if (TryGetApplicationManifestFileName(file, out string? fileName)) + { + applicationManifestFile = filteredFiles.SingleOrDefault(f => f.Name.Equals(fileName, StringComparison.OrdinalIgnoreCase)); + } - string fileArgs = $@"-update ""{manifestFile}"" {args}"; + string fileArgs = $@"-update ""{applicationManifestFile}"" {args}"; - if (manifestFile is not null && !await SignAsync(fileArgs, manifestFile, rsaPrivateKey, certificate, options)) + if (applicationManifestFile is not null && !await SignAsync(fileArgs, applicationManifestFile, rsaPrivateKey, certificate, options)) { - string message = string.Format(CultureInfo.CurrentCulture, Resources.SigningFailed, manifestFile.FullName); + string message = string.Format(CultureInfo.CurrentCulture, Resources.SigningFailed, applicationManifestFile.FullName); throw new Exception(message); } @@ -166,9 +170,9 @@ await Parallel.ForEachAsync(files, _parallelOptions, async (file, state) => foreach (FileInfo deploymentManifestFile in deploymentManifestFiles) { fileArgs = $@"-update ""{deploymentManifestFile.FullName}"" {args} {publisherParam}"; - if (manifestFile is not null) + if (applicationManifestFile is not null) { - fileArgs += $@" -appm ""{manifestFile.FullName}"""; + fileArgs += $@" -appm ""{applicationManifestFile.FullName}"""; } if (options.DescriptionUrl is not null) { @@ -276,51 +280,58 @@ public void CopySigningDependencies(FileInfo deploymentManifestFile, DirectoryIn } /// - /// Try and find the application manifest (.manifest) file from a clickonce application manifest (.application / .vsto + /// Try and find the application manifest (.manifest) file from a ClickOnce application manifest (.application / .vsto /// There might not be one, if the user is attempting to only re-sign the deployment manifest without touching other files. - /// This is necessary because there might be multiple *.manifest files present, e.g. if a DLL that's part of the clickonce - /// package ships its own assembly manifest which isn't a clickonce application manifest. + /// This is necessary because there might be multiple *.manifest files present, e.g. if a DLL that's part of the ClickOnce + /// package ships its own assembly manifest which isn't a ClickOnce application manifest. /// - /// - /// A string containing the file name of the Application manifest, or null if it couldn't be found. + /// A representing a deployment manifest file. + /// A representing a manifest file name or null if one isn't found. + /// true if the application manifest file name was found; otherwise, false. /// - private string? GetApplicationManifestForDeploymentManifest(FileInfo deploymentManifest) + private bool TryGetApplicationManifestFileName(FileInfo deploymentManifest, [NotNullWhen(true)] out string? applicationManifestFileName) { + applicationManifestFileName = null; + var xml = new XmlDocument(); xml.Load(deploymentManifest.FullName); // there should only be a single result here, if the file is a valid clickonce manifest. - var dependentAssemblies = xml.GetElementsByTagName("dependentAssembly"); + XmlNodeList dependentAssemblies = xml.GetElementsByTagName("dependentAssembly"); if (dependentAssemblies.Count != 1) { Logger.LogDebug(Resources.ApplicationManifestNotFound); - return null; + return false; } - var node = dependentAssemblies.Item(0); + XmlNode? node = dependentAssemblies.Item(0); if (node is null || node.Attributes is null) { Logger.LogDebug(Resources.ApplicationManifestNotFound); - return null; + return false; } - var applicationManifestFileNameAttribute = node.Attributes["codebase"]; - if (applicationManifestFileNameAttribute is null || string.IsNullOrEmpty(applicationManifestFileNameAttribute.Value)) + XmlAttribute? codebaseAttribute = node.Attributes["codebase"]; + if (codebaseAttribute is null || string.IsNullOrEmpty(codebaseAttribute.Value)) { Logger.LogDebug(Resources.ApplicationManifestNotFound); - return null; + return false; } - // the codebase attribute can be a relative file path (e.g. Application Files\MyApp_1_0_0_0\MyApp.dll.manifest) or + // The codebase attribute can be a relative file path (e.g. Application Files\MyApp_1_0_0_0\MyApp.dll.manifest) or // a URI (e.g. https://my.cdn.com/clickonce/MyApp/ApplicationFiles/MyApp_1_0_0_0/MyApp.dll.manifest) so we need to // handle both cases and extract just the file name part. // - // we only try and parse absolute uris, because a relative uri can just be treated like a file path for our purposes - if (Uri.TryCreate(applicationManifestFileNameAttribute.Value, UriKind.Absolute, out var uri)) + // We only try and parse absolute URI's, because a relative URI can just be treated like a file path for our purposes. + if (Uri.TryCreate(codebaseAttribute.Value, UriKind.Absolute, out Uri? uri)) + { + applicationManifestFileName = Path.GetFileName(uri.LocalPath); // works for http(s) and file:// uris + } + else { - Path.GetFileName(uri.LocalPath); // works for http(s) and file:// uris + applicationManifestFileName = Path.GetFileName(codebaseAttribute.Value); } - return Path.GetFileName(applicationManifestFileNameAttribute.Value); + return !string.IsNullOrEmpty(applicationManifestFileName); } } } diff --git a/src/Sign.Core/Resources.resx b/src/Sign.Core/Resources.resx index e245e5a1..4a724708 100644 --- a/src/Sign.Core/Resources.resx +++ b/src/Sign.Core/Resources.resx @@ -277,5 +277,6 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. \ No newline at end of file diff --git a/src/Sign.Core/xlf/Resources.cs.xlf b/src/Sign.Core/xlf/Resources.cs.xlf index 91d1048e..b1f410ae 100644 --- a/src/Sign.Core/xlf/Resources.cs.xlf +++ b/src/Sign.Core/xlf/Resources.cs.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.de.xlf b/src/Sign.Core/xlf/Resources.de.xlf index 438f85b9..725fbe50 100644 --- a/src/Sign.Core/xlf/Resources.de.xlf +++ b/src/Sign.Core/xlf/Resources.de.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.es.xlf b/src/Sign.Core/xlf/Resources.es.xlf index 449e1c8c..4fa9ab4e 100644 --- a/src/Sign.Core/xlf/Resources.es.xlf +++ b/src/Sign.Core/xlf/Resources.es.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.fr.xlf b/src/Sign.Core/xlf/Resources.fr.xlf index 16a6fdc7..aadb1760 100644 --- a/src/Sign.Core/xlf/Resources.fr.xlf +++ b/src/Sign.Core/xlf/Resources.fr.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.it.xlf b/src/Sign.Core/xlf/Resources.it.xlf index 950c4155..ae5bebf0 100644 --- a/src/Sign.Core/xlf/Resources.it.xlf +++ b/src/Sign.Core/xlf/Resources.it.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.ja.xlf b/src/Sign.Core/xlf/Resources.ja.xlf index d55bf2c1..8568693d 100644 --- a/src/Sign.Core/xlf/Resources.ja.xlf +++ b/src/Sign.Core/xlf/Resources.ja.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.ko.xlf b/src/Sign.Core/xlf/Resources.ko.xlf index fe2390d1..f86d9637 100644 --- a/src/Sign.Core/xlf/Resources.ko.xlf +++ b/src/Sign.Core/xlf/Resources.ko.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.pl.xlf b/src/Sign.Core/xlf/Resources.pl.xlf index 012cdd71..f84d9308 100644 --- a/src/Sign.Core/xlf/Resources.pl.xlf +++ b/src/Sign.Core/xlf/Resources.pl.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.pt-BR.xlf b/src/Sign.Core/xlf/Resources.pt-BR.xlf index 077a9235..5b407134 100644 --- a/src/Sign.Core/xlf/Resources.pt-BR.xlf +++ b/src/Sign.Core/xlf/Resources.pt-BR.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.ru.xlf b/src/Sign.Core/xlf/Resources.ru.xlf index 0ad097de..b5149850 100644 --- a/src/Sign.Core/xlf/Resources.ru.xlf +++ b/src/Sign.Core/xlf/Resources.ru.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.tr.xlf b/src/Sign.Core/xlf/Resources.tr.xlf index d935eb59..eeaedf19 100644 --- a/src/Sign.Core/xlf/Resources.tr.xlf +++ b/src/Sign.Core/xlf/Resources.tr.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.zh-Hans.xlf b/src/Sign.Core/xlf/Resources.zh-Hans.xlf index 405fe481..0f1ab7aa 100644 --- a/src/Sign.Core/xlf/Resources.zh-Hans.xlf +++ b/src/Sign.Core/xlf/Resources.zh-Hans.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. diff --git a/src/Sign.Core/xlf/Resources.zh-Hant.xlf b/src/Sign.Core/xlf/Resources.zh-Hant.xlf index e39345aa..6090abea 100644 --- a/src/Sign.Core/xlf/Resources.zh-Hant.xlf +++ b/src/Sign.Core/xlf/Resources.zh-Hant.xlf @@ -5,7 +5,7 @@ Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. Did not find exactly 1 <dependentAssembly> element with a non-empty 'codebase' attribute within the deployment manifest. - + {Locked="<dependentAssembly>"} is an XML element. {Locked="codebase"} is an XML attribute. Signing SignTool job with {count} files. From 17f105153ccc31669f6c6bd8bea7d91472ea5571 Mon Sep 17 00:00:00 2001 From: Damon Tivel Date: Fri, 11 Oct 2024 16:39:43 -0700 Subject: [PATCH 8/8] Load XML documents safely and add tests. --- .../DataFormatSigners/ClickOnceSigner.cs | 19 +- src/Sign.Core/ServiceProvider.cs | 3 +- src/Sign.Core/Xml/IXmlDocumentLoader.cs | 13 + src/Sign.Core/Xml/XmlDocumentLoader.cs | 30 +++ test/Sign.Core.Test/ServiceProviderTests.cs | 3 +- .../ClickOnceSignerTests.cs | 224 ++++++++++++++++-- test/Sign.Core.Test/SignerTests.cs | 3 +- .../TestInfrastructure/SignerSpy.cs | 5 +- .../Xml/XmlDocumentLoaderTests.cs | 93 ++++++++ 9 files changed, 364 insertions(+), 29 deletions(-) create mode 100644 src/Sign.Core/Xml/IXmlDocumentLoader.cs create mode 100644 src/Sign.Core/Xml/XmlDocumentLoader.cs create mode 100644 test/Sign.Core.Test/Xml/XmlDocumentLoaderTests.cs diff --git a/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs b/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs index c25b040f..47c53941 100644 --- a/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs +++ b/src/Sign.Core/DataFormatSigners/ClickOnceSigner.cs @@ -22,6 +22,7 @@ internal sealed class ClickOnceSigner : RetryingSigner, IDataFormatSigner private readonly IManifestSigner _manifestSigner; private readonly ParallelOptions _parallelOptions = new() { MaxDegreeOfParallelism = 4 }; private readonly IFileMatcher _fileMatcher; + private readonly IXmlDocumentLoader _xmlDocumentLoader; // Dependency injection requires a public constructor. public ClickOnceSigner( @@ -31,7 +32,8 @@ public ClickOnceSigner( IMageCli mageCli, IManifestSigner manifestSigner, ILogger logger, - IFileMatcher fileMatcher) + IFileMatcher fileMatcher, + IXmlDocumentLoader xmlDocumentLoader) : base(logger) { ArgumentNullException.ThrowIfNull(signatureAlgorithmProvider, nameof(signatureAlgorithmProvider)); @@ -40,12 +42,14 @@ public ClickOnceSigner( ArgumentNullException.ThrowIfNull(mageCli, nameof(mageCli)); ArgumentNullException.ThrowIfNull(manifestSigner, nameof(manifestSigner)); ArgumentNullException.ThrowIfNull(fileMatcher, nameof(fileMatcher)); + ArgumentNullException.ThrowIfNull(xmlDocumentLoader, nameof(xmlDocumentLoader)); _signatureAlgorithmProvider = signatureAlgorithmProvider; _certificateProvider = certificateProvider; _mageCli = mageCli; _manifestSigner = manifestSigner; _fileMatcher = fileMatcher; + _xmlDocumentLoader = xmlDocumentLoader; // Need to delay this as it'd create a dependency loop if directly in the ctor _aggregatingSigner = new Lazy(() => serviceProvider.GetService()!); @@ -148,7 +152,7 @@ await Parallel.ForEachAsync(files, _parallelOptions, async (file, state) => if (string.IsNullOrEmpty(options.PublisherName)) { string publisherName = certificate.SubjectName.Name; - + // get the DN. it may be quoted publisherParam = $@"-pub ""{publisherName.Replace("\"", "")}"""; } @@ -232,7 +236,6 @@ protected override async Task SignCoreAsync(string? args, FileInfo file, R return false; } - private IEnumerable GetFiles(DirectoryInfo clickOnceRoot) { return clickOnceRoot.EnumerateFiles("*", SearchOption.AllDirectories); @@ -288,15 +291,15 @@ public void CopySigningDependencies(FileInfo deploymentManifestFile, DirectoryIn /// A representing a deployment manifest file. /// A representing a manifest file name or null if one isn't found. /// true if the application manifest file name was found; otherwise, false. - /// - private bool TryGetApplicationManifestFileName(FileInfo deploymentManifest, [NotNullWhen(true)] out string? applicationManifestFileName) + /// This is non-private only for unit testing. + internal bool TryGetApplicationManifestFileName(FileInfo deploymentManifest, [NotNullWhen(true)] out string? applicationManifestFileName) { applicationManifestFileName = null; - var xml = new XmlDocument(); - xml.Load(deploymentManifest.FullName); + XmlDocument xmlDoc = _xmlDocumentLoader.Load(deploymentManifest); + // there should only be a single result here, if the file is a valid clickonce manifest. - XmlNodeList dependentAssemblies = xml.GetElementsByTagName("dependentAssembly"); + XmlNodeList dependentAssemblies = xmlDoc.GetElementsByTagName("dependentAssembly"); if (dependentAssemblies.Count != 1) { Logger.LogDebug(Resources.ApplicationManifestNotFound); diff --git a/src/Sign.Core/ServiceProvider.cs b/src/Sign.Core/ServiceProvider.cs index ed37d238..881baa9c 100644 --- a/src/Sign.Core/ServiceProvider.cs +++ b/src/Sign.Core/ServiceProvider.cs @@ -75,6 +75,7 @@ internal static ServiceProvider CreateDefault( services.AddSingleton(); services.AddSingleton(); services.AddSingleton(); + services.AddSingleton(); services.AddSingleton(); if (addServices is not null) @@ -85,4 +86,4 @@ internal static ServiceProvider CreateDefault( return new ServiceProvider(services.BuildServiceProvider()); } } -} \ No newline at end of file +} diff --git a/src/Sign.Core/Xml/IXmlDocumentLoader.cs b/src/Sign.Core/Xml/IXmlDocumentLoader.cs new file mode 100644 index 00000000..878ddfd1 --- /dev/null +++ b/src/Sign.Core/Xml/IXmlDocumentLoader.cs @@ -0,0 +1,13 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE.txt file in the project root for more information. + +using System.Xml; + +namespace Sign.Core +{ + internal interface IXmlDocumentLoader + { + XmlDocument Load(FileInfo file); + } +} diff --git a/src/Sign.Core/Xml/XmlDocumentLoader.cs b/src/Sign.Core/Xml/XmlDocumentLoader.cs new file mode 100644 index 00000000..ee049d59 --- /dev/null +++ b/src/Sign.Core/Xml/XmlDocumentLoader.cs @@ -0,0 +1,30 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE.txt file in the project root for more information. + +using System.Xml; + +namespace Sign.Core +{ + internal sealed class XmlDocumentLoader : IXmlDocumentLoader + { + public XmlDocument Load(FileInfo file) + { + ArgumentNullException.ThrowIfNull(file); + + XmlReaderSettings settings = new() + { + DtdProcessing = DtdProcessing.Prohibit, + XmlResolver = null // prevent external entity resolution + }; + XmlDocument xmlDoc = new(); + + using (XmlReader reader = XmlReader.Create(file.FullName, settings)) + { + xmlDoc.Load(reader); + } + + return xmlDoc; + } + } +} diff --git a/test/Sign.Core.Test/ServiceProviderTests.cs b/test/Sign.Core.Test/ServiceProviderTests.cs index ab63cabc..433e15e9 100644 --- a/test/Sign.Core.Test/ServiceProviderTests.cs +++ b/test/Sign.Core.Test/ServiceProviderTests.cs @@ -58,6 +58,7 @@ public void CreateDefault_Always_RegistersRequiredServices() Assert.NotNull(serviceProvider.GetRequiredService()); Assert.NotNull(serviceProvider.GetRequiredService()); Assert.NotNull(serviceProvider.GetRequiredService()); + Assert.NotNull(serviceProvider.GetRequiredService()); Assert.NotNull(serviceProvider.GetRequiredService()); } @@ -149,4 +150,4 @@ public bool IsEnabled(LogLevel logLevel) } } } -} \ No newline at end of file +} diff --git a/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs b/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs index bf665eee..bef2a3c2 100644 --- a/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs +++ b/test/Sign.Core.Test/SignatureProviders/ClickOnceSignerTests.cs @@ -4,6 +4,7 @@ using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; +using System.Xml; using Microsoft.Extensions.Logging; using Moq; @@ -14,6 +15,8 @@ public sealed class ClickOnceSignerTests : IDisposable private readonly DirectoryService _directoryService = new(Mock.Of>()); private readonly ClickOnceSigner _signer; + private static readonly XmlDocumentLoader _xmlDocumentReader = new(); + private const string DeploymentManifestValidContent = @" @@ -27,8 +30,22 @@ public sealed class ClickOnceSignerTests : IDisposable - -"; + "; + + private const string DeploymentManifestCodeBaseTemplate = @" + + + + + + + + + + + + + "; public ClickOnceSignerTests() { @@ -39,7 +56,8 @@ public ClickOnceSignerTests() Mock.Of(), Mock.Of(), Mock.Of>(), - Mock.Of()); + Mock.Of(), + _xmlDocumentReader); } public void Dispose() @@ -58,7 +76,8 @@ public void Constructor_WhenSignatureAlgorithmProviderIsNull_Throws() Mock.Of(), Mock.Of(), Mock.Of>(), - Mock.Of())); + Mock.Of(), + Mock.Of())); Assert.Equal("signatureAlgorithmProvider", exception.ParamName); } @@ -74,7 +93,8 @@ public void Constructor_WhenCertificateProviderIsNull_Throws() Mock.Of(), Mock.Of(), Mock.Of>(), - Mock.Of())); + Mock.Of(), + Mock.Of())); Assert.Equal("certificateProvider", exception.ParamName); } @@ -90,7 +110,8 @@ public void Constructor_WhenServiceProviderIsNull_Throws() Mock.Of(), Mock.Of(), Mock.Of>(), - Mock.Of())); + Mock.Of(), + Mock.Of())); Assert.Equal("serviceProvider", exception.ParamName); } @@ -106,7 +127,8 @@ public void Constructor_WhenMageCliIsNull_Throws() mageCli: null!, Mock.Of(), Mock.Of>(), - Mock.Of())); + Mock.Of(), + Mock.Of())); Assert.Equal("mageCli", exception.ParamName); } @@ -122,7 +144,8 @@ public void Constructor_WhenManifestSignerIsNull_Throws() Mock.Of(), manifestSigner: null!, Mock.Of>(), - Mock.Of())); + Mock.Of(), + Mock.Of())); Assert.Equal("manifestSigner", exception.ParamName); } @@ -138,7 +161,8 @@ public void Constructor_WhenLoggerIsNull_Throws() Mock.Of(), Mock.Of(), logger: null!, - Mock.Of())); + Mock.Of(), + Mock.Of())); Assert.Equal("logger", exception.ParamName); } @@ -154,11 +178,29 @@ public void Constructor_WhenFileMatcherIsNull_Throws() Mock.Of(), Mock.Of(), Mock.Of>(), - fileMatcher: null!)); + fileMatcher: null!, + Mock.Of())); Assert.Equal("fileMatcher", exception.ParamName); } + [Fact] + public void Constructor_WhenXmlDocumentLoaderIsNull_Throws() + { + ArgumentNullException exception = Assert.Throws( + () => new ClickOnceSigner( + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of(), + Mock.Of>(), + Mock.Of(), + xmlDocumentLoader: null!)); + + Assert.Equal("xmlDocumentLoader", exception.ParamName); + } + [Fact] public void CanSign_WhenFileIsNull_Throws() { @@ -334,7 +376,8 @@ public async Task SignAsync_WhenFilesIsClickOnceFile_Signs(string publisherName) mageCli.Object, manifestSigner.Object, logger, - fileMatcher.Object); + fileMatcher.Object, + _xmlDocumentReader); await signer.SignAsync(new[] { applicationFile }, options); @@ -448,7 +491,8 @@ public async Task SignAsync_WhenFilesIsClickOnceFileWithoutContent_Signs() mageCli.Object, manifestSigner.Object, logger, - fileMatcher.Object); + fileMatcher.Object, + _xmlDocumentReader); await signer.SignAsync(new[] { applicationFile }, options); @@ -542,7 +586,8 @@ public void CopySigningDependencies_CopiesCorrectFiles() mageCli.Object, manifestSigner.Object, logger, - fileMatcher.Object); + fileMatcher.Object, + _xmlDocumentReader); using (TemporaryDirectory signingDirectory = new(_directoryService)) { @@ -601,7 +646,6 @@ public async Task SignAsync_WhenFilesIsClickOnce_DetectsCorrectManifest() ", "MyApp_1_0_0_0", "Some.Dependency.dll.manifest"); - SignOptions options = new( "ApplicationName", "PublisherName", @@ -678,11 +722,13 @@ public async Task SignAsync_WhenFilesIsClickOnce_DetectsCorrectManifest() mageCli.Object, manifestSigner.Object, logger, - fileMatcher.Object); + fileMatcher.Object, + _xmlDocumentReader); await signer.SignAsync(new[] { applicationFile }, options); - // Verify that files have been renamed back. + // None of the files were .deploy files and so none would have been renamed. + // Still, make sure they exist. foreach (FileInfo file in containerSpy.Files) { file.Refresh(); @@ -704,6 +750,152 @@ public async Task SignAsync_WhenFilesIsClickOnce_DetectsCorrectManifest() } } + [Fact] + public void TryGetApplicationManifestFileName_WhenCodeBaseAttributeIsMissing_ReturnsFalse() + { + using (TemporaryDirectory temporaryDirectory = new(_directoryService)) + { + string xml = ModifyXml( + DeploymentManifestCodeBaseTemplate, + (XmlDocument xmlDoc, XmlNamespaceManager xmlNamespaceManager) => + { + XmlNodeList nodes = xmlDoc.SelectNodes( + "//asmv1:assembly/asmv2:dependency/asmv2:dependentAssembly[@codebase]", + xmlNamespaceManager)!; + + foreach (XmlNode node in nodes) + { + XmlAttribute codebaseAttribute = node.Attributes!["codebase"]!; + + node.Attributes.Remove(codebaseAttribute); + } + }); + FileInfo file = CreateFile(temporaryDirectory.Directory, xml); + + bool actualResult = _signer.TryGetApplicationManifestFileName(file, out string? applicationManifestFileName); + + Assert.False(actualResult); + } + } + + [Fact] + public void TryGetApplicationManifestFileName_WhenDependentAssemblyElementIsMissing_ReturnsFalse() + { + using (TemporaryDirectory temporaryDirectory = new(_directoryService)) + { + string xml = ModifyXml( + DeploymentManifestCodeBaseTemplate, + (XmlDocument xmlDoc, XmlNamespaceManager xmlNamespaceManager) => + { + XmlNodeList nodes = xmlDoc.SelectNodes( + "//asmv1:assembly/asmv2:dependency/asmv2:dependentAssembly", + xmlNamespaceManager)!; + + foreach (XmlNode node in nodes) + { + node.ParentNode!.RemoveChild(node); + } + }); + FileInfo file = CreateFile(temporaryDirectory.Directory, xml); + + bool actualResult = _signer.TryGetApplicationManifestFileName(file, out string? applicationManifestFileName); + + Assert.False(actualResult); + } + } + + [Fact] + public void TryGetApplicationManifestFileName_WhenMultipleDependentAssemblyElementsArePresent_ReturnsFalse() + { + using (TemporaryDirectory temporaryDirectory = new(_directoryService)) + { + string xml = ModifyXml( + string.Format(DeploymentManifestCodeBaseTemplate, @"MyApp_1_0_0_0\MyApp.dll.manifest"), + (XmlDocument xmlDoc, XmlNamespaceManager xmlNamespaceManager) => + { + XmlNodeList nodes = xmlDoc.SelectNodes( + "//asmv1:assembly/asmv2:dependency/asmv2:dependentAssembly", + xmlNamespaceManager)!; + + foreach (XmlNode node in nodes) + { + XmlNode duplicateNode = node.CloneNode(deep: true); + + node.ParentNode!.AppendChild(duplicateNode); + } + }); + FileInfo file = CreateFile(temporaryDirectory.Directory, xml); + + bool actualResult = _signer.TryGetApplicationManifestFileName(file, out string? applicationManifestFileName); + + Assert.False(actualResult); + } + } + + [Theory] + [InlineData(@"MyApp_1_0_0_0\MyApp.dll.manifest")] + [InlineData("https://unit.test/MyApp.dll.manifest")] + public void TryGetApplicationManifestFileName_WhenCodeBaseIsValid_ReturnsTrue(string codebase) + { + using (TemporaryDirectory temporaryDirectory = new(_directoryService)) + { + string xml = string.Format(DeploymentManifestCodeBaseTemplate, codebase); + FileInfo file = CreateFile(temporaryDirectory.Directory, xml); + + bool actualResult = _signer.TryGetApplicationManifestFileName(file, out string? applicationManifestFileName); + + Assert.True(actualResult); + Assert.Equal("MyApp.dll.manifest", applicationManifestFileName); + } + } + + private static string ModifyXml(string xml, Action modify) + { + XmlReaderSettings settings = new() + { + DtdProcessing = DtdProcessing.Prohibit, + XmlResolver = null // prevent external entity resolution + }; + XmlDocument xmlDoc = new(); + + using (StringReader stringReader = new(xml)) + using (XmlReader xmlReader = XmlReader.Create(stringReader, settings)) + { + xmlDoc.Load(xmlReader); + } + + XmlNamespaceManager namespaceManager = new(xmlDoc.NameTable); + + namespaceManager.AddNamespace("asmv1", "urn:schemas-microsoft-com:asm.v1"); + namespaceManager.AddNamespace("asmv2", "urn:schemas-microsoft-com:asm.v2"); + namespaceManager.AddNamespace("asmv3", "urn:schemas-microsoft-com:asm.v3"); + namespaceManager.AddNamespace("dsig", "http://www.w3.org/2000/09/xmldsig#"); + namespaceManager.AddNamespace("co.v1", "urn:schemas-microsoft-com:clickonce.v1"); + namespaceManager.AddNamespace("co.v2", "urn:schemas-microsoft-com:clickonce.v2"); + namespaceManager.AddNamespace("xrml", "urn:mpeg:mpeg21:2003:01-REL-R-NS"); + namespaceManager.AddNamespace("xsi", "http://www.w3.org/2001/XMLSchema-instance"); + + modify(xmlDoc, namespaceManager); + + using (StringWriter stringWriter = new()) + using (XmlWriter xmlWriter = XmlWriter.Create(stringWriter)) + { + xmlDoc.WriteTo(xmlWriter); + xmlWriter.Flush(); + + return stringWriter.GetStringBuilder().ToString(); + } + } + + private static FileInfo CreateFile(DirectoryInfo directory, string xml) + { + FileInfo file = new(Path.Combine(directory.FullName, "test.xml")); + + File.WriteAllText(file.FullName, xml); + + return file; + } + private static FileInfo AddFile( ContainerSpy containerSpy, DirectoryInfo directory, diff --git a/test/Sign.Core.Test/SignerTests.cs b/test/Sign.Core.Test/SignerTests.cs index 5de3769e..909ce1d9 100644 --- a/test/Sign.Core.Test/SignerTests.cs +++ b/test/Sign.Core.Test/SignerTests.cs @@ -609,6 +609,7 @@ private ServiceProvider Create() services.AddSingleton(); services.AddSingleton(); services.AddSingleton(); + services.AddSingleton(); services.AddSingleton(); return new ServiceProvider(services.BuildServiceProvider()); @@ -624,4 +625,4 @@ private static void RegisterSipsFromIniFile() Kernel32.LoadLibraryW($@"{baseDirectory}\mssign32.dll"); } } -} \ No newline at end of file +} diff --git a/test/Sign.Core.Test/TestInfrastructure/SignerSpy.cs b/test/Sign.Core.Test/TestInfrastructure/SignerSpy.cs index d7a83083..b10c5dcd 100644 --- a/test/Sign.Core.Test/TestInfrastructure/SignerSpy.cs +++ b/test/Sign.Core.Test/TestInfrastructure/SignerSpy.cs @@ -49,7 +49,8 @@ internal SignerSpy() mageCli, manifestSigner, logger, - fileMatcher), + fileMatcher, + new XmlDocumentLoader()), new NuGetSigner(signatureAlgorithmProvider, certificateProvider, nuGetSignTool, logger), new VsixSigner(signatureAlgorithmProvider, certificateProvider, openVsixSignTool, logger) }; @@ -67,4 +68,4 @@ public Task SignAsync(IEnumerable files, SignOptions options) return Task.CompletedTask; } } -} \ No newline at end of file +} diff --git a/test/Sign.Core.Test/Xml/XmlDocumentLoaderTests.cs b/test/Sign.Core.Test/Xml/XmlDocumentLoaderTests.cs new file mode 100644 index 00000000..fc81ccea --- /dev/null +++ b/test/Sign.Core.Test/Xml/XmlDocumentLoaderTests.cs @@ -0,0 +1,93 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE.txt file in the project root for more information. + +using System.Xml; +using Microsoft.Extensions.Logging; +using Moq; + +namespace Sign.Core.Test +{ + public sealed class XmlDocumentLoaderTests : IDisposable + { + private readonly XmlDocumentLoader _loader; + private readonly TemporaryDirectory _temporaryDirectory; + + public XmlDocumentLoaderTests() + { + _loader = new XmlDocumentLoader(); + _temporaryDirectory = new TemporaryDirectory(new DirectoryService(Mock.Of>())); + } + + public void Dispose() + { + _temporaryDirectory.Dispose(); + } + + [Fact] + public void Load_WhenFileIsNull_Throws() + { + ArgumentNullException exception = Assert.Throws(() => _loader.Load(file: null!)); + + Assert.Equal("file", exception.ParamName); + } + + [Fact] + public void Load_WhenXmlContainsAnEmbeddedDtd_Throws() + { + string xml = @" + + + + + +]> + + you + me + Reminder + Don't forget this weekend! +"; + FileInfo file = CreateFile(xml); + + Assert.Throws(() => _loader.Load(file)); + } + + [Fact] + public void Load_WhenXmlContainsAnExternalEntity_Throws() + { + string xml = @" + +]> +&ext;"; + FileInfo file = CreateFile(xml); + + Assert.Throws(() => _loader.Load(file)); + } + + [Fact] + public void Load_WhenXmlIsAcceptable_ReturnsXmlDocument() + { + string xml = @"b"; + FileInfo file = CreateFile(xml); + + XmlDocument xmlDoc = _loader.Load(file); + + Assert.Equal("a", xmlDoc.DocumentElement!.Name); + Assert.Equal("b", xmlDoc.DocumentElement.InnerText); + } + + private FileInfo CreateFile(string xml) + { + FileInfo file = new(Path.Combine(_temporaryDirectory.Directory.FullName, "test.xml")); + + File.WriteAllText(file.FullName, xml); + + file.Refresh(); + + return file; + } + } +}