Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce Actionable Errors #34788

Merged
merged 8 commits into from Apr 19, 2019
Merged

Introduce Actionable Errors #34788

merged 8 commits into from Apr 19, 2019

Conversation

@gsamokovarov
Copy link
Contributor

@gsamokovarov gsamokovarov commented Dec 25, 2018

The idea of actionable errors has been floating around for a few years. First
introduced by @vipulnsward in #26542 (thanks Vipul) and later on expanded
by @causztic and myself in this year's GSoC. The current implementation is the
result of the lessons we learnt from the previous ones and, I hope, provides
the simplest API and implementation.


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:

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. 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. An error can have multiple actions as well.

The actions are dispatched by simple forms. No XHR's or any JS involved in
the process. The current page address is remembered and if the action was
successful, the dispatching middleware will issue a redirect back to it.

This is because to make an "action progress" page we'd need to either:

  1. Define an interface of the returned values of the action class macro.
  2. Capture STD{OUT,ERR} and display it.
  3. Probably something I'm missing... 馃槄

In any case, I think we can avoid all of the manutia around progress display and
do a simple post submission for now.

Here are a few screenshots of the error pages of actionable errors:

The builtin pending migrations error is now actionable

screen shot 2018-12-25 at 16 17 23

A custom actionable error with multiple actions

screen shot 2018-12-25 at 16 17 41

When an action fails

screen shot 2018-12-25 at 16 19 15

@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch from afd77bb to 726fc8f Dec 25, 2018
Copy link
Member

@robin850 robin850 left a comment

Awesome ! 馃帀

activesupport/test/actionable_error_test.rb Outdated Show resolved Hide resolved
@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch from 726fc8f to 90dc382 Dec 25, 2018
@simi
Copy link
Contributor

@simi simi commented Dec 25, 2018

What's empty actionpack/lib/action_dispatch/middleware/templates/rescues/_actions.text.erb for?

@nynhex
Copy link

@nynhex nynhex commented Dec 26, 2018

^great holiday pr

Copy link
Member

@gmcgibbon gmcgibbon left a comment

This is looking great @gsamokovarov! Really excited to see this in Rails 6!! 馃憦

@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch 2 times, most recently from aea4fa3 to 6dcdd3e Dec 26, 2018
gsamokovarov added a commit to gsamokovarov/rails that referenced this pull request Dec 26, 2018
The `#csrf_meta_tags` and `#token_tag` Action View helper methods are
expecting the class in which are included to explicitly define the
method `#protect_against_forgery?` or else they will fail with
`NoMethodError`.

This is a problem if you want to use Action View outside of Rails
applications. For example, in rails#34788 I used the `#button_to` helper
inside of the error pages templates that have a custom
`ActionView::Base` subclass, which did not defined
`#protect_against_forgery?` and trying to call the button failed.

I had to dig inside of Action View to find-out what's was going on. I
think we should either set a default method implementation in the
helpers or check for the method definition, but don't explicitly require
the presence of `#protect_against_forgery?` in every `ActionViews::Base`
subclass as the errors are hard to figure out.
gsamokovarov added a commit to gsamokovarov/rails that referenced this pull request Dec 27, 2018
The `#csrf_meta_tags` and `#token_tag` Action View helper methods are
expecting the class in which are included to explicitly define the
method `#protect_against_forgery?` or else they will fail with
`NoMethodError`.

This is a problem if you want to use Action View outside of Rails
applications. For example, in rails#34788 I used the `#button_to` helper
inside of the error pages templates that have a custom
`ActionView::Base` subclass, which did not defined
`#protect_against_forgery?` and trying to call the button failed.

I had to dig inside of Action View to find-out what's was going on. I
think we should either set a default method implementation in the
helpers or check for the method definition, but don't explicitly require
the presence of `#protect_against_forgery?` in every `ActionViews::Base`
subclass as the errors are hard to figure out.
gsamokovarov added a commit to gsamokovarov/rails that referenced this pull request Dec 27, 2018
The `#csrf_meta_tags` and `#token_tag` Action View helper methods are
expecting the class in which are included to explicitly define the
method `#protect_against_forgery?` or else they will fail with
`NoMethodError`.

This is a problem if you want to use Action View outside of Rails
applications. For example, in rails#34788 I used the `#button_to` helper
inside of the error pages templates that have a custom
`ActionView::Base` subclass, which did not defined
`#protect_against_forgery?` and trying to call the button failed.

I had to dig inside of Action View to find-out what's was going on. I
think we should either set a default method implementation in the
helpers or check for the method definition, but don't explicitly require
the presence of `#protect_against_forgery?` in every `ActionViews::Base`
subclass as the errors are hard to figure out.
@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch from 6dcdd3e to e4a7885 Dec 27, 2018
@matthewd
Copy link
Member

@matthewd matthewd commented Dec 27, 2018

I'm disappointed to leave behind both stateful actions and any progress/output channel, but getting a solid foundation in sounds great.

@rails-bot rails-bot bot added the railties label Dec 28, 2018
@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch 3 times, most recently from 36eaef8 to 65e47ac Jan 1, 2019
@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch 2 times, most recently from 1d3e589 to 03ba01e Jan 15, 2019
@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch from 03ba01e to 84754f7 Feb 3, 2019
@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch from 6e28560 to 7b43d22 Feb 6, 2019
@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch from 7b43d22 to 30e4dd1 Feb 18, 2019
* Introduce `ActionDispatch::ActionableExceptions`.

The `ActionDispatch::ActionableExceptions` middleware dispatches actions
from `ActiveSupport::ActionableError` descendants.

This comment has been minimized.

@vipulnsward

vipulnsward Feb 19, 2019
Member

We can probably expand on what this does though, or what the end goal is that we get out of this?

This comment has been minimized.

@vipulnsward

vipulnsward Feb 19, 2019
Member

I see we actually expand this below, probably a one line note here would be great.

@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch from 30e4dd1 to 8af39ad Mar 1, 2019
Copy link
Member

@kaspth kaspth left a comment

I'm liking this a lot! Let me know if you need more reviews, I'll help you get this in for Rails 6 鉂わ笍

include ActiveSupport::ActionableError

action "Run pending migrations" do
ActiveRecord::Tasks::DatabaseTasks.migrate

This comment has been minimized.

@kaspth

kaspth Mar 10, 2019
Member

I don't think this location should know about the DatabaseTasks. How about we let action accept a command option instead? E.g.:

action "Run pending migrations", command: "db:migrate"

def action(name, command: nil, &block)
  _actions[name] = block || -> { Rails::Command.invoke command }
end

Although then Active Support knows about Rails commands鈥 馃槃

This comment has been minimized.

@kaspth

kaspth Apr 18, 2019
Member

Let's deal with this part later. I do like the idea that other libraries could extend action with their own behaviors ala:

# Somewhere in railties
ActiveSupport::ActionableErrors.register_behavior :command do |command|
  Rails::Command.invoke command
end

# Back in Active Support:
def action(name, **behaviors, &block)
  _actions[name] = block || extract_behavior(behaviors)
end

def extract_behavior(behaviors)
  if match = (behaviors.keys & self.class.behaviors.keys).first
    callable, value = self.class.behaviors[match], behaviors[match]

    -> { callable.call(value) }
  end
end

Hm, but then we'll have load order dependencies鈥 馃槄 Well, this was fun to think about! I still think we should find a way such that Migration doesn't know about the migrate task, but that's later.

This comment has been minimized.

@gsamokovarov

gsamokovarov Apr 19, 2019
Author Contributor

Hey, yeah, I like that we can execute arbitrary code here now and we're not explicitly coupled with commands, but they are kinda easy to invoke if that's your desire.

module ActionableError
extend Concern

NonActionable = Class.new(StandardError)

This comment has been minimized.

@kaspth

kaspth Mar 10, 2019
Member

I thought we used the class NonActionable < StandardError; end style?

This comment has been minimized.

@gsamokovarov

gsamokovarov Apr 18, 2019
Author Contributor

Okay, will change this.

# module and invoke the +action+ class macro to define the action. An action
# needs a name and a block to execute.
module ActionableError
extend Concern

This comment has been minimized.

@kaspth

kaspth Mar 10, 2019
Member

Should we put autoload :ActionableError beneath Concern instead and skip the require? Because this organization just nullifies the Concern autoload.

This comment has been minimized.

@gsamokovarov

gsamokovarov Apr 18, 2019
Author Contributor

Seems good!

request = ActionDispatch::Request.new(env)
return @app.call(env) unless actionable_request?(request)

ActiveSupport::ActionableError.dispatch(request.params["error"], request.params["action"])

This comment has been minimized.

@kaspth

kaspth Mar 10, 2019
Member

I'll just reference #35489 in passing, though I'm still not sure if it changes anything for this.

actionpack/CHANGELOG.md Outdated Show resolved Hide resolved
actionpack/CHANGELOG.md Show resolved Hide resolved
@gsamokovarov
Copy link
Contributor Author

@gsamokovarov gsamokovarov commented Apr 11, 2019

Oh, @kaspth, haven't seen your round of comments. I will take a look at them and respond back in a bit.

@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch from 8af39ad to 82a56df Apr 19, 2019
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.
@gsamokovarov gsamokovarov force-pushed the gsamokovarov:actionable-errors branch from 82a56df to feaaa75 Apr 19, 2019
@gsamokovarov
Copy link
Contributor Author

@gsamokovarov gsamokovarov commented Apr 19, 2019

@kaspth, @vipulnsward Sorry for taking me so long to get back to you. Can you folks take a "final" (final, final 馃槀) look?

@kaspth kaspth merged commit 10da0a2 into rails:master Apr 19, 2019
2 checks passed
2 checks passed
buildkite/rails Build #60592 passed (11 minutes, 1 second)
Details
codeclimate All good!
Details
@palkan
Copy link
Contributor

@palkan palkan commented Apr 19, 2019

馃帀 馃帀 馃帀

@gsamokovarov
Copy link
Contributor Author

@gsamokovarov gsamokovarov commented Apr 19, 2019

鉁岋笍

@l33z3r
Copy link

@l33z3r l33z3r commented Mar 21, 2021

Guys, I just checked my production logs and somebody crafted a request:

POST /rails/actions?error=ActiveRecord::PendingMigrationError&action=Run%20pending%20migrations&location=%0djavascript:alert(1)//%0aaaaaa

They were able to trigger an attempt at running migrations on my production environment.
Is this a massive security hole?

@gsamokovarov
Copy link
Contributor Author

@gsamokovarov gsamokovarov commented Mar 22, 2021

What rails version are you running on and do you happen to have config.consider_all_requests_local turned on in production? After #40689 the middleware should not be in the production middleware stack as well but this will happen in Rails 7.

@l33z3r
Copy link

@l33z3r l33z3r commented Mar 22, 2021

I am running rails 6.0.2.2. I do not have config.consider_all_requests_local turned on in production. Seems that pull request will solve the issue. I just manually resolved it by removing the middleware in application.rb. thanks

@rafaelfranca
Copy link
Member

@rafaelfranca rafaelfranca commented Mar 22, 2021

That security hole was fixed in 6.0.3.2. https://github.com/advisories/GHSA-c6qr-h5vq-59jc. You should make sure your Rails version is up to date because that is not the only security issue that you have open in your app.

@l33z3r
Copy link

@l33z3r l33z3r commented Apr 6, 2021

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet