Csrf protection plug #338

gjaldon · 2014-09-13T19:01:57Z

Couldn't get the last 2 tests to pass. How to persist token in the session in the tests?

chrismccord · 2014-09-13T19:06:51Z

lib/phoenix/plugs/csrf_protection.ex

+
+  defp get_csrf_token(conn), do: Conn.get_session(conn, :csrf_token)
+
+  defp token_length, do: 32


This looks like it's unused? Where do we persist the token in the session? I'm not seeing token generation/persistence anywhere?

chrismccord · 2014-09-13T19:14:41Z

@gjaldon Before running the tests you can just great a conn and generate a token for it before simulating the request. I'm not seeing any token generation/persistence code though? Once that's in place I can take a look at setting the test cases up

josevalim · 2014-09-13T19:43:12Z

lib/phoenix/plugs/csrf_protection.ex

+  defp valid_authenticity_token?(conn, token), do: get_csrf_token(conn) == token
+
+
+  defp get_csrf_token(conn), do: Conn.get_session(conn, :csrf_token)


This is assuming the session was previously fetched. Is this assumption ok? Should we fetch automatically or should we document the user must do so? Given Phoenix fetches the session automatically, I would go with assume the session was previously fetched and explicitly document it (in the moduledoc).

gjaldon · 2014-09-14T09:40:45Z

I've applied most of the changes suggested. I have yet to look into how to handle a request with an invalid authenticity token though(still have to look up why Rails makes this configurable).

I generate and persist the token in the session if it does not previously exist here but just realized this won't work since all first requests will always be unprotected. I'm thinking of replacing it with a function like:

def csrf_token_param(conn) do
  if token = get_csrf_token(conn) do
    token
  else
    token = generate_csrf_token(token_length)
    store.put(sid, conn.private[:plug_session], store_config)
  end
end

This will be called in the View/Template so a csrf_token is always generated.

In my tests, the session doesn't persist across multiple requests. It looks like it's because I'm using cookie store which requires a browser. I'm thinking of using a different store in the test. Will that be okay?

josevalim · 2014-09-14T09:46:56Z

I generate and persist the token in the session if it does not previously exist here but just realized this won't work since all first requests will always be unprotected.

That's exactly how CSRF is supposed to work. The first request is usually a GET that sets up the token, we should not set the token or generate one in the same request as it would lead all blank requests (with no token) to pass.

In my tests, the session doesn't persist across multiple requests. It looks like it's because I'm using cookie store which requires a browser.

You need to recycle the connection. Something like:

conn = conn(:get, ...)
conn = call(conn, ...)

conn = recycle(conn)
call(conn, ...)

The recycle will move the cookies set in the response to a cookie to be sent as request (as a browser would).

chrismccord · 2014-09-14T11:25:50Z

lib/phoenix/plugs/csrf_protection.ex

+  def init(opts), do: opts
+
+  def call(%Conn{method: method} = conn, _opts) when method in @protected_methods do
+    conn = ensure_csrf_token(conn)


If we generate it here, it will necessarily be verified no matter what. So we'll need to expose the generate so it's call callable from elsewhere, i.e. the Views as you've discussed.

It is very hard to do in the views because the views would now have to return a new connection. In general I would argue for it to be generated before hand.

We should probably ensure it exists only on GET requests?

Oh right, good point. We have to do it before. Yeah I think generate it on get requests in the non @protected_methods clause would be fine.

gjaldon · 2014-09-14T15:50:02Z

I updated all the specs now to use recycle/2 and provided 2 different ways to handle invalid_token that is configurable by setting the raise_error option. I also applied all the other suggestions and the docs should be updated now.

How do I test if a response was sent? Was it correct to use send_resp/3 in place of put_status in handle_invalid_token/1? Let me know if there's anything else I missed.

gjaldon · 2014-09-14T15:54:58Z

Closes #305

gjaldon · 2014-09-16T13:59:26Z

@chrismccord @josevalim this is ready for review, btw. 😃

chrismccord · 2014-09-16T14:57:50Z

lib/phoenix/plugs/csrf_protection.ex

+
+  ## Examples
+
+      plug Phoenix.Plugs.CsrfProtection, raise_error: false


We shouldn't raise an error. Instead we should halt the connection or clear the session and continue. Maybe halt: true ? Personally, I'm for always halting until someone needs/wants the ability for the stack to continue. The safest approach is to halt always. So maybe we do away with an option entirely for now.

gjaldon · 2014-09-16T15:34:56Z

@chrismccord thanks for the inputs! Should be ready for review again.

chrismccord · 2014-09-16T15:51:08Z

test/phoenix/plugs/csrf_protection_test.exs

+  end
+
+  test "unprotected requests are always valid" do
+    simulate_request_without_token(:get, "/") |> CsrfProtection.call([])


We need asserts for all these

@chrismccord Done adding all the assertions!

chrismccord · 2014-09-24T17:19:32Z

@gjaldon Can you rebase off master? We have ConnHelper now. RouteHelper no longer exists. Once we get the conflicts cleared up this should be just about ready to go.

josevalim · 2014-09-24T20:40:09Z

I would honestly define a new exception, raise it on the plug, implement Plug.Exception for it, and call it a day.

chrismccord · 2014-09-24T20:41:24Z

I would honestly define a new exception, raise it on the plug, implement Plug.Exception for it, and call it a day.

👍

gjaldon · 2014-09-25T15:55:59Z

I applied all of @josevalim's suggestions, updated the tests and rebased to master. Let me know if there's anything I missed 😄

josevalim · 2014-09-26T07:07:46Z

I plan to review this soonish.

josevalim · 2014-11-19T16:44:48Z

Can we merge this after 0.6.0? I am considering adding this to plug but I need more time. :S

chrismccord · 2014-11-19T16:53:40Z

Yep. It can wait. Worst case we can release a 6.x with it.

stevedomin · 2014-11-25T19:30:53Z

Hi guys,

Since the goal of plug is to create composable modules that can be used by different web apps shouldn't this stuff be extracted from phoenix and be put either in a separate repo or in the plug repo itself maybe (csrf protection is quite important to any website really so I definitely think it can be part of the "essentials") ?

chrismccord · 2014-11-25T19:37:07Z

Plug definitely lets the community easily share composable modules. Our approach right now is to implement what we need in Phoenix, then extract if/when it makes sense. So far, the extracted stuff has made its way back into Plug, but in the future it's possible we'll release standalone projects. It's not a priority right now though. Up through 1.0, we won't be extracting much as it will slow us down since APIs are subject to change.

stevedomin · 2014-11-25T19:48:41Z

That makes sense, thanks Chris.

josevalim · 2014-11-30T12:32:15Z

Just want to say sorry for the delay on this @gjaldon. It is on my plate to review this and the forms work next.

josevalim · 2014-12-01T14:31:04Z

lib/phoenix/plugs/csrf_protection.ex

+      plug Phoenix.Plugs.CsrfProtection
+
+  """
+  @protected_methods ~w(POST PUT PATCH DELETE)


Instead of checking for those, let's protect for ALL except HEAD and GET.

josevalim · 2014-12-01T14:32:12Z

I have reviewed and this looks great. Can you please submit this pull request in the Plug repo and I will get to merge it asap. :)

josevalim · 2014-12-01T14:38:30Z

lib/phoenix/plugs/csrf_protection.ex

+    Error raised when CSRF token is invalid.
+    """
+
+    defexception [:message]


In latest Plug you don't need to implement Plug.Exception, just add a field called plug_status: 403.

You should also specify the default error message here, instead of doing it when raising. :)

josevalim · 2014-12-01T15:03:54Z

Two more notes:

It would be nice if we have a mechanism to skip the CSRF token if something is set inside the conn.private. For that, we could check if conn.private[:plug_skip_csrf_protection] is set to true and skip it. This would be useful for testing.
We should also add protection to this style of cross requests: rails/rails@1650bb3

Feel free to ping me if you need more info!

gjaldon · 2014-12-02T19:46:08Z

No problem about the delay, @josevalim! Will apply your suggestions and submit the PR in the Plug repo. Will likely finish this over the next few days. :)

gjaldon · 2014-12-10T13:37:49Z

Closing in favor of adding CSRF to Plug and #523

Mention Phoenix.Socket.assign/3 in the channel guide

chrismccord reviewed Sep 13, 2014
View reviewed changes

josevalim reviewed Sep 13, 2014
View reviewed changes

gjaldon force-pushed the csrf-protection-plug branch from f816f79 to 7a792dc Compare September 14, 2014 08:03

chrismccord reviewed Sep 14, 2014
View reviewed changes

gjaldon changed the title ~~[WIP] Csrf protection plug~~ Csrf protection plug Sep 14, 2014

chrismccord reviewed Sep 16, 2014
View reviewed changes

josevalim closed this Sep 24, 2014

josevalim reopened this Sep 24, 2014

gjaldon force-pushed the csrf-protection-plug branch from 7d3f356 to f8e4293 Compare September 25, 2014 15:53

chrismccord mentioned this pull request Nov 19, 2014

Add CSRF Plug #305

Closed

gjaldon added 7 commits November 20, 2014 11:18

Config for toggling csrf_protection

3860acb

Initial tests for CSRFProtection

9001aac

Test for validating token in header

fdcfe9d

Test Plug directly

84e8c4d

Token generation and persistence

cb9c118

Use recycle in tests

415040a

Handling invalid authenticity token

3aa23e4

gjaldon force-pushed the csrf-protection-plug branch from e62c2a2 to 168abe9 Compare November 20, 2014 03:19

gjaldon added 5 commits November 20, 2014 11:20

Remove raise_error config and assert response is sent

101cf2f

Assert that conn has not halted

9c27611

Add test for missing csrf_token

a30aad3

Update tests and raise error on invalid token

3fbcdd2

Remove logging and update default config

168abe9

josevalim reviewed Dec 1, 2014
View reviewed changes

gjaldon mentioned this pull request Dec 5, 2014

CSRF Plug elixir-plug/plug#137

Closed

gjaldon closed this Dec 10, 2014

gjaldon deleted the csrf-protection-plug branch December 10, 2014 13:37

Gazler pushed a commit to Gazler/phoenix that referenced this pull request Sep 17, 2015

Merge pull request phoenixframework#338 from leikind/master

9e187fd

Mention Phoenix.Socket.assign/3 in the channel guide

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Csrf protection plug #338

Csrf protection plug #338

gjaldon commented Sep 13, 2014

chrismccord Sep 13, 2014

chrismccord commented Sep 13, 2014

josevalim Sep 13, 2014

gjaldon commented Sep 14, 2014

josevalim commented Sep 14, 2014

chrismccord Sep 14, 2014

josevalim Sep 14, 2014

josevalim Sep 14, 2014

chrismccord Sep 14, 2014

gjaldon commented Sep 14, 2014

gjaldon commented Sep 14, 2014

gjaldon commented Sep 16, 2014

chrismccord Sep 16, 2014

gjaldon commented Sep 16, 2014

chrismccord Sep 16, 2014

gjaldon Sep 16, 2014

chrismccord commented Sep 24, 2014

josevalim commented Sep 24, 2014

chrismccord commented Sep 24, 2014

gjaldon commented Sep 25, 2014

josevalim commented Sep 26, 2014

josevalim commented Nov 19, 2014

chrismccord commented Nov 19, 2014

stevedomin commented Nov 25, 2014

chrismccord commented Nov 25, 2014

stevedomin commented Nov 25, 2014

josevalim commented Nov 30, 2014

josevalim Dec 1, 2014

josevalim commented Dec 1, 2014

josevalim Dec 1, 2014

josevalim Dec 1, 2014

josevalim commented Dec 1, 2014

gjaldon commented Dec 2, 2014

gjaldon commented Dec 10, 2014


		defp get_csrf_token(conn), do: Conn.get_session(conn, :csrf_token)

		defp token_length, do: 32

		defp valid_authenticity_token?(conn, token), do: get_csrf_token(conn) == token


		defp get_csrf_token(conn), do: Conn.get_session(conn, :csrf_token)


		## Examples

		plug Phoenix.Plugs.CsrfProtection, raise_error: false

Csrf protection plug #338

Csrf protection plug #338

Conversation

gjaldon commented Sep 13, 2014

Choose a reason for hiding this comment

chrismccord commented Sep 13, 2014

Choose a reason for hiding this comment

gjaldon commented Sep 14, 2014

josevalim commented Sep 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjaldon commented Sep 14, 2014

gjaldon commented Sep 14, 2014

gjaldon commented Sep 16, 2014

Choose a reason for hiding this comment

gjaldon commented Sep 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrismccord commented Sep 24, 2014

josevalim commented Sep 24, 2014

chrismccord commented Sep 24, 2014

gjaldon commented Sep 25, 2014

josevalim commented Sep 26, 2014

josevalim commented Nov 19, 2014

chrismccord commented Nov 19, 2014

stevedomin commented Nov 25, 2014

chrismccord commented Nov 25, 2014

stevedomin commented Nov 25, 2014

josevalim commented Nov 30, 2014

Choose a reason for hiding this comment

josevalim commented Dec 1, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josevalim commented Dec 1, 2014

gjaldon commented Dec 2, 2014

gjaldon commented Dec 10, 2014