improve validation of SDK key so we won't throw an exception that contains the key (#293)

Author: eli-darkly

src/main/java/com/launchdarkly/sdk/server/LDClient.java

@@ -41,6 +41,7 @@
 import static com.launchdarkly.sdk.EvaluationDetail.NO_VARIATION;
 import static com.launchdarkly.sdk.server.DataModel.FEATURES;
 import static com.launchdarkly.sdk.server.DataModel.SEGMENTS;
+import static com.launchdarkly.sdk.server.Util.isAsciiHeaderValue;
 
 /**
  * A client for the LaunchDarkly API. Client instances are thread-safe. Applications should instantiate
@@ -82,8 +83,15 @@ public final class LDClient implements LDClientInterface {
  * values; it will still continue trying to connect in the background. You can detect whether
  * initialization has succeeded by calling {@link #isInitialized()}. If you prefer to customize
  * this behavior, use {@link LDClient#LDClient(String, LDConfig)} instead.
+ * <p>
+ * For rules regarding the throwing of unchecked exceptions for error conditions, see
+ * {@link LDClient#LDClient(String, LDConfig)}.
  *
  * @param sdkKey the SDK key for your LaunchDarkly environment
+ * @throws IllegalArgumentException if a parameter contained a grossly malformed value;
+ * for security reasons, in case of an illegal SDK key, the exception message does
+ * not include the key
+ * @throws NullPointerException if a non-nullable parameter was null
  * @see LDClient#LDClient(String, LDConfig)
  */
  public LDClient(String sdkKey) {
@@ -136,14 +144,32 @@ private static final DataModel.Segment getSegment(DataStore store, String key) {
  * // do whatever is appropriate if initialization has timed out
  * }
  * </code></pre>
+ * <p>
+ * This constructor can throw unchecked exceptions if it is immediately apparent that
+ * the SDK cannot work with these parameters. For instance, if the SDK key contains a
+ * non-printable character that cannot be used in an HTTP header, it will throw an
+ * {@link IllegalArgumentException} since the SDK key is normally sent to LaunchDarkly
+ * in an HTTP header and no such value could possibly be valid. Similarly, a null
+ * value for a non-nullable parameter may throw a {@link NullPointerException}. The
+ * constructor will not throw an exception for any error condition that could only be
+ * detected after making a request to LaunchDarkly (such as an SDK key that is simply
+ * wrong despite being valid ASCII, so it is invalid but not illegal); those are logged
+ * and treated as an unsuccessful initialization, as described above. 
  *
  * @param sdkKey the SDK key for your LaunchDarkly environment
  * @param config a client configuration object
+ * @throws IllegalArgumentException if a parameter contained a grossly malformed value;
+ * for security reasons, in case of an illegal SDK key, the exception message does
+ * not include the key
+ * @throws NullPointerException if a non-nullable parameter was null
  * @see LDClient#LDClient(String, LDConfig)
  */
  public LDClient(String sdkKey, LDConfig config) {
  checkNotNull(config, "config must not be null");
  this.sdkKey = checkNotNull(sdkKey, "sdkKey must not be null");
+ if (!isAsciiHeaderValue(sdkKey) ) {
+ throw new IllegalArgumentException("SDK key contained an invalid character");
+ }
  this.offline = config.offline;
 
  this.sharedExecutor = createSharedExecutor(config);

src/main/java/com/launchdarkly/sdk/server/Util.java

@@ -37,6 +37,25 @@ static Headers.Builder getHeadersBuilderFor(HttpConfiguration config) {
  return builder;
  }
 
+ // This is specifically testing whether the string would be considered a valid HTTP header value
+ // *by the OkHttp client*. The actual HTTP spec does not prohibit characters >= 127; OkHttp's
+ // check is overly strict, as was pointed out in https://github.com/square/okhttp/issues/2016.
+ // But all OkHttp 3.x and 4.x versions so far have continued to enforce that check. Control
+ // characters other than a tab are always illegal.
+ //
+ // The value we're mainly concerned with is the SDK key (Authorization header). If an SDK key
+ // accidentally has (for instance) a newline added to it, we don't want to end up having OkHttp
+ // throw an exception mentioning the value, which might get logged (https://github.com/square/okhttp/issues/6738).
+ static boolean isAsciiHeaderValue(String value) {
+ for (int i = 0; i < value.length(); i++) {
+ char ch = value.charAt(i);
+ if ((ch < 0x20 || ch > 0x7e) && ch != '\t') {
+ return false;
+ }
+ }
+ return true;
+ }
+ 
  static void configureHttpClientBuilder(HttpConfiguration config, OkHttpClient.Builder builder) {
  builder.connectionPool(new ConnectionPool(5, 5, TimeUnit.SECONDS))
  .connectTimeout(config.getConnectTimeout())

src/test/java/com/launchdarkly/sdk/server/LDClientTest.java

@@ -37,6 +37,9 @@
 import static org.easymock.EasyMock.capture;
 import static org.easymock.EasyMock.expect;
 import static org.easymock.EasyMock.isA;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
@@ -83,6 +86,32 @@ public void constructorWithConfigThrowsExceptionForNullSdkKey() throws Exception
  }
  }
 
+ @Test
+ public void constructorThrowsExceptionForSdkKeyWithControlCharacter() throws Exception {
+ try (LDClient client = new LDClient(SDK_KEY + "\n")) {
+ fail("expected exception");
+ } catch (IllegalArgumentException e) {
+ assertThat(e.getMessage(), not(containsString(SDK_KEY)));
+ }
+ }
+
+ @Test
+ public void constructorWithConfigThrowsExceptionForSdkKeyWithControlCharacter() throws Exception {
+ try (LDClient client = new LDClient(SDK_KEY + "\n", LDConfig.DEFAULT)) {
+ fail("expected exception");
+ } catch (IllegalArgumentException e) {
+ assertThat(e.getMessage(), not(containsString(SDK_KEY)));
+ }
+ }
+
+ @Test
+ public void constructorAllowsSdkKeyToBeEmpty() throws Exception {
+ // It may seem counter-intuitive to allow this, but if someone is using the SDK in offline
+ // mode, or with a file data source or a test fixture, they may reasonably assume that it's
+ // OK to pass an empty string since the key won't actually be used.
+ try (LDClient client = new LDClient(SDK_KEY + "")) {}
+ }
+
  @Test
  public void constructorThrowsExceptionForNullConfig() throws Exception {
  try (LDClient client = new LDClient(SDK_KEY, null)) {