Bug with ActiveResource/ActionPack/protect_from_forgery

Hi,

(I didn't get any answers over on rails-talk; I'm hoping it's ok that
I escalate it to this list instead.)

I have a Rails 2.1.1 web app, and a Rails 2.1.1 app acting as a client
by using ActiveResource.

From the client, I can find, create, and update resources owned by the
web app.

However, I can not delete any. Calling the .destroy method in
ActiveResource generates a 422 from the web app.

Not sure why this would be the case, since I thought
protect_from_forgery only protects HTML and JS requests, whereas
ActiveResource should be sending XML requests.

Any idea if this is a bug in ActiveResource that I should dig into/
submit a patch for, or
is this actually by design and I'm not understanding something about
how to achieve deletes via ActiveResource?

Thanks!
Jeff

Hi,

(I didn't get any answers over on rails-talk; I'm hoping it's ok that
I escalate it to this list instead.)

I have a Rails 2.1.1 web app, and a Rails 2.1.1 app acting as a client
by using ActiveResource.

From the client, I can find, create, and update resources owned by the
web app.

However, I can not delete any. Calling the .destroy method in
ActiveResource generates a 422 from the web app.

Not sure why this would be the case, since I thought
protect_from_forgery only protects HTML and JS requests, whereas
ActiveResource should be sending XML requests.

Any idea if this is a bug in ActiveResource that I should dig into/
submit a patch for, or
is this actually by design and I'm not understanding something about
how to achieve deletes via ActiveResource?

This does sound like a bug or misconfiguration somewhere along the
line. The request verification logic should trigger *everything*
that isn't a :get and doesn't have a content-type of one of these:

    @@unverifiable_types = Set.new [:text, :json, :csv, :xml, :rss,
:atom, :yaml]

Thanks for that - I'll try to dig into it this week and see if I can
figure out what's going on.

Jeff

I see the issue now. ActionController:Base includes
RequestForgeryProtection, which defines this method:

def verifiable_request_format?
  request.content_type.nil? || request.content_type.verify_request?
end

That first part (request.content_type.nil?) always returns true for
DELETE requests, because ActiveResource only sends an Accept header:

# connection.rb
  HTTP_FORMAT_HEADER_NAMES = { :get => 'Accept',
      :put => 'Content-Type',
      :post => 'Content-Type',
      :delete => 'Accept'
    }

I think the Delete request needs to send both headers, since I think
ActiveResource wants to receive the deleted content as well.

So far my attempts to send both headers are breaking existing tests,
so I'm working on updating the tests.

Thanks
Jeff

I've changed my mind a bit... see below.

> > This does sound like a bug or misconfiguration somewhere along the
> > line. The request verification logic should trigger *everything*
> > that isn't a :get and doesn't have a content-type of one of these:

> > @@unverifiable_types = Set.new [:text, :json, :csv, :xml, :rss,
> > :atom, :yaml]

I see the issue now. ActionController:Base includes
RequestForgeryProtection, which defines this method:

def verifiable_request_format?
request.content_type.nil? || request.content_type.verify_request?
end

That first part (request.content_type.nil?) always returns true for
DELETE requests, because ActiveResource only sends an Accept header:

# connection.rb
HTTP_FORMAT_HEADER_NAMES = { :get => 'Accept',
:put => 'Content-Type',
:post => 'Content-Type',
:delete => 'Accept'
}

I think the Delete request needs to send both headers, since I think
ActiveResource wants to receive the deleted content as well.

I'm now thinking that ActiveResource is doing the right thing: DELETE
requests should not be sending a Content-Type. It's the
RequestForgeryProtection module that needs a tweak. Instead of this:

      def verifiable_request_format?
        request.content_type.nil? ||
request.content_type.verify_request?
      end

it should be

      def verifiable_request_format?
        (request.content_type.nil? && request.method != :delete) ||
request.content_type.verify_request?
      end

I think the only reason the request.content_type.nil? check is there
is to prevent someone from evading verification by omitting the
content type entirely. However, I think that's fine for DELETE
requests. So if the content type is nil, and it's a DELETE request,
then verify_request? will get a chance to do it's thing (currently,
that side of the expression is short-circuited and never runs).

If this sounds good to everyone, I'll submit a patch this weekend. Not
sure if the door is already closed on 2.2, but it would be good get
this slipped in if possible.

Thanks
Jeff

...Aaaaand now I see someone beat me to it :slight_smile:

http://rails.lighthouseapp.com/projects/8994/tickets/1145-bug-invalidauthenticitytoken-incorrectly-raised-for-xml-controllerdestroy-request#ticket-1145-5

I'll let the people on that thread submit a patch instead.

Sorry for the noise.
Jeff

Hello again,

Looking for +1's and/or feedback on this long-standing ticket, that I
submitted a patch for:

http://rails.lighthouseapp.com/projects/8994-ruby-on-rails/tickets/1145

Executive summary: currently in Rails 2.1, ActiveResource DELETEs
won't work if the server app is using forgery protection. This should
now be fixed by virtue of tightening up the rules that we use for
forgery proteection (only worry about html/url-encoded content
requests).

Most of the insight came from Matthjs and DHH confirmed the aproach -
the question is whether my implementation works for anyone else
besides me :slight_smile:

Thanks!
Jeff