This project is archived and is in readonly mode.

#6002 ✓committed
Ravil Bayramgalin

[PATCH] Ignore domain :all option if host is IP address or localhost

Reported by Ravil Bayramgalin | November 18th, 2010 @ 02:43 PM | in 3.x

Currently, if you use :all option for cookie store domain, cookies are created with wrong domain when site accessed via ip address or localhost.

For example, if we set

config.session_store :cookie_store, :key => '_site_session', :domain => :all

then cookie will be set with wrong domain '.1.1' after we visit our site via ip address 192.168.1.1.

I've attached a small and simple patch which fixes this behavior.

Comments and changes to this ticket

  • Ravil Bayramgalin

    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.

  • Ravil Bayramgalin

    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

    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 ;).

  • Ravil Bayramgalin

    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

    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.

  • Ravil Bayramgalin

    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.

  • Fjan

    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.

  • Rafael Mendonça França

    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...

  • Ravil Bayramgalin

    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.

  • Ravil Bayramgalin

    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.

  • Fjan

    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.

  • Rafael Mendonça França

    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.

  • Ravil Bayramgalin

    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

  • Ravil Bayramgalin

    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.

  • Ravil Bayramgalin

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

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

  • Rafael Mendonça França

    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.

  • Ravil Bayramgalin

    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?

  • Rafael Mendonça França

    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.

  • Ravil Bayramgalin

    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 ;)

  • Rafael Mendonça França
  • Piotr Sarnacki

    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.

  • Ravil Bayramgalin

    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

    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...

  • Ravil Bayramgalin

    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...

  • Aaron Hamid

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

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

Create your profile

Help contribute to this project by taking a few moments to create your personal profile. Create your profile »

<h2 style="font-size: 14px">Tickets have moved to Github</h2>

The new ticket tracker is available at <a href="https://github.com/rails/rails/issues">https://github.com/rails/rails/issues</a>

Referenced by

Pages