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

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

Merged
merged 36 commits into from
Jan 24, 2024
Merged
Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
a125dcf
Fixing comments
joakime Jan 8, 2024
0844436
Adding TODOs about possible HTTP/2 & HTTP/3 violations
joakime Jan 8, 2024
46b3d6a
Adding missing javadoc
joakime Jan 8, 2024
2167aa5
Issue #11253 - Cleanup ComplianceViolation behavior to allow Cookie /…
joakime Jan 8, 2024
b86a415
Revert change to jetty-logging.properties
joakime Jan 8, 2024
d8b5bb8
Fix javadoc typos
joakime Jan 11, 2024
c0d0021
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/co…
joakime Jan 15, 2024
3818664
Issue #11253 - Alternate approach with split ComplianceViolation.List…
joakime Jan 16, 2024
ab199ac
Merge remote-tracking branch 'origin/jetty-12.0.x' into fix/12.0.x/co…
joakime Jan 16, 2024
20e7e41
Fix HttpParserTest
joakime Jan 16, 2024
ee568b6
Cleanup implementation
joakime Jan 16, 2024
1e525b8
Do not use deprecated method in our own code
joakime Jan 17, 2024
c5eda63
Fix ComplianceViolations2616Test failures
joakime Jan 17, 2024
3182982
Fixing ee9/ee8 RequestTest
joakime Jan 17, 2024
f7e3441
Updates from review
joakime Jan 17, 2024
af2ef3c
WIP on cleanup of ComplianceViolation.Listener instantiation
gregw Jan 18, 2024
9cc8053
WIP on cleanup of ComplianceViolation.Listener instantiation
gregw Jan 18, 2024
607934d
WIP on cleanup of ComplianceViolation.Listener instantiation
gregw Jan 18, 2024
9a2643a
WIP on cleanup of ComplianceViolation.Listener instantiation
gregw Jan 18, 2024
7f1f08b
WIP on cleanup of ComplianceViolation.Listener instantiation
gregw Jan 18, 2024
3c631fa
Merge branch 'jetty-12.0.x' into fix/12.0.x/cookie-compliance-logging2
gregw Jan 20, 2024
3357919
javadoc
gregw Jan 20, 2024
9601204
Store listeners in AbstractConnector
gregw Jan 20, 2024
dc5de07
Cleanup and reorganization of Proposal 2
joakime Jan 22, 2024
cf1f776
Merge branch 'fix/12.0.x/cookie-compliance-logging3' into fix/12.0.x/…
joakime Jan 23, 2024
c266d16
updates from review
gregw Jan 23, 2024
cdf8fcd
updates from review
gregw Jan 23, 2024
d7ac0d9
Changes from review
joakime Jan 23, 2024
9c4c264
Merge remote-tracking branch 'origin/fix/12.0.x/cookie-compliance-log…
joakime Jan 23, 2024
b4f4003
Fixing build
joakime Jan 23, 2024
823b0a1
Revert RequestHandler change
joakime Jan 23, 2024
34a67e2
Support ComplianceViolation.Listener in ee10 multipart/form
joakime Jan 23, 2024
1177674
Correct since versions
joakime Jan 23, 2024
5e275dd
Rename HttpChannel.init() to .initialize()
joakime Jan 23, 2024
80f8322
avoid double allocation of compliance listener list
gregw Jan 24, 2024
fdc0cd8
Avoid request specific listener in cookie cache
gregw Jan 24, 2024
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
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,14 @@

package org.eclipse.jetty.http;

import java.util.ArrayList;
import java.util.List;
import java.util.Set;

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

/**
* A Compliance Violation represents a requirement of an RFC, specification or Jetty implementation
* that may be allowed to be violated if it is included in a {@link ComplianceViolation.Mode}.
Expand Down Expand Up @@ -75,13 +81,119 @@ interface Mode
Set<? extends ComplianceViolation> getAllowed();
}

record Event(ComplianceViolation.Mode mode, ComplianceViolation violation, String details)
{
@Override
public String toString()
{
return String.format("%s (see %s) in mode %s for %s",
violation.getDescription(), violation.getURL(), mode, details);
}
}

/**
* A listener that can be notified of violations.
*/
interface Listener
{
Listener NOOP = new Listener() {};

/**
* Initialize the listener in preparation for a new request life cycle.
* @return The Listener instance to use for the request life cycle.
*/
default Listener initialize()
{
return this;
}

/**
* A new Request has begun.
*
* @param request the request attributes, or null if the Request does not exist yet (eg: during parsing of HTTP/1.1 headers, before request is created)
*/
default void onRequestBegin(Attributes request)
joakime marked this conversation as resolved.
Show resolved Hide resolved
{
}

/**
* A Request has ended.
*
* @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)
{
}

/**
* The compliance violation event.
*
* @param event the compliance violation event
*/
default void onComplianceViolation(Event event)
joakime marked this conversation as resolved.
Show resolved Hide resolved
{
onComplianceViolation(event.mode, event.violation, event.details);
}

/**
* The compliance violation event.
*
* @param mode the mode
* @param violation the violation
* @param details the details
* @deprecated use {@link #onComplianceViolation(Event)} instead. Will be removed in Jetty 12.1.0
*/
@Deprecated(since = "12.0.5", forRemoval = true)
default void onComplianceViolation(Mode mode, ComplianceViolation violation, String details)
{
}
}

class LoggingListener implements Listener
{
private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolation.class);

@Override
public void onComplianceViolation(Event event)
{
if (LOG.isDebugEnabled())
LOG.debug(event.toString());
}
}

class CapturingListener implements Listener
{
public static final String VIOLATIONS_ATTR_KEY = "org.eclipse.jetty.http.compliance.violations";

private final List<Event> events;

public CapturingListener()
{
this(null);
}

private CapturingListener(List<Event> events)
{
this.events = events;
}

@Override
public Listener initialize()
{
return new CapturingListener(new ArrayList<>());
}

@Override
public void onRequestBegin(Attributes request)
{
if (request != null)
request.setAttribute(VIOLATIONS_ATTR_KEY, events);
}

@Override
public void onComplianceViolation(Event event)
{
events.add(event);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
//
// ========================================================================
// Copyright (c) 1995 Mort Bay Consulting Pty Ltd and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0
// which is available at https://www.apache.org/licenses/LICENSE-2.0.
//
// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0
// ========================================================================
//

package org.eclipse.jetty.http;

public class ComplianceViolationException extends IllegalArgumentException
{
private final ComplianceViolation.Event event;

public ComplianceViolationException(ComplianceViolation.Mode mode, ComplianceViolation violation, String details)
{
this(new ComplianceViolation.Event(mode, violation, details));
}

public ComplianceViolationException(ComplianceViolation.Event event)
{
super(event.toString());
this.event = event;
}

public ComplianceViolation.Event getEvent()
{
return event;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ else if (tokenstart >= 0)
protected void reportComplianceViolation(CookieCompliance.Violation violation, String reason)
{
if (_complianceListener != null)
_complianceListener.onComplianceViolation(_complianceMode, violation, reason);
_complianceListener.onComplianceViolation(new ComplianceViolation.Event(_complianceMode, violation, reason));
}

protected boolean isRFC6265RejectedCharacter(char c)
Expand Down
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 @@ -156,8 +157,14 @@ public String getDescription()

/**
* The request attribute which may be set to record any allowed HTTP violations.
* @deprecated use {@link ComplianceViolation.CapturingListener#VIOLATIONS_ATTR_KEY} instead.<br>
* (Note: new ATTR captures all Compliance violations, not just HTTP.<br>
* Make sure you have {@code HttpConnectionFactory.setRecordHttpComplianceViolations(true)}.<br>
* Also make sure that a {@link ComplianceViolation.CapturingListener} has been added as a bean to
* either the {@code Connector} or {@code Server} for the Attribute to be created.)
*/
public static final String VIOLATIONS_ATTR = "org.eclipse.jetty.http.compliance.violations";
@Deprecated(since = "12.0.5", forRemoval = true)
public static final String VIOLATIONS_ATTR = ComplianceViolation.CapturingListener.VIOLATIONS_ATTR_KEY;

/**
* The HttpCompliance mode that supports <a href="https://tools.ietf.org/html/rfc7230">RFC 7230</a>
Expand Down Expand Up @@ -210,7 +217,8 @@ public static HttpCompliance valueOf(String name)
if (compliance.getName().equals(name))
return compliance;
}
LOG.warn("Unknown HttpCompliance mode {}", name);
if (name.indexOf(',') == -1) // skip warning if delimited, will be handled by .from() properly as a CUSTOM mode.
LOG.warn("Unknown HttpCompliance mode {}", name);
return null;
}

Expand Down Expand Up @@ -351,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,
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());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -229,7 +229,6 @@ public enum State
private final HttpHandler _handler;
private final RequestHandler _requestHandler;
private final ResponseHandler _responseHandler;
private final ComplianceViolation.Listener _complianceListener;
private final int _maxHeaderBytes;
private final HttpCompliance _complianceMode;
private final Utf8StringBuilder _uri = new Utf8StringBuilder(INITIAL_URI_LENGTH);
Expand Down Expand Up @@ -309,7 +308,6 @@ private HttpParser(RequestHandler requestHandler, ResponseHandler responseHandle
_responseHandler = responseHandler;
_maxHeaderBytes = maxHeaderBytes;
_complianceMode = compliance;
_complianceListener = (ComplianceViolation.Listener)(_handler instanceof ComplianceViolation.Listener ? _handler : null);
}

public long getBeginNanoTime()
Expand Down Expand Up @@ -357,8 +355,8 @@ protected void reportComplianceViolation(Violation violation)

protected void reportComplianceViolation(Violation violation, String reason)
{
if (_complianceListener != null)
_complianceListener.onComplianceViolation(_complianceMode, violation, reason);
if (_requestHandler != null)
_requestHandler.onViolation(new ComplianceViolation.Event(_complianceMode, violation, reason));
}

protected String caseInsensitiveHeader(String orig, String normative)
Expand Down Expand Up @@ -1520,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 @@ -1990,6 +1992,8 @@ public String toString()
*/
public interface HttpHandler
{
default void messageBegin() {}

boolean content(ByteBuffer item);

boolean headerComplete();
Expand Down Expand Up @@ -2020,6 +2024,14 @@ default void parsedTrailer(HttpField field)
*/
void earlyEOF();

/**
* Called to indicate that a {@link ComplianceViolation} has occurred.
* @param event the Compliance Violation event
*/
default void onViolation(ComplianceViolation.Event event)
{
}

/**
* Called to signal that a bad HTTP message has been received.
*
Expand Down
Loading