From 887b8982f8a55497fe0c642e15204ab381b3de7c Mon Sep 17 00:00:00 2001 From: AmirMS <104940545+AmelBawa-msft@users.noreply.github.com> Date: Fri, 10 Jul 2026 15:18:48 -0700 Subject: [PATCH 1/6] Validate download destination --- src/WingetCreateCore/Common/PackageParser.cs | 11 ++++++- .../WingetCreateTests/TestUtils.cs | 17 ++++++++++ .../UnitTests/PackageParserTests.cs | 31 +++++++++++++++++++ 3 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/WingetCreateCore/Common/PackageParser.cs b/src/WingetCreateCore/Common/PackageParser.cs index 560c1a81..12f0ee8a 100644 --- a/src/WingetCreateCore/Common/PackageParser.cs +++ b/src/WingetCreateCore/Common/PackageParser.cs @@ -178,7 +178,7 @@ public static async Task DownloadFileAsync(string url, bool allowHttp, l } string urlFile = Path.GetFileName(url.Split('?').Last()); - string contentDispositionFile = response.Content.Headers.ContentDisposition?.FileName?.Trim('"'); + string contentDispositionFile = Path.GetFileName(response.Content.Headers.ContentDisposition?.FileName?.Trim('"')); string requestUrlFileName = Path.GetFileName(response.RequestMessage?.RequestUri?.ToString()); if (!Directory.Exists(InstallerDownloadPath)) @@ -189,6 +189,15 @@ public static async Task DownloadFileAsync(string url, bool allowHttp, l // If no relevant filename can be obtained for the installer download, use a temporary filename as last option. string targetFileName = contentDispositionFile.NullIfEmpty() ?? urlFile.NullIfEmpty() ?? requestUrlFileName.NullIfEmpty() ?? Path.GetTempFileName(); string targetFile = GetNumericFilename(Path.Combine(InstallerDownloadPath, targetFileName)); + + // Defense in depth: ensure the resolved path stays under the installer download directory. + string downloadRoot = Path.GetFullPath(InstallerDownloadPath).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + string fullTargetFile = Path.GetFullPath(targetFile); + if (!fullTargetFile.StartsWith(downloadRoot + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException("Resolved download path escapes the installer download directory."); + } + using var targetFileStream = File.OpenWrite(targetFile); var contentStream = await response.Content.ReadAsStreamAsync(); diff --git a/src/WingetCreateTests/WingetCreateTests/TestUtils.cs b/src/WingetCreateTests/WingetCreateTests/TestUtils.cs index 99f5e916..0fe143f0 100644 --- a/src/WingetCreateTests/WingetCreateTests/TestUtils.cs +++ b/src/WingetCreateTests/WingetCreateTests/TestUtils.cs @@ -97,6 +97,23 @@ public static void SetMockHttpResponseContent(string installerName) PackageParser.SetHttpMessageHandler(httpMessageHandler); } + /// + /// Sets the mock http response content along with a server-supplied Content-Disposition filename. + /// + /// File name of the installer. + /// The filename value to advertise in the Content-Disposition header. + public static void SetMockHttpResponseContent(string installerName, string contentDispositionFileName) + { + var content = new ByteArrayContent(File.ReadAllBytes(GetTestFile(installerName))); + content.Headers.ContentType = new MediaTypeHeaderValue("application/octet-stream"); + content.Headers.ContentDisposition = new ContentDispositionHeaderValue("attachment") + { + FileName = contentDispositionFileName, + }; + httpResponseMessage.Content = content; + PackageParser.SetHttpMessageHandler(httpMessageHandler); + } + /// /// Obtains the relative filepath of the resources test data directory. /// diff --git a/src/WingetCreateTests/WingetCreateTests/UnitTests/PackageParserTests.cs b/src/WingetCreateTests/WingetCreateTests/UnitTests/PackageParserTests.cs index cc8575b1..ec8079ac 100644 --- a/src/WingetCreateTests/WingetCreateTests/UnitTests/PackageParserTests.cs +++ b/src/WingetCreateTests/WingetCreateTests/UnitTests/PackageParserTests.cs @@ -66,6 +66,37 @@ public void ParseExeInstallerFile() ClassicAssert.AreEqual(InstallerType.Exe, manifests.InstallerManifest.Installers.First().InstallerType); } + /// + /// Verifies that a server-supplied Content-Disposition filename containing directory + /// traversal sequences cannot cause the installer to be written outside the intended download + /// directory. + /// + [Test] + public void DownloadFileDispositionStaysInDownloadDirectory() + { + string downloadedPath = null; + try + { + string url = $"https://fakedomain.com/{TestConstants.TestExeInstaller}"; + string invalidFileName = @"..\..\..\..\example.exe"; + TestUtils.SetMockHttpResponseContent(TestConstants.TestExeInstaller, invalidFileName); + + downloadedPath = PackageParser.DownloadFileAsync(url, false).Result; + + string downloadRoot = Path.GetFullPath(PackageParser.InstallerDownloadPath).TrimEnd(Path.DirectorySeparatorChar, Path.AltDirectorySeparatorChar); + string fullDownloadedPath = Path.GetFullPath(downloadedPath); + Assert.That(fullDownloadedPath, Does.StartWith(downloadRoot + Path.DirectorySeparatorChar), "Downloaded file escaped the installer download directory."); + ClassicAssert.AreEqual("example.exe", Path.GetFileName(downloadedPath)); + } + finally + { + if (downloadedPath != null && File.Exists(downloadedPath)) + { + File.Delete(downloadedPath); + } + } + } + /// /// Downloads the MSI installer file from HTTPS localhost and parses the package to create a manifest object. /// From 0fbf6d9ff265dab42be6a8f5b27bbcd8acc3c5f9 Mon Sep 17 00:00:00 2001 From: AmirMS <104940545+AmelBawa-msft@users.noreply.github.com> Date: Tue, 21 Jul 2026 10:34:44 -0700 Subject: [PATCH 2/6] Fix UpdateZipWithMultipleNestedInstallers --- .../WingetCreateTests/TestUtils.cs | 23 ++++++++++- .../UnitTests/UpdateCommandTests.cs | 39 +++++++++++++------ 2 files changed, 50 insertions(+), 12 deletions(-) diff --git a/src/WingetCreateTests/WingetCreateTests/TestUtils.cs b/src/WingetCreateTests/WingetCreateTests/TestUtils.cs index 0fe143f0..4d36949e 100644 --- a/src/WingetCreateTests/WingetCreateTests/TestUtils.cs +++ b/src/WingetCreateTests/WingetCreateTests/TestUtils.cs @@ -187,6 +187,23 @@ public static List CreateResourceCopy(string resource, int numberOfCopie return copyPaths; } + /// + /// Deletes existing copies of the specified resource (base and numbered variants) from the test resources directory. + /// + /// Name of the resource file whose copies should be deleted. + public static void DeleteResourceCopies(string resourceName) + { + string resourcePath = GetTestFile(resourceName); + string directory = Path.GetDirectoryName(resourcePath); + string fileName = Path.GetFileNameWithoutExtension(resourcePath); + string fileExt = Path.GetExtension(resourcePath); + + foreach (string file in Directory.GetFiles(directory, fileName + "*" + fileExt)) + { + File.Delete(file); + } + } + /// /// Adds files to an existing test zip archive. /// @@ -230,7 +247,11 @@ public static void DeleteCachedFiles(List testFileNames) { foreach (string fileName in testFileNames) { - File.Delete(Path.Combine(PackageParser.InstallerDownloadPath, fileName)); + string filePath = Path.Combine(PackageParser.InstallerDownloadPath, fileName); + if (File.Exists(filePath)) + { + File.Delete(filePath); + } } } diff --git a/src/WingetCreateTests/WingetCreateTests/UnitTests/UpdateCommandTests.cs b/src/WingetCreateTests/WingetCreateTests/UnitTests/UpdateCommandTests.cs index 5a35975e..544581c7 100644 --- a/src/WingetCreateTests/WingetCreateTests/UnitTests/UpdateCommandTests.cs +++ b/src/WingetCreateTests/WingetCreateTests/UnitTests/UpdateCommandTests.cs @@ -1381,24 +1381,41 @@ public async Task UpdateMultipleZipInstallers() [Test] public async Task UpdateZipWithMultipleNestedInstallers() { + // Ensure a clean slate + TestUtils.DeleteResourceCopies(TestConstants.TestPortableInstaller); + // Create copies of test exe installer to be used as portable installers List portableFilePaths = TestUtils.CreateResourceCopy(TestConstants.TestExeInstaller, 4, TestConstants.TestPortableInstaller); - // Add the generated portable installers to the test zip installer - TestUtils.AddFilesToZip(TestConstants.TestZipInstaller, portableFilePaths); + Manifests updatedManifests; + List initialManifestContent; - // Delete cached zip installer from other test runs so that the modified zip installer is downloaded - TestUtils.DeleteCachedFiles(new List { TestConstants.TestZipInstaller }); + try + { + // Add the generated portable installers to the test zip installer + TestUtils.AddFilesToZip(TestConstants.TestZipInstaller, portableFilePaths); - TestUtils.InitializeMockDownloads(TestConstants.TestZipInstaller); - string installerUrl = $"https://fakedomain.com/{TestConstants.TestZipInstaller}"; - (UpdateCommand command, var initialManifestContent) = GetUpdateCommandAndManifestData("TestPublisher.ZipMultipleNestedInstallers", null, this.tempPath, new[] { $"{installerUrl}|x64", $"{installerUrl}|x86", $"{installerUrl}|arm", $"{installerUrl}|arm64|user", $"{installerUrl}|arm64|machine" }); + // Delete cached zip installer from other test runs so that the modified zip installer is downloaded + TestUtils.DeleteCachedFiles(new List { TestConstants.TestZipInstaller }); - var updatedManifests = await RunUpdateCommand(command, initialManifestContent); + TestUtils.InitializeMockDownloads(TestConstants.TestZipInstaller); + string installerUrl = $"https://fakedomain.com/{TestConstants.TestZipInstaller}"; + (UpdateCommand command, initialManifestContent) = GetUpdateCommandAndManifestData("TestPublisher.ZipMultipleNestedInstallers", null, this.tempPath, new[] { $"{installerUrl}|x64", $"{installerUrl}|x86", $"{installerUrl}|arm", $"{installerUrl}|arm64|user", $"{installerUrl}|arm64|machine" }); - // Perform test clean up before any assertions - portableFilePaths.ForEach(File.Delete); - TestUtils.RemoveFilesFromZip(TestConstants.TestZipInstaller, portableFilePaths.Select(Path.GetFileName).ToList()); + updatedManifests = await RunUpdateCommand(command, initialManifestContent); + } + finally + { + // Perform test clean up before any assertions + portableFilePaths.ForEach(path => + { + if (File.Exists(path)) + { + File.Delete(path); + } + }); + TestUtils.RemoveFilesFromZip(TestConstants.TestZipInstaller, portableFilePaths.Select(Path.GetFileName).ToList()); + } ClassicAssert.IsNotNull(updatedManifests, "Command should have succeeded"); From f0802ebd07900780a6ac2f5716c9a94593de70d4 Mon Sep 17 00:00:00 2001 From: AmirMS <104940545+AmelBawa-msft@users.noreply.github.com> Date: Tue, 21 Jul 2026 14:15:52 -0700 Subject: [PATCH 3/6] Fix UpdateZipWithMultipleNestedInstallers --- src/WingetCreateTests/WingetCreateTests/TestUtils.cs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/WingetCreateTests/WingetCreateTests/TestUtils.cs b/src/WingetCreateTests/WingetCreateTests/TestUtils.cs index 4d36949e..3d05435a 100644 --- a/src/WingetCreateTests/WingetCreateTests/TestUtils.cs +++ b/src/WingetCreateTests/WingetCreateTests/TestUtils.cs @@ -245,10 +245,18 @@ public static void RemoveFilesFromZip(string zipResourceName, List fileN /// Name of the test files to delete. public static void DeleteCachedFiles(List testFileNames) { + string downloadDirectory = PackageParser.InstallerDownloadPath; + if (!Directory.Exists(downloadDirectory)) + { + return; + } + foreach (string fileName in testFileNames) { - string filePath = Path.Combine(PackageParser.InstallerDownloadPath, fileName); - if (File.Exists(filePath)) + string baseName = Path.GetFileNameWithoutExtension(fileName); + string fileExt = Path.GetExtension(fileName); + + foreach (string filePath in Directory.GetFiles(downloadDirectory, baseName + "*" + fileExt)) { File.Delete(filePath); } From c9ae7530d8d7d749d8c6734415f25542a3df1a2c Mon Sep 17 00:00:00 2001 From: AmirMS <104940545+AmelBawa-msft@users.noreply.github.com> Date: Tue, 21 Jul 2026 21:18:01 -0700 Subject: [PATCH 4/6] Use token --- pipelines/azure-pipelines.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pipelines/azure-pipelines.yml b/pipelines/azure-pipelines.yml index 2109e532..3e5bd7fe 100644 --- a/pipelines/azure-pipelines.yml +++ b/pipelines/azure-pipelines.yml @@ -144,7 +144,7 @@ extends: runSettingsFile: 'src\WingetCreateTests\WingetCreateTests\Test.runsettings' overrideTestrunParameters: '-WingetPkgsTestRepoOwner microsoft -WingetPkgsTestRepo winget-pkgs-submission-test' env: - WINGET_CREATE_APP_KEY: $(GitHubApp_PrivateKey) + WINGET_CREATE_GITHUB_TOKEN: $(GitHubPAT) - task: 1ES.PublishPipelineArtifact@1 inputs: From e4d0590afbc2348680b47b4f4e8e0ec8fa0f82b7 Mon Sep 17 00:00:00 2001 From: AmirMS <104940545+AmelBawa-msft@users.noreply.github.com> Date: Wed, 22 Jul 2026 14:19:55 -0700 Subject: [PATCH 5/6] Update app id --- pipelines/azure-pipelines.yml | 2 +- src/WingetCreateCore/Common/Constants.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pipelines/azure-pipelines.yml b/pipelines/azure-pipelines.yml index 3e5bd7fe..2109e532 100644 --- a/pipelines/azure-pipelines.yml +++ b/pipelines/azure-pipelines.yml @@ -144,7 +144,7 @@ extends: runSettingsFile: 'src\WingetCreateTests\WingetCreateTests\Test.runsettings' overrideTestrunParameters: '-WingetPkgsTestRepoOwner microsoft -WingetPkgsTestRepo winget-pkgs-submission-test' env: - WINGET_CREATE_GITHUB_TOKEN: $(GitHubPAT) + WINGET_CREATE_APP_KEY: $(GitHubApp_PrivateKey) - task: 1ES.PublishPipelineArtifact@1 inputs: diff --git a/src/WingetCreateCore/Common/Constants.cs b/src/WingetCreateCore/Common/Constants.cs index 4a6ca81a..6119c4fe 100644 --- a/src/WingetCreateCore/Common/Constants.cs +++ b/src/WingetCreateCore/Common/Constants.cs @@ -31,7 +31,7 @@ public static class Constants /// /// App Id for the Winget-Create GitHub App. /// - public const int GitHubAppId = 965520; + public const int GitHubAppId = 100205; /// /// Link to the GitHub releases page for the winget-create tool. From ffd6e884c3ad1368fadaafa3ff4d8236f8f0976d Mon Sep 17 00:00:00 2001 From: AmirMS <104940545+AmelBawa-msft@users.noreply.github.com> Date: Wed, 22 Jul 2026 15:27:38 -0700 Subject: [PATCH 6/6] Address copilot comments --- src/WingetCreateCore/Common/PackageParser.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/WingetCreateCore/Common/PackageParser.cs b/src/WingetCreateCore/Common/PackageParser.cs index 12f0ee8a..d64d617b 100644 --- a/src/WingetCreateCore/Common/PackageParser.cs +++ b/src/WingetCreateCore/Common/PackageParser.cs @@ -186,8 +186,8 @@ public static async Task DownloadFileAsync(string url, bool allowHttp, l Directory.CreateDirectory(InstallerDownloadPath); } - // If no relevant filename can be obtained for the installer download, use a temporary filename as last option. - string targetFileName = contentDispositionFile.NullIfEmpty() ?? urlFile.NullIfEmpty() ?? requestUrlFileName.NullIfEmpty() ?? Path.GetTempFileName(); + // If no relevant filename can be obtained for the installer download, use a random filename as last option. + string targetFileName = contentDispositionFile.NullIfEmpty() ?? urlFile.NullIfEmpty() ?? requestUrlFileName.NullIfEmpty() ?? Path.GetRandomFileName(); string targetFile = GetNumericFilename(Path.Combine(InstallerDownloadPath, targetFileName)); // Defense in depth: ensure the resolved path stays under the installer download directory.