Skip to content

Commit

Permalink
Fix for Source Argument Validation in SourceWorkflow for Default Sour…
Browse files Browse the repository at this point in the history
…ce Type (microsoft#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:**

![image](https://github.com/user-attachments/assets/591b3ec2-ae6c-42ac-8258-169596657c4a)

**After fix:**


![image](https://github.com/user-attachments/assets/2078b72a-d746-440c-a44e-2ec05c812e26)


<!-- 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)
  • Loading branch information
Madhusudhan-MSFT authored Oct 21, 2024
1 parent 9bfeedb commit be13187
Show file tree
Hide file tree
Showing 4 changed files with 39 additions and 1 deletion.
12 changes: 12 additions & 0 deletions src/AppInstallerCLICore/Workflows/SourceFlow.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
18 changes: 18 additions & 0 deletions src/AppInstallerCLIE2ETests/SourceCommand.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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"));
Expand All @@ -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"));
Expand Down Expand Up @@ -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"));
}

/// <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>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
7 changes: 6 additions & 1 deletion src/AppInstallerRepositoryCore/RepositorySource.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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 << "]");
Expand Down Expand Up @@ -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)
{
Expand Down

0 comments on commit be13187

Please sign in to comment.