Automatically strip Authorization header on redirects #3885
Conversation
| /// gets or sets the PreserveAuthorizationOnRedirect property | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This property overrides compatability with web requests on Windows. |
| @@ -57,7 +73,7 @@ public abstract partial class WebRequestPSCmdlet : PSCmdlet | |||
| /// <param name="response">Instance of a WebResponse object to be processed</param> | |||
| internal abstract void ProcessResponse(HttpResponseMessage response); | |||
|
|
|||
| #endregion Abstract Methods | |||
| #endregion Abstract Methods | |||
There was a problem hiding this comment.
Generally, you should not mix formatting changes with functional changes. Have them as separate PRs to make them more readable and reviewable.
| using (client = GetHttpClient(false)) | ||
| using (HttpRequestMessage redirectRequest = GetRequest(response.Headers.Location)) | ||
| { | ||
| string msg = string.Format("{0}->{1}", request.RequestUri.AbsoluteUri, response.Headers.Location.AbsoluteUri); |
There was a problem hiding this comment.
You should put this in WebCmdletStrings.resx
There was a problem hiding this comment.
I'll remove it. Not appropriate for release.
| @@ -223,6 +223,43 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" { | |||
| $jsonContent.headers.'User-Agent' | Should Match "WindowsPowerShell" | |||
| } | |||
|
|
|||
| # Verifies Invoke-WebRequest with -PreserveAuthorizationOnRedirect preserves | |||
| # the authorization header whe a 302 redirection | |||
| It "Validate Invoke-WebRequest with -PreserveAuthorizationOnRedirect preserves the authorization header on 302 redirects" { | |||
There was a problem hiding this comment.
Don't we need to support other 3xx redirect codes?
https://en.wikipedia.org/wiki/URL_redirection#HTTP_status_codes_3xx
| # the authorization header whe a 302 redirection | ||
| It "Validate Invoke-WebRequest with -PreserveAuthorizationOnRedirect preserves the authorization header on 302 redirects" { | ||
| try { | ||
| Start-HttpListener -AsJob |
There was a problem hiding this comment.
There's a race condition on Mac where stopping the httplistener and then starting a new one, the OS may not have cleaned up the actual underlying http service so the next start fails as the reservation still exists. In anycase, you should rebase against upstream/master as this script already starts and stops httplistener for all tests.
| Start-HttpListener -AsJob | ||
|
|
||
| $headers = @{"Authorization" = "test"} | ||
| $response = Invoke-WebRequest -Uri "http://localhost:8080/PowerShell?test=redirect" -Headers $headers -PreserveAuthorizationOnRedirect |
There was a problem hiding this comment.
Seems like we should test multiple redirects to make sure it's stripped off the first one and subsequent redirects get handled correctly (3 may be sufficient)
| Start-HttpListener -AsJob | ||
|
|
||
| $headers = @{"Authorization" = "test"} | ||
| $response = Invoke-WebRequest -Uri "http://localhost:8080/PowerShell?test=redirect" -Headers $headers -PreserveAuthorizationOnRedirect |
There was a problem hiding this comment.
Should also have a test case where -PreserveAuthorizationOnRedirect is specified, but no Authorization header was supplied
| stripAuthorization | ||
| ) | ||
| { | ||
| _cancelToken.Cancel(); |
There was a problem hiding this comment.
_cancelToken.Cancel(); [](start = 15, length = 23)
I am a little confused by this logic. It looks like an asynchronous call is being cancelled here. Is there a possibility of a race condition? Can you add a comment indicating what is being cancelled?
|
This seems to have a lot of unrelated changes (26 files now changed) now. |
|
@dantraMSFT looks like you accidentally pulled in other commits, you need to do a |
| /// authorization header needs to be preserved across redirects. | ||
| /// </remarks> | ||
| [Parameter] | ||
| public virtual SwitchParameter PreserveAuthorizationOnRedirect { get; set; } |
There was a problem hiding this comment.
You need to make the corresponding doc change in PowerShell/PowerShell-Docs and mention it here to link the two
There was a problem hiding this comment.
It's in progress. Will submit a PR shortly and update the description to reference it.
There was a problem hiding this comment.
Also, do we need this capability with invoke-restmethod?
| ( | ||
| (intCode >= 300 && intCode < 304) | ||
| || | ||
| intCode == 307 |
There was a problem hiding this comment.
Since you have this specific checks it seems you should have TestCases that go through the different status codes
There was a problem hiding this comment.
308 isn't currently supported in CoreCLR - HttpStatusCode doesn't have a define for it so there's no way for the code to see it.
There was a problem hiding this comment.
Refactored all tests to cover each redirect status code.
| [Microsoft.PowerShell.Commands.WebResponseObject] $Response | ||
| ) | ||
|
|
||
| $index = $Response.RawContent.IndexOf("{") |
There was a problem hiding this comment.
why isn't $response.content | convertfrom-json sufficient? this code seems unnecessary
There was a problem hiding this comment.
Because I just want the json part of the raw content. The rest is extraneous for my purposes and it is also not valid json.
NOTE: $response.Content is a byte array.
There was a problem hiding this comment.
Looking at your usage below, you pass in the output of Invoke-WebRequest to this helper. This helper isn't necessary. Line 189 below can just be:
$result.Content = $result.Output.Content | ConvertFrom-JsonThis is assuming the output you create in httplistener is json (and if not, perhaps it should be).
There was a problem hiding this comment.
The listener does add the output as json and I've even tried setting ContentType and ContentEncoding with no change in behavior. It appears it may be an issue on the client side. For now, I'll update the function to detect byte[] versus string and convert the byte[] to string before calling ConvertFrom-Json so it will work with arbitrary servers.
| try | ||
| { | ||
| $headers = @{"Authorization" = "test"} | ||
| if ($UsePost) |
There was a problem hiding this comment.
It would be better to have this as a -testcase where one of the parameters is $method and you can then pass Get and Post
There was a problem hiding this comment.
This is only a choice between GET and POST. No other methods are being used.
There was a problem hiding this comment.
lines 182 and 186 below only differ by use of -Method. You should either change $UsePost to $Method or use splatting so there's only a single Invoke-WebRequest line.
There was a problem hiding this comment.
I'll update the function to use a ValidateSet and default to GET. I don't want to accept arbitrary methods since I'm explicitly testing the POST->GET logic on redirects.
| return $result | ||
| } | ||
|
|
||
| Describe "Invoke-WebRequest redirect tests" -Tags ("Feature", "Redirect") { |
There was a problem hiding this comment.
Is there a specific reason you want to add a new Describe rather than having your tests part of the existing Invoke-WebRequest tests?
There was a problem hiding this comment.
I placed the tests in a new describe so I can run them in isolation while developing them. it turns out appveyor doesn't allow extra tag so I'll be removing it shortly.
| $response.Error | Should BeNullOrEmpty | ||
|
|
||
| # ensure Authorization header has been preserved. | ||
| $response.Content.Headers -contains "Authorization" | Should Be $true |
There was a problem hiding this comment.
should validate the value as well
|
|
||
| $response.Error | Should BeNullOrEmpty | ||
| # ensure Authorization header was stripped | ||
| $response.Content.Headers -contains "Authorization" | Should Be $true |
There was a problem hiding this comment.
Actually, I can't fix it without fixing ConvertTo-Json. $request.Headers is a WebHeaderCollection which is a NamedValueCollection (ICollection). ConvertTo-Json (and Select-Object for that matter) treat it as a collection and would have to special case the type.
| $response.Error | Should BeNullOrEmpty | ||
|
|
||
| # ensure user-agent is present (i.e., no false positives ) | ||
| $response.Content.Headers -contains "User-Agent" | Should Be $true |
There was a problem hiding this comment.
why not validate all the headers are there except authorization?
There was a problem hiding this comment.
I don't know explicitly what other headers 'should' be there so I don't have a source of truth to compare against what is echoed in the response. I know the authorization header should or should not be in a specific echoed response so I can verify it.
| try | ||
| { | ||
| $headers = @{"Authorization" = "test"} | ||
| if ($UsePost) |
There was a problem hiding this comment.
lines 182 and 186 below only differ by use of -Method. You should either change $UsePost to $Method or use splatting so there's only a single Invoke-WebRequest line.
| [Microsoft.PowerShell.Commands.WebResponseObject] $Response | ||
| ) | ||
|
|
||
| $index = $Response.RawContent.IndexOf("{") |
There was a problem hiding this comment.
Looking at your usage below, you pass in the output of Invoke-WebRequest to this helper. This helper isn't necessary. Line 189 below can just be:
$result.Content = $result.Output.Content | ConvertFrom-JsonThis is assuming the output you create in httplistener is json (and if not, perhaps it should be).
| ) | ||
|
|
||
| It "Validates Invoke-WebRequest with -PreserveAuthorizationOnRedirect preserves the authorization header on redirect" -TestCases $redirectTests { | ||
| param($redirectType, $redirectedMethod) |
There was a problem hiding this comment.
Not entirely sure about the Pester syntax, but do you have to have $redirectedMethod here since you don't use it? Or does Pester complain?
There was a problem hiding this comment.
$response.Content is a byte array, not string ($result.Output.Content is this byte array). $response.RawContent has a few lines of text inserted by the underlying API followed by the json form of the original request; inserted by the httplistener. The purpose of the function is to extract the json from the RawContent and return it explicitly since it is needed to verify the incoming request.
There was a problem hiding this comment.
I'll see if I can drop the unused $redirectedMethod params.
There was a problem hiding this comment.
I'm remove the unused parameters.
| return $result | ||
| } | ||
|
|
||
| Describe "Invoke-WebRequest redirect tests" -Tags "Feature" { |
There was a problem hiding this comment.
I thought you were going to put these tests into an existing Describe?
| /// authorization header needs to be preserved across redirects. | ||
| /// </remarks> | ||
| [Parameter] | ||
| public virtual SwitchParameter PreserveAuthorizationOnRedirect { get; set; } |
There was a problem hiding this comment.
Also, do we need this capability with invoke-restmethod?
|
Please squash when merged. |
| @@ -217,6 +240,10 @@ internal virtual HttpRequestMessage GetRequest(Uri uri) | |||
| } | |||
| else | |||
| { | |||
| if (stripAuthorization && entry.Key == HttpKnownHeaderNames.Authorization.ToString()) | |||
| { | |||
There was a problem hiding this comment.
We try to avoid == with string comparisons and instead use string.Equals(a,b, StringComparison.Ordinal) or the appropriate comparison - this makes it clearer that the author considered culture and case when writing the code.
| /// <remarks> | ||
| /// This property overrides compatibility with web requests on Windows. | ||
| /// On FullCLR (WebRequest), authorization headers are stripped during redirect. | ||
| /// CoreCLR (HTTPClient) does not have this behavior so web reqeusts that work on |
There was a problem hiding this comment.
reqeusts [](start = 68, length = 8)
Spelling.
| @@ -410,7 +511,7 @@ protected override void ProcessRecord() | |||
| WriteVerbose(linkVerboseMsg); | |||
| } | |||
|
|
|||
| using (HttpRequestMessage request = GetRequest(uri)) | |||
| using (HttpRequestMessage request = GetRequest(uri, false)) | |||
There was a problem hiding this comment.
false [](start = 76, length = 5)
Just a suggestion - we like to use named parameters when using bool constants - it makes it much easier to understand the code, so this could be GetResult(uri, stripAuthorization: false).
…edirects (#1) WebRequestPSCmdlet: Automatically strip Authorization header on 302 redirects
Remove Redirect tag - not allowed by appveyor.
Handle string and byte[] in Convert Update ExecuteRedirectRequest to accept a method name (GET or POST) Add a delay to Start-HTTPListener to mitigate slower systems.
Integrate httpslistener changes
…edirects (#1) WebRequestPSCmdlet: Automatically strip Authorization header on 302 redirects
…edirects (#1) WebRequestPSCmdlet: Automatically strip Authorization header on 302 redirects
Remove Redirect tag - not allowed by appveyor.
Handle string and byte[] in Convert Update ExecuteRedirectRequest to accept a method name (GET or POST) Add a delay to Start-HTTPListener to mitigate slower systems.
Integrate httpslistener changes
|
@lzybkr Would you review my changes so I can get this merged before I go OOF. |
|
I'll squash, but I'll ask you to write the commit message that follows our guidelines - you can just comment here or squash the changes yourself and update the PR, either way is fine. |
|
@lzybkr Description updated. I think this is good now; can we get it merged. |
Fix #2227
Update Invoke-WebRequest and Invoke-RestMethod cmdlets to strip an Authorization header on redirect.
The FullCLR implementation uses WebRequest to perform the request which silently strips the Authorization header when a redirect occurs. The CoreCLR implementation uses HttpClient to perform the request which does not strip the authorization header. The change explicitly handles the initial redirect, removes the authorization header and submits the request to location in the response.
A new switch, PreserveAuthorizationOnRedirect, disables this handling.
See MicrosoftDocs/PowerShell-Docs#1269 for the doc change to document the new switch -PerserveAuthorizationOnRedirect.