OWASP / railsgoat

A vulnerable version of Rails that follows the OWASP Top 10
railsgoat.cktricky.com
MIT License
858 stars 666 forks source link

Fixed Several Unit Tests #320

Closed nvisium-john-poulin closed 4 years ago

nvisium-john-poulin commented 6 years ago
cktricky commented 6 years ago

@nvisium-john-poulin did you find documentation that led you to the raise_server_errors conclusion?

nvisium-john-poulin commented 6 years ago

@cktricky The actual error message from the CSRF test included the following at the bottom of the exception:

# --- Caused by: ---
 # Capybara::CapybaraError:
 #   Your application server raised an error - It has been raised in your test code because Capybara.raise_server_errors == true
 #   /Users/jpoulin/.rvm/gems/ruby-2.4.3/gems/capybara-2.18.0/lib/capybara/session.rb:145:in `raise_server_error!'

I played around with a little in an attempt to expect { }.to raise_error but could not get this working in our CSRF example. It warrants a bit more effort before we just globally disable the server errors, in my opinion. I'll see if I can play with it a bit more. I think @jmmastey had some ideas?

cktricky commented 6 years ago

Ahh, that makes sense and I agree, warrants a little more discovery/experimentation before we globally disable (if we can achieve it another way). Barring @jmmastey doesn't have some Easy Solution™ 😄, I'll take a look at any other options we might have 👍

jmmastey commented 6 years ago

What if we made this particular test a little funky by adding an explicit rescue to it? The CSRF is the only test that fails, right? We ought to be able to rescue the Capybara::CapybaraError at the very least.

jmmastey commented 6 years ago

Or we could set our CSRF to nil sessions instead of throwing exceptions, I suppose. But catching the exception still seems like the right outcome.

cktricky commented 4 years ago

Talked to @forced-request offline and we might close this in favor of #372. John will let me know (since he no longer has the ability to close this himself)

cktricky commented 4 years ago

I'm gonna close this one for now since I believe the work was done in another PR