search

tumblr / jumblr

Tumblr API v2 Java Client
Apache License 2.0
278 stars 105 forks source link

Handle empty response bodies #33

Closed orac closed 10 years ago

orac commented 10 years ago

I've seen a problem in production where RequestBuilder.clear NPEs because gson.fromJson returns null, which it does when the passed-in string (which is the response body) is empty.

I've added a unit test for this case. The test looks a bit silly because it relies on mocking the response with a really implausible one. There's nothing to stop you writing more mock responses with nonsense values, which is the main shortcoming of this kind of test. But in this case, this is just a regression test for a bug I've seen in the wild, not just an invalid situation I've made up. Long-term, it would be better to be able to plug in a fake server and test the whole response decoding pipeline: since the API doesn't use SSL, the 'server' could be sending any garbage down the line.

Armed with the test case, I've fixed the bug, and also found and fixed a limitation of JumblrException: it couldn't handle an empty response body either.

seejohnrun commented 10 years ago

Heya - thank you for the PR Do you have any idea of what types of requests you're seeing empty bodies on?

Just want to make sure we're not covering up a larger issue with this

orac commented 10 years ago

I've never seen this myself, but I've added more logging in production now to see this going wrong in live conditions. It can happen when the user's on a captive Wi-Fi network, so the response isn't coming from Tumblr at all but from the login page of the Wi-Fi network.

The bigger issue this is covering up is that it's not possible to use SSL with certificate pinning with this library, so the remote endpoint might not be Tumblr at all.

seejohnrun commented 10 years ago

Hmm that's an interesting case - what do you think the appropriate action to take is then? This PR or a larger feature?

orac commented 10 years ago

I would like this fix integrated for now so that I don't have to have local changes to avoid my app crashing in production. Long-term, I think it's in your interest as well as your users' to protect your API from MITM attacks by adding SSL with certificate pinning.

seejohnrun commented 10 years ago

Okay - that all sounds good - thanks for the PR