GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Automatically strip Authorization header on redirects by dantraMSFT · Pull Request #3885 · PowerShell/PowerShell

Conversation

Dan Travison (dantraMSFT) commented May 30, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) requested a review from Steve Lee (SteveL-MSFT)

May 30, 2017 19:55

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label

May 30, 2017

Steve Lee (SteveL-MSFT) requested changes

May 31, 2017

View reviewed changes

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

Steve Lee (SteveL-MSFT) May 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Typo: compatibility

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) May 31, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

Steve Lee (SteveL-MSFT) May 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Generally, you should not mix formatting changes with functional changes. Have them as separate PRs to make them more readable and reviewable.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

Steve Lee (SteveL-MSFT) May 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You should put this in WebCmdletStrings.resx

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) May 31, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'll remove it. Not appropriate for release.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) May 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Don't we need to support other 3xx redirect codes?

https://en.wikipedia.org/wiki/URL_redirection#HTTP_status_codes_3xx

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) May 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) May 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) May 31, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Should also have a test case where -PreserveAuthorizationOnRedirect is specified, but no Authorization header was supplied

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) reviewed

May 31, 2017

View reviewed changes

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

Paul Higinbotham (PaulHigin) May 31, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

_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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Paul Higinbotham (PaulHigin) commented Jun 5, 2017

Copy link

Copy Markdown

Contributor

This seems to have a lot of unrelated changes (26 files now changed) now.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) commented Jun 5, 2017

Copy link

Copy Markdown

Member

Dan Travison (@dantraMSFT) looks like you accidentally pulled in other commits, you need to do a rebase -i to clean this up. Let me know if you need help with this.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Mike Richmond (mirichmo) assigned Jason Shirk (lzybkr)

Jun 6, 2017

Dan Travison (dantraMSFT) changed the title ~~Automatically strip Authorization header on 302 redirects~~ Automatically strip Authorization header on redirects

Jun 7, 2017

Steve Lee (SteveL-MSFT) requested changes

Jun 7, 2017

View reviewed changes

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You need to make the corresponding doc change in PowerShell/PowerShell-Docs and mention it here to link the two

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It's in progress. Will submit a PR shortly and update the description to reference it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Jun 9, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Also, do we need this capability with invoke-restmethod?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

isn't 308 also valid?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Since you have this specific checks it seems you should have TestCases that go through the different status codes

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Refactored all tests to cover each redirect status code.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why isn't $response.content | convertfrom-json sufficient? this code seems unnecessary

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 7, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Jun 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

This is assuming the output you create in httplistener is json (and if not, perhaps it should be).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 9, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

This is only a choice between GET and POST. No other methods are being used.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) Jun 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 9, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Is there a specific reason you want to add a new Describe rather than having your tests part of the existing Invoke-WebRequest tests?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

should validate the value as well

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will fix.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

validate the value

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will fix

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) Jun 7, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

why not validate all the headers are there except authorization?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 7, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) requested changes

Jun 8, 2017

View reviewed changes

Comment thread

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

Steve Lee (SteveL-MSFT) Jun 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) Jun 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

This is assuming the output you create in httplistener is json (and if not, perhaps it should be).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

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

Steve Lee (SteveL-MSFT) Jun 8, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

$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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 8, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'll see if I can drop the unused $redirectedMethod params.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 9, 2017

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm remove the unused parameters.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) requested changes

Jun 9, 2017

View reviewed changes

Comment thread

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

Steve Lee (SteveL-MSFT) Jun 9, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I thought you were going to put these tests into an existing Describe?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Steve Lee (SteveL-MSFT) requested changes

Jun 9, 2017

View reviewed changes

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

Steve Lee (SteveL-MSFT) Jun 9, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Also, do we need this capability with invoke-restmethod?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) commented Jun 12, 2017

Copy link

Copy Markdown

Contributor Author

Please squash when merged.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Steve Lee (SteveL-MSFT) approved these changes

Jun 12, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) reviewed

Jun 14, 2017

View reviewed changes

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

Jason Shirk (lzybkr) Jun 14, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 14, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) reviewed

Jun 14, 2017

View reviewed changes

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

Jason Shirk (lzybkr) Jun 14, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

reqeusts [](start = 68, length = 8)

Spelling.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 14, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) reviewed

Jun 14, 2017

View reviewed changes

Comment thread

...PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebRequestPSCmdlet.CoreClr.cs Outdated

Jason Shirk (lzybkr) Jun 14, 2017

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) Jun 14, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Updated.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Jason Shirk (lzybkr) suggested changes

Jun 14, 2017

View reviewed changes

Jason Shirk (lzybkr) left a comment

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

There were a couple of minor issues I'd like to see addressed before I merge.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Dan Travison (dantraMSFT) and others added 12 commits

June 14, 2017 13:08


          WebRequestPSCmdlet: Automatically strip Authorization header on redir…

…ects


          Handle all redirection codes

93578d6


          WebRequestPSCmdlet: Automatically strip Authorization header on 302 r…

9f54b6b

…edirects (#1)

WebRequestPSCmdlet: Automatically strip Authorization header on 302 redirects


          Handle all redirection codes

9ebdee0


          Refactor tests to use -TestCases to reduce redundant code.

90f7fd0


          Fix redirect tets - move supportedRedirects into script block.

f379e4a


          Refactor redirect tests to cover all redirect codes for each test.

1769d0e

Remove Redirect tag - not allowed by appveyor.


          Remove unused $redirectedMethod parameter

370438a

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.


          Add Content-Type header in HTTPListener and remove Convert-ResponseCo…

10ed934

…ntent


          Add Invoke-RestMethod tests and move tests into existing describe block.

706b584

Integrate httpslistener changes


          WebRequestPSCmdlet: Automatically strip Authorization header on redir…

7eeebbf

…ects


          WebRequestPSCmdlet: Automatically strip Authorization header on 302 r…

1cf6853

…edirects (#1)

WebRequestPSCmdlet: Automatically strip Authorization header on 302 redirects

Dan Travison (dantraMSFT) and others added 11 commits

June 14, 2017 13:12


          Handle all redirection codes

12d9014


          WebRequestPSCmdlet: Automatically strip Authorization header on 302 r…

3ecd5a7

…edirects (#1)

WebRequestPSCmdlet: Automatically strip Authorization header on 302 redirects


          Handle all redirection codes

8f837fa


          Refactor tests to use -TestCases to reduce redundant code.

e814056


          Fix redirect tets - move supportedRedirects into script block.

18850e4


          Refactor redirect tests to cover all redirect codes for each test.

94d5c41

Remove Redirect tag - not allowed by appveyor.


          Remove unused $redirectedMethod parameter

de27f76

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.


          Add Content-Type header in HTTPListener and remove Convert-ResponseCo…

9d2b095

…ntent


          Add Invoke-RestMethod tests and move tests into existing describe block.

Integrate httpslistener changes


          Address review feedback.

1b9eb0f


          Change CRLF to LF

b969b20

Dan Travison (dantraMSFT) commented Jun 15, 2017

Copy link

Copy Markdown

Contributor Author

Jason Shirk (@lzybkr) Would you review my changes so I can get this merged before I go OOF.
Thanks.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Jason Shirk (lzybkr) approved these changes

Jun 15, 2017

View reviewed changes

Jason Shirk (lzybkr) commented Jun 15, 2017

Copy link

Copy Markdown

Contributor

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Dan Travison (dantraMSFT) commented Jun 15, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor Author

Jason Shirk (@lzybkr) Description updated. I think this is good now; can we get it merged.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

Jason Shirk (lzybkr) merged commit 039ed67 into PowerShell:master

Jun 15, 2017

Mark Kraus (markekraus) mentioned this pull request

Jan 20, 2018

Web Cmdlets Fail to Redirect to Relative Paths when Authorization Header Supplied but -PreserveAuthorizationOnRedirect Not Present #5967

Closed

Florian Feldhaus (ffeldhaus) mentioned this pull request

Apr 2, 2018

WebRequestPSCmdlet does not check existence of Location header when Authorization is present #6543

Closed

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request

Aug 6, 2026


          Strip authorization header on redirects with web cmdlets (PowerShell#…

f295951

…3885)

Invoke-WebRequest and Invoke-RestMethod cmdlets will now strip authorization header on redirect unless the new parameter `-PreserveAuthorizationOnRedirect` is specified.

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.

Fixes PowerShell#2227

This was referenced Aug 9, 2026

feat(v21): implement Media Brain P0 V1 laiqian0239-glitch/yance#197

Merged

workflow: harden client builds and artifacts bashrusakh/rustdesk#4

Merged

coderabbitai Bot mentioned this pull request

Aug 25, 2026

fix: remediate 2026-08-25 adversarial audit (batch4) FixPortal/fixportal-claude-skills#90

Merged

Reviewers

Steve Lee (SteveL-MSFT)

SteveL-MSFT approved these changes

+2 more reviewers

Paul Higinbotham (PaulHigin)

PaulHigin left review comments

Jason Shirk (lzybkr)

lzybkr approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

Jason Shirk (lzybkr)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Dan Travison (dantraMSFT) commented May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Paul Higinbotham (PaulHigin) commented Jun 5, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Jun 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dan Travison (dantraMSFT) Jun 7, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Dan Travison (dantraMSFT) commented May 30, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Dan Travison (dantraMSFT) Jun 7, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Dan Travison (dantraMSFT) Jun 14, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.