-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Clarify misleading AllowAutoRedirect remarks #6778
base: main
Are you sure you want to change the base?
Conversation
Tagging subscribers to this area: @dotnet/ncl Issue DetailsProblemThis snippet is misleading in some cases:
It's quite true that the Authorization header is cleared for an auto-redirect, but the rest of the paragraph strongly ("Instead, ...") makes it sound like you can naïvely rip out code that previously set the The problem is that this suggestion only works if you're dealing with a server where authentication is mandatory, which is not the case in all situations where the Authorization header might be sent. The GitHub API allows authorization but does not require it; the benefit if provided is a much higher rate limit. The GitHub API also uses redirects when a file is requested from a repo that has been moved or renamed (when the old location is requested). If you attempt this solution for the GitHub API, your SummaryI tried to keep the existing text intact as much as possible while adding caveats and clarification so the reader can tell more easily whether the old suggestion applies to his or her use case. Then I added a new short paragraph explaining the impact for the optional authorization use case; as far as I can tell, there is no way to use FYI to my team member @DasSkelett in case you feel like doing some proof-reading and confirming this is what you've seen in testing.
|
Docs Build status updates of commit e939e9e: ✅ Validation status: passed
For more details, please refer to the build report. Note: Broken links written as relative paths are included in the above build report. For broken links written as absolute paths or external URLs, see the broken link report. For any questions, please:
|
@aik-jahoda @wfurt @scalablecory @dotnet/ncl : could anyone give this a look? Is the proposed change technically OK? |
As HttpWebRequest is deprecated I would not extend documentation. Instead add reference to HttpClient. Also we should verify behaviour of HttpClient when auto redirect, however I believe it behaves same for security reason. For the security reason I would not advise how to "fix" missing header and let people to think twice before they implement it (perhaps call the target URL directly). |
Problem
This snippet is misleading in some cases:
It's quite true that the Authorization header is cleared for an auto-redirect, but the rest of the paragraph strongly ("Instead, ...") makes it sound like you can naïvely rip out code that previously set the
Authorization
header and replace it by implementing and registering a newIAuthenticationModule
-implementing object. I read this and happily set off to implement that solution, only to conclude a few hours later that it was a dead end in my case.The problem is that this suggestion only works if you're dealing with a server where authentication is mandatory, which is not the case in all situations where the Authorization header might be sent. The GitHub API allows authorization but does not require it; the benefit if provided is a much higher rate limit. The GitHub API also uses redirects when a file is requested from a repo that has been moved or renamed (when the old location is requested).
If you attempt this solution for the GitHub API, your
IAuthenticationModule
code will never be called, because the server never demands authentication. Not only will your auto-redirects not be authorized (and therefore be subject to the GitHub API's lower anonymous rate limiting), as before attempting this solution, but so will your initial requests. It's lose-lose.Summary
I tried to keep the existing text intact as much as possible while adding caveats and clarification so the reader can tell more easily whether the old suggestion applies to his or her use case.
Then I added a new short paragraph explaining the impact for the optional authorization use case; as far as I can tell, there is no way to use
AllowAutoRedirect
successfully in this case, so it must be set tofalse
and the redirects handled manually. If an official tech expert can correctly tell me differently, I will be delighted beyond words.FYI to my team member @DasSkelett in case you feel like doing some proof-reading and confirming this is what you've seen in testing.