Reflected XSS vulnerability in Testpage overlay

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5717-reflected-xss-testpage-overlay-vulnerability.yaml

@@ -0,0 +1,4 @@
+---
+type: security
+issue: 5717
+title: "Fixed the reflected XSS vulnerability in the HAPI FHIR Testpage Overlay module."

hapi-fhir-testpage-overlay/src/main/java/ca/uhn/fhir/to/Controller.java

@@ -36,6 +36,7 @@
 import ca.uhn.fhir.to.model.TransactionRequest;
 import ca.uhn.fhir.to.util.HfqlRenderingUtil;
 import ca.uhn.fhir.util.StopWatch;
+import ca.uhn.fhir.util.UrlUtil;
 import com.google.gson.stream.JsonWriter;
 import jakarta.annotation.Nonnull;
 import jakarta.annotation.Nullable;
@@ -279,15 +280,15 @@ public String actionRead(
 			theModel.put("errorMsg", toDisplayError(e.toString(), e));
 			return "resource";
 		}
-		String id = sanitizeUrlPart(defaultString(theServletRequest.getParameter("id")));
+		String id = sanitizeUrlPart(UrlUtil.unescape(theServletRequest.getParameter("id")));
 		if (StringUtils.isBlank(id)) {
 			populateModelForResource(theServletRequest, theRequest, theModel);
 			theModel.put("errorMsg", toDisplayError("No ID specified", null));
 			return "resource";
 		}
 		ResultType returnsResource = ResultType.RESOURCE;
 
-		String versionId = sanitizeUrlPart(defaultString(theServletRequest.getParameter("vid")));
+		String versionId = sanitizeUrlPart(UrlUtil.unescape((theServletRequest.getParameter("vid"))));
 		String outcomeDescription;
 		if (StringUtils.isBlank(versionId)) {
 			versionId = null;
@@ -800,9 +801,9 @@ public String actionOperation(
 			final BindingResult theBindingResult,
 			final ModelMap theModel) {
 
-		String instanceType = theReq.getParameter("instanceType");
-		String instanceId = theReq.getParameter("instanceId");
-		String operationName = theReq.getParameter("operationName");
+		String instanceType = sanitizeUrlPart((UrlUtil.unescape(theReq.getParameter("instanceType"))));
+		String instanceId = sanitizeUrlPart((UrlUtil.unescape(theReq.getParameter("instanceId"))));
+		String operationName = sanitizeUrlPart((UrlUtil.unescape(theReq.getParameter("operationName"))));
 
 		boolean finished = false;
 

hapi-fhir-testpage-overlay/src/test/java/ca/uhn/fhir/jpa/test/WebTest.java

@@ -31,6 +31,7 @@
 import org.htmlunit.cssparser.parser.CSSErrorHandler;
 import org.htmlunit.html.HtmlAnchor;
 import org.htmlunit.html.HtmlButton;
+import org.htmlunit.html.HtmlElement;
 import org.htmlunit.html.HtmlPage;
 import org.htmlunit.html.HtmlTable;
 import org.htmlunit.html.HtmlTableCell;
@@ -41,12 +42,14 @@
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.test.web.servlet.MockMvc;
-import org.springframework.test.web.servlet.htmlunit.MockMvcWebConnection;
 import org.springframework.test.web.servlet.setup.MockMvcBuilders;
 import org.springframework.web.context.support.AnnotationConfigWebApplicationContext;
 import org.springframework.web.servlet.DispatcherServlet;
@@ -57,10 +60,15 @@
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.util.List;
+import java.util.Optional;
+import java.util.function.Predicate;
+import java.util.stream.Stream;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.junit.jupiter.params.provider.Arguments.arguments;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.Mockito.when;
 
@@ -191,6 +199,47 @@ public void testInvokeCustomOperation() throws IOException {
 		assertThat(summaryPage.asNormalizedText(), containsString("Result Narrative\t\nHELLO WORLD DOCUMENT"));
 	}
 
+	private static Stream<Arguments> getButtonMappingPredicates() {
+		Predicate<HtmlElement> readButtonPredicate = t -> t.getAttribute("value").equals("read");
+		Predicate<HtmlElement> summaryButtonPredicate = t -> t.asNormalizedText().equals("$summary");
+		return Stream.of(arguments(readButtonPredicate), arguments(summaryButtonPredicate));
+	}
+
+	@ParameterizedTest
+	@MethodSource(value = "getButtonMappingPredicates")
+	public void testInvokeOperation_withReflectedXssAttack_resultHtmlIsSanitized(
+		Predicate<HtmlElement> theButtonMappingPredicate) throws IOException {
+
+		register5Patients();
+
+		HtmlPage searchResultPage = searchForPatients();
+		HtmlTable controlsTable = searchResultPage.getHtmlElementById("resultControlsTable");
+		List<HtmlTableRow> controlRows = controlsTable.getBodies().get(0).getRows();
+		HtmlTableCell controlsCell = controlRows.get(0).getCell(0);
+
+		// find the button
+		HtmlElement summaryButton = controlsCell
+			.getElementsByTagName("button")
+			.stream()
+			.filter(theButtonMappingPredicate)
+			.findFirst()
+			.orElseThrow();
+
+		// alter button attributes to imitate Reflected XSS attack
+		summaryButton.setAttribute("data2", "A0%3Cscript%3Ealert(2)%3C/script%3E");
+		summaryButton.setAttribute("data3", "%24diff%3Cscript%3Ealert(1)%3C/script%3E");
+		HtmlPage summaryPage = summaryButton.click();
+
+		// validate that there is no <script> span in result summary page
+		Optional<HtmlElement> scriptSpans = summaryPage.getHtmlElementById("requestUrlAnchor")
+			.getElementsByTagName("span")
+			.stream()
+			.filter(span -> span.asXml().contains("<script>"))
+			.findAny();
+
+		assertTrue(scriptSpans.isEmpty());
+	}
+
 	@Test
 	public void testInvokeCustomOperation_Validate() throws IOException {
 		register5Patients();