Repository navigation
fix(studio): escape LIKE wildcards in every search and name the escape character - #4458
123123213weqw wants to merge 1 commit into
Conversation
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Excellent security fix that addresses SQL LIKE wildcard injection across all Studio search endpoints. The implementation is thorough, well-tested, and the cross-database compatibility analysis (CHAR(92) vs backslash escaping) demonstrates careful engineering.
Key strengths:
- Comprehensive coverage: 18 files updated consistently with a single utility class
- Cross-database correctness: CHAR(92) works on both H2 and MySQL, including NO_BACKSLASH_ESCAPES mode
- Strong test coverage: 2434 tests passing, including specific cases for %, _, and \ characters
- Clear documentation: PR body explains the rationale and testing methodology
Minor observations:
- The hardcoded notLikeRight calls in RocketMQMetadataProvider are correctly left alone with a test pinning their wildcards
- QueryHistoryService consolidation removes duplication while maintaining the same behavior
This is a well-executed fix that prevents both accidental wildcard matching and potential SQL injection through search parameters.
Automated review by RockteMQ-AI
lizhimins
left a comment
There was a problem hiding this comment.
Thanks — this is the most complete treatment of the LIKE-escaping problem we have seen, and the dialect work is the part that matters most.
What we verified. The bug is real and reachable: MybatisPlusAuditRepository hands search straight to .like("operator", ...), .like("resource_name", ...) and .like("detail", ...), MyBatis-Plus wraps it as %search% with no ESCAPE, so search=% matches every row and search=_ matches every non-empty value — which on the audit page also pushes the CSV export past its 10 000-row cap. On this PR's base the sweep is complete: all nine classes that call .like() are converted, and the three hardcoded notLikeRight system-topic exclusions in RocketMQMetadataProvider are deliberately left alone and pinned by a dedicated test, which is the right judgement. ESCAPE CHAR(92) is the only spelling of the three candidates that is valid on both engines, and SqlLikeEscapingAgainstDatabaseTest proves it by executing real SQL against an H2 connection in both plain and MODE=MySQL configurations and asserting that H2 rejects the '\\' spelling. That is executable evidence rather than a claim, and it is the reason we are pursuing this PR rather than #4393. The 19 new tests all follow the ...Test naming convention and are mutation-sensitive: reverting any call site to .like(...) reddens the corresponding wiring assertion, and changing LIKE_ESCAPE_CLAUSE back to a quoted spelling reddens escapeClauseShouldSpellTheBackslashByCodePointTest plus all six database tests. Deleting QueryHistoryService's private escapeLike satisfies the shared-logic-in-common/util convention, and the helper is a final class with a private constructor using Spring StringUtils.hasText, with no new dependencies and no changes to application.yml or CI.
One thing to be aware of: #4666, #4667 and #4668 propose narrower fixes for the auth, alert and audit search paths. They are mutually exclusive with this PR — same call sites, no ESCAPE clause, and each keeps its own private escape copy — so of the four, this is the one we are asking to rework and pursue. That raises the bar for it: once it lands there should be no unescaped user-supplied LIKE left anywhere in the repository.
Blocking items.
-
Rebase onto
rocketmq-studioand sweep the tenth LIKE site. All Studio PRs now merge onto therocketmq-studiobranch, and this one is based onmaster. That branch carries #4606, which addedMybatisPlusAiConversationRepository— its conversation-title search calls.like(StringUtils.hasText(search), "title", escapeLike(search))and ships its own line-for-line privateescapeLike, with a comment saying a shared util is tracked separately upstream. That comment is pointing at this PR. It also has noESCAPEclause, so on the H2 dev profile it is only half effective. Please convert it toapply(likePredicate("title"), contains(search)), delete the private copy, and add a wiring assertion that the emitted SQL carriesESCAPE CHAR(92). -
The second LIKE in
MybatisPlusSettingsRepositoryis missed. Alongside thejsoncontains-match you did convert,findDataSourcesalso buildsLOWER(json) LIKE CONCAT('%"type":"', LOWER({0}), '"%')for thetypefilter. That parameter comes fromSettingsControlleras a bare@RequestParam(required = false) String typewith no enum validation, sotype=%widens the match exactly like the search term does. Please route it throughSqlLikeUtils.escapewith an explicitESCAPEclause as well. If you judge it out of scope, say so in the description and open an issue — but it should not be left silently, because item 1 above only counts as "the last one" if this is handled too.
Optional, not blocking.
likePredicate(String column)interpolates the column name into the SQL string. Every call site you converted passes a compile-time literal, so it is safe today, but this is a public utility: a javadoc line stating thatcolumnmust be a compile-time literal and never a request parameter would prevent someone discovering that the hard way.AuthService's username LIKE is converted but has no wiring test of its own; the description says it is covered by the ACL/audit set, and those test different classes. Either add a smallAuthServiceTestassertion or adjust the wording.- We checked the pagination interaction statically rather than by running it:
MyBatisConfiginstallsPaginationInnerInterceptorwithmybatis-plus-jsqlparser, and jsqlparser'sLikeExpression.setEscapeaccepts anExpression, soESCAPE CHAR(92)should survive the auto-generated count SQL — the repository already uses function expressions inside paginated queries (LOWER/CONCATinMybatisPlusSettingsRepository,JSON_CONTAINS/JSON_OBJECTinMybatisPlusAlertRepository). If you have a MySQL instance handy, confirming one paged search would close that out properly.
Once items 1 and 2 are addressed we can merge. Please retarget the base to rocketmq-studio when you rebase.
…e character A Studio search box passes its value to MyBatis-Plus, which wraps it as %value% and appends no ESCAPE clause, so the caller's own % and _ keep their pattern meaning: search=% matches every audit row, search=_ matches every non-empty value, and search=100%_done matches any row that merely contains 100 followed by four arbitrary characters. The list view, the insight cards and the CSV export all widen instead of narrowing, and a full-table match can push the capped export over its 10000-record limit and fail it with a 400. Add common/util/SqlLikeUtils, which escapes \, % and _ and builds the predicate with an explicit ESCAPE clause, and apply it at the boundary where a request parameter first becomes a query condition: auth users, ACL users and rules, alert rules, audit logs, instances, cloud credentials, data sources, the topic and group searches of the metadata provider, and the message/trace query history that already carried a private copy of the escaping rule. The clause spells the escape character as CHAR(92) rather than a quoted backslash, because no single quoted spelling works on both engines. Measured on H2 2.4.240 (plain and the MODE=MySQL of the dev profile) and on MySQL 8.0.46 through a JDBC prepared statement: ESCAPE '\' H2: accepted MySQL: syntax error near ''\'' at line 1 ESCAPE '\\' H2: Error in LIKE ESCAPE MySQL: accepted ESCAPE CHAR(92) H2: accepted MySQL: accepted CHAR(92) also stays correct when MySQL runs with NO_BACKSLASH_ESCAPES, where the backslash is not the assumed LIKE escape character either. The hard-coded notLikeRight system-topic exclusions are left alone, and a test pins their three wildcards so a later escape sweep cannot take them over. Two LIKE sites were still outside that sweep on the current rocketmq-studio head: the conversation-title search in MybatisPlusAiConversationRepository, which kept its own private escapeLike and named no escape character, and the type filter in MybatisPlusSettingsRepository.findDataSources, which builds its pattern through CONCAT and bound the raw parameter. Both now go through SqlLikeUtils, so the sweep is complete: no unescaped user-supplied LIKE is left in the repository. Also from the review: likePredicate documents that its column argument must be a compile-time literal (it is interpolated, not bound); the username search in AuthService gets the wiring assertion the other call sites already had; and the pagination interaction is now covered by a paged search against a real database instead of by inspection - AiConversationPersistenceIntegrationTest runs one through the MyBatis-Plus pagination interceptor, so the COUNT query jsqlparser derives from the same predicate is executed too. References apache#4289
e0cde51 to
e1e8ddb
Compare
|
Rebased and reworked — the branch is now Blocking items.
Optional items, all three. On that last one: I ran one paged search through — and the literal semantics hold ( The suite is green where MySQL 8 is reachable: 3145 tests on the untouched #4666, #4667 and #4668 are superseded by this one — I am leaving them open only until this lands, and will close them then unless you would rather they go now. |
|
All blocking items from the last review are on the current head: the tenth site ( |
|
This pull request has been automatically closed because there has been no activity for 7 days. If you would like to continue working on this, please reopen the PR with updated changes. |
|
Continued in #6483 — rebased onto the current |
Which Issue(s) This PR Fixes
Brief Description
Every Studio search box hands its value to MyBatis-Plus, which wraps it as
%value%and appends noESCAPEclause, so the caller's own%,_and\keep their pattern meaning:search=%matches every audit row (operator,resource_nameordetailLIKE '%%').search=_matches every row whose column is a non-empty string.search=100%_donematches any row that merely contains100followed by four arbitrary characters.The result set widens instead of narrowing, which is the opposite of what an operator typing those characters is asking for (SQL text,
a_bstyle ids, wildcard-ish names). On the audit page a full-table match also pushes the CSV export over its 10 000-record cap and fails it with a 400 instead of returning the filtered subset.common/util/SqlLikeUtilsescapes the three characters and builds the predicate with an explicitESCAPEclause, and it is applied once per value at the boundary where a request parameter first becomes a query condition: auth users, ACL users and rules, alert rules, audit logs, instances, cloud credentials, data sources, AI conversations, the metadata provider topic and group searches, and the message/trace query history that already carried a private copy of the escaping rule. That private copy is now gone, so the repository keeps one implementation.The escape character is spelled
CHAR(92), not a quoted backslash. No single quoted spelling works on both engines, and I measured rather than assumed. H2 2.4.240 (the versionmvn testresolves, in both the plain and theMODE=MySQLmode the dev profile uses) and MySQL 8.0.46 through a JDBC prepared statement:That matters because the suite has no MySQL, so a clause that only H2 accepts passes CI and breaks production.
CHAR(92)also stays correct when MySQL runs withNO_BACKSLASH_ESCAPES, where the backslash is not the assumedLIKEescape character either. The three hard-codednotLikeRightsystem-topic exclusions inRocketMQMetadataProviderare deliberately left alone; a test pins their wildcards so a later escape sweep cannot take them over.What the review asked for, and what changed
rocketmq-studioand rebased onto4c697f07.MybatisPlusAiConversationRepository's conversation-title search now callsapply(SqlLikeUtils.likePredicate("title"), SqlLikeUtils.contains(search)), its privateescapeLikeand the "shared util is tracked separately upstream" comment are deleted, andMybatisPlusAiConversationRepositoryTest#findPageShouldEscapeLikeWildcardsInTheTitleSearchTestasserts the emitted SQL carriesESCAPE CHAR(92). On this head,grep -rn "\.like(" server/src/main/javareturns nothing, which is the invariant you asked for once this lands.MybatisPlusSettingsRepositoryis fixed, not deferred:findDataSources'stypefilter bindsSqlLikeUtils.escape(normalizedType)and carries the same clause through the named constantDATA_SOURCE_TYPE_PREDICATE, with a wiring assertion of its own.typestays a free-form@RequestParam, buttype=%no longer widens the match.likePredicatedocuments thatcolumnis interpolated into the SQL text and must therefore be a compile-time literal, never a request parameter.AuthService's username search has its own wiring assertion inAuthServiceDatabaseTestinstead of being covered "by the ACL/audit set". The pagination interaction is no longer a static claim — see below.How Did You Test This Change?
That is the whole suite green, including the
@SpringBootTestclasses that need a reachable MySQL 8 (StudioApplicationTest,HealthProbeIntegrationTest,AuthService*IntegrationTest,RmqAlertStateMapperIntegrationTest): I ran it on a host that has MySQL 8.0.46 withutf8mb4_0900_ai_ci, which is also the engine those classes target by default. On a machine without MySQL they report 17 identical context-load errors on the untouched baseline as well, which is what the earlier revision of this description documented.Test coverage added:
SqlLikeUtilsTest— the escaping of each wildcard, the escape character itself and their combinations; blank andnullhandling, including thatcontainsanswersnullso a blank term cannot become a match-everything%%.SqlLikeEscapingAgainstDatabaseTest— runs the real predicate through H2 in both modes and asserts the literal match next to the over-matching pattern (%matches 1 row escaped vs 5 unescaped;100_donematches 1 vs 3), plus a case asserting that H2 rejects the'\\'spelling, which is the executable half of theCHAR(92)choice.AuthServiceDatabaseTest,MybatisPlusAiConversationRepositoryTest,RocketMQMetadataProviderTest,MybatisPlusSettingsRepositoryTest— search andtype,MybatisPlusAuditRepositoryTest,MybatisPlusInstanceRepositoryTest,MybatisPlusAclRepositoryTest,MybatisPlusCloudCredentialRepositoryTest) asserting both the escaped bound value and that the emitted SQL carriesESCAPE CHAR(92).RocketMQMetadataProviderTest.paginatedTopicSearchShouldKeepTheHardCodedNotLikePatternsTest— the three system-topic exclusions keep their own wildcards.Red/green, with the two new call sites reverted and the tests kept:
The pagination interaction is now executed, on both engines. You checked it statically; here is the same predicate going through
PaginationInnerInterceptor, so theCOUNTquery jsqlparser derives from it runs too.On H2,
AiConversationPersistenceIntegrationTest#theSearchShouldMatchALiteralPercentOrUnderscoreInTheTitleTestseedsprogress 100%_doneandprogress 100xydoneand searches both terms throughselectPage.On MySQL 8.0.46 (
utf8mb4_0900_ai_ci), a paged search of the same shape, with the mapper's SQL logging on:The count SQL keeps
ESCAPE CHAR(92)and the semantics are the intended ones:%matches the row that really contains a per cent sign rather than every row,_the rows that really contain an underscore, and a second page is served from the same predicate.Overlap with other open pull requests
ESCAPEclause, a private escape copy each. They are superseded by this PR; I am leaving them open only until this one lands, and will close them then unless you would rather they go now.ESCAPE '\'spelling is the one the table above shows failing on MySQL. It is their call to withdraw, not mine.Checklist
fix:)...Testweb/src/i18n/— not applicable, no UI text addedmvn testruns the ArchUnit checks)