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

CSRF protection prevents some webkit users from submitting forms #21948

Closed
zetter opened this issue Oct 13, 2015 · 99 comments
Closed

CSRF protection prevents some webkit users from submitting forms #21948

zetter opened this issue Oct 13, 2015 · 99 comments

Comments

@zetter
Copy link
Contributor

@zetter zetter commented Oct 13, 2015

Hi,

We've recently been investigating reports from our users that they are unable to submit forms.

Upon investigation it appears that browsers can get in a state where Rail's CSRF (Cross-Site Request Forgery) protection stops the form being submitted.

To reproduce

It's possible to produce a minimal Rails app which has this problem:

rails new csrf-test
cd csrf-test
bundle exec rails generate scaffold Test test:string
bundle exec rake db:migrate
bundle exec rails server

How to replicate it on mobile Safari (tested on iOS9):

  • Load a page containing a form (will be http://localhost:3000/tests/new in this example).
  • Quit Safari by double-tap the home button and swipe up.
  • Open Safari from the home screen. You should see the same page with the form.
  • Submit the form.

You will see the Rails invalid authenticity token error- this is a "The change you wanted was rejected" message in production, or an ActionController::InvalidAuthenticityToken in development. I've also made a video that follows theese steps.

How to replicate on Desktop Safari (tested on Safari 9.0 on OSX)

  • Go to 'Safari' > 'Preferences...' > 'General' and set 'Safari opens with:' to 'All windows from last session'.
  • Load a page containing a form (will be http://localhost:3000/tests/new in this example).
  • Quit Safari (with CMD+Q)
  • Open Safari. You should see the same page with the form.
  • Submit the form.

This problem seems to happen regardless if:

  • the app is served HTTP or HTTPS
  • the app's environment is development or production
  • the browser is manually quit by the user, or quit by the OS (to save memory)

I have also been able to replicate on Chrome on Android. I haven't yet been able to replicate it on Chrome and Firefox on OSX using their 'restore tabs' options like I did in Safari. There may be other browsers that are affected.

What's happening

Looking at the Rails logs, and the cookie submitted by the browser I believe that the browsers are caching the page, but clearing session cookies. This means the form has a authenticity_token parameter, but the Rails session cookie has been cleared so has no corresponding _csrf_token.

Here is a annotated log showing this:

# Browser loads the form for the first time
Started GET "/tests/new" for 127.0.0.1 at 2015-10-13 09:23:18 +0100
  ActiveRecord::SchemaMigration Load (0.1ms)  SELECT "schema_migrations".* FROM "schema_migrations"
Processing by TestsController#new as HTML
  Rendered tests/_form.html.erb (37.0ms)
  Rendered tests/new.html.erb within layouts/application (41.4ms)
Completed 200 OK in 256ms (Views: 243.3ms | ActiveRecord: 0.3ms)

# (Asset requests ommited)

# Browser quits, clearing session cookies
# Browser re-opens, reloads the page from cache without doing a request

# Browser posts the form:
Started POST "/tests" for 127.0.0.1 at 2015-10-13 09:23:37 +0100
Processing by TestsController#create as HTML
  Parameters: {"utf8"=>"✓", "authenticity_token"=>"IhsNUyL6Y/riLIujH+ExkTZN9pEPfwAVVB/t9pwrnkIR6lw1bAl3ZFY+bPg+zqMf3pj3qeY0vgbKblrWgr0vnQ==", "test"=>{"test"=>""}, "commit"=>"Create Test"}
Can't verify CSRF token authenticity
Completed 422 Unprocessable Entity in 1ms (ActiveRecord: 0.0ms)

ActionController::InvalidAuthenticityToken (ActionController::InvalidAuthenticityToken):
  actionpack (4.2.4) lib/action_controller/metal/request_forgery_protection.rb:181:in `handle_unverified_request'
  actionpack (4.2.4) lib/action_controller/metal/request_forgery_protection.rb:209:in `handle_unverified_request'
  actionpack (4.2.4) lib/action_controller/metal/request_forgery_protection.rb:204:in `verify_authenticity_token'
  # (Stack trace truncated)

I've tried to find more documentation about this kind of caching that browsers do. I found this article about the WebKit Page Cache but it appears to be out of date (it says HTTPS pages do not use the Page Cache, but I have seen this problem on HTTP and HTTPS pages). If anyone can find more about this I'd love to know.

I'd like to know:

  • Have others seen this problem? I haven't been able to find reported before, or find any documentation regarding this, but perhaps I have missed something.
  • Is there a way that Rails could be changed that would prevent this happening?
  • Are there any workarounds for this that we could apply in our application?

Thanks for any help.

@rafaelfranca

This comment has been minimized.

Copy link
Member

@rafaelfranca rafaelfranca commented Oct 13, 2015

Is there a way that Rails could be changed that would prevent this happening?

I can't see any way to prevent this happening without opening the application to possible CSRF attacks.

@pixeltrix

This comment has been minimized.

Copy link
Member

@pixeltrix pixeltrix commented Oct 13, 2015

@zetter does changing the Cache-Control header to no-store, no-cache make any difference?

@zetter

This comment has been minimized.

Copy link
Contributor Author

@zetter zetter commented Oct 13, 2015

@zetter does changing the Cache-Control header to no-store, no-cache make any difference?

I've added this line in the application config:

config.action_dispatch.default_headers.merge!('Cache-Control' => 'no-store, no-cache')

Mobile Safari has the same problem, however I can no longer reproduce this in Desktop Safari and Chrome on Android.

it does feel odd to me that the page is no-store but this isn't respected by Mobile Safari but is by the other browsers. Perhaps this is a bug in Mobile Safari although, as I said above I found it very difficult to find documentation around the expected behaviour of this kind of page caching. Perhaps there is another combination of cache headers that makes this or another work around for Mobile Safari?

UPDATE: I found that Mobile Safari does respect the 'no-store' header, however adding the header and refreshing the page isn't enough. For it to take effect also had to clear the cache using the 'Clear History and Website Data' option from Safari Settings.

This problem might also be avoided if the CSRF token was in a persistent cookie, but I'm not sure if there are security implications for doing so. It would be really helpful if anyone could explain to me if this would be fine, or a dangerous thing, to do.

@alepore

This comment has been minimized.

Copy link

@alepore alepore commented Oct 20, 2015

interesting topic, i see this error quite often.
@zetter here there's some mention about CSRF token in cookies, but it's used with javascript for caching purposes https://www.fastly.com/blog/Caching-the-Uncacheable-CSRF-security

@zetter

This comment has been minimized.

Copy link
Contributor Author

@zetter zetter commented Oct 29, 2015

An update:

  • I've deployed the change to serve all pages of our application to use a 'no-store' header. We're tracking how many times we see invalid authenticity token errors so next week I should be able to report how effective this change was for us.
  • I've tried working round the issue in Mobile Safari by experimenting with headers and client side JS (such as hooking into load/unload events) but have been unable to. I plan to raise a bug with Webkit/Apple, and will link to this when I do.
  • I'm curious how other frameworks handle this problem, for example, I know that Django uses a similar mechanism for CSRF prevention so will check to see if they have already solved this problem.

@alepore Thanks for sharing. I'm not sure if any of the solutions there help since it's they are about solving the problem of being able to use CSRF prevention token in combination with server-side caching.

@zetter

This comment has been minimized.

Copy link
Contributor Author

@zetter zetter commented Oct 29, 2015

My investigation into Django:

Django uses a similar mechanism to rails to prevent CSRF attacks- a token is stored in a cookie is compared to a tokens submitted with a form. What is different is how they store the token in a cookie in the default case:

Comparing Rails and Django

Rails adds the token to the session cookie under the _csrf_token key. Since the Rails session cookie has no expiry set, it will be cleared when the browser closes.

Django puts adds the token in it's own cookie called CSRF_COOKIE. This is a persistent cookie that expires in a year. If subsequent requests are made, the cookie's expiry is updated.

Why does Rails behave in this way?

Rail's CSRF protection comes from the csrf_killer plugin. The plugin had the following warning:

Make sure the session cookies that Rails creates are non-persistent. Check in Firefox and look for "Expires: at end of session"

This warning got imported into Rails from the csrf_killer documentation in 2c73115. It has since been replaced in 7d8474e by this warning

It is common to use persistent cookies to store user information, with cookies.permanent for example. In this case, the cookies will not be cleared and the out of the box CSRF protection will not be effective.

Read full section of the guide. I'm not sure if "the cookies will not be cleared" in the statement above is referring to when a user signs out, when a session ends, or when an invalid authenticity token is given.

Why does Django behave in this way?

In contrast, it looks like Django made the explicit decision to store CSRF prevention token in a permanent cookie. From the Django docs:

The reason for setting a long-lived expiration time is to avoid problems in the case of a user closing a browser or bookmarking a page and then loading that page from a browser cache. Without persistent cookies, the form submission would fail in this case.

You can also read the bug which caused this change in Django, a question on the Django mailing list asking if persistent cookies are safe and a related bug for how Internet Explorer can be configured to block persistent cookies.

Is it secure to store the CSRF prevention token in a separate permanent cookie?

Storing the token in a permanent cookie would fix the original issue as it would no longer expire when the browser closed. It looks like Django has made the choice that it is. I couldn't find any information around CSRF attacks that suggests it wouldn't be. The only item I found was on the OWASP Wiki which suggests that the shorter the lifespan of the token the better, without any justification:

To further enhance the security of this proposed design, consider randomizing the CSRF token parameter name and or value for each request. Implementing this approach results in the generation of per-request tokens as opposed to per-session tokens. Note, however, that this may result in usability concerns. For example, the "Back" button browser capability is often hindered as the previous page may contain a token that is no longer valid. Interaction with this previous page will result in a CSRF false positive security event at the server.

I'd love to know from anyone who knows more about CSRF attacks to find out if it would be safe to store the CSRF prevention token in a permanent cookie (both for our application, and for Rails).

@jeremy

This comment has been minimized.

Copy link
Member

@jeremy jeremy commented Oct 29, 2015

Thank you for the investigation and writeup, @zetter. Agree with Django's reasoning and decision 👍 May be a bit tricky to introduce to Rails apps in a compatible way, though.

@aganov

This comment has been minimized.

Copy link

@aganov aganov commented Nov 5, 2015

I can confirm the issue, combination El Capitan + Safari + http2, not a single working form... However there is no problem with Safari on Yosemite...

Edit: after disabling HTTP/2, the problem disappears

@zetter

This comment has been minimized.

Copy link
Contributor Author

@zetter zetter commented Nov 9, 2015

Another update:

Here's a graph of showing the number of our users seeing form authenticity token errors as a proportion of the number of visits to our site. It looks like the rate of errors have dropped since we introduced no-store cache header.

screen shot 2015-11-09 at 09 00 10

Note that the traffic pattern to our site is unusual as a lot of our users visit weekly, this means they may have a page open in a browser from a visit a previous week. I think that's why even though there was an initial drop in errors, another drop came a week later as the cohort of the previous weeks refreshed the pages and got the updated headers.

I still plan to:

  • raise a bug with Safari/Webkit about the inconsistent behaviour of no-store as compared to other browsers (UPDATE: I found Safari is respecting the no-store header, it just needs to be reset- see above comment for more)
  • try storing the token as a permanent cookie in our app to see if error rates are further decreased
@aganov

This comment has been minimized.

Copy link

@aganov aganov commented Nov 9, 2015

@zetter Just wondering, is your server HTTP/2 enabled?

@zetter

This comment has been minimized.

Copy link
Contributor Author

@zetter zetter commented Nov 9, 2015

@zetter Just wondering, is your server HTTP/2 enabled?

@aganov It is not, and I have not tested my original example with a HTTP/2 server.

My understanding of HTTP/2 is that there are no changes to cookies or headers so the behaviour I am seeing shouldn't change. You said before that there wasn't a single working form- were you following my original reproduction example? or is this another app that might have a different problem?

@javan

This comment has been minimized.

Copy link
Member

@javan javan commented Nov 17, 2015

try storing the token as a permanent cookie in our app to see if error rates are further decreased

You can try that by giving your session cookie an expire_after value to persist it across browsing sessions:

Rails.application.config.session_store :cookie_store, key: '_myapp_session', expire_after: 2.weeks

You'll need to consider possible security implications based on your session data. In my case, it solved the problem.

+1 for moving the CSRF token to its own permanent cookie.

@mastahyeti

This comment has been minimized.

Copy link
Contributor

@mastahyeti mastahyeti commented Nov 25, 2015

I saw a case a few years ago where the Django CSRF behavior resulted in a security vuln for an app. With an XSS on foo.example.com, an attacker was able to set the CSRF token cookie for .example.com causing it to be sent with requests to example.com and *.example.com. The attacker-controlled CSRF token clobbered the one that had been set by the site at example.com. Because the attacker knows the new CSRF token value, they can then perform CSRF attacks.

This sort of attack doesn't affect Rails applications because the CSRF token is stored in a signed cookie. Most applications will reset_session during login, so even if an attacker can set an unauthenticated cookiestore cookie (session fixation), the user will receive a new CSRF token after authenticating.

Django's CSRF token storage seems like a pretty big weakness to me. If you decide to store Rails CSRF tokens in a separate cookie, they should definitely still be signed.

As for the security impact of using a permanent cookie, the risk seems the same as using a permanent cookie for the whole cookiestore session. The CSRF token is generally the most sensitive thing stored in a session, so if there's anything that should be deleted when the browser is closed it's that. This issue seems like a Safari bug to me. Safari is notoriously bad at handling cookies. I think suggesting that apps work around the issue by using a permanent cookie for the session is better than working around the Safari bug in the framework.

@ctpelnar1988

This comment has been minimized.

Copy link

@ctpelnar1988 ctpelnar1988 commented Dec 11, 2015

Try changing "protect_from_forgery with: :exception" to=> "protect_from_forgery with: :null_session"

:null_session - Provides an empty session during request but doesn't reset it completely. Used as default if :with option is not specified.

@bcardiff

This comment has been minimized.

Copy link

@bcardiff bcardiff commented Dec 11, 2015

For me, using :null_session, together with some straight devise usage, it ended up in an infinite redirection loop in an Safari/iPad. For the time being I am using Chrome for iPad.

@mikebaldry

This comment has been minimized.

Copy link

@mikebaldry mikebaldry commented Feb 3, 2016

Any thoughts or update on this? I think its hidden a lot in the default mode, as people just get a null session and we quietly log it. I've switched to raise and now I can see this is affecting a fair few people.

markottaviani added a commit to ophrescue/RescueRails that referenced this issue Feb 15, 2016
in Mobile Safari and Mobile Chrome
Ref rails/rails#21948 (comment)
@chrisnicola

This comment has been minimized.

Copy link
Contributor

@chrisnicola chrisnicola commented Feb 18, 2016

I've been seeing something similar since switching to the :exception behaviour. Specifically from unauthenticated endpoints. We are also mostly an Angular/JSON app so CSRF is a bit lower value for us but we like to keep in enabled.

I've switched to the :reset_session behaviour on any unauthenticated endpoints such as creating a new user or logging in which seems sensible and doesn't cause any problems.

@pixeltrix

This comment has been minimized.

Copy link
Member

@pixeltrix pixeltrix commented Mar 29, 2016

Urgh, looks like we're seeing this on https://petition.parliament.uk. However it looks like we're seeing it across multiple browsers and not limited to Mobile Safari.

@Bramjetten

This comment has been minimized.

Copy link

@Bramjetten Bramjetten commented Apr 4, 2016

We were seeing a lot of InvalidAuthenticityToken exceptions. After reading this thread and further inspection I saw that the exceptions were only caused by mobile browsers. Adding 'Cache-Control' => 'no-store, no-cache' seems to have fixed it. Immediate drop in exceptions. Is this something we always have to account for or is there a better solution?

@jeremy

This comment has been minimized.

Copy link
Member

@jeremy jeremy commented Apr 4, 2016

@Bramjetten That means no browser caching, though. Using a persistent cookie for your session is prob a more reasonable fix for you.

@Bramjetten

This comment has been minimized.

Copy link

@Bramjetten Bramjetten commented Apr 4, 2016

@jeremy That's what I did at first, but it didn't seem to help.

@jeremy

This comment has been minimized.

Copy link
Member

@jeremy jeremy commented Apr 4, 2016

May need to give it some time as people establish new sessions?
On Mon, Apr 4, 2016 at 09:27 Bram Jetten notifications@github.com wrote:

@jeremy https://github.com/jeremy That's what I did at first, but it
didn't seem to help.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#21948 (comment)

pixeltrix added a commit to alphagov/e-petitions that referenced this issue Apr 11, 2016
Mobile Safari has a tendency to use cached form values even when the
cache control headers tell it otherwise. However the session cookie
has expired so when the form is submitted the CSRF token is invalid.

See rails/rails#21948 for further details.

Fixes #451.
@Spone

This comment has been minimized.

Copy link

@Spone Spone commented Jun 21, 2018

Same as @rudolfolah, I had to use protect_from_forgery with: :exception, prepend: true to fix the issue.

This new behavior is present since Rails 5.0 and has been added by this commit: 3979403

See also Devise README: https://github.com/plataformatec/devise/blob/715192a7709a4c02127afb067e66230061b82cf2/README.md#controller-filters-and-helpers

@lucascaton

This comment has been minimized.

Copy link
Contributor

@lucascaton lucascaton commented Aug 17, 2018

I can confirm that it still happens with a brand new app:

  • Ruby 2.5.1
  • Rails 5.2.1
  • Safari 11.1.2 (13605.3.8)
  • macOS 10.13.6 (High Sierra)
@lucascaton

This comment has been minimized.

Copy link
Contributor

@lucascaton lucascaton commented Aug 17, 2018

hi @maclover7 - can you please remove the needs feedback label?

@rails-bot

This comment has been minimized.

Copy link

@rails-bot rails-bot bot commented Nov 15, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-2-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Nov 15, 2018
@rails-bot rails-bot bot closed this Nov 22, 2018
@rdlugosz

This comment has been minimized.

Copy link

@rdlugosz rdlugosz commented Jan 31, 2019

The bot auto-closed this issue, but I'd say it's still relevant as I've noticed an upswing in these exceptions as more traffic goes to mobile devices.

Seems like the best solution for a lot of apps is the Cache-Control suggestion above combined with redirecting the user to the root_path along with a helpful message suggesting they retry when CSRF does fail. The persistent cookie idea is interesting, but appears to have its own issues (?)

Maybe just a matter of updating the docs and suggesting people add cache control and some handling if they're going to use the :exception style of CSRF enforcement?

@rishabhsairawat

This comment has been minimized.

Copy link

@rishabhsairawat rishabhsairawat commented Feb 4, 2019

I can confirm the original issue by @zetter is still relevant and can be reproduced easily.

This needs to be re-opened.

@elquimista

This comment has been minimized.

Copy link

@elquimista elquimista commented Feb 6, 2019

@NGarate

This comment has been minimized.

Copy link

@NGarate NGarate commented Feb 15, 2019

I have the same issue. On every webkit browser I try, not in other browsers.

@lucascaton

This comment has been minimized.

Copy link
Contributor

@lucascaton lucascaton commented Mar 1, 2019

Hey @rafaelfranca, could you please shed some light?
Sorry if you aren't the right person to @ in this case.

Thank you.

@BenFenner

This comment has been minimized.

Copy link

@BenFenner BenFenner commented Mar 1, 2019

If you need extremely easy steps to reproduce on any browser and any Rails site using protect_from_forgery with: exception, including GitHub itself, check my previous post:
#21948 (comment)

All of our sites are on Rails 5.2.2 now, so I can check there, but it's going to be the same story.

@pixeltrix

This comment has been minimized.

Copy link
Member

@pixeltrix pixeltrix commented Mar 1, 2019

@BenFenner out of curiosity what would you expect it to do in your example? AFAICS there are two options:

  1. Ignore the CSRF token error or make the token valid somehow and process the request
  2. Redirect to the login page

Option 1 is a massive security hole so that just leaves option 2 which is what you'll get when you change protect_from_forgery to use :null_session.

@rdlugosz

This comment has been minimized.

Copy link

@rdlugosz rdlugosz commented Mar 1, 2019

The problem with approach #2 is that these users may not expect to have been logged out of the site and will get annoyed that they're frequently being asked to log in again. A third option exists: redirect to home with a message asking that they retry their request. This gets the token refreshed, prevents a potentially insecure action, and allows the user to continue without having to re-login.

@pixeltrix

This comment has been minimized.

Copy link
Member

@pixeltrix pixeltrix commented Mar 1, 2019

@rdlugosz that's an app-level concern, not something we can do at the framework level, e.g.

class PostsController < ApplicationController
  rescue_from ActionController::InvalidAuthenticityToken do
    redirect_to root_url, alert: "Your request has expired, please try again"
  end
end

I think the best we can do to fix this is to add a section to the security guide on the causes and how you can deal with the problem.

@travisp

This comment has been minimized.

Copy link
Contributor

@travisp travisp commented Mar 1, 2019

It seems that there may be some approaches to deal with this that might feel better to the user, for example:

  1. use an approach that logs out the user in all tabs when logged out in one (achievable with websockets, localstorage events)
  2. update CSRF tokens or refresh the session in all tabs when session changes (again with websockets or localstorage events).

But these solutions feel probably beyond the scope of this issue as the exact desired behavior might be application specific. It would be helpful to at least describe this issue in the security guide so that developers stumbling upon this can understand what's happening and make an informed choice.

Does this issue exist only when users are logging in/out and they have multiple tabs open, or are there other scenarios it could happen?

@rdlugosz

This comment has been minimized.

Copy link

@rdlugosz rdlugosz commented Mar 1, 2019

I agree that updating the docs may be the best resolution to this, failing the proper vetting and implementation of a persistent cookie like Django uses as described earlier (which I'm not qualified to do; that needs careful attention from the sec team).

One issue in this thread is that two different scenarios have gotten co-mingled. The original issue has to do with mobile browsers not persisting the CSRF state between extended sessions (i.e., I have a tab open in safari on my iphone, leave it for a while, and then when I come back the next search I submit to the app causes an invalid token error). Later, a separate scenario was described that involves multiple tabs and logging out in one of them... that's an entirely different situation than what started this issue.

So @travisp, no - it isn't really about multiple tabs, although there may be a related case that involves them.

rosa added a commit to basecamp/google_sign_in that referenced this issue Mar 20, 2019
Due to some issues with Rails sessions expiry time and browsers
restoring open pages/tabs with stale CSRF tokens but expired sessions
(see rails/rails#21948), posting to
/authorization fails for some users due to failed CSRF checks.

Since this endpoint simply redirects to initiate the Google OAuth flow,
there's nothing to be compromised by forging that request. It's similar
to disabling CSRF protection on login.
rosa added a commit to basecamp/google_sign_in that referenced this issue Mar 21, 2019
Due to some issues with Rails sessions expiry time and browsers
restoring open pages/tabs with stale CSRF tokens but expired sessions
(see rails/rails#21948), posting to
/authorization fails for some users due to failed CSRF checks.

Since this endpoint simply redirects to initiate the Google OAuth flow,
there's nothing to be compromised by forging that request. It's similar
to disabling CSRF protection on login.
@monroemann

This comment has been minimized.

Copy link

@monroemann monroemann commented May 10, 2019

We can confirm that at least one of our users is having this same issue right now. We haven't yet been able to solve it.

genezys added a commit to genezys/linkedin_sign_in that referenced this issue May 10, 2019
Due to some issues with Rails sessions expiry time and browsers
restoring open pages/tabs with stale CSRF tokens but expired sessions
(see rails/rails#21948), posting to /authorization fails for some users
due to failed CSRF checks.

Since this endpoint simply redirects to initiate the Google OAuth flow,
there's nothing to be compromised by forging that request. It's similar
to disabling CSRF protection on login.
@Richard-Degenne

This comment has been minimized.

Copy link

@Richard-Degenne Richard-Degenne commented May 13, 2019

Hi, I've been following this issue for quite some time now, and I'm just wondering.

To developers suffering from this issue, does your project use Devise for authentication?

Please react to this comment with 👍 for yes, 👎 for no.

@AlexanderZaytsev

This comment has been minimized.

Copy link

@AlexanderZaytsev AlexanderZaytsev commented Sep 2, 2019

A heads-up to someone trying to find out why current_user is being nil inside your Devise::RegistrationsController even though it calls autheticate_user! behind the scenes for actions like update.

Basically, I create guest accounts for users to try out my service (I have an attribute role=guest in my User model). After some time, I display a form suggesting users to "save" their account by entering an email and a password. For years I couldn't understand how this code would result in NoMethodError: undefined method id' for nil:NilClass`:

class Users::RegistrationsController < Devise::RegistrationsController
  def update
    @user = User.find(current_user.id)
    # Update @user from role=guest to role=user, etc
  end
end

So it turns out, here's what caused the error:

  1. I create a guest account for a user.
  2. Some time later I display a form and ask the user to enter their email & password.
  3. At this step, the user closes/switches from their iOS browser to another app.
  4. Some time later, the user opens their iOS browser again, sees the old form and tries entering email & password and submitting it.
  5. It results in an invalid CSRF token, which results in current_user being nil inside the update action. Also, this resulted in the user's session being reset, resulting in a loss of the their guest account & data they entered.

The fix I'm using now:
Raise an exception in ApplicationController

class ApplicationController < ActionController::Base
  protect_from_forgery prepend: true, with: :exception
end

And rescue it inside Users::RegistrationsController

class Users::RegistrationsController < Devise::RegistrationsController
  rescue_from ActionController::InvalidAuthenticityToken do
    redirect_to request.referrer, alert: "Your request has expired, please try again"
  end
end

This way, the user stays logged in, sees "Your request has expired, please try again" and is presented with a fresh form with a valid csrf token.

It took me a few years to hunt down this bug until I got fed up with it today :) Hopefully this info will help someone with a similar problem.

@pixeltrix

This comment has been minimized.

Copy link
Member

@pixeltrix pixeltrix commented Nov 13, 2019

So it turns out that Firefox may autocomplete hidden fields:

https://bugzilla.mozilla.org/show_bug.cgi?id=520561

Has anyone seen this as a cause of 422 errors? For example someone could log out in one tab and then hit refresh in another which may result in an stale authenticity_token being injected into the hidden field.

@mindaslab

This comment has been minimized.

Copy link

@mindaslab mindaslab commented Jan 19, 2020

It's not a Rails problem if you are using nginx, puma along with certbot. Its happening because certain stuff to authenticate is not available for the Rails app, before reaching Rails it gets filtered out.

To fix it you can see this https://stackoverflow.com/questions/39012356/devise-cant-verify-csrf-token-authenticity-the-https-enabled-on-server-no-jso

tekin added a commit to DFE-Digital/dfe-teachers-payment-service that referenced this issue Jan 28, 2020
Some browsers have a behaviour where although they will delete a session
cookie when the app is shutdown, they will still serve a cached version
of the page on relaunch. This causes issues with CSRF protection as it
relies on the token embedded in the page/form matching the token in the
session cookie. So a user that then submits the form will get a 422
error (`ActionController::InvalidAuthenticityToken`) because their is
nothing in the session. We get around this by giving the session cookie
a life beyond the browser session. We can do this without risk of
exposing user data as we still explicitly expire a active claim and
admin sessions based on the `last_seen_at` timestamp.

See rails/rails#21948 for further details on
the issue.
tekin added a commit to DFE-Digital/dfe-teachers-payment-service that referenced this issue Jan 28, 2020
Some browsers have a behaviour where although they will delete a session
cookie when the app is shutdown, they will still serve a cached version
of the page on relaunch. This causes issues with CSRF protection as it
relies on the token embedded in the page/form matching the token in the
session cookie. So a user that then submits the form will get a 422
error (`ActionController::InvalidAuthenticityToken`) because their is
nothing in the session. We get around this by giving the session cookie
a life beyond the browser session. We can do this without risk of
exposing user data as we still explicitly expire a active claim and
admin sessions based on the `last_seen_at` timestamp.

See rails/rails#21948 for further details on
the issue.
tekin added a commit to DFE-Digital/dfe-teachers-payment-service that referenced this issue Jan 29, 2020
Some browsers have a behaviour where although they will delete a session
cookie when the app is shutdown, they will still serve a cached version
of the page on relaunch. This causes issues with CSRF protection as it
relies on the token embedded in the page/form matching the token in the
session cookie. So a user that then submits the form will get a 422
error (`ActionController::InvalidAuthenticityToken`) because their is
nothing in the session. We get around this by giving the session cookie
a life beyond the browser session. We can do this without risk of
exposing user data as we still explicitly expire a active claim and
admin sessions based on the `last_seen_at` timestamp.

See rails/rails#21948 for further details on
the issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.