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.
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
Add Rack::Lint to ContentSecurityPolicy::Middleware tests #48812
Add Rack::Lint to ContentSecurityPolicy::Middleware tests #48812
Conversation
50feb03
to
bccbb33
Compare
@@ -25,9 +25,8 @@ module ActionDispatch # :nodoc: | |||
# end | |||
class ContentSecurityPolicy | |||
class Middleware | |||
CONTENT_TYPE = "Content-Type" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was not used / referenced.
bccbb33
to
5094853
Compare
def test_does_not_override_app_content_security_policy | ||
app = proc { [200, { "content-security-policy" => @default_csp }, []] } | ||
_, headers, _ = Rack::Lint.new( | ||
ActionDispatch::ContentSecurityPolicy::Middleware.new(Rack::Lint.new(app)) | ||
).call(@env) | ||
|
||
assert_equal @default_csp, headers["content-security-policy"] | ||
end | ||
|
||
def test_does_not_override_app_content_security_policy_for_rack_v2 | ||
app = proc { [200, { "Content-Security-Policy" => @default_csp }, []] } | ||
_, headers, _ = ActionDispatch::ContentSecurityPolicy::Middleware.new(app).call(@env) | ||
|
||
assert_equal @default_csp, headers["content-security-policy"] | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using conditional headers, we'll probably have to make this a single test that also uses conditional headers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opted to drop this v2
test as I don't know how to set the gem version for this test only.
Perhaps that is overthinking it anyway, as these dependencies should be set by the environment.
I did this in my local machine fwiw:
% export RACK=2.2.4
% bundle
# ...
% bundle show rack
/home/spin/.bundle/rails/gems/rack-2.2.4
% bin/test test/dispatch/content_security_policy_test.rb
#...
34 runs, 190 assertions, 0 failures, 0 errors, 0 skips
To ensure Rails is and remains compliant with [the Rack 3 spec](https://github.com/rack/rack/blob/6d16306192349e665e4ec820a9bfcc0967009b6a/UPGRADE-GUIDE.md) we can add `Rack::Lint` to the Rails middleware tests. This adds additional test coverage to `ContentSecurityPolicy::Middleware` to validate that its input and output follow the Rack SPEC. The changes made are because of: - [Response Headers must be lower case](https://github.com/rack/rack/blob/6d16306192349e665e4ec820a9bfcc0967009b6a/UPGRADE-GUIDE.md#response-headers-must-be-lower-case) Added tests to ensure that CSP headers set by an app are not overridden, regardless of the casing. An example of this is Sidekiq: https://github.com/sidekiq/sidekiq/blob/b3225ce/lib/sidekiq/web/application.rb#L353
5094853
to
d1381dc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Motivation / Background
To ensure Rails is and remains compliant with the Rack 3 spec we can add
Rack::Lint
to the Rails middleware tests.Detail
This adds additional test coverage to
ContentSecurityPolicy::Middleware
to validate that its input and output follow the Rack SPEC.The changes made are because of:
Added tests to ensure that CSP headers set by an app are not overridden, regardless of the casing.
An example of this is Sidekiq: https://github.com/sidekiq/sidekiq/blob/b3225ce/lib/sidekiq/web/application.rb#L353
Additional information
Checklist
Before submitting the PR make sure the following are checked:
[Fix #issue-number]