Issue #11253 - Cleanup ComplianceViolation behavior to allow Cookie / URI / MultiPart compliance to also receive listener events.

[joakime]

• Introduce new events on ComplianceViolation.Listener
• Introduce new ComplianceViolation.Listener.initialize() to allow for a new Listener at the appropriate time (to support per-request listeners)
• Introduce new ComplianceViolation.CapturingListener
• Introduce new HttpConfiguration.(add/remove/get)ComplianceViolationListener() methods.
• Deprecate/Remove handling of recordComplianceViolations in HttpConnection and HttpConnectionFactory classes.
• Produce warnings if use of ComplianceViolation.Listener as beans is still present.
• Add ComplianceViolation.Listener support to UriCompliance locations.
• Add ComplianceViolation.Listener support to MultiPartCompliance locations.
• Add ComplianceViolation.Listener support to CookieCompliance locations.
• Add ComplianceViolation.Listener support to HttpCompliance locations in HTTP/2 and HTTP/3

[joakime]

Need to handle MultiPartParser (core) for ComplianceViolation too.

[gregw]

I'm just going to write down some stream of consciousness thoughts about this:

First principle is that we really do not want any cost to this mechanism for the 99.99% of requests without violations, nor the 99.99% of deployments that don't care even if there are. We are talking about the 0.01*0.01 = 0.0001% of users here! Thus we should not incur ANY expense unless: a) there is a violation; and b) there is a violation listener.

There are two distinct use-cases that we are trying to support:

1. Reporting violations as they happen to any org.eclipse.jetty.http.ComplianceViolation.Listener#onComplianceViolation found on the connector associated with a request (or connection if there is not yet a request)
2. Keeping a list of violations that can be retrieved via a request attribute so that some handler/filter can access the violations that relate to the current request. I note that the current implementation keeps a list of string reports of the violations, not the events themselves... so it appears very special purpose.

So this strikes me as we are implementing two mechanisms when we only should have one. Ideally if a deployment wants a list of violations to be made available via a request attribute, then they should be able to write a listener that adds any violation to a list obtained as an attribute. However, there are two problems with this: I) the ComplianceViolation.Listener API does not have a way of reporting the associated request; and II) for some violations the associated request will not yet be created.

Ignoring the 2. use-case for now, we could simplify the 1. reporting of violations by adding a ComplianceViolation.Listener getComplianceViolationListener() to the ConnectionMetaData interface, as an instance of this is known both to the request and to the connection that parses/creates the request. So all violations could be reported to this instance (which could be a NOOP instance if there is no listeners configured). This would at least allows us to report all violations via the same mechanism.

However, it would not allow a deployment to create a list of violations associated with a request because the request context of a violation is not known to such a listener. So we need to think of a way to extend this, so such a behaviour can be added. Can we add some concept of scope to the violation listener API, and if so, then what is the exact lifecycle of that? It can't be request attributes, as they are only created once the request is created. We could perhaps move the request attributes to be at the scope of the HttpChannel or HttpChannelState, but because of references that might be kept to immutable request, we cannot allow a request to simply use attributes from the HttpChannelState as they may be recycled.

I think the solution might be that a deployment that wishes to keep a list of violations will need to use the HttpChannel.Factory mechanism to return a specialized HttpChannel that wraps the ConnectionMetaData so that it can intercept violations and keep the list as a ConnectionMetaData attribute.... ah that won't work because a ConnectionMetaData is shared by multiplexed requests.... so the list will have to be kept on the specialized ConnectionMetaData instance or something like that.

@joakime would the client be content with a mechanism that requires the connectors to be extended to add such a factory?

Eitherway, I think we should first start off by making a single solution for use-case 1. that reports violations via a single pathway (probably in ConnectionMetaData) and then look to see how we can keep lists of violations associated with request.

[joakime]

@gregw After a conversation with @sbordet I think this current solution is the path that you are hoping for.

[joakime]

@gregw this PR now builds and tests green, but there are still 3 more things left to do.

• Introduce MultiPartCompliance checks to Jetty Core version of MultiPart
• Introduce HttpCompliance checks to HTTP/2
• Introduce HttpCompliance checks to HTTP/3

This PR is growing in size, and I felt I these 3 things could easily be a new PR after this one.

As for the HTTP/2 and HTTP/3 checks against HttpCompliance, check the changes in this PR to the HttpCompliance enums, theres's a few TODOs that @sbordet and I identified as possibly fitting in HTTP/2 and HTTP/3 too.

[joakime]

There are a few entry points to all of this.

To start with, is the configuration of this.

The Connector

1. If the Connector has an HttpConnectionFactory, the configuration .setRecordHttpComplianceViolations(boolean) is used to enable ComplianceViolation event reporting of any kind. (meaning there are no ComplianceViolation.Listener's used if that boolean is false)
2. The Connector (and optionally the Server) can have Beans that are ComplianceViolation.Listener and/or ComplianceViolation.ListenerFactory instances.
   A Listener is used as-is, a ListenerFactory is used at the start of each transport (HTTP/1, HTTP/2, HTTP/3) to establish a "per-request"-ish ComplianceViolation.Listener

The Transports

Each of the Transports initialize their ComplianceViolation.Listener via the new Server.newComplianceViolationListener(Connector) call.
The resulting ComplianceViolation.Listener (can be null) will be stored in the Request attributes under the class name (so that later code can use it, such as UriCompliance, CookieCompliance, and MultPartCompliance)

1. HTTP/1 is initialized at HttpConnection.RequestHandler constructor
2. HTTP/2 is initialized at HttpStreamOverHTTP2.onRequest(HeadersFrame)
3. HTTP/3 is initialized at HttpStreamOverHTTP3.onRequest(HeadersFrame)

The Implemented ComplianceViolation.Listener classes

There are 3 implemented ComplianceViolation.Listener classes in the codebase now.

1. ComplianceViolation.LoggingListener - only LOGs the events. Added automatically by HttpConnectionFactory.configure(Connector connector) when configuration allows it.
2. ComplianceViolation.CapturingListener - captures the Events in a List<Event> and puts them in a Request attribute when the Request exists. It is never installed by default under any configuration, must be added via a ComplianceViolation.CapturingListenerFactory that is added as a bean to a Connector or Server instance.

[gregw]

This is better, but I think that it becomes expensive as soon as you have a single listener, as new instances will always be created.

[joakime]

@gregw I just pushed a fix for the bugs you discovered.

[joakime]

Going to move this entire logic to HttpConfiguration instead.
Per ideas mentioned at #11301 (comment)

[joakime]

This should be documented in the adoc documentation as well.
This PR should include that documentation update.
The porting guide might need to be updated as well.