Loosen the domain regex to accept '.' #29

tubaxenor · 2022-11-22T10:13:16Z

Domain leading with . should be allowed since it represents all subdomain are sharing the same cookie.

tubaxenor · 2022-11-22T10:14:55Z

Seems like the leading dot is old standard according to the discussion here: https://stackoverflow.com/a/23086139
But I think this lib should still support this instead of treating it as invalid value.

tubaxenor · 2022-11-22T14:19:42Z

Looks like I did the regex wrong, @nobu @hsbt any chance I can get some help here?

lib/cgi/cookie.rb

casperisfine · 2022-11-23T08:05:58Z

LGTM, but you probably want to squash these commits and to remove that blank line in the diff.

casperisfine · 2022-11-23T09:32:49Z

The CI failures are already on master.

mame · 2022-11-24T02:02:10Z

This is a consequence of our decision to comply with RFC 6265.

https://www.rfc-editor.org/rfc/rfc6265#section-4.1.1

 domain-value      = <subdomain>
                       ; defined in [RFC1034], Section 3.5, as
                       ; enhanced by [RFC1123], Section 2.1

It looks like the referenced RFCs do not allow a leading dot.

https://www.rfc-editor.org/rfc/rfc1034#section-3.5
https://www.rfc-editor.org/rfc/rfc1123#section-2

However, a leading dot was required in RFC 2109 (which has been already obsoleted).

https://datatracker.ietf.org/doc/html/rfc2109#section-4.2.2

   Domain=domain
      Optional.  The Domain attribute specifies the domain for which the
      cookie is valid.  An explicitly specified domain must always start
      with a dot.

We may allow leading dots for compatibility. @akr @nurse What do you think?

lib/cgi/cookie.rb

Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>

kylesnowschwartz · 2022-11-25T06:24:00Z

Hi all, we are encountering this problem on my project when trying to update to the patched version of Ruby 2.7.7. Our test builds are failing due to the leading . on our cookie domains. This is a Rails project version 2.6.7. It would be great to have this PR out so that we can patch up this security flaw as soon as possible.

casperisfine · 2022-11-25T07:43:12Z

I just noticed that this is breaking Rails test suite as well: https://buildkite.com/rails/rails/builds/91200#0184aae9-a971-4423-8bb6-60e7a14ec3fb/1048-1057

kenvatian · 2022-11-25T10:54:29Z

Thank you for your work, and for picking up this issue so quickly.

My 2 cents: I think, sticking with RFC6265 Section 4.1.1 is the future. To get there, maybe a version is released to deprecate the leading dot and allow various frameworks a grace period to remove support of the leading dot. Then release another version that raises an exception for the leading dot. Thanks.

P.S. As per https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie#domaindomain-value, it's odd that frameworks are still producing domain value with leading dot, because

...if a domain is specified, then subdomains are always included.

mame · 2022-11-28T03:27:17Z

@naruse found the following statements in RFC 6265

5.2.3. The Domain Attribute

snip

If the first character of the attribute-value string is %x2E ("."):

Let cookie-domain be the attribute-value without the leading %x2E
(".") character.

So it looks like RFC 6265 actually allows a leading dot. I no longer see any reason not to merge this PR.

(ruby/cgi#29) * Loosen the domain regex to accept '.' Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org> ruby/cgi@5e09d632f3 Co-authored-by: Hiroshi SHIBATA <hsbt@ruby-lang.org>

…_with_all_domains This commit addresses the following error by using cgi 0.3.6 that includes ruby/cgi#29 ```ruby $ ruby -v ruby 3.1.3p185 (2022-11-24 revision 1a6b16756e) [x86_64-linux] $ gem info cgi *** LOCAL GEMS *** cgi (0.3.5) Author: Yukihiro Matsumoto Homepage: https://github.com/ruby/cgi Licenses: Ruby, BSD-2-Clause Installed at (default): /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0 Support for the Common Gateway Interface protocol. $ ``` ``` $ cd actionpack $ bin/test test/dispatch/session/cookie_store_test.rb -n test_session_store_with_all_domains Running 27 tests in a single process (parallelization threshold is 50) Run options: -n test_session_store_with_all_domains --seed 24288 E Error: CookieStoreTest#test_session_store_with_all_domains: ArgumentError: invalid domain: ".example.com" /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/3.1.0/cgi/cookie.rb:128:in `domain=' /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/3.1.0/cgi/cookie.rb:95:in `initialize' /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/mock.rb:239:in `new' /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/mock.rb:239:in `block in parse_cookies_from_header' /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/mock.rb:236:in `each' /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/mock.rb:236:in `parse_cookies_from_header' /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/rack-2.2.4/lib/rack/mock.rb:187:in `initialize' /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/rack-test-2.0.2/lib/rack/test.rb:360:in `new' /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/rack-test-2.0.2/lib/rack/test.rb:360:in `process_request' /home/yahonda/.rbenv/versions/3.1.3/lib/ruby/gems/3.1.0/gems/rack-test-2.0.2/lib/rack/test.rb:155:in `request' /home/yahonda/src/github.com/rails/rails/actionpack/lib/action_dispatch/testing/integration.rb:285:in `process' /home/yahonda/src/github.com/rails/rails/actionpack/lib/action_dispatch/testing/integration.rb:16:in `get' /home/yahonda/src/github.com/rails/rails/actionpack/lib/action_dispatch/testing/integration.rb:376:in `get' /home/yahonda/src/github.com/rails/rails/actionpack/test/dispatch/session/cookie_store_test.rb:420:in `get' /home/yahonda/src/github.com/rails/rails/actionpack/test/dispatch/session/cookie_store_test.rb:379:in `block in test_session_store_with_all_domains' /home/yahonda/src/github.com/rails/rails/actionpack/test/dispatch/session/cookie_store_test.rb:438:in `block in with_test_route_set' /home/yahonda/src/github.com/rails/rails/actionpack/test/abstract_unit.rb:157:in `with_routing' /home/yahonda/src/github.com/rails/rails/actionpack/test/dispatch/session/cookie_store_test.rb:424:in `with_test_route_set' /home/yahonda/src/github.com/rails/rails/actionpack/test/dispatch/session/cookie_store_test.rb:378:in `test_session_store_with_all_domains' bin/test test/dispatch/session/cookie_store_test.rb:377 Finished in 0.079807s, 12.5303 runs/s, 0.0000 assertions/s. 1 runs, 0 assertions, 0 failures, 1 errors, 0 skips $ ``` Related to ruby/cgi#29 rails#46578

taketo1113 · 2022-12-13T00:40:00Z

Any plans to backport this PR to v0.2.x (for ruby 3.0.x) and v0.1.x (for ruby 2.7.x)?

If no backport, it seems to need updating cgi gem per release of ruby 3.0.5+.

Per ruby/cgi#29 the current shipped version of the CGI gem doesn't allow for leading dots in domain names, which breaks setting cookies like `.example.com`.

* FIX: Ensure we have a patched version of CGI gem Per ruby/cgi#29 the current shipped version of the CGI gem doesn't allow for leading dots in domain names, which breaks setting cookies like `.example.com`. * Update Gemfile Co-authored-by: Jarek Radosz <jradosz@gmail.com> Co-authored-by: Jarek Radosz <jradosz@gmail.com>

…5dcf5326ea2c8e2047a3bddeb0fbb7e7d07649,b335d899fff3cc22b022c9ee2ceb636d714bf1a7: [Backport #19153] [ruby/cgi] Fix test_cgi_cookie_new_with_domain to pass on older rubies ruby/cgi@05f0c58048 --- test/cgi/test_cgi_cookie.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) [ruby/cgi] Prepare to release 0.3.6 ruby/cgi@710a647855 --- lib/cgi.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) [ruby/cgi] Loosen the domain regex to accept '.' (ruby/cgi#29) * Loosen the domain regex to accept '.' Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org> ruby/cgi@5e09d632f3 Co-authored-by: Hiroshi SHIBATA <hsbt@ruby-lang.org> --- lib/cgi/cookie.rb | 2 +- test/cgi/test_cgi_cookie.rb | 3 +++ 2 files changed, 4 insertions(+), 1 deletion(-) [ruby/cgi] Bump up 0.3.6 ruby/cgi@827b7d43cc --- lib/cgi.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

See ruby/cgi#29

One test failure which isn't a regression - it's because of the CGI gem [0]. With the CGI gem monkeypatched locally, it all works. [0] https://discuss.rubyonrails.org/t/invalid-domain-example-com-in-rspec-after-changing-session-store-to-domain-all/81922 [1] ruby/cgi#29 Signed-off-by: Sam James <sam@gentoo.org>

In preparation for an update to Ruby 3.0; starting in Ruby 2.7.7, Ruby by default targets version 0.3.5 of the `cgi` gem, which fixed a security issue but also made the domain validation more restrictive than necessary. Version 0.3.6 loosens the restrictions. - https://github.com/ruby/cgi/releases/tag/v0.3.6 - ruby/cgi#29 - https://johnathan.org/ruby-2-7-7-invalid-domain/ Without this change, we get several hundred `ArgumentError: invalid domain: ".code.org"` errors in Dashboard tests on Ruby 3. With this change, we do not.

gareth · 2023-04-23T17:29:39Z

So it looks like RFC 6265 actually allows a leading dot. I no longer see any reason not to merge this PR.

This is true but slightly misleading. RFC6265 doesn't strictly allow a leading dot (it actually defines the domain parameter as not permitting a leading dot), but it gives a fallback behaviour for UAs to be able to handle older RFC2965 cookies.

I recognise that this was probably already taken into account as part of "our decision to comply with RFC 6265", but being clear about the difference is important, because there is discussion pointing to this PR as justification for keeping leading dots on cookies in Rails.

In fact, there's an argument that maybe this PR didn't go far enough? As you quoted, the RFC6265 behaviour is this:

Let cookie-domain be the attribute-value without the leading %x2E (".") character.

However the library persists the leading dot:

$ irb -r cgi
irb(main):001:0> cookie = CGI::Cookie.new "name" => "foo", "domain" => ".example.com"
=> []
irb(main):002:0> cookie.domain
=> ".example.com"

This PR was enough to stop exceptions being raised for cookies that browsers would handle though, and that's clearly a good thing regardless of the extra details.

Ruby stdlib doesn't allow cookies to be set with domains with leading dots. Rails, unfortunately, sets cookies with domains with leading dots (this is technically incorrect to do) A newer version of the CGI library that handles this case is available, but only by including the gem version of the library. See: ruby/cgi#29

See ruby/cgi#29

casperisfine reviewed Nov 22, 2022

View reviewed changes

lib/cgi/cookie.rb Outdated Show resolved Hide resolved

tubaxenor requested a review from casperisfine November 23, 2022 05:56

Loosen the domain regex to accept '.'

77fb46c

tubaxenor force-pushed the relax-domain-label-more branch from 468c92c to 77fb46c Compare November 23, 2022 08:22

nobu requested changes Nov 24, 2022

View reviewed changes

lib/cgi/cookie.rb Outdated Show resolved Hide resolved

Update lib/cgi/cookie.rb

6a49473

Co-authored-by: Nobuyoshi Nakada <nobu@ruby-lang.org>

hsbt mentioned this pull request Nov 25, 2022

cgi 0.34 or higher does not allow .example.com as domain #35

Closed

Merge branch 'master' into relax-domain-label-more

59c20fe

nobu approved these changes Nov 25, 2022

View reviewed changes

flavorjones mentioned this pull request Nov 25, 2022

Setting cookies domain doesn't work with Ruby 3.1.3 and integration tests rails/rails#46578

Closed

hsbt added 2 commits November 28, 2022 12:35

Merge branch 'master' into relax-domain-label-more

1d1517a

Merge branch 'master' into relax-domain-label-more

718a590

hsbt merged commit 5e09d63 into ruby:master Nov 28, 2022

yahonda mentioned this pull request Nov 28, 2022

Use cgi 0.3.6 or higher to address CookieStoreTest#test_session_store_with_all_domains rails/rails#46595

Merged

5 tasks

grosscr mentioned this pull request Nov 28, 2022

Merge CGI-0.3.5 ruby/ruby#6784

Merged

tubaxenor deleted the relax-domain-label-more branch November 30, 2022 07:57

cyril-sf mentioned this pull request Nov 30, 2022

Since 2.7.7 CGI::Cookie raises ArgumentError when cookie domains is prefixed with a dot #36

Closed

xfalcox mentioned this pull request Dec 22, 2022

FIX: Ensure we have a patched version of CGI gem discourse/discourse#19588

Merged

nikolai-b added a commit to cyclestreets/cyclescape that referenced this pull request Mar 17, 2023

Fix CI

a87d895

See ruby/cgi#29

Hamms mentioned this pull request Apr 20, 2023

Update cgi Gem to 0.3.6 code-dot-org/code-dot-org#51427

Merged

gareth mentioned this pull request Apr 23, 2023

Make Rails cookies RFC6265-compliant with domain: :all rails/rails#48036

Merged

4 tasks

heliocola mentioned this pull request Jun 2, 2023

Update ruby version to 3.0.6, latest in the 3.0 series. forem/forem#19523

Closed

2 tasks

nikolai-b added a commit to cyclestreets/cyclescape that referenced this pull request Aug 8, 2023

Fix CI

a8c7262

See ruby/cgi#29

nikolai-b added a commit to cyclestreets/cyclescape that referenced this pull request Aug 12, 2023

Fix CI

39694ad

See ruby/cgi#29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Loosen the domain regex to accept '.' #29

Loosen the domain regex to accept '.' #29

tubaxenor commented Nov 22, 2022

tubaxenor commented Nov 22, 2022

tubaxenor commented Nov 22, 2022

casperisfine commented Nov 23, 2022

casperisfine commented Nov 23, 2022

mame commented Nov 24, 2022

kylesnowschwartz commented Nov 25, 2022

casperisfine commented Nov 25, 2022

kenvatian commented Nov 25, 2022

mame commented Nov 28, 2022

taketo1113 commented Dec 13, 2022

gareth commented Apr 23, 2023 •

edited

Loading

Loosen the domain regex to accept '.' #29

Loosen the domain regex to accept '.' #29

Conversation

tubaxenor commented Nov 22, 2022

tubaxenor commented Nov 22, 2022

tubaxenor commented Nov 22, 2022

casperisfine commented Nov 23, 2022

casperisfine commented Nov 23, 2022

mame commented Nov 24, 2022

kylesnowschwartz commented Nov 25, 2022

casperisfine commented Nov 25, 2022

kenvatian commented Nov 25, 2022

mame commented Nov 28, 2022

taketo1113 commented Dec 13, 2022

gareth commented Apr 23, 2023 • edited Loading

gareth commented Apr 23, 2023 •

edited

Loading