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

Added support for the SameSite attribute in Cookies #10050

Merged
merged 8 commits into from Mar 12, 2020

Conversation

@dvlato
Copy link
Contributor

dvlato commented Feb 21, 2020

Motivation:

Netty currently does not support the SameSite attribute for response cookies (see issue #8161 for discussion).

Modifications:

The attribute has been added to the DefaultCookie class as a quick fix since adding new methods to the Cookie interface would be backwards-incompatible.
ServerCookieEncoder and ClientCookieDecoder have been updated accordingly to process this value. No validation for allowed values (Lax, None, Strict) has been implemented.

Result:
Response cookies with the SameSite attribute set can be read or written by Netty.
Fixes #8161

Motivation:

Netty currently does not support the SameSite attribute for response cookies (see issue #8161 for discussion).

Modifications:

The attribute has been added to the DefaultCookie class as a quick fix since adding new methods to the Cookie interface would be backwards-incompatible.
ServerCookieEncoder and ClientCookieDecoder have been updated accordingly to process this value. No validation for allowed values (Lax, None, Strict) has been implemented.

Result:

Response cookies with the SameSite attribute set can be read or written by Netty.
@netty-bot

This comment has been minimized.

Copy link

netty-bot commented Feb 21, 2020

Can one of the admins verify this patch?

Copy link
Contributor

bryce-anderson left a comment

Generally looks good. I'm not strongly opinionated about using an enum though it could save a string allocation. I agree on needing the wire representation of "None" since otherwise None is not possible with Chrome and soon to be the rest of the browsers.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 24, 2020

/cc @trustin as well

@dvlato dvlato requested review from normanmaurer and slandelle Feb 25, 2020
@normanmaurer normanmaurer requested a review from bryce-anderson Feb 25, 2020
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 26, 2020

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 26, 2020

@netty-bot test this please

Copy link
Contributor

slandelle left a comment

According to spec, SameSite value matching is case sensitive, so we shouldn't trying to fallback to equalsIgnoreCase.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 26, 2020

ccording to spec, SameSite value matching is case sensitive, so we should trying to fallback to equalsIgnoreCase.

@slandelle shouldn't ?

@slandelle

This comment has been minimized.

Copy link
Contributor

slandelle commented Feb 26, 2020

@normanmaurer fixed comment

David Latorre
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 26, 2020

@netty-bot test this please

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Feb 27, 2020

@netty-bot test this please

@dvlato can you also sign our ICLA if you did not do yet:
https://netty.io/s/icla

}
}
}
return null;

This comment has been minimized.

Copy link
@johnou

johnou Feb 28, 2020

Contributor

should be non null, according to spec it should fallback to None..?

https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-05#section-4.1

If the "SameSite" attribute's value is "Strict", the cookie will only
be sent along with "same-site" requests. If the value is "Lax", the
cookie will be sent with same-site requests, and with "cross-site"
top-level navigations, as described in Section 5.3.7.1. If the value
is "None", the cookie will be sent with same-site and cross-site
requests. If the "SameSite" attribute's value is something other
than these three known keywords, the attribute's value will be
treated as "None".

This comment has been minimized.

Copy link
@dvlato

dvlato Feb 28, 2020

Author Contributor

I understand "will be treated" to refer to the behaviour explained in the paragraph regarding whether the cookie will be sent or not:

If the value is "None", the cookie will be sent with same-site and cross-site requests

Both here and in the normative section (https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-05#section-5.3.7) it mentions behaviour or "treated as" and in no case it seems to mention we should replace the value on the wire.

That section might mean we should always keep the received value for SameSite but we have agreed on the contrary. In any case, cookies are most likely going to be revisited in the next major version - I would suggest that we could keep unknown attributes as well and not only unknown values, so things like this "new" SameSite attribute don't impact netty users.

@normanmaurer normanmaurer requested a review from slandelle Feb 28, 2020
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 2, 2020

@slandelle @bryce-anderson any other comments ?

@slandelle

This comment has been minimized.

Copy link
Contributor

slandelle commented Mar 2, 2020

LGTM

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 3, 2020

@netty-bot test this please

@normanmaurer normanmaurer added this to the 4.1.47.Final milestone Mar 3, 2020
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 3, 2020

If I don't hear back from @bryce-anderson today I will merge tomorrow :)

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 4, 2020

@dvlato can you please sign our ICLA and let me know once done ? After this is done I will merge this one.

https://netty.io/s/icla

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 5, 2020

@dvlato ping :)

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 9, 2020

Once the ICLA was signed by @dvlato I will pull this in. Great work

@dvlato

This comment has been minimized.

Copy link
Contributor Author

dvlato commented Mar 12, 2020

Hi @normanmaurer , I have signed the ICLA now. Sorry for the delay.

Thanks a lot for the reviews and I hope this PR can help until there is a proper fix for this!

@normanmaurer normanmaurer merged commit e4af5c3 into netty:4.1 Mar 12, 2020
3 checks passed
3 checks passed
pull request validation (centos6-java11) Build finished.
Details
pull request validation (centos6-java12) Build finished.
Details
pull request validation (centos6-java8) Build finished.
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 12, 2020

@dvlato thanks!

normanmaurer added a commit that referenced this pull request Mar 12, 2020
Motivation:

Netty currently does not support the SameSite attribute for response cookies (see issue #8161 for discussion).

Modifications:

The attribute has been added to the DefaultCookie class as a quick fix since adding new methods to the Cookie interface would be backwards-incompatible.
ServerCookieEncoder and ClientCookieDecoder have been updated accordingly to process this value. No validation for allowed values (Lax, None, Strict) has been implemented.

Result:

Response cookies with the SameSite attribute set can be read or written by Netty.

Co-authored-by: David Latorre <a-dlatorre@hotels.com>
@dvlato dvlato deleted the dvlato:issue-354-samesite-attribute branch Mar 12, 2020
gnehcgnaw pushed a commit to gnehcgnaw/netty that referenced this pull request Mar 13, 2020
* '4.1' of https://github.com/netty/netty: (614 commits)
  Use WebSocketVersion.toAsciiString() as header value when possible (netty#10105)
  Don't override HOST header if provided by user already in WebSocketClientHandshaker (netty#10104)
  Added support for the SameSite attribute in Cookies (netty#10050)
  Let LzfEncoder support length aware ability. (netty#10082)
  Fix WebSocketClientHandshaker not generating correct handshake request when path is empty (netty#10095)
  [maven-release-plugin] prepare for next development iteration
  [maven-release-plugin] prepare release netty-4.1.47.Final
  Replace several magic numbers. (netty#10094)
  Add a HTTP/2 client example using the newer frames approach the current HTTP/2 client example uses the older 'HTTP/1 <--> HTTP/2' translation approach. (netty#10081)
  Add a log level check simply before logging. (netty#10093)
  Use MacOSDnsServerAddressStreamProvider when on the classpath and we … (netty#10079)
  http multipart decode with chinese chars should work (netty#10089)
  Add log level check simply before logging. (netty#10080)
  Update ProtocolDetectionResult to fix typo (netty#10086)
  Make sure we always flush window update frames in AbstractHttp2StreamChannel (netty#10075)
  Fix AssertionError in ScheduledFutureTask (netty#10073)
  Add test cases for StringUtil. (netty#10074)
  Preserve order when using alternate event loops (netty#10069)
  Add test cases for MathUtil. (netty#10071)
  [maven-release-plugin] prepare for next development iteration
  ...

# Conflicts:
#	pom.xml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.