Rails Ruby on Rails

Comments and changes to this ticket

• You flagged this item as spam.
  

  Scott Taylor February 13th, 2009 @ 01:48 AM

  See the comments in #1823 for more info

• You flagged this item as spam.
  

  Scott Taylor February 13th, 2009 @ 04:51 AM

  • Tag changed from “2.3, actionpack, bugs, cookiestore, sessions” to “2.3, actionpack, bug, bugs, cookiestore, patch, sessions”

  Nolan - Can you try out the patch I'm attaching to this ticket?
  

  It looks like the Set-Cookie header isn't being set properly. Mongrel expects an options hash like {:Set-Cookie => "var=value"}, but occasionally an array will be given instead, like so: {"Set-Cookie" => ["var=value", "var2=value"]}

  Looks like thin handles these nested values. Mongrel doesn't, and ends up sending the header "Set-Cookie: " - i.e., with an empty value.

  • tmp.diff 3.6 KB

• You flagged this item as spam.
  

  Scott Taylor February 13th, 2009 @ 04:56 AM

  Actually, try the following patch. The code is a bit cleaner.

  • 0001-Set-Cookie-header-should-always-be-a-string-never-a.patch 4.3 KB

• You flagged this item as spam.
  

  Nolan Eakins February 14th, 2009 @ 01:26 AM

  Well Scott, your patch seemed to fix the issue I was having. Congrats.

  Now to annoy a comitter...

• 

  josh February 14th, 2009 @ 03:44 AM

  • Milestone cleared.
  • Assigned user set to “josh”

• You flagged this item as spam.
  

  Tim Pope February 14th, 2009 @ 09:44 AM

  The real issue (at least that I'm seeing) is that multiple cookies in a single response are concatenated into a single malformed Set-Cookie header (as if Array#to_s is called) rather than separate headers. The patch prevents this by giving the last cookie precedence but at the cost of limiting a response to setting a single cookie.

  I've yet to find the proper fix for this issue but -1 on this patch.

• You flagged this item as spam.
  

  Nolan Eakins February 14th, 2009 @ 06:51 PM

  • Assigned user cleared.

  I'm starting to look at this a little deeper. I'm seeing a ton of duplicated cookie building code in ActionPack and Rack. Namely in:

  • Rails actionpack/lib/action_controller/response.rb actionpack/lib/action_controller/session/cookie_store.rb
  • Rack: rack/lib/rack/response.rb

  Not sure why there needs to be three implementations that at first glance seem to do the same.

• You flagged this item as spam.
  

  Nolan Eakins February 14th, 2009 @ 06:52 PM

  • Assigned user set to “josh”

  Oops. Cleared the assigned user on this ticket.

• You flagged this item as spam.
  

  Nolan Eakins February 14th, 2009 @ 06:59 PM

  Looks like actionpack/lib/action_controller/session/abstract_store.rb also includes the same cookie building snippet as those files. Definitely getting off topic here, but I'm seeing this all over the place:

            cookie = Rack::Utils.escape(@key) + '=' + Rack::Utils.escape(sid)
            cookie << "; domain=#{options[:domain]}" if options[:domain]
            cookie << "; path=#{options[:path]}" if options[:path]
            if options[:expire_after]
              expiry = Time.now + options[:expire_after]
              cookie << "; expires=#{expiry.httpdate}"
            end
            cookie << "; Secure" if options[:secure]
            cookie << "; HttpOnly" if options[:httponly]
  
            headers = response[1]
            case a = headers[SET_COOKIE]
            when Array
              a << cookie
            when String
              headers[SET_COOKIE] = [a, cookie]
            when nil
              headers[SET_COOKIE] = cookie
            end
          end
  

• You flagged this item as spam.
  

  Nolan Eakins February 14th, 2009 @ 08:06 PM

  More poking. My finger is tempted to point at the mongrel_rails command for not using Rack. Did come across http://www.ruby-forum.com/topic/... which is a thread about this exact issue.

• You flagged this item as spam.
  

  Josh Pencheon February 14th, 2009 @ 09:32 PM

  I commented on Scott's ticket, but I seem to be having a similar problem with Passenger.

• 

  josh February 15th, 2009 @ 12:27 AM

  There were some 1.9 spec changes in Rack recently. Headers need to be strings instead of arrays.

  
            cookie = build_cookie(@key, cookie.merge(options))
            unless headers[HTTP_SET_COOKIE].blank?
              headers[HTTP_SET_COOKIE] << "\n#{cookie}"
            else
              headers[HTTP_SET_COOKIE] = cookie
            end
  

  Not sure how this changes things. Can you please try again with edge rails.

• You flagged this item as spam.
  

  Nolan Eakins February 15th, 2009 @ 12:35 AM

  Joshua, I just tried master with mongrel_rails and no improvement there. I should give Mongrel's head a try, but a no go w/ 1.1.5 which given my poking around earlier today I'm assuming is not using Rack.

• You flagged this item as spam.
  

  David Reese February 15th, 2009 @ 05:03 PM

  On edge rails (238a6bb62dc153743a0abc6eb1e35392ac799d65) and passenger (2.0.6, osx), I still have problems setting any kind of cookies. On edge last week (as of feb 10), I had the same problem running mongrel; today, mongrel sets cookies where passenger fails.

  == the details...

  Literally spent hours one day last week trying to debug this, and got nowhere. Watching the cookies get set with the charles proxy, my impression was that the header was malformed -- too many newlines I think -- and the cookie header ended up in the content. Testing now again with edge seems to support that -- I'm actually getting "Set-Cookie: _myapp_session=BAh7CToPc2Vzc2lv..." displayed in the browser, before the <!DOCTYPE. !

  But there are so many moving parts (rails/rack/passenger) I wasn't sure where to look to narrow down the problem. Hopefully you know where to look? Not sure if I'm doing anything weird -- it's a pretty vanilla resful_auth setup.

• You flagged this item as spam.
  

  Scott Taylor February 16th, 2009 @ 01:23 AM

  I take Tim Pope's and Nolan's comments to heart. I'm not super fond of the patch, and there is a crapload of duplicated code (almost all of the session cookie store and the abstract session store are duplicated code).

  I'll look into searching for all of the places where a cookie may be built into an array, which is the real issue.

  Does the http spec allow sending multiple Set-Cookie headers?

• 

  josh February 16th, 2009 @ 09:23 PM

  • State changed from “new” to “open”

  Confirmed. Working on issue.

  The fix should be applied to ActionController::CGIHandler

  
  $ curl -I localhost:3000/hello
  HTTP/1.1 200 OK
  Connection: close
  Date: Mon, 16 Feb 2009 21:21:35 GMT
  X-Runtime: 2
  ETag: "bea8252ff4e80f41719ea13cdf007273"
  X-Test: 1
  Content-Type: text/html; charset=utf-8
  Cache-Control: private, max-age=0, must-revalidate
  Content-Length: 14
  
  $ curl -I localhost:3000/hello
  HTTP/1.1 200 OK
  Connection: close
  Date: Mon, 16 Feb 2009 21:21:54 GMT
  Status: 200
  ETag: "bea8252ff4e80f41719ea13cdf007273"
  X-Runtime: 5
  Cache-Control: private, max-age=0, must-revalidate
  X-Test: 1
  2
  3
  Server: Mongrel 1.1.5
  Content-Type: text/html; charset=utf-8
  Set-Cookie: 
  Content-Length: 14
  

• 

  Repository February 17th, 2009 @ 04:18 AM

  • State changed from “open” to “resolved”

  (from [b6e56efe07cb3c2e999216f995403aa9206226a2]) Special case in deprecated CGI proxy layer for Mongrel CGI cookies [#1957 state:resolved] http://github.com/rails/rails/co...

• You flagged this item as spam.
  

  Nolan Eakins February 17th, 2009 @ 05:10 AM

  Good job there Josh. Checked out "master" and mongrel_rails allowed me to login. Seems to fix my issue. Now I just need a reason to drop Thin. :-)

• You flagged this item as spam.
  

  David Reese February 17th, 2009 @ 05:23 AM

  Didn't fix what I assumed was the same issue -- the session isn't working in Passenger either, which Josh Pencheon and I commented on above.

  On edge after that last commit (b6e56), I still get the "Set-Cookie" header showing up in the rendered HTML. Firefox 3, OSX, Passenger.

  If I should open a new ticket ("Sessions break in 2.3 with Passenger"), I will, but I don't know much more than I mentioned in my last comment.

• 

  josh February 17th, 2009 @ 05:33 AM

  @David The latest version version of passenger does not support the new rack 1.9 spec yet. I'll hook up with Hongli and we'll get this in.

• You flagged this item as spam.
  

  David Reese February 17th, 2009 @ 05:43 AM

  Thanks josh, I missed that somehow.

  So, all passenger users will have to upgrade (to a future Passenger version) to run 2.3? We should put that in writing somewhere, right? Just so nobody trying to help test 2.3 wastes any time trying to fix their cookies (like i did).

• 

  josh February 17th, 2009 @ 05:58 AM

  David, I'm not sure. It may be possible for them to release a backwards and forward compat version of passenger. We'll see.

  Watch the passenger blog for updates: http://blog.phusion.nl/

• You flagged this item as spam.
  

  David Reese February 17th, 2009 @ 05:44 PM

  @Joshua -- it's a marketing/documentation thing more than anything -- that nowhere on the blog post about the Rails 2.3 beta, or on the release notes, or the changelog even, does it say anything about the session issue with Passenger. I'm not sure where I file a bug on the release notes?

  Let me put it another way -- the 2.3RC1 blog post says, "So please help us do thorough testing of this release candidate. Lots of the underpinnings changed. Especially the move to Rack." So how many other people like me are trying it out, trying to be helpful and tracking down bugs, and then getting stuck on the passenger issue? Like, maybe there should be a "Known issues" list or something.

  Just trying to be helpful here, let me know if there's something I'm missing.

• You flagged this item as spam.
  

  Luigi Montanez February 18th, 2009 @ 07:40 PM

  The Phusion guys just fixed the Passenger problem:

  http://github.com/FooBarWidget/p...

  Just clone the repo and follow the README. Hurrah!

• 

  Ryan Bigg October 9th, 2010 @ 09:45 PM

  • Tag cleared.
  • Importance changed from “” to “Low”

  Automatic cleanup of spam.

• 

  Jeff Kreeftmeijer November 8th, 2010 @ 08:23 AM

  Automatic cleanup of spam.

• You flagged this item as spam.
  

  bingbing March 26th, 2011 @ 02:05 AM

  louis vuitton watches