set_locale_from_url needs some checking
cernyjakub opened this issue · 12 comments
In current implementation, set_locale_from_url takes any value from params[:locale] and tries to set locale.
Recently, I experienced a massive attack, which tried to SQL inject thru locale parameter.
I propose to check if params[:locale] is included in I18n.available_locales first.
regards!
Jakub
Hi!
Thanks for reporting this.
If there is a security issue caused by route_translator, the github repo is not the proper place to talk about that.
As far as I know, the locale param is checked by I18n.locale when enforce_available_locales is true:
enforce_available_locales is true by default starting from Rails 4.1: https://edgeguides.rubyonrails.org/upgrading_ruby_on_rails.html#i18n-enforcing-available-locales
Unless you turned off enforce_available_locales (and Rails advises to not do that), there should be no security concerns.
So I think that you would route_translator to avoid potential DOS attacks by checking the locale parameter before trying to set the current_locale. Am I right?
Hi, you are right, enforce_available_locales performs the check, but if the params[:locale] contains rubbish, I18n::InvalidLocale exception is thrown.
In my particular case, the server was flooded with bogus param values and my exception tracker almost exploded ;) So, some kind of DOS was possible.
I think it would be better to check that params[:locale] contains correct value first, and then try to set locale. Incorrect value almost always means some kind of error, so I think it is better to do nothing then to cause exception to be risen.
Just a side note - in this particular case the problematic URL was the RepresentationsController used by ActiveStorage - which does not use locales at all...
cheers!
jakub
Thanks.
@cernyjakub any chance to have a reproducible test case?
I mean, a configuration that allows user to send the locale param to route_translator via the url.
At the moment, I'm not able to reproduce because the locale param is taken by the url itself. I mean, If there is a route, then the locale is defined by the url itself (so we have a match and the locale is available)
In my case, the locale was transmitted as normal GET parameter:
http://myhost/my_app/any_url?locale=nasty_bad_string
I hope it helps..
cheers!
jakub
@cernyjakub thanks, but I'm not able to replicate
Maybe there is some configuration parameter different from mine and I can't understand which one
Rails 5.2.3
route_translator 6.0.0
Ruby 2.6.3
config/routes.rb
Rails.application.routes.draw do
localized do
resources :products
get :product_detail, to: 'products#show'
end
end
config/application.rb
require_relative 'boot'
require 'rails/all'
# Require the gems listed in Gemfile, including any gems
# you've limited to :test, :development, or :production.
Bundler.require(*Rails.groups)
module RouteTranslatorRails5221
class Application < Rails::Application
# Initialize configuration defaults for originally generated Rails version.
config.load_defaults 5.2
# Settings in config/environments/* take precedence over those specified here.
# Application configuration can go into files in config/initializers
# -- all .rb files in that directory are automatically loaded after loading
# the framework and any gems in your application.
I18n.available_locales = %i[en it]
I18n.default_locale = :en
end
end
config/initializers/route_translator.rb not present
$ rails routes
$ rails routes
Prefix Verb URI Pattern Controller#Action
products_it GET /it/prodotti(.:format) products#index {:locale=>"it"}
products_en GET /products(.:format) products#index {:locale=>"en"}
POST /it/prodotti(.:format) products#create {:locale=>"it"}
POST /products(.:format) products#create {:locale=>"en"}
new_product_it GET /it/prodotti/new(.:format) products#new {:locale=>"it"}
new_product_en GET /products/new(.:format) products#new {:locale=>"en"}
edit_product_it GET /it/prodotti/:id/edit(.:format) products#edit {:locale=>"it"}
edit_product_en GET /products/:id/edit(.:format) products#edit {:locale=>"en"}
product_it GET /it/prodotti/:id(.:format) products#show {:locale=>"it"}
product_en GET /products/:id(.:format) products#show {:locale=>"en"}
PATCH /it/prodotti/:id(.:format) products#update {:locale=>"it"}
PATCH /products/:id(.:format) products#update {:locale=>"en"}
PUT /it/prodotti/:id(.:format) products#update {:locale=>"it"}
PUT /products/:id(.:format) products#update {:locale=>"en"}
DELETE /it/prodotti/:id(.:format) products#destroy {:locale=>"it"}
DELETE /products/:id(.:format) products#destroy {:locale=>"en"}
product_detail_it GET /it/dettagli-prodotto-:id(.:format) products#show {:locale=>"it"}
product_detail_en GET /product-detail-:id(.:format) products#show {:locale=>"en"}
rails_service_blob GET /rails/active_storage/blobs/:signed_id/*filename(.:format) active_storage/blobs#show
rails_blob_representation GET /rails/active_storage/representations/:signed_blob_id/:variation_key/*filename(.:format) active_storage/representations#show
rails_disk_service GET /rails/active_storage/disk/:encoded_key/*filename(.:format) active_storage/disk#show
update_rails_disk_service PUT /rails/active_storage/disk/:encoded_token(.:format) active_storage/disk#update
rails_direct_uploads POST /rails/active_storage/direct_uploads(.:format) active_storage/direct_uploads#create
Ok, replicated.
It happens in active_storage controllers
http://localhost:3000/rails/active_storage/blobs/2e2e2/wfdw?locale=fewfwef
Wouldn't it be better to leave Rails handle it like it already does? By that I mean enforce_available_locale! is already checking if this is a valid locale. Checking it before would only do the same job twice.
@cernyjakub You could simply ignore the error in your error tracker configuration (example if you are using Raven / Sentry):
Raven.configure do |config|
app_ignore = %w[
I18n::InvalidLocale
]
config.excluded_exceptions = app_ignore + Raven::Configuration::IGNORE_DEFAULT
... More config here
endIf you really want to prevent users trying to inject an invalid locale from seeing an error page, you could do the following your ApplicationController to force users with an invalid locale to a valid one:
class ApplicationController < ActionController::Base
rescue_from I18n::InvalidLocale, with: -> { I18n.locale = I18n.default_locale }
endOf course I could resolve this in my app. But as shown above, the error is triggered by ActiveStorage controller - the standard part of rails stack.
see https://github.com/rails/rails/blob/master/activestorage/app/controllers/active_storage/base_controller.rb - this controller is not derived from ApplicationController, so the aforementioned solution would not help.
I think it is not good idea to have to monkey patch ActiveStorage for all apps using this gem...
cheers!
jakub
Good point, this happens because this gem is inserting the around_action in ActionController::Base directly:
I agree that route_translator should check the locale without raising an exception.
One of the option I was thinking about, is to release a new major version that will explicitly require developers to add the around_filter to their ApplicationController, so it will not pollute ActionController::Base
This would be a great solution. Honestly - in most of my apps, I only use route_translator on a small subset of controllers (mainly the public facing URLs). All the back-end stuf like admin, assets, etc. does not need it at all. And perhaps it is cleaner to require user to explicitly turn on locale setting (and give chance to use other means of setting locale - personally I prefer to handle this myself..).
cheers!
jakub
@cernyjakub I will consider the explicit around_filter change, but we still need to check the locale.
eg:
Rails.application.routes.draw do
localized do
get 'dummy', to: 'dummy#dummy'
end
get 'unlocalized', to: 'dummy#unlocalized'
end
Even if you explicitly add around_action :set_locale_from_url in DummyController, then requests to unlocalized with a locale not included in I18n.available_locales will result in an InvalidLocale error being raised
Feel free to review 166bd84 86756e5 1fc53de #198
I will release a new Major release, even if this should not be a breaking change for most of the use cases of this gem