-
Notifications
You must be signed in to change notification settings - Fork 253
Kyfast/report to directive #557
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
base: feature-7.2
Are you sure you want to change the base?
Conversation
|
Hi @tmaier thanks for your interest in secure_headers. I'm a maintainer and I've been testing all of the open PRs manually for compatibility. When I was testing #556 I noticed that implementation doesn’t deep copy reporting_endpoints in the dup method, unlike this implementation. I found that the reporting-endpoints header wasn’t always preserved on pages where overrides were used. When the config is overridden, the reporting endpoints might get dropped or unintentionally changed. Without a deep copy, changes to reporting endpoints for one request can accidentally get shared with other requests or configs—basically, updates can “leak” in weird ways, especially in threaded environments. I only ran into this in an app that uses overrides, did you happen to test this out on an app that uses secure_headers overrides? |
647cb49 to
340326a
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.
Pull request overview
This PR adds support for the modern CSP report-to directive and the Reporting-Endpoints header, which are part of the HTTP Reporting API. The changes enable more flexible CSP violation reporting while maintaining backward compatibility with the legacy report-uri directive.
Key changes:
- Implements a new
ReportingEndpointsheader class for configuring reporting endpoints via the Reporting-Endpoints header - Adds the
report-toCSP directive to specify named reporting endpoints - Refactors code style from
class << selfblocks todef self.method definitions across multiple header classes for consistency
Reviewed changes
Copilot reviewed 22 out of 25 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/secure_headers/headers/reporting_endpoints.rb | New header implementation for Reporting-Endpoints with validation |
| lib/secure_headers/headers/policy_management.rb | Adds report-to directive definition, validation, and ordering logic |
| lib/secure_headers/headers/content_security_policy.rb | Implements report-to directive building and ordering |
| lib/secure_headers/configuration.rb | Adds reporting_endpoints configuration attribute and dup support |
| lib/secure_headers.rb | Requires the new reporting_endpoints module and minor style improvement |
| spec/lib/secure_headers/headers/reporting_endpoints_spec.rb | Comprehensive tests for new header validation and formatting |
| spec/lib/secure_headers/headers/policy_management_spec.rb | Tests for report-to directive validation |
| spec/lib/secure_headers/headers/content_security_policy_spec.rb | Tests for report-to directive output and ordering |
| spec/lib/secure_headers_spec.rb | Integration tests for report-to with overrides, appends, and reporting_endpoints header generation |
| README.md | Documentation for report-to and reporting_endpoints configuration, badge update |
| .rubocop.yml | Updates RuboCop configuration to use modern plugins syntax |
| Multiple header files | Style refactoring from class << self to def self. syntax |
| Multiple spec files | Style improvements for hash literal spacing consistency |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| config.reporting_endpoints = { | ||
| "csp-endpoint": "https://example.com/reports" |
Copilot
AI
Dec 17, 2025
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.
The README example has the same symbol keys issue. The configuration should use string keys (hash rocket syntax) instead of symbol syntax to match the implementation's validation requirements.
Line 128-130 should use:
"csp-endpoint" => "https://example.com/reports"
instead of:
"csp-endpoint": "https://example.com/reports"
| "csp-endpoint": "https://example.com/reports" | |
| "csp-endpoint" => "https://example.com/reports" |
| describe "report_to with overrides and appends" do | ||
| let(:request) { double("Request", scheme: "https", env: {}) } | ||
|
|
||
| it "overrides the report_to directive" do | ||
| Configuration.default do |config| | ||
| config.csp = { | ||
| default_src: %w('self'), | ||
| script_src: %w('self'), | ||
| report_to: "endpoint-1" | ||
| } | ||
| end | ||
|
|
||
| SecureHeaders.override_content_security_policy_directives(request, report_to: "endpoint-2") | ||
| headers = SecureHeaders.header_hash_for(request) | ||
| csp_header = headers[ContentSecurityPolicyConfig::HEADER_NAME] | ||
| expect(csp_header).to include("report-to endpoint-2") | ||
| end | ||
|
|
||
| it "includes report_to when appending CSP directives" do | ||
| Configuration.default do |config| | ||
| config.csp = { | ||
| default_src: %w('self'), | ||
| script_src: %w('self') | ||
| } | ||
| end | ||
|
|
||
| SecureHeaders.append_content_security_policy_directives(request, report_to: "new-endpoint") | ||
| headers = SecureHeaders.header_hash_for(request) | ||
| csp_header = headers[ContentSecurityPolicyConfig::HEADER_NAME] | ||
| expect(csp_header).to include("report-to new-endpoint") | ||
| end | ||
|
|
||
| it "handles report_to with report_uri together" do | ||
| Configuration.default do |config| | ||
| config.csp = { | ||
| default_src: %w('self'), | ||
| script_src: %w('self'), | ||
| report_uri: %w(/csp-report), | ||
| report_to: "reporting-endpoint" | ||
| } | ||
| end | ||
|
|
||
| headers = SecureHeaders.header_hash_for(request) | ||
| csp_header = headers[ContentSecurityPolicyConfig::HEADER_NAME] | ||
| # Both should be present | ||
| expect(csp_header).to include("report-to reporting-endpoint") | ||
| expect(csp_header).to include("report-uri /csp-report") | ||
| # report-to should come before report-uri (alphabetical order) | ||
| expect(csp_header.index("report-to")).to be < csp_header.index("report-uri") | ||
| end | ||
| end |
Copilot
AI
Dec 17, 2025
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.
The test coverage for report-to directive is limited to enforced CSP policies. Consider adding tests for report-to with csp_report_only configurations to ensure the directive works correctly in report-only mode as well. This would include testing scenarios like:
- Setting report-to on csp_report_only config
- Overriding report-to on report-only policy
- Appending report-to to report-only policy
Given that report-to is specifically designed for CSP reporting and is mentioned in the PR description as complementing report-uri, it should be tested with both enforcement modes.
| def format_endpoints(config) | ||
| config.map do |name, url| | ||
| %{#{name}="#{url}"} | ||
| end.join(", ") | ||
| end |
Copilot
AI
Dec 17, 2025
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.
The format_endpoints method does not escape special characters in endpoint names or URLs. If a URL contains double quotes, it could break the header format or potentially lead to header injection vulnerabilities.
For example, if a URL is: https://example.com/report?name="malicious", the resulting header would be malformed:
reporting-endpoints: endpoint="https://example.com/report?name="malicious""
Consider adding validation to reject URLs or endpoint names containing double quotes, or properly escape them according to the Reporting-Endpoints specification. According to the HTTP Reporting API specification, URLs should be properly quoted and escaped.
| "csp-endpoint": "https://report-uri.io/example-csp", | ||
| "csp-report-only": "https://report-uri.io/example-csp-report-only" |
Copilot
AI
Dec 17, 2025
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.
The README example uses symbol keys with colons at the end (Ruby 1.9+ syntax) for the reporting_endpoints configuration, but the implementation expects string keys. This inconsistency could lead to configuration errors.
In lines 98-101, the example shows:
"csp-endpoint": "https://..."
But based on the ReportingEndpoints.validate_config! method (line 27 in reporting_endpoints.rb), which checks name.is_a?(String), the implementation expects string keys like:
"csp-endpoint" => "https://..."
While Ruby symbols and string keys might work interchangeably in some cases, the validation explicitly checks for String instances, and using symbol keys with the colon syntax is misleading since they're actually symbols, not strings.
| "csp-endpoint": "https://report-uri.io/example-csp", | |
| "csp-report-only": "https://report-uri.io/example-csp-report-only" | |
| "csp-endpoint" => "https://report-uri.io/example-csp", | |
| "csp-report-only" => "https://report-uri.io/example-csp-report-only" |
…ount for backwards compatibility with report-uri
340326a to
ace6403
Compare
closes #512
All PRs:
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 and MDN report-to directive
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