Security: Add zero-byte config data rejection (#12)

jkebinger · claude · web-flow · commit 0bde383c6139 · 2025-09-29T14:25:45.000-05:00
* Add zero-byte config data rejection for security Implement comprehensive zero-byte protobuf rejection across all config loading mechanisms to prevent potential security issues from malformed data: Security Features: - Reject zero-byte config data from HTTP responses with IllegalArgumentException - Reject zero-byte config data from cache with IllegalArgumentException - Ignore zero-byte config data from SSE streams with warning logs - Exceptions trigger internal retry logic without propagating to embedding applications Implementation Details: - HttpClient: Added zero-byte validation in requestConfigsFromURI() for HTTP responses and cached data - SseConfigStreamingSubscriber: Added zero-byte detection in FlowSubscriber.onNext() for SSE streams - All rejections logged with appropriate warning messages for debugging Test Coverage: - 4 new HttpClient tests covering zero-byte rejection from HTTP responses, cache, and valid config processing - 5 new SseConfigStreamingSubscriber tests covering zero-byte SSE data, empty payloads, and valid processing - Fixed existing tests to use valid non-zero protobuf data instead of empty default instances 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Bump version to 0.3.27 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> * Update child module versions to 0.3.27 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
diff --git a/micronaut/pom.xml b/micronaut/pom.xml
@@ -4,7 +4,7 @@
   <parent>
     <groupId>com.reforge</groupId>
     <artifactId>sdk-parent</artifactId>
-    <version>0.3.26</version>
+    <version>0.3.27</version>
   </parent>
 
   <artifactId>sdk-micronaut-extension</artifactId>
diff --git a/pom.xml b/pom.xml
@@ -11,7 +11,7 @@
   <groupId>com.reforge</groupId>
   <artifactId>sdk-parent</artifactId>
 
-  <version>0.3.26</version>
+  <version>0.3.27</version>
   <packaging>pom</packaging>
   <name>Reforge SDK Parent POM</name>
   <description>Parent POM for Reforge SDK modules providing feature flags, configuration management, and A/B testing capabilities</description>
diff --git a/sdk/pom.xml b/sdk/pom.xml
@@ -4,7 +4,7 @@
   <parent>
     <groupId>com.reforge</groupId>
     <artifactId>sdk-parent</artifactId>
-    <version>0.3.26</version>
+    <version>0.3.27</version>
   </parent>
 
   <artifactId>sdk</artifactId>
diff --git a/sdk/src/main/java/com/reforge/sdk/internal/HttpClient.java b/sdk/src/main/java/com/reforge/sdk/internal/HttpClient.java
@@ -275,6 +275,10 @@ private CompletableFuture<HttpResponse<Supplier<Prefab.Configs>>> requestConfigs
           // Build a synthetic response for the 200 case.
           Supplier<Prefab.Configs> supplier = () -> {
             try {
+              if (bodyBytes.length == 0) {
+                LOG.warn("Rejecting zero-byte config data from HTTP response");
+                throw new IllegalArgumentException("Zero-byte config data is not valid");
+              }
               return Prefab.Configs.parseFrom(bodyBytes);
             } catch (IOException e) {
               throw new UncheckedIOException(e);
@@ -287,6 +291,10 @@ private CompletableFuture<HttpResponse<Supplier<Prefab.Configs>>> requestConfigs
           // For other status codes, simply wrap the response.
           Supplier<Prefab.Configs> supplier = () -> {
             try (ByteArrayInputStream bais = new ByteArrayInputStream(response.body())) {
+              if (response.body().length == 0) {
+                LOG.warn("Rejecting zero-byte config data from HTTP response");
+                throw new IllegalArgumentException("Zero-byte config data is not valid");
+              }
               return Prefab.Configs.parseFrom(bais);
             } catch (IOException e) {
               throw new UncheckedIOException(e);
@@ -324,6 +332,10 @@ private HttpResponse<Supplier<Prefab.Configs>> createCachedHitResponse(
   ) {
     Supplier<Prefab.Configs> supplier = () -> {
       try {
+        if (entry.data.length == 0) {
+          LOG.warn("Rejecting zero-byte config data from cache");
+          throw new IllegalArgumentException("Zero-byte config data is not valid");
+        }
         return Prefab.Configs.parseFrom(entry.data);
       } catch (IOException e) {
         throw new UncheckedIOException(e);
diff --git a/sdk/src/main/java/com/reforge/sdk/internal/SseConfigStreamingSubscriber.java b/sdk/src/main/java/com/reforge/sdk/internal/SseConfigStreamingSubscriber.java
@@ -117,13 +117,16 @@ public void onNext(Event item) {
           hasReceivedData.set(true);
           String dataPayload = dataEvent.getData().trim();
           if (!dataPayload.isEmpty()) {
-            Prefab.Configs configs = Prefab.Configs.parseFrom(
-              Base64.getDecoder().decode(dataPayload)
-            );
-            if (!configs.hasConfigServicePointer()) {
-              LOG.debug("Ignoring empty config keep-alive");
+            byte[] decodedData = Base64.getDecoder().decode(dataPayload);
+            if (decodedData.length == 0) {
+              LOG.warn("Ignoring zero-byte config data from SSE stream");
             } else {
-              configConsumer.accept(configs);
+              Prefab.Configs configs = Prefab.Configs.parseFrom(decodedData);
+              if (!configs.hasConfigServicePointer()) {
+                LOG.debug("Ignoring empty config keep-alive");
+              } else {
+                configConsumer.accept(configs);
+              }
             }
           }
         } catch (InvalidProtocolBufferException e) {
diff --git a/sdk/src/test/java/com/reforge/sdk/internal/HttpClientTest.java b/sdk/src/test/java/com/reforge/sdk/internal/HttpClientTest.java
@@ -48,7 +48,12 @@ void setup() {
   @Test
   void testFailoverForConfigFetch() throws Exception {
     // Use byte[]–based mocks since requestConfigsFromURI uses BodyHandlers.ofByteArray().
-    Prefab.Configs dummyConfigs = Prefab.Configs.newBuilder().build();
+    Prefab.Configs dummyConfigs = Prefab.Configs
+      .newBuilder()
+      .setConfigServicePointer(
+        Prefab.ConfigServicePointer.newBuilder().setProjectId(123L)
+      )
+      .build();
     byte[] dummyBytes = dummyConfigs.toByteArray();
 
     HttpResponse<byte[]> failureResponse = mock(HttpResponse.class);
@@ -128,7 +133,12 @@ void testFailoverForSSEConnection() throws Exception {
 
   @Test
   void testBasicCaching() throws Exception {
-    Prefab.Configs dummyConfigs = Prefab.Configs.newBuilder().build();
+    Prefab.Configs dummyConfigs = Prefab.Configs
+      .newBuilder()
+      .setConfigServicePointer(
+        Prefab.ConfigServicePointer.newBuilder().setProjectId(123L)
+      )
+      .build();
     byte[] dummyBytes = dummyConfigs.toByteArray();
 
     HttpResponse<byte[]> httpResponse200 = mock(HttpResponse.class);
@@ -175,7 +185,12 @@ void testBasicCaching() throws Exception {
   @Test
   void testConditionalGet304() throws Exception {
     // In order to trigger a conditional GET, we insert a cached entry that is expired.
-    Prefab.Configs dummyConfigs = Prefab.Configs.newBuilder().build();
+    Prefab.Configs dummyConfigs = Prefab.Configs
+      .newBuilder()
+      .setConfigServicePointer(
+        Prefab.ConfigServicePointer.newBuilder().setProjectId(123L)
+      )
+      .build();
     byte[] dummyBytes = dummyConfigs.toByteArray();
     // Use a time far enough in the past to ensure expiration.
     long past = System.currentTimeMillis() - 10_000;
@@ -225,7 +240,12 @@ void testConditionalGet304() throws Exception {
 
   @Test
   void testClearCache() throws Exception {
-    Prefab.Configs dummyConfigs = Prefab.Configs.newBuilder().build();
+    Prefab.Configs dummyConfigs = Prefab.Configs
+      .newBuilder()
+      .setConfigServicePointer(
+        Prefab.ConfigServicePointer.newBuilder().setProjectId(123L)
+      )
+      .build();
     byte[] dummyBytes = dummyConfigs.toByteArray();
 
     HttpResponse<byte[]> httpResponse200 = mock(HttpResponse.class);
@@ -280,7 +300,12 @@ void testClearCache() throws Exception {
   @Test
   void testNoCacheResponseAlwaysRevalidates() throws Exception {
     // Create a valid Prefab.Configs instance and its serialized form.
-    Prefab.Configs dummyConfigs = Prefab.Configs.newBuilder().build();
+    Prefab.Configs dummyConfigs = Prefab.Configs
+      .newBuilder()
+      .setConfigServicePointer(
+        Prefab.ConfigServicePointer.newBuilder().setProjectId(123L)
+      )
+      .build();
     byte[] dummyBytes = dummyConfigs.toByteArray();
 
     // Simulate a 200 response with Cache-Control: no-cache and an ETag.
@@ -365,4 +390,170 @@ void testNoCacheResponseAlwaysRevalidates() throws Exception {
     assertThat(sentRequest.headers().firstValue("If-None-Match"))
       .contains("etag-no-cache");
   }
+
+  @Test
+  void testZeroByteConfigRejectionFromHttpResponse() throws Exception {
+    // Mock a 200 response that returns zero bytes
+    byte[] zeroBytes = new byte[0];
+    HttpResponse<byte[]> zeroByteResponse = mock(HttpResponse.class);
+    when(zeroByteResponse.statusCode()).thenReturn(200);
+    when(zeroByteResponse.body()).thenReturn(zeroBytes);
+    when(zeroByteResponse.headers()).thenReturn(HttpHeaders.of(Map.of(), (k, v) -> true));
+
+    CompletableFuture<HttpResponse<byte[]>> futureZeroBytes = CompletableFuture.completedFuture(
+      zeroByteResponse
+    );
+    when(
+      mockHttpClient.sendAsync(
+        any(HttpRequest.class),
+        any(HttpResponse.BodyHandler.class)
+      )
+    )
+      .thenReturn(futureZeroBytes);
+
+    // Request configs - this should eventually fail after retries
+    CompletableFuture<HttpResponse<Supplier<Prefab.Configs>>> result = prefabHttpClient.requestConfigs(
+      0L
+    );
+
+    HttpResponse<Supplier<Prefab.Configs>> response = result.get();
+    assertThat(response.statusCode()).isEqualTo(200);
+
+    // Try to get the body - this should throw IllegalArgumentException
+    try {
+      response.body().get();
+      assertThat(false)
+        .as("Expected IllegalArgumentException for zero-byte config")
+        .isTrue();
+    } catch (IllegalArgumentException e) {
+      // Should get IllegalArgumentException from zero-byte rejection
+      assertThat(e.getMessage()).contains("Zero-byte config data is not valid");
+    }
+  }
+
+  @Test
+  void testZeroByteConfigRejectionFromNon200Response() throws Exception {
+    // Mock a 404 response that returns zero bytes
+    byte[] zeroBytes = new byte[0];
+    HttpResponse<byte[]> zeroByteResponse = mock(HttpResponse.class);
+    when(zeroByteResponse.statusCode()).thenReturn(404);
+    when(zeroByteResponse.body()).thenReturn(zeroBytes);
+    when(zeroByteResponse.headers()).thenReturn(HttpHeaders.of(Map.of(), (k, v) -> true));
+
+    CompletableFuture<HttpResponse<byte[]>> futureZeroBytes = CompletableFuture.completedFuture(
+      zeroByteResponse
+    );
+    when(
+      mockHttpClient.sendAsync(
+        any(HttpRequest.class),
+        any(HttpResponse.BodyHandler.class)
+      )
+    )
+      .thenReturn(futureZeroBytes);
+
+    // Request configs - this should eventually fail after retries
+    CompletableFuture<HttpResponse<Supplier<Prefab.Configs>>> result = prefabHttpClient.requestConfigs(
+      0L
+    );
+
+    HttpResponse<Supplier<Prefab.Configs>> response = result.get();
+    assertThat(response.statusCode()).isEqualTo(404);
+
+    // Try to get the body - this should throw IllegalArgumentException
+    try {
+      response.body().get();
+      assertThat(false)
+        .as("Expected IllegalArgumentException for zero-byte config")
+        .isTrue();
+    } catch (IllegalArgumentException e) {
+      // Should get IllegalArgumentException from zero-byte rejection
+      assertThat(e.getMessage()).contains("Zero-byte config data is not valid");
+    }
+  }
+
+  @Test
+  void testZeroByteConfigRejectionFromCache() throws Exception {
+    // Insert zero-byte data directly into cache
+    URI uri = URI.create("http://a.example.com/api/v2/configs/0");
+    Field cacheField = HttpClient.class.getDeclaredField("configCache");
+    cacheField.setAccessible(true);
+    @SuppressWarnings("unchecked")
+    Cache<URI, HttpClient.CacheEntry> cache = (Cache<URI, HttpClient.CacheEntry>) cacheField.get(
+      prefabHttpClient
+    );
+
+    // Create a cache entry with zero-byte data that is still fresh
+    long future = System.currentTimeMillis() + 60_000;
+    HttpClient.CacheEntry zeroByteCacheEntry = new HttpClient.CacheEntry(
+      new byte[0],
+      "zero-byte-etag",
+      future
+    );
+    cache.put(uri, zeroByteCacheEntry);
+
+    // Request configs - should return cached response but fail when accessing body
+    CompletableFuture<HttpResponse<Supplier<Prefab.Configs>>> result = prefabHttpClient.requestConfigs(
+      0L
+    );
+
+    HttpResponse<Supplier<Prefab.Configs>> response = result.get();
+    assertThat(response.statusCode()).isEqualTo(200);
+    assertThat(response.headers().firstValue("X-Cache")).contains("HIT");
+
+    // Try to get the body - this should throw IllegalArgumentException
+    try {
+      response.body().get();
+      assertThat(false)
+        .as("Expected IllegalArgumentException for zero-byte cached config")
+        .isTrue();
+    } catch (IllegalArgumentException e) {
+      // Should get IllegalArgumentException from zero-byte rejection
+      assertThat(e.getMessage()).contains("Zero-byte config data is not valid");
+    }
+  }
+
+  @Test
+  void testValidConfigProcessingAfterZeroByteRejectionImplementation() throws Exception {
+    // This test verifies that valid configs can still be processed normally
+    // even when zero-byte rejection is in place
+
+    Prefab.Configs validConfigs = Prefab.Configs
+      .newBuilder()
+      .setConfigServicePointer(
+        Prefab.ConfigServicePointer.newBuilder().setProjectId(456L)
+      )
+      .build();
+    byte[] validBytes = validConfigs.toByteArray();
+    HttpResponse<byte[]> validResponse = mock(HttpResponse.class);
+    when(validResponse.statusCode()).thenReturn(200);
+    when(validResponse.body()).thenReturn(validBytes);
+    when(validResponse.headers()).thenReturn(HttpHeaders.of(Map.of(), (k, v) -> true));
+
+    CompletableFuture<HttpResponse<byte[]>> futureValidBytes = CompletableFuture.completedFuture(
+      validResponse
+    );
+
+    when(
+      mockHttpClient.sendAsync(
+        any(HttpRequest.class),
+        any(HttpResponse.BodyHandler.class)
+      )
+    )
+      .thenReturn(futureValidBytes);
+
+    // Request configs - should succeed with valid data
+    CompletableFuture<HttpResponse<Supplier<Prefab.Configs>>> result = prefabHttpClient.requestConfigs(
+      0L
+    );
+
+    HttpResponse<Supplier<Prefab.Configs>> response = result.get();
+    assertThat(response.statusCode()).isEqualTo(200);
+
+    // Should be able to get valid configs without exception
+    Prefab.Configs configs = response.body().get();
+    assertThat(configs).isEqualTo(validConfigs);
+
+    // Should have called sendAsync once
+    verify(mockHttpClient, times(1)).sendAsync(any(), any());
+  }
 }
diff --git a/sdk/src/test/java/com/reforge/sdk/internal/SseConfigStreamingSubscriberTest.java b/sdk/src/test/java/com/reforge/sdk/internal/SseConfigStreamingSubscriberTest.java
