Skip to content

Commit 19de22b

Browse files
authored
Don't set upgrade-insecure-requests directive for HTTP requests (#570)
## Plan to fix upgrade_insecure_requests for HTTP requests - [x] Understand the current implementation and test structure - [x] Modify `header_hash_for` method in `lib/secure_headers.rb` to remove `upgrade_insecure_requests` from CSP when request is HTTP - [x] Add tests to verify `upgrade_insecure_requests` is removed for HTTP requests - [x] Add tests to verify `upgrade_insecure_requests` is preserved for HTTPS requests - [x] Run existing tests to ensure no regressions (251 tests pass) - [x] Fix linting issues (rubocop clean) - [x] Refactor to extract helper method and reduce code duplication - [x] Request code review (no issues found) - [x] Run security scanner (no alerts) - [x] Revert .gitignore changes per review feedback ## Summary This PR fixes issue where `upgrade_insecure_requests` was set even for HTTP requests, breaking local development. The fix follows the same pattern as HSTS - the directive is now only included when serving over HTTPS. ### Security Summary No security vulnerabilities detected by CodeQL scanner. <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>Don't upgrade insecure requests when the page is served over HTTP</issue_title> > <issue_description>I want to set `upgrade_insecure_requests` only if the page is requested over HTTPS, because I don't use HTTPS when developing on localhost (i.e. when developing, I browse http://localhost, not https://localhost). > > Right now if I set `upgrade_insecure_requests`, I can't develop locally since all internal resource requests are upgraded, and since my local server doesn't support HTTPS, they fail. > > I think this should be a fairly typical scenario. Would this be considered a bug, or is there a work-around for it? > > ### Relevant software versions > > * rack 1.6 > * secure_headers 3.6.7 > > ### Expected outcome > > 1. I configure CSP to `upgrade_insecure_requests`. > 1. I browse http://localhost while developing. > 1. `upgrade_insecure_requests` header shouldn't be set, since it's pointless as the webpage itself is being insecurely served. > > ### Actual outcome > > 1. `upgrade_insecure_requests` header is set and all internal resources are broken. > > ### Config > > ``` > SecureHeaders::Configuration.default do |config| > config.cookies = { > secure: true, > httponly: true, > samesite: { > lax: true > } > } > # Add "; preload" and submit the site to hstspreload.org for best protection. > config.hsts = "max-age=#{20.years.to_i}; includeSubdomains" > config.x_frame_options = "DENY" > config.x_content_type_options = "nosniff" > config.x_xss_protection = "1; mode=block" > config.x_download_options = "noopen" > config.x_permitted_cross_domain_policies = "none" > config.referrer_policy = "same-origin" > config.clear_site_data = [ > "cache", > "cookies", > "storage", > "executionContexts" > ] > config.csp = { > # "meta" values. these will shaped the header, but the values are not included in the header. > # report_only: true, # default: false [DEPRECATED from 3.5.0: instead, configure csp_report_only] > preserve_schemes: true, # default: false. Schemes are removed from host sources to save bytes and discourage mixed content. > > # directive values: these values will directly translate into source directives > default_src: %w('self'), > base_uri: %w('self'), > #block_all_mixed_content: true, # see http://www.w3.org/TR/mixed-content/ > child_src: %w('self'), # if child-src isn't supported, the value for frame-src will be set. > connect_src: %w(), > font_src: %w('self'), > form_action: %w('self'), > frame_ancestors: %w('none'), > img_src: %w('self' *), > manifest_src: %w('self'), > media_src: %w('self'), > object_src: %w('self'), > plugin_types: %w(), > script_src: %w('self' 'unsafe-inline' maps.googleapis.com), > style_src: %w('self' 'unsafe-inline'), > upgrade_insecure_requests: true, # see https://www.w3.org/TR/upgrade-insecure-requests/ > } > end > ``` > > ### Generated headers > > ``` > [...] > Content-Security-Policy:default-src 'self'; base-uri 'self'; child-src 'self'; font-src 'self'; form-action 'self'; frame-ancestors 'none'; img-src *; manifest-src 'self'; media-src 'self'; object-src 'self'; script-src 'self' 'unsafe-inline' maps.googleapis.com; style-src 'self' 'unsafe-inline'; upgrade-insecure-requests > [...] > ``` > </issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > </comments> > </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #348 <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/github/secure_headers/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo.
2 parents b3557f7 + 1c66c22 commit 19de22b

File tree

2 files changed

+85
-0
lines changed

2 files changed

+85
-0
lines changed

lib/secure_headers.rb

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ def opt_out_of_all_protection(request)
133133
# request.
134134
#
135135
# StrictTransportSecurity is not applied to http requests.
136+
# upgrade_insecure_requests is not applied to http requests.
136137
# See #config_for to determine which config is used for a given request.
137138
#
138139
# Returns a hash of header names => header values. The value
@@ -146,6 +147,11 @@ def header_hash_for(request)
146147

147148
if request.scheme != HTTPS
148149
headers.delete(StrictTransportSecurity::HEADER_NAME)
150+
151+
# Remove upgrade_insecure_requests from CSP headers for HTTP requests
152+
# as it doesn't make sense to upgrade requests when the page itself is served over HTTP
153+
remove_upgrade_insecure_requests_from_csp!(headers, config.csp)
154+
remove_upgrade_insecure_requests_from_csp!(headers, config.csp_report_only)
149155
end
150156
headers
151157
end
@@ -242,6 +248,23 @@ def content_security_policy_nonce(request, script_or_style)
242248
def override_secure_headers_request_config(request, config)
243249
request.env[SECURE_HEADERS_CONFIG] = config
244250
end
251+
252+
# Private: removes upgrade_insecure_requests directive from a CSP config
253+
# if it's present, and updates the headers hash with the modified CSP.
254+
#
255+
# headers - the headers hash to update
256+
# csp_config - the CSP config to check and potentially modify
257+
#
258+
# Returns nothing (modifies headers in place)
259+
def remove_upgrade_insecure_requests_from_csp!(headers, csp_config)
260+
return if csp_config.opt_out?
261+
return unless csp_config.directive_value(ContentSecurityPolicy::UPGRADE_INSECURE_REQUESTS)
262+
263+
modified_config = csp_config.dup
264+
modified_config.update_directive(ContentSecurityPolicy::UPGRADE_INSECURE_REQUESTS, false)
265+
header_name, value = ContentSecurityPolicy.make_header(modified_config)
266+
headers[header_name] = value if header_name && value
267+
end
245268
end
246269

247270
# These methods are mixed into controllers and delegate to the class method

spec/lib/secure_headers_spec.rb

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -436,6 +436,68 @@ module SecureHeaders
436436

437437
end
438438
end
439+
440+
it "does not set upgrade-insecure-requests if request is over HTTP" do
441+
reset_config
442+
Configuration.default do |config|
443+
config.csp = {
444+
default_src: %w('self'),
445+
script_src: %w('self'),
446+
upgrade_insecure_requests: true
447+
}
448+
end
449+
450+
plaintext_request = Rack::Request.new({})
451+
hash = SecureHeaders.header_hash_for(plaintext_request)
452+
expect(hash[ContentSecurityPolicyConfig::HEADER_NAME]).to eq("default-src 'self'; script-src 'self'")
453+
expect(hash[ContentSecurityPolicyConfig::HEADER_NAME]).not_to include("upgrade-insecure-requests")
454+
end
455+
456+
it "sets upgrade-insecure-requests if request is over HTTPS" do
457+
reset_config
458+
Configuration.default do |config|
459+
config.csp = {
460+
default_src: %w('self'),
461+
script_src: %w('self'),
462+
upgrade_insecure_requests: true
463+
}
464+
end
465+
466+
https_request = Rack::Request.new("HTTPS" => "on")
467+
hash = SecureHeaders.header_hash_for(https_request)
468+
expect(hash[ContentSecurityPolicyConfig::HEADER_NAME]).to eq("default-src 'self'; script-src 'self'; upgrade-insecure-requests")
469+
end
470+
471+
it "does not set upgrade-insecure-requests in report-only mode if request is over HTTP" do
472+
reset_config
473+
Configuration.default do |config|
474+
config.csp_report_only = {
475+
default_src: %w('self'),
476+
script_src: %w('self'),
477+
upgrade_insecure_requests: true
478+
}
479+
end
480+
481+
plaintext_request = Rack::Request.new({})
482+
hash = SecureHeaders.header_hash_for(plaintext_request)
483+
expect(hash[ContentSecurityPolicyReportOnlyConfig::HEADER_NAME]).to eq("default-src 'self'; script-src 'self'")
484+
expect(hash[ContentSecurityPolicyReportOnlyConfig::HEADER_NAME]).not_to include("upgrade-insecure-requests")
485+
end
486+
487+
it "sets upgrade-insecure-requests in report-only mode if request is over HTTPS" do
488+
reset_config
489+
Configuration.default do |config|
490+
config.csp_report_only = {
491+
default_src: %w('self'),
492+
script_src: %w('self'),
493+
upgrade_insecure_requests: true
494+
}
495+
end
496+
497+
https_request = Rack::Request.new("HTTPS" => "on")
498+
hash = SecureHeaders.header_hash_for(https_request)
499+
expect(hash[ContentSecurityPolicyReportOnlyConfig::HEADER_NAME]).to eq("default-src 'self'; script-src 'self'; upgrade-insecure-requests")
500+
end
439501
end
440502
end
441503

0 commit comments

Comments
 (0)