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

Add csrf token to all forms #4

Merged
merged 6 commits into from Aug 29, 2018
Merged

Add csrf token to all forms #4

merged 6 commits into from Aug 29, 2018

Conversation

aturkewi
Copy link
Contributor

Closes #3

Adds CSRF support to all forms.

I was a little torn on how to make this token accessible in the view. I ended up opting to put it in conn.assigns rather than explicitly passing it into Template.details and then down through all the partials. The reason for this was because this would require changing a lot of files and pretty much every template test. If this is the preferred route to go though, I can update the PR to work that way.

I also wasn't sure the best way to put in a test for this, so I just added a test ensuring that the CSRF token was on the page. I also didn't put any tests around :protect_from_forgery because it felt outside of the scope of this project. I would expect the app implementing this package would handle this. I did test using this build of the package in an app that I am working on both with and without :protect_from_forgery and it worked fine both ways (so sending up the csrf token is not a problem even if the protect is not on).

Please let me know if there's anything that needs to be changed or updated!

Copy link
Owner

@tompave tompave left a comment

Choose a reason for hiding this comment

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

Looks good, thanks 👍

@tompave
Copy link
Owner

tompave commented Aug 28, 2018

Actually, sorry, I think that this template needs the hidden field too:

https://github.com/tompave/fun_with_flags_ui/blob/master/lib/fun_with_flags/ui/templates/new.html.eex#L27

@aturkewi
Copy link
Contributor Author

Good catch. Just did a global search to check every <form to ensure the token was there and found a delete form that it was missing from as well. Should be all set now.

@tompave
Copy link
Owner

tompave commented Aug 29, 2018

That's great, thank you.

@tompave tompave merged commit 3213752 into tompave:master Aug 29, 2018
@aturkewi aturkewi deleted the add-csrf branch August 29, 2018 13:08
@dideler
Copy link

dideler commented May 9, 2019

Hi all, wondering how to go about adding the same protection when using Plug (and not using Phoenix).

Was getting this warning with fun_with_flags v1.1.0 and fun_with_flags_ui v0.7.1.

[warn] CSRF protection won't work unless your host application uses the session plug module=FunWithFlags.UI.Router function=protect_from_forgery/2 line=308

Tried adding the Plug.CSRFProtection plug but then I run into session fetching issues.

I'll troubleshoot this more later, but asking in case it's obvious to someone else.

@aturkewi
Copy link
Contributor Author

aturkewi commented May 9, 2019

Hi @dideler it sounds like you may need to fetch the session before calling the Plug.CSRFProtection plug. I think you should be able to do that by adding Plug.Conn.fetch_session before the csrf protection plug.

@dideler
Copy link

dideler commented May 10, 2019

The router now looks like this

defmodule Flags.Router do
  use Plug.Router

  plug Plug.Static, at: "/", from: :flags
  plug :match
  plug :dispatch

  plug :fetch_session
  plug Plug.CSRFProtection

  forward "/flags", to: FunWithFlags.UI.Router, init_opts: [namespace: "flags"]
end

But getting an error that the plug needs to be configured.

cannot fetch session without a configured session plug

Tried configuring the session plug; the diff now looks like

+ plug Plug.Session, store: :ets, key: "_flags_session", table: :session
+ plug :fetch_session
+ plug Plug.CSRFProtection

Which gives this error when using the flags UI

(Plug.Conn.AlreadySentError) the response was already sent

With the warning

CSRF protection won't work unless your host application uses the session

Parking for now, but will give an update if I make any progress.

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.

Add support to protect from forgery
3 participants