Add -SkipHeaderValidation Support to ContentType on Web Cmdlets

[markekraus]

PR Summary

• Extends -SkipHeaderValidation to work with -ContentType to allow Content-Type headers which are not standards compliant,
• moves existing -SkipHeaderValidation tests to a context
• Adds tests
• closes Inconsistent behavior from Invoke-RestMethod from Windows Powershell to Powershell Core #6002

Documentation will need to be updated to reflect that -SkipHeaderValidation works with -ContentType

PR Checklist

Note: Please mark anything not applicable to this PR NA.

• PR has a meaningful title
  • Use the present tense and imperative mood when describing your changes
• Summarized changes
• User facing Documentation needed
  • Issue filed - Issue link: Document -SkipHeaderValidation support for -ContentType on Web Cmdlets MicrosoftDocs/PowerShell-Docs#2083
• Change is not breaking
• Make sure you've added a new test if existing tests do not effectively test the code changed
  • Add [feature] if the change is significant or affects feature tests
• This PR is ready to merge and is not Work in Progress.
  • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready.

[dantraMSFT]

I think it would be helpful to write a warning when the exceptions are suppressed due to SkipHeaderValidation; otherwise, there will be no indication that the header was not used.

[markekraus]

to me.. supplying -SkipHeaderValidation means "suppress all header related messages". Perhaps verbose would be better?

[dantraMSFT]

I guess I see it otherwise; I don't want validation to occur but if it can't be added at all, I believe I would need to know. For example, I'm aware of cases where header values are needed that do not conform to the standard but the target site requires it; however, if the header cannot even be added, it's something I will need to address because the request is no longer conveying what I expect.

[markekraus]

Hmm i think I'm a bit confused. We are not discarding any header values.The section of code where this comment thread lives is not the section where headers are added to the request. this section of code is where it tryes to determine request body encoding from -ContentType. We try/catch here because the value foo throws FormatException on new MediaTypeHeaderValue("foo").

The section where headers are added, the headers will always be added (here)

The only case in which request.Headers.TryAddWithoutValidation() or request.Content.Headers.TryAddWithoutValidation() would not add headers would be if the headers already existed. This is not possible because we are enumerating a dictionary so the keys will be case-insensitve unique. The headers will always be added and in fact the tests in the project prove that.

[dantraMSFT]

I don't see any tests for the ArgumentException path of the MediaTypeHeaderValue try/catch block.

[markekraus]

@dantraMSFT This is still WIP

[dantraMSFT]

Understood.

[markekraus]

Actually... we do hit the MediaTypeHeaderValue try block already in the tests "Verifies Invoke-WebRequest default ContentType handling reports an error is returned for an invalid Content-Type header value and -Body" and "Verifies Invoke-RestMethod default ContentType handling reports an error is returned for an invalid Content-Type header value and -Body".

We actually can't reach the CharSet section (for now) because... new MediaTypeHeaderValue("application/json; charset=utf-8"); will throw FormatException.... dotnet/corefx#16290

Effectively... This is all dead code already:

PowerShell/src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs

Lines 1480 to 1491 in 45bf1dc

	if (!string.IsNullOrEmpty(mediaTypeHeaderValue.CharSet))
	{
	try
	{
	encoding = Encoding.GetEncoding(mediaTypeHeaderValue.CharSet);
	}
	catch (ArgumentException ex)
	{
	ErrorRecord er = new ErrorRecord(ex, "WebCmdletEncodingException", ErrorCategory.InvalidArgument, ContentType);
	ThrowTerminatingError(er);
	}
	}

That would explain why I can't find any tests for -ContentType 'text/plain; charset=utf-8' or similar.. it wouldn't even be possible. It already throws a FormatException

[dantraMSFT]

Closed

[markekraus]

@dantraMSFT This is ready for review now.

[markekraus]

@dantraMSFT Ping

[JamesWTruher]

not sure whether these are tabs which have snuck in

[markekraus]

The section was moved from a #region to a Context, so indentation was added. see line https://github.com/markekraus/PowerShell/blob/eb541749b92cb5eb9f8c9a1d1658bed2af05f015/test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1#L804

[JamesWTruher]

can we use the new Pester should -throw -errorid <...>?

[markekraus]

We could. I made this PR before the project changed to Pester 4. However, The guidance does still say to use ShouldBeErrorId https://github.com/PowerShell/PowerShell/blob/master/docs/testing-guidelines/WritingPesterTests.md

[adityapatwardhan]

@markekraus Thank you for your contribution!