Add -SkipHeaderValidation switch to Invoke-WebRequest and Invoke-RestMethod to support adding headers without validating the header value. #4085

dantraMSFT · 2017-06-22T22:58:12Z

Some sites require header values that do not conform to strict validation in the CoreCLR's HttpRequestMessage.Headers collection causing calls to Invoke-WebRequest and Invoke-RestMethod to fail with a FormatException.

This change adds a -SkipHeaderValidation switch that allows the headers to be added without validation.

See PR MicrosoftDocs/PowerShell-Docs#1387 for the associated doc change.

SteveL-MSFT · 2017-06-23T00:54:37Z

You need to rebase and only include the commits for this PR

dantraMSFT · 2017-06-23T16:19:14Z

Rebased.

SteveL-MSFT · 2017-06-23T17:26:47Z

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

@@ -957,7 +1029,7 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {
    ## 'HttpClientHandler.ServerCertificateCustomValidationCallback' currently doesn't work in netcoreapp2.0 on Mac at all.
    ## This is tracked by powershell issue #3648.
    It "Validate Invoke-RestMethod -SkipCertificateCheck" -Pending:$IsOSX {
-
+        


Looks like you accidentally added whitespace

I updated the file and removed all trailing whitespace per the using-vscode.md to get the file to a 'clean' state.
note that when I diff my version against powershell/master, the only whitespace difference I see is at line 172 where trailing whitespace was removed. I don't see any of the other whitespace differences that the online diff viewer is reporting.

Local and online diff sometimes surprise us.
Here may be a real difference in 'rn' and ``n`.

SteveL-MSFT · 2017-06-23T17:28:53Z

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

+        $response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8080/PowerShell?test=echo" -headers $headers
+
+        $response.Error | Should Not BeNullOrEmpty
+        $response.Error.FullyQualifiedErrorId | Should Be "System.FormatException,Microsoft.PowerShell.Commands.InvokeWebRequestCommand"


Use the ShouldBeErrorId pattern https://github.com/PowerShell/PowerShell/search?utf8=%E2%9C%93&q=ShouldBeErrorId&type=

I can't use ShouldBeErrorId for those statements; it expects a ScriptBlock to execute and throw an exception.

SteveL-MSFT · 2017-06-23T17:29:08Z

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

+        $response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8081/PowerShell?test=echo" -headers $headers -Cmdlet "Invoke-RestMethod"
+
+        $response.Error | Should Not BeNullOrEmpty
+        $response.Error.FullyQualifiedErrorId | Should Be "System.FormatException,Microsoft.PowerShell.Commands.InvokeRestMethodCommand"


Use the ShouldBeErrorId pattern

ShouldBeErrorId does not work here; it expects a script block and then catches the exception to verify the error id.

SteveL-MSFT · 2017-06-23T17:30:12Z

test/tools/Modules/HttpListener/HttpListener.psm1

@@ -134,6 +134,13 @@ Function Start-HTTPListener {
                            $contentType = $queryItems["contenttype"]
                            $output = $queryItems["output"]
                        }
+
+                        # Echo the request as the output.
+                        "echo"


"echo" doesn't seem to be necessary as "response" basically does the same thing. I would suggest updating "response" if you want json (perhaps based on "contenttype"

Changing "response" to use JSON for the output breaks many tests. I will update the comment for echo but won't change response at this time.

I'm not suggesting always return json. "response" takes a contenttype argument you can use.

I think it makes more sense to have a separate 'echo' test. The returned data for response would be very different; it currently doesn't echo the request, and using contentype to control that doesn't make sense to me.

dantraMSFT · 2017-07-05T16:26:53Z

@TravisEz13 please merge this PR.

TravisEz13 · 2017-07-07T21:55:22Z

@dantraMSFT Please resolve merge conflicts

…Method to support adding headers without validating the header value.

dantraMSFT · 2017-07-10T17:19:35Z

@TravisEz13: Conflicts resolved.

TravisEz13 · 2017-07-11T18:37:11Z

TravisEz13 dismissed SteveL-MSFT’s stale review

July 11, 2017 18:37

PR has change

@dantraMSFT You used an "email address" to commit which is not associated with your GitHub account. Please add the association.
@SteveL-MSFT I'm removing your approval since the PR has changed.

@SteveL-MSFT, please reapprove this PR.

dantraMSFT · 2017-07-12T23:06:55Z

@TravisEz13: Can you look at this? Steve is out until next week.

iSazonov · 2017-07-13T05:49:47Z

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

+        $headers = @{"If-Match" = "*"}
+        $response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8080/PowerShell?test=echo" -headers $headers
+
+        $response.Error | Should BeNullOrEmpty


We can remove this - next line makes the check.

The check is intentional. Checking for $result.Error provides a better failure id if the underlying request fails versus the NullReferenceException that would occur in the next statement.

I believe NullReferenceException would occur only if we call a method but we call a property.

You're right but the end result does not change. If the request does not complete successfully, I want to know that. Testing the content doesn't help me diagnose that and, in fact, obfuscates it.

Thanks for clarify!

iSazonov · 2017-07-13T05:50:01Z

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

+        $headers = @{"If-Match" = "12345"}
+        $response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8080/PowerShell?test=echo" -headers $headers
+
+        $response.Error | Should Not BeNullOrEmpty


The check is intentional. Checking for $result.Error provides a better failure id if the underlying request fails versus the NullReferenceException that would occur in the next statement.

iSazonov · 2017-07-13T05:50:20Z

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

+        $headers = @{"If-Match" = "12345"}
+        $response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8080/PowerShell?test=echo" -headers $headers -SkipHeaderValidation
+
+        $response.Error | Should BeNullOrEmpty


The check is intentional. Checking for $result.Error provides a better failure id if the underlying request fails versus the NullReferenceException that would occur in the next statement.

iSazonov · 2017-07-13T05:50:33Z

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

+        $headers = @{"If-Match" = "*"}
+        $response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8081/PowerShell?test=echo" -headers $headers -Cmdlet "Invoke-RestMethod"
+
+        $response.Error | Should BeNullOrEmpty


iSazonov · 2017-07-13T05:50:43Z

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

+        $headers = @{"If-Match" = "12345"}
+        $response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8081/PowerShell?test=echo" -headers $headers -Cmdlet "Invoke-RestMethod"
+
+        $response.Error | Should Not BeNullOrEmpty


The check is intentional. Checking for $result.Error provides a better failure id if the underlying request fails versus the NullReferenceException that would occur in the next statement.

iSazonov · 2017-07-13T05:56:48Z

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

+        $headers = @{"If-Match" = "12345"}
+        $response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8081/PowerShell?test=echo" -headers $headers -SkipHeaderValidation -Cmdlet "Invoke-RestMethod"
+
+        $response.Error | Should BeNullOrEmpty


The check is intentional. Checking for $result.Error provides a better failure id if the underlying request fails versus the NullReferenceException that would occur in the next statement.

TravisEz13 · 2017-07-14T17:24:33Z

Thanks everyone

msftclas added the cla-not-required label Jun 22, 2017

SteveL-MSFT requested changes Jun 23, 2017

View reviewed changes

SteveL-MSFT previously approved these changes Jun 23, 2017

View reviewed changes

mirichmo assigned TravisEz13 Jun 28, 2017

dantraMSFT closed this Jul 5, 2017

dantraMSFT reopened this Jul 5, 2017

msftclas added the cla-not-required label Jul 5, 2017

dantraMSFT and others added 2 commits July 7, 2017 15:38

Add -SkipHeaderValidation switch to Invoke-WebRequest and Invoke-Rest…

7f5c3aa

…Method to support adding headers without validating the header value.

Fix whitespace

5603a73

iSazonov reviewed Jul 13, 2017

View reviewed changes

TravisEz13 added the WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module label Jul 14, 2017

TravisEz13 merged commit ea758a5 into PowerShell:master Jul 14, 2017

dantraMSFT deleted the dantra/issue2895 branch July 14, 2017 17:45

markekraus mentioned this pull request Aug 3, 2017

Extend -SkipHeaderValidation to include -UserAgent to support non-standard User-Agent headers #4479

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add -SkipHeaderValidation switch to Invoke-WebRequest and Invoke-RestMethod to support adding headers without validating the header value. #4085

Add -SkipHeaderValidation switch to Invoke-WebRequest and Invoke-RestMethod to support adding headers without validating the header value. #4085

Add -SkipHeaderValidation switch to Invoke-WebRequest and Invoke-RestMethod to support adding headers without validating the header value. #4085

Add -SkipHeaderValidation switch to Invoke-WebRequest and Invoke-RestMethod to support adding headers without validating the header value. #4085

Conversation

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment