Skip to content
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.

Sign up for GitHub

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

Use config.load_defaults for rails 7 with overrides #23176

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+25 −3
Open

Use config.load_defaults for rails 7 with overrides #23176

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f7395d4
Use config.load_defaults for rails 7 with overrides
jrafanie Sep 5, 2024
a393d4a
Disable x-xss-protection as it's the defaults now
jrafanie Sep 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions config/application.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,10 @@ class Application < Rails::Application

# Disable ActionCable's request forgery protection
# This is basically matching a set of allowed origins which is not good for us
# Note, similarly named forgery protections in action controller are set to true
# https://github.com/rails/rails/blob/d437ae311f1b9dc40b442e40eb602e020cec4e49/railties/lib/rails/application/configuration.rb#L115C12-L115C69
# 5.0 sets: action_controller.forgery_protection_origin_check = true
# 5.2 sets: action_controller.default_protect_from_forgery = true
config.action_cable.disable_request_forgery_protection = false
# Matching the origin against the HOST header is much more convenient
config.action_cable.allow_same_origin_as_host = true
Expand All @@ -110,8 +114,20 @@ class Application < Rails::Application

config.autoload_paths += config.eager_load_paths

# config.load_defaults 6.1
# Disable defaults as ActiveRecord::Base.belongs_to_required_by_default = true causes MiqRegion.seed to fail validation on belongs_to maintenance zone
# FYI, this is where load_defaults is defined as of 7.2:
# https://github.com/rails/rails/blob/d437ae311f1b9dc40b442e40eb602e020cec4e49/railties/lib/rails/application/configuration.rb#L92
config.load_defaults 7.0

# TODO: Find and fixed any deprecated behavior. Opt in later.
config.active_support.remove_deprecated_time_with_zone_name = false
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# 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
Copy link
Member Author

@jrafanie jrafanie Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fryguy @kbrock do any of the "Using default value from rails 7" section above concern you? They were either nil/false previously or as provided above. Overrides section above describes the overrides we have in the application.rb so we can try to resolve them in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - cross repo is green but I wanted to review the changes I found between current and going to rails 7 defaults.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll have to go through them, because I don't really understand each of them.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to just spit out the configuration? (Like a rails CLI command or something?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# NOTE: If you are going to make changes to autoload_paths, please make
# sure they are all strings. Rails will push these paths into the
Expand Down
8 changes: 7 additions & 1 deletion config/initializers/secure_headers.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,13 @@
config.x_content_type_options = "nosniff"
# X-XSS-Protection
# X-Permitted-Cross-Domain-Policies
config.x_xss_protection = "1; mode=block"

#FYI, this was deprecated and disabled in rails 7. Using content security policy is the desired behavior going forward:
# https://github.com/rails/rails/commit/1f4714c3f798df227222f531141880b8e1b4170a
# https://github.com/rails/rails/blob/d437ae311f1b9dc40b442e40eb602e020cec4e49/railties/lib/rails/application/configuration.rb#L227
# Disable x-xss-protection as it's being dropped by other big stakeholders for legitimate security reasons:
# https://github.com/github/secure_headers/issues/439
config.x_xss_protection = "0"
config.referrer_policy = "no-referrer-when-downgrade"
# Content-Security-Policy
# Need google fonts in fonts_src for https://fonts.googleapis.com/css?family=IBM+Plex+Sans+Condensed%7CIBM+Plex+Sans:400,600&display=swap (For carbon-charts download)
Expand Down