commit be131879e1385e571ad0f31c26b4f726d6240554
parent 9bfeedbce44be1d6a9c2597b85026b840d05cc30
Author: Madhusudhan-MSFT <53235553+Madhusudhan-MSFT@users.noreply.github.com>
Date: Mon, 21 Oct 2024 14:42:33 -0700
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:**
<!-- To check a checkbox place an "x" between the brackets. e.g: [x] -->
- [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)
Diffstat:
4 files changed, 39 insertions(+), 1 deletion(-)
diff --git 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
@@ -45,6 +45,9 @@ namespace AppInstallerCLIE2ETests
[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 @@ namespace AppInstallerCLIE2ETests
[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"));
@@ -104,6 +110,18 @@ namespace AppInstallerCLIE2ETests
}
/// <summary>
+ /// Test source add with duplicate source url.
+ /// </summary>
+ [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"));
+ }
+
+ /// <summary>
/// Test source add with invalid url.
/// </summary>
[Test]
diff --git 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<SourceDetails> 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
@@ -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<std::unique_ptr<ISourceFactory>()>&& factory)
{
