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, "/");