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

Fixes #1477

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

@TimWolla TimWolla left a comment

Choose a reason for hiding this comment

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

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

@GrahamCampbell
Copy link
Member

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

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

@kkopachev
Copy link
Contributor Author

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

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

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

Rebased on latest master. Please take a look.

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

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