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

Throttle does not read params when posted as Json #122

Closed
abdalafer opened this issue Apr 13, 2015 · 12 comments
Closed

Throttle does not read params when posted as Json #122

abdalafer opened this issue Apr 13, 2015 · 12 comments

Comments

@abdalafer
Copy link

In my application we have an authentication POST call which contains a json in the body.
{"session": {"email":"[email protected], "password":"*****"}}

My rack attack config contains the following throttle:
Rack::Attack.throttle('logins', :limit => 3, :period => 60.seconds) do |req|
req.params['session']['email'] if req.path == '/login' && req.post?
end

The issue is that req.params is nil, I also have application/json as content-type, this works within the app just fine, but for some reason it is not parsed or picked up here. Now if I include fields in the query string or as non json parameters in the body, it works. What can be updated in order for this to work properly?

@zmillman
Copy link
Contributor

Rails has extra Rack middleware for parsing JSON params that is below Rack::Attack in the stack. (i.e. JSON params are parsed after being processed by Rack::Attack).

ActionDispatch in Rails does the parsing with this middleware class: https://github.com/rails/rails/blob/master/actionpack/lib/action_dispatch/middleware/params_parser.rb#L31

There should be a way to move the middleware around so that the params parser will be higher in the stack than Rack::Attack, but that's not something I've tried. This guide should at least point you in the right direction: http://edgeguides.rubyonrails.org/rails_on_rack.html#action-dispatcher-middleware-stack

@abdalafer
Copy link
Author

That makes sense. This is the order of my stack:
use ActionDispatch::Static
use Rack::Lock
use #ActiveSupport::Cache::Strategy::LocalCache::Middleware:0x007fadee7dcd98
use Rack::Runtime
use Rack::MethodOverride
use ActionDispatch::RequestId
use Rails::Rack::Logger
use ActionDispatch::ShowExceptions
use ActionDispatch::DebugExceptions
use ActionDispatch::RemoteIp
use ActionDispatch::Reloader
use ActionDispatch::Callbacks
use ActiveRecord::ConnectionAdapters::ConnectionManagement
use ActiveRecord::QueryCache
use ActionDispatch::Cookies
use ActionDispatch::Session::CookieStore
use ActionDispatch::Flash
use ActionDispatch::ParamsParser
use Clearance::RackSession
use ActionDispatch::Head
use Rack::ConditionalGet
use Rack::ETag
use ActionDispatch::BestStandardsSupport
use Warden::Manager
use Rack::Attack
run ........::Application.routes

I think the best method would be to move the Rack::Attack up, instead of moving the ActionDispatch

@abdalafer
Copy link
Author

I moved RackAttack to after the ActionDispatch::ParamsParser:
..
..
use ActionDispatch::Callbacks
use ActiveRecord::ConnectionAdapters::ConnectionManagement
use ActiveRecord::QueryCache
use ActionDispatch::Cookies
use ActionDispatch::Session::CookieStore
use ActionDispatch::Flash
use ActionDispatch::ParamsParser
use Rack::Attack
use Clearance::RackSession
use ActionDispatch::Head
use Rack::ConditionalGet
use Rack::ETag
use ActionDispatch::BestStandardsSupport
use Warden::Manager
..
.
But it still gives me nil for req['params'], I have put Rack::Attack in several different positions and it does not affect it.

@zmillman
Copy link
Contributor

Hmmm. I don't know what's going wrong, but it seems like this issue is outside the scope of Rack::Attack. (Unless the problem is from changing req.params to req['params']?)

You could probably get a more helpful answer over at the main Rails project where they maintain ActionDispatch's middleware: https://github.com/rails/rails

(Worst case scenario, I'd recommend extending Rack::Attack::Request to implement a json_params helper method that uses ActionDispatch::ParamsParser to get your login param)

@abdalafer
Copy link
Author

Thank you, are there any examples that you know of, of extending Rack::Attack::Request?

@zmillman
Copy link
Contributor

There's an example in the gem source code here https://github.com/kickstarter/rack-attack/blob/master/lib/rack/attack/request.rb and it's also discussed in issues #73 and #113.

If you're wondering where to put the monkey-patch, I like this organizational approach: http://stackoverflow.com/questions/3420680/monkey-patching-in-rails-3

@abdalafer
Copy link
Author

When trying your suggestion, I found the params:

Rack::Attack.throttle('logins', :limit => 3, :period => 60.seconds) do |req|
email = req.env['action_dispatch.request.request_parameters'][:session]['email']
email if req.path == '/login' && req.post?
end

This works perfectly, I did not do the monkey-patch, all I did was make sure that Rack::Attack loads after the ActionDispatch::ParamsParser:

in application.rb
config.middleware.insert_after(ActionDispatch::ParamsParser, Rack::Attack)

@zmillman
Copy link
Contributor

Ah, glad to hear everything's working for you now 👍

Please feel free to close this issue :)

@abdalafer
Copy link
Author

Sure, thanks for your help.

@peterfication
Copy link

In Rails 5 there is no ActionDispatch::ParamsParser anymore. I tried to fiend a solution to have req.params somehow but I haven't been successful.

This is what I use now, but I'm not totally satisfied:

Rack::Attack.throttle('login', limit: 6, period: 60.seconds) do |req|
  begin
    # req.params for JSON is not yet available at this step
    JSON.parse(req.body.string)['email'] if req.path == '/login' && req.post?
  rescue JSON::ParserError => e
  end
end

Does anybody have an idea how to achieve this by not parsing the JSON manually?

@jspooner
Copy link

Thanks @peterfication for your solution.

I'm using PhusionPassenger and discovered that I needed to call rewind after read. Otherwise, the next call to read would result in a nil response and the controller would throw an ActionDispatch::Http::Parameters::ParseError.

  throttle("logins/email", limit: 5, period: 30.seconds) do |req|
    begin
      email = JSON.parse(req.body.read())['email']
      req.body.rewind
      email if req.path == '/api/v2/authenticate' && req.post?
    rescue JSON::ParserError => e
    end
  end

@Aryk
Copy link

Aryk commented Nov 22, 2021

Just FYI for anyone reading this now. I think the recommended way that avoid the read/rewind problem, etc...would be to do the solution found here:

#399

  real_req = ActionDispatch::Request.new(req.env)

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

No branches or pull requests

6 participants