From a125dcf494f1e31de1a2055acad8ebdba75fb46e Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 8 Jan 2024 15:01:46 -0600 Subject: [PATCH 01/31] Fixing comments --- .../eclipse/jetty/http/RFC6265CookieParserTest.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java index 5741ff1136f9..a678f40b6a96 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java @@ -51,7 +51,7 @@ public void testRFC2965Single() assertCookie("Cookies[1]", cookies.get(1), "Customer", "WILE_E_COYOTE", 0, null); assertCookie("Cookies[2]", cookies.get(2), "$Path", "/acme", 0, null); - // There attributes are seen as just normal cookies, so no violations + // Normal cookies with attributes, so no violations assertThat(parser.violations.size(), is(0)); // Same again, but allow attributes which are ignored @@ -60,7 +60,7 @@ public void testRFC2965Single() assertThat("Cookies.length", cookies.size(), is(1)); assertCookie("Cookies[0]", cookies.get(0), "Customer", "WILE_E_COYOTE", 0, null); - // There attributes are seen as just normal cookies, so no violations + // There are 2 attributes that are seen as violations assertThat(parser.violations.size(), is(2)); // Same again, but allow attributes which are not ignored @@ -69,10 +69,10 @@ public void testRFC2965Single() assertThat("Cookies.length", cookies.size(), is(1)); assertCookie("Cookies[0]", cookies.get(0), "Customer", "WILE_E_COYOTE", 1, "/acme"); - // There attributes are seen as just normal cookies, so no violations + // There are 2 attributes that are seen as violations assertThat(parser.violations.size(), is(2)); - // Same test with RFC 6265 strict should throw. + // Same test, but with RFC 6265 strict. parser = new TestCookieParser(CookieCompliance.RFC6265_STRICT); cookies = parser.parseFields(rawCookie); assertThat("Cookies.length", cookies.size(), is(3)); @@ -80,7 +80,7 @@ public void testRFC2965Single() assertCookie("Cookies[1]", cookies.get(1), "Customer", "WILE_E_COYOTE", 0, null); assertCookie("Cookies[2]", cookies.get(2), "$Path", "/acme", 0, null); - // There attributes are seen as just normal cookies, so no violations + // Normal cookies with attributes, so no violations assertThat(parser.violations.size(), is(0)); } From 08444362267c61dc01ebb726f1e11a832d59771a Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 8 Jan 2024 15:02:11 -0600 Subject: [PATCH 02/31] Adding TODOs about possible HTTP/2 & HTTP/3 violations --- .../src/main/java/org/eclipse/jetty/http/HttpCompliance.java | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index 95c1e0412714..5e75d04189ac 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -82,6 +82,7 @@ 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"), /** @@ -89,6 +90,7 @@ public enum Violation implements ComplianceViolation * 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"), /** @@ -110,6 +112,7 @@ 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"), /** @@ -117,6 +120,7 @@ public enum Violation implements ComplianceViolation * 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"), /** @@ -124,6 +128,7 @@ public enum Violation implements ComplianceViolation * 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; From 46b3d6a2d6532b687bec578c5eba7487f2c47312 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 8 Jan 2024 15:02:23 -0600 Subject: [PATCH 03/31] Adding missing javadoc --- .../src/main/java/org/eclipse/jetty/http/CookieParser.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieParser.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieParser.java index 8c6c16d5c781..4e6f10b454e4 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieParser.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieParser.java @@ -26,6 +26,8 @@ public interface CookieParser { /** *

A factory method to create a new parser suitable for the compliance mode.

+ * + * @param handler the handler for Cookie Parsing events * @param compliance The compliance mode to use for parsing. * @param complianceListener A listener for compliance violations or null. * @return A CookieParser instance. From 2167aa530264cbaf04190a441b2bb16c6febed23 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 8 Jan 2024 16:52:01 -0600 Subject: [PATCH 04/31] Issue #11253 - Cleanup ComplianceViolation behavior to allow Cookie / URI / MultiPart compliance to also receive listener events. --- .../jetty/http/ComplianceViolation.java | 18 +++ .../http/ComplianceViolationException.java | 35 ++++++ .../jetty/http/ComplianceViolations.java | 111 ++++++++++++++++++ .../org/eclipse/jetty/http/HttpParser.java | 13 +- .../jetty/http/MultiPartCompliance.java | 111 ++++++++++++++++++ .../java/org/eclipse/jetty/http/Syntax.java | 8 +- .../org/eclipse/jetty/http/UriCompliance.java | 4 +- .../eclipse/jetty/server/HttpCookieUtils.java | 22 +++- .../org/eclipse/jetty/server/Request.java | 5 +- .../org/eclipse/jetty/server/Response.java | 26 +++- .../server/internal/HttpChannelState.java | 62 +++++++++- .../jetty/server/internal/HttpConnection.java | 47 ++------ .../jetty/server/ServerHttpCookieTest.java | 8 +- .../test/resources/jetty-logging.properties | 2 +- .../eclipse/jetty/ee9/nested/Dispatcher.java | 4 +- .../ee9/nested/MultiPartFormInputStream.java | 31 ++--- .../org/eclipse/jetty/ee9/nested/Request.java | 40 +++---- 17 files changed, 443 insertions(+), 104 deletions(-) create mode 100644 jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolationException.java create mode 100644 jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolations.java create mode 100644 jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartCompliance.java diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java index 6c3068d52ea1..664053f66465 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java @@ -75,13 +75,31 @@ interface Mode Set getAllowed(); } + public static 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 { + /** + * The compliance violation event. + */ + default void onComplianceViolation(Event event) + { + } + default void onComplianceViolation(Mode mode, ComplianceViolation violation, String details) { + onComplianceViolation(new Event(mode, violation, details)); } } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolationException.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolationException.java new file mode 100644 index 000000000000..ea13e58029f6 --- /dev/null +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolationException.java @@ -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; + } +} diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolations.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolations.java new file mode 100644 index 000000000000..58c2ff2727d7 --- /dev/null +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolations.java @@ -0,0 +1,111 @@ +// +// ======================================================================== +// 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; + +import java.util.ArrayList; +import java.util.List; + +import org.eclipse.jetty.util.Attributes; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * A representation of ComplianceViolation.Listeners events that have occurred. + */ +public class ComplianceViolations implements ComplianceViolation.Listener +{ + public static final String VIOLATIONS_ATTR = ComplianceViolations.class.getName(); + private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolations.class); + private final List userListeners; + private List events; + private Attributes attributes; + + /** + * Construct a new ComplianceViolations that will notify user listeners. + * @param userListeners the user listeners to notify, null or empty is allowed. + */ + public ComplianceViolations(List userListeners) + { + this.userListeners = userListeners; + } + + @Override + public void onComplianceViolation(ComplianceViolation.Event event) + { + assert event != null; + + notifyUserListeners(event); + addEvent(event); + if (LOG.isDebugEnabled()) + LOG.debug(event.toString()); + } + + /** + * Clear out the tracked {@link Attributes} object and events; + */ + public void recycle() + { + this.events = null; + this.attributes = null; + } + + /** + * Start tracking an {@link Attributes} implementation to store the events in. + * @param attributes the attributes object to store the event list in. Stored in key name {@link #VIOLATIONS_ATTR} + */ + public void setAttribute(Attributes attributes) + { + this.attributes = attributes; + this.attributes.setAttribute(VIOLATIONS_ATTR, events); + } + + /** + * The events that have been recorded. + * + * @return the list of Events recorded. + */ + public List getEvents() + { + return events; + } + + private void addEvent(ComplianceViolation.Event event) + { + if (this.events == null) + { + this.events = new ArrayList<>(); + if (this.attributes != null) + setAttribute(this.attributes); + } + this.events.add(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); + } + } + } +} diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index 0a407f776469..aa9674c753ea 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -294,22 +294,27 @@ public HttpParser(RequestHandler handler, HttpCompliance compliance) public HttpParser(RequestHandler handler, int maxHeaderBytes, HttpCompliance compliance) { - this(handler, null, maxHeaderBytes, compliance == null ? compliance() : compliance); + this(handler, null, maxHeaderBytes, compliance == null ? compliance() : compliance, null); + } + + public HttpParser(RequestHandler handler, int maxHeaderBytes, HttpCompliance compliance, ComplianceViolation.Listener complianceListener) + { + this(handler, null, maxHeaderBytes, compliance == null ? compliance() : compliance, complianceListener); } public HttpParser(ResponseHandler handler, int maxHeaderBytes, HttpCompliance compliance) { - this(null, handler, maxHeaderBytes, compliance == null ? compliance() : compliance); + this(null, handler, maxHeaderBytes, compliance == null ? compliance() : compliance, null); } - private HttpParser(RequestHandler requestHandler, ResponseHandler responseHandler, int maxHeaderBytes, HttpCompliance compliance) + private HttpParser(RequestHandler requestHandler, ResponseHandler responseHandler, int maxHeaderBytes, HttpCompliance compliance, ComplianceViolation.Listener complianceListener) { _handler = requestHandler != null ? requestHandler : responseHandler; _requestHandler = requestHandler; _responseHandler = responseHandler; _maxHeaderBytes = maxHeaderBytes; _complianceMode = compliance; - _complianceListener = (ComplianceViolation.Listener)(_handler instanceof ComplianceViolation.Listener ? _handler : null); + _complianceListener = complianceListener; } public long getBeginNanoTime() diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartCompliance.java new file mode 100644 index 000000000000..5ff3b819e684 --- /dev/null +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartCompliance.java @@ -0,0 +1,111 @@ +// +// ======================================================================== +// 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; + +import java.util.Arrays; +import java.util.EnumSet; +import java.util.List; +import java.util.Set; + +/** + * The compliance mode for MultiPart handling. + */ +public class MultiPartCompliance implements ComplianceViolation.Mode +{ + public enum Violation implements ComplianceViolation + { + CONTENT_TRANSFER_ENCODING("https://tools.ietf.org/html/rfc7578#section-4.7", "Content-Transfer-Encoding is deprecated"); + + private final String url; + private final String description; + + Violation(String url, String description) + { + this.url = url; + this.description = description; + } + + @Override + public String getName() + { + return name(); + } + + @Override + public String getURL() + { + return url; + } + + @Override + public String getDescription() + { + return description; + } + } + + public static final MultiPartCompliance RFC7578 = new MultiPartCompliance( + "RFC7578", EnumSet.of(Violation.CONTENT_TRANSFER_ENCODING)); + + private static final List KNOWN_MODES = Arrays.asList(RFC7578); + + public static MultiPartCompliance valueOf(String name) + { + for (MultiPartCompliance compliance : KNOWN_MODES) + { + if (compliance.getName().equals(name)) + return compliance; + } + return null; + } + + private final String name; + private final Set violations; + + public MultiPartCompliance(String name, Set violations) + { + this.name = name; + this.violations = violations; + } + + @Override + public String getName() + { + return name; + } + + @Override + public boolean allows(ComplianceViolation violation) + { + return violations.contains(violation); + } + + @Override + public Set getKnown() + { + return EnumSet.allOf(Violation.class); + } + + @Override + public Set getAllowed() + { + return violations; + } + + @Override + public String toString() + { + return String.format("%s@%x%s", name, hashCode(), violations); + } +} diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/Syntax.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/Syntax.java index 91fa85c74d66..fa24eeb59694 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/Syntax.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/Syntax.java @@ -108,7 +108,7 @@ public static void requireValidRFC6265CookieValue(String value) // Has starting DQUOTE if (valueLen <= 1 || (value.charAt(valueLen - 1) != '"')) { - throw new IllegalArgumentException("RFC6265 Cookie values must have balanced DQUOTES (if used)"); + throw new ComplianceViolationException(CookieCompliance.RFC6265, CookieCompliance.Violation.INVALID_COOKIES, "RFC6265 Cookie values must have balanced DQUOTES (if used)"); } // adjust search range to exclude DQUOTES @@ -122,16 +122,16 @@ public static void requireValidRFC6265CookieValue(String value) // 0x00 - 0x1F are low order control characters // 0x7F is the DEL control character if ((c <= 0x1F) || (c == 0x7F)) - throw new IllegalArgumentException("RFC6265 Cookie values may not contain control characters"); + throw new ComplianceViolationException(CookieCompliance.RFC6265, CookieCompliance.Violation.INVALID_COOKIES, "RFC6265 Cookie values may not contain control characters"); if ((c == ' ' /* 0x20 */) || (c == '"' /* 0x2C */) || (c == ';' /* 0x3B */) || (c == '\\' /* 0x5C */)) { - throw new IllegalArgumentException("RFC6265 Cookie values may not contain character: [" + c + "]"); + throw new ComplianceViolationException(CookieCompliance.RFC6265, CookieCompliance.Violation.INVALID_COOKIES, "RFC6265 Cookie values may not contain character: [" + c + "]"); } if (c >= 0x80) - throw new IllegalArgumentException("RFC6265 Cookie values characters restricted to US-ASCII: 0x" + Integer.toHexString(c)); + throw new ComplianceViolationException(CookieCompliance.RFC6265, CookieCompliance.Violation.INVALID_COOKIES, "RFC6265 Cookie values characters restricted to US-ASCII: 0x" + Integer.toHexString(c)); } } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java index ff4d1ad8270c..9704c8825a31 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java @@ -360,12 +360,14 @@ private static Set copyOf(Set violations) return EnumSet.copyOf(violations); } - public static String checkUriCompliance(UriCompliance compliance, HttpURI uri) + public static String checkUriCompliance(UriCompliance compliance, HttpURI uri, ComplianceViolation.Listener listener) { for (UriCompliance.Violation violation : UriCompliance.Violation.values()) { if (uri.hasViolation(violation) && (compliance == null || !compliance.allows(violation))) return violation.getDescription(); + else if (listener != null) + listener.onComplianceViolation(compliance, violation, uri.toString()); } return null; } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpCookieUtils.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpCookieUtils.java index 815807dc123f..8e81362a754d 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpCookieUtils.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpCookieUtils.java @@ -18,6 +18,7 @@ import java.util.Locale; import java.util.Map; +import org.eclipse.jetty.http.ComplianceViolationException; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpField; @@ -211,9 +212,17 @@ public static String getRFC6265SetCookie(HttpCookie httpCookie) if (name == null || name.length() == 0) throw new IllegalArgumentException("Bad cookie name"); - // Name is checked for legality by servlet spec, but can also be passed directly so check again for quoting - // Per RFC6265, Cookie.name follows RFC2616 Section 2.2 token rules - Syntax.requireValidRFC2616Token(name, "RFC6265 Cookie name"); + try + { + // Name is checked for legality by servlet spec, but can also be passed directly so check again for quoting + // Per RFC6265, Cookie.name follows RFC2616 Section 2.2 token rules + Syntax.requireValidRFC2616Token(name, "RFC6265 Cookie name"); + } + catch (IllegalArgumentException e) + { + throw new ComplianceViolationException(CookieCompliance.RFC6265, CookieCompliance.Violation.INVALID_COOKIES, "RFC6265 Cookie name must be a valid RFC2616 Token"); + } + // Ensure that Per RFC6265, Cookie.value follows syntax rules String value = httpCookie.getValue(); Syntax.requireValidRFC6265CookieValue(value); @@ -399,13 +408,14 @@ private HttpCookieUtils() public static class SetCookieHttpField extends HttpField { private final HttpCookie _cookie; - private final CookieCompliance _compliance; + private final String _value; public SetCookieHttpField(HttpCookie cookie, CookieCompliance compliance) { super(HttpHeader.SET_COOKIE, (String)null); this._cookie = cookie; - _compliance = compliance; + // trigger compliance check + this._value = getSetCookie(this._cookie, compliance); } public HttpCookie getHttpCookie() @@ -416,7 +426,7 @@ public HttpCookie getHttpCookie() @Override public String getValue() { - return getSetCookie(_cookie, _compliance); + return this._value; } } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 6af4e71d07b8..90d475ad4b5a 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -34,6 +34,7 @@ import java.util.function.Function; import java.util.function.Predicate; +import org.eclipse.jetty.http.ComplianceViolations; import org.eclipse.jetty.http.CookieCache; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpFields; @@ -589,8 +590,8 @@ static List getCookies(Request request) CookieCache cookieCache = (CookieCache)request.getComponents().getCache().getAttribute(CACHE_ATTRIBUTE); if (cookieCache == null) { - // TODO compliance listeners? - cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance(), null); + ComplianceViolations complianceViolations = request.getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); + cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance(), complianceViolations); request.getComponents().getCache().setAttribute(CACHE_ATTRIBUTE, cookieCache); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index cdca4131fc10..e7ce14882dc9 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -20,6 +20,8 @@ import java.util.concurrent.TimeoutException; import java.util.function.Supplier; +import org.eclipse.jetty.http.ComplianceViolationException; +import org.eclipse.jetty.http.ComplianceViolations; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpException; @@ -378,7 +380,16 @@ static void addCookie(Response response, HttpCookie cookie) Request request = response.getRequest(); CookieCompliance compliance = request.getConnectionMetaData().getHttpConfiguration().getResponseCookieCompliance(); - response.getHeaders().add(new HttpCookieUtils.SetCookieHttpField(HttpCookieUtils.checkSameSite(cookie, request.getContext()), compliance)); + try + { + response.getHeaders().add(new HttpCookieUtils.SetCookieHttpField(HttpCookieUtils.checkSameSite(cookie, request.getContext()), compliance)); + } + catch (ComplianceViolationException e) + { + ComplianceViolations complianceViolations = request.getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); + complianceViolations.onComplianceViolation(e.getEvent()); + throw e; + } // Expire responses with set-cookie headers, so they do not get cached. if (!response.getHeaders().contains(HttpHeader.EXPIRES)) @@ -402,7 +413,18 @@ static void putCookie(Response response, HttpCookie cookie) Request request = response.getRequest(); HttpConfiguration httpConfiguration = request.getConnectionMetaData().getHttpConfiguration(); CookieCompliance compliance = httpConfiguration.getResponseCookieCompliance(); - HttpField setCookie = new HttpCookieUtils.SetCookieHttpField(HttpCookieUtils.checkSameSite(cookie, request.getContext()), compliance); + + HttpField setCookie; + try + { + setCookie = new HttpCookieUtils.SetCookieHttpField(HttpCookieUtils.checkSameSite(cookie, request.getContext()), compliance); + } + catch (ComplianceViolationException e) + { + ComplianceViolations complianceViolations = request.getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); + complianceViolations.onComplianceViolation(e.getEvent()); + throw e; + } boolean expires = false; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index 2220fd636104..fc9a09ea1d4c 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -16,7 +16,9 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.nio.channels.WritePendingException; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Objects; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeoutException; @@ -27,6 +29,8 @@ import java.util.function.Supplier; import org.eclipse.jetty.http.BadMessageException; +import org.eclipse.jetty.http.ComplianceViolation; +import org.eclipse.jetty.http.ComplianceViolations; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; @@ -43,10 +47,12 @@ import org.eclipse.jetty.io.Content; import org.eclipse.jetty.server.Components; import org.eclipse.jetty.server.ConnectionMetaData; +import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.Context; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.HttpChannel; import org.eclipse.jetty.server.HttpConfiguration; +import org.eclipse.jetty.server.HttpConnectionFactory; import org.eclipse.jetty.server.HttpStream; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.RequestLog; @@ -102,6 +108,7 @@ private enum StreamSendState private final SerializedInvoker _readInvoker; private final SerializedInvoker _writeInvoker; private final ResponseHttpFields _responseHeaders = new ResponseHttpFields(); + private final ComplianceViolations _complianceViolations; private Thread _handling; private boolean _handled; private StreamSendState _streamSendState = StreamSendState.SENDING; @@ -125,6 +132,46 @@ public HttpChannelState(ConnectionMetaData connectionMetaData) // The SerializedInvoker is used to prevent infinite recursion of callbacks calling methods calling callbacks etc. _readInvoker = new HttpChannelSerializedInvoker(); _writeInvoker = new HttpChannelSerializedInvoker(); + _complianceViolations = newComplianceViolations(); + } + + /** + * @return a new ComplianceViolations if configuration enables it. + */ + private ComplianceViolations newComplianceViolations() + { + Connector connector = getConnectionMetaData().getConnector(); + if (connector == null) + return null; + + HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class); + if (httpConnectionFactory != null) + { + // Is this connector recording compliance violations? + if (httpConnectionFactory.isRecordHttpComplianceViolations()) + { + // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled + // This also means that any user provided ComplianceViolation.Listener beans will only be + // used when the configuration on the HttpConnectionFactory allows then to be used. + + // Look for optional user provided ComplianceViolation.Listeners + List userListeners = new ArrayList<>(); + userListeners.addAll(connector.getBeans(ComplianceViolation.Listener.class)); + userListeners.addAll(connector.getServer().getBeans(ComplianceViolation.Listener.class)); + + // Establish ComplianceViolations Tracker + ComplianceViolations violations = new ComplianceViolations(userListeners); + + // Store tracker (to be accessed elsewhere) + // If this bean is not present on the connector, then it is safe to assume that the + // HttpConnectionFactory is not allowing the recording of compliance violations. + connector.addBean(violations); + + return violations; + } + } + + return null; } @Override @@ -148,6 +195,8 @@ public void recycle() _request = null; _response = null; _oldIdleTimeout = 0; + if (_complianceViolations != null) + _complianceViolations.recycle(); // Break the link between channel and stream. _stream = null; _committedContentLength = -1; @@ -237,6 +286,15 @@ public Runnable onRequest(MetaData.Request request) if (LOG.isDebugEnabled()) LOG.debug("onRequest {} {}", request, this); + // Handle UriCompliance Violations + if (request.getHttpURI().hasViolations()) + { + UriCompliance compliance = getHttpConfiguration().getUriCompliance(); + String badMessage = UriCompliance.checkUriCompliance(compliance, request.getHttpURI(), _complianceViolations); + if (badMessage != null) + throw new BadMessageException(badMessage); + } + try (AutoLock ignored = _lock.lock()) { if (_stream == null) @@ -261,6 +319,8 @@ public Runnable onRequest(MetaData.Request request) if (idleTO >= 0 && _oldIdleTimeout != idleTO) _stream.setIdleTimeout(idleTO); + _complianceViolations.setAttribute(_request); + // This is deliberately not serialized to allow a handler to block. return _handlerInvoker; } @@ -572,7 +632,7 @@ public void run() HttpURI uri = request.getHttpURI(); if (uri.hasViolations()) { - String badMessage = UriCompliance.checkUriCompliance(getConnectionMetaData().getHttpConfiguration().getUriCompliance(), uri); + String badMessage = UriCompliance.checkUriCompliance(getConnectionMetaData().getHttpConfiguration().getUriCompliance(), uri, _complianceViolations); if (badMessage != null) throw new BadMessageException(badMessage); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 7035d901d2e1..1ba352374beb 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -16,7 +16,6 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.nio.channels.WritePendingException; -import java.util.ArrayList; import java.util.List; import java.util.Objects; import java.util.Set; @@ -28,7 +27,7 @@ import java.util.concurrent.atomic.LongAdder; import org.eclipse.jetty.http.BadMessageException; -import org.eclipse.jetty.http.ComplianceViolation; +import org.eclipse.jetty.http.ComplianceViolations; import org.eclipse.jetty.http.HostPortHttpField; import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpException; @@ -165,7 +164,8 @@ protected HttpGenerator newHttpGenerator() protected HttpParser newHttpParser(HttpCompliance compliance) { - HttpParser parser = new HttpParser(_requestHandler, getHttpConfiguration().getRequestHeaderSize(), compliance); + ComplianceViolations complianceViolations = getHttpChannel().getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); + HttpParser parser = new HttpParser(_requestHandler, getHttpConfiguration().getRequestHeaderSize(), compliance, complianceViolations); parser.setHeaderCacheSize(getHttpConfiguration().getHeaderCacheSize()); parser.setHeaderCacheCaseSensitive(getHttpConfiguration().isHeaderCacheCaseSensitive()); return parser; @@ -305,30 +305,6 @@ public void setUseOutputDirectByteBuffers(boolean useOutputDirectByteBuffers) _useOutputDirectByteBuffers = useOutputDirectByteBuffers; } - protected void onComplianceViolation(ComplianceViolation.Mode mode, ComplianceViolation violation, String details) - { - //TODO configure this somewhere else - //TODO what about cookie compliance - //TODO what about http2 & 3 - //TODO test this in core - if (isRecordHttpComplianceViolations()) - { - HttpStreamOverHTTP1 stream = _stream.get(); - if (stream != null) - { - if (stream._complianceViolations == null) - { - stream._complianceViolations = new ArrayList<>(); - } - String record = String.format("%s (see %s) in mode %s for %s in %s", - violation.getDescription(), violation.getURL(), mode, details, HttpConnection.this); - stream._complianceViolations.add(record); - if (LOG.isDebugEnabled()) - LOG.debug(record); - } - } - } - @Override public ByteBuffer onUpgradeFrom() { @@ -933,7 +909,7 @@ public String toString() } } - protected class RequestHandler implements HttpParser.RequestHandler, ComplianceViolation.Listener + protected class RequestHandler implements HttpParser.RequestHandler { private Throwable _failure; @@ -1070,12 +1046,6 @@ public void earlyEOF() getServer().getThreadPool().execute(todo); } } - - @Override - public void onComplianceViolation(ComplianceViolation.Mode mode, ComplianceViolation violation, String details) - { - HttpConnection.this.onComplianceViolation(mode, violation, details); - } } protected class HttpStreamOverHTTP1 implements HttpStream @@ -1184,7 +1154,8 @@ public Runnable headerComplete() if (_uri.hasViolations()) { compliance = getHttpConfiguration().getUriCompliance(); - String badMessage = UriCompliance.checkUriCompliance(compliance, _uri); + ComplianceViolations complianceViolations = getHttpChannel().getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); + String badMessage = UriCompliance.checkUriCompliance(compliance, _uri, complianceViolations); if (badMessage != null) throw new BadMessageException(badMessage); } @@ -1198,7 +1169,11 @@ public Runnable headerComplete() { HttpCompliance httpCompliance = getHttpConfiguration().getHttpCompliance(); if (httpCompliance.allows(MISMATCHED_AUTHORITY)) - onComplianceViolation(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString()); + { + ComplianceViolations complianceViolations = getHttpChannel().getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); + if (complianceViolations != null) + complianceViolations.onComplianceViolation(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString()); + } else throw new BadMessageException("Authority!=Host"); } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java index a8575d5dbcd3..cec91cfd72bf 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java @@ -16,6 +16,7 @@ import java.util.List; import java.util.stream.Stream; +import org.eclipse.jetty.http.ComplianceViolations; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.CookieParser; import org.eclipse.jetty.http.HttpCookie; @@ -63,6 +64,7 @@ public boolean handle(Request request, Response response, Callback callback) thr Fields.Field setCookie = parameters.get("SetCookie"); if (setCookie != null) { + ComplianceViolations complianceViolations = request.getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); CookieParser parser = CookieParser.newParser(new CookieParser.Handler() { @Override @@ -70,7 +72,7 @@ public void addCookie(String name, String value, int version, String domain, Str { Response.addCookie(response, HttpCookie.build(name, value, version).domain(domain).path(path).comment(comment).build()); } - }, RFC2965, null); + }, RFC2965, complianceViolations); parser.parseField(setCookie.getValue()); } @@ -82,7 +84,9 @@ public void addCookie(String name, String value, int version, String domain, Str return true; } }); - _httpConfiguration = _connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration(); + HttpConnectionFactory httpConnectionFactory = _connector.getConnectionFactory(HttpConnectionFactory.class); + httpConnectionFactory.setRecordHttpComplianceViolations(true); + _httpConfiguration = httpConnectionFactory.getHttpConfiguration(); _server.start(); } diff --git a/jetty-core/jetty-server/src/test/resources/jetty-logging.properties b/jetty-core/jetty-server/src/test/resources/jetty-logging.properties index 553a8d6b3285..523e59d56a9d 100644 --- a/jetty-core/jetty-server/src/test/resources/jetty-logging.properties +++ b/jetty-core/jetty-server/src/test/resources/jetty-logging.properties @@ -1,4 +1,4 @@ -#org.eclipse.jetty.LEVEL=DEBUG +org.eclipse.jetty.LEVEL=DEBUG #org.eclipse.jetty.http.HttpParser.LEVEL=INFO #org.eclipse.jetty.util.LEVEL=DEBUG #org.eclipse.jetty.io.LEVEL=DEBUG diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java index fb496b78d705..2c140f89c56b 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java @@ -26,6 +26,7 @@ import jakarta.servlet.http.HttpServletMapping; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.ComplianceViolations; import org.eclipse.jetty.http.HttpException; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.UriCompliance; @@ -246,7 +247,8 @@ private static void checkUriViolations(HttpURI uri, Request baseRequest) { HttpChannel channel = baseRequest.getHttpChannel(); UriCompliance compliance = channel == null || channel.getHttpConfiguration() == null ? null : channel.getHttpConfiguration().getUriCompliance(); - String illegalState = UriCompliance.checkUriCompliance(compliance, uri); + ComplianceViolations complianceViolations = channel.getConnector().getBean(ComplianceViolations.class); + String illegalState = UriCompliance.checkUriCompliance(compliance, uri, complianceViolations); if (illegalState != null) throw new IllegalStateException(illegalState); } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java index 6371dc63ac9a..a5d33d12d43d 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/MultiPartFormInputStream.java @@ -28,9 +28,9 @@ import java.nio.file.Path; import java.nio.file.StandardCopyOption; import java.nio.file.StandardOpenOption; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; -import java.util.EnumSet; import java.util.Iterator; import java.util.List; import java.util.stream.Collectors; @@ -38,7 +38,9 @@ import jakarta.servlet.MultipartConfigElement; import jakarta.servlet.ServletInputStream; import jakarta.servlet.http.Part; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HttpField; +import org.eclipse.jetty.http.MultiPartCompliance; import org.eclipse.jetty.util.BufferUtil; import org.eclipse.jetty.util.ByteArrayOutputStream2; import org.eclipse.jetty.util.ExceptionUtil; @@ -92,12 +94,14 @@ private enum State DELETED } + record NonCompliance(ComplianceViolation.Mode mode, MultiPartCompliance.Violation violation, String detail) {} + private static final Logger LOG = LoggerFactory.getLogger(MultiPartFormInputStream.class); private static final QuotedStringTokenizer QUOTED_STRING_TOKENIZER = QuotedStringTokenizer.builder().delimiters(";").ignoreOptionalWhiteSpace().allowEmbeddedQuotes().build(); private final AutoLock _lock = new AutoLock(); private final MultiMap _parts = new MultiMap<>(); - private final EnumSet _nonComplianceWarnings = EnumSet.noneOf(NonCompliance.class); + private final List _nonComplianceWarnings = new ArrayList<>(); private final InputStream _in; private final MultipartConfigElement _config; private final File _contextTmpDir; @@ -110,27 +114,10 @@ private enum State private volatile int _bufferSize = 16 * 1024; private State state = State.UNPARSED; - public enum NonCompliance - { - TRANSFER_ENCODING("https://tools.ietf.org/html/rfc7578#section-4.7"); - - final String _rfcRef; - - NonCompliance(String rfcRef) - { - _rfcRef = rfcRef; - } - - public String getURL() - { - return _rfcRef; - } - } - /** * @return an EnumSet of non compliances with the RFC that were accepted by this parser */ - public EnumSet getNonComplianceWarnings() + public List getNonComplianceWarnings() { return _nonComplianceWarnings; } @@ -711,12 +698,12 @@ public void parsedField(String key, String value) else if (key.equalsIgnoreCase("content-type")) contentType = value; - // Transfer encoding is not longer considers as it is deprecated as per + // Content Transfer encoding is no longer considered as it is deprecated as per // https://tools.ietf.org/html/rfc7578#section-4.7 if (key.equalsIgnoreCase("content-transfer-encoding")) { if (!"8bit".equalsIgnoreCase(value) && !"binary".equalsIgnoreCase(value)) - _nonComplianceWarnings.add(NonCompliance.TRANSFER_ENCODING); + _nonComplianceWarnings.add(new NonCompliance(MultiPartCompliance.RFC7578, MultiPartCompliance.Violation.CONTENT_TRANSFER_ENCODING, value)); } } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index 9a9d36d04767..b23d46a143c1 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -65,9 +65,9 @@ import jakarta.servlet.http.PushBuilder; import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.ComplianceViolation; +import org.eclipse.jetty.http.ComplianceViolations; import org.eclipse.jetty.http.CookieCache; import org.eclipse.jetty.http.CookieCompliance; -import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; @@ -83,6 +83,7 @@ import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.security.UserIdentity; +import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.HttpCookieUtils; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.Session; @@ -533,19 +534,13 @@ public HttpChannelState getHttpChannelState() return _channel.getState(); } + /** + * @deprecated use core level ComplianceViolation.Listener instead. - will be removed in Jetty 12.1.0 + */ + @Deprecated(since = "12.0.6", forRemoval = true) public ComplianceViolation.Listener getComplianceViolationListener() { - if (_channel instanceof ComplianceViolation.Listener) - { - return (ComplianceViolation.Listener)_channel; - } - - ComplianceViolation.Listener listener = _channel.getConnector().getBean(ComplianceViolation.Listener.class); - if (listener == null) - { - listener = _channel.getServer().getBean(ComplianceViolation.Listener.class); - } - return listener; + return null; } /** @@ -1925,7 +1920,7 @@ private Collection getParts(MultiMap params) throws IOException _multiParts = newMultiParts(config, maxFormKeys); Collection parts = _multiParts.getParts(); - setNonComplianceViolationsOnRequest(); + reportComplianceViolations(); String formCharset = null; Part charsetPart = _multiParts.getPart("_charset_"); @@ -1991,20 +1986,21 @@ else if (getCharacterEncoding() != null) return _multiParts.getParts(); } - private void setNonComplianceViolationsOnRequest() + private void reportComplianceViolations() { - @SuppressWarnings("unchecked") - List violations = (List)getAttribute(HttpCompliance.VIOLATIONS_ATTR); - if (violations != null) - return; + Connector connector = getHttpChannel().getConnector(); + if (connector == null) + return; // not using a connector? + + ComplianceViolations complianceViolations = connector.getBean(ComplianceViolations.class); + if (complianceViolations == null) + return; // no violation reporting active - EnumSet nonComplianceWarnings = _multiParts.getNonComplianceWarnings(); - violations = new ArrayList<>(); + List nonComplianceWarnings = _multiParts.getNonComplianceWarnings(); for (MultiPartFormInputStream.NonCompliance nc : nonComplianceWarnings) { - violations.add(nc.name() + ": " + nc.getURL()); + complianceViolations.onComplianceViolation(nc.mode(), nc.violation(), nc.detail()); } - setAttribute(HttpCompliance.VIOLATIONS_ATTR, violations); } private MultiPartFormInputStream newMultiParts(MultipartConfigElement config, int maxParts) throws IOException From b86a4156ca09ec3fab4e9fa286f752ced0585702 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 8 Jan 2024 16:57:15 -0600 Subject: [PATCH 05/31] Revert change to jetty-logging.properties --- .../jetty-server/src/test/resources/jetty-logging.properties | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-core/jetty-server/src/test/resources/jetty-logging.properties b/jetty-core/jetty-server/src/test/resources/jetty-logging.properties index 523e59d56a9d..553a8d6b3285 100644 --- a/jetty-core/jetty-server/src/test/resources/jetty-logging.properties +++ b/jetty-core/jetty-server/src/test/resources/jetty-logging.properties @@ -1,4 +1,4 @@ -org.eclipse.jetty.LEVEL=DEBUG +#org.eclipse.jetty.LEVEL=DEBUG #org.eclipse.jetty.http.HttpParser.LEVEL=INFO #org.eclipse.jetty.util.LEVEL=DEBUG #org.eclipse.jetty.io.LEVEL=DEBUG From d8b5bb8daac9502437822138593f8e0bafd214b8 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Thu, 11 Jan 2024 11:25:47 -0600 Subject: [PATCH 06/31] Fix javadoc typos --- .../main/java/org/eclipse/jetty/http/MultiPartByteRanges.java | 2 +- .../src/main/java/org/eclipse/jetty/http/MultiPartFormData.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java index 814518d6b7e3..0afe1dfd836b 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartByteRanges.java @@ -45,7 +45,7 @@ * Content.Source content = ...; * * // Create and configure MultiPartByteRanges. - * MultiPartByteRanges byteRanges = new MultiPartByteRanges(boundary); + * MultiPartByteRanges.Parser byteRanges = new MultiPartByteRanges.Parser(boundary); * * // Parse the content. * byteRanges.parse(content) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index 8e8b15122f11..c8b056f71a34 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -55,7 +55,7 @@ * Content.Source content = ...; * * // Create and configure MultiPartFormData. - * MultiPartFormData formData = new MultiPartFormData(boundary); + * MultiPartFormData.Parser formData = new MultiPartFormData.Parser(boundary); * // Where to store the files. * formData.setFilesDirectory(Path.of("/tmp")); * // Max 1 MiB files. From 3818664b8c0575127fd5a095a72f49ebbf01061c Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 16 Jan 2024 15:15:24 -0600 Subject: [PATCH 07/31] Issue #11253 - Alternate approach with split ComplianceViolation.Listener --- .../jetty/http/ComplianceViolation.java | 151 +++++++++++++++++- .../jetty/http/ComplianceViolations.java | 111 ------------- .../org/eclipse/jetty/http/HttpParser.java | 25 +-- .../server/internal/HttpStreamOverHTTP2.java | 10 ++ .../jetty/http2/tests/HTTP2CServerTest.java | 2 +- .../server/internal/HttpStreamOverHTTP3.java | 10 ++ .../jetty/server/HttpConnectionFactory.java | 12 +- .../org/eclipse/jetty/server/Request.java | 6 +- .../org/eclipse/jetty/server/Response.java | 10 +- .../java/org/eclipse/jetty/server/Server.java | 39 +++++ .../server/internal/HttpChannelState.java | 62 +------ .../jetty/server/internal/HttpConnection.java | 55 +++++-- .../jetty/server/DelayedServerTest.java | 2 +- .../jetty/server/ExtendedServerTest.java | 2 +- .../ForwardedRequestCustomizerTest.java | 2 +- .../jetty/server/ResponseCompleteTest.java | 2 +- .../jetty/server/ServerHttpCookieTest.java | 6 +- .../org/eclipse/jetty/server/ServerTest.java | 2 +- .../SlowClientWithPipelinedRequestTest.java | 2 +- .../org/eclipse/jetty/server/StopTest.java | 2 +- .../eclipse/jetty/ee9/nested/Dispatcher.java | 6 +- .../org/eclipse/jetty/ee9/nested/Request.java | 12 +- 22 files changed, 301 insertions(+), 230 deletions(-) delete mode 100644 jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolations.java diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java index 664053f66465..d4e46e55922a 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java @@ -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}. @@ -90,16 +96,159 @@ public String toString() */ interface Listener { + /** + * 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) + { + } + + /** + * A Request has ended. + * + * @param request the request attribtues, 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) { + 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) { - onComplianceViolation(new Event(mode, violation, details)); + } + } + + /** + * A Listener that represents multiple user {@link ComplianceViolation.Listener} instances + */ + class ListenerCollection implements Listener + { + private static final Logger LOG = LoggerFactory.getLogger(ListenerCollection.class); + private final List 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 userListeners) + { + this.userListeners = userListeners; + } + + /** + * Get a specific ComplianceViolation.Listener from collected user listeners + * @param clazz the class to look for + * @return the instance of the class in the user listeners + * @param the type of class + */ + public T getUserListener(Class clazz) + { + for (ComplianceViolation.Listener listener : userListeners) + { + if (clazz.isInstance(listener)) + return clazz.cast(listener); + } + return null; + } + + @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); + } + } + } + } + + interface ListenerFactory + { + Listener newComplianceViolationListener(); + } + + public class LoggingListenerFactory implements ListenerFactory, Listener + { + private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolation.class); + + @Override + public void onComplianceViolation(Event event) + { + if (LOG.isDebugEnabled()) + LOG.debug(event.toString()); + } + + @Override + public Listener newComplianceViolationListener() + { + return this; + } + } + + public class CapturingListenerFactory implements ListenerFactory + { + @Override + public Listener newComplianceViolationListener() + { + return new CapturingListener(); + } + } + + public class CapturingListener implements Listener + { + private List events = new ArrayList<>(); + + @Override + public void onRequestBegin(Attributes request) + { + if (request != null) + request.setAttribute(ComplianceViolation.class.getPackageName() + ".complianceViolations", events); + } + + @Override + public void onRequestEnd(Attributes request) + { + } + + @Override + public void onComplianceViolation(Event event) + { + events.add(event); } } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolations.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolations.java deleted file mode 100644 index 58c2ff2727d7..000000000000 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolations.java +++ /dev/null @@ -1,111 +0,0 @@ -// -// ======================================================================== -// 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; - -import java.util.ArrayList; -import java.util.List; - -import org.eclipse.jetty.util.Attributes; -import org.slf4j.Logger; -import org.slf4j.LoggerFactory; - -/** - * A representation of ComplianceViolation.Listeners events that have occurred. - */ -public class ComplianceViolations implements ComplianceViolation.Listener -{ - public static final String VIOLATIONS_ATTR = ComplianceViolations.class.getName(); - private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolations.class); - private final List userListeners; - private List events; - private Attributes attributes; - - /** - * Construct a new ComplianceViolations that will notify user listeners. - * @param userListeners the user listeners to notify, null or empty is allowed. - */ - public ComplianceViolations(List userListeners) - { - this.userListeners = userListeners; - } - - @Override - public void onComplianceViolation(ComplianceViolation.Event event) - { - assert event != null; - - notifyUserListeners(event); - addEvent(event); - if (LOG.isDebugEnabled()) - LOG.debug(event.toString()); - } - - /** - * Clear out the tracked {@link Attributes} object and events; - */ - public void recycle() - { - this.events = null; - this.attributes = null; - } - - /** - * Start tracking an {@link Attributes} implementation to store the events in. - * @param attributes the attributes object to store the event list in. Stored in key name {@link #VIOLATIONS_ATTR} - */ - public void setAttribute(Attributes attributes) - { - this.attributes = attributes; - this.attributes.setAttribute(VIOLATIONS_ATTR, events); - } - - /** - * The events that have been recorded. - * - * @return the list of Events recorded. - */ - public List getEvents() - { - return events; - } - - private void addEvent(ComplianceViolation.Event event) - { - if (this.events == null) - { - this.events = new ArrayList<>(); - if (this.attributes != null) - setAttribute(this.attributes); - } - this.events.add(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); - } - } - } -} diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index aa9674c753ea..4180c331c506 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -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); @@ -294,27 +293,21 @@ public HttpParser(RequestHandler handler, HttpCompliance compliance) public HttpParser(RequestHandler handler, int maxHeaderBytes, HttpCompliance compliance) { - this(handler, null, maxHeaderBytes, compliance == null ? compliance() : compliance, null); - } - - public HttpParser(RequestHandler handler, int maxHeaderBytes, HttpCompliance compliance, ComplianceViolation.Listener complianceListener) - { - this(handler, null, maxHeaderBytes, compliance == null ? compliance() : compliance, complianceListener); + this(handler, null, maxHeaderBytes, compliance == null ? compliance() : compliance); } public HttpParser(ResponseHandler handler, int maxHeaderBytes, HttpCompliance compliance) { - this(null, handler, maxHeaderBytes, compliance == null ? compliance() : compliance, null); + this(null, handler, maxHeaderBytes, compliance == null ? compliance() : compliance); } - private HttpParser(RequestHandler requestHandler, ResponseHandler responseHandler, int maxHeaderBytes, HttpCompliance compliance, ComplianceViolation.Listener complianceListener) + private HttpParser(RequestHandler requestHandler, ResponseHandler responseHandler, int maxHeaderBytes, HttpCompliance compliance) { _handler = requestHandler != null ? requestHandler : responseHandler; _requestHandler = requestHandler; _responseHandler = responseHandler; _maxHeaderBytes = maxHeaderBytes; _complianceMode = compliance; - _complianceListener = complianceListener; } public long getBeginNanoTime() @@ -362,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) @@ -2025,6 +2018,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. * diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java index e237ad9b6fdb..a45a9403a931 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java @@ -18,6 +18,7 @@ import java.util.function.BiConsumer; import java.util.function.Supplier; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HttpException; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpMethod; @@ -38,6 +39,8 @@ import org.eclipse.jetty.io.EndPoint; 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; @@ -84,6 +87,13 @@ public Runnable onRequest(HeadersFrame frame) Runnable handler = _httpChannel.onRequest(_requestMetaData); + ComplianceViolation.Listener listener = Server.newComplianceViolationListener(_httpChannel.getConnectionMetaData().getConnector()); + Request request = _httpChannel.getRequest(); + request.setAttribute(ComplianceViolation.Listener.class.getName(), listener); + listener.onRequestBegin(request); + // Note UriCompliance is done by HandlerInvoker + // TODO: perform HttpCompliance violation checks? + if (frame.isEndStream()) { try (AutoLock ignored = lock.lock()) diff --git a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2CServerTest.java b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2CServerTest.java index ffbc90e291df..a2c657488894 100644 --- a/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2CServerTest.java +++ b/jetty-core/jetty-http2/jetty-http2-tests/src/test/java/org/eclipse/jetty/http2/tests/HTTP2CServerTest.java @@ -304,7 +304,7 @@ public void testHTTP20DirectWithoutH2C() throws Exception @Override public Connection newConnection(Connector connector, EndPoint endPoint) { - HttpConnection connection = new HttpConnection(getHttpConfiguration(), connector, endPoint, isRecordHttpComplianceViolations()) + HttpConnection connection = new HttpConnection(getHttpConfiguration(), connector, endPoint) { @Override public void onFillable() diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index 74f7af1422e2..366bfece5062 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -19,6 +19,7 @@ import java.util.function.BiConsumer; import java.util.function.Supplier; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HttpException; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpMethod; @@ -33,6 +34,8 @@ import org.eclipse.jetty.io.Content; 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; @@ -74,6 +77,13 @@ public Runnable onRequest(HeadersFrame frame) Runnable handler = httpChannel.onRequest(requestMetaData); + ComplianceViolation.Listener listener = Server.newComplianceViolationListener(this.httpChannel.getConnectionMetaData().getConnector()); + Request request = this.httpChannel.getRequest(); + request.setAttribute(ComplianceViolation.Listener.class.getName(), listener); + listener.onRequestBegin(request); + // Note UriCompliance is done by HandlerInvoker + // TODO: perform HttpCompliance violation checks? + if (frame.isLast()) { try (AutoLock ignored = lock.lock()) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java index 55167f4ae631..070d96c08809 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java @@ -15,6 +15,7 @@ import java.util.Objects; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; @@ -27,7 +28,7 @@ * {@link HttpConnection}s are configured by a {@link HttpConfiguration} instance that is either created by * default or passed in to the constructor. */ -public class HttpConnectionFactory extends AbstractConnectionFactory implements HttpConfiguration.ConnectionFactory +public class HttpConnectionFactory extends AbstractConnectionFactory implements HttpConfiguration.ConnectionFactory, ConnectionFactory.Configuring { private final HttpConfiguration _config; private boolean _recordHttpComplianceViolations; @@ -54,6 +55,13 @@ public HttpConfiguration getHttpConfiguration() return _config; } + @Override + public void configure(Connector connector) + { + if (isRecordHttpComplianceViolations()) + addBean(new ComplianceViolation.LoggingListenerFactory()); + } + public boolean isRecordHttpComplianceViolations() { return _recordHttpComplianceViolations; @@ -87,7 +95,7 @@ public void setUseOutputDirectByteBuffers(boolean useOutputDirectByteBuffers) @Override public Connection newConnection(Connector connector, EndPoint endPoint) { - HttpConnection connection = new HttpConnection(_config, connector, endPoint, isRecordHttpComplianceViolations()); + HttpConnection connection = new HttpConnection(_config, connector, endPoint); connection.setUseInputDirectByteBuffers(isUseInputDirectByteBuffers()); connection.setUseOutputDirectByteBuffers(isUseOutputDirectByteBuffers()); return configure(connection, connector, endPoint); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 90d475ad4b5a..724af9850c4e 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -34,7 +34,7 @@ import java.util.function.Function; import java.util.function.Predicate; -import org.eclipse.jetty.http.ComplianceViolations; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.CookieCache; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpFields; @@ -590,8 +590,8 @@ static List getCookies(Request request) CookieCache cookieCache = (CookieCache)request.getComponents().getCache().getAttribute(CACHE_ATTRIBUTE); if (cookieCache == null) { - ComplianceViolations complianceViolations = request.getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); - cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance(), complianceViolations); + ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)request.getAttribute(ComplianceViolation.Listener.class.getName()); + cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance(), complianceViolationListener); request.getComponents().getCache().setAttribute(CACHE_ATTRIBUTE, cookieCache); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index e7ce14882dc9..3bca1397e00f 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -20,8 +20,8 @@ import java.util.concurrent.TimeoutException; import java.util.function.Supplier; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.ComplianceViolationException; -import org.eclipse.jetty.http.ComplianceViolations; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpException; @@ -386,8 +386,8 @@ static void addCookie(Response response, HttpCookie cookie) } catch (ComplianceViolationException e) { - ComplianceViolations complianceViolations = request.getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); - complianceViolations.onComplianceViolation(e.getEvent()); + ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)request.getAttribute(ComplianceViolation.Listener.class.getName()); + complianceViolationListener.onComplianceViolation(e.getEvent()); throw e; } @@ -421,8 +421,8 @@ static void putCookie(Response response, HttpCookie cookie) } catch (ComplianceViolationException e) { - ComplianceViolations complianceViolations = request.getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); - complianceViolations.onComplianceViolation(e.getEvent()); + ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)request.getAttribute(ComplianceViolation.Listener.class.getName()); + complianceViolationListener.onComplianceViolation(e.getEvent()); throw e; } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index bed6d6fbcffa..793be20afc94 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -29,6 +29,7 @@ import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.DateGenerator; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; @@ -151,6 +152,44 @@ public Server(@Name("threadPool") ThreadPool threadPool, @Name("scheduler") Sche addBean(FileSystemPool.INSTANCE, false); } + /** + * Get a new ComplianceViolation.Listener suitable for given Connector. + * + * @param connector the connector to base the ComplianceViolation.Listener off of. + * @return the ComplianceViolation.Listener implementation, or null if {@link HttpConnectionFactory#isRecordHttpComplianceViolations()} is false, + * or there are no ComplianceViolation.Listener implementations registered. + */ + public static ComplianceViolation.Listener newComplianceViolationListener(Connector connector) + { + HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class); + if (httpConnectionFactory == null) + return null; + + // Is this connector recording compliance violations? + if (!httpConnectionFactory.isRecordHttpComplianceViolations()) + return null; + + // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled + // This also means that any user provided ComplianceViolation.Listener beans will only be + // used when the configuration on the HttpConnectionFactory allows then to be used. + + // Look for optional user provided ComplianceViolation.ListenerSupplier + List userListeners = new ArrayList<>(); + for (ComplianceViolation.ListenerFactory listenerFactory: connector.getBeans(ComplianceViolation.ListenerFactory.class)) + userListeners.add(listenerFactory.newComplianceViolationListener()); + for (ComplianceViolation.ListenerFactory listenerFactory: connector.getServer().getBeans(ComplianceViolation.ListenerFactory.class)) + userListeners.add(listenerFactory.newComplianceViolationListener()); + + // No listeners? then we are done + if (userListeners.isEmpty()) + return null; + // Only 1 listener, just return it. + if (userListeners.size() == 1) + return userListeners.get(0); + // More than 1, establish ComplianceViolations collection + return new ComplianceViolation.ListenerCollection(userListeners); + } + public Handler getDefaultHandler() { return _defaultHandler; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index fc9a09ea1d4c..f1016530bf3f 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -16,9 +16,7 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.nio.channels.WritePendingException; -import java.util.ArrayList; import java.util.HashMap; -import java.util.List; import java.util.Objects; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeoutException; @@ -30,7 +28,6 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.ComplianceViolation; -import org.eclipse.jetty.http.ComplianceViolations; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpHeader; @@ -47,12 +44,10 @@ import org.eclipse.jetty.io.Content; import org.eclipse.jetty.server.Components; import org.eclipse.jetty.server.ConnectionMetaData; -import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.Context; import org.eclipse.jetty.server.Handler; import org.eclipse.jetty.server.HttpChannel; import org.eclipse.jetty.server.HttpConfiguration; -import org.eclipse.jetty.server.HttpConnectionFactory; import org.eclipse.jetty.server.HttpStream; import org.eclipse.jetty.server.Request; import org.eclipse.jetty.server.RequestLog; @@ -108,7 +103,6 @@ private enum StreamSendState private final SerializedInvoker _readInvoker; private final SerializedInvoker _writeInvoker; private final ResponseHttpFields _responseHeaders = new ResponseHttpFields(); - private final ComplianceViolations _complianceViolations; private Thread _handling; private boolean _handled; private StreamSendState _streamSendState = StreamSendState.SENDING; @@ -132,46 +126,6 @@ public HttpChannelState(ConnectionMetaData connectionMetaData) // The SerializedInvoker is used to prevent infinite recursion of callbacks calling methods calling callbacks etc. _readInvoker = new HttpChannelSerializedInvoker(); _writeInvoker = new HttpChannelSerializedInvoker(); - _complianceViolations = newComplianceViolations(); - } - - /** - * @return a new ComplianceViolations if configuration enables it. - */ - private ComplianceViolations newComplianceViolations() - { - Connector connector = getConnectionMetaData().getConnector(); - if (connector == null) - return null; - - HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class); - if (httpConnectionFactory != null) - { - // Is this connector recording compliance violations? - if (httpConnectionFactory.isRecordHttpComplianceViolations()) - { - // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled - // This also means that any user provided ComplianceViolation.Listener beans will only be - // used when the configuration on the HttpConnectionFactory allows then to be used. - - // Look for optional user provided ComplianceViolation.Listeners - List userListeners = new ArrayList<>(); - userListeners.addAll(connector.getBeans(ComplianceViolation.Listener.class)); - userListeners.addAll(connector.getServer().getBeans(ComplianceViolation.Listener.class)); - - // Establish ComplianceViolations Tracker - ComplianceViolations violations = new ComplianceViolations(userListeners); - - // Store tracker (to be accessed elsewhere) - // If this bean is not present on the connector, then it is safe to assume that the - // HttpConnectionFactory is not allowing the recording of compliance violations. - connector.addBean(violations); - - return violations; - } - } - - return null; } @Override @@ -195,8 +149,6 @@ public void recycle() _request = null; _response = null; _oldIdleTimeout = 0; - if (_complianceViolations != null) - _complianceViolations.recycle(); // Break the link between channel and stream. _stream = null; _committedContentLength = -1; @@ -286,15 +238,6 @@ public Runnable onRequest(MetaData.Request request) if (LOG.isDebugEnabled()) LOG.debug("onRequest {} {}", request, this); - // Handle UriCompliance Violations - if (request.getHttpURI().hasViolations()) - { - UriCompliance compliance = getHttpConfiguration().getUriCompliance(); - String badMessage = UriCompliance.checkUriCompliance(compliance, request.getHttpURI(), _complianceViolations); - if (badMessage != null) - throw new BadMessageException(badMessage); - } - try (AutoLock ignored = _lock.lock()) { if (_stream == null) @@ -319,8 +262,6 @@ public Runnable onRequest(MetaData.Request request) if (idleTO >= 0 && _oldIdleTimeout != idleTO) _stream.setIdleTimeout(idleTO); - _complianceViolations.setAttribute(_request); - // This is deliberately not serialized to allow a handler to block. return _handlerInvoker; } @@ -632,7 +573,8 @@ public void run() HttpURI uri = request.getHttpURI(); if (uri.hasViolations()) { - String badMessage = UriCompliance.checkUriCompliance(getConnectionMetaData().getHttpConfiguration().getUriCompliance(), uri, _complianceViolations); + ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)request.getAttribute(ComplianceViolation.Listener.class.getName()); + String badMessage = UriCompliance.checkUriCompliance(getConnectionMetaData().getHttpConfiguration().getUriCompliance(), uri, complianceViolationListener); if (badMessage != null) throw new BadMessageException(badMessage); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 1ba352374beb..f6b6b1375c35 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -27,7 +27,7 @@ import java.util.concurrent.atomic.LongAdder; import org.eclipse.jetty.http.BadMessageException; -import org.eclipse.jetty.http.ComplianceViolations; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HostPortHttpField; import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpException; @@ -99,7 +99,6 @@ public class HttpConnection extends AbstractMetaDataConnection implements Runnab private final Lazy _attributes = new Lazy(); private final DemandContentCallback _demandContentCallback = new DemandContentCallback(); private final SendCallback _sendCallback = new SendCallback(); - private final boolean _recordHttpComplianceViolations; private final LongAdder bytesIn = new LongAdder(); private final LongAdder bytesOut = new LongAdder(); private final AtomicBoolean _handling = new AtomicBoolean(false); @@ -132,7 +131,16 @@ protected static HttpConnection setCurrentConnection(HttpConnection connection) return last; } + /** + * @deprecated use {@link #HttpConnection(HttpConfiguration, Connector, EndPoint)} instead. Will be removed in Jetty 12.1.0 + */ + @Deprecated(since = "12.0.5", forRemoval = true) public HttpConnection(HttpConfiguration configuration, Connector connector, EndPoint endPoint, boolean recordComplianceViolations) + { + this(configuration, connector, endPoint); + } + + public HttpConnection(HttpConfiguration configuration, Connector connector, EndPoint endPoint) { super(connector, configuration, endPoint); _id = __connectionIdGenerator.getAndIncrement(); @@ -141,7 +149,6 @@ public HttpConnection(HttpConfiguration configuration, Connector connector, EndP _httpChannel = newHttpChannel(connector.getServer(), configuration); _requestHandler = newRequestHandler(); _parser = newHttpParser(configuration.getHttpCompliance()); - _recordHttpComplianceViolations = recordComplianceViolations; if (LOG.isDebugEnabled()) LOG.debug("New HTTP Connection {}", this); } @@ -152,9 +159,13 @@ public InvocationType getInvocationType() return getServer().getInvocationType(); } + /** + * @deprecated No replacement, no longer used within {@link HttpConnection}, will be removed in Jetty 12.1.0 + */ + @Deprecated(since = "12.0.5", forRemoval = true) public boolean isRecordHttpComplianceViolations() { - return _recordHttpComplianceViolations; + return false; } protected HttpGenerator newHttpGenerator() @@ -164,8 +175,7 @@ protected HttpGenerator newHttpGenerator() protected HttpParser newHttpParser(HttpCompliance compliance) { - ComplianceViolations complianceViolations = getHttpChannel().getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); - HttpParser parser = new HttpParser(_requestHandler, getHttpConfiguration().getRequestHeaderSize(), compliance, complianceViolations); + HttpParser parser = new HttpParser(_requestHandler, getHttpConfiguration().getRequestHeaderSize(), compliance); parser.setHeaderCacheSize(getHttpConfiguration().getHeaderCacheSize()); parser.setHeaderCacheCaseSensitive(getHttpConfiguration().isHeaderCacheCaseSensitive()); return parser; @@ -183,7 +193,7 @@ protected HttpStreamOverHTTP1 newHttpStream(String method, String uri, HttpVersi protected RequestHandler newRequestHandler() { - return new RequestHandler(); + return new RequestHandler(getConnector()); } public Server getServer() @@ -912,6 +922,12 @@ public String toString() protected class RequestHandler implements HttpParser.RequestHandler { private Throwable _failure; + private ComplianceViolation.Listener _listener; + + public RequestHandler(Connector connector) + { + _listener = Server.newComplianceViolationListener(connector); + } @Override public void startRequest(String method, String uri, HttpVersion version) @@ -959,6 +975,13 @@ public boolean contentComplete() return false; } + @Override + public void onViolation(ComplianceViolation.Event event) + { + if (_listener != null) + _listener.onComplianceViolation(event); + } + @Override public void parsedTrailer(HttpField field) { @@ -977,6 +1000,8 @@ public boolean messageComplete() stream._chunk = new Trailers(_trailers.asImmutable()); else stream._chunk = Content.Chunk.EOF; + + _listener.onRequestEnd(getHttpChannel().getRequest()); return false; } @@ -986,6 +1011,8 @@ public void badMessage(HttpException failure) if (LOG.isDebugEnabled()) LOG.debug("badMessage {} {}", HttpConnection.this, failure); + _listener.onRequestEnd(getHttpChannel().getRequest()); + _failure = (Throwable)failure; _generator.setPersistent(false); @@ -1154,13 +1181,12 @@ public Runnable headerComplete() if (_uri.hasViolations()) { compliance = getHttpConfiguration().getUriCompliance(); - ComplianceViolations complianceViolations = getHttpChannel().getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); - String badMessage = UriCompliance.checkUriCompliance(compliance, _uri, complianceViolations); + String badMessage = UriCompliance.checkUriCompliance(compliance, _uri, _requestHandler._listener); if (badMessage != null) throw new BadMessageException(badMessage); } - // Check host field matches the authority in the any absolute URI or is not blank + // Check host field matches the authority in the absolute URI or is not blank if (_hostField != null) { if (_uri.isAbsolute()) @@ -1170,9 +1196,8 @@ public Runnable headerComplete() HttpCompliance httpCompliance = getHttpConfiguration().getHttpCompliance(); if (httpCompliance.allows(MISMATCHED_AUTHORITY)) { - ComplianceViolations complianceViolations = getHttpChannel().getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); - if (complianceViolations != null) - complianceViolations.onComplianceViolation(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString()); + if (_requestHandler._listener != null) + _requestHandler._listener.onComplianceViolation(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString()); } else throw new BadMessageException("Authority!=Host"); @@ -1217,6 +1242,10 @@ public boolean is100ContinueExpected() Runnable handle = _httpChannel.onRequest(_request); ++_requests; + Request request = _httpChannel.getRequest(); + request.setAttribute(ComplianceViolation.Listener.class.getName(), _requestHandler._listener); + _requestHandler._listener.onRequestBegin(request); + if (_complianceViolations != null && !_complianceViolations.isEmpty()) { _httpChannel.getRequest().setAttribute(HttpCompliance.VIOLATIONS_ATTR, _complianceViolations); diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/DelayedServerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/DelayedServerTest.java index 0047a14fc050..3a1ec920d762 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/DelayedServerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/DelayedServerTest.java @@ -48,7 +48,7 @@ private static class DelayedHttpConnection extends HttpConnection { public DelayedHttpConnection(HttpConfiguration config, Connector connector, EndPoint endPoint) { - super(config, connector, endPoint, false); + super(config, connector, endPoint); } @Override diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ExtendedServerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ExtendedServerTest.java index b5517412145a..7d9aa6812f80 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ExtendedServerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ExtendedServerTest.java @@ -91,7 +91,7 @@ private static class ExtendedHttpConnection extends HttpConnection { public ExtendedHttpConnection(HttpConfiguration config, Connector connector, EndPoint endPoint) { - super(config, connector, endPoint, false); + super(config, connector, endPoint); } @Override diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java index a2e154d19d97..2b8b97036a4d 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ForwardedRequestCustomizerTest.java @@ -85,7 +85,7 @@ public void init() throws Exception @Override public Connection newConnection(Connector connector, EndPoint endPoint) { - HttpConnection connection = new HttpConnection(getHttpConfiguration(), connector, endPoint, isRecordHttpComplianceViolations()) + HttpConnection connection = new HttpConnection(getHttpConfiguration(), connector, endPoint) { @Override public SocketAddress getLocalSocketAddress() diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseCompleteTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseCompleteTest.java index e6e1d003aa77..482cfe2a1e5c 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseCompleteTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ResponseCompleteTest.java @@ -129,7 +129,7 @@ public void testHandleCallbackCompleting(boolean throwFromHandler) throws Except @Override public Connection newConnection(Connector connector, EndPoint endPoint) { - HttpConnection connection = new HttpConnection(getHttpConfiguration(), connector, endPoint, isRecordHttpComplianceViolations()) + HttpConnection connection = new HttpConnection(getHttpConfiguration(), connector, endPoint) { @Override protected HttpStreamOverHTTP1 newHttpStream(String method, String uri, HttpVersion version) diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java index cec91cfd72bf..a1a163e9bebb 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java @@ -16,7 +16,7 @@ import java.util.List; import java.util.stream.Stream; -import org.eclipse.jetty.http.ComplianceViolations; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.CookieParser; import org.eclipse.jetty.http.HttpCookie; @@ -64,7 +64,7 @@ public boolean handle(Request request, Response response, Callback callback) thr Fields.Field setCookie = parameters.get("SetCookie"); if (setCookie != null) { - ComplianceViolations complianceViolations = request.getConnectionMetaData().getConnector().getBean(ComplianceViolations.class); + ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)request.getAttribute(ComplianceViolation.Listener.class.getName()); CookieParser parser = CookieParser.newParser(new CookieParser.Handler() { @Override @@ -72,7 +72,7 @@ public void addCookie(String name, String value, int version, String domain, Str { Response.addCookie(response, HttpCookie.build(name, value, version).domain(domain).path(path).comment(comment).build()); } - }, RFC2965, complianceViolations); + }, RFC2965, complianceViolationListener); parser.parseField(setCookie.getValue()); } diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java index 60f5c817b331..a29615be93ef 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerTest.java @@ -63,7 +63,7 @@ public void prepare() throws Exception @Override public Connection newConnection(Connector connector, EndPoint endPoint) { - HttpConnection connection = new HttpConnection(getHttpConfiguration(), connector, endPoint, isRecordHttpComplianceViolations()) + HttpConnection connection = new HttpConnection(getHttpConfiguration(), connector, endPoint) { @Override protected HttpChannel newHttpChannel(Server server, HttpConfiguration configuration) diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/SlowClientWithPipelinedRequestTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/SlowClientWithPipelinedRequestTest.java index 9defb422283a..12069ad7e157 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/SlowClientWithPipelinedRequestTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/SlowClientWithPipelinedRequestTest.java @@ -47,7 +47,7 @@ public void startServer(Handler handler) throws Exception @Override public Connection newConnection(Connector connector, EndPoint endPoint) { - return configure(new HttpConnection(getHttpConfiguration(), connector, endPoint, isRecordHttpComplianceViolations()) + return configure(new HttpConnection(getHttpConfiguration(), connector, endPoint) { @Override public void onFillable() diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/StopTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/StopTest.java index 8406d6f1a72f..48bcf7e9a61b 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/StopTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/StopTest.java @@ -134,7 +134,7 @@ public void testSlowClose(long stopTimeout, long closeWait, Matcher stopTi public Connection newConnection(Connector con, EndPoint endPoint) { // Slow closing connection - HttpConnection conn = new HttpConnection(getHttpConfiguration(), con, endPoint, isRecordHttpComplianceViolations()) + HttpConnection conn = new HttpConnection(getHttpConfiguration(), con, endPoint) { @Override public void onClose(Throwable cause) diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java index 2c140f89c56b..d289bc1f0878 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java @@ -26,7 +26,7 @@ import jakarta.servlet.http.HttpServletMapping; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; -import org.eclipse.jetty.http.ComplianceViolations; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HttpException; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.UriCompliance; @@ -247,8 +247,8 @@ private static void checkUriViolations(HttpURI uri, Request baseRequest) { HttpChannel channel = baseRequest.getHttpChannel(); UriCompliance compliance = channel == null || channel.getHttpConfiguration() == null ? null : channel.getHttpConfiguration().getUriCompliance(); - ComplianceViolations complianceViolations = channel.getConnector().getBean(ComplianceViolations.class); - String illegalState = UriCompliance.checkUriCompliance(compliance, uri, complianceViolations); + ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)baseRequest.getAttribute(ComplianceViolation.Listener.class.getName()); + String illegalState = UriCompliance.checkUriCompliance(compliance, uri, complianceViolationListener); if (illegalState != null) throw new IllegalStateException(illegalState); } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index b23d46a143c1..b862bb7aa0f6 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -65,7 +65,6 @@ import jakarta.servlet.http.PushBuilder; import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.ComplianceViolation; -import org.eclipse.jetty.http.ComplianceViolations; import org.eclipse.jetty.http.CookieCache; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.HttpCookie; @@ -83,7 +82,6 @@ import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.RuntimeIOException; import org.eclipse.jetty.security.UserIdentity; -import org.eclipse.jetty.server.Connector; import org.eclipse.jetty.server.HttpCookieUtils; import org.eclipse.jetty.server.Server; import org.eclipse.jetty.server.Session; @@ -1988,18 +1986,14 @@ else if (getCharacterEncoding() != null) private void reportComplianceViolations() { - Connector connector = getHttpChannel().getConnector(); - if (connector == null) - return; // not using a connector? - - ComplianceViolations complianceViolations = connector.getBean(ComplianceViolations.class); - if (complianceViolations == null) + ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)getAttribute(ComplianceViolation.Listener.class.getName()); + if (complianceViolationListener == null) return; // no violation reporting active List nonComplianceWarnings = _multiParts.getNonComplianceWarnings(); for (MultiPartFormInputStream.NonCompliance nc : nonComplianceWarnings) { - complianceViolations.onComplianceViolation(nc.mode(), nc.violation(), nc.detail()); + complianceViolationListener.onComplianceViolation(nc.mode(), nc.violation(), nc.detail()); } } From 20e7e419189c8134a6b3c94f8954debe51536a3e Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 16 Jan 2024 15:37:59 -0600 Subject: [PATCH 08/31] Fix HttpParserTest --- .../test/java/org/eclipse/jetty/http/HttpParserTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java index 59823a34d89a..b0c7c4bc3889 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpParserTest.java @@ -3237,7 +3237,7 @@ public void init() private boolean _messageCompleted; private final List _complianceViolation = new ArrayList<>(); - private class Handler implements HttpParser.RequestHandler, HttpParser.ResponseHandler, ComplianceViolation.Listener + private class Handler implements HttpParser.RequestHandler, HttpParser.ResponseHandler { @Override public boolean content(ByteBuffer ref) @@ -3337,9 +3337,9 @@ public void earlyEOF() } @Override - public void onComplianceViolation(ComplianceViolation.Mode mode, ComplianceViolation violation, String reason) + public void onViolation(ComplianceViolation.Event event) { - _complianceViolation.add(violation); + _complianceViolation.add(event.violation()); } } From ee568b65de3f55f68b4a8c5d8ef4151d63b98c0f Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 16 Jan 2024 15:49:32 -0600 Subject: [PATCH 09/31] Cleanup implementation --- .../eclipse/jetty/http/ComplianceViolation.java | 8 +------- .../server/internal/HttpStreamOverHTTP2.java | 3 ++- .../server/internal/HttpStreamOverHTTP3.java | 3 ++- .../jetty/server/HttpConnectionFactory.java | 2 +- .../java/org/eclipse/jetty/server/Server.java | 17 ++++++++++------- .../jetty/server/internal/HttpConnection.java | 9 ++++++--- .../src/test/resources/jetty-logging.properties | 2 +- 7 files changed, 23 insertions(+), 21 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java index d4e46e55922a..8e5aa4a2a24f 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java @@ -202,7 +202,7 @@ interface ListenerFactory Listener newComplianceViolationListener(); } - public class LoggingListenerFactory implements ListenerFactory, Listener + public class LoggingListener implements Listener { private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolation.class); @@ -212,12 +212,6 @@ public void onComplianceViolation(Event event) if (LOG.isDebugEnabled()) LOG.debug(event.toString()); } - - @Override - public Listener newComplianceViolationListener() - { - return this; - } } public class CapturingListenerFactory implements ListenerFactory diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java index a45a9403a931..659640e0586a 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java @@ -90,7 +90,8 @@ public Runnable onRequest(HeadersFrame frame) ComplianceViolation.Listener listener = Server.newComplianceViolationListener(_httpChannel.getConnectionMetaData().getConnector()); Request request = _httpChannel.getRequest(); request.setAttribute(ComplianceViolation.Listener.class.getName(), listener); - listener.onRequestBegin(request); + if (listener != null) + listener.onRequestBegin(request); // Note UriCompliance is done by HandlerInvoker // TODO: perform HttpCompliance violation checks? diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index 366bfece5062..5326fe0a55c8 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -80,7 +80,8 @@ public Runnable onRequest(HeadersFrame frame) ComplianceViolation.Listener listener = Server.newComplianceViolationListener(this.httpChannel.getConnectionMetaData().getConnector()); Request request = this.httpChannel.getRequest(); request.setAttribute(ComplianceViolation.Listener.class.getName(), listener); - listener.onRequestBegin(request); + if (listener != null) + listener.onRequestBegin(request); // Note UriCompliance is done by HandlerInvoker // TODO: perform HttpCompliance violation checks? diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java index 070d96c08809..cae2cdfc1b36 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java @@ -59,7 +59,7 @@ public HttpConfiguration getHttpConfiguration() public void configure(Connector connector) { if (isRecordHttpComplianceViolations()) - addBean(new ComplianceViolation.LoggingListenerFactory()); + addBean(new ComplianceViolation.LoggingListener()); } public boolean isRecordHttpComplianceViolations() diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 793be20afc94..49368f513f9f 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -170,15 +170,18 @@ public static ComplianceViolation.Listener newComplianceViolationListener(Connec return null; // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled - // This also means that any user provided ComplianceViolation.Listener beans will only be + // This also means that any user provided ComplianceViolation.ListenerFactory beans will only be // used when the configuration on the HttpConnectionFactory allows then to be used. - // Look for optional user provided ComplianceViolation.ListenerSupplier + // Look for optional user provided ComplianceViolation.Listener and ComplianceViolation.ListenerFactory beans List userListeners = new ArrayList<>(); - for (ComplianceViolation.ListenerFactory listenerFactory: connector.getBeans(ComplianceViolation.ListenerFactory.class)) - userListeners.add(listenerFactory.newComplianceViolationListener()); - for (ComplianceViolation.ListenerFactory listenerFactory: connector.getServer().getBeans(ComplianceViolation.ListenerFactory.class)) - userListeners.add(listenerFactory.newComplianceViolationListener()); + for (org.eclipse.jetty.util.component.Container container: List.of(connector, connector.getServer())) + { + for (ComplianceViolation.Listener listener: container.getBeans(ComplianceViolation.Listener.class)) + userListeners.add(listener); + for (ComplianceViolation.ListenerFactory listenerFactory: container.getBeans(ComplianceViolation.ListenerFactory.class)) + userListeners.add(listenerFactory.newComplianceViolationListener()); + } // No listeners? then we are done if (userListeners.isEmpty()) @@ -186,7 +189,7 @@ public static ComplianceViolation.Listener newComplianceViolationListener(Connec // Only 1 listener, just return it. if (userListeners.size() == 1) return userListeners.get(0); - // More than 1, establish ComplianceViolations collection + // More than 1, establish ComplianceViolation.ListenerCollection for user listeners return new ComplianceViolation.ListenerCollection(userListeners); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index f6b6b1375c35..d415ab14953e 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -1001,7 +1001,8 @@ public boolean messageComplete() else stream._chunk = Content.Chunk.EOF; - _listener.onRequestEnd(getHttpChannel().getRequest()); + if (_listener != null) + _listener.onRequestEnd(getHttpChannel().getRequest()); return false; } @@ -1011,7 +1012,8 @@ public void badMessage(HttpException failure) if (LOG.isDebugEnabled()) LOG.debug("badMessage {} {}", HttpConnection.this, failure); - _listener.onRequestEnd(getHttpChannel().getRequest()); + if (_listener != null) + _listener.onRequestEnd(getHttpChannel().getRequest()); _failure = (Throwable)failure; _generator.setPersistent(false); @@ -1244,7 +1246,8 @@ public boolean is100ContinueExpected() Request request = _httpChannel.getRequest(); request.setAttribute(ComplianceViolation.Listener.class.getName(), _requestHandler._listener); - _requestHandler._listener.onRequestBegin(request); + if (_requestHandler._listener != null) + _requestHandler._listener.onRequestBegin(request); if (_complianceViolations != null && !_complianceViolations.isEmpty()) { diff --git a/jetty-core/jetty-server/src/test/resources/jetty-logging.properties b/jetty-core/jetty-server/src/test/resources/jetty-logging.properties index 553a8d6b3285..523e59d56a9d 100644 --- a/jetty-core/jetty-server/src/test/resources/jetty-logging.properties +++ b/jetty-core/jetty-server/src/test/resources/jetty-logging.properties @@ -1,4 +1,4 @@ -#org.eclipse.jetty.LEVEL=DEBUG +org.eclipse.jetty.LEVEL=DEBUG #org.eclipse.jetty.http.HttpParser.LEVEL=INFO #org.eclipse.jetty.util.LEVEL=DEBUG #org.eclipse.jetty.io.LEVEL=DEBUG From 1e525b8487ee2a032e37f1f419e93200721e53d3 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 17 Jan 2024 10:24:27 -0600 Subject: [PATCH 10/31] Do not use deprecated method in our own code --- .../java/org/eclipse/jetty/http/CookieCutter.java | 2 +- .../java/org/eclipse/jetty/http/HttpCompliance.java | 3 ++- .../org/eclipse/jetty/http/RFC6265CookieParser.java | 2 +- .../java/org/eclipse/jetty/http/UriCompliance.java | 11 +++-------- .../eclipse/jetty/http/RFC6265CookieParserTest.java | 4 ++-- .../eclipse/jetty/server/internal/HttpConnection.java | 2 +- .../src/test/resources/jetty-logging.properties | 2 +- .../java/org/eclipse/jetty/ee9/nested/Request.java | 2 +- 8 files changed, 12 insertions(+), 16 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCutter.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCutter.java index 3478ae767f26..b81ceef8258b 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCutter.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCutter.java @@ -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) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index 5e75d04189ac..236395a6fe68 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -215,7 +215,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; } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265CookieParser.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265CookieParser.java index 1cae2f74e673..e64856a70233 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265CookieParser.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/RFC6265CookieParser.java @@ -444,7 +444,7 @@ else if (_complianceMode.allows(ATTRIBUTES)) protected void reportComplianceViolation(CookieCompliance.Violation violation, String reason) { if (_complianceListener != null) - _complianceListener.onComplianceViolation(_complianceMode, violation, reason); + _complianceListener.onComplianceViolation(new ComplianceViolation.Event(_complianceMode, violation, reason)); } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java index 9704c8825a31..fffb1cd8bdc7 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/UriCompliance.java @@ -191,7 +191,8 @@ public static UriCompliance valueOf(String name) if (compliance.getName().equals(name)) return compliance; } - LOG.warn("Unknown UriCompliance mode {}", name); + if (name.indexOf(',') == -1) // skip warning if delimited, will be handled by .from() properly as a CUSTOM mode. + LOG.warn("Unknown UriCompliance mode {}", name); return null; } @@ -255,10 +256,6 @@ public static UriCompliance from(String spec) if (exclude) element = element.substring(1); - // Ignore removed name. TODO: remove in future release. - if (element.equals("NON_CANONICAL_AMBIGUOUS_PATHS")) - continue; - Violation section = Violation.valueOf(element); if (exclude) violations.remove(section); @@ -268,8 +265,6 @@ public static UriCompliance from(String spec) compliance = new UriCompliance("CUSTOM" + __custom.getAndIncrement(), violations); } - if (LOG.isDebugEnabled()) - LOG.debug("UriCompliance from {}->{}", spec, compliance); return compliance; } @@ -367,7 +362,7 @@ public static String checkUriCompliance(UriCompliance compliance, HttpURI uri, C if (uri.hasViolation(violation) && (compliance == null || !compliance.allows(violation))) return violation.getDescription(); else if (listener != null) - listener.onComplianceViolation(compliance, violation, uri.toString()); + listener.onComplianceViolation(new ComplianceViolation.Event(compliance, violation, uri.toString())); } return null; } diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java index a678f40b6a96..dc84b00087fb 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/RFC6265CookieParserTest.java @@ -444,9 +444,9 @@ private TestCookieParser(CookieCompliance compliance) } @Override - public void onComplianceViolation(ComplianceViolation.Mode mode, ComplianceViolation violation, String details) + public void onComplianceViolation(ComplianceViolation.Event event) { - violations.add((CookieCompliance.Violation)violation); + violations.add((CookieCompliance.Violation)event.violation()); } private List parseFields(String... fields) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index d415ab14953e..416521ef8dba 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -1199,7 +1199,7 @@ public Runnable headerComplete() if (httpCompliance.allows(MISMATCHED_AUTHORITY)) { if (_requestHandler._listener != null) - _requestHandler._listener.onComplianceViolation(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString()); + _requestHandler._listener.onComplianceViolation(new ComplianceViolation.Event(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString())); } else throw new BadMessageException("Authority!=Host"); diff --git a/jetty-core/jetty-server/src/test/resources/jetty-logging.properties b/jetty-core/jetty-server/src/test/resources/jetty-logging.properties index 523e59d56a9d..553a8d6b3285 100644 --- a/jetty-core/jetty-server/src/test/resources/jetty-logging.properties +++ b/jetty-core/jetty-server/src/test/resources/jetty-logging.properties @@ -1,4 +1,4 @@ -org.eclipse.jetty.LEVEL=DEBUG +#org.eclipse.jetty.LEVEL=DEBUG #org.eclipse.jetty.http.HttpParser.LEVEL=INFO #org.eclipse.jetty.util.LEVEL=DEBUG #org.eclipse.jetty.io.LEVEL=DEBUG diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index b862bb7aa0f6..6d05b6e8e97d 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -1993,7 +1993,7 @@ private void reportComplianceViolations() List nonComplianceWarnings = _multiParts.getNonComplianceWarnings(); for (MultiPartFormInputStream.NonCompliance nc : nonComplianceWarnings) { - complianceViolationListener.onComplianceViolation(nc.mode(), nc.violation(), nc.detail()); + complianceViolationListener.onComplianceViolation(new ComplianceViolation.Event(nc.mode(), nc.violation(), nc.detail())); } } From c5eda63fe4c1c10d3ca71e528a9425d5013bbae1 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 17 Jan 2024 11:45:03 -0600 Subject: [PATCH 11/31] Fix ComplianceViolations2616Test failures --- .../java/org/eclipse/jetty/http/ComplianceViolation.java | 3 ++- .../jetty/ee10/servlet/ComplianceViolations2616Test.java | 8 +++++--- .../jetty/ee9/servlet/ComplianceViolations2616Test.java | 8 +++++--- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java index 8e5aa4a2a24f..8df8e9753ae6 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java @@ -225,13 +225,14 @@ public Listener newComplianceViolationListener() public class CapturingListener implements Listener { + public static final String VIOLATIONS_ATTR_KEY = "org.eclipse.jetty.http.compliance.violations"; private List events = new ArrayList<>(); @Override public void onRequestBegin(Attributes request) { if (request != null) - request.setAttribute(ComplianceViolation.class.getPackageName() + ".complianceViolations", events); + request.setAttribute(VIOLATIONS_ATTR_KEY, events); } @Override diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java index 093c0b29cd31..0baad7847d3d 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java @@ -30,6 +30,7 @@ import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.HttpConnectionFactory; @@ -59,14 +60,14 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha { if (request instanceof HttpServletRequest) { - List violations = (List)request.getAttribute("org.eclipse.jetty.http.compliance.violations"); + List violations = (List)request.getAttribute("org.eclipse.jetty.http.compliance.violations"); if (violations != null) { HttpServletResponse httpResponse = (HttpServletResponse)response; int i = 0; - for (String violation : violations) + for (ComplianceViolation.Event event : violations) { - httpResponse.setHeader("X-Http-Violation-" + (i++), violation); + httpResponse.setHeader("X-Http-Violation-" + (i++), event.toString()); } } } @@ -108,6 +109,7 @@ public static void startServer() throws Exception HttpConnectionFactory httpConnectionFactory = new HttpConnectionFactory(config); httpConnectionFactory.setRecordHttpComplianceViolations(true); connector = new LocalConnector(server, null, null, null, -1, httpConnectionFactory); + connector.addBean(new ComplianceViolation.CapturingListenerFactory()); ServletContextHandler context = new ServletContextHandler(); context.setContextPath("/"); diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java index c5b484693c9e..ceb7e5f18621 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java @@ -30,6 +30,7 @@ import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.server.HttpConfiguration; import org.eclipse.jetty.server.HttpConnectionFactory; @@ -59,14 +60,14 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha { if (request instanceof HttpServletRequest) { - List violations = (List)request.getAttribute("org.eclipse.jetty.http.compliance.violations"); + List violations = (List)request.getAttribute("org.eclipse.jetty.http.compliance.violations"); if (violations != null) { HttpServletResponse httpResponse = (HttpServletResponse)response; int i = 0; - for (String violation : violations) + for (ComplianceViolation.Event event : violations) { - httpResponse.setHeader("X-Http-Violation-" + (i++), violation); + httpResponse.setHeader("X-Http-Violation-" + (i++), event.toString()); } } } @@ -108,6 +109,7 @@ public static void startServer() throws Exception HttpConnectionFactory httpConnectionFactory = new HttpConnectionFactory(config); httpConnectionFactory.setRecordHttpComplianceViolations(true); connector = new LocalConnector(server, null, null, null, -1, httpConnectionFactory); + connector.addBean(new ComplianceViolation.CapturingListenerFactory()); ServletContextHandler context = new ServletContextHandler(); context.setContextPath("/"); From 31829821b1dc4b0877d8f8e69bb428ba3d369a85 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 17 Jan 2024 13:05:12 -0600 Subject: [PATCH 12/31] Fixing ee9/ee8 RequestTest --- .../org/eclipse/jetty/http/HttpCompliance.java | 8 +++++++- .../java/org/eclipse/jetty/ee9/nested/Request.java | 6 ++---- .../org/eclipse/jetty/ee9/nested/RequestTest.java | 14 ++++++++------ 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index 236395a6fe68..fefcc673ae99 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -161,8 +161,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.
+ * (Note: new ATTR captures all Compliance violations, not just HTTP.
+ * Make sure you have {@code HttpConnectionFactory.setRecordHttpComplianceViolations(true)}.
+ * Also make sure that a {@link ComplianceViolation.CapturingListenerFactory} 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 RFC 7230 diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index 6d05b6e8e97d..a3c279dec471 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -1987,13 +1987,11 @@ else if (getCharacterEncoding() != null) private void reportComplianceViolations() { ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)getAttribute(ComplianceViolation.Listener.class.getName()); - if (complianceViolationListener == null) - return; // no violation reporting active - List nonComplianceWarnings = _multiParts.getNonComplianceWarnings(); for (MultiPartFormInputStream.NonCompliance nc : nonComplianceWarnings) { - complianceViolationListener.onComplianceViolation(new ComplianceViolation.Event(nc.mode(), nc.violation(), nc.detail())); + if (complianceViolationListener != null) + complianceViolationListener.onComplianceViolation(new ComplianceViolation.Event(nc.mode(), nc.violation(), nc.detail())); } } diff --git a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java index 6d3009b9ff48..bfadcb2d8008 100644 --- a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java +++ b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java @@ -52,8 +52,8 @@ import jakarta.servlet.http.Part; import jakarta.servlet.http.PushBuilder; import org.eclipse.jetty.http.BadMessageException; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.CookieCompliance; -import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpException; import org.eclipse.jetty.http.HttpField; @@ -130,6 +130,7 @@ public void init() throws Exception _server = new Server(); _context = new ContextHandler(_server); HttpConnectionFactory http = new HttpConnectionFactory(); + http.setRecordHttpComplianceViolations(true); http.setInputBufferSize(1024); http.getHttpConfiguration().setRequestHeaderSize(512); http.getHttpConfiguration().setResponseHeaderSize(512); @@ -137,6 +138,7 @@ public void init() throws Exception http.getHttpConfiguration().addCustomizer(new ForwardedRequestCustomizer()); http.getHttpConfiguration().setRequestCookieCompliance(CookieCompliance.RFC6265_LEGACY); _connector = new LocalConnector(_server, http); + _connector.addBean(new ComplianceViolation.CapturingListenerFactory()); _server.addConnector(_connector); _connector.setIdleTimeout(500); _handler = new RequestHandler(); @@ -494,7 +496,7 @@ public void testMultiPart(WorkDir workDir) throws Exception endPoint.addInput(request); String response = endPoint.getResponse(); assertThat(response, startsWith("HTTP/1.1 200")); - assertThat(response, containsString("Violation: TRANSFER_ENCODING")); + assertThat(response, containsString("Violation: CONTENT_TRANSFER_ENCODING")); // We know the previous request has completed if another request can be processed on the same connection. String cleanupRequest = "GET /foo/cleanup HTTP/1.1\r\n" + @@ -2272,12 +2274,12 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques assertNotNull(foo); assertTrue(foo.getSize() > 0); response.setStatus(200); - List violations = (List)request.getAttribute(HttpCompliance.VIOLATIONS_ATTR); - if (violations != null) + List violationEvents = (List)request.getAttribute(ComplianceViolation.CapturingListener.VIOLATIONS_ATTR_KEY); + if (violationEvents != null) { - for (String v : violations) + for (ComplianceViolation.Event event : violationEvents) { - response.addHeader("Violation", v); + response.addHeader("Violation", event.violation().toString()); } } From f7e3441a6501d55530e6f68207b4523c34ab8912 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Wed, 17 Jan 2024 15:36:31 -0600 Subject: [PATCH 13/31] Updates from review --- .../jetty/http/ComplianceViolation.java | 50 +++++++++++++++---- 1 file changed, 40 insertions(+), 10 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java index 8df8e9753ae6..4e75121dd826 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java @@ -15,6 +15,7 @@ import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.Set; import org.eclipse.jetty.util.Attributes; @@ -152,7 +153,43 @@ class ListenerCollection implements Listener */ public ListenerCollection(List userListeners) { - this.userListeners = userListeners; + Objects.requireNonNull(userListeners); + if (userListeners.isEmpty()) + throw new IllegalStateException("Listener list is empty"); + this.userListeners = userListeners; + } + + @Override + public void onRequestBegin(Attributes request) + { + for (ComplianceViolation.Listener listener : userListeners) + { + try + { + listener.onRequestBegin(request); + } + catch (Exception e) + { + LOG.warn("Unable to notify {}.onRequestBegin({})", listener.getClass().getName(), request, e); + } + } + + } + + @Override + public void onRequestEnd(Attributes request) + { + for (ComplianceViolation.Listener listener : userListeners) + { + try + { + listener.onRequestEnd(request); + } + catch (Exception e) + { + LOG.warn("Unable to notify {}.onRequestEnd({})", listener.getClass().getName(), request, e); + } + } } /** @@ -175,15 +212,7 @@ public T getUserListener(Class clazz) 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) + for (Listener listener : userListeners) { try { @@ -238,6 +267,7 @@ public void onRequestBegin(Attributes request) @Override public void onRequestEnd(Attributes request) { + events = new ArrayList<>(); } @Override From af2ef3c7285d60db6eabed680470316e8fc56297 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 18 Jan 2024 11:43:15 +1100 Subject: [PATCH 14/31] WIP on cleanup of ComplianceViolation.Listener instantiation --- .../jetty/http/ComplianceViolation.java | 88 ++++++++++++++----- .../java/org/eclipse/jetty/server/Server.java | 58 ++++++------ .../jetty/server/internal/HttpConnection.java | 30 ++++--- 3 files changed, 112 insertions(+), 64 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java index 8df8e9753ae6..1a2b551d456e 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java @@ -89,13 +89,22 @@ 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() {}; + + default Listener initialize() + { + return this; + } + /** * A new Request has begun. * @@ -148,6 +157,7 @@ class ListenerCollection implements Listener /** * Construct a new ComplianceViolations that will notify user listeners. + * * @param userListeners the user listeners to notify, null or empty is allowed. */ public ListenerCollection(List userListeners) @@ -155,11 +165,37 @@ public ListenerCollection(List userListeners) this.userListeners = userListeners; } + @Override + public Listener initialize() + { + List cloned = null; + for (ComplianceViolation.Listener listener : userListeners) + { + Listener initialized = listener.initialize(); + if (initialized != listener) + { + cloned = new ArrayList<>(userListeners.size()); + for (ComplianceViolation.Listener l : userListeners) + { + if (l == listener) + break; + cloned.add(l); + } + } + if (cloned != null) + cloned.add(initialized); + } + if (cloned == null) + return this; + return new ListenerCollection(cloned); + } + /** * Get a specific ComplianceViolation.Listener from collected user listeners + * * @param clazz the class to look for - * @return the instance of the class in the user listeners * @param the type of class + * @return the instance of the class in the user listeners */ public T getUserListener(Class clazz) { @@ -197,12 +233,7 @@ private void notifyUserListeners(ComplianceViolation.Event event) } } - interface ListenerFactory - { - Listener newComplianceViolationListener(); - } - - public class LoggingListener implements Listener + class LoggingListener implements Listener { private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolation.class); @@ -214,36 +245,53 @@ public void onComplianceViolation(Event event) } } - public class CapturingListenerFactory implements ListenerFactory + class CapturingListener implements Listener { + public static final String VIOLATIONS_ATTR_KEY = "org.eclipse.jetty.http.compliance.violations"; + @Override - public Listener newComplianceViolationListener() + public Listener initialize() { - return new CapturingListener(); - } - } + return new Listener() + { + private List events = new ArrayList<>(); - public class CapturingListener implements Listener - { - public static final String VIOLATIONS_ATTR_KEY = "org.eclipse.jetty.http.compliance.violations"; - private List events = new ArrayList<>(); + @Override + public void onRequestBegin(Attributes request) + { + if (request != null) + request.setAttribute(VIOLATIONS_ATTR_KEY, events); + } + + @Override + public void onRequestEnd(Attributes request) + { + } + + @Override + public void onComplianceViolation(Event event) + { + events.add(event); + } + }; + } @Override public void onRequestBegin(Attributes request) { - if (request != null) - request.setAttribute(VIOLATIONS_ATTR_KEY, events); + throw new IllegalStateException("!initialized"); } @Override public void onRequestEnd(Attributes request) { + throw new IllegalStateException("!initialized"); } @Override public void onComplianceViolation(Event event) { - events.add(event); + throw new IllegalStateException("!initialized"); } } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 49368f513f9f..13a7763664a9 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -23,9 +23,11 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.TreeMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; @@ -85,6 +87,7 @@ public class Server extends Handler.Wrapper implements Attributes private final Context _serverContext = new ServerContext(); private final AutoLock _dateLock = new AutoLock(); private final MimeTypes.Mutable _mimeTypes = new MimeTypes.Mutable(); + private final Map _complianceViolationListener = new TreeMap<>(); private String _serverInfo = __serverInfo; private boolean _stopAtShutdown; private boolean _dumpAfterStart; @@ -159,38 +162,33 @@ public Server(@Name("threadPool") ThreadPool threadPool, @Name("scheduler") Sche * @return the ComplianceViolation.Listener implementation, or null if {@link HttpConnectionFactory#isRecordHttpComplianceViolations()} is false, * or there are no ComplianceViolation.Listener implementations registered. */ - public static ComplianceViolation.Listener newComplianceViolationListener(Connector connector) + public static ComplianceViolation.Listener getComplianceViolationListener(Connector connector) { - HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class); - if (httpConnectionFactory == null) - return null; - - // Is this connector recording compliance violations? - if (!httpConnectionFactory.isRecordHttpComplianceViolations()) - return null; - - // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled - // This also means that any user provided ComplianceViolation.ListenerFactory beans will only be - // used when the configuration on the HttpConnectionFactory allows then to be used. - - // Look for optional user provided ComplianceViolation.Listener and ComplianceViolation.ListenerFactory beans - List userListeners = new ArrayList<>(); - for (org.eclipse.jetty.util.component.Container container: List.of(connector, connector.getServer())) + return connector.getServer()._complianceViolationListener.computeIfAbsent(connector, c -> { - for (ComplianceViolation.Listener listener: container.getBeans(ComplianceViolation.Listener.class)) - userListeners.add(listener); - for (ComplianceViolation.ListenerFactory listenerFactory: container.getBeans(ComplianceViolation.ListenerFactory.class)) - userListeners.add(listenerFactory.newComplianceViolationListener()); - } - - // No listeners? then we are done - if (userListeners.isEmpty()) - return null; - // Only 1 listener, just return it. - if (userListeners.size() == 1) - return userListeners.get(0); - // More than 1, establish ComplianceViolation.ListenerCollection for user listeners - return new ComplianceViolation.ListenerCollection(userListeners); + Server server = connector.getServer(); + + // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled + // This also means that any user provided ComplianceViolation.ListenerFactory beans will only be + // used when the configuration on the HttpConnectionFactory allows then to be used. + HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class); + if (httpConnectionFactory == null || !httpConnectionFactory.isRecordHttpComplianceViolations()) + return ComplianceViolation.Listener.NOOP; + + // Look for optional user provided ComplianceViolation.Listener and ComplianceViolation.ListenerFactory beans + List userListeners = new ArrayList<>(); + userListeners.addAll(connector.getBeans(ComplianceViolation.Listener.class)); + userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); + + // No listeners? then we are done + if (userListeners.isEmpty()) + return ComplianceViolation.Listener.NOOP; + // Only 1 listener, just return it. + if (userListeners.size() == 1) + return userListeners.get(0); + // More than 1, establish ComplianceViolation.ListenerCollection for user listeners + return new ComplianceViolation.ListenerCollection(userListeners); + }); } public Handler getDefaultHandler() diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 416521ef8dba..95ac1e291b2d 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -103,6 +103,8 @@ public class HttpConnection extends AbstractMetaDataConnection implements Runnab private final LongAdder bytesOut = new LongAdder(); private final AtomicBoolean _handling = new AtomicBoolean(false); private final HttpFields.Mutable _headerBuilder = HttpFields.build(); + private final ComplianceViolation.Listener _complianceViolationListener; + private ComplianceViolation.Listener _complianceViolationInitializedListener; private volatile RetainableByteBuffer _retainableByteBuffer; private HttpFields.Mutable _trailers; private Runnable _onRequest; @@ -148,6 +150,7 @@ public HttpConnection(HttpConfiguration configuration, Connector connector, EndP _generator = newHttpGenerator(); _httpChannel = newHttpChannel(connector.getServer(), configuration); _requestHandler = newRequestHandler(); + _complianceViolationListener = Server.getComplianceViolationListener(connector); _parser = newHttpParser(configuration.getHttpCompliance()); if (LOG.isDebugEnabled()) LOG.debug("New HTTP Connection {}", this); @@ -563,6 +566,10 @@ private boolean parseRequestBuffer() if (_parser.isTerminated()) throw new RuntimeIOException("Parser is terminated"); + + if (_complianceViolationInitializedListener == null) + _complianceViolationInitializedListener = _complianceViolationListener.initialize(); + boolean handle = _parser.parseNext(_retainableByteBuffer == null ? BufferUtil.EMPTY_BUFFER : _retainableByteBuffer.getByteBuffer()); if (LOG.isDebugEnabled()) @@ -922,11 +929,9 @@ public String toString() protected class RequestHandler implements HttpParser.RequestHandler { private Throwable _failure; - private ComplianceViolation.Listener _listener; public RequestHandler(Connector connector) { - _listener = Server.newComplianceViolationListener(connector); } @Override @@ -978,8 +983,7 @@ public boolean contentComplete() @Override public void onViolation(ComplianceViolation.Event event) { - if (_listener != null) - _listener.onComplianceViolation(event); + _complianceViolationInitializedListener.onComplianceViolation(event); } @Override @@ -1001,8 +1005,7 @@ public boolean messageComplete() else stream._chunk = Content.Chunk.EOF; - if (_listener != null) - _listener.onRequestEnd(getHttpChannel().getRequest()); + _complianceViolationInitializedListener.onRequestBegin(getHttpChannel().getRequest()); return false; } @@ -1012,8 +1015,7 @@ public void badMessage(HttpException failure) if (LOG.isDebugEnabled()) LOG.debug("badMessage {} {}", HttpConnection.this, failure); - if (_listener != null) - _listener.onRequestEnd(getHttpChannel().getRequest()); + _complianceViolationInitializedListener.onRequestEnd(getHttpChannel().getRequest()); _failure = (Throwable)failure; _generator.setPersistent(false); @@ -1183,7 +1185,7 @@ public Runnable headerComplete() if (_uri.hasViolations()) { compliance = getHttpConfiguration().getUriCompliance(); - String badMessage = UriCompliance.checkUriCompliance(compliance, _uri, _requestHandler._listener); + String badMessage = UriCompliance.checkUriCompliance(compliance, _uri, _complianceViolationInitializedListener); if (badMessage != null) throw new BadMessageException(badMessage); } @@ -1198,8 +1200,7 @@ public Runnable headerComplete() HttpCompliance httpCompliance = getHttpConfiguration().getHttpCompliance(); if (httpCompliance.allows(MISMATCHED_AUTHORITY)) { - if (_requestHandler._listener != null) - _requestHandler._listener.onComplianceViolation(new ComplianceViolation.Event(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString())); + _complianceViolationInitializedListener.onComplianceViolation(new ComplianceViolation.Event(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString())); } else throw new BadMessageException("Authority!=Host"); @@ -1245,9 +1246,9 @@ public boolean is100ContinueExpected() ++_requests; Request request = _httpChannel.getRequest(); - request.setAttribute(ComplianceViolation.Listener.class.getName(), _requestHandler._listener); - if (_requestHandler._listener != null) - _requestHandler._listener.onRequestBegin(request); + // TODO why is this still needed? + request.setAttribute(ComplianceViolation.Listener.class.getName(), _complianceViolationInitializedListener); + _complianceViolationInitializedListener.onRequestBegin(request); if (_complianceViolations != null && !_complianceViolations.isEmpty()) { @@ -1525,6 +1526,7 @@ public void succeeded() } _httpChannel.recycle(); + _complianceViolationInitializedListener = null; if (_expects100Continue) { From 9cc80531c82866db19db672bcd6b99d58e179639 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 18 Jan 2024 16:15:12 +1100 Subject: [PATCH 15/31] WIP on cleanup of ComplianceViolation.Listener instantiation removed attribute access in favour of a HttpChannelState field --- .../jetty/http/ComplianceViolation.java | 46 ++++----------- .../eclipse/jetty/http/HttpCompliance.java | 2 +- .../server/internal/HttpStreamOverHTTP2.java | 7 +-- .../server/internal/HttpStreamOverHTTP3.java | 7 +-- .../org/eclipse/jetty/server/Components.java | 3 + .../org/eclipse/jetty/server/HttpChannel.java | 6 ++ .../org/eclipse/jetty/server/Request.java | 4 +- .../org/eclipse/jetty/server/Response.java | 7 +-- .../java/org/eclipse/jetty/server/Server.java | 56 ++++++++++--------- .../server/internal/HttpChannelState.java | 18 +++++- .../jetty/server/internal/HttpConnection.java | 21 ++----- .../jetty/server/ServerHttpCookieTest.java | 12 +--- .../servlet/ComplianceViolations2616Test.java | 2 +- .../eclipse/jetty/ee9/nested/Dispatcher.java | 4 +- .../org/eclipse/jetty/ee9/nested/Request.java | 9 +-- .../eclipse/jetty/ee9/nested/RequestTest.java | 2 +- .../servlet/ComplianceViolations2616Test.java | 2 +- 17 files changed, 89 insertions(+), 119 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java index 1a2b551d456e..a42572ca566c 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java @@ -81,7 +81,7 @@ interface Mode Set getAllowed(); } - public static record Event(ComplianceViolation.Mode mode, ComplianceViolation violation, String details) + record Event(ComplianceViolation.Mode mode, ComplianceViolation violation, String details) { @Override public String toString() @@ -91,8 +91,6 @@ public String toString() } } - ; - /** * A listener that can be notified of violations. */ @@ -168,43 +166,26 @@ public ListenerCollection(List userListeners) @Override public Listener initialize() { - List cloned = null; + List initialized = null; for (ComplianceViolation.Listener listener : userListeners) { - Listener initialized = listener.initialize(); - if (initialized != listener) + Listener listening = listener.initialize(); + if (listening != listener) { - cloned = new ArrayList<>(userListeners.size()); + initialized = new ArrayList<>(userListeners.size()); for (ComplianceViolation.Listener l : userListeners) { if (l == listener) break; - cloned.add(l); + initialized.add(l); } } - if (cloned != null) - cloned.add(initialized); + if (initialized != null) + initialized.add(listening); } - if (cloned == null) + if (initialized == null) return this; - return new ListenerCollection(cloned); - } - - /** - * Get a specific ComplianceViolation.Listener from collected user listeners - * - * @param clazz the class to look for - * @param the type of class - * @return the instance of the class in the user listeners - */ - public T getUserListener(Class clazz) - { - for (ComplianceViolation.Listener listener : userListeners) - { - if (clazz.isInstance(listener)) - return clazz.cast(listener); - } - return null; + return new ListenerCollection(initialized); } @Override @@ -254,7 +235,7 @@ public Listener initialize() { return new Listener() { - private List events = new ArrayList<>(); + private final List events = new ArrayList<>(); @Override public void onRequestBegin(Attributes request) @@ -263,11 +244,6 @@ public void onRequestBegin(Attributes request) request.setAttribute(VIOLATIONS_ATTR_KEY, events); } - @Override - public void onRequestEnd(Attributes request) - { - } - @Override public void onComplianceViolation(Event event) { diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index fefcc673ae99..68218c383279 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -164,7 +164,7 @@ public String getDescription() * @deprecated use {@link ComplianceViolation.CapturingListener#VIOLATIONS_ATTR_KEY} instead.
* (Note: new ATTR captures all Compliance violations, not just HTTP.
* Make sure you have {@code HttpConnectionFactory.setRecordHttpComplianceViolations(true)}.
- * Also make sure that a {@link ComplianceViolation.CapturingListenerFactory} has been added as a bean to + * 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.) */ @Deprecated(since = "12.0.5", forRemoval = true) diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java index 659640e0586a..70eefb6b42a6 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java @@ -85,13 +85,10 @@ public Runnable onRequest(HeadersFrame frame) { _requestMetaData = (MetaData.Request)frame.getMetaData(); + ComplianceViolation.Listener listener = Server.getComplianceViolationListener(_httpChannel.getConnectionMetaData().getConnector()).initialize(); Runnable handler = _httpChannel.onRequest(_requestMetaData); - - ComplianceViolation.Listener listener = Server.newComplianceViolationListener(_httpChannel.getConnectionMetaData().getConnector()); Request request = _httpChannel.getRequest(); - request.setAttribute(ComplianceViolation.Listener.class.getName(), listener); - if (listener != null) - listener.onRequestBegin(request); + listener.onRequestBegin(request); // Note UriCompliance is done by HandlerInvoker // TODO: perform HttpCompliance violation checks? diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index 5326fe0a55c8..bfd638ef1216 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -75,13 +75,10 @@ public Runnable onRequest(HeadersFrame frame) { requestMetaData = (MetaData.Request)frame.getMetaData(); + ComplianceViolation.Listener listener = Server.getComplianceViolationListener(this.httpChannel.getConnectionMetaData().getConnector()).initialize(); Runnable handler = httpChannel.onRequest(requestMetaData); - - ComplianceViolation.Listener listener = Server.newComplianceViolationListener(this.httpChannel.getConnectionMetaData().getConnector()); Request request = this.httpChannel.getRequest(); - request.setAttribute(ComplianceViolation.Listener.class.getName(), listener); - if (listener != null) - listener.onRequestBegin(request); + listener.onRequestBegin(request); // Note UriCompliance is done by HandlerInvoker // TODO: perform HttpCompliance violation checks? diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java index b6e32ea937fd..1be2863c96a5 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java @@ -13,6 +13,7 @@ package org.eclipse.jetty.server; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.thread.Scheduler; @@ -43,4 +44,6 @@ public interface Components * @return A Map, which may be an empty map that discards all items. */ Attributes getCache(); + + ComplianceViolation.Listener getComplianceViolationListener(); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 382fc544bf89..8d541209bcfd 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -36,6 +36,12 @@ public interface HttpChannel extends Invocable */ ConnectionMetaData getConnectionMetaData(); + /** + * @return the {@link Components} associated with this {@code HttpChannel}. + * @see Request#getComponents() + */ + Components getComponents(); + /** * Set the {@link HttpStream} to associate to this channel.. * @param httpStream the {@link HttpStream} to associate to this channel. diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 724af9850c4e..327cc75c4c75 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -34,7 +34,6 @@ import java.util.function.Function; import java.util.function.Predicate; -import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.CookieCache; import org.eclipse.jetty.http.HttpCookie; import org.eclipse.jetty.http.HttpFields; @@ -590,8 +589,7 @@ static List getCookies(Request request) CookieCache cookieCache = (CookieCache)request.getComponents().getCache().getAttribute(CACHE_ATTRIBUTE); if (cookieCache == null) { - ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)request.getAttribute(ComplianceViolation.Listener.class.getName()); - cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance(), complianceViolationListener); + cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance(), request.getComponents().getComplianceViolationListener()); request.getComponents().getCache().setAttribute(CACHE_ATTRIBUTE, cookieCache); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index 3bca1397e00f..0966c368d3ce 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -20,7 +20,6 @@ import java.util.concurrent.TimeoutException; import java.util.function.Supplier; -import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.ComplianceViolationException; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.HttpCookie; @@ -386,8 +385,7 @@ static void addCookie(Response response, HttpCookie cookie) } catch (ComplianceViolationException e) { - ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)request.getAttribute(ComplianceViolation.Listener.class.getName()); - complianceViolationListener.onComplianceViolation(e.getEvent()); + request.getComponents().getComplianceViolationListener().onComplianceViolation(e.getEvent()); throw e; } @@ -421,8 +419,7 @@ static void putCookie(Response response, HttpCookie cookie) } catch (ComplianceViolationException e) { - ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)request.getAttribute(ComplianceViolation.Listener.class.getName()); - complianceViolationListener.onComplianceViolation(e.getEvent()); + request.getComponents().getComplianceViolationListener().onComplianceViolation(e.getEvent()); throw e; } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 13a7763664a9..7495bf948a6d 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -22,12 +22,12 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; +import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; -import java.util.TreeMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; @@ -87,7 +87,7 @@ public class Server extends Handler.Wrapper implements Attributes private final Context _serverContext = new ServerContext(); private final AutoLock _dateLock = new AutoLock(); private final MimeTypes.Mutable _mimeTypes = new MimeTypes.Mutable(); - private final Map _complianceViolationListener = new TreeMap<>(); + private final Map _complianceViolationListener = new HashMap<>(); private String _serverInfo = __serverInfo; private boolean _stopAtShutdown; private boolean _dumpAfterStart; @@ -164,31 +164,8 @@ public Server(@Name("threadPool") ThreadPool threadPool, @Name("scheduler") Sche */ public static ComplianceViolation.Listener getComplianceViolationListener(Connector connector) { - return connector.getServer()._complianceViolationListener.computeIfAbsent(connector, c -> - { - Server server = connector.getServer(); - - // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled - // This also means that any user provided ComplianceViolation.ListenerFactory beans will only be - // used when the configuration on the HttpConnectionFactory allows then to be used. - HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class); - if (httpConnectionFactory == null || !httpConnectionFactory.isRecordHttpComplianceViolations()) - return ComplianceViolation.Listener.NOOP; - - // Look for optional user provided ComplianceViolation.Listener and ComplianceViolation.ListenerFactory beans - List userListeners = new ArrayList<>(); - userListeners.addAll(connector.getBeans(ComplianceViolation.Listener.class)); - userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); - - // No listeners? then we are done - if (userListeners.isEmpty()) - return ComplianceViolation.Listener.NOOP; - // Only 1 listener, just return it. - if (userListeners.size() == 1) - return userListeners.get(0); - // More than 1, establish ComplianceViolation.ListenerCollection for user listeners - return new ComplianceViolation.ListenerCollection(userListeners); - }); + ComplianceViolation.Listener listener = connector.getServer()._complianceViolationListener.get(connector); + return listener == null ? ComplianceViolation.Listener.NOOP : listener; } public Handler getDefaultHandler() @@ -617,6 +594,31 @@ protected void doStart() throws Exception { try { + _complianceViolationListener.computeIfAbsent(connector, c -> + { + Server server = connector.getServer(); + + // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled + // This also means that any user provided ComplianceViolation.ListenerFactory beans will only be + // used when the configuration on the HttpConnectionFactory allows then to be used. + HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class); + if (httpConnectionFactory == null || !httpConnectionFactory.isRecordHttpComplianceViolations()) + return ComplianceViolation.Listener.NOOP; + + // Look for optional user provided ComplianceViolation.Listener and ComplianceViolation.ListenerFactory beans + List userListeners = new ArrayList<>(); + userListeners.addAll(connector.getBeans(ComplianceViolation.Listener.class)); + userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); + + // No listeners? then we are done + if (userListeners.isEmpty()) + return ComplianceViolation.Listener.NOOP; + // Only 1 listener, just return it. + if (userListeners.size() == 1) + return userListeners.get(0); + // More than 1, establish ComplianceViolation.ListenerCollection for user listeners + return new ComplianceViolation.ListenerCollection(userListeners); + }); connector.start(); } catch (Throwable e) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index f1016530bf3f..f8c07355811d 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -119,6 +119,7 @@ private enum StreamSendState private Throwable _callbackFailure; private Attributes _cache; private boolean _expects100Continue; + private ComplianceViolation.Listener _complianceViolationListener; public HttpChannelState(ConnectionMetaData connectionMetaData) { @@ -126,6 +127,19 @@ public HttpChannelState(ConnectionMetaData connectionMetaData) // The SerializedInvoker is used to prevent infinite recursion of callbacks calling methods calling callbacks etc. _readInvoker = new HttpChannelSerializedInvoker(); _writeInvoker = new HttpChannelSerializedInvoker(); + _complianceViolationListener = Server.getComplianceViolationListener(_connectionMetaData.getConnector()).initialize(); + } + + @Override + public Components getComponents() + { + return this; + } + + @Override + public ComplianceViolation.Listener getComplianceViolationListener() + { + return _complianceViolationListener; } @Override @@ -158,6 +172,7 @@ public void recycle() _onFailure = null; _callbackFailure = null; _expects100Continue = false; + _complianceViolationListener = _complianceViolationListener.initialize(); } } @@ -573,8 +588,7 @@ public void run() HttpURI uri = request.getHttpURI(); if (uri.hasViolations()) { - ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)request.getAttribute(ComplianceViolation.Listener.class.getName()); - String badMessage = UriCompliance.checkUriCompliance(getConnectionMetaData().getHttpConfiguration().getUriCompliance(), uri, complianceViolationListener); + String badMessage = UriCompliance.checkUriCompliance(getConnectionMetaData().getHttpConfiguration().getUriCompliance(), uri, request.getComponents().getComplianceViolationListener()); if (badMessage != null) throw new BadMessageException(badMessage); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 95ac1e291b2d..539a41c2b694 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -103,8 +103,6 @@ public class HttpConnection extends AbstractMetaDataConnection implements Runnab private final LongAdder bytesOut = new LongAdder(); private final AtomicBoolean _handling = new AtomicBoolean(false); private final HttpFields.Mutable _headerBuilder = HttpFields.build(); - private final ComplianceViolation.Listener _complianceViolationListener; - private ComplianceViolation.Listener _complianceViolationInitializedListener; private volatile RetainableByteBuffer _retainableByteBuffer; private HttpFields.Mutable _trailers; private Runnable _onRequest; @@ -150,7 +148,6 @@ public HttpConnection(HttpConfiguration configuration, Connector connector, EndP _generator = newHttpGenerator(); _httpChannel = newHttpChannel(connector.getServer(), configuration); _requestHandler = newRequestHandler(); - _complianceViolationListener = Server.getComplianceViolationListener(connector); _parser = newHttpParser(configuration.getHttpCompliance()); if (LOG.isDebugEnabled()) LOG.debug("New HTTP Connection {}", this); @@ -567,9 +564,6 @@ private boolean parseRequestBuffer() if (_parser.isTerminated()) throw new RuntimeIOException("Parser is terminated"); - if (_complianceViolationInitializedListener == null) - _complianceViolationInitializedListener = _complianceViolationListener.initialize(); - boolean handle = _parser.parseNext(_retainableByteBuffer == null ? BufferUtil.EMPTY_BUFFER : _retainableByteBuffer.getByteBuffer()); if (LOG.isDebugEnabled()) @@ -983,7 +977,7 @@ public boolean contentComplete() @Override public void onViolation(ComplianceViolation.Event event) { - _complianceViolationInitializedListener.onComplianceViolation(event); + getHttpChannel().getComponents().getComplianceViolationListener().onComplianceViolation(event); } @Override @@ -1005,7 +999,7 @@ public boolean messageComplete() else stream._chunk = Content.Chunk.EOF; - _complianceViolationInitializedListener.onRequestBegin(getHttpChannel().getRequest()); + getHttpChannel().getComponents().getComplianceViolationListener().onRequestBegin(getHttpChannel().getRequest()); return false; } @@ -1015,7 +1009,7 @@ public void badMessage(HttpException failure) if (LOG.isDebugEnabled()) LOG.debug("badMessage {} {}", HttpConnection.this, failure); - _complianceViolationInitializedListener.onRequestEnd(getHttpChannel().getRequest()); + getHttpChannel().getComponents().getComplianceViolationListener().onRequestEnd(getHttpChannel().getRequest()); _failure = (Throwable)failure; _generator.setPersistent(false); @@ -1185,7 +1179,7 @@ public Runnable headerComplete() if (_uri.hasViolations()) { compliance = getHttpConfiguration().getUriCompliance(); - String badMessage = UriCompliance.checkUriCompliance(compliance, _uri, _complianceViolationInitializedListener); + String badMessage = UriCompliance.checkUriCompliance(compliance, _uri, getHttpChannel().getComponents().getComplianceViolationListener()); if (badMessage != null) throw new BadMessageException(badMessage); } @@ -1200,7 +1194,7 @@ public Runnable headerComplete() HttpCompliance httpCompliance = getHttpConfiguration().getHttpCompliance(); if (httpCompliance.allows(MISMATCHED_AUTHORITY)) { - _complianceViolationInitializedListener.onComplianceViolation(new ComplianceViolation.Event(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString())); + getHttpChannel().getComponents().getComplianceViolationListener().onComplianceViolation(new ComplianceViolation.Event(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString())); } else throw new BadMessageException("Authority!=Host"); @@ -1246,9 +1240,7 @@ public boolean is100ContinueExpected() ++_requests; Request request = _httpChannel.getRequest(); - // TODO why is this still needed? - request.setAttribute(ComplianceViolation.Listener.class.getName(), _complianceViolationInitializedListener); - _complianceViolationInitializedListener.onRequestBegin(request); + getHttpChannel().getComponents().getComplianceViolationListener().onRequestBegin(request); if (_complianceViolations != null && !_complianceViolations.isEmpty()) { @@ -1526,7 +1518,6 @@ public void succeeded() } _httpChannel.recycle(); - _complianceViolationInitializedListener = null; if (_expects100Continue) { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java index a1a163e9bebb..18e84dab10f9 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java @@ -64,15 +64,9 @@ public boolean handle(Request request, Response response, Callback callback) thr Fields.Field setCookie = parameters.get("SetCookie"); if (setCookie != null) { - ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)request.getAttribute(ComplianceViolation.Listener.class.getName()); - CookieParser parser = CookieParser.newParser(new CookieParser.Handler() - { - @Override - public void addCookie(String name, String value, int version, String domain, String path, String comment) - { - Response.addCookie(response, HttpCookie.build(name, value, version).domain(domain).path(path).comment(comment).build()); - } - }, RFC2965, complianceViolationListener); + ComplianceViolation.Listener complianceViolationListener = request.getComponents().getComplianceViolationListener(); + CookieParser parser = CookieParser.newParser((name, value, version, domain, path, comment) -> + Response.addCookie(response, HttpCookie.build(name, value, version).domain(domain).path(path).comment(comment).build()), RFC2965, complianceViolationListener); parser.parseField(setCookie.getValue()); } diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java index 0baad7847d3d..9a898b7e0911 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java @@ -109,7 +109,7 @@ public static void startServer() throws Exception HttpConnectionFactory httpConnectionFactory = new HttpConnectionFactory(config); httpConnectionFactory.setRecordHttpComplianceViolations(true); connector = new LocalConnector(server, null, null, null, -1, httpConnectionFactory); - connector.addBean(new ComplianceViolation.CapturingListenerFactory()); + connector.addBean(new ComplianceViolation.CapturingListener()); ServletContextHandler context = new ServletContextHandler(); context.setContextPath("/"); diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java index d289bc1f0878..916d94c19103 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java @@ -26,7 +26,6 @@ import jakarta.servlet.http.HttpServletMapping; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; -import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HttpException; import org.eclipse.jetty.http.HttpURI; import org.eclipse.jetty.http.UriCompliance; @@ -247,8 +246,7 @@ private static void checkUriViolations(HttpURI uri, Request baseRequest) { HttpChannel channel = baseRequest.getHttpChannel(); UriCompliance compliance = channel == null || channel.getHttpConfiguration() == null ? null : channel.getHttpConfiguration().getUriCompliance(); - ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)baseRequest.getAttribute(ComplianceViolation.Listener.class.getName()); - String illegalState = UriCompliance.checkUriCompliance(compliance, uri, complianceViolationListener); + String illegalState = UriCompliance.checkUriCompliance(compliance, uri, baseRequest.getCoreRequest().getComponents().getComplianceViolationListener()); if (illegalState != null) throw new IllegalStateException(illegalState); } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index a3c279dec471..ff197e5b4c44 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -538,7 +538,7 @@ public HttpChannelState getHttpChannelState() @Deprecated(since = "12.0.6", forRemoval = true) public ComplianceViolation.Listener getComplianceViolationListener() { - return null; + return getCoreRequest().getComponents().getComplianceViolationListener(); } /** @@ -1986,13 +1986,10 @@ else if (getCharacterEncoding() != null) private void reportComplianceViolations() { - ComplianceViolation.Listener complianceViolationListener = (ComplianceViolation.Listener)getAttribute(ComplianceViolation.Listener.class.getName()); + ComplianceViolation.Listener complianceViolationListener = getCoreRequest().getComponents().getComplianceViolationListener(); List nonComplianceWarnings = _multiParts.getNonComplianceWarnings(); for (MultiPartFormInputStream.NonCompliance nc : nonComplianceWarnings) - { - if (complianceViolationListener != null) - complianceViolationListener.onComplianceViolation(new ComplianceViolation.Event(nc.mode(), nc.violation(), nc.detail())); - } + complianceViolationListener.onComplianceViolation(new ComplianceViolation.Event(nc.mode(), nc.violation(), nc.detail())); } private MultiPartFormInputStream newMultiParts(MultipartConfigElement config, int maxParts) throws IOException diff --git a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java index bfadcb2d8008..607d1af343cf 100644 --- a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java +++ b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java @@ -138,7 +138,7 @@ public void init() throws Exception http.getHttpConfiguration().addCustomizer(new ForwardedRequestCustomizer()); http.getHttpConfiguration().setRequestCookieCompliance(CookieCompliance.RFC6265_LEGACY); _connector = new LocalConnector(_server, http); - _connector.addBean(new ComplianceViolation.CapturingListenerFactory()); + _connector.addBean(new ComplianceViolation.CapturingListener()); _server.addConnector(_connector); _connector.setIdleTimeout(500); _handler = new RequestHandler(); diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java index ceb7e5f18621..3c9ad6b49b38 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java @@ -109,7 +109,7 @@ public static void startServer() throws Exception HttpConnectionFactory httpConnectionFactory = new HttpConnectionFactory(config); httpConnectionFactory.setRecordHttpComplianceViolations(true); connector = new LocalConnector(server, null, null, null, -1, httpConnectionFactory); - connector.addBean(new ComplianceViolation.CapturingListenerFactory()); + connector.addBean(new ComplianceViolation.CapturingListener()); ServletContextHandler context = new ServletContextHandler(); context.setContextPath("/"); From 607934d9940f3b5b7aa8eba46d451963612e3f10 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 18 Jan 2024 16:25:19 +1100 Subject: [PATCH 16/31] WIP on cleanup of ComplianceViolation.Listener instantiation removed attribute access in favour of a HttpChannelState field --- .../jetty/http/ComplianceViolation.java | 41 ++++++++----------- 1 file changed, 18 insertions(+), 23 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java index a42572ca566c..a573cf7174af 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java @@ -98,6 +98,10 @@ interface Listener { Listener NOOP = new Listener() {}; + /** + * Initialize the listener in preparation for a new request life cycle. + * @return The {@link Listener} instance to use for the request life cycle. + */ default Listener initialize() { return this; @@ -230,44 +234,35 @@ class CapturingListener implements Listener { public static final String VIOLATIONS_ATTR_KEY = "org.eclipse.jetty.http.compliance.violations"; - @Override - public Listener initialize() - { - return new Listener() - { - private final List events = new ArrayList<>(); + private final List events; - @Override - public void onRequestBegin(Attributes request) - { - if (request != null) - request.setAttribute(VIOLATIONS_ATTR_KEY, events); - } + public CapturingListener() + { + this(null); + } - @Override - public void onComplianceViolation(Event event) - { - events.add(event); - } - }; + private CapturingListener(List events) + { + this.events = events; } @Override - public void onRequestBegin(Attributes request) + public Listener initialize() { - throw new IllegalStateException("!initialized"); + return new CapturingListener(new ArrayList<>()); } @Override - public void onRequestEnd(Attributes request) + public void onRequestBegin(Attributes request) { - throw new IllegalStateException("!initialized"); + if (request != null) + request.setAttribute(VIOLATIONS_ATTR_KEY, events); } @Override public void onComplianceViolation(Event event) { - throw new IllegalStateException("!initialized"); + events.add(event); } } } From 9a2643a015c686f04925cde77fd5870109d46253 Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 18 Jan 2024 16:30:43 +1100 Subject: [PATCH 17/31] WIP on cleanup of ComplianceViolation.Listener instantiation removed attribute access in favour of a HttpChannelState field --- .../java/org/eclipse/jetty/server/Server.java | 59 ++++++++++--------- 1 file changed, 30 insertions(+), 29 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 7495bf948a6d..3a37dbc64c58 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -22,12 +22,12 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; @@ -87,7 +87,7 @@ public class Server extends Handler.Wrapper implements Attributes private final Context _serverContext = new ServerContext(); private final AutoLock _dateLock = new AutoLock(); private final MimeTypes.Mutable _mimeTypes = new MimeTypes.Mutable(); - private final Map _complianceViolationListener = new HashMap<>(); + private final Map _complianceViolationListener = new ConcurrentHashMap<>(); private String _serverInfo = __serverInfo; private boolean _stopAtShutdown; private boolean _dumpAfterStart; @@ -164,8 +164,32 @@ public Server(@Name("threadPool") ThreadPool threadPool, @Name("scheduler") Sche */ public static ComplianceViolation.Listener getComplianceViolationListener(Connector connector) { - ComplianceViolation.Listener listener = connector.getServer()._complianceViolationListener.get(connector); - return listener == null ? ComplianceViolation.Listener.NOOP : listener; + // TODO store listener in a field of AbstractConnector? + return connector.getServer()._complianceViolationListener.computeIfAbsent(connector, c -> + { + Server server = connector.getServer(); + + // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled + // This also means that any user provided ComplianceViolation.ListenerFactory beans will only be + // used when the configuration on the HttpConnectionFactory allows then to be used. + HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class); + if (httpConnectionFactory == null || !httpConnectionFactory.isRecordHttpComplianceViolations()) + return ComplianceViolation.Listener.NOOP; + + // Look for optional user provided ComplianceViolation.Listener and ComplianceViolation.ListenerFactory beans + List userListeners = new ArrayList<>(); + userListeners.addAll(connector.getBeans(ComplianceViolation.Listener.class)); + userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); + + // No listeners? then we are done + if (userListeners.isEmpty()) + return ComplianceViolation.Listener.NOOP; + // Only 1 listener, just return it. + if (userListeners.size() == 1) + return userListeners.get(0); + // More than 1, establish ComplianceViolation.ListenerCollection for user listeners + return new ComplianceViolation.ListenerCollection(userListeners); + }); } public Handler getDefaultHandler() @@ -594,31 +618,7 @@ protected void doStart() throws Exception { try { - _complianceViolationListener.computeIfAbsent(connector, c -> - { - Server server = connector.getServer(); - - // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled - // This also means that any user provided ComplianceViolation.ListenerFactory beans will only be - // used when the configuration on the HttpConnectionFactory allows then to be used. - HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class); - if (httpConnectionFactory == null || !httpConnectionFactory.isRecordHttpComplianceViolations()) - return ComplianceViolation.Listener.NOOP; - - // Look for optional user provided ComplianceViolation.Listener and ComplianceViolation.ListenerFactory beans - List userListeners = new ArrayList<>(); - userListeners.addAll(connector.getBeans(ComplianceViolation.Listener.class)); - userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); - - // No listeners? then we are done - if (userListeners.isEmpty()) - return ComplianceViolation.Listener.NOOP; - // Only 1 listener, just return it. - if (userListeners.size() == 1) - return userListeners.get(0); - // More than 1, establish ComplianceViolation.ListenerCollection for user listeners - return new ComplianceViolation.ListenerCollection(userListeners); - }); + getComplianceViolationListener(connector); connector.start(); } catch (Throwable e) @@ -704,6 +704,7 @@ protected void doStop() throws Exception multiException = ExceptionUtil.combine(multiException, e); } } + _complianceViolationListener.clear(); // And finally stop everything else try From 7f1f08b1b0cc45910764852842f06f3db1ae18fa Mon Sep 17 00:00:00 2001 From: gregw Date: Thu, 18 Jan 2024 16:38:28 +1100 Subject: [PATCH 18/31] WIP on cleanup of ComplianceViolation.Listener instantiation removed attribute access in favour of a HttpChannelState field --- .../src/main/java/org/eclipse/jetty/server/Server.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 3a37dbc64c58..1124c4cd5a2e 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -164,7 +164,7 @@ public Server(@Name("threadPool") ThreadPool threadPool, @Name("scheduler") Sche */ public static ComplianceViolation.Listener getComplianceViolationListener(Connector connector) { - // TODO store listener in a field of AbstractConnector? + // TODO store listener in a field of AbstractConnection? return connector.getServer()._complianceViolationListener.computeIfAbsent(connector, c -> { Server server = connector.getServer(); From 3357919280043ee52a5e8159f984907e9c934117 Mon Sep 17 00:00:00 2001 From: gregw Date: Sat, 20 Jan 2024 16:53:12 +0900 Subject: [PATCH 19/31] javadoc --- .../src/main/java/org/eclipse/jetty/server/Server.java | 1 - 1 file changed, 1 deletion(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 1124c4cd5a2e..d27209d6246b 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -618,7 +618,6 @@ protected void doStart() throws Exception { try { - getComplianceViolationListener(connector); connector.start(); } catch (Throwable e) From 960120435aceb7b26445feb2c55b10f496c0da93 Mon Sep 17 00:00:00 2001 From: gregw Date: Sat, 20 Jan 2024 17:05:34 +0900 Subject: [PATCH 20/31] Store listeners in AbstractConnector --- .../jetty/server/AbstractConnector.java | 33 +++++++++++++++++++ .../java/org/eclipse/jetty/server/Server.java | 31 +---------------- 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java index 13bd50fec21d..aa5f4e424cb8 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java @@ -31,6 +31,7 @@ import java.util.concurrent.locks.Condition; import java.util.stream.Collectors; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.io.ArrayByteBufferPool; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.EndPoint; @@ -159,6 +160,7 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co private int _acceptorPriorityDelta = -2; private boolean _accepting = true; private ThreadPoolBudget.Lease _lease; + private ComplianceViolation.Listener _complianceViolationListener; /** * @param server The {@link Server} this connector will be added to, must not be null @@ -321,6 +323,36 @@ public boolean isShutdownDone() LOG.info("Started {}", this); } + public ComplianceViolation.Listener getComplianceViolationListener() + { + if (_complianceViolationListener == null) + { + Server server = getServer(); + + // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled + // This also means that any user provided ComplianceViolation.ListenerFactory beans will only be + // used when the configuration on the HttpConnectionFactory allows then to be used. + HttpConnectionFactory httpConnectionFactory = getConnectionFactory(HttpConnectionFactory.class); + if (httpConnectionFactory == null || !httpConnectionFactory.isRecordHttpComplianceViolations()) + return ComplianceViolation.Listener.NOOP; + + // Look for optional user provided ComplianceViolation.Listener and ComplianceViolation.ListenerFactory beans + List userListeners = new ArrayList<>(); + userListeners.addAll(getBeans(ComplianceViolation.Listener.class)); + userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); + + // No listeners? then we are done + if (userListeners.isEmpty()) + return ComplianceViolation.Listener.NOOP; + // Only 1 listener, just return it. + if (userListeners.size() == 1) + return userListeners.get(0); + // More than 1, establish ComplianceViolation.ListenerCollection for user listeners + _complianceViolationListener = new ComplianceViolation.ListenerCollection(userListeners); + } + return _complianceViolationListener; + } + protected void interruptAcceptors() { try (AutoLock lock = _lock.lock()) @@ -372,6 +404,7 @@ protected void doStop() throws Exception removeBean(a); _shutdown = null; + _complianceViolationListener = null; LOG.info("Stopped {}", this); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index d27209d6246b..4e74a2a1a708 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -23,11 +23,9 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; -import java.util.Map; import java.util.Objects; import java.util.Optional; import java.util.Set; -import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; @@ -87,7 +85,6 @@ public class Server extends Handler.Wrapper implements Attributes private final Context _serverContext = new ServerContext(); private final AutoLock _dateLock = new AutoLock(); private final MimeTypes.Mutable _mimeTypes = new MimeTypes.Mutable(); - private final Map _complianceViolationListener = new ConcurrentHashMap<>(); private String _serverInfo = __serverInfo; private boolean _stopAtShutdown; private boolean _dumpAfterStart; @@ -164,32 +161,7 @@ public Server(@Name("threadPool") ThreadPool threadPool, @Name("scheduler") Sche */ public static ComplianceViolation.Listener getComplianceViolationListener(Connector connector) { - // TODO store listener in a field of AbstractConnection? - return connector.getServer()._complianceViolationListener.computeIfAbsent(connector, c -> - { - Server server = connector.getServer(); - - // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled - // This also means that any user provided ComplianceViolation.ListenerFactory beans will only be - // used when the configuration on the HttpConnectionFactory allows then to be used. - HttpConnectionFactory httpConnectionFactory = connector.getConnectionFactory(HttpConnectionFactory.class); - if (httpConnectionFactory == null || !httpConnectionFactory.isRecordHttpComplianceViolations()) - return ComplianceViolation.Listener.NOOP; - - // Look for optional user provided ComplianceViolation.Listener and ComplianceViolation.ListenerFactory beans - List userListeners = new ArrayList<>(); - userListeners.addAll(connector.getBeans(ComplianceViolation.Listener.class)); - userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); - - // No listeners? then we are done - if (userListeners.isEmpty()) - return ComplianceViolation.Listener.NOOP; - // Only 1 listener, just return it. - if (userListeners.size() == 1) - return userListeners.get(0); - // More than 1, establish ComplianceViolation.ListenerCollection for user listeners - return new ComplianceViolation.ListenerCollection(userListeners); - }); + return connector instanceof AbstractConnector abstractConnector ? abstractConnector.getComplianceViolationListener() : ComplianceViolation.Listener.NOOP; } public Handler getDefaultHandler() @@ -703,7 +675,6 @@ protected void doStop() throws Exception multiException = ExceptionUtil.combine(multiException, e); } } - _complianceViolationListener.clear(); // And finally stop everything else try From dc5de07fa812721a610cf9b0a1c901c05a294aca Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Mon, 22 Jan 2024 15:00:02 -0600 Subject: [PATCH 21/31] Cleanup and reorganization of Proposal 2 --- .../jetty/http/ComplianceViolation.java | 73 +----------- .../eclipse/jetty/http/HttpCompliance.java | 64 ++++++++++- .../org/eclipse/jetty/http/HttpParser.java | 6 + .../eclipse/jetty/http/MultiPartFormData.java | 11 ++ .../internal/HTTP2ServerConnection.java | 1 + .../server/internal/HttpStreamOverHTTP2.java | 8 +- .../server/internal/HttpStreamOverHTTP3.java | 8 +- .../internal/ServerHTTP3StreamConnection.java | 1 + .../jetty/server/AbstractConnector.java | 108 +++++++++++++++--- .../org/eclipse/jetty/server/HttpChannel.java | 9 +- .../jetty/server/HttpConfiguration.java | 40 +++++++ .../jetty/server/HttpConnectionFactory.java | 16 ++- .../server/internal/HttpChannelState.java | 7 +- .../jetty/server/internal/HttpConnection.java | 6 + 14 files changed, 251 insertions(+), 107 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java index a573cf7174af..60743c761f3c 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java @@ -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() { @@ -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) { @@ -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 - { - private static final Logger LOG = LoggerFactory.getLogger(ListenerCollection.class); - private final List 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 userListeners) - { - this.userListeners = userListeners; - } - - @Override - public Listener initialize() - { - List 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); diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index 68218c383279..9d36bff2fc48 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -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; @@ -82,7 +83,6 @@ 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"), /** @@ -90,7 +90,6 @@ public enum Violation implements ComplianceViolation * 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"), /** @@ -112,7 +111,6 @@ 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"), /** @@ -120,7 +118,6 @@ public enum Violation implements ComplianceViolation * 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"), /** @@ -128,7 +125,6 @@ public enum Violation implements ComplianceViolation * 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; @@ -363,4 +359,62 @@ private static Set copyOf(Set 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()); + } } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java index c2f0c4e5642b..3dd28fe43dd1 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpParser.java @@ -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; @@ -1988,6 +1992,8 @@ public String toString() */ public interface HttpHandler { + default void messageBegin() {} + boolean content(ByteBuffer item); boolean headerComplete(); diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index c8b056f71a34..af8699dcc5a5 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -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 violations instead? + // that the ee10 layer could use? + private ComplianceViolation.Listener complianceViolationListener; private boolean useFilesForPartsWithoutFileName; private Path filesDirectory; private long maxFileSize = -1; @@ -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); diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java index 15855ed47600..8925643b1686 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java @@ -258,6 +258,7 @@ private HttpChannel pollHttpChannel() httpChannel = httpChannels.poll(); if (httpChannel == null) httpChannel = httpChannelFactory.newHttpChannel(this); + httpChannel.init(); return httpChannel; } diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java index 70eefb6b42a6..17c845abecae 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java @@ -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; @@ -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; @@ -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(); 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()) { diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index bfd638ef1216..831844badf20 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -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; @@ -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; @@ -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()) { diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/ServerHTTP3StreamConnection.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/ServerHTTP3StreamConnection.java index 5ded0ab5e897..18814f8c1450 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/ServerHTTP3StreamConnection.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/ServerHTTP3StreamConnection.java @@ -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); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java index aa5f4e424cb8..fdb463a4f283 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java @@ -330,25 +330,26 @@ public ComplianceViolation.Listener getComplianceViolationListener() Server server = getServer(); // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled - // This also means that any user provided ComplianceViolation.ListenerFactory beans will only be - // used when the configuration on the HttpConnectionFactory allows then to be used. - HttpConnectionFactory httpConnectionFactory = getConnectionFactory(HttpConnectionFactory.class); - if (httpConnectionFactory == null || !httpConnectionFactory.isRecordHttpComplianceViolations()) - return ComplianceViolation.Listener.NOOP; - - // Look for optional user provided ComplianceViolation.Listener and ComplianceViolation.ListenerFactory beans - List userListeners = new ArrayList<>(); - userListeners.addAll(getBeans(ComplianceViolation.Listener.class)); - userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); - - // No listeners? then we are done - if (userListeners.isEmpty()) - return ComplianceViolation.Listener.NOOP; - // Only 1 listener, just return it. - if (userListeners.size() == 1) - return userListeners.get(0); - // More than 1, establish ComplianceViolation.ListenerCollection for user listeners - _complianceViolationListener = new ComplianceViolation.ListenerCollection(userListeners); + // This also means that any user provided ComplianceViolation.Listener beans will only be + // used when the HttpConfiguration allows then to be used. + HttpConfiguration httpConfiguration = getBean(HttpConfiguration.class); + if (!httpConfiguration.isNotifyComplianceViolations()) + _complianceViolationListener = ComplianceViolation.Listener.NOOP; + else + { + // Look for optional user provided ComplianceViolation.Listener beans + List userListeners = new ArrayList<>(); + userListeners.addAll(getBeans(ComplianceViolation.Listener.class)); + if (server != null) + userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); + + if (userListeners.isEmpty()) + _complianceViolationListener = ComplianceViolation.Listener.NOOP; + else if (userListeners.size() == 1) + _complianceViolationListener = userListeners.get(0); + else + _complianceViolationListener = new ComplianceViolationListenerCollection(userListeners); + } } return _complianceViolationListener; } @@ -809,4 +810,73 @@ public String toString() hashCode(), getDefaultProtocol(), getProtocols().stream().collect(Collectors.joining(", ", "(", ")"))); } + + /** + * A Listener that represents multiple user {@link ComplianceViolation.Listener} instances + */ + private static class ComplianceViolationListenerCollection implements ComplianceViolation.Listener + { + private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolationListenerCollection.class); + private final List userListeners; + + /** + * Construct a new ComplianceViolations that will notify user listeners. + * + * @param userListeners the user listeners to notify, null or empty is allowed. + */ + public ComplianceViolationListenerCollection(List userListeners) + { + this.userListeners = userListeners; + } + + @Override + public ComplianceViolation.Listener initialize() + { + List initialized = null; + for (ComplianceViolation.Listener listener : userListeners) + { + ComplianceViolation.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 ComplianceViolationListenerCollection(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); + } + } + } + } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 8d541209bcfd..3e5b0f60c527 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -43,7 +43,7 @@ public interface HttpChannel extends Invocable Components getComponents(); /** - * Set the {@link HttpStream} to associate to this channel.. + * Set the {@link HttpStream} to associate to this channel. * @param httpStream the {@link HttpStream} to associate to this channel. */ void setHttpStream(HttpStream httpStream); @@ -106,9 +106,16 @@ public interface HttpChannel extends Invocable /** * Recycle the HttpChannel, so that a new cycle of calling {@link #setHttpStream(HttpStream)}, * {@link #onRequest(MetaData.Request)} etc. may be performed on the channel. + * @see #init() */ void recycle(); + /** + * Initialize the HttpChannel when a new cycle of request handling begins. + * @see #recycle() + */ + void init(); + /** *

A factory for {@link HttpChannel} instances.

* diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java index 7e0a1cf0cb3d..0b17ffb8e1c9 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java @@ -24,6 +24,7 @@ import org.eclipse.jetty.http.HttpFields; import org.eclipse.jetty.http.HttpMethod; import org.eclipse.jetty.http.HttpScheme; +import org.eclipse.jetty.http.MultiPartCompliance; import org.eclipse.jetty.http.UriCompliance; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.Index; @@ -75,6 +76,8 @@ public class HttpConfiguration implements Dumpable private UriCompliance _uriCompliance = UriCompliance.DEFAULT; private CookieCompliance _requestCookieCompliance = CookieCompliance.RFC6265; private CookieCompliance _responseCookieCompliance = CookieCompliance.RFC6265; + private MultiPartCompliance _multiPartCompliance = MultiPartCompliance.RFC7578; + private boolean _notifyComplianceViolations = false; private boolean _notifyRemoteAsyncErrors = true; private boolean _relativeRedirectAllowed = true; private HostPort _serverAuthority; @@ -147,6 +150,8 @@ public HttpConfiguration(HttpConfiguration config) _httpCompliance = config._httpCompliance; _requestCookieCompliance = config._requestCookieCompliance; _responseCookieCompliance = config._responseCookieCompliance; + _multiPartCompliance = config._multiPartCompliance; + _notifyComplianceViolations = config._notifyComplianceViolations; _notifyRemoteAsyncErrors = config._notifyRemoteAsyncErrors; _relativeRedirectAllowed = config._relativeRedirectAllowed; _uriCompliance = config._uriCompliance; @@ -628,6 +633,41 @@ public void setResponseCookieCompliance(CookieCompliance cookieCompliance) _responseCookieCompliance = cookieCompliance == null ? CookieCompliance.RFC6265 : cookieCompliance; } + /** + * @return the {@link MultiPartCompliance} used for validating multipart form syntax. + */ + public MultiPartCompliance getMultiPartCompliance() + { + return _multiPartCompliance; + } + + /** + * @param multiPartCompliance the {@link MultiPartCompliance} used for validating multipart form syntax. + */ + public void setMultiPartCompliance(MultiPartCompliance multiPartCompliance) + { + this._multiPartCompliance = multiPartCompliance; + } + + /** + * Set whether compliance violations (from {@link HttpCompliance}, {@link CookieCompliance}, {@link UriCompliance}, and {@link MultiPartCompliance}) + * should be reported to the registered ComplianceViolation.Listeners. + * + * @return true if Compliance Violations should be notified via the ComplianceViolation.Listener mechanism. + */ + public boolean isNotifyComplianceViolations() + { + return _notifyComplianceViolations; + } + + /** + * @param notifyComplianceViolations true + */ + public void setNotifyComplianceViolations(boolean notifyComplianceViolations) + { + _notifyComplianceViolations = notifyComplianceViolations; + } + /** * Set whether remote errors, when detected, are notified to async applications. * @param notifyRemoteAsyncErrors whether remote errors, when detected, are notified to async applications diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java index cae2cdfc1b36..764bf859e6ec 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java @@ -31,7 +31,6 @@ public class HttpConnectionFactory extends AbstractConnectionFactory implements HttpConfiguration.ConnectionFactory, ConnectionFactory.Configuring { private final HttpConfiguration _config; - private boolean _recordHttpComplianceViolations; private boolean _useInputDirectByteBuffers; private boolean _useOutputDirectByteBuffers; @@ -58,18 +57,27 @@ public HttpConfiguration getHttpConfiguration() @Override public void configure(Connector connector) { - if (isRecordHttpComplianceViolations()) + // TODO: need HTTP/2 and HTTP/3 version of this + if (getHttpConfiguration().isNotifyComplianceViolations()) addBean(new ComplianceViolation.LoggingListener()); } + /** + * @deprecated use {@link HttpConfiguration#isNotifyComplianceViolations()} instead. will be removed in Jetty 12.1.0 + */ + @Deprecated(since = "12.0.5", forRemoval = true) public boolean isRecordHttpComplianceViolations() { - return _recordHttpComplianceViolations; + return _config.isNotifyComplianceViolations(); } + /** + * @deprecated use {@link HttpConfiguration#setNotifyComplianceViolations(boolean)} instead. will be removed in Jetty 12.1.0 + */ + @Deprecated(since = "12.0.5", forRemoval = true) public void setRecordHttpComplianceViolations(boolean recordHttpComplianceViolations) { - this._recordHttpComplianceViolations = recordHttpComplianceViolations; + _config.setNotifyComplianceViolations(recordHttpComplianceViolations); } public boolean isUseInputDirectByteBuffers() diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index f8c07355811d..d6dace0d0303 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -127,6 +127,11 @@ public HttpChannelState(ConnectionMetaData connectionMetaData) // The SerializedInvoker is used to prevent infinite recursion of callbacks calling methods calling callbacks etc. _readInvoker = new HttpChannelSerializedInvoker(); _writeInvoker = new HttpChannelSerializedInvoker(); + } + + @Override + public void init() + { _complianceViolationListener = Server.getComplianceViolationListener(_connectionMetaData.getConnector()).initialize(); } @@ -172,7 +177,7 @@ public void recycle() _onFailure = null; _callbackFailure = null; _expects100Continue = false; - _complianceViolationListener = _complianceViolationListener.initialize(); + _complianceViolationListener = null; } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 539a41c2b694..dacba8b7c6c5 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -928,6 +928,12 @@ public RequestHandler(Connector connector) { } + @Override + public void messageBegin() + { + _httpChannel.init(); + } + @Override public void startRequest(String method, String uri, HttpVersion version) { From c266d1632fb0af407eb4ce6b234d171e4f064b4d Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 23 Jan 2024 21:57:54 +0900 Subject: [PATCH 22/31] updates from review --- .../jetty/server/AbstractConnector.java | 106 +------------ .../org/eclipse/jetty/server/Components.java | 5 + .../jetty/server/HttpConfiguration.java | 149 +++++++++++++++++- .../jetty/server/HttpConnectionFactory.java | 4 - .../java/org/eclipse/jetty/server/Server.java | 13 -- .../server/internal/HttpChannelState.java | 9 +- .../servlet/ComplianceViolations2616Test.java | 3 +- .../eclipse/jetty/ee9/nested/RequestTest.java | 2 +- .../servlet/ComplianceViolations2616Test.java | 2 +- 9 files changed, 165 insertions(+), 128 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java index fdb463a4f283..e74a5f86e6e0 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java @@ -160,7 +160,6 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co private int _acceptorPriorityDelta = -2; private boolean _accepting = true; private ThreadPoolBudget.Lease _lease; - private ComplianceViolation.Listener _complianceViolationListener; /** * @param server The {@link Server} this connector will be added to, must not be null @@ -272,6 +271,10 @@ public int getAcceptors() @Override protected void doStart() throws Exception { + if (!getBeans(ComplianceViolation.Listener.class).isEmpty() || + !getServer().getBeans(ComplianceViolation.Listener.class).isEmpty()) + LOG.warn("ComplianceViolation.Listeners must now be set on HttpConfiguration"); + getConnectionFactories().stream() .filter(ConnectionFactory.Configuring.class::isInstance) .map(ConnectionFactory.Configuring.class::cast) @@ -323,37 +326,6 @@ public boolean isShutdownDone() LOG.info("Started {}", this); } - public ComplianceViolation.Listener getComplianceViolationListener() - { - if (_complianceViolationListener == null) - { - Server server = getServer(); - - // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled - // This also means that any user provided ComplianceViolation.Listener beans will only be - // used when the HttpConfiguration allows then to be used. - HttpConfiguration httpConfiguration = getBean(HttpConfiguration.class); - if (!httpConfiguration.isNotifyComplianceViolations()) - _complianceViolationListener = ComplianceViolation.Listener.NOOP; - else - { - // Look for optional user provided ComplianceViolation.Listener beans - List userListeners = new ArrayList<>(); - userListeners.addAll(getBeans(ComplianceViolation.Listener.class)); - if (server != null) - userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); - - if (userListeners.isEmpty()) - _complianceViolationListener = ComplianceViolation.Listener.NOOP; - else if (userListeners.size() == 1) - _complianceViolationListener = userListeners.get(0); - else - _complianceViolationListener = new ComplianceViolationListenerCollection(userListeners); - } - } - return _complianceViolationListener; - } - protected void interruptAcceptors() { try (AutoLock lock = _lock.lock()) @@ -405,7 +377,6 @@ protected void doStop() throws Exception removeBean(a); _shutdown = null; - _complianceViolationListener = null; LOG.info("Stopped {}", this); } @@ -810,73 +781,4 @@ public String toString() hashCode(), getDefaultProtocol(), getProtocols().stream().collect(Collectors.joining(", ", "(", ")"))); } - - /** - * A Listener that represents multiple user {@link ComplianceViolation.Listener} instances - */ - private static class ComplianceViolationListenerCollection implements ComplianceViolation.Listener - { - private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolationListenerCollection.class); - private final List userListeners; - - /** - * Construct a new ComplianceViolations that will notify user listeners. - * - * @param userListeners the user listeners to notify, null or empty is allowed. - */ - public ComplianceViolationListenerCollection(List userListeners) - { - this.userListeners = userListeners; - } - - @Override - public ComplianceViolation.Listener initialize() - { - List initialized = null; - for (ComplianceViolation.Listener listener : userListeners) - { - ComplianceViolation.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 ComplianceViolationListenerCollection(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); - } - } - } - } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java index 1be2863c96a5..0dbbea45ef72 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java @@ -45,5 +45,10 @@ public interface Components */ Attributes getCache(); + /** + * @return A {@link ComplianceViolation.Listener} instance that has been + * {@link ComplianceViolation.Listener#initialize() initialized} + * for the current request cycle. + */ ComplianceViolation.Listener getComplianceViolationListener(); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java index 0b17ffb8e1c9..5283e4bfd0e1 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java @@ -15,10 +15,12 @@ import java.io.IOException; import java.net.SocketAddress; +import java.util.ArrayList; import java.util.List; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpFields; @@ -26,6 +28,7 @@ import org.eclipse.jetty.http.HttpScheme; import org.eclipse.jetty.http.MultiPartCompliance; import org.eclipse.jetty.http.UriCompliance; +import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.HostPort; import org.eclipse.jetty.util.Index; import org.eclipse.jetty.util.Jetty; @@ -33,6 +36,8 @@ import org.eclipse.jetty.util.annotation.ManagedObject; import org.eclipse.jetty.util.component.Dumpable; import org.eclipse.jetty.util.component.DumpableCollection; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * HTTP Configuration. @@ -53,6 +58,8 @@ public class HttpConfiguration implements Dumpable .caseSensitive(false) .mutable() .build(); + private final List _complianceViolationListeners = new CopyOnWriteArrayList<>(); + private ComplianceViolation.Listener _combinedComplianceViolationListener; private int _outputBufferSize = 32 * 1024; private int _outputAggregationSize = _outputBufferSize / 4; private int _requestHeaderSize = 8 * 1024; @@ -77,7 +84,6 @@ public class HttpConfiguration implements Dumpable private CookieCompliance _requestCookieCompliance = CookieCompliance.RFC6265; private CookieCompliance _responseCookieCompliance = CookieCompliance.RFC6265; private MultiPartCompliance _multiPartCompliance = MultiPartCompliance.RFC7578; - private boolean _notifyComplianceViolations = false; private boolean _notifyRemoteAsyncErrors = true; private boolean _relativeRedirectAllowed = true; private HostPort _serverAuthority; @@ -124,6 +130,7 @@ public HttpConfiguration() public HttpConfiguration(HttpConfiguration config) { _customizers.addAll(config._customizers); + _complianceViolationListeners.addAll(config._complianceViolationListeners); for (String s : config._formEncodedMethods.keySet()) { _formEncodedMethods.put(s, Boolean.TRUE); @@ -151,7 +158,6 @@ public HttpConfiguration(HttpConfiguration config) _requestCookieCompliance = config._requestCookieCompliance; _responseCookieCompliance = config._responseCookieCompliance; _multiPartCompliance = config._multiPartCompliance; - _notifyComplianceViolations = config._notifyComplianceViolations; _notifyRemoteAsyncErrors = config._notifyRemoteAsyncErrors; _relativeRedirectAllowed = config._relativeRedirectAllowed; _uriCompliance = config._uriCompliance; @@ -657,7 +663,7 @@ public void setMultiPartCompliance(MultiPartCompliance multiPartCompliance) */ public boolean isNotifyComplianceViolations() { - return _notifyComplianceViolations; + return _complianceViolationListeners.stream().anyMatch(ComplianceViolation.LoggingListener.class::isInstance); } /** @@ -665,7 +671,15 @@ public boolean isNotifyComplianceViolations() */ public void setNotifyComplianceViolations(boolean notifyComplianceViolations) { - _notifyComplianceViolations = notifyComplianceViolations; + if (notifyComplianceViolations == isNotifyComplianceViolations()) + return; + if (notifyComplianceViolations) + _complianceViolationListeners.add(new ComplianceViolation.LoggingListener()); + else + _complianceViolationListeners.stream() + .filter(ComplianceViolation.LoggingListener.class::isInstance) + .forEach(_complianceViolationListeners::remove); + _combinedComplianceViolationListener = null; } /** @@ -788,6 +802,32 @@ public int getMaxUnconsumedRequestContentReads() return _maxUnconsumedRequestContentReads; } + public void addComplianceViolationListener(ComplianceViolation.Listener listener) + { + _complianceViolationListeners.add(listener); + _combinedComplianceViolationListener = null; + } + + public void removeComplianceViolationListener(ComplianceViolation.Listener listener) + { + _complianceViolationListeners.remove(listener); + _combinedComplianceViolationListener = null; + } + + public ComplianceViolation.Listener getCombinedComplianceViolationListener() + { + if (_combinedComplianceViolationListener == null) + { + _combinedComplianceViolationListener = switch (_complianceViolationListeners.size()) + { + case 0 -> ComplianceViolation.Listener.NOOP; + case 1 -> _complianceViolationListeners.get(0); + default -> new ComplianceViolationListenerCollection(_complianceViolationListeners); + }; + } + return _combinedComplianceViolationListener; + } + @Override public String dump() { @@ -837,4 +877,105 @@ public String toString() _securePort, _customizers); } + + /** + * A Listener that represents multiple user {@link ComplianceViolation.Listener} instances + */ + private static class ComplianceViolationListenerCollection implements ComplianceViolation.Listener + { + private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolationListenerCollection.class); + private final List _listeners; + + /** + * Construct a new ComplianceViolations that will notify user listeners. + * + * @param listeners the user listeners to notify, null or empty is allowed. + */ + public ComplianceViolationListenerCollection(List listeners) + { + _listeners = listeners; + } + + @Override + public ComplianceViolation.Listener initialize() + { + List initialized = null; + for (ComplianceViolation.Listener listener : _listeners) + { + ComplianceViolation.Listener listening = listener.initialize(); + if (listening != listener) + { + initialized = new ArrayList<>(_listeners.size()); + for (ComplianceViolation.Listener l : _listeners) + { + if (l == listener) + break; + initialized.add(l); + } + } + if (initialized != null) + initialized.add(listening); + } + if (initialized == null) + return this; + return new ComplianceViolationListenerCollection(initialized); + } + + @Override + public void onRequestBegin(Attributes request) + { + for (ComplianceViolation.Listener listener : _listeners) + { + try + { + listener.onRequestBegin(request); + } + catch (Throwable t) + { + LOG.warn("Unable to begin request on {}", listener, t); + } + } + } + + @Override + public void onRequestEnd(Attributes request) + { + for (ComplianceViolation.Listener listener : _listeners) + { + try + { + listener.onRequestEnd(request); + } + catch (Throwable t) + { + LOG.warn("Unable to end request on {}", listener, t); + } + } + } + + @Override + public void onComplianceViolation(ComplianceViolation.Event event) + { + assert event != null; + notifyUserListeners(event); + } + + private void notifyUserListeners(ComplianceViolation.Event event) + { + if (_listeners == null || _listeners.isEmpty()) + return; + + for (ComplianceViolation.Listener listener : _listeners) + { + try + { + listener.onComplianceViolation(event); + } + catch (Throwable t) + { + LOG.warn("Unable to notify ComplianceViolation.Listener implementation at {} of event {}", listener, event, t); + } + } + } + } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java index 764bf859e6ec..519f139231ac 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java @@ -15,7 +15,6 @@ import java.util.Objects; -import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.io.Connection; import org.eclipse.jetty.io.EndPoint; @@ -57,9 +56,6 @@ public HttpConfiguration getHttpConfiguration() @Override public void configure(Connector connector) { - // TODO: need HTTP/2 and HTTP/3 version of this - if (getHttpConfiguration().isNotifyComplianceViolations()) - addBean(new ComplianceViolation.LoggingListener()); } /** diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 4e74a2a1a708..bed6d6fbcffa 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -29,7 +29,6 @@ import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; -import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.DateGenerator; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; @@ -152,18 +151,6 @@ public Server(@Name("threadPool") ThreadPool threadPool, @Name("scheduler") Sche addBean(FileSystemPool.INSTANCE, false); } - /** - * Get a new ComplianceViolation.Listener suitable for given Connector. - * - * @param connector the connector to base the ComplianceViolation.Listener off of. - * @return the ComplianceViolation.Listener implementation, or null if {@link HttpConnectionFactory#isRecordHttpComplianceViolations()} is false, - * or there are no ComplianceViolation.Listener implementations registered. - */ - public static ComplianceViolation.Listener getComplianceViolationListener(Connector connector) - { - return connector instanceof AbstractConnector abstractConnector ? abstractConnector.getComplianceViolationListener() : ComplianceViolation.Listener.NOOP; - } - public Handler getDefaultHandler() { return _defaultHandler; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index d6dace0d0303..f205c7f0e857 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -132,7 +132,7 @@ public HttpChannelState(ConnectionMetaData connectionMetaData) @Override public void init() { - _complianceViolationListener = Server.getComplianceViolationListener(_connectionMetaData.getConnector()).initialize(); + _complianceViolationListener = getHttpConfiguration().getCombinedComplianceViolationListener().initialize(); } @Override @@ -790,7 +790,7 @@ public ConnectionMetaData getConnectionMetaData() return _connectionMetaData; } - private HttpChannelState getHttpChannelState() + public HttpChannelState getHttpChannelState() { try (AutoLock ignore = _lock.lock()) { @@ -1074,6 +1074,11 @@ private ChannelResponse(ChannelRequest request) _httpFields = getResponseHttpFields(_request.lockedGetHttpChannelState()); } + public ChannelRequest getChannelRequest() + { + return _request; + } + protected ResponseHttpFields getResponseHttpFields(HttpChannelState httpChannelState) { return httpChannelState._responseHeaders; diff --git a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java index 9a898b7e0911..f3bea6be187b 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java +++ b/jetty-ee10/jetty-ee10-servlet/src/test/java/org/eclipse/jetty/ee10/servlet/ComplianceViolations2616Test.java @@ -105,11 +105,12 @@ public static void startServer() throws Exception HttpConfiguration config = new HttpConfiguration(); config.setSendServerVersion(false); config.setHttpCompliance(HttpCompliance.RFC2616_LEGACY); + config.addComplianceViolationListener(new ComplianceViolation.LoggingListener()); + config.addComplianceViolationListener(new ComplianceViolation.CapturingListener()); HttpConnectionFactory httpConnectionFactory = new HttpConnectionFactory(config); httpConnectionFactory.setRecordHttpComplianceViolations(true); connector = new LocalConnector(server, null, null, null, -1, httpConnectionFactory); - connector.addBean(new ComplianceViolation.CapturingListener()); ServletContextHandler context = new ServletContextHandler(); context.setContextPath("/"); diff --git a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java index 607d1af343cf..c64ad121971a 100644 --- a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java +++ b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/RequestTest.java @@ -137,8 +137,8 @@ public void init() throws Exception http.getHttpConfiguration().setOutputBufferSize(2048); http.getHttpConfiguration().addCustomizer(new ForwardedRequestCustomizer()); http.getHttpConfiguration().setRequestCookieCompliance(CookieCompliance.RFC6265_LEGACY); + http.getHttpConfiguration().addComplianceViolationListener(new ComplianceViolation.CapturingListener()); _connector = new LocalConnector(_server, http); - _connector.addBean(new ComplianceViolation.CapturingListener()); _server.addConnector(_connector); _connector.setIdleTimeout(500); _handler = new RequestHandler(); diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java index 3c9ad6b49b38..e8f49f5436d9 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ComplianceViolations2616Test.java @@ -105,11 +105,11 @@ public static void startServer() throws Exception HttpConfiguration config = new HttpConfiguration(); config.setSendServerVersion(false); config.setHttpCompliance(HttpCompliance.RFC2616_LEGACY); + config.addComplianceViolationListener(new ComplianceViolation.CapturingListener()); HttpConnectionFactory httpConnectionFactory = new HttpConnectionFactory(config); httpConnectionFactory.setRecordHttpComplianceViolations(true); connector = new LocalConnector(server, null, null, null, -1, httpConnectionFactory); - connector.addBean(new ComplianceViolation.CapturingListener()); ServletContextHandler context = new ServletContextHandler(); context.setContextPath("/"); From cdf8fcd3f6260f01e6e6bf8a070c17d80454bd76 Mon Sep 17 00:00:00 2001 From: gregw Date: Tue, 23 Jan 2024 23:00:08 +0900 Subject: [PATCH 23/31] updates from review --- .../src/main/java/org/eclipse/jetty/http/HttpCompliance.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index 9d36bff2fc48..fc8455771101 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -370,6 +370,9 @@ public static void checkHttpCompliance(MetaData.Request request, HttpCompliance HttpFields fields = request.getHttpFields(); for (HttpField httpField: fields) { + if (httpField.getHeader() == null) + continue; + switch (httpField.getHeader()) { case CONTENT_LENGTH -> From d7ac0d9d3e39031b4d41fb248803e21d7a11cea9 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 23 Jan 2024 12:11:20 -0600 Subject: [PATCH 24/31] Changes from review --- .../server/internal/HttpStreamOverHTTP2.java | 2 +- .../server/internal/HttpStreamOverHTTP3.java | 2 +- .../jetty/server/AbstractConnector.java | 103 ---------------- .../org/eclipse/jetty/server/Components.java | 3 - .../org/eclipse/jetty/server/HttpChannel.java | 35 ++++-- .../jetty/server/HttpConfiguration.java | 34 ++++-- .../jetty/server/HttpConnectionFactory.java | 21 ++-- .../org/eclipse/jetty/server/Request.java | 3 +- .../org/eclipse/jetty/server/Response.java | 4 +- .../java/org/eclipse/jetty/server/Server.java | 13 -- .../server/internal/HttpChannelState.java | 113 ++++++++++++++++-- .../jetty/server/internal/HttpConnection.java | 12 +- .../jetty/server/ServerHttpCookieTest.java | 2 +- .../eclipse/jetty/ee9/nested/Dispatcher.java | 2 +- .../org/eclipse/jetty/ee9/nested/Request.java | 4 +- 15 files changed, 187 insertions(+), 166 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java index 17c845abecae..195fe08b98c3 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HttpStreamOverHTTP2.java @@ -86,7 +86,7 @@ public Runnable onRequest(HeadersFrame frame) _requestMetaData = (MetaData.Request)frame.getMetaData(); // Grab freshly initialized ComplianceViolation.Listener here, no need to reinitialize. - ComplianceViolation.Listener listener = _httpChannel.getComponents().getComplianceViolationListener(); + ComplianceViolation.Listener listener = _httpChannel.getComplianceViolationListener(); Runnable handler = _httpChannel.onRequest(_requestMetaData); Request request = _httpChannel.getRequest(); listener.onRequestBegin(request); diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java index 831844badf20..ac09155c693f 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/HttpStreamOverHTTP3.java @@ -76,7 +76,7 @@ public Runnable onRequest(HeadersFrame frame) requestMetaData = (MetaData.Request)frame.getMetaData(); // Grab freshly initialized ComplianceViolation.Listener here, no need to reinitialize. - ComplianceViolation.Listener listener = httpChannel.getComponents().getComplianceViolationListener(); + ComplianceViolation.Listener listener = httpChannel.getComplianceViolationListener(); Runnable handler = httpChannel.onRequest(requestMetaData); Request request = this.httpChannel.getRequest(); listener.onRequestBegin(request); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java index fdb463a4f283..13bd50fec21d 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java @@ -31,7 +31,6 @@ import java.util.concurrent.locks.Condition; import java.util.stream.Collectors; -import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.io.ArrayByteBufferPool; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.EndPoint; @@ -160,7 +159,6 @@ public abstract class AbstractConnector extends ContainerLifeCycle implements Co private int _acceptorPriorityDelta = -2; private boolean _accepting = true; private ThreadPoolBudget.Lease _lease; - private ComplianceViolation.Listener _complianceViolationListener; /** * @param server The {@link Server} this connector will be added to, must not be null @@ -323,37 +321,6 @@ public boolean isShutdownDone() LOG.info("Started {}", this); } - public ComplianceViolation.Listener getComplianceViolationListener() - { - if (_complianceViolationListener == null) - { - Server server = getServer(); - - // Only add the ComplianceViolations instance if the recording of Compliance Violations is enabled - // This also means that any user provided ComplianceViolation.Listener beans will only be - // used when the HttpConfiguration allows then to be used. - HttpConfiguration httpConfiguration = getBean(HttpConfiguration.class); - if (!httpConfiguration.isNotifyComplianceViolations()) - _complianceViolationListener = ComplianceViolation.Listener.NOOP; - else - { - // Look for optional user provided ComplianceViolation.Listener beans - List userListeners = new ArrayList<>(); - userListeners.addAll(getBeans(ComplianceViolation.Listener.class)); - if (server != null) - userListeners.addAll(server.getBeans(ComplianceViolation.Listener.class)); - - if (userListeners.isEmpty()) - _complianceViolationListener = ComplianceViolation.Listener.NOOP; - else if (userListeners.size() == 1) - _complianceViolationListener = userListeners.get(0); - else - _complianceViolationListener = new ComplianceViolationListenerCollection(userListeners); - } - } - return _complianceViolationListener; - } - protected void interruptAcceptors() { try (AutoLock lock = _lock.lock()) @@ -405,7 +372,6 @@ protected void doStop() throws Exception removeBean(a); _shutdown = null; - _complianceViolationListener = null; LOG.info("Stopped {}", this); } @@ -810,73 +776,4 @@ public String toString() hashCode(), getDefaultProtocol(), getProtocols().stream().collect(Collectors.joining(", ", "(", ")"))); } - - /** - * A Listener that represents multiple user {@link ComplianceViolation.Listener} instances - */ - private static class ComplianceViolationListenerCollection implements ComplianceViolation.Listener - { - private static final Logger LOG = LoggerFactory.getLogger(ComplianceViolationListenerCollection.class); - private final List userListeners; - - /** - * Construct a new ComplianceViolations that will notify user listeners. - * - * @param userListeners the user listeners to notify, null or empty is allowed. - */ - public ComplianceViolationListenerCollection(List userListeners) - { - this.userListeners = userListeners; - } - - @Override - public ComplianceViolation.Listener initialize() - { - List initialized = null; - for (ComplianceViolation.Listener listener : userListeners) - { - ComplianceViolation.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 ComplianceViolationListenerCollection(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); - } - } - } - } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java index 1be2863c96a5..b6e32ea937fd 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Components.java @@ -13,7 +13,6 @@ package org.eclipse.jetty.server; -import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.util.Attributes; import org.eclipse.jetty.util.thread.Scheduler; @@ -44,6 +43,4 @@ public interface Components * @return A Map, which may be an empty map that discards all items. */ Attributes getCache(); - - ComplianceViolation.Listener getComplianceViolationListener(); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 3e5b0f60c527..7c0b0d5d9dad 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -17,6 +17,7 @@ import java.util.function.Consumer; import java.util.function.Predicate; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.MetaData; import org.eclipse.jetty.server.internal.HttpChannelState; import org.eclipse.jetty.util.thread.Invocable; @@ -37,13 +38,7 @@ public interface HttpChannel extends Invocable ConnectionMetaData getConnectionMetaData(); /** - * @return the {@link Components} associated with this {@code HttpChannel}. - * @see Request#getComponents() - */ - Components getComponents(); - - /** - * Set the {@link HttpStream} to associate to this channel. + * Set the {@link HttpStream} to associate to this channel.. * @param httpStream the {@link HttpStream} to associate to this channel. */ void setHttpStream(HttpStream httpStream); @@ -116,6 +111,32 @@ public interface HttpChannel extends Invocable */ void init(); + /** + * @return the active {@link ComplianceViolation.Listener} + */ + ComplianceViolation.Listener getComplianceViolationListener(); + + /** + * @param request attempt to resolve the HttpChannel from the provided request + * @return the HttpChannel if found + * @throws IllegalStateException if unable to find HttpChannel + */ + static HttpChannel from(Request request) + { + while (true) + { + if (request instanceof Request.Wrapper wrapper) + request = wrapper.getWrapped(); + else + break; + } + + if (request.getComponents() instanceof HttpChannel httpChannel) + return httpChannel; + + throw new IllegalStateException("Unable to find HttpChannel from " + request); + } + /** *

A factory for {@link HttpChannel} instances.

* diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java index 0b17ffb8e1c9..0812943338ce 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConfiguration.java @@ -15,10 +15,13 @@ import java.io.IOException; import java.net.SocketAddress; +import java.util.ArrayList; import java.util.List; +import java.util.Objects; import java.util.Set; import java.util.concurrent.CopyOnWriteArrayList; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.CookieCompliance; import org.eclipse.jetty.http.HttpCompliance; import org.eclipse.jetty.http.HttpFields; @@ -53,6 +56,7 @@ public class HttpConfiguration implements Dumpable .caseSensitive(false) .mutable() .build(); + private final List _complianceViolationListeners = new ArrayList<>(); private int _outputBufferSize = 32 * 1024; private int _outputAggregationSize = _outputBufferSize / 4; private int _requestHeaderSize = 8 * 1024; @@ -77,7 +81,6 @@ public class HttpConfiguration implements Dumpable private CookieCompliance _requestCookieCompliance = CookieCompliance.RFC6265; private CookieCompliance _responseCookieCompliance = CookieCompliance.RFC6265; private MultiPartCompliance _multiPartCompliance = MultiPartCompliance.RFC7578; - private boolean _notifyComplianceViolations = false; private boolean _notifyRemoteAsyncErrors = true; private boolean _relativeRedirectAllowed = true; private HostPort _serverAuthority; @@ -151,7 +154,7 @@ public HttpConfiguration(HttpConfiguration config) _requestCookieCompliance = config._requestCookieCompliance; _responseCookieCompliance = config._responseCookieCompliance; _multiPartCompliance = config._multiPartCompliance; - _notifyComplianceViolations = config._notifyComplianceViolations; + _complianceViolationListeners.addAll(config._complianceViolationListeners); _notifyRemoteAsyncErrors = config._notifyRemoteAsyncErrors; _relativeRedirectAllowed = config._relativeRedirectAllowed; _uriCompliance = config._uriCompliance; @@ -650,22 +653,31 @@ public void setMultiPartCompliance(MultiPartCompliance multiPartCompliance) } /** - * Set whether compliance violations (from {@link HttpCompliance}, {@link CookieCompliance}, {@link UriCompliance}, and {@link MultiPartCompliance}) - * should be reported to the registered ComplianceViolation.Listeners. - * - * @return true if Compliance Violations should be notified via the ComplianceViolation.Listener mechanism. + * Add a {@link ComplianceViolation.Listener} to the configuration + * @param listener the listener to add + */ + public void addComplianceViolationListener(ComplianceViolation.Listener listener) + { + this._complianceViolationListeners.add(Objects.requireNonNull(listener)); + } + + /** + * Remove a {@link ComplianceViolation.Listener} from the configuration + * @param listener the listener to remove + * @return {@code true} if this list contained the specified element */ - public boolean isNotifyComplianceViolations() + public boolean removeComplianceViolationListener(ComplianceViolation.Listener listener) { - return _notifyComplianceViolations; + return this._complianceViolationListeners.remove(Objects.requireNonNull(listener)); } /** - * @param notifyComplianceViolations true + * Get the list of configured {@link ComplianceViolation.Listener} to use. + * @return the list of configured listeners */ - public void setNotifyComplianceViolations(boolean notifyComplianceViolations) + public List getComplianceViolationListeners() { - _notifyComplianceViolations = notifyComplianceViolations; + return this._complianceViolationListeners; } /** diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java index 764bf859e6ec..2d72d143afeb 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java @@ -13,6 +13,7 @@ package org.eclipse.jetty.server; +import java.util.Collection; import java.util.Objects; import org.eclipse.jetty.http.ComplianceViolation; @@ -21,6 +22,8 @@ import org.eclipse.jetty.io.EndPoint; import org.eclipse.jetty.server.internal.HttpConnection; import org.eclipse.jetty.util.annotation.Name; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * A Connection Factory for HTTP Connections. @@ -30,6 +33,7 @@ */ public class HttpConnectionFactory extends AbstractConnectionFactory implements HttpConfiguration.ConnectionFactory, ConnectionFactory.Configuring { + private static final Logger LOG = LoggerFactory.getLogger(HttpConnectionFactory.class); private final HttpConfiguration _config; private boolean _useInputDirectByteBuffers; private boolean _useOutputDirectByteBuffers; @@ -57,27 +61,30 @@ public HttpConfiguration getHttpConfiguration() @Override public void configure(Connector connector) { - // TODO: need HTTP/2 and HTTP/3 version of this - if (getHttpConfiguration().isNotifyComplianceViolations()) - addBean(new ComplianceViolation.LoggingListener()); + Collection userListeners = getBeans(ComplianceViolation.Listener.class); + if (userListeners != null && !userListeners.isEmpty()) + for (ComplianceViolation.Listener listener: userListeners) + LOG.warn("Connector based ComplianceViolation.Listener {} not used from Beans, use HttpConfiguration.addComplianceViolationListener() instead", listener); } /** - * @deprecated use {@link HttpConfiguration#isNotifyComplianceViolations()} instead. will be removed in Jetty 12.1.0 + * @return always returns false + * @deprecated use {@link HttpConfiguration#getComplianceViolationListeners()} instead to know if there are any {@link ComplianceViolation.Listener} to notify. this method will be removed in Jetty 12.1.0 */ @Deprecated(since = "12.0.5", forRemoval = true) public boolean isRecordHttpComplianceViolations() { - return _config.isNotifyComplianceViolations(); + return false; } /** - * @deprecated use {@link HttpConfiguration#setNotifyComplianceViolations(boolean)} instead. will be removed in Jetty 12.1.0 + * Does nothing. + * @deprecated use {@link HttpConfiguration#addComplianceViolationListener(ComplianceViolation.Listener)} instead. this method will be removed in Jetty 12.1.0 */ @Deprecated(since = "12.0.5", forRemoval = true) public void setRecordHttpComplianceViolations(boolean recordHttpComplianceViolations) { - _config.setNotifyComplianceViolations(recordHttpComplianceViolations); + // no-op } public boolean isUseInputDirectByteBuffers() diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 327cc75c4c75..027f6651a3e4 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -589,7 +589,8 @@ static List getCookies(Request request) CookieCache cookieCache = (CookieCache)request.getComponents().getCache().getAttribute(CACHE_ATTRIBUTE); if (cookieCache == null) { - cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance(), request.getComponents().getComplianceViolationListener()); + // TODO: CookieCache is per connection, needs to be per request for ComplianceViolation.Listener + cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance(), HttpChannel.from(request).getComplianceViolationListener()); request.getComponents().getCache().setAttribute(CACHE_ATTRIBUTE, cookieCache); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java index 0966c368d3ce..fbf17e921413 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Response.java @@ -385,7 +385,7 @@ static void addCookie(Response response, HttpCookie cookie) } catch (ComplianceViolationException e) { - request.getComponents().getComplianceViolationListener().onComplianceViolation(e.getEvent()); + HttpChannel.from(request).getComplianceViolationListener().onComplianceViolation(e.getEvent()); throw e; } @@ -419,7 +419,7 @@ static void putCookie(Response response, HttpCookie cookie) } catch (ComplianceViolationException e) { - request.getComponents().getComplianceViolationListener().onComplianceViolation(e.getEvent()); + HttpChannel.from(request).getComplianceViolationListener().onComplianceViolation(e.getEvent()); throw e; } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java index 4e74a2a1a708..bed6d6fbcffa 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Server.java @@ -29,7 +29,6 @@ import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.TimeUnit; -import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.DateGenerator; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; @@ -152,18 +151,6 @@ public Server(@Name("threadPool") ThreadPool threadPool, @Name("scheduler") Sche addBean(FileSystemPool.INSTANCE, false); } - /** - * Get a new ComplianceViolation.Listener suitable for given Connector. - * - * @param connector the connector to base the ComplianceViolation.Listener off of. - * @return the ComplianceViolation.Listener implementation, or null if {@link HttpConnectionFactory#isRecordHttpComplianceViolations()} is false, - * or there are no ComplianceViolation.Listener implementations registered. - */ - public static ComplianceViolation.Listener getComplianceViolationListener(Connector connector) - { - return connector instanceof AbstractConnector abstractConnector ? abstractConnector.getComplianceViolationListener() : ComplianceViolation.Listener.NOOP; - } - public Handler getDefaultHandler() { return _defaultHandler; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index d6dace0d0303..fcf29ddb799a 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -16,7 +16,9 @@ import java.io.IOException; import java.nio.ByteBuffer; import java.nio.channels.WritePendingException; +import java.util.ArrayList; import java.util.HashMap; +import java.util.List; import java.util.Objects; import java.util.concurrent.CompletableFuture; import java.util.concurrent.TimeoutException; @@ -132,13 +134,17 @@ public HttpChannelState(ConnectionMetaData connectionMetaData) @Override public void init() { - _complianceViolationListener = Server.getComplianceViolationListener(_connectionMetaData.getConnector()).initialize(); - } + List listeners = _connectionMetaData.getHttpConfiguration().getComplianceViolationListeners(); - @Override - public Components getComponents() - { - return this; + ComplianceViolation.Listener listener; + if (listeners.isEmpty()) + listener = ComplianceViolation.Listener.NOOP; + else if (listeners.size() == 1) + listener = listeners.get(0); + else + listener = new CompositeComplianceViolationListener(listeners); + + _complianceViolationListener = listener.initialize(); } @Override @@ -593,7 +599,7 @@ public void run() HttpURI uri = request.getHttpURI(); if (uri.hasViolations()) { - String badMessage = UriCompliance.checkUriCompliance(getConnectionMetaData().getHttpConfiguration().getUriCompliance(), uri, request.getComponents().getComplianceViolationListener()); + String badMessage = UriCompliance.checkUriCompliance(getConnectionMetaData().getHttpConfiguration().getUriCompliance(), uri, HttpChannel.from(request).getComplianceViolationListener()); if (badMessage != null) throw new BadMessageException(badMessage); } @@ -1763,4 +1769,97 @@ protected void onError(Runnable task, Throwable failure) failureTask.run(); } } + + /** + * A Listener that represents multiple user {@link ComplianceViolation.Listener} instances + */ + private static class CompositeComplianceViolationListener implements ComplianceViolation.Listener + { + private static final Logger LOG = LoggerFactory.getLogger(CompositeComplianceViolationListener.class); + private final List userListeners; + + /** + * Construct a new ComplianceViolations that will notify user listeners. + * + * @param userListeners the user listeners to notify, null or empty is allowed. + */ + public CompositeComplianceViolationListener(List userListeners) + { + this.userListeners = userListeners; + } + + @Override + public void onRequestEnd(Attributes request) + { + for (ComplianceViolation.Listener listener : userListeners) + { + try + { + listener.onRequestEnd(request); + } + catch (Exception e) + { + LOG.warn("Unable to notify ComplianceViolation.Listener implementation at {} of onRequestEnd {}", listener, request, e); + } + } + } + + @Override + public void onRequestBegin(Attributes request) + { + for (ComplianceViolation.Listener listener : userListeners) + { + try + { + listener.onRequestBegin(request); + } + catch (Exception e) + { + LOG.warn("Unable to notify ComplianceViolation.Listener implementation at {} of onRequestBegin {}", listener, request, e); + } + } + } + + @Override + public ComplianceViolation.Listener initialize() + { + List initialized = null; + for (ComplianceViolation.Listener listener : userListeners) + { + ComplianceViolation.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 CompositeComplianceViolationListener(initialized); + } + + @Override + public void onComplianceViolation(ComplianceViolation.Event event) + { + assert event != null; + 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); + } + } + } + } } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index dacba8b7c6c5..c46ba3f40380 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -983,7 +983,7 @@ public boolean contentComplete() @Override public void onViolation(ComplianceViolation.Event event) { - getHttpChannel().getComponents().getComplianceViolationListener().onComplianceViolation(event); + getHttpChannel().getComplianceViolationListener().onComplianceViolation(event); } @Override @@ -1005,7 +1005,7 @@ public boolean messageComplete() else stream._chunk = Content.Chunk.EOF; - getHttpChannel().getComponents().getComplianceViolationListener().onRequestBegin(getHttpChannel().getRequest()); + getHttpChannel().getComplianceViolationListener().onRequestBegin(getHttpChannel().getRequest()); return false; } @@ -1015,7 +1015,7 @@ public void badMessage(HttpException failure) if (LOG.isDebugEnabled()) LOG.debug("badMessage {} {}", HttpConnection.this, failure); - getHttpChannel().getComponents().getComplianceViolationListener().onRequestEnd(getHttpChannel().getRequest()); + getHttpChannel().getComplianceViolationListener().onRequestEnd(getHttpChannel().getRequest()); _failure = (Throwable)failure; _generator.setPersistent(false); @@ -1185,7 +1185,7 @@ public Runnable headerComplete() if (_uri.hasViolations()) { compliance = getHttpConfiguration().getUriCompliance(); - String badMessage = UriCompliance.checkUriCompliance(compliance, _uri, getHttpChannel().getComponents().getComplianceViolationListener()); + String badMessage = UriCompliance.checkUriCompliance(compliance, _uri, getHttpChannel().getComplianceViolationListener()); if (badMessage != null) throw new BadMessageException(badMessage); } @@ -1200,7 +1200,7 @@ public Runnable headerComplete() HttpCompliance httpCompliance = getHttpConfiguration().getHttpCompliance(); if (httpCompliance.allows(MISMATCHED_AUTHORITY)) { - getHttpChannel().getComponents().getComplianceViolationListener().onComplianceViolation(new ComplianceViolation.Event(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString())); + getHttpChannel().getComplianceViolationListener().onComplianceViolation(new ComplianceViolation.Event(httpCompliance, MISMATCHED_AUTHORITY, _uri.asString())); } else throw new BadMessageException("Authority!=Host"); @@ -1246,7 +1246,7 @@ public boolean is100ContinueExpected() ++_requests; Request request = _httpChannel.getRequest(); - getHttpChannel().getComponents().getComplianceViolationListener().onRequestBegin(request); + getHttpChannel().getComplianceViolationListener().onRequestBegin(request); if (_complianceViolations != null && !_complianceViolations.isEmpty()) { diff --git a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java index 18e84dab10f9..d22f562142dc 100644 --- a/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java +++ b/jetty-core/jetty-server/src/test/java/org/eclipse/jetty/server/ServerHttpCookieTest.java @@ -64,7 +64,7 @@ public boolean handle(Request request, Response response, Callback callback) thr Fields.Field setCookie = parameters.get("SetCookie"); if (setCookie != null) { - ComplianceViolation.Listener complianceViolationListener = request.getComponents().getComplianceViolationListener(); + ComplianceViolation.Listener complianceViolationListener = HttpChannel.from(request).getComplianceViolationListener(); CookieParser parser = CookieParser.newParser((name, value, version, domain, path, comment) -> Response.addCookie(response, HttpCookie.build(name, value, version).domain(domain).path(path).comment(comment).build()), RFC2965, complianceViolationListener); parser.parseField(setCookie.getValue()); diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java index 916d94c19103..ea5fcbabf69c 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Dispatcher.java @@ -246,7 +246,7 @@ private static void checkUriViolations(HttpURI uri, Request baseRequest) { HttpChannel channel = baseRequest.getHttpChannel(); UriCompliance compliance = channel == null || channel.getHttpConfiguration() == null ? null : channel.getHttpConfiguration().getUriCompliance(); - String illegalState = UriCompliance.checkUriCompliance(compliance, uri, baseRequest.getCoreRequest().getComponents().getComplianceViolationListener()); + String illegalState = UriCompliance.checkUriCompliance(compliance, uri, org.eclipse.jetty.server.HttpChannel.from(baseRequest.getCoreRequest()).getComplianceViolationListener()); if (illegalState != null) throw new IllegalStateException(illegalState); } diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java index ff197e5b4c44..f9bbcf90cf56 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/Request.java @@ -538,7 +538,7 @@ public HttpChannelState getHttpChannelState() @Deprecated(since = "12.0.6", forRemoval = true) public ComplianceViolation.Listener getComplianceViolationListener() { - return getCoreRequest().getComponents().getComplianceViolationListener(); + return org.eclipse.jetty.server.HttpChannel.from(getCoreRequest()).getComplianceViolationListener(); } /** @@ -1986,7 +1986,7 @@ else if (getCharacterEncoding() != null) private void reportComplianceViolations() { - ComplianceViolation.Listener complianceViolationListener = getCoreRequest().getComponents().getComplianceViolationListener(); + ComplianceViolation.Listener complianceViolationListener = org.eclipse.jetty.server.HttpChannel.from(getCoreRequest()).getComplianceViolationListener(); List nonComplianceWarnings = _multiParts.getNonComplianceWarnings(); for (MultiPartFormInputStream.NonCompliance nc : nonComplianceWarnings) complianceViolationListener.onComplianceViolation(new ComplianceViolation.Event(nc.mode(), nc.violation(), nc.detail())); From b4f4003e0e8e2a2fabd286b973e9d008a9c304fa Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 23 Jan 2024 12:39:02 -0600 Subject: [PATCH 25/31] Fixing build --- .../jetty/server/AbstractConnector.java | 1 + .../jetty/server/HttpConnectionFactory.java | 23 ++++++------------- 2 files changed, 8 insertions(+), 16 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java index 84da2a3c2b43..e74a5f86e6e0 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/AbstractConnector.java @@ -31,6 +31,7 @@ import java.util.concurrent.locks.Condition; import java.util.stream.Collectors; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.io.ArrayByteBufferPool; import org.eclipse.jetty.io.ByteBufferPool; import org.eclipse.jetty.io.EndPoint; diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java index 2d72d143afeb..1f40920dab14 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java @@ -13,7 +13,6 @@ package org.eclipse.jetty.server; -import java.util.Collection; import java.util.Objects; import org.eclipse.jetty.http.ComplianceViolation; @@ -31,7 +30,7 @@ * {@link HttpConnection}s are configured by a {@link HttpConfiguration} instance that is either created by * default or passed in to the constructor. */ -public class HttpConnectionFactory extends AbstractConnectionFactory implements HttpConfiguration.ConnectionFactory, ConnectionFactory.Configuring +public class HttpConnectionFactory extends AbstractConnectionFactory implements HttpConfiguration.ConnectionFactory { private static final Logger LOG = LoggerFactory.getLogger(HttpConnectionFactory.class); private final HttpConfiguration _config; @@ -58,33 +57,25 @@ public HttpConfiguration getHttpConfiguration() return _config; } - @Override - public void configure(Connector connector) - { - Collection userListeners = getBeans(ComplianceViolation.Listener.class); - if (userListeners != null && !userListeners.isEmpty()) - for (ComplianceViolation.Listener listener: userListeners) - LOG.warn("Connector based ComplianceViolation.Listener {} not used from Beans, use HttpConfiguration.addComplianceViolationListener() instead", listener); - } - /** - * @return always returns false - * @deprecated use {@link HttpConfiguration#getComplianceViolationListeners()} instead to know if there are any {@link ComplianceViolation.Listener} to notify. this method will be removed in Jetty 12.1.0 + * @deprecated use {@link HttpConfiguration#getComplianceViolationListeners()} instead to know if there + * are any {@link ComplianceViolation.Listener} to notify. this method will be removed in Jetty 12.1.0 */ @Deprecated(since = "12.0.5", forRemoval = true) public boolean isRecordHttpComplianceViolations() { - return false; + return !_config.getComplianceViolationListeners().isEmpty(); } /** * Does nothing. - * @deprecated use {@link HttpConfiguration#addComplianceViolationListener(ComplianceViolation.Listener)} instead. this method will be removed in Jetty 12.1.0 + * @deprecated use {@link HttpConfiguration#addComplianceViolationListener(ComplianceViolation.Listener)} instead. + * this method will be removed in Jetty 12.1.0 */ @Deprecated(since = "12.0.5", forRemoval = true) public void setRecordHttpComplianceViolations(boolean recordHttpComplianceViolations) { - // no-op + _config.addComplianceViolationListener(new ComplianceViolation.LoggingListener()); } public boolean isUseInputDirectByteBuffers() From 823b0a1ff01940e81081531264ef0671ad8ccfd7 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 23 Jan 2024 13:12:07 -0600 Subject: [PATCH 26/31] Revert RequestHandler change --- .../org/eclipse/jetty/server/internal/HttpConnection.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index c46ba3f40380..b0f10a9e141f 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -193,7 +193,7 @@ protected HttpStreamOverHTTP1 newHttpStream(String method, String uri, HttpVersi protected RequestHandler newRequestHandler() { - return new RequestHandler(getConnector()); + return new RequestHandler(); } public Server getServer() @@ -924,10 +924,6 @@ protected class RequestHandler implements HttpParser.RequestHandler { private Throwable _failure; - public RequestHandler(Connector connector) - { - } - @Override public void messageBegin() { From 34a67e2045766dfb8086e75b09e66da2e624da12 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 23 Jan 2024 13:41:17 -0600 Subject: [PATCH 27/31] Support ComplianceViolation.Listener in ee10 multipart/form --- .../eclipse/jetty/http/MultiPartFormData.java | 20 +++++++++++++++---- .../servlet/ServletMultiPartFormData.java | 7 ++++++- 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java index af8699dcc5a5..7e3b43efd664 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/MultiPartFormData.java @@ -77,13 +77,22 @@ private MultiPartFormData() { } + /** + * @deprecated use {@link #from(Attributes, ComplianceViolation.Listener, String, Function)} instead. This method will be removed in Jetty 12.1.0 + */ + @Deprecated(since = "12.0.6", forRemoval = true) public static CompletableFuture from(Attributes attributes, String boundary, Function> parse) + { + return from(attributes, ComplianceViolation.Listener.NOOP, boundary, parse); + } + + public static CompletableFuture from(Attributes attributes, ComplianceViolation.Listener listener, String boundary, Function> parse) { @SuppressWarnings("unchecked") CompletableFuture futureParts = (CompletableFuture)attributes.getAttribute(MultiPartFormData.class.getName()); if (futureParts == null) { - futureParts = parse.apply(new Parser(boundary)); + futureParts = parse.apply(new Parser(boundary, listener)); attributes.setAttribute(MultiPartFormData.class.getName(), futureParts); } return futureParts; @@ -200,9 +209,6 @@ 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 violations instead? - // that the ee10 layer could use? private ComplianceViolation.Listener complianceViolationListener; private boolean useFilesForPartsWithoutFileName; private Path filesDirectory; @@ -213,8 +219,14 @@ public static class Parser private Parts parts; public Parser(String boundary) + { + this(boundary, null); + } + + public Parser(String boundary, ComplianceViolation.Listener complianceViolationListener) { parser = new MultiPart.Parser(Objects.requireNonNull(boundary), listener); + this.complianceViolationListener = complianceViolationListener != null ? complianceViolationListener : ComplianceViolation.Listener.NOOP; } public CompletableFuture parse(Content.Source content) diff --git a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java index 90a6e5ba884d..2eb0db801797 100644 --- a/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java +++ b/jetty-ee10/jetty-ee10-servlet/src/main/java/org/eclipse/jetty/ee10/servlet/ServletMultiPartFormData.java @@ -26,6 +26,7 @@ import jakarta.servlet.MultipartConfigElement; import jakarta.servlet.ServletRequest; import jakarta.servlet.http.Part; +import org.eclipse.jetty.http.ComplianceViolation; import org.eclipse.jetty.http.HttpField; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.MimeTypes; @@ -37,6 +38,7 @@ import org.eclipse.jetty.io.Content; import org.eclipse.jetty.io.content.InputStreamContentSource; import org.eclipse.jetty.server.ConnectionMetaData; +import org.eclipse.jetty.server.HttpChannel; import org.eclipse.jetty.util.StringUtil; /** @@ -100,8 +102,11 @@ public static CompletableFuture from(ServletRequest servletRequest, Strin ? servletContextRequest.getContext().getTempDirectory() : new File(config.getLocation()); + HttpChannel httpChannel = HttpChannel.from(servletContextRequest); + ComplianceViolation.Listener complianceViolationListener = httpChannel.getComplianceViolationListener(); + // Look for an existing future MultiPartFormData.Parts - CompletableFuture futureFormData = MultiPartFormData.from(servletContextRequest, boundary, parser -> + CompletableFuture futureFormData = MultiPartFormData.from(servletContextRequest, complianceViolationListener, boundary, parser -> { try { From 1177674293cfc3dafbe7eec1ec5e5e6454bc2854 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 23 Jan 2024 13:41:35 -0600 Subject: [PATCH 28/31] Correct since versions --- .../main/java/org/eclipse/jetty/http/ComplianceViolation.java | 2 +- .../src/main/java/org/eclipse/jetty/http/HttpCompliance.java | 2 +- .../java/org/eclipse/jetty/server/HttpConnectionFactory.java | 4 ++-- .../org/eclipse/jetty/server/internal/HttpConnection.java | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java index 60743c761f3c..938dbd108a1b 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/ComplianceViolation.java @@ -143,7 +143,7 @@ default void onComplianceViolation(Event event) * @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) + @Deprecated(since = "12.0.6", forRemoval = true) default void onComplianceViolation(Mode mode, ComplianceViolation violation, String details) { } diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java index fc8455771101..02b514935626 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpCompliance.java @@ -163,7 +163,7 @@ public String getDescription() * 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.) */ - @Deprecated(since = "12.0.5", forRemoval = true) + @Deprecated(since = "12.0.6", forRemoval = true) public static final String VIOLATIONS_ATTR = ComplianceViolation.CapturingListener.VIOLATIONS_ATTR_KEY; /** diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java index 1f40920dab14..82b373364c15 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpConnectionFactory.java @@ -61,7 +61,7 @@ public HttpConfiguration getHttpConfiguration() * @deprecated use {@link HttpConfiguration#getComplianceViolationListeners()} instead to know if there * are any {@link ComplianceViolation.Listener} to notify. this method will be removed in Jetty 12.1.0 */ - @Deprecated(since = "12.0.5", forRemoval = true) + @Deprecated(since = "12.0.6", forRemoval = true) public boolean isRecordHttpComplianceViolations() { return !_config.getComplianceViolationListeners().isEmpty(); @@ -72,7 +72,7 @@ public boolean isRecordHttpComplianceViolations() * @deprecated use {@link HttpConfiguration#addComplianceViolationListener(ComplianceViolation.Listener)} instead. * this method will be removed in Jetty 12.1.0 */ - @Deprecated(since = "12.0.5", forRemoval = true) + @Deprecated(since = "12.0.6", forRemoval = true) public void setRecordHttpComplianceViolations(boolean recordHttpComplianceViolations) { _config.addComplianceViolationListener(new ComplianceViolation.LoggingListener()); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index b0f10a9e141f..906aba1dc999 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -134,7 +134,7 @@ protected static HttpConnection setCurrentConnection(HttpConnection connection) /** * @deprecated use {@link #HttpConnection(HttpConfiguration, Connector, EndPoint)} instead. Will be removed in Jetty 12.1.0 */ - @Deprecated(since = "12.0.5", forRemoval = true) + @Deprecated(since = "12.0.6", forRemoval = true) public HttpConnection(HttpConfiguration configuration, Connector connector, EndPoint endPoint, boolean recordComplianceViolations) { this(configuration, connector, endPoint); @@ -162,7 +162,7 @@ public InvocationType getInvocationType() /** * @deprecated No replacement, no longer used within {@link HttpConnection}, will be removed in Jetty 12.1.0 */ - @Deprecated(since = "12.0.5", forRemoval = true) + @Deprecated(since = "12.0.6", forRemoval = true) public boolean isRecordHttpComplianceViolations() { return false; From 5e275dd4bc260101d604d9469c719d13e1d239d8 Mon Sep 17 00:00:00 2001 From: Joakim Erdfelt Date: Tue, 23 Jan 2024 16:50:30 -0600 Subject: [PATCH 29/31] Rename HttpChannel.init() to .initialize() --- .../jetty/http2/server/internal/HTTP2ServerConnection.java | 2 +- .../http3/server/internal/ServerHTTP3StreamConnection.java | 2 +- .../src/main/java/org/eclipse/jetty/server/HttpChannel.java | 4 ++-- .../org/eclipse/jetty/server/internal/HttpChannelState.java | 2 +- .../org/eclipse/jetty/server/internal/HttpConnection.java | 2 +- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java index 8925643b1686..8446dcea492b 100644 --- a/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java +++ b/jetty-core/jetty-http2/jetty-http2-server/src/main/java/org/eclipse/jetty/http2/server/internal/HTTP2ServerConnection.java @@ -258,7 +258,7 @@ private HttpChannel pollHttpChannel() httpChannel = httpChannels.poll(); if (httpChannel == null) httpChannel = httpChannelFactory.newHttpChannel(this); - httpChannel.init(); + httpChannel.initialize(); return httpChannel; } diff --git a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/ServerHTTP3StreamConnection.java b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/ServerHTTP3StreamConnection.java index 18814f8c1450..215f95f3e527 100644 --- a/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/ServerHTTP3StreamConnection.java +++ b/jetty-core/jetty-http3/jetty-http3-server/src/main/java/org/eclipse/jetty/http3/server/internal/ServerHTTP3StreamConnection.java @@ -53,7 +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(); + httpChannel.initialize(); HttpStreamOverHTTP3 httpStream = new HttpStreamOverHTTP3(this, httpChannel, stream); httpChannel.setHttpStream(httpStream); stream.setAttachment(httpStream); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 7c0b0d5d9dad..493902872583 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -101,7 +101,7 @@ public interface HttpChannel extends Invocable /** * Recycle the HttpChannel, so that a new cycle of calling {@link #setHttpStream(HttpStream)}, * {@link #onRequest(MetaData.Request)} etc. may be performed on the channel. - * @see #init() + * @see #initialize() */ void recycle(); @@ -109,7 +109,7 @@ public interface HttpChannel extends Invocable * Initialize the HttpChannel when a new cycle of request handling begins. * @see #recycle() */ - void init(); + void initialize(); /** * @return the active {@link ComplianceViolation.Listener} diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index fcf29ddb799a..f62ed8c1826c 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -132,7 +132,7 @@ public HttpChannelState(ConnectionMetaData connectionMetaData) } @Override - public void init() + public void initialize() { List listeners = _connectionMetaData.getHttpConfiguration().getComplianceViolationListeners(); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 906aba1dc999..1d938ee8f6f2 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -927,7 +927,7 @@ protected class RequestHandler implements HttpParser.RequestHandler @Override public void messageBegin() { - _httpChannel.init(); + _httpChannel.initialize(); } @Override From 80f8322505364fbc36849f6d74e724c7a438f706 Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 24 Jan 2024 11:24:57 +0900 Subject: [PATCH 30/31] avoid double allocation of compliance listener list --- .../server/internal/HttpChannelState.java | 75 +++++++++---------- 1 file changed, 35 insertions(+), 40 deletions(-) diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java index f62ed8c1826c..06b35d83832a 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpChannelState.java @@ -135,16 +135,12 @@ public HttpChannelState(ConnectionMetaData connectionMetaData) public void initialize() { List listeners = _connectionMetaData.getHttpConfiguration().getComplianceViolationListeners(); - - ComplianceViolation.Listener listener; - if (listeners.isEmpty()) - listener = ComplianceViolation.Listener.NOOP; - else if (listeners.size() == 1) - listener = listeners.get(0); - else - listener = new CompositeComplianceViolationListener(listeners); - - _complianceViolationListener = listener.initialize(); + _complianceViolationListener = switch (listeners.size()) + { + case 0 -> ComplianceViolation.Listener.NOOP; + case 1 -> listeners.get(0).initialize(); + default -> new InitializedCompositeComplianceViolationListener(listeners); + }; } @Override @@ -1773,25 +1769,43 @@ protected void onError(Runnable task, Throwable failure) /** * A Listener that represents multiple user {@link ComplianceViolation.Listener} instances */ - private static class CompositeComplianceViolationListener implements ComplianceViolation.Listener + private static class InitializedCompositeComplianceViolationListener implements ComplianceViolation.Listener { - private static final Logger LOG = LoggerFactory.getLogger(CompositeComplianceViolationListener.class); - private final List userListeners; + private static final Logger LOG = LoggerFactory.getLogger(InitializedCompositeComplianceViolationListener.class); + private final List _listeners; /** - * Construct a new ComplianceViolations that will notify user listeners. + * Construct a new ComplianceViolations that will initialize the list of listeners and notify events to all. * - * @param userListeners the user listeners to notify, null or empty is allowed. + * @param unInitializedListeners the user listeners to initialized and notify. Null or empty list is not allowed.. */ - public CompositeComplianceViolationListener(List userListeners) + public InitializedCompositeComplianceViolationListener(List unInitializedListeners) { - this.userListeners = userListeners; + List initialized = null; + for (ComplianceViolation.Listener listener : unInitializedListeners) + { + ComplianceViolation.Listener listening = listener.initialize(); + if (listening != listener) + { + initialized = new ArrayList<>(unInitializedListeners.size()); + for (ComplianceViolation.Listener l : unInitializedListeners) + { + if (l == listener) + break; + initialized.add(l); + } + } + if (initialized != null) + initialized.add(listening); + } + + _listeners = initialized == null ? unInitializedListeners : initialized; } @Override public void onRequestEnd(Attributes request) { - for (ComplianceViolation.Listener listener : userListeners) + for (ComplianceViolation.Listener listener : _listeners) { try { @@ -1807,7 +1821,7 @@ public void onRequestEnd(Attributes request) @Override public void onRequestBegin(Attributes request) { - for (ComplianceViolation.Listener listener : userListeners) + for (ComplianceViolation.Listener listener : _listeners) { try { @@ -1823,33 +1837,14 @@ public void onRequestBegin(Attributes request) @Override public ComplianceViolation.Listener initialize() { - List initialized = null; - for (ComplianceViolation.Listener listener : userListeners) - { - ComplianceViolation.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 CompositeComplianceViolationListener(initialized); + throw new IllegalStateException("already initialized"); } @Override public void onComplianceViolation(ComplianceViolation.Event event) { assert event != null; - for (ComplianceViolation.Listener listener : userListeners) + for (ComplianceViolation.Listener listener : _listeners) { try { From fdc0cd82c55e5ac542641d09ae00f91fc00321ae Mon Sep 17 00:00:00 2001 From: gregw Date: Wed, 24 Jan 2024 11:47:43 +0900 Subject: [PATCH 31/31] Avoid request specific listener in cookie cache --- .../org/eclipse/jetty/http/CookieCache.java | 27 ++++++++++++++++--- .../org/eclipse/jetty/server/HttpChannel.java | 11 +------- .../org/eclipse/jetty/server/Request.java | 5 ++-- 3 files changed, 26 insertions(+), 17 deletions(-) diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCache.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCache.java index f6c37d860af3..5e6c34961b62 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCache.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/CookieCache.java @@ -31,21 +31,30 @@ * cookies are not re-parsed. * */ -public class CookieCache implements CookieParser.Handler +public class CookieCache implements CookieParser.Handler, ComplianceViolation.Listener { protected static final Logger LOG = LoggerFactory.getLogger(CookieCache.class); protected final List _rawFields = new ArrayList<>(); protected List _cookieList; private final CookieParser _parser; + private List _violations; public CookieCache() { - this(CookieCompliance.RFC6265, null); + this(CookieCompliance.RFC6265); } - public CookieCache(CookieCompliance compliance, ComplianceViolation.Listener complianceListener) + public CookieCache(CookieCompliance compliance) { - _parser = CookieParser.newParser(this, compliance, complianceListener); + _parser = CookieParser.newParser(this, compliance, this); + } + + @Override + public void onComplianceViolation(ComplianceViolation.Event event) + { + if (_violations == null) + _violations = new ArrayList<>(); + _violations.add(event); } @Override @@ -67,6 +76,11 @@ public void addCookie(String cookieName, String cookieValue, int cookieVersion, } public List getCookies(HttpFields headers) + { + return getCookies(headers, ComplianceViolation.Listener.NOOP); + } + + public List getCookies(HttpFields headers, ComplianceViolation.Listener complianceViolationListener) { boolean building = false; ListIterator raw = _rawFields.listIterator(); @@ -136,6 +150,8 @@ public List getCookies(HttpFields headers) _cookieList = new ArrayList<>(); try { + if (_violations != null) + _violations.clear(); _parser.parseFields(_rawFields); } catch (CookieParser.InvalidCookieException invalidCookieException) @@ -144,6 +160,9 @@ public List getCookies(HttpFields headers) } } + if (_violations != null && !_violations.isEmpty()) + _violations.forEach(complianceViolationListener::onComplianceViolation); + return _cookieList == null ? Collections.emptyList() : _cookieList; } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java index 493902872583..3d565d51ca02 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/HttpChannel.java @@ -123,17 +123,8 @@ public interface HttpChannel extends Invocable */ static HttpChannel from(Request request) { - while (true) - { - if (request instanceof Request.Wrapper wrapper) - request = wrapper.getWrapped(); - else - break; - } - - if (request.getComponents() instanceof HttpChannel httpChannel) + if (Request.unWrap(request).getComponents() instanceof HttpChannel httpChannel) return httpChannel; - throw new IllegalStateException("Unable to find HttpChannel from " + request); } diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java index 027f6651a3e4..9ea357ce6c3b 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/Request.java @@ -589,12 +589,11 @@ static List getCookies(Request request) CookieCache cookieCache = (CookieCache)request.getComponents().getCache().getAttribute(CACHE_ATTRIBUTE); if (cookieCache == null) { - // TODO: CookieCache is per connection, needs to be per request for ComplianceViolation.Listener - cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance(), HttpChannel.from(request).getComplianceViolationListener()); + cookieCache = new CookieCache(request.getConnectionMetaData().getHttpConfiguration().getRequestCookieCompliance()); request.getComponents().getCache().setAttribute(CACHE_ATTRIBUTE, cookieCache); } - cookies = cookieCache.getCookies(request.getHeaders()); + cookies = cookieCache.getCookies(request.getHeaders(), HttpChannel.from(request).getComplianceViolationListener()); request.setAttribute(COOKIE_ATTRIBUTE, cookies); return cookies; }