From da264e2d7ec33af6e1f2351deb9b506bd34251ff Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Wed, 12 Aug 2026 17:02:14 +0200 Subject: [PATCH] Honor qualified Cache-Control: private="field" in a shared cache (RFC 9111 section 5.2.2.7) A shared cache now honours the qualified form of the private response directive. Instead of rejecting the whole response, it stores the response with the header fields named by the directive removed from the stored copy, while the response returned to the requesting client retains them; the directive limits only where a field may be stored, not whether it may be delivered to the client that issued the request. The named fields are removed when a response is first stored, when a Vary variant is stored during negotiation, and when a stored entry is updated by a 304 revalidation. On a revalidation the fields are removed from both the 304 response and the entry it updates, so the header merge cannot carry a previously stored field into the updated entry. A bare private directive continues to make the whole response non-storable by a shared cache, and a repeated private directive is last-value-wins. The decision is taken in the protocol layer by CachingExec and AsyncCachingExec, reusing the Cache-Control already parsed for the exchange; the caching backends remain pure storage. The private field list is parsed the same way as the no-cache field list. --- .../http/cache/ResponseCacheControl.java | 38 ++++++- .../http/impl/cache/AsyncCachingExec.java | 49 ++++++--- .../impl/cache/CacheControlHeaderParser.java | 14 +++ .../client5/http/impl/cache/CachingExec.java | 30 ++++-- .../http/impl/cache/CachingExecBase.java | 101 ++++++++++++++++++ .../impl/cache/ResponseCachingPolicy.java | 5 +- .../impl/cache/CacheControlParserTest.java | 32 ++++++ .../impl/cache/TestAsyncCachingExecChain.java | 20 ++++ .../http/impl/cache/TestCachingExecChain.java | 69 +++++++++++- .../impl/cache/TestProtocolRequirements.java | 34 ++++-- .../impl/cache/TestResponseCachingPolicy.java | 15 +++ 11 files changed, 373 insertions(+), 34 deletions(-) diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/ResponseCacheControl.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/ResponseCacheControl.java index 57eafacb23..f439b8a724 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/ResponseCacheControl.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/cache/ResponseCacheControl.java @@ -104,6 +104,11 @@ public final class ResponseCacheControl implements CacheControl { */ private final Set noCacheFields; + /** + * A set of field names specified in the "private" directive of the Cache-Control header. + */ + private final Set privateFields; + private final boolean undefined; /** @@ -128,13 +133,15 @@ public final class ResponseCacheControl implements CacheControl { * @param staleWhileRevalidate The stale-while-revalidate value from the Cache-Control header. * @param staleIfError The stale-if-error value from the Cache-Control header. * @param noCacheFields The set of field names specified in the "no-cache" directive of the Cache-Control header. + * @param privateFields The set of field names specified in the "private" directive of the Cache-Control header. * @param mustUnderstand The must-understand value from the Cache-Control header. * @param immutable The immutable value from the Cache-Control header. */ ResponseCacheControl(final long maxAge, final long sharedMaxAge, final boolean mustRevalidate, final boolean noCache, final boolean noStore, final boolean cachePrivate, final boolean proxyRevalidate, final boolean cachePublic, final long staleWhileRevalidate, final long staleIfError, - final Set noCacheFields, final boolean mustUnderstand, final boolean immutable) { + final Set noCacheFields, final Set privateFields, final boolean mustUnderstand, + final boolean immutable) { this.maxAge = maxAge; this.sharedMaxAge = sharedMaxAge; this.noCache = noCache; @@ -146,6 +153,7 @@ public final class ResponseCacheControl implements CacheControl { this.staleWhileRevalidate = staleWhileRevalidate; this.staleIfError = staleIfError; this.noCacheFields = noCacheFields != null ? Collections.unmodifiableSet(noCacheFields) : Collections.emptySet(); + this.privateFields = privateFields != null ? Collections.unmodifiableSet(privateFields) : Collections.emptySet(); this.undefined = maxAge == -1 && sharedMaxAge == -1 && !noCache && @@ -272,6 +280,16 @@ public Set getNoCacheFields() { return noCacheFields; } + /** + * Returns an unmodifiable set of field names specified in the "private" directive of the Cache-Control header. + * + * @return The set of field names specified in the "private" directive. + * @since 5.7 + */ + public Set getPrivateFields() { + return privateFields; + } + /** * Returns the 'immutable' Cache-Control directive status. * @@ -356,6 +374,7 @@ public static class Builder { private long staleWhileRevalidate = -1; private long staleIfError = -1; private Set noCacheFields; + private Set privateFields; private boolean mustUnderstand; private boolean immutable; @@ -467,6 +486,21 @@ public Builder setNoCacheFields(final String... noCacheFields) { return this; } + public Set getPrivateFields() { + return privateFields; + } + + public Builder setPrivateFields(final Set privateFields) { + this.privateFields = privateFields; + return this; + } + + public Builder setPrivateFields(final String... privateFields) { + this.privateFields = new HashSet<>(); + this.privateFields.addAll(Arrays.asList(privateFields)); + return this; + } + public boolean isMustUnderstand() { return mustUnderstand; } @@ -487,7 +521,7 @@ public Builder setImmutable(final boolean immutable) { public ResponseCacheControl build() { return new ResponseCacheControl(maxAge, sharedMaxAge, mustRevalidate, noCache, noStore, cachePrivate, proxyRevalidate, - cachePublic, staleWhileRevalidate, staleIfError, noCacheFields, mustUnderstand, immutable); + cachePublic, staleWhileRevalidate, staleIfError, noCacheFields, privateFields, mustUnderstand, immutable); } } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java index a928ca75b2..62ccb63a44 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/AsyncCachingExec.java @@ -587,6 +587,7 @@ class CachingAsyncDataConsumer implements AsyncDataConsumer { private final String exchangeId; private final AsyncExecCallback fallback; private final HttpResponse backendResponse; + private final ResponseCacheControl responseCacheControl; private final EntityDetails entityDetails; private final AtomicBoolean writtenThrough; private final AtomicReference bufferRef; @@ -596,10 +597,12 @@ class CachingAsyncDataConsumer implements AsyncDataConsumer { final String exchangeId, final AsyncExecCallback fallback, final HttpResponse backendResponse, + final ResponseCacheControl responseCacheControl, final EntityDetails entityDetails) { this.exchangeId = exchangeId; this.fallback = fallback; this.backendResponse = backendResponse; + this.responseCacheControl = responseCacheControl; this.entityDetails = entityDetails; this.writtenThrough = new AtomicBoolean(false); this.bufferRef = new AtomicReference<>(entityDetails != null ? new ByteArrayBuffer(1024) : null); @@ -741,7 +744,7 @@ public void cancelled() { LOG.debug("{} caching backend response", exchangeId); } final CachingAsyncDataConsumer cachingDataConsumer = new CachingAsyncDataConsumer( - exchangeId, asyncExecCallback, backendResponse, entityDetails); + exchangeId, asyncExecCallback, backendResponse, responseCacheControl, entityDetails); cachingConsumerRef.set(cachingDataConsumer); return cachingDataConsumer; } @@ -756,14 +759,15 @@ public void handleInformationResponse(final HttpResponse response) throws HttpEx asyncExecCallback.handleInformationResponse(response); } - void triggerNewCacheEntryResponse(final HttpResponse backendResponse, final Instant responseDate, final ByteArrayBuffer buffer) { + void triggerNewCacheEntryResponse(final HttpResponse backendResponse, final ResponseCacheControl responseCacheControl, + final Instant responseDate, final ByteArrayBuffer buffer) { final String exchangeId = scope.exchangeId; final HttpCacheContext context = HttpCacheContext.cast(scope.clientContext); final CancellableDependency operation = scope.cancellableDependency; operation.setDependency(responseCache.store( target, request, - backendResponse, + responseToStore(responseCacheControl, backendResponse), buffer, requestDate, responseDate, @@ -776,6 +780,7 @@ public void completed(final CacheHit hit) { } try { final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, hit.entry); + restorePrivateFields(cacheResponse, responseCacheControl, backendResponse); context.setCacheEntry(hit.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); } catch (final ResourceIOException ex) { @@ -808,6 +813,19 @@ void triggerCachedResponse(final HttpCacheEntry entry) { } } + void triggerCachedResponse(final HttpCacheEntry entry, final ResponseCacheControl responseCacheControl, + final HttpResponse originResponse) { + final HttpCacheContext context = HttpCacheContext.cast(scope.clientContext); + try { + final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, entry); + restorePrivateFields(cacheResponse, responseCacheControl, originResponse); + context.setCacheEntry(entry); + triggerResponse(cacheResponse, scope, asyncExecCallback); + } catch (final ResourceIOException ex) { + asyncExecCallback.failed(ex); + } + } + @Override public void completed() { final String exchangeId = scope.exchangeId; @@ -817,6 +835,7 @@ public void completed() { return; } final HttpResponse backendResponse = cachingDataConsumer.backendResponse; + final ResponseCacheControl responseCacheControl = cachingDataConsumer.responseCacheControl; final ByteArrayBuffer buffer = cachingDataConsumer.bufferRef.getAndSet(null); // Handle 304 Not Modified responses @@ -831,10 +850,10 @@ public void completed(final CacheMatch result) { LOG.debug("{} existing cache entry found, updating cache entry", exchangeId); } responseCache.update( - hit, + hitToStore(responseCacheControl, hit), target, request, - backendResponse, + responseToStore(responseCacheControl, backendResponse), requestDate, responseDate, new FutureCallback() { @@ -844,7 +863,7 @@ public void completed(final CacheHit updated) { if (LOG.isDebugEnabled()) { LOG.debug("{} cache entry updated, generating response from updated entry", exchangeId); } - triggerCachedResponse(updated.entry); + triggerCachedResponse(updated.entry, responseCacheControl, backendResponse); } @Override public void failed(final Exception cause) { @@ -864,7 +883,7 @@ public void cancelled() { }); } else { - triggerNewCacheEntryResponse(backendResponse, responseDate, buffer); + triggerNewCacheEntryResponse(backendResponse, responseCacheControl, responseDate, buffer); } } @@ -893,7 +912,7 @@ public void completed(final CacheMatch result) { } triggerCachedResponse(hit.entry); } else { - triggerNewCacheEntryResponse(backendResponse, responseDate, buffer); + triggerNewCacheEntryResponse(backendResponse, responseCacheControl, responseDate, buffer); } } @@ -909,7 +928,7 @@ public void cancelled() { })); } else { - triggerNewCacheEntryResponse(backendResponse, responseDate, buffer); + triggerNewCacheEntryResponse(backendResponse, responseCacheControl, responseDate, buffer); } } } @@ -1062,11 +1081,12 @@ void revalidateCacheEntry( void triggerUpdatedCacheEntryResponse(final HttpResponse backendResponse, final Instant responseDate) { final CancellableDependency operation = scope.cancellableDependency; + final ResponseCacheControl backendCacheControl = CacheControlHeaderParser.INSTANCE.parse(backendResponse); operation.setDependency(responseCache.update( - hit, + hitToStore(backendCacheControl, hit), target, request, - backendResponse, + responseToStore(backendCacheControl, backendResponse), requestDate, responseDate, new FutureCallback() { @@ -1075,6 +1095,7 @@ void triggerUpdatedCacheEntryResponse(final HttpResponse backendResponse, final public void completed(final CacheHit updated) { try { final SimpleHttpResponse cacheResponse = generateCachedResponse(request, updated.entry, responseDate); + restorePrivateFields(cacheResponse, backendCacheControl, backendResponse); context.setCacheEntry(updated.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); } catch (final ResourceIOException ex) { @@ -1422,11 +1443,12 @@ void updateVariantCacheEntry(final HttpResponse backendResponse, final Instant r context.setCacheResponseStatus(CacheResponseStatus.VALIDATED); cacheUpdates.getAndIncrement(); + final ResponseCacheControl backendCacheControl = CacheControlHeaderParser.INSTANCE.parse(backendResponse); operation.setDependency(responseCache.storeFromNegotiated( - match, + hitToStore(backendCacheControl, match), target, request, - backendResponse, + responseToStore(backendCacheControl, backendResponse), requestDate, responseDate, new FutureCallback() { @@ -1435,6 +1457,7 @@ void updateVariantCacheEntry(final HttpResponse backendResponse, final Instant r public void completed(final CacheHit hit) { try { final SimpleHttpResponse cacheResponse = generateCachedResponse(request, hit.entry, responseDate); + restorePrivateFields(cacheResponse, backendCacheControl, backendResponse); context.setCacheEntry(hit.entry); triggerResponse(cacheResponse, scope, asyncExecCallback); } catch (final ResourceIOException ex) { diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java index 6aae254507..ce15a9d05c 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CacheControlHeaderParser.java @@ -189,6 +189,20 @@ public final ResponseCacheControl parseResponse(final Iterator
headerIte builder.setNoStore(true); } else if (name.equalsIgnoreCase(HeaderConstants.CACHE_CONTROL_PRIVATE)) { builder.setCachePrivate(true); + if (value != null) { + final Tokenizer.Cursor valCursor = new ParserCursor(0, value.length()); + final Set privateFields = new HashSet<>(); + while (!valCursor.atEnd()) { + final String token = tokenParser.parseToken(value, valCursor, VALUE_DELIMS); + if (!TextUtils.isBlank(token)) { + privateFields.add(token); + } + if (!valCursor.atEnd()) { + valCursor.updatePos(valCursor.getPos() + 1); + } + } + builder.setPrivateFields(privateFields); + } } else if (name.equalsIgnoreCase(HeaderConstants.CACHE_CONTROL_PROXY_REVALIDATE)) { builder.setProxyRevalidate(true); } else if (name.equalsIgnoreCase(HeaderConstants.CACHE_CONTROL_PUBLIC)) { diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java index 1b745ffce7..31dca9fe0a 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExec.java @@ -59,6 +59,7 @@ import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.HttpVersion; import org.apache.hc.core5.http.Method; @@ -429,8 +430,11 @@ ClassicHttpResponse revalidateCacheEntry( cacheUpdates.getAndIncrement(); } if (statusCode == HttpStatus.SC_NOT_MODIFIED) { - final CacheHit updated = responseCache.update(hit, target, request, backendResponse, requestDate, responseDate); + final ResponseCacheControl backendCacheControl = CacheControlHeaderParser.INSTANCE.parse(backendResponse); + final CacheHit updated = responseCache.update(hitToStore(backendCacheControl, hit), target, request, + responseToStore(backendCacheControl, backendResponse), requestDate, responseDate); final SimpleHttpResponse cacheResponse = generateCachedResponse(request, updated.entry, responseDate); + restorePrivateFields(cacheResponse, backendCacheControl, backendResponse); context.setCacheEntry(updated.entry); return convert(cacheResponse); } @@ -531,7 +535,7 @@ ClassicHttpResponse handleBackendResponse( if (LOG.isDebugEnabled()) { LOG.debug("{} caching backend response", exchangeId); } - return cacheAndReturnResponse(target, request, scope, backendResponse, requestDate, responseDate); + return cacheAndReturnResponse(target, request, scope, responseCacheControl, backendResponse, requestDate, responseDate); } if (LOG.isDebugEnabled()) { LOG.debug("{} backend response is not cacheable", exchangeId); @@ -543,6 +547,7 @@ ClassicHttpResponse cacheAndReturnResponse( final HttpHost target, final SimpleHttpRequest request, final ExecChain.Scope scope, + final ResponseCacheControl responseCacheControl, final ClassicHttpResponse backendResponse, final Instant requestSent, final Instant responseReceived) throws IOException { @@ -555,13 +560,14 @@ ClassicHttpResponse cacheAndReturnResponse( final CacheHit hit = result != null ? result.hit : null; if (hit != null) { final CacheHit updated = responseCache.update( - hit, + hitToStore(responseCacheControl, hit), target, request, - backendResponse, + responseToStore(responseCacheControl, backendResponse), requestSent, responseReceived); final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, updated.entry); + restorePrivateFields(cacheResponse, responseCacheControl, backendResponse); context.setCacheEntry(hit.entry); return convert(cacheResponse); } @@ -591,7 +597,9 @@ ClassicHttpResponse cacheAndReturnResponse( } backendResponse.close(); + final HttpResponse responseToCache = responseToStore(responseCacheControl, backendResponse); CacheHit hit; + boolean stored = false; if (cacheConfig.isFreshnessCheckEnabled() && statusCode != HttpStatus.SC_NOT_MODIFIED) { final CacheMatch result = responseCache.match(target ,request); hit = result != null ? result.hit : null; @@ -600,18 +608,23 @@ ClassicHttpResponse cacheAndReturnResponse( LOG.debug("{} backend already contains fresher cache entry", exchangeId); } } else { - hit = responseCache.store(target, request, backendResponse, buf, requestSent, responseReceived); + hit = responseCache.store(target, request, responseToCache, buf, requestSent, responseReceived); + stored = true; if (LOG.isDebugEnabled()) { LOG.debug("{} backend response successfully cached", exchangeId); } } } else { - hit = responseCache.store(target, request, backendResponse, buf, requestSent, responseReceived); + hit = responseCache.store(target, request, responseToCache, buf, requestSent, responseReceived); + stored = true; if (LOG.isDebugEnabled()) { LOG.debug("{} backend response successfully cached (freshness check skipped)", exchangeId); } } final SimpleHttpResponse cacheResponse = responseGenerator.generateResponse(request, hit.entry); + if (stored) { + restorePrivateFields(cacheResponse, responseCacheControl, backendResponse); + } context.setCacheEntry(hit.entry); return convert(cacheResponse); } @@ -705,8 +718,11 @@ ClassicHttpResponse negotiateResponseFromVariants( context.setCacheResponseStatus(CacheResponseStatus.VALIDATED); cacheUpdates.getAndIncrement(); - final CacheHit hit = responseCache.storeFromNegotiated(match, target, request, backendResponse, requestDate, responseDate); + final ResponseCacheControl backendCacheControl = CacheControlHeaderParser.INSTANCE.parse(backendResponse); + final CacheHit hit = responseCache.storeFromNegotiated(hitToStore(backendCacheControl, match), target, request, + responseToStore(backendCacheControl, backendResponse), requestDate, responseDate); final SimpleHttpResponse cacheResponse = generateCachedResponse(request, hit.entry, responseDate); + restorePrivateFields(cacheResponse, backendCacheControl, backendResponse); context.setCacheEntry(hit.entry); return convert(cacheResponse); } catch (final IOException | RuntimeException ex) { diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java index 53b6a92b6c..e04cc10fd0 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/CachingExecBase.java @@ -27,11 +27,14 @@ package org.apache.hc.client5.http.impl.cache; import java.time.Instant; +import java.util.Iterator; +import java.util.Set; import java.util.concurrent.atomic.AtomicLong; import org.apache.hc.client5.http.async.methods.SimpleHttpResponse; import org.apache.hc.client5.http.cache.HttpCacheEntry; import org.apache.hc.client5.http.cache.ResourceIOException; +import org.apache.hc.client5.http.cache.ResponseCacheControl; import org.apache.hc.core5.http.EntityDetails; import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpHeaders; @@ -39,6 +42,8 @@ import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.Method; +import org.apache.hc.core5.http.message.BasicHttpResponse; +import org.apache.hc.core5.http.message.HeaderGroup; public class CachingExecBase { @@ -129,6 +134,102 @@ SimpleHttpResponse generateGatewayTimeout() { return SimpleHttpResponse.create(HttpStatus.SC_GATEWAY_TIMEOUT, "Gateway Timeout"); } + /** + * Returns the response to be persisted by the cache. When the cache is shared and the response + * carries a qualified {@code private} directive, the header fields named by that directive are + * removed from a copy so that a shared cache does not store them. The original response is + * returned unchanged when the cache is not shared or the directive names no field. + */ + HttpResponse responseToStore(final ResponseCacheControl responseCacheControl, final HttpResponse originResponse) { + final Set privateFields = responseCacheControl.getPrivateFields(); + if (!cacheConfig.isSharedCache() || !responseCacheControl.isCachePrivate() || privateFields.isEmpty()) { + return originResponse; + } + final BasicHttpResponse stripped = new BasicHttpResponse(originResponse.getCode(), originResponse.getReasonPhrase()); + stripped.setVersion(originResponse.getVersion()); + stripped.setHeaders(originResponse.getHeaders()); + for (final String field : privateFields) { + stripped.removeHeaders(field); + } + return stripped; + } + + /** + * Re-attaches the header fields named by a qualified {@code private} directive to the response + * returned to the requesting client. The directive limits only where the fields may be stored, + * not whether they may be delivered to the client that issued the request, so the fields removed + * from the stored copy by {@link #responseToStore} are restored here from the origin response. + */ + void restorePrivateFields(final SimpleHttpResponse response, final ResponseCacheControl responseCacheControl, + final HttpResponse originResponse) { + final Set privateFields = responseCacheControl.getPrivateFields(); + if (!cacheConfig.isSharedCache() || !responseCacheControl.isCachePrivate() || privateFields.isEmpty()) { + return; + } + for (final String field : privateFields) { + final Header[] originHeaders = originResponse.getHeaders(field); + if (originHeaders.length > 0) { + response.removeHeaders(field); + for (final Header header : originHeaders) { + response.addHeader(header); + } + } + } + } + + /** + * Returns the stored entry a 304 revalidation should update, with the fields named by a qualified + * {@code private} directive removed. This complements {@link #responseToStore}: because the header + * merge preserves a stored field when the 304 does not carry it, a shared cache must strip the field + * from both the stored entry and the 304 to keep it out of the updated entry. The entry is returned + * unchanged when the cache is not shared, the directive names no field, or the entry does not carry + * any of the named fields. + */ + HttpCacheEntry entryToStore(final ResponseCacheControl responseCacheControl, final HttpCacheEntry entry) { + final Set privateFields = responseCacheControl.getPrivateFields(); + if (!cacheConfig.isSharedCache() || !responseCacheControl.isCachePrivate() || privateFields.isEmpty()) { + return entry; + } + final HeaderGroup responseHeaders = new HeaderGroup(); + for (final Iterator
it = entry.headerIterator(); it.hasNext(); ) { + responseHeaders.addHeader(it.next()); + } + boolean modified = false; + for (final String field : privateFields) { + if (responseHeaders.containsHeader(field)) { + responseHeaders.removeHeaders(field); + modified = true; + } + } + if (!modified) { + return entry; + } + final HeaderGroup requestHeaders = new HeaderGroup(); + for (final Iterator
it = entry.requestHeaderIterator(); it.hasNext(); ) { + requestHeaders.addHeader(it.next()); + } + return new HttpCacheEntry( + entry.getRequestInstant(), + entry.getResponseInstant(), + entry.getRequestMethod(), + entry.getRequestURI(), + requestHeaders, + entry.getRequestContent(), + entry.getStatus(), + responseHeaders, + entry.getResource(), + entry.hasVariants() ? entry.getVariants() : null); + } + + /** + * Returns the {@link CacheHit} a 304 revalidation should update, with the stored entry passed + * through {@link #entryToStore}. The original hit is returned when no field is stripped. + */ + CacheHit hitToStore(final ResponseCacheControl responseCacheControl, final CacheHit hit) { + final HttpCacheEntry stripped = entryToStore(responseCacheControl, hit.entry); + return stripped == hit.entry ? hit : new CacheHit(hit.rootKey, hit.variantKey, stripped); + } + Instant getCurrentDate() { return Instant.now(); } diff --git a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java index 77b38a493f..4cfa7349c7 100644 --- a/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java +++ b/httpclient5-cache/src/main/java/org/apache/hc/client5/http/impl/cache/ResponseCachingPolicy.java @@ -146,7 +146,7 @@ public boolean isResponseCacheable(final RequestCacheControl requestCacheControl return false; } // Status code is in a recognized range; treat no-store as overridden. - if (sharedCache && cacheControl.isCachePrivate()) { + if (sharedCache && cacheControl.isCachePrivate() && cacheControl.getPrivateFields().isEmpty()) { LOG.debug("Response is private and cannot be cached by a shared cache"); return false; } @@ -255,7 +255,8 @@ protected boolean isExplicitlyNonCacheable(final ResponseCacheControl cacheContr // The response is considered explicitly non-cacheable if it contains // "no-store" or (if sharedCache is true) "private" directives. // Note that "no-cache" is considered cacheable but requires validation before use. - return cacheControl.isNoStore() || sharedCache && cacheControl.isCachePrivate(); + return cacheControl.isNoStore() + || sharedCache && cacheControl.isCachePrivate() && cacheControl.getPrivateFields().isEmpty(); } protected boolean isExplicitlyCacheable(final ResponseCacheControl cacheControl, final HttpResponse response) { diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/CacheControlParserTest.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/CacheControlParserTest.java index 49f6c3609b..8b7ec7542d 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/CacheControlParserTest.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/CacheControlParserTest.java @@ -28,6 +28,7 @@ import static org.junit.jupiter.api.Assertions.assertAll; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; @@ -207,6 +208,37 @@ void testParseNoCacheFieldsMessy() { assertEquals(120, cacheControl.getStaleWhileRevalidate()); } + @Test + void testParsePrivateFields() { + final Header header = new BasicHeader("Cache-Control", "private=\"X-Private, X-Secret\", s-maxage=3600"); + final ResponseCacheControl cacheControl = parser.parseResponse(Collections.singletonList(header).iterator()); + + assertTrue(cacheControl.isCachePrivate()); + assertEquals(2, cacheControl.getPrivateFields().size()); + assertTrue(cacheControl.getPrivateFields().contains("X-Private")); + assertTrue(cacheControl.getPrivateFields().contains("X-Secret")); + } + + @Test + void testParseBarePrivateHasNoFields() { + final Header header = new BasicHeader("Cache-Control", "private, s-maxage=3600"); + final ResponseCacheControl cacheControl = parser.parseResponse(Collections.singletonList(header).iterator()); + + assertTrue(cacheControl.isCachePrivate()); + assertTrue(cacheControl.getPrivateFields().isEmpty()); + } + + @Test + void testParseMultiplePrivateDirectivesLastWins() { + // A repeated private directive is not cumulative; only the last one takes effect. + final Header header = new BasicHeader("Cache-Control", "private=\"X-A\", private=\"X-B\", s-maxage=3600"); + final ResponseCacheControl cacheControl = parser.parseResponse(Collections.singletonList(header).iterator()); + + assertTrue(cacheControl.isCachePrivate()); + assertEquals(1, cacheControl.getPrivateFields().size()); + assertTrue(cacheControl.getPrivateFields().contains("X-B")); + assertFalse(cacheControl.getPrivateFields().contains("X-A")); + } @Test void testParseMultipleHeaders() { diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestAsyncCachingExecChain.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestAsyncCachingExecChain.java index 61c4146c48..22e05c8e59 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestAsyncCachingExecChain.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestAsyncCachingExecChain.java @@ -32,8 +32,10 @@ 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.async.methods.SimpleHttpResponse; import org.apache.hc.client5.http.cache.CacheResponseStatus; import org.apache.hc.client5.http.cache.HttpCacheContext; +import org.apache.hc.client5.http.cache.ResponseCacheControl; import org.apache.hc.core5.concurrent.CancellableDependency; import org.apache.hc.core5.http.HttpHeaders; import org.apache.hc.core5.http.HttpHost; @@ -42,6 +44,7 @@ import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.Method; import org.apache.hc.core5.http.message.BasicHttpRequest; +import org.apache.hc.core5.http.message.BasicHttpResponse; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -99,4 +102,21 @@ void testOnlyIfCachedAndNoCacheEntryBackendNotCalled() throws Exception { Assertions.assertEquals(CacheResponseStatus.CACHE_MODULE_RESPONSE, context.getCacheResponseStatus()); } + @Test + void testSharedCacheStripsQualifiedPrivateFieldFromStoredCopyButRetainsForClient() { + final HttpResponse origin = new BasicHttpResponse(HttpStatus.SC_OK, "OK"); + origin.setHeader("Cache-Control", "max-age=3600, private=\"X-Personal\""); + origin.setHeader("X-Personal", "secret"); + final ResponseCacheControl cacheControl = CacheControlHeaderParser.INSTANCE.parse(origin); + + // A shared cache must not store the field named by the qualified private directive. + final HttpResponse stored = impl.responseToStore(cacheControl, origin); + Assertions.assertFalse(stored.containsHeader("X-Personal")); + + // The qualified private directive limits only storage; the requesting client still receives the field. + final SimpleHttpResponse clientResponse = SimpleHttpResponse.create(HttpStatus.SC_OK, "OK"); + impl.restorePrivateFields(clientResponse, cacheControl, origin); + Assertions.assertTrue(clientResponse.containsHeader("X-Personal")); + } + } diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java index 2a05f79463..c102920b60 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestCachingExecChain.java @@ -47,7 +47,9 @@ import org.apache.hc.client5.http.cache.CacheResponseStatus; import org.apache.hc.client5.http.cache.HttpCacheContext; import org.apache.hc.client5.http.cache.HttpCacheEntry; +import org.apache.hc.client5.http.cache.HttpCacheEntryFactory; import org.apache.hc.client5.http.cache.HttpCacheStorage; +import org.apache.hc.client5.http.cache.ResponseCacheControl; import org.apache.hc.client5.http.classic.ExecChain; import org.apache.hc.client5.http.classic.ExecRuntime; import org.apache.hc.client5.http.classic.methods.HttpGet; @@ -61,6 +63,7 @@ import org.apache.hc.core5.http.HttpEntity; import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpHost; +import org.apache.hc.core5.http.HttpResponse; import org.apache.hc.core5.http.HttpStatus; import org.apache.hc.core5.http.Method; import org.apache.hc.core5.http.io.entity.ByteArrayEntity; @@ -76,6 +79,7 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.Mockito; import org.mockito.MockitoAnnotations; @@ -142,6 +146,65 @@ void testCacheableResponsesGoIntoCache() throws Exception { Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); } + @Test + void testSharedCacheStripsQualifiedPrivateFieldFromStoredCopyButReturnsItToClient() throws Exception { + final ClassicHttpRequest req = HttpTestUtils.makeDefaultRequest(); + final ClassicHttpResponse resp = HttpTestUtils.make200Response(); + resp.setHeader("Cache-Control", "max-age=3600, private=\"X-Personal\""); + resp.setHeader("X-Personal", "secret"); + + Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp); + + final ClassicHttpResponse clientResponse = execute(req); + + // The qualified private directive limits only storage; the requesting client still receives the field. + Assertions.assertTrue(clientResponse.containsHeader("X-Personal")); + + // The shared cache must not store the field named by the qualified private directive. + final ArgumentCaptor stored = ArgumentCaptor.forClass(HttpResponse.class); + Mockito.verify(cache).store(Mockito.eq(host), Mockito.any(), stored.capture(), + Mockito.any(), Mockito.any(), Mockito.any()); + Assertions.assertFalse(stored.getValue().containsHeader("X-Personal")); + } + + @Test + void testSharedCacheDoesNotStoreResponseWithBarePrivateDirective() throws Exception { + final ClassicHttpRequest req = HttpTestUtils.makeDefaultRequest(); + final ClassicHttpResponse resp = HttpTestUtils.make200Response(); + resp.setHeader("Cache-Control", "max-age=3600, private"); + + Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp); + + execute(req); + + Mockito.verify(cache, Mockito.never()).store(Mockito.any(), Mockito.any(), + Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); + } + + @Test + void testQualifiedPrivateFieldStrippedFrom304MergedEntryForSharedCache() { + // A shared cache holds an entry the origin did not originally mark private, so it carries X-Personal. + final HttpCacheEntry staleEntry = HttpTestUtils.makeCacheEntry( + new BasicHeader("Cache-Control", "max-age=3600"), + new BasicHeader("ETag", "\"v1\""), + new BasicHeader("X-Personal", "old-secret")); + + // A 304 revalidation now marks X-Personal private and re-sends a fresh value. + final ClassicHttpResponse response = HttpTestUtils.make304Response(); + response.setHeader("Cache-Control", "private=\"X-Personal\""); + response.setHeader("X-Personal", "new-secret"); + final ResponseCacheControl cacheControl = CacheControlHeaderParser.INSTANCE.parse(response); + + // The exec strips the field from both the stored entry and the 304 before the merge, so the + // header merge (which keeps a stored header the 304 does not carry) cannot reintroduce it. + final HttpCacheEntry merged = HttpCacheEntryFactory.INSTANCE.createUpdated( + Instant.now(), Instant.now(), host, HttpTestUtils.makeDefaultRequest(), + impl.responseToStore(cacheControl, response), + impl.entryToStore(cacheControl, staleEntry)); + + Assertions.assertFalse(merged.containsHeader("X-Personal")); + } + @Test void testResponseToRequestWithNoStoreIsNotCached() throws Exception { final ClassicHttpRequest req = HttpTestUtils.makeDefaultRequest(); @@ -928,7 +991,7 @@ void testTooLargeResponsesAreNotCached() throws Exception { originResponse.setHeader("Date", DateUtils.formatStandardDate(responseGenerated)); originResponse.setHeader("ETag", "\"etag\""); - impl.cacheAndReturnResponse(host, cacheRequest, scope, originResponse, requestSent, responseReceived); + impl.cacheAndReturnResponse(host, cacheRequest, scope, CacheControlHeaderParser.INSTANCE.parse(originResponse), originResponse, requestSent, responseReceived); Mockito.verify(cache, Mockito.never()).store( Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any()); @@ -965,7 +1028,7 @@ void testSmallEnoughResponsesAreCached() throws Exception { Mockito.eq(requestSent), Mockito.eq(responseReceived))).thenReturn(new CacheHit("key", httpCacheEntry)); - impl.cacheAndReturnResponse(host, cacheRequest, scope, originResponse, requestSent, responseReceived); + impl.cacheAndReturnResponse(host, cacheRequest, scope, CacheControlHeaderParser.INSTANCE.parse(originResponse), originResponse, requestSent, responseReceived); Mockito.verify(mockCache).store( Mockito.any(), @@ -1263,7 +1326,7 @@ void testNotModifiedResponseUpdatesCacheEntry() throws Exception { .thenReturn(new CacheHit("key", cacheEntry)); // Call cacheAndReturnResponse with 304 Not Modified response - final ClassicHttpResponse cachedResponse = impl.cacheAndReturnResponse(host, cacheRequest, scope, backendResponse, requestSent, responseReceived); + final ClassicHttpResponse cachedResponse = impl.cacheAndReturnResponse(host, cacheRequest, scope, CacheControlHeaderParser.INSTANCE.parse(backendResponse), backendResponse, requestSent, responseReceived); // Verify cache entry is updated Mockito.verify(mockCache).update( diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java index 1e96940ac3..e1792a71d8 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestProtocolRequirements.java @@ -1954,22 +1954,42 @@ void testCacheControlPrivateOnFieldIsNotReturnedBySharedCache() throws Exception final ClassicHttpRequest req1 = new BasicClassicHttpRequest("GET", "/"); final ClassicHttpResponse resp1 = HttpTestUtils.make200Response(); resp1.setHeader("X-Personal", "stuff"); - resp1.setHeader("Cache-Control", "private=\"X-Personal\",s-maxage=3600"); + resp1.setHeader("X-Public", "ok"); + resp1.setHeader("Cache-Control", "private=\"X-Personal\", s-maxage=3600"); Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp1); final ClassicHttpRequest req2 = new BasicClassicHttpRequest("GET", "/"); - final ClassicHttpResponse resp2 = HttpTestUtils.make200Response(); - // this backend request MAY happen - Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp2); + final ClassicHttpResponse first = execute(req1); + Assertions.assertNotNull(first.getFirstHeader("X-Personal")); - execute(req1); final ClassicHttpResponse result = execute(req2); Assertions.assertNull(result.getFirstHeader("X-Personal")); + Assertions.assertNotNull(result.getFirstHeader("X-Public")); + Mockito.verify(mockExecChain, Mockito.times(1)).proceed(Mockito.any(), Mockito.any()); + } + } - Mockito.verify(mockExecChain, Mockito.atLeastOnce()).proceed(Mockito.any(), Mockito.any()); - Mockito.verify(mockExecChain, Mockito.atMost(2)).proceed(Mockito.any(), Mockito.any()); + @Test + void testPrivateFieldRemovedFromEntryUpdatedByRevalidation() throws Exception { + if (config.isSharedCache()) { + final ClassicHttpRequest req1 = new BasicClassicHttpRequest("GET", "/"); + final ClassicHttpResponse resp1 = HttpTestUtils.make200Response(); + resp1.setHeader("X-Personal", "stuff"); + resp1.setHeader("ETag", "\"v1\""); + resp1.setHeader("Cache-Control", "max-age=0"); + Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp1); + execute(req1); + + final ClassicHttpResponse resp304 = new BasicClassicHttpResponse(HttpStatus.SC_NOT_MODIFIED); + resp304.setHeader("ETag", "\"v1\""); + resp304.setHeader("Cache-Control", "private=\"X-Personal\", s-maxage=3600"); + Mockito.when(mockExecChain.proceed(Mockito.any(), Mockito.any())).thenReturn(resp304); + execute(new BasicClassicHttpRequest("GET", "/")); + + final ClassicHttpResponse result = execute(new BasicClassicHttpRequest("GET", "/")); + Assertions.assertNull(result.getFirstHeader("X-Personal")); } } diff --git a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java index 12b0050e41..5bd09019d3 100644 --- a/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java +++ b/httpclient5-cache/src/test/java/org/apache/hc/client5/http/impl/cache/TestResponseCachingPolicy.java @@ -109,6 +109,21 @@ void testArbitraryMethodNotCacheable() { Assertions.assertFalse(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response)); } + @Test + void testBarePrivateNotCacheableInSharedCache() { + policy = new ResponseCachingPolicy(true, false, false); + responseCacheControl = ResponseCacheControl.builder().setCachePrivate(true).build(); + Assertions.assertFalse(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response)); + } + + @Test + void testQualifiedPrivateCacheableInSharedCache() { + policy = new ResponseCachingPolicy(true, false, false); + responseCacheControl = ResponseCacheControl.builder() + .setCachePrivate(true).setPrivateFields("X-Private").build(); + Assertions.assertTrue(policy.isResponseCacheable(requestCacheControl, responseCacheControl, request, response)); + } + @Test void testResponseToRequestWithNoStoreIsNotCacheable() { request = new BasicHttpRequest(Method.GET, "/");