Skip to content
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

Jetty 9.4.x 5104 incorrect via header #5144

Merged
merged 8 commits into from
Aug 13, 2020
126 changes: 126 additions & 0 deletions jetty-http/src/main/java/org/eclipse/jetty/http/HttpFields.java
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import java.util.Objects;
import java.util.Set;
import java.util.StringTokenizer;
import java.util.function.BiFunction;
import java.util.function.ToIntFunction;
import java.util.stream.Stream;

Expand Down Expand Up @@ -88,6 +89,115 @@ public HttpFields(HttpFields fields)
_size = fields._size;
}

/**
* <p>Computes a single field for the given HTTP header name and for existing fields with the same name.</p>
*
* <p>The compute function receives the field name and a list of fields with the same name
* so that their values can be used to compute the value of the field that is returned
* by the compute function.
* If the compute function returns {@code null}, the fields with the given name are removed.</p>
* <p>This method comes handy when you want to add an HTTP header if it does not exist,
* or add a value if the HTTP header already exists, similarly to
* {@link Map#compute(Object, BiFunction)}.</p>
*
* <p>This method can be used to {@link #put(HttpField) put} a new field (or blindly replace its value):</p>
* <pre>
* httpFields.computeField("X-New-Header",
* (name, fields) -> new HttpField(name, "NewValue"));
* </pre>
*
* <p>This method can be used to coalesce many fields into one:</p>
* <pre>
* // Input:
* GET / HTTP/1.1
* Host: localhost
* Cookie: foo=1
* Cookie: bar=2,baz=3
* User-Agent: Jetty
*
* // Computation:
* httpFields.computeField("Cookie", (name, fields) ->
* {
* // No cookies, nothing to do.
* if (fields == null)
* return null;
*
* // Coalesces all cookies.
* String coalesced = fields.stream()
* .flatMap(field -> Stream.of(field.getValues()))
* .collect(Collectors.joining(", "));
*
* // Returns a single Cookie header with all cookies.
* return new HttpField(name, coalesced);
* }
*
* // Output:
* GET / HTTP/1.1
* Host: localhost
* Cookie: foo=1, bar=2, baz=3
* User-Agent: Jetty
* </pre>
*
* <p>This method can be used to replace a field:</p>
* <pre>
* httpFields.computeField("X-Length", (name, fields) ->
* {
* if (fields == null)
* return null;
*
* // Get any value among the X-Length headers.
* String length = fields.stream()
* .map(HttpField::getValue)
* .findAny()
* .orElse("0");
*
* // Replace X-Length headers with X-Capacity header.
* return new HttpField("X-Capacity", length);
* });
* </pre>
*
* <p>This method can be used to remove a field:</p>
* <pre>
* httpFields.computeField("Connection", (name, fields) -> null);
* </pre>
*
* @param name the HTTP header name
* @param computeFn the compute function
*/
public void computeField(String name, BiFunction<String, List<HttpField>, HttpField> computeFn)
gregw marked this conversation as resolved.
Show resolved Hide resolved
{
boolean found = false;
ListIterator<HttpField> iterator = listIterator();
while (iterator.hasNext())
{
HttpField field = iterator.next();
if (field.getName().equalsIgnoreCase(name))
{
if (found)
{
// Remove other headers with the same name, since
// we have computed one from all of them already.
iterator.remove();
}
else
{
found = true;
HttpField newField = computeFn.apply(name, Collections.unmodifiableList(getFields(name)));
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not keen on this implementation as for the common cases it will create a bit more garbage than necessary. However I don't really want to hold up this PR any more, so perhaps I'll do a different PR to improve this implementation.... and look at using it in other places.

if (newField == null)
iterator.remove();
else
iterator.set(newField);
}
}
}
if (!found)
{
HttpField newField = computeFn.apply(name, null);
if (newField != null)
put(newField);
Copy link
Contributor

Choose a reason for hiding this comment

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

working on a better impl... meanwhile use add here not put, as you have already iterated and discovered that it is not found.

}
}

public int size()
{
return _size;
Expand Down Expand Up @@ -189,6 +299,22 @@ public List<HttpField> getFields(HttpHeader header)
return fields == null ? Collections.emptyList() : fields;
}

public List<HttpField> getFields(String name)
{
List<HttpField> fields = null;
for (int i = 0; i < _size; i++)
{
HttpField f = _fields[i];
if (f.getName().equalsIgnoreCase(name))
{
if (fields == null)
fields = new ArrayList<>();
fields.add(f);
}
}
return fields == null ? Collections.emptyList() : fields;
}

public boolean contains(HttpField field)
{
for (int i = _size; i-- > 0; )
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@
import java.util.Set;
import java.util.concurrent.Executor;
import java.util.concurrent.TimeoutException;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import javax.servlet.AsyncContext;
import javax.servlet.ServletConfig;
import javax.servlet.ServletContext;
Expand All @@ -48,6 +50,7 @@
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpHeaderValue;
import org.eclipse.jetty.http.HttpStatus;
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.util.HttpCookieStore;
import org.eclipse.jetty.util.ProcessorUtils;
import org.eclipse.jetty.util.StringUtil;
Expand Down Expand Up @@ -452,6 +455,14 @@ protected boolean expects100Continue(HttpServletRequest request)
return HttpHeaderValue.CONTINUE.is(request.getHeader(HttpHeader.EXPECT.asString()));
}

protected Request newProxyRequest(HttpServletRequest request, String rewrittenTarget)
{
return getHttpClient().newRequest(rewrittenTarget)
.method(request.getMethod())
.version(HttpVersion.fromString(request.getProtocol()))
.attribute(CLIENT_REQUEST_ATTRIBUTE, request);
}

protected void copyRequestHeaders(HttpServletRequest clientRequest, Request proxyRequest)
{
// First clear possibly existing headers, as we are going to copy those from the client request.
Expand Down Expand Up @@ -513,9 +524,50 @@ protected void addProxyHeaders(HttpServletRequest clientRequest, Request proxyRe
addXForwardedHeaders(clientRequest, proxyRequest);
}

/**
* Adds the HTTP {@code Via} header to the proxied request.
*
* @param proxyRequest the request being proxied
* @see #addViaHeader(HttpServletRequest, Request)
*/
protected void addViaHeader(Request proxyRequest)
{
proxyRequest.header(HttpHeader.VIA, "http/1.1 " + getViaHost());
HttpServletRequest clientRequest = (HttpServletRequest)proxyRequest.getAttributes().get(CLIENT_REQUEST_ATTRIBUTE);
addViaHeader(clientRequest, proxyRequest);
}

/**
* <p>Adds the HTTP {@code Via} header to the proxied request, taking into account data present in the client request.</p>
* <p>This method considers the protocol of the client request when forming the proxied request. If it
* is HTTP, then the protocol name will not be included in the {@code Via} header that is sent by the proxy, and only
* the protocol version will be sent. If it is not, the entire protocol (name and version) will be included.
* If the client request includes a {@code Via} header, the result will be appended to that to form a chain.</p>
*
* @param clientRequest the client request
* @param proxyRequest the request being proxied
* @see <a href="https://tools.ietf.org/html/rfc7230#section-5.7.1">RFC 7230 section 5.7.1</a>
*/
protected void addViaHeader(HttpServletRequest clientRequest, Request proxyRequest)
{
String protocol = clientRequest.getProtocol();
String[] parts = protocol.split("/", 2);
// Retain only the version if the protocol is HTTP.
String protocolPart = parts.length == 2 && "HTTP".equalsIgnoreCase(parts[0]) ? parts[1] : protocol;
String viaHeaderValue = protocolPart + " " + getViaHost();
proxyRequest.getHeaders().computeField(HttpHeader.VIA.asString(), (name, viaFields) ->
{
if (viaFields == null || viaFields.isEmpty())
return new HttpField(name, viaHeaderValue);
String separator = ", ";
String newValue = viaFields.stream()
Copy link
Contributor

Choose a reason for hiding this comment

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

I trust that this fluent style will resolve to something efficient enough for the common case of a single Via field? If not then it might not be a bad idea to have special handling for viaFields.size()==1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gregw lost in the rest of the proxying it's a case where I'd prefer less code than more special case handling.
The typical case is that we are the only proxy, so viaFields.size()==0, which is already optimized.

.flatMap(field -> Stream.of(field.getValues()))
.filter(value -> !StringUtil.isBlank(value))
.collect(Collectors.joining(separator));
if (newValue.length() > 0)
newValue += separator;
newValue += viaHeaderValue;
return new HttpField(HttpHeader.VIA, newValue);
});
}

protected void addXForwardedHeaders(HttpServletRequest clientRequest, Request proxyRequest)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@
import org.eclipse.jetty.client.api.Result;
import org.eclipse.jetty.client.util.DeferredContentProvider;
import org.eclipse.jetty.http.HttpHeader;
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.io.ByteBufferPool;
import org.eclipse.jetty.io.RuntimeIOException;
import org.eclipse.jetty.util.BufferUtil;
Expand Down Expand Up @@ -92,9 +91,7 @@ protected void service(HttpServletRequest clientRequest, HttpServletResponse pro
return;
}

final Request proxyRequest = getHttpClient().newRequest(rewrittenTarget)
.method(clientRequest.getMethod())
.version(HttpVersion.fromString(clientRequest.getProtocol()));
Request proxyRequest = newProxyRequest(clientRequest, rewrittenTarget);

copyRequestHeaders(clientRequest, proxyRequest);

Expand All @@ -115,7 +112,6 @@ protected void service(HttpServletRequest clientRequest, HttpServletResponse pro

if (expects100Continue(clientRequest))
{
proxyRequest.attribute(CLIENT_REQUEST_ATTRIBUTE, clientRequest);
proxyRequest.attribute(CONTINUE_ACTION_ATTRIBUTE, (Runnable)() ->
{
try
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
import org.eclipse.jetty.client.api.Result;
import org.eclipse.jetty.client.util.DeferredContentProvider;
import org.eclipse.jetty.client.util.InputStreamContentProvider;
import org.eclipse.jetty.http.HttpVersion;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.IteratingCallback;

Expand Down Expand Up @@ -76,9 +75,7 @@ protected void service(final HttpServletRequest request, final HttpServletRespon
return;
}

final Request proxyRequest = getHttpClient().newRequest(rewrittenTarget)
.method(request.getMethod())
.version(HttpVersion.fromString(request.getProtocol()));
Request proxyRequest = newProxyRequest(request, rewrittenTarget);

copyRequestHeaders(request, proxyRequest);

Expand All @@ -95,7 +92,6 @@ protected void service(final HttpServletRequest request, final HttpServletRespon
{
DeferredContentProvider deferred = new DeferredContentProvider();
proxyRequest.content(deferred);
proxyRequest.attribute(CLIENT_REQUEST_ATTRIBUTE, request);
proxyRequest.attribute(CONTINUE_ACTION_ATTRIBUTE, (Runnable)() ->
{
try
Expand Down
Loading