From e259319f06382315f7c276bb3c603b15e812f914 Mon Sep 17 00:00:00 2001 From: Dimitris Kavvathas Date: Fri, 23 Sep 2022 15:31:39 +0200 Subject: [PATCH] SONAR-17343 Add security headers in error HTTP code responses --- .../server/app/SecureErrorReportValve.java | 40 ++++++++++++ .../sonar/server/app/TomcatErrorHandling.java | 2 +- .../platform/web/SecurityServletFilter.java | 21 +++++-- .../app/SecurityErrorReportValveTest.java | 62 +++++++++++++++++++ .../server/app/TomcatErrorHandlingTest.java | 3 +- .../web/SecurityServletFilterTest.java | 36 ++++++++--- 6 files changed, 146 insertions(+), 18 deletions(-) create mode 100644 server/sonar-webserver/src/main/java/org/sonar/server/app/SecureErrorReportValve.java create mode 100644 server/sonar-webserver/src/test/java/org/sonar/server/app/SecurityErrorReportValveTest.java diff --git a/server/sonar-webserver/src/main/java/org/sonar/server/app/SecureErrorReportValve.java b/server/sonar-webserver/src/main/java/org/sonar/server/app/SecureErrorReportValve.java new file mode 100644 index 00000000000..3c8db3332c0 --- /dev/null +++ b/server/sonar-webserver/src/main/java/org/sonar/server/app/SecureErrorReportValve.java @@ -0,0 +1,40 @@ +/* + * SonarQube + * Copyright (C) 2009-2022 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +package org.sonar.server.app; + +import java.io.IOException; +import javax.servlet.ServletException; +import org.apache.catalina.connector.Request; +import org.apache.catalina.connector.Response; +import org.apache.catalina.valves.ErrorReportValve; +import org.sonar.server.platform.web.SecurityServletFilter; + +/** + * Extending the ErrorReportValve to add security HTTP headers in all responses. + */ +public class SecureErrorReportValve extends ErrorReportValve { + + @Override + public void invoke(Request request, Response response) throws IOException, ServletException { + SecurityServletFilter.addSecurityHeaders(request, response); + super.invoke(request, response); + } +} diff --git a/server/sonar-webserver/src/main/java/org/sonar/server/app/TomcatErrorHandling.java b/server/sonar-webserver/src/main/java/org/sonar/server/app/TomcatErrorHandling.java index cdba2e4fb42..1089d1a1e6e 100644 --- a/server/sonar-webserver/src/main/java/org/sonar/server/app/TomcatErrorHandling.java +++ b/server/sonar-webserver/src/main/java/org/sonar/server/app/TomcatErrorHandling.java @@ -24,7 +24,7 @@ import org.apache.catalina.valves.ErrorReportValve; public class TomcatErrorHandling { void configure(Tomcat tomcat) { - ErrorReportValve valve = new ErrorReportValve(); + ErrorReportValve valve = new SecureErrorReportValve(); valve.setShowServerInfo(false); valve.setShowReport(false); tomcat.getHost().getPipeline().addValve(valve); diff --git a/server/sonar-webserver/src/main/java/org/sonar/server/platform/web/SecurityServletFilter.java b/server/sonar-webserver/src/main/java/org/sonar/server/platform/web/SecurityServletFilter.java index 30193cd0acc..3e4f10d0fbb 100644 --- a/server/sonar-webserver/src/main/java/org/sonar/server/platform/web/SecurityServletFilter.java +++ b/server/sonar-webserver/src/main/java/org/sonar/server/platform/web/SecurityServletFilter.java @@ -55,24 +55,35 @@ public class SecurityServletFilter implements Filter { } // WARNING, headers must be added before the doFilter, otherwise they won't be added when response is already committed (for instance when a WS is called) + addSecurityHeaders(httpRequest, httpResponse); + chain.doFilter(httpRequest, httpResponse); + } + + /** + * Adds security HTTP headers in the response. The headers are added using {@code setHeader()}, which overwrites existing headers. + */ + public static void addSecurityHeaders(HttpServletRequest httpRequest, HttpServletResponse httpResponse) { // Clickjacking protection // See https://www.owasp.org/index.php/Clickjacking_Protection_for_Java_EE // The protection is disabled on purpose for integration in external systems like Github (/integration/github). String path = httpRequest.getRequestURI().replaceFirst(httpRequest.getContextPath(), ""); if (!path.startsWith("/integration/")) { - httpResponse.addHeader("X-Frame-Options", "SAMEORIGIN"); + httpResponse.setHeader("X-Frame-Options", "SAMEORIGIN"); + } + + // If the request is secure, the Strict-Transport-Security header is added. + if ("https".equals(httpRequest.getHeader("x-forwarded-proto"))) { + httpResponse.setHeader("Strict-Transport-Security", "max-age=31536000; includeSubDomains;"); } // Cross-site scripting // See https://www.owasp.org/index.php/List_of_useful_HTTP_headers - httpResponse.addHeader("X-XSS-Protection", "1; mode=block"); + httpResponse.setHeader("X-XSS-Protection", "1; mode=block"); // MIME-sniffing // See https://www.owasp.org/index.php/List_of_useful_HTTP_headers - httpResponse.addHeader("X-Content-Type-Options", "nosniff"); - - chain.doFilter(httpRequest, httpResponse); + httpResponse.setHeader("X-Content-Type-Options", "nosniff"); } @Override diff --git a/server/sonar-webserver/src/test/java/org/sonar/server/app/SecurityErrorReportValveTest.java b/server/sonar-webserver/src/test/java/org/sonar/server/app/SecurityErrorReportValveTest.java new file mode 100644 index 00000000000..062cd972d98 --- /dev/null +++ b/server/sonar-webserver/src/test/java/org/sonar/server/app/SecurityErrorReportValveTest.java @@ -0,0 +1,62 @@ +/* + * SonarQube + * Copyright (C) 2009-2022 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + +package org.sonar.server.app; + +import java.io.IOException; +import javax.servlet.ServletException; +import org.apache.catalina.connector.Request; +import org.apache.catalina.connector.Response; +import org.apache.catalina.valves.ValveBase; +import org.junit.Test; + +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +public class SecurityErrorReportValveTest { + + SecureErrorReportValve underTest = spy(SecureErrorReportValve.class); + + @Test + public void add_security_headers() throws ServletException, IOException { + var request = mock(Request.class); + var response = mock(Response.class); + + underTest.setNext(new ValveBase() { + @Override + public void invoke(Request request, Response response) { + } + }); + + when(request.getMethod()).thenReturn("GET"); + when(request.getRequestURI()).thenReturn("/"); + when(request.getContextPath()).thenReturn(""); + when(request.getHeader("x-forwarded-proto")).thenReturn("https"); + + underTest.invoke(request, response); + + verify(response).setHeader("X-Frame-Options", "SAMEORIGIN"); + verify(response).setHeader("X-XSS-Protection", "1; mode=block"); + verify(response).setHeader("X-Content-Type-Options", "nosniff"); + verify(response).setHeader("Strict-Transport-Security", "max-age=31536000; includeSubDomains;"); + } +} diff --git a/server/sonar-webserver/src/test/java/org/sonar/server/app/TomcatErrorHandlingTest.java b/server/sonar-webserver/src/test/java/org/sonar/server/app/TomcatErrorHandlingTest.java index fa999482938..1c1d31dbcca 100644 --- a/server/sonar-webserver/src/test/java/org/sonar/server/app/TomcatErrorHandlingTest.java +++ b/server/sonar-webserver/src/test/java/org/sonar/server/app/TomcatErrorHandlingTest.java @@ -20,7 +20,6 @@ package org.sonar.server.app; import org.apache.catalina.startup.Tomcat; -import org.apache.catalina.valves.ErrorReportValve; import org.junit.Test; import org.mockito.Mockito; @@ -35,6 +34,6 @@ public class TomcatErrorHandlingTest { public void enable_access_logs_by_Default() { Tomcat tomcat = mock(Tomcat.class, Mockito.RETURNS_DEEP_STUBS); underTest.configure(tomcat); - verify(tomcat.getHost().getPipeline()).addValve(any(ErrorReportValve.class)); + verify(tomcat.getHost().getPipeline()).addValve(any(SecureErrorReportValve.class)); } } diff --git a/server/sonar-webserver/src/test/java/org/sonar/server/platform/web/SecurityServletFilterTest.java b/server/sonar-webserver/src/test/java/org/sonar/server/platform/web/SecurityServletFilterTest.java index eb0254c331e..b73e0c67fa4 100644 --- a/server/sonar-webserver/src/test/java/org/sonar/server/platform/web/SecurityServletFilterTest.java +++ b/server/sonar-webserver/src/test/java/org/sonar/server/platform/web/SecurityServletFilterTest.java @@ -26,6 +26,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import org.junit.Test; +import static org.junit.Assert.assertNull; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; @@ -82,14 +83,29 @@ public class SecurityServletFilterTest { } @Test - public void set_security_headers() throws Exception { + public void set_security_headers_non_secure_request() throws Exception { HttpServletRequest request = newRequest("GET", "/"); + when(request.getHeader("x-forwarded-proto")).thenReturn("https"); underTest.doFilter(request, response, chain); - verify(response).addHeader("X-Frame-Options", "SAMEORIGIN"); - verify(response).addHeader("X-XSS-Protection", "1; mode=block"); - verify(response).addHeader("X-Content-Type-Options", "nosniff"); + verify(response).setHeader("X-Frame-Options", "SAMEORIGIN"); + verify(response).setHeader("X-XSS-Protection", "1; mode=block"); + verify(response).setHeader("X-Content-Type-Options", "nosniff"); + assertNull(response.getHeader("Strict-Transport-Security")); + } + + @Test + public void set_security_headers_secure_request() throws ServletException, IOException { + HttpServletRequest request = newRequest("GET", "/"); + when(request.getHeader("x-forwarded-proto")).thenReturn("https"); + + underTest.doFilter(request, response, chain); + + verify(response).setHeader("X-Frame-Options", "SAMEORIGIN"); + verify(response).setHeader("X-XSS-Protection", "1; mode=block"); + verify(response).setHeader("X-Content-Type-Options", "nosniff"); + verify(response).setHeader("Strict-Transport-Security", "max-age=31536000; includeSubDomains;"); } @Test @@ -98,9 +114,9 @@ public class SecurityServletFilterTest { underTest.doFilter(request, response, chain); - verify(response, never()).addHeader(eq("X-Frame-Options"), anyString()); - verify(response).addHeader("X-XSS-Protection", "1; mode=block"); - verify(response).addHeader("X-Content-Type-Options", "nosniff"); + verify(response, never()).setHeader(eq("X-Frame-Options"), anyString()); + verify(response).setHeader("X-XSS-Protection", "1; mode=block"); + verify(response).setHeader("X-Content-Type-Options", "nosniff"); } @Test @@ -112,9 +128,9 @@ public class SecurityServletFilterTest { underTest.doFilter(request, response, chain); - verify(response, never()).addHeader(eq("X-Frame-Options"), anyString()); - verify(response).addHeader("X-XSS-Protection", "1; mode=block"); - verify(response).addHeader("X-Content-Type-Options", "nosniff"); + verify(response, never()).setHeader(eq("X-Frame-Options"), anyString()); + verify(response).setHeader("X-XSS-Protection", "1; mode=block"); + verify(response).setHeader("X-Content-Type-Options", "nosniff"); } private static HttpServletRequest newRequest(String httpMethod, String path) { -- 2.39.5