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

Added a warning when stripping authentication from requests on a redirect #5375

Closed
wants to merge 2 commits into from

Conversation

kmaork
Copy link

@kmaork kmaork commented Mar 2, 2020

Hello!
After seeing this question, I thought a warning should be added when authentication is stripped from redirected requests, in order to save users a significant amount of debugging time.
Apparently the matter has already been discussed in this issue, and it has been agreed upon that some message should be passed to the user, like a debug log or a warning. But requests still does not use logs, and it seems people are still confused by this.
Maybe it is time to add a warning?

@kmaork
Copy link
Author

kmaork commented Mar 13, 2020

Does the PR need another review?

@kmaork
Copy link
Author

kmaork commented May 8, 2020

Can we make progress with this? 🙂 @yanz-androidify

@Ezhvsalate
Copy link

Great PR, lost an hour today trying to understand why my service was broken after moved to OpenShift (it internally redirected requests). This warning could have helped me a lot.

@IroNEDR
Copy link

IroNEDR commented Apr 8, 2021

This PR would come really handy @yanz-androidify @kmaork why does it still say that the merging is blocked? How can we get someone with write access to review and approve this PR? It's waiting since May.
The current state of requests makes it nearly unusable for me when the host is on some cloud provider's server which does automatic redirects on ingress.

@scorchio
Copy link

scorchio commented Oct 26, 2021

I've lost a good 2 hours trying to find out what's wrong when I was playing around with PyGithub, which is using requests under the hood.

@nateprewitt, could you please take a look and provide some feedback? It would be great if requests could be a bit more helpful in such scenarios.

@yannicschroeer
Copy link

yannicschroeer commented Dec 6, 2021

Please add this. The current behavior is annoyingly implicit!

@kmaork
Copy link
Author

kmaork commented Dec 16, 2021

Hey @nateprewitt , @sigmavirus24 , @sethmlarson - this PR was approved ages ago by @yanz-androidify but seems to have been forgotten since. Any chance of it getting reviewed?

@sigmavirus24
Copy link
Contributor

@kmaork I don't see an approval from that account nor are they a maintainer (nor does that account seem to exist any longer).

As it stands, yes this can be annoyingly secure and hidden from the user but this usage of warnings is inappropriate for the solution. It's not the right mechanism and it will result in more issues generated here than users helped.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants