Search code, repositories, users, issues, pull requests...

Copy link

Contributor

@AmelBawa-msft AmelBawa-msft commented Aug 5, 2024 •

edited

Loading

Description

flowchart TD
 %%{init:{'flowchart':{"defaultRenderer": "elk"}}}%%
 A[winget cli] --> B
 B{is configuration?} --> |yes| C[Get URI Zone]
 B --> |no| D{Package catalog trusted?}
 D --> |no| C
 D --> |yes| E(Resume operation)
 C --> F{Zone blocked by policy?}
 F -->|yes| G(APPINSTALLER_CLI_ERROR_BLOCKED_BY_POLICY)
 F --> |no| H{Zone Internet or Untrusted?}
 H --> |yes| I{SmartScreen blocked by policy?}
 H --> |no| E
 I --> |no| J{Blocked by SmartScreen?}
 I --> |yes| E
 J --> |yes| K(APPINSTALLER_CLI_ERROR_BLOCKED_BY_REPUTATION_SERVICE)
 J --> |no| E
 style G fill:red,stroke:darkred,color:white
 style K fill:red,stroke:darkred,color:white
 style E fill:green,stroke:darkgreen,color:white

TODO

Finalize strings

Follow ups

Add MotW HostUrl validation (TODO link to issue)

Microsoft Reviewers: Open in CodeFlow

AmelBawa-msft added 6 commits

August 2, 2024 10:16


 Init URI validation

2b0915e


 Updated result

e7ea781


 empty space

80e3d07


 Misc enhancements

04be98c


 Updated admx/adml

d108187


 Validate untrusted zone

eb33c58

Trenly

Trenly reviewed

Aug 5, 2024

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp Outdated Show resolved Hide resolved

AmelBawa-msft commented

Aug 6, 2024

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp Outdated

auto isAllowed = configurationPolicies->at(zone);

if(!isAllowed)

{

context.Reporter.Error() << "Configuration is disabled for Zone: " << zone << std::endl;

Copy link

Contributor Author

@AmelBawa-msft AmelBawa-msft Aug 6, 2024

Choose a reason for hiding this comment

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

TODO:

Finalizing strings
Move to resource file

AmelBawa-msft commented

Aug 6, 2024

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp Outdated Show resolved Hide resolved

JohnMcPMS

JohnMcPMS reviewed

Aug 6, 2024

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp Outdated Show resolved Hide resolved

JohnMcPMS

JohnMcPMS reviewed

Aug 7, 2024

doc/admx/en-US/DesktopAppInstaller.adml Outdated

If you enable this setting, the specified proxy will be used by default.</string>

<string id="EnableDSCAllowedZones">Enable App Installer Allowed Zones for DSC</string>

Copy link

Member

@JohnMcPMS JohnMcPMS Aug 7, 2024

Choose a reason for hiding this comment

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

Waiting for PM input?

Copy link

Contributor Author

@AmelBawa-msft AmelBawa-msft Aug 9, 2024

Choose a reason for hiding this comment

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

Yes, but also, we're still thinking about the scope that this policy should cover (DSC only, or more ... )

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp Outdated

switch (response.Decision())

{

case AppInstaller::UriValidation::UriValidationDecision::Block:

context.Reporter.Error() << std::endl << "Blocked by smart screen" << std::endl << "Feedback: " << response.Feedback() << std::endl;

Copy link

Member

@JohnMcPMS JohnMcPMS Aug 7, 2024

Choose a reason for hiding this comment

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

If Feedback isn't localized, it should probably only be sent to the log and not stdout.

Copy link

Contributor Author

@AmelBawa-msft AmelBawa-msft Aug 9, 2024

Choose a reason for hiding this comment

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

All strings are not finalized/localized yet, there's an email thread for deciding on a couple things including localization

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp Outdated Show resolved Hide resolved

src/Internal/UriValidation/UriValidation.cpp Show resolved Hide resolved

src/Internal/UriValidation/pch.h Outdated Show resolved Hide resolved

Trenly

Trenly reviewed

Aug 8, 2024

https://learn.microsoft.com/en-us/defender-endpoint/web-protection-overview#order-of-precedence

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp Outdated Show resolved Hide resolved

src/AppInstallerSharedLib/Public/winget/GroupPolicy.h Outdated Show resolved Hide resolved

src/Internal/UriValidation/UriValidation.cpp Show resolved Hide resolved

src/Internal/UriValidation/UriValidation.h

// The decision made based on the Uri validation.

enum class UriValidationDecision

{

Allow,

Copy link

Contributor

@Trenly Trenly Aug 8, 2024

Choose a reason for hiding this comment

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

Isn't there also a Warn response that SmartScreen can return?

Copy link

Contributor Author

@AmelBawa-msft AmelBawa-msft Aug 9, 2024

Choose a reason for hiding this comment

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

You're right, there's a PUA response as well. However, PUA/Warn will also be treated as Blocked, but the details will be communicated in the logs instead and the logic will be in the injected code. I can potentially reduce this enum to a bool. I will revisit this later once things are a little bit more finalized.

src/Internal/UriValidation/UriValidation.h Show resolved Hide resolved

src/Internal/UriValidation/UriValidation.h Outdated

// Uri to give feedback to smart screen about the decision.

std::string m_feedback;

public:

UriValidationResult(UriValidationDecision decision) : m_decision(decision), m_feedback(std::string()) {}

Copy link

Contributor

@Trenly Trenly Aug 8, 2024

Choose a reason for hiding this comment

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

Should there be a default (enpty) constructor?

src/Internal/UriValidation/UriValidation.cpp Outdated Show resolved Hide resolved

src/Internal/UriValidation/UriValidation.vcxproj Outdated

</ItemGroup>

Copy link

Contributor

@Trenly Trenly Aug 8, 2024

Choose a reason for hiding this comment

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

This doesnt seem to match the version of cppwinrt in NOTICE.txt. Does the version need to be bumped across all the solution files, or is there something I’m missing?

winget-cli/NOTICE

Line 769 in 2306d08

Microsoft.Windows.CppWinRT 2.0.210503.1 - MIT

AmelBawa-msft added 18 commits

September 30, 2024 15:16


 Resolved conflicts

e369739


 Move URI validation to a new file

7b48281


 Addressing comments

d393bf1


 remove pch

92b9076


 Add URI validation to download

d2c9af3


 Resolved conflicts

3aa8415


 Updated TPV

c07480e


 Renaming

1b90b32


 Include dir for diff plat

7f25660


 Update vcxproj

29b0dc8


 Resolved conflict

b8e11fe


 rm package reference

8f04683


 Fix includes

caa13c2


 Create a task

0ac0ab4


 String resource

5ed92b6

CE

4b656d6


 Added GP

54b4e6b


 Added enum

1fb3c76

AmelBawa-msft added 4 commits

October 16, 2024 14:28


 Added condition

b5bdb1e


 Added tests

5f209d1


 Resolved conflict

865cb0a


 Fix UT

3aa97f4

@AmelBawa-msft AmelBawa-msft changed the title ~~(削除) Validate configuration Urls (削除ここまで)~~ (追記) URI validation for winget cli commands (追記ここまで)

Oct 17, 2024

AmelBawa-msft added 2 commits

October 18, 2024 10:04


 Fix UT

227511a


 Terminate after validation

d84cf21

@AmelBawa-msft AmelBawa-msft marked this pull request as ready for review

October 23, 2024 20:35

@AmelBawa-msft AmelBawa-msft requested a review from a team as a code owner

October 23, 2024 20:35


 Renaming

ca649fe

AmelBawa-msft commented

Oct 31, 2024

doc/admx/en-US/DesktopAppInstaller.adml Outdated

Comment on lines 125 to 128

<string id="EnableWindowsPackageManagerAllowedSecurityZones">Enable App Installer Allowed Zones for the Windows Package Manager</string>

<string id="EnableWindowsPackageManagerSmartScreenCheck">Enable Microsoft SmartScreen checks for the Windows Package Manager</string>

Copy link

Contributor Author

@AmelBawa-msft AmelBawa-msft Oct 31, 2024

Choose a reason for hiding this comment

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

Adding @RDMacLachlan to help with the strings for the new group policies in the ADML

AmelBawa-msft commented

Nov 1, 2024

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

<comment>{Locked="{0}"} Error message displayed when the provided uri is not well formed. {0} is a placeholder replaced by the provided uri.</comment>

</data>

<value>This operation was blocked as unsafe by Microsoft Defender SmartScreen.</value>

Copy link

Contributor Author

@AmelBawa-msft AmelBawa-msft Nov 1, 2024

Choose a reason for hiding this comment

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

Adding @RDMacLachlan to help with the displayed messages (I copied the messages from App Installer with some modifications)

AmelBawa-msft added 2 commits

November 1, 2024 10:52


 Update comment

1d6408c


 Resolve conflicts

70d600c

florelis

florelis reviewed

Nov 6, 2024

src/AppInstallerCLI.sln Outdated

Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "Microsoft.Management.Deployment.OutOfProc", "Microsoft.Management.Deployment.OutOfProc\Microsoft.Management.Deployment.OutOfProc.vcxproj", "{0BA531C8-CF0C-405B-8221-0FE51BA529D1}"

ProjectSection(ProjectDependencies) = postProject

{2B00D362-AC92-41F3-A8D2-5B1599BDCA01} = {2B00D362-AC92-41F3-A8D2-5B1599BDCA01}

EndProjectSection

EndProject

Global

GlobalSection(SolutionConfigurationPlatforms) = preSolution

Debug|ARM = Debug|ARM

Copy link

Member

@florelis florelis Nov 5, 2024

Choose a reason for hiding this comment

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

We're building for ARM now?

src/AppInstallerCLICore/AppInstallerCLICore.vcxproj Outdated Show resolved Hide resolved

src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters Show resolved Hide resolved

src/Internal/UriValidation/UriValidation.h

class UriValidationResult

{

public:

UriValidationResult(UriValidationDecision decision) : m_decision(decision), m_feedback(std::string()) {}

Copy link

Member

@florelis florelis Nov 5, 2024

Choose a reason for hiding this comment

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

I think you don't need to pass the empty string for m_feedback because that's what the default constructor does

Suggested change

UriValidationResult(UriValidationDecision decision) : m_decision(decision), m_feedback(std::string()) {}

UriValidationResult(UriValidationDecision decision) : m_decision(decision), m_feedback() {}

Or you could initialize it to empty in the declaration and not mention it here

src/Internal/UriValidation/UriValidation.h

{

public:

UriValidationResult(UriValidationDecision decision) : m_decision(decision), m_feedback(std::string()) {}

UriValidationResult(UriValidationDecision decision, std::string feedback) : m_decision(decision), m_feedback(feedback) {}

Copy link

Member

@florelis florelis Nov 5, 2024

Choose a reason for hiding this comment

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

feedback should probably be a std::string_view or a std::string&& to avoid a copy. Not that it would make much difference in this case

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp Outdated

Comment on lines 1304 to 1313

auto progressScope = context.Reporter.BeginAsyncProgress(true);

progressScope->Callback().SetProgressMessage(Resource::String::ConfigurationInitializing());

anon::ConfigureProcessorForUse(context, ConfigurationProcessor{ anon::CreateConfigurationSetProcessorFactory(context) });

}

void CreateConfigurationProcessor(Context& context)

{

context <<

ExecuteUriValidation(UriValidationSource::ConfigurationSource) <<

Copy link

Member

@florelis florelis Nov 6, 2024

Choose a reason for hiding this comment

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

Could ExecuteUriValidation happen after we set up the progress reporting and message? Just in case it takes a while

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

<comment>Describes a Group Policy that can enable the use of the --proxy option to set a proxy</comment>

</data>

<value>Enable Windows Package Manager smart screen validation</value>

Copy link

Member

@florelis florelis Nov 6, 2024

Choose a reason for hiding this comment

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

Suggested change

<value>Enable Windows Package Manager smart screen validation</value>

<value>Enable Windows Package Manager SmartScreen validation</value>

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

</data>

<value>Enable Windows App Installer Allowed Security Zones</value>

Copy link

Member

@florelis florelis Nov 6, 2024

Choose a reason for hiding this comment

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

Suggested change

<value>Enable Windows App Installer Allowed Security Zones</value>

<value>Enable Windows Package Manager Allowed Security Zones</value>

src/AppInstallerCLIPackage/Shared/Strings/en-us/winget.resw

<comment>Error message displayed when an operation is using a URI that was blocked by Microsoft Defender SmartScreen.</comment>

</data>

<value>The operation you are attempting to apply has been blocked by your administrator.</value>

Copy link

Member

@florelis florelis Nov 6, 2024

Choose a reason for hiding this comment

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

This message could probably be more specific to zones. I think we already have a generic "not allowed by policy" string

src/AppInstallerCLITests/WorkflowCommon.h

Comment on lines +16 to +17

#define REQUIRE_OUTPUT_HAS_LOC(_output_, _resource_) \

REQUIRE(_output_.str().find(Resource::LocString(_resource_).get()) != std::string::npos);

Copy link

Member

@florelis florelis Nov 6, 2024

Choose a reason for hiding this comment

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

I like this, we should use it everywhere we do this check

florelis

florelis reviewed

Nov 6, 2024

src/AppInstallerCLICore/Workflows/UriValidationFlow.cpp Outdated Show resolved Hide resolved

@florelis

Copy link

Member

florelis commented Nov 6, 2024

I think we should add a command line flag to bypass SmartScreen if it fails, and an admin setting to disable the SmartScreen checks without needing to use group policy.

AmelBawa-msft added 7 commits

January 30, 2025 13:17


 Resolved conflicts

cad9fe3


 Revert ARM in sln

b8614e1


 Addressing comments

cd83bfe


 Update group policy behavior

c49b9d2


 Added admin setting

205c582


 uri validation in source add flow

18a77f5