Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Allow Rack::Static to assign HTTP Header based on Rules #422

Merged
merged 10 commits into from Nov 3, 2012

Conversation

Projects
None yet
7 participants
Contributor

thomasklemm commented Aug 31, 2012

Hey,

this PR allows Rack::Static to assign different headers to served files based on certain rules.

UPDATE: Update Docs to reflect the most current version

  # Set custom HTTP Headers for based on rules:
  #
  #     use Rack::Static, :root => 'public',
  #         :header_rules => [
  #           [rule, {header_field => content, header_field => content}],
  #           [rule, {header_field => content}]
  #         ]
  #
  #  Rules for selecting files:
  #
  #  1) All files
  #     Provide the :all symbol
  #     :all => Matches every file
  #
  #  2) Folders
  #     Provide the folder path as a string
  #     '/folder' or '/folder/subfolder' => Matches files in a certain folder
  #
  #  3) File Extensions
  #     Provide the file extensions as an array
  #     ['css', 'js'] or %w(css js) => Matches files ending in .css or .js
  #
  #  4) Regular Expressions / Regexp
  #     Provide a regular expression
  #     %r{\.(?:css|js)\z} => Matches files ending in .css or .js
  #     /\.(?:eot|ttf|otf|woff|svg)\z/ => Matches files ending in
  #       the most common web font formats (.eot, .ttf, .otf, .woff, .svg)
  #       Note: This Regexp is available as a shortcut, using the :fonts rule
  #
  #  5) Font Shortcut
  #     Provide the :fonts symbol
  #     :fonts => Uses the Regexp rule stated right above to match all common web font endings
  #
  #  Rule Ordering:
  #    Rules are applied in the order that they are provided.
  #    List rather general rules above special ones.
  #
  #  Complete example use case including HTTP header rules:
  #
  #     use Rack::Static, :root => 'public',
  #         :header_rules => [
  #           # Cache all static files in public caches (e.g. Rack::Cache)
  #           #  as well as in the browser
  #           [:all, {'Cache-Control' => 'public, max-age=31536000'}],
  #
  #           # Provide web fonts with cross-origin access-control-headers
  #           #  Firefox requires this when serving assets using a Content Delivery Network
  #           [:fonts, {'Access-Control-Allow-Origin' => '*'}]
  #         ]
  #

UPDATE END

Note: I removed the :headers option in favor of :header_rules => {:global => {}}.
It has been added through a PR of mine a few weeks ago that did not make it into a release yet.

Thanks for reviewing & your opinions. I think there might be some demand for this, it can be especially useful to add certain access control headers that firefox requires web fonts to be served with when served via a CDN.

Thomas

This pull request fails (merged 308e57e into d084a14).

Contributor

thomasklemm commented Aug 31, 2012

Hm... Tests pass in MRI Ruby 1.9.3, 1.9.2 and jruby, but fail in 1.8.7, rubinius and ree. There seem to be some implementation differences either in the case statement or in the String.start_with?(...) method.

Contributor

thomasklemm commented Aug 31, 2012

I did some tests in MRI Ruby 1.8.7, rewriting the case statement to use if / elsif clauses, which seems to work. However, the String.start_with? still causes the tests to fail, but only sometimes! I don't see why this happens, it seems to pass or fail rather randomly.

Contributor

gioele commented Aug 31, 2012

Rack::Static is getting quite a bit chubby these days. Wouldn't it be better to keep this middleware component small and clean (it is distributed and available in every Rack installation, any addition to it increases the attack surface of Rack-based systems).

What about focusing on another more advanced middleware component with its own gem like a Rack::StaticAdvanced or something similar?

This pull request fails (merged a446874 into d084a14).

Contributor

thomasklemm commented Aug 31, 2012

New HTTP Header Tests still fail about half the time of 1.8.7 (I did not look further for rbx and ree); fails one or two tests from the header tests randomly. I don't know why.

Could this be some kind of thread-safety issue that 1.8.7 handles worse than 1.9.x?

Contributor

hannesg commented Aug 31, 2012

No, but in 1.8.7 ( and therefore ree and some versions of rbx ) hashes are not ordered. Simply said: the rules are applied in different sequence every run. Maybe try specifying the rules as an array of arrays or use distinct rules?

This pull request fails (merged 4b0309b into d084a14).

Contributor

thomasklemm commented Aug 31, 2012

@hannesg Thanks, this was the reason. I implemented the option of providing header rules as an array of arrays for Ruby 1.8.7, rbx and ree. I'll look for a way to exclude the tests for providing a hash on these platforms.

Any feedback on how it's done or the structuring of the rules?

Contributor

thomasklemm commented Aug 31, 2012

Current Travis Output:

Bacon::Error: 0.104006.>(0.104006) failed
120 ./test/spec_runtime.rb:47: Rack::Runtime - should allow multiple timers to be set
121 ./test/spec_runtime.rb:32
122 ./test/spec_runtime.rb:5
123
124586 tests, 1769 assertions, 1 failures, 0 errors

I'd consider this unrelated to this PR?

This pull request passes (merged 85bc7bf into d084a14).

Contributor

thomasklemm commented Sep 3, 2012

It's working on all platforms now. What do you think?

Contributor

thomasklemm commented Sep 15, 2012

I refactored the implementation of the array of arrays option for ruby versions & engines whose hashes are unordered to make it nicer to work with. It also adds less baggage than before, though of course it is not as clean as it could be if only a hash were supported.

Contributor

thomasklemm commented Sep 15, 2012

Actually, providing HTTP header rules as an array of arrays gives a nicer and potentially also less error-prone syntax. It is the only option now.

See the updated docs at the top for a quick glance at the whole feature.

@raggi raggi and 1 other commented on an outdated diff Nov 2, 2012

@@ -8,3 +8,4 @@ stage
Gemfile.lock
.rbx
doc
+.rbenv-version
@raggi

raggi Nov 2, 2012

Owner

This is not relevant to this commit.

Please consider using your own global ignores for such things.

@thomasklemm

thomasklemm Nov 2, 2012

Contributor

Done. Thanks for the hint!

@raggi raggi and 1 other commented on an outdated diff Nov 2, 2012

lib/rack/static.rb
+ set_header(headers) if @path.match(/\.(?:ttf|otf|eot|woff|svg)\z/)
+ when String # Folder
+ path = ::Rack::Utils.unescape(@path)
+ set_header(headers) if (path.start_with?(rule) || path.start_with?('/' + rule))
+ when Array # Extension/Extensions
+ extensions = rule.join('|')
+ set_header(headers) if @path.match(/\.(#{extensions})\z/)
+ when Regexp # Flexible Regexp
+ set_header(headers) if @path.match(rule)
+ else
+ end
+ end
+
+ def set_header(headers)
+ headers.each { |field, content| @headers[field] = content }
+ end
@raggi

raggi Nov 2, 2012

Owner

The meaning of set_header and set_headers is very confusing to readers. This feels wrong.

Maybe rename set_headers to apply_header_rules, and then set_header to set_headers

@thomasklemm

thomasklemm Nov 2, 2012

Contributor

Agreed and done.

@raggi raggi and 1 other commented on an outdated diff Nov 2, 2012

lib/rack/static.rb
+ # Convert HTTP header rules to HTTP headers
+ def set_headers
+ @header_rules.each do |rule, headers|
+ apply_rule(rule, headers)
+ end
+ end
+
+ def apply_rule(rule, headers)
+ case rule
+ when :all # All files
+ set_header(headers)
+ when :fonts # Fonts Shortcut
+ set_header(headers) if @path.match(/\.(?:ttf|otf|eot|woff|svg)\z/)
+ when String # Folder
+ path = ::Rack::Utils.unescape(@path)
+ set_header(headers) if (path.start_with?(rule) || path.start_with?('/' + rule))
@raggi

raggi Nov 2, 2012

Owner

If I remember correctly, start_with? was introduced in 1.8.7.

@thomasklemm

thomasklemm Nov 2, 2012

Contributor

You are right, it was introduced in 1.8.7. Does rack still support 1.8.6?

@raggi raggi and 1 other commented on an outdated diff Nov 2, 2012

lib/rack/static.rb
@file_server.call(env)
else
@app.call(env)
end
end
+ # Convert HTTP header rules to HTTP headers
+ def set_headers
+ @header_rules.each do |rule, headers|
+ apply_rule(rule, headers)
+ end
+ end
+
+ def apply_rule(rule, headers)
+ case rule
+ when :all # All files
+ set_header(headers)
+ when :fonts # Fonts Shortcut
+ set_header(headers) if @path.match(/\.(?:ttf|otf|eot|woff|svg)\z/)
@raggi

raggi Nov 2, 2012

Owner

It may make more sense to use Mime for this.

@thomasklemm

thomasklemm Nov 2, 2012

Contributor

Mime would only allow to work with extensions, not folders, would it?

Owner

raggi commented Nov 2, 2012

I do agree with the others that we're in danger of Static becoming very oversized.

Are you actually using something like this in production? Why not use a good file server?

Contributor

thomasklemm commented Nov 2, 2012

The reason behind this PR is that on Heroku you cannot use Nginx et al. for serving your static files, but instead rely on Rails' ActionDispatch::Static or Rack::Static. As both of these implementations base on Rack::File, I submitted a PR a while ago which was merged enabling to serve custom HTTP headers. The current Rack::Static implementation allows to set these custom headers on all files, while this one aims to allow developers to set HTTP headers for individual file extensions or folders based on simple rules.

Currently I am using a quite similar custom Rack middleware to set appropriate headers for Webfonts. From my point of view it would be much cleaner however to have support for this customary task directly in the respective fileserver in use.

tl;dr This PR's functionality is relevant to anyone who is trying to use Rack::Static as their fileserver in a production environment. The main reason behind using Rack::Static is that you run your app in an environment where it's not possible to set an nginx instance in front (consider Heroku for instance). As soon as your files require custom HTTP headers you quickly hit the current limitations.

// cc Richard Schneeman at Heroku (@schneems) (Associated Heroku support ticket)

Contributor

gioele commented Nov 2, 2012

Given this problem, and the fact that there are many other people using Heroku facing the same issue, wouldn't a new dedicated middleware component for serving static files with advanced configurations be a better choice?

Owner

raggi commented Nov 2, 2012

Files in public are served by a standard (fast) file server on Heroku, and I would strongly recommend that you use that instead, as it'll save you a lot of dyno cpu time.

Contributor

thomasklemm commented Nov 3, 2012

That could have been the case with the Bamboo stack. Their current production stack Cedar however has no such option. They even recommend to use Rack::Static in various docs, for example on creating static sites with Ruby. For Rails Heroku injects a plugin enabling ActionDispatch::Static to serve static files by default as described in their buildpack. So, in a Ruby environment on Heroku either Rack::Static or ActionDispatch:.Static are granted the honors to deliver static files.

// cc @heroku

Contributor

thomasklemm commented Nov 3, 2012

IMO a great setup for handling static files in Rails on Heroku at the moment is:

  1. Decide to use either ActionDispatch::Static or Rack::Static.
  2. Reorder your Rack middleware for this static file server to be the first in line (before Rack::Cache).
  3. Set up Amazon Cloudfront as your CDN. Files will then only be served once (or a few times) per deploy from Heroku, and any further request will be served from Cloudfront's cache. As Cloudfront serves files with the HTTP headers they carry when requested from Heroku first, any headers required must be set beforehand when serving the file from your Heroku dyno.

For non-Rails environments Rack::Static basically is the go-to option. Custom HTTP headers for individual files could therefore be set in Rack::Static or by a (more or less hacky) Rack middleware further down the stack.

@raggi raggi added a commit that referenced this pull request Nov 3, 2012

@raggi raggi Merge pull request #422 from thomasklemm/master
Allow Rack::Static to assign HTTP Header based on Rules
7f847c4

@raggi raggi merged commit 7f847c4 into rack:master Nov 3, 2012

1 check passed

default The Travis build passed
Details
Owner

raggi commented Nov 3, 2012

Thanks for all your hard work on this, making the review changes and being responsive!

Contributor

schneems commented Nov 3, 2012

❤️ thanks for all the work to both of you.

Contributor

thomasklemm commented Nov 3, 2012

Cool, thanks a lot for the feedback and help!

I'd love to be able to use this functionality with ActionDispatch::Static. Is there any reason this needs to be coupled with Rack::Static?

For example, I'd like to be able to do something like this with ActionDispatch::Static:

# Rack::Headers is a new hypothetical middleware component
config.middleware.use Rack::Headers, :header_rules => [
    # Cache assets with checksums aggressively 
    [%r(^/assets/\w+-\w{32}\.), {'Cache-Control' => "public, max-age=#{1.year.to_i}"}],
    # Cache other assets more moderately
    [%r(^/assets/),  {'Cache-Control' => "public, max-age=#{1.week.to_i}"})]
    ]

Most use cases of Rack::Static aren't going to need a flexible rules engine. It seems cleaner to have this functionality tacked on as its own middleware only when its needed.

What do you guys think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment