Conversation
4f5686e to
930157e
Compare
Do not join cookies with new like if they weren't before fix(middleware): ensure headers are wrapped with `Rack::Headers` Add `Rack::Headers` wrapping to middleware to prevent header manipulation issues. Added a test to verify cookies remain as an array when flagged if already in array format.
While this gem now uses lowercase headers, the Rails default configuration still defines non-lowercase headers. As a result, our Railtie will not remove those conflicting headers. This change ensures that we're accounting for both lowercase and non-lowercase default headers in Rails.
CSP3 more explicitly calls this out: > If path A consists of one character that is equal to the U+002F > SOLIDUS character (/) and path B is empty, return "Matches". A URL like `example.com/foo` will match a connect-src of `example.com`, as well as `example.com/`, so having two connect-srcs listed like this is redundant. fix: allow URIs with schema to have trailing slashes normalised Co-authored-by: Dusty Greif <dgreif@users.noreply.github.com>
Fix rake task file count output message
2967cd5 to
6ac6e72
Compare
Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>m>
Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>
6ac6e72 to
e5f347e
Compare
closes #512 ## All PRs: * [x] Has tests * [x] Documentation updated ## Adding a new header (Reporting-Endpoints) **Is the header supported by any user agent?* Yes - Chrome 116+, Edge 116+, Opera 102+ (via Reporting API) **What does it do?** Defines HTTP reporting endpoints for CSP violations and other security/performance reports using the HTTP Reporting API **What are the valid values?** Comma-separated pairs of [name="url"] where url must be HTTPS (e.g., csp-violations="https://example.com/reports") **Where does the specification live?** [MDN Reporting-Endpoints](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Reporting-Endpoints) and [MDN report-to directive](https://developer.mozilla.org/en-US/docs/Web/HTTP/Reference/Headers/Content-Security-Policy/report-to) ## Adding a new CSP directive (report-to) **Is the directive supported by any user agent?** Yes - Chrome 69+, Edge 79+, Firefox 110+, Safari 15.1+ **What does it do?** Specifies a named reporting endpoint (defined via Reporting-Endpoints header) where CSP violations should be reported, replacing or complementing report-uri **What are the valid values?** A single string endpoint name (e.g., report-to csp-violations), must match a name defined in the Reporting-Endpoints header --------- Co-authored-by: Kylie Stradley <kyfast@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Aggregates the 7.2 release changes, including Rack 3 / Rails compatibility fixes, CSP enhancements (reporting + HTTP/HTTPS behavior), and an opt-out mechanism to fully disable secure_headers.
Changes:
- Add
SecureHeaders::Configuration.disable!to fully disable header injection and middleware behavior. - Improve CSP features: support
report-to+reporting-endpoints, normalize trailing slashes in CSP sources, and omitupgrade-insecure-requestson HTTP requests. - Improve framework compatibility: Rack 3 header/cookie handling updates and Railtie removal of Rails default headers regardless of header casing; refactor hash-generation rake task into a helper.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/spec_helper.rb | Adds reset helper for new disabled/noop configuration state. |
| spec/lib/secure_headers_spec.rb | Adds/extends specs for disable!, CSP upgrade-insecure-requests behavior, report-to/reporting-endpoints. |
| spec/lib/secure_headers/task_helper_spec.rb | New specs for extracted rake-task helper behavior. |
| spec/lib/secure_headers/railtie_spec.rb | New specs to validate case-insensitive Rails default header removal. |
| spec/lib/secure_headers/middleware_spec.rb | Adds coverage for Rack::Headers conversion, cookie handling, and disabled behavior. |
| spec/lib/secure_headers/headers/reporting_endpoints_spec.rb | New unit tests for Reporting-Endpoints header generation/validation. |
| spec/lib/secure_headers/headers/policy_management_spec.rb | Adds validation coverage for new report_to directive type. |
| spec/lib/secure_headers/headers/content_security_policy_spec.rb | Adds CSP tests for trailing-slash normalization and report-to formatting/order. |
| spec/lib/secure_headers/configuration_spec.rb | Adds coverage for Configuration.disable! lifecycle and interactions. |
| lib/tasks/tasks.rake | Refactors hash generation task to use extracted helper and improves output message. |
| lib/secure_headers/task_helper.rb | Extracts rake-task hashing logic into a reusable helper module. |
| lib/secure_headers/railtie.rb | Removes conflicting Rails default headers case-insensitively. |
| lib/secure_headers/middleware.rb | Improves Rack 3 compatibility (Rack::Headers) and preserves Set-Cookie arrays when present. |
| lib/secure_headers/headers/reporting_endpoints.rb | Introduces Reporting-Endpoints header support + config validation. |
| lib/secure_headers/headers/policy_management.rb | Adds report_to directive support and validation wiring. |
| lib/secure_headers/headers/content_security_policy.rb | Adds report-to directive rendering and CSP source trailing-slash normalization. |
| lib/secure_headers/configuration.rb | Adds disable!/disabled? and new reporting_endpoints configuration attribute + dup support. |
| lib/secure_headers.rb | Wires in reporting-endpoints, HTTP-specific CSP behavior, and documents upgrade-insecure-requests behavior in code comments. |
| README.md | Documents report-to/reporting-endpoints usage and how to fully disable secure_headers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # formatted header value based on the config. | ||
| def make_header(config = nil) | ||
| return if config.nil? || config == OPT_OUT | ||
| validate_config!(config) |
There was a problem hiding this comment.
make_header will emit a reporting-endpoints header with an empty value when config is an empty Hash (since format_endpoints({}) returns "" and generate_headers treats empty string as truthy). Consider treating an empty Hash like nil/OPT_OUT (return no header) or making validate_config! reject empty configs.
| validate_config!(config) | |
| validate_config!(config) | |
| return if config.is_a?(Hash) && config.empty? |
| @@ -0,0 +1,123 @@ | |||
| # frozen_string_literal: true | |||
| require "spec_helper" | |||
| require "secure_headers/task_helper" | |||
There was a problem hiding this comment.
This spec uses Tempfile.create but never requires the tempfile stdlib, which will cause NameError: uninitialized constant Tempfile in a clean test run. Add require "tempfile" near the top of the file (or in spec_helper.rb).
| require "secure_headers/task_helper" | |
| require "secure_headers/task_helper" | |
| require "tempfile" |
| end | ||
| }.not_to raise_error | ||
|
|
||
| # After reset_config, disabled? returns nil (not false) because @disabled is removed |
There was a problem hiding this comment.
The comment says disabled? returns nil (not false), but Configuration.disabled? is implemented as defined?(@disabled) && @disabled, which returns false when @disabled is removed. Please update the comment to match the actual behavior to avoid confusion.
| # After reset_config, disabled? returns nil (not false) because @disabled is removed | |
| # After reset_config, disabled? returns a falsy value because @disabled is removed |
| def normalize_uri_paths(source_list) | ||
| source_list.map do |source| | ||
| # Normalize domains ending in a single / as without omitting the slash accomplishes the same. | ||
| # https://www.w3.org/TR/CSP3/#match-paths § 6.6.2.10 Step 2 | ||
| begin | ||
| uri = URI(source) | ||
| if uri.path == "/" | ||
| next source.chomp("/") | ||
| end | ||
| rescue URI::InvalidURIError |
There was a problem hiding this comment.
normalize_uri_paths calls URI(source)/rescues URI::InvalidURIError, but this file never requires the stdlib uri. In a clean Ruby process this will raise NameError: uninitialized constant URI. Add require "uri" (or equivalent) before using URI.
Fixes: #541
Fixes: #514
Fixes: #450
Fixes: #348