Rails Ruby on Rails → 3.x

Comments and changes to this ticket

• You flagged this item as spam.
  

  Ravil Bayramgalin November 18th, 2010 @ 02:51 PM

  I'm not sure of a reason, but I can not attach a patch via lighthouse. So you can get it at http://gist.github.com/705069. Sorry, for inconvenience.

• You flagged this item as spam.
  

  Ravil Bayramgalin November 18th, 2010 @ 11:00 PM

  • Assigned user changed from “Jeff Kreeftmeijer” to “Santiago Pastorino”

  http://gist.github.com/705069 - I've updated patch to prevent setting of localhost as domain for cookies.

• 

  Santiago Pastorino November 19th, 2010 @ 04:09 PM

  • State changed from “new” to “open”
  • Milestone set to “3.x”
  • Importance changed from “” to “Low”

  Ravil, please change the regexp to match IPv4 and IPv6 addresses.
  Thanks for helping ;).

• You flagged this item as spam.
  

  Ravil Bayramgalin November 19th, 2010 @ 05:13 PM

  http://gist.github.com/705069 - I've updated patch so explicit domain for cookies with domain :all option is set only when host is a domain name.

  Always glad to help ;)

• 

  Santiago Pastorino November 19th, 2010 @ 08:14 PM

  I think you should set the domain for ipv4 and ipv6 addresses.
  And fix the regexp to match this addresses.
  Thanks.

• You flagged this item as spam.
  

  Ravil Bayramgalin November 19th, 2010 @ 11:58 PM

  In irc we've come to conclusion that rails should set nil domain for ipv4/ipv6 addresses. It equals to implicit setting of domain for cookies. Therefore behavior of attached patch is appropriate.

  However, Santiago suggested to try and add a check for ip address inside of domain regexp. Unfortunately upon closer inspection it seems really hard to do if not impossible, so I suggest to use current patch as it is.

• You flagged this item as spam.
  

  Fjan December 6th, 2010 @ 03:51 PM

  The patch still sets an illegal cookie domain name on localhost, you could improve it a little bit by checking @host =~ /\w.[a-z]\w/i
  this will make sure there is at least one dot and not all of them are numbers.

• You flagged this item as spam.
  

  Rafael Mendonça França December 6th, 2010 @ 04:53 PM

  Domains like lvh.me don't works too. It generate a cookie for lvh.me, and for each subdomains. This is doing my flash messages don't works as it should.

  I create a ticket for this. https://rails.lighthouseapp.com/projects/8994/tickets/6120-flash-me...

• You flagged this item as spam.
  

  Ravil Bayramgalin December 7th, 2010 @ 04:29 AM

  Fjan, could you give an example why do you think that patch sets illegal cookie domain name on localhost? As you could see in tests from patch I explicitly check this case:

  def test_cookie_with_all_domain_option_using_localhost
    @request.host = "localhost"
    get :set_cookie_with_domain
    assert_response :success
    assert_cookie_header "user_name=rizwanreza; path=/"
  end
  

  And all tests pass. It works because both /^[\d.]+$/ and DOMAIN_REGEXP check a mandatory dot in hostname, if both checks fail then "when" expression returns default value for "else" branch which is nil. Setting options[:domain]=nil makes rails not to set an explicit domain name for cookies therefore we're using an implicit domain name which is completely legal.

• You flagged this item as spam.
  

  Ravil Bayramgalin December 7th, 2010 @ 04:34 AM

  Rafael Mendonça França, I would gladly provide an additional patch for domain :all handling so you could set an arbitrary TLD length or provide a list of base domain names on which your application is running. But it will be a separate path from this one because of the patch goals.

  And to supply a new patch in this direction I need to know will current patch be applied or not so I could use appropriate codebase.

• You flagged this item as spam.
  

  Fjan December 7th, 2010 @ 10:30 AM

  @Ravil, you are right, I stand corrected. Would be nice if we can think of a regex that does both checks in one go though.

• You flagged this item as spam.
  

  Rafael Mendonça França December 7th, 2010 @ 12:20 PM

  @Ravil, thank you. I would really appreciate if you submit another patch for this.

• You flagged this item as spam.
  

  Ravil Bayramgalin December 9th, 2010 @ 05:34 PM

  @Fjan, yes, it would be nice to make a domain regexp which not conforms to ip address. Initially I had the same idea, unfortunately, during development i quickly discovered that it's not possible. Lets take a look at current domain regexp - ([^.]).([^.]|.....|......)$

  It's pretty self-explanatory if you take a close look. But to exclude ip address from this regexp we would need to add a condition that's not all symbols are digitals (there is at least one non-digital symbol except from dot), and in the same time we still need to match different parts of host to separate domain and subdomain. It's a perfect place for Regexp.intersection, unfortunately, there is no such method only Regexp.union. Lookahead/lookbehind won't work in this case, cause then we won't match a whole string.

  But I've found a good alternative to having a one regexp
  

  (@host !~ /^[\d.]+$/) && (@host =~ DOMAIN_REGEXP)
  

  So I've rewrote my patch and added commentary so it will be easier to understand at first glance.

  New patch is located here https://gist.github.com/734907

• You flagged this item as spam.
  

  Ravil Bayramgalin December 9th, 2010 @ 05:38 PM

  @Rafael, I've adding a second patch which supports supplying list of domains for cookies.

  For example, you could set :domain => %w(.lvh.me .some-other-domain.com) and your cookies will be shared on lvh.me and some-other-domain.com domains.

• You flagged this item as spam.
  

  Ravil Bayramgalin December 9th, 2010 @ 05:48 PM

  @Rafael, oops, sorry forgot to attach a link to second patch https://gist.github.com/734913

• You flagged this item as spam.
  

  Rafael Mendonça França December 9th, 2010 @ 05:59 PM

  It's look nice. But my problem is when I have domains like this www.app.me, mysubdomain.app.me. They should share the same cookies with domain .app.me. But they have a domain for each case.

• You flagged this item as spam.
  

  Ravil Bayramgalin December 9th, 2010 @ 06:05 PM

  To share cookies with subdomains for one domain, you could simply set :domain => '.app.me' or does this not work for you?

• You flagged this item as spam.
  

  Rafael Mendonça França December 9th, 2010 @ 06:13 PM

  Sure. This works fine, but I have to set the cookie store for each environment in my application, making the session_store initialer useless.

  It would be interesting that the domain could be modified without the need to set the session_store in each environment.

• You flagged this item as spam.
  

  Ravil Bayramgalin December 9th, 2010 @ 06:16 PM

  Oh, I see. Then if you prefer to set a custom tld length instead of passing a list of domains, give me another day I'll cook up one more patch ;)

• You flagged this item as spam.
  

  Rafael Mendonça França December 9th, 2010 @ 06:19 PM

  Ok. Thanks @Ravil.

• 

  Piotr Sarnacki December 10th, 2010 @ 08:57 AM

  First patch (https://gist.github.com/734907) looks good to me.

  Could you please open a new ticket for the second one? This will get messy if both are discussed here.

• You flagged this item as spam.
  

  Ravil Bayramgalin December 15th, 2010 @ 12:39 PM

  • Assigned user changed from “Santiago Pastorino” to “Piotr Sarnacki”

  Ok, sure I'll create a separate ticket for additional functionality but can you please merge this one if it looks good? Cause additional functionality is based on changes in first patch anyway.

• 

  Repository December 15th, 2010 @ 01:04 PM

  • State changed from “open” to “committed”

  (from [439c23dce33148064c258eaf6e79f9d4563c88a4]) Fix edge cases for domain :all option on cookie store

  Dont set explicit domain for cookies if host is not a domain name

  [#6002 state:committed]

  Signed-off-by: Santiago Pastorino santiago@wyeworks.com
  https://github.com/rails/rails/commit/439c23dce33148064c258eaf6e79f...

• You flagged this item as spam.
  

  Ravil Bayramgalin December 19th, 2010 @ 08:00 PM

  @Rafael, sorry it took me a long time, but I have finally added requested functionality to set custom tld length. You can see patch at https://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets...

• You flagged this item as spam.
  

  Aaron Hamid April 3rd, 2011 @ 04:58 AM

  this is awesome, thanks Ravil! I spent so much time working around issues related to this.