diff --git a/CHANGELOG.md b/CHANGELOG.md index 82aa7ffc2f..fcae35383a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ * [#2384](https://github.com/ruby-grape/grape/pull/2384): Allow to use `before/after/rescue_from` methods in any order when using `mount` - [@jcagarcia](https://github.com/jcagarcia). * [#2390](https://github.com/ruby-grape/grape/pull/2390): Drop support for Ruby 2.6 and Rails 5 - [@ericproulx](https://github.com/ericproulx). * [#2393](https://github.com/ruby-grape/grape/pull/2393): Optimize AttributeTranslator - [@ericproulx](https://github.com/ericproulx). +* [#2395](https://github.com/ruby-grape/grape/pull/2395): Set `max-age` to 0 when `cookies.delete` - [@ericproulx](https://github.com/ericproulx). * Your contribution here. #### Fixes diff --git a/Gemfile b/Gemfile index ddd2a49f25..c2c7a20755 100644 --- a/Gemfile +++ b/Gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/multi_json.gemfile b/gemfiles/multi_json.gemfile index b1c6e91e0c..286f89b12b 100644 --- a/gemfiles/multi_json.gemfile +++ b/gemfiles/multi_json.gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/multi_xml.gemfile b/gemfiles/multi_xml.gemfile index 02fdd91cf5..6e2a29733f 100644 --- a/gemfiles/multi_xml.gemfile +++ b/gemfiles/multi_xml.gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/rack_1_0.gemfile b/gemfiles/rack_1_0.gemfile index 7aa8c64518..b02c560d92 100644 --- a/gemfiles/rack_1_0.gemfile +++ b/gemfiles/rack_1_0.gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/rack_2_0.gemfile b/gemfiles/rack_2_0.gemfile index 69d7ec28f9..9c61fba047 100644 --- a/gemfiles/rack_2_0.gemfile +++ b/gemfiles/rack_2_0.gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/rack_3_0.gemfile b/gemfiles/rack_3_0.gemfile index 24ad9ac310..b11d7a6447 100644 --- a/gemfiles/rack_3_0.gemfile +++ b/gemfiles/rack_3_0.gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/rack_edge.gemfile b/gemfiles/rack_edge.gemfile index 0e1133d74c..37e9a1d4a7 100644 --- a/gemfiles/rack_edge.gemfile +++ b/gemfiles/rack_edge.gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/rails_6_0.gemfile b/gemfiles/rails_6_0.gemfile index 996b1210b6..af2f0a1a37 100644 --- a/gemfiles/rails_6_0.gemfile +++ b/gemfiles/rails_6_0.gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/rails_6_1.gemfile b/gemfiles/rails_6_1.gemfile index 3b8c16a3eb..d0a55806df 100644 --- a/gemfiles/rails_6_1.gemfile +++ b/gemfiles/rails_6_1.gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/rails_7_0.gemfile b/gemfiles/rails_7_0.gemfile index 914f94c8f9..8bca742d12 100644 --- a/gemfiles/rails_7_0.gemfile +++ b/gemfiles/rails_7_0.gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/rails_7_1.gemfile b/gemfiles/rails_7_1.gemfile index 4fc39fe8f5..fdea5d5e08 100644 --- a/gemfiles/rails_7_1.gemfile +++ b/gemfiles/rails_7_1.gemfile @@ -26,7 +26,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/gemfiles/rails_edge.gemfile b/gemfiles/rails_edge.gemfile index cb144118e7..b38d714221 100644 --- a/gemfiles/rails_edge.gemfile +++ b/gemfiles/rails_edge.gemfile @@ -25,7 +25,6 @@ group :development do end group :test do - gem 'cookiejar' gem 'grape-entity', '~> 0.6', require: false gem 'mime-types' gem 'rack-jsonp', require: 'rack/jsonp' diff --git a/lib/grape/cookies.rb b/lib/grape/cookies.rb index 51d02112e4..7afdb67c2b 100644 --- a/lib/grape/cookies.rb +++ b/lib/grape/cookies.rb @@ -33,9 +33,10 @@ def each(&block) @cookies.each(&block) end + # see https://github.com/rack/rack/blob/main/lib/rack/utils.rb#L338-L340 # rubocop:disable Layout/SpaceBeforeBrackets def delete(name, **opts) - options = opts.merge(value: 'deleted', expires: Time.at(0)) + options = opts.merge(max_age: '0', value: '', expires: Time.at(0)) self.[]=(name, options) end # rubocop:enable Layout/SpaceBeforeBrackets diff --git a/spec/grape/api_spec.rb b/spec/grape/api_spec.rb index 9ed9253587..8c9d876034 100644 --- a/spec/grape/api_spec.rb +++ b/spec/grape/api_spec.rb @@ -4,7 +4,6 @@ describe Grape::API do subject do - puts described_class Class.new(described_class) end diff --git a/spec/grape/endpoint_spec.rb b/spec/grape/endpoint_spec.rb index 5788bd9131..14d99c6277 100644 --- a/spec/grape/endpoint_spec.rb +++ b/spec/grape/endpoint_spec.rb @@ -175,37 +175,42 @@ def app get('/get/cookies') - expect(Array(last_response.headers['Set-Cookie']).flat_map { |h| h.split("\n") }.sort).to eql [ - 'cookie3=symbol', - 'cookie4=secret+code+here', - 'my-awesome-cookie1=is+cool', - 'my-awesome-cookie2=is+cool+too; domain=my.example.com; path=/; secure' - ] + expect(last_response.cookie_jar).to contain_exactly( + { 'name' => 'cookie3', 'value' => 'symbol' }, + { 'name' => 'cookie4', 'value' => 'secret code here' }, + { 'name' => 'my-awesome-cookie1', 'value' => 'is cool' }, + { 'name' => 'my-awesome-cookie2', 'value' => 'is cool too', 'domain' => 'my.example.com', 'path' => '/', 'secure' => true } + ) end it 'sets browser cookies and does not set response cookies' do + set_cookie %w[username=mrplum sandbox=true] subject.get('/username') do cookies[:username] end - get('/username', {}, 'HTTP_COOKIE' => 'username=mrplum; sandbox=true') + get '/username' expect(last_response.body).to eq('mrplum') - expect(last_response.headers['Set-Cookie']).to be_nil + expect(last_response.cookie_jar).to be_empty end it 'sets and update browser cookies' do + set_cookie %w[username=user sandbox=false] subject.get('/username') do cookies[:sandbox] = true if cookies[:sandbox] == 'false' cookies[:username] += '_test' end - get('/username', {}, 'HTTP_COOKIE' => 'username=user; sandbox=false') + + get '/username' expect(last_response.body).to eq('user_test') - cookies = Array(last_response.headers['Set-Cookie']).flat_map { |h| h.split("\n") } - expect(cookies[0]).to match(/username=user_test/) - expect(cookies[1]).to match(/sandbox=true/) + expect(last_response.cookie_jar).to contain_exactly( + { 'name' => 'sandbox', 'value' => 'true' }, + { 'name' => 'username', 'value' => 'user_test' } + ) end it 'deletes cookie' do + set_cookie %w[delete_this_cookie=1 and_this=2] subject.get('/test') do sum = 0 cookies.each do |name, val| @@ -214,22 +219,16 @@ def app end sum end - get '/test', {}, 'HTTP_COOKIE' => 'delete_this_cookie=1; and_this=2' + get '/test' expect(last_response.body).to eq('3') - cookies = Array(last_response.headers['Set-Cookie']).flat_map { |h| h.split("\n") }.to_h do |set_cookie| - cookie = CookieJar::Cookie.from_set_cookie 'http://localhost/test', set_cookie - [cookie.name, cookie] - end - expect(cookies.size).to eq(2) - %w[and_this delete_this_cookie].each do |cookie_name| - cookie = cookies[cookie_name] - expect(cookie).not_to be_nil - expect(cookie.value).to eq('deleted') - expect(cookie.expired?).to be true - end + expect(last_response.cookie_jar).to contain_exactly( + { 'name' => 'and_this', 'value' => '', 'max-age' => 0, 'expires' => Time.at(0) }, + { 'name' => 'delete_this_cookie', 'value' => '', 'max-age' => 0, 'expires' => Time.at(0) } + ) end it 'deletes cookies with path' do + set_cookie %w[delete_this_cookie=1 and_this=2] subject.get('/test') do sum = 0 cookies.each do |name, val| @@ -238,20 +237,12 @@ def app end sum end - get('/test', {}, 'HTTP_COOKIE' => 'delete_this_cookie=1; and_this=2') + get '/test' expect(last_response.body).to eq('3') - cookies = Array(last_response.headers['Set-Cookie']).flat_map { |h| h.split("\n") }.to_h do |set_cookie| - cookie = CookieJar::Cookie.from_set_cookie 'http://localhost/test', set_cookie - [cookie.name, cookie] - end - expect(cookies.size).to eq(2) - %w[and_this delete_this_cookie].each do |cookie_name| - cookie = cookies[cookie_name] - expect(cookie).not_to be_nil - expect(cookie.value).to eq('deleted') - expect(cookie.path).to eq('/test') - expect(cookie.expired?).to be true - end + expect(last_response.cookie_jar).to contain_exactly( + { 'name' => 'and_this', 'path' => '/test', 'value' => '', 'max-age' => 0, 'expires' => Time.at(0) }, + { 'name' => 'delete_this_cookie', 'path' => '/test', 'value' => '', 'max-age' => 0, 'expires' => Time.at(0) } + ) end end diff --git a/spec/integration/rack/v3/headers_spec.rb b/spec/integration/rack/v3/headers_spec.rb index 3be2c1e28b..1007cd4389 100644 --- a/spec/integration/rack/v3/headers_spec.rb +++ b/spec/integration/rack/v3/headers_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -describe Grape::Http::Headers do +describe Grape::Http::Headers, if: Gem::Version.new(Rack.release) >= Gem::Version.new('3') do it { expect(described_class::ALLOW).to eq('allow') } it { expect(described_class::LOCATION).to eq('location') } it { expect(described_class::TRANSFER_ENCODING).to eq('transfer-encoding') } diff --git a/spec/support/cookie_jar.rb b/spec/support/cookie_jar.rb new file mode 100644 index 0000000000..1e67a95a60 --- /dev/null +++ b/spec/support/cookie_jar.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'uri' + +module Rack + class MockResponse + def cookie_jar + @cookie_jar ||= Array(headers['Set-Cookie']).flat_map { |h| h.split("\n") }.map { |c| Cookie.new(c).to_h } + end + + # https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie + class Cookie + attr_reader :attributes + + def initialize(raw) + @attributes = raw.split(/;\s*/).flat_map.with_index do |attribute, i| + attribute, value = attribute.split('=', 2) + if i.zero? + [['name', attribute], ['value', unescape(value)]] + else + [[attribute.downcase, parse_value(attribute, value)]] + end + end.to_h.freeze + end + + def to_h + @attributes.dup + end + + def to_s + @attributes.to_s + end + + private + + def unescape(value) + URI.decode_www_form_component(value, Encoding::UTF_8) + end + + def parse_value(attribute, value) + case attribute + when 'expires' + Time.parse(value) + when 'max-age' + value.to_i + when 'secure', 'httponly', 'partitioned' + true + else + unescape(value) + end + end + end + end +end