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

[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.
  • Loading branch information
Madhusudhan-MSFT committed Oct 19, 2024
1 parent 9bfeedb commit edcfcc9
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 "TestSource1" "https:\\testsource1" --trust - level trusted

Check failure on line 60 in src/AppInstallerCLICore/Workflows/SourceFlow.cpp

View workflow job for this annotation

GitHub Actions / Check Spelling

`testsource` is not a recognized word. (unrecognized-spelling)
// winget source add "TestSource2" "https:\\testsource1" --trust - level trusted

Check failure on line 61 in src/AppInstallerCLICore/Workflows/SourceFlow.cpp

View workflow job for this annotation

GitHub Actions / Check Spelling

`testsource` is not a recognized word. (unrecognized-spelling)
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 edcfcc9

Please sign in to comment.