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

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.

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

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

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.

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.

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. #6120 Flash message don't clear

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.

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.

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

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

@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

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

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

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.

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

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.

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

Ok. Thanks @Ravil.

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.

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

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