Skip to content
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 16 additions & 6 deletions omod/src/main/java/org/openmrs/web/xss/XSSFilter.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,19 +23,20 @@
import org.springframework.web.multipart.support.DefaultMultipartHttpServletRequest;

public class XSSFilter implements Filter {


private static final String WEB_SERVICE_PATH = "/ws/";

Check warning on line 27 in omod/src/main/java/org/openmrs/web/xss/XSSFilter.java

View check run for this annotation

SonarQubeCloud / SonarCloud Code Analysis

Refactor your code to get this URI from a customizable parameter.

See more on https://sonarcloud.io/project/issues?id=openmrs_openmrs-module-legacyui&issues=AZ7OwGPqhsNhAUEFGDCj&open=AZ7OwGPqhsNhAUEFGDCj&pullRequest=265

@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);
}

Expand All @@ -48,4 +49,13 @@
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);
}
}
124 changes: 124 additions & 0 deletions omod/src/test/java/org/openmrs/web/xss/XSSFilterTest.java
Original file line number Diff line number Diff line change
@@ -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", "<script>alert(1)</script>");

filter.doFilter(request, new MockHttpServletResponse(), chain);

assertNotNull(chain.capturedRequest);
assertEquals("&lt;script&gt;alert(1)&lt;/script&gt;", 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", "<x>");

filter.doFilter(request, new MockHttpServletResponse(), chain);

assertNotNull(chain.capturedRequest);
assertEquals("<x>", 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", "<x>");

filter.doFilter(request, new MockHttpServletResponse(), chain);

assertNotNull(chain.capturedRequest);
assertEquals("&lt;x&gt;", 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;
}
}
}