Skip to content

Add support for non-ASCII header values #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Jun 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions .github/workflows/test.yml
Original file line number Diff line number Diff line change
@@ -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:
Expand All @@ -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
Expand All @@ -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}
Expand All @@ -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
Expand Down
8 changes: 4 additions & 4 deletions build.savant
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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")
Expand Down
8 changes: 4 additions & 4 deletions java-http.iml
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Expand Down Expand Up @@ -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>
Expand Down
23 changes: 12 additions & 11 deletions src/main/java/io/fusionauth/http/server/HTTPRequestProcessor.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this should be ok? We will be allocating at least 4k on each request - this is quite a bit more than the previous StringBuilder - but I suppose that was likely having to re-allocate the underlying array right away.

Perhaps it is very common for the HTTP request preamble to exceed 4k? Any performance concern on this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume the previous approach did a lot of re-allocations and copying while it was parsing the prelude. This feels large enough to not need re-allocation in most cases, while not going overboard.


private final HTTPServerConfiguration configuration;

Expand Down Expand Up @@ -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;
Expand Down
4 changes: 3 additions & 1 deletion src/main/java/io/fusionauth/http/util/HTTPTools.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to header values, this method is used when parsing the response status message. Does the RFC allow for this type of character there as well? Or is there any risk to using this same logic for both of these use cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That part hasn't been superseded by RFC9110, so according to RFC7230, the status reason can still (unfortunately) contain obs-text.

The reason-phrase element exists for the sole purpose of providing a
textual description associated with the numeric status code, mostly
out of deference to earlier Internet application protocols that were
more frequently used with interactive text clients. A client SHOULD
ignore the reason-phrase content.

 reason-phrase  = *( HTAB / SP / VCHAR / obs-text )

}

/**
Expand Down
15 changes: 13 additions & 2 deletions src/test/java/io/fusionauth/http/BaseTest.java
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -15,7 +15,6 @@
*/
package io.fusionauth.http;

import java.io.File;
import java.io.IOException;
import java.io.InputStream;
import java.io.OutputStream;
Expand Down Expand Up @@ -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);
}
Expand Down
50 changes: 49 additions & 1 deletion src/test/java/io/fusionauth/http/CoreTest.java
Original file line number Diff line number Diff line change
@@ -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.
Expand Down Expand Up @@ -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) -> {
Expand Down