Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP
Browse files

HEAD requests on static files should have proper Content-Length

Code originally intending to fix byte encodings was overwriting the
proper Content-Length headers set by send_file

also contains 2 trailing whitespace fixes
  • Loading branch information...
commit 00b63ec5a5497aa2fa9777a3e899134718a13fd6 1 parent 7614d3b
@jormon jormon authored
Showing with 6 additions and 3 deletions.
  1. +5 −3 lib/sinatra/base.rb
  2. +1 −0  test/static_test.rb
View
8 lib/sinatra/base.rb
@@ -77,7 +77,9 @@ def finish
headers.delete "Content-Length"
headers.delete "Content-Type"
elsif Array === body and not [204, 304].include?(status.to_i)
- headers["Content-Length"] = body.inject(0) { |l, p| l + Rack::Utils.bytesize(p) }.to_s
+ # if some other code has already set Content-Length, don't muck with it
+ # currently, this would be the static file-handler
+ headers["Content-Length"] ||= body.inject(0) { |l, p| l + Rack::Utils.bytesize(p) }.to_s
@DAddYE
DAddYE added a note

mmm I've a lot of problems after this commit, for reference see this: padrino/padrino-framework#931

@rkh Owner
rkh added a note

Hmmm. Interesting. I am not sure how to best solve this. What we should do is reset the Content-Length header on body updates, but I was under the impression that Rack::Response is already doing this.

@rkh Owner
rkh added a note

Alternative would be to check for HEAD requests.

@jormon
jormon added a note

Apologies for the breakage downstream. I only wrote this bugfix to address the extra test-case below, which is an important part of the HTTP spec. I'm not super familiar with the sinatra codebase, this was my first and only contribution. But if you can write a failing test, I'll fix the bug!

@DAddYE
DAddYE added a note

@jormon / @rkh cannot make sense add 404 to this: elsif Array === body and not [204, 304].include?(status.to_i) ?

@rkh Owner
rkh added a note

Adding 404 would be wrong. These are status codes with no body, whereas 404 has a body. If you could write a breaking test, that would make things a ton easier. Also: #608.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
end
# Rack::Response#finish sometimes returns self as response body. We don't want that.
@@ -986,7 +988,7 @@ def handle_exception!(boom)
def error_block!(key, *block_params)
base = settings
while base.respond_to?(:errors)
- next base = base.superclass unless args_array = base.errors[key]
+ next base = base.superclass unless args_array = base.errors[key]
args_array.reverse_each do |args|
first = args == args_array.first
args += [block_params]
@@ -1092,7 +1094,7 @@ def disable(*opts)
# Define a custom error handler. Optionally takes either an Exception
# class, or an HTTP status code to specify which errors should be
# handled.
- def error(*codes, &block)
+ def error(*codes, &block)
args = compile! "ERROR", //, block
codes = codes.map { |c| Array(c) }.flatten
codes << Exception if codes.empty?
View
1  test/static_test.rb
@@ -37,6 +37,7 @@ class StaticTest < Test::Unit::TestCase
assert ok?
assert_equal '', body
assert response.headers.include?('Last-Modified')
+ assert_equal File.size(__FILE__).to_s, response['Content-Length']
end
%w[POST PUT DELETE].each do |verb|
@DAddYE

mmm I've a lot of problems after this commit, for reference see this: padrino/padrino-framework#931

@rkh

Hmmm. Interesting. I am not sure how to best solve this. What we should do is reset the Content-Length header on body updates, but I was under the impression that Rack::Response is already doing this.

@rkh

Alternative would be to check for HEAD requests.

@jormon

Apologies for the breakage downstream. I only wrote this bugfix to address the extra test-case below, which is an important part of the HTTP spec. I'm not super familiar with the sinatra codebase, this was my first and only contribution. But if you can write a failing test, I'll fix the bug!

@DAddYE

@jormon / @rkh cannot make sense add 404 to this: elsif Array === body and not [204, 304].include?(status.to_i) ?

@rkh

Adding 404 would be wrong. These are status codes with no body, whereas 404 has a body. If you could write a breaking test, that would make things a ton easier. Also: #608.

Please sign in to comment.
Something went wrong with that request. Please try again.