#3419: Fix unsafe redirect errors by using `allow_other_host` #3429

murny · 2024-03-29T16:05:26Z

This should be a quick fix to the following issues:

Both having to do with Rails 7 new logic to prevent "unsafe redirects" to external websites.

In the first issue, the external website appears to be ERA's domain such as https://era.library.ualberta.ca/items/3bcc9b7f-5b22-4cf4-ba4c-ede791eb3d1c/view/e35dcade-8076-487..., so perhaps maybe just need to make this recognized as a internal URL instead?

In the second case it's our OAuth flow so the website is redirecting to https://login.ualberta.ca/

Both of these seem safe (since we are setting these urls and its not coming from user input). So allow_other_host seems like a quick fix to resolve these issues.

But, curious if there is a better way to handle this? Like, could you "approvelist" these domains/urls? (Doesn't appear to be a way to do this from first glance).

Also, maybe we should more gracefully handle these errors in the future: rails/rails@c3758a7
But these seem to be the last redirect_to that could be "external" urls so maybe don't need this.

UPDATE: We ended up refactoring both use cases to just "redirect" better, so we shouldn't need the allow_other_host argument anymore. Also added a catch all that would just redirect back to root page if we somehow missed something.

pgwillia

This is a really elegant solution to a puzzle that had me stumped last week. Thanks for noticing the change in rails and following up on this! 🙏

pgwillia · 2024-04-05T22:14:49Z

app/controllers/application_controller.rb

@@ -83,7 +83,7 @@ def user_not_authorized
      # referer gets funky with omniauth and all the redirects it does,
      # so handle this sanely by ignoring any referer coming from omniauth (/auth/) path
      if request.referer&.exclude?('auth')
-        redirect_to request.referer
+        redirect_to(request.referer, allow_other_host: true)


request.referer could be user input. Do you have concerns about this with allow_other_host: true?

Yeah, we will have to look into what's the purpose of this. I believe its basically like the user went to a protected/login URL, and it will kick them back to where they came from and show them a "you must be logged in to see this" error flash message.

So if that is the case, then I think probably what we want is to use:

redirect_back_or_to(root_path) and all this logic goes away.

This method comes from Rails and has some built in logic that basically does what we want to do...but it will only redirect to request.referrer if its an internal URL (url from our application). If its an external URL it just takes them back to the homepage of our application.

So I think this would improve everything 🤔

I think you're right and redirect_back_or_to(root_path) is what we want to use.

I think the allow_other_host: true solution has some risk to it, especially in combination with request.referrer, which is why the default changed in Rails.

Will make this change 🙏

murny · 2024-04-25T17:43:19Z

app/controllers/application_controller.rb

@@ -118,7 +111,8 @@ def render_400(exception = nil)
  end

  def redirect_back_to
-    redirect_to session.delete(:forwarding_url) || session.delete(:previous_user_location) || root_path
+    redirect_to(session.delete(:forwarding_url) || session.delete(:previous_user_location) || root_path,


This one could probably be improved. We set both previous_user_location and forwarding_url in places.

previous_user_location is done via request.fullpath
forwarding_url is done via request.original_url

Both are the "current" url that was requested (so it should always be our url), so we really shouldn't need allow_other_host.

As an example the one problem is https://era.library.ualberta.ca/items/3bcc9b7f-5b22-4cf4-ba4c-ede791eb3d1c/view/e35dcade-8076-487... should be treated as an local URL and not an external URL. I assume this is being set via the forwarding_url using the request.original_url. But if it was instead being set with request.fullpath then this issue should all go away.

I think just removing forwarding_url and just using previous_user_location always (which is a relative path) should fix this problem. We won't need allow_other_host anymore 🤔

Will do some additional testing here to see if this is something we can do to simplifying everything.

I like that approach. Perhaps we could do this in combination with your suggestion above for adding

rescue_from ActionController::Redirecting::UnsafeRedirectError do redirect_to root_url end

The redirect should always be safe so we shouldn't see this, but at least it would avoid the 500 error if we're wrong?🙃

Yeah I like this idea! Let's do that as well. Will do some further testing here but I think this is a good solution and will simplify this code at the same time 👏

murny · 2024-04-30T00:10:10Z

app/controllers/application_controller.rb

    else
-      session[:forwarding_url] = request.original_url if request.get?
+      session[:forwarding_url] = request.fullpath if request.get?


We shouldn't need the full URL here, the path works just fine, and we use it for previous_user_location anyway, so now both places are doing the same thing (and hopefully this removes the issues with the full URL sometimes being treated as an external URL that we saw in Rollbar).

murny · 2024-04-30T00:15:55Z

app/controllers/application_controller.rb

@@ -118,7 +111,7 @@ def render_400(exception = nil)
  end

  def redirect_back_to
-    redirect_to session.delete(:forwarding_url) || session.delete(:previous_user_location) || root_path
+    redirect_to(session.delete(:forwarding_url) || session.delete(:previous_user_location) || root_path)


Alright lets break down this logic:

session[:forwarding_url] is for when you go to a protected URL (like `/profile) and you are not signed in, you will get thrown back to the main root page and asked to login.

When you login successfully, we will redirect you back to this protected URL like /profile.

session[:previous_user_location] stores the current page you are on before you went to the "login" page. So if I was viewing a particular page and I click the login button, after logging in, I hopefully will still be looking at the particular page I was viewing a few seconds ago.

So we need both of these things as they handle similar but different logic. Both of these are now relative urls. E.g we are storing / or /profile here and not full URLs. If any of these are set, then we will redirect to this location. And if not set, then we redirect to root_url. (I think previous_user_location will almost always be set though like 99% of the time, unless you just went directly to the login page which I suppose we would fallback to root_path here 🤔).

So we should never be dealing with external URLs here and hence no need for allow_other_host argument

murny · 2024-04-30T00:17:12Z

app/controllers/application_controller.rb

    return if read_only_mode_enabled?

    @current_user = user
-    session[:user_id] = user.try(:id)
-
-    # rubocop:disable Style/GuardClause


Saw this disable guard clause, so tried to refactor it.

Seems like we can just move the user check up. If we are given a user argument that is blank, then bail, otherwise @current_user is set with the user and everything else should just work fine now.

murny · 2024-04-30T00:18:45Z

app/controllers/application_controller.rb

@@ -15,6 +15,10 @@ class ApplicationController < ActionController::Base

  rescue_from JupiterCore::SolrBadRequestError, with: :render_400

+  rescue_from ActionController::Redirecting::UnsafeRedirectError do


Here's the catchall, if we somehow are still redirecting to an external URL (which shouldn't be happening anymore, as the places where this was current problem are now relative URLs now) then we will simply just redirect back to the login page. This way, no more allow_other_host unless you absolutely know what your doing.

CHANGELOG.md

pgwillia

Looks good to me 👍 I feel good about not going the allow_other_hosts route. Thanks for thinking this through :)

Co-authored-by: Tricia Jenkins <[email protected]>

murny requested review from ConnorSheremeta, jefferya, lagoan and pgwillia as code owners March 29, 2024 16:05

pgwillia reviewed Apr 5, 2024

View reviewed changes

murny added 3 commits April 17, 2024 14:17

#3419: Fix unsafe redirect errors by using allow_other_host

49ac0b3

Fix other case of unsafe redirect error

4cf525d

Add changelog entry

9768102

murny force-pushed the fix-unsafe-redirect-errors branch from 2010ccd to 9768102 Compare April 17, 2024 20:18

murny added 4 commits April 17, 2024 14:21

Use redirect_back_or_to instead of our custom logic for referrer

7b4ef51

Merge branch 'master' into fix-unsafe-redirect-errors

0a3fcb0

Merge branch 'master' into fix-unsafe-redirect-errors

8f68b36

Merge branch 'master' into fix-unsafe-redirect-errors

0f9e9cf

murny commented Apr 25, 2024

View reviewed changes

murny added 2 commits April 26, 2024 12:24

Merge branch 'master' into fix-unsafe-redirect-errors

eab4459

Fix neededing allow_other_host in redirect by just writting better code

0c695eb

murny commented Apr 30, 2024

View reviewed changes

Merge branch 'master' into fix-unsafe-redirect-errors

9a2941f

pgwillia reviewed May 1, 2024

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

pgwillia previously approved these changes May 1, 2024

View reviewed changes

Update CHANGELOG.md

572dc99

Co-authored-by: Tricia Jenkins <[email protected]>

murny dismissed pgwillia’s stale review via 572dc99 May 2, 2024 02:47

pgwillia approved these changes May 2, 2024

View reviewed changes

murny merged commit 49f709e into master May 2, 2024
4 checks passed

murny deleted the fix-unsafe-redirect-errors branch May 2, 2024 17:01

This was referenced Jun 21, 2024

Callback / error 500 / can't log in #3423

Closed

Fix unsafe redirect errors from Rollbar #3419

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3419: Fix unsafe redirect errors by using `allow_other_host` #3429

#3419: Fix unsafe redirect errors by using `allow_other_host` #3429

murny commented Mar 29, 2024 •

edited

Loading

pgwillia left a comment

pgwillia Apr 5, 2024

murny Apr 8, 2024 •

edited

Loading

pgwillia Apr 16, 2024

murny Apr 17, 2024

murny Apr 25, 2024 •

edited

Loading

pgwillia Apr 25, 2024

murny Apr 25, 2024

murny Apr 30, 2024 •

edited

Loading

murny Apr 30, 2024 •

edited

Loading

murny Apr 30, 2024

murny Apr 30, 2024

pgwillia left a comment

		@@ -15,6 +15,10 @@ class ApplicationController < ActionController::Base

		rescue_from JupiterCore::SolrBadRequestError, with: :render_400

		rescue_from ActionController::Redirecting::UnsafeRedirectError do

#3419: Fix unsafe redirect errors by using allow_other_host #3429

#3419: Fix unsafe redirect errors by using allow_other_host #3429

Conversation

murny commented Mar 29, 2024 • edited Loading

pgwillia left a comment

Choose a reason for hiding this comment

pgwillia Apr 5, 2024

Choose a reason for hiding this comment

murny Apr 8, 2024 • edited Loading

Choose a reason for hiding this comment

pgwillia Apr 16, 2024

Choose a reason for hiding this comment

murny Apr 17, 2024

Choose a reason for hiding this comment

murny Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

pgwillia Apr 25, 2024

Choose a reason for hiding this comment

murny Apr 25, 2024

Choose a reason for hiding this comment

murny Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

murny Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

murny Apr 30, 2024

Choose a reason for hiding this comment

murny Apr 30, 2024

Choose a reason for hiding this comment

pgwillia left a comment

Choose a reason for hiding this comment

#3419: Fix unsafe redirect errors by using `allow_other_host` #3429

#3419: Fix unsafe redirect errors by using `allow_other_host` #3429

murny commented Mar 29, 2024 •

edited

Loading

murny Apr 8, 2024 •

edited

Loading

murny Apr 25, 2024 •

edited

Loading

murny Apr 30, 2024 •

edited

Loading

murny Apr 30, 2024 •

edited

Loading