Add option -NoProxy to WebCmdlet #3447

TheFlyingCorpse · 2017-03-28T19:23:29Z

This commit adds a -NoProxy parameter to the WebCmdlet, used by both Invoke-WebRequest and Invoke-RestMethod. The fix is intended to allow for bypassing a DefaultWebProxy, such as a system configured one for direct requests.

References #3418

I did not figure out where I can update the help files relevant to this, I'd appreciate being pointed in the right direction for this.

msftclas · 2017-03-28T19:23:33Z

@TheFlyingCorpse,
Thanks for having already signed the Contribution License Agreement. Your agreement was validated by Microsoft. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

SteveL-MSFT · 2017-03-28T20:00:32Z

Can you add a test? Thanks!

SteveL-MSFT

Please add a test

lzybkr · 2017-03-28T20:25:43Z

...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

+        /// gets or sets the NoProxy property
+        /// </summary>
+        [Parameter]
+        public virtual SwitchParameter NoProxy { get; set; }


Is there a good reason to make the property/parameter virtual?

To be honest, I copied what was already used in that file for the other switches present.

Should -noproxy and -proxy* parameters be in different parametersets?

That sounds like a good idea, I'll add a few more tests and make that change.

It seems that it is better not to do different parameter sets because we will not be able to do:

function Foo($NoProxy) { Invoke-WebRequest -NoProxy:$NoProxy -Proxy "http://proxy.server.com" }

For consistency, it seems like these should be mutually exclusive (thus different parametersets). PowerShell doesn't work like Unix cmdlines where last one wins (in which case, the above example would be -Proxy overrides -NoProxy despite $true or $false). I've typically used splatting to construct parameters dynamically. I think if we want something more terse, we should introduce a more general approach as a language construct.

Point taken, updating now.

I'm confused about the switch, all the more so in a separate parameter set. I would prefer not to complicate and just make -Proxy $null

TheFlyingCorpse · 2017-03-28T20:40:15Z

Tests added, if you want me to, I can add scenarios to verify it fails when proxy is defined to the made up proxy as a countermeasure to -NoProxy.
I did not add a test for the following two scenarios, I am not certain if they are in or out of scope:

Internet Explorer proxy defined, it is however how I developed the -NoProxy addition.
Proxy defined via "netsh winhttp", in the testing I did by hand this did not have any effect on Invoke-WebRequest or Invoke-RestMethod

iSazonov · 2017-03-29T12:29:15Z

Probably enough:

Invoke-WebRequest -Uri http://httpbin.org/headers -Proxy http://httpbin.org -NoProxy   - return expected result
Invoke-WebRequest -Uri http://httpbin.org/headers -Proxy http://httpbin.org -NoProxy:$false - return empty content

TheFlyingCorpse · 2017-03-29T15:48:12Z

Updated tests quite a bit.

I did not split the parameters up to different parametersets as @iSazonov made a great case for why it ought not to be split up.

SteveL-MSFT · 2017-03-29T16:28:38Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+    It "Validate Invoke-WebRequest error for -MaximumRedirection with Environment http_proxy set" {
+
+        # Store the current proxy.
+        $currentProxy = [System.Environment]::GetEnvironmentVariable("http_proxy")


I think it may be cleaner to put storing and restoring of http_proxy (if defined) in BeforeEach{} and AfterEach{} blocks rather than having repeated code.

SteveL-MSFT · 2017-03-29T16:30:13Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+
+        # Store the current proxy.
+        $currentProxy = [System.Environment]::GetEnvironmentVariable("http_proxy")
+        [System.Environment]::SetEnvironmentVariable("http_proxy","http://localhost:8080")


You should be able to rely on:

$savedProxy = $env:http_proxy ... $env:http_proxy = "http://localhost:8080" ... $env:http_proxy = $savedProxy

SteveL-MSFT · 2017-03-29T16:34:09Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -247,7 +247,115 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {

        $result = ExecuteWebCommand -command $command
        $result.Error.FullyQualifiedErrorId | Should Be "WebCmdletWebResponseException,Microsoft.PowerShell.Commands.InvokeWebRequestCommand"
+    }
+
+    It "Validate Invoke-WebRequest returns User-Agent where -NoProxy with -Proxy set" {


If we agree that -NoProxy and -Proxy are mutually exclusive, then this test should validate you get an AmbiguousParameterSet,Microsoft.PowerShell.Commands.NewPSSessionCommand exception

SteveL-MSFT · 2017-03-29T16:37:17Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+        $command = "Invoke-WebRequest -Uri 'http://httpbin.org/redirect/3' -MaximumRedirection 2 -TimeoutSec 5"
+
+        $result = ExecuteWebCommand -command $command
+        $result.Error.FullyQualifiedErrorId | Should Be "WebCmdletWebResponseException,Microsoft.PowerShell.Commands.InvokeWebRequestCommand"


Not clear to me what this test is intended to validate. Since localhost:8080 is not a proxy, no redirection will actually happen and the error returned is that it can't contact the proxy.

SteveL-MSFT · 2017-03-29T16:37:53Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+        [System.Environment]::SetEnvironmentVariable("http_proxy",$currentProxy)
+    }
+
+    It "Validate Invoke-WebRequest error for -MaximumRedirection with Environment https_proxy set" {


Same as above, not clear to me why -MaximumRedirection is interesting here as a test with invalid proxy

TheFlyingCorpse · 2017-03-29T21:14:00Z

Updated, incorporated ParameterSetName's and updated the tests to reflect this change.

To be able to make the -NoProxy not use -Proxy, I had to modify a bit more than expected since there already was defined two ParameterSets: StandardMethod and CustomMethod, there are now two extra that redirects to the existing one. Without the change to this, the method would never get since the new names introduced would break this. The method would break on both use of -Proxy and -NoProxy since the switch using ParameterSetName would not match either existing Method.

joeyaiello

Given all the discussion in #3418, I'm explicitly giving this one my 👍

SteveL-MSFT · 2017-03-30T23:30:33Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+
+    It "Validate Invoke-WebRequest returns User-Agent where -NoProxy with Environment http_proxy Set" {
+
+        $env:http_proxy="http://localhost:8080"


the http and https tests are essentially the same, I think it may be better to use Pester's TestCase feature. See a sample here:

PowerShell/test/powershell/Language/Parser/LanguageAndParser.TestFollowup.Tests.ps1

Line 147 in 02b5f35

$testCase = @(

TheFlyingCorpse · 2017-03-31T19:03:32Z

Note: I didn't find a good way with the mock to have a dynamic proxy when using $env to assign it inside the function when using parameters, if the workaround which uses "pure" PowerShell is not accepted, let me know what the alternative is :-)

msftclas added the cla-already-signed label Mar 28, 2017

SteveL-MSFT requested changes Mar 28, 2017

View reviewed changes

lzybkr reviewed Mar 28, 2017

View reviewed changes

TheFlyingCorpse force-pushed the feature/webcmdlet-add-noproxy-switch branch from 7bcc221 to af53884 Compare March 28, 2017 20:35

TheFlyingCorpse force-pushed the feature/webcmdlet-add-noproxy-switch branch from af53884 to 1d9f2de Compare March 29, 2017 15:47

SteveL-MSFT requested changes Mar 29, 2017

View reviewed changes

TheFlyingCorpse force-pushed the feature/webcmdlet-add-noproxy-switch branch from 1d9f2de to 4da3e63 Compare March 29, 2017 21:13

daxian-dbw assigned mirichmo Mar 30, 2017

joeyaiello mentioned this pull request Mar 30, 2017

Add -NoProxy parameter to Invoke-WebRequest #3418

Closed

joeyaiello approved these changes Mar 30, 2017

View reviewed changes

SteveL-MSFT requested changes Mar 30, 2017

View reviewed changes

Add option -NoProxy to WebCmdlet

1d31ab4

TheFlyingCorpse force-pushed the feature/webcmdlet-add-noproxy-switch branch from 4da3e63 to 1d31ab4 Compare March 31, 2017 19:05

SteveL-MSFT approved these changes Mar 31, 2017

View reviewed changes

mirichmo merged commit a770ecd into PowerShell:master Apr 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option -NoProxy to WebCmdlet #3447

Add option -NoProxy to WebCmdlet #3447

TheFlyingCorpse commented Mar 28, 2017

msftclas commented Mar 28, 2017

SteveL-MSFT commented Mar 28, 2017

SteveL-MSFT left a comment

lzybkr Mar 28, 2017

TheFlyingCorpse Mar 28, 2017

SteveL-MSFT Mar 28, 2017

TheFlyingCorpse Mar 28, 2017

iSazonov Mar 29, 2017

SteveL-MSFT Mar 29, 2017

TheFlyingCorpse Mar 29, 2017

iSazonov Mar 29, 2017

TheFlyingCorpse commented Mar 28, 2017 •

edited

Loading

iSazonov commented Mar 29, 2017

TheFlyingCorpse commented Mar 29, 2017 •

edited

Loading

SteveL-MSFT Mar 29, 2017

SteveL-MSFT Mar 29, 2017

SteveL-MSFT Mar 29, 2017

SteveL-MSFT Mar 29, 2017

SteveL-MSFT Mar 29, 2017

TheFlyingCorpse commented Mar 29, 2017 •

edited

Loading

joeyaiello left a comment

SteveL-MSFT Mar 30, 2017

TheFlyingCorpse commented Mar 31, 2017


		It "Validate Invoke-WebRequest returns User-Agent where -NoProxy with Environment http_proxy Set" {

		$env:http_proxy="http://localhost:8080"

Add option -NoProxy to WebCmdlet #3447

Add option -NoProxy to WebCmdlet #3447

Conversation

TheFlyingCorpse commented Mar 28, 2017

msftclas commented Mar 28, 2017

SteveL-MSFT commented Mar 28, 2017

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheFlyingCorpse commented Mar 28, 2017 • edited Loading

iSazonov commented Mar 29, 2017

TheFlyingCorpse commented Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheFlyingCorpse commented Mar 29, 2017 • edited Loading

joeyaiello left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheFlyingCorpse commented Mar 31, 2017

TheFlyingCorpse commented Mar 28, 2017 •

edited

Loading

TheFlyingCorpse commented Mar 29, 2017 •

edited

Loading

TheFlyingCorpse commented Mar 29, 2017 •

edited

Loading