Prevent SQL injection in message metadata column search

[pacmano1]

Problem

Custom metadata column names in message search are interpolated into SQL as identifiers (MyBatis ${}) across all database dialects — ${element.columnName} (metaDataSearch) and ${column} (textSearchMetaDataColumns) — and arrive from the REST parameters with no validation, so an authenticated MESSAGES_VIEW user can inject arbitrary SQL. The search value (#{...}) and operator (enum-constrained) were already safe; only the column name is affected. Ref GHSA-mf46-33w3-84j6.

Fix

Validate every referenced metadata column name against the channel's own defined columns before the filter reaches the data layer:

• MetaDataColumnValidator (new, server/util): pure check, lazy channel-column lookup, returns the first unknown column name (or null).
• MessageServlet: primary gate at the REST boundary; an unknown column returns 400. Applied to all message-search entry points, in both the parameter and MessageFilter-body forms (getMessages, getMessageCount, reprocessMessages, removeMessages, exportMessagesServer).
• DonkeyMessageController: defense-in-depth backstop for callers that reach the controller directly (e.g. plugins via MessageController.getInstance()), throwing on an unknown column.

Column names are matched exactly against the channel's (upper-cased) defined columns. No SQL/mapper changes; the value and operator are untouched.

Tests

• MetaDataColumnValidatorTest — unit tests the check: unknown/known/non-uppercase/null column, and that the channel lookup is skipped when the filter references no custom column.
• MessageServletTest — REST-boundary reject/allow paths.
• Full clean :server build green (unit tests included).

Verifying

Verified against a running server (Derby) with a channel that has a custom metadata column:

• metaDataSearch=<definedColumn> = foo → 200 (search runs normally).
• metaDataSearch=<undefinedColumn> = foo → 400 (rejected before reaching SQL).

[github-actions]

Test Results

668 tests  +14   668 ✅ +14   3m 37s ⏱️ +54s
109 suites + 1     0 💤 ± 0 
109 files   + 1     0 ❌ ± 0 

Results for commit fe47f02. ± Comparison against base commit a08c114.

[Copilot]

Pull request overview

This PR closes a SQL injection vector in message search by validating custom metadata column identifiers (previously interpolated into SQL via MyBatis ${}) against the channel’s defined metadata columns before the filter reaches the data layer.

Changes:

• Added MetaDataColumnValidator utility to detect unknown metadata column references in MessageFilter.
• Enforced validation at the REST boundary (MessageServlet) and added a defense-in-depth backstop in DonkeyMessageController.
• Added unit tests covering validator behavior and REST rejection/allow paths.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
server/src/main/java/com/mirth/connect/server/util/MetaDataColumnValidator.java	New utility to detect unknown custom metadata columns referenced by message search filters.
server/src/main/java/com/mirth/connect/server/api/servlets/MessageServlet.java	Validates filter-referenced metadata columns at API entry points; rejects unknown columns with 400.
server/src/main/java/com/mirth/connect/server/controllers/DonkeyMessageController.java	Adds controller-level validation as a backstop for non-REST callers.
server/src/test/java/com/mirth/connect/server/util/MetaDataColumnValidatorTest.java	Unit tests for validator behavior and lazy lookup behavior.
server/src/test/java/com/mirth/connect/server/api/servlets/MessageServletTest.java	Tests that REST message count rejects/accepts based on metadata column validity.

Comments suppressed due to low confidence (1)

server/src/main/java/com/mirth/connect/server/util/MetaDataColumnValidator.java:69

• MetaDataColumnValidator can throw a NullPointerException if filter.getMetaDataSearch() contains a null element (e.g., a client sends a JSON array with a null entry). That would turn a bad request into a 500 and could be used for DoS; treat a null element the same as an unknown column and return "null" instead.

        if (hasMetaDataSearch) {
            for (MetaDataSearchElement element : filter.getMetaDataSearch()) {
                if (element.getColumnName() == null || !allowedColumns.contains(element.getColumnName())) {
                    return String.valueOf(element.getColumnName());
                }
            }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[mgaffigan]

Replacing newlines is playing whack-a-mole, but this should be fixed to structured logging ("{}" in the log message format).

[mgaffigan]

It seems fine. See below for notes if you feel like making edits.

[mgaffigan]

Do we need to do this twice? Seems extra and easy to have bugs at one of the two layers. I suspect the goal was a 400 instead of a 500 - a typed exception might be a better route for that.

[mgaffigan]

We're having to "remember" to call this validator several times. Is there an alternative that's harder to accidentally omit? Seems like all entrypoints eventually lead to searchAll

[mgaffigan]

Replacing newlines is playing whack-a-mole, but this should be fixed to structured logging ("{}" in the log message format).