-
Notifications
You must be signed in to change notification settings - Fork 253
Add Configuration.disable! to completely disable secure_headers #568
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
Conversation
|
@copilot can you rebase this PR onto the |
I've successfully rebased the PR onto the The branch is ready but requires a force push to update the remote. Since I don't have permission to force push directly, could you please run: git push --force-with-lease origin copilot/add-static-config-optionThe rebased commits now include all changes from |
Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>
Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>
Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>
Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>
Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>
f76080a to
b93ac8d
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 introduces a runtime disable mechanism for the secure_headers gem, allowing applications to conditionally bypass all security header generation without requiring configuration. This addresses the need for gradual rollouts and per-deployment control.
Key Changes:
- Added
Configuration.disable!andConfiguration.disabled?methods for runtime control - Modified
default_configto return a NOOP configuration when disabled, bypassingNotYetConfiguredError - Refactored NOOP configuration creation to eliminate code duplication between
disable!andNOOP_OVERRIDE
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| lib/secure_headers/configuration.rb | Core implementation of disable!/disabled? methods, default_config bypass logic, and NOOP refactoring |
| spec/lib/secure_headers/configuration_spec.rb | Unit tests for disable! functionality and disabled state verification |
| spec/lib/secure_headers_spec.rb | Integration test verifying headers are not set when disabled |
| spec/lib/secure_headers/middleware_spec.rb | Middleware tests confirming no headers or cookie flags when disabled |
| spec/spec_helper.rb | Added clear_disabled helper method and integrated into reset_config |
| README.md | Documentation for the disable! feature with usage examples and use cases |
| .gitignore | Added vendor/bundle to ignore list |
Comments suppressed due to low confidence (1)
lib/secure_headers/configuration.rb:44
- The
defaultmethod will fail when called afterdisable!has been invoked. Theoverride(NOOP_OVERRIDE, ...)call at line 39 will work fine, but if@disabledis true, the NOOP_OVERRIDE override will be registered. However, there's no check to prevent callingdefaultafterdisable!, which could lead to confusing behavior where the library is supposed to be disabled but a default config gets set.
Consider either:
- Raising an error if
defaultis called afterdisable! - Clearing the
@disabledflag and@noop_configwhendefaultis called (re-enabling the library) - Making the disabled state override any subsequent configuration attempts
def default(&block)
if defined?(@default_config)
raise AlreadyConfiguredError, "Policy already configured"
end
# Define a built-in override that clears all configuration options and
# results in no security headers being set.
override(NOOP_OVERRIDE, &method(:create_noop_config_block))
new_config = new(&block).freeze
new_config.validate_config!
@default_config = new_config
end
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot Can you address the feedback in this thread: #568 (review) |
- Register NOOP_OVERRIDE in disable! to avoid ArgumentError - Clear @default_config when disable! is called after default - Move clear_disabled inside class << self for consistency - Add comprehensive edge case tests (calling disable! after default, default after disable!, interaction with overrides/dup) - Document behavior in method comments Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>
Addressed all feedback from the review (commit 8f188da): Issue 1 - NOOP_OVERRIDE registration: Now registers NOOP_OVERRIDE in Issue 2 - Inconsistent state: Now clears Issue 3 - Test helper location: Moved Issue 4 - Missing test coverage: Added 6 comprehensive edge case tests covering:
All 269 tests pass. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot A call to |
- disable! now raises AlreadyConfiguredError if default has been called - default now raises AlreadyConfiguredError if disable! has been called - Updated tests to verify error conditions - Updated documentation to reflect mutual exclusion Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>
Implemented mutual exclusion (commit af7f61b): disable! after default: Now raises default after disable!: Now raises Added tests for both error conditions. All 270 tests pass. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot Can you undo any changes to the |
Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>
fletchto99
left a comment
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 from me, I'll get one more review from prodsec before merging into the 7.2 feature branch
|
|
||
| ```ruby | ||
| if ENV["ENABLE_STRICT_HEADERS"] | ||
| SecureHeaders::Configuration.default do |config| |
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.
since this is a configuration setting, this can only be done at server startup right? I think that's implied but would it be valuable to make that clearer in case someone gets the idea that they could disable this during runtime? I could go either way on whether or not that's overkill
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.
Good point. I think that's worth documenting.
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've also made it so that they're mutually exclusive preventing one from trying to configure and then disable.
rei-moo
left a comment
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.
Look OK, although we'll need to do some manual testing before the 7.2 release to make sure there aren't any strange quirks that come along with this change.
|
|
||
| However, I would consider these headers anyways depending on your load and bandwidth requirements. | ||
|
|
||
| ## Disabling secure_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.
Do you think this is important enough to be in the README or would it make sense to document it elsewhere?
|
@copilot can you update the readme instructions to indicate it can only done during startup. Either it's disabled or configured but cannot be changed later. |
Clarify in README that Configuration.disable! and Configuration.default must be called during application startup and cannot be changed at runtime. Co-authored-by: fletchto99 <718681+fletchto99@users.noreply.github.com>
Updated README to clarify startup-time configuration (commit 6a78f95). Added documentation that |
@disabledflag to ConfigurationConfiguration.disable!class method to set the flagConfiguration.disabled?class method to check the flagConfiguration.default_configto return NOOP config when disabledConfiguration.disable!Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.