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

Support for subdomains #73

Closed
SirRawlins opened this issue Jun 17, 2014 · 5 comments
Closed

Support for subdomains #73

SirRawlins opened this issue Jun 17, 2014 · 5 comments

Comments

@SirRawlins
Copy link

I've been implementing rack-attack this morning but had a requirement to work with the request based on it's subdomain, rather than path.

It seem that the Rack::Request class doesn't provide any way to access the subdomain of the request, which made things a little tricky.

At the moment I have borrowed a similar approach to the one described here http://tannerburson.com/2009/01/extracting-subdomains-in-sinatra.html/ but instead monkey-patched the Rack::Attack::Request class to provide the subdomains method. a'la

# We re-open the request class to add the subdomains method
module Rack
  class Attack
    class Request < ::Rack::Request

      def subdomains(tld_len=1) # we set tld_len to 1, use 2 for co.uk or similar
        # cache the result so we only compute it once.
        @env['rack.env.subdomains'] ||= lambda {
          # check if the current host is an IP address, if so return an empty array
          return [] if (host.nil? ||
                        /\d{1,3}\.\d{1,3}\.\d{1,3}\.\d{1,3}$/.match(host))
          host.split('.')[0...(1 - tld_len - 2)] # pull everything except the TLD
        }.call
      end

    end
  end
end

This then allows me to apply rules based on the subdomain, like this:

# Secure the admin interface to allowed admin IP addresses.
blacklist('Secure admin interface by IP') do |req|
    # Request are blocked if the return value is truthy
    # Check to see if this is an admin interface request and that the
    # IP is not included in the environments whitelist.
    req.subdomains.first == 'admin' && ENV['ADMIN_IP_WHITELIST'].exclude?(req.ip)
end

Do you think there is an argument for adding this to rack-attack code base? If so I'd be happy to put together a pull-request.

Robert

@zmillman
Copy link
Contributor

For your use case, you could pretty easily do:

blacklist('Secure admin interface by IP') do |req|
    # Request are blocked if the return value is truthy
    # Check to see if this is an admin interface request and that the
    # IP is not included in the environments whitelist.
    req.host.split('.').first == 'admin' && ENV['ADMIN_IP_WHITELIST'].exclude?(req.ip)
end

I have a feeling that adding a subdomains helper method might be out of scope since it's pretty easy for most people to write their own. I'm not actually even sure why there's a Rack::Attack::Request object -- I can't find anywhere in the code that it's used. Perhaps @ktheory could weigh in?

@SirRawlins
Copy link
Author

Thanks for the suggestion! And I do agree that this use case probably is outside of scope - just thought it was worth floating the idea.

The Rack::Attack::Request object is just a simple facade for the sake of abstraction by the looks of things - it does nothing but extend the Rack::Request.

It might be nice to make the Request class configurable though, that way I could include the logic as I proposed without monkey patching.

Sent from my iPhone

On 17 Jun 2014, at 18:10, Zach Millman [email protected] wrote:

For your use case, you could pretty easily do:

blacklist('Secure admin interface by IP') do |req|
# Request are blocked if the return value is truthy
# Check to see if this is an admin interface request and that the
# IP is not included in the environments whitelist.
req.host.split('.').first == 'admin' && ENV['ADMIN_IP_WHITELIST'].exclude?(req.ip)
end
I have a feeling that adding a subdomains helper method might be out of scope since it's pretty easy for most people to write their own. I'm not actually even sure why there's a Rack::Attack::Request object -- I can't find anywhere in the code that it's used. Perhaps @ktheory could weigh in?


Reply to this email directly or view it on GitHub.

Support Time Limited is a company registered in England and Wales. Registered number: 06527328. Registered office: 145-157, St. John Street , London, EC1V 4PW.

@ktheory
Copy link
Collaborator

ktheory commented Jun 17, 2014

@SirRawlins, @zmillman Yup, I agree that a subdomains method is out of scope for Rack::Attack.

You can easily add it to your app; or not-so-easily convince the rack maintainers to add it to Rack::Request.

It might be nice to make the Request class configurable though

@SirRawlins, I'm not how you mean. Please elaborate.

Closing the issue b/c there's no further action needed.

@ktheory ktheory closed this as completed Jun 17, 2014
@SirRawlins
Copy link
Author

@ktheory thanks for getting back to me.

With regards to the configurable class my thoughts are that we drop the Rack::Attack::Request class all together as it's not adding any functionality, and instead have the lib directly reference Rack::Request.

However, this could be overridden in the config such as.

config.request_class => MyApp::MyRequestClass

My request class could then extend the Rack::Request, adding any functionality required, but without having to reopen the Request class and monkey patch any behaviour.

The end result is little different to what I've outline above, it's just a little tidier as it removed the need to Monkey Patch.

@zmillman
Copy link
Contributor

@SirRawlins: I really like how straightforward the method definition for call is in the README. Since Rack::Attack::Request is already designated as a safe place to add monkey patch helpers, it seems like it's unnecessary to add more configuration options. The change probably wouldn't make the code tidier for 90% of devs using Rack::Attack.

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

4 participants