-
Notifications
You must be signed in to change notification settings - Fork 30
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks 👍
Actually, sorry, I think that this template needs the hidden field too: |
Good catch. Just did a global search to check every |
That's great, thank you. |
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.
Tried adding the I'll troubleshoot this more later, but asking in case it's obvious to someone else. |
Hi @dideler it sounds like you may need to fetch the session before calling the |
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.
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
With the warning
Parking for now, but will give an update if I make any progress. |
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 intoTemplate.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!