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

Remove curl auth on cross-domain redirects #2845

Merged
merged 1 commit into from Mar 13, 2022

Conversation

kkopachev
Copy link
Contributor

@kkopachev kkopachev commented Jan 28, 2021

Fixes #1477

@kkopachev kkopachev force-pushed the issue-1477 branch 2 times, most recently from abe294e to c78f124 Compare Jan 28, 2021
Copy link
Contributor

@TimWolla TimWolla left a comment

As a non-maintainer that eyeballed the diff this LGTM.

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Feb 6, 2021

Do we actually want to support someone circumventing Guzzle's abstraction, and setting auth headers directly through curl options?

@TimWolla
Copy link
Contributor

TimWolla commented Feb 6, 2021

@GrahamCampbell My understanding is that this is related to this:

guzzle/src/Client.php

Lines 405 to 413 in 3a0543e

case 'digest':
// @todo: Do not rely on curl
$options['curl'][\CURLOPT_HTTPAUTH] = \CURLAUTH_DIGEST;
$options['curl'][\CURLOPT_USERPWD] = "$value[0]:$value[1]";
break;
case 'ntlm':
$options['curl'][\CURLOPT_HTTPAUTH] = \CURLAUTH_NTLM;
$options['curl'][\CURLOPT_USERPWD] = "$value[0]:$value[1]";
break;

@GrahamCampbell
Copy link
Member

GrahamCampbell commented Feb 6, 2021

Ah, I see that "todo" is what needs resolving.

@kkopachev
Copy link
Contributor Author

kkopachev commented Feb 9, 2021

I looked at the todo item before submitting this PR and resolved to submitting PR anyway. I have 2 reasons for that:

  • This PR fixes issue. Probably not the ideal way for ideal world, but it fixes it now.
  • It is 6 years since this todo was first created. Digest auth provide no advantages over https+basic auth and therefore not so much popular and usage would probably decline over time. NTLM auth is explicitly disabled at stream handler because such auth requires keep-alive connection and stream handler closes stream after each request.

tl;dr: a fix is better than no fix for issue being open for so long with low chances of proper fix.

@kkopachev
Copy link
Contributor Author

kkopachev commented May 25, 2021

Is there anything else I could do for this PR?

@stale
Copy link

stale bot commented Sep 22, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 2 weeks if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale No activity for a long time label Sep 22, 2021
@kkopachev
Copy link
Contributor Author

kkopachev commented Sep 28, 2021

ping? Somebody wants to re-run all checks and merge it in?

@stale stale bot removed the lifecycle/stale No activity for a long time label Sep 28, 2021
@GrahamCampbell GrahamCampbell requested a review from Nyholm Oct 18, 2021
@stale
Copy link

stale bot commented Feb 17, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 2 weeks if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale No activity for a long time label Feb 17, 2022
@stale stale bot removed the lifecycle/stale No activity for a long time label Mar 1, 2022
@kkopachev
Copy link
Contributor Author

kkopachev commented Mar 1, 2022

Rebased on latest master. Please take a look.

Nyholm
Nyholm approved these changes Mar 13, 2022
Copy link
Member

@Nyholm Nyholm left a comment

Thank you

@GrahamCampbell GrahamCampbell merged commit cc80b00 into guzzle:master Mar 13, 2022
23 checks passed
@kkopachev kkopachev deleted the issue-1477 branch Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Problem with 302 redirect and Auth on initial (but not redirected) link
4 participants