When working in a large rails app, I noticed in a flamegraph of a particular request that ~68ms was spent in the `xml_name_escape` method. I also ran an allocation tracer, which showed this method at the top of the list for String allocations.
This patch updates this method to avoid 3 String allocations:
- 2 allocations saved by using gsub! instead of gsub
- 1 allocation saved by concatenating to an existing string instead of allocating a new output string
For a rough benchmark in our rails app, I wrote a test with an allocation tracer around rendering a small view component.
- 244 String allocations before this change
- 228 String allocations from switching to gsub!
- 220 String allocations with this full patch
A ~10% reduction in String allocations in a real-world example seemed like a good justification for this small change.
This moves some of the documentation from the `ErrorReporter` class
directly to the `handle` and `record` methods, and fleshes those out.
This also tweaks the documentation of the other `ErrorReporter` methods.
For redis cache store, running `Rails.cache.delete_multi(["foo", "bar"])`
will lead to an error when redis is down. Other existing cache store methods
already implement failure safety.
Instead we lazily check for pipelining support. `Redis::Distributed`
immediately raise when `pipelined` is called.
Also instead of using `mset` we use a pipelined. The performance
difference is extremly minimal, but it allows us to re-use `write_entry`
hence more shared code and we can speed up `write_multi` with an
expiry.
In #30893, the `credentials:edit` command was changed to prevent saving
invalid YAML:
```yaml
# some_invalid_yaml.yml
secret_key_base: ...
- new_key: new value
```
```console
$ EDITOR='cp some_invalid_yaml.yml' bin/rails credentials:edit
ruby-3.1.2/lib/ruby/3.1.0/psych.rb:455:in `parse': (<unknown>): did not find expected key while parsing a block mapping at line 3 column 1 (Psych::SyntaxError)
$ bin/rails credentials:show
secret_key_base: ...
```
However, throwing away user input is not ideal. Such behavior can be
particularly troublesome when copying secrets from ephemeral sources.
This commit changes the `credentials:edit` command to always save user
input, and display a helpful warning when saving invalid YAML:
```console
$ EDITOR='cp some_invalid_yaml.yml' bin/rails credentials:edit
File encrypted and saved.
WARNING: Invalid YAML in '/path/to/app/config/credentials.yml.enc'.
(/path/to/app/config/credentials.yml.enc): did not find expected key while parsing a block mapping at line 3 column 1
Your application will not be able to load 'config/credentials.yml.enc' until the error has been fixed.
$ bin/rails credentials:show
# some_invalid_yaml.yml
secret_key_base: ...
- new_key: new value
```
This commit also applies the same fix to the `encrypted:edit` command.
Prior to this commit, if `config.require_master_key` was set to true in
`config/application.rb`, the `credentials:edit` command could not
automatically generate a master key file:
```ruby
# In config/application.rb
config.require_master_key = true
```
```console
# No previously existing credentials
$ rm config/master.key config/credentials.yml.enc
```
```console
$ bin/rails credentials:edit
activesupport/lib/active_support/encrypted_file.rb:118:in `handle_missing_key': Missing encryption key to decrypt file with. Ask your team for your master key and write it to config/master.key or put it in the ENV['RAILS_MASTER_KEY']. (ActiveSupport::EncryptedFile::MissingKeyError)
```
This commit adds a `EncryptedFile#key?` method that can be used to check
for the existence of a key without raising `MissingKeyError`, and the
`credentials:edit` command now uses this method:
```console
$ bin/rails credentials:edit
Adding config/master.key to store the encryption key: ...
Save this in a password manager your team can access.
If you lose the key, no one, including you, can access anything encrypted with it.
create config/master.key
```
This commit also applies the same fix to the `encrypted:edit` command.
Prior to this commit, `EncryptedFile` would internally memoize when a
key file was non-existent. This meant that a key file generated after
checking `encrypted_file.key.nil?` would not be recognized, unless a new
`EncryptedFile` instance was used. (Though setting the key in a
designated environment variable instead would entirely bypass this
memoization.)
This commit changes `EncryptedFile` to only memoize when the key file
has been read. Consequently, this commit memoizes the `EncryptedFile`
(or, specifically, `EncryptedConfiguration`) instance used by
`CredentialsCommand`.
This commit also adds more test coverage around key file generation for
`CredentialsCommand`.
We should not need to check defined? here because we are only interested
in whether @html_safe is truthy or falsy.
We can use an aliased attr_reader to make this even faster by skipping
both method dispatch.
Co-authored-by: Jean Boussier <jean.boussier@gmail.com>
Although Ruby provides `Base64.urlsafe_encode64` and
`Base64.urlsafe_decode64` methods, the technical term is "URL-safe",
with "URL" and "safe" as separate words.
For better readability, this commit renames the `:urlsafe` option for
`MessageEncryptor` and `MessageVerifier` to `:url_safe`.
with some additional changes made later:
1. Flip the condition for backward compatibility
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
2. Improve custom behavior test
Co-authored-by: Jonathan Hefner <jonathan@hefner.pro>
3. Fix indentation
This adds a `:urlsafe` option to the `MessageEncryptor` constructor.
When enabled, this option ensures that messages use a URL-safe encoding.
This matches the `MessageVerifier` `:urlsafe` option added in #45419.
Follow-up to #43924.
This commit refactors the logic around assembling and extracting the
parts of a message (namely: the encrypted data, the IV, and the auth
tag). It also provides a small but reproducible performance increase
for a roundtrip.
Benchmark:
```ruby
require "benchmark/ips"
require "active_support/message_encryptor"
DATA = "x" * 100
ENCRYPTOR = ActiveSupport::MessageEncryptor.new(SecureRandom.random_bytes(32))
Benchmark.ips do |x|
x.report("roundtrip") do
ENCRYPTOR.decrypt_and_verify(ENCRYPTOR.encrypt_and_sign(DATA))
end
end
```
Before:
```
Warming up --------------------------------------
roundtrip 1.342k i/100ms
Calculating -------------------------------------
roundtrip 13.525k (± 1.5%) i/s - 68.442k in 5.061532s
```
After:
```
Warming up --------------------------------------
roundtrip 1.409k i/100ms
Calculating -------------------------------------
roundtrip 14.125k (± 1.4%) i/s - 71.859k in 5.088419s
```
The `test_urlsafe` test could not fail when `urlsafe: false` because the
serialized input data did not contain a bit sequence that would encode
to a non-URL-safe character. The `test_no_padding` *could* fail when
`urlsafe: false`, except when the default serializer uses JSON, because
the serialized input data would be a multiple of 3 bytes, thus not
requiring any padding.
This commit replaces those tests with a falsifiable test using a
carefully chosen input string.
In minitest/minitest@6e06ac9 minitest changed such that it now accepts
`kwargs` instead of requiring kwargs to be shoved into the args array.
This is a good change but required some updates to our test code to get
the new version of minitest passing.
Changes are as follows:
1) Lock minitest to 5.15 for Ruby 2.7. We don't love this change but
it's pretty difficult to get 2.7 and 3.0 to play nicely together with
the new kwargs changes. Dropping 2.7 support isn't an option right
now for Rails. This is safe because all of the code changes here are
internal methods to Rails like assert_called_with. Applications
shouldn't be consuming them as they are no-doc'd.
2) Update the `assert_called_with` method to take any kwargs but also
the returns kwarg.
3) Update callers of `assert_called_with` to move the kwargs outside the
args array.
4) Update the message from marshaled exceptions. In 5.16 the exception
message is "result not reported" instead of "Wrapped undumpable
exception".
Co-authored-by: Matthew Draper <matthew@trebex.net>
urlsafe option was introduced to MessageVerifier in
09c3f36a96 but it can generate strings
containing padding character ("=") which is not urlsafe.
Fix not to pad when base64 encode.
MessageVerifier uses Base64.strict_encode64 and generated strings are
not urlsafe. Though the goal is to make MessageVerifier generated
strings urlsafe, We can not simply switch to Base64.urlsafe_encode64
because it will be a breaking change. Thus, as a first step, urlsafe
option is added to the MessageVerifier initializer.
The `assert_called_with` helper allows passing a multi-dimensional array to
mock multiple calls to the same method for a given block. This works
fine now, but when adding support for real kwargs arguments to line up with
recent upgrades in Minitest, this approach is no longer workable because
we can't pass multiple sets of differing kwargs.
Rather than complicated this method further, this commit removes the
multi-call form of `assert_called_with` and modifies the tests that
currently make use of that functionality to just use the underlying
`Minitest::Mock` calls.
Co-authored-by: Eileen M. Uchitelle <eileencodes@gmail.com>
Ian C. Anderson