Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-5366 - Added ContentSecurityPolicyFilter which stops framing of … #2989

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -74,6 +74,7 @@
import org.apache.nifi.web.ContentAccess;
import org.apache.nifi.web.NiFiWebConfigurationContext;
import org.apache.nifi.web.UiExtensionType;
import org.apache.nifi.web.security.ContentSecurityPolicyFilter;
import org.eclipse.jetty.annotations.AnnotationConfiguration;
import org.eclipse.jetty.server.Connector;
import org.eclipse.jetty.server.Handler;
Expand Down Expand Up @@ -502,6 +503,11 @@ private WebAppContext loadWar(final File warFile, final String contextPath, fina
// 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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From what I can tell by tracing the Jetty code, this is almost equivalent to just assigning the Filter directly via addFilter(). Do you have a resource or documentation that indicates why using a FilterHolder is preferable?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I thought there was no way to set the Filter name without passing the FilterHolder but it looks like if you pass the classname as a string it will set the filter name. Setting the filter name is helpful for debugging. Looks like most filters being added use the FilterHolder method definition, not exactly sure why however.

cspFilter.setName(ContentSecurityPolicyFilter.class.getSimpleName());
webappContext.addFilter(cspFilter, "/*", EnumSet.allOf(DispatcherType.class));

try {
// configure the class loader - webappClassLoader -> jetty nar -> web app's nar -> ...
webappContext.setClassLoader(new WebAppClassLoader(parentClassLoader, webappContext));
Expand Down
Expand Up @@ -154,5 +154,16 @@
<artifactId>jettison</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.springframework</groupId>
<artifactId>spring-test</artifactId>
<version>5.0.6.RELEASE</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-servlet</artifactId>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's move the non-test dependencies above so they are all together with the compile dependencies and the test dependencies are together. Not a technical necessity, but good for logical grouping and identification.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed one unnecessary dependency and changed the other to scope test (as it's only used in the unit test).

<scope>test</scope>
</dependency>
</dependencies>
</project>
@@ -0,0 +1,57 @@
/*
* 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.nifi.web.security;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import javax.servlet.FilterChain;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;
import javax.servlet.Filter;
import javax.servlet.FilterConfig;

/**
* A filter to apply the Content Security Policy (which supersedes the X-Frame-Options header).
*
*/
public class ContentSecurityPolicyFilter implements Filter {
private static final String HEADER = "Content-Security-Policy";
private static final String POLICY = "frame-ancestors 'self'";

private static final Logger logger = LoggerFactory.getLogger(ContentSecurityPolicyFilter.class);

@Override
public void doFilter(final ServletRequest req, final ServletResponse resp, final FilterChain filterChain)
throws IOException, ServletException {

final HttpServletResponse response = (HttpServletResponse) resp;
response.setHeader(HEADER, POLICY);

filterChain.doFilter(req, resp);
}

@Override
public void init(final FilterConfig config) {
}

@Override
public void destroy() {
}
}
@@ -0,0 +1,70 @@
/*
* 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.nifi.web.security;

import org.eclipse.jetty.servlet.FilterHolder;
import org.junit.Test;
import org.mockito.Mockito;
import org.springframework.mock.web.MockHttpServletResponse;

import javax.servlet.FilterChain;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import java.io.IOException;

import static org.junit.Assert.assertEquals;

public class ContentSecurityPolicyFilterTest {

@Test
public void testCSPHeaderApplied() throws ServletException, IOException {
// Arrange

FilterHolder originFilter = new FilterHolder(new ContentSecurityPolicyFilter());

// Set up request
HttpServletRequest mockRequest = Mockito.mock(HttpServletRequest.class);
MockHttpServletResponse mockResponse = new MockHttpServletResponse();
FilterChain mockFilterChain = Mockito.mock(FilterChain.class);

// Action
originFilter.getFilter().doFilter(mockRequest, mockResponse, mockFilterChain);

// Verify
assertEquals("frame-ancestors 'self'", mockResponse.getHeader("Content-Security-Policy"));
}

@Test
public void testCSPHeaderAppliedOnlyOnce() throws ServletException, IOException {
// Arrange

FilterHolder originFilter = new FilterHolder(new ContentSecurityPolicyFilter());

// Set up request
HttpServletRequest mockRequest = Mockito.mock(HttpServletRequest.class);
MockHttpServletResponse mockResponse = new MockHttpServletResponse();
FilterChain mockFilterChain = Mockito.mock(FilterChain.class);

// Action
originFilter.getFilter().doFilter(mockRequest, mockResponse, mockFilterChain);
originFilter.getFilter().doFilter(mockRequest, mockResponse, mockFilterChain);

// Verify
assertEquals("frame-ancestors 'self'", mockResponse.getHeader("Content-Security-Policy"));
}

}