Ignore SSL errors #176

Closed
wants to merge 6 commits into
from

Conversation

Projects
None yet
6 participants
Contributor

rectalogic commented Sep 27, 2011

Ignore SSL errors, e.g. when testing against a staging server with self-signed certs etc.

Contributor

niklasb commented Sep 28, 2011

I'd very much like to see this as an optional feature, but as I understand it, you disable certificate validation permanently by default, which is to be considered very bad in regards of security (and actually makes the use of SSL completely useless). Have I missed something?

I agree with @niklasb, this should be optional. I would love this feature though, it's actually stopping me from using capybara-webkit with a current project. Based on another similar discussion (#152) this feature would need an accompanying test to be accepted.

since this is a testing framework... I wouldn't consider it "very bad" or even "making ssl useless." It still allows you to test your production or development environments for acceptance.

Contributor

niklasb commented Sep 28, 2011

@tobowers That's true, for that (main) purpose it's not that much of a security problem. Capybara can however also be used for screen scraping, where I consider certificate validation to be important.
Don't get me wrong, I definitely love this feature, but IMHO ignoring SSL validation errors should neither be the only option, nor should it be the default. Rather, there should be some IgnoreSSLErrors command that sets an internal flag that is checked when an SSL error occurs.
@rectalogic If you want, I can write an integration test for this feature.

Make ignoring SSL errors an option.
webkit_server takes an --ignore-ssl-errors argument.
Contributor

rectalogic commented Sep 28, 2011

OK, so I added a new command line option to webkit_server, --ignore-ssl-errors. So by default this won't be specified and we won't ignore errors.

What needs to be done now (and I don't have time to do this) is to expose this through the ruby API, e.g. add :ignore_ssl_errors to the options for Capybara::Driver::Webkit::Browser and it can popen webkit_server with --ignore-ssl-errors or not based on the options.

Contributor

niklasb commented Sep 28, 2011

@rectalogic as soon as I find the time, I can add the option to Capybara::Driver::Webkit::Browser and write a test for your new functionality. I'll let you know when I got this finished so you can merge it.

Contributor

niklasb commented Sep 28, 2011

Contributor

niklasb commented Sep 29, 2011

@rectalogic I pushed some fixes, can you also include them?

Owner

jferris commented Oct 14, 2011

I get two test failures after merging this:

Failures:

  1) Capybara::Driver::Webkit::Browser handling of SSL validation errors doesn't accept a self-signed certificate by default
     Failure/Error: @server.shutdown
     Errno::ENOTCONN:
       Socket is not connected
     # ./spec/browser_spec.rb:71:in `shutdown'
     # ./spec/browser_spec.rb:71

  2) Capybara::Driver::Webkit::Browser handling of SSL validation errors accepts a self-signed certificate if configured to do so
     Failure/Error: @server.shutdown
     Errno::ENOTCONN:
       Socket is not connected
     # ./spec/browser_spec.rb:71:in `shutdown'
     # ./spec/browser_spec.rb:71

Finished in 169.71 seconds
614 examples, 2 failures

I used Ruby 1.8.7-p334.

Contributor

niklasb commented Oct 14, 2011

I only tested on Ruby 1.9.1 but will fix it for Ruby 1.8.7

Contributor

niklasb commented Oct 14, 2011

It passes for me on Ruby 1.8.7 if I make a little change in browser.rb (will push it to ignore-ssl-errors). I cannot reproduce the exception you describe on Ruby 1.8.7-p352 [x86_64]. Can you debug this on your machine? The actual specs seem to pass, only the destruction of the temporary SSL-enabled server fails.

niklasb added a commit to niklasb/capybara-webkit that referenced this pull request Oct 14, 2011

Contributor

niklasb commented Oct 14, 2011

@jferris: can you try whether the specs pass if you replace shutdown with close?

niklasb added a commit to niklasb/capybara-webkit that referenced this pull request Oct 14, 2011

Contributor

halogenandtoast commented Oct 14, 2011

I might be misinterpreting the difference between shutdown and close and I wanted to clarify. #shutdown sends the shutdown(2) signal which will cause the full-duplex connection to shut down on both ends. We're currently running on OSX so not sure why ENOTCONN is being raised. However I don't see why you'd have both a shutdown and a close. If shutdown isn't supported won't close go through each of the file descriptors for the duplex and close them?

Contributor

niklasb commented Oct 14, 2011

@halogenandtoast I noticed that the listening port cannot be listened upon right after simply closing the socket. As I am however sure that the server and its connections will not be used any longer after the tests are run, I thought it might be better to shutdown the socket before closing, so that there are no dangling connections. Most of the information that I got about this is from http://stackoverflow.com/questions/409783/socket-shutdown-vs-socket-close . I am open to suggestions here.

Quote from the thread regarding shutdown:
"However, it does not deallocate the socket and you still need to call close afterward."
Robert S. Barnes

Contributor

halogenandtoast commented Oct 14, 2011

Since OSX doesn't seem to work with the shutdown, I just want to confirm that #shutdown followed by #close doesn't error out with something like IOError: closed stream

niklasb added a commit to niklasb/capybara-webkit that referenced this pull request Oct 14, 2011

Contributor

niklasb commented Oct 14, 2011

I removed the questionable code. Even if connections are dangling, the specs should always run without problems. Current code lives in branch ignore-ssl-errors

halogenandtoast added a commit that referenced this pull request Oct 14, 2011

halogenandtoast added a commit that referenced this pull request Oct 14, 2011

Contributor

halogenandtoast commented Oct 14, 2011

Merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment