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

[Cookie] Implement RFC 6265 for Cookie #26141

Closed
caesar-chen opened this issue May 11, 2018 · 5 comments
Closed

[Cookie] Implement RFC 6265 for Cookie #26141

caesar-chen opened this issue May 11, 2018 · 5 comments
Assignees
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions
Milestone

Comments

@caesar-chen
Copy link
Contributor

Current Cookie implementation follows RFC 2109, which is obsoleted by RFC 6265.

Per RFC 2109:

4.3.2  Rejecting Cookies

   To prevent possible security or privacy violations, a user agent
   rejects a cookie (shall not store its information) if any of the
   following is true:

   * The value for the Path attribute is not a prefix of the request-
     URI.

   * The value for the Domain attribute contains no embedded dots or
     does not start with a dot.

   * The value for the request-host does not domain-match the Domain
     attribute.

RFC 6265 is more tolerant for accepting Cookies, and removes those restrictions. Modern browsers support RFC 6265, and we should match the behavior.

Especially, there are three areas needed to be improved/changed. Path scoping, domain restriction, and other fields (for example, Version attribute, also obsoleted by RFC 6265). Currently we have 5 related issues because of not implementing RFC 6265.

Path related issues

#21440

  • Ideas: Starts with this comment, and a comment below. This issue is about the case that adding Cookies without explicitly setting a Path attribute. (The analysis done by @ism is great, are you interested in proposing a PR for it?)
  • Test cases: Verify that if Path attribute-value is empty or if the first character of the attribute value is not %x2F ("/"), whether the Cookie path is correctly set to the default path (the algorithm to compute the default-path of a cookie is defined in RFC 6265).

#22372

  • Ideas: Limiting the path check in the code, since RFC 6265 removes the restriction.
  • Test cases: Verify that Cookies set to a different path than the current request uri path are not rejected, and can be stored in the CookieContainer.

#25226

  • Ideas: Starts with the repro here. We may want to change VerifySetDefaults function.
  • Test cases: Set Cookies which Path attribute is not a prefix of the request URI. Verify client side CookieContainer will accept the Cookies fine. Even better, verify that those Cookies will be send for the next request (refer to the code in Repro).

Domain related issues

#19746

  • Ideas: Starts with the repro in issue description.
  • Test cases: If a server which sets a cookie with an explicit domain without a leading dot, this cookie will be accepted/stored (Despite of version of Cookie).

#20942

  • Ideas: Dupe of the above issue. The repro and analysis provided by @Nothing4You are with high quality. @Nothing4You are you interested in proposing a PR for it?
  • Test cases: Similar to the issue above.

Action plan

The suggested way to approach this umbrella issue is that:

  1. Adding tests first to demonstrate correct behavior
  2. Verifying the correctness of the test Cookie data by testing with browser
  3. Disable them using this issue number (be prepared that those tests may fail on Framework, which still follows the old RFC)
  4. Submitting PRs to make product change.

External contributions are welcomed!

@davidsh
Copy link
Contributor

davidsh commented Mar 16, 2019

It seems that RFC 6265 may have a revision soon:
https://httpwg.org/http-extensions/rfc6265bis.html

@psmulovics
Copy link

psmulovics commented Apr 10, 2020

I'm bringing a usecase where we are affected.

We face this when authenticating using OIDC programmatically.

We try to access a resource service.ms.com/api/Users which is served by an ASP.NET Core resource server.
On the client side (.net framework console app in the test case), we have autoredirect enabled on the HttpClient.

Instead of console client apps, there could be WPF app users, who need to authenticate. In “real-life” usually a browser is popped up, where the user can enter their credentials. Similar to the login button in Visual Studio.

The cookie exchange:
The service will redirect to PingFederate and return two ASP.NET served cookies for the /signin-oidc path. These cookies will not be stored in the cookie container.

Set-Cookie: .AspNetCore.OpenIdConnect.Nonce.CfDJ8PTm4o2fNlZBg7FWyr-_XnhUMyFm-…redacted….--ybO8o9hW6oYPz0Xmpsk=N; expires=Wed, 08 Apr 2020 07:24:04 GMT; path=/signin-oidc; samesite=none; httponly
Set-Cookie: .AspNetCore.Correlation.oidc.tx8tzv8jsKA0YUQJ1xc8ZqtjHZIRIdPTbEU7y2zTNZk=N; expires=Wed, 08 Apr 2020 07:24:04 GMT; path=/signin-oidc; samesite=none; httponly

When IDP returns with the code and state, we should redirect to service.ms.com/signin-oidc with the cookies above and with the values of code and state (provided by the IDP). Without the cookies the service fails the signin request.
This works perfectly fine in browsers though.

By turning off autodredirect on the client side (and manually handling them), we can explicitly put the cookies In the container, and that is a workaround, but it seems hacky. (similar code as here: #25226 )

@karelz
Copy link
Member

karelz commented May 19, 2020

@antonfirsov can you please take a look at the part highlighted by @psmulovics above? #26141 (comment)
It would be really good to have it in 5.0. Thanks!

@ism
Copy link

ism commented May 20, 2020

@psmulovics, it's a 3-year old bug covered here #21440, where cookies are not set if you set cookies after POST method with redirect. Your workaround is correct and this is the best you can do at this moment.

CarnaViire added a commit that referenced this issue Jul 24, 2020
Minimal fix for domain-related cookie issues of #26141

To fully comply with RFC 6265, one should remove deprecated cookie 
properties, such as Version, from public API. So only the stated 
issues with leading dot were addressed now.

Also note that the leading dot was not stripped from the domain even 
though RFC 6265 proposed it. This behavior was chosen because 
browsers like Chrome and Edge also don't strip the leading dot.
antonfirsov added a commit that referenced this issue Jul 28, 2020
Resolves "Path related issues" of #26141
(described in #21440, #22372 and #25226) 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
@antonfirsov
Copy link
Member

antonfirsov commented Aug 4, 2020

We fixed the path and domain related issues reported in the opening comment with #39250 and #39781.

I'm closing this in favor of #40328. Summarized our current status in that new issue.

Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this issue Aug 10, 2020
Minimal fix for domain-related cookie issues of dotnet#26141

To fully comply with RFC 6265, one should remove deprecated cookie 
properties, such as Version, from public API. So only the stated 
issues with leading dot were addressed now.

Also note that the leading dot was not stripped from the domain even 
though RFC 6265 proposed it. This behavior was chosen because 
browsers like Chrome and Edge also don't strip the leading dot.
Jacksondr5 pushed a commit to Jacksondr5/runtime that referenced this issue Aug 10, 2020
Resolves "Path related issues" of dotnet#26141
(described in dotnet#21440, dotnet#22372 and dotnet#25226) 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
@dotnet dotnet locked as resolved and limited conversation to collaborators Dec 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net enhancement Product code improvement that does NOT require public API changes/additions
Projects
None yet
Development

No branches or pull requests

7 participants