aaronrenner / phx_gen_auth

An authentication system generator for Phoenix 1.5 applications.
772 stars 55 forks source link

Newlines generated in session token #83

Closed thepratt closed 4 years ago

thepratt commented 4 years ago

https://github.com/aaronrenner/phx_gen_auth/blob/0772ec8054ebd05391f57c6d1511817f44ba6952/priv/templates/phx.gen.auth/schema_token.ex#L32

I've pinpointed the above as causing the following error to emerge when attempting to authorise with a cookie:

(Plug.Conn.InvalidHeaderError) value for header "set-cookie" contains control feed (\r) or newline (\n): <<109, 101, 109, 98, 101, 114, 95, 97, 117, 116, 104, 61, 90, 19, 252, 11, 129, 235, 219, 107, 22, 191, 10, 222, 41, 109, 159, 20, 14, 30, 141, 20, 224, 121, 223, 163, 205, 213, 9, 148, 19, 147, 209, 164, 59, 32, 112, 97, 116, 104, ...>>

Note: 10 is a newline byte.

For my own project, I've amended this to base64 before passing the value along. I'd want to open a discussion for which stage newlines should be prevented from entering the cookie header, or if a fixed dictionary to replace strong_rand_bytes would be preferred.

My updated line reads as

token =
      :crypto.strong_rand_bytes(@rand_size)
      |> Base.encode64(padding: false)
josevalim commented 4 years ago

The default session store is cookie-based, which means this value gets signed and base64d by the session store. If you are storing this value by hand, then yes, you must encode it. But i would recommend keeping the cookie store since the cookie store + database lookups gives you the best of both words: i.e. it is not easy to temper and it can still be expired server side.

josevalim commented 4 years ago

Also, if you are not signing it and keeping it directly in the cookie, then you are also opening yourself up to timing attacks, so I would recommend to stick with the structure generated by mix phx.gen.auth.

thepratt commented 4 years ago

That makes sense.

I do have signing enabled for the application itself, however, it's disabled in testing. During tests is the only case that I've experienced the above error.

josevalim commented 4 years ago

Yes, so please don't disable signing. If you do, then explicitly encode it. :)

thepratt commented 4 years ago

Thanks for your help @josevalim , I'll continue down the route of keeping signing on regardless of the environment - tests in this instance.

As my Phoenix application is an API, I only have a cookie and not a session to pass around. Having signing enabled for tests resulted in a different error due to secret_key_base not being picked up by a test session, but this is something else I'd need to work through and not related to phx_gen_auth.

For reference

  4) test DELETE /members/log_out logs the member out (MyWeb.MemberSessionControllerTest)
     test/my_web/controllers/member_session_controller_test.exs:51
     ** (FunctionClauseError) no function clause matching in Plug.Crypto.sign/4

     The following arguments were given to Plug.Crypto.sign/4:

         # 1
         nil

         # 2
         "member_auth_cookie"

         # 3
         <<...>>

         # 4
         [keys: Plug.Keys, max_age: 86400]

     Attempted function clauses (showing 1 out of 1):

         def sign(key_base, salt, data, opts) when is_binary(key_base) and is_binary(salt)

     code: conn = conn |> log_in_member(member) |> delete(Routes.member_session_path(conn, :delete))
     stacktrace:
       (plug_crypto 1.1.2) lib/plug/crypto.ex:172: Plug.Crypto.sign/4
       (plug 1.10.4) lib/plug/conn.ex:1479: Plug.Conn.maybe_sign_or_encrypt_cookie/4
       (plug 1.10.4) lib/plug/conn.ex:1463: Plug.Conn.put_resp_cookie/4
       test/my_web/controllers/member_session_controller_test.exs:52: (test)