Fix stored XSS in finance notification charge summary report

[labkey-martyp]

Rationale

FinanceNotification.writeResultTable builds the ONPRC charge summary report by concatenating editor-entered database values (financial analyst, project, alias/account, OGA project number, category) and their derived URLs directly into HTML with no 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 — a stored XSS with privilege-escalation potential toward admins. This is the ONPRC counterpart to the BillingNotification fix. DCMFinanceNotification was a registered subclass that overrode writeResultTable with a near-duplicate copy of the same unescaped report; rather than carry and fix a second copy, it is removed entirely.

Related Pull Requests

• Fix stored XSS in finance notification charge summary report #1778 (release25.7-SNAPSHOT counterpart)
• Fix stored XSS in billing notification charge summary report ehrModules#1159

Changes

• FinanceNotification: wrap every editor-entered value (financial analyst, project, account, project number, category) and its derived href URL in PageFlowUtil.filter in the per-financial-analyst tables, and filter the top category summary table (url and category), which was unescaped in this copy; add the org.labkey.api.util.PageFlowUtil import.
• DCMFinanceNotification: remove the class entirely and drop its NotificationService registration and import from ONPRC_BillingModule.

[brentlogan]

@labkey-martyp here's the comment from Copilot:

At a quick review, the change looks directionally correct for fixing stored XSS because it is HTML-encoding user-controlled values before inserting them into generated HTML. Specifically, category, financialAnalyst, tokens[1], tokens[2], tokens[3], and the generated URLs are now passed through PageFlowUtil.filter(...) before being rendered.

A few things I'd look at before approving:

URL encoding vs. HTML encoding

• The PR uses PageFlowUtil.filter(url) on values that are being placed inside an href attribute.
• HTML-encoding protects against breaking out of the attribute, but it does not URL-encode query parameter values.
• I noticed URLs are built using values such as tokens[1] directly in query strings:

	  "&query.project/displayName~eq=" + tokens[1]

• If project names can contain spaces, &, =, %, etc., those URLs may still be malformed. Consider proper URL encoding when constructing query parameters.

Partial XSS coverage

• The modified fields are filtered, but the PR only shows changes in a few rendering locations.
• Since this was reported as a stored XSS issue, I'd verify there aren't other HTML-generating methods in FinanceNotification that still append user-controlled values without encoding.

Numeric values are not filtered

• Values such as:

    totalsMap.get("total")
  

  remain unfiltered
• This is probably fine if they are guaranteed numeric, but I'd double-check the types. If any can become strings from query results, they could still be a rendering vector.

Deletion of DCMFinanceNotification.java

• The PR deletes DCMFinanceNotification.java.
• That may be unrelated to the XSS fix or perfectly intentional, but it's the one change that stands out. I'd confirm:
  • nothing references the deleted class anymore,
  • all functionality was migrated elsewhere,
  • coverage/tests still exercise the same notification paths.

Overall

I don't see an obvious regression in the displayed diff. The biggest thing I'd comment on is whether query parameter values should be URL-encoded rather than relying solely on PageFlowUtil.filter() after the URL has already been assembled. That is the only potentially substantive issue that jumps out from the changes shown.

[labkey-martyp]

@brentlogan I added the additional URL encoding to the method that is the focus of this PR.