From aea207ae413919c9320ce2b7df1b5afa7b4872f2 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Mon, 9 Feb 2026 12:31:44 +0100 Subject: [PATCH] HTTPCLIENT-2414 - Fix Basic auth cache scoping across path prefixes Preserve AuthExchange pathPrefix on reset to avoid preemptive Authorization reuse outside the protection space. --- .../sync/TestClientAuthentication.java | 143 +++++++++++++++- .../http/impl/async/AsyncProtocolExec.java | 6 +- .../http/impl/classic/ProtocolExec.java | 6 +- .../impl/async/TestAsyncProtocolExec.java | 159 ++++++++++++++++++ .../http/impl/classic/TestProtocolExec.java | 33 ++++ 5 files changed, 334 insertions(+), 13 deletions(-) create mode 100644 httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java diff --git a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientAuthentication.java b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientAuthentication.java index b22c00ee78..da7767e3af 100644 --- a/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientAuthentication.java +++ b/httpclient5-testing/src/test/java/org/apache/hc/client5/testing/sync/TestClientAuthentication.java @@ -26,13 +26,17 @@ */ package org.apache.hc.client5.testing.sync; +import static org.hamcrest.MatcherAssert.assertThat; + import java.io.ByteArrayInputStream; import java.nio.charset.StandardCharsets; import java.security.SecureRandom; import java.util.Arrays; import java.util.Collections; +import java.util.List; import java.util.Queue; import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; import java.util.stream.Collectors; @@ -57,6 +61,7 @@ import org.apache.hc.client5.http.impl.auth.CredentialsProviderBuilder; import org.apache.hc.client5.http.protocol.HttpClientContext; import org.apache.hc.client5.testing.BasicTestAuthenticator; +import org.apache.hc.client5.testing.auth.AuthResult; import org.apache.hc.client5.testing.auth.Authenticator; import org.apache.hc.client5.testing.auth.BearerAuthenticationHandler; import org.apache.hc.client5.testing.classic.AuthenticatingDecorator; @@ -82,6 +87,7 @@ import org.apache.hc.core5.http.protocol.HttpContext; import org.apache.hc.core5.http.support.BasicResponseBuilder; import org.apache.hc.core5.net.URIAuthority; +import org.hamcrest.CoreMatchers; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -339,8 +345,9 @@ void testBasicAuthenticationCredentialsCaching() throws Exception { Mockito.verify(authStrategy).select(Mockito.any(), Mockito.any(), Mockito.any()); - Assertions.assertEquals(Arrays.asList(401, 200, 200, 200, 200, 200), - responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList())); + assertThat( + responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList()), + CoreMatchers.equalTo(Arrays.asList(401, 200, 200, 200, 200, 200))); } @Test @@ -381,8 +388,9 @@ void testBasicAuthenticationCredentialsCachingByPathPrefix() throws Exception { // There should be only single auth strategy call for all successful message exchanges Mockito.verify(authStrategy).select(Mockito.any(), Mockito.any(), Mockito.any()); - Assertions.assertEquals(Arrays.asList(401, 200, 200, 200, 200), - responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList())); + assertThat( + responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList()), + CoreMatchers.equalTo(Arrays.asList(401, 200, 200, 200, 200))); responseQueue.clear(); authCache.clear(); @@ -400,10 +408,11 @@ void testBasicAuthenticationCredentialsCachingByPathPrefix() throws Exception { } // There should be an auth strategy call for all successful message exchanges - Mockito.verify(authStrategy, Mockito.times(2)).select(Mockito.any(), Mockito.any(), Mockito.any()); + Mockito.verify(authStrategy, Mockito.times(3)).select(Mockito.any(), Mockito.any(), Mockito.any()); - Assertions.assertEquals(Arrays.asList(200, 401, 200, 200, 401, 200), - responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList())); + assertThat( + responseQueue.stream().map(HttpResponse::getCode).collect(Collectors.toList()), + CoreMatchers.equalTo(Arrays.asList(200, 401, 200, 401, 200, 401, 200))); } @Test @@ -814,4 +823,124 @@ void testBearerTokenAuthentication() throws Exception { }); } + @Test + void testBasicAuthenticationCredentialsCachingDifferentPathPrefixesSameContext() throws Exception { + final List requests = new CopyOnWriteArrayList<>(); + final Authenticator authenticator = new BasicTestAuthenticator("test:test", "test realm") { + @Override + public AuthResult perform(final URIAuthority authority, + final String requestUri, + final String credentials) { + requests.add(new RequestSnapshot(requestUri, credentials != null)); + return super.perform(authority, requestUri, credentials); + } + }; + configureServerWithBasicAuth(authenticator, bootstrap -> bootstrap.register("*", new EchoHandler())); + final HttpHost target = startServer(); + + final List responseCodes = new CopyOnWriteArrayList<>(); + + configureClient(builder -> builder + .addResponseInterceptorLast((response, entity, context) -> responseCodes.add(response.getCode()))); + + final TestClient client = client(); + + final CredentialsProvider credentialsProvider = CredentialsProviderBuilder.create() + .add(target, "test", "test".toCharArray()) + .build(); + + final HttpClientContext context = HttpClientContext.create(); + context.setAuthCache(new BasicAuthCache()); + context.setCredentialsProvider(credentialsProvider); + + for (final String requestPath : new String[]{"/blah/a", "/blubb/b"}) { + final HttpGet httpGet = new HttpGet(requestPath); + client.execute(target, httpGet, context, response -> { + EntityUtils.consume(response.getEntity()); + return null; + }); + } + + Assertions.assertEquals(Arrays.asList(401, 200, 401, 200), responseCodes); + + assertHandshakePerPath(requests, "/blah/a"); + assertHandshakePerPath(requests, "/blubb/b"); + } + + private static void assertHandshakePerPath(final List requests, final String path) { + final List filtered = requests.stream() + .filter(r -> path.equals(r.path)) + .collect(Collectors.toList()); + + Assertions.assertEquals(2, filtered.size(), "Expected 2 requests for " + path + " (challenge + retry)"); + Assertions.assertFalse(filtered.get(0).hasAuthorization, "First request to " + path + " must not be preemptive"); + Assertions.assertTrue(filtered.get(1).hasAuthorization, "Second request to " + path + " must carry Authorization"); + } + + private static final class RequestSnapshot { + private final String path; + private final boolean hasAuthorization; + + private RequestSnapshot(final String path, final boolean hasAuthorization) { + this.path = path; + this.hasAuthorization = hasAuthorization; + } + } + + @Test + void testBasicAuthenticationCredentialsCachingPerPrefixAndReuseWithinPrefix() throws Exception { + final List requests = new CopyOnWriteArrayList<>(); + final Authenticator authenticator = new BasicTestAuthenticator("test:test", "test realm") { + @Override + public AuthResult perform(final URIAuthority authority, + final String requestUri, + final String credentials) { + requests.add(new RequestSnapshot(requestUri, credentials != null)); + return super.perform(authority, requestUri, credentials); + } + }; + configureServerWithBasicAuth(authenticator, bootstrap -> bootstrap.register("*", new EchoHandler())); + final HttpHost target = startServer(); + + final List responseCodes = new CopyOnWriteArrayList<>(); + configureClient(builder -> builder + .addResponseInterceptorLast((response, entity, context) -> responseCodes.add(response.getCode()))); + + final TestClient client = client(); + + final CredentialsProvider credentialsProvider = CredentialsProviderBuilder.create() + .add(target, "test", "test".toCharArray()) + .build(); + + final HttpClientContext context = HttpClientContext.create(); + context.setAuthCache(new BasicAuthCache()); + context.setCredentialsProvider(credentialsProvider); + + // First hit per prefix must challenge; subsequent hits under same prefix should be preemptive (200 only) + for (final String requestPath : new String[]{"/blah/a", "/blubb/b", "/blubb/c", "/blah/d"}) { + final HttpGet httpGet = new HttpGet(requestPath); + client.execute(target, httpGet, context, response -> { + EntityUtils.consume(response.getEntity()); + return null; + }); + } + + Assertions.assertEquals(Arrays.asList(401, 200, 401, 200, 200, 200), responseCodes); + + assertHandshakePerPath(requests, "/blah/a"); + assertHandshakePerPath(requests, "/blubb/b"); + assertPreemptivePerPath(requests, "/blubb/c"); + assertPreemptivePerPath(requests, "/blah/d"); + } + + private static void assertPreemptivePerPath(final List requests, final String path) { + final List filtered = requests.stream() + .filter(r -> path.equals(r.path)) + .collect(Collectors.toList()); + + Assertions.assertEquals(1, filtered.size(), "Expected 1 request for " + path + " (preemptive)"); + Assertions.assertTrue(filtered.get(0).hasAuthorization, "Request to " + path + " must carry Authorization"); + } + + } 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 1b35f8cb34..706793e98b 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 @@ -159,9 +159,6 @@ public void execute( // that of the previous authentication exchange. targetAuthExchange.reset(); } - if (targetAuthExchange.getPathPrefix() == null) { - targetAuthExchange.setPathPrefix(pathPrefix); - } if (authCacheKeeper != null) { authCacheKeeper.loadPreemptively(target, pathPrefix, targetAuthExchange, clientContext); @@ -340,6 +337,9 @@ private boolean needAuthentication( targetNeedsAuth = authenticator.handleResponse(target, ChallengeType.TARGET, response, targetAuthStrategy, targetAuthExchange, context); + if (!targetAuthExchange.isConnectionBased() && targetAuthExchange.getPathPrefix() == null) { + targetAuthExchange.setPathPrefix(pathPrefix); + } if (authCacheKeeper != null) { authCacheKeeper.updateOnResponse(target, pathPrefix, targetAuthExchange, context); } 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 b927dbc798..4514ddfe94 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 @@ -163,9 +163,6 @@ public ClassicHttpResponse execute( // that of the previous authentication exchange. targetAuthExchange.reset(); } - if (targetAuthExchange.getPathPrefix() == null) { - targetAuthExchange.setPathPrefix(pathPrefix); - } if (authCacheKeeper != null) { authCacheKeeper.loadPreemptively(target, pathPrefix, targetAuthExchange, context); @@ -301,6 +298,9 @@ private boolean needAuthentication( targetNeedsAuth = authenticator.handleResponse(target, ChallengeType.TARGET, response, targetAuthStrategy, targetAuthExchange, context); + if (!targetAuthExchange.isConnectionBased() && targetAuthExchange.getPathPrefix() == null) { + targetAuthExchange.setPathPrefix(pathPrefix); + } if (authCacheKeeper != null) { authCacheKeeper.updateOnResponse(target, pathPrefix, targetAuthExchange, context); } 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 new file mode 100644 index 0000000000..d9f9939699 --- /dev/null +++ b/httpclient5/src/test/java/org/apache/hc/client5/http/impl/async/TestAsyncProtocolExec.java @@ -0,0 +1,159 @@ +/* + * ==================================================================== + * 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.impl.async; + +import java.io.IOException; +import java.util.Collections; +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.async.AsyncExecCallback; +import org.apache.hc.client5.http.async.AsyncExecChain; +import org.apache.hc.client5.http.async.AsyncExecRuntime; +import org.apache.hc.client5.http.auth.AuthExchange; +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.BasicScheme; +import org.apache.hc.client5.http.impl.auth.CredentialsProviderBuilder; +import org.apache.hc.client5.http.protocol.HttpClientContext; +import org.apache.hc.core5.concurrent.CancellableDependency; +import org.apache.hc.core5.http.EntityDetails; +import org.apache.hc.core5.http.HttpHeaders; +import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.HttpRequest; +import org.apache.hc.core5.http.HttpResponse; +import org.apache.hc.core5.http.message.BasicHttpResponse; +import org.apache.hc.core5.http.nio.AsyncDataConsumer; +import org.apache.hc.core5.http.nio.AsyncEntityProducer; +import org.apache.hc.core5.http.support.BasicRequestBuilder; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.MockitoAnnotations; + +class TestAsyncProtocolExec { + + @Mock + private AuthenticationStrategy targetAuthStrategy; + @Mock + private AuthenticationStrategy proxyAuthStrategy; + @Mock + private AsyncExecChain chain; + @Mock + private AsyncExecRuntime execRuntime; + + private AsyncProtocolExec protocolExec; + private HttpHost target; + + @BeforeEach + void setup() { + MockitoAnnotations.openMocks(this); + protocolExec = new AsyncProtocolExec(targetAuthStrategy, proxyAuthStrategy, null, true); + target = new HttpHost("http", "foo", 80); + } + + @Test + void testAuthExchangePathPrefixRestoredAfterChallenge() 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()); + + Mockito.when(targetAuthStrategy.select( + Mockito.eq(ChallengeType.TARGET), + Mockito.any(), + Mockito.any())) + .thenReturn(Collections.singletonList(new BasicScheme())); + + Mockito.when(execRuntime.isEndpointConnected()).thenReturn(true); + + final AtomicInteger proceedCount = new AtomicInteger(0); + + Mockito.doAnswer(invocation -> { + final AsyncExecCallback cb = invocation.getArgument(3, AsyncExecCallback.class); + final int i = proceedCount.getAndIncrement(); + + if (i == 0) { + final HttpResponse resp1 = new BasicHttpResponse(401, "Huh?"); + resp1.setHeader(HttpHeaders.WWW_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + cb.handleResponse(resp1, (EntityDetails) null); + cb.completed(); + } else { + final HttpResponse resp2 = new BasicHttpResponse(200, "OK"); + cb.handleResponse(resp2, (EntityDetails) null); + cb.completed(); + } + return null; + }).when(chain).proceed(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); + + final HttpRequest request = BasicRequestBuilder.get("http://foo/blah/a").build(); + + final CancellableDependency dependency = Mockito.mock(CancellableDependency.class); + final AsyncExecChain.Scope scope = new AsyncExecChain.Scope( + "test", + route, + request, + dependency, + context, + execRuntime, + null, + new AtomicInteger(1)); + + final AsyncExecCallback asyncCallback = new AsyncExecCallback() { + @Override + public AsyncDataConsumer handleResponse(final HttpResponse response, final EntityDetails entityDetails) + throws IOException { + return Mockito.mock(AsyncDataConsumer.class); + } + + @Override + public void handleInformationResponse(final HttpResponse response) { + } + + @Override + public void completed() { + } + + @Override + public void failed(final Exception cause) { + Assertions.fail(cause); + } + }; + + protocolExec.execute(request, (AsyncEntityProducer) null, scope, chain, asyncCallback); + + Assertions.assertEquals(2, proceedCount.get()); + + final AuthExchange authExchange = context.getAuthExchange(target); + Assertions.assertEquals("/blah/", authExchange.getPathPrefix()); + } +} 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 9f92bec242..7bf32fea51 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 @@ -276,4 +276,37 @@ void testExecEntityEnclosingRequest() throws Exception { Assertions.assertEquals(401, response.getCode()); } + @Test + void testAuthExchangePathPrefixRestoredAfterChallenge() throws Exception { + final HttpRoute route = new HttpRoute(target); + final ClassicHttpRequest request = new HttpGet("http://foo/blah/a"); + final HttpClientContext context = HttpClientContext.create(); + + context.setCredentialsProvider(CredentialsProviderBuilder.create() + .add(new AuthScope(target), "user", "pass".toCharArray()) + .build()); + + final ClassicHttpResponse response1 = new BasicClassicHttpResponse(401, "Huh?"); + response1.setHeader(HttpHeaders.WWW_AUTHENTICATE, StandardAuthScheme.BASIC + " realm=test"); + final ClassicHttpResponse response2 = new BasicClassicHttpResponse(200, "OK"); + + Mockito.when(chain.proceed(Mockito.same(request), Mockito.any())) + .thenReturn(response1, response2); + + Mockito.when(targetAuthStrategy.select( + Mockito.eq(ChallengeType.TARGET), + 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 AuthExchange authExchange = context.getAuthExchange(target); + Assertions.assertEquals("/blah/", authExchange.getPathPrefix()); + } }