Previously, the method always asserts the status is `:redirect` which
allows for any kind of 3XX response. However, sometimes it is worthwhile
to precise the status code of the redirect. For example, a Rails
application may want to verify the redirect is a 301 (Moved Permanently)
and not the default 302 (Found). The new method argument makes this
convenient to do in one assertion.
Rubinius has not been maintained since May 2020 and based on the
discussion at https://github.com/rails/rails/pull/44984 ,
I think we can remove Rubinius specific code from Rails.
Right now many helpers have to deal with two modes of operation to
capture view output.
The main one is to swap the `@output_buffer` variable with a new buffer.
But since some view implementations such as `builder` keep a reference
on the buffer they were initialized with, this doesn't always work.
So additionally, the various capturing helpers also record the buffer
length prior to executing the block, and then `slice!` the buffer back
to its original size.
This is wasteful and make the code rather unclear.
Now that `OutputBuffer` is a delegator, I'd like to refactor all this
so that:
- @output_buffer is no longer re-assigned
- A single OutputBuffer instance is used for the entire response rendering
- Instead capturing is done through `OutputBuffer#capture`
Once the above is achieved, it should allow us to enabled Erubi's
`:chain_appends` option and get some reduced template size and some
performance.
Not re-assigning `@output_buffer` will also allow template to access
the local variable instead of an instance variable, which is cheaper.
But more importantly, that should make the code easier to understand
and easier to be compatible with `StreamingBuffer`.
This commit just ensures we're green with the main branch of rack test.
The changes are things we should have done anyway, and are backwards
compatible with older versions of rack test
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>
In
f075be3dcb
did_you_mean and error_highlight now use `detailed_message` over
`message` to display errors.
For cases where we are testing `message`, in 3.2 and above we need to
test against `detailed_message` instead.
As far as I can tell in a Rails console when these errors are raised the
`detailed_message` is used so we shouldn't need to make other changes to
Rails. The only case where this isn't true is in the Railties changes -
we are explicitly formatting the did you mean message so we need to be
sure to call `detailed_message` here.
This fixes most of the failing tests for ruby-trunk.
Previously, as of 80aaa111884247e6aa17b7bbab268c7719847521,
ActionController::Parameters has defined hash as:
[@parameters.hash, @permitted].hash
Defining hash means that eql? must be defined, and eql? must be at
least as strict as the hash value generated. That is, for any two
objects which return a different hash value, `a.eql?(b)` should return
false. Otherwise, because hash values have a random seed added, and in
some cases have only some of their bits compared, their behaviour in a
hash becomes undefined. Previously we were breaking this expectation by
allowing a deprecated comparison between Parameters and a plain hash.
This commit fixes eql? to match hash, only returning true when the class
matches as well as the permitted? and parameters values (ie. eql? never
allows the deprecated relaxed equality branch).
This also adds the class to the hash and eql? check, which previously
wasn't there, which isn't strictly necessary to fix this but I think is
a best practice.
Since 5745a3c0928ee5604ce80af19348efb42189f1d6, if a controller defines
a `headers` method it will be called by this line, and the return value
will be mutated. This was also preventing the "Vary" header from being
sent to the client.
Co-authored-by: Oleksandr Bezruchenko <alex.bezruchenko@intercom.io>
Co-authored-by: Iliana Hadzhiatanasova <iliana.hadzhiatanasova@intercom.io>
Currently if you use `helper_method` to define a method, and inside that method you get an error, the backtrace is off by one line.
This PR fixes that so that the backtrace now points to the line where you called `helper_method`.
PR #23733 was supposed to deprecate and remove the ability to compare
Hash objects with AC::Parameters objects. Unfortunately it seems that
we still accidentally support that.
This PR adds a deprecation warning so that we can remove it in the
future.
Allow `ActionController::Parameters.to_h` to receive a block to provide
parity with `Hash#to_h`. The provided block recieves `key, value` and
yields a two-element array/keypair which can be transformed in the
resulting Hash.
https://ruby-doc.org/core-2.7.5/Hash.html#method-i-to_h
This has been broken since the logging context was added in
6be9c498bccd8dbc99b4b451841fcf73c7061d48
Also added a higher level test to ensure that this isn't broken again in
the future.
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.
