[GitHub] [wicket] eozmen410 opened a new pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

classic Classic list List threaded Threaded
27 messages Options
12
Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] eozmen410 opened a new pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox

eozmen410 opened a new pull request #442:
URL: https://github.com/apache/wicket/pull/442


   Hello Wicket devs!
   
   This PR adds Cross-Origin Opener Policy and Cross-Origin Embedder Policy support for Wicket.
   
   COOP is a security mitigation that lets developers isolate their resources against side-channel attacks and information leaks. COEP  prevents a document from loading any non-same-origin resources which don't explicitly grant the document permission to be loaded. COOP and COEP are independent mechanisms and they can be enabled, tested and deployed separately. Using COEP and COOP together allows developers to safely use powerful features such as `SharedArrayBuffer`, `performance.measureMemory()`, and the JS Self-Profiling API. Both COOP and COEP require adding headers to the response. COOP and COEP are now supported by all major browsers. See [https://web.dev/why-coop-coep/](https://web.dev/why-coop-coep/) for reference.
   
   Here's a summary of all the changes:
   * We have added 2 new request cycle listeners, the `CoopRequestCycleListener` and `CoepRequestCycleListener` to handle adding the headers for the respective security mitigations.
   
   * The listeners can be configured using the `CoopConfiguration` and `CoepConfiguration` classes that use the builder pattern for ease of use by Wicket users.
   
   * Using `CoopConfiguration` Wicket users will be able to specify the policy they want for COOP (`same-origin`, `same-origin-allow-popups` or `unsafe-none`) and add exempted paths to specify the endpoints for which COOP will not be enabled.
   
   * Similarly using `CoepConfiguration` Wicket users will be able to add exempted paths for which COEP will be disabled and specify if they want COEP to be enforcing (header set as `Cross-Origin-Embedder-Policy`) or reporting (header set as `Cross-Origin-Embedder-Policy-Report-Only`)
   
   * We have added 2 new methods to the `WebApplication` class to make it convenient for users to `enableCoop` and `enableCoep`.
   
   Here are sample uses of enabling these mechnisms, in the `init()` method of the `WebApplication`:
   
   ```
   //enabling Cross-Origin Opener Policy
   enableCoop(new CoopConfiguration.Builder()
    .withMode(CoopMode.SAME_ORIGIN).withExemptions("<exemptions>").build());
   //enabling Cross-Origin Embedder Policy
   enableCoep(new CoepConfiguration.Builder()
    .withMode(CoepMode.ENFORCING).withExemptions("<exemptions>").build());
   ```


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] svenmeier commented on a change in pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox

svenmeier commented on a change in pull request #442:
URL: https://github.com/apache/wicket/pull/442#discussion_r468756843



##########
File path: wicket-core/src/main/java/org/apache/wicket/coop/CoopConfiguration.java
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.coop;
+
+import org.apache.wicket.request.http.WebResponse;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Specifies the configuration for Cross-Origin Opener Policy to be used for
+ * {@link CoopRequestCycleListener}. Users can specify the paths that should be exempt from COOP and
+ * one of 3 modes (<code>UNSAFE_NONE, SAME_ORIGIN, SAME_ORIGIN_ALLOW_POPUPS</code>) for the policy.
+ *
+ * You can enable COOP headers by adding it to the request cycle listeners in your
+ * {@link org.apache.wicket.protocol.http.WebApplication#init() application's init method}:
+ *
+ * <pre>
+ * &#064;Override
+ * protected void init()
+ * {
+ * // ...
+ * enableCoop(new CoopConfiguration.Builder().withMode(CoopMode.SAME_ORIGIN)
+ * .withExemptions("EXEMPTED PATHS").build());
+ * // ...
+ * }
+ * </pre>
+ *
+ * @author Santiago Diaz - [hidden email]
+ * @author Ecenaz Jen Ozmen - [hidden email]
+ *
+ * @see CoopRequestCycleListener
+ */
+public class CoopConfiguration
+{
+ public enum CoopMode
+ {
+ UNSAFE_NONE("unsafe-none"),
+ SAME_ORIGIN("same-origin"),
+ SAME_ORIGIN_ALLOW_POPUPS("same-origin-allow-popups");
+
+ final String keyword;
+
+ CoopMode(String keyword)
+ {
+ this.keyword = keyword;
+ }
+ }
+
+ static String COOP_HEADER = "Cross-Origin-Opener-Policy";
+
+ private final Set<String> exemptions;
+ private final CoopMode mode;
+
+
+ private CoopConfiguration(Set<String> exemptions, CoopMode mode)
+ {
+ this.exemptions = exemptions;
+ this.mode = mode;
+ }
+
+ public static class Builder

Review comment:
       I know the Builder pattern is considered best-practice nowadays :)
   But since we don't use it in Wicket, we could live without it here for this simple type of configuration, to be more inline with the rest of the code.




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] svenmeier commented on a change in pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

svenmeier commented on a change in pull request #442:
URL: https://github.com/apache/wicket/pull/442#discussion_r468758100



##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/WebApplication.java
##########
@@ -1111,4 +1115,14 @@ public ContentSecurityPolicySettings getCspSettings()
  }
  return cspSettings;
  }
+
+ public void enableCoop(CoopConfiguration coopConfig)

Review comment:
       I don't think we should add these helpers, WebApplication has enough methods already and adding a listener quite easy.




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] svenmeier commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

svenmeier commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-672136241


   Are you sure two separate listeners are the best solution? COEP is a single header only and to quote [https://web.dev/coop-coep/]: "Use a combination of COOP and COEP HTTP headers to opt a web page into a special cross-origin isolated state."
   I wonder whether a single listener would be better from the usage perspective.


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] eozmen410 commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

eozmen410 commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-672731231


   Hi @svenmeier!
   
   Thanks for your review. :)  
   
   > Are you sure two separate listeners are the best solution? COEP is a single header only and to quote [https://web.dev/coop-coep/]: "Use a combination of COOP and COEP HTTP headers to opt a web page into a special cross-origin isolated state."
   I wonder whether a single listener would be better from the usage perspective.
   
   Here's our reasoning behind having 2 separate listeners:
   * We chose to have two separate listeners for COOP and COEP because they are independent mechanisms that can be deployed separately. Setting COEP headers ensures that a document can only load resources from the same origin, or resources explicitly marked as loadable from another origin. This means that, when COEP is enabled, all the resources loaded from cross-origin need to support CORS and will be blocked if they don't. COOP on the other hand does not require CORS support. It's sufficient to set the COOP response header to process-isolate your document so potential attackers can't access your global object if they were opening a pop-up (preventing XS-leaks). If the application doesn't depend on cross-origin interactions between top-level windows, there should be no observable effect of enabling COOP, while enabling COEP might require some refactoring on the Wicket user's part if they are using cross-origin resources.
   
   You're right in pointing out that enabling COOP and COEP together is the best practice for cross-origin isolation. We thought we would give Wicket users more flexibility if they wanted to enable them separately for any reason.
   
   Having a single listener for both COOP and COEP would also achieve the same results, if we provide flexible ways to configure the listener. In that case, while we can provide a single configuration, it might be somewhat confusing to have `CoepMode` and `CoopMode` especially since the acronyms COEP and COOP are already fairly confusing! Having separate configs also has less risk of introducing non-backward compatible configs on that object.
   
   If you think having a single listener is better from a usability perspective, we can change our implementation to merge COOP and COEP into a single listener and provide two separate config objects and handy constructors for the listener that can receive either `CooepConfiguration` or `CoopConfiguration` or both. 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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

martin-g commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-672736379


   > especially since the acronyms COEP and COOP are already fairly confusing!
   
   I'm glad it is not just me finding those acronyms too cryptic.
   I, personally, don't mind to use long name like `CrossOriginOpenerPolicyConfiguration`. The IDEs help to type it quickly and it is much clearer to (re-)read the code.


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on a change in pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

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



##########
File path: wicket-core/src/main/java/org/apache/wicket/coop/CoopConfiguration.java
##########
@@ -0,0 +1,110 @@
+/*
+ * 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.coop;
+
+import org.apache.wicket.request.http.WebResponse;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+/**
+ * Specifies the configuration for Cross-Origin Opener Policy to be used for
+ * {@link CoopRequestCycleListener}. Users can specify the paths that should be exempt from COOP and
+ * one of 3 modes (<code>UNSAFE_NONE, SAME_ORIGIN, SAME_ORIGIN_ALLOW_POPUPS</code>) for the policy.
+ *
+ * You can enable COOP headers by adding it to the request cycle listeners in your
+ * {@link org.apache.wicket.protocol.http.WebApplication#init() application's init method}:
+ *
+ * <pre>
+ * &#064;Override
+ * protected void init()
+ * {
+ * // ...
+ * enableCoop(new CoopConfiguration.Builder().withMode(CoopMode.SAME_ORIGIN)
+ * .withExemptions("EXEMPTED PATHS").build());
+ * // ...
+ * }
+ * </pre>
+ *
+ * @author Santiago Diaz - [hidden email]
+ * @author Ecenaz Jen Ozmen - [hidden email]
+ *
+ * @see CoopRequestCycleListener
+ */
+public class CoopConfiguration
+{
+ public enum CoopMode
+ {
+ UNSAFE_NONE("unsafe-none"),
+ SAME_ORIGIN("same-origin"),
+ SAME_ORIGIN_ALLOW_POPUPS("same-origin-allow-popups");
+
+ final String keyword;
+
+ CoopMode(String keyword)
+ {
+ this.keyword = keyword;
+ }
+ }
+
+ static String COOP_HEADER = "Cross-Origin-Opener-Policy";
+
+ private final Set<String> exemptions;
+ private final CoopMode mode;
+
+
+ private CoopConfiguration(Set<String> exemptions, CoopMode mode)
+ {
+ this.exemptions = exemptions;
+ this.mode = mode;
+ }
+
+ public static class Builder
+ {
+ // provide default values
+ private Set<String> exemptions = new HashSet<>();
+ private CoopMode mode = CoopMode.SAME_ORIGIN;
+
+ public Builder withExemptions(String... exemptions)
+ {
+ this.exemptions.addAll(Arrays.asList(exemptions));
+ return this;
+ }
+
+ public Builder withMode(CoopMode mode)
+ {
+ this.mode = mode;
+ return this;
+ }
+
+ public CoopConfiguration build()
+ {
+ return new CoopConfiguration(exemptions, mode);
+ }
+ }
+
+ public boolean isExempted(String path)
+ {
+ return exemptions.contains(path);
+ }
+
+ public void addCoopHeader(WebResponse resp)

Review comment:
       Any reason this method is here ?
   The config classes (often called XyzSettings in Wicket) usually are POJOs with getters and setters. IMO such `action` method should be in the listener class.




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on a change in pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

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



##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/WebApplication.java
##########
@@ -1111,4 +1115,14 @@ public ContentSecurityPolicySettings getCspSettings()
  }
  return cspSettings;
  }
+
+ public void enableCoop(CoopConfiguration coopConfig)

Review comment:
       How about this idea:
   - add the CoopConfiguration and CoepConfiguration as fields in SecuritySettings.
   - add third mode `DISABLED` to the existing ones (`ENFORCING` and `REPORTING`)
   - in WebApplication#init() add some logic to auto-add the Coop/Coep listener(s) when they are enabled
   
   This way the developer will have to configure the security settings and don't bother how they are applied.




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on a change in pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

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



##########
File path: wicket-core/src/main/java/org/apache/wicket/protocol/http/WebApplication.java
##########
@@ -1111,4 +1115,14 @@ public ContentSecurityPolicySettings getCspSettings()
  }
  return cspSettings;
  }
+
+ public void enableCoop(CoopConfiguration coopConfig)

Review comment:
       How about this idea:
   - add the CoopConfiguration and CoepConfiguration as fields in SecuritySettings.
   - add third mode `DISABLED` to the existing ones (`ENFORCING` and `REPORTING`)
   - in Application#initApplication() add some logic to auto-add the Coop/Coep listener(s) when they are enabled
   
   This way the developer will have to configure the security settings and don't bother how they are applied.




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] eozmen410 commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

eozmen410 commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-673966055


   Hi @martin-g , @svenmeier !
   
   > How about this idea:
   add the CoopConfiguration and CoepConfiguration as fields in SecuritySettings.
   add third mode DISABLED to the existing ones (ENFORCING and REPORTING)
   in Application#initApplication() add some logic to auto-add the Coop/Coep listener(s) when they are enabled
   This way the developer will have to configure the security settings and don't bother how they are applied.
   
   We made the changes suggested by @martin-g in the comment above. Now the config objects live in `SecuritySettings` and users can use the setter methods to configure COOP and COEP. I've also renamed the configs to have longer names to avoid any confusion between the acronyms! If the configs are not `DISABLED` the listeners are added automatically in `Application#initApplication()`. Users can use the following lines in the `init()` method to enable COOP or COEP.
   
   ```
   getSecuritySettings().setCrossOriginOpenerPolicyConfiguration(CoopMode.SAME_ORIGIN, "exemptions");
   getSecuritySettings().setCrossOriginEmbedderPolicyConfiguration(CoepMode.ENFORCING, "exemptions");
   ```
   
   


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on a change in pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

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



##########
File path: wicket-core/src/main/java/org/apache/wicket/Application.java
##########
@@ -33,6 +33,10 @@
 import org.apache.wicket.application.IComponentInitializationListener;
 import org.apache.wicket.application.IComponentInstantiationListener;
 import org.apache.wicket.application.OnComponentTagListenerCollection;
+import org.apache.wicket.coep.CoepRequestCycleListener;

Review comment:
       nit: I'd rename of Coep/Coop to their full name version - CrossOriginEmbedderPolicyRequestCycleListener.

##########
File path: wicket-core/src/main/java/org/apache/wicket/coep/CoepConfiguration.java
##########
@@ -0,0 +1,108 @@
+/*

Review comment:
       It'd have been better to put `coep` package under `org.apache.wicket.security` parent package but I see that the `csp` package is already in the root (`o.a.w`) so it would be inconsistent to do this now for `coep` and `coop` ... :-/

##########
File path: wicket-core/src/main/java/org/apache/wicket/coop/CoopRequestCycleListener.java
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.coop;
+
+import org.apache.wicket.request.IRequestHandler;
+import org.apache.wicket.request.cycle.IRequestCycleListener;
+import org.apache.wicket.request.cycle.RequestCycle;
+import org.apache.wicket.request.http.WebResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.http.HttpServletRequest;
+
+/**
+ * Sets <a href="https://github.com/whatwg/html/pull/5334/files">Cross-Origin Opener Policy</a>
+ * headers on the responses based on the policy specified by {@link CrossOriginOpenerPolicyConfiguration}. The header
+ * is not set for the paths that are exempted from COOP.
+ *
+ * COOP is a mitigation against cross-origin information leaks and is used to make websites
+ * cross-origin isolated. Setting the COOP header allows you to ensure that a top-level window is
+ * isolated from other documents by putting them in a different browsing context group, so they
+ * cannot directly interact with the top-level window. Using COEP and COOP together allows
+ * developers to safely use * powerful features such as <code>SharedArrayBuffer</code>,
+ * <code>performance.measureMemory()</code>, * and the JS Self-Profiling API.See
+ * {@link org.apache.wicket.coep.CoepRequestCycleListener} for instructions * on how to enable COOP.
+ * Read more about cross-origin isolation on
+ * <a href="https://web.dev/why-coop-coep/">https://web.dev/why-coop-coep/</a>
+ *
+ *
+ * @author Santiago Diaz - [hidden email]
+ * @author Ecenaz Jen Ozmen - [hidden email]
+ *
+ * @see CrossOriginOpenerPolicyConfiguration
+ * @see org.apache.wicket.settings.SecuritySettings
+ */
+public class CoopRequestCycleListener implements IRequestCycleListener
+{
+ private static final Logger log = LoggerFactory.getLogger(CoopRequestCycleListener.class);
+
+ static final String COOP_HEADER = "Cross-Origin-Opener-Policy";
+
+ private CrossOriginOpenerPolicyConfiguration coopConfig;
+
+ public CoopRequestCycleListener(CrossOriginOpenerPolicyConfiguration cooopConfig)
+ {
+ this.coopConfig = cooopConfig;
+ }
+
+ @Override
+ public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
+ {
+ HttpServletRequest request = (HttpServletRequest)cycle.getRequest().getContainerRequest();

Review comment:
       Better check that the container request is HttpServletRequest before casting it.
   Otherwise tests using MockWebRequest will have do manually disable these listeners.




----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] eozmen410 commented on a change in pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

eozmen410 commented on a change in pull request #442:
URL: https://github.com/apache/wicket/pull/442#discussion_r475662770



##########
File path: wicket-core/src/main/java/org/apache/wicket/coop/CoopRequestCycleListener.java
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.coop;
+
+import org.apache.wicket.request.IRequestHandler;
+import org.apache.wicket.request.cycle.IRequestCycleListener;
+import org.apache.wicket.request.cycle.RequestCycle;
+import org.apache.wicket.request.http.WebResponse;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import javax.servlet.http.HttpServletRequest;
+
+/**
+ * Sets <a href="https://github.com/whatwg/html/pull/5334/files">Cross-Origin Opener Policy</a>
+ * headers on the responses based on the policy specified by {@link CrossOriginOpenerPolicyConfiguration}. The header
+ * is not set for the paths that are exempted from COOP.
+ *
+ * COOP is a mitigation against cross-origin information leaks and is used to make websites
+ * cross-origin isolated. Setting the COOP header allows you to ensure that a top-level window is
+ * isolated from other documents by putting them in a different browsing context group, so they
+ * cannot directly interact with the top-level window. Using COEP and COOP together allows
+ * developers to safely use * powerful features such as <code>SharedArrayBuffer</code>,
+ * <code>performance.measureMemory()</code>, * and the JS Self-Profiling API.See
+ * {@link org.apache.wicket.coep.CoepRequestCycleListener} for instructions * on how to enable COOP.
+ * Read more about cross-origin isolation on
+ * <a href="https://web.dev/why-coop-coep/">https://web.dev/why-coop-coep/</a>
+ *
+ *
+ * @author Santiago Diaz - [hidden email]
+ * @author Ecenaz Jen Ozmen - [hidden email]
+ *
+ * @see CrossOriginOpenerPolicyConfiguration
+ * @see org.apache.wicket.settings.SecuritySettings
+ */
+public class CoopRequestCycleListener implements IRequestCycleListener
+{
+ private static final Logger log = LoggerFactory.getLogger(CoopRequestCycleListener.class);
+
+ static final String COOP_HEADER = "Cross-Origin-Opener-Policy";
+
+ private CrossOriginOpenerPolicyConfiguration coopConfig;
+
+ public CoopRequestCycleListener(CrossOriginOpenerPolicyConfiguration cooopConfig)
+ {
+ this.coopConfig = cooopConfig;
+ }
+
+ @Override
+ public void onRequestHandlerResolved(RequestCycle cycle, IRequestHandler handler)
+ {
+ HttpServletRequest request = (HttpServletRequest)cycle.getRequest().getContainerRequest();

Review comment:
       We ran into this problem when running the tests in wicket-core and we disabled the listeners in the `DummyApplication` for `RequestCycleListenerTest` by overriding the init method [here](https://github.com/salcho/wicket/blob/a7a1d97ea33a97ad730a03d642e38242a9c7e28b/wicket-core/src/test/java/org/apache/wicket/request/cycle/RequestCycleListenerTest.java#L69-L78) since it was only this test case that was causing the problem. We can also revert these changes and add a check for `HttpServletRequest` 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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] eozmen410 commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

eozmen410 commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-679173295


   Thanks for the review @martin-g ! I've renamed the request cycle listeners :) Is there anything else we can do to get this 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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] eozmen410 commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

eozmen410 commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-679196121


   > IMHO the additions to Application would be better located in WebApplication#internalInit().
   > CSP is enforced there too.
   
   AFAICT the `internalInit()` method is called before `init()`, since we expect the users to configure their policies for COOP and COEP in the `init()` method, and we will add the listeners with the configs if they are not `DISABLED`, if we move the `secuirtyInit()` to `WebApplication#internalInit()` the configuration decisions made by the user in their `WebApplication#init()` will not have an effect on listener behavior.
   


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] eozmen410 edited a comment on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

eozmen410 edited a comment on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-679196121


   > IMHO the additions to Application would be better located in WebApplication#internalInit().
   > CSP is enforced there too.
   
   AFAICT the `internalInit()` method is called before `init()`, since we expect the users to configure their policies for COOP and COEP in the `init()` method, and we will add the listeners with the configs if they are not `DISABLED`, if we move the `secuirtyInit()` to `WebApplication#internalInit()` the configuration decisions made by the user in their `WebApplication#init()` will not have an effect on listener behavior. 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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] svenmeier commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

svenmeier commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-679218281


   The listeners should always query the configuration anyway - what if the app changes the configuration later on?


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] eozmen410 commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

eozmen410 commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-679853448


   > The listeners should always query the configuration anyway - what if the app changes the configuration later on?
   
   By our design right now, changes made to the configuration after the `init()` method will not have an effect on the listener. We didn't think users would want to change their policies at runtime. Also since `securityInit()` is called once during initialization, if a listener is not added in the `secuirtyInit()` (configuration was `DISABLED` in `init()`) and later on changes the configuration (enable at runtime) the listener would not be added.
   
   Are there use cases for changing the policies at runtime?


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] svenmeier commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

svenmeier commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-679857204


   It's a matter of consistency, please see how CSPRequestCycleListener does it.
   
   We have to take care of naming too: securityInit() is *not* where 'an application's security is initialized'.


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] salcho commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

salcho commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-679951678


   > It's a matter of consistency, please see how CSPRequestCycleListener does it.
   >
   > We have to take care of naming too: securityInit() is _not_ where 'an application's security is initialized'.
   
   Hi @svenmeier!
   
   We have updated the name of the method now, thanks for the suggestion! We were just wondering if you could elaborate a bit on what the use case is for changing security configurations at runtime. Do you have any cases where a developer would like to change the set of scripts that are allowed to run or the set of endpoints intended to be used cross-origin at runtime?
   
   While I agree that consistency is important, I also think it makes sense to programmatically add listeners that depend on user config. IMHO, the current approach has two downsides:
   
   - Listeners get added to the stack regardless of user config, adding latency to the processing of each request even if they don't act on the request in any way (in CSP for instance, the `mustProtect` method is called on every request even if `ContentSecurityPolicySettings` is empty).
   
   - Security configurations are not immutable. This is often the source of vulnerabilities because an attacker can usually force a code path that disables/corrupts security mitigations. Typical examples of this are [reflecting origins in CORS headers at runtime](https://hackerone.com/reports/896093), [bypassing CSP when it's built at runtime with user input](https://portswigger.net/research/bypassing-csp-with-policy-injection) or [creating arbitrary redirects on hosts based on user input](https://hackerone.com/reports/210875).
   
   Perhaps Wicket has some good reasons why CSP works the way it does currently! If that isn't the case, do you think this could be a good opportunity to improve performance/security? I think having a config-dependent `init` that runs after the user has a chance to configure their app would achieve this!
   
   We could keep consistency by moving the adding of the CSP listener to this proposed new `init` too, so that the listener is never added if no CSP has been configured. This would set a nice trend for future listeners.
   
   These are principles we've used in other contributions (see Struts' implementation of [COOP](https://github.com/apache/struts/pull/432), [CSP](https://github.com/apache/struts/pull/430) and [Fetch Metadata](https://github.com/apache/struts/pull/426)) and would really like to know if Wicket has use cases where these principles don't apply, so we'd definitely appreciate some feedback!


----------------------------------------------------------------
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:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [wicket] martin-g commented on pull request #442: [WICKET-6805] Add Cross-Origin Opener Policy and Cross-Origin Embedder Policy support

GitBox
In reply to this post by GitBox

martin-g commented on pull request #442:
URL: https://github.com/apache/wicket/pull/442#issuecomment-679958123


   > * Listeners get added to the stack regardless of user config, adding latency to the processing of each request even if they don't act on the request in any way (in CSP for instance, the `mustProtect` method is called on every request even if `ContentSecurityPolicySettings` is empty)
   
   This is a good point! I don't remember it being discussed. And I think it was not considered.
   IMO the CSP listener should be reworked to behave like the new Coop/Coep ones.
   If an application needs to reconfigure something after the application init phase it can always go through the long path:
   - iterate over the request cycle listeners
   - remove any of them
   - change configuration
   - (re)add any of them


----------------------------------------------------------------
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:
[hidden email]


12