-
Notifications
You must be signed in to change notification settings - Fork 7.2k
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.
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
Fix bug with managing redirection and keepAuthorization in Web cmdlets #18902
Fix bug with managing redirection and keepAuthorization in Web cmdlets #18902
Conversation
@CarloToso Can you enhance our helper test tool - WebListener - to cover the scenario? Currently it supports redirection but not take into account authorization header (I guess we could use I see only reason we do custom redirection processing is that we want to keep the auth header. So I suggest rename |
It is the same as |
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
…Cmdlet/Common/WebRequestPSCmdlet.Common.cs Co-authored-by: xtqqczze <45661989+xtqqczze@users.noreply.github.com>
If we change it handleRedirect to keepAuthorization, it will be misleading after we add -PreserveHTTPMethodOnRedirect or -AllowInsicureRedirect. We could add something like this |
I don't see a problem here. As I said only reason we do manual redirect processing is that we want to keep auth header. So variable names should reflect to our intention. And comments could be updated too. |
@@ -533,7 +533,7 @@ internal virtual void ValidateParameters() | |||
} | |||
|
|||
// Resume requires OutFile. | |||
if (Resume.IsPresent && OutFile is null) | |||
if (Resume && OutFile is null) |
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 reduces readability. 😕
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.
reverted
@iSazonov It looks like there already are some tests that cover -PreserveAuthorizationOnRedirect |
Can you point these tests? I see there is Authorization controller but it doesn't cover redirection scenario. |
@iSazonov I found the -PreserveAuthorizationOnRedirect parameter in WebCmdlet.Tests.ps1 and I think it covers this case |
If the test is passed before the PR it is a broken test. |
I think the tests are not broken, they just never checked for |
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.
Please confirm that the new tests fail before the PR.
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1
Outdated
Show resolved
Hide resolved
...t.PowerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
Outdated
Show resolved
Hide resolved
@CarloToso Please resolve merge conflicts. |
@CarloToso Please resolve merge conflict again. |
bool keepAuthorizationOnRedirect = WebSession is not null | ||
&& WebSession.Headers is not null | ||
&& PreserveAuthorizationOnRedirect.IsPresent | ||
&& WebSession.Headers.ContainsKey(HttpKnownHeaderNames.Authorization); | ||
|
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 seems PreserveAuthorizationOnRedirect.IsPresent
should be on first place.
if (handleRedirect | ||
&& WebSession.MaximumRedirection is not 0 | ||
&& IsRedirectCode(response.StatusCode) | ||
&& response.Headers.Location is not null) |
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 is AllowInsecureRedirect removed?
We merged #18546 without tests. Time to add tests for AllowInsecureRedirect
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.
Added some tests #18939, AllowInsecureRedirect
is in handleRedirect
after this PR (line 1422)
/rebase |
@CarloToso Please rebase to get tests. |
This PR has Quantification details
Why proper sizing of changes matters
Optimal pull request sizes drive a better predictable PR flow as they strike a
What can I do to optimize my changes
How to interpret the change counts in git diff output
Was this comment helpful? 👍 :ok_hand: :thumbsdown: (Email) |
🎉 Handy links: |
PR Summary
The current behaviour of
if (keepAuthorization && IsRedirectCode(response.StatusCode) && response.Headers.Location is not null)
is wrong, I tested it by settingkeepAuthorization = true
--> this setshandleRetirect = true
--> and then setshandler.AllowAutoRedirect = false
. WithoutsessionRedirect
(we can choose a better name) it ignores theMaximumRedirection
parameter.Test: maunally set
keepAuthorization = true
before theif
, thenInvoke-WebRequest "http://mockbin.org/redirect/308?to=https://mockbin.org/redirect/302?to=https://mockbin.org/status/200" -MaximumRedirection 0 -SkipHttpErrorCheck
--> expected 308, result 200Proposed fix -->
PR Context
Fix Bug, merge before #18546 and #18894