Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

WebCmdlets check if absolute uri before checking scheme#19468

Merged
daxian-dbw merged 7 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
CarloToso:absoluteuriCarloToso/PowerShell:absoluteuriCopy head branch name to clipboard
Apr 12, 2023
Merged

WebCmdlets check if absolute uri before checking scheme#19468
daxian-dbw merged 7 commits into
PowerShell:masterPowerShell/PowerShell:masterfrom
CarloToso:absoluteuriCarloToso/PowerShell:absoluteuriCopy head branch name to clipboard

Conversation

@CarloToso

Copy link
Copy Markdown
Contributor

PR Summary

  • Check if absolute uri before checking scheme

History:

PR Context

Fix #19461

PR Checklist

@CarloToso CarloToso requested a review from PaulHigin as a code owner April 7, 2023 19:04
@ghost ghost assigned TravisEz13 Apr 7, 2023
@iSazonov

iSazonov commented Apr 8, 2023

Copy link
Copy Markdown
Collaborator

Please add new test.

$command = "Invoke-WebRequest -Uri '$uri' -SkipCertificateCheck"

$result = ExecuteWebCommand -command $command
$result.Error.FullyQualifiedErrorId | Should -Be "WebCmdletWebResponseException,Microsoft.PowerShell.Commands.InvokeWebRequestCommand"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Before the fix was it "InsecureRedirection" exception? Could you please explain why now it is WebCmdletWebResponseException?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I introduced "InsecureRedirection" exception in #18595, previously it was WebCmdletWebResponseException.
If the uri has no scheme checking for a scheme would throw an error, so we skip the check and we go back to the old error on https to http redirection

@daxian-dbw daxian-dbw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The issue author has confirmed the fix solves the issue for his scenarios: #19461 (comment)

@CarloToso please update the comment as we discussed in #19468 (comment) and then I will merge the PR. Thanks!

@CarloToso

Copy link
Copy Markdown
Contributor Author

@daxian-dbw done

@daxian-dbw daxian-dbw enabled auto-merge (squash) April 12, 2023 00:27
@daxian-dbw daxian-dbw merged commit 3dd943d into PowerShell:master Apr 12, 2023
@iSazonov iSazonov added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Apr 12, 2023
@ghost

ghost commented Apr 20, 2023

Copy link
Copy Markdown

🎉v7.4.0-preview.3 has been released which incorporates this pull request.:tada:

Handy links:

@CarloToso CarloToso deleted the absoluteuri branch April 20, 2023 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Insecure Proxy Behavior with Invoke-(RestMethod|WebRequest) on 7.4.0-preview2

4 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.