From be131879e1385e571ad0f31c26b4f726d6240554 Mon Sep 17 00:00:00 2001
From: Madhusudhan-MSFT <53235553+Madhusudhan-MSFT@users.noreply.github.com>
Date: Mon, 21 Oct 2024 14:42:33 -0700
Subject: [PATCH] Fix for Source Argument Validation in SourceWorkflow for
Default Source Type (#4891)
Fix for Source Argument Validation in SourceWorkflow for Default Source
Type
[Issues:] The current validation in the source flow correctly detects
duplicate source names. However, when the source argument is validated
along with the source type, it allows different source names with the
same arguments for the empty source type. This happens because, during
source type comparison, if the source type is not provided, it defaults
to an empty string, which is compared against the default type
(Microsoft.PreIndexed). This allows multiple different source names with
the same argument to be considered valid sources. For an empty source
type, the default source type is assigned during the source add
operation, not beforehand. Consequently, after the source add operation
is finished, the source will have some source arguments, but only the
name will differ.
[Fix:]
For an empty source type, we should compare against the default source
to prevent different source names with the same arguments for the
default types. During validation, we obtain the default type to replace
the empty source type and use it for comparison to validate argument
duplication.
- Extended source tests to validate the duplicate source argument
scenario for the default source type.
- Fixed additional failing source origin tests with appropriate fixes.
[How Validated:]
- Compiled the latest modifications and deployed the
AppInstallerCLIPackage.
- Executed CLI SourceTests to ensure all tests pass without issues.
**[Manual validation:]**
**Before fix:**
**After fix:**
- [x] I have signed the [Contributor License
Agreement](https://cla.opensource.microsoft.com/microsoft/winget-pkgs).
- [ ] This pull request is related to an issue.
-----
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/microsoft/winget-cli/pull/4891)
---
.../Workflows/SourceFlow.cpp | 12 ++++++++++++
src/AppInstallerCLIE2ETests/SourceCommand.cs | 18 ++++++++++++++++++
.../Public/winget/RepositorySource.h | 3 +++
.../RepositorySource.cpp | 7 ++++++-
4 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/src/AppInstallerCLICore/Workflows/SourceFlow.cpp b/src/AppInstallerCLICore/Workflows/SourceFlow.cpp
index a6a1dee6d4..7d8df61ffd 100644
--- a/src/AppInstallerCLICore/Workflows/SourceFlow.cpp
+++ b/src/AppInstallerCLICore/Workflows/SourceFlow.cpp
@@ -52,6 +52,18 @@ namespace AppInstaller::CLI::Workflow
std::string_view arg = context.Args.GetArg(Args::Type::SourceArg);
std::string_view type = context.Args.GetArg(Args::Type::SourceType);
+ // In the absence of a specified type, the default is Microsoft.PreIndexed.Package for comparison.
+ // The default type assignment to the source takes place during the add operation (Source::Add in Repository.cpp).
+ // This is necessary for the comparison to function correctly; otherwise, it would allow the addition of multiple
+ // sources with different names but the same argument for all default type cases.
+ // For example, the following commands would be allowed, but they acts as different alias to same source:
+ // winget source add "mysource1" "https:\\mysource" --trust - level trusted
+ // winget source add "mysource2" "https:\\mysource" --trust - level trusted
+ if (type.empty())
+ {
+ type = Repository::Source::GetDefaultSourceType();
+ }
+
for (const auto& details : sourceList)
{
if (Utility::ICUCaseInsensitiveEquals(details.Name, name))
diff --git a/src/AppInstallerCLIE2ETests/SourceCommand.cs b/src/AppInstallerCLIE2ETests/SourceCommand.cs
index 27ee083e13..c873a6d72b 100644
--- a/src/AppInstallerCLIE2ETests/SourceCommand.cs
+++ b/src/AppInstallerCLIE2ETests/SourceCommand.cs
@@ -45,6 +45,9 @@ public void SourceAdd()
[Test]
public void SourceAddWithTrustLevel()
{
+ // Remove the test source.
+ TestCommon.RunAICLICommand("source remove", Constants.TestSourceName);
+
var result = TestCommon.RunAICLICommand("source add", $"SourceTest {Constants.TestSourceUrl} --trust-level trusted");
Assert.AreEqual(Constants.ErrorCode.S_OK, result.ExitCode);
Assert.True(result.StdOut.Contains("Done"));
@@ -62,6 +65,9 @@ public void SourceAddWithTrustLevel()
[Test]
public void SourceAddWithStoreOriginTrustLevel()
{
+ // Remove the test source.
+ TestCommon.RunAICLICommand("source remove", Constants.TestSourceName);
+
var result = TestCommon.RunAICLICommand("source add", $"SourceTest {Constants.TestSourceUrl} --trust-level storeOrigin");
Assert.AreEqual(Constants.ErrorCode.ERROR_SOURCE_DATA_INTEGRITY_FAILURE, result.ExitCode);
Assert.True(result.StdOut.Contains("The source data is corrupted or tampered"));
@@ -103,6 +109,18 @@ public void SourceAddWithDuplicateName()
Assert.True(result.StdOut.Contains("A source with the given name already exists and refers to a different location"));
}
+ ///
+ /// Test source add with duplicate source url.
+ ///
+ [Test]
+ public void SourceAddWithDuplicateSourceUrl()
+ {
+ // Add source with duplicate url should fail
+ var result = TestCommon.RunAICLICommand("source add", $"TestSource2 {Constants.TestSourceUrl}");
+ Assert.AreEqual(Constants.ErrorCode.ERROR_SOURCE_ARG_ALREADY_EXISTS, result.ExitCode);
+ Assert.True(result.StdOut.Contains("A source with a different name already refers to this location"));
+ }
+
///
/// Test source add with invalid url.
///
diff --git a/src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h b/src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h
index 3bdcdb2822..c01a1611f9 100644
--- a/src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h
+++ b/src/AppInstallerRepositoryCore/Public/winget/RepositorySource.h
@@ -335,6 +335,9 @@ namespace AppInstaller::Repository
// Get a list of all available SourceDetails.
static std::vector GetCurrentSources();
+ // Get a default source type is the source type used when adding a source without specifying a type.
+ static std::string_view GetDefaultSourceType();
+
private:
void InitializeSourceReference(std::string_view name);
diff --git a/src/AppInstallerRepositoryCore/RepositorySource.cpp b/src/AppInstallerRepositoryCore/RepositorySource.cpp
index 5c279f4de0..d178b6890d 100644
--- a/src/AppInstallerRepositoryCore/RepositorySource.cpp
+++ b/src/AppInstallerRepositoryCore/RepositorySource.cpp
@@ -880,7 +880,7 @@ namespace AppInstaller::Repository
// AddSourceForDetails will also check for empty, but we need the actual type before that for validation.
if (sourceDetails.Type.empty())
{
- sourceDetails.Type = ISourceFactory::GetForType("")->TypeName();
+ sourceDetails.Type = GetDefaultSourceType();
}
AICLI_LOG(Repo, Info, << "Adding source: Name[" << sourceDetails.Name << "], Type[" << sourceDetails.Type << "], Arg[" << sourceDetails.Arg << "]");
@@ -1040,6 +1040,11 @@ namespace AppInstaller::Repository
}
}
+ std::string_view Source::GetDefaultSourceType()
+ {
+ return ISourceFactory::GetForType("")->TypeName();
+ }
+
#ifndef AICLI_DISABLE_TEST_HOOKS
void TestHook_SetSourceFactoryOverride(const std::string& type, std::function()>&& factory)
{