From cc70b7b187529d1abd67f0da6addf625ada39541 Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 11:59:08 -0400 Subject: [PATCH 01/21] fix(report): safely enable query by example Signed-off-by: Ben Heerema --- dependencies-lock.json | 24 +++ pom.xml | 16 ++ .../data/QueryByExampleSqlValidator.java | 152 ++++++++++++++++++ .../QueryByExampleValidationException.java | 35 ++++ .../carlos/report/data/RptByExampleData.java | 105 +++++++----- .../carlos/report/data/RptResultStruct.java | 11 +- .../report/pageUtil/RptByExample2Action.java | 66 +++++--- src/main/resources/carlos.properties | 3 + .../resources/oscarResources_en.properties | 5 + .../resources/oscarResources_es.properties | 5 + .../resources/oscarResources_fr.properties | 5 + .../resources/oscarResources_pl.properties | 5 + .../resources/oscarResources_pt_BR.properties | 5 + .../WEB-INF/jsp/oscarReport/RptByExample.jsp | 34 +++- ...ReportActionSecurityMigrationUnitTest.java | 54 +++++-- .../data/QueryByExampleSqlValidatorTest.java | 94 +++++++++++ .../report/data/RptByExampleDataTest.java | 106 ++++++++++++ 17 files changed, 645 insertions(+), 80 deletions(-) create mode 100644 src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java create mode 100644 src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleValidationException.java create mode 100644 src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java create mode 100644 src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java diff --git a/dependencies-lock.json b/dependencies-lock.json index 428e37b5300..c7829800ce7 100644 --- a/dependencies-lock.json +++ b/dependencies-lock.json @@ -255,6 +255,14 @@ "type" : "jar", "optional" : false, "integrity" : "sha512:ak529k22lHqP1DCq+VRYn/k+WKRVFbfX2LPZlrBBSh6rRgD6olc8zroC3xwa2UkhjFSvQFFEUa7y3MbjkY24sQ==" + }, { + "groupId" : "com.github.jsqlparser", + "artifactId" : "jsqlparser", + "version" : "5.3", + "scope" : "compile", + "type" : "jar", + "optional" : false, + "integrity" : "sha512:YLJ1qDvLVOE0hRuDwPgQLT4mPEZjZnlObat+LOk5vNLw0dEZOkWdT+c6wYgSvyXpRPw7+iO2YK0XgVcYWESegA==" }, { "groupId" : "com.github.librepdf.openpdf", "artifactId" : "openpdf-html", @@ -1186,6 +1194,14 @@ "type" : "jar", "optional" : false, "integrity" : "sha512:6oPQclFmUY/UxgXW0HXw/IcSjKRVy9/6wIVOBVKOeZpPu7OAUHGLCgbpQmwujcd/qVlLZXCDu1HHErGF1OiacQ==" + }, { + "groupId" : "net.sf.jopt-simple", + "artifactId" : "jopt-simple", + "version" : "5.0.4", + "scope" : "compile", + "type" : "jar", + "optional" : false, + "integrity" : "sha512:y8J+C22mrktiRTU9ZibS48FxwwJqVV+iHo72GzBxTihtuFCG0aV8FnAW6Kfwe+KiQ+NLOrUEsYd4BvO87F35hg==" }, { "groupId" : "net.sf.saxon", "artifactId" : "Saxon-HE", @@ -3171,6 +3187,14 @@ "type" : "jar", "optional" : false, "integrity" : "sha512:H6mQ0VvRefB/+8Rg1YCm/QVi5F3ui9SpQFkXU2t49FwNb2RLZ/hdeBx1iqVu/5Cu8j7tzJvX9f+Ieme3Fgg+YQ==" + }, { + "groupId" : "org.openjdk.jmh", + "artifactId" : "jmh-core", + "version" : "1.37", + "scope" : "compile", + "type" : "jar", + "optional" : false, + "integrity" : "sha512:jkNMyJ98ijA14XJnXZ+RRBUDmtjcQDqfSjBu++MknCDaA0OqUev547n4umdG6FRKxWH7zWLynbsWO38QyWwfNA==" }, { "groupId" : "org.opensaml", "artifactId" : "opensaml-core-api", diff --git a/pom.xml b/pom.xml index 4c9216803bd..b8c64db6973 100644 --- a/pom.xml +++ b/pom.xml @@ -129,6 +129,8 @@ 1.4.0 4.5.0-jakarta + + 5.3 5.19.0 @@ -445,6 +447,13 @@ commons-csv 1.14.1 + + + + com.github.jsqlparser + jsqlparser + ${jsqlparser.version} + @@ -1446,6 +1455,13 @@ org.jacoco jacoco-maven-plugin 0.8.14 + + + + net.sf.jsqlparser.* + + diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java new file mode 100644 index 00000000000..ff48b9cea3a --- /dev/null +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java @@ -0,0 +1,152 @@ +/** + * Copyright (c) 2026 CARLOS Contributors. All Rights Reserved. + * + * This software is published under the GPL GNU General Public License. + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + * + * CARLOS EMR Project + * https://github.com/carlos-emr/carlos + */ +package io.github.carlos_emr.carlos.report.data; + +import java.sql.SQLException; +import java.util.Locale; +import java.util.Properties; +import java.util.Set; +import java.util.regex.Pattern; + +import io.github.carlos_emr.carlos.db.LegacyJdbcQuery; +import net.sf.jsqlparser.JSQLParserException; +import net.sf.jsqlparser.parser.CCJSqlParserUtil; +import net.sf.jsqlparser.statement.Statement; +import net.sf.jsqlparser.statement.select.Select; +import net.sf.jsqlparser.statement.select.SetOperationList; +import net.sf.jsqlparser.util.TablesNamesFinder; + +/** Fail-closed validation for request-submitted Query-by-Example SQL. */ +public final class QueryByExampleSqlValidator { + private static final Pattern LOCKING_SELECT = Pattern.compile( + "\\bfor\\s+(?:update|share)\\b|\\block\\s+in\\s+share\\s+mode\\b", + Pattern.CASE_INSENSITIVE); + private static final Pattern OUTPUT_OPERATION = Pattern.compile("\\binto\\b", Pattern.CASE_INSENSITIVE); + private static final Set BLOCKED_FUNCTIONS = Set.of( + "sleep", "benchmark", "get_lock", "release_lock", "is_free_lock", "load_file"); + + private QueryByExampleSqlValidator() { + } + + public static LegacyJdbcQuery.TrustedSql validate(String sql, Properties properties) + throws QueryByExampleValidationException { + LegacyJdbcQuery.TrustedSql trustedSql; + try { + trustedSql = LegacyJdbcQuery.trustedSelectSql(sql); + } catch (SQLException e) { + throw new QueryByExampleValidationException(e.getMessage(), e); + } + + Statement statement; + try { + statement = CCJSqlParserUtil.parse(sql); + } catch (JSQLParserException | RuntimeException e) { + throw new QueryByExampleValidationException("The query could not be parsed as a single SELECT", e); + } + + if (!(statement instanceof Select) || statement instanceof SetOperationList) { + throw new QueryByExampleValidationException("Only one SELECT statement is allowed"); + } + + String sqlWithoutStringLiterals = stripStringLiterals(sql); + if (LOCKING_SELECT.matcher(sqlWithoutStringLiterals).find()) { + throw new QueryByExampleValidationException("Locking SELECT statements are not allowed"); + } + if (OUTPUT_OPERATION.matcher(sqlWithoutStringLiterals).find()) { + throw new QueryByExampleValidationException("SELECT output operations are not allowed"); + } + rejectBlockedFunctions(sqlWithoutStringLiterals); + rejectOtherSchemas(statement, applicationSchema(properties)); + return trustedSql; + } + + static String applicationSchema(Properties properties) throws QueryByExampleValidationException { + String configuredName = properties == null ? null : properties.getProperty("db_name"); + if (configuredName == null || configuredName.isBlank()) { + throw new QueryByExampleValidationException("The application database schema is not configured"); + } + String schema = configuredName.split("\\?", 2)[0].trim(); + if (schema.isEmpty()) { + throw new QueryByExampleValidationException("The application database schema is not configured"); + } + return unquoteIdentifier(schema); + } + + private static void rejectOtherSchemas(Statement statement, String applicationSchema) + throws QueryByExampleValidationException { + Set tables = new TablesNamesFinder().getTables(statement); + for (String table : tables) { + String normalizedTable = unquoteIdentifier(table); + int lastDot = normalizedTable.lastIndexOf('.'); + if (lastDot > 0) { + String qualifier = normalizedTable.substring(0, lastDot); + if (!qualifier.equalsIgnoreCase(applicationSchema)) { + throw new QueryByExampleValidationException("Queries may only read the application database schema"); + } + } + } + } + + private static void rejectBlockedFunctions(String sql) throws QueryByExampleValidationException { + String normalized = sql.toLowerCase(Locale.ROOT); + for (String function : BLOCKED_FUNCTIONS) { + Pattern invocation = Pattern.compile("(?"); } + int rowCount = 0; while (rs.next()) { + rowCount++; sb.append(""); for (int j = 0; j < columns; j++) { sb.append(""); @@ -89,7 +98,7 @@ public static String getStructure(ResultSet rs) throws SQLException { sb.append(""); } sb.append(""); - return sb.toString(); + return new StructuredResult(sb.toString(), rowCount); } //improvement over getStructure() - changed CSS naming conventions, added enterspaces for cleaner html, diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java index 6785d8b46d3..4ff9dc61ad4 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java @@ -31,9 +31,10 @@ package io.github.carlos_emr.carlos.report.pageUtil; import java.io.IOException; +import java.sql.SQLException; +import java.sql.SQLTimeoutException; import java.util.Collection; import java.util.Date; -import java.util.List; import java.util.Properties; import jakarta.servlet.ServletException; @@ -41,9 +42,8 @@ import jakarta.servlet.http.HttpServletResponse; import io.github.carlos_emr.carlos.report.data.RptByExampleData; +import io.github.carlos_emr.carlos.report.data.QueryByExampleValidationException; import io.github.carlos_emr.carlos.managers.SecurityInfoManager; -import io.github.carlos_emr.carlos.PMmodule.dao.SecUserRoleDao; -import io.github.carlos_emr.carlos.PMmodule.model.SecUserRole; import io.github.carlos_emr.carlos.commn.dao.ReportByExamplesDao; import io.github.carlos_emr.carlos.commn.model.ReportByExamples; import io.github.carlos_emr.carlos.utility.LoggedInInfo; @@ -59,12 +59,14 @@ import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; /** - * Struts2 action for the Query-by-Example report tool. Allows admin users to execute - * custom SQL queries, persist them as recent searches, and display results. + * Struts2 action for the Query-by-Example report tool. Allows authorized report users + * to execute custom read-only SQL queries, persist successful searches, and display results. * * @since 2003-07-22 */ public class RptByExample2Action extends ActionSupport { + public static final String ENABLED_PROPERTY = "QUERY_BY_EXAMPLE_ENABLED"; + HttpServletRequest request = ServletActionContext.getRequest(); HttpServletResponse response = ServletActionContext.getResponse(); @@ -96,34 +98,54 @@ public String execute() String providerNo = loggedInInfo.getLoggedInProviderNo(); - SecUserRoleDao secUserRoleDao = SpringUtils.getBean(SecUserRoleDao.class); - - List userRoles = secUserRoleDao.findByRoleNameAndProviderNo("admin", providerNo); - if (userRoles.isEmpty()) { - throw new SecurityException("missing required admin privileges to run query by example"); - } - RptByExampleQueryBeanHandler hd = new RptByExampleQueryBeanHandler(); Collection favorites = hd.getFavoriteCollection(providerNo); request.setAttribute("favorites", favorites); - if (sql != null) { - write2Database(sql, providerNo); - } else - sql = ""; + sql = sql == null ? "" : sql; + request.setAttribute("submittedSql", sql); - RptByExampleData exampleData = new RptByExampleData(); - Properties proppies = CarlosProperties.getInstance(); + if (!"POST".equalsIgnoreCase(request.getMethod())) { + return SUCCESS; + } - String results = exampleData.exampleReportGenerate(sql, proppies) == null ? null : exampleData.exampleReportGenerate(sql, proppies); - String resultText = exampleData.exampleTextGenerate(sql, proppies) == null ? null : exampleData.exampleTextGenerate(sql, proppies); + Properties properties = CarlosProperties.getInstance(); + if (!isEnabled(properties)) { + request.setAttribute("queryDisabled", true); + RptByExampleData.audit(providerNo, sql, 0, 0, "disabled"); + return SUCCESS; + } - request.setAttribute("results", results); - request.setAttribute("resultText", resultText); + if (sql.isBlank()) { + request.setAttribute("queryValidationError", true); + RptByExampleData.audit(providerNo, sql, 0, 0, "rejected"); + return SUCCESS; + } + + try { + RptByExampleData.QueryResult result = new RptByExampleData().execute(sql, properties, providerNo); + request.setAttribute("results", result.html()); + request.setAttribute("resultRowCount", result.rowCount()); + write2Database(sql, providerNo); + } catch (QueryByExampleValidationException e) { + request.setAttribute("queryValidationError", true); + } catch (SQLTimeoutException e) { + request.setAttribute("queryTimeout", true); + } catch (SQLException | RuntimeException e) { + request.setAttribute("queryExecutionError", true); + } return SUCCESS; } + static boolean isEnabled(Properties properties) { + String configured = properties.getProperty(ENABLED_PROPERTY); + return configured == null || configured.isBlank() + || configured.equalsIgnoreCase("true") + || configured.equalsIgnoreCase("yes") + || configured.equalsIgnoreCase("on"); + } + public void write2Database(String query, String providerNo) { if (query != null && query.compareTo("") != 0) { ReportByExamples r = new ReportByExamples(); diff --git a/src/main/resources/carlos.properties b/src/main/resources/carlos.properties index d4c28812ae3..2b475ecbcc0 100644 --- a/src/main/resources/carlos.properties +++ b/src/main/resources/carlos.properties @@ -50,6 +50,9 @@ buildVersion=${build.JOB_NAME} ${build.BUILD_NUMBER} # legacy functionality. Ensure that you leave the tags behind the field when renaming the database. db_name = oscar_mcmaster?zeroDateTimeBehavior=round&useOldAliasMetadataBehavior=true&jdbcCompliantTruncation=false +# Authorized Query-by-Example users may run validated, read-only SELECT queries. +QUERY_BY_EXAMPLE_ENABLED = true + # username db_username = root diff --git a/src/main/resources/oscarResources_en.properties b/src/main/resources/oscarResources_en.properties index d7494d7448b..6a7ecb590dc 100644 --- a/src/main/resources/oscarResources_en.properties +++ b/src/main/resources/oscarResources_en.properties @@ -8923,6 +8923,11 @@ oscarReport.RptByExample.MsgViewQueryHistory=View Query History oscarReport.RptByExample.MsgLoadQuery=Load Query oscarReport.RptByExample.MsgRunQuery=Run Query +oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disabled. Your query has not been run. +oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. +oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. +oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. +oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. oscarReport.RptByExample.MsgConfirmDelete=Are you sure you want to delete the selected query? diff --git a/src/main/resources/oscarResources_es.properties b/src/main/resources/oscarResources_es.properties index 35b70a92161..ea784b60dfc 100644 --- a/src/main/resources/oscarResources_es.properties +++ b/src/main/resources/oscarResources_es.properties @@ -6446,6 +6446,11 @@ oscarReport.RptByExample.MsgViewQueryHistory=Ver historial de consultas oscarReport.RptByExample.MsgLoadQuery=Cargar consulta oscarReport.RptByExample.MsgRunQuery=Ejecutar +oscarReport.RptByExample.MsgDisabled=La ejecuci\u00f3n directa de consultas est\u00e1 desactivada. Su consulta no se ha ejecutado. +oscarReport.RptByExample.MsgValidationError=Solo se permite una consulta SELECT de solo lectura en la base de datos de la aplicaci\u00f3n. No se permiten comentarios, UNION, bloqueos, operaciones de salida ni funciones prohibidas. +oscarReport.RptByExample.MsgTimeout=La consulta super\u00f3 el l\u00edmite de 15 segundos y se detuvo. +oscarReport.RptByExample.MsgExecutionError=No se pudo completar la consulta. Rev\u00edsela e int\u00e9ntelo de nuevo. +oscarReport.RptByExample.MsgResultLimit=Los resultados est\u00e1n limitados a {0} filas. oscarReport.RptByExample.MsgConfirmDelete=\u00bfEst\u00e1 seguro de que desea eliminar la consulta seleccionada? diff --git a/src/main/resources/oscarResources_fr.properties b/src/main/resources/oscarResources_fr.properties index 1a01c467cee..acf4a4f5a8a 100644 --- a/src/main/resources/oscarResources_fr.properties +++ b/src/main/resources/oscarResources_fr.properties @@ -5628,6 +5628,11 @@ oscarReport.RptByExample.MsgRefresh=Actualiser oscarReport.RptByExample.MsgViewQueryHistory=Voir l\u2019historique des requ\u00eates oscarReport.RptByExample.MsgLoadQuery=Charger la requ\u00eate oscarReport.RptByExample.MsgRunQuery=Ex\u00e9cuter +oscarReport.RptByExample.MsgDisabled=L\u2019ex\u00e9cution directe des requ\u00eates est actuellement d\u00e9sactiv\u00e9e. Votre requ\u00eate n\u2019a pas \u00e9t\u00e9 ex\u00e9cut\u00e9e. +oscarReport.RptByExample.MsgValidationError=Une seule requ\u00eate SELECT en lecture seule dans la base de donn\u00e9es de l\u2019application est autoris\u00e9e. Les commentaires, UNION, verrouillages, op\u00e9rations de sortie et fonctions interdites ne sont pas permis. +oscarReport.RptByExample.MsgTimeout=La requ\u00eate a d\u00e9pass\u00e9 la limite de 15 secondes et a \u00e9t\u00e9 arr\u00eat\u00e9e. +oscarReport.RptByExample.MsgExecutionError=La requ\u00eate n\u2019a pas pu \u00eatre ex\u00e9cut\u00e9e. V\u00e9rifiez-la et r\u00e9essayez. +oscarReport.RptByExample.MsgResultLimit=Les r\u00e9sultats sont limit\u00e9s \u00e0 {0} lignes. oscarReport.RptByExample.MsgConfirmDelete=\u00cates-vous s\u00fbr de vouloir supprimer la requ\u00eate s\u00e9lectionn\u00e9e\u00a0? diff --git a/src/main/resources/oscarResources_pl.properties b/src/main/resources/oscarResources_pl.properties index 8e4b767be72..027b71887ba 100644 --- a/src/main/resources/oscarResources_pl.properties +++ b/src/main/resources/oscarResources_pl.properties @@ -5864,6 +5864,11 @@ oscarReport.RptByExample.MsgViewQueryHistory=Historia zapyta\u0144 oscarReport.RptByExample.MsgLoadQuery=Za\u0142aduj zapytanie oscarReport.RptByExample.MsgRunQuery=Wykonaj +oscarReport.RptByExample.MsgDisabled=Bezpo\u015brednie wykonywanie zapyta\u0144 jest obecnie wy\u0142\u0105czone. Zapytanie nie zosta\u0142o wykonane. +oscarReport.RptByExample.MsgValidationError=Dozwolone jest tylko jedno zapytanie SELECT w trybie tylko do odczytu w bazie aplikacji. Komentarze, UNION, blokady, operacje wyj\u015bciowe i zabronione funkcje nie s\u0105 dozwolone. +oscarReport.RptByExample.MsgTimeout=Zapytanie przekroczy\u0142o limit 15 sekund i zosta\u0142o zatrzymane. +oscarReport.RptByExample.MsgExecutionError=Nie uda\u0142o si\u0119 wykona\u0107 zapytania. Sprawd\u017a je i spr\u00f3buj ponownie. +oscarReport.RptByExample.MsgResultLimit=Wyniki s\u0105 ograniczone do {0} wierszy. oscarReport.RptByExample.MsgConfirmDelete=Czy na pewno chcesz usun\u0105\u0107 wybran\u0105 kwerend\u0119? diff --git a/src/main/resources/oscarResources_pt_BR.properties b/src/main/resources/oscarResources_pt_BR.properties index f48663bb5d6..a58f770bebe 100644 --- a/src/main/resources/oscarResources_pt_BR.properties +++ b/src/main/resources/oscarResources_pt_BR.properties @@ -7351,6 +7351,11 @@ oscarReport.RptByExample.MsgViewQueryHistory=Ver hist\u00f3rico de consultas oscarReport.RptByExample.MsgLoadQuery=Carregar consulta oscarReport.RptByExample.MsgRunQuery=Executar +oscarReport.RptByExample.MsgDisabled=A execu\u00e7\u00e3o direta de consultas est\u00e1 desativada. Sua consulta n\u00e3o foi executada. +oscarReport.RptByExample.MsgValidationError=Apenas uma consulta SELECT somente leitura no banco de dados do aplicativo \u00e9 permitida. Coment\u00e1rios, UNION, bloqueios, opera\u00e7\u00f5es de sa\u00edda e fun\u00e7\u00f5es proibidas n\u00e3o s\u00e3o permitidos. +oscarReport.RptByExample.MsgTimeout=A consulta excedeu o limite de 15 segundos e foi interrompida. +oscarReport.RptByExample.MsgExecutionError=N\u00e3o foi poss\u00edvel concluir a consulta. Verifique-a e tente novamente. +oscarReport.RptByExample.MsgResultLimit=Os resultados est\u00e3o limitados a {0} linhas. oscarReport.RptByExample.MsgConfirmDelete=Tem certeza de que deseja excluir a consulta selecionada? diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp index ae034eb4eb4..9b2b53248d2 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp @@ -49,7 +49,7 @@ - results — HTML string of query results (rendered unescaped; backend-generated) Security: - - Requires _report or _admin.reporting read privilege + - Requires _report or _admin read privilege - CSRF token auto-injected by CsrfGuardScriptInjectionFilter @since 2001-2002 @@ -68,9 +68,9 @@ String roleName$ = session.getAttribute("userrole") + "," + session.getAttribute("user"); boolean authed = true; %> - + <%authed = false; %> - <%response.sendRedirect(request.getContextPath() + "/securityError?type=_report&type=_admin.reporting");%> + <%response.sendRedirect(request.getContextPath() + "/securityError?type=_report&type=_admin");%> <% if (!authed) { @@ -161,9 +161,30 @@ + class="form-control form-control-sm">${carlos:forHtml(submittedSql)} + + + + + + + + + + + + +
@@ -211,6 +232,11 @@
+

+ + + +

${results}
diff --git a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java index 30ef00ac5de..f71a261dfa4 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java @@ -21,10 +21,9 @@ */ package io.github.carlos_emr.carlos.report; -import java.util.Collections; - -import io.github.carlos_emr.carlos.PMmodule.dao.SecUserRoleDao; +import io.github.carlos_emr.CarlosProperties; import io.github.carlos_emr.carlos.commn.dao.ReportByExamplesDao; +import io.github.carlos_emr.carlos.commn.dao.ReportByExamplesFavoriteDao; import io.github.carlos_emr.carlos.commn.dao.ReportTemplatesDao; import io.github.carlos_emr.carlos.managers.SecurityInfoManager; import io.github.carlos_emr.carlos.report.pageUtil.RptByExample2Action; @@ -71,15 +70,16 @@ class ReportActionSecurityMigrationUnitTest extends CarlosUnitTestBase { private MockHttpServletResponse response; private SecurityInfoManager securityInfoManager; private LoggedInInfo loggedInInfo; - private SecUserRoleDao secUserRoleDao; + private ReportByExamplesDao reportByExamplesDao; @BeforeEach void setUp() { securityInfoManager = mock(SecurityInfoManager.class); - secUserRoleDao = mock(SecUserRoleDao.class); + reportByExamplesDao = mock(ReportByExamplesDao.class); registerMock(SecurityInfoManager.class, securityInfoManager); - registerMock(SecUserRoleDao.class, secUserRoleDao); - registerMock(ReportByExamplesDao.class, mock(ReportByExamplesDao.class)); + registerMock(ReportByExamplesDao.class, reportByExamplesDao); + ReportByExamplesFavoriteDao favoritesDao = mock(ReportByExamplesFavoriteDao.class); + registerMock(ReportByExamplesFavoriteDao.class, favoritesDao); registerMock(ReportTemplatesDao.class, mock(ReportTemplatesDao.class)); request = new MockHttpServletRequest(); @@ -92,7 +92,7 @@ void setUp() { loggedInInfo = mock(LoggedInInfo.class); when(loggedInInfo.getLoggedInProviderNo()).thenReturn("999998"); - when(secUserRoleDao.findByRoleNameAndProviderNo("admin", "999998")).thenReturn(Collections.emptyList()); + when(favoritesDao.findByProvider("999998")).thenReturn(java.util.Collections.emptyList()); } @AfterEach @@ -142,7 +142,7 @@ void shouldRequireAdminOrReportReadPrivilege_forMigratedActions() { @Test @DisplayName("migrated actions skip report privilege check when admin read is present") - void shouldSkipReportReadPrivilege_whenAdminReadPrivilegeAllowsAccess() { + void shouldSkipReportReadPrivilege_whenAdminReadPrivilegeAllowsAccess() throws Exception { LoggedInInfo.setLoggedInInfoIntoSession(request.getSession(), loggedInInfo); when(securityInfoManager.hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null)).thenReturn(true); @@ -156,7 +156,7 @@ void shouldSkipReportReadPrivilege_whenAdminReadPrivilegeAllowsAccess() { @Test @DisplayName("migrated actions allow report read privilege when admin read is missing") - void shouldAllowAccess_whenReportReadPrivilegeAllowsAccess() { + void shouldAllowAccess_whenReportReadPrivilegeAllowsAccess() throws Exception { LoggedInInfo.setLoggedInInfoIntoSession(request.getSession(), loggedInInfo); when(securityInfoManager.hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null)).thenReturn(false); when(securityInfoManager.hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null)).thenReturn(true); @@ -169,6 +169,34 @@ void shouldAllowAccess_whenReportReadPrivilegeAllowsAccess() { .hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null); } + @Test + @DisplayName("RptByExample keeps the form available but does not execute or save when disabled") + void shouldKeepFormWithoutSaving_whenQueryByExampleIsDisabled() throws Exception { + LoggedInInfo.setLoggedInInfoIntoSession(request.getSession(), loggedInInfo); + when(securityInfoManager.hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null)).thenReturn(false); + when(securityInfoManager.hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null)).thenReturn(true); + request.setMethod("POST"); + RptByExample2Action action = new RptByExample2Action(); + action.setSql("select demographic_no from demographic"); + + CarlosProperties properties = CarlosProperties.getInstance(); + String previousValue = properties.getProperty(RptByExample2Action.ENABLED_PROPERTY); + try { + properties.setProperty(RptByExample2Action.ENABLED_PROPERTY, "false"); + assertThat(action.execute()).isEqualTo(ActionSupport.SUCCESS); + } finally { + if (previousValue == null) { + properties.remove(RptByExample2Action.ENABLED_PROPERTY); + } else { + properties.setProperty(RptByExample2Action.ENABLED_PROPERTY, previousValue); + } + } + + assertThat(request.getAttribute("queryDisabled")).isEqualTo(true); + assertThat(request.getAttribute("submittedSql")).isEqualTo("select demographic_no from demographic"); + verifyNoInteractions(reportByExamplesDao); + } + @Test @DisplayName("UploadTemplates passes LoggedInInfo when adding and editing templates") void shouldPassLoggedInInfo_whenAddingAndEditingTemplates() { @@ -206,10 +234,8 @@ private void assertMissingPrivilegeFails(ActionSupport action) { .hasMessage(MISSING_ADMIN_OR_REPORT); } - private void assertAuthorizedMigrationGateAllowsActionBody() { - assertThatThrownBy(() -> new RptByExample2Action().execute()) - .isInstanceOf(SecurityException.class) - .hasMessage("missing required admin privileges to run query by example"); + private void assertAuthorizedMigrationGateAllowsActionBody() throws Exception { + assertThat(new RptByExample2Action().execute()).isEqualTo(ActionSupport.SUCCESS); request.setParameter("templateid", "template-1"); assertThat(new ExportTemplate2Action().execute()).isEqualTo(ActionSupport.SUCCESS); diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java new file mode 100644 index 00000000000..7da9fad6969 --- /dev/null +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java @@ -0,0 +1,94 @@ +/** + * Copyright (c) 2026 CARLOS Contributors. All Rights Reserved. + * + * This software is published under the GPL GNU General Public License. + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + * + * CARLOS EMR Project + * https://github.com/carlos-emr/carlos + */ +package io.github.carlos_emr.carlos.report.data; + +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import java.util.Properties; +import java.util.stream.Stream; + +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.MethodSource; + +@Tag("unit") +@Tag("report") +@Tag("security") +class QueryByExampleSqlValidatorTest { + private final Properties properties = properties("oscar_mcmaster?useUnicode=true"); + + @Test + @DisplayName("allows one SELECT using application tables, joins, and subqueries") + void shouldAllowReadOnlyApplicationSelects() { + assertThatCode(() -> QueryByExampleSqlValidator.validate( + "select d.demographic_no from demographic d join provider p on p.provider_no=d.provider_no " + + "where exists (select 1 from appointment a where a.demographic_no=d.demographic_no)", + properties)).doesNotThrowAnyException(); + assertThatCode(() -> QueryByExampleSqlValidator.validate( + "select * from `oscar_mcmaster`.`demographic`", properties)).doesNotThrowAnyException(); + } + + @Test + @DisplayName("does not treat prohibited function names inside string literals as invocations") + void shouldAllowBlockedFunctionNameInsideLiteral() { + assertThatCode(() -> QueryByExampleSqlValidator.validate("select 'sleep(1)'", properties)) + .doesNotThrowAnyException(); + } + + @ParameterizedTest(name = "rejects: {0}") + @MethodSource("unsafeQueries") + @DisplayName("rejects unsafe or out-of-scope SQL") + void shouldRejectUnsafeQueries(String sql) { + assertThatThrownBy(() -> QueryByExampleSqlValidator.validate(sql, properties)) + .isInstanceOf(QueryByExampleValidationException.class); + } + + private static Stream unsafeQueries() { + return Stream.of( + "show tables", + "describe demographic", + "explain select * from demographic", + "update demographic set last_name='x'", + "select * from demographic union select * from provider", + "select * from demographic; select * from provider", + "select * from demographic -- comment", + "select * from other_database.demographic", + "select sleep(1)", + "select benchmark(1000, md5('x'))", + "select get_lock('qbe', 1)", + "select release_lock('qbe')", + "select is_free_lock('qbe')", + "select load_file('/etc/passwd')", + "select * from demographic for update", + "select * from demographic for share", + "select demographic_no into @number from demographic"); + } + + private static Properties properties(String databaseName) { + Properties properties = new Properties(); + properties.setProperty("db_name", databaseName); + return properties; + } +} diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java new file mode 100644 index 00000000000..2a45ea9142e --- /dev/null +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java @@ -0,0 +1,106 @@ +/** + * Copyright (c) 2026 CARLOS Contributors. All Rights Reserved. + * + * This software is published under the GPL GNU General Public License. + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License + * as published by the Free Software Foundation; either version 2 + * of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place - Suite 330, Boston, MA 02111-1307, USA. + * + * CARLOS EMR Project + * https://github.com/carlos-emr/carlos + */ +package io.github.carlos_emr.carlos.report.data; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.ResultSetMetaData; +import java.sql.SQLException; +import java.util.Properties; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; +import org.mockito.InOrder; + +@Tag("unit") +@Tag("report") +@Tag("security") +class RptByExampleDataTest { + private Connection connection; + private PreparedStatement statement; + private ResultSet resultSet; + private ResultSetMetaData metadata; + private RptByExampleData reportData; + private Properties properties; + + @BeforeEach + void setUp() throws SQLException { + connection = mock(Connection.class); + statement = mock(PreparedStatement.class); + resultSet = mock(ResultSet.class); + metadata = mock(ResultSetMetaData.class); + reportData = new RptByExampleData(() -> connection); + properties = new Properties(); + properties.setProperty("db_name", "oscar_mcmaster?useUnicode=true"); + + when(connection.isReadOnly()).thenReturn(false); + when(connection.prepareStatement("select demographic_no from demographic", + ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY)).thenReturn(statement); + when(statement.executeQuery()).thenReturn(resultSet); + when(resultSet.getMetaData()).thenReturn(metadata); + when(metadata.getColumnCount()).thenReturn(1); + when(metadata.getColumnName(1)).thenReturn("demographic_no"); + when(resultSet.next()).thenReturn(true, false); + when(resultSet.getString("demographic_no")).thenReturn("42"); + } + + @Test + @DisplayName("executes once using a bounded read-only JDBC statement and restores connection state") + void shouldExecuteBoundedReadOnlyQueryAndRestoreConnection() throws SQLException { + RptByExampleData.QueryResult result = reportData.execute( + "select demographic_no from demographic", properties, "999998"); + + assertThat(result.rowCount()).isEqualTo(1); + assertThat(result.html()).contains("demographic_no").contains("42"); + verify(statement).setMaxRows(RptByExampleData.MAX_ROWS); + verify(statement).setQueryTimeout(RptByExampleData.QUERY_TIMEOUT_SECONDS); + + InOrder order = inOrder(connection, statement, resultSet); + order.verify(connection).setReadOnly(true); + order.verify(statement).executeQuery(); + order.verify(resultSet).close(); + order.verify(statement).close(); + order.verify(connection).setReadOnly(false); + order.verify(connection).close(); + } + + @Test + @DisplayName("rejects unsafe SQL before acquiring a database connection") + void shouldRejectBeforeConnecting() throws SQLException { + assertThatThrownBy(() -> reportData.execute("delete from demographic", properties, "999998")) + .isInstanceOf(QueryByExampleValidationException.class); + + verify(connection, never()).prepareStatement(org.mockito.ArgumentMatchers.anyString(), + org.mockito.ArgumentMatchers.anyInt(), org.mockito.ArgumentMatchers.anyInt()); + } +} From 4477e8ee489f3ee0a38b2b418a3d55a4eeecb486 Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 13:21:09 -0400 Subject: [PATCH 02/21] fix(report): address query review findings Signed-off-by: Ben Heerema --- dependencies-lock.json | 16 ------- pom.xml | 7 +++ .../carlos_emr/carlos/db/LegacyJdbcQuery.java | 36 +++++++++++++-- .../data/QueryByExampleSqlValidator.java | 37 ++++++++-------- .../carlos/report/data/RptByExampleData.java | 32 +++++++++++++- .../carlos/report/data/RptResultStruct.java | 30 ++++++------- .../report/pageUtil/RptByExample2Action.java | 30 +++++++++---- .../resources/oscarResources_en.properties | 1 + .../resources/oscarResources_es.properties | 12 ++--- .../resources/oscarResources_fr.properties | 12 ++--- .../resources/oscarResources_pl.properties | 12 ++--- .../resources/oscarResources_pt_BR.properties | 12 ++--- .../WEB-INF/jsp/oscarReport/RptByExample.jsp | 9 +++- .../carlos/db/LegacyJdbcQueryUnitTest.java | 3 ++ ...ReportActionSecurityMigrationUnitTest.java | 44 +++++++++++++++++++ .../data/QueryByExampleSqlValidatorTest.java | 25 ++++++++++- .../report/data/RptByExampleDataTest.java | 43 ++++++++++++++++-- 17 files changed, 270 insertions(+), 91 deletions(-) diff --git a/dependencies-lock.json b/dependencies-lock.json index c7829800ce7..99acb8663e0 100644 --- a/dependencies-lock.json +++ b/dependencies-lock.json @@ -1194,14 +1194,6 @@ "type" : "jar", "optional" : false, "integrity" : "sha512:6oPQclFmUY/UxgXW0HXw/IcSjKRVy9/6wIVOBVKOeZpPu7OAUHGLCgbpQmwujcd/qVlLZXCDu1HHErGF1OiacQ==" - }, { - "groupId" : "net.sf.jopt-simple", - "artifactId" : "jopt-simple", - "version" : "5.0.4", - "scope" : "compile", - "type" : "jar", - "optional" : false, - "integrity" : "sha512:y8J+C22mrktiRTU9ZibS48FxwwJqVV+iHo72GzBxTihtuFCG0aV8FnAW6Kfwe+KiQ+NLOrUEsYd4BvO87F35hg==" }, { "groupId" : "net.sf.saxon", "artifactId" : "Saxon-HE", @@ -3187,14 +3179,6 @@ "type" : "jar", "optional" : false, "integrity" : "sha512:H6mQ0VvRefB/+8Rg1YCm/QVi5F3ui9SpQFkXU2t49FwNb2RLZ/hdeBx1iqVu/5Cu8j7tzJvX9f+Ieme3Fgg+YQ==" - }, { - "groupId" : "org.openjdk.jmh", - "artifactId" : "jmh-core", - "version" : "1.37", - "scope" : "compile", - "type" : "jar", - "optional" : false, - "integrity" : "sha512:jkNMyJ98ijA14XJnXZ+RRBUDmtjcQDqfSjBu++MknCDaA0OqUev547n4umdG6FRKxWH7zWLynbsWO38QyWwfNA==" }, { "groupId" : "org.opensaml", "artifactId" : "opensaml-core-api", diff --git a/pom.xml b/pom.xml index b8c64db6973..057bd318f15 100644 --- a/pom.xml +++ b/pom.xml @@ -453,6 +453,13 @@ com.github.jsqlparser jsqlparser ${jsqlparser.version} + + + + org.openjdk.jmh + jmh-core + + diff --git a/src/main/java/io/github/carlos_emr/carlos/db/LegacyJdbcQuery.java b/src/main/java/io/github/carlos_emr/carlos/db/LegacyJdbcQuery.java index 5d735d2c6fa..cdfc724c7ac 100644 --- a/src/main/java/io/github/carlos_emr/carlos/db/LegacyJdbcQuery.java +++ b/src/main/java/io/github/carlos_emr/carlos/db/LegacyJdbcQuery.java @@ -332,21 +332,22 @@ public static void validateSafeSelectQuery(String sql) throws SQLException { throw new SQLException("Potential SQL injection pattern detected"); } - if (containsSqlWord(normalized, "union")) { + String normalizedSqlSyntax = stripQuotedSqlSections(sql).toLowerCase(Locale.ROOT); + if (containsSqlWord(normalizedSqlSyntax, "union")) { throw new SQLException("Unsafe SQL detected: UNION not permitted"); } String[] blockedWords = {"insert", "update", "delete", "drop", "alter", "create", "truncate", "grant", "revoke", "exec", "execute", "call", "merge", "commit", "rollback"}; for (String word : blockedWords) { - if (containsSqlWord(normalized, word)) { + if (containsSqlWord(normalizedSqlSyntax, word)) { throw new SQLException("Unsafe SQL detected: prohibited keyword"); } } String[] blockedPhrases = {"into outfile", "into dumpfile", "load_file", "load data"}; for (String phrase : blockedPhrases) { - if (normalized.contains(phrase)) { + if (normalizedSqlSyntax.contains(phrase)) { throw new SQLException("Unsafe SQL detected: prohibited keyword"); } } @@ -615,6 +616,35 @@ private static boolean containsSqlWord(String sql, String word) { return false; } + private static String stripQuotedSqlSections(String sql) { + StringBuilder stripped = new StringBuilder(sql.length()); + char quote = '\0'; + for (int i = 0; i < sql.length(); i++) { + char current = sql.charAt(i); + char next = i + 1 < sql.length() ? sql.charAt(i + 1) : '\0'; + if (quote == '\0') { + if (current == '\'' || current == '"' || current == '`') { + quote = current; + stripped.append(' '); + } else { + stripped.append(current); + } + } else if (quote != '`' && current == '\\' && next != '\0') { + stripped.append(" "); + i++; + } else if (current == quote && next == quote) { + stripped.append(" "); + i++; + } else if (current == quote) { + quote = '\0'; + stripped.append(' '); + } else { + stripped.append(' '); + } + } + return stripped.toString(); + } + private static boolean startsWithSqlWord(String sql, String word) { return sql.startsWith(word) && (sql.length() == word.length() || !isSqlIdentifierPart(sql.charAt(word.length()))); diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java index ff48b9cea3a..56a0d5c7688 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java @@ -22,7 +22,6 @@ package io.github.carlos_emr.carlos.report.data; import java.sql.SQLException; -import java.util.Locale; import java.util.Properties; import java.util.Set; import java.util.regex.Pattern; @@ -41,19 +40,17 @@ public final class QueryByExampleSqlValidator { "\\bfor\\s+(?:update|share)\\b|\\block\\s+in\\s+share\\s+mode\\b", Pattern.CASE_INSENSITIVE); private static final Pattern OUTPUT_OPERATION = Pattern.compile("\\binto\\b", Pattern.CASE_INSENSITIVE); - private static final Set BLOCKED_FUNCTIONS = Set.of( - "sleep", "benchmark", "get_lock", "release_lock", "is_free_lock", "load_file"); + private static final Pattern BLOCKED_FUNCTION = Pattern.compile( + "(? tables = new TablesNamesFinder().getTables(statement); + Set tables; + try { + tables = new TablesNamesFinder().getTables(statement); + } catch (RuntimeException e) { + throw new QueryByExampleValidationException("The query table references could not be validated", e); + } for (String table : tables) { String normalizedTable = unquoteIdentifier(table); int lastDot = normalizedTable.lastIndexOf('.'); @@ -107,13 +113,8 @@ private static void rejectOtherSchemas(Statement statement, String applicationSc } private static void rejectBlockedFunctions(String sql) throws QueryByExampleValidationException { - String normalized = sql.toLowerCase(Locale.ROOT); - for (String function : BLOCKED_FUNCTIONS) { - Pattern invocation = Pattern.compile("(?"); for (int i = 0; i < columns; i++) { // for each column in result set - columnNames[i] = rsmd.getColumnName(i + 1); + columnLabels[i] = rsmd.getColumnLabel(i + 1); // put names in array // use i+1 or else you're going to get an exception // insert headings for table sb.append(""); - sb.append(Encode.forHtml(columnNames[i])); + sb.append(Encode.forHtml(columnLabels[i])); sb.append(""); } int rowCount = 0; while (rs.next()) { rowCount++; - sb.append(""); + sb.append(""); for (int j = 0; j < columns; j++) { sb.append(""); - sb.append(Encode.forHtml(Misc.getString(rs, columnNames[j]))); + sb.append(Encode.forHtml(Misc.getString(rs, j + 1))); sb.append(""); } - rowColor = rowColor.compareTo("rowColor1") == 0 ? "rowColor2" : "rowColor1"; + rowColor = rowColor.equals("rowColor1") ? "rowColor2" : "rowColor1"; sb.append(""); } sb.append(""); diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java index 4ff9dc61ad4..cc1ef7525d9 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java @@ -122,17 +122,28 @@ public String execute() return SUCCESS; } + RptByExampleData.QueryResult result; try { - RptByExampleData.QueryResult result = new RptByExampleData().execute(sql, properties, providerNo); - request.setAttribute("results", result.html()); - request.setAttribute("resultRowCount", result.rowCount()); - write2Database(sql, providerNo); + result = new RptByExampleData().execute(sql, properties, providerNo); } catch (QueryByExampleValidationException e) { request.setAttribute("queryValidationError", true); + return SUCCESS; } catch (SQLTimeoutException e) { request.setAttribute("queryTimeout", true); + return SUCCESS; } catch (SQLException | RuntimeException e) { request.setAttribute("queryExecutionError", true); + return SUCCESS; + } + + request.setAttribute("results", result.html()); + request.setAttribute("resultRowCount", result.rowCount()); + request.setAttribute("resultLimit", RptByExampleData.MAX_ROWS); + try { + write2Database(sql, providerNo); + } catch (RuntimeException e) { + request.setAttribute("queryHistoryError", true); + RptByExampleData.audit(providerNo, sql, 0, result.rowCount(), "history_failed"); } return SUCCESS; @@ -140,10 +151,13 @@ public String execute() static boolean isEnabled(Properties properties) { String configured = properties.getProperty(ENABLED_PROPERTY); - return configured == null || configured.isBlank() - || configured.equalsIgnoreCase("true") - || configured.equalsIgnoreCase("yes") - || configured.equalsIgnoreCase("on"); + if (configured == null || configured.isBlank()) { + return true; + } + String normalized = configured.trim(); + return normalized.equalsIgnoreCase("true") + || normalized.equalsIgnoreCase("yes") + || normalized.equalsIgnoreCase("on"); } public void write2Database(String query, String providerNo) { diff --git a/src/main/resources/oscarResources_en.properties b/src/main/resources/oscarResources_en.properties index 6a7ecb590dc..84a1c4f4097 100644 --- a/src/main/resources/oscarResources_en.properties +++ b/src/main/resources/oscarResources_en.properties @@ -8927,6 +8927,7 @@ oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disable oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. +oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. oscarReport.RptByExample.MsgConfirmDelete=Are you sure you want to delete the selected query? diff --git a/src/main/resources/oscarResources_es.properties b/src/main/resources/oscarResources_es.properties index ea784b60dfc..37f5596fd17 100644 --- a/src/main/resources/oscarResources_es.properties +++ b/src/main/resources/oscarResources_es.properties @@ -6446,11 +6446,13 @@ oscarReport.RptByExample.MsgViewQueryHistory=Ver historial de consultas oscarReport.RptByExample.MsgLoadQuery=Cargar consulta oscarReport.RptByExample.MsgRunQuery=Ejecutar -oscarReport.RptByExample.MsgDisabled=La ejecuci\u00f3n directa de consultas est\u00e1 desactivada. Su consulta no se ha ejecutado. -oscarReport.RptByExample.MsgValidationError=Solo se permite una consulta SELECT de solo lectura en la base de datos de la aplicaci\u00f3n. No se permiten comentarios, UNION, bloqueos, operaciones de salida ni funciones prohibidas. -oscarReport.RptByExample.MsgTimeout=La consulta super\u00f3 el l\u00edmite de 15 segundos y se detuvo. -oscarReport.RptByExample.MsgExecutionError=No se pudo completar la consulta. Rev\u00edsela e int\u00e9ntelo de nuevo. -oscarReport.RptByExample.MsgResultLimit=Los resultados est\u00e1n limitados a {0} filas. +# TODO: translate Query-by-Example execution messages. +oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disabled. Your query has not been run. +oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. +oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. +oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. +oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. +oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. oscarReport.RptByExample.MsgConfirmDelete=\u00bfEst\u00e1 seguro de que desea eliminar la consulta seleccionada? diff --git a/src/main/resources/oscarResources_fr.properties b/src/main/resources/oscarResources_fr.properties index acf4a4f5a8a..b9a05ae984e 100644 --- a/src/main/resources/oscarResources_fr.properties +++ b/src/main/resources/oscarResources_fr.properties @@ -5628,11 +5628,13 @@ oscarReport.RptByExample.MsgRefresh=Actualiser oscarReport.RptByExample.MsgViewQueryHistory=Voir l\u2019historique des requ\u00eates oscarReport.RptByExample.MsgLoadQuery=Charger la requ\u00eate oscarReport.RptByExample.MsgRunQuery=Ex\u00e9cuter -oscarReport.RptByExample.MsgDisabled=L\u2019ex\u00e9cution directe des requ\u00eates est actuellement d\u00e9sactiv\u00e9e. Votre requ\u00eate n\u2019a pas \u00e9t\u00e9 ex\u00e9cut\u00e9e. -oscarReport.RptByExample.MsgValidationError=Une seule requ\u00eate SELECT en lecture seule dans la base de donn\u00e9es de l\u2019application est autoris\u00e9e. Les commentaires, UNION, verrouillages, op\u00e9rations de sortie et fonctions interdites ne sont pas permis. -oscarReport.RptByExample.MsgTimeout=La requ\u00eate a d\u00e9pass\u00e9 la limite de 15 secondes et a \u00e9t\u00e9 arr\u00eat\u00e9e. -oscarReport.RptByExample.MsgExecutionError=La requ\u00eate n\u2019a pas pu \u00eatre ex\u00e9cut\u00e9e. V\u00e9rifiez-la et r\u00e9essayez. -oscarReport.RptByExample.MsgResultLimit=Les r\u00e9sultats sont limit\u00e9s \u00e0 {0} lignes. +# TODO: translate Query-by-Example execution messages. +oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disabled. Your query has not been run. +oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. +oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. +oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. +oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. +oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. oscarReport.RptByExample.MsgConfirmDelete=\u00cates-vous s\u00fbr de vouloir supprimer la requ\u00eate s\u00e9lectionn\u00e9e\u00a0? diff --git a/src/main/resources/oscarResources_pl.properties b/src/main/resources/oscarResources_pl.properties index 027b71887ba..0323c830d93 100644 --- a/src/main/resources/oscarResources_pl.properties +++ b/src/main/resources/oscarResources_pl.properties @@ -5864,11 +5864,13 @@ oscarReport.RptByExample.MsgViewQueryHistory=Historia zapyta\u0144 oscarReport.RptByExample.MsgLoadQuery=Za\u0142aduj zapytanie oscarReport.RptByExample.MsgRunQuery=Wykonaj -oscarReport.RptByExample.MsgDisabled=Bezpo\u015brednie wykonywanie zapyta\u0144 jest obecnie wy\u0142\u0105czone. Zapytanie nie zosta\u0142o wykonane. -oscarReport.RptByExample.MsgValidationError=Dozwolone jest tylko jedno zapytanie SELECT w trybie tylko do odczytu w bazie aplikacji. Komentarze, UNION, blokady, operacje wyj\u015bciowe i zabronione funkcje nie s\u0105 dozwolone. -oscarReport.RptByExample.MsgTimeout=Zapytanie przekroczy\u0142o limit 15 sekund i zosta\u0142o zatrzymane. -oscarReport.RptByExample.MsgExecutionError=Nie uda\u0142o si\u0119 wykona\u0107 zapytania. Sprawd\u017a je i spr\u00f3buj ponownie. -oscarReport.RptByExample.MsgResultLimit=Wyniki s\u0105 ograniczone do {0} wierszy. +# TODO: translate Query-by-Example execution messages. +oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disabled. Your query has not been run. +oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. +oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. +oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. +oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. +oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. oscarReport.RptByExample.MsgConfirmDelete=Czy na pewno chcesz usun\u0105\u0107 wybran\u0105 kwerend\u0119? diff --git a/src/main/resources/oscarResources_pt_BR.properties b/src/main/resources/oscarResources_pt_BR.properties index a58f770bebe..b244d0bbde9 100644 --- a/src/main/resources/oscarResources_pt_BR.properties +++ b/src/main/resources/oscarResources_pt_BR.properties @@ -7351,11 +7351,13 @@ oscarReport.RptByExample.MsgViewQueryHistory=Ver hist\u00f3rico de consultas oscarReport.RptByExample.MsgLoadQuery=Carregar consulta oscarReport.RptByExample.MsgRunQuery=Executar -oscarReport.RptByExample.MsgDisabled=A execu\u00e7\u00e3o direta de consultas est\u00e1 desativada. Sua consulta n\u00e3o foi executada. -oscarReport.RptByExample.MsgValidationError=Apenas uma consulta SELECT somente leitura no banco de dados do aplicativo \u00e9 permitida. Coment\u00e1rios, UNION, bloqueios, opera\u00e7\u00f5es de sa\u00edda e fun\u00e7\u00f5es proibidas n\u00e3o s\u00e3o permitidos. -oscarReport.RptByExample.MsgTimeout=A consulta excedeu o limite de 15 segundos e foi interrompida. -oscarReport.RptByExample.MsgExecutionError=N\u00e3o foi poss\u00edvel concluir a consulta. Verifique-a e tente novamente. -oscarReport.RptByExample.MsgResultLimit=Os resultados est\u00e3o limitados a {0} linhas. +# TODO: translate Query-by-Example execution messages. +oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disabled. Your query has not been run. +oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. +oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. +oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. +oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. +oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. oscarReport.RptByExample.MsgConfirmDelete=Tem certeza de que deseja excluir a consulta selecionada? diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp index 9b2b53248d2..fa30f02a8c9 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp @@ -161,7 +161,7 @@ + class="form-control form-control-sm">${carlos:forHtmlContent(submittedSql)}
@@ -184,6 +184,11 @@ + + +
@@ -234,7 +239,7 @@

- +

${results} diff --git a/src/test/java/io/github/carlos_emr/carlos/db/LegacyJdbcQueryUnitTest.java b/src/test/java/io/github/carlos_emr/carlos/db/LegacyJdbcQueryUnitTest.java index 8cede3d535a..38076aebc93 100644 --- a/src/test/java/io/github/carlos_emr/carlos/db/LegacyJdbcQueryUnitTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/db/LegacyJdbcQueryUnitTest.java @@ -61,6 +61,9 @@ void releaseLegacyJdbcResources() { void shouldAllowSelectOnlyQueries_forAdminReportBoundary() { assertThatCode(() -> validateSafeSelectQuery("select demographic_no from demographic")) .doesNotThrowAnyException(); + assertThatCode(() -> validateSafeSelectQuery( + "select 'update delete create drop' as instruction from demographic")) + .doesNotThrowAnyException(); } @Test diff --git a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java index f71a261dfa4..e44ea72a6fe 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java @@ -21,11 +21,15 @@ */ package io.github.carlos_emr.carlos.report; +import java.util.Properties; + import io.github.carlos_emr.CarlosProperties; import io.github.carlos_emr.carlos.commn.dao.ReportByExamplesDao; import io.github.carlos_emr.carlos.commn.dao.ReportByExamplesFavoriteDao; import io.github.carlos_emr.carlos.commn.dao.ReportTemplatesDao; +import io.github.carlos_emr.carlos.commn.model.ReportByExamples; import io.github.carlos_emr.carlos.managers.SecurityInfoManager; +import io.github.carlos_emr.carlos.report.data.RptByExampleData; import io.github.carlos_emr.carlos.report.pageUtil.RptByExample2Action; import io.github.carlos_emr.carlos.report.reportByTemplate.ReportFactory; import io.github.carlos_emr.carlos.report.reportByTemplate.ReportManager; @@ -51,6 +55,9 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockConstruction; import static org.mockito.Mockito.never; @@ -197,6 +204,43 @@ void shouldKeepFormWithoutSaving_whenQueryByExampleIsDisabled() throws Exception verifyNoInteractions(reportByExamplesDao); } + @Test + @DisplayName("RptByExample keeps successful results when query history cannot be saved") + void shouldKeepResults_whenQueryHistorySaveFails() throws Exception { + LoggedInInfo.setLoggedInInfoIntoSession(request.getSession(), loggedInInfo); + when(securityInfoManager.hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null)).thenReturn(false); + when(securityInfoManager.hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null)).thenReturn(true); + request.setMethod("POST"); + RptByExample2Action action = new RptByExample2Action(); + action.setSql("select demographic_no from demographic"); + doThrow(new IllegalStateException("history unavailable")) + .when(reportByExamplesDao).persist(any(ReportByExamples.class)); + + CarlosProperties properties = CarlosProperties.getInstance(); + String previousValue = properties.getProperty(RptByExample2Action.ENABLED_PROPERTY); + try (MockedConstruction reportData = mockConstruction( + RptByExampleData.class, + (mock, context) -> when(mock.execute( + eq("select demographic_no from demographic"), any(Properties.class), eq("999998"))) + .thenReturn(new RptByExampleData.QueryResult("
", 1)))) { + properties.setProperty(RptByExample2Action.ENABLED_PROPERTY, " yes "); + assertThat(action.execute()).isEqualTo(ActionSupport.SUCCESS); + assertThat(reportData.constructed()).hasSize(1); + } finally { + if (previousValue == null) { + properties.remove(RptByExample2Action.ENABLED_PROPERTY); + } else { + properties.setProperty(RptByExample2Action.ENABLED_PROPERTY, previousValue); + } + } + + assertThat(request.getAttribute("results")).isEqualTo("
"); + assertThat(request.getAttribute("queryHistoryError")).isEqualTo(true); + assertThat(request.getAttribute("queryExecutionError")).isNull(); + assertThat(request.getAttribute("resultLimit")).isEqualTo(RptByExampleData.MAX_ROWS); + verify(reportByExamplesDao).persist(any(ReportByExamples.class)); + } + @Test @DisplayName("UploadTemplates passes LoggedInInfo when adding and editing templates") void shouldPassLoggedInInfo_whenAddingAndEditingTemplates() { diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java index 7da9fad6969..a9ddf6d4c8a 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java @@ -55,6 +55,30 @@ void shouldAllowReadOnlyApplicationSelects() { void shouldAllowBlockedFunctionNameInsideLiteral() { assertThatCode(() -> QueryByExampleSqlValidator.validate("select 'sleep(1)'", properties)) .doesNotThrowAnyException(); + assertThatCode(() -> QueryByExampleSqlValidator.validate( + "select 'update delete create drop' as instruction", properties)) + .doesNotThrowAnyException(); + } + + @Test + @DisplayName("structurally rejects set-operation SELECTs") + void shouldRejectSetOperationSelects() { + assertThatThrownBy(() -> QueryByExampleSqlValidator.validate( + "select demographic_no from demographic union select provider_no from provider", properties)) + .isInstanceOf(QueryByExampleValidationException.class) + .hasMessage("Only one SELECT statement is allowed"); + } + + @Test + @DisplayName("fails closed when the application schema is not configured") + void shouldRejectMissingApplicationSchema() { + Properties missing = new Properties(); + Properties blank = properties(" ?useUnicode=true"); + + assertThatThrownBy(() -> QueryByExampleSqlValidator.applicationSchema(missing)) + .isInstanceOf(QueryByExampleValidationException.class); + assertThatThrownBy(() -> QueryByExampleSqlValidator.applicationSchema(blank)) + .isInstanceOf(QueryByExampleValidationException.class); } @ParameterizedTest(name = "rejects: {0}") @@ -71,7 +95,6 @@ private static Stream unsafeQueries() { "describe demographic", "explain select * from demographic", "update demographic set last_name='x'", - "select * from demographic union select * from provider", "select * from demographic; select * from provider", "select * from demographic -- comment", "select * from other_database.demographic", diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java index 2a45ea9142e..b529873e80c 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java @@ -23,6 +23,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.doThrow; import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -34,6 +35,7 @@ import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; +import java.sql.SQLTimeoutException; import java.util.Properties; import org.junit.jupiter.api.BeforeEach; @@ -63,15 +65,15 @@ void setUp() throws SQLException { properties = new Properties(); properties.setProperty("db_name", "oscar_mcmaster?useUnicode=true"); - when(connection.isReadOnly()).thenReturn(false); + when(connection.isReadOnly()).thenReturn(true); when(connection.prepareStatement("select demographic_no from demographic", ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY)).thenReturn(statement); when(statement.executeQuery()).thenReturn(resultSet); when(resultSet.getMetaData()).thenReturn(metadata); when(metadata.getColumnCount()).thenReturn(1); - when(metadata.getColumnName(1)).thenReturn("demographic_no"); + when(metadata.getColumnLabel(1)).thenReturn("demographic_no"); when(resultSet.next()).thenReturn(true, false); - when(resultSet.getString("demographic_no")).thenReturn("42"); + when(resultSet.getString(1)).thenReturn("42"); } @Test @@ -90,10 +92,43 @@ void shouldExecuteBoundedReadOnlyQueryAndRestoreConnection() throws SQLException order.verify(statement).executeQuery(); order.verify(resultSet).close(); order.verify(statement).close(); - order.verify(connection).setReadOnly(false); + order.verify(connection).setReadOnly(true); order.verify(connection).close(); } + @Test + @DisplayName("preserves a timeout when restoring connection state also fails") + void shouldPreserveTimeout_whenReadOnlyRestoreFails() throws SQLException { + SQLTimeoutException timeout = new SQLTimeoutException("timed out"); + SQLException restoreFailure = new SQLException("restore failed"); + when(connection.isReadOnly()).thenReturn(false); + when(statement.executeQuery()).thenThrow(timeout); + doThrow(restoreFailure).when(connection).setReadOnly(false); + + assertThatThrownBy(() -> reportData.execute( + "select demographic_no from demographic", properties, "999998")) + .isSameAs(timeout) + .satisfies(thrown -> assertThat(thrown.getSuppressed()).containsExactly(restoreFailure)); + + verify(connection).setReadOnly(false); + verify(connection).close(); + } + + @Test + @DisplayName("renders duplicate column labels using their positional values") + void shouldRenderPositionalValues_whenColumnLabelsAreDuplicated() throws SQLException { + when(metadata.getColumnCount()).thenReturn(2); + when(metadata.getColumnLabel(1)).thenReturn("id"); + when(metadata.getColumnLabel(2)).thenReturn("id"); + when(resultSet.getString(1)).thenReturn("first"); + when(resultSet.getString(2)).thenReturn("second"); + + RptResultStruct.StructuredResult result = RptResultStruct.getStructureWithCount(resultSet); + + assertThat(result.html()).contains("first").contains("second"); + assertThat(result.rowCount()).isEqualTo(1); + } + @Test @DisplayName("rejects unsafe SQL before acquiring a database connection") void shouldRejectBeforeConnecting() throws SQLException { From ee4a8dbee8cc95864dd683bfa77492ddac53196c Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 13:43:30 -0400 Subject: [PATCH 03/21] fix(report): harden schema comparison Signed-off-by: Ben Heerema --- .../carlos/report/data/QueryByExampleSqlValidator.java | 7 ++++++- .../carlos/report/pageUtil/RptByExample2Action.java | 9 ++++----- .../report/data/QueryByExampleSqlValidatorTest.java | 1 + 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java index 56a0d5c7688..3a347e41c28 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java @@ -22,6 +22,7 @@ package io.github.carlos_emr.carlos.report.data; import java.sql.SQLException; +import java.util.Locale; import java.util.Properties; import java.util.Set; import java.util.regex.Pattern; @@ -105,7 +106,7 @@ private static void rejectOtherSchemas(Statement statement, String applicationSc int lastDot = normalizedTable.lastIndexOf('.'); if (lastDot > 0) { String qualifier = normalizedTable.substring(0, lastDot); - if (!qualifier.equalsIgnoreCase(applicationSchema)) { + if (!canonicalIdentifier(qualifier).equals(canonicalIdentifier(applicationSchema))) { throw new QueryByExampleValidationException("Queries may only read the application database schema"); } } @@ -150,4 +151,8 @@ private static String stripStringLiterals(String sql) { private static String unquoteIdentifier(String identifier) { return identifier.replace("`", "").replace("\"", "").trim(); } + + private static String canonicalIdentifier(String identifier) { + return identifier.toLowerCase(Locale.ROOT); + } } diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java index cc1ef7525d9..d9fd9b2d1f3 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java @@ -35,6 +35,7 @@ import java.sql.SQLTimeoutException; import java.util.Collection; import java.util.Date; +import java.util.Locale; import java.util.Properties; import jakarta.servlet.ServletException; @@ -105,7 +106,7 @@ public String execute() sql = sql == null ? "" : sql; request.setAttribute("submittedSql", sql); - if (!"POST".equalsIgnoreCase(request.getMethod())) { + if (!"POST".equals(request.getMethod())) { return SUCCESS; } @@ -154,10 +155,8 @@ static boolean isEnabled(Properties properties) { if (configured == null || configured.isBlank()) { return true; } - String normalized = configured.trim(); - return normalized.equalsIgnoreCase("true") - || normalized.equalsIgnoreCase("yes") - || normalized.equalsIgnoreCase("on"); + String normalized = configured.trim().toLowerCase(Locale.ROOT); + return normalized.equals("true") || normalized.equals("yes") || normalized.equals("on"); } public void write2Database(String query, String providerNo) { diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java index a9ddf6d4c8a..542053af9b2 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java @@ -98,6 +98,7 @@ private static Stream unsafeQueries() { "select * from demographic; select * from provider", "select * from demographic -- comment", "select * from other_database.demographic", + "select * from o\u017Fcar_mcmaster.demographic", "select sleep(1)", "select benchmark(1000, md5('x'))", "select get_lock('qbe', 1)", From 233df2be3aaa1a43c2731e40db4b3989aaf8bfd5 Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 13:55:05 -0400 Subject: [PATCH 04/21] chore(report): document scanner boundaries Signed-off-by: Ben Heerema --- .../carlos_emr/carlos/report/data/RptByExampleData.java | 8 ++++++++ .../carlos/report/pageUtil/RptByExample2Action.java | 3 +++ 2 files changed, 11 insertions(+) diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java b/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java index d3d27364bbf..1b8ae14e364 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java @@ -39,6 +39,7 @@ import org.apache.commons.codec.digest.DigestUtils; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import io.github.carlos_emr.carlos.db.LegacyJdbcQuery; import io.github.carlos_emr.carlos.utility.MiscUtils; @@ -67,6 +68,13 @@ public RptByExampleData() { this.connectionProvider = connectionProvider; } + @SuppressFBWarnings( + value = { + "SQL_INJECTION_JDBC", + "SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING", + "THROWS_METHOD_THROWS_RUNTIMEEXCEPTION" + }, + justification = "Validated dynamic SQL is intentional; runtime failures are audited and handled by the action") public QueryResult execute(String sql, Properties properties, String providerNo) throws SQLException { long startedAt = System.nanoTime(); String outcome = "failed"; diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java index d9fd9b2d1f3..e0be4926c11 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java @@ -150,6 +150,9 @@ public String execute() return SUCCESS; } + @SuppressFBWarnings( + value = "IMPROPER_UNICODE", + justification = "Locale.ROOT normalization is safe for controlled ASCII feature-flag values") static boolean isEnabled(Properties properties) { String configured = properties.getProperty(ENABLED_PROPERTY); if (configured == null || configured.isBlank()) { From d37a0bf9525c5960c8679143610404ad4d3158f4 Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 14:01:51 -0400 Subject: [PATCH 05/21] test(report): complete query review follow-ups Signed-off-by: Ben Heerema --- .../data/QueryByExampleSqlValidator.java | 20 ++++++++++++++++++- .../QueryByExampleValidationException.java | 17 +++++++++++++++- .../carlos/report/data/RptByExampleData.java | 4 +++- .../report/pageUtil/RptByExample2Action.java | 1 + .../resources/oscarResources_en.properties | 4 ++-- .../resources/oscarResources_es.properties | 4 ++-- .../resources/oscarResources_fr.properties | 4 ++-- .../resources/oscarResources_pl.properties | 4 ++-- .../resources/oscarResources_pt_BR.properties | 4 ++-- .../WEB-INF/jsp/oscarReport/RptByExample.jsp | 5 ++++- .../data/QueryByExampleSqlValidatorTest.java | 8 ++++---- .../report/data/RptByExampleDataTest.java | 20 +++++++++++++++++-- 12 files changed, 75 insertions(+), 20 deletions(-) diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java index 3a347e41c28..33ca5acafc1 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java @@ -35,7 +35,16 @@ import net.sf.jsqlparser.statement.select.SetOperationList; import net.sf.jsqlparser.util.TablesNamesFinder; -/** Fail-closed validation for request-submitted Query-by-Example SQL. */ +/** + * Fail-closed validation for request-submitted Query-by-Example SQL. + * + *

Accepted input is one non-locking, non-output {@code SELECT} whose table + * references are unqualified or belong to the configured application schema. + * Comments, statement separators, set operations, write/control keywords, and + * prohibited database functions are rejected.

+ * + * @since 2026-08-06 + */ public final class QueryByExampleSqlValidator { private static final Pattern LOCKING_SELECT = Pattern.compile( "\\bfor\\s+(?:update|share)\\b|\\block\\s+in\\s+share\\s+mode\\b", @@ -48,6 +57,15 @@ public final class QueryByExampleSqlValidator { private QueryByExampleSqlValidator() { } + /** + * Validates SQL and returns the same text wrapped for the trusted JDBC boundary. + * + * @param sql request-submitted SQL to validate + * @param properties application properties containing a non-blank {@code db_name} + * @return the unchanged SQL represented as {@link LegacyJdbcQuery.TrustedSql} + * @throws QueryByExampleValidationException if the SQL is empty, cannot be parsed, + * is not one allowed {@code SELECT}, or references an unapproved schema or operation + */ public static LegacyJdbcQuery.TrustedSql validate(String sql, Properties properties) throws QueryByExampleValidationException { if (sql == null || sql.isBlank()) { diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleValidationException.java b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleValidationException.java index 743712bcd31..591f78ac9aa 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleValidationException.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleValidationException.java @@ -23,12 +23,27 @@ import java.sql.SQLException; -/** Indicates that a Query-by-Example submission was rejected before execution. */ +/** + * Indicates that a Query-by-Example submission was rejected before execution. + * + * @since 2026-08-06 + */ public class QueryByExampleValidationException extends SQLException { + /** + * Creates a validation exception with a user-safe diagnostic message. + * + * @param message description of why validation failed + */ public QueryByExampleValidationException(String message) { super(message); } + /** + * Creates a validation exception retaining the parser or validation failure. + * + * @param message description of why validation failed + * @param cause underlying parser or SQL validation failure + */ public QueryByExampleValidationException(String message, Throwable cause) { super(message, cause); } diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java b/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java index 1b8ae14e364..2d85959cda9 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java @@ -44,7 +44,9 @@ import io.github.carlos_emr.carlos.utility.MiscUtils; /** - * This classes main function FluReportGenerate collects a group of patients with flu in the last specified date + * Validates and executes Query-by-Example SQL for authorized report users. + * Queries run read-only with row and timeout limits, and every outcome is audited + * without recording the submitted SQL text. */ public class RptByExampleData { public static final int MAX_ROWS = 1_000; diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java index e0be4926c11..c7008c1b05b 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java @@ -131,6 +131,7 @@ public String execute() return SUCCESS; } catch (SQLTimeoutException e) { request.setAttribute("queryTimeout", true); + request.setAttribute("queryTimeoutSeconds", RptByExampleData.QUERY_TIMEOUT_SECONDS); return SUCCESS; } catch (SQLException | RuntimeException e) { request.setAttribute("queryExecutionError", true); diff --git a/src/main/resources/oscarResources_en.properties b/src/main/resources/oscarResources_en.properties index 84a1c4f4097..4c400808485 100644 --- a/src/main/resources/oscarResources_en.properties +++ b/src/main/resources/oscarResources_en.properties @@ -8925,10 +8925,10 @@ oscarReport.RptByExample.MsgLoadQuery=Load Query oscarReport.RptByExample.MsgRunQuery=Run Query oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disabled. Your query has not been run. oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. -oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. +oscarReport.RptByExample.MsgTimeout=The query exceeded the {0}-second time limit and was stopped. oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. -oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. +oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). oscarReport.RptByExample.MsgConfirmDelete=Are you sure you want to delete the selected query? diff --git a/src/main/resources/oscarResources_es.properties b/src/main/resources/oscarResources_es.properties index 37f5596fd17..8eb0bf1d29f 100644 --- a/src/main/resources/oscarResources_es.properties +++ b/src/main/resources/oscarResources_es.properties @@ -6449,10 +6449,10 @@ oscarReport.RptByExample.MsgRunQuery=Ejecutar # TODO: translate Query-by-Example execution messages. oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disabled. Your query has not been run. oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. -oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. +oscarReport.RptByExample.MsgTimeout=The query exceeded the {0}-second time limit and was stopped. oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. -oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. +oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). oscarReport.RptByExample.MsgConfirmDelete=\u00bfEst\u00e1 seguro de que desea eliminar la consulta seleccionada? diff --git a/src/main/resources/oscarResources_fr.properties b/src/main/resources/oscarResources_fr.properties index b9a05ae984e..ad2accfcbd2 100644 --- a/src/main/resources/oscarResources_fr.properties +++ b/src/main/resources/oscarResources_fr.properties @@ -5631,10 +5631,10 @@ oscarReport.RptByExample.MsgRunQuery=Ex\u00e9cuter # TODO: translate Query-by-Example execution messages. oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disabled. Your query has not been run. oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. -oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. +oscarReport.RptByExample.MsgTimeout=The query exceeded the {0}-second time limit and was stopped. oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. -oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. +oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). oscarReport.RptByExample.MsgConfirmDelete=\u00cates-vous s\u00fbr de vouloir supprimer la requ\u00eate s\u00e9lectionn\u00e9e\u00a0? diff --git a/src/main/resources/oscarResources_pl.properties b/src/main/resources/oscarResources_pl.properties index 0323c830d93..525e54925fa 100644 --- a/src/main/resources/oscarResources_pl.properties +++ b/src/main/resources/oscarResources_pl.properties @@ -5867,10 +5867,10 @@ oscarReport.RptByExample.MsgRunQuery=Wykonaj # TODO: translate Query-by-Example execution messages. oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disabled. Your query has not been run. oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. -oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. +oscarReport.RptByExample.MsgTimeout=The query exceeded the {0}-second time limit and was stopped. oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. -oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. +oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). oscarReport.RptByExample.MsgConfirmDelete=Czy na pewno chcesz usun\u0105\u0107 wybran\u0105 kwerend\u0119? diff --git a/src/main/resources/oscarResources_pt_BR.properties b/src/main/resources/oscarResources_pt_BR.properties index b244d0bbde9..3c71c778d19 100644 --- a/src/main/resources/oscarResources_pt_BR.properties +++ b/src/main/resources/oscarResources_pt_BR.properties @@ -7354,10 +7354,10 @@ oscarReport.RptByExample.MsgRunQuery=Executar # TODO: translate Query-by-Example execution messages. oscarReport.RptByExample.MsgDisabled=Direct query execution is currently disabled. Your query has not been run. oscarReport.RptByExample.MsgValidationError=Only one read-only SELECT from the application database is allowed. Comments, UNION, locking, output operations, and prohibited functions are not permitted. -oscarReport.RptByExample.MsgTimeout=The query exceeded the 15-second time limit and was stopped. +oscarReport.RptByExample.MsgTimeout=The query exceeded the {0}-second time limit and was stopped. oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. -oscarReport.RptByExample.MsgResultLimit=Results are limited to {0} rows. +oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). oscarReport.RptByExample.MsgConfirmDelete=Tem certeza de que deseja excluir a consulta selecionada? diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp index fa30f02a8c9..87f5aafcb85 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp @@ -176,7 +176,9 @@ @@ -239,6 +241,7 @@

+

diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java index 542053af9b2..10792eeba3d 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java @@ -41,7 +41,7 @@ class QueryByExampleSqlValidatorTest { @Test @DisplayName("allows one SELECT using application tables, joins, and subqueries") - void shouldAllowReadOnlyApplicationSelects() { + void shouldAllowReadOnlyApplicationSelects_whenSchemaIsConfigured() { assertThatCode(() -> QueryByExampleSqlValidator.validate( "select d.demographic_no from demographic d join provider p on p.provider_no=d.provider_no " + "where exists (select 1 from appointment a where a.demographic_no=d.demographic_no)", @@ -52,7 +52,7 @@ void shouldAllowReadOnlyApplicationSelects() { @Test @DisplayName("does not treat prohibited function names inside string literals as invocations") - void shouldAllowBlockedFunctionNameInsideLiteral() { + void shouldAllowBlockedFunctionNameInsideLiteral_whenFunctionTextIsQuoted() { assertThatCode(() -> QueryByExampleSqlValidator.validate("select 'sleep(1)'", properties)) .doesNotThrowAnyException(); assertThatCode(() -> QueryByExampleSqlValidator.validate( @@ -62,7 +62,7 @@ void shouldAllowBlockedFunctionNameInsideLiteral() { @Test @DisplayName("structurally rejects set-operation SELECTs") - void shouldRejectSetOperationSelects() { + void shouldRejectSetOperationSelects_whenMultipleQueriesAreCombined() { assertThatThrownBy(() -> QueryByExampleSqlValidator.validate( "select demographic_no from demographic union select provider_no from provider", properties)) .isInstanceOf(QueryByExampleValidationException.class) @@ -71,7 +71,7 @@ void shouldRejectSetOperationSelects() { @Test @DisplayName("fails closed when the application schema is not configured") - void shouldRejectMissingApplicationSchema() { + void shouldRejectMissingApplicationSchema_whenSchemaIsUnavailable() { Properties missing = new Properties(); Properties blank = properties(" ?useUnicode=true"); diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java index b529873e80c..0700e53de85 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java @@ -78,7 +78,7 @@ void setUp() throws SQLException { @Test @DisplayName("executes once using a bounded read-only JDBC statement and restores connection state") - void shouldExecuteBoundedReadOnlyQueryAndRestoreConnection() throws SQLException { + void shouldExecuteBoundedReadOnlyQuery_whenConnectionStartsReadOnly() throws SQLException { RptByExampleData.QueryResult result = reportData.execute( "select demographic_no from demographic", properties, "999998"); @@ -96,6 +96,22 @@ void shouldExecuteBoundedReadOnlyQueryAndRestoreConnection() throws SQLException order.verify(connection).close(); } + @Test + @DisplayName("restores a writable connection after a successful query") + void shouldRestoreWritableConnection_whenQuerySucceeds() throws SQLException { + when(connection.isReadOnly()).thenReturn(false); + + reportData.execute("select demographic_no from demographic", properties, "999998"); + + InOrder order = inOrder(connection, statement, resultSet); + order.verify(connection).setReadOnly(true); + order.verify(statement).executeQuery(); + order.verify(resultSet).close(); + order.verify(statement).close(); + order.verify(connection).setReadOnly(false); + order.verify(connection).close(); + } + @Test @DisplayName("preserves a timeout when restoring connection state also fails") void shouldPreserveTimeout_whenReadOnlyRestoreFails() throws SQLException { @@ -131,7 +147,7 @@ void shouldRenderPositionalValues_whenColumnLabelsAreDuplicated() throws SQLExce @Test @DisplayName("rejects unsafe SQL before acquiring a database connection") - void shouldRejectBeforeConnecting() throws SQLException { + void shouldRejectBeforeConnecting_whenSqlIsUnsafe() throws SQLException { assertThatThrownBy(() -> reportData.execute("delete from demographic", properties, "999998")) .isInstanceOf(QueryByExampleValidationException.class); From 5962421139d42f218205547f60a32a77407d1d4c Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 16:17:12 -0400 Subject: [PATCH 06/21] fix(report): bound query by example execution Signed-off-by: Ben Heerema --- .../data/QueryByExampleSqlValidator.java | 141 +++++++++++++--- .../QueryByExampleValidationException.java | 2 + .../carlos/report/data/RptByExampleData.java | 35 ++-- .../carlos/report/data/RptResultStruct.java | 151 +++++++++++++++--- .../report/pageUtil/RptByExample2Action.java | 4 +- .../RptByExamplesAllFavorites2Action.java | 5 +- .../RptByExamplesFavorite2Action.java | 5 +- .../RptViewAllQueryByExamples2Action.java | 5 +- .../resources/oscarResources_en.properties | 1 + .../resources/oscarResources_es.properties | 2 + .../resources/oscarResources_fr.properties | 2 + .../resources/oscarResources_pl.properties | 2 + .../resources/oscarResources_pt_BR.properties | 2 + .../WEB-INF/jsp/oscarReport/RptByExample.jsp | 7 + .../oscarReport/RptByExamplesAllFavorites.jsp | 6 +- .../jsp/oscarReport/RptByExamplesFavorite.jsp | 4 +- .../oscarReport/RptViewAllQueryByExamples.jsp | 6 +- ...ReportActionSecurityMigrationUnitTest.java | 21 ++- .../data/QueryByExampleSqlValidatorTest.java | 24 +++ .../report/data/RptByExampleDataTest.java | 37 ++++- 20 files changed, 383 insertions(+), 79 deletions(-) diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java index 33ca5acafc1..8f26e3cf2d3 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java @@ -22,6 +22,7 @@ package io.github.carlos_emr.carlos.report.data; import java.sql.SQLException; +import java.util.List; import java.util.Locale; import java.util.Properties; import java.util.Set; @@ -29,8 +30,13 @@ import io.github.carlos_emr.carlos.db.LegacyJdbcQuery; import net.sf.jsqlparser.JSQLParserException; +import net.sf.jsqlparser.expression.AnalyticExpression; +import net.sf.jsqlparser.expression.Function; +import net.sf.jsqlparser.expression.NextValExpression; +import net.sf.jsqlparser.expression.UserVariable; import net.sf.jsqlparser.parser.CCJSqlParserUtil; import net.sf.jsqlparser.statement.Statement; +import net.sf.jsqlparser.statement.select.PlainSelect; import net.sf.jsqlparser.statement.select.Select; import net.sf.jsqlparser.statement.select.SetOperationList; import net.sf.jsqlparser.util.TablesNamesFinder; @@ -46,13 +52,39 @@ * @since 2026-08-06 */ public final class QueryByExampleSqlValidator { + public static final int MAX_SQL_CHARACTERS = 16_384; + private static final Pattern LOCKING_SELECT = Pattern.compile( "\\bfor\\s+(?:update|share)\\b|\\block\\s+in\\s+share\\s+mode\\b", Pattern.CASE_INSENSITIVE); - private static final Pattern OUTPUT_OPERATION = Pattern.compile("\\binto\\b", Pattern.CASE_INSENSITIVE); - private static final Pattern BLOCKED_FUNCTION = Pattern.compile( - "(? ALLOWED_FUNCTIONS = Set.of( + "abs", "acos", "adddate", "addtime", "ascii", "asin", "atan", "atan2", "avg", + "bin", "bit_and", "bit_length", "bit_or", "bit_xor", "ceil", "ceiling", "char_length", + "character_length", "coalesce", "concat", "concat_ws", "conv", "convert_tz", "cos", "cot", + "count", "crc32", "curdate", "current_date", "current_time", "current_timestamp", "curtime", + "date", "date_add", "date_format", "date_sub", "datediff", "day", "dayname", "dayofmonth", + "dayofweek", "dayofyear", "degrees", "elt", "exp", "field", "find_in_set", "floor", "format", + "from_base64", "from_days", "from_unixtime", "get_format", "greatest", "hex", "hour", "if", + "ifnull", "instr", "lcase", "least", "left", "length", "ln", "localtime", "localtimestamp", + "locate", "log", "log10", "log2", "lower", "lpad", "ltrim", "makedate", "maketime", "max", + "md5", "microsecond", "mid", "min", "minute", "mod", "month", "monthname", "now", "nullif", + "oct", "octet_length", "ord", "period_add", "period_diff", "pi", "pow", "power", "quarter", + "quote", "radians", "rand", "repeat", "replace", "reverse", "right", "round", "rpad", "rtrim", + "sec_to_time", "second", "sha", "sha1", "sha2", "sign", "sin", "soundex", "space", "sqrt", + "std", "stddev", "stddev_pop", "stddev_samp", "str_to_date", "strcmp", "subdate", "substr", + "substring", "substring_index", "subtime", "sum", "sysdate", "tan", "time", "time_format", + "time_to_sec", "timediff", "timestamp", "timestampadd", "timestampdiff", "to_base64", "to_days", + "trim", "truncate", "ucase", "unhex", "unix_timestamp", "upper", "utc_date", "utc_time", + "utc_timestamp", "variance", "var_pop", "var_samp", "week", "weekday", "weekofyear", "year", + "yearweek", "cume_dist", "dense_rank", "first_value", "lag", "last_value", "lead", "nth_value", + "ntile", "percent_rank", "rank", "row_number"); private QueryByExampleSqlValidator() { } @@ -68,7 +100,13 @@ private QueryByExampleSqlValidator() { */ public static LegacyJdbcQuery.TrustedSql validate(String sql, Properties properties) throws QueryByExampleValidationException { - if (sql == null || sql.isBlank()) { + if (sql == null) { + throw new QueryByExampleValidationException("SQL query must not be empty"); + } + if (sql.length() > MAX_SQL_CHARACTERS) { + throw new QueryByExampleValidationException("SQL query exceeds the allowed length"); + } + if (sql.isBlank()) { throw new QueryByExampleValidationException("SQL query must not be empty"); } @@ -83,15 +121,14 @@ public static LegacyJdbcQuery.TrustedSql validate(String sql, Properties propert throw new QueryByExampleValidationException("Only one SELECT statement is allowed"); } - String sqlWithoutStringLiterals = stripStringLiterals(sql); - if (LOCKING_SELECT.matcher(sqlWithoutStringLiterals).find()) { + String sqlWithoutQuotedSections = stripQuotedSections(sql); + if (LOCKING_SELECT.matcher(sqlWithoutQuotedSections).find()) { throw new QueryByExampleValidationException("Locking SELECT statements are not allowed"); } - if (OUTPUT_OPERATION.matcher(sqlWithoutStringLiterals).find()) { + if (OUTPUT_OR_PROCEDURE_OPERATION.matcher(sqlWithoutQuotedSections).find()) { throw new QueryByExampleValidationException("SELECT output operations are not allowed"); } - rejectBlockedFunctions(sqlWithoutStringLiterals); - rejectOtherSchemas(statement, applicationSchema(properties)); + rejectUnsafeExpressionsAndOtherSchemas(statement, applicationSchema(properties)); try { return LegacyJdbcQuery.trustedSelectSql(sql); } catch (SQLException e) { @@ -111,14 +148,24 @@ static String applicationSchema(Properties properties) throws QueryByExampleVali return unquoteIdentifier(schema); } - private static void rejectOtherSchemas(Statement statement, String applicationSchema) + private static void rejectUnsafeExpressionsAndOtherSchemas(Statement statement, String applicationSchema) throws QueryByExampleValidationException { Set tables; + SqlSafetyVisitor visitor = new SqlSafetyVisitor(); try { - tables = new TablesNamesFinder().getTables(statement); + tables = visitor.getTables(statement); } catch (RuntimeException e) { throw new QueryByExampleValidationException("The query table references could not be validated", e); } + if (visitor.hasOutputOperation()) { + throw new QueryByExampleValidationException("SELECT output operations are not allowed"); + } + if (visitor.hasVariables()) { + throw new QueryByExampleValidationException("Session and system variables are not allowed"); + } + if (!visitor.disallowedFunctions().isEmpty()) { + throw new QueryByExampleValidationException("The query uses a function outside the allowed set"); + } for (String table : tables) { String normalizedTable = unquoteIdentifier(table); int lastDot = normalizedTable.lastIndexOf('.'); @@ -131,26 +178,20 @@ private static void rejectOtherSchemas(Statement statement, String applicationSc } } - private static void rejectBlockedFunctions(String sql) throws QueryByExampleValidationException { - if (BLOCKED_FUNCTION.matcher(sql).find()) { - throw new QueryByExampleValidationException("The query uses a prohibited database function"); - } - } - - private static String stripStringLiterals(String sql) { + private static String stripQuotedSections(String sql) { StringBuilder stripped = new StringBuilder(sql.length()); char quote = '\0'; for (int i = 0; i < sql.length(); i++) { char current = sql.charAt(i); char next = i + 1 < sql.length() ? sql.charAt(i + 1) : '\0'; if (quote == '\0') { - if (current == '\'' || current == '"') { + if (current == '\'' || current == '"' || current == '`') { quote = current; stripped.append(' '); } else { stripped.append(current); } - } else if (current == '\\' && next != '\0') { + } else if (quote != '`' && current == '\\' && next != '\0') { stripped.append(" "); i++; } else if (current == quote && next == quote) { @@ -173,4 +214,62 @@ private static String unquoteIdentifier(String identifier) { private static String canonicalIdentifier(String identifier) { return identifier.toLowerCase(Locale.ROOT); } + + private static final class SqlSafetyVisitor extends TablesNamesFinder { + private final Set disallowedFunctions = new java.util.HashSet<>(); + private boolean variables; + private boolean outputOperation; + + @Override + public Void visit(Function function, S context) { + List nameParts = function.getMultipartName(); + String functionName = canonicalIdentifier(unquoteIdentifier(function.getName())); + if (nameParts == null || nameParts.size() != 1 || !ALLOWED_FUNCTIONS.contains(functionName)) { + disallowedFunctions.add(functionName); + } + return super.visit(function, context); + } + + @Override + public Void visit(AnalyticExpression function, S context) { + String functionName = canonicalIdentifier(unquoteIdentifier(function.getName())); + if (!ALLOWED_FUNCTIONS.contains(functionName)) { + disallowedFunctions.add(functionName); + } + return super.visit(function, context); + } + + @Override + public Void visit(UserVariable variable, S context) { + variables = true; + return super.visit(variable, context); + } + + @Override + public Void visit(NextValExpression sequence, S context) { + variables = true; + return super.visit(sequence, context); + } + + @Override + public Void visit(PlainSelect select, S context) { + if ((select.getIntoTables() != null && !select.getIntoTables().isEmpty()) + || select.getIntoTempTable() != null) { + outputOperation = true; + } + return super.visit(select, context); + } + + Set disallowedFunctions() { + return disallowedFunctions; + } + + boolean hasVariables() { + return variables; + } + + boolean hasOutputOperation() { + return outputOperation; + } + } } diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleValidationException.java b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleValidationException.java index 591f78ac9aa..ee7dc42072c 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleValidationException.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleValidationException.java @@ -29,6 +29,8 @@ * @since 2026-08-06 */ public class QueryByExampleValidationException extends SQLException { + private static final long serialVersionUID = 1L; + /** * Creates a validation exception with a user-safe diagnostic message. * diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java b/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java index 2d85959cda9..1064736d5ae 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java @@ -50,6 +50,7 @@ */ public class RptByExampleData { public static final int MAX_ROWS = 1_000; + public static final int MAX_OUTPUT_CHARACTERS = 1_000_000; public static final int QUERY_TIMEOUT_SECONDS = 15; @FunctionalInterface @@ -57,7 +58,7 @@ interface ConnectionProvider { Connection getConnection() throws SQLException; } - public record QueryResult(String html, int rowCount) { + public record QueryResult(String html, int rowCount, boolean truncated, long durationMillis) { } private final ConnectionProvider connectionProvider; @@ -71,12 +72,8 @@ public RptByExampleData() { } @SuppressFBWarnings( - value = { - "SQL_INJECTION_JDBC", - "SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING", - "THROWS_METHOD_THROWS_RUNTIMEEXCEPTION" - }, - justification = "Validated dynamic SQL is intentional; runtime failures are audited and handled by the action") + value = "THROWS_METHOD_THROWS_RUNTIMEEXCEPTION", + justification = "Runtime failures are audited and handled by the action") public QueryResult execute(String sql, Properties properties, String providerNo) throws SQLException { long startedAt = System.nanoTime(); String outcome = "failed"; @@ -89,15 +86,14 @@ public QueryResult execute(String sql, Properties properties, String providerNo) Exception executionFailure = null; try { connection.setReadOnly(true); - // codeql[java/sql-injection] -- TrustedSql is created only after structural SELECT validation. - try (PreparedStatement statement = connection.prepareStatement(trustedSql.sql(), - ResultSet.TYPE_FORWARD_ONLY, ResultSet.CONCUR_READ_ONLY)) { // nosemgrep: java.lang.security.audit.formatted-sql-string-deepsemgrep.formatted-sql-string-deepsemgrep -- validated TrustedSql boundary + try (PreparedStatement statement = prepareValidatedStatement(connection, trustedSql)) { statement.setMaxRows(MAX_ROWS); statement.setQueryTimeout(QUERY_TIMEOUT_SECONDS); - try (ResultSet resultSet = statement.executeQuery()) { // NOSONAR javasecurity:S3649 -- validated, read-only SELECT boundary - RptResultStruct.StructuredResult structured = RptResultStruct.getStructureWithCount(resultSet); + try (ResultSet resultSet = statement.executeQuery()) { + RptResultStruct.StructuredResult structured = RptResultStruct.getStructureWithCount( + resultSet, MAX_OUTPUT_CHARACTERS); rowCount = structured.rowCount(); - queryResult = new QueryResult(structured.html(), rowCount); + queryResult = new QueryResult(structured.html(), rowCount, structured.truncated(), 0); } } } catch (SQLException | RuntimeException e) { @@ -116,7 +112,8 @@ public QueryResult execute(String sql, Properties properties, String providerNo) } } outcome = "success"; - return queryResult; + return new QueryResult(queryResult.html(), queryResult.rowCount(), queryResult.truncated(), + elapsedMillis(startedAt)); } catch (QueryByExampleValidationException e) { outcome = "rejected"; throw e; @@ -134,6 +131,16 @@ public QueryResult execute(String sql, Properties properties, String providerNo) } } + @SuppressFBWarnings( + value = {"SQL_INJECTION_JDBC", "SQL_PREPARED_STATEMENT_GENERATED_FROM_NONCONSTANT_STRING"}, + justification = "This narrow sink accepts only TrustedSql created by structural SELECT validation") + private static PreparedStatement prepareValidatedStatement(Connection connection, + LegacyJdbcQuery.TrustedSql trustedSql) throws SQLException { + // codeql[java/sql-injection] -- TrustedSql is created only after structural SELECT validation. + return connection.prepareStatement(trustedSql.sql(), ResultSet.TYPE_FORWARD_ONLY, + ResultSet.CONCUR_READ_ONLY); // nosemgrep: java.lang.security.audit.formatted-sql-string-deepsemgrep.formatted-sql-string-deepsemgrep -- validated TrustedSql boundary + } + public static void audit(String providerNo, String sql, long durationMillis, int rowCount, String outcome) { String query = sql == null ? "" : sql; String queryHash = queryHash(query); diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java b/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java index f75a0a7c9b0..8117f0b9eaf 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java @@ -40,13 +40,18 @@ import io.github.carlos_emr.Misc; import org.owasp.encoder.Encode; +import java.io.IOException; +import java.io.Reader; import java.sql.ResultSet; import java.sql.ResultSetMetaData; import java.sql.SQLException; public class RptResultStruct { - public record StructuredResult(String html, int rowCount) { + private static final int MIN_OUTPUT_CHARACTERS = 64; + private static final int CLOSING_MARKUP_RESERVE = 32; + + public record StructuredResult(String html, int rowCount, boolean truncated) { } public static String getStructure(ResultSet rs) throws SQLException { @@ -61,40 +66,146 @@ public static String getStructure(ResultSet rs) throws SQLException { * @throws SQLException if the result set cannot be read */ public static StructuredResult getStructureWithCount(ResultSet rs) throws SQLException { + return getStructureWithCount(rs, Integer.MAX_VALUE); + } + + /** + * Generates an encoded HTML table within a fixed output budget. + * + * @param rs result set positioned before its first row + * @param maxOutputCharacters maximum number of rendered HTML characters + * @return encoded table markup, rendered row count, and whether output was truncated + * @throws SQLException if the result set cannot be read + */ + public static StructuredResult getStructureWithCount(ResultSet rs, int maxOutputCharacters) throws SQLException { + if (maxOutputCharacters < MIN_OUTPUT_CHARACTERS) { + throw new IllegalArgumentException("HTML output limit is too small"); + } // assuming multiple rows in rs - StringBuilder sb = new StringBuilder(); + LimitedHtmlBuilder html = new LimitedHtmlBuilder(maxOutputCharacters); ResultSetMetaData rsmd = rs.getMetaData(); int columns = rsmd.getColumnCount(); String rowColor = "rowColor1"; - String[] columnLabels = new String[columns]; - sb.append(""); + html.appendMarkup("
"); for (int i = 0; i < columns; i++) { // for each column in result set - columnLabels[i] = rsmd.getColumnLabel(i + 1); - // put names in array - // use i+1 or else you're going to get an exception - // insert headings for table - sb.append(""); + if (!html.appendMarkup("
"); - sb.append(Encode.forHtml(columnLabels[i])); - sb.append("") + || !html.appendEncoded(rsmd.getColumnLabel(i + 1))) { + html.appendClosingMarkup("
"); + return new StructuredResult(html.toString(), 0, true); + } + if (!html.appendMarkup("")) { + html.appendClosingMarkup(""); + return new StructuredResult(html.toString(), 0, true); + } } int rowCount = 0; - while (rs.next()) { + boolean stopRendering = false; + while (!stopRendering && rs.next()) { rowCount++; - sb.append(""); + if (!html.appendMarkup("")) { + break; + } for (int j = 0; j < columns; j++) { - sb.append(""); - sb.append(Encode.forHtml(Misc.getString(rs, j + 1))); - sb.append(""); - + if (!html.appendMarkup("")) { + stopRendering = true; + break; + } + try (Reader value = rs.getCharacterStream(j + 1)) { + if (value != null && !html.appendEncoded(value)) { + stopRendering = true; + } + } catch (IOException e) { + throw new SQLException("Could not render query result", e); + } + if (stopRendering) { + html.appendClosingMarkup(""); + break; + } + if (!html.appendMarkup("")) { + html.appendClosingMarkup(""); + stopRendering = true; + break; + } } rowColor = rowColor.equals("rowColor1") ? "rowColor2" : "rowColor1"; - sb.append(""); + if (stopRendering || !html.appendMarkup("")) { + html.appendClosingMarkup(""); + stopRendering = true; + } + } + html.appendClosingMarkup(""); + return new StructuredResult(html.toString(), rowCount, html.isTruncated()); + } + + private static final class LimitedHtmlBuilder { + private static final int READ_BUFFER_SIZE = 2_048; + + private final StringBuilder html; + private final int contentLimit; + private boolean truncated; + + LimitedHtmlBuilder(int maxOutputCharacters) { + contentLimit = maxOutputCharacters - CLOSING_MARKUP_RESERVE; + html = new StringBuilder(Math.min(maxOutputCharacters, 8_192)); + } + + boolean appendMarkup(String markup) { + if (markup.length() > remaining()) { + truncated = true; + return false; + } + html.append(markup); + return true; + } + + boolean appendEncoded(String value) { + return appendEncodedChunk(Encode.forHtml(value == null ? "" : value)); + } + + boolean appendEncoded(Reader value) throws IOException { + char[] buffer = new char[READ_BUFFER_SIZE]; + int read; + while ((read = value.read(buffer)) != -1) { + if (!appendEncodedChunk(Encode.forHtml(new String(buffer, 0, read)))) { + return false; + } + } + return true; + } + + private boolean appendEncodedChunk(String encoded) { + int remaining = remaining(); + if (encoded.length() <= remaining) { + html.append(encoded); + return true; + } + if (remaining > 0) { + int contentCharacters = Math.max(0, remaining - 1); + html.append(encoded, 0, contentCharacters).append('\u2026'); + } + truncated = true; + return false; + } + + void appendClosingMarkup(String markup) { + html.append(markup); + } + + boolean isTruncated() { + return truncated; + } + + private int remaining() { + return contentLimit - html.length(); + } + + @Override + public String toString() { + return html.toString(); } - sb.append(""); - return new StructuredResult(sb.toString(), rowCount); } //improvement over getStructure() - changed CSS naming conventions, added enterspaces for cleaner html, diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java index c7008c1b05b..4e47b4a7271 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java @@ -141,11 +141,13 @@ public String execute() request.setAttribute("results", result.html()); request.setAttribute("resultRowCount", result.rowCount()); request.setAttribute("resultLimit", RptByExampleData.MAX_ROWS); + request.setAttribute("resultTruncated", result.truncated()); + request.setAttribute("resultCharacterLimit", RptByExampleData.MAX_OUTPUT_CHARACTERS); try { write2Database(sql, providerNo); } catch (RuntimeException e) { request.setAttribute("queryHistoryError", true); - RptByExampleData.audit(providerNo, sql, 0, result.rowCount(), "history_failed"); + RptByExampleData.audit(providerNo, sql, result.durationMillis(), result.rowCount(), "history_failed"); } return SUCCESS; diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesAllFavorites2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesAllFavorites2Action.java index b8f88146656..78deca29999 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesAllFavorites2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesAllFavorites2Action.java @@ -55,8 +55,9 @@ public class RptByExamplesAllFavorites2Action extends ActionSupport { public String execute() throws ServletException, IOException { LoggedInInfo loggedInInfo = LoggedInInfo.getLoggedInInfoFromSession(request); - if (!securityInfoManager.hasPrivilege(loggedInInfo, "_report", "r", null)) { - throw new SecurityException("missing required sec object (_report)"); + if (!securityInfoManager.hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null) + && !securityInfoManager.hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null)) { + throw new SecurityException("missing required sec object (_admin or _report)"); } diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java index 87a1147c931..406bd54bfab 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java @@ -64,8 +64,9 @@ public class RptByExamplesFavorite2Action extends ActionSupport { @SuppressFBWarnings(value = "IMPROPER_UNICODE", justification = "case-insensitive comparison of an internal/domain value (status/flag/enum/MIME/code); not a security or authorization decision") public String execute() throws ServletException, IOException { LoggedInInfo loggedInInfo = LoggedInInfo.getLoggedInInfoFromSession(request); - if (!securityInfoManager.hasPrivilege(loggedInInfo, "_report", "r", null)) { - throw new SecurityException("missing required sec object (_report)"); + if (!securityInfoManager.hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null) + && !securityInfoManager.hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null)) { + throw new SecurityException("missing required sec object (_admin or _report)"); } String providerNo = (String) request.getSession().getAttribute("user"); diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptViewAllQueryByExamples2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptViewAllQueryByExamples2Action.java index ee213fcf534..07afe049d99 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptViewAllQueryByExamples2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptViewAllQueryByExamples2Action.java @@ -59,8 +59,9 @@ public class RptViewAllQueryByExamples2Action extends ActionSupport { public String execute() throws ServletException, IOException { LoggedInInfo loggedInInfo = LoggedInInfo.getLoggedInInfoFromSession(request); - if (!securityInfoManager.hasPrivilege(loggedInInfo, "_report", "r", null)) { - throw new SecurityException("missing required sec object (_report)"); + if (!securityInfoManager.hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null) + && !securityInfoManager.hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null)) { + throw new SecurityException("missing required sec object (_admin or _report)"); } RptByExampleQueryBeanHandler hd = new RptByExampleQueryBeanHandler(startDate, endDate); diff --git a/src/main/resources/oscarResources_en.properties b/src/main/resources/oscarResources_en.properties index 4c400808485..68491f6fb86 100644 --- a/src/main/resources/oscarResources_en.properties +++ b/src/main/resources/oscarResources_en.properties @@ -8929,6 +8929,7 @@ oscarReport.RptByExample.MsgTimeout=The query exceeded the {0}-second time limit oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). +oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. oscarReport.RptByExample.MsgConfirmDelete=Are you sure you want to delete the selected query? diff --git a/src/main/resources/oscarResources_es.properties b/src/main/resources/oscarResources_es.properties index 8eb0bf1d29f..df6664c50c6 100644 --- a/src/main/resources/oscarResources_es.properties +++ b/src/main/resources/oscarResources_es.properties @@ -6453,6 +6453,8 @@ oscarReport.RptByExample.MsgTimeout=The query exceeded the {0}-second time limit oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). +# TODO: translate +oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. oscarReport.RptByExample.MsgConfirmDelete=\u00bfEst\u00e1 seguro de que desea eliminar la consulta seleccionada? diff --git a/src/main/resources/oscarResources_fr.properties b/src/main/resources/oscarResources_fr.properties index ad2accfcbd2..ff2d00431b9 100644 --- a/src/main/resources/oscarResources_fr.properties +++ b/src/main/resources/oscarResources_fr.properties @@ -5635,6 +5635,8 @@ oscarReport.RptByExample.MsgTimeout=The query exceeded the {0}-second time limit oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). +# TODO: translate +oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. oscarReport.RptByExample.MsgConfirmDelete=\u00cates-vous s\u00fbr de vouloir supprimer la requ\u00eate s\u00e9lectionn\u00e9e\u00a0? diff --git a/src/main/resources/oscarResources_pl.properties b/src/main/resources/oscarResources_pl.properties index 525e54925fa..2f9e45a7125 100644 --- a/src/main/resources/oscarResources_pl.properties +++ b/src/main/resources/oscarResources_pl.properties @@ -5871,6 +5871,8 @@ oscarReport.RptByExample.MsgTimeout=The query exceeded the {0}-second time limit oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). +# TODO: translate +oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. oscarReport.RptByExample.MsgConfirmDelete=Czy na pewno chcesz usun\u0105\u0107 wybran\u0105 kwerend\u0119? diff --git a/src/main/resources/oscarResources_pt_BR.properties b/src/main/resources/oscarResources_pt_BR.properties index 3c71c778d19..e5dfa7b49e3 100644 --- a/src/main/resources/oscarResources_pt_BR.properties +++ b/src/main/resources/oscarResources_pt_BR.properties @@ -7358,6 +7358,8 @@ oscarReport.RptByExample.MsgTimeout=The query exceeded the {0}-second time limit oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Check the query and try again. oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). +# TODO: translate +oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. oscarReport.RptByExample.MsgConfirmDelete=Tem certeza de que deseja excluir a consulta selecionada? diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp index 87f5aafcb85..cea4838b075 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp @@ -245,6 +245,13 @@

+ + + ${results}
diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp index 4c0663a02ed..67f6cbd2257 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp @@ -47,7 +47,7 @@ (items expose: id, queryName, query) Security: - - Requires _report or _admin.reporting read privilege + - Requires _report or _admin read privilege - CSRF token auto-injected by CsrfGuardScriptInjectionFilter @since 2001-2002 @@ -61,9 +61,9 @@ String roleName$ = session.getAttribute("userrole") + "," + session.getAttribute("user"); boolean authed = true; %> - + <%authed = false; %> - <%response.sendRedirect(request.getContextPath() + "/securityError?type=_report&type=_admin.reporting");%> + <%response.sendRedirect(request.getContextPath() + "/securityError?type=_report&type=_admin");%> <% if (!authed) { diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp index 483f7cecf8a..aa5b7fe6685 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp @@ -34,9 +34,9 @@ String roleName$ = (String) session.getAttribute("userrole") + "," + (String) session.getAttribute("user"); boolean authed = true; %> - + <%authed = false; %> - <%response.sendRedirect(request.getContextPath() + "/securityError?type=_report&type=_admin.reporting");%> + <%response.sendRedirect(request.getContextPath() + "/securityError?type=_report&type=_admin");%> <% if (!authed) { diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptViewAllQueryByExamples.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptViewAllQueryByExamples.jsp index 7a94d7d5648..b012778d5e8 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptViewAllQueryByExamples.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptViewAllQueryByExamples.jsp @@ -48,7 +48,7 @@ - endDate — End date filter currently applied (String) Security: - - Requires _report or _admin.reporting read privilege + - Requires _report or _admin read privilege - CSRF token auto-injected by CsrfGuardScriptInjectionFilter @since 2001-2002 @@ -62,9 +62,9 @@ String roleName$ = session.getAttribute("userrole") + "," + session.getAttribute("user"); boolean authed = true; %> - + <%authed = false; %> - <%response.sendRedirect(request.getContextPath() + "/securityError?type=_report&type=_admin.reporting");%> + <%response.sendRedirect(request.getContextPath() + "/securityError?type=_report&type=_admin");%> <% if (!authed) { diff --git a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java index e44ea72a6fe..3a8d88c0797 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java @@ -31,6 +31,9 @@ import io.github.carlos_emr.carlos.managers.SecurityInfoManager; import io.github.carlos_emr.carlos.report.data.RptByExampleData; import io.github.carlos_emr.carlos.report.pageUtil.RptByExample2Action; +import io.github.carlos_emr.carlos.report.pageUtil.RptByExamplesAllFavorites2Action; +import io.github.carlos_emr.carlos.report.pageUtil.RptByExamplesFavorite2Action; +import io.github.carlos_emr.carlos.report.pageUtil.RptViewAllQueryByExamples2Action; import io.github.carlos_emr.carlos.report.reportByTemplate.ReportFactory; import io.github.carlos_emr.carlos.report.reportByTemplate.ReportManager; import io.github.carlos_emr.carlos.report.reportByTemplate.Reporter; @@ -136,14 +139,17 @@ void shouldRequireAdminOrReportReadPrivilege_forMigratedActions() { when(securityInfoManager.hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null)).thenReturn(false); assertMissingPrivilegeFails(new RptByExample2Action()); + assertMissingPrivilegeFails(new RptViewAllQueryByExamples2Action()); + assertMissingPrivilegeFails(new RptByExamplesAllFavorites2Action()); + assertMissingPrivilegeFails(new RptByExamplesFavorite2Action()); assertMissingPrivilegeFails(new ExportTemplate2Action()); assertMissingPrivilegeFails(new GenerateOutFiles2Action()); assertMissingPrivilegeFails(new GenerateReport2Action()); assertMissingPrivilegeFails(new UploadTemplates2Action()); - verify(securityInfoManager, times(5)) + verify(securityInfoManager, times(8)) .hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null); - verify(securityInfoManager, times(5)) + verify(securityInfoManager, times(8)) .hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null); } @@ -155,7 +161,7 @@ void shouldSkipReportReadPrivilege_whenAdminReadPrivilegeAllowsAccess() throws E assertAuthorizedMigrationGateAllowsActionBody(); - verify(securityInfoManager, times(5)) + verify(securityInfoManager, times(8)) .hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null); verify(securityInfoManager, never()) .hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null); @@ -170,9 +176,9 @@ void shouldAllowAccess_whenReportReadPrivilegeAllowsAccess() throws Exception { assertAuthorizedMigrationGateAllowsActionBody(); - verify(securityInfoManager, times(5)) + verify(securityInfoManager, times(8)) .hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null); - verify(securityInfoManager, times(5)) + verify(securityInfoManager, times(8)) .hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null); } @@ -222,7 +228,7 @@ void shouldKeepResults_whenQueryHistorySaveFails() throws Exception { RptByExampleData.class, (mock, context) -> when(mock.execute( eq("select demographic_no from demographic"), any(Properties.class), eq("999998"))) - .thenReturn(new RptByExampleData.QueryResult("
", 1)))) { + .thenReturn(new RptByExampleData.QueryResult("
", 1, false, 23)))) { properties.setProperty(RptByExample2Action.ENABLED_PROPERTY, " yes "); assertThat(action.execute()).isEqualTo(ActionSupport.SUCCESS); assertThat(reportData.constructed()).hasSize(1); @@ -280,6 +286,9 @@ private void assertMissingPrivilegeFails(ActionSupport action) { private void assertAuthorizedMigrationGateAllowsActionBody() throws Exception { assertThat(new RptByExample2Action().execute()).isEqualTo(ActionSupport.SUCCESS); + assertThat(new RptViewAllQueryByExamples2Action().execute()).isEqualTo(ActionSupport.SUCCESS); + assertThat(new RptByExamplesAllFavorites2Action().execute()).isEqualTo(ActionSupport.SUCCESS); + assertThat(new RptByExamplesFavorite2Action().execute()).isEqualTo(ActionSupport.SUCCESS); request.setParameter("templateid", "template-1"); assertThat(new ExportTemplate2Action().execute()).isEqualTo(ActionSupport.SUCCESS); diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java index 10792eeba3d..3630f664f7e 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java @@ -48,6 +48,14 @@ void shouldAllowReadOnlyApplicationSelects_whenSchemaIsConfigured() { properties)).doesNotThrowAnyException(); assertThatCode(() -> QueryByExampleSqlValidator.validate( "select * from `oscar_mcmaster`.`demographic`", properties)).doesNotThrowAnyException(); + assertThatCode(() -> QueryByExampleSqlValidator.validate( + "select count(*), date_format(date_of_birth, '%Y') from demographic", properties)) + .doesNotThrowAnyException(); + assertThatCode(() -> QueryByExampleSqlValidator.validate( + "select row_number() over (order by demographic_no) from demographic", properties)) + .doesNotThrowAnyException(); + assertThatCode(() -> QueryByExampleSqlValidator.validate( + "select `into` from demographic", properties)).doesNotThrowAnyException(); } @Test @@ -81,6 +89,16 @@ void shouldRejectMissingApplicationSchema_whenSchemaIsUnavailable() { .isInstanceOf(QueryByExampleValidationException.class); } + @Test + @DisplayName("rejects over-limit SQL before parsing") + void shouldRejectOverLimitSql_whenSubmissionIsTooLarge() { + String sql = "select '" + "x".repeat(QueryByExampleSqlValidator.MAX_SQL_CHARACTERS) + "'"; + + assertThatThrownBy(() -> QueryByExampleSqlValidator.validate(sql, properties)) + .isInstanceOf(QueryByExampleValidationException.class) + .hasMessage("SQL query exceeds the allowed length"); + } + @ParameterizedTest(name = "rejects: {0}") @MethodSource("unsafeQueries") @DisplayName("rejects unsafe or out-of-scope SQL") @@ -105,6 +123,12 @@ private static Stream unsafeQueries() { "select release_lock('qbe')", "select is_free_lock('qbe')", "select load_file('/etc/passwd')", + "select custom_reporting_udf(demographic_no) from demographic", + "select oscar_mcmaster.custom_reporting_udf(demographic_no) from demographic", + "select custom_reporting_udf(demographic_no) over () from demographic", + "select @query_by_example_variable", + "select @@version", + "select next value for report_sequence", "select * from demographic for update", "select * from demographic for share", "select demographic_no into @number from demographic"); diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java index 0700e53de85..be8e8bd7b98 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java @@ -30,6 +30,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import java.io.StringReader; import java.sql.Connection; import java.sql.PreparedStatement; import java.sql.ResultSet; @@ -73,7 +74,7 @@ void setUp() throws SQLException { when(metadata.getColumnCount()).thenReturn(1); when(metadata.getColumnLabel(1)).thenReturn("demographic_no"); when(resultSet.next()).thenReturn(true, false); - when(resultSet.getString(1)).thenReturn("42"); + when(resultSet.getCharacterStream(1)).thenAnswer(ignored -> new StringReader("42")); } @Test @@ -130,14 +131,31 @@ void shouldPreserveTimeout_whenReadOnlyRestoreFails() throws SQLException { verify(connection).close(); } + @Test + @DisplayName("preserves query failure when restoring an originally read-only connection also fails") + void shouldPreserveQueryFailure_whenOriginallyReadOnlyRestoreFails() throws SQLException { + SQLException queryFailure = new SQLException("query failed"); + SQLException restoreFailure = new SQLException("restore failed"); + when(statement.executeQuery()).thenThrow(queryFailure); + org.mockito.Mockito.doNothing().doThrow(restoreFailure).when(connection).setReadOnly(true); + + assertThatThrownBy(() -> reportData.execute( + "select demographic_no from demographic", properties, "999998")) + .isSameAs(queryFailure) + .satisfies(thrown -> assertThat(thrown.getSuppressed()).containsExactly(restoreFailure)); + + verify(connection, org.mockito.Mockito.times(2)).setReadOnly(true); + verify(connection).close(); + } + @Test @DisplayName("renders duplicate column labels using their positional values") void shouldRenderPositionalValues_whenColumnLabelsAreDuplicated() throws SQLException { when(metadata.getColumnCount()).thenReturn(2); when(metadata.getColumnLabel(1)).thenReturn("id"); when(metadata.getColumnLabel(2)).thenReturn("id"); - when(resultSet.getString(1)).thenReturn("first"); - when(resultSet.getString(2)).thenReturn("second"); + when(resultSet.getCharacterStream(1)).thenAnswer(ignored -> new StringReader("first")); + when(resultSet.getCharacterStream(2)).thenAnswer(ignored -> new StringReader("second")); RptResultStruct.StructuredResult result = RptResultStruct.getStructureWithCount(resultSet); @@ -145,6 +163,19 @@ void shouldRenderPositionalValues_whenColumnLabelsAreDuplicated() throws SQLExce assertThat(result.rowCount()).isEqualTo(1); } + @Test + @DisplayName("truncates encoded result output at the configured character budget") + void shouldTruncateEncodedOutput_whenCellExceedsCharacterBudget() throws SQLException { + when(resultSet.getCharacterStream(1)).thenAnswer(ignored -> new StringReader("<".repeat(1_000))); + + RptResultStruct.StructuredResult result = RptResultStruct.getStructureWithCount(resultSet, 128); + + assertThat(result.truncated()).isTrue(); + assertThat(result.rowCount()).isEqualTo(1); + assertThat(result.html()).hasSizeLessThanOrEqualTo(128).endsWith(""); + assertThat(result.html()).contains("<"); + } + @Test @DisplayName("rejects unsafe SQL before acquiring a database connection") void shouldRejectBeforeConnecting_whenSqlIsUnsafe() throws SQLException { From 3f20ffb9539ac995931e5592b502dabc4bc4bdbc Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 16:52:58 -0400 Subject: [PATCH 07/21] fix(report): close query review edge cases Signed-off-by: Ben Heerema --- .../data/QueryByExampleSqlValidator.java | 41 +++++++++++-- .../carlos/report/data/RptByExampleData.java | 12 ++-- .../carlos/report/data/RptResultStruct.java | 42 +++++++++++--- .../report/pageUtil/RptByExample2Action.java | 1 + .../RptByExamplesFavorite2Action.java | 4 ++ .../resources/oscarResources_en.properties | 1 + .../resources/oscarResources_es.properties | 2 + .../resources/oscarResources_fr.properties | 2 + .../resources/oscarResources_pl.properties | 2 + .../resources/oscarResources_pt_BR.properties | 2 + .../WEB-INF/jsp/oscarReport/RptByExample.jsp | 7 +++ .../jsp/oscarReport/RptByExamplesFavorite.jsp | 18 ++++++ ...ReportActionSecurityMigrationUnitTest.java | 58 +++++++++++++++++-- .../data/QueryByExampleSqlValidatorTest.java | 2 + .../report/data/RptByExampleDataTest.java | 25 +++++++- 15 files changed, 196 insertions(+), 23 deletions(-) diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java index 8f26e3cf2d3..9e70339fe4b 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java @@ -36,6 +36,7 @@ import net.sf.jsqlparser.expression.UserVariable; import net.sf.jsqlparser.parser.CCJSqlParserUtil; import net.sf.jsqlparser.statement.Statement; +import net.sf.jsqlparser.statement.select.ParenthesedSelect; import net.sf.jsqlparser.statement.select.PlainSelect; import net.sf.jsqlparser.statement.select.Select; import net.sf.jsqlparser.statement.select.SetOperationList; @@ -117,7 +118,7 @@ public static LegacyJdbcQuery.TrustedSql validate(String sql, Properties propert throw new QueryByExampleValidationException("The query could not be parsed as a single SELECT", e); } - if (!(statement instanceof Select select) || select instanceof SetOperationList) { + if (!(statement instanceof Select select) || containsSetOperation(select)) { throw new QueryByExampleValidationException("Only one SELECT statement is allowed"); } @@ -160,6 +161,12 @@ private static void rejectUnsafeExpressionsAndOtherSchemas(Statement statement, if (visitor.hasOutputOperation()) { throw new QueryByExampleValidationException("SELECT output operations are not allowed"); } + if (visitor.hasLockingOperation()) { + throw new QueryByExampleValidationException("Locking SELECT statements are not allowed"); + } + if (visitor.hasSetOperation()) { + throw new QueryByExampleValidationException("Set operations are not allowed"); + } if (visitor.hasVariables()) { throw new QueryByExampleValidationException("Session and system variables are not allowed"); } @@ -178,6 +185,14 @@ private static void rejectUnsafeExpressionsAndOtherSchemas(Statement statement, } } + private static boolean containsSetOperation(Select select) { + if (select instanceof SetOperationList) { + return true; + } + return select instanceof ParenthesedSelect parenthesedSelect + && containsSetOperation(parenthesedSelect.getSelect()); + } + private static String stripQuotedSections(String sql) { StringBuilder stripped = new StringBuilder(sql.length()); char quote = '\0'; @@ -191,9 +206,6 @@ private static String stripQuotedSections(String sql) { } else { stripped.append(current); } - } else if (quote != '`' && current == '\\' && next != '\0') { - stripped.append(" "); - i++; } else if (current == quote && next == quote) { stripped.append(" "); i++; @@ -219,6 +231,8 @@ private static final class SqlSafetyVisitor extends TablesNamesFinder { private final Set disallowedFunctions = new java.util.HashSet<>(); private boolean variables; private boolean outputOperation; + private boolean lockingOperation; + private boolean setOperation; @Override public Void visit(Function function, S context) { @@ -257,9 +271,20 @@ public Void visit(PlainSelect select, S context) { || select.getIntoTempTable() != null) { outputOperation = true; } + if (select.getForMode() != null || select.getForClause() != null + || select.getForUpdateTable() != null || select.isSkipLocked() + || select.isNoWait() || select.getWait() != null) { + lockingOperation = true; + } return super.visit(select, context); } + @Override + public Void visit(SetOperationList operations, S context) { + setOperation = true; + return super.visit(operations, context); + } + Set disallowedFunctions() { return disallowedFunctions; } @@ -271,5 +296,13 @@ boolean hasVariables() { boolean hasOutputOperation() { return outputOperation; } + + boolean hasLockingOperation() { + return lockingOperation; + } + + boolean hasSetOperation() { + return setOperation; + } } } diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java b/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java index 1064736d5ae..c203d0eebd8 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java @@ -58,7 +58,8 @@ interface ConnectionProvider { Connection getConnection() throws SQLException; } - public record QueryResult(String html, int rowCount, boolean truncated, long durationMillis) { + public record QueryResult(String html, int rowCount, boolean truncated, boolean rowLimitReached, + long durationMillis) { } private final ConnectionProvider connectionProvider; @@ -87,13 +88,14 @@ public QueryResult execute(String sql, Properties properties, String providerNo) try { connection.setReadOnly(true); try (PreparedStatement statement = prepareValidatedStatement(connection, trustedSql)) { - statement.setMaxRows(MAX_ROWS); + statement.setMaxRows(MAX_ROWS + 1); statement.setQueryTimeout(QUERY_TIMEOUT_SECONDS); try (ResultSet resultSet = statement.executeQuery()) { RptResultStruct.StructuredResult structured = RptResultStruct.getStructureWithCount( - resultSet, MAX_OUTPUT_CHARACTERS); + resultSet, MAX_OUTPUT_CHARACTERS, MAX_ROWS); rowCount = structured.rowCount(); - queryResult = new QueryResult(structured.html(), rowCount, structured.truncated(), 0); + queryResult = new QueryResult(structured.html(), rowCount, structured.truncated(), + structured.rowLimitReached(), 0); } } } catch (SQLException | RuntimeException e) { @@ -113,7 +115,7 @@ public QueryResult execute(String sql, Properties properties, String providerNo) } outcome = "success"; return new QueryResult(queryResult.html(), queryResult.rowCount(), queryResult.truncated(), - elapsedMillis(startedAt)); + queryResult.rowLimitReached(), elapsedMillis(startedAt)); } catch (QueryByExampleValidationException e) { outcome = "rejected"; throw e; diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java b/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java index 8117f0b9eaf..2a1f73c4d98 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java @@ -51,7 +51,7 @@ public class RptResultStruct { private static final int MIN_OUTPUT_CHARACTERS = 64; private static final int CLOSING_MARKUP_RESERVE = 32; - public record StructuredResult(String html, int rowCount, boolean truncated) { + public record StructuredResult(String html, int rowCount, boolean truncated, boolean rowLimitReached) { } public static String getStructure(ResultSet rs) throws SQLException { @@ -66,7 +66,7 @@ public static String getStructure(ResultSet rs) throws SQLException { * @throws SQLException if the result set cannot be read */ public static StructuredResult getStructureWithCount(ResultSet rs) throws SQLException { - return getStructureWithCount(rs, Integer.MAX_VALUE); + return getStructureWithCount(rs, Integer.MAX_VALUE, Integer.MAX_VALUE); } /** @@ -78,9 +78,26 @@ public static StructuredResult getStructureWithCount(ResultSet rs) throws SQLExc * @throws SQLException if the result set cannot be read */ public static StructuredResult getStructureWithCount(ResultSet rs, int maxOutputCharacters) throws SQLException { + return getStructureWithCount(rs, maxOutputCharacters, Integer.MAX_VALUE); + } + + /** + * Generates an encoded HTML table within output and row budgets. + * + * @param rs result set positioned before its first row + * @param maxOutputCharacters maximum number of rendered HTML characters + * @param maxRows maximum number of rows to render + * @return encoded table markup, rendered row count, and truncation state + * @throws SQLException if the result set cannot be read + */ + public static StructuredResult getStructureWithCount(ResultSet rs, int maxOutputCharacters, int maxRows) + throws SQLException { if (maxOutputCharacters < MIN_OUTPUT_CHARACTERS) { throw new IllegalArgumentException("HTML output limit is too small"); } + if (maxRows < 1) { + throw new IllegalArgumentException("Row limit must be positive"); + } // assuming multiple rows in rs LimitedHtmlBuilder html = new LimitedHtmlBuilder(maxOutputCharacters); @@ -94,16 +111,16 @@ public static StructuredResult getStructureWithCount(ResultSet rs, int maxOutput if (!html.appendMarkup("") || !html.appendEncoded(rsmd.getColumnLabel(i + 1))) { html.appendClosingMarkup(""); - return new StructuredResult(html.toString(), 0, true); + return new StructuredResult(html.toString(), 0, true, false); } if (!html.appendMarkup("")) { html.appendClosingMarkup(""); - return new StructuredResult(html.toString(), 0, true); + return new StructuredResult(html.toString(), 0, true, false); } } int rowCount = 0; boolean stopRendering = false; - while (!stopRendering && rs.next()) { + while (!stopRendering && rowCount < maxRows && rs.next()) { rowCount++; if (!html.appendMarkup("")) { break; @@ -136,8 +153,9 @@ public static StructuredResult getStructureWithCount(ResultSet rs, int maxOutput stopRendering = true; } } + boolean rowLimitReached = !stopRendering && rowCount == maxRows && rs.next(); html.appendClosingMarkup(""); - return new StructuredResult(html.toString(), rowCount, html.isTruncated()); + return new StructuredResult(html.toString(), rowCount, html.isTruncated(), rowLimitReached); } private static final class LimitedHtmlBuilder { @@ -184,12 +202,22 @@ private boolean appendEncodedChunk(String encoded) { } if (remaining > 0) { int contentCharacters = Math.max(0, remaining - 1); - html.append(encoded, 0, contentCharacters).append('\u2026'); + int safeCharacters = entitySafePrefixLength(encoded, contentCharacters); + html.append(encoded, 0, safeCharacters).append('\u2026'); } truncated = true; return false; } + private static int entitySafePrefixLength(String encoded, int requestedLength) { + if (requestedLength == 0) { + return 0; + } + int lastEntityStart = encoded.lastIndexOf('&', requestedLength - 1); + int lastEntityEnd = encoded.lastIndexOf(';', requestedLength - 1); + return lastEntityStart > lastEntityEnd ? lastEntityStart : requestedLength; + } + void appendClosingMarkup(String markup) { html.append(markup); } diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java index 4e47b4a7271..1130263dc49 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExample2Action.java @@ -142,6 +142,7 @@ public String execute() request.setAttribute("resultRowCount", result.rowCount()); request.setAttribute("resultLimit", RptByExampleData.MAX_ROWS); request.setAttribute("resultTruncated", result.truncated()); + request.setAttribute("resultRowLimitReached", result.rowLimitReached()); request.setAttribute("resultCharacterLimit", RptByExampleData.MAX_OUTPUT_CHARACTERS); try { write2Database(sql, providerNo); diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java index 406bd54bfab..2946aa14275 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java @@ -68,6 +68,10 @@ public String execute() throws ServletException, IOException { && !securityInfoManager.hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null)) { throw new SecurityException("missing required sec object (_admin or _report)"); } + if (!"POST".equalsIgnoreCase(request.getMethod())) { + response.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED); + return NONE; + } String providerNo = (String) request.getSession().getAttribute("user"); diff --git a/src/main/resources/oscarResources_en.properties b/src/main/resources/oscarResources_en.properties index 68491f6fb86..13c25e77eba 100644 --- a/src/main/resources/oscarResources_en.properties +++ b/src/main/resources/oscarResources_en.properties @@ -8930,6 +8930,7 @@ oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Che oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. +oscarReport.RptByExample.MsgRowLimitReached=More than {0} rows matched. Only the first {0} rows are shown. oscarReport.RptByExample.MsgConfirmDelete=Are you sure you want to delete the selected query? diff --git a/src/main/resources/oscarResources_es.properties b/src/main/resources/oscarResources_es.properties index df6664c50c6..5de18253ece 100644 --- a/src/main/resources/oscarResources_es.properties +++ b/src/main/resources/oscarResources_es.properties @@ -6455,6 +6455,8 @@ oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it coul oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). # TODO: translate oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. +# TODO: translate +oscarReport.RptByExample.MsgRowLimitReached=More than {0} rows matched. Only the first {0} rows are shown. oscarReport.RptByExample.MsgConfirmDelete=\u00bfEst\u00e1 seguro de que desea eliminar la consulta seleccionada? diff --git a/src/main/resources/oscarResources_fr.properties b/src/main/resources/oscarResources_fr.properties index ff2d00431b9..a7d1482c874 100644 --- a/src/main/resources/oscarResources_fr.properties +++ b/src/main/resources/oscarResources_fr.properties @@ -5637,6 +5637,8 @@ oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it coul oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). # TODO: translate oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. +# TODO: translate +oscarReport.RptByExample.MsgRowLimitReached=More than {0} rows matched. Only the first {0} rows are shown. oscarReport.RptByExample.MsgConfirmDelete=\u00cates-vous s\u00fbr de vouloir supprimer la requ\u00eate s\u00e9lectionn\u00e9e\u00a0? diff --git a/src/main/resources/oscarResources_pl.properties b/src/main/resources/oscarResources_pl.properties index 2f9e45a7125..eedf2037cc3 100644 --- a/src/main/resources/oscarResources_pl.properties +++ b/src/main/resources/oscarResources_pl.properties @@ -5873,6 +5873,8 @@ oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it coul oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). # TODO: translate oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. +# TODO: translate +oscarReport.RptByExample.MsgRowLimitReached=More than {0} rows matched. Only the first {0} rows are shown. oscarReport.RptByExample.MsgConfirmDelete=Czy na pewno chcesz usun\u0105\u0107 wybran\u0105 kwerend\u0119? diff --git a/src/main/resources/oscarResources_pt_BR.properties b/src/main/resources/oscarResources_pt_BR.properties index e5dfa7b49e3..16af3ceb187 100644 --- a/src/main/resources/oscarResources_pt_BR.properties +++ b/src/main/resources/oscarResources_pt_BR.properties @@ -7360,6 +7360,8 @@ oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it coul oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). # TODO: translate oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. +# TODO: translate +oscarReport.RptByExample.MsgRowLimitReached=More than {0} rows matched. Only the first {0} rows are shown. oscarReport.RptByExample.MsgConfirmDelete=Tem certeza de que deseja excluir a consulta selecionada? diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp index cea4838b075..3088af10b22 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExample.jsp @@ -252,6 +252,13 @@
+ + + ${results}
diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp index aa5b7fe6685..b1b10cbaf01 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp @@ -29,6 +29,24 @@ --%> +<%-- + RptByExamplesFavorite.jsp + ========================= + Purpose: Edit a saved Query-by-Example favorite before returning to the + favorites list. + + Features: + - Requires _report or _admin read privilege + - Localized favorite-name and SQL editing form + - POST-only submission to RptByExamplesFavorite + + Parameters (set by backing action): + - favoriteName — Display name for the favorite + - newQuery — SQL text being edited + + @since 2001-2002 +--%> + <%@ taglib uri="/WEB-INF/security.tld" prefix="security" %> <% String roleName$ = (String) session.getAttribute("userrole") + "," + (String) session.getAttribute("user"); diff --git a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java index 3a8d88c0797..8f00c663d73 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java @@ -64,6 +64,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.mockConstruction; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.spy; import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyNoInteractions; @@ -81,6 +82,7 @@ class ReportActionSecurityMigrationUnitTest extends CarlosUnitTestBase { private SecurityInfoManager securityInfoManager; private LoggedInInfo loggedInInfo; private ReportByExamplesDao reportByExamplesDao; + private ReportByExamplesFavoriteDao favoritesDao; @BeforeEach void setUp() { @@ -88,7 +90,7 @@ void setUp() { reportByExamplesDao = mock(ReportByExamplesDao.class); registerMock(SecurityInfoManager.class, securityInfoManager); registerMock(ReportByExamplesDao.class, reportByExamplesDao); - ReportByExamplesFavoriteDao favoritesDao = mock(ReportByExamplesFavoriteDao.class); + favoritesDao = mock(ReportByExamplesFavoriteDao.class); registerMock(ReportByExamplesFavoriteDao.class, favoritesDao); registerMock(ReportTemplatesDao.class, mock(ReportTemplatesDao.class)); @@ -161,7 +163,7 @@ void shouldSkipReportReadPrivilege_whenAdminReadPrivilegeAllowsAccess() throws E assertAuthorizedMigrationGateAllowsActionBody(); - verify(securityInfoManager, times(8)) + verify(securityInfoManager, times(7)) .hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null); verify(securityInfoManager, never()) .hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null); @@ -176,9 +178,9 @@ void shouldAllowAccess_whenReportReadPrivilegeAllowsAccess() throws Exception { assertAuthorizedMigrationGateAllowsActionBody(); - verify(securityInfoManager, times(8)) + verify(securityInfoManager, times(7)) .hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null); - verify(securityInfoManager, times(8)) + verify(securityInfoManager, times(7)) .hasPrivilege(loggedInInfo, "_report", SecurityInfoManager.READ, null); } @@ -208,6 +210,51 @@ void shouldKeepFormWithoutSaving_whenQueryByExampleIsDisabled() throws Exception assertThat(request.getAttribute("queryDisabled")).isEqualTo(true); assertThat(request.getAttribute("submittedSql")).isEqualTo("select demographic_no from demographic"); verifyNoInteractions(reportByExamplesDao); + verify(favoritesDao).findByProvider("999998"); + } + + @Test + @DisplayName("RptByExamplesFavorite rejects non-POST requests before writing") + void shouldRejectNonPostMethods_beforeFavoriteWrite() throws Exception { + LoggedInInfo.setLoggedInInfoIntoSession(request.getSession(), loggedInInfo); + when(securityInfoManager.hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null)).thenReturn(true); + request.setMethod("GET"); + RptByExamplesFavorite2Action getAction = spy(new RptByExamplesFavorite2Action()); + getAction.setQuery("select demographic_no from demographic"); + + assertThat(getAction.execute()).isEqualTo(ActionSupport.NONE); + assertThat(response.getStatus()).isEqualTo(405); + verify(getAction, never()).write2Database(any(), any(), any()); + + response = new MockHttpServletResponse(); + servletActionContextMock.when(ServletActionContext::getResponse).thenReturn(response); + request.setMethod("HEAD"); + RptByExamplesFavorite2Action headAction = spy(new RptByExamplesFavorite2Action()); + headAction.setQuery("select demographic_no from demographic"); + + assertThat(headAction.execute()).isEqualTo(ActionSupport.NONE); + assertThat(response.getStatus()).isEqualTo(405); + verify(headAction, never()).write2Database(any(), any(), any()); + verifyNoInteractions(reportByExamplesDao, favoritesDao); + } + + @Test + @DisplayName("RptByExamplesFavorite permits an authorized POST mutation") + void shouldAllowFavoriteWrite_whenAuthorizedPostIsSubmitted() throws Exception { + LoggedInInfo.setLoggedInInfoIntoSession(request.getSession(), loggedInInfo); + when(securityInfoManager.hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null)).thenReturn(true); + request.getSession().setAttribute("user", "999998"); + request.setMethod("POST"); + RptByExamplesFavorite2Action action = spy(new RptByExamplesFavorite2Action()); + action.setFavoriteName("Active patients"); + action.setQuery("select demographic_no from demographic"); + org.mockito.Mockito.doNothing().when(action).write2Database( + "999998", "Active patients", "select demographic_no from demographic"); + + assertThat(action.execute()).isEqualTo(ActionSupport.SUCCESS); + + verify(action).write2Database("999998", "Active patients", "select demographic_no from demographic"); + verify(favoritesDao).findByProvider("999998"); } @Test @@ -228,7 +275,7 @@ void shouldKeepResults_whenQueryHistorySaveFails() throws Exception { RptByExampleData.class, (mock, context) -> when(mock.execute( eq("select demographic_no from demographic"), any(Properties.class), eq("999998"))) - .thenReturn(new RptByExampleData.QueryResult("
", 1, false, 23)))) { + .thenReturn(new RptByExampleData.QueryResult("
", 1, false, false, 23)))) { properties.setProperty(RptByExample2Action.ENABLED_PROPERTY, " yes "); assertThat(action.execute()).isEqualTo(ActionSupport.SUCCESS); assertThat(reportData.constructed()).hasSize(1); @@ -288,7 +335,6 @@ private void assertAuthorizedMigrationGateAllowsActionBody() throws Exception { assertThat(new RptByExample2Action().execute()).isEqualTo(ActionSupport.SUCCESS); assertThat(new RptViewAllQueryByExamples2Action().execute()).isEqualTo(ActionSupport.SUCCESS); assertThat(new RptByExamplesAllFavorites2Action().execute()).isEqualTo(ActionSupport.SUCCESS); - assertThat(new RptByExamplesFavorite2Action().execute()).isEqualTo(ActionSupport.SUCCESS); request.setParameter("templateid", "template-1"); assertThat(new ExportTemplate2Action().execute()).isEqualTo(ActionSupport.SUCCESS); diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java index 3630f664f7e..ecae24b2eb5 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java @@ -114,6 +114,7 @@ private static Stream unsafeQueries() { "explain select * from demographic", "update demographic set last_name='x'", "select * from demographic; select * from provider", + "(select demographic_no from demographic union select provider_no from provider)", "select * from demographic -- comment", "select * from other_database.demographic", "select * from o\u017Fcar_mcmaster.demographic", @@ -130,6 +131,7 @@ private static Stream unsafeQueries() { "select @@version", "select next value for report_sequence", "select * from demographic for update", + "select '\\' as value from demographic for update", "select * from demographic for share", "select demographic_no into @number from demographic"); } diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java index be8e8bd7b98..c54023f1b01 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java @@ -85,7 +85,7 @@ void shouldExecuteBoundedReadOnlyQuery_whenConnectionStartsReadOnly() throws SQL assertThat(result.rowCount()).isEqualTo(1); assertThat(result.html()).contains("demographic_no").contains("42"); - verify(statement).setMaxRows(RptByExampleData.MAX_ROWS); + verify(statement).setMaxRows(RptByExampleData.MAX_ROWS + 1); verify(statement).setQueryTimeout(RptByExampleData.QUERY_TIMEOUT_SECONDS); InOrder order = inOrder(connection, statement, resultSet); @@ -174,6 +174,29 @@ void shouldTruncateEncodedOutput_whenCellExceedsCharacterBudget() throws SQLExce assertThat(result.rowCount()).isEqualTo(1); assertThat(result.html()).hasSizeLessThanOrEqualTo(128).endsWith(""); assertThat(result.html()).contains("<"); + String cell = result.html().substring(result.html().indexOf("") + 4, result.html().indexOf("")); + assertThat(cell).matches("(?:<)*…"); + } + + @Test + @DisplayName("reports omitted rows when the result exceeds the rendering row limit") + void shouldReportRowLimit_whenResultContainsAnotherRow() throws SQLException { + when(resultSet.next()).thenReturn(true, true); + + RptResultStruct.StructuredResult result = RptResultStruct.getStructureWithCount(resultSet, 1_000, 1); + + assertThat(result.rowCount()).isEqualTo(1); + assertThat(result.rowLimitReached()).isTrue(); + assertThat(result.html()).contains("42"); + } + + @Test + @DisplayName("does not report omitted rows when the result exactly reaches the row limit") + void shouldNotReportRowLimit_whenResultExactlyMatchesLimit() throws SQLException { + RptResultStruct.StructuredResult result = RptResultStruct.getStructureWithCount(resultSet, 1_000, 1); + + assertThat(result.rowCount()).isEqualTo(1); + assertThat(result.rowLimitReached()).isFalse(); } @Test From 3324c576af40ba46eb521001b48ed3db13dba98b Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 17:09:55 -0400 Subject: [PATCH 08/21] fix(report): advertise favorite post method Signed-off-by: Ben Heerema --- .../carlos/report/pageUtil/RptByExamplesFavorite2Action.java | 1 + src/main/resources/oscarResources_en.properties | 2 +- src/main/resources/oscarResources_es.properties | 2 +- src/main/resources/oscarResources_fr.properties | 2 +- src/main/resources/oscarResources_pl.properties | 2 +- src/main/resources/oscarResources_pt_BR.properties | 2 +- .../carlos/report/ReportActionSecurityMigrationUnitTest.java | 2 ++ 7 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java index 2946aa14275..daa05fa6caf 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java @@ -69,6 +69,7 @@ public String execute() throws ServletException, IOException { throw new SecurityException("missing required sec object (_admin or _report)"); } if (!"POST".equalsIgnoreCase(request.getMethod())) { + response.setHeader("Allow", "POST"); response.sendError(HttpServletResponse.SC_METHOD_NOT_ALLOWED); return NONE; } diff --git a/src/main/resources/oscarResources_en.properties b/src/main/resources/oscarResources_en.properties index 13c25e77eba..646ae54056a 100644 --- a/src/main/resources/oscarResources_en.properties +++ b/src/main/resources/oscarResources_en.properties @@ -8930,7 +8930,7 @@ oscarReport.RptByExample.MsgExecutionError=The query could not be completed. Che oscarReport.RptByExample.MsgHistoryError=The query ran successfully, but it could not be saved to your query history. oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. -oscarReport.RptByExample.MsgRowLimitReached=More than {0} rows matched. Only the first {0} rows are shown. +oscarReport.RptByExample.MsgRowLimitReached=Additional rows were omitted after the {0}-row display limit was reached. oscarReport.RptByExample.MsgConfirmDelete=Are you sure you want to delete the selected query? diff --git a/src/main/resources/oscarResources_es.properties b/src/main/resources/oscarResources_es.properties index 5de18253ece..e86f638e0a6 100644 --- a/src/main/resources/oscarResources_es.properties +++ b/src/main/resources/oscarResources_es.properties @@ -6456,7 +6456,7 @@ oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). # TODO: translate oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. # TODO: translate -oscarReport.RptByExample.MsgRowLimitReached=More than {0} rows matched. Only the first {0} rows are shown. +oscarReport.RptByExample.MsgRowLimitReached=Additional rows were omitted after the {0}-row display limit was reached. oscarReport.RptByExample.MsgConfirmDelete=\u00bfEst\u00e1 seguro de que desea eliminar la consulta seleccionada? diff --git a/src/main/resources/oscarResources_fr.properties b/src/main/resources/oscarResources_fr.properties index a7d1482c874..5c0a64e91c7 100644 --- a/src/main/resources/oscarResources_fr.properties +++ b/src/main/resources/oscarResources_fr.properties @@ -5638,7 +5638,7 @@ oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). # TODO: translate oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. # TODO: translate -oscarReport.RptByExample.MsgRowLimitReached=More than {0} rows matched. Only the first {0} rows are shown. +oscarReport.RptByExample.MsgRowLimitReached=Additional rows were omitted after the {0}-row display limit was reached. oscarReport.RptByExample.MsgConfirmDelete=\u00cates-vous s\u00fbr de vouloir supprimer la requ\u00eate s\u00e9lectionn\u00e9e\u00a0? diff --git a/src/main/resources/oscarResources_pl.properties b/src/main/resources/oscarResources_pl.properties index eedf2037cc3..a2ecf1de324 100644 --- a/src/main/resources/oscarResources_pl.properties +++ b/src/main/resources/oscarResources_pl.properties @@ -5874,7 +5874,7 @@ oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). # TODO: translate oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. # TODO: translate -oscarReport.RptByExample.MsgRowLimitReached=More than {0} rows matched. Only the first {0} rows are shown. +oscarReport.RptByExample.MsgRowLimitReached=Additional rows were omitted after the {0}-row display limit was reached. oscarReport.RptByExample.MsgConfirmDelete=Czy na pewno chcesz usun\u0105\u0107 wybran\u0105 kwerend\u0119? diff --git a/src/main/resources/oscarResources_pt_BR.properties b/src/main/resources/oscarResources_pt_BR.properties index 16af3ceb187..8eb1c877f2d 100644 --- a/src/main/resources/oscarResources_pt_BR.properties +++ b/src/main/resources/oscarResources_pt_BR.properties @@ -7361,7 +7361,7 @@ oscarReport.RptByExample.MsgResultLimit=Returned {0} rows (limit: {1}). # TODO: translate oscarReport.RptByExample.MsgOutputTruncated=Output was truncated after {0} characters. # TODO: translate -oscarReport.RptByExample.MsgRowLimitReached=More than {0} rows matched. Only the first {0} rows are shown. +oscarReport.RptByExample.MsgRowLimitReached=Additional rows were omitted after the {0}-row display limit was reached. oscarReport.RptByExample.MsgConfirmDelete=Tem certeza de que deseja excluir a consulta selecionada? diff --git a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java index 8f00c663d73..675084e4d71 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java @@ -224,6 +224,7 @@ void shouldRejectNonPostMethods_beforeFavoriteWrite() throws Exception { assertThat(getAction.execute()).isEqualTo(ActionSupport.NONE); assertThat(response.getStatus()).isEqualTo(405); + assertThat(response.getHeader("Allow")).isEqualTo("POST"); verify(getAction, never()).write2Database(any(), any(), any()); response = new MockHttpServletResponse(); @@ -234,6 +235,7 @@ void shouldRejectNonPostMethods_beforeFavoriteWrite() throws Exception { assertThat(headAction.execute()).isEqualTo(ActionSupport.NONE); assertThat(response.getStatus()).isEqualTo(405); + assertThat(response.getHeader("Allow")).isEqualTo("POST"); verify(headAction, never()).write2Database(any(), any(), any()); verifyNoInteractions(reportByExamplesDao, favoritesDao); } From 5ebfa982fd90690a1cfcb1c7b4c4b2c0e3932706 Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 18:09:53 -0400 Subject: [PATCH 09/21] fix(report): secure query favorites Signed-off-by: Ben Heerema --- .../dao/ReportByExamplesFavoriteDao.java | 2 +- .../dao/ReportByExamplesFavoriteDaoImpl.java | 11 +-- .../report/bean/RptByExampleQueryBean.java | 2 - .../data/QueryByExampleSqlValidator.java | 41 +++++++---- .../RptByExamplesAllFavorites2Action.java | 2 +- .../RptByExamplesFavorite2Action.java | 64 +++++++++++++---- .../oscarReport/RptByExamplesAllFavorites.jsp | 5 +- .../jsp/oscarReport/RptByExamplesFavorite.jsp | 27 ++++--- ...tByExamplesFavoriteDaoIntegrationTest.java | 52 +++++--------- ...ReportActionSecurityMigrationUnitTest.java | 72 +++++++++++++++++++ 10 files changed, 191 insertions(+), 87 deletions(-) diff --git a/src/main/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDao.java b/src/main/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDao.java index 98a84641b41..eacd6f7a796 100644 --- a/src/main/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDao.java +++ b/src/main/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDao.java @@ -36,7 +36,7 @@ import io.github.carlos_emr.carlos.commn.model.ReportByExamplesFavorite; public interface ReportByExamplesFavoriteDao extends AbstractDao { - List findByQuery(String query); + List findByProviderAndQuery(String providerNo, String query); List findByEverything(String providerNo, String favoriteName, String queryString); diff --git a/src/main/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDaoImpl.java b/src/main/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDaoImpl.java index 4bec1b1747d..9029125f9de 100644 --- a/src/main/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDaoImpl.java +++ b/src/main/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDaoImpl.java @@ -46,15 +46,16 @@ public ReportByExamplesFavoriteDaoImpl() { } @Override - public List findByQuery(String query) { - Query q = createQuery("ex", "ex.query LIKE ?1"); - q.setParameter(1, query); - return q.getResultList(); + public List findByProviderAndQuery(String providerNo, String queryString) { + Query query = createQuery("ex", "ex.providerNo = ?1 AND ex.query = ?2"); + query.setParameter(1, providerNo); + query.setParameter(2, queryString); + return query.getResultList(); } @Override public List findByEverything(String providerNo, String favoriteName, String queryString) { - Query query = createQuery("ex", "ex.providerNo = ?1 AND ex.name LIKE ?2 OR ex.query LIKE ?3"); + Query query = createQuery("ex", "ex.providerNo = ?1 AND ex.name = ?2 AND ex.query = ?3"); query.setParameter(1, providerNo); query.setParameter(2, favoriteName); query.setParameter(3, queryString); diff --git a/src/main/java/io/github/carlos_emr/carlos/report/bean/RptByExampleQueryBean.java b/src/main/java/io/github/carlos_emr/carlos/report/bean/RptByExampleQueryBean.java index ab18d5069f4..65d4d09b2e9 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/bean/RptByExampleQueryBean.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/bean/RptByExampleQueryBean.java @@ -31,7 +31,6 @@ package io.github.carlos_emr.carlos.report.bean; import org.owasp.encoder.Encode; -import io.github.carlos_emr.carlos.utility.MiscUtils; public class RptByExampleQueryBean { @@ -52,7 +51,6 @@ public RptByExampleQueryBean(int id, String query, String queryName) { this.query = query; this.queryName = queryName; this.queryWithEscapeChar = Encode.forJavaScript(query); - MiscUtils.getLogger().debug("query with javascript escape char: " + queryWithEscapeChar); } public RptByExampleQueryBean(String providerLastName, String providerFirstName, String query, String date) { diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java index 9e70339fe4b..6dde970d170 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java @@ -101,6 +101,18 @@ private QueryByExampleSqlValidator() { */ public static LegacyJdbcQuery.TrustedSql validate(String sql, Properties properties) throws QueryByExampleValidationException { + validateSqlText(sql); + Select select = parseSingleSelect(sql); + rejectUnsafeTextOperations(sql); + rejectUnsafeExpressionsAndOtherSchemas(select, applicationSchema(properties)); + try { + return LegacyJdbcQuery.trustedSelectSql(sql); + } catch (SQLException e) { + throw new QueryByExampleValidationException(e.getMessage(), e); + } + } + + private static void validateSqlText(String sql) throws QueryByExampleValidationException { if (sql == null) { throw new QueryByExampleValidationException("SQL query must not be empty"); } @@ -110,7 +122,9 @@ public static LegacyJdbcQuery.TrustedSql validate(String sql, Properties propert if (sql.isBlank()) { throw new QueryByExampleValidationException("SQL query must not be empty"); } + } + private static Select parseSingleSelect(String sql) throws QueryByExampleValidationException { Statement statement; try { statement = CCJSqlParserUtil.parse(sql); @@ -121,7 +135,10 @@ public static LegacyJdbcQuery.TrustedSql validate(String sql, Properties propert if (!(statement instanceof Select select) || containsSetOperation(select)) { throw new QueryByExampleValidationException("Only one SELECT statement is allowed"); } + return select; + } + private static void rejectUnsafeTextOperations(String sql) throws QueryByExampleValidationException { String sqlWithoutQuotedSections = stripQuotedSections(sql); if (LOCKING_SELECT.matcher(sqlWithoutQuotedSections).find()) { throw new QueryByExampleValidationException("Locking SELECT statements are not allowed"); @@ -129,12 +146,6 @@ public static LegacyJdbcQuery.TrustedSql validate(String sql, Properties propert if (OUTPUT_OR_PROCEDURE_OPERATION.matcher(sqlWithoutQuotedSections).find()) { throw new QueryByExampleValidationException("SELECT output operations are not allowed"); } - rejectUnsafeExpressionsAndOtherSchemas(statement, applicationSchema(properties)); - try { - return LegacyJdbcQuery.trustedSelectSql(sql); - } catch (SQLException e) { - throw new QueryByExampleValidationException(e.getMessage(), e); - } } static String applicationSchema(Properties properties) throws QueryByExampleValidationException { @@ -196,7 +207,8 @@ private static boolean containsSetOperation(Select select) { private static String stripQuotedSections(String sql) { StringBuilder stripped = new StringBuilder(sql.length()); char quote = '\0'; - for (int i = 0; i < sql.length(); i++) { + int i = 0; + while (i < sql.length()) { char current = sql.charAt(i); char next = i + 1 < sql.length() ? sql.charAt(i + 1) : '\0'; if (quote == '\0') { @@ -206,15 +218,18 @@ private static String stripQuotedSections(String sql) { } else { stripped.append(current); } - } else if (current == quote && next == quote) { - stripped.append(" "); - i++; - } else if (current == quote) { - quote = '\0'; - stripped.append(' '); } else { + if (current == quote && next == quote) { + stripped.append(" "); + i += 2; + continue; + } + if (current == quote) { + quote = '\0'; + } stripped.append(' '); } + i++; } return stripped.toString(); } diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesAllFavorites2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesAllFavorites2Action.java index 78deca29999..9307ea36813 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesAllFavorites2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesAllFavorites2Action.java @@ -61,7 +61,7 @@ public String execute() } - String providerNo = (String) request.getSession().getAttribute("user"); + String providerNo = loggedInInfo.getLoggedInProviderNo(); RptByExampleQueryBeanHandler hd = new RptByExampleQueryBeanHandler(providerNo); request.setAttribute("allFavorites", hd); return SUCCESS; diff --git a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java index daa05fa6caf..e88ed62f626 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/pageUtil/RptByExamplesFavorite2Action.java @@ -31,6 +31,7 @@ import java.io.IOException; import java.util.List; +import java.util.Objects; import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; @@ -39,7 +40,6 @@ import org.apache.commons.lang3.StringUtils; import io.github.carlos_emr.carlos.commn.dao.ReportByExamplesFavoriteDao; import io.github.carlos_emr.carlos.commn.model.ReportByExamplesFavorite; -import io.github.carlos_emr.carlos.utility.MiscUtils; import io.github.carlos_emr.carlos.utility.SpringUtils; import io.github.carlos_emr.carlos.report.bean.RptByExampleQueryBeanHandler; @@ -74,23 +74,23 @@ public String execute() throws ServletException, IOException { return NONE; } - String providerNo = (String) request.getSession().getAttribute("user"); + String providerNo = loggedInInfo.getLoggedInProviderNo(); if (!StringUtils.isEmpty(this.getNewQuery())) { // Edit case - this.setQuery(this.getNewQuery()); - if (!StringUtils.isEmpty(this.getNewName())) { - this.setFavoriteName(this.getNewName()); + if (hasFavoriteId()) { + ReportByExamplesFavorite favorite = requireOwnedFavorite(providerNo, this.getId()); + this.setQuery(favorite.getQuery()); + this.setFavoriteName(favorite.getName()); } else { - ReportByExamplesFavoriteDao dao = SpringUtils.getBean(ReportByExamplesFavoriteDao.class); - for (ReportByExamplesFavorite f : dao.findByQuery(this.getNewQuery())) { - this.setFavoriteName(f.getName()); - } + prepareNewFavorite(providerNo); } return "edit"; } else if ("true".equalsIgnoreCase(this.getToDelete())) { // Deletion case - deleteQuery(this.getId()); + deleteQuery(providerNo, this.getId()); + } else if (hasFavoriteId()) { + updateFavorite(providerNo, this.getId(), this.getFavoriteName(), this.getQuery()); } else { // Add to favorite case String favoriteName = this.getFavoriteName(); @@ -110,9 +110,6 @@ public void write2Database(String providerNo, String favoriteName, String query) return; } - MiscUtils.getLogger().debug("Fav " + favoriteName + " query " + query); - - ReportByExamplesFavoriteDao dao = SpringUtils.getBean(ReportByExamplesFavoriteDao.class); List favorites = dao.findByEverything(providerNo, favoriteName, query); if (favorites.isEmpty()) { ReportByExamplesFavorite r = new ReportByExamplesFavorite(); @@ -131,8 +128,45 @@ public void write2Database(String providerNo, String favoriteName, String query) } - public void deleteQuery(String id) { - dao.remove(Integer.parseInt(id)); + public void deleteQuery(String providerNo, String id) { + dao.remove(requireOwnedFavorite(providerNo, id)); + } + + private void prepareNewFavorite(String providerNo) { + this.setQuery(this.getNewQuery()); + if (!StringUtils.isEmpty(this.getNewName())) { + this.setFavoriteName(this.getNewName()); + return; + } + List favorites = dao.findByProviderAndQuery(providerNo, this.getNewQuery()); + if (!favorites.isEmpty()) { + this.setFavoriteName(favorites.get(0).getName()); + } + } + + private void updateFavorite(String providerNo, String id, String favoriteName, String query) { + ReportByExamplesFavorite favorite = requireOwnedFavorite(providerNo, id); + favorite.setName(favoriteName); + favorite.setQuery(StringUtils.defaultString(query)); + dao.merge(favorite); + } + + private ReportByExamplesFavorite requireOwnedFavorite(String providerNo, String id) { + final int favoriteId; + try { + favoriteId = Integer.parseInt(id); + } catch (NumberFormatException e) { + throw new SecurityException("Invalid favorite selection", e); + } + ReportByExamplesFavorite favorite = dao.find(favoriteId); + if (favorite == null || !Objects.equals(providerNo, favorite.getProviderNo())) { + throw new SecurityException("Favorite does not belong to the current provider"); + } + return favorite; + } + + private boolean hasFavoriteId() { + return StringUtils.isNotBlank(this.getId()) && !"error".equals(this.getId()); } diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp index 67f6cbd2257..979375e5b70 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp @@ -104,9 +104,10 @@ * @param {string} text1 - The raw SQL query text (JS-attribute-encoded by JSP) * @param {string} text2 - The display name of the favourite (JS-attribute-encoded by JSP) */ - function set(text1, text2) { + function set(text1, text2, id) { document.getElementById('favoritesForm').newQuery.value = text1; document.getElementById('favoritesForm').newName.value = text2; + document.getElementById('favoritesForm').id.value = id; } /** @@ -189,7 +190,7 @@ + onclick="set('${carlos:forJavaScript(favorite.query)}', '${carlos:forJavaScript(favorite.queryName)}', '${carlos:forJavaScript(favorite.id)}'); document.getElementById('favoritesForm').submit(); return false;"/> -<%@ page import="java.util.*,io.github.carlos_emr.carlos.report.data.*" %> <%@ taglib uri="jakarta.tags.fmt" prefix="fmt" %> +<%@ taglib uri="carlos" prefix="carlos" %> - - + - - + + <fmt:message key="oscarReport.RptByExample.MsgQueryByExamples"/> - <fmt:message key="oscarReport.RptByExample.MsgEditMyFavorite"/> - - + + +
"; + private static final String CELL_START = ""; public record StructuredResult(String html, int rowCount, boolean truncated, boolean rowLimitReached) { } @@ -92,72 +97,94 @@ public static StructuredResult getStructureWithCount(ResultSet rs, int maxOutput */ public static StructuredResult getStructureWithCount(ResultSet rs, int maxOutputCharacters, int maxRows) throws SQLException { - if (maxOutputCharacters < MIN_OUTPUT_CHARACTERS) { - throw new IllegalArgumentException("HTML output limit is too small"); - } - if (maxRows < 1) { - throw new IllegalArgumentException("Row limit must be positive"); - } - - // assuming multiple rows in rs + validateLimits(maxOutputCharacters, maxRows); LimitedHtmlBuilder html = new LimitedHtmlBuilder(maxOutputCharacters); - ResultSetMetaData rsmd = rs.getMetaData(); - int columns = rsmd.getColumnCount(); - String rowColor = "rowColor1"; html.appendMarkup("
@@ -97,16 +95,16 @@ - + - + - + onclick="history.back();"/> @@ -118,7 +116,6 @@
- diff --git a/src/test/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDaoIntegrationTest.java b/src/test/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDaoIntegrationTest.java index ffd9b91e269..56d9301c6a1 100644 --- a/src/test/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDaoIntegrationTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/commn/dao/ReportByExamplesFavoriteDaoIntegrationTest.java @@ -36,7 +36,7 @@ /** * Integration tests for {@link ReportByExamplesFavoriteDao} covering persist, - * findByQuery, findByEverything, and findByProvider. + * provider-scoped lookup, findByEverything, and findByProvider. * *

Migrated from legacy {@code ReportByExamplesFavoriteDaoTest} (JUnit 4 / DaoTestFixtures).

* @@ -92,32 +92,21 @@ void shouldFindFavorite_whenValidIdProvided() { } @Nested - @DisplayName("findByQuery") - class FindByQuery { + @DisplayName("findByProviderAndQuery") + class FindByProviderAndQuery { @Test @Tag("query") - @DisplayName("should return favorites with matching query string using LIKE") - void shouldReturnFavorites_whenQueryMatches() { - createFavorite("200001", "Fav1", "SELECT demographics"); - createFavorite("200002", "Fav2", "SELECT appointments"); - createFavorite("200003", "Fav3", "INSERT something"); + @DisplayName("should not return another provider's matching query") + void shouldReturnOnlyCurrentProviderFavorites_whenQueryMatches() { + createFavorite("200010", "Mine", "SELECT appointments"); + createFavorite("200011", "Theirs", "SELECT appointments"); - List results = dao.findByQuery("SELECT%"); + List results = + dao.findByProviderAndQuery("200010", "SELECT appointments"); - assertThat(results).hasSize(2); - assertThat(results).allMatch(f -> f.getQuery().startsWith("SELECT")); - } - - @Test - @Tag("query") - @DisplayName("should return empty list when no query matches") - void shouldReturnEmptyList_whenNoQueryMatches() { - createFavorite("200001", "Fav1", "SELECT something"); - - List results = dao.findByQuery("NO_MATCH%"); - - assertThat(results).isEmpty(); + assertThat(results).singleElement() + .satisfies(favorite -> assertThat(favorite.getName()).isEqualTo("Mine")); } } @@ -127,30 +116,27 @@ class FindByEverything { @Test @Tag("query") - @DisplayName("should return favorites matching provider and name") - void shouldReturnFavorites_whenProviderAndNameMatch() { + @DisplayName("should require provider, name, and query to match") + void shouldReturnFavorites_whenAllFieldsMatch() { createFavorite("300001", "MatchFav", "some query"); createFavorite("300001", "OtherFav", "other query"); createFavorite("300002", "MatchFav", "diff query"); - List results = dao.findByEverything("300001", "MatchFav", "NO_MATCH"); + List results = dao.findByEverything("300001", "MatchFav", "some query"); - assertThat(results).isNotEmpty(); - assertThat(results).anyMatch(f -> - f.getProviderNo().equals("300001") && f.getName().equals("MatchFav")); + assertThat(results).singleElement() + .satisfies(favorite -> assertThat(favorite.getProviderNo()).isEqualTo("300001")); } @Test @Tag("query") - @DisplayName("should return favorites matching query string via OR clause") - void shouldReturnFavorites_whenQueryStringMatchesViaOr() { + @DisplayName("should not return another provider's matching query") + void shouldReturnEmpty_whenOnlyQueryMatches() { createFavorite("300003", "SomeFav", "unique query string"); - // The findByEverything method uses OR for query: providerNo = ?1 AND name LIKE ?2 OR query LIKE ?3 List results = dao.findByEverything("NOPROVIDER", "NONAME", "unique query string"); - assertThat(results).isNotEmpty(); - assertThat(results).anyMatch(f -> f.getQuery().equals("unique query string")); + assertThat(results).isEmpty(); } } diff --git a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java index 675084e4d71..efa8e2f424d 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/ReportActionSecurityMigrationUnitTest.java @@ -28,6 +28,7 @@ import io.github.carlos_emr.carlos.commn.dao.ReportByExamplesFavoriteDao; import io.github.carlos_emr.carlos.commn.dao.ReportTemplatesDao; import io.github.carlos_emr.carlos.commn.model.ReportByExamples; +import io.github.carlos_emr.carlos.commn.model.ReportByExamplesFavorite; import io.github.carlos_emr.carlos.managers.SecurityInfoManager; import io.github.carlos_emr.carlos.report.data.RptByExampleData; import io.github.carlos_emr.carlos.report.pageUtil.RptByExample2Action; @@ -259,6 +260,61 @@ void shouldAllowFavoriteWrite_whenAuthorizedPostIsSubmitted() throws Exception { verify(favoritesDao).findByProvider("999998"); } + @Test + @DisplayName("RptByExamplesFavorite rejects deletion of another provider's favorite") + void shouldRejectFavoriteDelete_whenFavoriteBelongsToAnotherProvider() { + authorizeFavoritePost(); + ReportByExamplesFavorite favorite = favorite(42, "100001", "Other provider", "select 1"); + when(favoritesDao.find(42)).thenReturn(favorite); + + RptByExamplesFavorite2Action action = new RptByExamplesFavorite2Action(); + action.setToDelete("true"); + action.setId("42"); + + assertThatThrownBy(action::execute) + .isInstanceOf(SecurityException.class) + .hasMessage("Favorite does not belong to the current provider"); + verify(favoritesDao, never()).remove(favorite); + } + + @Test + @DisplayName("RptByExamplesFavorite rejects editing another provider's favorite") + void shouldRejectFavoriteEdit_whenFavoriteBelongsToAnotherProvider() { + authorizeFavoritePost(); + ReportByExamplesFavorite favorite = favorite(42, "100001", "Other provider", "select 1"); + when(favoritesDao.find(42)).thenReturn(favorite); + + RptByExamplesFavorite2Action action = new RptByExamplesFavorite2Action(); + action.setId("42"); + action.setNewName("Spoofed name"); + action.setNewQuery("select 2"); + + assertThatThrownBy(action::execute) + .isInstanceOf(SecurityException.class) + .hasMessage("Favorite does not belong to the current provider"); + verify(favoritesDao, never()).merge(favorite); + } + + @Test + @DisplayName("RptByExamplesFavorite updates only the current provider's selected favorite") + void shouldUpdateFavorite_whenFavoriteBelongsToCurrentProvider() throws Exception { + authorizeFavoritePost(); + ReportByExamplesFavorite favorite = favorite(42, "999998", "Old name", "select 1"); + when(favoritesDao.find(42)).thenReturn(favorite); + + RptByExamplesFavorite2Action action = new RptByExamplesFavorite2Action(); + action.setId("42"); + action.setFavoriteName("New name"); + action.setQuery("select 2"); + + assertThat(action.execute()).isEqualTo(ActionSupport.SUCCESS); + + assertThat(favorite.getName()).isEqualTo("New name"); + assertThat(favorite.getQuery()).isEqualTo("select 2"); + verify(favoritesDao).merge(favorite); + verify(favoritesDao).findByProvider("999998"); + } + @Test @DisplayName("RptByExample keeps successful results when query history cannot be saved") void shouldKeepResults_whenQueryHistorySaveFails() throws Exception { @@ -327,6 +383,22 @@ private void assertMissingLoggedInInfoFails(ActionSupport action) { .hasMessage(MISSING_ADMIN_OR_REPORT); } + private void authorizeFavoritePost() { + LoggedInInfo.setLoggedInInfoIntoSession(request.getSession(), loggedInInfo); + when(securityInfoManager.hasPrivilege(loggedInInfo, "_admin", SecurityInfoManager.READ, null)) + .thenReturn(true); + request.setMethod("POST"); + } + + private static ReportByExamplesFavorite favorite(int id, String providerNo, String name, String query) { + ReportByExamplesFavorite favorite = new ReportByExamplesFavorite(); + favorite.setId(id); + favorite.setProviderNo(providerNo); + favorite.setName(name); + favorite.setQuery(query); + return favorite; + } + private void assertMissingPrivilegeFails(ActionSupport action) { assertThatThrownBy(action::execute) .isInstanceOf(SecurityException.class) From a6e0988764d08c6461f54f58439276f49fdf5b91 Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 18:18:49 -0400 Subject: [PATCH 10/21] fix(report): harden favorite form markup Signed-off-by: Ben Heerema --- .../jsp/oscarReport/RptByExamplesAllFavorites.jsp | 14 ++++++++------ .../jsp/oscarReport/RptByExamplesFavorite.jsp | 8 ++++---- 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp index 979375e5b70..38843497af1 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesAllFavorites.jsp @@ -105,9 +105,10 @@ * @param {string} text2 - The display name of the favourite (JS-attribute-encoded by JSP) */ function set(text1, text2, id) { - document.getElementById('favoritesForm').newQuery.value = text1; - document.getElementById('favoritesForm').newName.value = text2; - document.getElementById('favoritesForm').id.value = id; + const form = document.getElementById('favoritesForm'); + form.elements['newQuery'].value = text1; + form.elements['newName'].value = text2; + form.elements['id'].value = id; } /** @@ -118,9 +119,10 @@ */ function confirmDelete(id) { if (confirm(msgConfirmDelete)) { - document.getElementById('favoritesForm').toDelete.value = 'true'; - document.getElementById('favoritesForm').id.value = id; - document.getElementById('favoritesForm').submit(); + const form = document.getElementById('favoritesForm'); + form.elements['toDelete'].value = 'true'; + form.elements['id'].value = id; + form.submit(); } } diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp index cecd8013384..af185c404cd 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp @@ -69,15 +69,15 @@ - + - + href="${carlos:forHtmlAttribute(pageContext.request.contextPath)}/encounter/encounterStyles.css"/> + <fmt:message key="oscarReport.RptByExample.MsgQueryByExamples"/> - <fmt:message key="oscarReport.RptByExample.MsgEditMyFavorite"/> -
+ From 2d4ad22a2d18913734181aa0aa6e7645b585df58 Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 21:29:50 -0400 Subject: [PATCH 11/21] test(report): include query tests in CI Signed-off-by: Ben Heerema --- ...lidatorTest.java => QueryByExampleSqlValidatorUnitTest.java} | 2 +- ...{RptByExampleDataTest.java => RptByExampleDataUnitTest.java} | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) rename src/test/java/io/github/carlos_emr/carlos/report/data/{QueryByExampleSqlValidatorTest.java => QueryByExampleSqlValidatorUnitTest.java} (99%) rename src/test/java/io/github/carlos_emr/carlos/report/data/{RptByExampleDataTest.java => RptByExampleDataUnitTest.java} (99%) diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorUnitTest.java similarity index 99% rename from src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java rename to src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorUnitTest.java index ecae24b2eb5..ce8597d794d 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidatorUnitTest.java @@ -36,7 +36,7 @@ @Tag("unit") @Tag("report") @Tag("security") -class QueryByExampleSqlValidatorTest { +class QueryByExampleSqlValidatorUnitTest { private final Properties properties = properties("oscar_mcmaster?useUnicode=true"); @Test diff --git a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataUnitTest.java similarity index 99% rename from src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java rename to src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataUnitTest.java index c54023f1b01..0e475f30597 100644 --- a/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataTest.java +++ b/src/test/java/io/github/carlos_emr/carlos/report/data/RptByExampleDataUnitTest.java @@ -48,7 +48,7 @@ @Tag("unit") @Tag("report") @Tag("security") -class RptByExampleDataTest { +class RptByExampleDataUnitTest { private Connection connection; private PreparedStatement statement; private ResultSet resultSet; From 2f99beb29f363853d4483244f26397df3e7c83fc Mon Sep 17 00:00:00 2001 From: Ben Heerema Date: Thu, 6 Aug 2026 22:04:41 -0400 Subject: [PATCH 12/21] fix(report): address live quality findings Signed-off-by: Ben Heerema --- .../carlos_emr/carlos/db/LegacyJdbcQuery.java | 34 +++-- .../data/QueryByExampleSqlValidator.java | 33 ++--- .../carlos/report/data/RptByExampleData.java | 97 ++++++++----- .../carlos/report/data/RptResultStruct.java | 129 +++++++++++------- .../jsp/oscarReport/RptByExamplesFavorite.jsp | 19 ++- ...torActionGetRejectionContractUnitTest.java | 2 + .../report/data/RptByExampleDataUnitTest.java | 56 +++++++- 7 files changed, 241 insertions(+), 129 deletions(-) diff --git a/src/main/java/io/github/carlos_emr/carlos/db/LegacyJdbcQuery.java b/src/main/java/io/github/carlos_emr/carlos/db/LegacyJdbcQuery.java index cdfc724c7ac..4dfeec882c1 100644 --- a/src/main/java/io/github/carlos_emr/carlos/db/LegacyJdbcQuery.java +++ b/src/main/java/io/github/carlos_emr/carlos/db/LegacyJdbcQuery.java @@ -619,32 +619,36 @@ private static boolean containsSqlWord(String sql, String word) { private static String stripQuotedSqlSections(String sql) { StringBuilder stripped = new StringBuilder(sql.length()); char quote = '\0'; - for (int i = 0; i < sql.length(); i++) { + int i = 0; + while (i < sql.length()) { char current = sql.charAt(i); char next = i + 1 < sql.length() ? sql.charAt(i + 1) : '\0'; if (quote == '\0') { - if (current == '\'' || current == '"' || current == '`') { - quote = current; - stripped.append(' '); - } else { - stripped.append(current); - } - } else if (quote != '`' && current == '\\' && next != '\0') { - stripped.append(" "); + quote = sqlQuoteDelimiter(current); + stripped.append(quote == '\0' ? current : ' '); i++; - } else if (current == quote && next == quote) { + continue; + } + boolean escapedPair = (quote != '`' && current == '\\' && next != '\0') + || (current == quote && next == quote); + if (escapedPair) { stripped.append(" "); - i++; - } else if (current == quote) { + i += 2; + continue; + } + if (current == quote) { quote = '\0'; - stripped.append(' '); - } else { - stripped.append(' '); } + stripped.append(' '); + i++; } return stripped.toString(); } + private static char sqlQuoteDelimiter(char candidate) { + return candidate == '\'' || candidate == '"' || candidate == '`' ? candidate : '\0'; + } + private static boolean startsWithSqlWord(String sql, String word) { return sql.startsWith(word) && (sql.length() == word.length() || !isSqlIdentifierPart(sql.charAt(word.length()))); diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java index 6dde970d170..94f40fece6e 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/QueryByExampleSqlValidator.java @@ -212,28 +212,29 @@ private static String stripQuotedSections(String sql) { char current = sql.charAt(i); char next = i + 1 < sql.length() ? sql.charAt(i + 1) : '\0'; if (quote == '\0') { - if (current == '\'' || current == '"' || current == '`') { - quote = current; - stripped.append(' '); - } else { - stripped.append(current); - } - } else { - if (current == quote && next == quote) { - stripped.append(" "); - i += 2; - continue; - } - if (current == quote) { - quote = '\0'; - } - stripped.append(' '); + quote = sqlQuoteDelimiter(current); + stripped.append(quote == '\0' ? current : ' '); + i++; + continue; + } + if (current == quote && next == quote) { + stripped.append(" "); + i += 2; + continue; } + if (current == quote) { + quote = '\0'; + } + stripped.append(' '); i++; } return stripped.toString(); } + private static char sqlQuoteDelimiter(char candidate) { + return candidate == '\'' || candidate == '"' || candidate == '`' ? candidate : '\0'; + } + private static String unquoteIdentifier(String identifier) { return identifier.replace("`", "").replace("\"", "").trim(); } diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java b/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java index c203d0eebd8..11c0f879a63 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/RptByExampleData.java @@ -81,38 +81,8 @@ public QueryResult execute(String sql, Properties properties, String providerNo) int rowCount = 0; try { LegacyJdbcQuery.TrustedSql trustedSql = QueryByExampleSqlValidator.validate(sql, properties); - QueryResult queryResult; - try (Connection connection = connectionProvider.getConnection()) { - boolean originalReadOnly = connection.isReadOnly(); - Exception executionFailure = null; - try { - connection.setReadOnly(true); - try (PreparedStatement statement = prepareValidatedStatement(connection, trustedSql)) { - statement.setMaxRows(MAX_ROWS + 1); - statement.setQueryTimeout(QUERY_TIMEOUT_SECONDS); - try (ResultSet resultSet = statement.executeQuery()) { - RptResultStruct.StructuredResult structured = RptResultStruct.getStructureWithCount( - resultSet, MAX_OUTPUT_CHARACTERS, MAX_ROWS); - rowCount = structured.rowCount(); - queryResult = new QueryResult(structured.html(), rowCount, structured.truncated(), - structured.rowLimitReached(), 0); - } - } - } catch (SQLException | RuntimeException e) { - executionFailure = e; - throw e; - } finally { - try { - connection.setReadOnly(originalReadOnly); - } catch (SQLException restoreFailure) { - if (executionFailure != null) { - executionFailure.addSuppressed(restoreFailure); - } else { - throw restoreFailure; - } - } - } - } + QueryResult queryResult = executeWithConnection(trustedSql); + rowCount = queryResult.rowCount(); outcome = "success"; return new QueryResult(queryResult.html(), queryResult.rowCount(), queryResult.truncated(), queryResult.rowLimitReached(), elapsedMillis(startedAt)); @@ -143,6 +113,61 @@ private static PreparedStatement prepareValidatedStatement(Connection connection ResultSet.CONCUR_READ_ONLY); // nosemgrep: java.lang.security.audit.formatted-sql-string-deepsemgrep.formatted-sql-string-deepsemgrep -- validated TrustedSql boundary } + private QueryResult executeWithConnection(LegacyJdbcQuery.TrustedSql trustedSql) throws SQLException { + try (Connection connection = connectionProvider.getConnection()) { + return executeValidatedQuery(connection, trustedSql); + } + } + + private static QueryResult executeValidatedQuery(Connection connection, LegacyJdbcQuery.TrustedSql trustedSql) + throws SQLException { + boolean originalReadOnly = connection.isReadOnly(); + try { + connection.setReadOnly(true); + } catch (SQLException setupFailure) { + restoreReadOnlyAfterFailure(connection, originalReadOnly, setupFailure); + throw setupFailure; + } + QueryResult result; + try { + result = executeStatement(connection, trustedSql); + } catch (SQLException | RuntimeException executionFailure) { + restoreReadOnlyAfterFailure(connection, originalReadOnly, executionFailure); + throw executionFailure; + } + connection.setReadOnly(originalReadOnly); + return result; + } + + private static QueryResult executeStatement(Connection connection, LegacyJdbcQuery.TrustedSql trustedSql) + throws SQLException { + try (PreparedStatement statement = prepareValidatedStatement(connection, trustedSql)) { + statement.setMaxRows(MAX_ROWS + 1); + statement.setQueryTimeout(QUERY_TIMEOUT_SECONDS); + return readStatementResult(statement); + } + } + + private static QueryResult readStatementResult(PreparedStatement statement) throws SQLException { + try (ResultSet resultSet = statement.executeQuery()) { + RptResultStruct.StructuredResult structured = RptResultStruct.getStructureWithCount( + resultSet, MAX_OUTPUT_CHARACTERS, MAX_ROWS); + return new QueryResult(structured.html(), structured.rowCount(), structured.truncated(), + structured.rowLimitReached(), 0); + } + } + + private static void restoreReadOnlyAfterFailure(Connection connection, boolean originalReadOnly, + Exception executionFailure) { + try { + connection.setReadOnly(originalReadOnly); + } catch (SQLException restoreFailure) { + if (restoreFailure != executionFailure) { + executionFailure.addSuppressed(restoreFailure); + } + } + } + public static void audit(String providerNo, String sql, long durationMillis, int rowCount, String outcome) { String query = sql == null ? "" : sql; String queryHash = queryHash(query); @@ -156,9 +181,11 @@ private static long elapsedMillis(long startedAt) { } private static void logFailure(String providerNo, String sql, String sqlState, String exceptionType) { - MiscUtils.getLogger().warn( - "Query-by-Example failure provider={} queryHash={} sqlState={} exceptionType={}", - providerNo, queryHash(sql), sqlState, exceptionType); + if (MiscUtils.getLogger().isWarnEnabled()) { + MiscUtils.getLogger().warn( + "Query-by-Example failure provider={} queryHash={} sqlState={} exceptionType={}", + providerNo, queryHash(sql), sqlState, exceptionType); + } } private static String queryHash(String sql) { diff --git a/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java b/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java index 2a1f73c4d98..4e107e6175b 100644 --- a/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java +++ b/src/main/java/io/github/carlos_emr/carlos/report/data/RptResultStruct.java @@ -50,6 +50,11 @@ public class RptResultStruct { private static final int MIN_OUTPUT_CHARACTERS = 64; private static final int CLOSING_MARKUP_RESERVE = 32; + private static final String TABLE_END = "
"; + private static final String HEADER_END = ""; + private static final String ROW_END = "
"; + private static final String CELL_END = "
"); - for (int i = 0; i < columns; i++) { // for each column in result set - if (!html.appendMarkup("
") - || !html.appendEncoded(rsmd.getColumnLabel(i + 1))) { - html.appendClosingMarkup("
"); - return new StructuredResult(html.toString(), 0, true, false); - } - if (!html.appendMarkup("")) { - html.appendClosingMarkup(""); - return new StructuredResult(html.toString(), 0, true, false); - } + if (!appendHeaders(html, rsmd, columns)) { + html.appendClosingMarkup(TABLE_END); + return new StructuredResult(html.toString(), 0, true, false); } + int rowCount = 0; boolean stopRendering = false; + String rowColor = "rowColor1"; while (!stopRendering && rowCount < maxRows && rs.next()) { rowCount++; - if (!html.appendMarkup("")) { - break; - } - for (int j = 0; j < columns; j++) { - if (!html.appendMarkup("")) { - stopRendering = true; - break; - } - try (Reader value = rs.getCharacterStream(j + 1)) { - if (value != null && !html.appendEncoded(value)) { - stopRendering = true; - } - } catch (IOException e) { - throw new SQLException("Could not render query result", e); - } - if (stopRendering) { - html.appendClosingMarkup(""); - break; - } - if (!html.appendMarkup("")) { - html.appendClosingMarkup(""); - stopRendering = true; - break; - } - } + stopRendering = !appendRow(html, rs, columns, rowColor); rowColor = rowColor.equals("rowColor1") ? "rowColor2" : "rowColor1"; - if (stopRendering || !html.appendMarkup("")) { - html.appendClosingMarkup(""); - stopRendering = true; - } } boolean rowLimitReached = !stopRendering && rowCount == maxRows && rs.next(); - html.appendClosingMarkup(""); + html.appendClosingMarkup(TABLE_END); return new StructuredResult(html.toString(), rowCount, html.isTruncated(), rowLimitReached); } + private static void validateLimits(int maxOutputCharacters, int maxRows) { + if (maxOutputCharacters < MIN_OUTPUT_CHARACTERS) { + throw new IllegalArgumentException("HTML output limit is too small"); + } + if (maxRows < 1) { + throw new IllegalArgumentException("Row limit must be positive"); + } + } + + private static boolean appendHeaders(LimitedHtmlBuilder html, ResultSetMetaData metadata, int columns) + throws SQLException { + for (int i = 1; i <= columns; i++) { + if (!appendHeader(html, metadata.getColumnLabel(i))) { + return false; + } + } + return true; + } + + private static boolean appendHeader(LimitedHtmlBuilder html, String label) { + if (!html.appendMarkup("")) { + return false; + } + if (!html.appendEncoded(label) || !html.appendMarkup(HEADER_END)) { + html.appendClosingMarkup(HEADER_END); + return false; + } + return true; + } + + private static boolean appendRow(LimitedHtmlBuilder html, ResultSet resultSet, int columns, String rowColor) + throws SQLException { + if (!html.appendMarkup("")) { + return false; + } + for (int column = 1; column <= columns; column++) { + if (!appendCell(html, resultSet, column)) { + html.appendClosingMarkup(ROW_END); + return false; + } + } + if (!html.appendMarkup(ROW_END)) { + html.appendClosingMarkup(ROW_END); + return false; + } + return true; + } + + private static boolean appendCell(LimitedHtmlBuilder html, ResultSet resultSet, int column) throws SQLException { + if (!html.appendMarkup(CELL_START)) { + return false; + } + boolean complete; + try (Reader value = resultSet.getCharacterStream(column)) { + complete = value == null || html.appendEncoded(value); + } catch (IOException e) { + throw new SQLException("Could not render query result", e); + } + if (!complete || !html.appendMarkup(CELL_END)) { + html.appendClosingMarkup(CELL_END); + return false; + } + return true; + } + private static final class LimitedHtmlBuilder { private static final int READ_BUFFER_SIZE = 2_048; diff --git a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp index af185c404cd..ee26e86e1ea 100644 --- a/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp +++ b/src/main/webapp/WEB-INF/jsp/oscarReport/RptByExamplesFavorite.jsp @@ -79,11 +79,11 @@ - +