devise: Timeoutable timeouts causing Rails to flash[:timedout] as "true"

I know this has been mentioned in other tickets, but it appears to still be an issue, as of 2.0.4. From the current, master’s failure_app.rb:

def redirect_url
  if warden_message == :timeout
    flash[:timedout] = true
    attempted_path || scope_path
  else
    scope_path
  end
end

The third line there is causing the Rails Flash::FlashHash object instance to carry a flash[:timedout] whose value is of TrueClass. For generalized flash implementations, such as the one generated and recommended by this gem:

<%- flash.each do |name, msg| -%>
  <%= content_tag :div, msg, :id => "flash_#{name}" %>
<%- end -%>

The following HTML is being generated and displayed to visitors:

<div id="flash_timedout">true</div>

Obviously, this is non-ideal. 😃 Is this :timedout flash message actually used for anything? I see that the failure_app is explicitly keeping the :timedout flash message, even though it’s simply a true statement:

def redirect
  store_location!
  if flash[:timedout] && flash[:alert]
    flash.keep(:timedout)
    flash.keep(:alert)
  else
    flash[:alert] = i18n_message
  end
  redirect_to redirect_url
end

Based on a cursory reading of this code, it seems to me that a simple instance variable, which is set to true/false and nilified with each Rack call request, would provide the same functionality and work significantly better.

And, yes, I did see @josevalim’s comment on the use of Flash… I just wonder if either the README and example application layout should be updated to reflect the check for String-iness or the surprising usage of Flash for non-user messages should be removed.

About this issue

Original URL
State: closed
Created 12 years ago
Reactions: 6
Comments: 18 (8 by maintainers)

Commits related to this issue

Merge pull request #1993 from ahmeij/master Add message about :timedout flash to the Readme (resolved #1777) — committed to heartcombo/devise by deleted user 12 years ago
Fix duplicate flash error message caused by devise timeout Previously, if you revisited Bridge Troll after having timed out, you would see one error saying 'Your session expired' and another saying '... — committed to railsbridge/bridge_troll by tjgrathwell 11 years ago
fix devise issue with flash see https://github.com/plataformatec/devise/issues/1777 — committed to whatsopt/WhatsOpt by relf 8 years ago
prevent devise from rendering "true" on session timeout Devise sets `flash[:timedout] = true` which renders "true" with a standard handling of `flash`. Devise says that flash isn't necessarily only m... — committed to MassBailFund/MassBailFund by AlexRiina 5 years ago
Enable Devise timeoutable module Expire the user session after 1 hour of inactivity. Timeoutable uses the FlashHash as global storage which causes issues when displaying flash messages. The workarou... — committed to hmcts/hwf-staffapp by leoapost 4 years ago
Enable Devise timeoutable module (#1062) Expire the user session after one hour of inactivity. Timeoutable uses the FlashHash as storage which causes issues when displaying flash messages and a wo... — committed to hmcts/hwf-staffapp by leoapost 4 years ago
fixed devise flash message of timedout (https://github.com/heartcombo/devise/issues/1777) — committed to ittokunvim/mizusirazu by deleted user 3 years ago
Hide 'timedout' flashes from Devise Devise uses `flash[:timedout]` for recording when a session is timed out and this is automatically displayed to the user. Adding `timedout` to the list of messages... — committed to ministryofjustice/Claim-for-Crown-Court-Defence by jrmhaig 2 years ago

Most upvoted comments

At least in Rails 4.2.4 ActionDispatch::Flash::FlashHash does not extend from Hash and does not have an #except method. So it’s not really a viable option.

While I agree this is a good use of the flash hash, I think its a little stubborn to keep that as a justification when clearly its causing heartburn for people. I mean the intended use of the flash has is clearly messaging, with default methods for :notice and :alert keys and so forth.

+23

tanelsuurhans on Oct 23, 2015

@deivid-rodriguez no because it is a fine use of flash. 😃 Flash is not only about messages but temporary storage.

josevalim on Apr 9, 2015

@ginolon Something like this inside your each block should do the trick

next unless value.is_a? String

thiagotonon on Oct 22, 2015

Would it not be possible to create a separate hash that has the same life cycle as the flash but is specifically for storing Devise specific state?

I guess I would need to look more closely at how this is being used to understand the design needs but it seems like using the flash, which is supposed to be for displaying messages to a user from a controller, to store authentication system state, is a bit of a code smell.

Anyone who was accessing the flash for display like:

- flash.each do |key, msg|

is going to get bit by the :timedout key when they upgrade to Devise 2.0, and flash messages on timeouts are not something that most devs will be testing if they assume that Devise should handle that automagically.

jesseclark on Jan 22, 2013

Given that it’s quite common to print flash messages just as-is, at the very least this unexpected behavior should be mentioned somewhere. Got bitten by this issue today as well, but I could not find any mention of it in the README even though this issue has been closed.

I understand it’s convenient to use flash to have the message expire automatically from the session, but sprinkling respond_to? everywhere is not very clean either (where’d that have us all end up?). Could thread-local variables be used as an alternative to instance variables and the env? It’s just an idea, no idea if it’s doable.

Thanks!

Burgestrand on Apr 26, 2012