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

Enable :except option to :protect_from_csrf. #1539

Merged
merged 1 commit into from Dec 31, 2013

Conversation

Projects
None yet
3 participants
@namusyaka
Member

namusyaka commented Dec 28, 2013

ref #1258
I'd suggest to add the :except option to :protect_from_csrf.
This is a way of solving as whitelist.
We can use sinatra's routing path for except option because this was made from Sinatra::Base.
What do you think?

/cc @padrino/core-members @postmodern

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Dec 28, 2013

Member

Looks good.

Member

ujifgc commented Dec 28, 2013

Looks good.

ujifgc added a commit that referenced this pull request Dec 31, 2013

Merge pull request #1539 from padrino/enable-except-option
Enable :except option to :protect_from_csrf.

@ujifgc ujifgc merged commit d9f5a94 into master Dec 31, 2013

1 check passed

default The Travis CI build passed
Details

@ujifgc ujifgc deleted the enable-except-option branch Dec 31, 2013

@namusyaka

This comment has been minimized.

Show comment
Hide comment
@namusyaka

namusyaka Dec 31, 2013

Member

Thanks for reviewing.
BTW, should we add the :only option?

Member

namusyaka commented Dec 31, 2013

Thanks for reviewing.
BTW, should we add the :only option?

@ujifgc

This comment has been minimized.

Show comment
Hide comment
@ujifgc

ujifgc Dec 31, 2013

Member

I don't think so. We can get back to it if it's requested.

Member

ujifgc commented Dec 31, 2013

I don't think so. We can get back to it if it's requested.

@@ -339,11 +339,28 @@ def setup_csrf_protection(builder)
check_csrf_protection_dependency
if protect_from_csrf?
if protect_from_csrf.is_a?(Hash)
except = Array(protect_from_csrf[:except])
if !except.empty?

This comment has been minimized.

@skade

skade Dec 31, 2013

Member

I am late to the party, but if !except.empty? is unless except.empty.

@skade

skade Dec 31, 2013

Member

I am late to the party, but if !except.empty? is unless except.empty.

This comment has been minimized.

@namusyaka

namusyaka Dec 31, 2013

Member

Oops, Thank you. I'll fix it.

@namusyaka

namusyaka Dec 31, 2013

Member

Oops, Thank you. I'll fix it.

This comment has been minimized.

@ujifgc

ujifgc Dec 31, 2013

Member

BTW, @namusyaka what's your primary programming language?

@ujifgc

ujifgc Dec 31, 2013

Member

BTW, @namusyaka what's your primary programming language?

This comment has been minimized.

@namusyaka

namusyaka Dec 31, 2013

Member

I love JavaScript and Ruby.

@namusyaka

namusyaka Dec 31, 2013

Member

I love JavaScript and Ruby.

# returns a proc to avoid csrf protection.
def ignore_csrf_protection
Proc.new { env['HTTP_X_CSRF_TOKEN'] = session[:csrf] = SecureRandom.hex(32) }

This comment has been minimized.

@skade

skade Dec 31, 2013

Member

Please check whether I am reading this right:

This randomly changes the CSRF value when hitting an excepted path. Consider the following case:

  • User navigates to a form, gets a CSRF token
  • In another tab, the user navigates to an excepted URL (e.g., the FAQ)
    • Possibly a background AJAX-Request?
  • The user fills the form, submits
  • Request fails, as the forms token doesn't match the session token

Also, this filter even runs on safe urls, like GET requests.

Bypassing the authenticity token middleware instead of hacking through it seems to be a better option.

@skade

skade Dec 31, 2013

Member

Please check whether I am reading this right:

This randomly changes the CSRF value when hitting an excepted path. Consider the following case:

  • User navigates to a form, gets a CSRF token
  • In another tab, the user navigates to an excepted URL (e.g., the FAQ)
    • Possibly a background AJAX-Request?
  • The user fills the form, submits
  • Request fails, as the forms token doesn't match the session token

Also, this filter even runs on safe urls, like GET requests.

Bypassing the authenticity token middleware instead of hacking through it seems to be a better option.

This comment has been minimized.

@namusyaka

namusyaka Dec 31, 2013

Member

Let me make sure I understand you correctly.
You mean, we should use the new authenticity token middleware (the :except optional dedicated) instead of Sinatra?

@namusyaka

namusyaka Dec 31, 2013

Member

Let me make sure I understand you correctly.
You mean, we should use the new authenticity token middleware (the :except optional dedicated) instead of Sinatra?

This comment has been minimized.

@ujifgc

ujifgc Dec 31, 2013

Member

I guess it could be just Proc.new { env['HTTP_X_CSRF_TOKEN'] = session[:csrf], right? We don't want to break session existing csrf?

@ujifgc

ujifgc Dec 31, 2013

Member

I guess it could be just Proc.new { env['HTTP_X_CSRF_TOKEN'] = session[:csrf], right? We don't want to break session existing csrf?

This comment has been minimized.

@skade

skade Dec 31, 2013

Member

@ujifgc That would break if no CSRF token was ever given to the user or there is no session middleware.

No, I mean that we should not invoke CSRF protection at all when no CSRF is wanted. Currently, the implementation basically fakes a successful request.

@skade

skade Dec 31, 2013

Member

@ujifgc That would break if no CSRF token was ever given to the user or there is no session middleware.

No, I mean that we should not invoke CSRF protection at all when no CSRF is wanted. Currently, the implementation basically fakes a successful request.

This comment has been minimized.

@namusyaka

namusyaka Dec 31, 2013

Member

@ujifgc Like it.
Well, will it be like this?

      def ignore_csrf_protection
        Proc.new do
          env['HTTP_X_CSRF_TOKEN'] = session[:csrf] if request.post?
        end
      end
@namusyaka

namusyaka Dec 31, 2013

Member

@ujifgc Like it.
Well, will it be like this?

      def ignore_csrf_protection
        Proc.new do
          env['HTTP_X_CSRF_TOKEN'] = session[:csrf] if request.post?
        end
      end

This comment has been minimized.

@ujifgc

ujifgc Dec 31, 2013

Member

@skade do you know a way not to invoke Rack::Protection::AuthenticityToken if it's already used in the builder?

@ujifgc

ujifgc Dec 31, 2013

Member

@skade do you know a way not to invoke Rack::Protection::AuthenticityToken if it's already used in the builder?

This comment has been minimized.

@skade
@skade

This comment has been minimized.

@ujifgc

ujifgc Dec 31, 2013

Member

@skade yes, it makes sense. We could subclass AuthenticityToken, upgrade it with url exceptions and use it instead of the original.

@ujifgc

ujifgc Dec 31, 2013

Member

@skade yes, it makes sense. We could subclass AuthenticityToken, upgrade it with url exceptions and use it instead of the original.

This comment has been minimized.

@skade

skade Dec 31, 2013

Member

In the long run: Rack::Builder shows it's age, especially with things like that :(.

@skade

skade Dec 31, 2013

Member

In the long run: Rack::Builder shows it's age, especially with things like that :(.

This comment has been minimized.

@namusyaka

namusyaka Dec 31, 2013

Member

Yeah, makes sense.
And, I thought that the option should use Proc instead of Array.

@namusyaka

namusyaka Dec 31, 2013

Member

Yeah, makes sense.
And, I thought that the option should use Proc instead of Array.

namusyaka added a commit to namusyaka/padrino-framework that referenced this pull request Dec 31, 2013

Ortuna added a commit to Ortuna/padrino-framework that referenced this pull request Jan 17, 2014

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