Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require selection argument on install/show/search/uninstall #2125

Merged
merged 2 commits into from
Apr 27, 2022
Merged

Require selection argument on install/show/search/uninstall #2125

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0a7408d
Prevent install/show/search/uninstall from being used without some so…
JohnMcPMS Apr 26, 2022
3217bff
Update E2E tests
JohnMcPMS Apr 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions src/AppInstallerCLICore/Argument.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.
#include "pch.h"
#include "Argument.h"
#include "Command.h"
#include "Resources.h"
#include <winget/UserSettings.h>

Expand Down Expand Up @@ -108,6 +109,19 @@ namespace AppInstaller::CLI
args.push_back(ForType(Args::Type::VerboseLogs));
}

void Argument::ValidatePackageSelectionArgumentSupplied(const Execution::Args& args)
{
for (Args::Type type : { Args::Type::Query, Args::Type::Manifest, Args::Type::Id, Args::Type::Name, Args::Type::Moniker, Args::Type::Tag, Args::Type::Command })
{
if (args.Contains(type))
{
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to ensure the query is also not an empty string?
#1249

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be possible, but allowing an empty string does allow for an effective --all being "". The current change prevents accidental "everything" searches, which I feel was the primary value. Preventing intentional searching for everything seems like it would be due to the current performance limitations rather than due to protecting users.

If the user types in winget search "", it could be an accident, but it is more likely that it is intentional than winget search. I could see scripts constructing the command line resulting in more frequent empty strings as well, but if the script author wants to block that, they can.

Without any other reasons, I'm not inclined to block the empty strings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those are fair points, and are things I hadn't considered. After thinking more, I do agree that simply requiring an argument solves the potential issues here

}
}

throw CommandException(Resource::String::NoPackageSelectionArgumentProvided);
}

Argument::Visibility Argument::GetVisibility() const
{
if (!ExperimentalFeature::IsEnabled(m_feature))
Expand Down
5 changes: 5 additions & 0 deletions src/AppInstallerCLICore/Argument.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ namespace AppInstaller::CLI
// Gets the common arguments for all commands.
static void GetCommon(std::vector<Argument>& args);

// Static argument validation helpers; throw CommandException when validation fails.

// Requires that some form of package selection argument is present
static void ValidatePackageSelectionArgumentSupplied(const Execution::Args& args);

// Arguments are not localized at this time.
Utility::LocIndView Name() const { return Utility::LocIndView{ m_name }; }
char Alias() const { return m_alias; }
Expand Down
4 changes: 3 additions & 1 deletion src/AppInstallerCLICore/Commands/InstallCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ namespace AppInstaller::CLI

void InstallCommand::ValidateArgumentsInternal(Args& execArgs) const
{
Argument::ValidatePackageSelectionArgumentSupplied(execArgs);

if (execArgs.Contains(Args::Type::Manifest) &&
(execArgs.Contains(Args::Type::Query) ||
execArgs.Contains(Args::Type::Id) ||
Expand All @@ -115,7 +117,7 @@ namespace AppInstaller::CLI
}
if (execArgs.Contains(Args::Type::InstallArchitecture))
{
Utility::Architecture selectedArch = Utility::ConvertToArchitectureEnum(std::string(execArgs.GetArg(Args::Type::InstallArchitecture)));
Utility::Architecture selectedArch = Utility::ConvertToArchitectureEnum(std::string(execArgs.GetArg(Args::Type::InstallArchitecture)));
if ((selectedArch == Utility::Architecture::Unknown) || (Utility::IsApplicableArchitecture(selectedArch) == Utility::InapplicableArchitecture))
{
std::vector<Utility::LocIndString> applicableArchitectures;
Expand Down
5 changes: 5 additions & 0 deletions src/AppInstallerCLICore/Commands/SearchCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,11 @@ namespace AppInstaller::CLI
return "https://aka.ms/winget-command-search";
}

void SearchCommand::ValidateArgumentsInternal(Args& execArgs) const
{
Argument::ValidatePackageSelectionArgumentSupplied(execArgs);
}

void SearchCommand::ExecuteInternal(Context& context) const
{
context.SetFlags(Execution::ContextFlag::TreatSourceFailuresAsWarning);
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Commands/SearchCommand.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace AppInstaller::CLI
std::string HelpLink() const override;

protected:
void ValidateArgumentsInternal(Execution::Args& execArgs) const override;
void ExecuteInternal(Execution::Context& context) const override;
};
}
20 changes: 20 additions & 0 deletions src/AppInstallerCLICore/Commands/ShowCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
#include "Workflows/WorkflowBase.h"
#include "Resources.h"

using namespace AppInstaller::CLI::Execution;

namespace AppInstaller::CLI
{
std::vector<Argument> ShowCommand::GetArguments() const
Expand Down Expand Up @@ -49,6 +51,24 @@ namespace AppInstaller::CLI
return "https://aka.ms/winget-command-show";
}

void ShowCommand::ValidateArgumentsInternal(Args& execArgs) const
{
Argument::ValidatePackageSelectionArgumentSupplied(execArgs);

if (execArgs.Contains(Args::Type::Manifest) &&
(execArgs.Contains(Args::Type::Query) ||
execArgs.Contains(Args::Type::Id) ||
execArgs.Contains(Args::Type::Name) ||
execArgs.Contains(Args::Type::Moniker) ||
execArgs.Contains(Args::Type::Version) ||
execArgs.Contains(Args::Type::Channel) ||
execArgs.Contains(Args::Type::Source) ||
execArgs.Contains(Args::Type::Exact)))
{
throw CommandException(Resource::String::BothManifestAndSearchQueryProvided);
}
}

void ShowCommand::ExecuteInternal(Execution::Context& context) const
{
context.SetFlags(Execution::ContextFlag::TreatSourceFailuresAsWarning);
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Commands/ShowCommand.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ namespace AppInstaller::CLI
std::string HelpLink() const override;

protected:
void ValidateArgumentsInternal(Execution::Args& execArgs) const override;
void ExecuteInternal(AppInstaller::CLI::Execution::Context& context) const override;
};
}
2 changes: 2 additions & 0 deletions src/AppInstallerCLICore/Commands/UninstallCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ namespace AppInstaller::CLI

void UninstallCommand::ValidateArgumentsInternal(Execution::Args& execArgs) const
{
Argument::ValidatePackageSelectionArgumentSupplied(execArgs);

if (execArgs.Contains(Execution::Args::Type::Manifest) &&
(execArgs.Contains(Execution::Args::Type::Query) ||
execArgs.Contains(Execution::Args::Type::Id) ||
Expand Down
1 change: 1 addition & 0 deletions src/AppInstallerCLICore/Resources.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@ namespace AppInstaller::CLI::Resource
WINGET_DEFINE_RESOURCE_STRINGID(NoExperimentalFeaturesMessage);
WINGET_DEFINE_RESOURCE_STRINGID(NoInstalledPackageFound);
WINGET_DEFINE_RESOURCE_STRINGID(NoPackageFound);
WINGET_DEFINE_RESOURCE_STRINGID(NoPackageSelectionArgumentProvided);
WINGET_DEFINE_RESOURCE_STRINGID(NoPackagesFoundInImportFile);
WINGET_DEFINE_RESOURCE_STRINGID(NoUninstallInfoFound);
WINGET_DEFINE_RESOURCE_STRINGID(NoVTArgumentDescription);
Expand Down
3 changes: 3 additions & 0 deletions src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1287,4 +1287,7 @@ Please specify one of them using the `--source` option to proceed.</value>
<data name="WaitArgumentDescription" xml:space="preserve">
<value>Prompts the user to press any key before exiting</value>
</data>
<data name="NoPackageSelectionArgumentProvided" xml:space="preserve">
<value>No package selection argument was provided; see the help for details about finding a package.</value>
</data>
</root>