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

Jump to bottom

Enable :except option to :protect_from_csrf. #1539

Merged
merged 1 commit into from Dec 31, 2013
Merged

Enable :except option to :protect_from_csrf. #1539

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 17 additions & 0 deletions padrino-core/lib/padrino-core/application.rb
View file
Open in desktop
Expand Up @@ -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?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, Thank you. I'll fix it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love JavaScript and Ruby.

except.each{|except_path| exception_router.before(except_path, &ignore_csrf_protection) }
builder.use exception_router
end
end
builder.use(Rack::Protection::AuthenticityToken,
options_for_csrf_protection_setup)
end
end

# the sinatra app for routes to disable the csrf protection.
def exception_router
@exception_router ||= Class.new(Sinatra::Base)
end

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ujifgc Skipping isn't easy with builder alone, but a pattern like this works:

http://icelab.com.au/articles/wrapping-rack-middleware-to-exclude-certain-urls-for-rails-streaming-responses/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

end

# returns the options used in the builder for csrf protection setup
def options_for_csrf_protection_setup
options = { :logger => logger }
Expand Down
47 changes: 46 additions & 1 deletion padrino-core/test/test_csrf_protection.rb
View file
Open in desktop
Expand Up @@ -76,5 +76,50 @@
assert_equal 403, status
end
end

context "with :except option" do
before do
mock_app do
enable :sessions
set :protect_from_csrf, :except => ["/", "/foo"]
post("/") { "Hello" }
post("/foo") { "Hello, foo" }
post("/bar") { "Hello, bar" }
end
end

should "allow ignoring CSRF protection on specific routes" do
post "/"
assert_equal 200, status
post "/foo"
assert_equal 200, status
post "/bar"
assert_equal 403, status
end
end

context "with middleware" do
before do
class Middleware < Sinatra::Base
post("/middleware") { "Hello, middleware" }
post("/dummy") { "Hello, dummy" }
end
mock_app do
enable :sessions
set :protect_from_csrf, :except => ["/", "/middleware"]
use Middleware
post("/") { "Hello" }
end
end

should "allow ignoring CSRF protection on specific routes of middleware" do
post "/"
assert_equal 200, status
post "/middleware"
assert_equal 200, status
post "/dummy"
assert_equal 403, status
end
end
end
end
end