Repository navigation
Conversation
…LIKE wildcards MybatisPlusAlertRepository bound the raw search term into .like(...) in both paged rule feeds: ruleQuery (line 84) and findRulesPage (lines 94 and 96). The term was therefore interpreted as a LIKE pattern rather than as literal text: '%' matched any run of characters, '_' matched any single character and '\' escaped the character after it. Searching for a rule whose name or metric contains a percent sign or an underscore listed unrelated rules, and a term containing a backslash could match nothing at all. Escape '\', '%' and '_' before the pattern is built, following the existing QueryHistoryService.escapeLike house pattern, and keep the helper local to the file. Both paged searches are covered: findRulePage (name) and findRulesPage (name OR metric). Test: MybatisPlusAlertRepositoryTest#findRulePageShouldEscapeLikeWildcardsInTheSearchTermTest and #findRulesPageShouldEscapeLikeWildcardsInTheSearchTermTest capture the QueryWrapper and assert the bound parameter value; both fail with the unescaped pattern before the fix and pass after it.
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Summary
Fixes SQL LIKE wildcard injection in alert rule search — %, _ and \ in search terms are now escaped before binding to the LIKE predicate, so users get literal matching instead of pattern matching.
Clean, focused fix with good test coverage (ArgumentCaptor approach verifies the exact bound values). The escape order is correct (backslash first, then wildcards). LGTM.
Automated review by github-manager-bot
lizhimins
left a comment
There was a problem hiding this comment.
The escaping itself is right, but this PR cannot land as it stands — it collides with #4458 and is a strict subset of it.
Concretely:
- Same lines as #4458. Both PRs rewrite
MybatisPlusAlertRepository.java:84/94/96(the.like(...)calls onname/metric/ the third search column).git merge-treebetween the two branches reports a conflict on exactly those lines, so only one of them can go in unchanged. #4458 is still open and was submitted earlier. - Coverage. #4458 escapes nine LIKE call sites across the search paths; this one covers the three in the alert repository. Everything here is already in there.
- Missing
ESCAPEclause. The new privateescapeLike(line 89) escapes\,%and_in the bound parameter, but the predicate is still emitted as a plainLIKE ?with noESCAPE '\'clause. MySQL's default escape character happens to be\, so it works today by accident rather than by contract — and it breaks on any deployment that sets a differentsql_mode/NO_BACKSLASH_ESCAPES. #4458 has the same gap, so this is worth fixing once, in one place.
What would make this actionable: rebase on top of #4458 once it lands and re-submit only what it still misses (if anything), with an explicit ESCAPE clause on the predicates. Until then there is nothing to merge that #4458 does not already carry, so I am marking this as requiring changes rather than merging or closing it outright.
|
Thanks for the detailed comparison. I confirmed that #4458 is still open and that this branch overlaps its alert-repository hunk, so I will not push another competing implementation onto this PR. I will wait for #4458 to be resolved, then rebase on the resulting |
|
Closing this overlapping PR after checking #4458's current head. It covers the same alert-rule LIKE predicates and names the escape character explicitly with ESCAPE CHAR(92); there is no independent change left in this branch. Thanks for the review. |
Which Issue(s) This PR Fixes
Brief Description
MybatisPlusAlertRepositorybound the raw search term into.like(...)in both paged rule feeds —ruleQuery(server/src/main/java/org/apache/rocketmq/studio/ops/alert/MybatisPlusAlertRepository.java:84) andfindRulesPage(:94and:96) — so the term was interpreted as a LIKE pattern instead of as literal text:%matched any run of characters,_matched any single character, and\escaped whatever followed it. Searching for a rule whose name or metric contains a percent sign or an underscore therefore listed unrelated rules, and a term containing a backslash could match nothing at all.The pattern is now built from an escaped term (
\,%and_escaped), the same way the existingQueryHistoryService.escapeLikehouse pattern does it, with the helper kept local to the file that needs it. Both paged searches are covered:findRulePage(name only) andfindRulesPage(name OR metric). A term without wildcard characters produces exactly the same pattern as before, so the existinghasBusinessRulePageFiltersexpectation (%lag%) is unchanged.How Did You Test This Change?
Test written first and run against the unmodified tree (
red):After the fix (
green):Neighbouring classes, to show the change does not disturb the paged feeds or the controllers:
Test coverage:
findRulePageShouldEscapeLikeWildcardsInTheSearchTermTest— searches fora%b_c\dand asserts the wrapper binds%a\%b\_c\\d%, i.e. the wildcard characters are escaped and the backslash itself is escaped first.findRulesPageShouldEscapeLikeWildcardsInTheSearchTermTest— the name-or-metric feed: searching" 50%_off "binds the trimmed, escaped%50\%\_off%to both columns.QueryWrapperwith anArgumentCaptor(callinggetSqlSegment()beforegetParamNameValuePairs()) and assert the bound parameter, so they exercise the SQL that would be sent and need no database.Checklist
fix:)...Testweb/src/i18n/— not applicable, no UI text addedmvn testruns the ArchUnit checks)Overlap with another open pull request
#4458(escape LIKE wildcards in every search and name the escape character, by123123213weqw) changes thissame file for the same root cause, as part of one patch that covers every
LIKEpredicate in the repository.Measured with
git merge-tree --write-tree <this head> <its head>: the two branches conflict inMybatisPlusAlertRepository.java. Both sides merge into the current trunk individually (merge-treeexits 0 for each againstrocketmq-studio), so this is a genuine collision between two patches to the same lines rather than one of usbeing stale. Whichever lands second resolves it mechanically, and if #4458 is merged first this change is
redundant for
server/src/main/java/org/apache/rocketmq/studio/ops/alert/MybatisPlusAlertRepository.javaand can be closed.One correction, because an earlier version of this paragraph had it wrong: #4458's base is
rocketmq-studio(the same trunk this change targets), notmaster. The remaining difference is style - thispatch escapes the term inside the existing query builder, in the style the repository already uses
(
QueryHistoryService.escapeLike), instead of introducing a shared utility.