fix: XSS vulnerability in autocomplete endpoints #280
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,10 +27,8 @@ | |
| * CARLOS has no affiliation with OSCAR or McMaster University. | ||
| */ | ||
|
|
||
| package io.github.carlos_emr.carlos.commn.web; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| import javax.servlet.http.HttpServletRequest; | ||
|
|
@@ -40,8 +38,10 @@ | |
| import com.fasterxml.jackson.databind.node.ObjectNode; | ||
|
|
||
| import io.github.carlos_emr.carlos.utility.SpringUtils; | ||
| import io.github.carlos_emr.carlos.utility.LoggedInInfo; | ||
| import io.github.carlos_emr.carlos.managers.SecurityInfoManager; | ||
|
|
||
| import org.owasp.encoder.Encode; | ||
| import io.github.carlos_emr.carlos.commn.dao.ProviderDataDao; | ||
|
|
||
| /** | ||
|
|
@@ -50,14 +50,22 @@ | |
| import com.opensymphony.xwork2.ActionSupport; | ||
| import org.apache.struts2.ServletActionContext; | ||
|
|
||
| @SuppressWarnings("deprecation") | ||
| public class SearchProviderAutoComplete2Action extends ActionSupport { | ||
| HttpServletRequest request = ServletActionContext.getRequest(); | ||
| HttpServletResponse response = ServletActionContext.getResponse(); | ||
|
|
||
| private static final ObjectMapper objectMapper = new ObjectMapper(); | ||
| private SecurityInfoManager securityInfoManager = SpringUtils.getBean(SecurityInfoManager.class); | ||
| private ProviderDataDao providerDataDao = SpringUtils.getBean(ProviderDataDao.class); | ||
|
|
||
| public String execute() throws Exception { | ||
| LoggedInInfo loggedInInfo = LoggedInInfo.getLoggedInInfoFromSession(request); | ||
|
|
||
| if (!securityInfoManager.hasPrivilege(loggedInInfo, "_provider", SecurityInfoManager.READ, null)) { | ||
| throw new RuntimeException("missing required security object (_provider)"); | ||
| } | ||
|
|
||
| if ("labSearch".equals(request.getParameter("method"))) { | ||
| return labSearch(); | ||
|
Comment on lines
62
to
70
Contributor
There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check Struts2 DMI configuration
echo "=== Checking for Struts2 DMI configuration ==="
rg -n 'struts.enable.DynamicMethodInvocation' --type=properties --type=xml --type=conf -C 2
echo -e "\n=== Checking for struts2 XML config files ==="
fd -e "xml" | grep -i struts | head -20
echo -e "\n=== Looking for SearchProviderAutoComplete configuration ==="
rg -n 'SearchProviderAutoComplete' --type=xml -C 5Repository: carlos-emr/carlos Length of output: 6541 🏁 Script executed: #!/bin/bash
# Read the full SearchProviderAutoComplete2Action.java file
echo "=== Full SearchProviderAutoComplete2Action.java content ==="
fd -name "SearchProviderAutoComplete2Action.java" -type fRepository: carlos-emr/carlos Length of output: 368 🏁 Script executed: #!/bin/bash
# Find the file and read it
find . -name "SearchProviderAutoComplete2Action.java" -type f 2>/dev/null | head -1 | xargs wc -lRepository: carlos-emr/carlos Length of output: 157 🏁 Script executed: #!/bin/bash
# Read the full SearchProviderAutoComplete2Action.java file
cat -n ./src/main/java/io/github/carlos_emr/carlos/commn/web/SearchProviderAutoComplete2Action.javaRepository: carlos-emr/carlos Length of output: 6796 🏁 Script executed: #!/bin/bash
# Check for DMI configuration in properties files and constants
echo "=== Checking for DMI in struts configuration ==="
rg -n 'DynamicMethodInvocation' --type=java --type=xml
echo ""
echo "=== Checking struts.properties or application.properties ==="
fd "\.properties$" | xargs rg -n 'struts.enable' 2>/dev/null || echo "No struts.enable properties found"
echo ""
echo "=== Checking for default struts configuration ==="
rg -n 'struts\.' src/main/resources/ 2>/dev/null | head -20 || echo "No src/main/resources found"Repository: carlos-emr/carlos Length of output: 344 🏁 Script executed: #!/bin/bash
# Check if there's a struts properties file or Spring XML configuration
find . -name "*.properties" -o -name "*.xml" | grep -E "(struts|spring)" | grep -v "test" | grep -v "TrackingRegexp"Repository: carlos-emr/carlos Length of output: 303 Make The - public String labSearch() throws Exception {
+ private String labSearch() throws Exception {🤖 Prompt for AI Agents
Collaborator
There was a problem hiding this comment. @claude, this still requires authentication checks and the nature of this platform does largely treat authentication as circle of care, but this is something to look at further in general as the code base matures. Does this allow for bypass of authentication check? I don't think so? Please make a issue ticket for this for follow up.
Contributor
There was a problem hiding this comment.
There was a problem hiding this comment. Ask❓@claude, this still requires authentication checks and the nature of this platform does largely treat authentication as circle of care, but this is something to look at further in general as the code base matures. Does this allow for bypass of authentication check? I don't think so? Please make a issue ticket for this for follow up. Answer:Does this allow a bypass of the authentication/authorization check? Why it could be bypassableIn if (!securityInfoManager.hasPrivilege(loggedInInfo, "_provider", SecurityInfoManager.READ, null)) {
throw new RuntimeException("missing required security object (_provider)");
}However, If DMI is disabled (common in hardened setups), then this direct invocation path generally won’t work, and you’re right that it likely does not bypass authz in practice. But because this is security-sensitive and configuration can drift over time (or differ across deployments), it’s still worth addressing as defense-in-depth. Recommended follow-up (low-risk hardening)Any one of these would close the potential gap:
Option (1) is simplest and aligns with CodeRabbit’s suggestion, provided there are no external callers (and since Struts uses reflection, “external caller” here includes the framework via DMI). Follow-up issue ticket to createTitle:
|
||
| } | ||
|
|
@@ -69,8 +77,41 @@ public String execute() throws Exception { | |
| searchStr = request.getParameter("name"); | ||
| } | ||
|
|
||
| // Handle null or empty search string | ||
| if (searchStr == null || searchStr.trim().isEmpty()) { | ||
| response.setContentType("text/x-json"); | ||
| response.getWriter().write("{\"results\":[]}"); | ||
| return null; | ||
| } | ||
|
|
||
| // Parse search string for firstName and lastName (replaces deprecated | ||
| // ProviderData.searchProvider) | ||
| String firstName = null; | ||
| String lastName; | ||
| if (searchStr.indexOf(",") != -1) { | ||
| String[] array = searchStr.split(",", -1); | ||
| lastName = array[0].trim(); | ||
| firstName = array.length > 1 ? array[1].trim() : ""; | ||
| } else { | ||
| lastName = searchStr.trim(); | ||
| } | ||
|
|
||
| // Use the DAO directly with the newer model class | ||
| List<io.github.carlos_emr.carlos.commn.model.ProviderData> providers = providerDataDao.findByName(firstName, | ||
| lastName, true); | ||
|
|
||
| // Convert to the expected map format for JSON response | ||
| List<java.util.Map<String, String>> provList = new java.util.ArrayList<>(); | ||
| for (io.github.carlos_emr.carlos.commn.model.ProviderData p : providers) { | ||
| java.util.Map<String, String> result = new java.util.HashMap<>(); | ||
| result.put("providerNo", p.getId()); | ||
| result.put("firstName", p.getFirstName() != null ? Encode.forHtml(p.getFirstName()) : ""); | ||
| result.put("lastName", p.getLastName() != null ? Encode.forHtml(p.getLastName()) : ""); | ||
| result.put("ohipNo", p.getOhipNo() != null ? Encode.forHtml(p.getOhipNo()) : ""); | ||
| provList.add(result); | ||
| } | ||
|
|
||
| java.util.HashMap<String, Object> d = new java.util.HashMap<>(); | ||
| d.put("results", provList); | ||
|
|
||
| response.setContentType("text/x-json"); | ||
|
|
@@ -83,12 +124,22 @@ public String execute() throws Exception { | |
| public String labSearch() throws Exception { | ||
|
|
||
| String searchStr = request.getParameter("term"); | ||
|
|
||
| // Handle null or empty search string | ||
| if (searchStr == null || searchStr.trim().isEmpty()) { | ||
| response.setContentType("text/x-json"); | ||
| response.getWriter().write("[]"); | ||
| return null; | ||
| } | ||
|
|
||
| String firstName, lastName; | ||
|
|
||
| if (searchStr.indexOf(",") != -1) { | ||
| String[] searchParams = searchStr.split(",", -1); | ||
| // note - the -1 is added because split discards a last empty string by default, | ||
| // so "smith,".split(",") returns ["smith"], not ["smith",""]. | ||
| // adding the -1 causes split to return the 2 element array in this situation, | ||
| // to avoid an index out of bounds error when setting the firstName | ||
| lastName = searchParams[0].trim(); | ||
| firstName = searchParams[1].trim(); | ||
| } else { | ||
|
|
@@ -97,21 +148,21 @@ public String labSearch() throws Exception { | |
| } | ||
|
|
||
| ProviderDataDao providerDataDao = SpringUtils.getBean(ProviderDataDao.class); | ||
| List<io.github.carlos_emr.carlos.commn.model.ProviderData> provList = providerDataDao.findByName(firstName, | ||
| lastName, true); | ||
| List<java.util.LinkedHashMap<String, String>> searchResults = new java.util.ArrayList<>(); | ||
|
|
||
| for (io.github.carlos_emr.carlos.commn.model.ProviderData provData : provList) { | ||
| java.util.LinkedHashMap<String, String> node = new java.util.LinkedHashMap<>(); | ||
| String provLastName = provData.getLastName() != null ? Encode.forHtml(provData.getLastName()) : ""; | ||
| String provFirstName = provData.getFirstName() != null ? Encode.forHtml(provData.getFirstName()) : ""; | ||
| node.put("label", provLastName + ", " + provFirstName); | ||
| node.put("value", provData.getId()); | ||
| searchResults.add(node); | ||
| } | ||
|
|
||
| response.setContentType("text/x-json"); | ||
| response.getWriter().write(objectMapper.writeValueAsString(searchResults)); | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.