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

IDNs are not properly converted to ASCII on automatic redirects #2423

Closed
acelaya opened this issue Dec 7, 2019 · 9 comments
Closed

IDNs are not properly converted to ASCII on automatic redirects #2423

acelaya opened this issue Dec 7, 2019 · 9 comments
Milestone

Comments

@acelaya
Copy link

acelaya commented Dec 7, 2019

Guzzle version(s) affected: 6.5

Description

I have been waiting for this PR to be merged, #2286, which brings IDN support to guzzle.

The thing is that I have just tested v6.5, which includes it, and I have realized that the idn_to_ascii function is only applied to the first domain provided by the user, but if for some reason some other IDN is returned in the Location header and guzzle is configured to automatically follow redirects, it does not apply it to those, causing some domains to not properly resolve.

How to reproduce

I have some internationalized domain names that can be used to test, which I also use for my e2e tests.

These are all the use cases I cover:

For now, I just disable automatically following redirects and use a recursive function.

@gmponos
Copy link
Member

gmponos commented Dec 8, 2019

Will this solve the problem? #2424

@acelaya
Copy link
Author

acelaya commented Dec 8, 2019

Yeah, I guess it should :)

I can try if my tests pass using your fork.

@acelaya
Copy link
Author

acelaya commented Dec 8, 2019

Hey @gmponos.

I have tried installing your version on my project, by adding your fork as a VCS composer repository, but then, when I use the dev-issue#2423 version constraint, it thinks I want to install commit with ID 2423 from branch issue, and fails.

I have tried escaping/encoding the # char in different ways, but it always fails to resolve to the proper branch.

Could you rename the branch so that it does not include special chars?

@GrahamCampbell
Copy link
Member

You could just edit the vendor folder with the changes from the PR. Would be faster...

@acelaya
Copy link
Author

acelaya commented Dec 8, 2019

You are completely right. I overengineered the whole process 😅

@acelaya
Copy link
Author

acelaya commented Dec 8, 2019

@gmponos I just tested your changes by copy-pasting the code as @GrahamCampbell suggested, and I can confirm it works 😄

@gmponos
Copy link
Member

gmponos commented Dec 9, 2019

Btw I think I fixed that on the related PR #2424

I also pushed a branch issue2423 without # in case you want to retest it :)

@acelaya
Copy link
Author

acelaya commented Dec 10, 2019

Hey @gmponos

I tested again with your branch, and it continues working. Thanks!

@sagikazarmark sagikazarmark added this to the 6.5.1 milestone Dec 18, 2019
@Nyholm
Copy link
Member

Nyholm commented Dec 21, 2019

This is fixed by #2424

Will be released in 6.5.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants