Allow a blank/empty ContentType to be passed to Invoke-WebRequest for compatibility with PS6 and earlier #11421

spongemike2 · 2019-12-21T18:08:20Z

PR Summary

Regarding Invoke-WebRequest:

A change was made to PowerShell Core with commit 2285ece613 that changes the way web request headers are handled when the web request has no body. Previously, they would be ignored. Now, they are processed anyway, including the ContentType header, which makes no sense when there's no body. Now, the headers are processed anyway, and if there's a ContentType header with an empty string value, it will fail.

When a customer tries to pass in a blank (i.e., $null) ContentType to a function that takes a string because they have no body, it gets treated as an empty string, not $null.

This works fin in Windows PowerShell (PS5 and earlier) and PowerShell Core 6, because their code bases don't have commit 2285ece613.

When it fails, they get a message saying that they can avoid this by passing -SkipHeaderValidation, but this flag isn't available in Windows PowerShell, and using it would require a crude hard-coded version check to see if you're on Windows PowerShell or PowerShell Core.

This change merely skips adding headers to the request if the header value is nothing (null or whitespace).

Question: Should we only do this skip for the ContentType header?

Fix #11396

PR Context

Without this, PS scripts that run fine in Windows PowerShell and PowerShell Core 6 break.

PR Checklist

…no body. Regarding `Invoke-WebRequest`: A change was made to PowerShell Core with commit `2285ece613` that changes the way web request headers are handled when the web request has no body. Previously, they would be ignored. Now, they are processed anyway, including the `ContentType` header, which makes no sense when there's no body. Now, the headers are processed anyway, and if there's a `ContentType` header with an empty string value, it will fail. When a customer tries to pass in a blank (i.e., `$null`) `ContentType` to a function that takes a `string` because they have no body, it gets treated as an empty string, not `$null`. This works fin in Windows PowerShell (PS5 and earlier) and PowerShell Core 6, because their code bases don't have commit `2285ece613`. When it fails, they get a message saying that they can avoid this by passing `-SkipHeaderValidation`, but this flag isn't available in Windows PowerShell, and using it would require a crude hard-coded version check to see if you're on Windows PowerShell or PowerShell Core. This change merely skips adding headers to the request if the header value is nothing (null or whitespace). **Question**: Should we only do this *skip* for the `ContentType` header? PowerShell#11396

Fix for issue 11396 regarding blank ContentType for web request with no body

msftclas · 2019-12-21T18:08:33Z

All CLA requirements met.

jazzdelightsme

Collecting some discussions about empty HTTP headers from elsewhere:

From the Fetch issue referenced by Steve:

I'm sorry but can someone just say Yes or No whether empty header values are legal HTTP?
Are they allowed by HTTP? Yes. Are they allowed / defined to have meaning for an individual header field? It depends on the field's definition. That said, it's not good practice to do it.

And dotnet seems to currently enforce the "it's not a good idea to do it" opinion (it rejects empty headers).

Suppose at some point, the dotnet Issue was addressed, and they decided to allow empty headers. Would we (PowerShell) then want to expose that ability in PS (to have an empty header value)? We could remove this null/empty check, and then the dotnet API would allow it… and it would be sent to the server. Would the HTTP server allow it? Dunno. So that could be a breaking change, too.

Or we could just say "no, you just can't send empty headers with PS". Given that that is already the case (you can't send an empty HTTP header, else dotnet throws (although I don't know if that behavior has always been that way with dotnet)), and nobody has complained (actually I have no idea if anyone has complained, but it hasn't become a blocking issue that has been addressed--I'm relying on actual PS people to validate what I'm saying here), it seems like a reasonable position to stick to.

Given all that, I think this change looks good.

spongemike2 · 2019-12-21T20:32:27Z

One option to make it even more future-proof would be to add another condition to this check, and only ignore blank headers if the body is also empty or non-existent. Then, it will actually behave closer to the PS6 version of the code, which doesn't allow any custom headers if there's no body.

vexx32 · 2019-12-21T21:03:55Z

@spongemike2 I may lack some context or experience in this area, but from memory there are some perfectly valid requests that can be made with headers and no body, aren't there?

spongemike2 · 2019-12-21T21:31:26Z

@vexx32, I am not an expert in this area, but if I had to guess, I would say yes, which is presumably why the change was made that broke this. However, headers that have no value? I think that's unlikely to be useful. Rather than specifying a header with a blank value, which dotnetcore doesn't allow, one could get effectively the same information conveyed by omitting the header altogether. Which is why I think (even though I'm not 100% sure) that my change should be safe.

iSazonov · 2019-12-23T10:06:36Z

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

+                if (!string.IsNullOrWhiteSpace(entry.Value))
                {
-                    try
+                    if (SkipHeaderValidation)


~~If our intention is to follow Core (that is to send headers with empty value) the check should be here:~~ Update.

Also please add a test.

SteveL-MSFT

Can we add a test? I believe there is an existing test case to validate headers are passed, so just augment it with an empty header.

spongemike2 · 2019-12-28T07:12:02Z

Test added. Verified that the new test fails prior to this change, and succeeds with it.

vexx32 · 2019-12-28T07:20:28Z

@spongemike2 could you edit the PR title summary to something a bit more descriptive? Thanks! 😊

spongemike2 · 2019-12-28T07:31:47Z

@spongemike2 could you edit the PR title summary to something a bit more descriptive? Thanks!

Done.

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

spongemike2 · 2020-01-01T01:35:52Z

Anything else needed?

iSazonov · 2020-01-06T20:19:21Z

I think It is a breaking change and we should add CL-BreakingChange

daxian-dbw · 2020-01-06T20:36:45Z

It looks to me a regression we made in 7.0 time frame and we are fixing it before 7.0 GA. It's a behavior changes only comparing with 7.0 previews, so I think it's not a breaking change. But do let me know if I'm missing something.

spongemike2 · 2020-01-06T20:42:50Z

It looks to me a regression we made in 7.0 time frame and we are fixing it before 7.0 GA. It's a behavior changes only comparing with 7.0 previews, so I think it's not a breaking change. But do let me know if I'm missing something.

I agree. It would have been an unknown breaking change if it wasn't fixed before GA, but we caught it and fixed it.

iSazonov · 2020-01-07T04:29:07Z

I agree if this will be in GA.

…mpty value for backward compatibility (#11421)

ghost · 2020-01-16T23:38:23Z

🎉v7.0.0-rc.2 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

Michael J. Lyons and others added 2 commits December 21, 2019 09:15

Merge pull request #1 from spongemike2/spongemike2/FixIssue11396

c332183

Fix for issue 11396 regarding blank ContentType for web request with no body

ghost assigned daxian-dbw Dec 21, 2019

jazzdelightsme approved these changes Dec 21, 2019

View reviewed changes

vexx32 added the WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module label Dec 21, 2019

iSazonov reviewed Dec 23, 2019

View reviewed changes

SteveL-MSFT reviewed Dec 23, 2019

View reviewed changes

SteveL-MSFT added this to the GA-consider milestone Dec 23, 2019

Add a test for blank ContentType in Invoke-WebRequest

1ffa16f

spongemike2 changed the title ~~Spongemike2/fix11396~~ Allow a blank/empty ContentType to be passed to Invoke-WebRequest for compatibility with PS6 and earlier Dec 28, 2019

iSazonov reviewed Dec 28, 2019

View reviewed changes

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1 Outdated Show resolved Hide resolved

Add a test for blank ContentType in Invoke-RestMethod

31c63d8

iSazonov added CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log and removed WG-Cmdlets-Utility cmdlets in the Microsoft.PowerShell.Utility module labels Jan 1, 2020

iSazonov approved these changes Jan 1, 2020

View reviewed changes

daxian-dbw merged commit 1376d31 into PowerShell:master Jan 6, 2020

spongemike2 deleted the spongemike2/Fix11396 branch January 6, 2020 20:22

adityapatwardhan removed this from the GA-consider milestone Jan 7, 2020

adityapatwardhan added this to the GA-approved milestone Jan 7, 2020

daxian-dbw pushed a commit that referenced this pull request Jan 10, 2020

Make web cmdlets skip processing the content headers with a null or e…

125d5b9

…mpty value for backward compatibility (#11421)

daxian-dbw modified the milestones: GA-approved, 7.0.0-rc.2 Jan 11, 2020

ghost mentioned this pull request Jan 16, 2020

Breaking change: Invoke-WebRequest -ContentType with empty string now fails #11396

Closed

TravisEz13 mentioned this pull request Oct 12, 2022

Back-port to 7.0: Update list of PowerShell team members in release tools #18266

Merged

22 tasks

Allow a blank/empty ContentType to be passed to Invoke-WebRequest for compatibility with PS6 and earlier #11421

Allow a blank/empty ContentType to be passed to Invoke-WebRequest for compatibility with PS6 and earlier #11421

Uh oh!

Conversation

spongemike2 commented Dec 21, 2019 • edited by iSazonov Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Context

PR Checklist

Uh oh!

msftclas commented Dec 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jazzdelightsme left a comment

Choose a reason for hiding this comment

Uh oh!

spongemike2 commented Dec 21, 2019

Uh oh!

vexx32 commented Dec 21, 2019

Uh oh!

spongemike2 commented Dec 21, 2019

Uh oh!

iSazonov Dec 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveL-MSFT left a comment

Choose a reason for hiding this comment

Uh oh!

spongemike2 commented Dec 28, 2019

Uh oh!

vexx32 commented Dec 28, 2019

Uh oh!

spongemike2 commented Dec 28, 2019

Uh oh!

Uh oh!

spongemike2 commented Jan 1, 2020

Uh oh!

iSazonov commented Jan 6, 2020

Uh oh!

daxian-dbw commented Jan 6, 2020

Uh oh!

spongemike2 commented Jan 6, 2020

Uh oh!

iSazonov commented Jan 7, 2020

Uh oh!

ghost commented Jan 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

spongemike2 commented Dec 21, 2019 •

edited by iSazonov

Loading

msftclas commented Dec 21, 2019 •

edited

Loading

iSazonov Dec 23, 2019 •

edited

Loading