github / secure_headers

Manages application of security headers with many safe defaults
MIT License
3.16k stars 252 forks source link

Guide for transitioning from secure_headers to vanilla rails csp #466

Open oreoshake opened 3 years ago

oreoshake commented 3 years ago

This is more of a meta issue where I'm going to drop notes before actually coming up with a documented plan.

We intend to go through this process in the not so distant future and will have lessons to learn. GitHub uses the following features:

https://bauland42.com/ruby-on-rails-content-security-policy-csp/ does a good job of explaining both APIs. https://medium.com/just-tech/content-security-policy-ruby-on-rails-836d0c7ba098j is a story of someone doing exactly this (and calling out one important difference.

base config

Creating your base config from the secure_headers config is mostly a find/replace. Instead of a hash, you're configuring an object with methods named after directives. They take varargs so the arrays need to be splatted.


# TODO: provide find/replace regex for automating this

# secure_headers
SecureHeaders::Configuration.default do |config|
  config.csp = {
    directive_name: some_array
  }
end

# rails
Rails.application.config.content_security_policy do |config|
  config.directive_name *some_array
end

dynamic modifications

As mentioned in https://bauland42.com/ruby-on-rails-content-security-policy-csp/, the rails API overwrites config values. In secure_headers land, we generally append sources to an existing list rather than have to redefine the entire list upon use.

def some_controller_action
  the_value = request.content_security_policy.frame_src
  is_now_nil =request.content_security_policy.frame_src 
end

def some_controller_action_overwriting
  # whatever was in frame_src was lost/overwritten and this only contains the new_sources
  request.content_security_policy.frame_src new_sources
end

def some_controller_action_appending
  current_config = request.content_security_policy.frame_src
  request.content_security_policy.frame_src *(current_config + new_sources)
end

So the obvious workaround is to grab the current config and then re-set the value along with additions. This shim seems to generally solve the problem while backporting some functionality, notably:

  # this is just one option. We don't use the railtie so we're calling `SecureHeaders.append_content_security_policy_directives(request, additions)`. A simple multiline regex can convert these to `append_content_security_policy_directives(additions)`. I intend to write a rubocop for this so maybe I'll get the autocorrect working.

  def append_content_security_policy_directives(additions)
      # TODO: add allowlist for keys in the additions hash (re-use some constant)

      if GitHub.flipper[:vanilla_csp].enabled?(current_user)
        # current_content_security_policy gives us a safe, duplicate copy to use.
        current_policy = current_content_security_policy
        additions.each do |directive, source_values|

          # TODO handle other unsupported values
          next if directive == :preserve_schemes
          if directive == :report_uri
            request.content_security_policy.report_uri source_values
          else
            current_value = current_policy.send(directive)
            if config.nil?
              config = []
              config = current_policy.default_src 
            end

            if current_value == %w('none')
              request.content_security_policy.send(directive, *source_values.compact.uniq)
            else
              request.content_security_policy.send(directive, *(current_value + source_values).compact.uniq)
            end
          end
        end
      else
        SecureHeaders.append_content_security_policy_directives(request, additions)
      end
    end

    def some_before_filter_on_every_request
      if GitHub.flipper[:vanilla_csp].enabled?(current_user)
        # we don't need to disable built-in rails because it exits if a policy is set
        # depending on how middleware is loaded, we could end up generating a policy that is overwritten by secure_headers
        SecureHeaders.opt_out_of_header(request, :csp)
      end
    end

testing

By default, rails does not include middleware when running tests. If you're testing your CSP (hint: you should) then you've already done this in your test class:

  def app
    @app ||= Rack::Builder.new do
      use SecureHeaders::Middleware
      use ActionDispatch::ContentSecurityPolicy::Middleware # this is the new addition
      run GitHub::Application
    end
  end
end

differing policies

If you're testing your policies (seriously: do it) then you won't get exact matches without some extra steps.

Rails won't dedup anything so you'll be calling uniq if you care. Rails doesn't like nils in config values, secure_headers doesn't mind and will filter them. secure_headers tries to do fancy stuff around consolidating * or overlapping config values, rails is more literal. secure_headers omits schemes by default, rails does what you tell it to.

secure_headers generates a policy that is default_src ... everything else ... report_uri whereas rails doesn't enforce this order.

config values that won't translate

The preserve_schemes / disable_nonce_backwards_compatibility concepts do not exist in rails.

secure_headers with CSP disabled is somewhat of an edge case (for me)

I've never done it but while I was working on this, a test of ours that mucks with 1st X-Frame-Options raised an error about default_src not being configured 😆 thar be dragons.

non-html resources don't get CSP

I'll add details, but there's a situation where you need to send csp on PDF resources and you may want to for e.g. api requests.

And more

I'll keep updating this issue body as we roll out the change and I'd love to incorporate anything you've learned.

agrobbin commented 1 year ago

@oreoshake I thought it worth mentioning that I just opened rails/rails#46859, which allows modification of Rails' global CSP. It is a much simpler implementation than what you describe in your comment above, but it's what I've been using in some projects via a monkey patch, and has worked reasonably well!

oreoshake commented 1 year ago

That’s wonderful @agrobbin!

zendesk-yianna commented 1 year ago

This is really good work! Thank you :)