diff --git a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/EventSourceConfig.java b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/EventSourceConfig.java index f8a8700a9d..78daf209b2 100644 --- a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/EventSourceConfig.java +++ b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/EventSourceConfig.java @@ -29,6 +29,7 @@ import java.util.Objects; import org.apache.hc.client5.http.sse.impl.ExponentialJitterBackoff; +import org.apache.hc.core5.util.Args; /** * Immutable configuration for {@link EventSource} behavior, primarily covering @@ -43,6 +44,8 @@ * The strategy can also honor server hints (SSE {@code retry:} field or HTTP {@code Retry-After}). *
  • {@link #maxReconnects}: maximum number of reconnect attempts before giving up. * A value of {@code -1} means unlimited attempts.
  • + *
  • {@link #maxLineLength}: maximum length of a single line of the stream.
  • + *
  • {@link #maxEventSize}: maximum size of the data accumulated for a single event.
  • * * *

    Thread-safety

    @@ -74,20 +77,55 @@ public final class EventSourceConfig { */ public final int maxReconnects; + /** + * Default value of {@link #maxLineLength}, 1 MiB. + * + * @since 5.7 + */ + public static final int DEFAULT_MAX_LINE_LENGTH = 1024 * 1024; + + /** + * Default value of {@link #maxEventSize}, 8 MiB. + * + * @since 5.7 + */ + public static final int DEFAULT_MAX_EVENT_SIZE = 8 * 1024 * 1024; + + /** + * Maximum length of a single line, excluding the line terminator. Counted in characters + * by the char parser and in bytes by the byte parser. A longer line fails the stream. + * + * @since 5.7 + */ + public final int maxLineLength; + + /** + * Maximum size of the {@code data:} content accumulated for one event before the + * terminating blank line. Counted in characters. A larger event fails the stream. + * + * @since 5.7 + */ + public final int maxEventSize; + /** * Default configuration: * * * @since 5.7 */ public static final EventSourceConfig DEFAULT = builder().build(); - private EventSourceConfig(final BackoffStrategy backoff, final int maxReconnects) { + private EventSourceConfig(final BackoffStrategy backoff, final int maxReconnects, + final int maxLineLength, final int maxEventSize) { this.backoff = backoff; this.maxReconnects = maxReconnects; + this.maxLineLength = maxLineLength; + this.maxEventSize = maxEventSize; } /** @@ -111,6 +149,8 @@ public static final class Builder { private BackoffStrategy backoff = new ExponentialJitterBackoff(1_000L, 30_000L, 2.0, 250L); private int maxReconnects = -1; + private int maxLineLength = DEFAULT_MAX_LINE_LENGTH; + private int maxEventSize = DEFAULT_MAX_EVENT_SIZE; /** * Sets the reconnect/backoff strategy. @@ -139,6 +179,30 @@ public Builder maxReconnects(final int nmaxReconnects) { return this; } + /** + * Sets the maximum length of a single line. + * + * @param maxLineLength maximum line length, must be positive + * @return this builder + * @since 5.7 + */ + public Builder maxLineLength(final int maxLineLength) { + this.maxLineLength = Args.positive(maxLineLength, "Max line length"); + return this; + } + + /** + * Sets the maximum size of the data accumulated for a single event. + * + * @param maxEventSize maximum event size, must be positive + * @return this builder + * @since 5.7 + */ + public Builder maxEventSize(final int maxEventSize) { + this.maxEventSize = Args.positive(maxEventSize, "Max event size"); + return this; + } + /** * Builds an immutable {@link EventSourceConfig}. * @@ -146,7 +210,7 @@ public Builder maxReconnects(final int nmaxReconnects) { * @since 5.7 */ public EventSourceConfig build() { - return new EventSourceConfig(backoff, maxReconnects); + return new EventSourceConfig(backoff, maxReconnects, maxLineLength, maxEventSize); } } } diff --git a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/ByteSseEntityConsumer.java b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/ByteSseEntityConsumer.java index d346c84446..e415df2edc 100644 --- a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/ByteSseEntityConsumer.java +++ b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/ByteSseEntityConsumer.java @@ -30,10 +30,13 @@ import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; +import org.apache.hc.client5.http.sse.EventSourceConfig; import org.apache.hc.core5.annotation.Internal; import org.apache.hc.core5.http.ContentType; import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.MessageConstraintException; import org.apache.hc.core5.http.nio.entity.AbstractBinAsyncEntityConsumer; +import org.apache.hc.core5.util.Args; /** * Low-allocation binary consumer for Server-Sent Events (SSE). @@ -52,6 +55,8 @@ *
  • Implements WHATWG SSE fields: {@code data}, {@code id}, {@code event}, {@code retry}. * Unknown fields and malformed {@code retry} values are ignored.
  • *
  • At end of stream, discards any incomplete line or event.
  • + *
  • Fails the stream with {@link MessageConstraintException} if a line exceeds the maximum + * line length in bytes or an event accumulates more than the maximum event size.
  • * * *

    Thread-safety

    @@ -71,6 +76,8 @@ public final class ByteSseEntityConsumer extends AbstractBinAsyncEntityConsumer< private static final byte SPACE = (byte) ' '; private final SseCallbacks cb; + private final int maxLineLength; + private final int maxEventSize; // line accumulator private byte[] lineBuf = new byte[256]; @@ -88,7 +95,13 @@ public final class ByteSseEntityConsumer extends AbstractBinAsyncEntityConsumer< private boolean bomDone = false; // once true, no further BOM detection public ByteSseEntityConsumer(final SseCallbacks callbacks) { + this(callbacks, EventSourceConfig.DEFAULT_MAX_LINE_LENGTH, EventSourceConfig.DEFAULT_MAX_EVENT_SIZE); + } + + public ByteSseEntityConsumer(final SseCallbacks callbacks, final int maxLineLength, final int maxEventSize) { this.cb = callbacks; + this.maxLineLength = Args.positive(maxLineLength, "Max line length"); + this.maxEventSize = Args.positive(maxEventSize, "Max event size"); } @Override @@ -101,7 +114,16 @@ public void streamStart(final ContentType contentType) throws HttpException, IOE } @Override - protected void data(final ByteBuffer src, final boolean endOfStream) { + protected void data(final ByteBuffer src, final boolean endOfStream) throws IOException { + try { + consumeBytes(src, endOfStream); + } catch (final MessageConstraintException ex) { + releaseResources(); + throw ex; + } + } + + private void consumeBytes(final ByteBuffer src, final boolean endOfStream) throws IOException { if (!bomDone) { while (src.hasRemaining() && bomMatched < 3) { final int expected = (bomMatched == 0) ? 0xEF : (bomMatched == 1 ? 0xBB : 0xBF); @@ -142,7 +164,7 @@ protected void data(final ByteBuffer src, final boolean endOfStream) { } } - private void processByte(final byte b) { + private void processByte(final byte b) throws IOException { if (b == LF) { if (pendingCr) { // LF completing a CRLF pair; the line was already emitted on the CR. @@ -168,7 +190,10 @@ private void flushEndOfStream() { type = null; } - private void appendByte(final byte b) { + private void appendByte(final byte b) throws MessageConstraintException { + if (lineLen >= maxLineLength) { + throw new MessageConstraintException("Maximum line length exceeded: " + maxLineLength); + } ensureCapacity(lineLen + 1); lineBuf[lineLen++] = b; } @@ -189,13 +214,14 @@ public void releaseResources() { lineLen = 0; pendingCr = false; data.setLength(0); + data.trimToSize(); id = null; type = null; bomMatched = 0; bomDone = false; } - private void handleLine(final byte[] buf, final int len) { + private void handleLine(final byte[] buf, final int len) throws IOException { if (len == 0) { dispatch(); return; @@ -222,6 +248,9 @@ private void handleLine(final byte[] buf, final int len) { // Compare ASCII field name without allocations if (fLen == 4 && buf[0] == 'd' && buf[1] == 'a' && buf[2] == 't' && buf[3] == 'a') { final String v = new String(buf, vStart, len - vStart, StandardCharsets.UTF_8); + if ((long) data.length() + v.length() + 1 > maxEventSize) { + throw new MessageConstraintException("Maximum event size exceeded: " + maxEventSize); + } data.append(v).append('\n'); } else if (fLen == 5 && buf[0] == 'e' && buf[1] == 'v' && buf[2] == 'e' && buf[3] == 'n' && buf[4] == 't') { type = new String(buf, vStart, len - vStart, StandardCharsets.UTF_8); diff --git a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/DefaultEventSource.java b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/DefaultEventSource.java index 4a1f8b049a..a6152519dd 100644 --- a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/DefaultEventSource.java +++ b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/DefaultEventSource.java @@ -119,6 +119,8 @@ public final class DefaultEventSource implements EventSource { private final Executor callbackExecutor; private final BackoffStrategy backoff; private final int maxReconnects; + private final int maxLineLength; + private final int maxEventSize; private final SseParser parser; private final AtomicBoolean started = new AtomicBoolean(false); @@ -195,6 +197,8 @@ public DefaultEventSource(final CloseableHttpAsyncClient client, final EventSourceConfig cfg = (config != null) ? config : EventSourceConfig.DEFAULT; this.backoff = cfg.backoff; this.maxReconnects = cfg.maxReconnects; + this.maxLineLength = cfg.maxLineLength; + this.maxEventSize = cfg.maxEventSize; this.parser = parser != null ? parser : SseParser.CHAR; } @@ -414,8 +418,8 @@ public void onRetry(final long retryMs) { }; final AsyncEntityConsumer entity = - (parser == SseParser.BYTE) ? new ByteSseEntityConsumer(cbs) - : new SseEntityConsumer(cbs); + (parser == SseParser.BYTE) ? new ByteSseEntityConsumer(cbs, maxLineLength, maxEventSize) + : new SseEntityConsumer(cbs, maxLineLength, maxEventSize); return new SseResponseConsumer(entity, ms -> retryAfterHintMs = Math.max(0L, ms)); } diff --git a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/ServerSentEventReader.java b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/ServerSentEventReader.java index 3dd7b1ebb4..8f096c9a36 100644 --- a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/ServerSentEventReader.java +++ b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/ServerSentEventReader.java @@ -26,7 +26,12 @@ */ package org.apache.hc.client5.http.sse.impl; +import java.io.IOException; + +import org.apache.hc.client5.http.sse.EventSourceConfig; import org.apache.hc.core5.annotation.Internal; +import org.apache.hc.core5.http.MessageConstraintException; +import org.apache.hc.core5.util.Args; /** * Minimal-allocation SSE line parser. @@ -54,15 +59,27 @@ default void onLastEventId(final String id) { } private final Callback cb; + private final int maxEventSize; private final StringBuilder data = new StringBuilder(128); private String type; // defaults to "message" private String id; public ServerSentEventReader(final Callback cb) { + this(cb, EventSourceConfig.DEFAULT_MAX_EVENT_SIZE); + } + + public ServerSentEventReader(final Callback cb, final int maxEventSize) { this.cb = cb; + this.maxEventSize = Args.positive(maxEventSize, "Max event size"); } - public void line(final String line) { + /** + * Processes one line without its terminator. + * + * @throws MessageConstraintException if the data accumulated for the current event + * exceeds the maximum event size + */ + public void line(final String line) throws IOException { // Trim possible trailing CR without reallocating final int L0 = line.length(); int end = L0; @@ -114,6 +131,11 @@ public void line(final String line) { // data: (append newline; removed on dispatch) if (vStart <= end) { + if ((long) data.length() + (end - vStart) + 1 > maxEventSize) { + data.setLength(0); + data.trimToSize(); + throw new MessageConstraintException("Maximum event size exceeded: " + maxEventSize); + } data.append(line, vStart, end).append('\n'); } diff --git a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/SseEntityConsumer.java b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/SseEntityConsumer.java index c1bc08764f..d6f33edaae 100644 --- a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/SseEntityConsumer.java +++ b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/impl/SseEntityConsumer.java @@ -32,10 +32,13 @@ import java.nio.CharBuffer; import java.nio.charset.StandardCharsets; +import org.apache.hc.client5.http.sse.EventSourceConfig; import org.apache.hc.core5.annotation.Internal; import org.apache.hc.core5.http.ContentType; import org.apache.hc.core5.http.HttpException; +import org.apache.hc.core5.http.MessageConstraintException; import org.apache.hc.core5.http.nio.entity.AbstractCharAsyncEntityConsumer; +import org.apache.hc.core5.util.Args; /** * Internal char-level SSE consumer that converts decoded UTF-8 characters @@ -53,6 +56,9 @@ * through {@link SseCallbacks}. * * + *

    A line longer than the maximum line length, or an event accumulating more than the + * maximum event size, fails the stream with {@link MessageConstraintException}.

    + * *

    Thread-safety: Not thread-safe. One instance is expected to be * used by a single decoding flow on an I/O thread.

    * @@ -63,13 +69,21 @@ public final class SseEntityConsumer extends AbstractCharAsyncEntityConsumer= maxLineLength) { + throw new MessageConstraintException("Maximum line length exceeded: " + maxLineLength); + } partial.append(c); } } - if (endOfStream) { - partial.setLength(0); - } } @Override @@ -130,6 +156,7 @@ protected Void generateContent() { @Override public void releaseResources() { partial.setLength(0); + partial.trimToSize(); pendingCr = false; reader = null; } diff --git a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/package-info.java b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/package-info.java index 6fa57894b2..cf6ed0a168 100644 --- a/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/package-info.java +++ b/httpclient5-sse/src/main/java/org/apache/hc/client5/http/sse/package-info.java @@ -89,6 +89,9 @@ *
  • Backoff: Provide a {@link org.apache.hc.client5.http.sse.BackoffStrategy} via * {@link org.apache.hc.client5.http.sse.EventSourceConfig}. Server {@code retry:} lines and * HTTP {@code Retry-After} headers are honored when present.
  • + *
  • Limits: {@link org.apache.hc.client5.http.sse.EventSourceConfig} bounds the line length + * (default 1 MiB) and the data accumulated for one event (default 8 MiB). A stream that exceeds + * either limit is failed and the connection is closed.
  • *
  • Headers: Add defaults on the {@link org.apache.hc.client5.http.sse.SseExecutorBuilder} * or per-connection using {@link org.apache.hc.client5.http.sse.EventSource#setHeader(String, String)}.
  • *
  • Parser: {@link org.apache.hc.client5.http.sse.impl.SseParser#CHAR} (default) is spec-compliant; diff --git a/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/ByteSseEntityConsumerTest.java b/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/ByteSseEntityConsumerTest.java index dd34c05dc4..7c189003fc 100644 --- a/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/ByteSseEntityConsumerTest.java +++ b/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/ByteSseEntityConsumerTest.java @@ -28,6 +28,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.nio.ByteBuffer; @@ -36,6 +37,7 @@ import org.apache.hc.client5.http.sse.impl.ByteSseEntityConsumer; import org.apache.hc.client5.http.sse.impl.SseCallbacks; import org.apache.hc.core5.http.ContentType; +import org.apache.hc.core5.http.MessageConstraintException; import org.junit.jupiter.api.Test; class ByteSseEntityConsumerTest { @@ -212,4 +214,55 @@ void resetsLastEventIdWithoutDispatchingEvent() throws Exception { assertEquals("", cb.lastEventId); assertNull(cb.data); } + + private static ByteBuffer bytes(final String s) { + return ByteBuffer.wrap(s.getBytes(StandardCharsets.UTF_8)); + } + + @Test + void failsOnUnterminatedLineLongerThanLimit() throws Exception { + final Cb cb = new Cb(); + final ByteSseEntityConsumer c = new ByteSseEntityConsumer(cb, 32, 1024); + c.streamStart(ContentType.parse("text/event-stream")); + + c.consume(bytes("data: 0123456789012345678901234")); + assertThrows(MessageConstraintException.class, () -> c.consume(bytes("0123456789"))); + assertNull(cb.data); + } + + @Test + void failsOnManyDataLinesWithoutDispatch() throws Exception { + final Cb cb = new Cb(); + final ByteSseEntityConsumer c = new ByteSseEntityConsumer(cb, 64, 100); + c.streamStart(ContentType.parse("text/event-stream")); + + assertThrows(MessageConstraintException.class, () -> { + for (int i = 0; i < 1000; i++) { + c.consume(bytes("data: 0123456789\n")); + } + }); + assertNull(cb.data); + } + + @Test + void acceptsLineExactlyAtLimit() throws Exception { + final Cb cb = new Cb(); + final ByteSseEntityConsumer c = new ByteSseEntityConsumer(cb, 16, 1024); + c.streamStart(ContentType.parse("text/event-stream")); + + c.consume(bytes("data: 0123456789\n\n")); + + assertEquals("0123456789", cb.data); + } + + @Test + void acceptsMultiLineEventUnderLimits() throws Exception { + final Cb cb = new Cb(); + final ByteSseEntityConsumer c = new ByteSseEntityConsumer(cb, 32, 64); + c.streamStart(ContentType.parse("text/event-stream")); + + c.consume(bytes("data: line0\ndata: line1\ndata: line2\n\n")); + + assertEquals("line0\nline1\nline2", cb.data); + } } diff --git a/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/ServerSentEventReaderTest.java b/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/ServerSentEventReaderTest.java index 32360698ba..82c547cc3c 100644 --- a/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/ServerSentEventReaderTest.java +++ b/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/ServerSentEventReaderTest.java @@ -28,8 +28,10 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import org.apache.hc.client5.http.sse.impl.ServerSentEventReader; +import org.apache.hc.core5.http.MessageConstraintException; import org.junit.jupiter.api.Test; class ServerSentEventReaderTest { @@ -63,7 +65,7 @@ public void onRetryChange(final long retryMs) { } @Test - void parsesMultiLineEventWithDefaults() { + void parsesMultiLineEventWithDefaults() throws Exception { final Capt c = new Capt(); final ServerSentEventReader r = new ServerSentEventReader(c); @@ -78,7 +80,7 @@ void parsesMultiLineEventWithDefaults() { } @Test - void parsesEventTypeAndCommentAndRetry() { + void parsesEventTypeAndCommentAndRetry() throws Exception { final Capt c = new Capt(); final ServerSentEventReader r = new ServerSentEventReader(c); @@ -96,7 +98,7 @@ void parsesEventTypeAndCommentAndRetry() { } @Test - void ignoresIdWithNul() { + void ignoresIdWithNul() throws Exception { final Capt c = new Capt(); final ServerSentEventReader r = new ServerSentEventReader(c); @@ -109,7 +111,7 @@ void ignoresIdWithNul() { } @Test - void updatesLastEventIdOnBlankLineWithoutDispatchingEvent() { + void updatesLastEventIdOnBlankLineWithoutDispatchingEvent() throws Exception { final Capt c = new Capt(); final ServerSentEventReader r = new ServerSentEventReader(c); @@ -124,7 +126,7 @@ void updatesLastEventIdOnBlankLineWithoutDispatchingEvent() { } @Test - void resetsLastEventIdOnBlankLineWithoutDispatchingEvent() { + void resetsLastEventIdOnBlankLineWithoutDispatchingEvent() throws Exception { final Capt c = new Capt(); final ServerSentEventReader r = new ServerSentEventReader(c); @@ -139,4 +141,26 @@ void resetsLastEventIdOnBlankLineWithoutDispatchingEvent() { assertEquals("", c.lastEventId); assertNull(c.data); } + + @Test + void failsWhenDataExceedsMaxEventSize() throws Exception { + final Capt c = new Capt(); + final ServerSentEventReader r = new ServerSentEventReader(c, 16); + + r.line("data: 01234567"); + assertThrows(MessageConstraintException.class, () -> r.line("data: 01234567")); + assertNull(c.data); + } + + @Test + void acceptsEventExactlyAtMaxEventSize() throws Exception { + final Capt c = new Capt(); + final ServerSentEventReader r = new ServerSentEventReader(c, 16); + + r.line("data: 0123456"); + r.line("data: 012345"); + r.line(""); + + assertEquals("0123456\n012345", c.data); + } } diff --git a/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/SseEntityConsumerTest.java b/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/SseEntityConsumerTest.java index d8ecedb335..ec28042335 100644 --- a/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/SseEntityConsumerTest.java +++ b/httpclient5-sse/src/test/java/org/apache/hc/client5/http/sse/SseEntityConsumerTest.java @@ -28,6 +28,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; @@ -36,6 +37,7 @@ import org.apache.hc.client5.http.sse.impl.SseCallbacks; import org.apache.hc.client5.http.sse.impl.SseEntityConsumer; import org.apache.hc.core5.http.ContentType; +import org.apache.hc.core5.http.MessageConstraintException; import org.junit.jupiter.api.Test; class SseEntityConsumerTest { @@ -176,4 +178,55 @@ void resetsLastEventIdWithoutDispatchingEvent() throws Exception { assertEquals("", cb.lastEventId); assertNull(cb.data); } + + @Test + void failsOnUnterminatedLineLongerThanLimit() throws Exception { + final Cb cb = new Cb(); + final SseEntityConsumer c = new SseEntityConsumer(cb, 32, 1024); + c.streamStart(ContentType.parse("text/event-stream")); + + c.data(CharBuffer.wrap("data: 0123456789012345678901234"), false); + assertThrows(MessageConstraintException.class, + () -> c.data(CharBuffer.wrap("0123456789"), false)); + assertNull(cb.data); + } + + @Test + void failsOnManyDataLinesWithoutDispatch() throws Exception { + final Cb cb = new Cb(); + final SseEntityConsumer c = new SseEntityConsumer(cb, 64, 100); + c.streamStart(ContentType.parse("text/event-stream")); + + assertThrows(MessageConstraintException.class, () -> { + for (int i = 0; i < 1000; i++) { + c.data(CharBuffer.wrap("data: 0123456789\n"), false); + } + }); + assertNull(cb.data); + } + + @Test + void acceptsLineExactlyAtLimit() throws Exception { + final Cb cb = new Cb(); + final SseEntityConsumer c = new SseEntityConsumer(cb, 16, 1024); + c.streamStart(ContentType.parse("text/event-stream")); + + c.data(CharBuffer.wrap("data: 0123456789\n\n"), false); + + assertEquals("0123456789", cb.data); + } + + @Test + void acceptsMultiLineEventUnderLimits() throws Exception { + final Cb cb = new Cb(); + final SseEntityConsumer c = new SseEntityConsumer(cb, 32, 64); + c.streamStart(ContentType.parse("text/event-stream")); + + for (int i = 0; i < 3; i++) { + c.data(CharBuffer.wrap("data: line" + i + "\n"), false); + } + c.data(CharBuffer.wrap("\n"), false); + + assertEquals("line0\nline1\nline2", cb.data); + } } diff --git a/httpclient5-testing/pom.xml b/httpclient5-testing/pom.xml index 97c1e96401..36062df988 100644 --- a/httpclient5-testing/pom.xml +++ b/httpclient5-testing/pom.xml @@ -126,7 +126,7 @@ org.apache.maven.plugins maven-surefire-plugin - slow,time-out,stale-conn + slow,time-out,stale-conn,cancellation diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestHttp1Async.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestHttp1Async.java index 700c631b36..cf5b0a477f 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestHttp1Async.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/async/TestHttp1Async.java @@ -46,6 +46,7 @@ import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.URIScheme; import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -139,6 +140,7 @@ void testSharedPool() throws Exception { Assertions.assertEquals(2048, body3.length()); } + @Tag("cancellation") @Test void testRequestCancellation() throws Exception { configureServer(bootstrap -> bootstrap.register("/random/*", AsyncRandomHandler::new)); diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java index 17173a4253..0eb8de18fa 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientRequestExecution.java @@ -65,7 +65,7 @@ import org.apache.hc.core5.net.URIBuilder; import org.apache.hc.core5.util.TimeValue; import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; /** @@ -316,7 +316,8 @@ void testAbsoluteRequestURIWithFragment() throws Exception { Assertions.assertEquals(uri, location); } - @Test @Disabled("Fails intermittently with GitHub Actions") + @Tag("cancellation") + @Test void testRequestCancellation() throws Exception { startServer(); final HttpHost target = startServer(); diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java index 706793e98b..3a7de2326a 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/async/AsyncProtocolExec.java @@ -217,7 +217,8 @@ public AsyncDataConsumer handleResponse( if (needAuthentication( targetAuthExchange, proxyAuthExchange, - proxy != null ? proxy : target, + // A 407 received through a tunnel comes from the origin, not from the proxy + route.isTunnelled() ? null : proxy, target, pathPrefix, response, @@ -319,11 +320,11 @@ private boolean needAuthentication( } } - final boolean proxyAuthRequested = authenticator.isChallenged( + final boolean proxyAuthRequested = proxy != null && authenticator.isChallenged( proxy, ChallengeType.PROXY, response, proxyAuthExchange, context); - final boolean proxyMutualAuthRequired = authenticator.isChallengeExpected(proxyAuthExchange); + final boolean proxyMutualAuthRequired = proxy != null && authenticator.isChallengeExpected(proxyAuthExchange); - if (authCacheKeeper != null) { + if (proxy != null && authCacheKeeper != null) { if (proxyAuthRequested) { authCacheKeeper.updateOnChallenge(proxy, null, proxyAuthExchange, context); } else { diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java index 4514ddfe94..cc73b224e5 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/classic/ProtocolExec.java @@ -207,7 +207,8 @@ public ClassicHttpResponse execute( if (needAuthentication( targetAuthExchange, proxyAuthExchange, - proxy != null ? proxy : target, + // A 407 received through a tunnel comes from the origin, not from the proxy + route.isTunnelled() ? null : proxy, target, pathPrefix, response, @@ -280,11 +281,11 @@ private boolean needAuthentication( } } - final boolean proxyAuthRequested = authenticator.isChallenged( + final boolean proxyAuthRequested = proxy != null && authenticator.isChallenged( proxy, ChallengeType.PROXY, response, proxyAuthExchange, context); - final boolean proxyMutualAuthRequired = authenticator.isChallengeExpected(proxyAuthExchange); + final boolean proxyMutualAuthRequired = proxy != null && authenticator.isChallengeExpected(proxyAuthExchange); - if (authCacheKeeper != null) { + if (proxy != null && authCacheKeeper != null) { if (proxyAuthRequested) { authCacheKeeper.updateOnChallenge(proxy, null, proxyAuthExchange, context); } else { diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java index 78e9476b1e..49cef36be2 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/BasicHttpClientConnectionManager.java @@ -491,7 +491,8 @@ public void connect(final ConnectionEndpoint endpoint, final TimeValue timeout, this.connectionOperator.connect( connection, firstHop, - route.getTargetName(), + // The target name applies to the target only, never to an intermediate proxy + route.getProxyHost() != null ? null : route.getTargetName(), route.getLocalSocketAddress(), connectTimeout, socketConfig, diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java index 2945e8ed7d..6d670bf3e6 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/io/PoolingHttpClientConnectionManager.java @@ -565,7 +565,8 @@ public void connect(final ConnectionEndpoint endpoint, final TimeValue timeout, this.connectionOperator.connect( conn, firstHop, - route.getTargetName(), + // The target name applies to the target only, never to an intermediate proxy + route.getProxyHost() != null ? null : route.getTargetName(), unixDomainSocket, route.getLocalSocketAddress(), connectTimeout, diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java index c5215a4156..d030b9b3f3 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/impl/nio/PoolingAsyncClientConnectionManager.java @@ -523,7 +523,8 @@ public Future connect( connectionInitiator, firstHop, unixDomainSocket, - route.getTargetName(), + // The target name applies to the target only, never to an intermediate proxy + route.getProxyHost() != null ? null : route.getTargetName(), route.getLocalSocketAddress(), connectTimeout, connectAttachment, diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java index d403ff18df..0aba6a75ae 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategy.java @@ -99,6 +99,10 @@ abstract class AbstractClientTlsStrategy implements TlsStrategy, TlsSocketStrate this.sslBufferManagement = sslBufferManagement != null ? sslBufferManagement : SSLBufferMode.STATIC; this.hostnameVerificationPolicy = hostnameVerificationPolicy != null ? hostnameVerificationPolicy : (hostnameVerifier != null ? HostnameVerificationPolicy.BOTH : HostnameVerificationPolicy.BUILTIN); + Args.check(this.hostnameVerificationPolicy != HostnameVerificationPolicy.CLIENT + && this.hostnameVerificationPolicy != HostnameVerificationPolicy.BOTH + || hostnameVerifier != null, + "Hostname verifier is required for hostname verification policy " + this.hostnameVerificationPolicy); this.hostnameVerifier = hostnameVerifier; } diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java index bcf9bfd344..d635866f73 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/ConscryptClientTlsStrategy.java @@ -69,7 +69,8 @@ public ConscryptClientTlsStrategy( final String[] supportedCipherSuites, final SSLBufferMode sslBufferManagement, final HostnameVerifier hostnameVerifier) { - this(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, HostnameVerificationPolicy.CLIENT, hostnameVerifier); + this(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, + hostnameVerifier != null ? HostnameVerificationPolicy.CLIENT : HostnameVerificationPolicy.BUILTIN, hostnameVerifier); } /** diff --git a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java index c52997473f..2792df98f0 100644 --- a/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java +++ b/httpclient5/src/main/java/org/apache/hc/client5/http/ssl/DefaultClientTlsStrategy.java @@ -101,7 +101,8 @@ public DefaultClientTlsStrategy( final SSLBufferMode sslBufferManagement, final HostnameVerifier hostnameVerifier, final Factory tlsDetailsFactory) { - super(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, HostnameVerificationPolicy.CLIENT, hostnameVerifier); + super(sslContext, supportedProtocols, supportedCipherSuites, sslBufferManagement, + hostnameVerifier != null ? HostnameVerificationPolicy.CLIENT : HostnameVerificationPolicy.BUILTIN, hostnameVerifier); this.tlsDetailsFactory = tlsDetailsFactory; } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java index dff99e94e6..7fcae912dc 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java @@ -27,11 +27,14 @@ package org.apache.hc.client5.http.impl.async; import java.io.IOException; +import java.util.ArrayList; import java.util.Collections; +import java.util.List; import java.util.concurrent.atomic.AtomicInteger; import org.apache.hc.client5.http.AuthenticationStrategy; import org.apache.hc.client5.http.HttpRoute; +import org.apache.hc.client5.http.RouteInfo; import org.apache.hc.client5.http.async.AsyncExecCallback; import org.apache.hc.client5.http.async.AsyncExecChain; import org.apache.hc.client5.http.async.AsyncExecRuntime; @@ -39,6 +42,7 @@ import org.apache.hc.client5.http.auth.AuthScope; import org.apache.hc.client5.http.auth.ChallengeType; import org.apache.hc.client5.http.auth.StandardAuthScheme; +import org.apache.hc.client5.http.impl.auth.BasicAuthCache; import org.apache.hc.client5.http.impl.auth.BasicScheme; import org.apache.hc.client5.http.impl.auth.CredentialsProviderBuilder; import org.apache.hc.client5.http.protocol.HttpClientContext; @@ -157,4 +161,111 @@ public void failed(final Exception cause) { final AuthExchange authExchange = context.getAuthExchange(target); Assertions.assertEquals("/blah/", authExchange.getPathPrefix()); } + + private static final class RecordingCallback implements AsyncExecCallback { + + private final List codes = new ArrayList<>(); + private final AtomicInteger completed = new AtomicInteger(0); + + @Override + public AsyncDataConsumer handleResponse(final HttpResponse response, final EntityDetails entityDetails) { + codes.add(response.getCode()); + return Mockito.mock(AsyncDataConsumer.class); + } + + @Override + public void handleInformationResponse(final HttpResponse response) { + } + + @Override + public void completed() { + completed.incrementAndGet(); + } + + @Override + public void failed(final Exception cause) { + Assertions.fail(cause); + } + + } + + private void stubChain(final AtomicInteger proceedCount, final List requests, final int... codes) + throws Exception { + Mockito.doAnswer(invocation -> { + requests.add(invocation.getArgument(0, HttpRequest.class)); + final AsyncExecCallback cb = invocation.getArgument(3, AsyncExecCallback.class); + final int code = codes[Math.min(proceedCount.getAndIncrement(), codes.length - 1)]; + final HttpResponse resp = new BasicHttpResponse(code, "Huh?"); + if (code == 407) { + resp.setHeader(HttpHeaders.PROXY_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + } + cb.handleResponse(resp, (EntityDetails) null); + cb.completed(); + return null; + }).when(chain).proceed(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); + } + + @Test + void testProxyChallengeThroughTunnelWithAuthCacheIsReturnedToCaller() throws Exception { + final AsyncProtocolExec cachingExec = new AsyncProtocolExec(targetAuthStrategy, proxyAuthStrategy, null, false); + final HttpHost httpsTarget = new HttpHost("https", "foo", 443); + final HttpHost proxy = new HttpHost("http", "proxy", 8080); + final HttpRoute route = new HttpRoute(httpsTarget, null, proxy, true, + RouteInfo.TunnelType.TUNNELLED, RouteInfo.LayerType.LAYERED); + final BasicAuthCache authCache = new BasicAuthCache(); + final HttpClientContext context = HttpClientContext.create(); + context.setAuthCache(authCache); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(proxy), "proxyuser", "proxypass".toCharArray()) + .build()); + Mockito.when(execRuntime.isEndpointConnected()).thenReturn(true); + final AtomicInteger proceedCount = new AtomicInteger(0); + final List requests = new ArrayList<>(); + stubChain(proceedCount, requests, 407); + + final HttpRequest request = BasicRequestBuilder.get("https://foo/test").build(); + final AsyncExecChain.Scope scope = new AsyncExecChain.Scope("test", route, request, + Mockito.mock(CancellableDependency.class), context, execRuntime, null, new AtomicInteger(1)); + final RecordingCallback callback = new RecordingCallback(); + + cachingExec.execute(request, (AsyncEntityProducer) null, scope, chain, callback); + + Assertions.assertEquals(Collections.singletonList(407), callback.codes); + Assertions.assertEquals(1, callback.completed.get()); + Assertions.assertEquals(1, proceedCount.get()); + Assertions.assertNull(requests.get(0).getFirstHeader(HttpHeaders.PROXY_AUTHORIZATION)); + Mockito.verifyNoInteractions(proxyAuthStrategy); + Assertions.assertEquals(AuthExchange.State.UNCHALLENGED, context.getAuthExchange(proxy).getState()); + Assertions.assertNull(authCache.get(proxy)); + } + + @Test + void testProxyChallengeOnPlainRouteStillAuthenticates() throws Exception { + final HttpHost proxy = new HttpHost("http", "proxy", 8080); + final HttpRoute route = new HttpRoute(target, null, proxy, false); + final HttpClientContext context = HttpClientContext.create(); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(proxy), "proxyuser", "proxypass".toCharArray()) + .build()); + Mockito.when(proxyAuthStrategy.select( + Mockito.eq(ChallengeType.PROXY), + Mockito.any(), + Mockito.any())) + .thenReturn(Collections.singletonList(new BasicScheme())); + Mockito.when(execRuntime.isEndpointConnected()).thenReturn(true); + final AtomicInteger proceedCount = new AtomicInteger(0); + final List requests = new ArrayList<>(); + stubChain(proceedCount, requests, 407, 200); + + final HttpRequest request = BasicRequestBuilder.get("http://foo/test").build(); + final AsyncExecChain.Scope scope = new AsyncExecChain.Scope("test", route, request, + Mockito.mock(CancellableDependency.class), context, execRuntime, null, new AtomicInteger(1)); + final RecordingCallback callback = new RecordingCallback(); + + protocolExec.execute(request, (AsyncEntityProducer) null, scope, chain, callback); + + Assertions.assertEquals(2, proceedCount.get()); + Assertions.assertEquals(Collections.singletonList(200), callback.codes); + Assertions.assertNotNull(requests.get(1).getFirstHeader(HttpHeaders.PROXY_AUTHORIZATION)); + } } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java index 3417e66ff5..9588a42972 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/classic/TestProtocolExec.java @@ -34,6 +34,7 @@ import org.apache.hc.client5.http.AuthenticationStrategy; import org.apache.hc.client5.http.HttpRoute; +import org.apache.hc.client5.http.RouteInfo; import org.apache.hc.client5.http.auth.AuthExchange; import org.apache.hc.client5.http.auth.AuthScope; import org.apache.hc.client5.http.auth.ChallengeType; @@ -43,6 +44,7 @@ import org.apache.hc.client5.http.classic.methods.HttpGet; import org.apache.hc.client5.http.classic.methods.HttpPost; import org.apache.hc.client5.http.entity.EntityBuilder; +import org.apache.hc.client5.http.impl.auth.BasicAuthCache; import org.apache.hc.client5.http.impl.auth.BasicScheme; import org.apache.hc.client5.http.impl.auth.CredentialsProviderBuilder; import org.apache.hc.client5.http.protocol.HttpClientContext; @@ -58,6 +60,7 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.junit.jupiter.MockitoExtension; @@ -295,4 +298,105 @@ void testAuthExchangePathPrefixRestoredAfterChallenge() throws Exception { final AuthExchange authExchange = context.getAuthExchange(target); Assertions.assertEquals("/blah/", authExchange.getPathPrefix()); } + + @Test + void testProxyChallengeThroughTunnelIsReturnedToCaller() throws Exception { + final HttpHost httpsTarget = new HttpHost("https", "foo", 443); + final HttpHost proxy = new HttpHost("http", "proxy", 8080); + final HttpRoute route = new HttpRoute(httpsTarget, null, proxy, true, + RouteInfo.TunnelType.TUNNELLED, RouteInfo.LayerType.LAYERED); + final HttpClientContext context = HttpClientContext.create(); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(proxy), "proxyuser", "proxypass".toCharArray()) + .build()); + final ClassicHttpRequest request = new HttpGet("https://foo/test"); + final ClassicHttpResponse response = new BasicClassicHttpResponse(407, "Huh?"); + response.setHeader(HttpHeaders.PROXY_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + + Mockito.when(chain.proceed(Mockito.same(request), Mockito.any())).thenReturn(response); + + final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context); + final ClassicHttpResponse finalResponse = protocolExec.execute(request, scope, chain); + Assertions.assertEquals(407, finalResponse.getCode()); + Mockito.verify(chain, Mockito.times(1)).proceed(request, scope); + Mockito.verifyNoInteractions(proxyAuthStrategy); + Assertions.assertNull(context.getAuthExchange(proxy).getAuthScheme()); + } + + @Test + void testProxyChallengeWithoutProxyIsReturnedToCaller() throws Exception { + final HttpRoute route = new HttpRoute(target); + final HttpClientContext context = HttpClientContext.create(); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(target), "user", "pass".toCharArray()) + .build()); + final ClassicHttpRequest request = new HttpGet("http://foo/test"); + final ClassicHttpResponse response = new BasicClassicHttpResponse(407, "Huh?"); + response.setHeader(HttpHeaders.PROXY_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + + Mockito.when(chain.proceed(Mockito.same(request), Mockito.any())).thenReturn(response); + + final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context); + final ClassicHttpResponse finalResponse = protocolExec.execute(request, scope, chain); + Assertions.assertEquals(407, finalResponse.getCode()); + Mockito.verify(chain, Mockito.times(1)).proceed(request, scope); + Mockito.verifyNoInteractions(proxyAuthStrategy); + } + + @Test + void testProxyChallengeThroughTunnelWithAuthCacheIsReturnedToCaller() throws Exception { + final ProtocolExec cachingExec = new ProtocolExec(targetAuthStrategy, proxyAuthStrategy, null, false); + final HttpHost httpsTarget = new HttpHost("https", "foo", 443); + final HttpHost proxy = new HttpHost("http", "proxy", 8080); + final HttpRoute route = new HttpRoute(httpsTarget, null, proxy, true, + RouteInfo.TunnelType.TUNNELLED, RouteInfo.LayerType.LAYERED); + final BasicAuthCache authCache = new BasicAuthCache(); + final HttpClientContext context = HttpClientContext.create(); + context.setAuthCache(authCache); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(proxy), "proxyuser", "proxypass".toCharArray()) + .build()); + final ClassicHttpRequest request = new HttpGet("https://foo/test"); + final ClassicHttpResponse response = new BasicClassicHttpResponse(407, "Huh?"); + response.setHeader(HttpHeaders.PROXY_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + + Mockito.when(chain.proceed(Mockito.same(request), Mockito.any())).thenReturn(response); + + final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context); + final ClassicHttpResponse finalResponse = cachingExec.execute(request, scope, chain); + Assertions.assertSame(response, finalResponse); + Mockito.verify(chain, Mockito.times(1)).proceed(request, scope); + Mockito.verifyNoInteractions(proxyAuthStrategy); + Assertions.assertNull(request.getFirstHeader(HttpHeaders.PROXY_AUTHORIZATION)); + Assertions.assertEquals(AuthExchange.State.UNCHALLENGED, context.getAuthExchange(proxy).getState()); + Assertions.assertNull(authCache.get(proxy)); + } + + @Test + void testProxyChallengeOnPlainRouteStillAuthenticates() throws Exception { + final HttpHost proxy = new HttpHost("http", "proxy", 8080); + final HttpRoute route = new HttpRoute(target, null, proxy, false); + final HttpClientContext context = HttpClientContext.create(); + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(proxy), "proxyuser", "proxypass".toCharArray()) + .build()); + final ClassicHttpRequest request = new HttpGet("http://foo/test"); + final ClassicHttpResponse response1 = new BasicClassicHttpResponse(407, "Huh?"); + response1.setHeader(HttpHeaders.PROXY_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + final ClassicHttpResponse response2 = new BasicClassicHttpResponse(200, "OK"); + + Mockito.when(chain.proceed(Mockito.any(), Mockito.any())).thenReturn(response1, response2); + Mockito.when(proxyAuthStrategy.select( + Mockito.eq(ChallengeType.PROXY), + Mockito.any(), + Mockito.any())).thenReturn(Collections.singletonList(new BasicScheme())); + Mockito.when(execRuntime.isConnectionReusable()).thenReturn(true); + + final ExecChain.Scope scope = new ExecChain.Scope("test", route, request, execRuntime, context); + final ClassicHttpResponse finalResponse = protocolExec.execute(request, scope, chain); + Assertions.assertEquals(200, finalResponse.getCode()); + final ArgumentCaptor captor = ArgumentCaptor.forClass(ClassicHttpRequest.class); + Mockito.verify(chain, Mockito.times(2)).proceed(captor.capture(), Mockito.same(scope)); + Assertions.assertNotNull(captor.getAllValues().get(1).getFirstHeader(HttpHeaders.PROXY_AUTHORIZATION)); + } } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java index b045626750..e5f8416018 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/io/TestPoolingHttpClientConnectionManager.java @@ -53,6 +53,7 @@ import org.apache.hc.core5.http.HttpHost; import org.apache.hc.core5.http.config.Lookup; import org.apache.hc.core5.http.io.SocketConfig; +import org.apache.hc.core5.net.URIAuthority; import org.apache.hc.core5.http2.HttpVersionPolicy; import org.apache.hc.core5.pool.PoolEntry; import org.apache.hc.core5.pool.StrictConnPool; @@ -407,5 +408,52 @@ void testConcurrentShutdown() throws InterruptedException { Assertions.assertTrue(mgr.isClosed(), "Connection manager should be shutdown after concurrent calls to shutdown."); } + @Test + void testHttpsProxyVerifiedAgainstProxyNotTargetName() throws Exception { + final HttpHost target = new HttpHost("https", "somehost", 443); + final URIAuthority targetName = new URIAuthority("virtualhost", 443); + final HttpHost proxy = new HttpHost("https", "someproxy", 8443); + final InetAddress remote = InetAddress.getByAddress(new byte[] {10, 0, 0, 1}); + final HttpRoute route = new HttpRoute(target, targetName, null, proxy, true); + Assertions.assertTrue(route.isTunnelled()); + + final PoolEntry entry = new PoolEntry<>(route, TimeValue.NEG_ONE_MILLISECOND); + entry.assignConnection(conn); + + Mockito.when(conn.isOpen()).thenReturn(false); + Mockito.when(future.get(1, TimeUnit.SECONDS)).thenReturn(entry); + Mockito.when(pool.lease( + Mockito.eq(route), + Mockito.eq(null), + Mockito.any(), + Mockito.eq(null))) + .thenReturn(future); + + final LeaseRequest connRequest = mgr.lease("some-id", route, null); + final ConnectionEndpoint endpoint = connRequest.get(Timeout.ofSeconds(1)); + Assertions.assertNotNull(endpoint); + + final HttpClientContext context = HttpClientContext.create(); + final TlsConfig tlsConfig = TlsConfig.custom().build(); + mgr.setDefaultTlsConfig(tlsConfig); + + Mockito.when(dnsResolver.resolve("someproxy", 8443)).thenReturn(Collections.singletonList(new InetSocketAddress(remote, 8443))); + Mockito.when(schemePortResolver.resolve(proxy.getSchemeName(), proxy)).thenReturn(8443); + Mockito.when(tlsSocketStrategyLookup.lookup("https")).thenReturn(tlsSocketStrategy); + Mockito.when(detachedSocketFactory.create(Mockito.any(), Mockito.any())).thenReturn(socket); + Mockito.when(tlsSocketStrategy.upgrade( + Mockito.any(), Mockito.any(), Mockito.anyInt(), Mockito.any(), Mockito.any())).thenReturn(upgradedSocket); + + mgr.connect(endpoint, null, context); + + Mockito.verify(tlsSocketStrategy).upgrade(socket, "someproxy", 8443, null, context); + + Mockito.when(conn.isOpen()).thenReturn(true); + Mockito.when(conn.getSocket()).thenReturn(socket); + + mgr.upgrade(endpoint, context); + + Mockito.verify(tlsSocketStrategy).upgrade(socket, "virtualhost", 443, tlsConfig, context); + } } diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java index 20a2bbd407..a052ca16e7 100644 --- a/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/AbstractClientTlsStrategyTest.java @@ -27,17 +27,21 @@ package org.apache.hc.client5.http.ssl; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; import java.security.cert.X509Certificate; +import javax.net.ssl.HostnameVerifier; import javax.net.ssl.SSLEngine; import javax.net.ssl.SSLParameters; import javax.net.ssl.SSLSession; import javax.security.auth.x500.X500Principal; +import org.apache.hc.core5.function.Factory; import org.apache.hc.core5.reactor.ssl.SSLBufferMode; import org.apache.hc.core5.reactor.ssl.TlsDetails; import org.apache.hc.core5.ssl.SSLContexts; @@ -110,5 +114,31 @@ TlsDetails createTlsDetails(final SSLEngine sslEngine) { } + @Test + void testClientPolicyRequiresHostnameVerifier() { + assertThrows(IllegalArgumentException.class, () -> new DefaultClientTlsStrategy( + SSLContexts.createDefault(), HostnameVerificationPolicy.CLIENT, null)); + assertThrows(IllegalArgumentException.class, () -> new DefaultClientTlsStrategy( + SSLContexts.createDefault(), HostnameVerificationPolicy.BOTH, null)); + } + + @Test + void testBuilderClientPolicyRequiresHostnameVerifier() { + final ClientTlsStrategyBuilder builder = ClientTlsStrategyBuilder.create() + .setSslContext(SSLContexts.createDefault()) + .setHostVerificationPolicy(HostnameVerificationPolicy.CLIENT); + assertThrows(IllegalArgumentException.class, builder::buildClassic); + } + + @Test + @SuppressWarnings("deprecation") + void testNullHostnameVerifierDefaultsToBuiltinPolicy() { + assertDoesNotThrow(() -> new DefaultClientTlsStrategy( + SSLContexts.createDefault(), null, null, SSLBufferMode.STATIC, + (HostnameVerifier) null, (Factory) null)); + assertDoesNotThrow(() -> new DefaultClientTlsStrategy(SSLContexts.createDefault())); + assertDoesNotThrow(() -> new ConscryptClientTlsStrategy(SSLContexts.createDefault())); + } + } \ No newline at end of file diff --git a/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestNullHostnameVerifierPolicy.java b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestNullHostnameVerifierPolicy.java new file mode 100644 index 0000000000..02b9ee22b0 --- /dev/null +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/ssl/TestNullHostnameVerifierPolicy.java @@ -0,0 +1,147 @@ +/* + * ==================================================================== + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + * ==================================================================== + * + * This software consists of voluntary contributions made by many + * individuals on behalf of the Apache Software Foundation. For more + * information on the Apache Software Foundation, please see + * . + * + */ + + +package org.apache.hc.client5.http.ssl; + +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.IOException; +import java.net.InetAddress; +import java.net.Socket; + +import javax.net.ssl.HostnameVerifier; +import javax.net.ssl.SSLContext; +import javax.net.ssl.SSLEngine; +import javax.net.ssl.SSLException; +import javax.net.ssl.SSLServerSocket; +import javax.net.ssl.SSLSocket; + +import org.apache.hc.core5.function.Factory; +import org.apache.hc.core5.reactor.ssl.SSLBufferMode; +import org.apache.hc.core5.reactor.ssl.TlsDetails; +import org.apache.hc.core5.ssl.SSLContexts; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +/** + * Handshakes against a server whose certificate is issued for {@code localhost} to make sure + * that a strategy created without a {@link javax.net.ssl.HostnameVerifier} still verifies the host name. + */ +class TestNullHostnameVerifierPolicy { + + private static final char[] PASSWORD = "nopassword".toCharArray(); + + private SSLContext sslContext; + private SSLServerSocket serverSocket; + private Thread serverThread; + + @BeforeEach + void setUp() throws Exception { + sslContext = SSLContexts.custom() + .loadTrustMaterial(getClass().getResource("/test.keystore"), PASSWORD) + .loadKeyMaterial(getClass().getResource("/test.keystore"), PASSWORD, PASSWORD) + .build(); + serverSocket = (SSLServerSocket) sslContext.getServerSocketFactory() + .createServerSocket(0, 1, InetAddress.getLoopbackAddress()); + serverThread = new Thread(() -> { + try (final SSLSocket socket = (SSLSocket) serverSocket.accept()) { + socket.startHandshake(); + } catch (final IOException ignore) { + // the client may abort the handshake + } + }); + serverThread.setDaemon(true); + serverThread.start(); + } + + @AfterEach + void tearDown() throws Exception { + serverSocket.close(); + serverThread.join(5000); + } + + private SSLSocket upgrade(final AbstractClientTlsStrategy strategy, final String target) throws IOException { + final Socket socket = new Socket(InetAddress.getLoopbackAddress(), serverSocket.getLocalPort()); + try { + return strategy.upgrade(socket, target, serverSocket.getLocalPort(), null, null); + } catch (final IOException | RuntimeException ex) { + socket.close(); + throw ex; + } + } + + @SuppressWarnings("deprecation") + private AbstractClientTlsStrategy defaultStrategy() { + return new DefaultClientTlsStrategy( + sslContext, null, null, SSLBufferMode.STATIC, + (HostnameVerifier) null, (Factory) null); + } + + @SuppressWarnings("deprecation") + private AbstractClientTlsStrategy conscryptStrategy() { + return new ConscryptClientTlsStrategy( + sslContext, null, null, SSLBufferMode.STATIC, (HostnameVerifier) null); + } + + @Test + void testDefaultStrategyRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(defaultStrategy(), "example.invalid")); + } + + @Test + void testDefaultStrategyAcceptsMatchingHost() throws Exception { + try (final SSLSocket socket = upgrade(defaultStrategy(), "localhost")) { + assertNotNull(socket.getSession().getPeerCertificates()); + } + } + + @Test + void testConscryptStrategyRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(conscryptStrategy(), "example.invalid")); + } + + @Test + void testConscryptStrategyAcceptsMatchingHost() throws Exception { + try (final SSLSocket socket = upgrade(conscryptStrategy(), "localhost")) { + assertNotNull(socket.getSession().getPeerCertificates()); + } + } + + @Test + void testConscryptDefaultConstructorRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(new ConscryptClientTlsStrategy(sslContext), "example.invalid")); + } + + @Test + void testDefaultConstructorRejectsMismatchingHost() { + assertThrows(SSLException.class, () -> upgrade(new DefaultClientTlsStrategy(sslContext), "example.invalid")); + } + +} diff --git a/httpclient5/src/test/resources/test.keystore b/httpclient5/src/test/resources/test.keystore new file mode 100644 index 0000000000..f8d5ace1ad Binary files /dev/null and b/httpclient5/src/test/resources/test.keystore differ diff --git a/pom.xml b/pom.xml index 36db97227e..ac7e99dc0e 100644 --- a/pom.xml +++ b/pom.xml @@ -60,7 +60,7 @@ - 8 + 17 17 ${javaVersion} ${javaVersion}