From 55f0361fd5802526bdfdfa15592cb58474b9b2b2 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Mon, 22 Jun 2026 15:54:24 -0600 Subject: [PATCH 1/5] Fix stored XSS in finance notification charge summary report FinanceNotification.createChargeSummaryReport is a divergent copy of the ehr_billing BillingNotification report and interpolated editor-entered database values (investigator, project, debitedAccount, projectNumber, category) and their derived URLs directly into HTML without escaping. That HTML is rendered verbatim into the LDK RunNotificationAction admin preview via HtmlString.unsafe and is also sent as the HTML email body, so a stored payload in any of those project/alias fields executed in the browser of any user who previewed the notification or received the email. Wrap every tainted value with PageFlowUtil.filter in both the per-financial-analyst tables and the top category summary table (which was also unescaped in this copy), matching the fix applied to BillingNotification. --- .../notification/FinanceNotification.java | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/onprc_billing/src/org/labkey/onprc_billing/notification/FinanceNotification.java b/onprc_billing/src/org/labkey/onprc_billing/notification/FinanceNotification.java index 3ebd9db8a..090fbcc9a 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/notification/FinanceNotification.java +++ b/onprc_billing/src/org/labkey/onprc_billing/notification/FinanceNotification.java @@ -39,6 +39,7 @@ import org.labkey.api.query.QueryService; import org.labkey.api.query.UserSchema; import org.labkey.api.security.User; +import org.labkey.api.util.PageFlowUtil; import org.labkey.onprc_billing.ONPRC_BillingManager; import org.labkey.onprc_billing.ONPRC_BillingModule; import org.labkey.onprc_billing.ONPRC_BillingSchema; @@ -429,7 +430,7 @@ protected void writeResultTable(final StringBuilder msg, Date lastInvoiceEnd, Ca Container container = containerMap.get(category); String url = getExecuteQueryUrl(container, ONPRC_BillingSchema.NAME, categoryToQuery.get(category), null) + "&query.param.StartDate=" + getDateFormat(c).format(start.getTime()) + "&query.param.EndDate=" + getDateFormat(c).format(endDate.getTime()); - msg.append("" + category + "" + totalsMap.get("total") + "" + _dollarFormat.format(totalsMap.get("totalCost")) + ""); + msg.append("" + PageFlowUtil.filter(category) + "" + totalsMap.get("total") + "" + _dollarFormat.format(totalsMap.get("totalCost")) + ""); } msg.append("

"); @@ -474,8 +475,8 @@ protected void writeResultTable(final StringBuilder msg, Date lastInvoiceEnd, Ca String baseUrl = getExecuteQueryUrl(containerMap.get(category), ONPRC_BillingSchema.NAME, categoryToQuery.get(category), null) + "&query.param.StartDate=" + getDateFormat(c).format(start.getTime()) + "&query.param.EndDate=" + getDateFormat(c).format(endDate.getTime()); String projUrl = baseUrl + ("None".equals(tokens[1]) ? "&query.project/displayName~isblank" : "&query.project/displayName~eq=" + tokens[1]); - msg.append("" + financialAnalyst + ""); //the FA - msg.append("" + tokens[1] + ""); + msg.append("" + PageFlowUtil.filter(financialAnalyst) + ""); //the FA + msg.append("" + PageFlowUtil.filter(tokens[1]) + ""); //alias String accountUrl = null; @@ -487,22 +488,22 @@ protected void writeResultTable(final StringBuilder msg, Date lastInvoiceEnd, Ca if (accountUrl != null) { - msg.append("" + tokens[2] + ""); + msg.append("" + PageFlowUtil.filter(tokens[2]) + ""); } else { - msg.append("" + (tokens[2]) + ""); + msg.append("" + PageFlowUtil.filter(tokens[2]) + ""); } - msg.append("" + (tokens[3]) + ""); - msg.append("" + category + ""); + msg.append("" + PageFlowUtil.filter(tokens[3]) + ""); + msg.append("" + PageFlowUtil.filter(category) + ""); for (FieldDescriptor fd : foundCols) { if (totals.containsKey(fd.getFieldName())) { String url = projUrl + fd.getFilter(); - msg.append("" + totals.get(fd.getFieldName()) + ""); + msg.append("" + totals.get(fd.getFieldName()) + ""); } else { From 334c507fd73221e0aeeb05a9a7124d22fcc0d372 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Mon, 22 Jun 2026 16:04:38 -0600 Subject: [PATCH 2/5] Fix stored XSS in DCM finance notification charge summary report DCMFinanceNotification overrides FinanceNotification.writeResultTable with its own copy of the report, so the escaping fix to the parent class did not cover it. It concatenated editor-entered database values (project, alias/account, OGA project number, category) and their derived URLs directly into HTML without escaping, reaching the same LDK RunNotificationAction admin preview (HtmlString.unsafe) and HTML email sink. It is registered as an active notification in ONPRC_BillingModule. Wrap every tainted value with PageFlowUtil.filter in both the per-project tables and the top category summary table, mapping to this override's token layout (tokens[0] project, tokens[1] alias, tokens[2] OGA project number). --- .../notification/DCMFinanceNotification.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/onprc_billing/src/org/labkey/onprc_billing/notification/DCMFinanceNotification.java b/onprc_billing/src/org/labkey/onprc_billing/notification/DCMFinanceNotification.java index 2da58ca5c..4b70e2d1b 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/notification/DCMFinanceNotification.java +++ b/onprc_billing/src/org/labkey/onprc_billing/notification/DCMFinanceNotification.java @@ -2,6 +2,7 @@ import org.apache.commons.lang3.StringUtils; import org.labkey.api.data.Container; +import org.labkey.api.util.PageFlowUtil; import org.labkey.onprc_billing.ONPRC_BillingManager; import org.labkey.onprc_billing.ONPRC_BillingSchema; @@ -57,7 +58,7 @@ protected void writeResultTable(final StringBuilder msg, Date lastInvoiceEnd, Ca Container container = containerMap.get(category); String url = getExecuteQueryUrl(container, ONPRC_BillingSchema.NAME, categoryToQuery.get(category), null) + "&query.param.StartDate=" + getDateFormat(c).format(start.getTime()) + "&query.param.EndDate=" + getDateFormat(c).format(endDate.getTime()); - msg.append("" + category + "" + totalsMap.get("total") + "" + _dollarFormat.format(totalsMap.get("totalCost")) + ""); + msg.append("" + PageFlowUtil.filter(category) + "" + totalsMap.get("total") + "" + _dollarFormat.format(totalsMap.get("totalCost")) + ""); } msg.append("

"); @@ -141,7 +142,7 @@ protected void writeResultTable(final StringBuilder msg, Date lastInvoiceEnd, Ca String baseUrl = getExecuteQueryUrl(containerMap.get(category), ONPRC_BillingSchema.NAME, categoryToQuery.get(category), null) + "&query.param.StartDate=" + getDateFormat(c).format(start.getTime()) + "&query.param.EndDate=" + getDateFormat(c).format(endDate.getTime()); String projUrl = baseUrl + ("None".equals(tokens[0]) ? "&query.project/displayName~isblank" : "&query.project/displayName~eq=" + tokens[0]); - msg.append("" + tokens[0] + ""); + msg.append("" + PageFlowUtil.filter(tokens[0]) + ""); //alias String accountUrl = null; @@ -153,22 +154,22 @@ protected void writeResultTable(final StringBuilder msg, Date lastInvoiceEnd, Ca if (accountUrl != null) { - msg.append("" + tokens[1] + ""); + msg.append("" + PageFlowUtil.filter(tokens[1]) + ""); } else { - msg.append("" + (tokens[1]) + ""); + msg.append("" + PageFlowUtil.filter(tokens[1]) + ""); } - msg.append("" + (tokens[2]) + ""); - msg.append("" + category + ""); + msg.append("" + PageFlowUtil.filter(tokens[2]) + ""); + msg.append("" + PageFlowUtil.filter(category) + ""); for (FieldDescriptor fd : toShow) { if (totals.containsKey(fd.getFieldName())) { String url = projUrl + fd.getFilter(); - msg.append("" + totals.get(fd.getFieldName()) + ""); + msg.append("" + totals.get(fd.getFieldName()) + ""); } else { From 1ad773cb6de28fe5b31a59d4e257316dc70e8cc5 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Wed, 24 Jun 2026 12:56:15 -0600 Subject: [PATCH 3/5] Remove DCMFinanceNotification and its registration --- .../onprc_billing/ONPRC_BillingModule.java | 2 - .../notification/DCMFinanceNotification.java | 187 ------------------ 2 files changed, 189 deletions(-) delete mode 100644 onprc_billing/src/org/labkey/onprc_billing/notification/DCMFinanceNotification.java diff --git a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java index d46499053..5e4a10903 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java +++ b/onprc_billing/src/org/labkey/onprc_billing/ONPRC_BillingModule.java @@ -49,7 +49,6 @@ import org.labkey.onprc_billing.dataentry.ChargesARTCoreFormType; import org.labkey.onprc_billing.dataentry.ReversalFormType; import org.labkey.onprc_billing.notification.BillingValidationNotification; -import org.labkey.onprc_billing.notification.DCMFinanceNotification; import org.labkey.onprc_billing.notification.FinanceNotification; import org.labkey.onprc_billing.pipeline.BillingPipelineProvider; import org.labkey.onprc_billing.query.BillingAuditProvider; @@ -119,7 +118,6 @@ protected void doStartupAfterSpringConfig(ModuleContext moduleContext) PipelineService.get().registerPipelineProvider(new BillingPipelineProvider(this)); NotificationService.get().registerNotification(new FinanceNotification()); - NotificationService.get().registerNotification(new DCMFinanceNotification()); NotificationService.get().registerNotification(new BillingValidationNotification()); EHRService.get().registerTableCustomizer(this, ONPRC_BillingCustomizer.class); diff --git a/onprc_billing/src/org/labkey/onprc_billing/notification/DCMFinanceNotification.java b/onprc_billing/src/org/labkey/onprc_billing/notification/DCMFinanceNotification.java deleted file mode 100644 index 4b70e2d1b..000000000 --- a/onprc_billing/src/org/labkey/onprc_billing/notification/DCMFinanceNotification.java +++ /dev/null @@ -1,187 +0,0 @@ -package org.labkey.onprc_billing.notification; - -import org.apache.commons.lang3.StringUtils; -import org.labkey.api.data.Container; -import org.labkey.api.util.PageFlowUtil; -import org.labkey.onprc_billing.ONPRC_BillingManager; -import org.labkey.onprc_billing.ONPRC_BillingSchema; - -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Calendar; -import java.util.Date; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.TreeMap; - -/** - - */ -public class DCMFinanceNotification extends FinanceNotification -{ - @Override - public String getName() - { - return "DCM Finance Notification"; - } - - @Override - public String getEmailSubject(Container c) - { - return "DCM Finance/Billing Alerts: " + getDateTimeFormat(c).format(new Date()); - } - - @Override - public String getCronString() - { - return "0 30 7 * * ?"; - } - - @Override - public String getScheduleDescription() - { - return "every day at 7:30AM"; - } - - @Override - protected void writeResultTable(final StringBuilder msg, Date lastInvoiceEnd, Calendar start, Calendar endDate, final Map>>> dataMap, final Map> totalsByCategory, Map categoryToQuery, Map containerMap, Container c) - { - msg.append("Charge Summary:

"); - msg.append("The table below summarizes projected charges since the since the last invoice date of " + getDateFormat(c).format(lastInvoiceEnd)); - - msg.append(""); - for (String category : totalsByCategory.keySet()) - { - Map totalsMap = totalsByCategory.get(category); - Container container = containerMap.get(category); - - String url = getExecuteQueryUrl(container, ONPRC_BillingSchema.NAME, categoryToQuery.get(category), null) + "&query.param.StartDate=" + getDateFormat(c).format(start.getTime()) + "&query.param.EndDate=" + getDateFormat(c).format(endDate.getTime()); - msg.append(""); - } - msg.append("
Category# ItemsAmount
" + PageFlowUtil.filter(category) + "" + totalsMap.get("total") + "" + _dollarFormat.format(totalsMap.get("totalCost")) + "


"); - - msg.append("The tables below highlight any suspicious or abnormal items, grouped by project. These will not necessarily be problems, but may warrant investigation.

"); - - //NOTE: this is a slightly odd approach, but the primary notification groups these data by FA. we take this map and group it instead by project/alias - Map>> newDataMap = new TreeMap<>(); - Set foundCols = new LinkedHashSet<>(); - for (String financialAnalyst : dataMap.keySet()) - { - //first collect all distinct columns we will need to show - outerloop: - for (FieldDescriptor fd : _fields) - { - for (String key : dataMap.get(financialAnalyst).keySet()) - { - Map> projectDataByCategory = dataMap.get(financialAnalyst).get(key); - for (String category : projectDataByCategory.keySet()) - { - if (projectDataByCategory.get(category).containsKey(fd.getFieldName())) - { - foundCols.add(fd); - continue outerloop; - } - } - } - } - - //then transform the initial Map based on FA into one based on project/alias - for (String key : dataMap.get(financialAnalyst).keySet()) - { - List tokens = new ArrayList<>(Arrays.asList(key.split("<>"))); - tokens.remove(0); //remove FA - String newKey = StringUtils.join(tokens, "<>"); - Map> newDataByCategory = newDataMap.get(newKey); - if (newDataByCategory == null) - newDataByCategory = new TreeMap<>(); - - Map> dataByCategory = dataMap.get(financialAnalyst).get(key); - for (String category : dataByCategory.keySet()) - { - Map newTotals = newDataByCategory.get(category); - if (newTotals == null) - newTotals = new TreeMap<>(); - - Map totals = dataByCategory.get(category); - for (String t : totals.keySet()) - { - Integer newVal = newTotals.containsKey(t) ? newTotals.get(t) : 0; - newVal += totals.get(t); - newTotals.put(t, newVal); - } - - newDataByCategory.put(category, newTotals); - newDataMap.put(newKey, newDataByCategory); - } - } - } - - //reorder columns based on initial order - LinkedHashSet toShow = new LinkedHashSet<>(); - toShow.addAll(Arrays.asList(_fields)); - toShow.retainAll(foundCols); - - //now build the table itself - msg.append(""); - for (FieldDescriptor fd : toShow) - { - msg.append(""); - } - msg.append(""); - - //and append rows - for (String key : newDataMap.keySet()) - { - String[] tokens = key.split("<>"); - Map> dataByCategory = newDataMap.get(key); - for (String category : dataByCategory.keySet()) - { - Map totals = dataByCategory.get(category); - - String baseUrl = getExecuteQueryUrl(containerMap.get(category), ONPRC_BillingSchema.NAME, categoryToQuery.get(category), null) + "&query.param.StartDate=" + getDateFormat(c).format(start.getTime()) + "&query.param.EndDate=" + getDateFormat(c).format(endDate.getTime()); - String projUrl = baseUrl + ("None".equals(tokens[0]) ? "&query.project/displayName~isblank" : "&query.project/displayName~eq=" + tokens[0]); - msg.append(""); - - //alias - String accountUrl = null; - Container financeContainer = ONPRC_BillingManager.get().getBillingContainer(containerMap.get(category)); - if (financeContainer != null && !"Unknown".equals((tokens[1]))) - { - accountUrl = getExecuteQueryUrl(financeContainer, ONPRC_BillingSchema.NAME, "aliases", null, null) + "&query.alias~eq=" + tokens[1]; - } - - if (accountUrl != null) - { - msg.append(""); - } - else - { - msg.append(""); - } - - msg.append(""); - msg.append(""); - - for (FieldDescriptor fd : toShow) - { - if (totals.containsKey(fd.getFieldName())) - { - String url = projUrl + fd.getFilter(); - msg.append("" + totals.get(fd.getFieldName()) + ""); - } - else - { - msg.append(""); - } - } - - msg.append(""); - } - } - - msg.append("
ProjectAliasOGA ProjectCategory" + fd.getLabel() + "
" + PageFlowUtil.filter(tokens[0]) + "" + PageFlowUtil.filter(tokens[1]) + "" + PageFlowUtil.filter(tokens[1]) + "" + PageFlowUtil.filter(tokens[2]) + "" + PageFlowUtil.filter(category) + "


"); - msg.append("

"); - } -} From 12721c91eea1cc2eee273d2584c0270556869c8e Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Tue, 4 Aug 2026 05:19:19 -0600 Subject: [PATCH 4/5] URL-encode query parameter values in finance notification links PageFlowUtil.filter escapes the HTML metacharacters but leaves %, #, + and space untouched, so HTML-encoding the assembled URL protected the href attribute without making the appended query parameter values well-formed. A project name or alias containing % produced an invalid percent-escape; one containing & split into an extra query parameter, because the browser decodes the escaped ampersand back to a literal & when parsing the attribute; and # truncated the query string at a fragment. Each appended value is now wrapped in PageFlowUtil.encodeURIComponent so it is percent-encoded before PageFlowUtil.filter encodes the resulting URL for the attribute context. The two layers address different contexts and both are needed. Also encodes the StartDate and EndDate parameter values, which are formatted with the admin-configured LookAndFeel date format and can therefore contain spaces. This is a link-correctness fix rather than a residual XSS: filter already prevented breaking out of the quoted href, and the scheme and host come from urlExecuteQuery. Raised in code review. --- .../onprc_billing/notification/FinanceNotification.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/onprc_billing/src/org/labkey/onprc_billing/notification/FinanceNotification.java b/onprc_billing/src/org/labkey/onprc_billing/notification/FinanceNotification.java index 090fbcc9a..f1a112165 100644 --- a/onprc_billing/src/org/labkey/onprc_billing/notification/FinanceNotification.java +++ b/onprc_billing/src/org/labkey/onprc_billing/notification/FinanceNotification.java @@ -429,7 +429,7 @@ protected void writeResultTable(final StringBuilder msg, Date lastInvoiceEnd, Ca Map totalsMap = totalsByCategory.get(category); Container container = containerMap.get(category); - String url = getExecuteQueryUrl(container, ONPRC_BillingSchema.NAME, categoryToQuery.get(category), null) + "&query.param.StartDate=" + getDateFormat(c).format(start.getTime()) + "&query.param.EndDate=" + getDateFormat(c).format(endDate.getTime()); + String url = getExecuteQueryUrl(container, ONPRC_BillingSchema.NAME, categoryToQuery.get(category), null) + "&query.param.StartDate=" + PageFlowUtil.encodeURIComponent(getDateFormat(c).format(start.getTime())) + "&query.param.EndDate=" + PageFlowUtil.encodeURIComponent(getDateFormat(c).format(endDate.getTime())); msg.append("" + PageFlowUtil.filter(category) + "" + totalsMap.get("total") + "" + _dollarFormat.format(totalsMap.get("totalCost")) + ""); } msg.append("

"); @@ -473,8 +473,8 @@ protected void writeResultTable(final StringBuilder msg, Date lastInvoiceEnd, Ca { Map totals = dataByCategory.get(category); - String baseUrl = getExecuteQueryUrl(containerMap.get(category), ONPRC_BillingSchema.NAME, categoryToQuery.get(category), null) + "&query.param.StartDate=" + getDateFormat(c).format(start.getTime()) + "&query.param.EndDate=" + getDateFormat(c).format(endDate.getTime()); - String projUrl = baseUrl + ("None".equals(tokens[1]) ? "&query.project/displayName~isblank" : "&query.project/displayName~eq=" + tokens[1]); + String baseUrl = getExecuteQueryUrl(containerMap.get(category), ONPRC_BillingSchema.NAME, categoryToQuery.get(category), null) + "&query.param.StartDate=" + PageFlowUtil.encodeURIComponent(getDateFormat(c).format(start.getTime())) + "&query.param.EndDate=" + PageFlowUtil.encodeURIComponent(getDateFormat(c).format(endDate.getTime())); + String projUrl = baseUrl + ("None".equals(tokens[1]) ? "&query.project/displayName~isblank" : "&query.project/displayName~eq=" + PageFlowUtil.encodeURIComponent(tokens[1])); msg.append("" + PageFlowUtil.filter(financialAnalyst) + ""); //the FA msg.append("" + PageFlowUtil.filter(tokens[1]) + ""); @@ -483,7 +483,7 @@ protected void writeResultTable(final StringBuilder msg, Date lastInvoiceEnd, Ca Container financeContainer = ONPRC_BillingManager.get().getBillingContainer(containerMap.get(category)); if (financeContainer != null && !"Unknown".equals((tokens[2]))) { - accountUrl = getExecuteQueryUrl(financeContainer, ONPRC_BillingSchema.NAME, "aliases", null, null) + "&query.alias~eq=" + tokens[2]; + accountUrl = getExecuteQueryUrl(financeContainer, ONPRC_BillingSchema.NAME, "aliases", null, null) + "&query.alias~eq=" + PageFlowUtil.encodeURIComponent(tokens[2]); } if (accountUrl != null) From 836f7da634140f5f4c7713dbd875fed6b2fff746 Mon Sep 17 00:00:00 2001 From: Marty Pradere Date: Tue, 4 Aug 2026 22:06:53 -0600 Subject: [PATCH 5/5] Remove deleted DCM Finance Notification from ONPRC billing test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The testNotifications set still named "DCM Finance Notification", whose class and NotificationService registration were removed earlier on this branch. The test iterates the notification admin page and only acts on rows whose label is in the set, so the stale name produced no failure — it simply never matched, silently dropping that assertion instead of reporting the lost coverage. --- .../src/org/labkey/test/tests/onprc_ehr/ONPRC_BillingTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/onprc_ehr/test/src/org/labkey/test/tests/onprc_ehr/ONPRC_BillingTest.java b/onprc_ehr/test/src/org/labkey/test/tests/onprc_ehr/ONPRC_BillingTest.java index 2d2c2eecc..7febbe253 100644 --- a/onprc_ehr/test/src/org/labkey/test/tests/onprc_ehr/ONPRC_BillingTest.java +++ b/onprc_ehr/test/src/org/labkey/test/tests/onprc_ehr/ONPRC_BillingTest.java @@ -108,7 +108,6 @@ public void testNotifications() //run finance notifications Set notifications = new HashSet<>(); - notifications.add("DCM Finance Notification"); notifications.add("Finance Notification"); beginAt(WebTestHelper.getBaseURL() + "/ldk/" + getContainerPath() + "/notificationAdmin.view");