Security fixes

[toolmantim]

Fixes some quite serious security faults with delayed_job_web:

• SQL injection attack via ?queues parameter
• Destructive actions via GET (they're now POST)
• CSRF protection for destructive actions
• Adds Rack::Protection (via an updated Sinatra)

Without these changes, someone could quite easily drop your entire DB via an image tag.

[andyatkinson]

Hi @toolmantim. Fan of your blog back in the day! These look like good changes. I'm helping maintain this gem, so I had a couple questions:

Changing the gets to posts makes sense. The authenticity token makes sense. Would you mind adding tests for the post requests? I realize you didn't author the code originally, but since the code is changing, it is a good time to add some tests to document the functionality.

Are there any downsides to adding session support? I was wondering whether that was necessary for the authenticity token support. I'm guessing it is required, as the CSRF form post value is compared to a value that was set in session, just double checking.

Is this a good version of Sinatra to update to? Apparently this gem was using an old version. I was wondering about upgraders of this gem running into conflicts with sinatra or its dependencies elsewhere within their rails app. If you are using this gem within your rails app, with this version of Sinatra, that is at least one data point that it is ok. I don't have a real world test or data for this, that is why I was asking.

Let me know what you think about these things. Thanks.

[andyatkinson]

Also, would you mind squashing the commits to one with "security fixes" then the individual messages? I prefer that style. Or if you think they are significant to have as separate commits, please let me know. Thanks.

[toolmantim]

Well let's firstly just fix and release the SQL injection and GET/POST. They're both fairly major. I'll open two separate PRs with one commit in each, just so they're visible and trackable. And we can just get them out ASAP. And will fix those POST tests.

The other two look like they raise questions, so they can be in separate PRs and released later.

As for Sinatra versions, what Rails/Ruby is this gem officially compatible with? The latest Sinatra works fine mounted in the latest Rails 3 realease, at least for me.

[andyatkinson]

@toolmantim Ok, I can also squash these and merge them if you want to follow-up with a test, that would be great. I was going to merge it but I saw one failing test from the (limited) existing tests I'm wondering if you could take a look at? It looks like the test is invalid now since the request type changed.

~/Projects/delayed_job_web (security-fixes) $ rake
/Users/andy/.rbenv/versions/2.0.0-p353/bin/ruby -I"lib:lib:test" -I"/Users/andy/.rbenv/versions/2.0.0-p353/lib/ruby/gems/2.0.0/gems/rake-10.1.1/lib" "/Users/andy/.rbenv/versions/2.0.0-p353/lib/ruby/gems/2.0.0/gems/rake-10.1.1/lib/rake/rake_test_loader.rb" "test/**/test_*.rb"
Run options:

# Running tests:

[1/6] TestDelayedJobWeb#test: DelayedJobWeb should get '/enqueued'.  = 0.08 s
  1) Failure:
TestDelayedJobWeb#test: DelayedJobWeb should get '/enqueued'.  [/Users/andy/Projects/delayed_job_web/test/test_delayed_job_web.rb:38]:
NoMethodError - undefined method `order' for Delayed::Job:Class:
    /Users/andy/Projects/delayed_job_web/lib/delayed_job_web/application/app.rb:86:in `block (2 levels) in <class:DelayedJobWeb>'

[toolmantim]

I think separate commits let people understand exactly the problem, and the fix. What benefit does squashing them have?

I'll do separate PR's without the "SEC" where each one can be tested/reviewed.

What version of Ruby + AR are we aiming for? I see lots of 1.8isms

[andyatkinson]

A single commit related to security fixes would be easier to revert if we find issues later. But it is subjective. Multiple commits here are fine. If you want to update the existing test that is failing here on this PR, and push your fix to the same branch, I'll fetch, merge, and push a new gem version.

[toolmantim]

Done: #52 #53

I'll do another with the Sinatra update + CSRF token as a separate PR, as it probably needs a bit more of a check to make sure it works alongside people's Rails apps configurations etc.

[toolmantim]

Also added #55 — feel free to merge all these into master and cut a new gem.