#175 Add support for SameSite cookies #271

aldaris · 2019-10-14T22:06:36Z

This PR adds support for SameSite attributes, allowing consumers of the API setting cookies with SameSite flags None, Lax, and Strict.
#175

joakime · 2019-11-15T14:31:07Z

The current Google Chrome 80 (canary builds) are already enforcing SameSite and will produce many errors / warnings if it's not used properly.

joakime · 2019-11-15T14:35:32Z

@aldaris this PR is failing the Eclipse ECA checks.

Make sure you have a filed ECA at eclipse - follow details link
Make sure your commit uses the same email address on file with your ECA
Make sure you follow the Developer Certificate of Origin as by using the "Signed Off By" features of git commit - https://wiki.eclipse.org/Development_Resources/Contributing_via_Git

yang-wei · 2020-01-06T01:04:18Z

@joakime @aldaris Can we just make this commit goes into next release ?

aldaris · 2020-01-06T09:58:36Z

@yang-wei The ECA related concerns should not be a problem any more, but the code still needs to be reviewed.

yang-wei · 2020-01-08T08:49:48Z

for anyone who is looking for alternative, we do this for our scala code

  private def setNoneSamesiteCookie(response: HttpServletResponse) {
    val setCookieHeader = "Set-Cookie"
    val l = new ListBuffer[String]()
    response.getHeaders(setCookieHeader).toArray().foreach(s => {
      l += String.format("%s; %s", s, "SameSite=None")
    })
    l.toList match {
      case head :: tail => {
      // this will clean all headers and set the first header with samesite
        response.setHeader(setCookieHeader, head)
        tail.foreach(s => {
          response.addHeader(setCookieHeader, s)
        })
      }
      case Nil => None
    }
  }

driverpt · 2020-01-08T09:38:58Z

Ok, but isn't that overriden if we set other Cookies ?

yang-wei · 2020-01-09T05:52:01Z

overriden with the attribute SameSite=None.
In my case I want all my cookies to have SameSite=None attributes. Feel free to change my code according to your needs

api/src/main/java/jakarta/servlet/http/Cookie.java

joakime

Avoid using user's default locale in toUpperCase.

Also, perhaps don't override toString() for literal, instead have new toLiteral() method?

api/src/main/java/jakarta/servlet/http/Cookie.java

aldaris · 2020-01-16T13:05:07Z

I've used @since Servlet 4.x in the JavaDoc as I have no idea what version this will translate to. Also not sure whether you want to mention that this is Jakarta Servlet or something.

Also I was wondering if you were happy with the current wording of the JavaDoc.

joakime

+1 from me.

gregw

A few more changes... but almost there!

api/src/main/java/jakarta/servlet/SessionCookieConfig.java

api/src/main/java/jakarta/servlet/http/Cookie.java

aldaris · 2020-01-16T23:31:20Z

api/src/main/java/jakarta/servlet/http/Cookie.java

+         *
+         * <p>
+         * Note that this mode is browser dependent, and browsers may require cookies to have the {@literal Secure}
+         * attribute to be set at the same time.


/*
I wonder if this should mention the horrors of:
https://www.chromium.org/updates/same-site/incompatible-clients
Chances are it would turn outdated eventually though.
*/

I think a general warning that browsers may interpret spec differently is fine.... Perhaps we should say that on every feature :)

aldaris · 2020-01-16T23:38:52Z

If you can give me a final thumbs up on the review, I'll squash my commits so you can merge this. :)

gregw

LGTM

gregw · 2020-01-17T04:48:34Z

@aldaris I can squash and merge from git if you like.

markt-asf · 2020-01-17T15:45:51Z

I thought the plan was not to change the API at all for 5.0. Or is this planned for 5.1? If so, we need to create a 5.0.x branch.

gregw · 2020-01-17T17:10:39Z

@markt-asf good question. But we really need to make changes like this to stay relevant.
Ideally we'd do a 4.1 release in the javax space with this and then make 5.0 the exact jakarta equivalent.... but I think we can't legally make any such changes in javax space.
So this is either in 5.0 or has to be punted to 5.1. Personally I think it would be crazy if our inability to change javax specs resulted in us holding back reasonable maintenance changes like this from a major 5.0 release, thus requiring a 5.1 straight away afterwards.

@bshannon thoughts?

bshannon · 2020-01-18T02:33:04Z

You definitely can't change the javax version.

You can make changes like this in 5.0, but the review process requires you to call them out,
and it may be harder to justify them. If 5.1 is 6 months after 5.0, is this still critical in 5.0?
(6 months is just an example, not a commitment.)

It would probably be best to engage with the platform project team and/or the spec committee
to see what the reaction to something like this is going to be before you commit to it.

gregw · 2020-01-18T04:53:47Z

The argument to include this feature in 5.0 is that browsers already support this feature, there is demand for its usage and containers are already adding non-standard configuration to support it. To not have it in 5.0 just makes the standard less relevant to current needs.

For now I'll leave this unmerged until we engage with platform project team and/or the spec committee and develop firm plans for 5.0 and 5.1

Signed-off-by: Peter Major <peter.major@forgerock.com>

stuartwdouglas

There are some minor changes related to now targetting 5.1, but other than that I think it is mostly ready to go.

Once this goes in we need to add some TCK tests for it though, and we have not really sorted out exactly how that is going to work yet.

stuartwdouglas · 2020-10-13T23:24:08Z

api/src/main/java/jakarta/servlet/SessionCookieConfig.java

+     *
+     * @see jakarta.servlet.http.Cookie#setSameSite(SameSite)
+     *
+     * @since Servlet 5.0


Needs to be 5.1 now

And replace <tt> over all place by <code> or {@code}. The <tt> is disallowed since the HTML5 era and newer versions of the Javadoc tool is prone to error out on them.

stuartwdouglas · 2020-10-13T23:24:15Z

api/src/main/java/jakarta/servlet/SessionCookieConfig.java

+     *
+     * @see jakarta.servlet.http.Cookie#getSameSite
+     *
+     * @since Servlet 5.0


stuartwdouglas · 2020-10-13T23:25:11Z

api/src/main/java/jakarta/servlet/http/Cookie.java

+     * @param sameSite the <i>SameSite</i> attribute's value; may be null if the attribute should be a container
+     *                 provided context default or not set if there is no context default.
+     *
+     * @since Servlet 5.0


stuartwdouglas · 2020-10-13T23:25:17Z

api/src/main/java/jakarta/servlet/http/Cookie.java

+     * @return the <i>SameSite</i> attribute associated with this cookie; may be null to indicate that the attribute
+     *         should be a container provided context default or not set if there is no context default.
+     *
+     * @since Servlet 5.0


stuartwdouglas · 2020-10-13T23:27:33Z

api/src/main/java/jakarta/servlet/http/Cookie.java

+    /**
+     * The available values that can be used with the cookie's <i>SameSite</i> attribute.
+     */
+    public enum SameSite {


IMHO this should be a javadoc only feature. I don't see any benefit to including this in the ServletContext.

stuartwdouglas · 2020-10-13T23:30:50Z

This also needs an entry in the changelog of the spec document

s3curitybug · 2021-02-21T19:47:27Z

Any update on this?

s3curitybug · 2021-03-15T09:32:06Z

Could someone show up and explain what is happening with this PR? I've been willing to set the samesite attribute in my cookies for so long and this PR has been blocked for months, apparently waiting for small changes.

thenatog · 2021-03-23T01:27:48Z

Would be great to see this one merged in.

BalusC · 2021-04-17T12:37:48Z

api/src/main/java/jakarta/servlet/http/Cookie.java

+         * Retrieves the {@link SameSite} instance from its String representation.
+         *
+         * @param value the String to be converted
+         * @return the corresponding {@link SameSite} enum


It should document @throws IllegalArgumentException as well for the unhappy path.

dagnelies · 2021-04-25T21:24:25Z

Oh man, this should have been merged ages ago.

It was reported in #175 in 2017, you got a generous PR in 2019 and now is 2021 and it's still in limbo.

I never imagined it would take so many years to add a damn property to cookies. Everybody builds workarounds since ages because of this!

Can't you just swap the @since 5.0 into @since 5.1 and be done with it?! It's soooo overdue.

My apologies for the rudeness.

BalusC · 2021-04-26T10:29:09Z

It was proposed 2016 https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-cookie-same-site but indeed still hasn't yet ended up in the standard spec, they're after 5 years still drafting it https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-07 (7th revision!) The current PR uses a predefined enum instead of a free text string and this moving target is very prone to cause forward and backward compatibility problems in the future.

It's much better to add a generic Cookie#setAttribute method as suggested here #175 (comment) until they have crystallized it into the spec.

BalusC · 2021-04-26T11:58:26Z

I've prepped a PR to add Cookie#setAttribute: #399

* Add Cookie#setAttribute #175 #271 * Skip null value from check for reserved token * Alternative approach to cookie attribute handling github.com//pull/399#discussion_r624276777 * Make sure path is referenced via constant * In hindsight, token check doesn't need to be performed on value * Match casing with spec; mapping is case insensitive anyway * Optimized Cookie to lazily create attribute map * Added junit test * Updates from review * Fixed quote * surefire plugin * Updates from review * Updates from review Co-authored-by: Bauke Scholtz <balusc@gmail.com>

markt-asf · 2021-10-07T09:48:31Z

This has been superseded by the addition of generic attribute support.

Lonzak · 2021-10-07T10:00:54Z

@markt-asf Do you have a link for the generic attribute support? To which version was this added? This would make it more future proof - nice.

markt-asf · 2021-10-07T10:12:20Z

#401 - it will be in Servlet 6.0

Lonzak · 2021-10-07T10:21:13Z

Damn 6.0 is quite late - so now there is no solution for 5.1, right?

pizzi80 · 2023-12-06T10:59:36Z

I think this issue should be reopened and implemented in Servlet 6.1

The SameSite attribute it's too important to be declassed as a generic attribute,
there are a lot of conseguences around not setting or setting the wrong value!

If I use the generic setAttribute method and make a typo,
the webapp may no longer work properly!

For real world developers it's really important to have a set-in-stone fundamental attribute like this one

https://web.dev/articles/samesite-cookie-recipes?hl=en
https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#creating_cookies

BalusC · 2023-12-06T11:53:37Z

SameSite spec is still in draft mode: https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-13 13th cycle already!
We really want to put it in Servlet spec but as long as they don't we can't.

joakime · 2023-12-06T13:59:47Z

@pizzi80 I actually think the generic setAttribute method is ideal.

Esp considering how quickly the Cookie spec is evolving.

SameSite attribute - https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-13#section-4.1.2.7
Partitioned attribute - https://developers.google.com/privacy-sandbox/3pcd#partitioned and https://developer.mozilla.org/en-US/docs/Web/Privacy/Partitioned_cookies
Cookie name prefixes (and validation associated with it) - https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-rfc6265bis-13#section-4.1.3

It's not ideal to have to wait for another Servlet spec release (and associated implementations) to handle the evolution of Cookie in your webapp.

dagnelies · 2023-12-06T14:02:55Z

We really want to put it in Servlet spec but as long as they don't we can't.

Why can't you?

There is the spec committee on one side and reality on the other.
Chrome supports SameSite since 2016, Firefox since 2018 and all other browsers since many years already. Everybody uses it, everybody supports it ...except for Java Servlets ...still missing it 7 years later ....because of the "status" of some document. Seriously, what's the harm of adding this?! Everybody needs it.

joakime · 2023-12-06T14:04:24Z

If I use the generic setAttribute method and make a typo,
the webapp may no longer work properly!

You can make typos in many places in Servlet and your webapp may no longer work.
(eg: url-patterns, request-dispatchers, query strings, pretty much most response headers, Content-Type, CORS, Content-Encoding, etc)

BalusC · 2023-12-06T14:38:24Z

Chrome supports SameSite since 2016, Firefox since 2018 and all other browsers since many years already

Browsers don't represent a spec.

Seriously, what's the harm of adding this

Jakarta EE isn't a random "3rd party library". Jakarta EE is also a spec.

Just use setAttribute(). It will be there forever. Direct future complaints about unfinished HTTP specs to HTTP spec guys please, not to us.

aldaris mentioned this pull request Oct 14, 2019

SameSite Cookie Support #175

Closed

aldaris force-pushed the samesite-cookie branch from 8010c93 to 637f1cd Compare November 16, 2019 23:37

aldaris force-pushed the samesite-cookie branch from 637f1cd to 7b4549f Compare January 6, 2020 10:01

ymajoros suggested changes Jan 15, 2020

View reviewed changes

api/src/main/java/jakarta/servlet/http/Cookie.java Outdated Show resolved Hide resolved

aldaris force-pushed the samesite-cookie branch from 934de5d to 6e10cb3 Compare January 15, 2020 10:34

gregw requested changes Jan 15, 2020

View reviewed changes

api/src/main/java/jakarta/servlet/http/Cookie.java Show resolved Hide resolved

api/src/main/java/jakarta/servlet/http/Cookie.java Outdated Show resolved Hide resolved

ymajoros suggested changes Jan 15, 2020

View reviewed changes

api/src/main/java/jakarta/servlet/http/Cookie.java Outdated Show resolved Hide resolved

gregw mentioned this pull request Jan 16, 2020

Same site cookie config jakartaee/jakartaee-schemas#1

Closed

joakime reviewed Jan 16, 2020

View reviewed changes

api/src/main/java/jakarta/servlet/http/Cookie.java Outdated Show resolved Hide resolved

joakime suggested changes Jan 16, 2020

View reviewed changes

api/src/main/java/jakarta/servlet/http/Cookie.java Outdated Show resolved Hide resolved

joakime approved these changes Jan 16, 2020

View reviewed changes

gregw requested changes Jan 16, 2020

View reviewed changes

api/src/main/java/jakarta/servlet/SessionCookieConfig.java Outdated Show resolved Hide resolved

api/src/main/java/jakarta/servlet/SessionCookieConfig.java Outdated Show resolved Hide resolved

api/src/main/java/jakarta/servlet/http/Cookie.java Outdated Show resolved Hide resolved

aldaris commented Jan 16, 2020

View reviewed changes

gregw approved these changes Jan 17, 2020

View reviewed changes

aldaris force-pushed the samesite-cookie branch from dc3041b to f82c842 Compare January 17, 2020 06:23

gregw added this to In Progress in 5.0 via automation Jan 18, 2020

chkal mentioned this pull request Apr 18, 2020

NewCookie should support SameSite attribute jakartaee/rest#862

Closed

chkal mentioned this pull request May 2, 2020

Add support for SameSite cookie attribute to NewCookie class jakartaee/rest#868

Merged

aldaris added 2 commits May 7, 2020 11:01

jakartaee#175 Add support for SameSite cookies

c399b13

Signed-off-by: Peter Major <peter.major@forgerock.com>

review comments

309082b

Signed-off-by: Peter Major <peter.major@forgerock.com>

aldaris force-pushed the samesite-cookie branch from f82c842 to 309082b Compare May 7, 2020 14:50

melloware mentioned this pull request Jun 17, 2020

Missing Secure Attribute in Encrypted Session (SSL) Cookie primefaces/primefaces#6041

Merged

vladak mentioned this pull request Sep 22, 2020

OpenGrokProject cookie misusing the sameSite attribute oracle/opengrok#3164

Closed

stuartwdouglas requested changes Oct 13, 2020

View reviewed changes

csaltos mentioned this pull request Apr 6, 2021

Investigate SameSite Cookie Support lift/framework#1828

Closed

BalusC reviewed Apr 17, 2021

View reviewed changes

BalusC mentioned this pull request Apr 26, 2021

Add Cookie#setAttribute #399

Closed

markt-asf closed this Oct 7, 2021

#175 Add support for SameSite cookies #271

#175 Add support for SameSite cookies #271

Conversation

aldaris commented Oct 14, 2019 • edited by gregw

joakime commented Nov 15, 2019

joakime commented Nov 15, 2019

yang-wei commented Jan 6, 2020

aldaris commented Jan 6, 2020

yang-wei commented Jan 8, 2020

driverpt commented Jan 8, 2020

yang-wei commented Jan 9, 2020

joakime left a comment

Choose a reason for hiding this comment

aldaris commented Jan 16, 2020 • edited

joakime left a comment

Choose a reason for hiding this comment

gregw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldaris commented Jan 16, 2020

gregw left a comment

Choose a reason for hiding this comment

gregw commented Jan 17, 2020

markt-asf commented Jan 17, 2020

gregw commented Jan 17, 2020

bshannon commented Jan 18, 2020

gregw commented Jan 18, 2020

stuartwdouglas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BalusC Apr 17, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuartwdouglas commented Oct 13, 2020

s3curitybug commented Feb 21, 2021

s3curitybug commented Mar 15, 2021

thenatog commented Mar 23, 2021

Choose a reason for hiding this comment

dagnelies commented Apr 25, 2021 • edited

BalusC commented Apr 26, 2021 • edited

BalusC commented Apr 26, 2021

markt-asf commented Oct 7, 2021

Lonzak commented Oct 7, 2021

markt-asf commented Oct 7, 2021

Lonzak commented Oct 7, 2021

pizzi80 commented Dec 6, 2023

BalusC commented Dec 6, 2023 • edited

joakime commented Dec 6, 2023 • edited

dagnelies commented Dec 6, 2023 • edited

joakime commented Dec 6, 2023

BalusC commented Dec 6, 2023 • edited

aldaris commented Oct 14, 2019 •

edited by gregw

aldaris commented Jan 16, 2020 •

edited

BalusC Apr 17, 2021 •

edited

dagnelies commented Apr 25, 2021 •

edited

BalusC commented Apr 26, 2021 •

edited

BalusC commented Dec 6, 2023 •

edited

joakime commented Dec 6, 2023 •

edited

dagnelies commented Dec 6, 2023 •

edited

BalusC commented Dec 6, 2023 •

edited