Skip to content
New issue

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

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fresh_when in combination with CSP not working since 7.0.2.4 #44974

Closed
fschwahn opened this issue Apr 28, 2022 · 5 comments
Closed

fresh_when in combination with CSP not working since 7.0.2.4 #44974

fschwahn opened this issue Apr 28, 2022 · 5 comments

Comments

@fschwahn
Copy link
Contributor

Steps to reproduce

# frozen_string_literal: true

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "rails", "7.0.2.4"
  gem 'capybara'
  gem 'selenium-webdriver'
  gem 'webdrivers'
  gem 'puma'
end

require "rack/test"
require "action_controller/railtie"

class TestApp < Rails::Application
  config.root = __dir__
  config.hosts << "example.org"
  config.session_store :cookie_store, key: "cookie_store_key"
  secrets.secret_key_base = "secret_key_base"

  config.logger = Logger.new($stdout)
  Rails.logger  = config.logger

  config.content_security_policy do |policy|
    policy.base_uri    :none
    policy.script_src  :strict_dynamic
  end
  config.content_security_policy_nonce_generator = ->(_request) { SecureRandom.base64(16) }
  config.content_security_policy_nonce_directives = %w(script-src)
  config.content_security_policy_report_only = false

  routes.draw do
    get "/" => "test#index"
  end
end

class TestController < ActionController::Base
  include Rails.application.routes.url_helpers

  TEMPLATE = DATA.read

  def index
    fresh_when(etag: "9b4e7e16173dd6780e9584a68bc5ef07", last_modified: Time.new(2020, 1, 1).utc) and return
    render inline: TEMPLATE
  end
end

require "minitest/autorun"

class BugTest < ActionDispatch::SystemTestCase
  driven_by :selenium, using: :chrome

  def test_returns_success
    page.accept_alert("Hello") do
      page.visit "/"
    end

    page.accept_alert("Hello") do
      page.visit "/"
    end
  end

  private
    def app
      Rails.application
    end
end
__END__
<html>
<head>
  <%= javascript_tag("alert('Hello')", nonce: true) %>
</head>
</html>

Expected behavior

I'm not sure what's expected here, or if this is just fundamentally incompatible. Cached responses probably shouldn't include CSP headers?

Actual behavior

Since #44635 cached responses (using fresh_when) also return CSP headers, breaking JS in such pages.

System configuration

Rails version: 7.2.0.4 (also affects 6.1.5.1, and probably all versions with above mentioned PR).

Ruby version: 2.7.4p191
@skipkayhil
Copy link
Member

skipkayhil commented Apr 28, 2022 •
edited

Hey @fschwahn, thanks for the report! I believe the solution to this would be to change the nonce_generator to use a value that doesn't change every request (like ->(r) { r.session.id.to_s }). I updated the security docs to describe the trade offs of this approach in #44830, but it's only in the edge guides for now (I'll request a backport).

Let me know if this works for you or if you have other questions!

@fschwahn
Copy link
Contributor Author

@skipkayhil Yes, I think you are correct. Changing the nonce generator as described in #44830 makes the test pass on both 7.2.0.3 and 7.2.0.4.

So this is more a fundamental incompatibility between this particular nonce generator, and conditional GET requests, and this just worked coincidentally due to the bug fixed in #44635.

@fschwahn
Copy link
Contributor Author

@skipkayhil one more question, the guide states

This generation method is compatible with ETags, however its security depends on
the session id being sufficiently random and not being exposed in insecure
cookies.

Is this true when using the default rails cookie session store? I assume it is, but the wording here makes me a bit uneasy 😅

@skipkayhil
Copy link
Member

skipkayhil commented Apr 29, 2022 •
edited

This generation method is compatible with ETags, however its security depends on
the session id being sufficiently random and not being exposed in insecure
cookies.

Is this true when using the default rails cookie session store? I assume it is, but the wording here makes me a bit uneasy 😅

I'm glad you asked! The wording is definitely intentional to ensure that a good configuration is used. The cookie store does use an encrypted cookie jar here:

rails/actionpack/lib/action_dispatch/middleware/session/cookie_store.rb

Line 118 in 6086c51

request.cookie_jar.signed_or_encrypted

Edit: actually after reading through more docs I found that the CookieStore documentation says this as well

Your cookies will be encrypted using your apps secret_key_base.

@fschwahn
Copy link
Contributor Author

Great - maybe this information could also be mentioned in the guide? After reading that segment I was not sure how to verify that this was indeed secure. If the rails defaults are already safe, this could eliminate some uncertainty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@fschwahn @skipkayhil