[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-31 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r252705728
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
 ##
 @@ -586,7 +586,14 @@ private WebAppContext loadWar(final File warFile, final 
String contextPath, fina
 // configure the max form size (3x the default)
 webappContext.setMaxFormContentSize(60);
 
-addHTTPHeaders(webappContext);
+ArrayList> filters = new ArrayList<>();
 
 Review comment:
   Updated


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-28 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r251531600
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
 ##
 @@ -602,6 +599,28 @@ private WebAppContext loadWar(final File warFile, final 
String contextPath, fina
 return webappContext;
 }
 
+private void addHTTPHeaders(WebAppContext webappContext) {
+// Add a filter to set the X-Frame-Options header
+FilterHolder xfoFilter = new FilterHolder(new XFrameOptionsFilter());
 
 Review comment:
   I could potentially drop that and do a much simpler add to the webappContext?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-28 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r251531021
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
 ##
 @@ -602,6 +599,28 @@ private WebAppContext loadWar(final File warFile, final 
String contextPath, fina
 return webappContext;
 }
 
+private void addHTTPHeaders(WebAppContext webappContext) {
+// Add a filter to set the X-Frame-Options header
+FilterHolder xfoFilter = new FilterHolder(new XFrameOptionsFilter());
 
 Review comment:
   I believe the only reason I did this was so that I could set the name for 
the holder/filter, which made it easier to debug when I was having issues 
adding filters correctly. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-28 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r251527821
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
 ##
 @@ -583,13 +586,7 @@ private WebAppContext loadWar(final File warFile, final 
String contextPath, fina
 // configure the max form size (3x the default)
 webappContext.setMaxFormContentSize(60);
 
-// add a filter to set the X-Frame-Options filter
-webappContext.addFilter(new FilterHolder(FRAME_OPTIONS_FILTER), "/*", 
EnumSet.allOf(DispatcherType.class));
-
-// add a filter to set the Content Security Policy frame-ancestors 
directive
-FilterHolder cspFilter = new FilterHolder(new 
ContentSecurityPolicyFilter());
-cspFilter.setName(ContentSecurityPolicyFilter.class.getSimpleName());
-webappContext.addFilter(cspFilter, "/*", 
EnumSet.allOf(DispatcherType.class));
+addHTTPHeaders(webappContext);
 
 Review comment:
   Fixed this as well.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-28 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r251457960
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
 ##
 @@ -602,6 +599,28 @@ private WebAppContext loadWar(final File warFile, final 
String contextPath, fina
 return webappContext;
 }
 
+private void addHTTPHeaders(WebAppContext webappContext) {
+// Add a filter to set the X-Frame-Options header
+FilterHolder xfoFilter = new FilterHolder(new XFrameOptionsFilter());
+xfoFilter.setName(XFrameOptionsFilter.class.getSimpleName());
+webappContext.addFilter(xfoFilter, "/*", 
EnumSet.allOf(DispatcherType.class));
+
+// Add a filter to set the Content Security Policy frame-ancestors 
directive
+FilterHolder cspFilter = new FilterHolder(new 
ContentSecurityPolicyFilter());
 
 Review comment:
   I've updated the PR with a commit to do what I think you're suggesting. Let 
me know if it's in any way correct :)


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-25 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r251139976
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-web-security/src/main/java/org/apache/nifi/web/security/headers/ContentSecurityPolicyFilter.java
 ##
 @@ -28,7 +28,7 @@
 import javax.servlet.FilterConfig;
 
 /**
- * A filter to apply the Content Security Policy (which supersedes the 
X-Frame-Options header).
+ * A filter to apply the Content Security Policy header.
  *
  */
 public class ContentSecurityPolicyFilter implements Filter {
 
 Review comment:
   We can apply the `default-src 'self'` directive but it also requires adding 
`'unsafe-inline'`, otherwise our many inline scripts will be blocked by 
default. They're already allowed anyway, so I guess there's no problem with 
this.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-23 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r250238788
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/test/java/org/apache/nifi/web/server/JettyServerTest.java
 ##
 @@ -155,29 +155,4 @@ public void 
testConfigureSslContextFactoryWithPkcsTrustStore() {
 verify(contextFactory).setTrustStoreType(trustStoreType);
 
verify(contextFactory).setTrustStoreProvider(BouncyCastleProvider.PROVIDER_NAME);
 }
-
-@Test
-public void testNoDuplicateXFrameOptions() throws NoSuchFieldException, 
IllegalAccessException, ServletException, IOException {
 
 Review comment:
   Removed. I think this was simply a bad test and that what it was checking 
for is handled by another unit test in HTTPHeaderFiltersTest.
   
   I also think I should probably add some integration tests to check for these 
HTTP headers in actual responses from JettyServer? But I didn't know where to 
get an example on how we do that. At the least, the tests in 
HTTPHeaderFiltersTest checks that each individual filter adds the headers we 
expect. It just doesn't check whether the filters are added correctly and used 
by JettyServer.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-23 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r250238788
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/test/java/org/apache/nifi/web/server/JettyServerTest.java
 ##
 @@ -155,29 +155,4 @@ public void 
testConfigureSslContextFactoryWithPkcsTrustStore() {
 verify(contextFactory).setTrustStoreType(trustStoreType);
 
verify(contextFactory).setTrustStoreProvider(BouncyCastleProvider.PROVIDER_NAME);
 }
-
-@Test
-public void testNoDuplicateXFrameOptions() throws NoSuchFieldException, 
IllegalAccessException, ServletException, IOException {
 
 Review comment:
   Removed. I think this was simply a bad test and that what it was checking 
for is handled by another unit test in HTTPHeaderFiltersTest.
   
   I also think I should probably add some integration tests to check for these 
HTTP headers in actual responses from JettyServer? But I didn't know where to 
get an example on how we do that. At the least, the tests in 
HTTPHeaderFiltersTest checks that each individual filter adds the headers we 
expect.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-23 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r250238788
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/test/java/org/apache/nifi/web/server/JettyServerTest.java
 ##
 @@ -155,29 +155,4 @@ public void 
testConfigureSslContextFactoryWithPkcsTrustStore() {
 verify(contextFactory).setTrustStoreType(trustStoreType);
 
verify(contextFactory).setTrustStoreProvider(BouncyCastleProvider.PROVIDER_NAME);
 }
-
-@Test
-public void testNoDuplicateXFrameOptions() throws NoSuchFieldException, 
IllegalAccessException, ServletException, IOException {
 
 Review comment:
   Removed. I think this was simply a bad test and that what it was checking 
for is handled by another unit test in HTTPHeaderFiltersTest.
   
   I also think I should probably add some integration tests to check for these 
HTTP headers in actual responses, but I didn't know where to get an example on 
how we do that.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-23 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r250236583
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
 ##
 @@ -602,6 +599,28 @@ private WebAppContext loadWar(final File warFile, final 
String contextPath, fina
 return webappContext;
 }
 
+private void addHTTPHeaders(WebAppContext webappContext) {
+// Add a filter to set the X-Frame-Options header
+FilterHolder xfoFilter = new FilterHolder(new XFrameOptionsFilter());
+xfoFilter.setName(XFrameOptionsFilter.class.getSimpleName());
+webappContext.addFilter(xfoFilter, "/*", 
EnumSet.allOf(DispatcherType.class));
+
+// Add a filter to set the Content Security Policy frame-ancestors 
directive
+FilterHolder cspFilter = new FilterHolder(new 
ContentSecurityPolicyFilter());
+cspFilter.setName(ContentSecurityPolicyFilter.class.getSimpleName());
+webappContext.addFilter(cspFilter, "/*", 
EnumSet.allOf(DispatcherType.class));
+
+// Add a filter to set the HSTS header
 
 Review comment:
   I'm not certain of the performance impact. From what I've seen when 
debugging Jetty Filters, the call stack is pretty deep. I believe that for each 
request, Jetty will iterate through the contexts until it finds a context 
match, and then iterate through the attached filters? I'm not certain how 
significant the impact of adding a few more filters will be. Do we have a 
request response time filter for request/response times or did I imagine that? 
We could try using that to check how much longer requests take. 
   
   I believe adding X-XSS-Protection will reduce performance when the browser 
performs XSS checks?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] thenatog commented on a change in pull request #3273: NIFI-5968 - Added the X-XSS-Protection and Strict-Transport-Security …

2019-01-23 Thread GitBox 
thenatog commented on a change in pull request #3273: NIFI-5968 - Added the 
X-XSS-Protection and Strict-Transport-Security …
URL: https://github.com/apache/nifi/pull/3273#discussion_r250225707
 
 

 ##
 File path: 
nifi-nar-bundles/nifi-framework-bundle/nifi-framework/nifi-web/nifi-jetty/src/main/java/org/apache/nifi/web/server/JettyServer.java
 ##
 @@ -583,13 +586,7 @@ private WebAppContext loadWar(final File warFile, final 
String contextPath, fina
 // configure the max form size (3x the default)
 webappContext.setMaxFormContentSize(60);
 
-// add a filter to set the X-Frame-Options filter
-webappContext.addFilter(new FilterHolder(FRAME_OPTIONS_FILTER), "/*", 
EnumSet.allOf(DispatcherType.class));
-
-// add a filter to set the Content Security Policy frame-ancestors 
directive
-FilterHolder cspFilter = new FilterHolder(new 
ContentSecurityPolicyFilter());
-cspFilter.setName(ContentSecurityPolicyFilter.class.getSimpleName());
-webappContext.addFilter(cspFilter, "/*", 
EnumSet.allOf(DispatcherType.class));
+addHTTPHeaders(webappContext);
 
 Review comment:
   Ah yes, I thought about that and then forgot. I'll look more into that.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services