Adapt RFC 6265 in path handling for cookies

[antonfirsov]

This PR resolves the "Path related issues" in #26141 by adapting RFC 6265 for path management.

• Removes the restriction for the Path attribute (it's no longer expected to prefix the request path)
• Introduces the default-path calculation and path matching algorithms as defined in section 5.1.4 of the new RFC
• Adds integration tests for HttpClient based on user scenarios described in the issues

The work is mostly based on @ism's analysis in #21440 (comment). This is a behavioral breaking change compared to previous versions of .NET.

This PR does not:

• Solve the "Domain related issues"
• Deal with the deprecated Version attribute

I plan to add those changes in follow-up PR-s. I believe that the improved path management by it's own can unblock many users.

/cc @psmulovics @Tratcher

EDIT

Marked as draft because of the concerns in #39250 (comment)

[Dotnet-GitSync-Bot]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[ghost]

Tagging subscribers to this area: @dotnet/ncl
Notify danmosemsft if you want to be subscribed.

[psmulovics]

Thank you, this is amazing! Looking forward to the other side of the PR too, but will try this when it would be in a nightly build.

[antonfirsov]

/CC @geoffkizer @wfurt

[geoffkizer]

This change basically looks fine to me (a couple minor issues above).

But I'd still like to understand what exact scenarios are changing behavior, and whether we think these are at all common or not.

[geoffkizer]

Re additional issues not addressed here:

• Solve the "Domain related issues"

This is just allowing domains without a leading dot, right? This seems pretty simple to do, so is there any reason this wouldn't make it for 5.0?

• Deal with the deprecated Version attribute

Can you explain what's necessary to do here?

[antonfirsov]

@geoffkizer thanks!

But I'd still like to understand what exact scenarios are changing behavior, and whether we think these are at all common or not.

The two cases I see:

1. User code depends on rejecting cookies coming from different Path
2. Before the change, a cookie having Path=/foo was accepted for URL http://x.y/fooBAR. We will start rejecting it now.

It looks like both cases should be very uncommon.

This seems pretty simple to do, so is there any reason this wouldn't make it for 5.0?

It's more about the Version attribute actually. Since RFC 6265 deprecated it, normally we should remove all logic depending on it (including all code dealing with CookieVariant and probably obsolete the Cookie.Version property.

As alternative, we may decide to keep that logic for now, and handle removing of Version in 3rd PR later, or maybe open a separate issue for Version.

[geoffkizer]

What customer benefit is there to removing the Version logic? It doesn't seem to me like this is going to help users much (unless I'm missing something).

[geoffkizer]

It looks like both cases should be very uncommon.

Yeah, I agree. I would vote to just go ahead and make these changes.

As long as we also get the domain change in for 5.0 (which seems like it should be easy to do), I think we will be in a fine place for 5.0.

[geoffkizer]

Minor feedback above.

[geoffkizer]

LGTM in general, minor feedback above.

Would be good to get a review from @Tratcher as well.

[antonfirsov]

After doing more experiments and reading further in RFC 6265, I've got more concerned about the change, marking the PR as WIP until those concerns are resolved.

I realized that clients are still required to path-match the request URL when accepting cookies as per section 5.4:

The user agent MUST use an algorithm equivalent to the following
algorithm to compute the "cookie-string" from a cookie store and a
request-uri:

1. Let cookie-list be the set of cookies from the cookie store that
   meets all of the following requirements:
   [....]

• The request-uri's path path-matches the cookie's path.

According to this, now I'm afraid that @Tratcher's findings in #25226 (comment) are probably inaccurate:

The client requires the cookie path to be less specific than the current request (e.g. request is /foo/bar and cookie can be /foo) but we are setting a path that's more specific (e.g. request is / and we set /signin-oidc). There's no provision for this behavior in the spec (https://tools.ietf.org/html/rfc6265.html#section-5.2.4) and browsers handle it fine.

It looks like Chrome+Edge does not accept a cookie with a more specific path /login on a request having the root path /, while Firefox does. Now I'm wondering about what we consider as correct behavior. If customer applications do work with browsers, I'm probably missing something. @Tratcher @psmulovics @ism any thoughts?

[psmulovics]

Hahh. We only use Chrome / Edge, so we considered that the 'expected truth'. Based on the RFC too, Firefox seems to tbe the odd one out as the other two seems to follow it more to the text.

[antonfirsov]

Yeah but this is bad news for your use case isn't it? If we decide to implement the strict RFC-conform solution Chrome does, your authentication logic will keep failing since the cookies are set with the path /signin-oidc which does not match path like /api/Users. Or am I missing something about these authentication scenarios? Do they work with (today's) version of Chrome?

/cc @javiercn

[Tratcher]

Yeah but this is bad news for your use case isn't it? If we decide to implement the strict RFC-conform solution Chrome does, your authentication logic will keep failing since the cookies are set with the path /signin-oidc which does not match path like /api/Users. Or am I missing something about these authentication scenarios? Do they work with (today's) version of Chrome?

/cc @javiercn

This scenario does work today in all browsers. HttpClient is the only place we've had trouble with it.

[antonfirsov]

This scenario does work today in all browsers. HttpClient is the only place we've had trouble with it.

@Tratcher this is either not true, or I'm missing an important detail.

The following app prints COOKIE MISSING with both Chrome and Firefox:
https://gist.github.com/antonfirsov/bebfe4221db2f5b487f2756190d98855

Notes:

• The second two (commented out) cases work
• With Firefox I can see the cookie in Dev Tools but it's not being sent back in the request

[Tratcher]

This scenario does work today in all browsers. HttpClient is the only place we've had trouble with it.

@Tratcher this is either not true, or I'm missing an important detail.

The following app prints COOKIE MISSING with both Chrome and Firefox:
https://gist.github.com/antonfirsov/bebfe4221db2f5b487f2756190d98855

Notes:

• The second two (commented out) cases work
• With Firefox I can see the cookie in Dev Tools but it's not being sent back in the request

See this scenario from the original issue: #26141 (comment)

These OpenIdConnect cookies work in all browsers (or I would have been canned by now).

Here's an updated version of your sample that more closely reflects the OIDC scenario:

public void Configure(IApplicationBuilder app, IWebHostEnvironment env)
        {
            app.UseRouting();

            app.UseEndpoints(endpoints =>
            {
                endpoints.MapGet("/", async ctx =>
                {
                    if (!ctx.Request.Cookies.ContainsKey("LoginInvoked"))
                    {
                        ctx.Response.Redirect("/login/user");
                    }
                    else
                    {
                        if (ctx.Request.Cookies.ContainsKey("LoggedIn"))
                        {
                            await ctx.Response.WriteAsync($"WORKS");
                        }
                        else
                        {
                            await ctx.Response.WriteAsync($"COOKIE MISSING LoggedIn");
                        }
                    }
                });

                endpoints.MapGet("/login/user", async ctx =>
                {
                    ctx.Response.Cookies.Append("LoginInvoked", "true", new CookieOptions() { Path = "/" });

                    ctx.Response.Cookies.Append("LoggingIn", "true", new CookieOptions() { Path = "/return" });

                    ctx.Response.Redirect("/return");
                });


                endpoints.MapGet("/return", async ctx =>
                {
                    if (!ctx.Request.Cookies.ContainsKey("LoggingIn"))
                    {
                        await ctx.Response.WriteAsync($"COOKIE MISSING LoggingIn");
                        return;
                    }

                    ctx.Response.Cookies.Append("LoggedIn", "true", new CookieOptions() { Path = "/" });

                    ctx.Response.Redirect("/");
                });
            });
        }

It's the LoggingIn cookie that HttpClient has a problem with because its path is completely different from the request path that assigned it (/return vs /login/user). The browser is just fine with returning that cookie when a request is made to the specified path /return.

[antonfirsov]

@Tratcher thank you for making the use case clear, it helped a lot! This case should indeed work with the new RFC.

[antonfirsov]

@geoffkizer I want to add a few more tests based on the login scenarios before removing the draft mark.

Regarding the removal of the Version property:
There's no direct customer value, it's more about consistence, standard-conformance and maintainability. I suggest to close #26141 when the second (domain) PR is merged and open a new issue to track that topic for 6.0/Future.

[Tratcher]

I realized that clients are still required to path-match the request URL when accepting cookies as per section 5.4:

That section is about sending request cookies, not about accepting response cookies. Our issues have been on accepting, not sending.

[antonfirsov]

Had to push several changes to HttpClientHandlerTest.Cookies.cs to deal with issues on .NET Framework. These did not affect the approved product-code changes though.

@ManickaP any chance you can review that particular file?

[antonfirsov]

The inner loop test hang is unrelated and tracked in #37895.

[antonfirsov]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[antonfirsov]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ManickaP]

HttpClientHandlerTest.Cookies.cs changes looks good, minor questions.

[antonfirsov]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[ManickaP]

@antonfirsov you're hitting #39056 in outerloop (among others), please disregard this one, it's known and in my pipeline.

[danmoseley]

@antonfirsov this is labeled breaking change. Can you please open an issue with https://github.com/dotnet/docs/issues/new?template=dotnet-breaking-change.md

I don't see one

[antonfirsov]

/azp run runtime-libraries outerloop

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[CarnaViire]

Docs merged, so will remove "needs-breaking-change-doc-created" label