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

Rails Router Catchall Picks Up ActiveStorage Routes #31228

Open
Petercopter opened this issue Nov 25, 2017 · 44 comments · May be fixed by #51716
Open

Rails Router Catchall Picks Up ActiveStorage Routes #31228

Petercopter opened this issue Nov 25, 2017 · 44 comments · May be fixed by #51716

Comments

@Petercopter
Copy link

Steps to reproduce

Using ActiveStorage, Webpacker, and React Router. Setup Rails routing for a catchall route

  get '*path', controller: 'react', action: 'index', as: :react

Expected behavior

Catchall Route should ignore ActiveStorage Routing

Actual behavior

This also picks up the ActiveStorage routing: /rails/active_storage, so the React Router incorrectly tries to route to the file.

System configuration

Rails version:
5.2.0.alpha d3893ec

Ruby version:
2.4.2

@rafaelfranca
Copy link
Member

This is not a problem only with Active Storage. Any engine that you include in your application that has routes will have the same problem. So I don't think there is something we can do in our side. Could you try to mount Active Storage engine in our routes like this?

mount '/', to: ActiveStorage::Engine

Make sure it is before your catch all route.

@Petercopter
Copy link
Author

@rafaelfranca Thanks for looking into this. I just tried

mount ActiveStorage::Engine, at: '/'

and

mount ActiveStorage::Engine, at: '/rails/active_storage'

Neither one is picking up the routing before the catchall.

Then I went looking at another Rails application I work on where we have a catchall route. I noticed that the engines are mounted there as well, and that even the ActionCable server needs to be mounted:

mount ActionCable.server => '/cable'
mount PgHero::Engine, at: 'pghero'
mount Sidekiq::Web => '/sidekiq'

So it would seem that I just need to figure out how to get the ActiveStorage engine mounted onto the routing, so it kicks in before the catchall route.

@Petercopter
Copy link
Author

It looks like you're familiar with routing issues, I see you've looked into #30783 as well. I'm still poking around to understand the problem more.

@kieraneglin
Copy link

@peterc were you able to find a solution to this?

@kieraneglin
Copy link

kieraneglin commented Dec 19, 2017

For all those who follow, this will work in the meantime:

  get '*all', to: 'application#mount', constraints: lambda { |req|
    req.path.exclude? 'rails/active_storage'
  }

It's kinda ugly and probably could be done better, but it works.

@Petercopter
Copy link
Author

@kieraneglin I haven't poked at it lately. For now, I'm just routing one path to work around it.

I like your solution. Ugly and works is better than other options, I'll take it! 👍

@rails-bot rails-bot bot added the stale label Mar 20, 2018
@rails-bot
Copy link

rails-bot bot commented Mar 20, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-1-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@kieraneglin
Copy link

Can still reproduce on 5.2.0b2

@rails-bot rails-bot bot removed the stale label Mar 20, 2018
@trvsdnn
Copy link
Contributor

trvsdnn commented Apr 12, 2018

Still looking for a better solution to this one. It got me late last night while I was testing 5.2 and ActiveStorage on an old project. Catch all routes are fairly common and the way this breaks isn't immediately obvious.

Better solution and perhaps a note in the guide?

@jspang
Copy link

jspang commented Apr 13, 2018

9 out of ten of my Rails projects from the last 10 years need catch all routes for something (eg. dynamic paths for CMS-based public web content with user generated nice urls).

Although the routing is a bit complicated, I would much prefer if developers were told to put the routes in the routes.rb file to allow control of route order, path naming etc. for ActiveStorage.

  • Thanks to @kieraneglin for the hack that allowed me to take ActiveStorage for a spin on the latest 5.2 project (and I like it). ;-)

@geetfun
Copy link

geetfun commented Apr 19, 2018

I agree with @jspang in that it would be clearer where these routes are coming from if they were explicitly defined by the programmer in routes.rb. Given that there is some level of set up for existing apps (ie. upgraded Rails apps) with bin/rails active_storage:install, adding the routes explicitly in the routes.rb can serve to allow for customization of the order of the routes.

New apps can have the routes.rb generated with the active storage routes automagically inserted by default.

@rails-bot rails-bot bot added the stale label Jul 18, 2018
@rails-bot
Copy link

rails-bot bot commented Jul 18, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-2-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@kieraneglin
Copy link

Can still reproduce on 5.2 stable

@rails-bot
Copy link

rails-bot bot commented Oct 16, 2018

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-2-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Oct 16, 2018
@tom-brown
Copy link

I can still reproduce on 5.2 stable. I am trying to redirect routing errors to root_path, and I have tried solutions at this stackoverflow link and this medium link, all with the same outcome: broken active_storage links to uploaded images. kieraneglin's suggestion above works like a charm.

@rails-bot rails-bot bot removed the stale label Oct 22, 2018
@zmajstor
Copy link

maybe, it's better to add the catch-all route after application initialization, as described here:
#671 (comment)

viktorsmari added a commit to fablabbcn/smartcitizen-api that referenced this issue Jan 9, 2019
Disabling match all in routes, until a better solution is found.
See: rails/rails#31228
viktorsmari added a commit to fablabbcn/smartcitizen-api that referenced this issue Jan 10, 2019
* Use active storage for users profile picture

Added a new endpoint /me/profile
Also on /me the user object has a profile_picture which has a timeout?
We still have the older user.avatar

* ActiveStorage: add route for /rails/active_storage

Add full URL to .profile_picture

Open up for CORS in development

* ActiveStorage conflict with routes match all

Disabling match all in routes, until a better solution is found.
See: rails/rails#31228

* Gemfile: aws gem was added multiple times

* Git info: .chomp was misplaced

* Gemfile: remove GCS and /profile_url test endpoint

* Storage: not using GC
@rails-bot
Copy link

rails-bot bot commented Jan 26, 2019

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 5-2-stable branch or on master, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@rails-bot rails-bot bot added the stale label Jan 26, 2019
@kieraneglin
Copy link

@rafaelfranca Thanks for the update. If no solution is proposed, would you be open to a PR that adds documentation/caveats around AS+catchall routes?

@rails-bot
Copy link

rails-bot bot commented Apr 14, 2021

This issue has been automatically marked as stale because it has not been commented on for at least three months.
The resources of the Rails team are limited, and so we are asking for your help.
If you can still reproduce this error on the 6-1-stable branch or on main, please reply with all of the information you have about it in order to keep the issue open.
Thank you for all your contributions.

@AbhishekSChauhan
Copy link

AbhishekSChauhan commented Oct 31, 2021

I am newbie to rails and this routing system but i have tried something like from wildcard routes
get '/*page', to: 'home#index', page: /(?!blogs|comments|tags|users|admin|rails).*/

@cesarjr
Copy link

cesarjr commented Oct 25, 2022

Hi!

My app frequently raises a ActionController::RoutingError. Sometimes it happens because some user typed a wrong route or things like Google trying to find robots.txt.

I'd like to treat this cases with an error page or a json error case content type is json. So, a wrote this:

Rails.application.routes.draw do
  #... many things here
  match '*unmatched', to: 'errors#route_not_found', via: :all
end  
class ErrorsController < ApplicationController
  def route_not_found
    respond_to do |format|
      format.json { render json: { error: 'The request resource does not exist' }, status: :not_found }
      format.html { redirect_to '/#/404' }
    end
  end
end

Everything run as I expected except Active Storage.

Reading all the content of this thread made me think if I'm not implementing the wrong solution for my problem.

Is it really a bug in Rails? Or is there a way more correct to solve my problem?

I suppose that I'm doing the wrong thing because it seems something common to have in almost any app.

Thank you!

@skipkayhil
Copy link
Member

Is it really a bug in Rails? Or is there a way more correct to solve my problem?

Check out config.exceptions_app, I believe that's what you're looking for.

@SampsonCrowley
Copy link
Contributor

Why 2 major versions later does activestorage need it's own way outside of the conventions to route things whatsoever?? why is it not drawing routes like every single other engine and letting people mount them normally? that is the real solution

@SampsonCrowley
Copy link
Contributor

SampsonCrowley commented Dec 14, 2022

For those that come in here on rails 7, I discovered a much better solution (other than actually fixing activestorage to not be non-conforming) that will guarantee that your catchall comes last

in config/application.rb add a catch_all initializer that runs after add_internal_routes and append your catchall as a lambda to be called by run_after_load_paths

initializer "catch_all", after: :add_internal_routes, before: :set_routes_reloader_hook do |app|
  routes_reloader.run_after_load_paths = -> do
    app.routes.append do
      get "*path", to: "dashboard#not_found", constraints: ->(request) do
        !request.xhr? && request.format.html?
      end
    end
  end
end

@jeppester
Copy link

jeppester commented Mar 28, 2023

I'm digging into Active Storage these days because I'm going to use it for a side project. To be honest, on behalf of the rails community, I'm embarrassed over what I'm learning.

First I discovered that everyone can by default upload files and retrieve those files from your storage through direct uploads. The documentation is not mentioning this at all. On top of that the authentication example given by the docs disable all the active storage routing, which is an unnecessary trade-off. It will convince lots of devs that those routes are hard to guess anyway, and so they will leave the direct upload hole open for anyone to use - a bit like how mongodb was insecure by default.

On top of that we have the routing which breaks catch-all routes for no reason other than "plug and play". I could live with this, if I was able to both cancel the "automagical" drawing of routes with the draw_routes option, and then manually calling the same code from my routes file, but the routes file has nothing to hook into.

That such glaring issues have not been addressed at this point makes me worried if Active Storage is a solution I can trust. If it's even used by DHH and the likes, or if it's another glorified experiment that everyone then had to live with (like coffee script and webpacker).

I'll consider making a PR for the solution I outlined with having a separate method draw the routes. It seems like it won't break current functionality.

@searls
Copy link
Contributor

searls commented Feb 15, 2024

I just got bit by this trying to write a system test that needs these routes to interact with the Active Storage disk service, still occurs on main at 9e01d9.

In order to help anyone who might find this via Google, what I was experiencing in my test was that images loaded via url_for(variant_or_representation) generate URLs like this:


http://127.0.0.1:50227/rails/active_storage/representations/redirect/eyJfcmFpbHMiOnsiZGF0YSI6MjUsInB1ciI6ImJsb2JfaWQifX0=--e9cd0d944950f96dbfb187534dc166d45acaeb1d/eyJfcmFpbHMiOnsiZGF0YSI6eyJmb3JtYXQiOiJwbmciLCJyZXNpemVfdG9fZmlsbCI6WzQwMCw0MDBdfSwicHVyIjoidmFyaWF0aW9uIn19--a5f82b4c83548ebe3778b59dca48bb0f99ac16a6/image.png

Which I could see from rails routes was correctly drawn to be handled by ActiveStorage::Representations::RedirectController, but was nevertheless being handled by ErrorsController#not_found based on my test.log:

Started GET "/rails/active_storage/representations/redirect/eyJfcmFpbHMiOnsiZGF0YSI6MjUsInB1ciI6ImJsb2JfaWQifX0=--e9cd0d944950f96dbfb187534dc166d45acaeb1d/eyJfcmFpbHMiOnsiZGF0YSI6eyJmb3JtYXQiOiJwbmciLCJyZXNpemVfdG9fZmlsbCI6WzQwMCw0MDBdfSwicHVyIjoidmFyaWF0aW9uIn19--a5f82b4c83548ebe3778b59dca48bb0f99ac16a6/image.png" for 127.0.0.1 at 2024-02-15 16:30:56 -0500
Processing by ErrorsController#not_found as PNG
  Parameters: {"path"=>"rails/active_storage/representations/redirect/eyJfcmFpbHMiOnsiZGF0YSI6MjUsInB1ciI6ImJsb2JfaWQifX0=--e9cd0d944950f96dbfb187534dc166d45acaeb1d/eyJfcmFpbHMiOnsiZGF0YSI6eyJmb3JtYXQiOiJwbmciLCJyZXNpemVfdG9fZmlsbCI6WzQwMCw0MDBdfSwicHVyIjoidmFyaWF0aW9uIn19--a5f82b4c83548ebe3778b59dca48bb0f99ac16a6/image"}

Anyway, I'm sure one of the workarounds above will fix this, but it does seem like this is a confusing enough failure state that it'd be worth attempting a refactor of how the ActiveStorage engine draws its routes

@searls
Copy link
Contributor

searls commented Feb 15, 2024

Speaking of workarounds, here's the one that worked best for me while being minimally invasive.

Changing the end of config/routes.rb from this:

get "*path", to: "errors#not_found"

To this:

  get "*path", to: "errors#not_found", constraints: lambda { |req|
    !req.path.start_with? "/rails/active_storage"
  }

@lgebhardt
Copy link

lgebhardt commented Apr 18, 2024

  get "*path", to: "errors#not_found", constraints: lambda { |req|
    !req.path.start_with? "/rails/active_storage"
  }

This will still let any request that starts with /rails/active_storage through even if it's not a valid route. Adding in the catch_all route after the internal routes, as shown in #31228 (comment), is working well.

@justinko
Copy link
Contributor

What do you think about this? main...justinko:rails:activestorage-routes

In your app, you would just do:

config.active_storage.draw_routes = false

# inside your app's routes.rb
draw "rails/active_storage"

The routes are pretty complex so don't think mount ActiveStorage::Engine will be an option.

@searls
Copy link
Contributor

searls commented Apr 22, 2024

seems reasonable!

@jeppester
Copy link

jeppester commented Apr 23, 2024

What do you think about this? main...justinko:rails:activestorage-routes

In your app, you would just do:

config.active_storage.draw_routes = false

# inside your app's routes.rb
draw "rails/active_storage"

The routes are pretty complex so don't think mount ActiveStorage::Engine will be an option.

Seems like a simple and reasonable workaround to me 👍

justinko added a commit to justinko/rails that referenced this issue Apr 23, 2024
justinko added a commit to justinko/rails that referenced this issue Apr 24, 2024
Fixes rails#31228

Revert "Add the ability to explicitly load the ActiveStorage routes"

This reverts commit 2faa138.

Add the routes via method

Try ActiveStorage::Routes

fix routes_test.rb

no instance_eval
justinko added a commit to justinko/rails that referenced this issue Apr 24, 2024
@zzak zzak reopened this Apr 24, 2024
@rails-bot rails-bot bot removed the stale label Apr 24, 2024
justinko added a commit to justinko/rails that referenced this issue Apr 27, 2024
justinko added a commit to justinko/rails that referenced this issue Apr 27, 2024
justinko added a commit to justinko/rails that referenced this issue May 2, 2024
@justinko justinko linked a pull request May 2, 2024 that will close this issue
4 tasks
justinko added a commit to justinko/rails that referenced this issue May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet