[GitHub] [wicket] martin-g commented on a change in pull request #439: Wicket 6786: Add Fetch Metadata support

[GitBox]

martin-g commented on a change in pull request #439:
URL: https://github.com/apache/wicket/pull/439#discussion_r451530419



##########
File path: 
wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
##########
@@ -372,6 +394,23 @@ public void onRequestHandlerResolved(RequestCycle cycle, 
IRequestHandler handler
                }
        }
 
+       @Override
+       public void onEndRequest(RequestCycle cycle)
+       {
+               // set vary headers to avoid caching responses processed by 
Fetch Metadata
+               // caching these responses may return 403 responses to 
legitimate requests
+               // or defeat the protection
+               if (cycle.getResponse() instanceof WebResponse)
+               {
+                       WebResponse webResponse = 
(WebResponse)cycle.getResponse();
+                       if (webResponse.isHeaderSupported())
+                       {
+                               webResponse.addHeader(VARY_HEADER, 
SEC_FETCH_DEST_HEADER + ", "

Review comment:
       The value could be yet another constant field. There is no need to 
concatenate it for each and every request.

##########
File path: 
wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
##########
@@ -33,6 +33,9 @@
 import org.apache.wicket.request.cycle.IRequestCycleListener;
 import org.apache.wicket.request.cycle.RequestCycle;
 import org.apache.wicket.request.http.WebRequest;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.*;

Review comment:
       nit: this static import should be at the top and should not use `*`

##########
File path: 
wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
##########
@@ -178,6 +181,8 @@ public String toString()
         */
        private Collection<String> acceptedOrigins = new ArrayList<>();
 
+       private final ResourceIsolationPolicy fetchMetadataPolicy = new 
DefaultResourceIsolationPolicy();

Review comment:
       I think this policy should be replaceable/configurable here, i.e. should 
be passed as constructor parameter.
   The default constructor should use `DefaultResourceIsolationPolicy`




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org