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.

Sign up for GitHub

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

Jump to bottom

Compliance Violation Listener cleanup of alternate proposal #11301

Closed
wants to merge 1 commit into from
Closed

Compliance Violation Listener cleanup of alternate proposal #11301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
73 changes: 2 additions & 71 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ interface Listener

/**
* Initialize the listener in preparation for a new request life cycle.
* @return The {@link Listener} instance to use for the request life cycle.
* @return The Listener instance to use for the request life cycle.
*/
default Listener initialize()
{
Expand All @@ -119,7 +119,7 @@ default void onRequestBegin(Attributes request)
/**
* A Request has ended.
*
* @param request the request attribtues, or null if Request does not exist yet (eg: during handling of a {@link BadMessageException})
* @param request the request attributes, or null if Request does not exist yet (eg: during handling of a {@link BadMessageException})
*/
default void onRequestEnd(Attributes request)
{
Expand Down Expand Up @@ -149,75 +149,6 @@ default void onComplianceViolation(Mode mode, ComplianceViolation violation, Str
}
}

/**
* A Listener that represents multiple user {@link ComplianceViolation.Listener} instances
*/
class ListenerCollection implements Listener
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to AbstractConnector (as it was the only place using it)

{
private static final Logger LOG = LoggerFactory.getLogger(ListenerCollection.class);
private final List<ComplianceViolation.Listener> userListeners;

/**
* Construct a new ComplianceViolations that will notify user listeners.
*
* @param userListeners the user listeners to notify, null or empty is allowed.
*/
public ListenerCollection(List<ComplianceViolation.Listener> userListeners)
{
this.userListeners = userListeners;
}

@Override
public Listener initialize()
{
List<ComplianceViolation.Listener> initialized = null;
for (ComplianceViolation.Listener listener : userListeners)
{
Listener listening = listener.initialize();
if (listening != listener)
{
initialized = new ArrayList<>(userListeners.size());
for (ComplianceViolation.Listener l : userListeners)
{
if (l == listener)
break;
initialized.add(l);
}
}
if (initialized != null)
initialized.add(listening);
}
if (initialized == null)
return this;
return new ListenerCollection(initialized);
}

@Override
public void onComplianceViolation(ComplianceViolation.Event event)
{
assert event != null;
notifyUserListeners(event);
}

private void notifyUserListeners(ComplianceViolation.Event event)
{
if (userListeners == null || userListeners.isEmpty())
return;

for (ComplianceViolation.Listener listener : userListeners)
{
try
{
listener.onComplianceViolation(event);
}
catch (Exception e)
{
LOG.warn("Unable to notify ComplianceViolation.Listener implementation at {} of event {}", listener, event, e);
}
}
}
}

class LoggingListener implements Listener
{
private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolation.class);
Expand Down
64 changes: 59 additions & 5 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.Set;
import java.util.concurrent.atomic.AtomicInteger;

import org.eclipse.jetty.util.StringUtil;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand Down Expand Up @@ -82,15 +83,13 @@ public enum Violation implements ComplianceViolation
* as invalid even if the multiple values are the same. A deployment may include this violation to allow multiple
* {@code Content-Length} values to be received, but only if they are identical.
*/
// TODO: see if this is a violation in HTTP/2 & HTTP/3 as well
MULTIPLE_CONTENT_LENGTHS("https://tools.ietf.org/html/rfc7230#section-3.3.2", "Multiple Content-Lengths"),

/**
* Since <a href="https://tools.ietf.org/html/rfc7230#section-3.3.1">RFC 7230</a>, the HTTP protocol has required that
* a request is invalid if it contains both a {@code Transfer-Encoding} field and {@code Content-Length} field.
* A deployment may include this violation to allow both fields to be in a received request.
*/
// TODO: see if this is a violation in HTTP/2 & HTTP/3 as well
TRANSFER_ENCODING_WITH_CONTENT_LENGTH("https://tools.ietf.org/html/rfc7230#section-3.3.1", "Transfer-Encoding and Content-Length"),

/**
Expand All @@ -112,23 +111,20 @@ public enum Violation implements ComplianceViolation
* says that a Server must reject a request duplicate host headers.
* A deployment may include this violation to allow duplicate host headers on a received request.
*/
// TODO: see if this is a violation in HTTP/2 & HTTP/3 as well
DUPLICATE_HOST_HEADERS("https://www.rfc-editor.org/rfc/rfc7230#section-5.4", "Duplicate Host Header"),

/**
* Since <a href="https://www.rfc-editor.org/rfc/rfc7230#section-2.7.1">RFC 7230</a>, the HTTP protocol
* should reject a request if the Host headers contains an invalid / unsafe authority.
* A deployment may include this violation to allow unsafe host headesr on a received request.
*/
// TODO: see if this is a violation in HTTP/2 & HTTP/3 as well
UNSAFE_HOST_HEADER("https://www.rfc-editor.org/rfc/rfc7230#section-2.7.1", "Invalid Authority"),

/**
* Since <a href="https://www.rfc-editor.org/rfc/rfc7230#section-5.4">RFC 7230: Section 5.4</a>, the HTTP protocol
* must reject a request if the target URI has an authority that is different than a provided Host header.
* A deployment may include this violation to allow different values on the target URI and the Host header on a received request.
*/
// TODO: see if this is a violation in HTTP/2 & HTTP/3 as well
MISMATCHED_AUTHORITY("https://www.rfc-editor.org/rfc/rfc7230#section-5.4", "Mismatched Authority");

private final String url;
Expand Down Expand Up @@ -363,4 +359,62 @@ private static Set<Violation> copyOf(Set<Violation> violations)
return EnumSet.noneOf(Violation.class);
return EnumSet.copyOf(violations);
}

public static void checkHttpCompliance(MetaData.Request request, HttpCompliance mode,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Used by HTTP/2 and HTTP/3

ComplianceViolation.Listener listener)
{
boolean seenContentLength = false;
boolean seenTransferEncoding = false;
boolean seenHostHeader = false;

HttpFields fields = request.getHttpFields();
for (HttpField httpField: fields)
{
switch (httpField.getHeader())
{
case CONTENT_LENGTH ->
{
if (seenContentLength)
assertAllowed(Violation.MULTIPLE_CONTENT_LENGTHS, mode, listener);
String[] lengths = httpField.getValues();
if (lengths.length > 1)
assertAllowed(Violation.MULTIPLE_CONTENT_LENGTHS, mode, listener);
if (seenTransferEncoding)
assertAllowed(Violation.TRANSFER_ENCODING_WITH_CONTENT_LENGTH, mode, listener);
seenContentLength = true;
}
case TRANSFER_ENCODING ->
{
if (seenContentLength)
assertAllowed(Violation.TRANSFER_ENCODING_WITH_CONTENT_LENGTH, mode, listener);
seenTransferEncoding = true;
}
case HOST ->
{
if (seenHostHeader)
assertAllowed(Violation.DUPLICATE_HOST_HEADERS, mode, listener);
String[] hostValues = httpField.getValues();
if (hostValues.length > 1)
assertAllowed(Violation.DUPLICATE_HOST_HEADERS, mode, listener);
for (String hostValue: hostValues)
if (StringUtil.isBlank(hostValue))
assertAllowed(Violation.UNSAFE_HOST_HEADER, mode, listener);
String authority = request.getHttpURI().getHost();
if (StringUtil.isBlank(authority))
assertAllowed(Violation.UNSAFE_HOST_HEADER, mode, listener);
seenHostHeader = true;
}
}
}
}

private static void assertAllowed(Violation violation, HttpCompliance mode, ComplianceViolation.Listener listener)
{
if (mode.allows(violation))
listener.onComplianceViolation(new ComplianceViolation.Event(
mode, violation, violation.getDescription()
));
else
throw new BadMessageException(violation.getDescription());
}
}
6 changes: 6 additions & 0 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1518,6 +1518,10 @@ public boolean parseNext(ByteBuffer buffer)
// Start a request/response
if (_state == State.START)
{
if (_requestHandler != null)
_requestHandler.messageBegin();
if (_responseHandler != null)
_responseHandler.messageBegin();
_version = null;
_method = null;
_methodString = null;
Expand Down Expand Up @@ -1988,6 +1992,8 @@ public String toString()
*/
public interface HttpHandler
{
default void messageBegin() {}

boolean content(ByteBuffer item);

boolean headerComplete();
Expand Down
11 changes: 11 additions & 0 deletions jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,10 @@ public static class Parser
{
private final PartsListener listener = new PartsListener();
private final MultiPart.Parser parser;
// TODO: it's a bit odd to hang onto a ComplianceViolation.Listener here.
// as that would require the user to pass it in. Maybe a List<Event> violations instead?
// that the ee10 layer could use?
private ComplianceViolation.Listener complianceViolationListener;
Comment on lines +203 to +206
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 -1 on keeping lists of violations just in case later on there is a violation listener we need to pass it too.

Just have this field with an optional constructor. If it is constructed without the listener, then there is no listener.

private boolean useFilesForPartsWithoutFileName;
private Path filesDirectory;
private long maxFileSize = -1;
Expand Down Expand Up @@ -517,6 +521,13 @@ public void onPart(String name, String fileName, HttpFields headers)
memoryFileSize = 0;
try (AutoLock ignored = lock.lock())
{
if (headers.contains("content-transfer-encoding"))
{
String value = headers.get("content-transfer-encoding");
if (!"8bit".equalsIgnoreCase(value) && !"binary".equalsIgnoreCase(value))
complianceViolationListener.onComplianceViolation(new ComplianceViolation.Event(MultiPartCompliance.RFC7578, MultiPartCompliance.Violation.CONTENT_TRANSFER_ENCODING, value));
}

MultiPart.Part part;
if (fileChannel != null)
part = new MultiPart.PathPart(name, fileName, headers, filePath);
Expand Down
1 change: 1 addition & 0 deletions ...2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ private HttpChannel pollHttpChannel()
httpChannel = httpChannels.poll();
if (httpChannel == null)
httpChannel = httpChannelFactory.newHttpChannel(this);
httpChannel.init();
Copy link
Contributor

Choose a reason for hiding this comment

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

I know I first used the "initialize" verb... but is it really the right verb? perhaps prepare?

Anyway, failing a better name, can this at least be initialize

return httpChannel;
}

Expand Down
8 changes: 5 additions & 3 deletions ...tp2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import java.util.function.Supplier;

import org.eclipse.jetty.http.ComplianceViolation;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.HttpException;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpMethod;
Expand All @@ -40,7 +41,6 @@
import org.eclipse.jetty.server.HttpChannel;
import org.eclipse.jetty.server.HttpStream;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.server.TunnelSupport;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.Callback;
Expand Down Expand Up @@ -85,12 +85,14 @@ public Runnable onRequest(HeadersFrame frame)
{
_requestMetaData = (MetaData.Request)frame.getMetaData();

ComplianceViolation.Listener listener = Server.getComplianceViolationListener(_httpChannel.getConnectionMetaData().getConnector()).initialize();
// Grab freshly initialized ComplianceViolation.Listener here, no need to reinitialize.
ComplianceViolation.Listener listener = _httpChannel.getComponents().getComplianceViolationListener();
Copy link
Contributor

Choose a reason for hiding this comment

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

@sbordet This is the exposed violations listeners in components. I agree it is kind of funny... but is it really any different to the exposed HttpConfiguration (see change below)?

The HttpConfiguration is public API, but also has getters for lists of Customizers, which users are not meant to use directly.

What if we moved the compliance listeners to HttpConfiguration? They'd still be able to be associated with a particular connector if desired? It would also be very much more explicit rather than just adding beans.

Copy link
Contributor Author

@joakime joakime Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If we move the configuration of ComplianceViolation.Listener (from being beans on Connector or Server) to HttpConfiguration then that simplifies a bunch of things too.

We could have methods:

  • addComplianceViolationListener(ComplianceViolation.Listener)
  • removeComplianceViolationListener(ComplianceViolation.Listener)
  • getComplianceViolationListeners() - returns the list of listeners
  • getActiveComplianceViolationListener() - returns the active (possibly composite, possible NOOP) ComplianceViolation.Listener suitable to use in the code.

The existence of any added listener would be the equivalent of HttpConfiguration.isNotifyComplianceViolations() of true (no need for this boolean).
We could also get rid of Server.getComplianceViolationListener(Connector connector) in favor of just using the HttpConfiguration.getActiveComplianceViolationListener().

This however, wouldn't get rid of, or change, the existing storage and management from HttpChannelState tho. right?
We still need the initialize() / onBeginRequest() / onEndRequest() calls from the right places in HTTP/1, HTTP/2, and HTTP/3

Runnable handler = _httpChannel.onRequest(_requestMetaData);
Request request = _httpChannel.getRequest();
listener.onRequestBegin(request);
// Note UriCompliance is done by HandlerInvoker
// TODO: perform HttpCompliance violation checks?
HttpCompliance httpCompliance = _httpChannel.getConnectionMetaData().getHttpConfiguration().getHttpCompliance();
HttpCompliance.checkHttpCompliance(_requestMetaData, httpCompliance, listener);

if (frame.isEndStream())
{
Expand Down
8 changes: 5 additions & 3 deletions ...tp3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import java.util.function.Supplier;

import org.eclipse.jetty.http.ComplianceViolation;
import org.eclipse.jetty.http.HttpCompliance;
import org.eclipse.jetty.http.HttpException;
import org.eclipse.jetty.http.HttpFields;
import org.eclipse.jetty.http.HttpMethod;
Expand All @@ -35,7 +36,6 @@
import org.eclipse.jetty.server.HttpChannel;
import org.eclipse.jetty.server.HttpStream;
import org.eclipse.jetty.server.Request;
import org.eclipse.jetty.server.Server;
import org.eclipse.jetty.util.BufferUtil;
import org.eclipse.jetty.util.Callback;
import org.eclipse.jetty.util.thread.AutoLock;
Expand Down Expand Up @@ -75,12 +75,14 @@ public Runnable onRequest(HeadersFrame frame)
{
requestMetaData = (MetaData.Request)frame.getMetaData();

ComplianceViolation.Listener listener = Server.getComplianceViolationListener(this.httpChannel.getConnectionMetaData().getConnector()).initialize();
// Grab freshly initialized ComplianceViolation.Listener here, no need to reinitialize.
ComplianceViolation.Listener listener = httpChannel.getComponents().getComplianceViolationListener();
Runnable handler = httpChannel.onRequest(requestMetaData);
Request request = this.httpChannel.getRequest();
listener.onRequestBegin(request);
// Note UriCompliance is done by HandlerInvoker
// TODO: perform HttpCompliance violation checks?
HttpCompliance httpCompliance = httpChannel.getConnectionMetaData().getHttpConfiguration().getHttpCompliance();
HttpCompliance.checkHttpCompliance(requestMetaData, httpCompliance, listener);

if (frame.isLast())
{
Expand Down
1 change: 1 addition & 0 deletions ...er/src/main/java/org/eclipse/jetty/http3/server/internal/ServerHTTP3StreamConnection.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ public Runnable onRequest(HTTP3StreamServer stream, HeadersFrame frame)
{
// Create new metadata for every request as the local or remote address may have changed.
HttpChannel httpChannel = httpChannelFactory.newHttpChannel(new MetaData());
httpChannel.init();
HttpStreamOverHTTP3 httpStream = new HttpStreamOverHTTP3(this, httpChannel, stream);
httpChannel.setHttpStream(httpStream);
stream.setAttachment(httpStream);
Expand Down
Loading