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

Behavioral difference between Linux and Windows in httprequest with 302 reply and # #25119

Closed
Petermarcu opened this issue Feb 21, 2018 · 14 comments · Fixed by dotnet/corefx#27360
Assignees
Labels
area-System.Net.Http os-linux Linux OS (any supported distro)
Milestone

Comments

@Petermarcu
Copy link
Member

Petermarcu commented Feb 21, 2018

@jdelrue commented on Thu Feb 01 2018

General

Operating system 1: Windows 10
Operating system 2: Ubuntu (16.04 or 17.04)

Execute:

using System;
using System.Net.Http;
using System.Threading.Tasks;

namespace redirtest
{
    class Program
    {
        static void Main(string[] args)
        {
            var res = Task.Run(async () => await method());
            res.Wait();
        }
        public static async Task method()
        {
            using (var client = new HttpClient(new HttpClientHandler
            {
                AllowAutoRedirect = true
            }))
            {
                var connect = await client.GetAsync("http://www.jimber.org/redirect.php");
                var requestUri = connect.RequestMessage.RequestUri.AbsoluteUri;
                Console.WriteLine(requestUri);

            }
        }
    }
}

Output Windows: http://www.google.be/#youdontgetthisinlinux
Output Linux: http://www.google.be/

Expected output is the Windows version. Workaround: Disable autoredirect and read location header.

@davidsh
Copy link
Contributor

davidsh commented Feb 21, 2018

cc: @stephentoub

Looks like the Uri fragment is missing from the redirect. CurlHandler will need to be fixed. Might need to fix SocketsHttpHandler.

@geoffkizer
Copy link
Contributor

We should add a test for this, and SocketsHttpHandler if necessary.

@stephentoub
Copy link
Member

We have this test:
https://github.com/dotnet/corefx/blob/d8d32ab5abbec82f4f1346f64e51e525508c6f39/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs#L914-L923
which would seem to validate the specified behavior, and it passes on all platforms. It's marked as [ActiveIssue] now, but that only happened a few days ago in https://github.com/dotnet/corefx/issues/27217 due to a spurious failure on Windows. I've not looked to see how that differs from the repro.

@geoffkizer
Copy link
Contributor

geoffkizer commented Feb 21, 2018

That does seem to match what the repro is doing.

@rmkerr
Copy link
Contributor

rmkerr commented Feb 21, 2018

I took a look at the spec, and the behavior outlined in the test does seem correct:

   If the Location value provided in a 3xx (Redirection) response does
   not have a fragment component, a user agent MUST process the
   redirection as if the value inherits the fragment component of the
   URI reference used to generate the request target (i.e., the
   redirection inherits the original reference's fragment, if any).

I have an Ubuntu 16.04 machine allocated right now, so I can test the repro to see if I get the same result.

@rmkerr
Copy link
Contributor

rmkerr commented Feb 21, 2018

I'm able to reproduce the issue on Ubuntu. I also gave the CurlHandler redirect code a cursory look, but didn't see anything obviously stripping the fragment.

@geoffkizer
Copy link
Contributor

Interesting, thanks Max. I can't see what's different between this and the test Stephen linked. Do you see anything?

@geoffkizer
Copy link
Contributor

Possibly the test is bogus somehow. I'm looking at SocketsHttpHandler's redirect logic, and I don't see it doing anything with fragments specifically. Yet this test is passing.

@rmkerr
Copy link
Contributor

rmkerr commented Feb 21, 2018

Yep, it's a bogus test. The key line of the is the comparison at the end:
https://github.com/dotnet/corefx/blob/d8d32ab5abbec82f4f1346f64e51e525508c6f39/src/System.Net.Http/tests/FunctionalTests/HttpClientHandlerTest.cs#L948 URI comparison does not consider the fragment. So the fragment isn't being tested at all here.

@geoffkizer
Copy link
Contributor

Well, that's bad. Glad we found this.

@stephentoub
Copy link
Member

URI comparison does not consider the fragment

Is that a bug of its own, or by design for some reason?

@stephentoub stephentoub self-assigned this Feb 22, 2018
@stephentoub
Copy link
Member

I fixed the test and SocketsHttpHandler in dotnet/corefx#27360.

The observed Linux behavior is due to libcurl explicitly never sending fragments. It's been that way by design since v7.20 in 2010.

Desktop also doesn't send fragments.

WinHttpHandler does send fragments, but it doesn't correctly handle the case called out as a "must" in the RFC where redirecting from a URI with a fragment to a URI without a fragment is supposed to inherit the fragment into the redirect.

The test special cases all of these.

@stephentoub
Copy link
Member

Also regarding my question:

Is that a bug of its own, or by design for some reason?

I see this is called out in the docs:
https://docs.microsoft.com/en-us/dotnet/api/system.uri.equals?view=netframework-4.7.1
"The Equals method compares the two instances without regard to user information (UserInfo) and fragment (Fragment) parts that they might contain. For example, given the URIs http://www.contoso.com/index.htm#search and http://user:password@www.contoso.com/index.htm, the Equals method would return true."

@rmkerr
Copy link
Contributor

rmkerr commented Feb 22, 2018

I had always assumed the behavior was correct because it was explicitly mentioned in the docs, but it actually looks a little less clear. There is a basis for our current behavior in section 6.1:

When URIs are compared to select (or avoid) a network action, such as retrieval
of a representation, fragment components (if any) should be excluded from
the comparison.

But ultimately in section 6.2.3 the spec states:

The fragment component is not subject to any scheme-based normalization; thus,
two URIs that differ only by the suffix "#" are considered different regardless of
the scheme.

That second section seems to clearly establish that the fragment should be included in equality checks. So our behavior looks like a deviation from the spec, even if it is one we probably don't want to fix.

@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Http os-linux Linux OS (any supported distro)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants