Skip to content
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

WebCmdlets check if absolute uri before checking scheme #19468

Merged
merged 7 commits into from
Apr 12, 2023

Conversation

CarloToso
Copy link
Contributor

PR Summary

  • Check if absolute uri before checking scheme

History:

PR Context

Fix #19461

PR Checklist

@ghost ghost assigned TravisEz13 Apr 7, 2023
@iSazonov
Copy link
Collaborator

iSazonov commented Apr 8, 2023

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

Copy link
Member

@daxian-dbw daxian-dbw left a comment

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
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
Copy link

ghost commented Apr 20, 2023

🎉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