Use config.load_defaults for rails 7 with overrides #23176
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@miq-bot cross-repo-tests /all |
miq-bot
pushed a commit
to ManageIQ/manageiq-cross_repo-tests
that referenced
this pull request
Sep 5, 2024
From Pull Request: ManageIQ/manageiq#23176
miq-bot
pushed a commit
to ManageIQ/manageiq-cross_repo-tests
that referenced
this pull request
Sep 5, 2024
From Pull Request: ManageIQ/manageiq#23176
This was referenced Sep 5, 2024
|
I'm looking at cross repo failures manageiq-contentmanageiq-providers-amazonSeems to be caused by active_record.partial_inserts = false manageiq-ui-classic |
|
I'm testing this using this in my config/application.rb. click me config.load_defaults 6.1
#1
config.action_dispatch.default_headers = {
"X-Frame-Options" => "SAMEORIGIN",
"X-XSS-Protection" => "0",
"X-Content-Type-Options" => "nosniff",
"X-Download-Options" => "noopen",
"X-Permitted-Cross-Domain-Policies" => "none",
"Referrer-Policy" => "strict-origin-when-cross-origin"
}
# 2
# config.action_dispatch.return_only_request_media_type_on_content_type = true
config.action_dispatch.return_only_request_media_type_on_content_type = false
# 3
# config.action_dispatch.cookies_serializer = nil
config.action_dispatch.cookies_serializer = :json
# 4
# config.action_view.button_to_generates_button_tag = nil
config.action_view.button_to_generates_button_tag = true
# 5
# config.action_view.apply_stylesheet_media_default = true
config.action_view.apply_stylesheet_media_default = false
# 6
# config.active_support.hash_digest_class = OpenSSL::Digest::SHA1
config.active_support.hash_digest_class = OpenSSL::Digest::SHA256
# 7
# config.active_support.key_generator_hash_digest_class = nil
config.active_support.key_generator_hash_digest_class = OpenSSL::Digest::SHA256
# 8
# config.active_support.remove_deprecated_time_with_zone_name = nil
config.active_support.remove_deprecated_time_with_zone_name = true
# 9
# config.active_support.cache_format_version = nil
config.active_support.cache_format_version = 7.0
# 10
# config.active_support.use_rfc4122_namespaced_uuids = nil
config.active_support.use_rfc4122_namespaced_uuids = true
# 11
# config.active_support.executor_around_test_case = nil
config.active_support.executor_around_test_case = true
# 12
# config.active_support.disable_to_s_conversion = false
config.active_support.disable_to_s_conversion = true
# 13
# config.action_mailer.smtp_timeout = nil
config.action_mailer.smtp_timeout = 5
# 14
# config.active_record.verify_foreign_keys_for_fixtures = nil
config.active_record.verify_foreign_keys_for_fixtures = true
# 15
# config.active_record.partial_inserts = nil
config.active_record.partial_inserts = false
# 16
# config.active_record.automatic_scope_inversing = nil
config.active_record.automatic_scope_inversing = true
# 17
# config.action_controller.raise_on_open_redirects = false
config.action_controller.raise_on_open_redirects = true
# 18
# config.action_controller.wrap_parameters_by_default = false
config.action_controller.wrap_parameters_by_default = true
That's from here. Only active storage is unconfigured so no need to test. |
jrafanie
force-pushed
the
use_rails_7_load_defaults
branch
2 times, most recently
from
1d788ba
to
8a87f56
Compare
jrafanie
commented
Sep 6, 2024
|
|
||
| # Disable this setting as it causes MiqRegion.seed to fail validation on belongs_to maintenance zone. | ||
| # TODO: We should fix this so we don't need to carry this override. | ||
| config.active_record.belongs_to_required_by_default = false |
There was a problem hiding this comment.
Below are the value before and value with rails 7 default
Overrides(seen above):
# Log the deprecations so we can fix them.
config.active_support.remove_deprecated_time_with_zone_name = true
config.active_support.disable_to_s_conversion = true
# Causes test failures in content, amazon provider, and ui-classic without it
config.active_record.partial_inserts = false
# X-XSS-Protection is overridden from the rails 7 default back to "1; mode=block"
# Rails 7:
config.action_dispatch.default_headers = {
"X-Frame-Options" => "SAMEORIGIN",
"X-XSS-Protection" => "0",
"X-Content-Type-Options" => "nosniff",
"X-Download-Options" => "noopen",
"X-Permitted-Cross-Domain-Policies" => "none",
"Referrer-Policy" => "strict-origin-when-cross-origin"
}
# Previously:
config.action_dispatch.default_headers = {
"X-Frame-Options" => "SAMEORIGIN",
"X-XSS-Protection" => "1; mode=block"
"X-Content-Type-Options" => "nosniff",
"X-Download-Options" => "noopen",
"X-Permitted-Cross-Domain-Policies" => "none",
"Referrer-Policy" => "strict-origin-when-cross-origin"
}
Using default value from rails 7
Note, prior values were either nil or false.
config.action_dispatch.return_only_request_media_type_on_content_type = false
config.action_dispatch.cookies_serializer = :json
config.action_view.button_to_generates_button_tag = true
config.action_view.apply_stylesheet_media_default = false
config.active_support.key_generator_hash_digest_class = OpenSSL::Digest::SHA256
config.active_support.cache_format_version = 7.0
config.active_support.use_rfc4122_namespaced_uuids = true
config.active_support.executor_around_test_case = true
config.action_mailer.smtp_timeout = 5
config.active_record.verify_foreign_keys_for_fixtures = true
config.active_record.automatic_scope_inversing = true
config.action_controller.raise_on_open_redirects = true
config.action_controller.wrap_parameters_by_default = true
# Using default value from rails 7. Was previously: OpenSSL::Digest::SHA1
config.active_support.hash_digest_class = OpenSSL::Digest::SHA256
There was a problem hiding this comment.
There was a problem hiding this comment.
FYI - cross repo is green but I wanted to review the changes I found between current and going to rails 7 defaults.
There was a problem hiding this comment.
I'll have to go through them, because I don't really understand each of them.
There was a problem hiding this comment.
FYI, here's the git blame for 7.0 defaults. Note, I've already reviewed the overrides seen above. I think the others we're accepting the rails 7 defaults should either be covered by tests or something we'll find out when others start playing with it. The OpenSSL changes may not be tested in our tests that well so it's on the list of more concerning.
There was a problem hiding this comment.
Funny enough, there was an issue where someone was pointing out how these defaults are not universally documented. https://www.github.com/rails/rails/issues/50238
There was a problem hiding this comment.
Is there a way to just spit out the configuration? (Like a rails CLI command or something?)
There was a problem hiding this comment.
Is there a way to just spit out the configuration? (Like a rails CLI command or something?)
Wouldn't that be great? I couldn't find a way. I was walking the configuration and trying to print it and it was a mess. A git diff of the configuration would be great.
For now, I think we run the rake task to update our configuration each time we try to do upgrades and check the new defaults. I think it won't impose new defaults on existing apps so you'd have to bring in new configuration changes and then review the new defaults to determine which you want to accept or reject.
I think we can accept several of these and make plans to fix the rest. I think the X-XSS-Protection has been proven to be unusable so we can probably just drop that unless we know why we need to keep it.
There was a problem hiding this comment.
config.action_controller.raise_on_open_redirects = true... sounds like a good change to raise if redirect to is called on untrusted URL
Fun. Redirecting to untrusted URLs was specifically highlighted in a security course I just took.
There was a problem hiding this comment.
I'm going to test with x-xss-protection disabled. It sounds like that is the way going forward for secure headers, rails and even browsers not supporting it.
See: https://www.github.com/github/secure_headers/issues/439
jrafanie
changed the title
jrafanie
force-pushed
the
use_rails_7_load_defaults
branch
from
8a87f56
to
a6aa43c
Compare
kbrock
reviewed
Sep 19, 2024
| config.active_support.disable_to_s_conversion = false | ||
|
|
||
| # TODO: If disabled, causes cross repo test failures in content, ui-classic and amazon provider | ||
| config.active_record.partial_inserts = true |
There was a problem hiding this comment.
I'm guessing this will not bother inserting default values.
This makes me concerned.
Wonder if defaults setup in attribute or default_value_for confuse our implementation.
There was a problem hiding this comment.
yeah, I haven't dug into it but did read that defaults are the area that can be broken by this. I would assume our defaulting mechanisms could be adjusted to make the tests and code pass with this disabled.
|
https://www.github.com/rails/rails/pull/36946
|
https://github.com/rails/rails/blob/d437ae311f1b9dc40b442e40eb602e020cec4e49/railties/lib/rails/application/configuration.rb#L92 * belongs_to_required_by_default must be overridden or seeding fails * Partial inserts cause test failures in ui-classic, content, and amazon provider * Need to investigate the X-XSS-Protection change before using default of disabling it * Allow deprecations to be found and fixed Fixes ManageIQ#23172
Both rails and secure header are now shipping with 0 by default. See: https://www.github.com/github/secure_headers/issues/439
jrafanie
force-pushed
the
use_rails_7_load_defaults
branch
from
a6aa43c
to
a393d4a
Compare
|
Checked commits jrafanie/manageiq@f7395d4~...a393d4a with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint config/application.rb
config/initializers/secure_headers.rb
|
jrafanie
added a commit
to jrafanie/manageiq
that referenced
this pull request
Sep 26, 2024
jrafanie
added a commit
to jrafanie/manageiq
that referenced
this pull request
Sep 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As far as I can see, belongs_to_required_by_default, is the only override in load_defaults that we manually override. See:
https://github.com/rails/rails/blob/d437ae311f1b9dc40b442e40eb602e020cec4e49/railties/lib/rails/application/configuration.rb#L92
This change makes the override explicit.
Fixes #23172