If an app sets the `"Vary"` header in an action to a value other than `"Accept"`, the `_set_vary_header` method will reset it's value to `"Accept"`.
This commit checks the header to be sure that it does not already have a value before setting the header to `"Accept"`.
Previously, when generating a root path from a route having nested
optional scopes where second-level or greater scopes had a static part,
and when providing only a second-level option, it would result in an
invalid path leading with "//". Ex. a path generated from the route
"(:a)(foo/:b)", given `b: 'bar'`, would return "//foo/bar".
Commit 7670d60977 introduced this bug while fixing the edge case of
empty strings returned when no options are passed to such routes.
A previous commit fixed this for simpler hierarchical cases like
"(:a)(:b)(:c)", and this fixes it for "(:a)(foo/:b)(bar/:c)".
Previously, when generating a root path from a route having nested
optional scopes, and when providing an option, would result in an
invalid path leading with "//". For example, the route
"(:locale)(:region)", given `locale: 'es'`, would output "//es".
Commit 7670d60977 introduced this bug while fixing the edge case of
empty strings returned when no options are passed to such routes.
This commit fixes both cases.
For example, HasManyThroughSourceAssociationNotFoundError uses
to_sentence to create its message, but was hardcoded to use the `en`
locale. In cases where this locale did not exist, this caused the
exception to be suppressed in favour of an I18n::InvalidLocale
exception.
This change removes the hardcoded locale, which allows I18n to use its
default locale.
In #36996, the behavior of `content_type=` has been changed to treat a
`content_type` itself as a mime type if a `content_type` doesn't contain
`charset=...` part.
i.e.
```ruby
response.content_type = "text/html; fragment"
response.media_type # => "text/html; fragment"
response.content_type = "text/html; fragment; charset=utf-16"
response.media_type # => "text/html"
```
That is tricky and strange.
I think that we cannot distinguish whether extra part is a part of mime
type or not for now. So at least in Rails 6.0, we should not discard
extra part conservatively as before.
Request headers provided by servers are ASCII-8BIT encoded. This commit sets up the `ActionDispatch::TestRequest::DEFAULT_ENV` headers so that they are likewise encoded as ASCII-8BIT.
I'm not sure a better way to describe this, but we need to set extra
stuff in the Content-Type header and we're doing it via Mime
registration so that we can have the "html_fragment" method on the
respond_to type. This worked in Rails 5.2, but it's broken in 6.0
In the future, I think we should probably give `register` a block that
takes the response object so we can do more interesting and complex
stuff without doing weird string manipulation. But this patch fixes the
issue we have today.
We had two tests which assigned IO.console.winsize (to ensure output was
consistent), however it's possible for IO.console to be nil.
This commit makes these tests stub IO.console_size directly (the method
we actually call, we shouldn't have been relying on that calling
IO.console.winsize anyways) or passes the width when initializing the
class.
This allows tests to run without a TTY. This can be tested with ex.
ssh localhost "cd src/rails/actionpack && bundle exec rake"
or
(setsid bundle exec rake) </dev/null |& cat
These calls to `content_type` were triggering the deprecation from
c631e8d011 in upgraded applications.
We can use `media_type` in all of these cases to avoid the deprecation.
This hack prevails everywhere in the codebase by being copy & pasted, and it's actually not a negative thing but a necessary thing for framework implementors,
so it should better have a name and be a thing.
And with this commit, activesupport/test/abstract_unit.rb now doesn't silently autoload AS::TestCase,
so we're ready to establish clearner environment for running AS tests (probably in later commits)
In most cases it works now without explicit require because it's accidentally required through
active_support/core_ext/date_and_time/calculations.rb where we still call `try`,
but that would stop working if we changed the Calculations implementation and remove the require call there.
* Reproduce error caused by malformed parameters
Error:
RequestFormat#test_format_does_not_throw_exceptions_when_invalid_POST_parameters:
ActionDispatch::Http::Parameters::ParseError: 765: unexpected token at '{record:{content:24.12.1.146}}'
/Users/weppos/Mirrors/rails/actionpack/lib/action_dispatch/http/parameters.rb:113:in `rescue in parse_formatted_parameters'
/Users/weppos/Mirrors/rails/actionpack/lib/action_dispatch/http/parameters.rb:107:in `parse_formatted_parameters'
/Users/weppos/Mirrors/rails/actionpack/lib/action_dispatch/http/request.rb:360:in `block in POST'
/Users/weppos/.rvm/gems/ruby-2.3.3/gems/rack-2.0.3/lib/rack/request.rb:57:in `fetch'
/Users/weppos/.rvm/gems/ruby-2.3.3/gems/rack-2.0.3/lib/rack/request.rb:57:in `fetch_header'
/Users/weppos/Mirrors/rails/actionpack/lib/action_dispatch/http/request.rb:359:in `POST'
/Users/weppos/Mirrors/rails/actionpack/lib/action_dispatch/http/parameters.rb:53:in `parameters'
/Users/weppos/Mirrors/rails/actionpack/lib/action_dispatch/http/mime_negotiation.rb:62:in `block in formats'
/Users/weppos/.rvm/gems/ruby-2.3.3/gems/rack-2.0.3/lib/rack/request.rb:57:in `fetch'
/Users/weppos/.rvm/gems/ruby-2.3.3/gems/rack-2.0.3/lib/rack/request.rb:57:in `fetch_header'
/Users/weppos/Mirrors/rails/actionpack/lib/action_dispatch/http/mime_negotiation.rb:60:in `formats'
/Users/weppos/Mirrors/rails/actionpack/test/dispatch/request_test.rb:891:in `block in <class:RequestFormat>'
See GH-29983
* Capture parameter parsing error output and test it
This change prevents the log to be displayed in the tests.
Moreover, the assertion against the debug ensures that
the test effectively triggers the parsing error as expected.
* Use a generic value in the test
* Switch to assert_match
[Simone Carletti + Rafael Mendonça França]
Problem description (quoted from @rafaelfranca's excellent explanation in https://github.com/rails/jquery-ujs/issues/318#issuecomment-88129005):
> Let say that we requested /tasks/1 using Ajax, and the previous page has the same url. When we click the back button the browser tries to get the response from its cache and it gets the javascript response. With vary we "fix" this behavior because we are telling the browser that the url is the same but it is not from the same type what will skip the cache.
And there's a Rails issue discussing about this problem as well https://github.com/rails/rails/issues/25842
Also, according to [RFC 7231 7.1.4](https://tools.ietf.org/html/rfc7231#section-7.1.4)
> An origin server SHOULD send a Vary header field when its algorithm
> for selecting a representation varies based on aspects of the request
> message other than the method and request target
we should add `Vary: Accept` header when determining content based on the `Accept` header.
Although adding such header by default could cause unnecessary cache invalidation. But this PR only adds the header if:
- The format param is not provided
- The request is a `xhr` request
- The request has accept headers and the headers are valid
So if the user
- sends request with explicit format, like `/users/1.json`
- or sends a normal request (non xhr)
- or doesn't specify accept headers
then the header won't be added.
See the discussion in https://github.com/rails/rails/issues/25842 and
https://github.com/rails/rails/pull/36213 for more details.
- According to the HTTP 1.1 spec, the 307 redirection guarantees that
the method and the body will not be changed during redirection.
This PR fixes that since follow_redirect! would always follow the
redirection my making a GET request.
Ref https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/307
- I made a change in https://github.com/rails/rails/pull/36691 to
delegate route helper to a proxy class.
This didn't take into account that the `url_options` we redefine
in SystemTest would be ignored.
This PR fixes that by definin the url_options inside the proxy
- https://github.com/rails/rails/pull/36283 made a change to
make SystemTest inherits from ActiveSupport::TestCase instead
of ActionDispatch::IntegrationTest.
With this change, the route helpers are now directly included inside
the SystemTest class. This causes an edge case in case you have a
routes whos name starts with `test_`, minitest will consider it as a
test and will try to run it ab39d35fb4/lib/minitest/test.rb (L66)
This PR uses a proxy and deleted missing method to a dummy class
that has all the route helpers.
A HTTP feature policy is Yet Another HTTP header for instructing the
browser about which features the application intends to make use of and
to lock down access to others. This is a new security mechanism that
ensures that should an application become compromised or a third party
attempts an unexpected action, the browser will override it and maintain
the intended UX.
WICG specification: https://wicg.github.io/feature-policy/
The end result is a HTTP header that looks like the following:
```
Feature-Policy: geolocation 'none'; autoplay https://example.com
```
This will prevent the browser from using geolocation and only allow
autoplay on `https://example.com`. Full feature list can be found over
in the WICG repository[1].
As of today Chrome and Safari have public support[2] for this
functionality with Firefox working on support[3] and Edge still pending
acceptance of the suggestion[4].
#### Examples
Using an initializer
```rb
# config/initializers/feature_policy.rb
Rails.application.config.feature_policy do |f|
f.geolocation :none
f.camera :none
f.payment "https://secure.example.com"
f.fullscreen :self
end
```
In a controller
```rb
class SampleController < ApplicationController
def index
feature_policy do |f|
f.geolocation "https://example.com"
end
end
end
```
Some of you might realise that the HTTP feature policy looks pretty
close to that of a Content Security Policy; and you're right. So much so
that I used the Content Security Policy DSL from #31162 as the starting
point for this change.
This change *doesn't* introduce support for defining a feature policy on
an iframe and this has been intentionally done to split the HTTP header
and the HTML element (`iframe`) support. If this is successful, I'll
look to add that on it's own.
Full documentation on HTTP feature policies can be found at
https://wicg.github.io/feature-policy/. Google have also published[5] a
great in-depth write up of this functionality.
[1]: https://github.com/WICG/feature-policy/blob/master/features.md
[2]: https://www.chromestatus.com/feature/5694225681219584
[3]: https://bugzilla.mozilla.org/show_bug.cgi?id=1390801
[4]: https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/33507907-support-feature-policy
[5]: https://developers.google.com/web/updates/2018/06/feature-policy
Older versions of selenium had driver_path on
::Selenium::WebDriver::Chrome directly, not on Service. This avoids
errors on those old versions and will preload properly if webdrivers is
installed.
The webdrivers gem configures Selenium::WebDriver::Service.driver_path
as a proc which updates the web drivers and returns their path.
This commit introduces SystemTesting::Browser#preload, which runs this
proc early. This ensures that webdrivers update is run before forking
for parallel testing, but doesn't explicitly tie us to that gem (and I
think anything configured as driver_path probably makes sense to
eager-load).
that is enabled by a new environment variable - RAILS_SYSTEM_TESTING_SCREENSHOT_HTML=1
Add the ability to call `take_screenshot` more than once in a single test by prefixing the name of
the image file with a counter that is incremented on every `take_screenshot` call. This allows a
developer to see their pages in sequence when trying to debug test errors. This does not affect
the failure case where the prefix remains 'failures'
I changed to set CSP nonce to `style-src` directive in #32932.
But this causes an issue when `unsafe-inline` is specified to `style-src`
(If a nonce is present, a nonce takes precedence over `unsafe-inline`).
So, I fixed to nonce directives configurable. By configure this, users
can make CSP as before.
Fixes#35137.
I changed return value of `ActionDispatch::Response#content_type` in #36034.
But this change seems to an obstacle to upgrading. https://github.com/rails/rails/pull/36034#issuecomment-498795893
Therefore, I restored the behavior of `ActionDispatch::Response#content_type`
to 5.2 and deprecated old behavior. Also, made it possible to control the
behavior with the config.
I frequently find myself having to .compact but for blank. which means
on an array reject(&:blank?) (this is fine), or,
on a hash `.reject { |_k, v| v.blank? }` which is slightly more
frustrating and i usually write it as .reject(&:blank?) first and am
confused when it's trying to check if the keys are blank.
I've added the analagous .compact_blank! where there's a reject! to
build on (there's also a reject! in Set, but there's no other core_ext
touching Set so i've left that alone)
Since #35709, `Response#conten_type` returns only MIME type correctly.
It is a documented behavior that this method only returns MIME type, so
this change seems appropriate.
39de7fac05/actionpack/lib/action_dispatch/http/response.rb (L245-L249)
But unfortunately, some users expect this method to return all
Content-Type that does not contain charset. This seems to be breaking
changes.
We can change this behavior with the deprecate cycle.
But, in that case, a method needs that include Content-Type with
additional parameters. And that method name is probably the
`content_type` seems to properly.
So I changed the new behavior to more appropriate `media_type` method.
And `Response#content_type` changed (as the method name) to return Content-Type
header as it is.
Fixes#35709.
[Rafael Mendonça França & Yuuji Yaginuma ]
Each time a missing route is hit 32 lines of internal rails traces
are written to the log. This is overly verbose and doesn't offer
any actionable information to the user.
With this change we'll still write an error message showing the
route error but the trace will be omitted.
After @kamipo CR feedback we realized `Route#build` wasn't used. As it
is also private API being able to create Routes both with `#new` and
`#build` was redundant.
This commit changes from constructor's argument list to keyword
arguments in order to remove the dependency of remember parameters'
positions.
It also unifies all parameters extracted from the `scope` into
`scope_params`, which also takes care of providing the default values
for them.
This commit changes from constructor's argument list to keyword
arguments in order to remove the dependency of remember parameters'
positions.
The constructor already provided a default value for `internal`, this
commits takes the chance to also add default values for `precedence` and
`scope_options`.
When a route was defined within an optional scope, if that route didn't
take parameters the scope was lost when using path helpers. This patch
ensures scope is kept both when the route takes parameters or when it
doesn't.
Fixes#33219
Previously calling `ActionController::Parameters#transform_keys/!`
without passing a block would return an enumerator for the underlying
hash, which was inconsistent with the behaviour when a block was passed:
ActionController::Parameters.new(foo: "bar").transform_keys { |k| k }
=> <ActionController::Parameters {"foo"=>"bar"} permitted: false>
ActionController::Parameters.new(foo: "bar").transform_keys.each { |k| k }
=> {"foo"=>"bar"}
An enumerator for the parameters is now returned instead, ensuring that
evaluating it produces another parameters object instead of a hash:
ActionController::Parameters.new(foo: "bar").transform_keys.each { |k| k }
=> <ActionController::Parameters {"foo"=>"bar"} permitted: false>
In CookieJar.build, the name `hash` is used as block parameter name
for tap method.
However, it is actually not hash but a CookieJar's instance.
The name `hash` was confusing, so replace with `jar`.
Inherit from ActiveSupport::TestCase instead of ActionDispatch::IntegrationTest. Active Job automatically mixes its test helper into the latter, forcibly setting the test queue adapter before Capybara starts its app server.
As a bonus, we no longer need to remove the parts of the ActionDispatch::IntegrationTest API we don’t want to expose.
test for non-numeric key in nested attributes
test: extra blank line between tests removed
test for non-numeric key fixed (by Daniel)
Update according to feedback
`IntegrationTest#open_session` uses `dup` to create the new session,
which meant it had its own copy of `@assertions`. This prevented the
assertions from being correctly counted and reported.
Child sessions now have their `attr_accessor` overriden to delegate to
the root session.
The instrumentation proxy adds three stack frames per-middleware, even
when nothing is listening.
This commit, when the middleware stack is built, only adds
instrumentation when the `process_middleware.action_dispatch` event has
already been subscribed to.
The advantage to this is that we don't have any extra stack frames in
apps which don't need middleware instrumentation.
The disadvantage is that the subscriptions need to be in place when the
middleware stack is built (during app boot). I think this is likely okay
because temporary AS::Notifications subscriptions are strongly
discouraged.
Because controllers' `perform_caching` config is `true` by default, it
means we actually enable the caching in all those tests implicitly (and
it works). Which also means we can avoid repeatedly declaring that and
just specify it once in the setup method (just for declaration).
Previously we were calling the `take_failed_screenshot` method in an
`after_teardown` hook. However, this means that other teardown hooks
have to be executed before we take the screenshot. Since there can be
dynamic updates to the page after the assertion fails and before we
take a screenshot, it seems desirable to minimize that gap as much as
possible. Taking the screenshot in a `before_teardown` rather than an
`after_teardown` helps with that, and has a side benefit of allowing
us to remove the nested `ensure` commented on here:
https://github.com/rails/rails/pull/34411#discussion_r232819478
Currently ActionController::API doesn't include Caching module, so it
can't perform caching. And even if users include it later manually, it
won't inherit application's default cache store for action_controllers.
So the only way to solve this issue is to include Caching module in
ActionController::API, too.
This closes#35602
Actionable errors let's you dispatch actions from Rails' error pages. This
can help you save time if you have a clear action for the resolution of
common development errors.
The de-facto example are pending migrations. Every time pending migrations
are found, a middleware raises an error. With actionable errors, you can
run the migrations right from the error page. Other examples include Rails
plugins that need to run a rake task to setup themselves. They can now
raise actionable errors to run the setup straight from the error pages.
Here is how to define an actionable error:
```ruby
class PendingMigrationError < MigrationError #:nodoc:
include ActiveSupport::ActionableError
action "Run pending migrations" do
ActiveRecord::Tasks::DatabaseTasks.migrate
end
end
```
To make an error actionable, include the `ActiveSupport::ActionableError`
module and invoke the `action` class macro to define the action. An action
needs a name and a procedure to execute. The name is shown as the name of a
button on the error pages. Once clicked, it will invoke the given
procedure.
Unless I'm missing some undocumented use case, these modules aren't
needed in `StrongParameters` anymore since 8e221127ab. Also, all
actionpack tests are passing without them.
This change adds a `named_captures` method to
`ActionDispatch::Journey::Path::MatchData` in order to emulate a similar
method present on `Regex`'s `MatchData` present in Ruby core.
This method can be useful for introspection of routes without the need
to use `zip` while testing or developing in Rails core.
Previously, when using `render file:`, it was possible to render files
not only at an absolute path or relative to the current directory, but
relative to ANY view paths. This was probably done for absolutely
maximum compatibility when addressing CVE-2016-0752, but I think is
unlikely to be used in practice.
Tihs commit removes the ability to `render file:` with a path relative
to a non-fallback view path.
Make FallbackResolver.new private
To ensure nobody is making FallbackResolvers other than "/" and "".
Make reject_files_external_... no-op for fallbacks
Because there are only two values used for path: "" and "/", and
File.join("", "") == File.join("/", "") == "/", this method was only
testing that the absolute paths started at "/" (which of course all do).
This commit doesn't change any behaviour, but it makes it explicit that
the FallbackFileSystemResolver works this way.
Remove outside_app_allowed argument
Deprecate find_all_anywhere
This is now equivalent to find_all
Remove outside_app argument
Deprecate find_file for find
Both LookupContext#find_file and PathSet#find_file are now equivalent to
their respective #find methods.
* Fix typo in the value of ENCRYPTED_SIGNED_COOKIE_SALT constant
* Fix value of cookie in with authenticated encryption flag off in tests
[Emil Shakirov + Ryuta Kamizono]
The previous behaviour of render file: was essentially the same as
render template:, except that templates can be specified as an absolute
path on the filesystem.
This makes sense for historic reasons, but now render file: is almost
exclusively used to render raw files (not .erb) like public/404.html. In
addition to complicating the code in template/resolver.rb, I think the
current behaviour is surprising to developers.
This commit deprecates the existing "lookup a template from anywhere"
behaviour and replaces it with "render this file exactly as it is on
disk". Handlers will no longer be used (it will render the same as if
the :raw handler was used), but formats (.html, .xml, etc) will still be
detected (and will default to :plain).
The existing render file: behaviour was the path through which Rails
apps were vulnerable in the recent CVE-2019-5418. Although the
vulnerability has been patched in a fully backwards-compatible way, I
think it's a strong hint that we should drop the existing
previously-vulnerable behaviour if it isn't a benefit to developers.
- https://github.com/rails/rails/pull/35604 introduced a vulnerability fix
to raise an error in case the `HTTP_ACCEPT` headers contains malformated
mime type.
This will cause applications to throw a 500 if a User Agent sends an
invalid header.
This PR adds the `InvalidMimeType` in the default `rescue_responses` from
the ExceptionWrapper and will return a 406. I looked up the HTTP/1.1
RFC and it doesn't stand what should be returned when the UA
sends malformated mime type. Decided to get 406 as it seemed to be the
status the better suited for this.
When building the cookie_jar for the current test request.
It was possible for this method to override keys currently being set on the test itself.
In situations such as when making two requests mixing creating the cookie on the test and the controller.
This allows mime types in the form text/html, text/*, or */*
This required a few minor test/code changes where previously nil was
used as a mime string.
If the secret_key_base is nil in dev or test generate a key from random
bytes and store it in a tmp file. This prevents the app developers from
having to share / checkin the secret key for dev / test but also
maintains a key between app restarts in dev/test.
[CVE-2019-5420]
Co-Authored-By: eileencodes <eileencodes@gmail.com>
Co-Authored-By: John Hawthorn <john@hawthorn.email>
This commit ensures that locals are passed in to the template objects
when they are constructed, then removes the `locals=` mutator on the
template object. This means we don't need to mutate Template objects
with locals in the `decorate` method.
Carlos Antonio da Silva