diff --git a/omod/src/main/java/org/openmrs/web/xss/XSSFilter.java b/omod/src/main/java/org/openmrs/web/xss/XSSFilter.java index c3857ffa1..5061f7c75 100644 --- a/omod/src/main/java/org/openmrs/web/xss/XSSFilter.java +++ b/omod/src/main/java/org/openmrs/web/xss/XSSFilter.java @@ -23,19 +23,20 @@ import org.springframework.web.multipart.support.DefaultMultipartHttpServletRequest; public class XSSFilter implements Filter { - + + private static final String WEB_SERVICE_PATH = "/ws/"; + @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { - - if (!"GET".equalsIgnoreCase(((HttpServletRequest) request).getMethod())) { - if (ServletFileUpload.isMultipartContent((HttpServletRequest) request)) { + HttpServletRequest httpRequest = (HttpServletRequest) request; + if (!"GET".equalsIgnoreCase(httpRequest.getMethod()) && !isWebServiceRequest(httpRequest)) { + if (ServletFileUpload.isMultipartContent(httpRequest)) { request = new XSSMultipartRequestWrapper((DefaultMultipartHttpServletRequest) request); } else { - request = new XSSRequestWrapper((HttpServletRequest) request); + request = new XSSRequestWrapper(httpRequest); } } - chain.doFilter(request, response); } @@ -48,4 +49,13 @@ public void init(FilterConfig filterConfig) throws ServletException { public void destroy() { } + + private boolean isWebServiceRequest(HttpServletRequest request) { + String requestUri = request.getRequestURI(); + if (requestUri == null) { + return false; + } + String path = requestUri.substring(request.getContextPath().length()); + return path.startsWith(WEB_SERVICE_PATH); + } } diff --git a/omod/src/test/java/org/openmrs/web/xss/XSSFilterTest.java b/omod/src/test/java/org/openmrs/web/xss/XSSFilterTest.java new file mode 100644 index 000000000..3d5699b5b --- /dev/null +++ b/omod/src/test/java/org/openmrs/web/xss/XSSFilterTest.java @@ -0,0 +1,124 @@ +/** + * This Source Code Form is subject to the terms of the Mozilla Public License, + * v. 2.0. If a copy of the MPL was not distributed with this file, You can + * obtain one at http://mozilla.org/MPL/2.0/. OpenMRS is also distributed under + * the terms of the Healthcare Disclaimer located at http://openmrs.org/license. + * + * Copyright (C) OpenMRS Inc. OpenMRS is a registered trademark and the OpenMRS + * graphic logo is a trademark of OpenMRS Inc. + */ +package org.openmrs.web.xss; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; + +import java.nio.charset.StandardCharsets; + +import javax.servlet.FilterChain; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; + +import org.apache.commons.io.IOUtils; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpServletResponse; + +/** + * Unit tests for {@link XSSFilter}. These assert the central contract: web-service request bodies + * (REST/FHIR under {@code /ws/}) must pass through untouched, while legacy server-rendered form + * parameters are still HTML-sanitized. + */ +public class XSSFilterTest { + + private XSSFilter filter; + + private CapturingFilterChain chain; + + @BeforeEach + public void setUp() { + filter = new XSSFilter(); + chain = new CapturingFilterChain(); + } + + @Test + public void doFilter_shouldPassWebServiceJsonBodyThroughUnchanged() throws Exception { + String json = "{\"value\":\"Hello <> & friends\"}"; + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/openmrs/ws/rest/v1/obs"); + request.setContextPath("/openmrs"); + request.setContent(json.getBytes(StandardCharsets.UTF_8)); + + filter.doFilter(request, new MockHttpServletResponse(), chain); + + assertNotNull(chain.capturedRequest); + String body = IOUtils.toString(chain.capturedRequest.getInputStream(), StandardCharsets.UTF_8.name()); + assertEquals(json, body, + "Web-service request bodies must reach the resource layer as raw bytes, never HTML-encoded"); + } + + @Test + public void doFilter_shouldPassFhirRequestBodyThroughUnchanged() throws Exception { + String json = "{\"valueString\":\"a < b & c > d\"}"; + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/openmrs/ws/fhir2/R4/Observation"); + request.setContextPath("/openmrs"); + request.setContent(json.getBytes(StandardCharsets.UTF_8)); + + filter.doFilter(request, new MockHttpServletResponse(), chain); + + assertNotNull(chain.capturedRequest); + String body = IOUtils.toString(chain.capturedRequest.getInputStream(), StandardCharsets.UTF_8.name()); + assertEquals(json, body, "FHIR request bodies must also pass through untouched"); + } + + @Test + public void doFilter_shouldSanitizeLegacyFormParameters() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/openmrs/admin/concepts/concept.form"); + request.setContextPath("/openmrs"); + request.setParameter("name", ""); + + filter.doFilter(request, new MockHttpServletResponse(), chain); + + assertNotNull(chain.capturedRequest); + assertEquals("<script>alert(1)</script>", chain.capturedRequest.getParameter("name"), + "Legacy form parameters must still be HTML-encoded to defend server-rendered pages"); + } + + @Test + public void doFilter_shouldNotWrapWebServiceRequestsSoParametersAreUntouched() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/openmrs/ws/rest/v1/concept"); + request.setContextPath("/openmrs"); + request.setParameter("q", ""); + + filter.doFilter(request, new MockHttpServletResponse(), chain); + + assertNotNull(chain.capturedRequest); + assertEquals("", chain.capturedRequest.getParameter("q"), + "Web-service requests are not wrapped, so their parameters are not HTML-encoded either"); + } + + @Test + public void doFilter_shouldSanitizeLegacyRequestWhoseUrlMerelyContainsWsElsewhere() throws Exception { + // A legacy endpoint reached via a URL that contains "/ws/" somewhere other than the path + // prefix must NOT be treated as a web service, otherwise it could evade param sanitization. + MockHttpServletRequest request = new MockHttpServletRequest("POST", "/openmrs/patientDashboard/ws/edit.form"); + request.setContextPath("/openmrs"); + request.setParameter("name", ""); + + filter.doFilter(request, new MockHttpServletResponse(), chain); + + assertNotNull(chain.capturedRequest); + assertEquals("<x>", chain.capturedRequest.getParameter("name"), + "Only a /ws/ prefix (after the context path) is a web service; legacy paths stay sanitized"); + } + + private static class CapturingFilterChain implements FilterChain { + + private HttpServletRequest capturedRequest; + + @Override + public void doFilter(ServletRequest request, ServletResponse response) { + this.capturedRequest = (HttpServletRequest) request; + } + } +} \ No newline at end of file