Merge pull request #26 from FusionAuth/mmanes/header-fix

mmanes · web-flow · commit 08f9b71de340 · 2024-06-07T12:25:40.000-05:00
Add support for non-ASCII header values
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -1,9 +1,9 @@
 name: test
 
-# Run the tests when code is pushed to `master`
+# Run the tests when code is pushed to `main`
 on:
   push:
-    branches: [ master ]
+    branches: [ main ]
 
   # Allows you to run this workflow manually from the Actions tab
   workflow_dispatch:
@@ -13,8 +13,8 @@ jobs:
     runs-on: ubuntu-latest
 
     steps:
-      - uses: actions/checkout@v3
-      - uses: actions/setup-java@v3
+      - uses: actions/checkout@v4
+      - uses: actions/setup-java@v4
         with:
           distribution: 'temurin'
           java-version: 17
@@ -23,9 +23,9 @@ jobs:
           mkdir -p ~/dev/savant
           mkdir -p ~/.savant/plugins
           cd ~/dev/savant
-          curl -fSL https://github.com/savant-build/savant-core/releases/download/2.0.0-RC.6/savant-2.0.0-RC.6.tar.gz > savant.tar.gz
+          curl -fSL https://github.com/savant-build/savant-core/releases/download/2.0.0-RC.7/savant-2.0.0-RC.7.tar.gz > savant.tar.gz
           tar -xzf savant.tar.gz
-          ln -s savant-2.0.0-RC.6 current
+          ln -s savant-2.0.0-RC.7 current
           rm savant.tar.gz
           cat <<EOF > ~/.savant/plugins/org.savantbuild.plugin.java.properties
           17=${JAVA_HOME_17_X64}
@@ -38,7 +38,7 @@ jobs:
         shell: bash
       - name: Archive TestNG reports
         if: failure()
-        uses: actions/upload-artifact@v3
+        uses: actions/upload-artifact@v4
         with:
           name: testng-reports
           path: build/test-reports
diff --git a/build.savant b/build.savant
@@ -14,10 +14,10 @@
  * language governing permissions and limitations under the License.
  */
 jackson5Version = "3.0.1"
- restifyVersion = "4.2.1"
-testngVersion = "7.8.0"
+restifyVersion = "4.2.1"
+testngVersion = "7.10.2"
 
-project(group: "io.fusionauth", name: "java-http", version: "0.3.4", licenses: ["ApacheV2_0"]) {
+project(group: "io.fusionauth", name: "java-http", version: "0.3.5", licenses: ["ApacheV2_0"]) {
   workflow {
     fetch {
       // Dependency resolution order:
@@ -54,7 +54,7 @@ project(group: "io.fusionauth", name: "java-http", version: "0.3.4", licenses: [
 }
 
 // Plugins
-dependency = loadPlugin(id: "org.savantbuild.plugin:dependency:2.0.0-RC.6")
+dependency = loadPlugin(id: "org.savantbuild.plugin:dependency:2.0.0-RC.7")
 java = loadPlugin(id: "org.savantbuild.plugin:java:2.0.0-RC.6")
 javaTestNG = loadPlugin(id: "org.savantbuild.plugin:java-testng:2.0.0-RC.6")
 idea = loadPlugin(id: "org.savantbuild.plugin:idea:2.0.0-RC.7")
diff --git a/java-http.iml b/java-http.iml
@@ -72,11 +72,11 @@
     <orderEntry type="module-library" scope="TEST">
       <library>
         <CLASSES>
-          <root url="jar://$MODULE_DIR$/.savant/cache/org/testng/testng/7.8.0/testng-7.8.0.jar!/" />
+          <root url="jar://$MODULE_DIR$/.savant/cache/org/testng/testng/7.10.2/testng-7.10.2.jar!/" />
         </CLASSES>
         <JAVADOC />
         <SOURCES>
-          <root url="jar://$MODULE_DIR$/.savant/cache/org/testng/testng/7.8.0/testng-7.8.0-src.jar!/" />
+          <root url="jar://$MODULE_DIR$/.savant/cache/org/testng/testng/7.10.2/testng-7.10.2-src.jar!/" />
         </SOURCES>
       </library>
     </orderEntry>
@@ -105,11 +105,11 @@
     <orderEntry type="module-library" scope="TEST">
       <library>
         <CLASSES>
-          <root url="jar://$MODULE_DIR$/.savant/cache/org/webjars/jquery/3.6.1/jquery-3.6.1.jar!/" />
+          <root url="jar://$MODULE_DIR$/.savant/cache/org/webjars/jquery/3.7.1/jquery-3.7.1.jar!/" />
         </CLASSES>
         <JAVADOC />
         <SOURCES>
-          <root url="jar://$MODULE_DIR$/.savant/cache/org/webjars/jquery/3.6.1/jquery-3.6.1-src.jar!/" />
+          <root url="jar://$MODULE_DIR$/.savant/cache/org/webjars/jquery/3.7.1/jquery-3.7.1-src.jar!/" />
         </SOURCES>
       </library>
     </orderEntry>
diff --git a/src/main/java/io/fusionauth/http/server/HTTPRequestProcessor.java b/src/main/java/io/fusionauth/http/server/HTTPRequestProcessor.java
@@ -15,7 +15,9 @@
  */
 package io.fusionauth.http.server;
 
+import java.io.ByteArrayOutputStream;
 import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
 
 import io.fusionauth.http.HTTPMethod;
 import io.fusionauth.http.HTTPValues.Headers;
@@ -34,8 +36,8 @@
 public class HTTPRequestProcessor {
   private final int bufferSize;
 
-  // TODO : Should this be sized with a configuration parameter?
-  private final StringBuilder builder = new StringBuilder();
+  // Allocate a 4k buffer for starters, it will grow as needed.
+  private final ByteArrayOutputStream byteBuffer = new ByteArrayOutputStream(4096);
 
   private final HTTPServerConfiguration configuration;
 
@@ -78,28 +80,27 @@ public RequestState processBodyBytes() {
 
   public RequestState processPreambleBytes(ByteBuffer buffer) {
     while (buffer.hasRemaining()) {
-      // TODO : Can we get some performance using ByteBuffer rather than StringBuilder here?
 
       // If there is a state transition, store the value properly and reset the builder (if needed)
       byte ch = buffer.get();
       RequestPreambleState nextState = preambleState.next(ch);
       if (nextState != preambleState) {
         switch (preambleState) {
-          case RequestMethod -> request.setMethod(HTTPMethod.of(builder.toString()));
-          case RequestPath -> request.setPath(builder.toString());
-          case RequestProtocol -> request.setProtocol(builder.toString());
-          case HeaderName -> headerName = builder.toString();
-          case HeaderValue -> request.addHeader(headerName, builder.toString());
+          case RequestMethod -> request.setMethod(HTTPMethod.of(byteBuffer.toString(StandardCharsets.UTF_8)));
+          case RequestPath -> request.setPath(byteBuffer.toString(StandardCharsets.UTF_8));
+          case RequestProtocol -> request.setProtocol(byteBuffer.toString(StandardCharsets.UTF_8));
+          case HeaderName -> headerName = byteBuffer.toString(StandardCharsets.UTF_8);
+          case HeaderValue -> request.addHeader(headerName, byteBuffer.toString(StandardCharsets.UTF_8));
         }
 
         // If the next state is storing, reset the builder
         if (nextState.store()) {
-          builder.delete(0, builder.length());
-          builder.appendCodePoint(ch);
+          byteBuffer.reset();
+          byteBuffer.write(ch);
         }
       } else if (preambleState.store()) {
         // If the current state is storing, store the character
-        builder.appendCodePoint(ch);
+        byteBuffer.write(ch);
       }
 
       preambleState = nextState;
diff --git a/src/main/java/io/fusionauth/http/util/HTTPTools.java b/src/main/java/io/fusionauth/http/util/HTTPTools.java
@@ -119,8 +119,10 @@ public static boolean isURICharacter(byte ch) {
     return ch >= '!' && ch <= '~';
   }
 
+  // RFC9110 section-5.5 allows for "obs-text", which includes 0x80-0xFF, but really shouldn't be used.
   public static boolean isValueCharacter(byte ch) {
-    return isURICharacter(ch) || ch == ' ' || ch == '\t' || ch == '\n';
+    int intVal = ch & 0xFF;  // Convert the value into an integer without extending the sign bit.
+    return isURICharacter(ch) || intVal == ' ' || intVal == '\t' || intVal == '\n' || intVal >= 0x80;
   }
 
   /**
diff --git a/src/test/java/io/fusionauth/http/BaseTest.java b/src/test/java/io/fusionauth/http/BaseTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022-2023, FusionAuth, All Rights Reserved
+ * Copyright (c) 2022-2024, FusionAuth, All Rights Reserved
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -15,7 +15,6 @@
  */
 package io.fusionauth.http;
 
-import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
 import java.io.OutputStream;
@@ -144,6 +143,18 @@ public HttpClient makeClient(String scheme, CookieHandler cookieHandler) throws
     return builder.connectTimeout(ClientTimeout).build();
   }
 
+  public Socket makeClientSocket(String scheme) throws GeneralSecurityException, IOException {
+    Socket socket;
+    if (scheme.equals("https")) {
+      var ctx = SecurityTools.clientContext(rootCertificate);
+      socket = ctx.getSocketFactory().createSocket("127.0.0.1", 4242);
+    } else {
+      socket = new Socket("127.0.0.1", 4242);
+    }
+
+    return socket;
+  }
+
   public HTTPServer makeServer(String scheme, HTTPHandler handler, Instrumenter instrumenter) {
     return makeServer(scheme, handler, instrumenter, null);
   }
diff --git a/src/test/java/io/fusionauth/http/CoreTest.java b/src/test/java/io/fusionauth/http/CoreTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2022-2023, FusionAuth, All Rights Reserved
+ * Copyright (c) 2022-2024, FusionAuth, All Rights Reserved
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -787,6 +787,54 @@ public void unicode(String scheme) throws Exception {
     }
   }
 
+  @Test(dataProvider = "schemes")
+  public void utf8HeaderValues(String scheme) throws Exception {
+
+    var city = "São Paulo";
+
+    HTTPHandler handler = (req, res) -> {
+      res.setHeader(Headers.ContentType, "text/plain");
+      res.setHeader(Headers.ContentLength, "" + ExpectedResponse.length());
+      res.setHeader("X-Response-Header", city);
+      res.setStatus(200);
+
+      try {
+        OutputStream outputStream = res.getOutputStream();
+        outputStream.write(ExpectedResponse.getBytes());
+        outputStream.close();
+      } catch (IOException e) {
+        throw new RuntimeException(e);
+      }
+    };
+
+    // Java HttpClient only supports ASCII header values, so send request directly
+    try (HTTPServer ignore = makeServer(scheme, handler).start();
+         Socket sock = makeClientSocket(scheme)) {
+
+      var os = sock.getOutputStream();
+      var is = sock.getInputStream();
+      os.write(String.format("""
+                         GET /api/status HTTP/1.1\r
+                         Host: localhost:42\r
+                         X-Request-Header: %s\r
+                         \r
+                         """, city)
+                     .getBytes(StandardCharsets.UTF_8));
+      os.flush();
+
+      var resp = new String(is.readAllBytes(), StandardCharsets.UTF_8);
+
+      assertEquals(resp, String.format("""
+          HTTP/1.1 200 \r
+          content-length: 16\r
+          content-type: text/plain\r
+          connection: keep-alive\r
+          x-response-header: %s\r
+          \r
+          {"version":"42"}""", city));
+    }
+  }
+
   @Test(dataProvider = "schemes")
   public void writer(String scheme) throws Exception {
     HTTPHandler handler = (req, res) -> {

Original file line number	Diff line number	Diff line change
`@@ -119,8 +119,10 @@ public static boolean isURICharacter(byte ch) {`
`119`	`119`	`return ch >= '!' && ch <= '~';`
`120`	`120`	`}`
`121`	`121`
	`122`	`+ // RFC9110 section-5.5 allows for "obs-text", which includes 0x80-0xFF, but really shouldn't be used.`
`122`	`123`	`public static boolean isValueCharacter(byte ch) {`
`123`		`- return isURICharacter(ch) \|\| ch == ' ' \|\| ch == '\t' \|\| ch == '\n';`
	`124`	`+ int intVal = ch & 0xFF; // Convert the value into an integer without extending the sign bit.`
	`125`	`+ return isURICharacter(ch) \|\| intVal == ' ' \|\| intVal == '\t' \|\| intVal == '\n' \|\| intVal >= 0x80;`
`124`	`126`	`}`
`125`	`127`
`126`	`128`	`/**`