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/07/08 10:43:17 UTC
[GitHub] [wicket] salcho opened a new pull request #439: Wicket 6786
salcho opened a new pull request #439:
URL: https://github.com/apache/wicket/pull/439
Hello Wicket devs,
This PR builds Fetch Metadata support on top of Wicket's existing CSRF protection, namely:
- If a request has `Sec-Fetch-*` headers (i.e. comes from a modern browser), Fetch Metadata will be preferred. Otherwise, we will fall back to using the existing cross-request checks.
- One default Resource Isolation Policy is provided based on [https://web.dev/fetch-metadata/](https://web.dev/fetch-metadata/), which prevents all major cross-site request forgery attacks.
- If the `Origin` or `Referer` headers are present, Fetch Metadata will apply the same exemptions as the existing Origin-based protection, i.e. allowing cross-origin requests from exempted origins.
- The `Vary` header has been added to responses through `onEndRequest` to ensure that any cached responses include Fetch Metadata headers in their key. This is an added layer of security against cache poisoning.
----------------------------------------------------------------
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
[GitHub] [wicket] papegaaij commented on a change in pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
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
[GitHub] [wicket] papegaaij commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
papegaaij commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-667100509
Yes, I agree. The old approach is only a fallback scenario. When a browser sends Fetch Metadata, that is much more reliable and the old strategy should not be used. It is likely that we will encounter some corner cases with this piece of code, but IMHO the current code is easy enough to adapt by our users if that were to happen. I'm happy with this PR. @svenmeier shall I merge?
----------------------------------------------------------------
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
[GitHub] [wicket] svenmeier commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
svenmeier commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-667327014
I'm all for this new feature, and the new implementation works fine as it seems.
But the integration into the old CsrfPreventionRequestCycleListener is just hacky. The whole class is centered around the request origin, and it shows in the new method checkRequestFetchMetadata()
- it checks a origin whitelist, when actually it has nothing to do with origins (note the method name)
- it calls matchingOrigin, when actually it didn't check the origin
- it takes action according to conflictingOriginAction, when actually it doesn't detect any origin conflicts
There are many other inconsistencies that can easily spotted when reading the Javadoc - which haven't been updated btw.
If this is supposed to be a hack to get this new protection into production ASAP, go ahead - probably no users will see this code anyway.
But if we want to get this integrated nicely, I would suggest we create a new listener and deprecated the old one.
----------------------------------------------------------------
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
[GitHub] [wicket] salcho edited a comment on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho edited a comment on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-667535947
Hi @svenmeier , @papegaaij and @eozmen410
I think Sven is right in saying that the legacy CSRF protection makes many decisions based on the Origin that don't belong in Fetch Metadata. Since we all agree that the legacy protection is still useful to add support for legacy browsers, I've tried to find the right balance between the new security mechanism we want to provide and the legacy one in Wicket, without introducing breaking changes. I've pushed a new commit, here's a summary of the changes:
- It adds a new Fetch Metadata Request Cycle Listener that always runs the default resource isolation policy, which protects against all major CSRF attacks and can be extended in the future to add more policies.
- It makes the legacy CSRF Prevention listener implement ResourceIsolationPolicy. This allows us to reuse its logic around whitelisting origins, hosts and providing protection for legacy browsers. This is left as a choice for developers to opt in, if needed.
- It resolves the contradictory decision making trees of both the default and origin-based policies, such that we won't have the contradictions/false positives mentioned in one of my comments above.
- I have gone as far as marking the CSRF Prevention listener as deprecated! I don't know if this is the right decision and would be happy to get some feedback on this. Perhaps by marking it as such we can get developers to migrate to Fetch Metadata as a definitive security protection against CSRF attacks.
One possible improvement would be to extract the Origin-based protection entirely into a brand new OriginBasedResourceIsolationPolicy and run that by default. That could potentially provide the best of both worlds.
I hope this finds the right balance/compromises to keep Wicket users safe while introducing no breaking changes!
WDYT?
----------------------------------------------------------------
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
[GitHub] [wicket] papegaaij commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
papegaaij commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-670467609
This is what I came up with:
https://github.com/papegaaij/wicket/commit/c51372bd51e67276d86c8cf02336aa04e2f7a403
The biggest change is that a resource isolation policy now has 3 possible outcomes: ALLOWED, DISALLOWED and UNKNOWN. They are checked in order, and the first not UNKNOWN will determine the final outcome.
Other changes are the reintroduction of the configuration options from the old implementation and the 2 default policies are only added if you did not specify any.
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-655559390
Thanks for the quick review Martin! I've sorted out the commits now and updated your comments. We'll hope Edmond will enjoy his holiday and wait for his review :)
----------------------------------------------------------------
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
[GitHub] [wicket] svenmeier commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
svenmeier commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-670466262
I think we're overdoing it ;)
The actual implementation by @salcho was fine, for me the API wasn't just correct any more with all the old references to 'origin'.
This is the simplest solution I can think of, e.g. renaming some methods and going through a change of responsibility in #checkRequest() is enough:
https://gist.github.com/svenmeier/e2e29ddad37e453c693004946ab125e7
I'd deprecate the old listener, and give the new one a fancy new name.
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on a change in pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on a change in pull request #439:
URL: https://github.com/apache/wicket/pull/439#discussion_r463964508
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
##########
@@ -110,11 +110,15 @@
* <li>{@link #onSuppressed(HttpServletRequest, String, IRequestablePage)} when an origin was in
* conflict and the request should be suppressed</li>
* </ul>
+ *
+ * @see FetchMetadataRequestCycleListener
+ * @deprecated
*/
-public class CsrfPreventionRequestCycleListener implements IRequestCycleListener
+@Deprecated(since = "XXX")
Review comment:
@svenmeier any thoughts about what version should go here?
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-669064590
Thanks so much for your review @martin-g!
@papegaaij I believe we're ready for merge now that the changes have been approved :)
----------------------------------------------------------------
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
[GitHub] [wicket] papegaaij commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
papegaaij commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-655529019
I'm on vacation right now, I'll be able to have a look at this pr at the end of July.
----------------------------------------------------------------
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
[GitHub] [wicket] asfgit merged pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
asfgit merged pull request #439:
URL: https://github.com/apache/wicket/pull/439
----------------------------------------------------------------
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
[GitHub] [wicket] salcho edited a comment on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho edited a comment on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-667535947
Hi @svenmeier , @papegaaij and @eozmen410
I think Sven is right in saying that the legacy CSRF protection makes many decisions based on the Origin that don't belong in Fetch Metadata. Since we all agree that the legacy protection is still useful to add support for legacy browsers, I've tried to find the right balance between the new security mechanism we want to provide and the legacy one in Wicket, without introducing breaking changes. I've pushed a new commit, here's a summary of the changes:
- It adds a new Fetch Metadata Request Cycle Listener that always runs the default resource isolation policy, which protects against all major CSRF attacks and can be extended in the future to add more policies.
- It makes the legacy CSRF Prevention listener implement ResourceIsolationPolicy. This allows us to reuse its logic around whitelisting origins, hosts and providing protection for legacy browsers. This is left as a choice for developers to opt in, if needed.
- It resolves the contradictory decision making trees of both the default and origin-based policies, such that we won't have the contradictions/false positives mentioned in one of my comments above.
- I have gone as far as marking the CSRF Prevention listener as deprecated! I don't know if this is the right decision and would be happy to get some feedback on this. Perhaps by marking it as such we can get developers to migrate to Fetch Metadata as a definitive security protection against CSRF attacks.
One possible improvement would be to extract the Origin-based protection entirely into a brand new OriginBasedResourceIsolationPolicy and run that by default too. That could potentially provide the best of both worlds.
I hope this finds the right balance/compromises to keep Wicket users safe while introducing no breaking changes!
WDYT?
----------------------------------------------------------------
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
[GitHub] [wicket] papegaaij commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
papegaaij commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-670436058
> I notice that different reviewers have conflicting designs in mind. This conflict comes from the fact that @papegaaij's applications use the current `CsrfPreventionRequestCycleListener` not so much as a mitigation against CSRF attacks, but rather as an authorization or ACL component that exposes applications/pages/handlers to a set of whitelisted origins. Unfortunately, this is not what Fetch Metadata was designed for (and @svenmeier correctly points this out), but rather to simply reject cross-site requests to endpoints that aren't meant to be used cross-site, regardless of the source origin.
@salcho The current situation is a bit unfortunate indeed. My application does not use the whitelisting, but others might. Looing at the usage of these whitelists, I think the reason these whitelists exist is not for ACL, but again to prevent false positives. False positives are common with the old implementation and hard to solve.
> At this point, we can make changes to use Fetch Metadata as both a CSRF protection and an ACL component, but I believe we would be making Wicket a disservice by creating a security module that is: trivially bypassable (send a [CORS-safelisted request](https://fetch.spec.whatwg.org/#simple-header) to avoid setting the Origin header, which makes the check undecidable) and very hard to maintain, since the policies would be order-dependent and inter-dependent.
I don't think this is needed. CSRF protection is meant to do just that, it's not an ACL. However, it must be possible to exclude some requests from this protection. The old implementation allowed this via an `isChecked(IRequestHandler)` method, the new implementation should do the same.
I'll take a look at the PR, and see what I can do to the improve protection, keep the flexibility and prevent the false positives.
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-667535947
Hi @svenmeier , @papegaaij and @eozmen410
I think Sven is right in saying that the legacy CSRF protection makes many decisions based on the Origin that don't belong in Fetch Metadata. Since we all agree that the legacy protection is still useful to add support for legacy browsers, I've tried to find the right balance between the new security mechanism we want to provide and the legacy one in Wicket, without introducing breaking changes. I've pushed a new commit, here's a summary of the changes:
- It adds a new Fetch Metadata Request Cycle Listener that always runs the default resource isolation policy, which protects against all major CSRF attacks and can be extended in the future to add more policies.
- It makes the legacy CSRF Prevention listener implement ResourceIsolationPolicy. This allows us to reuse its logic around whitelisting origins, hosts and providing protection for legacy browsers. This is left as a choice for developers to opt in, if needed.
- I have gone as far as marking the CSRF Prevention listener as deprecated! I don't know if this is the right decision and would be happy to get some feedback on this. Perhaps by marking it as such we can get developers to migrate to Fetch Metadata as a definitive security protection against CSRF attacks.
I hope this finds the right balance/compromises to keep Wicket users safe while introducing no breaking changes!
WDYT?
----------------------------------------------------------------
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
[GitHub] [wicket] martin-g commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-669745946
> I've noticed that the code formatting is way off from Wicket standards. The code will need to be reformatted before it can be merged.
We can do that after merging. This shouldn't be a stopper for contributors!
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-666985197
Hi Sven,
Thanks for your review! While this moves away from the original proposal (see the conversation on WICKET-6786), I agree that having multiple Resource Isolation Policies is a better pattern. We'll push a commit soon!
On the point of the whitelist not being checked against sec-fetch-site: that is by design, because Fetch Metadata header values are enums rather than origins, so the site in sec-fetch-site doesn't actually carry an origin but rather whether the request is same-origin, same-site or navigational. This is a little confusing, because Wicket implements a whitelist of **origins** and it's not always possible to get those origins, so this is a best-effort method.
----------------------------------------------------------------
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
[GitHub] [wicket] papegaaij commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
papegaaij commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-670550861
Btw, I've also reformatted the code to adhere to the Wicket standards. @salcho and @svenmeier if you two agree with my changes, I can merge it all.
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-667014819
Hi @svenmeier!
@eozmen410 has pointed out something that I missed before that brings us back to the reason why this is implemented the way it is and why we rejected the alternative you propose in our design:
- The legacy check relies on the Referer and Origin headers to determine whether a request is cross-origin. These headers are not reliable since they are not always present and so the legacy check produces false positives, that is: it will, for instance, reject same-site requests that are legitimate because they might not carry Referer/Origin! Whenever we can determine the origin of a cross-origin request, this check works fine.
- Fetch Metadata solves this problem by explicitly marking the request as same/cross-origin reliably, even in the absence of Origin/Referer, which makes the new check false postive-free, hence improving the previous check. That is, in scenarios where the legacy check would have rejected a legitimate request (because there was no Origin header), the new check can now approve the request because sec-fetch-site indicates same-origin.
Because of this, running both checks serially would produce contradictions. The new check would approve and the legacy would reject. That is: whenever the legacy check makes a decision correctly, both legacy and new will agree on the final decision, but in cases where the legacy check produces a false positive, both legacy and new will contradict each other!
In the model you're proposing, if at least one check rejects then the request is fully rejected and so we would be back in the world of false positives that we're trying to get out of.
In summary: while having multiple Resource Isolation Policies is a good idea for the future (for implementing specific policies for iframes, for instance), this PR should be seen as improving **only one** of those policies and not as adding an additional one.
I hope that makes sense, but we would be happy to jump on a call or chat if you'd like to :)
What do you think?
----------------------------------------------------------------
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
[GitHub] [wicket] martin-g commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-655509362
It would be nice to clean up the commits before merging!
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-670680584
Your reference commit looks pretty good to me @papegaaij! +1 on merging with them!
Thank you!
----------------------------------------------------------------
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
[GitHub] [wicket] salcho edited a comment on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho edited a comment on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-667014819
Hi @svenmeier!
@eozmen410 has pointed out something that I missed before that brings us back to the reason why this is implemented the way it is and why we rejected the alternative you propose in our design:
- The legacy check relies on the Referer and Origin headers to determine whether a request is cross-origin. These headers are not reliable since they are not always present and so the legacy check produces false positives, that is: it will, for instance, reject same-site requests that are legitimate because they might not carry Referer/Origin! Whenever we can determine the origin of a cross-origin request, this check works fine.
- Fetch Metadata solves this problem by explicitly marking the request as same/cross-origin reliably, even in the absence of Origin/Referer, which makes the new check false postive-free, hence improving the previous check. That is, in scenarios where the legacy check would have rejected a legitimate request (because there was no Origin header), the new check can now approve the request because sec-fetch-site indicates same-origin.
Because of this, running both checks serially would produce contradictions. The new check would approve and the legacy would reject. That is: whenever the legacy check makes a decision correctly, both legacy and new will agree on the final decision, but in cases where the legacy check produces a false positive, both legacy and new will contradict each other!
In the model you're proposing, if at least one check rejects then the request is fully rejected and so we would be back in the world of false positives that we're trying to get out of.
In summary: while having multiple Resource Isolation Policies is a good idea for the future (for implementing specific policies for iframes, for instance), this PR should be seen as improving **only one** of those policies and not as adding an additional one.
I hope that makes sense, but we would be happy to jump on a call or chat if you'd like to :)
What do you think?
(tagging @papegaaij for visibility and correctness, since he's very familiar with the legacy check)
----------------------------------------------------------------
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
[GitHub] [wicket] martin-g commented on a change in pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [wicket] salcho commented on a change in pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on a change in pull request #439:
URL: https://github.com/apache/wicket/pull/439#discussion_r465257845
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
##########
@@ -0,0 +1,157 @@
+/*
+ * 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.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";
+
+ private final Set<String> exemptedPaths = new HashSet<>();
+ private final List<ResourceIsolationPolicy> resourceIsolationPolicies = new ArrayList<>();
+
+ FetchMetadataRequestCycleListener(ResourceIsolationPolicy... additionalPolicies) {
+ this.resourceIsolationPolicies.add(new DefaultResourceIsolationPolicy());
+ this.resourceIsolationPolicies.addAll(asList(additionalPolicies));
+ }
+
+ 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();
+
+ logAtDebug("Processing request to: {}", containerRequest.getPathInfo());
+ }
+
+ @Override
+ public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
+ {
+ handler = unwrap(handler);
+ Optional<IPageRequestHandler> pageRequestHandler = getPageRequestHandler(handler);
+ if (pageRequestHandler.isEmpty()) {
+ return;
+ }
+
+ IRequestablePage targetedPage = pageRequestHandler.get().getPage();
+ HttpServletRequest containerRequest = (HttpServletRequest)cycle.getRequest()
+ .getContainerRequest();
+
+ String pathInfo = containerRequest.getPathInfo();
+ if (exemptedPaths.contains(pathInfo)) {
+ logAtDebug("Allowing request to {} because it matches an exempted path", pathInfo);
+ return;
+ }
+
+ for (ResourceIsolationPolicy resourceIsolationPolicy : resourceIsolationPolicies) {
+ if (!resourceIsolationPolicy.isRequestAllowed(containerRequest, targetedPage)) {
+ logAtDebug(
+ "Isolation policy {} has rejected a request to {}",
+ resourceIsolationPolicy.getClass().getSimpleName(),
+ pathInfo
+ );
+ throw new AbortWithHttpErrorCodeException(ERROR_CODE, ERROR_MESSAGE);
+ }
+ }
+ }
+
+ @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 + ", "
+ + SEC_FETCH_SITE_HEADER + ", " + SEC_FETCH_MODE_HEADER);
Review comment:
sorry, this was a regression from our previous commit
----------------------------------------------------------------
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
[GitHub] [wicket] papegaaij commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
papegaaij commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-669769314
> > I've noticed that the code formatting is way off from Wicket standards. The code will need to be reformatted before it can be merged.
>
> We can do that after merging. This shouldn't be a stopper for contributors!
Agreed
----------------------------------------------------------------
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
[GitHub] [wicket] martin-g commented on a change in pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #439:
URL: https://github.com/apache/wicket/pull/439#discussion_r465064988
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
##########
@@ -0,0 +1,157 @@
+/*
+ * 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.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";
+
+ private final Set<String> exemptedPaths = new HashSet<>();
+ private final List<ResourceIsolationPolicy> resourceIsolationPolicies = new ArrayList<>();
+
+ FetchMetadataRequestCycleListener(ResourceIsolationPolicy... additionalPolicies) {
+ this.resourceIsolationPolicies.add(new DefaultResourceIsolationPolicy());
+ this.resourceIsolationPolicies.addAll(asList(additionalPolicies));
+ }
+
+ 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();
+
+ logAtDebug("Processing request to: {}", containerRequest.getPathInfo());
+ }
+
+ @Override
+ public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
+ {
+ handler = unwrap(handler);
+ Optional<IPageRequestHandler> pageRequestHandler = getPageRequestHandler(handler);
+ if (pageRequestHandler.isEmpty()) {
+ return;
+ }
+
+ IRequestablePage targetedPage = pageRequestHandler.get().getPage();
+ HttpServletRequest containerRequest = (HttpServletRequest)cycle.getRequest()
+ .getContainerRequest();
+
+ String pathInfo = containerRequest.getPathInfo();
+ if (exemptedPaths.contains(pathInfo)) {
+ logAtDebug("Allowing request to {} because it matches an exempted path", pathInfo);
+ return;
+ }
+
+ for (ResourceIsolationPolicy resourceIsolationPolicy : resourceIsolationPolicies) {
+ if (!resourceIsolationPolicy.isRequestAllowed(containerRequest, targetedPage)) {
+ logAtDebug(
+ "Isolation policy {} has rejected a request to {}",
+ resourceIsolationPolicy.getClass().getSimpleName(),
Review comment:
You can use `Classes.simpleName()` here. It handles better anonymous/inner classes
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
##########
@@ -0,0 +1,157 @@
+/*
+ * 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.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";
+
+ private final Set<String> exemptedPaths = new HashSet<>();
+ private final List<ResourceIsolationPolicy> resourceIsolationPolicies = new ArrayList<>();
+
+ FetchMetadataRequestCycleListener(ResourceIsolationPolicy... additionalPolicies) {
+ this.resourceIsolationPolicies.add(new DefaultResourceIsolationPolicy());
+ this.resourceIsolationPolicies.addAll(asList(additionalPolicies));
+ }
+
+ 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();
+
+ logAtDebug("Processing request to: {}", containerRequest.getPathInfo());
+ }
+
+ @Override
+ public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
+ {
+ handler = unwrap(handler);
+ Optional<IPageRequestHandler> pageRequestHandler = getPageRequestHandler(handler);
+ if (pageRequestHandler.isEmpty()) {
+ return;
+ }
+
+ IRequestablePage targetedPage = pageRequestHandler.get().getPage();
+ HttpServletRequest containerRequest = (HttpServletRequest)cycle.getRequest()
+ .getContainerRequest();
+
+ String pathInfo = containerRequest.getPathInfo();
+ if (exemptedPaths.contains(pathInfo)) {
+ logAtDebug("Allowing request to {} because it matches an exempted path", pathInfo);
+ return;
+ }
+
+ for (ResourceIsolationPolicy resourceIsolationPolicy : resourceIsolationPolicies) {
+ if (!resourceIsolationPolicy.isRequestAllowed(containerRequest, targetedPage)) {
+ logAtDebug(
+ "Isolation policy {} has rejected a request to {}",
+ resourceIsolationPolicy.getClass().getSimpleName(),
+ pathInfo
+ );
+ throw new AbortWithHttpErrorCodeException(ERROR_CODE, ERROR_MESSAGE);
+ }
+ }
+ }
+
+ @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 + ", "
+ + SEC_FETCH_SITE_HEADER + ", " + SEC_FETCH_MODE_HEADER);
+ }
+ }
+ }
+
+ private static IRequestHandler unwrap(IRequestHandler handler) {
+ while (handler instanceof IRequestHandlerDelegate)
Review comment:
nit: our code style is to use `{` and `}` even for one-liners
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
##########
@@ -110,11 +110,15 @@
* <li>{@link #onSuppressed(HttpServletRequest, String, IRequestablePage)} when an origin was in
* conflict and the request should be suppressed</li>
* </ul>
+ *
+ * @see FetchMetadataRequestCycleListener
+ * @deprecated
*/
-public class CsrfPreventionRequestCycleListener implements IRequestCycleListener
+@Deprecated(since = "XXX")
Review comment:
"9.1.0"
But IMO it needs some work before being deprecated. The logic should be extracted in a new class implementing ResourceIsolationPolicy that this listener should extend from. This way we could remove this class for Wicket 10 (as we do with all deprecated classes in a major version release).
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
##########
@@ -0,0 +1,157 @@
+/*
+ * 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.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";
+
+ private final Set<String> exemptedPaths = new HashSet<>();
+ private final List<ResourceIsolationPolicy> resourceIsolationPolicies = new ArrayList<>();
+
+ FetchMetadataRequestCycleListener(ResourceIsolationPolicy... additionalPolicies) {
+ this.resourceIsolationPolicies.add(new DefaultResourceIsolationPolicy());
+ this.resourceIsolationPolicies.addAll(asList(additionalPolicies));
+ }
+
+ 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();
+
+ logAtDebug("Processing request to: {}", containerRequest.getPathInfo());
+ }
+
+ @Override
+ public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
+ {
+ handler = unwrap(handler);
+ Optional<IPageRequestHandler> pageRequestHandler = getPageRequestHandler(handler);
+ if (pageRequestHandler.isEmpty()) {
+ return;
+ }
+
+ IRequestablePage targetedPage = pageRequestHandler.get().getPage();
+ HttpServletRequest containerRequest = (HttpServletRequest)cycle.getRequest()
+ .getContainerRequest();
+
+ String pathInfo = containerRequest.getPathInfo();
+ if (exemptedPaths.contains(pathInfo)) {
+ logAtDebug("Allowing request to {} because it matches an exempted path", pathInfo);
+ return;
+ }
+
+ for (ResourceIsolationPolicy resourceIsolationPolicy : resourceIsolationPolicies) {
+ if (!resourceIsolationPolicy.isRequestAllowed(containerRequest, targetedPage)) {
+ logAtDebug(
+ "Isolation policy {} has rejected a request to {}",
+ resourceIsolationPolicy.getClass().getSimpleName(),
+ pathInfo
+ );
+ throw new AbortWithHttpErrorCodeException(ERROR_CODE, ERROR_MESSAGE);
+ }
+ }
+ }
+
+ @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 + ", "
+ + SEC_FETCH_SITE_HEADER + ", " + SEC_FETCH_MODE_HEADER);
+ }
+ }
+ }
+
+ private static IRequestHandler unwrap(IRequestHandler handler) {
+ while (handler instanceof IRequestHandlerDelegate)
+ handler = ((IRequestHandlerDelegate)handler).getDelegateHandler();
+ return handler;
+ }
+
+ private Optional<IPageRequestHandler> getPageRequestHandler(IRequestHandler handler)
Review comment:
IMO there is no need to use `Optional` here since the only caller does not use it in functional style, but calls `.isEmpty()` + `.get()`. Using `null` is just fine.
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
##########
@@ -0,0 +1,157 @@
+/*
+ * 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.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";
+
+ private final Set<String> exemptedPaths = new HashSet<>();
+ private final List<ResourceIsolationPolicy> resourceIsolationPolicies = new ArrayList<>();
+
+ FetchMetadataRequestCycleListener(ResourceIsolationPolicy... additionalPolicies) {
+ this.resourceIsolationPolicies.add(new DefaultResourceIsolationPolicy());
+ this.resourceIsolationPolicies.addAll(asList(additionalPolicies));
+ }
+
+ 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();
+
+ logAtDebug("Processing request to: {}", containerRequest.getPathInfo());
+ }
+
+ @Override
+ public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
+ {
+ handler = unwrap(handler);
+ Optional<IPageRequestHandler> pageRequestHandler = getPageRequestHandler(handler);
+ if (pageRequestHandler.isEmpty()) {
+ return;
+ }
+
+ IRequestablePage targetedPage = pageRequestHandler.get().getPage();
+ HttpServletRequest containerRequest = (HttpServletRequest)cycle.getRequest()
+ .getContainerRequest();
+
+ String pathInfo = containerRequest.getPathInfo();
+ if (exemptedPaths.contains(pathInfo)) {
+ logAtDebug("Allowing request to {} because it matches an exempted path", pathInfo);
+ return;
+ }
+
+ for (ResourceIsolationPolicy resourceIsolationPolicy : resourceIsolationPolicies) {
+ if (!resourceIsolationPolicy.isRequestAllowed(containerRequest, targetedPage)) {
+ logAtDebug(
+ "Isolation policy {} has rejected a request to {}",
+ resourceIsolationPolicy.getClass().getSimpleName(),
+ pathInfo
+ );
+ throw new AbortWithHttpErrorCodeException(ERROR_CODE, ERROR_MESSAGE);
+ }
+ }
+ }
+
+ @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 + ", "
+ + SEC_FETCH_SITE_HEADER + ", " + SEC_FETCH_MODE_HEADER);
+ }
+ }
+ }
+
+ private static IRequestHandler unwrap(IRequestHandler handler) {
+ while (handler instanceof IRequestHandlerDelegate)
+ handler = ((IRequestHandlerDelegate)handler).getDelegateHandler();
+ return handler;
+ }
+
+ private Optional<IPageRequestHandler> getPageRequestHandler(IRequestHandler handler)
+ {
+ boolean isPageRequestHandler = handler instanceof IPageRequestHandler &&
+ !(handler instanceof RenderPageRequestHandler);
+ return isPageRequestHandler ? Optional.of((IPageRequestHandler) handler) : Optional.empty();
+ }
+
+ private static void logAtDebug(String message, Object... params) {
Review comment:
IMO this helper method is not needed. Slf4j does not add overhead if `{}` placeholders are used (and they are used!).
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
##########
@@ -0,0 +1,157 @@
+/*
+ * 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.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";
+
+ private final Set<String> exemptedPaths = new HashSet<>();
+ private final List<ResourceIsolationPolicy> resourceIsolationPolicies = new ArrayList<>();
+
+ FetchMetadataRequestCycleListener(ResourceIsolationPolicy... additionalPolicies) {
+ this.resourceIsolationPolicies.add(new DefaultResourceIsolationPolicy());
+ this.resourceIsolationPolicies.addAll(asList(additionalPolicies));
+ }
+
+ 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();
+
+ logAtDebug("Processing request to: {}", containerRequest.getPathInfo());
+ }
+
+ @Override
+ public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
+ {
+ handler = unwrap(handler);
+ Optional<IPageRequestHandler> pageRequestHandler = getPageRequestHandler(handler);
+ if (pageRequestHandler.isEmpty()) {
+ return;
+ }
+
+ IRequestablePage targetedPage = pageRequestHandler.get().getPage();
+ HttpServletRequest containerRequest = (HttpServletRequest)cycle.getRequest()
+ .getContainerRequest();
+
+ String pathInfo = containerRequest.getPathInfo();
+ if (exemptedPaths.contains(pathInfo)) {
+ logAtDebug("Allowing request to {} because it matches an exempted path", pathInfo);
+ return;
+ }
+
+ for (ResourceIsolationPolicy resourceIsolationPolicy : resourceIsolationPolicies) {
+ if (!resourceIsolationPolicy.isRequestAllowed(containerRequest, targetedPage)) {
+ logAtDebug(
+ "Isolation policy {} has rejected a request to {}",
+ resourceIsolationPolicy.getClass().getSimpleName(),
+ pathInfo
+ );
+ throw new AbortWithHttpErrorCodeException(ERROR_CODE, ERROR_MESSAGE);
+ }
+ }
+ }
+
+ @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 + ", "
+ + SEC_FETCH_SITE_HEADER + ", " + SEC_FETCH_MODE_HEADER);
Review comment:
Please define a constant for the value. No need to concatenate again and again for every request.
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/FetchMetadataRequestCycleListener.java
##########
@@ -0,0 +1,157 @@
+/*
+ * 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.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";
+
+ private final Set<String> exemptedPaths = new HashSet<>();
+ private final List<ResourceIsolationPolicy> resourceIsolationPolicies = new ArrayList<>();
+
+ FetchMetadataRequestCycleListener(ResourceIsolationPolicy... additionalPolicies) {
Review comment:
Why this constructor and the method below are package private ?
How am I supposed to make use of them in my app ?
----------------------------------------------------------------
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
[GitHub] [wicket] papegaaij commented on a change in pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
papegaaij commented on a change in pull request #439:
URL: https://github.com/apache/wicket/pull/439#discussion_r462179331
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
##########
@@ -400,7 +454,8 @@ protected String getSourceUri(HttpServletRequest containerRequest)
* @param page
* the page that is the target of the request
*/
- protected void checkRequest(HttpServletRequest request, String sourceUri, IRequestablePage page)
+ protected void checkRequestOriginReferer(HttpServletRequest request, String sourceUri,
+ IRequestablePage page)
Review comment:
The change in method name is an API break. This would delay this feature to Wicket 10.
To preserve the API, you can create a new method with the old signature to containing the if-block that's now in onRequestHandlerResolved. You will miss the cycle parameter, but that's not a big problem because you can get it via `RequestCycle.get()`
----------------------------------------------------------------
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
[GitHub] [wicket] svenmeier commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
svenmeier commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-666991264
Thanks for clarifying the whitelist handling. I was confused because checkRequestFetchMetadata() checks the origin too.
----------------------------------------------------------------
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
[GitHub] [wicket] papegaaij commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
papegaaij commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-670707046
I've merged the PR. Thanks for the excellent work @salcho and my apologies for the bumpy process. I think the whole discussion has led to a better implementation!
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-666262555
That's great! Is there anything else we can do to have this PR merged? :)
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-670427420
Hi @papegaaij , @svenmeier, @martin-g & @eozmen410
I notice that different reviewers have conflicting designs in mind. This conflict comes from the fact that @papegaaij's applications use the current `CsrfPreventionRequestCycleListener` not so much as a mitigation against CSRF attacks, but rather as an authorization or ACL component that exposes applications/pages/handlers to a set of whitelisted origins. Unfortunately, this is not what Fetch Metadata was designed for (and @svenmeier correctly points this out), but rather to simply reject cross-site requests to endpoints that aren't meant to be used cross-site, regardless of the source origin.
At this point, we can make changes to use Fetch Metadata as both a CSRF protection and an ACL component, but I believe we would be making Wicket a disservice by creating a security module that is: trivially bypassable (send a [CORS-safelisted request](https://fetch.spec.whatwg.org/#simple-header) to avoid setting the Origin header, which makes the check undecidable) and very hard to maintain, since the policies would be order-dependent and inter-dependent.
We are very happy to contribute to Wicket, but unfortunately we only have a limited number of resources to do so. Having said that, I have a proposal that I think can reconciliate both of these approaches:
- Revert all changes made to `CsrfPreventionRequestCycleListener`
- Contribute to Wicket with a pure `FetchMetadataRequestCycleListener` that is completely independent from the `Origin` header, providing effective protection against CSRF attacks.
- Allow other listeners to compose this new protection (either by composing the new listener or by picking indvidual resource isolation policies). This way Edmond can have full freedom to integrate Fetch Metadata into his module with all the nuances of his use case.
Our main goal is to provide easy to maintain security mitigations that are effective at keeping users safe in the web platform ([here's an example of this in another Apache project](https://github.com/apache/struts/pull/426)) and I believe this proposal achieves just that.
Would Wicket developers be interested in seeing something like this? We would be happy to push another set of changes if so.
Thank you all for your patience and I'm looking forward to reading your thoughts! :)
----------------------------------------------------------------
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
[GitHub] [wicket] svenmeier commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
svenmeier commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-666426580
> It would be great if another Wicket dev could have a look as well. Perhaps @martin-g or @svenmeier ?
I'll take a look.
----------------------------------------------------------------
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
[GitHub] [wicket] papegaaij commented on pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
papegaaij commented on pull request #439:
URL: https://github.com/apache/wicket/pull/439#issuecomment-666339622
It would be great if another Wicket dev could have a look as well. Perhaps @martin-g or @svenmeier ?
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on a change in pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on a change in pull request #439:
URL: https://github.com/apache/wicket/pull/439#discussion_r465266904
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
##########
@@ -110,11 +110,15 @@
* <li>{@link #onSuppressed(HttpServletRequest, String, IRequestablePage)} when an origin was in
* conflict and the request should be suppressed</li>
* </ul>
+ *
+ * @see FetchMetadataRequestCycleListener
+ * @deprecated
*/
-public class CsrfPreventionRequestCycleListener implements IRequestCycleListener
+@Deprecated(since = "XXX")
Review comment:
This solution looks much better now. I've created a new OriginBasedResourceIsolationPolicy, so Wicket will be able to defend against CSRF attacks on modern and legacy browsers. I've set 9.1.0 as you said but would be happy to iterate again if you think the Deprecated annotation shouldn't be here yet.
Thanks for your review!
----------------------------------------------------------------
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
[GitHub] [wicket] salcho commented on a change in pull request #439: Wicket 6786: Add Fetch Metadata support
Posted by GitBox <gi...@apache.org>.
salcho commented on a change in pull request #439:
URL: https://github.com/apache/wicket/pull/439#discussion_r462355253
##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/CsrfPreventionRequestCycleListener.java
##########
@@ -400,7 +454,8 @@ protected String getSourceUri(HttpServletRequest containerRequest)
* @param page
* the page that is the target of the request
*/
- protected void checkRequest(HttpServletRequest request, String sourceUri, IRequestablePage page)
+ protected void checkRequestOriginReferer(HttpServletRequest request, String sourceUri,
+ IRequestablePage page)
Review comment:
That's a good point, thanks Edmond! Jen has pushed a change following your suggestion so existing subclasses will still behave as they used to.
----------------------------------------------------------------
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