From 48e167317ea9d15c957af8e0e600cc837e46c0a4 Mon Sep 17 00:00:00 2001 From: Konstantin Haase Date: Sat, 17 Sep 2011 14:27:26 -0700 Subject: [PATCH] properly implement If-Matches and If-None-Matches handling according to RFC 2616, fixes #355 --- README.rdoc | 18 ++ lib/sinatra/base.rb | 38 ++- test/helper.rb | 4 + test/helpers_test.rb | 641 ++++++++++++++++++++++++++++++++++++++++--- 4 files changed, 647 insertions(+), 54 deletions(-) diff --git a/README.rdoc b/README.rdoc index bbbfe927..a0c5317c 100644 --- a/README.rdoc +++ b/README.rdoc @@ -1018,6 +1018,24 @@ try {rack-cache}[http://rtomayko.github.com/rack-cache/]: Use the :static_cache_control setting (see below) to add Cache-Control header info to static files. +According to RFC 2616 your application should behave differently if the +If-Matches of If-None-Matches header is set to * depending on whether +the resource requested is already in existence. Sinatra assumes resources for +safe (like get) and idempotent (like put) requests are already in existence, +whereas other resources (for instance for post requests), are treated as new +resources. You can change this behavior by passing in a :new_resource +option: + + get '/create' do + etag '', :new_resource => true + Article.create + erb :new_article + end + +If you still want to use a weak ETag, pass in a :kind option: + + etag '', :new_resource => true, :kind => :weak + === Sending Files For sending files, you can use the send_file helper method: diff --git a/lib/sinatra/base.rb b/lib/sinatra/base.rb index df51d3d3..cf5a432b 100644 --- a/lib/sinatra/base.rb +++ b/lib/sinatra/base.rb @@ -355,8 +355,12 @@ module Sinatra return unless time time = time_for time response['Last-Modified'] = time.httpdate - # compare based on seconds since epoch - halt 304 if Time.httpdate(env['HTTP_IF_MODIFIED_SINCE']).to_i >= time.to_i + + unless env['HTTP_IF_NONE_MATCH'] + # compare based on seconds since epoch + since = Time.httpdate(env['HTTP_IF_MODIFIED_SINCE']).to_i + halt 304 if since >= time.to_i + end rescue ArgumentError end @@ -369,22 +373,36 @@ module Sinatra # When the current request includes an 'If-None-Match' header with a # matching etag, execution is immediately halted. If the request method is # GET or HEAD, a '304 Not Modified' response is sent. - def etag(value, kind = :strong) - raise ArgumentError, ":strong or :weak expected" unless [:strong,:weak].include?(kind) + def etag(value, options = {}) + # Before touching this code, please double check RFC 2616 14.24 and 14.26. + options = {:kind => options} unless Hash === options + kind = options[:kind] || :strong + new_resource = options.fetch(:new_resource) { request.post? } + + unless [:strong, :weak].include?(kind) + raise ArgumentError, ":strong or :weak expected" + end + value = '"%s"' % value value = 'W/' + value if kind == :weak response['ETag'] = value - if etags = env['HTTP_IF_NONE_MATCH'] - etags = etags.split(/\s*,\s*/) - if etags.include?(value) or etags.include?('*') - halt 304 if request.safe? - else - halt 412 unless request.safe? + if success? or status == 304 + if etag_matches? env['HTTP_IF_NONE_MATCH'], new_resource + halt(request.safe? ? 304 : 412) + end + + if env['HTTP_IF_MATCH'] + halt 412 unless etag_matches? env['HTTP_IF_MATCH'], new_resource end end end + def etag_matches?(list, new_resource = request.post?) + return !new_resource if list == '*' + list.to_s.split(/\s*,\s*/).include? response['ETag'] + end + # Sugar for redirect (example: redirect back) def back request.referer diff --git a/test/helper.rb b/test/helper.rb index 972b416b..1f061d49 100644 --- a/test/helper.rb +++ b/test/helper.rb @@ -72,6 +72,10 @@ class Test::Unit::TestCase assert_equal value.lstrip.gsub(/\s*\n\s*/, ""), body.lstrip.gsub(/\s*\n\s*/, "") end + def assert_status(expected) + assert_equal Integer(expected), Integer(status) + end + def assert_like(a,b) pattern = /id=['"][^"']*["']|\s+/ assert_equal a.strip.gsub(pattern, ""), b.strip.gsub(pattern, "") diff --git a/test/helpers_test.rb b/test/helpers_test.rb index ca6d1c82..46055dda 100644 --- a/test/helpers_test.rb +++ b/test/helpers_test.rb @@ -960,69 +960,622 @@ class HelpersTest < Test::Unit::TestCase end describe 'etag' do - setup do - mock_app { - get '/' do - body { 'Hello World' } - etag 'FOO' - 'Boo!' + context "safe requests" do + it 'returns 200 for normal requests' do + mock_app do + get '/' do + etag 'foo' + 'ok' + end end - post '/' do - etag 'FOO' - 'Matches!' + get('/') + assert_status 200 + assert_body 'ok' + end + + context "If-None-Match" do + it 'returns 304 when If-None-Match is *' do + mock_app do + get '/' do + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_NONE_MATCH' => '*') + assert_status 304 + assert_body '' end - } + + it 'returns 200 when If-None-Match is * for new resources' do + mock_app do + get '/' do + etag 'foo', :new_resource => true + 'ok' + end + end + + get('/', {}, 'HTTP_IF_NONE_MATCH' => '*') + assert_status 200 + assert_body 'ok' + end + + it 'returns 304 when If-None-Match is * for existing resources' do + mock_app do + get '/' do + etag 'foo', :new_resource => false + 'ok' + end + end + + get('/', {}, 'HTTP_IF_NONE_MATCH' => '*') + assert_status 304 + assert_body '' + end + + it 'returns 304 when If-None-Match is the etag' do + mock_app do + get '/' do + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"') + assert_status 304 + assert_body '' + end + + it 'returns 304 when If-None-Match includes the etag' do + mock_app do + get '/' do + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar", "foo"') + assert_status 304 + assert_body '' + end + + it 'returns 200 when If-None-Match does not include the etag' do + mock_app do + get '/' do + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"') + assert_status 200 + assert_body 'ok' + end + + it 'ignores If-Modified-Since if If-None-Match does not match' do + mock_app do + get '/' do + etag 'foo' + last_modified Time.at(0) + 'ok' + end + end + + get('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"') + assert_status 200 + assert_body 'ok' + end + + it 'does not change a status code other than 2xx or 304' do + mock_app do + get '/' do + status 499 + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"') + assert_status 499 + assert_body 'ok' + end + + it 'does change 2xx status codes' do + mock_app do + get '/' do + status 299 + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"') + assert_status 304 + assert_body '' + end + + it 'does not send a body on 304 status codes' do + mock_app do + get '/' do + status 304 + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"') + assert_status 304 + assert_body '' + end + end + + context "If-Match" do + it 'returns 200 when If-Match is the etag' do + mock_app do + get '/' do + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_MATCH' => '"foo"') + assert_status 200 + assert_body 'ok' + end + + it 'returns 200 when If-Match includes the etag' do + mock_app do + get '/' do + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_MATCH' => '"foo", "bar"') + assert_status 200 + assert_body 'ok' + end + + it 'returns 200 when If-Match is *' do + mock_app do + get '/' do + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_MATCH' => '*') + assert_status 200 + assert_body 'ok' + end + + it 'returns 412 when If-Match is * for new resources' do + mock_app do + get '/' do + etag 'foo', :new_resource => true + 'ok' + end + end + + get('/', {}, 'HTTP_IF_MATCH' => '*') + assert_status 412 + assert_body '' + end + + it 'returns 200 when If-Match is * for existing resources' do + mock_app do + get '/' do + etag 'foo', :new_resource => false + 'ok' + end + end + + get('/', {}, 'HTTP_IF_MATCH' => '*') + assert_status 200 + assert_body 'ok' + end + + it 'returns 412 when If-Match does not include the etag' do + mock_app do + get '/' do + etag 'foo' + 'ok' + end + end + + get('/', {}, 'HTTP_IF_MATCH' => '"bar"') + assert_status 412 + assert_body '' + end + end end - it 'sets the ETag header' do - get '/' - assert_equal '"FOO"', response['ETag'] + context "idempotent requests" do + it 'returns 200 for normal requests' do + mock_app do + put '/' do + etag 'foo' + 'ok' + end + end + + put('/') + assert_status 200 + assert_body 'ok' + end + + context "If-None-Match" do + it 'returns 412 when If-None-Match is *' do + mock_app do + put '/' do + etag 'foo' + 'ok' + end + end + + put('/', {}, 'HTTP_IF_NONE_MATCH' => '*') + assert_status 412 + assert_body '' + end + + it 'returns 200 when If-None-Match is * for new resources' do + mock_app do + put '/' do + etag 'foo', :new_resource => true + 'ok' + end + end + + put('/', {}, 'HTTP_IF_NONE_MATCH' => '*') + assert_status 200 + assert_body 'ok' + end + + it 'returns 412 when If-None-Match is * for existing resources' do + mock_app do + put '/' do + etag 'foo', :new_resource => false + 'ok' + end + end + + put('/', {}, 'HTTP_IF_NONE_MATCH' => '*') + assert_status 412 + assert_body '' + end + + it 'returns 412 when If-None-Match is the etag' do + mock_app do + put '/' do + etag 'foo' + 'ok' + end + end + + put('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"') + assert_status 412 + assert_body '' + end + + it 'returns 412 when If-None-Match includes the etag' do + mock_app do + put '/' do + etag 'foo' + 'ok' + end + end + + put('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar", "foo"') + assert_status 412 + assert_body '' + end + + it 'returns 200 when If-None-Match does not include the etag' do + mock_app do + put '/' do + etag 'foo' + 'ok' + end + end + + put('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"') + assert_status 200 + assert_body 'ok' + end + + it 'ignores If-Modified-Since if If-None-Match does not match' do + mock_app do + put '/' do + etag 'foo' + last_modified Time.at(0) + 'ok' + end + end + + put('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"') + assert_status 200 + assert_body 'ok' + end + end + + context "If-Match" do + it 'returns 200 when If-Match is the etag' do + mock_app do + put '/' do + etag 'foo' + 'ok' + end + end + + put('/', {}, 'HTTP_IF_MATCH' => '"foo"') + assert_status 200 + assert_body 'ok' + end + + it 'returns 200 when If-Match includes the etag' do + mock_app do + put '/' do + etag 'foo' + 'ok' + end + end + + put('/', {}, 'HTTP_IF_MATCH' => '"foo", "bar"') + assert_status 200 + assert_body 'ok' + end + + it 'returns 200 when If-Match is *' do + mock_app do + put '/' do + etag 'foo' + 'ok' + end + end + + put('/', {}, 'HTTP_IF_MATCH' => '*') + assert_status 200 + assert_body 'ok' + end + + it 'returns 412 when If-Match is * for new resources' do + mock_app do + put '/' do + etag 'foo', :new_resource => true + 'ok' + end + end + + put('/', {}, 'HTTP_IF_MATCH' => '*') + assert_status 412 + assert_body '' + end + + it 'returns 200 when If-Match is * for existing resources' do + mock_app do + put '/' do + etag 'foo', :new_resource => false + 'ok' + end + end + + put('/', {}, 'HTTP_IF_MATCH' => '*') + assert_status 200 + assert_body 'ok' + end + + it 'returns 412 when If-Match does not include the etag' do + mock_app do + put '/' do + etag 'foo' + 'ok' + end + end + + put('/', {}, 'HTTP_IF_MATCH' => '"bar"') + assert_status 412 + assert_body '' + end + end end - it 'returns a body when conditional get misses' do - get '/' - assert_equal 200, status - assert_equal 'Boo!', body - end + context "post requests" do + it 'returns 200 for normal requests' do + mock_app do + post '/' do + etag 'foo' + 'ok' + end + end - it 'returns a body when posting with no If-None-Match header' do - post '/' - assert_equal 200, status - assert_equal 'Matches!', body - end + post('/') + assert_status 200 + assert_body 'ok' + end - it 'returns a body when conditional post matches' do - post '/', {}, { 'HTTP_IF_NONE_MATCH' => '"FOO"' } - assert_equal 200, status - assert_equal 'Matches!', body - end + context "If-None-Match" do + it 'returns 200 when If-None-Match is *' do + mock_app do + post '/' do + etag 'foo' + 'ok' + end + end - it 'halts with 412 when conditional post misses' do - post '/', {}, { 'HTTP_IF_NONE_MATCH' => '"BAR"' } - assert_equal 412, status - assert_equal '', body - end + post('/', {}, 'HTTP_IF_NONE_MATCH' => '*') + assert_status 200 + assert_body 'ok' + end - it 'halts when a conditional GET matches' do - get '/', {}, { 'HTTP_IF_NONE_MATCH' => '"FOO"' } - assert_equal 304, status - assert_equal '', body - end + it 'returns 200 when If-None-Match is * for new resources' do + mock_app do + post '/' do + etag 'foo', :new_resource => true + 'ok' + end + end - it 'should handle multiple ETag values in If-None-Match header' do - get '/', {}, { 'HTTP_IF_NONE_MATCH' => '"BAR", *' } - assert_equal 304, status - assert_equal '', body + post('/', {}, 'HTTP_IF_NONE_MATCH' => '*') + assert_status 200 + assert_body 'ok' + end + + it 'returns 412 when If-None-Match is * for existing resources' do + mock_app do + post '/' do + etag 'foo', :new_resource => false + 'ok' + end + end + + post('/', {}, 'HTTP_IF_NONE_MATCH' => '*') + assert_status 412 + assert_body '' + end + + it 'returns 412 when If-None-Match is the etag' do + mock_app do + post '/' do + etag 'foo' + 'ok' + end + end + + post('/', {}, 'HTTP_IF_NONE_MATCH' => '"foo"') + assert_status 412 + assert_body '' + end + + it 'returns 412 when If-None-Match includes the etag' do + mock_app do + post '/' do + etag 'foo' + 'ok' + end + end + + post('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar", "foo"') + assert_status 412 + assert_body '' + end + + it 'returns 200 when If-None-Match does not include the etag' do + mock_app do + post '/' do + etag 'foo' + 'ok' + end + end + + post('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"') + assert_status 200 + assert_body 'ok' + end + + it 'ignores If-Modified-Since if If-None-Match does not match' do + mock_app do + post '/' do + etag 'foo' + last_modified Time.at(0) + 'ok' + end + end + + post('/', {}, 'HTTP_IF_NONE_MATCH' => '"bar"') + assert_status 200 + assert_body 'ok' + end + end + + context "If-Match" do + it 'returns 200 when If-Match is the etag' do + mock_app do + post '/' do + etag 'foo' + 'ok' + end + end + + post('/', {}, 'HTTP_IF_MATCH' => '"foo"') + assert_status 200 + assert_body 'ok' + end + + it 'returns 200 when If-Match includes the etag' do + mock_app do + post '/' do + etag 'foo' + 'ok' + end + end + + post('/', {}, 'HTTP_IF_MATCH' => '"foo", "bar"') + assert_status 200 + assert_body 'ok' + end + + it 'returns 412 when If-Match is *' do + mock_app do + post '/' do + etag 'foo' + 'ok' + end + end + + post('/', {}, 'HTTP_IF_MATCH' => '*') + assert_status 412 + assert_body '' + end + + it 'returns 412 when If-Match is * for new resources' do + mock_app do + post '/' do + etag 'foo', :new_resource => true + 'ok' + end + end + + post('/', {}, 'HTTP_IF_MATCH' => '*') + assert_status 412 + assert_body '' + end + + it 'returns 200 when If-Match is * for existing resources' do + mock_app do + post '/' do + etag 'foo', :new_resource => false + 'ok' + end + end + + post('/', {}, 'HTTP_IF_MATCH' => '*') + assert_status 200 + assert_body 'ok' + end + + it 'returns 412 when If-Match does not include the etag' do + mock_app do + post '/' do + etag 'foo' + 'ok' + end + end + + post('/', {}, 'HTTP_IF_MATCH' => '"bar"') + assert_status 412 + assert_body '' + end + end end it 'uses a weak etag with the :weak option' do - mock_app { + mock_app do get '/' do etag 'FOO', :weak "that's weak, dude." end - } + end get '/' assert_equal 'W/"FOO"', response['ETag'] end