search

yesodweb / yesod

A RESTful Haskell web framework built on WAI.
http://www.yesodweb.com/
MIT License
2.64k stars 374 forks source link

Yesod auth email logout CSRF issue #1188

Open lethjakman opened 8 years ago

lethjakman commented 8 years ago

So now that most of the auth email forms function there seems to be one hangup left. Logout. It looks like the way it's currently working is a GET is run to /auth/logout, and then it's redirected to a POST where the clearing of the credentials happens. This seems confusing to me. What are your thoughts on changing the paradigm here? I have several ideas:

  1. Setting up a helper to generate a blank form with the CSRF token that posts directly to /auth/logout that's styled to look like a link.
  2. Using unobtrusive javascript like in Ruby on Rails to hijack the link based on a "method" attribute.
  3. Disabling CSRF for this form. This seems reasonable because all that could happen is removal of the cookie (which would be annoying...but not destructive. This would also be my least preferred method.)
snoyberg commented 8 years ago

It doesn't seem like (1) is very different from how we do things today, just with the added CSRF token in the intermediate generated form, right?

On Thu, Mar 17, 2016 at 6:03 AM, alex notifications@github.com wrote:

So now that most of the auth email forms function there seems to be one hangup left. Logout. It looks like the way it's currently working is a GET is run to /auth/logout, and then it's redirected to a POST where the clearing of the credentials happens. This seems confusing to me. What are your thoughts on changing the paradigm here? I have several ideas:

  1. Setting up a helper to generate a blank form with the CSRF token that posts directly to /auth/logout that's styled to look like a link.
  2. Using unobtrusive javascript like in Ruby on Rails to hijack the link based on a "method" attribute.
  3. Disabling CSRF for this form. This seems reasonable because all that could happen is removal of the cookie (which would be annoying...but not destructive. This would also be my least preferred method.)

— You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub https://github.com/yesodweb/yesod/issues/1188

lethjakman commented 8 years ago

Maybe there's some sort of helper that I don't know of. I couldn't find anything that would generate just a form with a method of choice, a CSRF token, and a button with some text. For (1) I was suggesting creating a helper that generates something like this with a CSRF token. This would allow you to remove this code and just post directly to the logout link. This seems to me like it would be cleaner, a bit less confusing, and it would save the request time of a redirect (which obviously isn't much, but it is something.) Depending on how the helper was written it could also allow for additional capabilities like making easy DELETE buttons/links. Similar things are also available through unobtrusive javascript, for example, this is how Rails does it here. Does something like this already exist and I just don't know about it?

snoyberg commented 8 years ago

I see what you mean about generating a form for the logout instead of a simple link. That can work, but I don't believe will work in all cases: some users are already generating links to AuthR LogoutR, and we want that to continue to work. So updating the generated form in the intermediate GET page to include the CSRF token seems prudent.

lethjakman commented 8 years ago

I see exactly what you mean. I could try and toss that in tonight if no one else does it first. What about the form helper ideas, do you see any of them being particularly useful to this project? I wouldn't mind putting something together if I knew which one you thought was best.

snoyberg commented 8 years ago

If by form helper, you mean a more extensible version of redirectToPost, that seems reasonable to me.

lethjakman commented 8 years ago

Hmmm, so this is working now that I've updated my Yesod. It looks like it was fixed here a few months ago so I'm not sure why it wasn't working locally. Anyways, I was just thinking of writing a helper that would help avoid the need for redirectToPost in the future. How about I submit a PR for this and you tell me what your thoughts are on it? It might take me a few days to find the time, but I think I can get something working.

MaxGabriel commented 8 years ago

Using unobtrusive javascript like in Ruby on Rails to hijack the link based on a "method" attribute.

It sounds like this auth logout issue is resolved, but I know at least one person has asked for that: http://stackoverflow.com/q/28345806/1176156. I think it's a good idea, since it's pretty common to have e.g. links to delete things.

snoyberg commented 8 years ago

A PR would be good, I think I may be having a little trouble picturing what it would look like right now.

lethjakman commented 8 years ago

OK, so rather than a PR I made a few examples because I wasn't completely happy with my helper and have some questions.

Haskell version

What I like:

Javascript version

What I like:

What are your thoughts? Obviously, these are a little rough.

Additionally, I have a few questions on the Yesod version:

  1. Is there a way to infer your request type based on your Route?
  2. Is there a way to add a class only when the Maybe is Just and not add the class if it's a Nothing without using a $maybe block and repeating the code?
snoyberg commented 8 years ago

The Haskell version makes a lot moe sense to me. I'm not sure what the advantage is of using Ajax for these links.

lethjakman commented 8 years ago

The only real benefit of the ajax version is that it works when you dynamically add a link to the page (via javascript) and you don't have to create a whole form. It doesn't really make a lot of sense with the way HTML was designed, but it's a common way of doing things in the Rails world.

OK. I have a Haskell version working, and I'd like to file a PR, but I have two questions.

  1. Is it possible to put an external lucius file in the core so that it's not just pasted into the HTML every time someone uses a widget? I looked for an example, but couldn't find any.
  2. Where would you like something like this? I was kind thinking Yesod.Forms.LinkHelper, but I'm not sure.
snoyberg commented 8 years ago
  1. You can just use [lucius|...|] inside the Widget
  2. Is this substantively different from the existing redirectToPost? Is there a reason to avoid putting these two functions together in yesod-core?
lethjakman commented 8 years ago

In my opinion they are fairly different. Redirect to post generates an extra page with the redirect while this is more of a form helper and runs the correct post method in your browser. It's not really a Handler.

Thank you for the quick response!

On Fri, Apr 8, 2016, 01:59 Michael Snoyman notifications@github.com wrote:

  1. You can just use [lucius|...|] inside the Widget
  2. Is this substantively different from the existing redirectToPost? Is there a reason to avoid putting these two functions together in yesod-core?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/yesodweb/yesod/issues/1188#issuecomment-207289712

snoyberg commented 8 years ago

I guess my more direct question is: does this have any dependency on the rest of the yesod-form machinery, or is it relying on just functionality in yesod-core itself?

On Fri, Apr 8, 2016 at 3:33 PM, alex notifications@github.com wrote:

In my opinion they are fairly different. Redirect to post generates an extra page with the redirect while this is more of a form helper and runs the correct post method in your browser. It's not really a Handler.

Thank you for the quick response!

On Fri, Apr 8, 2016, 01:59 Michael Snoyman notifications@github.com wrote:

  1. You can just use [lucius|...|] inside the Widget
  2. Is this substantively different from the existing redirectToPost? Is there a reason to avoid putting these two functions together in yesod-core?

— You are receiving this because you authored the thread. Reply to this email directly or view it on GitHub https://github.com/yesodweb/yesod/issues/1188#issuecomment-207289712

— You are receiving this because you commented. Reply to this email directly or view it on GitHub https://github.com/yesodweb/yesod/issues/1188#issuecomment-207413509

lethjakman commented 8 years ago

I see what you mean about yesod-core. It does not depend on yesod-form. I have filed a pull request: #1211