Skip to content

Fix bug with managing redirection and keepAuthorization in Web cmdlets #18902

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
iSazonov merged 18 commits into from
Jan 13, 2023
Merged

Fix bug with managing redirection and keepAuthorization in Web cmdlets #18902

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
4ad575b
fix bug with managing redirection
CarloToso Jan 7, 2023
a1671df
Update src/Microsoft.PowerShell.Commands.Utility/commands/utility/Web…
CarloToso Jan 7, 2023
8510729
keepAuthorization --> keepAuthorizationOnRedirect
CarloToso Jan 8, 2023
e378ef2
remove useless IsPresent
CarloToso Jan 8, 2023
2a9f147
missed two keepAuthorizationOnRedirect
CarloToso Jan 8, 2023
f5b6d1d
bool handleRedirect = keepAuthorizationOnRedirect
CarloToso Jan 8, 2023
e48bd39
better handleRedirect if logic
CarloToso Jan 8, 2023
1345784
revert Resume.IsPresent
CarloToso Jan 8, 2023
3ed7bd7
keepAuthorizationOnRedirect --> handleRedirect
CarloToso Jan 8, 2023
c2d3399
add test
CarloToso Jan 11, 2023
c4ed1b7
Update test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlet…
CarloToso Jan 12, 2023
7f5b298
Update src/Microsoft.PowerShell.Commands.Utility/commands/utility/Web…
CarloToso Jan 12, 2023
7f3fd07
remove comment
CarloToso Jan 12, 2023
bcfcfc1
Merge branch 'master' into Fix-handleRedirect-with-MaximumRedirection
CarloToso Jan 12, 2023
536de9d
Merge branch 'master' into Fix-handleRedirect-with-MaximumRedirection
CarloToso Jan 12, 2023
2771457
reorder keepAuthorizationOnRedirect
CarloToso Jan 12, 2023
7a8daeb
fix error
CarloToso Jan 12, 2023
289f5e8
Merge branch 'PowerShell:master' into Fix-handleRedirect-with-Maximum…
CarloToso Jan 13, 2023
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
40 changes: 18 additions & 22 deletions ...owerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -957,20 +957,13 @@ internal virtual HttpClient GetHttpClient(bool handleRedirect)
}

// This indicates GetResponse will handle redirects.
if (handleRedirect || AllowInsecureRedirect)
if (handleRedirect || WebSession.MaximumRedirection == 0)
{
handler.AllowAutoRedirect = false;
}
else if (WebSession.MaximumRedirection > -1)
else if (WebSession.MaximumRedirection > 0)
{
if (WebSession.MaximumRedirection == 0)
{
handler.AllowAutoRedirect = false;
}
else
{
handler.MaxAutomaticRedirections = WebSession.MaximumRedirection;
}
handler.MaxAutomaticRedirections = WebSession.MaximumRedirection;
}

handler.SslProtocols = (SslProtocols)SslProtocol;
Expand Down Expand Up @@ -1263,7 +1256,7 @@ private bool ShouldRetry(HttpStatusCode code)
);
}

internal virtual HttpResponseMessage GetResponse(HttpClient client, HttpRequestMessage request, bool keepAuthorization)
internal virtual HttpResponseMessage GetResponse(HttpClient client, HttpRequestMessage request, bool handleRedirect)
{
ArgumentNullException.ThrowIfNull(client);

Expand All @@ -1282,7 +1275,10 @@ internal virtual HttpResponseMessage GetResponse(HttpClient client, HttpRequestM
_cancelToken = new CancellationTokenSource();
response = client.SendAsync(req, HttpCompletionOption.ResponseHeadersRead, _cancelToken.Token).GetAwaiter().GetResult();

if ((keepAuthorization || (AllowInsecureRedirect && (WebSession.MaximumRedirection > 0 || WebSession.MaximumRedirection == -1))) && IsRedirectCode(response.StatusCode) && response.Headers.Location != null)
if (handleRedirect
&& WebSession.MaximumRedirection is not 0
&& IsRedirectCode(response.StatusCode)
&& response.Headers.Location is not null)
Comment on lines +1278 to +1281
Copy link
Collaborator

@iSazonov iSazonov Jan 12, 2023

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

Copy link
Contributor Author

@CarloToso CarloToso Jan 13, 2023 •
edited
Loading

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)

{
_cancelToken.Cancel();
_cancelToken = null;
Expand All @@ -1303,10 +1299,10 @@ internal virtual HttpResponseMessage GetResponse(HttpClient client, HttpRequestM

currentUri = new Uri(request.RequestUri, response.Headers.Location);
// Continue to handle redirection
using (client = GetHttpClient(handleRedirect: true))
using (client = GetHttpClient(handleRedirect))
using (HttpRequestMessage redirectRequest = GetRequest(currentUri))
{
response = GetResponse(client, redirectRequest, keepAuthorization);
response = GetResponse(client, redirectRequest, handleRedirect);
}
}

Expand Down Expand Up @@ -1344,7 +1340,7 @@ internal virtual HttpResponseMessage GetResponse(HttpClient client, HttpRequestM

WriteVerbose(reqVerboseMsg);

return GetResponse(client, requestWithoutRange, keepAuthorization);
return GetResponse(client, requestWithoutRange, handleRedirect);
}
}

Expand Down Expand Up @@ -1418,14 +1414,14 @@ protected override void ProcessRecord()
ValidateParameters();
PrepareSession();

// if the request contains an authorization header and PreserveAuthorizationOnRedirect is not set,
// If the request contains an authorization header and PreserveAuthorizationOnRedirect is not set,
// it needs to be stripped on the first redirect.
bool keepAuthorization = WebSession is not null
&& WebSession.Headers is not null
&& PreserveAuthorizationOnRedirect.IsPresent
&& WebSession.Headers.ContainsKey(HttpKnownHeaderNames.Authorization);
bool keepAuthorizationOnRedirect = PreserveAuthorizationOnRedirect.IsPresent
&& WebSession.Headers.ContainsKey(HttpKnownHeaderNames.Authorization);

bool handleRedirect = keepAuthorizationOnRedirect || AllowInsecureRedirect;

using (HttpClient client = GetHttpClient(keepAuthorization))
using (HttpClient client = GetHttpClient(handleRedirect))
{
int followedRelLink = 0;
Uri uri = Uri;
Expand Down Expand Up @@ -1459,7 +1455,7 @@ protected override void ProcessRecord()

WriteVerbose(reqVerboseMsg);

HttpResponseMessage response = GetResponse(client, request, keepAuthorization);
HttpResponseMessage response = GetResponse(client, request, handleRedirect);

string contentType = ContentHelper.GetContentType(response);
string respVerboseMsg = string.Format(
Expand Down
20 changes: 18 additions & 2 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 @@ -133,14 +133,22 @@ function ExecuteRedirectRequest {
$Method = 'GET',

[switch]
$PreserveAuthorizationOnRedirect
$PreserveAuthorizationOnRedirect,

[ValidateRange(0, [int]::MaxValue)]
[int]
$MaximumRedirection
)
$result = [PSObject]@{Output = $null; Error = $null; Content = $null}

try {
$headers = @{"Authorization" = "test"}
if ($Cmdlet -eq 'Invoke-WebRequest') {
$result.Output = Invoke-WebRequest -Uri $uri -Headers $headers -PreserveAuthorizationOnRedirect:$PreserveAuthorizationOnRedirect.IsPresent -Method $Method
if ($MaximumRedirection -gt 0) {
$result.Output = Invoke-WebRequest -Uri $uri -Headers $headers -PreserveAuthorizationOnRedirect:$PreserveAuthorizationOnRedirect.IsPresent -Method $Method -MaximumRedirection:$MaximumRedirection
} else {
$result.Output = Invoke-WebRequest -Uri $uri -Headers $headers -PreserveAuthorizationOnRedirect:$PreserveAuthorizationOnRedirect.IsPresent -Method $Method
}
$result.Content = $result.Output.Content | ConvertFrom-Json
} else {
$result.Output = Invoke-RestMethod -Uri $uri -Headers $headers -PreserveAuthorizationOnRedirect:$PreserveAuthorizationOnRedirect.IsPresent -Method $Method
Expand Down Expand Up @@ -885,6 +893,14 @@ Describe "Invoke-WebRequest tests" -Tags "Feature", "RequireAdminOnWindows" {
$response.Error | Should -BeNullOrEmpty
$response.Content.Headers."Authorization" | Should -BeExactly "test"
}

It "Validates Invoke-WebRequest with -PreserveAuthorizationOnRedirect respects -MaximumRedirection on redirect: <redirectType> <redirectedMethod>" -TestCases $redirectTests {
param($redirectType, $redirectedMethod)
$uri = Get-WebListenerUrl -Test 'Redirect' -TestValue '3' -Query @{type = $redirectType}
$response = ExecuteRedirectRequest -Uri $uri -PreserveAuthorizationOnRedirect -MaximumRedirection 2

$response.Error.FullyQualifiedErrorId | Should -Be "WebCmdletWebResponseException,Microsoft.PowerShell.Commands.InvokeWebRequestCommand"
}

It "Validates Invoke-WebRequest preserves the authorization header on multiple redirects: <redirectType>" -TestCases $redirectTests {
param($redirectType)
Expand Down