There's a fight here between DatabaseCleaner strategies - simpler to use
the default :transaction for model specs, but Capybara lives in a
different world where different connections are in play and
:transactions don't work. So, while introducing the more cumbersome
(though with more control) DatabaseCleaner gem and its truncation
strategy, I forgot to make sure the model specs had the fixtures present
that they depend on. This is fixed up now.
The user spec for invalid email was also failing - the regex there is
not savvy enough to handle rejecting two @ signs, so I made the invalid
value something still invalid to get it passing -- real regex validation
of email is ... impossible, so we'll roll with this and move on.
This isn't the cleanest approach, but should be good for now.
Obviously, there are two contexts for these specs: one is from the
maintainer's standpoint, the other is from the trainee who is using
RailsGoat for training.
The maintainer wants all of these specs to pass, to ensure the
vulnerabilities are still functional as vulnerabilities.
The trainee could potentially use these specs (though reading the specs
contains spoilers) to track and verify their fixes.
I've wired in a pending block around each assertion that checks a method
to see what the result of the pending call would be. You can see
examples of how this works with conditions here:
https://www.relishapp.com/rspec/rspec-core/v/2-14/docs/pending/pending-examples
This means these specs will all fail now by default (the trainee
context), but will pass, when vulnerable, if the RAILSGOAT_MAINTAINER
env var is set.
The only flaw at the moment is that in the trainee context, fixing the
vulnerabilities will result in the specs going from failing to
_pending_, not passing (which makes sense, given how we're using RSpec's
pending functionality).
Maybe it'd be simpler/better to have a boolean toggle of our own somehow
wrap the assertions in blocks to do explicitly what we want (flip-flop
the result based on the context).
This includes two scenarios - the work_info one mentioned in the
tutorials, but also one allowing downloading of source code, which may
belong somewhere else as I haven't worked through all the tutorials yet.
Adding Capybara to verify replay-ability of hacking vulnerabilities. I
imagine these may want to be kept on a different branch for QA and
educational purposes, but not distributed with master when forked.
This commit also includes demonstrating the SQL Injection vulnerability.
Disclaimer: changes like these in this sort of app are tricky because
it's harder to presume the intention of the code in question.
The prior line:
```
user.update_attributes(params[:user].reject { |k| k == ("password" || "password_confirmation") || "user_id" })
```
returns an empty hash, because of the way the block evaluates:
```
irb(main):002:0> k = 'foo'
=> "foo"
irb(main):003:0> k == ("password" || "password_confirmation") || "user_id"
=> "user_id"
```
Before the last change to that line, without 'user_id' outside the
params, it didn't evaluate properly either:
```
irb(main):007:0> k = 'password_confirmation'
=> "password_confirmation"
irb(main):008:0> k == ("password" || "password_confirmation")
=> false
irb(main):009:0> ("password" || "password_confirmation")
=> "password"
```
So, in the normal use case for this form, you can't update any other
attribute of the User. To me, that's probably the best argument for
making this change, but it does simplify the SQL Injection attack
(although perhaps the prior complication was intended).
Before this change, injecting conditional SQL into the user_id param in
the account_settings update call would allow the password of whatever
account is found (e.g. the first one if injecting 'OR 1=1') to be reset,
but without additional attacks, the email address of that account is not
known.
After this change, the email address of that account now is also updated
in addition to the password, making it simpler to get in as an admin --
though you're still presuming the first account to be an admin.
Mike McCabe