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

Clarify misleading AllowAutoRedirect remarks #6778

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HebaruSan
Copy link

@HebaruSan HebaruSan commented May 28, 2021

Problem

This snippet is misleading in some cases:

The Authorization header is cleared on auto-redirects and HttpWebRequest automatically tries to re-authenticate to the redirected location. In practice, this means that an application can't put custom authentication information into the Authorization header if it is possible to encounter redirection. Instead, the application must implement and register a custom authentication module. The System.Net.AuthenticationManager and related class are used to implement a custom authentication module. The AuthenticationManager.Register method registers a custom authentication module.

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 new IAuthenticationModule-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 to false 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.

@HebaruSan HebaruSan requested a review from a team as a code owner May 28, 2021 19:06
@ghost
Copy link

ghost commented May 28, 2021

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

Problem

This snippet is misleading in some cases:

The Authorization header is cleared on auto-redirects and HttpWebRequest automatically tries to re-authenticate to the redirected location. In practice, this means that an application can't put custom authentication information into the Authorization header if it is possible to encounter redirection. Instead, the application must implement and register a custom authentication module. The System.Net.AuthenticationManager and related class are used to implement a custom authentication module. The AuthenticationManager.Register method registers a custom authentication module.

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 new IAuthenticationModule-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 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 to false 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.

Author: HebaruSan
Assignees: -
Labels:

area-System.Net

Milestone: -

@opbld32
Copy link

opbld32 commented May 28, 2021

Docs Build status updates of commit e939e9e:

✅ Validation status: passed

File Status Preview URL Details
xml/System.Net/HttpWebRequest.xml ✅Succeeded View

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:

@ManickaP
Copy link
Member

@aik-jahoda @wfurt @scalablecory @dotnet/ncl : could anyone give this a look? Is the proposed change technically OK?

@aik-jahoda
Copy link
Contributor

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).

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

Successfully merging this pull request may close these issues.

4 participants