Followup on #54: Make XSRF optional

[ghost]

This changeset resolves @jkarni's request for @3noch to fix the tests in #54 which resulted in that PR being reverted just after it was merged.

It also resolves the 4th todo-item from #64, Add option without cookies.

However, this changeset undoes part of #54, namely its name, Add check-origin method for CSRF. The addition of the versatile hook for rejecting requests seemed like a bigger fish than making XSRF optional, and so #53 remains unfixed.

   data CookieSettings = CookieSettings {
   ...
-  -- | An arbitrary check for the request. Use this to implement validating
-  --   the Origin/Referer headers. Default @const True@.
-  , cookieCheckRequest      :: !(Request -> Bool)
-  } deriving (Generic)

[3noch]

@plredmond Dang that's very kind of you! I unfortunately finished the app using my fork and moved on without any chance to go back and clean it up. Your help is much appreciated, even though it does lack a key feature. But at least it's getting closer.

[ghost]

@3noch No problem! I needed this functionality for something I'm working on. Sorry for cutting out the cookieCheckRequest & associated calls.

[domenkozar]

@plredmond thanks for doing this, I'm going down the same path today wishing for CSRF to be optional.

I have one nitpicky comment - given that this library started with CSRF as a name and that https://en.wikipedia.org/wiki/Cross-site_request_forgery claims XSRF is equivalent to CSRF I see very little benefit changing the naming, confusing existing servant-auth user base.

I'll test these changes today and report back.

[3noch]

@domenkozar

I have one nitpicky comment - given that this library started with CSRF as a name and that https://en.wikipedia.org/wiki/Cross-site_request_forgery claims XSRF is equivalent to CSRF I see very little benefit changing the naming, confusing existing servant-auth user base.

This was my change originally. The library used both CSRF and XSRF in random places so I picked what seemed to be the dominant one and normalized. I think using both is the most confusing of the options.

[domenkozar]

$ git grep -i xsrf | wc -l
45
$ git grep -i csrf | wc -l
27

I didn't see that in the diff - I do agree consistency is important so nevermind my comment.

[domenkozar]

I've tested with defaultCookieSettings { cookieXsrfSetting = Nothing } and can confirm POST requests now have no XSRF checking and NO-XSRF-TOKEN cookie exists.

[domenkozar]

@plredmond could you rebase please?

[ghost]

Sure

[domenkozar]

@plredmond could you also add a test case for clearSession?

[ghost]

Yep. I'll do that Sunday or Monday.

[domenkozar]

@plredmond thanks - I think once it's proven that logic in clearSession works, we can merge this :)

[ghost]

@domenkozar clearSession was working because it cleared the session cookie which prevents further authentication.

However, clearSession logic was unconditionally adding a cookie named NO-XSRF-COOKIE. That has been fixed and tested in 05dd6f5. The new tests have extra calls to GET / to demonstrate the state of the authentication, but I could remove those if you think it's not necessary.

Additionally 5f78fc4 changes all server-spec tests not to rely on the default name for the session cookie. Let me know if you want this reverted.

[ghost]

Ok.. it looks like I can't use a few pieces of wreq or http-client because they aren't a part of the GHC-7.10.3 environment. I've got that environment building locally now and will see if I can get this working.

[ghost]

Hey, I think this is ready for review again. I didn't mention it, but the build for all versions is working now.

…

On Tue, Apr 24, 2018 at 10:23 AM, Domen Kožar ***@***.***> wrote: @plredmond <https://github.com/plredmond> thanks - I think once it's proven that logic in clearSession works, we can merge this :) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#92 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABB19FLUXFHSE5FacOSEgrPfv-8dH2QRks5trzV1gaJpZM4TVqbW> .

[domenkozar]

@plredmond do you think it would be better to delete the cookie instead of setting value to "value"? For example one might check for existence of the cookie if someone is signed in (not in a secure way).

[ghost]

I think you're right that deleting the cookie would be semantically cleaner. I don't know why the original author set the cookie to "value" instead of deleting the cookie. One possible explanation is that cookie deletion was flakey on older browsers. IE. A browser willing to set & read cookies is likely willing to set a cookie to a dummy value (where deletion might not work).

…

On Mon, Jun 4, 2018 at 1:15 PM, Domen Kožar ***@***.***> wrote: @plredmond <https://github.com/plredmond> do you think it would be better to delete the cookie instead of setting value to "value"? For example one might check for existence of the cookie if someone is signed in (not in a secure way). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#92 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABB19NnKbbFUb7A4qaW4c4FnVCXcobuNks5t5WtAgaJpZM4TVqbW> .

[3noch]

The Default instance for SetCookie sets the value to "value" IIRC. The other complication is that the Servant types actually specify exactly 2 cookies, so you'd have to make the types more complex.