https://github.com/rails/rails/pull/43209 immediately rejects
the request if no password is passed, but there are legitimate
uses for accepting authentication without a password.
Calling `skip_forgery_protection` without first calling
`protect_from_forgery`--either manually or through default
settings--raises an `ArgumentError` because `verify_authenticity_token`
has not been defined as a callback.
Since Rails 7.0 adds `skip_forgery_protection` to the
`Rails::WelcomeController` (PR #42864), this behavior means that setting
`default_protect_from_forgery` to false and visiting the Rails Welcome
page (`/`) raises an error.
This behavior also created an issue for `ActionMailbox` that was
previously fixed in the Mailbox controller by running
`skip_forgery_protection` only if `default_protect_from_forgery` was
true (PR #35935).
This PR addresses the underlying issue by setting the `raise` option for
`skip_before_action` to default to false inside
`skip_forgery_protection`.
The fix is implemented in `request_forgery_protection.rb`. The change to
`ActionMailbox`'s `base_controller.rb` removes the now-unnecessary
check of `default_protect_from_forgery`.
The tests added in `request_forgery_protection_test.rb` and
`routing_test.rb` both raise an error when run against the current
codebase and pass with the changes noted above.
Modern browsers don't render this HTML so it goes unused in practice.
The delivered bytes are therefore a small waste (although very small)
and unnecessary and could be optimized away.
Additionally, the HTML fails validation. Using the W3C v.Nu, we see the
following errors:
Warning: Consider adding a lang attribute to the html start tag to declare the language of this document.
Error: Start tag seen without seeing a doctype first. Expected <!DOCTYPE html>.
Error: Element head is missing a required instance of child element title.
These errors may surface in site-wide compliance tests (either internal
tests or external contractual tests). Avoid the false positives by
removing the HTML.
While these warnings and errors could be resolved, it would be simpler
on future maintenance to remove the body altogether (especially as it
isn't rendered by the browser). As the same string is copied around a
few places, this removes multiple touch points to resolve the current
validation errors as well as new ones.
Many other frameworks and web servers don't include an HTML body on
redirect, so there isn't a reason for Rails to do so. By removing the
custom Rails HTML, there are fewing "fingerprints" that a malicious bot
could use to identify the backend technologies.
Application controllers that wish to add a response body after calling
redirect_to can continue to do so.
Fix: https://github.com/rails/rails/issues/44496
It's really unfortunate, but since thread locals were copied
since a decade and we moved most of them into IsolatedExecutionState
we now need to copy it too to keep backward compatibility.
However I think it's one more sign that AC::Live should be
rethought.
Running the Action Pack tests outputs a warning:
./actionpack/test/controller/test_case_test.rb:1007: warning: instance variable @counter not initialized
Surrounding the line with silence_warnings cleans up the output.
In previous versions of Rails, a dynamic regex was built to find templates.
After that, PathParser started to be used to both match and sort templates.
With the dynamic regex, templates with lowdash locales (es_AR) were
found properly. But the PathParser regex does not match locales with this
format, only allowing dash (es-AR) or no dash (es). Templates with lowdash
locales have a wrong virtual path and get filtered.
In this commit the PathParser regex is extended to support the lowdash.
`params` contains `@logging_context` in its instance to notify
unpermitted parameters including the context through Rails
Instrumentation API. However, the logging context disappeared when
`params` is updated with some methods, such as `require`, `slice`,
`merge`, etc, so the subscriber of `unpermitted_parameters` could not
get the information.
This patch tries to initialize `Parameters` with `@logging_context`
where it makes sense to pass the information. The following methods will
be affected with this patch:
* `require`
* `deep_dup`
* `slice`
* `except`
* `extract!`
* `transform_values`
* `transform_keys`
* `deep_transform_keys`
* `select`
* `reject`
* `compact`
* `merge`
* `reverse_merge`
`ActionController::TestCase` keeps a `@controller` instance variable, which represents the controller being tested. At the end of each request inside a test, its [params and format](https://github.com/rails/rails/blob/main/actionpack/lib/action_controller/metal/testing.rb) are reset. But any other instance variables set in the test aren't reset. This creates a problem if you do something like this:
```ruby
class UserController
def show
@user ||= User.find(params[:id])
render plain: @user.name
end
end
```
```ruby
test "gets the user" do
get :show, params: { id: users(:one).id }
assert "one", response.body
get :show, params: { id: users(:two).id }
assert "two", response.body
end
```
The second assertion will fail, because `@user` won't be re-assigned in the second test (due to `||=`). This example is a bit contrived, but it shows how instance variables persisting between requests can lead to surprising outcomes.
This PR fixes this by clearing all instance variables that were created on the controller while a request was processed.
It explicitly excludes instance variables that were created *before* any requests were run. And it leaves the instance variable around until the *next* request in the test. This means that you can still do this:
```ruby
test "gets the user" do
@controller.user = users(:one) # assuming `Controller#user` users an ivar internally, you can set the ivar here...
get :show_current
assert "one", response.body
assert_equal users(:one), @controller.user # and you can read the ivar here
end
```
This allows `rescue_from` to be used to add a default fallback route:
```ruby
rescue_from ActionController::Redirecting::UnsafeRedirectError do
redirect_to root_url
end
```
Co-Authored-By: Chris Oliver <excid3@gmail.com>
Let's us separate what's location generation and what's protection: fewer
arguments, avoids overloading safe, and polluting response_options.
The exception message has been clarified a bit too.
The word "Crazy" has long been associated with mental illness. While
there may be other dictionary definitions, it's difficult for some of us
to separate the word from the stigmatization, gaslighting, and bullying
that often comes along with it.
This commit replaces instances of the word with various alternatives. I
find most of these more focused and descriptive than what we had before.
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).
[OWASP](https://owasp.org/www-project-secure-headers/#x-xss-protection)
suggests setting this header to '0' to disable the default behaviour
on old browsers as it can introduce additional security issues.
Added the new behaviour as a framework default from Rails 7.0.
Before, if a user set `file_fixture_path` to the same path as
`fixture_path`, this would have resulted in a deprecation warning such
as:
"Please modify the call from `fixture_file_upload('rails.jpg')` to
`fixture_file_upload('rails.jpg')"
This commit introduces a new `Journey::Ast` class that wraps the root
node of an ast. The purpose of this class is to reduce the number of
walks through the ast by taking a single pass through each node and
caching values for later use.
To avoid retaining additional memory, we clear out these ast objects
after eager loading.
Benefits
---
* Performance improvements (see benchmarks below)
* Keeps various ast manipulations together in a single class, rather
than scattered throughout
* Adding some names to things will hopefully make this code a little
easier to follow for future readers
Benchmarks
---
We benchmarked loading a real routes file with > 3500 routes.
master was at a9336a67b0 when we ran these. Note that these benchmarks
also include a few small changes that we didn't include in this commit,
but that we will follow up with after this gets merged - these
additional changes do not change the benchmarks significantly.
Time:
```
master - 0.798 ± 0.024 (500 runs)
this branch - 0.695 ± 0.029 (500 runs)
```
Allocations:
```
master - 980149 total allocated objects
this branch - 931357 total allocated objects
```
Stackprof:
Seeing `ActionDispatch::Journey::Visitors::Each#visit` more frequently
on the stack is what led us down this path in the first place. These
changes seem to have done the trick.
```
master:
TOTAL (pct) SAMPLES (pct) FRAME
52 (0.5%) 52 (0.5%) ActionDispatch::Journey::Nodes::Node#symbol?
58 (0.5%) 45 (0.4%) ActionDispatch::Journey::Scanner#scan
45 (0.4%) 45 (0.4%) ActionDispatch::Journey::Nodes::Cat#type
43 (0.4%) 43 (0.4%) ActionDispatch::Journey::Visitors::FunctionalVisitor#terminal
303 (2.7%) 43 (0.4%) ActionDispatch::Journey::Visitors::Each#visit
69 (0.6%) 40 (0.4%) ActionDispatch::Routing::Mapper::Scope#each
this commit:
TOTAL (pct) SAMPLES (pct) FRAME
82 (0.6%) 42 (0.3%) ActionDispatch::Journey::Scanner#next_token
31 (0.2%) 31 (0.2%) ActionDispatch::Journey::Nodes::Node#symbol?
30 (0.2%) 30 (0.2%) ActionDispatch::Journey::Nodes::Node#initialize
```
See also the benchmark script in https://github.com/rails/rails/pull/39935#issuecomment-887791294
Co-authored-by: Eric Milford <ericmilford@gmail.com>
As of Ruby 2.7 DidYouMean is included as a default gem, so there is no
need to check if DidYouMean is defined in the test suite. We still need
to check if the DidYouMean modules are defined in the actual code, as
someone might run Rails with DidYouMean disabled by using the
`--disable-did_you_mean` flag. This is ussually done for performance
reasons.
This commit also includes some of the changes made by Yuki in:
https://github.com/rails/rails/pull/39555
These changes include replacing Jaro with the more accurate
SpellChecker, and using DidYouMean::Correctable for simplere
corrections.
The DidYouMean::SpellChecker does have a treshold for corrections.
If there is not enough similarity it might not return a suggestion.
To stop the tests from failing some test data had to be changed.
For example, `non_existent` does not meet the treshold for `hello`, but
`ello` does:
DidYouMean::SpellChecker.new(dictionary: %w[hello]).correct('non_existent')
=> []
DidYouMean::SpellChecker.new(dictionary: %w[hello]).correct('ello')
=> ["hello"]
The treshold makes sense for spelling errors. But maybe we should add a
different SpellChecker that helps to get a suggestion even if there is
little overlap. For example for when a model only has 2 attributes
(title and body), it's helpful to get a suggestion for `name`
Co-Authored-By: Yuki Nishijima <yk.nishijima@gmail.com>
IE 6-7-8 has bug when 'Cache-Control: no-cache' head would break file download.
It's been fixed in IE9 (which is also ancient and barely used version).
Some extra details [here][1] and [here][2].
The way this fix is implemented clashes with what's been done in PR #40324. By
adding ':public' key to cache control header set it wipes default headers.
Since IE 6-7-8 are ancient browsers - let's just get rid of this hack.
[1]: https://stackoverflow.com/q/9766639/843067
[2]: https://stackoverflow.com/q/3415370/843067
Ref: https://github.com/rails/rails/pull/42020
It's not uncommon to want to forward request parameters,
as such the `:params` paramter of `url_for` often receive
an AC::Parameters instance.
When specifying numeric parameters, strong params lets you permit them all using the same permitted params for each.
For example params like,
```ruby
book: {
authors_attributes: {
'0': { name: "William Shakespeare", age_of_death: "52" },
'1': { name: "Unattributed Assistant" },
'2': "Not a hash",
'new_record': { name: "Some name" }
}
}
```
can be permitted with,
```
permit book: { authors_attributes: [ :name ] }
```
This returns the name keys for each of the numeric keyed params that have a name field,
```ruby
{ "book" => { "authors_attributes" => { "0" => { "name" => "William Shakespeare" }, "1" => { "name" => "Unattributed Assistant" } } } }
```
This is exactly what you want most of the time. Rarely you might need
to specify different keys for particular numeric attributes. This
allows another strong params syntax for those cases where you can
specify the keys allowed for each individual numerically keys attributes
hash.
After this change using the same params above, you can permit the name and age for only the `0` key and only the name for the `1` key,
```ruby
permit book: { authors_attributes: { '1': [ :name ], '0': [ :name, :age_of_death ] } }
```
This returns exactly the parameters that you specify,
```ruby
{ "book" => { "authors_attributes" => { "0" => { "name" => "William Shakespeare", "age_of_death" => "52" }, "1" => { "name" => "Unattributed Assistant" } } } }
```
Sidenote: this allows `permit` to do the equivalent to
```ruby
params.require(:book).permit(authors_attributes: { '1': [:name]})
```
without raising when `book: ... ` is not present.
The simpler syntax should be preferred, but in cases where you need more control, this is a nice option to have.
Summary
=======
Currently there is no way to set "Cache-Control: no-store" header using
built-in cache control methods ("expires_now"/"expires_in"/etc..). One of
the [top StackOverflow][1] answers currently suggests putting it directly
into header set.
Unfortunately, it cannot later be overridden in specific/individual actions by
calling say 'expires_in 5.minutes'. Resulting header in that case is
stays the same, i.e. 'Cache-Control: no-store'.
This:
1. Adds the 'no_store' method to set "Cache-Control: no-store" header.
2. Changes cache control "merge and normalize" code so default "no-store"
directive can be overridden using built in cache control methods mentioned
above.
What's the use of it
--------------------
Couple examples:
* To [prevent rendering stale content][3] if browser return button is used
('expires_now' does not help).
* To prevent browser disk cache being used. In some situations it's considered
a [privacy/security risk][4].
Other Information
=================
Mozilla developer docs for [Cache-Control][2] header.
[1]: https://stackoverflow.com/questions/10744169/rails-set-no-cache-method-cannot-disable-browser-caching-in-safari-and-opera
[2]: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control
[3]: https://engineering.mixmax.com/blog/chrome-back-button-cache-no-store/
[4]: https://portswigger.net/kb/issues/00700100_cacheable-https-response
This replaces the controller/action method of finding a path with the lookup_context which should always find the same thing as the render method finds.
