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.

Sign up for GitHub

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

Jump to bottom

Add SameSite to DiscardingCookie #10693

Merged
merged 2 commits into from Apr 7, 2021
Merged

Add SameSite to DiscardingCookie #10693

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0859b40
Add SameSite to DiscardingCookie
mkurz Feb 8, 2021
dc3c99f
MiMa
mkurz Feb 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 22 additions & 1 deletion core/play/src/main/java/play/mvc/Result.java
View file
Open in desktop
Expand Up @@ -514,8 +514,29 @@ public Result discardingCookie(String name, String path, String domain) {
* @param secure Whether the cookie to discard is secure
*/
public Result discardingCookie(String name, String path, String domain, boolean secure) {
return discardingCookie(name, path, domain, secure, null);
}

/**
* Discard a cookie in this result
*
* @param name The name of the cookie to discard, must not be null
* @param path The path of the cookie te discard, may be null
* @param domain The domain of the cookie to discard, may be null
* @param secure Whether the cookie to discard is secure
* @param sameSite The SameSite attribute of the cookie to discard, may be null
*/
public Result discardingCookie(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this means you'll have to switch to this API to fix the samesite issue for custom cookies?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you have to use this new method that takes the Cookie.SameSite param.

String name, String path, String domain, boolean secure, Cookie.SameSite sameSite) {
return withCookies(
new DiscardingCookie(name, path, Option.apply(domain), secure).toCookie().asJava());
new DiscardingCookie(
name,
path,
Option.apply(domain),
secure,
Option.apply(sameSite).map(ss -> ss.asScala()))
.toCookie()
.asJava());
}

/**
Expand Down
12 changes: 9 additions & 3 deletions core/play/src/main/scala/play/api/mvc/Cookie.scala
View file
Open in desktop
Expand Up @@ -133,8 +133,14 @@ object Cookie {
* @param domain the cookie domain
* @param secure whether this cookie is secured
*/
case class DiscardingCookie(name: String, path: String = "/", domain: Option[String] = None, secure: Boolean = false) {
def toCookie = Cookie(name, "", Some(Cookie.DiscardedMaxAge), path, domain, secure, false)
case class DiscardingCookie(
name: String,
path: String = "/",
domain: Option[String] = None,
secure: Boolean = false,
sameSite: Option[SameSite] = None
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually need pass the sameSite attribute to DiscardingCookie for deleting a cookie?

We probably want to use the broadest possible SameSite attribute for deleting, i.e.:

def toCookie = {
  // `SameSite=None` requires `Secure`
  val sameSite = if (secure) SameSite.None else SameSite.Lax
  Cookie(name, "", Some(Cookie.DiscardedMaxAge), path, domain, secure, false, Some(sameSite))
}

This will potentially overwrite the existing SameSite attribute, but that should be fine, since the cookie is being deleted anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmethvin Do you think it's safe to send SameSite=None per default for discarding a cookie (assuming secure = true, which is very likely nowadays)?
I have two serious concerns (both described here):
First: There was this infamous Safari bug, which caused SameSite=None to be treated like SameSite=Strict.
Second: Older Chromium versions can cause problems, given that such old version may still be used in other Chromium derived browsers.
For the Safari bug I don't see a problem however, because a cookie which does not get send cross domain, it should not matter if the browser handles the cookie as SameSite None or Strict, it still should discard the cookie in both cases (for cross domain it probably would not work, but it doesn't work now as well anyway). I am aware this Safari bug was fixed, however I guess there are millions of people out there that are stuck with iOS 12 and macOS 10.14
However for the Chromium derived browsers the problem would be that "These Chrome versions will reject a cookie with SameSite=None" which of course is much worse! Not sure how many such browsers out there, however for them the Discard cookie would suddenly be completely broken.

Another concern I have: Could there be cases where setting SameSite=None would allow an attacker to delete a cookie in situations were it would not have been possible when using Lax? I mean, as a framework user, I set Samesite=Lax in application.conf for the session cookie, but now the framework would overrule that setting when discarding the cookie, setting it to None... Imagine a very stupid case (based on this issue): You have a page that, when accessed, removes a cookie by sending the Set-Cookie header. Now when you access this side via an iframe, currently the Set-Cookie header, without sending a Samesite attribute, would default to Lax (in recent browser versions), meaning the Set-Cookie would be ignored. By setting SameSite to None by default for discarding cookies, suddenly the Set-Cookie would work now...
I hope you understand my example and I know it's a stupid one, my point is that maybe we are "opening" the discard cookie to be executed in cases where it shouldn't. I can't think of a other case right now but maybe there are ones...

Actually the more I think about this I think for the discard cookie we should just use the same SameSite attribute like when the cookie was created... I think that makes the most sense: In the end the user has control which SameSite attribute will be send (for session/flash cookies that would be in application.conf).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest I haven't thought through all the potential browser bugs and special cases, so I guess that's a good point.

Actually the more I think about this I think for the discard cookie we should just use the same SameSite attribute like when the cookie was created... I think that makes the most sense: In the end the user has control which SameSite attribute will be send (for session/flash cookies that would be in application.conf).

Yes, this would probably be the best solution. If we know the value of the original cookie that would be the most intuitive API. For session and flash cookies of course this would be easy, but I think for other cookies we would need to retrieve the original cookie from the request.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I think for other cookies we would need to retrieve the original cookie from the request.

I don't think that is something the framework should care about. The framework user created the cookie, so the user should just make sure to set the correct SameSite attribute when discarding the cookie.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gmethvin Actually this pull request is ready now.

) {
def toCookie = Cookie(name, "", Some(Cookie.DiscardedMaxAge), path, domain, secure, false, sameSite)
}

/**
Expand Down Expand Up @@ -492,7 +498,7 @@ trait CookieBaker[T <: AnyRef] { self: CookieDataCodec =>
}
}

def discard = DiscardingCookie(COOKIE_NAME, path, domain, secure)
def discard = DiscardingCookie(COOKIE_NAME, path, domain, secure, sameSite)
mkurz marked this conversation as resolved.
Show resolved Hide resolved

/**
* Builds the cookie object from the given data map.
Expand Down
8 changes: 8 additions & 0 deletions project/BuildSettings.scala
View file
Open in desktop
Expand Up @@ -313,6 +313,14 @@ object BuildSettings {
// Fix compile error on JDK15: Use direct AlgorithmId.get()
ProblemFilters
.exclude[IncompatibleMethTypeProblem]("play.core.server.ssl.CertificateGenerator.generateCertificate"),
// Add SameSite to DiscardingCookie
ProblemFilters.exclude[DirectMissingMethodProblem]("play.api.mvc.DiscardingCookie.apply"),
ProblemFilters.exclude[DirectMissingMethodProblem]("play.api.mvc.DiscardingCookie.copy"),
ProblemFilters.exclude[DirectMissingMethodProblem]("play.api.mvc.DiscardingCookie.this"),
ProblemFilters.exclude[IncompatibleSignatureProblem]("play.api.mvc.DiscardingCookie.curried"),
ProblemFilters.exclude[IncompatibleSignatureProblem]("play.api.mvc.DiscardingCookie.tupled"),
ProblemFilters.exclude[IncompatibleSignatureProblem]("play.api.mvc.DiscardingCookie.unapply"),
ProblemFilters.exclude[MissingTypesProblem]("play.api.mvc.DiscardingCookie$"),
),
unmanagedSourceDirectories in Compile += {
val suffix = CrossVersion.partialVersion(scalaVersion.value) match {
Expand Down