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.

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

Add option -NoProxy to WebCmdlet #3447

Merged
merged 1 commit into from Apr 1, 2017

Conversation

Projects
None yet
7 participants
@TheFlyingCorpse
Contributor

TheFlyingCorpse commented Mar 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@msftclas

msftclas Mar 28, 2017

@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

msftclas commented Mar 28, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@SteveL-MSFT

SteveL-MSFT Mar 28, 2017

Member

Can you add a test? Thanks!

Member

SteveL-MSFT commented Mar 28, 2017

Can you add a test? Thanks!

@SteveL-MSFT

Please add a test

@TheFlyingCorpse

This comment has been minimized.

Show comment
Hide comment
@TheFlyingCorpse

TheFlyingCorpse Mar 28, 2017

Contributor

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
Contributor

TheFlyingCorpse commented Mar 28, 2017

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

This comment has been minimized.

Show comment
Hide comment
@iSazonov

iSazonov Mar 29, 2017

Collaborator

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
Collaborator

iSazonov commented Mar 29, 2017

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

This comment has been minimized.

Show comment
Hide comment
@TheFlyingCorpse

TheFlyingCorpse Mar 29, 2017

Contributor

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.

Contributor

TheFlyingCorpse commented Mar 29, 2017

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.

@TheFlyingCorpse

This comment has been minimized.

Show comment
Hide comment
@TheFlyingCorpse

TheFlyingCorpse Mar 29, 2017

Contributor

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.

Contributor

TheFlyingCorpse commented Mar 29, 2017

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 👍

@TheFlyingCorpse

This comment has been minimized.

Show comment
Hide comment
@TheFlyingCorpse

TheFlyingCorpse Mar 31, 2017

Contributor

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

Contributor

TheFlyingCorpse commented Mar 31, 2017

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

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment