Skip to content

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

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.

Sign up for GitHub

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

Merged
TravisEz13 merged 2 commits into from
Jul 14, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7f5c3aa
Add -SkipHeaderValidation switch to Invoke-WebRequest and Invoke-Rest…
dantraMSFT Jun 22, 2017
5603a73
Fix whitespace
dantraMSFT Jun 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 21 additions & 4 deletions ...erShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,12 +58,21 @@ public abstract partial class WebRequestPSCmdlet : PSCmdlet
/// CoreCLR (HTTPClient) does not have this behavior so web requests that work on
/// PowerShell/FullCLR can fail with PowerShell/CoreCLR. To provide compatibility,
/// we'll detect requests with an Authorization header and automatically strip
/// the header when the first redirect occurs. This switch turns off this logic for
/// the header when the first redirect occurs. This switch turns off this logic for
/// edge cases where the authorization header needs to be preserved across redirects.
/// </remarks>
[Parameter]
public virtual SwitchParameter PreserveAuthorizationOnRedirect { get; set; }

/// <summary>
/// gets or sets the SkipHeaderValidation property
/// </summary>
/// <remarks>
/// This property adds headers to the request's header collection without validation.
/// </remarks>
[Parameter]
public virtual SwitchParameter SkipHeaderValidation { get; set; }

#region Abstract Methods

/// <summary>
Expand Down Expand Up @@ -240,14 +249,22 @@ internal virtual HttpRequestMessage GetRequest(Uri uri, bool stripAuthorization)
}
else
{
if (stripAuthorization
&&
if (stripAuthorization
&&
String.Equals(entry.Key, HttpKnownHeaderNames.Authorization.ToString(), StringComparison.OrdinalIgnoreCase)
)
{
continue;
}
request.Headers.Add(entry.Key, entry.Value);

if (SkipHeaderValidation)
{
request.Headers.TryAddWithoutValidation(entry.Key, entry.Value);
}
else
{
request.Headers.Add(entry.Key, entry.Value);
}
}
}
}
Expand Down
101 changes: 101 additions & 0 deletions test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,49 @@ function ExecuteRedirectRequest
return $result
}

# This function calls either Invoke-WebRequest or Invoke-RestMethod with the given uri
# using the custum headers and the optional SkipHeaderValidation switch.
function ExecuteRequestWithCustomHeaders
{
param (
[Parameter(Mandatory)]
[string]
$Uri,

[ValidateSet('Invoke-WebRequest', 'Invoke-RestMethod')]
[string] $Cmdlet = 'Invoke-WebRequest',

[Parameter(Mandatory)]
[ValidateNotNull()]
[Hashtable] $Headers,

[switch] $SkipHeaderValidation
)
$result = [PSObject]@{Output = $null; Error = $null; Content = $null}

try
{
if ($Cmdlet -eq 'Invoke-WebRequest')
{
$result.Output = Invoke-WebRequest -Uri $Uri -TimeoutSec 5 -Headers $Headers -SkipHeaderValidation:$SkipHeaderValidation.IsPresent
$result.Content = $result.Output.Content | ConvertFrom-Json
}
else
{
$result.Output = Invoke-RestMethod -Uri $Uri -TimeoutSec 5 -Headers $Headers -SkipHeaderValidation:$SkipHeaderValidation.IsPresent
# NOTE: $result.Output should already be a PSObject (Invoke-RestMethod converts the returned json automatically)
# so simply reference $result.Output
$result.Content = $result.Output
}
}
catch
{
$result.Error = $_
}

return $result
}

<#
Defines the list of redirect codes to test as well as the
expected Method when the redirection is handled.
Expand Down Expand Up @@ -662,6 +705,35 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {

#endregion Redirect tests

#region SkipHeaderVerification Tests

It "Verifies Invoke-WebRequest default header handling with no errors" {
$headers = @{"If-Match" = "*"}
$response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8080/PowerShell?test=echo" -headers $headers

$response.Error | Should BeNullOrEmpty
Copy link
Collaborator

@iSazonov iSazonov Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this - next line makes the check.

Copy link
Contributor Author

@dantraMSFT dantraMSFT Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@iSazonov iSazonov Jul 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dantraMSFT dantraMSFT Jul 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

@iSazonov iSazonov Jul 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarify!

$response.Content.Headers -contains "If-Match" | Should Be $true
}

It "Verifies Invoke-WebRequest default header handling reports an error is returned for an invalid If-Match header value" {
$headers = @{"If-Match" = "12345"}
$response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8080/PowerShell?test=echo" -headers $headers

$response.Error | Should Not BeNullOrEmpty
Copy link
Collaborator

@iSazonov iSazonov Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tha same.

Copy link
Contributor Author

@dantraMSFT dantraMSFT Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$response.Error.FullyQualifiedErrorId | Should Be "System.FormatException,Microsoft.PowerShell.Commands.InvokeWebRequestCommand"
Copy link
Member

@SteveL-MSFT SteveL-MSFT Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dantraMSFT dantraMSFT Jun 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@SteveL-MSFT SteveL-MSFT Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

$response.Error.Exception.Message | Should Be "The format of value '12345' is invalid."
}

It "Verifies Invoke-WebRequest header handling does not report an error when using -SkipHeaderValidation" {
$headers = @{"If-Match" = "12345"}
$response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8080/PowerShell?test=echo" -headers $headers -SkipHeaderValidation

$response.Error | Should BeNullOrEmpty
Copy link
Collaborator

@iSazonov iSazonov Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same.

Copy link
Contributor Author

@dantraMSFT dantraMSFT Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$response.Content.Headers -contains "If-Match" | Should Be $true
}

#endregion SkipHeaderVerification Tests

BeforeEach {
if ($env:http_proxy) {
$savedHttpProxy = $env:http_proxy
Expand Down Expand Up @@ -1124,6 +1196,35 @@ Describe "Invoke-RestMethod tests" -Tags "Feature" {

#endregion Redirect tests

#region SkipHeaderVerification tests

It "Verifies Invoke-RestMethod default header handling with no errors" {
$headers = @{"If-Match" = "*"}
$response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8081/PowerShell?test=echo" -headers $headers -Cmdlet "Invoke-RestMethod"

$response.Error | Should BeNullOrEmpty
Copy link
Collaborator

@iSazonov iSazonov Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tha same.

$response.Content.Headers -contains "If-Match" | Should Be $true
}

It "Verifies Invoke-RestMethod default header handling reports an error is returned for an invalid If-Match header value" {
$headers = @{"If-Match" = "12345"}
$response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8081/PowerShell?test=echo" -headers $headers -Cmdlet "Invoke-RestMethod"

$response.Error | Should Not BeNullOrEmpty
Copy link
Collaborator

@iSazonov iSazonov Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same.

Copy link
Contributor Author

@dantraMSFT dantraMSFT Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$response.Error.FullyQualifiedErrorId | Should Be "System.FormatException,Microsoft.PowerShell.Commands.InvokeRestMethodCommand"
Copy link
Member

@SteveL-MSFT SteveL-MSFT Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the ShouldBeErrorId pattern

Copy link
Contributor Author

@dantraMSFT dantraMSFT Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@SteveL-MSFT SteveL-MSFT Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

$response.Error.Exception.Message | Should Be "The format of value '12345' is invalid."
}

It "Verifies Invoke-RestMethod header handling does not report an error when using -SkipHeaderValidation" {
$headers = @{"If-Match" = "12345"}
$response = ExecuteRequestWithCustomHeaders -Uri "http://localhost:8081/PowerShell?test=echo" -headers $headers -SkipHeaderValidation -Cmdlet "Invoke-RestMethod"

$response.Error | Should BeNullOrEmpty
Copy link
Collaborator

@iSazonov iSazonov Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same.

Copy link
Contributor Author

@dantraMSFT dantraMSFT Jul 13, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$response.Content.Headers -contains "If-Match" | Should Be $true
}

#endregion SkipHeaderVerification tests

BeforeEach {
if ($env:http_proxy) {
$savedHttpProxy = $env:http_proxy
Expand Down
7 changes: 7 additions & 0 deletions test/tools/Modules/HttpListener/HttpListener.psm1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,13 @@ Function Start-HTTPListener {
$contentType = $queryItems["contenttype"]
$output = $queryItems["output"]
}

# Echo the request as the output.
"echo"
Copy link
Member

@SteveL-MSFT SteveL-MSFT Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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"

Copy link
Contributor Author

@dantraMSFT dantraMSFT Jun 23, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@dantraMSFT dantraMSFT Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@SteveL-MSFT SteveL-MSFT Jun 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

{
Write-Verbose -Message "Echo request"
$output = $request | ConvertTo-Json -Depth 6
}

<#
This test provides support for multiple redirection types as well as a custom
Expand Down