Skip to content

fix: guard against a null response Content-Type in BaseClient.makeRequest - #338

Closed
cheekychops wants to merge 1 commit into
recurly:v3-v2021-02-25from
cheekychops:fix/null-content-type-npe
Closed

fix: guard against a null response Content-Type in BaseClient.makeRequest#338
cheekychops wants to merge 1 commit into
recurly:v3-v2021-02-25from
cheekychops:fix/null-content-type-npe

Conversation

@cheekychops

Copy link
Copy Markdown

Fixes #261.

BaseClient.makeRequest reads the response content type and then dereferences it without a null check, in two places:

MediaType contentType = responseBody.contentType();

if (!response.isSuccessful()) {
  if (contentType.type().equals("application") && contentType.subtype().equals("json")) {   // here
    ...
  }
}
...
if (BINARY_TYPES.contains(contentType.type() + "/" + contentType.subtype())) {              // and here

ResponseBody.contentType() returns null whenever the response has no Content-Type header — a 204, or any response where an intermediary drops or malforms the header, since MediaType.parse also returns null on a malformed value. Both branches then throw:

java.lang.NullPointerException: Cannot invoke "okhttp3.MediaType.type()" because "contentType" is null
	at com.recurly.v3.BaseClient.makeRequest(BaseClient.java:174)
	at com.recurly.v3.Client.createSubscription(Client.java:5497)

Issue #261 reported the success-path occurrence in February 2024 and proposed the same one-line guard; this PR adds it on both paths, since we hit the error path in production too.

Why the error path matters as much as the success path

On a non-2xx response the null guard is what lets the existing fallback do its job. With it, ExceptionFactory.getExceptionClass(response) runs and callers get the correct typed exception carrying the status — BadGatewayException for a 502, and so on. Without it, a 502 that happens to arrive without a Content-Type reaches the caller as a bare NullPointerException with the status code lost, which is indistinguishable from a client bug.

Behaviour after the change

  • Non-2xx, no content type — falls through to ExceptionFactory.getExceptionClass(response), so the status-appropriate RecurlyException is thrown instead of an NPE.
  • 2xx, no content type — treated as non-binary and passed to jsonSerializer.deserialize, which returns null for an empty body. A 204 therefore yields null rather than throwing.
  • Any response that does carry a content type — completely unchanged.

Impact seen in production

This reached real customers for us on a Recurly-side response we did not control. In one case Recurly accepted a subscription change and answered with a response the client could not read, so our service raised the NPE and returned a 500 without recording the change. Every subsequent customer retry then got a 422 "The submitted values match the current subscriptions values.", because the change was already applied, and the customer could not complete it at all.

Separately, one occurrence was an Idempotency-Key retry: the original write timed out client-side and the same-key replay came back without a usable content type, so the retry surfaced as an NPE rather than the replayed outcome. The guard matters more than the line count suggests, because an NPE is not a RecurlyException and therefore escapes every catch block that callers write around this SDK.

Tests

Two cases added to BaseClientTest, following the existing testNonJsonError* pattern and using the MockClient.buildResponse(..., MediaType) overload with a null content type:

  • testMissingContentTypeError502 — asserts BadGatewayException rather than NullPointerException.
  • testMissingContentTypeNoContent — asserts a 204 returns null rather than throwing.

Both fail with the exact production NPE when the guard is reverted, and mvn -Dtest=BaseClientTest test is green with it (28 tests, 0 failures). Only BaseClient.java is touched, which carries no generated-code disclaimer.

…uest

ResponseBody.contentType() returns null when a response carries no
Content-Type header (a 204) or a malformed one, since MediaType.parse also
returns null on a malformed value. makeRequest dereferenced it without a null
check on both the error path and the success path, so such responses failed
with a NullPointerException instead of a RecurlyException.

On a non-2xx response the guard lets the existing fallback run, so callers get
the status-appropriate typed exception from ExceptionFactory rather than an NPE
with the status code lost. On a 2xx response the body is treated as non-binary
and deserialized, so a 204 yields null instead of throwing. Responses that do
carry a Content-Type are unaffected.

Adds two BaseClientTest cases following the existing testNonJsonError pattern,
using the MockClient.buildResponse overload that accepts a MediaType.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] if server respond with 204(no content) client getting NPE here

1 participant