In commit b3f570512, we changed the key generator hash digest class, and
we wrote:
> Since we haven't seen any Consul Democracy applications using
> encrypted messages and these messages become invalid with this change
> (...)
We didn't realize that ActiveStorage also used the old hash digest class
to generated the signed URLs used to access an image. This doesn't
affect us when we generate images using `image.variant`, because that
generates a new URL on the fly using the new hash digest class. However,
URLs referencing the images generated using the old hash digest class,
like the ones in the HTML content generated with CKEditor, would result
in 404 errors.
So we're rotating the signed IDs generated by earlier versions of
ActiveStorage. This way both new and old images will be correctly
displayed.
Note that, unlike cookies, which will keep working once rotated even if
we delete the code to rotate them, old ActiveStorage URLs will always
need the code rotating them in order to keep working.
As mentioned in commits 5311daadf and bb958daf0, using links combined
with JavaScript to generate POST (or, in this case, DELETE) requests to
the server has a few issues.
We're keeping the old `apply_stylesheet_media_default` option behavior
because removing `media="screen"` from our stylesheets would completely
break our `print` stylesheet, which would now load the default the
styles defined in `application.css`.
We're also keeping the old `:mini_magick` option to process images so
existing installations don't have to install libvips on their server. We
might change it in the future.
This option was deprecated in Rails 7.0 and removed in Rails 7.1 [1]. It
doesn't really affect us because we weren't using `to_s` with a
parameter anywhere in the application.
The Rubocop rule Rails/ToSWithArgument can be used to detect these cases
but, since we've never used them, and adding them now would cause the
application to crash and so it'll be obvious we've done something wrong,
I don't think it's necessary to add the rule.
[1] https://github.com/rails/rails/commit/e420c3380
This doesn't really affect us because we don'thave any multiple file
inputs in the application, but we're enabling it because it's the new
default configuration option.
Setting it to `true` was deprecated in Rails 7.0 and the option was was
removed in Rails 7.1, so in Rails 7.1 applications it isn't possible to
set it to `true` [1]. So we're setting it to `false` now.
[1] https://github.com/rails/rails/commit/689b27773
According to the Rails configuration guide [1], with this format, Rails
serializes cache entries more efficiently. Most importantly:
> All formats are backward and forward compatible, meaning cache entries
> written in one format can be read when using another format. This
> behavior makes it easy to migrate between formats without invalidating
> the entire cache.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-active-support-cache-format-version
The only change between these headers and the ones sent by Rails 6.1
application is that now the `X-XSS-Protection` header is set to zero. As
mentioned in the pull request introducing the change [1]:
> This header has been deprecated and the XSS auditor it triggered has
> been removed from all major modern browsers (in favour of Content
> Security Policy) that implemented this header to begin with (Firefox
> never did).
[1] Pull request 41769 in https://github.com/rails/rails
This configuration option disappeared in Rails 7.1 [1] (meaning it isn't
possible to set it to `false` in a Rails 7.1 application). Since it's
going to be our only option when upgrading to Rails 7.1, we're already
activating it now.
[1] https://github.com/rails/rails/commit/7b4affc78
As mentioned in the Rails configuration documentation [1] (note the link
points to the configuration guide for Rails 7.1, but only because the
documentation for this option wasn't as good in the configuration guide
for Rails 7.0; the behavior hasn't changed between these two versions),
this was done in the `wrap_parameters` initializer but now it can be
done using a new default configuration option.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-action-controller-wrap-parameters-by-default
This way we'll add an extra layer of protection from attacks that might
cause our application to redirect to an external host.
There's one place where we're allowing redirects to external hosts,
though: administrators can link external resources in notifications, and
we're redirecting to them after marking the notification as read.
Since the tests for the remote translations controller were
(accidentally) using an external redirect, we're updating them to use a
relative URL.
As mentioned in the Rails pull request [1], the main reason for partial
inserts is no longer relevant thanks to the `ignored_columns` method
(which we haven't even needed so far).
I don't have a preference regarding this setting; we're enabling it in
order to reduce the number of settings we customize.
[1] Pull request 42769 in https://github.com/rails/rails
It looks like we can't really benefit from this rule because usually we
need to specify the option anyway (maybe `user has_many :comments` is
one of the few exceptions). We might make some changes in the code when
Rubocop changes its Rails/InverseOf rule so it doesn't report this case
when using Rails 7, but, until then, we aren't changing anything so we
don't have to deal with false positives in Rubocop.
Before Rails 7.0 was released, neither the Mail gem or Rails were
providing a default timeout for SMTP, so there was a risk of processes
being stuck while sending emails.
That's no longer the case, though; we're using version 2.8.x of the Mail
gem, which already provides a default timeout [2].
Since the default timeout provided by the Mail gem is the same as the
default timeout provided by Rails 7.0, it doesn't matter whether we
enable this option. We're enabling because it's easier to just use the
default 7.0 configuration.
[1] Issue 41244 in https://github.com/rails/rails
[2] Pull request 1427 in https://github.com/mikel/mail
Not sure whether this affects us since we use RSpec; in any case, if it
affects us, it seems like a good idea, although we'll have to watch
whether some tests start failing more often.
We aren't getting any warnings when running our test suite, which means
that gems that depended on this method (like graphql [1]) have already
added compatibility for this case.
[1] Pull request 3774 in https://github.com/rmosolgo/graphql-ruby/
This is similar to what we did in commit 00a5dc921 when upgrading to
Rails 5.2. Quoting from that commit:
> Note this change will cause all fragment caching to expire. We
> consider it acceptable considering the page where caching is most
> important (stats) is barely affected by this change, since this change
> only affects the view, and the time-consuming operations are cached in
> the model.
>
> Comments are actually affected, though, and pages with thousands of
> comments might take a few extra seconds to load the first time they're
> accessed after this change. We don't think this is going to be an
> issue on existing CONSUL installations.
Note that enabling this options means all encrypted messages and cookies
generated the application become invalid, so we're adding a cookie
rotator in order to keep sessions from expiring when upgrading the
application, as recommended in the "Upgrading Ruby on Rails" guideline
[1].
Since we haven't seen any Consul Democracy applications using encrypted
messages and these messages become invalid with this change, we're also
removing the pre-Rails 5.2 encryption to authenticate messages
(AES-256-CBC) and switching to the default one since Rails 5.2
(AES-256-GCM). Since the configured encryption is used by the cookie
rotator initializer (through the ActiveSupport::MessageEncryptor.key_len
method), at first I thought this might affect the cookie rotator, but it
doesn't: upgrading works as expected, and existing sessions are still
active.
I'm adding a comment to remove the initializer once all cookies have
been migrated. I've added "Rails 7.1" in the comment because we usually
check for these comments when upgrading Rails, but we rarely check for
them when after releasing new versions of Consul Democracy.
[1] https://guides.rubyonrails.org/v7.0/upgrading_ruby_on_rails.html#key-generator-digest-class-changing-to-use-sha256
In Rails 6.1 and earlier, `button_to` generated a <button> tag when it
received the content as a block, but an <input> tag when receiving
the content as the first parameter.
That's why we were using blocks with `button_to` most of the time; for
starters, <button> tags accept pseudocontent and so are easier to style.
In Rails 7.0, `button_to` always generates a <button> tag [1], so we're
simplifying the code what uses `button_to`, passing the content as a
first parameter instead of passing it as a block.
[1] https://guides.rubyonrails.org/v7.1/configuring.html#config-action-view-button-to-generates-button-tag
The config.file_watcher option still exists but it's no longer included
in the default environtment file. Since we don't use it, we're removing
it.
The config.assets.assets.debug option is no longer true by default [1],
so it isn't included anymore.
The config.active_support.deprecation option is now omitted on
production in favor of config.active_support.report_deprecations, which
is false by default. I think it's OK to keep it this way, since we check
deprecations in the development and test environments but never on
production environments.
As mentioned in the Rails upgrade guide, sprockets-rails is no longer a
rails dependency and we need to explicitly include it in our Gemfile.
The behavior of queries trying to find an invalid enum value has changed
[2], so we're updating the tests accordingly.
The `favicon_link_tag` method has removed the deprecated `shortcut`
link type [3], so we're updating the tests accordingly.
The method `raw_filter` in ActiveSupport callbacks has been renamed to
`filter` [4], so we're updating the code accordingly.
[1] https://github.com/rails/rails/commit/adec7e7ba87e3
[2] https://github.com/rails/rails/commit/b68f0954
[3] Pull request 43850 in https://github.com/rails/rails
[4] Pull request 41598 in https://github.com/rails/rails
The `database:` argument from the `connected_to` method was deprecated
in Rails 6.1 [1] and removed in Rails 7.0 [2]. The ros-apartement gem
has already introduced fixes for this issue [3][4], but there has been
no release including these fixes.
So we're applying the fix in our code.
Note that, since Apartment already overwrites the `connected_to` method
by creating a `connected_to_with_tenant` method, we're calling the
`connected_to_without_tenant` method inside the patch, which is
equivalent to ActiveRecord's original `connected_to`.
[1] Pull request 37874 in https://github.com/rails/rails
[2] Pull request 40530 in https://github.com/rails/rails/pull
[3] Pull request 194 in https://github.com/rails-on-services/apartment
[4] Pull request 243 in https://github.com/rails-on-services/apartment
These warnings appear in the logs in the development environment, and,
with Rails 7, the application will crash. When running the tests, they
would appear in the standard error ouput if we set `config.cache_classes
= false` in the test environment but, since that isn't the case, they
don't.
To reproduce these warnings (or the lack of them), start a Rails console
in development and check the log/development.log file.
Since we're already setting `wordpress_oauth2` using the `option :name`
command in the `OmniAuth::Strategies::Wordpress` class, Devise can
automatically find the strategy. However, it wasn't working because we
were passing a string instead of a symbol.
Even though we don't load this file with Zeitwerk, we're doing it for
consistency.
If we tried to load this file using Zeitwerk, without this change we'd
get an error:
```
NameError: uninitialized constant OmniauthWordpress
```
We were getting a few errors when trying out Zeitwerk:
```
expected file lib/sms_api.rb to define constant SmsApi
expected file app/components/layout/common_html_attributes_component.rb
to define constant Layout::CommonHtmlAttributesComponent
```
In these cases, we aren't using an inflection because we also define the
`Verification::SmsController` and a few migrations containing `Html` in
their class name, and none of them would work if we defined the
inflection.
We were also getting an error regarding classes containing WYSIWYG in
its name:
```
NameError: uninitialized constant WYSIWYGSanitizer
Did you mean? WysiwygSanitizer
```
In this case, adding the acronym is easier, since we never use "Wysiwyg"
in the code but we use "WYSIWYG" in many places.
We're getting one warning when compiling the assets due to the code we
use from font-awesome-sass, and a lot of warnings due to the code we use
from foundation.
Since these warnings are very annoying, and we get them both when
deploying and every time we change an SCSS file in development, we're
silencing them.
I haven't found the way to pass the `quiet_deps` option to the Sprockets
processor, so I'm monkey-patching the Sass engine instead.
This is possible since audited 5.4.3, and IMHO it's useful because
audited has added a callback on touch, which could result in strange
behavior since we might use the `touch` method to expire the cache
without doing any changes.
While this doesn't affect our application because we're using the `on:`
option of the `audited` method in the budget investment model, it'll
make it easier to enable audited for other classes in custom
installations without having to worry about the (usually undesired)
touch callbacks.
We haven't updated this initializer for years, so here's the updated
version. The `expire_auth_token_on_timeout` doesn't seem to exist
anymore, and a few more options have been added.
Note that the default Devise initializer configures
`config.responder.error_status` and `config.responder.redirect_status`
so they follow Hotwire/Turbo conventions. For now, I'm commenting these
lines because we currently don't use Hotwire/Turbo.
We haven't been updating this file for a long time so, and this version
uses the `configure` method instead of the `config` hash.
We're also adding a few comments already included in previous versions
of wicked pdf that we hadn't copied to our initializer.
As Rails does with the application log and other tools. We
are going to use the same filtering rules we use in Consul Democracy.
We are renaming the initializer file name `filter_parameter_logging.rb` so
it's loaded before the errbit initializer.
In order to the display a warn text on the last attempt
before the account is locked, we need update
config.paranoid to false as the devise documentation
explains.
Adding "config.paranoid: false" implies further changes
to the code, so for now we unncomment the default value
"config.last_attempt_warning = true" and update it to false.
As it seems that adding complexity to the password is something that
might be wanted from the Consul applications, we added the necessary
changes to allow it.
In this version we simply:
- Uncomment the configuration variable "password_complexity"
- Set this variable without any restrictions
- Adapt the application so that everything still works normally.
One of the things that had to be done to adapt the application
was to remove the overwriting of the "self.included" method.
The original idea of overwriting the "self.included" method seems
to be the possibility of being able to overwrite the
:current_equal_password_validation validation.
The problem comes from the fact that by only calling that validation,
the rest of the validations that are defined (in this case
"password_complexity") are no longer applied.
It seems like a good idea to remove the overwrite of the "self.included"
method to allow all the defined validations to be applied and simply
overwrite the :current_equal_password_validation method so that
everything behaves the same.
:allow_passwords_equal_to_email configuration has been enabled too,
in order to allow existing records with this configuration.
Another change made was to uncomment the line:
and to keep everything working the same set the value to false:
config.email_validation = false.
This change has had to be made because in the documentation of
devise-security it says the following:
In other words, if we want to use the :secure_validatable module
we have to enable this configuration even if its value is "false".
If we kept the configuration variable commented out:
The following error appears:
"uninitialized constant Devise::Models::SecureValidatable::EmailValidator".
So it has been verified that if before making any change we
decommented the line and added the value of "false", the application
worked as normal.
In these commits ffe9ac70d8def3 we updated the devise-security version.
In these versions the 'password_regex' configuration key and some comments
were changed.
We update this file in order to use the new configuration key 'password_complexity' and keep comments updated.
We can remove the `new_framework_defaults_6_1` file by using Rails 6.1
default options and overwriting the one we haven't enabled.
We've experienced problems while running the tests (probably the same
would happen on production) when enabling the `has_many_inversing`
option. For example, after creating a legislation answer for a question
with no answers, calling `question.answers_count` would then return `2`
instead of `1`.
So we aren't enabling this option.
This is the default setting in Rails 6.1, and generates an extra tag in
the HTML which tells the browser to download and cache these files as
soon as possible, even before they're needed.
This might not be that relevant in our application, since on most pages
we only generate one CSS and one JS file. But it might make it easier to
move the `javascript_include_tag` statement to the bottom of the page in
the future if we detect that doing so increases performance.
