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

Add support for the SameSite cookie attribute. #607

Merged
merged 8 commits into from
Feb 10, 2020

Conversation

dvlato
Copy link
Contributor

@dvlato dvlato commented Feb 6, 2020

This PR handles issue #604 . Description of the attribute:
https://web.dev/samesite-cookies-explained/

*
* @see ClientCookieEncoder
*/
public final class ClientCookieDecoder {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class needs to be package private, ideally.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All external references removed and it's package private now (so we are using Netty's original ClientCookieDecoder in some tests).

@dvlato dvlato marked this pull request as ready for review February 7, 2020 16:34
@@ -81,7 +77,8 @@ public String encode(String name, String value) {
* @param cookie the cookie
* @return a single Set-Cookie header value
*/
public String encode(Cookie cookie) {
public String encode(NettyCookie cookie) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could possibly be package private.

@@ -146,7 +147,7 @@ public String encode(Cookie cookie) {
* @param cookies a bunch of cookies
* @return the corresponding bunch of Set-Cookie headers
*/
public List<String> encode(Cookie... cookies) {
public List<String> encode(NettyCookie... cookies) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be package private too, AFAICS.

@@ -194,20 +195,20 @@ public String encode(Cookie cookie) {
* @param cookies a bunch of cookies
* @return the corresponding bunch of Set-Cookie headers
*/
public List<String> encode(Iterable<? extends Cookie> cookies) {
Iterator<? extends Cookie> cookiesIt = requireNonNull(cookies, "cookies").iterator();
public List<String> encode(Iterable<? extends NettyCookie> cookies) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly all these encode methods could be package private.


client.send(
get("/")
.header(HttpHeaderNames.HOST, styxServer().proxyHttpHostHeader())
Copy link
Contributor

Choose a reason for hiding this comment

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

Static import.

@@ -286,7 +286,7 @@ class OriginsFileCompatibilitySpec : FunSpec() {
}
}

test("!TLS Settings modifications") {
test("TLS Settings modifications") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's keep this disabled.

@dvlato dvlato merged commit 27feb53 into ExpediaGroup:master Feb 10, 2020
@normanmaurer
Copy link

I wonder if you are interested in contributing something like this back into netty...

@mikkokar
Copy link
Contributor

I wonder if you are interested in contributing something like this back into netty...

@a-dlatorre I bet you will be looking into this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants