Prevent SQL injection in message metadata column search#361
Conversation
…tion
The message search mappers interpolate client-supplied custom metadata
column names into SQL as identifiers (MyBatis ${} substitution) in every
dialect, via the metaDataSearch and textSearchMetaDataColumns filters,
allowing SQL injection by an authenticated user. Validate each referenced
column name against the channel's own defined columns before the filter
reaches the data layer, rejecting unknown names. Enforced at the REST
boundary (MessageServlet, 400) and backstopped in DonkeyMessageController
for callers that reach it directly. The search value and operator were
already bound/enum-constrained.
Signed-off-by: Finnegan's Owner <44065187+pacmano1@users.noreply.github.com>
43f5aaf to
fe47f02
Compare
There was a problem hiding this comment.
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
MetaDataColumnValidatorutility to detect unknown metadata column references inMessageFilter. - Enforced validation at the REST boundary (
MessageServlet) and added a defense-in-depth backstop inDonkeyMessageController. - 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.
| String unknownColumn = MetaDataColumnValidator.findUnknownColumn(filter, () -> channelController.getMetaDataColumns(channelId)); | ||
| if (unknownColumn != null) { | ||
| logger.warn("Rejected message search for channel " + channelId + " referencing unknown metadata column: " + unknownColumn); | ||
| throw new MirthApiException(Response.Status.BAD_REQUEST); | ||
| } |
There was a problem hiding this comment.
Replacing newlines is playing whack-a-mole, but this should be fixed to structured logging ("{}" in the log message format).
mgaffigan
left a comment
mgaffigan
left a comment
There was a problem hiding this comment.
It seems fine. See below for notes if you feel like making edits.
| // Primary check at the REST boundary: reject any custom metadata column the filter references | ||
| // that is not defined on the channel. Column names are matched exactly against the channel's | ||
| // (upper-cased) columns, which come from the in-memory channel cache (no database hit) and are | ||
| // only looked up when the filter actually references a custom column. Callers that reach | ||
| // DonkeyMessageController directly (e.g. a plugin) are covered by a last-resort backstop there. | ||
| String unknownColumn = MetaDataColumnValidator.findUnknownColumn(filter, () -> channelController.getMetaDataColumns(channelId)); | ||
| if (unknownColumn != null) { | ||
| logger.warn("Rejected message search for channel " + channelId + " referencing unknown metadata column: " + unknownColumn); | ||
| throw new MirthApiException(Response.Status.BAD_REQUEST); | ||
| } |
There was a problem hiding this comment.
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.
|
|
||
| @Override | ||
| public void removeMessages(String channelId, MessageFilter filter) { | ||
| validateMetaDataColumns(channelId, filter); |
There was a problem hiding this comment.
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
| String unknownColumn = MetaDataColumnValidator.findUnknownColumn(filter, () -> channelController.getMetaDataColumns(channelId)); | ||
| if (unknownColumn != null) { | ||
| logger.warn("Rejected message search for channel " + channelId + " referencing unknown metadata column: " + unknownColumn); | ||
| throw new MirthApiException(Response.Status.BAD_REQUEST); | ||
| } |
There was a problem hiding this comment.
Replacing newlines is playing whack-a-mole, but this should be fixed to structured logging ("{}" in the log message format).
|
Fair points. Single gate: simplifies things. One caveat on reprocess: it's async, so a bad request still gets a response, but the call itself fails and logs an error. Logging: will update, but it's not a perfect fix. I'll file a separate issue with the details. Null elements: the null-entry NPE Copilot caught turns a bad request into a 500. Fixing it. |
Addresses review feedback on the two-layer approach. Validation now runs once from the FilterOptions constructor, which every message search path (get, count, remove, reprocess; export runs through get) builds exactly once before its batch loop. That is the single choke point a search must pass through, so a new search entry point cannot bypass the check, and it covers direct controller callers such as plugins, not just the REST layer. It does not belong in searchAll: searchAll sits inside the batch loop, which is skipped when no message id range matches (an empty channel, or a min/max filter that excludes everything), so an undefined column could pass unchecked there. FilterOptions is built regardless, so the check runs even then. MetaDataColumnException is a plain domain exception, left to bubble up. On the synchronous API paths the engine turns it into a 500 like any other uncaught controller exception; it is deliberately not mapped to a 400, because the engine has no central exception-to-status convention (every specific status is thrown explicitly at a servlet) and coupling this data-layer type to an HTTP status would be the only such case in the tree. The injection is blocked regardless of status, since the throw happens before any query runs. Reprocess is asynchronous: the throw aborts the run on the background thread and is logged rather than surfaced. The exception message is generic so the untrusted column name is not reflected back in the 500 body; the channel and column are kept as fields for parameterized logging on the reprocess path. Also guards null metadata-search elements and null defined-column entries, which would otherwise NPE in the validator. Signed-off-by: Finnegan's Owner <44065187+pacmano1@users.noreply.github.com>
…adata-column-search
|
Reworked since your review. What changed and why. Where the check lives: moved out of the per-endpoint calls into the Status on rejection: the check throws a plain Two notes on the 500: the exception message is generic, so the untrusted column name is not reflected back. The body does include a stack trace, but that's the engine's standard 500 behavior for every endpoint, not something introduced here. Live-tested on a running engine (populated channel):
Validator logic is unit-tested ( On verification vs. the current base: the boot test above was run before syncing this branch with |
mgaffigan
left a comment
mgaffigan
left a comment
There was a problem hiding this comment.
This looks great! Tests are a bit "extra", but certainly not a problem.
| * references a custom column, so a search that uses none costs no channel lookup. | ||
| * </p> | ||
| */ | ||
| public static String findUnknownColumn(MessageFilter filter, Supplier<List<MetaDataColumn>> definedColumnsSupplier) { |
There was a problem hiding this comment.
As a new public method, I'd prefer to see this return an Optional instead of requiring explicit null handling by the caller. The javadoc should include @param and @return tags.
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 authenticatedMESSAGES_VIEWuser 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 returns400. Applied to all message-search entry points, in both the parameter andMessageFilter-body forms (getMessages,getMessageCount,reprocessMessages,removeMessages,exportMessagesServer).DonkeyMessageController: defense-in-depth backstop for callers that reach the controller directly (e.g. plugins viaMessageController.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.:serverbuild 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).