-
Notifications
You must be signed in to change notification settings - Fork 594
Add SameSite attribute to Cookies #2928
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 SameSite attribute to Cookies #2928
Conversation
|
Test FAILed. Pull request validation reportMima FailuresOther Failed Tasksakka-http-core / Compile / ./ headerCheck failed because of There are files without headers! akka-http-core / Test / ./ headerCheck failed because of There are files without headers! |
|
Test PASSed. |
...filters/10.1.11.backwards.excludes/1354-add-HttpCookie-SameSite-attribute.backwards.excludes
Outdated
Show resolved
Hide resolved
jrudolph
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @marcospereira. Some comments below. The main theme is around keeping bin compat and how to model the new value between scaladsl and javadsl. WDYT, @raboof?
akka-http-core/src/test/scala/akka/http/scaladsl/model/headers/HttpCookieTest.scala
Outdated
Show resolved
Hide resolved
akka-http-core/src/test/scala/akka/http/javadsl/model/headers/HttpCookieSpec.scala
Show resolved
Hide resolved
akka-http-core/src/main/scala/akka/http/scaladsl/model/headers/HttpCookie.scala
Outdated
Show resolved
Hide resolved
akka-http-core/src/main/scala/akka/http/scaladsl/model/headers/HttpCookie.scala
Outdated
Show resolved
Hide resolved
akka-http-core/src/main/java/akka/http/javadsl/model/headers/SameSite.java
Outdated
Show resolved
Hide resolved
akka-http-core/src/test/scala/akka/http/scaladsl/model/headers/HttpCookieTest.scala
Outdated
Show resolved
Hide resolved
akka-http-core/src/main/java/akka/http/javadsl/model/headers/HttpCookie.java
Show resolved
Hide resolved
|
Great comments! Agreed on all points I think... |
|
Test FAILed. !!! Couldn't read commit file !!! |
...filters/10.1.11.backwards.excludes/1354-add-HttpCookie-SameSite-attribute.backwards.excludes
Show resolved
Hide resolved
|
Test PASSed. |
jrudolph
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems almost good to go. Anything you want to add still, @raboof?
OK AFAICS now! |
ennru
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Deprecating the constructor to prefer apply makes sense to me.
|
Test PASSed. |
akka-http-core/src/main/scala/akka/http/scaladsl/model/ErrorInfo.scala
Outdated
Show resolved
Hide resolved
jrudolph
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the deprecation for this is in it's good to be merged from my POV.
|
Test PASSed. |
Good one, done. Also removed the new |
jrudolph
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
🎆 |
|
We will have to decide if that needs backporting but maybe let's wait until asks for it. |
References #1354.
Status
This is a work in progress since I didn't handle any bin-compat issues yet.