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

Closed
nbibler opened this Issue Apr 4, 2012 · 18 comments

Comments

Projects
None yet
10 participants

nbibler commented Apr 4, 2012

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.

Owner

josevalim commented Apr 4, 2012

Yes, (unfortunately?) we are using such values. The problem with using instance variables in the Rack stack is that they are not threadsafe (the stack is shared between all threads). So I think our best option is to indeed improve the docs and let people know that Devise uses flash to keep other state besides flash messages.

nbibler commented Apr 4, 2012

That certainly makes sense. Another alternative, perhaps, is to use the Rack env to store a devise.timedout value, and look to that. The env is threadsafe and less surprising than Rails's flash.

Owner

josevalim commented Apr 4, 2012

Yeah, I tried it out with the env after I replied to you and I realized it won't work because redirects are involved. So we really need to store it in the session. :S

josevalim closed this Apr 17, 2012

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!

Owner

josevalim commented Apr 26, 2012

Nope, thread variables are not an alternative because we need to know this information in another request, which may happen in a different process, different machine. If someone want to sprinkle up the readme or add a note to the README we show when you call devise:install, patches are welcome.

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.

At best the flash notice is a nuisance, but at worst it is confusing to users.

ris321 commented Aug 31, 2013

Why can't this be put into the session hash? session[:timeout]

@tjgrathwell tjgrathwell added a commit to railsbridge/bridge_troll that referenced this issue Dec 11, 2013

@tjgrathwell tjgrathwell 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 'true'.

Devise inserts a :timedout key into the FlashHash, which careful programmers
have to avoid. See plataformatec/devise#1777

Also: pasting over the latest Devise en.yml for fun.
44ef346

@vreen vreen added a commit to vreen/growlyflash that referenced this issue Jan 15, 2014

@vreen vreen remove :timedout from the session hash (plataformatec/devise#1777) 8bf2188
Contributor

deivid-rodriguez commented Apr 9, 2015

@josevalim Would you be willing to accept a PR that moves this to the session hash? It seems like a more appropriate place than using flash messages, right?

Owner

josevalim commented Apr 9, 2015

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

Contributor

deivid-rodriguez commented Apr 9, 2015

@josevalim Alright, this is all very subjective.

I just happened to read the code handling this and it seemed like a code smell (https://github.com/plataformatec/devise/blob/master/lib/devise/failure_app.rb#L58-63). It's not clear what it does, it's not commented, it's not tested, there's quite a lot of issues linking here, and that use of keep doesn't intuitively suggest temporary storage...

Owner

josevalim commented Apr 9, 2015

We don't have unit tests but we do have integration tests. The flash is an implementation detail to how the feature works. Also, notice we also keep the alert, that's more of a bug of how flash behaves in Rails because it doesn't survive redirects...

Contributor

deivid-rodriguez commented Apr 9, 2015

Really? I thought they survived a single action... regardless of it being a redirect or not.

@ghost

ghost commented Apr 20, 2015

Excuse me, how to exclude ":timedout" flash messages from this code:

"flash.each do |msg_type, message|"

that I have in an helper?

dyatlov referenced this issue in sgruhier/foundation_rails_helper Apr 24, 2015

Closed

Flash helper needs improvement :) #98

@ginolon a bit late but perhaps something like this in your block:

unless msg_type == :timedout

or do a check before your block:

flash.except(:timedout).each do |msg_type, message|
@ghost

ghost commented Oct 13, 2015

@tonycchung If I use the except way, I got this:

undefined method 'except' for #<ActionDispatch::Flash::FlashHash:0x0000000d3df308>

def flash_messages(opts = {})
    flash.except(:timedout).each do |msg_type, message|
      concat(content_tag(:div, message, class: "alert #{bootstrap_class_for(msg_type)} alert-dismissible text-center fade in", role: 'alert') do
        concat(content_tag(:button, class: 'close', data: { dismiss: 'alert' }) do
          concat content_tag(:span, '&times;'.html_safe, 'aria-hidden' => true)
          concat content_tag(:span, 'Close', class: 'sr-only')
        end)
        concat message
      end)
    end
    nil
  end

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

next unless value.is_a? String

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.

@pivotal-casebook pivotal-casebook pushed a commit to Casecommons/calico that referenced this issue Jun 8, 2016

Jimmy Chao and Oana Tararache Fix timedout error message with flash[:timedout] = true 2c25801

@relf relf added a commit to relf/WhatsOpt that referenced this issue Nov 14, 2016

@relf relf fix devise issue with flash see plataformatec/devise#1777 2f30197

atruskie referenced this issue in QutBioacoustics/baw-server Jan 5, 2017

Closed

Weird notification bug #242

@janraasch janraasch added a commit to janraasch/devise that referenced this issue Mar 10, 2017

@janraasch janraasch Fix Timeoutable timeouts flash hash keys
See plataformatec#1777 for an in-depth discussion of the unexpected behaviour.

See plataformatec#4038 for the fix, but awaiting tests.

The behaviour still works as expected for redirects, see

```
test 'error message with i18n'
test 'error message with i18n with double redirect'
```

in `integration/timeoutable_test.rb`.
695d3c7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment