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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `redirect_back` and deprecate `redirect_to :back` #22506

Merged
merged 2 commits into from Dec 16, 2015

Conversation

@derekprior
Copy link
Contributor

derekprior commented Dec 5, 2015

redirect_to :back is a somewhat common pattern in Rails apps, but it
is not completely safe. There are a number of circumstances where HTTP
referrer information is not available on the request. This happens often
with bot traffic and occasionally to user traffic depending on browser
security settings.

When there is no referrer available on the request, redirect_to :back
will raise ActionController::RedirectBackError, usually resulting in
an application error.

redirect_to_back_or_default is a helper I have added to many rails
applications to avoid this situation and now I'm proposing it be added
to Rails proper and encouraged in place of redirect_to :back.

@rails-bot
Copy link

rails-bot commented Dec 5, 2015

r? @kaspth

(@rails-bot has picked a reviewer for you, use r? to override)

@simi
Copy link
Contributor

simi commented Dec 5, 2015

I think it is common pattern to use rescue_from in those situations.

class ApplicationController < ActionController::Base
  rescue_from ActionController::RedirectBackError, with: :redirect_to_default

  protected
  def redirect_to_default
    redirect_to root_url
  end
end
@claudiob
Copy link
Member

claudiob commented Dec 5, 2015

Hello and thanks for your contribution!

This happens often with bot traffic and occasionally to user traffic depending on browser
security settings.

You are making the point that redirects_to :back is not something you can rely on, and I agree with that.
For that reason, I have stopped using redirects_to :back altogether a while ago.

Could you provide a real case (with or without code) in which you would still need to redirect to the previous page if the referrer is present while, at the same time, redirect to a default page if not present?

That would be helpful to better understand the value of the PR. Thanks! 🎀

@derekprior
Copy link
Contributor Author

derekprior commented Dec 5, 2015

Thanks for the great questions @claudiob.

For that reason, I have stopped using redirects_to :back altogether a while ago.

I largely concur, but as a consultant exposed to dozens of applications, I continue to see redirect_to :back in app after app. In some cases it's simple enough to refactor to a static redirect, but in other cases :back with a sensible default is really the simplest thing that could possibly (very nearly always 😁) work.

Consider, for example, an application that manages products. Products can be viewed in many different contexts and from each of these views you are able to "Activate" of "Deactivate" the product. The controller for activations may look like this:

class ProductActivationsController < ActionController::Base
  def create
    Product.find(params[:product_id]).activate
    redirect_to :back, notice: "Product activated"
  end

  def destroy
    Product.find(params[:product_id]).deactivate
    redirect_to :back, notice: "Product deactivated"
  end
end

Because these controller actions can be called from various locations -- say from the admin products index, the admin product show, the store ui if you are an admin, or any number of various other places, the notion of "from wherever you came" is valuable. You can attempt to pass the return path explicitly as a parameter of the create and destroy action but then to avoid unvalidated redirects. It's a decent amount of code for a case when :back works well in all but a handful of cases.

If I were designing this solution in a clean room, I'd deprecate redirect_to :back and either rely on a gem for a smarter replacement or replace it with redirect_back_or_default so you would never get an exception.

Either way, I continue to see application that have dozens of not so straight-forward to solve ActionController::RedirectBackErrors in their error trackers. The simplest fix is generally to provide a default rather than the shotgun surgery that would be required to get the correct redirect for all occurrences. I'm not generally thrilled with this as a solution, but from a cost/benefit standpoint it tends to make a lot of sense.

I think it is common pattern to use rescue_from in those situations.

This can get out of hand if you have different defaults per action. It also relies on exceptions for flow control, which I'd prefer not to do.

@derekprior
Copy link
Contributor Author

derekprior commented Dec 5, 2015

All that said, I'd totally understand not accepting this PR, but then I'd suggest deprecating redirect_to :back.

@sgrif
Copy link
Contributor

sgrif commented Dec 6, 2015

I definitely agree that this should replace redirect_to :back with this. Our general philosophy has been to avoid constructs which could allow bad user input to make the server 500 trivially. rescue_from is definitely not sufficient.

That said, I'm not huge on the method name, it feels a bit long and unweildly to me.

@derekprior
Copy link
Contributor Author

derekprior commented Dec 6, 2015

I agree. If deprecating redirect_to :back is on the table then I'd suggest something like def redirect_back(default:, *args). But if the previous method isn't going to be deprecated then I think they are too near eachother.

@bogdan
Copy link
Contributor

bogdan commented Dec 6, 2015

Agree on deprecation of redirect_to :back. To the reasons mentioned above, I can add that passing magic symbols to a method seems bad API to me. rescue ActionController::RedirectBackError is bad idea when default differs per action.

@sgrif
Copy link
Contributor

sgrif commented Dec 6, 2015

I like redirect_back(default:, *args). I think deprecating redirect_to :back is a separate discussion (which I am in favor of), but we can use that method name regardless. My only other concern is that :default doesn't necessarily imply anything if I'm reading code and not familiar with this exact problem.

@simi
Copy link
Contributor

simi commented Dec 6, 2015

What about redirect_back(fallback: *args)

@sgrif
Copy link
Contributor

sgrif commented Dec 6, 2015

:fallback doesn't give much more context than :default. Maybe redirect_back(url_if_unknown:), though that's a bit verbose (maybe this needs to be verbose?)

@simi
Copy link
Contributor

simi commented Dec 6, 2015

I was about to avoid url or path as part of the argument name.

@derekprior
Copy link
Contributor Author

derekprior commented Dec 6, 2015

It's not always a URL. You could pass anything that worked with redirect_to. I think fallback is the best suggestion I've heard so far:

def create
  product = Product.find(params[:product_id]
  product.activate
  redirect_back(fallback: product, notice: "Activated")
end

In the context of redirect_back I think fallback is pretty clear. We could also use fallback_redirect?

@sgrif
Copy link
Contributor

sgrif commented Dec 6, 2015

I think that :fallback is only clear if you know how back works, and
understand that it may not be there. I don't think that we can assume that.

On Sun, Dec 6, 2015, 10:31 AM Derek Prior notifications@github.com wrote:

It's not always a URL. You could pass anything that worked with
redirect_to. I think fallback is the best suggestion I've heard so far:

def create
product = Product.find(params[:product_id]
product.activate
redirect_back(fallback: product, notice: "Activated")end

In the context of redirect_back I think fallback is pretty clear. We
could also use fallback_redirect?


Reply to this email directly or view it on GitHub
#22506 (comment).

@simi
Copy link
Contributor

simi commented Dec 6, 2015

@sgrif yes it is clear only if you know how :back works, but that should be explained in the documentation.

I think introducing of redirect_back, deprecation of redirect_to :back and leaving note in deprecation message and ActionController::RedirectBackError message will be great start for this feature.

@kaspth
Copy link
Member

kaspth commented Dec 13, 2015

r? @sgrif

@rails-bot rails-bot assigned sgrif and unassigned kaspth Dec 13, 2015
@sgrif
Copy link
Contributor

sgrif commented Dec 14, 2015

I think redirect_back(fallback:) is the way to go for now. @derekprior Can you update and rebase?

@derekprior derekprior force-pushed the derekprior:dp-redirect_to_back_or_default branch Dec 16, 2015
@derekprior
Copy link
Contributor Author

derekprior commented Dec 16, 2015

@sgrif updated and rebased.

@simi
Copy link
Contributor

simi commented Dec 16, 2015

No deprecation of redirect_to(:back) for now?

Anyway I think the documentation of redirect_to(:back) should mention potential dangers and link to this new method.

@sgrif
Copy link
Contributor

sgrif commented Dec 16, 2015

Let's just go ahead and deprecate, too

@derekprior
Copy link
Contributor Author

derekprior commented Dec 16, 2015

@sgrif Deprecation added.

@sgrif
sgrif reviewed Dec 16, 2015
View changes
actionpack/test/controller/redirect_test.rb Outdated
@@ -187,7 +191,11 @@ def test_relative_url_redirect_with_status_hash

def test_redirect_to_back_with_status
@request.env["HTTP_REFERER"] = "http://www.example.com/coming/from"
get :redirect_to_back_with_status

ActiveSupport::Deprecation.silence do

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 16, 2015

Contributor

These should all be assert_deprecated

@sgrif
sgrif reviewed Dec 16, 2015
View changes
actionpack/test/controller/redirect_test.rb Outdated
assert_response :redirect
assert_equal "http://www.example.com/coming/from", redirect_to_url
end

def test_redirect_to_back_is_deprecated

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 16, 2015

Contributor

We don't need an additional test here, the previous tests changed to assert_deprecated are enough.

@simi
simi reviewed Dec 16, 2015
View changes
actionpack/lib/action_controller/metal/redirecting.rb Outdated
ActiveSupport::Deprecation.warn(<<-MESSAGE.squish)
`redirect_to :back` is deprecated and will be removed from Rails 5.1.
Please use `redirect_back(fallback: fallback_location)` where
`fallback_location` represents the redirect to use if the request has

This comment has been minimized.

Copy link
@simi

simi Dec 16, 2015

Contributor

...represents the redirect Maybe location or url will be better to use here.

This comment has been minimized.

Copy link
@derekprior

derekprior Dec 16, 2015

Author Contributor

Yeah, I'll use location. It's not always a URL but rather something that will eventually be a URL. I think location is a better indicator of that.

This comment has been minimized.

Copy link
@sgrif

sgrif Dec 16, 2015

Contributor

What do you think about making the keyword argument fallback_location?

On Wed, Dec 16, 2015 at 8:15 AM Derek Prior notifications@github.com
wrote:

In actionpack/lib/action_controller/metal/redirecting.rb
#22506 (comment):

@@ -87,6 +103,12 @@ def _compute_redirect_to_location(request, options) #:nodoc:
when String
request.protocol + request.host_with_port + options
when :back

  •    ActiveSupport::Deprecation.warn(<<-MESSAGE.squish)
    
  •      `redirect_to :back` is deprecated and will be removed from Rails 5.1.
    
  •      Please use `redirect_back(fallback: fallback_location)` where
    
  •      `fallback_location` represents the redirect to use if the request has
    

Yeah, I'll use location. It's not always a URL but rather something that
will eventually be a URL. I think location is a better indicator of that.


Reply to this email directly or view it on GitHub
https://github.com/rails/rails/pull/22506/files#r47787127.

@derekprior derekprior force-pushed the derekprior:dp-redirect_to_back_or_default branch 2 times, most recently Dec 16, 2015
derekprior added 2 commits Dec 16, 2015
`redirect_to :back` is a somewhat common pattern in Rails apps, but it
is not completely safe. There are a number of circumstances where HTTP
referrer information is not available on the request. This happens often
with bot traffic and occasionally to user traffic depending on browser
security settings.

When there is no referrer available on the request, `redirect_to :back`
will raise `ActionController::RedirectBackError`, usually resulting in
an application error.

`redirect_back` takes a required `fallback_location` keyword argument
that specifies the redirect when the referrer information is not
available.  This prevents 500 errors caused by
`ActionController::RedirectBackError`.
Applications that use `redirect_to :back` can be forced to 500 by
clients that do not send the HTTP `Referer` (sic) header.
`redirect_back` requires the user to consider this possibility up front
and avoids this trivially-caused application error.
@derekprior derekprior force-pushed the derekprior:dp-redirect_to_back_or_default branch to dc4429c Dec 16, 2015
sgrif added a commit that referenced this pull request Dec 16, 2015
Add `redirect_to_back_or_default`
@sgrif sgrif merged commit 79ac05f into rails:master Dec 16, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@simi
Copy link
Contributor

simi commented Dec 17, 2015

thanks all 🎊

@derekprior derekprior changed the title Add `redirect_to_back_or_default` Add `redirect_back` and deprecate `redirect_to :back` Dec 17, 2015
@dazza-codes

This comment has been minimized.

Why is this not def redirect_back(fallback_location, **args)? Why the fallback_location: with the : after it?
Asking because rubocop complains about it (in a different project that has copy/pasted this method), like so:

app/controllers/application_controller.rb:84:39: E: unexpected token tCOMMA
(Using Ruby 2.0 parser; configure using TargetRubyVersion parameter, under AllCops)
  def redirect_back(fallback_location:, **args)
                                      ^

This comment has been minimized.

Copy link
Contributor

simi replied Feb 19, 2016

Using Ruby 2.0 parser...

Rails master supports Ruby 2.2.2+ only. You can force Ruby version to 2.2 for rubocop with this config:

AllCops:
    TargetRubyVersion: 2.2
shamozhixing pushed a commit to shamozhixing/gitlab-ce that referenced this pull request Aug 23, 2016
@derekprior derekprior deleted the derekprior:dp-redirect_to_back_or_default branch May 6, 2017
@emilebosch
Copy link

emilebosch commented May 26, 2017

Sorry im a bit sad about this. :( I like rails and i like that it optimized for happiness but this doesnt make me happy.

I'm ok with raises redirect errors, they are super loggable and rescuable. And then i know where this referer less stuff comes from.(ips, agents etcetc) But now i have to update all my code & rails engines etc etc. Am i missing something or does this really just do nothing except add work?

Why didn't you just made a comprise:

redirect_to :back, notice:"Yolo", fallback: '/'

And add a rails config that raises if fallback isn't present? But people that know what they are doing can just use whatever they want and put that config.raise_on_no_fallback = false

@juanantoniodamianv
Copy link

juanantoniodamianv commented Feb 23, 2018

Hi!
Can I check the value of :back, so that if it is a specific url, I will go back, and if not, go back to root?

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

10 participants
You can’t perform that action at this time.