You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@wicket.apache.org by GitBox <gi...@apache.org> on 2020/08/05 14:36:11 UTC
[GitHub] [wicket] papegaaij commented on a change in pull request #439: Wicket 6786: Add Fetch Metadata support
papegaaij commented on a change in pull request #439:
URL: https://github.com/apache/wicket/pull/439#discussion_r465770629
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.protocol.http;
+
+import static java.util.Arrays.asList;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.SEC_FETCH_DEST_HEADER;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.SEC_FETCH_MODE_HEADER;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.SEC_FETCH_SITE_HEADER;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.VARY_HEADER;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import javax.servlet.http.HttpServletRequest;
+import org.apache.wicket.core.request.handler.IPageRequestHandler;
+import org.apache.wicket.core.request.handler.RenderPageRequestHandler;
+import org.apache.wicket.request.IRequestHandler;
+import org.apache.wicket.request.IRequestHandlerDelegate;
+import org.apache.wicket.request.component.IRequestablePage;
+import org.apache.wicket.request.cycle.IRequestCycleListener;
+import org.apache.wicket.request.cycle.RequestCycle;
+import org.apache.wicket.request.http.WebResponse;
+import org.apache.wicket.request.http.flow.AbortWithHttpErrorCodeException;
+import org.apache.wicket.util.lang.Classes;
+import org.apache.wicket.util.string.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The Fetch Metadata Request Cycle Listener is Wicket's implementation of Fetch Metadata.
+ * This adds a layer of protection for modern browsers that prevents Cross-Site Request Forgery
+ * attacks.
+ *
+ * This request listener uses the {@link DefaultResourceIsolationPolicy} by default and can be
+ * customized with additional Resource Isolation Policies.
+ *
+ * This listener can be configured to add exempted URL paths that are intended to be used cross-site.
+ *
+ * Learn more about Fetch Metadata and resource isolation
+ * at <a href="https://web.dev/fetch-metadata/">https://web.dev/fetch-metadata/</a>
+ *
+ * @author Santiago Diaz - saldiaz@google.com
+ * @author Ecenaz Jen Ozmen - ecenazo@google.com
+ */
+public class FetchMetadataRequestCycleListener implements IRequestCycleListener {
+
+ private static final Logger log = LoggerFactory
+ .getLogger(FetchMetadataRequestCycleListener.class);
+ public static final int ERROR_CODE = 403;
+ public static final String ERROR_MESSAGE = "Forbidden";
+ public static final String VARY_HEADER_VALUE = SEC_FETCH_DEST_HEADER + ", "
+ + SEC_FETCH_SITE_HEADER + ", " + SEC_FETCH_MODE_HEADER;
+
+ private final Set<String> exemptedPaths = new HashSet<>();
+ private final List<ResourceIsolationPolicy> resourceIsolationPolicies = new ArrayList<>();
+
+ public FetchMetadataRequestCycleListener(ResourceIsolationPolicy... additionalPolicies) {
+ this.resourceIsolationPolicies.addAll(
+ asList(
+ new DefaultResourceIsolationPolicy(),
+ new OriginBasedResourceIsolationPolicy()
+ )
+ );
+
+ this.resourceIsolationPolicies.addAll(asList(additionalPolicies));
+ }
+
+ public void addExemptedPaths(String... exemptions) {
+ Arrays.stream(exemptions)
+ .filter(e -> !Strings.isEmpty(e))
+ .forEach(exemptedPaths::add);
+ }
+
+ @Override
+ public void onBeginRequest(RequestCycle cycle)
+ {
+ HttpServletRequest containerRequest = (HttpServletRequest)cycle.getRequest()
+ .getContainerRequest();
+
+ log.debug("Processing request to: {}", containerRequest.getPathInfo());
+ }
+
+ @Override
+ public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
+ {
+ handler = unwrap(handler);
+ IPageRequestHandler pageRequestHandler = getPageRequestHandler(handler);
+ if (pageRequestHandler == null) {
+ return;
+ }
+
+ IRequestablePage targetedPage = pageRequestHandler.getPage();
+ HttpServletRequest containerRequest = (HttpServletRequest)cycle.getRequest()
+ .getContainerRequest();
+
+ String pathInfo = containerRequest.getPathInfo();
+ if (exemptedPaths.contains(pathInfo)) {
+ if (log.isDebugEnabled()) {
+ log.debug("Allowing request to {} because it matches an exempted path",
+ new Object[]{pathInfo});
+ }
+ return;
+ }
+
+ for (ResourceIsolationPolicy resourceIsolationPolicy : resourceIsolationPolicies) {
+ if (!resourceIsolationPolicy.isRequestAllowed(containerRequest, targetedPage)) {
+ log.debug("Isolation policy {} has rejected a request to {}",
Review comment:
This loop brings us back to the original problem: the old implementation has false positives and now both have to return true. The idea was to use the old as a fallback in case the fetch metadata headers were missing.
The old implementation had 3 possible outcomes: allowed, disallowed and unknown (because no header was found). As far as I can see, the new implementation has the same 3 possible outcomes. The old implementation should only be consulted when the new implementation is unknown.
This also brings us to the next problem: what if both don't know? In the old implementation, you could configure the behavior for that. In fact, you could configure 3 different `CsrfAction`s for both 'unknown' and 'disallowed'. This implementation only supports one: `ABORT`. Both `SUPPRESS` and `ALLOW` are missing.
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.protocol.http;
+
+import static java.util.Arrays.asList;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.SEC_FETCH_DEST_HEADER;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.SEC_FETCH_MODE_HEADER;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.SEC_FETCH_SITE_HEADER;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.VARY_HEADER;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import javax.servlet.http.HttpServletRequest;
+import org.apache.wicket.core.request.handler.IPageRequestHandler;
+import org.apache.wicket.core.request.handler.RenderPageRequestHandler;
+import org.apache.wicket.request.IRequestHandler;
+import org.apache.wicket.request.IRequestHandlerDelegate;
+import org.apache.wicket.request.component.IRequestablePage;
+import org.apache.wicket.request.cycle.IRequestCycleListener;
+import org.apache.wicket.request.cycle.RequestCycle;
+import org.apache.wicket.request.http.WebResponse;
+import org.apache.wicket.request.http.flow.AbortWithHttpErrorCodeException;
+import org.apache.wicket.util.lang.Classes;
+import org.apache.wicket.util.string.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The Fetch Metadata Request Cycle Listener is Wicket's implementation of Fetch Metadata.
+ * This adds a layer of protection for modern browsers that prevents Cross-Site Request Forgery
+ * attacks.
+ *
+ * This request listener uses the {@link DefaultResourceIsolationPolicy} by default and can be
+ * customized with additional Resource Isolation Policies.
+ *
+ * This listener can be configured to add exempted URL paths that are intended to be used cross-site.
Review comment:
This setup only allows specific paths to be used cross-site. The old implementation had an `isChecked` method that would allow whitelisting pages or even specific requests. In our application, we use the latter. Specific components are annotated to be allowed cross-site. This cannot be done with just a path. Why not use the same `isChecked` as the old implementation?
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
##########
@@ -0,0 +1,160 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.wicket.protocol.http;
+
+import static java.util.Arrays.asList;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.SEC_FETCH_DEST_HEADER;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.SEC_FETCH_MODE_HEADER;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.SEC_FETCH_SITE_HEADER;
+import static org.apache.wicket.protocol.http.ResourceIsolationPolicy.VARY_HEADER;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
+import java.util.Set;
+import javax.servlet.http.HttpServletRequest;
+import org.apache.wicket.core.request.handler.IPageRequestHandler;
+import org.apache.wicket.core.request.handler.RenderPageRequestHandler;
+import org.apache.wicket.request.IRequestHandler;
+import org.apache.wicket.request.IRequestHandlerDelegate;
+import org.apache.wicket.request.component.IRequestablePage;
+import org.apache.wicket.request.cycle.IRequestCycleListener;
+import org.apache.wicket.request.cycle.RequestCycle;
+import org.apache.wicket.request.http.WebResponse;
+import org.apache.wicket.request.http.flow.AbortWithHttpErrorCodeException;
+import org.apache.wicket.util.lang.Classes;
+import org.apache.wicket.util.string.Strings;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * The Fetch Metadata Request Cycle Listener is Wicket's implementation of Fetch Metadata.
+ * This adds a layer of protection for modern browsers that prevents Cross-Site Request Forgery
+ * attacks.
+ *
+ * This request listener uses the {@link DefaultResourceIsolationPolicy} by default and can be
+ * customized with additional Resource Isolation Policies.
+ *
+ * This listener can be configured to add exempted URL paths that are intended to be used cross-site.
+ *
+ * Learn more about Fetch Metadata and resource isolation
+ * at <a href="https://web.dev/fetch-metadata/">https://web.dev/fetch-metadata/</a>
+ *
+ * @author Santiago Diaz - saldiaz@google.com
+ * @author Ecenaz Jen Ozmen - ecenazo@google.com
+ */
+public class FetchMetadataRequestCycleListener implements IRequestCycleListener {
+
+ private static final Logger log = LoggerFactory
+ .getLogger(FetchMetadataRequestCycleListener.class);
+ public static final int ERROR_CODE = 403;
+ public static final String ERROR_MESSAGE = "Forbidden";
+ public static final String VARY_HEADER_VALUE = SEC_FETCH_DEST_HEADER + ", "
+ + SEC_FETCH_SITE_HEADER + ", " + SEC_FETCH_MODE_HEADER;
+
+ private final Set<String> exemptedPaths = new HashSet<>();
+ private final List<ResourceIsolationPolicy> resourceIsolationPolicies = new ArrayList<>();
+
+ public FetchMetadataRequestCycleListener(ResourceIsolationPolicy... additionalPolicies) {
+ this.resourceIsolationPolicies.addAll(
+ asList(
+ new DefaultResourceIsolationPolicy(),
+ new OriginBasedResourceIsolationPolicy()
Review comment:
This way the `OriginBasedResourceIsolationPolicy` is internal to this class and there is no way to configure its whitelist. Also, the whitelist has no effect, because `DefaultResourceIsolationPolicy` will block the requests anyway.
----------------------------------------------------------------
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:
users@infra.apache.org