Fix Web Cmdlets for .NET Core 2.1 #6806
Conversation
markekraus
requested review from
JamesWTruher,
PaulHigin and
dantraMSFT
as code owners
|
I restarted CI Travis MacOs - it seems temporary failed. |
markekraus
changed the title
|
@TravisEz13 can you ping someone to review? I'd definitely like this to be merged before the next preview or RC since without it, there are effectively known regressions in master. |
|
@iSazonov Could you review this change? |
iSazonov
left a comment
iSazonov
left a comment
There was a problem hiding this comment.
@markekraus Please look https://www.codefactor.io/repository/github/powershell/powershell/pull/6806# It is not request to fix.
| using (client = GetHttpClient(false)) | ||
| using (HttpRequestMessage redirectRequest = GetRequest(currentUri, stripAuthorization:true)) | ||
| // Continue to handle redirection | ||
| using (client = GetHttpClient(true)) |
There was a problem hiding this comment.
Usually we use named parameter GetHttpClient(handleRedirect:true) - it is more readable.
There was a problem hiding this comment.
Not "usually". there are plenty of examples where the pattern is not used. It's also not listed in the guidance. Please add it to the guidance if you expect it in submissions. I will change it.
There was a problem hiding this comment.
We have so many patterns that it is impossible to document them all. So we have activated CodeFactor and will do fine tuning. We should then update our guidances.
|
Note: there are CodeFactor issues. We should try to fix these error in the new and changed code. This was added after you started this PR so I think we can defer this. |
4 participants
PR Summary
This PR switches the logic of when the Web Cmdlets handle redirects when the Authorization header is present. .NET Core 2.1 no longer sends the Authorization header by default (dotnet/corefx#26864). however, we introduced the ability to do so leveraging the previous default behavior through the use of the
-PreserveAuthorizationOnRedirectswitch.This PR also corrects a bug introduced 6.0.0 where certain redirect types redirect from POST to GET were set which should have passed through POST to POST and some were improperly passing through POST to POST which should have been doing POST to GET. This correction is a breaking change. It was made apparent as now the redirection behavior is being managed by CoreFX which is doing the correct behavior, test were added for both when CoreFX and the Web Cmdlets manage redirection.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests