URI validation for winget cli commands by AmelBawa-msft · Pull Request #4707 · microsoft/winget-cli

AmelBawa-msft · 2024-08-05T17:23:13Z

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

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

AmelBawa-msft · 2024-08-06T22:25:34Z

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

+            auto isAllowed = configurationPolicies->at(zone);
+            if(!isAllowed)
+            {
+                context.Reporter.Error() << "Configuration is disabled for Zone: " << zone << std::endl;


TODO:

Finalizing strings

Move to resource file

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

JohnMcPMS · 2024-08-07T21:34:52Z

doc/admx/en-US/DesktopAppInstaller.adml


 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>
+      <string id="EnableDSCAllowedZonesExplanation"></string>


Waiting for PM input?

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

JohnMcPMS · 2024-08-07T21:39:52Z

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

+            switch (response.Decision())
+            {
+            case AppInstaller::UriValidation::UriValidationDecision::Block:
+                context.Reporter.Error() << std::endl << "Blocked by smart screen" << std::endl << "Feedback: " << response.Feedback() << std::endl;


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

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

src/Internal/UriValidation/UriValidation.cpp

src/Internal/UriValidation/pch.h

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

src/AppInstallerSharedLib/Public/winget/GroupPolicy.h

src/Internal/UriValidation/UriValidation.cpp

Trenly · 2024-08-08T04:06:37Z

src/Internal/UriValidation/UriValidation.h

+    // The decision made based on the Uri validation.
+    enum class UriValidationDecision
+    {
+        Allow,


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

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

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

Trenly · 2024-08-08T04:12:08Z

src/Internal/UriValidation/UriValidation.h

+        // 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()) {}


Should there be a default (enpty) constructor?

src/Internal/UriValidation/UriValidation.cpp

Trenly · 2024-08-08T04:25:34Z

src/Internal/UriValidation/UriValidation.vcxproj

+    <None Include="packages.config" Condition="Exists('$(MSBuildProjectDirectory)\packages.config')" />
+  </ItemGroup>
+  <ItemGroup>
+    <PackageReference Include="Microsoft.Windows.CppWinRT" Version="2.0.230706.1" TargetFramework="native" />


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 · 2024-10-31T23:58:13Z

doc/admx/en-US/DesktopAppInstaller.adml

+      <string id="EnableWindowsPackageManagerAllowedSecurityZones">Enable App Installer Allowed Zones for the Windows Package Manager</string>
+      <string id="EnableWindowsPackageManagerAllowedSecurityZonesExplanation"></string>
+      <string id="EnableWindowsPackageManagerSmartScreenCheck">Enable Microsoft SmartScreen checks for the Windows Package Manager</string>
+      <string id="EnableWindowsPackageManagerSmartScreenCheckExplanation"></string>


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

AmelBawa-msft · 2024-11-01T00:01:47Z

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>
+  <data name="UriBlockedBySmartScreen" xml:space="preserve">
+    <value>This operation was blocked as unsafe by Microsoft Defender SmartScreen.</value>


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

florelis · 2024-11-05T22:12:22Z

src/AppInstallerCLI.sln

 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


We're building for ARM now?

src/AppInstallerCLICore/AppInstallerCLICore.vcxproj

src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters

florelis · 2024-11-05T23:03:00Z

src/Internal/UriValidation/UriValidation.h

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


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

florelis · 2024-11-05T23:04:11Z

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) {}


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

florelis · 2024-11-06T00:17:02Z

src/AppInstallerCLICore/Workflows/ConfigurationFlow.cpp

        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) <<


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

florelis · 2024-11-06T00:18:27Z

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>
+  <data name="PolicyEnableSmartScreenValidation" xml:space="preserve">
+    <value>Enable Windows Package Manager smart screen validation</value>


Suggested change

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

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

florelis · 2024-11-06T00:18:49Z

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

+    <comment></comment>
+  </data>
+  <data name="PolicyEnableAllowedSecurityZones" xml:space="preserve">
+    <value>Enable Windows App Installer Allowed Security Zones</value>


Suggested change

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

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

florelis · 2024-11-06T00:19:33Z

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>
+  <data name="UriSecurityZoneBlockedByPolicy" xml:space="preserve">
+    <value>The operation you are attempting to apply has been blocked by your administrator.</value>


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

florelis · 2024-11-06T00:21:40Z

src/AppInstallerCLITests/WorkflowCommon.h

+#define REQUIRE_OUTPUT_HAS_LOC(_output_, _resource_) \
+    REQUIRE(_output_.str().find(Resource::LocString(_resource_).get()) != std::string::npos);


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

src/AppInstallerCLICore/Workflows/UriValidationFlow.cpp

florelis · 2024-11-06T19:51:15Z

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 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 reviewed Aug 5, 2024

View reviewed changes

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

AmelBawa-msft commented Aug 6, 2024

View reviewed changes

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

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

JohnMcPMS reviewed Aug 6, 2024

View reviewed changes

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

JohnMcPMS reviewed Aug 7, 2024

View reviewed changes

Trenly reviewed Aug 8, 2024

View reviewed changes

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 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 marked this pull request as ready for review October 23, 2024 20:35

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

View reviewed changes

AmelBawa-msft commented Nov 1, 2024

View reviewed changes

AmelBawa-msft added 2 commits November 1, 2024 10:52

Update comment

1d6408c

Resolve conflicts

70d600c

florelis reviewed Nov 6, 2024

View reviewed changes

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

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

Updated group policy

e70a8d9

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

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

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

		#define REQUIRE_OUTPUT_HAS_LOC(_output_, _resource_) \
		REQUIRE(_output_.str().find(Resource::LocString(_resource_).get()) != std::string::npos);

Conversation

AmelBawa-msft commented Aug 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

TODO

Follow ups

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

florelis commented Nov 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

AmelBawa-msft commented Aug 5, 2024 •

edited

Loading