Fix interceptor stack overflow error

[NoahGreer]

The bug:
After creating enough new Client objects, the header interceptor chain grows so large that it causes a java.lang.StackOverflowError error the next time a Client tries to make a request.
This means that in a long-lived web application, using this library creates a ticking time bomb that will eventually cause it to break.

Stack trace:

java.lang.StackOverflowError
	at okhttp3.Headers.newBuilder(Headers.java:144)
	at okhttp3.Request$Builder.<init>(Request.java:140)
	at okhttp3.Request.newBuilder(Request.java:93)
	at com.recurly.v3.http.HeaderInterceptor.intercept(HeaderInterceptor.java:26)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:142)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:117)
	at com.recurly.v3.http.HeaderInterceptor.intercept(HeaderInterceptor.java:33)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:142)
	at okhttp3.internal.http.RealInterceptorChain.proceed(RealInterceptorChain.java:117)
	at com.recurly.v3.http.HeaderInterceptor.intercept(HeaderInterceptor.java:33)
...

The stack trace continues for thousands of lines repeating the last three lines.

To Reproduce:
Steps:
Enter a loop, and once per iteration, create a new Client object, and after every 100 client creations make an API call with the current client object. Eventually, the number of header interceptors will overflow the JVM's stack and cause a java.lang.StackOverflowError.
Note that this is not the actual use case, just a simple brute force example of how to reproduce the issue quickly.

Code to reproduce:

import com.recurly.v3.Client;
import com.recurly.v3.resources.Subscription;

public class Main {

    public static void main(String[] args) {
        final String apiKey = "ATestAPIKeyGoesHere"; 
        final String subscriptionId = "uuid-abcd123456";
        
        final long startMillis = System.currentTimeMillis();
        System.out.println("Starting StackOverflowError repro loop...");
        
        int i = 0;
        while (true) {
            try {
                // This call is the root cause of the issue
                final Client client = new Client(apiKey);
                /* Call API once after every 100 client creations because
                 * this call only shows the symptom but not the root cause.
                 */
                if (i % 100 == 0) {
                    final Subscription sub = client.getSubscription(subscriptionId);
                }
            } catch (final Throwable ex) {
                if (ex instanceof StackOverflowError) {
                    final long endMillis = System.currentTimeMillis();
                    final long durationMillis = endMillis - startMillis;
                    System.out.printf("Caught StackOverflowError on iteration %d after %dms\n", i, durationMillis);
                    ex.printStackTrace();
                    return;
                } else {
                    /* There will be API errors because of an invalid API key, 
                     * but that is not what we are testing, 
                     * so ignore all other errors.
                     */
                }
            }
            
            i++;
        }
    }
}

Example output:

Starting StackOverflowError repro loop...
Caught StackOverflowError on iteration 4600 after 12435ms
...

Followed by a stack trace.

Expected behavior:
Only one of each Interceptor type should be added for each client created, and a java.lang.StackOverflowError should never occur.

Your Environment

• Which version of this library are you using?
  3.5.0
• Which version of the language are you using?
  Java 8

Root cause and fix:
Cause:
The bug is primarily due to the reuse of the same OkHttpClient$Builder object in the BaseClient::newHttpClient method.
The same OkHttpClient$Builder instance is being used for each com.recurly.v3.Client being constructed. A new HeaderInterceptor is mistakenly being created and added to the same builder every time.
On the master branch, in the file BaseClient.java, the lines 53-55 contain the following erroneous code:

    if (!httpClientBuilder.interceptors().contains(headerInterceptor)) {
      httpClientBuilder.addInterceptor(headerInterceptor);
    }

The return type of httpClientBuilder.interceptors() is a List<Interceptor>.
The List::contains method uses object comparison, which requires that the objects being compared must override the Object::equals method to provide a mechanism for determining whether they are the same. Without that method override, all objects are considered unique.
Neither the child class com.recurly.v3.http.HeaderInterceptor nor the parent class okhttp3.Interceptor overrides the Object::equals method, so the above if statement will always evaluate to true and allow a duplicate header interceptor to be added each time.

Another flaw exists on lines 57-62 of the same file as above, code:

    if ("true".equals(System.getenv("RECURLY_INSECURE"))
        && "true".equals(System.getenv("RECURLY_DEBUG"))) {
      final HttpLoggingInterceptor logging = new HttpLoggingInterceptor();
      logging.setLevel(HttpLoggingInterceptor.Level.BASIC);
      httpClientBuilder.addInterceptor(logging);
    }

When the specified environment variables are set, each time a new com.recurly.v3.Client is created, a new HttpLoggingInterceptor will be added to the same builder, and would eventually cause the same java.lang.StackOverflowError when a client makes an API call.

Fix:
The simplest fix that would resolve both these flaws would be to remove the global OkHttpClient$Builder on line 27:
private static OkHttpClient.Builder httpClientBuilder = new OkHttpClient.Builder();
And refactor the BaseClient::newHttpClient method to inline the creation of a new builder.
This would also eliminate the need for checking whether the builder's interceptors list already contains the same interceptors because using a new builder each time would guarantee only one of each type of interceptor is added each time.
Code for refactored method:

  private static OkHttpClient newHttpClient(final String apiKey) {
    final OkHttpClient.Builder httpClientBuilder = new OkHttpClient.Builder();
    
    final String authToken = Credentials.basic(apiKey, "");
    final HeaderInterceptor headerInterceptor =
        new HeaderInterceptor(authToken, Client.API_VERSION);
    httpClientBuilder.addInterceptor(headerInterceptor);

    if ("true".equals(System.getenv("RECURLY_INSECURE"))
        && "true".equals(System.getenv("RECURLY_DEBUG"))) {
      final HttpLoggingInterceptor logging = new HttpLoggingInterceptor();
      logging.setLevel(HttpLoggingInterceptor.Level.BASIC);
      httpClientBuilder.addInterceptor(logging);
    }

    return httpClientBuilder.build();
  }

[NoahGreer]

Just checking in to see if there is anything more I need to do to have this reviewed and merged.

[douglasmiller]

@NoahGreer Thank you very much for reporting this issue and creating a pull request to fix it.

We apologize for taking so long to get back to you on this. We strive to have a much faster turn around, but got caught up in the development of our new golang client.

I was able to reproduce the issue with your example code (again, thank you very much for providing a working POC!). I'm verifying the fix now.

[bhelx]

I apologize also. You made it so easy for us. Great work on the minimal reproducible example!

[douglasmiller]

Everything checks out 👍.

Thank you so much (yes a third time) for such a detailed explanation and fix for this issue!

[douglasmiller]

This will be available with our next release

[NoahGreer]

Thank you both for reviewing and merging this! I appreciate the compliments. :)