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

Merge pull request #3054 from bfdes/http4s-3044 #3054

Merged
merged 5 commits into from Jan 13, 2020
Merged

Merge pull request #3054 from bfdes/http4s-3044 #3054

merged 5 commits into from Jan 13, 2020

Conversation

bfdes
Copy link
Contributor

@bfdes bfdes commented Jan 10, 2020

Fixes #3044.

Please suggest where I should add additional test cases.

According to the web.dev article linked in the issue:

  • SameSite=Lax by default
  • Must not set SameSite=None without Secure flag

Copy link
Contributor

@hamnis hamnis left a comment

First of all, Thanks for your PR.

Valid build error, there is a stack-overflow.
You also need to run scalafmt

@bfdes
Copy link
Contributor Author

@bfdes bfdes commented Jan 11, 2020

I've fixed the stack-overflow problem -- it was caused by infinite recursion in a render method.

The Scala 2.13 job is still failing though, albeit in an inconsistent manner. It passed for the build that fixed the recursion problem but failed for the subsequent commits, which were only formatting changes.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jan 12, 2020

Test failure is the standard irritating flake, so I restarted.

@@ -113,7 +114,8 @@ final case class ResponseCookie(
maxAge.foreach(writer.append("; Max-Age=").append(_))
domain.foreach(writer.append("; Domain=").append(_))
path.foreach(writer.append("; Path=").append(_))
if (secure) writer.append("; Secure")
writer.append("; SameSite").append(sameSite)
Copy link
Member

@rossabaker rossabaker Jan 12, 2020

Choose a reason for hiding this comment

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

Does this render the = somehow?

Copy link
Contributor Author

@bfdes bfdes Jan 12, 2020

Choose a reason for hiding this comment

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

No, that was an oversight on my part. Addressed in 6df6614.

@@ -98,6 +98,7 @@ final case class ResponseCookie(
maxAge: Option[Long] = None,
domain: Option[String] = None,
path: Option[String] = None,
sameSite: SameSite = SameSite.Lax,
Copy link
Member

@rossabaker rossabaker Jan 12, 2020

Choose a reason for hiding this comment

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

I wonder if this should be an Option, so we can round trip values, or continue to send cookies without this attribute. Semantically, Lax is the default, but by keeping it optional, it would remain possible to send cookies without this attribute.

Copy link
Contributor Author

@bfdes bfdes Jan 12, 2020

Choose a reason for hiding this comment

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

By round trip do you mean that the cookie should be returned unchanged if nothing is done? In which case would I also need to remove this change:

if (secure || sameSite == SameSite.None) writer.append("; Secure")

and go back to

if (secure) writer.append("; Secure")

Copy link
Member

@rossabaker rossabaker Jan 12, 2020

Choose a reason for hiding this comment

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

Ooh, yeah. That's unfortunate, because as is, we're trying to do the right thing by default here. And generally we make sure that we can go model -> string -> model, which is different from string -> model -> string. This is probably better how you wrote it.

Copy link
Contributor Author

@bfdes bfdes Jan 12, 2020

Choose a reason for hiding this comment

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

If this behaviour is desired do you think it is worth writing an (explicit) test for it?

hamnis
hamnis approved these changes Jan 12, 2020
@@ -98,6 +98,7 @@ final case class ResponseCookie(
maxAge: Option[Long] = None,
domain: Option[String] = None,
path: Option[String] = None,
sameSite: SameSite = SameSite.Lax,
Copy link
Member

@rossabaker rossabaker Jan 12, 2020

Choose a reason for hiding this comment

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

Ooh, yeah. That's unfortunate, because as is, we're trying to do the right thing by default here. And generally we make sure that we can go model -> string -> model, which is different from string -> model -> string. This is probably better how you wrote it.

@@ -367,6 +368,7 @@ object CSRF {
httpOnly: Boolean,
domain: Option[String] = None,
path: Option[String] = None,
sameSite: SameSite = SameSite.Lax,
Copy link
Contributor Author

@bfdes bfdes Jan 12, 2020

Choose a reason for hiding this comment

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

@rossabaker @hamnis do you mind double checking if this is appropriate?
I wasn't sure, because the middleware is supposed to be a complementary mechanism for combating CSRF.

Copy link
Contributor

@hamnis hamnis Jan 13, 2020

Choose a reason for hiding this comment

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

SameSite helps with CSRF, so I do think this is fine.

@ChristopherDavenport ChristopherDavenport changed the title SameSite cookie support Merge pull request #3054 from bfdes/http4s-3044 Jan 13, 2020
@ChristopherDavenport ChristopherDavenport merged commit 4fa0242 into http4s:master Jan 13, 2020
2 checks passed
@bfdes bfdes deleted the http4s-3044 branch Jan 13, 2020
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.

4 participants