Skip to content

Conversation

@bugthunk
Copy link
Contributor

No description provided.

@bugthunk bugthunk changed the title Add support for setting SameSite attribute Add support for setting SameSite attribute in cookies Sep 10, 2020
@bugthunk
Copy link
Contributor Author

This PR addresses #64

@stepcut
Copy link
Member

stepcut commented Sep 11, 2020

Thanks for taking time to submit this pull request. I see a few fixable issues:

1. The Read/Show instances for CookieXOriginOption should be derived

Show and Read are supposed to produce and parse Haskell code. But you are using them to unparse/parse HTTP.

show is supposed to produce a String which contains a Haskell expression equal to the one you are showing. So, if we do, putStrLn $ show SameSiteLax that will print SameSiteLax on the screen. If we copied that into GHCi, that would evaluate to SameSiteLax.

With your instance, it would print Lax, and if we copied that into GHCi, it would have no idea what we are talking about.

This is covered more explicitly here, though it is even harder to understand,

http://hackage.haskell.org/package/base-4.6.0.1/docs/Text-Show.html

2. cookiesParser does not actually parse the samesite value

It looks like cookiesParser always hardcodes the value to SameSiteNoValue. If the client strips the SameSite value from the cookie before sending it back to the server, then may that is ok. But if the client is supposed to send that back to the server, then it seems like the parser should actually parse it? I also wonder if there are situations where the server might need to parse that value -- such as when it is being used as a proxy or reverse proxy server.

3. the test suite fails

Because the Cookie constructor changed, the test suit fails. Please fix the test suite. If you are feeling especially inspired, you could also add some tests for the new cookie option. (As a side note, because the Cookie constructor changed, the new version needs to be 7.7.0 not 7.6.2)

4. bike shedding

Instead of,

    , sameSiteOpts  :: CookieXOriginOption

Why not just have:

    , sameSite  :: SameSite

Could CookieXOriginOption have constructors that were not samesite values? At the very least, let's drop the Opts and Option bits. Seems redundant, and none of the other cookie parameters use that naming. It should maybe be cookieSameSite except httpOnly and secure already broke that pattern. So sameSite is fine.

@bugthunk
Copy link
Contributor Author

Thanks for the feedback. Fixing tomorrow as my son broke his arm. Yes, it happened...

@bugthunk
Copy link
Contributor Author

  1. (Show/Read) Quite right, of course. I'm too used to hacks.
  2. (Not parsing SameSite) The SameSite attribute is a client-only setting and is not sent back to the server, i.e. it works the same as the httpOnly and secure parts of the data type. Therefore, I gave the new field the same treatment.
  3. (Test suite failure) IDK why this fell between the cracks. SNAFU I guess.
  4. (Bike shedding) Your suggestion is probably a lot clearer. I was thinking future-proofing but...really unnecessary, as it often is.

@bugthunk
Copy link
Contributor Author

"ping"? Very carefully and knowing that we're all very busy of course :)

@bugthunk
Copy link
Contributor Author

...here comes a careful ping again as per instructions from my dear lead developer.

@stepcut
Copy link
Member

stepcut commented Sep 29, 2020

Thanks for the reminder. Sorry about the delay -- I am in the midst of a 1000 mile move across the country. I have added a task to the project planning software to pull this bug -- so I can't possibly forget. I'll likely pull in the next couple days.

@bugthunk
Copy link
Contributor Author

Hi! Did your move go well?

@stepcut stepcut merged commit 65fd0f5 into Happstack:master Oct 21, 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.

2 participants