HTTPCLIENT-1371: Weak ETag Validation is Useful On PUT With If-Match

Contributed by James Leigh <james at 3roundstones dot com>


git-svn-id: https://svn.apache.org/repos/asf/httpcomponents/httpclient/trunk@1519005 13f79535-47bb-0310-9956-ffa450edef68

RELEASE_NOTES.txt

@@ -34,6 +34,8 @@ This release also includes all fixes from the stable 4.2.x release branch.
 
 Changelog
 -------------------
+* [HTTPCLIENT-1371] Weak ETag Validation is Useful On PUT With If-Match
+  Contributed by James Leigh <james at 3roundstones dot com>
 
 * [HTTPCLIENT-1384] Expose CacheInvalidator interface.
   Contributed by Nicolas Richeton <nicolas.richeton at free.fr>

httpclient-cache/src/main/java-deprecated/org/apache/http/impl/client/cache/CachingHttpClient.java

@@ -182,7 +182,7 @@ public class CachingHttpClient implements HttpClient {
         this.conditionalRequestBuilder = new ConditionalRequestBuilder();
 
         this.responseCompliance = new ResponseProtocolCompliance();
-        this.requestCompliance = new RequestProtocolCompliance();
+        this.requestCompliance = new RequestProtocolCompliance(config.isWeakETagOnPutDeleteAllowed());
 
         this.asynchRevalidator = makeAsynchronousValidator(config);
     }

httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CacheConfig.java

@@ -65,6 +65,14 @@ import org.apache.http.util.Args;
  * adherence to the existing standard, but you may want to
  * {@link Builder#setAllow303Caching(boolean) enable it}.
  *
+ * <p><b>Weak ETags on PUT/DELETE If-Match requests</b>. RFC2616 explicitly
+ * prohibits the use of weak validators in non-GET requests, however, the
+ * HTTPbis working group says while the limitation for weak validators on ranged
+ * requests makes sense, weak ETag validation is useful on full non-GET
+ * requests; e.g., PUT with If-Match. This behavior is off by default, to err on
+ * the side of a conservative adherence to the existing standard, but you may
+ * want to {@link Builder#setWeakETagOnPutDeleteAllowed(boolean) enable it}.
+ *
  * <p><b>Heuristic caching</b>. Per RFC2616, a cache may cache certain cache
  * entries even if no explicit cache control headers are set by the origin.
  * This behavior is off by default, but you may want to turn this on if you
@@ -113,6 +121,10 @@ public class CacheConfig implements Cloneable {
      */
     public final static boolean DEFAULT_303_CACHING_ENABLED = false;
 
+    /** Default setting to allow weak tags on PUT/DELETE methods
+     */
+    public final static boolean DEFAULT_WEAK_ETAG_ON_PUTDELETE_ALLOWED = false;
+
     /** Default setting for heuristic caching
      */
     public final static boolean DEFAULT_HEURISTIC_CACHING_ENABLED = false;
@@ -153,6 +165,7 @@ public class CacheConfig implements Cloneable {
     private int maxCacheEntries;
     private int maxUpdateRetries;
     private boolean allow303Caching;
+    private boolean weakETagOnPutDeleteAllowed;
     private boolean heuristicCachingEnabled;
     private float heuristicCoefficient;
     private long heuristicDefaultLifetime;
@@ -172,8 +185,9 @@ public class CacheConfig implements Cloneable {
         this.maxObjectSize = DEFAULT_MAX_OBJECT_SIZE_BYTES;
         this.maxCacheEntries = DEFAULT_MAX_CACHE_ENTRIES;
         this.maxUpdateRetries = DEFAULT_MAX_UPDATE_RETRIES;
-        this.allow303Caching = false;
+        this.allow303Caching = DEFAULT_303_CACHING_ENABLED;
-        this.heuristicCachingEnabled = false;
+        this.weakETagOnPutDeleteAllowed = DEFAULT_WEAK_ETAG_ON_PUTDELETE_ALLOWED;
+        this.heuristicCachingEnabled = DEFAULT_HEURISTIC_CACHING_ENABLED;
         this.heuristicCoefficient = DEFAULT_HEURISTIC_COEFFICIENT;
         this.heuristicDefaultLifetime = DEFAULT_HEURISTIC_LIFETIME;
         this.isSharedCache = true;
@@ -188,6 +202,7 @@ public class CacheConfig implements Cloneable {
             final int maxCacheEntries,
             final int maxUpdateRetries,
             final boolean allow303Caching,
+            final boolean weakETagOnPutDeleteAllowed,
             final boolean heuristicCachingEnabled,
             final float heuristicCoefficient,
             final long heuristicDefaultLifetime,
@@ -202,6 +217,7 @@ public class CacheConfig implements Cloneable {
         this.maxCacheEntries = maxCacheEntries;
         this.maxUpdateRetries = maxUpdateRetries;
         this.allow303Caching = allow303Caching;
+        this.weakETagOnPutDeleteAllowed = weakETagOnPutDeleteAllowed;
         this.heuristicCachingEnabled = heuristicCachingEnabled;
         this.heuristicCoefficient = heuristicCoefficient;
         this.heuristicDefaultLifetime = heuristicDefaultLifetime;
@@ -312,6 +328,14 @@ public class CacheConfig implements Cloneable {
         return allow303Caching;
     }
 
+    /**
+     * Returns whether weak etags is allowed with PUT/DELETE methods.
+     * @return {@code true} if it is allowed.
+     */
+    public boolean isWeakETagOnPutDeleteAllowed() {
+        return weakETagOnPutDeleteAllowed;
+    }
+
     /**
      * Returns whether heuristic caching is enabled.
      * @return {@code true} if it is enabled.
@@ -519,6 +543,7 @@ public class CacheConfig implements Cloneable {
         private int maxCacheEntries;
         private int maxUpdateRetries;
         private boolean allow303Caching;
+        private boolean weakETagOnPutDeleteAllowed;
         private boolean heuristicCachingEnabled;
         private float heuristicCoefficient;
         private long heuristicDefaultLifetime;
@@ -533,7 +558,8 @@ public class CacheConfig implements Cloneable {
             this.maxObjectSize = DEFAULT_MAX_OBJECT_SIZE_BYTES;
             this.maxCacheEntries = DEFAULT_MAX_CACHE_ENTRIES;
             this.maxUpdateRetries = DEFAULT_MAX_UPDATE_RETRIES;
-            this.allow303Caching = false;
+            this.allow303Caching = DEFAULT_303_CACHING_ENABLED;
+            this.weakETagOnPutDeleteAllowed = DEFAULT_WEAK_ETAG_ON_PUTDELETE_ALLOWED;
             this.heuristicCachingEnabled = false;
             this.heuristicCoefficient = DEFAULT_HEURISTIC_COEFFICIENT;
             this.heuristicDefaultLifetime = DEFAULT_HEURISTIC_LIFETIME;
@@ -579,6 +605,16 @@ public class CacheConfig implements Cloneable {
             return this;
         }
 
+        /**
+         * Allows or disallows weak etags to be used with PUT/DELETE If-Match requests.
+         * @param weakETagOnPutDeleteAllowed should be {@code true} to
+         *   permit weak etags, {@code false} to reject them.
+         */
+        public Builder setWeakETagOnPutDeleteAllowed(final boolean weakETagOnPutDeleteAllowed) {
+            this.weakETagOnPutDeleteAllowed = weakETagOnPutDeleteAllowed;
+            return this;
+        }
+
         /**
          * Enables or disables heuristic caching.
          * @param heuristicCachingEnabled should be {@code true} to
@@ -690,6 +726,7 @@ public class CacheConfig implements Cloneable {
                     maxCacheEntries,
                     maxUpdateRetries,
                     allow303Caching,
+                    weakETagOnPutDeleteAllowed,
                     heuristicCachingEnabled,
                     heuristicCoefficient,
                     heuristicDefaultLifetime,
@@ -710,6 +747,7 @@ public class CacheConfig implements Cloneable {
                 .append(", maxCacheEntries=").append(this.maxCacheEntries)
                 .append(", maxUpdateRetries=").append(this.maxUpdateRetries)
                 .append(", 303CachingEnabled=").append(this.allow303Caching)
+                .append(", weakETagOnPutDeleteAllowed=").append(this.weakETagOnPutDeleteAllowed)
                 .append(", heuristicCachingEnabled=").append(this.heuristicCachingEnabled)
                 .append(", heuristicCoefficient=").append(this.heuristicCoefficient)
                 .append(", heuristicDefaultLifetime=").append(this.heuristicDefaultLifetime)

httpclient-cache/src/main/java/org/apache/http/impl/client/cache/CachingExec.java

@@ -139,7 +139,7 @@ public class CachingExec implements ClientExecChain {
         this.suitabilityChecker = new CachedResponseSuitabilityChecker(this.validityPolicy, config);
         this.conditionalRequestBuilder = new ConditionalRequestBuilder();
         this.responseCompliance = new ResponseProtocolCompliance();
-        this.requestCompliance = new RequestProtocolCompliance();
+        this.requestCompliance = new RequestProtocolCompliance(config.isWeakETagOnPutDeleteAllowed());
         this.responseCachingPolicy = new ResponseCachingPolicy(
                 this.cacheConfig.getMaxObjectSize(), this.cacheConfig.isSharedCache(),
                 this.cacheConfig.isNeverCacheHTTP10ResponsesWithQuery(), this.cacheConfig.is303CachingEnabled());

httpclient-cache/src/main/java/org/apache/http/impl/client/cache/RequestProtocolCompliance.java

@@ -54,6 +54,17 @@ import org.apache.http.protocol.HTTP;
  */
 @Immutable
 class RequestProtocolCompliance {
+    private final boolean weakETagOnPutDeleteAllowed;
+
+    public RequestProtocolCompliance() {
+        super();
+        this.weakETagOnPutDeleteAllowed = false;
+    }
+
+    public RequestProtocolCompliance(final boolean weakETagOnPutDeleteAllowed) {
+        super();
+        this.weakETagOnPutDeleteAllowed = weakETagOnPutDeleteAllowed;
+    }
 
     private static final List<String> disallowedWithNoCache =
         Arrays.asList(HeaderConstants.CACHE_CONTROL_MIN_FRESH, HeaderConstants.CACHE_CONTROL_MAX_STALE, HeaderConstants.CACHE_CONTROL_MAX_AGE);
@@ -73,9 +84,11 @@ class RequestProtocolCompliance {
             theErrors.add(anError);
         }
 
-        anError = requestHasWeekETagForPUTOrDELETEIfMatch(request);
+        if (!weakETagOnPutDeleteAllowed) {
-        if (anError != null) {
+            anError = requestHasWeekETagForPUTOrDELETEIfMatch(request);
-            theErrors.add(anError);
+            if (anError != null) {
+                theErrors.add(anError);
+            }
         }
 
         anError = requestContainsNoCacheDirectiveWithFieldName(request);

httpclient-cache/src/test/java/org/apache/http/impl/client/cache/TestRequestProtocolCompliance.java

@@ -30,10 +30,13 @@ import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
+import java.util.Arrays;
+
 import org.apache.http.HttpEntityEnclosingRequest;
 import org.apache.http.HttpRequest;
 import org.apache.http.HttpVersion;
 import org.apache.http.ProtocolVersion;
+import org.apache.http.client.methods.HttpPut;
 import org.apache.http.client.methods.HttpRequestWrapper;
 import org.apache.http.message.BasicHttpEntityEnclosingRequest;
 import org.apache.http.message.BasicHttpRequest;
@@ -48,7 +51,35 @@ public class TestRequestProtocolCompliance {
     @Before
     public void setUp() {
         req = HttpTestUtils.makeDefaultRequest();
-        impl = new RequestProtocolCompliance();
+        impl = new RequestProtocolCompliance(false);
+    }
+
+    @Test
+    public void testRequestWithWeakETagAndRange() throws Exception {
+        req.setHeader("Range", "bytes=0-499");
+        req.setHeader("If-Range", "W/\"weak\"");
+        assertEquals(1, impl.requestIsFatallyNonCompliant(req).size());
+    }
+
+    @Test
+    public void testRequestWithWeekETagForPUTOrDELETEIfMatch() throws Exception {
+        req = new HttpPut("http://example.com/");
+        req.setHeader("If-Match", "W/\"weak\"");
+        assertEquals(1, impl.requestIsFatallyNonCompliant(req).size());
+    }
+
+    @Test
+    public void testRequestWithWeekETagForPUTOrDELETEIfMatchAllowed() throws Exception {
+        req = new HttpPut("http://example.com/");
+        req.setHeader("If-Match", "W/\"weak\"");
+        impl = new RequestProtocolCompliance(true);
+        assertEquals(Arrays.asList(), impl.requestIsFatallyNonCompliant(req));
+    }
+
+    @Test
+    public void testRequestContainsNoCacheDirectiveWithFieldName() throws Exception {
+        req.setHeader("Cache-Control", "no-cache=false");
+        assertEquals(1, impl.requestIsFatallyNonCompliant(req).size());
     }
 
     @Test