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.

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

AO3-6640 Remove config overrides from Rails 5.1 #4673

Closed

Conversation

brianjaustin
Copy link
Member

@brianjaustin brianjaustin commented Nov 27, 2023

Issue

https://otwarchive.atlassian.net/browse/AO3-6640

Purpose

Removes the previous upgrade's (5.1 -> 6) configuration override file. Of the overrides, one is not even a valid configuration option anymore, two are better for security if flipped to the new defaults, one is set in application.rb. The only one that may be tricky is to_time_preserves_timezone although it looks like we only use to_time on internal timestamps, which should already be UTC.

Credit

Brian Austin (they/he)

@Bilka2
Copy link
Contributor

Bilka2 commented Dec 4, 2023

per_form_csrf_tokens is not deprecated.
forgery_protection_origin_check is not deprecated.
Flipping these sounds like a good idea regarding security. If it breaks things, that will be found during testing. I don't have the knowledge to tell if it will break things just based on the code.

to_time_preserves_timezone looks okay to flip, all uses of to_time are on UTC timestamps:
Nearly all uses of to_time are inside the translation helpers time_ago_in_words method, which is mostly used for created_at or updated_at timestamps, so internal timestamps that should be in UTC. It is also used for reading.last_viewed, series.revised_at and work.revised_at.
series.revised_at is based on work.revised_at. work.revised_at can be set by Time.current in the work model's set_revised_at method. Time.current depends on the time zone set in the config, which is config.time_zone = "UTC". reading.last_viewed is set to Time.now which is in UTC.

The other two options are okay to remove for the reasons given in the Jira issue. In fact, raise_on_unfiltered_parameters was already set to the new default.

I tested bookmark blurb timestamps locally, they show the correct relative time for recent bookmarks. On my dev setup, I also posted a work, commented on it and made a search, that all worked. So I would say this is good to go.

@Bilka2
Copy link
Contributor

Bilka2 commented Dec 5, 2023

https://guides.rubyonrails.org/upgrading_ruby_on_rails.html#configure-framework-defaults gives more info on the file that's getting changed here. I noticed that config.load_defaults is set to 6.0, not 6.1. I think this should be updated to 6.1 (with potential overrides of new defaults in a new new_framework_defaults file) before the upgrade to 7.0. https://guides.rubyonrails.org/v7.0/configuring.html#default-values-for-target-version-6-1 has an overview of the changed defaults for 6.1.

@brianjaustin
Copy link
Member Author

Thanks @Bilka2, good catch! Do you think that should go here, or a separate PR?

@Bilka2
Copy link
Contributor

Bilka2 commented Dec 5, 2023

I'd say it should be a separate PR and issue, since all the changed config defaults will need to be looked into for whether they should be overridden, which is not a small task.

@brianjaustin brianjaustin deleted the AO3-6640-settings-clean branch January 27, 2024 16:37
@brianjaustin brianjaustin restored the AO3-6640-settings-clean branch January 27, 2024 18:05
@brianjaustin brianjaustin deleted the AO3-6640-settings-clean branch January 27, 2024 18:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants