Strict parsing of JSON response results in uncaught exceptions

[bryfox]

Describe the problem

When a server sends an invalid JSON response (for example, an empty body); the response parsing throws an exception and crashes the sample app.

What was the expected behavior?

It would be nice to be more lenient about an invalid response, and propagate it (or something relevant) in the onFailure callback.

Reproduction

1. Using SDK + sample app on main branch (at 4b11fd6)
2. Using a modified server response, return from /oauth/token:
   • Content-Type: application/json
   • Empty (or other invalid JSON) response body
3. Open sample app
4. Click "Log in with username & password"
5. Observe crash with whatever exception is thrown from Gson

Environment

• Version of this library used: 4b11fd6
• Which framework are you using, if applicable: only this repo
• Other modules/plugins/libraries that might be involved: none
• Any other relevant information you think would be useful:

[lbalmaceda]

@bryfox thanks for the report. Although this applies to a crafted and unexpected response, have you come across a valid use case where the server would return an empty object?

[bryfox]

Not under normal conditions, but we experienced a number of crashes in production during a short burst of high traffic for this reason. This app has not yet been updated to v2, but faces a similar situation:

Fatal Exception: java.lang.NullPointerException: Attempt to invoke interface method 'int java.util.Map.size()' on a null object reference
       at java.util.HashMap.putMapEntries(HashMap.java:500)
       at java.util.HashMap.<init>(HashMap.java:489)
       at com.auth0.android.authentication.AuthenticationException.<init>(AuthenticationException.java:79)
       at com.auth0.android.request.internal.AuthenticationErrorBuilder.from(AuthenticationErrorBuilder.java:29)
       at com.auth0.android.request.internal.AuthenticationErrorBuilder.from(AuthenticationErrorBuilder.java:12)
       at com.auth0.android.request.internal.BaseRequest.parseUnsuccessfulResponse(BaseRequest.java:139)
       at com.auth0.android.request.internal.SimpleRequest.onResponse(SimpleRequest.java:72)
       at com.squareup.okhttp.Call$AsyncCall.execute(Call.java:177)
       at com.squareup.okhttp.internal.NamedRunnable.run(NamedRunnable.java:33)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
       at java.lang.Thread.run(Thread.java:919)

[lbalmaceda]

In the first comment, you point to a commit from the branch main (v2). However, above you say you have not yet migrated to v2. Can you please clarify in which version you are observing this issue?

Also, I'm trying to pinpoint the endpoint that was used. Have you seen any interesting logs on the Auth0 dashboard when this happened? We should handle this scenario and fail through the provided callback as you suggest. But would like to understand how to reproduce this without intercepting the server response.

[bryfox]

Sorry for the confusion! In our production app, we're using Lock, and the corresponding version of this SDK (with a couple of minor customizations we made, but that shouldn't matter for this discussion). Given the recent v1 deprecation, I wanted to see if this will still be an issue with v2; I have reproduced with the v2 sample app only by proxying the response.

Unfortunately, I can't see back that far in the Auth0 logs (> 2 days). Our web clients also reported error responses from the same timeframe — 503 and 429 status codes, in case that's of help in narrowing down the behavior.

[lbalmaceda]

@bryfox I came back to this today. To summarize your situation as I understand it (please confirm), you are currently using Lock.Android v2 and Auth0.Android v1 (comes as a dependency from Lock). Your users often run into exceptions when parsing an error using the from(Map) method from the SDK v1; which is why you opened this in the first place.

You said you tried making Lock use the SDK v2 and you still run into this scenario. May I ask how you are making it use the SDK v2? Because the stack trace above is really specific to v1. The class AuthenticationErrorBuilder doesn't exist anymore for v2.

If you have a stack trace for v2 that you can share it will help us debug this behavior. Kotlin enforces nullability checks so it should be fairly easy to spot, but the compiler doesn't warn us of anything today.

[bryfox]

Your initial summary is mostly correct, except that this happened only very rarely. There were a number of users affected, but all crashes occurred within the span of a minute, during an unusual spike in traffic. We saw a similar burst in traffic a week later, and a similar number of crashes; otherwise, I don't believe we ever encountered this.

I haven't yet done anything with Lock and SDK v2. With v2, I only used the sample app in this repo.

Here are the basic steps I took for that synthetic test (forcing an empty response body)... In order to MITM responses, I updated sample by trusting user certs in the network security config (and I'm happy to provide details if needed). To mimic what we seemed to encounter in the wild, I rewrote the response from the token endpoint to have (a) an empty body and (b) a 500 status. In that case, the error adapter produces this:

com.auth0.sample E/AndroidRuntime: FATAL EXCEPTION: pool-1-thread-1
    Process: com.auth0.sample, PID: 6930
    java.lang.Error: java.io.EOFException: End of input at line 1 column 1 path $
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1173)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641)
        at java.lang.Thread.run(Thread.java:919)
     Caused by: java.io.EOFException: End of input at line 1 column 1 path $
        at com.google.gson.stream.JsonReader.nextNonWhitespace(JsonReader.java:1397)
        at com.google.gson.stream.JsonReader.doPeek(JsonReader.java:550)
        at com.google.gson.stream.JsonReader.peek(JsonReader.java:426)
        at com.google.gson.TypeAdapter$1.read(TypeAdapter.java:195)
        at com.google.gson.TypeAdapter.fromJson(TypeAdapter.java:260)
        at com.auth0.android.request.internal.GsonAdapter.fromJson(GsonAdapter.kt:59)
        at com.auth0.android.authentication.AuthenticationAPIClient$Companion$createErrorAdapter$1.fromJsonResponse(AuthenticationAPIClient.kt:678)
        at com.auth0.android.authentication.AuthenticationAPIClient$Companion$createErrorAdapter$1.fromJsonResponse(AuthenticationAPIClient.kt:664)
        at com.auth0.android.request.internal.BaseRequest.execute(BaseRequest.kt:116)
        at com.auth0.android.request.internal.BaseRequest$start$1.run(BaseRequest.kt:67)
        at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1167)
        at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:641) 
        at java.lang.Thread.run(Thread.java:919) 

Note if I leave the response status as successful, then the onSuccess callback in the app may be sent a null result, despite the non-null type. (Though I have no reason to think we ever saw a 2xx response with an empty body.)

com.auth0.sample E/AndroidRuntime: FATAL EXCEPTION: main
    Process: com.auth0.sample, PID: 7028
    java.lang.NullPointerException: Parameter specified as non-null is null: method kotlin.jvm.internal.Intrinsics.checkNotNullParameter, parameter result
        at com.auth0.sample.DatabaseLoginFragment$dbLogin$1.onSuccess(Unknown Source:2)
        at com.auth0.sample.DatabaseLoginFragment$dbLogin$1.onSuccess(DatabaseLoginFragment.kt:55)
        at com.auth0.android.request.internal.BaseRequest$start$1$1.run(BaseRequest.kt:69)
        at android.os.Handler.handleCallback(Handler.java:883)
        at android.os.Handler.dispatchMessage(Handler.java:100)
        at android.os.Looper.loop(Looper.java:214)
        at android.app.ActivityThread.main(ActivityThread.java:7356)
        at java.lang.reflect.Method.invoke(Native Method)
        at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:492)
        at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:930)

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you have not received a response for our team (apologies for the delay) and this is still a blocker, please reply with additional information or just a ping. Thank you for your contribution! 🙇‍♂️

[lbalmaceda]

@bryfox I still didn't encounter the scenario in which the server responded with an empty string response and we were expecting something different. I haven't played with proxying the request and changing the response. But I've been doing some research to try to narrow this down and see if we can cover it anyways. This is what I found.

Gson, the JSON library we use to deserialize the responses from the server, has different ways of deserializing JSON. Aparently, using gson#fromJson(type, input) would parse JSON in a "null safe" way, returning null in cases where the input is an empty string.

Using gson#getAdapter(type) instead, and then adapter.fromJson(input) would fail with an java.io.EOFException if the input is an empty string. But that's not the case for the deserializer that we specify explicitly as part of the Gson configuration. We do that for a few types, as you see here. Those implement the JsonDeserializer<T> class. I tested that myself.

// ### Without a custom deserializer
// returns null
gson.fromJson(" ", String::class.java)
// throws java.io.EOFException
gson.getAdapter(String::class.java).fromJson(" ")


// ### With a custom deserializer
// returns null
gson.fromJson(" ", Credentials::class.java)
// returns null
gson.getAdapter(Credentials::class.java).fromJson(" ")

Even when we have code in our custom deserializers (adapters) that would assert for empty or invalid input, that never gets called because Gson checks the input before invoking the adapter.
https://github.com/auth0/Auth0.Android/blob/main/auth0/src/main/java/com/auth0/android/request/internal/CredentialsDeserializer.kt#L19-L21

See the TreeTypeAdapter class from Gson.

But, according to this comment, we should be implementing TypeAdapter<T> instead if we want empty strings to throw an exception. The problem is that implementing this class makes you read a buffer part by part. And just like when you want to unparcel a Parcelable, you have to do that in the same order that it was parceled first. That implies that the order in which the properties of the JSON input that comes from the server matters, as a different order would break the implementation. Because we can't guarantee that the serialization strategy from the server won't change, and we don't want to introduce JSON parsing errors, we should not chase down this path.

I guess it could be a good idea to explore checking if the reader instance received here is empty or not. But because it's a stream and not a fixed string, it's not straightforward to check its length.

https://github.com/auth0/Auth0.Android/blob/main/auth0/src/main/java/com/auth0/android/request/internal/GsonAdapter.kt#L57-L59

Do you have any other ideas that we could try? Cheers

[bryfox]

@lbalmaceda I'm no longer working on the app that uses this library, but it looks like you now handle the ErrorAdapter cases (#486). So I can no longer reproduce with the sample app given the steps above and using a 500 status. I suspect that was the case seen in the wild.

Sounds like you're being cautious and looking to handle the RequestAdapter case (status 200 + empty body) as well, without reworking your Gson implementation too much. I think one approach, based on the NPE stack trace above, would be to explicitly check for that null (TypeAdapter.java#L255) and make sure that doesn't get passed through as your Callback result that expects non-null Credentials. e.g., in that last GsonAdapter link you posted, you could handle the returned null value (perhaps by throwing) rather than worrying about the input stream itself.

[lbalmaceda]

@bryfox Coming back to this after some days. 👋

Thanks for your suggestion. According to that, I should be stopping a potentially null value from even reaching the "success callback". And with the context in this issue thread, that would be for successful responses from the server (status code 200).

So basically, take this line and change it to check the output value is NOT null before returning it. Otherwise, throw an exception (i.e parsing exception). However, we would be "failing the request" after a successful server response. Don't get me wrong, I think that's better than getting an unhandled runtime exception.

Still, the BaseRequest class doesn't have the ability to tell if that T type that we should convert responses to is a "nullable" type or not. There are use cases in which we don't expect a response back and use the nullable type Void?. One example is the resetPassword method. In that implementation, the fromJson method returns null every time. So a null check like the one proposed would prevent these types of requests from succeeding.

I'm open to improving this JSON parsing logic, but I think we already invested quite a time in fixing the SDK for a use case that we couldn't even reproduce with the existing endpoints. I'll close this issue for now assuming that the PR you linked already fixed this use case, but I'm happy to re-open it if a valid use case that reproduces this is found. Thanks for sticking with the conversation. 🏅