properly implement If-Matches and If-None-Matches handling according to RFC 2616, fixes #355

This commit is contained in:
Konstantin Haase 2011-09-17 14:27:26 -07:00
parent 3ac3a29e81
commit 48e167317e
4 changed files with 647 additions and 54 deletions

View File

@ -1018,6 +1018,24 @@ try {rack-cache}[http://rtomayko.github.com/rack-cache/]:
Use the <tt>:static_cache_control</tt> setting (see below) to add
<tt>Cache-Control</tt> 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 <tt>*</tt> 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 <tt>:new_resource</tt>
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 <tt>:kind</tt> option:
etag '', :new_resource => true, :kind => :weak
=== Sending Files
For sending files, you can use the <tt>send_file</tt> helper method:

View File

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

View File

@ -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, "")

View File

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