Bind caller-supplied names as query parameters instead of concatenating (#6335)

* Bind the program-attribute name as a query parameter

getPatientProgramAttributeByAttributeName assembled its native query by
concatenating the attributeName argument straight into the SQL text. Bind
it as a named parameter (:attributeName) instead so the value is always
handled as a literal attribute-type name and can never alter the query.
The patient-id list stays inlined since those elements are ints.

Adds a regression test that a name containing SQL control characters is
matched literally and resolves no attribute type.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TpKNfxm3QjwUBDePEtFm2H

* Bind the global-property name in FilterUtil.getGlobalPropertyValue

Replace the concatenated global_property lookup with a PreparedStatement
so the property name is always treated as a literal value rather than
being spliced into the SQL text.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* Add regression test for FilterUtil.getGlobalPropertyValue name binding

Guards that the property name is matched literally: a name carrying SQL
control characters resolves no property (returns null) instead of being
interpreted as query text, which the earlier concatenated query allowed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: dkayiwa

api/src/main/java/org/openmrs/api/db/hibernate/HibernateProgramWorkflowDAO.java

@@ -475,12 +475,12 @@ public Map<Object, Object> getPatientProgramAttributeByAttributeName(List<Intege
 		        + " from patient p " + " join patient_program pp on p.patient_id = pp.patient_id and p.patient_id in ("
 		        + commaSeperatedPatientIds + ")"
 		        + " join patient_program_attribute ppa on pp.patient_program_id = ppa.patient_program_id and ppa.voided=0"
-		        + " join program_attribute_type ppt on ppa.attribute_type_id = ppt.program_attribute_type_id and ppt.name ='"
-		        + attributeName + "' "
+		        + " join program_attribute_type ppt on ppa.attribute_type_id = ppt.program_attribute_type_id and ppt.name = :attributeName "
 		        + " LEFT OUTER JOIN concept_name cn on ppa.value_reference = cn.concept_id and cn.concept_name_type= 'FULLY_SPECIFIED' and cn.voided=0 and ppt.datatype like '%ConceptDataType%'"
 		        + " group by p.patient_id",
 		    Object.class).addScalar("person_id", StandardBasicTypes.INTEGER)
-		        .addScalar("patientProgramAttributeValue", StandardBasicTypes.STRING).list();
+		        .addScalar("patientProgramAttributeValue", StandardBasicTypes.STRING)
+		        .setParameter("attributeName", attributeName).list();
 
 		for (Object o : list) {
 			Object[] arr = (Object[]) o;

api/src/test/java/org/openmrs/api/ProgramWorkflowServiceTest.java

@@ -15,6 +15,7 @@
 import java.util.Date;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Map;
 import java.util.Set;
 
 import org.junit.jupiter.api.BeforeEach;
@@ -26,6 +27,7 @@
 import org.openmrs.Encounter;
 import org.openmrs.Patient;
 import org.openmrs.PatientProgram;
+import org.openmrs.PatientProgramAttribute;
 import org.openmrs.PatientState;
 import org.openmrs.Program;
 import org.openmrs.ProgramAttributeType;
@@ -1117,4 +1119,40 @@ public void triggerStateConversion_shouldTestTransitionToState() {
 	//    	return props;
 	//    }
 
+	/**
+	 * Regression guard for the program-attribute lookup: the attribute name is bound as a query
+	 * parameter rather than concatenated into the native SQL, so a value carrying SQL control
+	 * characters is matched literally instead of altering the query.
+	 *
+	 * @see ProgramWorkflowService#getPatientProgramAttributeByAttributeName(List, String)
+	 */
+	@Test
+	public void getPatientProgramAttributeByAttributeName_shouldMatchTheAttributeNameLiterally() {
+		// attach a "ProgramId" attribute value to the existing enrollment for patient 2
+		ProgramAttributeType attributeType = pws.getProgramAttributeType(1);
+		assertEquals("ProgramId", attributeType.getName());
+
+		PatientProgramAttribute attribute = new PatientProgramAttribute();
+		attribute.setAttributeType(attributeType);
+		attribute.setValueReferenceInternal("1234");
+
+		PatientProgram patientProgram = pws.getPatientProgram(1);
+		patientProgram.addAttribute(attribute);
+		pws.savePatientProgram(patientProgram);
+		Context.flushSession();
+
+		List<Integer> patientIds = Arrays.asList(2);
+
+		// the real name resolves the stored value
+		Map<Object, Object> byName = pws.getPatientProgramAttributeByAttributeName(patientIds, "ProgramId");
+		assertTrue(byName.containsKey(2));
+		assertTrue(((String) byName.get(2)).contains("1234"));
+
+		// a name containing SQL quote characters is compared literally, so it resolves no attribute
+		// type and returns nothing, rather than being interpreted as part of the query
+		Map<Object, Object> byNameWithQuotes = pws.getPatientProgramAttributeByAttributeName(patientIds,
+		    "ProgramId' OR '1'='1");
+		assertTrue(byNameWithQuotes.isEmpty());
+	}
+
 }

web/src/main/java/org/openmrs/web/filter/util/FilterUtil.java

@@ -13,11 +13,9 @@
 import java.sql.PreparedStatement;
 import java.sql.ResultSet;
 import java.sql.SQLException;
-import java.util.List;
 
 import org.apache.commons.lang3.StringUtils;
 import org.openmrs.util.DatabaseUpdater;
-import org.openmrs.util.DatabaseUtil;
 import org.openmrs.util.OpenmrsConstants;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -268,10 +266,14 @@ public static String getGlobalPropertyValue(String globalPropertyName) {
 
 		try {
 			connection = DatabaseUpdater.getConnection();
-			List<List<Object>> results = DatabaseUtil.executeSQL(connection,
-			    "select property_value from global_property where property = '" + globalPropertyName + "'", true);
-			if (results.size() == 1 && results.get(0).size() == 1) {
-				propertyValue = results.get(0).get(0).toString();
+			try (PreparedStatement statement = connection
+			        .prepareStatement("select property_value from global_property where property = ?")) {
+				statement.setString(1, globalPropertyName);
+				try (ResultSet resultSet = statement.executeQuery()) {
+					if (resultSet.next()) {
+						propertyValue = resultSet.getString(1);
+					}
+				}
 			}
 		} catch (Exception e) {
 			log.error("Error while retrieving value for global property:" + globalPropertyName, e);

web/src/test/java/org/openmrs/web/filter/update/util/FilterUtilTest.java

@@ -16,6 +16,7 @@
 import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
 
 /**
  * Tests some of the methods on the {@link FilterUtil}
@@ -61,4 +62,18 @@ public void storeLocale_shouldNotUpdateStoredValueWhenStoringDifferentLocale() {
 		assertNotEquals(UPDATED_LOCALE, FilterUtil.restoreLocale(FilterUtil.ADMIN_USERNAME));
 	}
 
+	/**
+	 * @see FilterUtil#getGlobalPropertyValue(String)
+	 */
+	@Test
+	public void getGlobalPropertyValue_shouldMatchThePropertyNameLiterally() {
+		// a real property name resolves its stored value
+		assertEquals("7", FilterUtil.getGlobalPropertyValue("concept.true"));
+
+		// a name carrying SQL control characters is bound as a literal, so it matches no property
+		// and returns null. On the earlier string-concatenated query this payload resolved a single
+		// row and returned "7", so this asserts the value is now treated as data, not query text.
+		assertNull(FilterUtil.getGlobalPropertyValue("nonexistent' OR property = 'concept.true"));
+	}
+
 }