fix: prevent SQL injection in model-data API#304
fix: prevent SQL injection in model-data API#304chilingling merged 5 commits intoopentiny:developfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 10 minutes and 48 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughThis PR introduces SQL injection prevention by adding a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java (1)
296-322:⚠️ Potential issue | 🟠 Major
orderTypeis validated but never applied to the generated SQL — DESC requests are silently ignored.
queryWithPageonly forwardsorderByintodynamicQuery;dto.getOrderType()is validated invalidateQueryFieldsbut never used when buildingORDER BY. Combined with the test change (fromorderBy="id DESC"toorderBy="id" + orderType="DESC"),DESCordering is lost at runtime even though the API contract now advertises a separateorderType.🐛 Proposed fix: propagate orderType and append it to the SQL
public Map<String, Object> queryWithPage(DynamicQuery dto) { String tableName = getTableName(dto.getNameEn()); List<String> fields = dto.getFields(); Map<String, Object> conditions = dto.getParams(); String orderBy = dto.getOrderBy(); + String orderType = dto.getOrderType(); Integer pageNum = dto.getCurrentPage(); Integer pageSize = dto.getPageSize(); validateQueryFields(dto); @@ - List<Map<String, Object>> data = dynamicQuery( - tableName, fields, conditions, orderBy, limit); + String effectiveOrderBy = (orderBy != null && !orderBy.isEmpty() && orderType != null && !orderType.isEmpty()) + ? orderBy + " " + orderType + : orderBy; + List<Map<String, Object>> data = dynamicQuery( + tableName, fields, conditions, effectiveOrderBy, limit);Alternatively, extend
dynamicQuery's signature to takeorderTypeexplicitly and append it after identifier validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java` around lines 296 - 322, The ordering direction from DynamicQuery (dto.getOrderType()) is validated in validateQueryFields but never applied in queryWithPage, causing DESC requests to be ignored; update queryWithPage to combine the validated orderBy and orderType (or extend dynamicQuery to accept an orderType param) and pass the composed ordering into dynamicQuery (e.g., build "orderBy + ' ' + orderType" after validation) so that dynamicQuery receives the direction and ORDER BY is correctly emitted; refer to queryWithPage, validateQueryFields, dynamicQuery and dto.getOrderType when making the change.
🧹 Nitpick comments (3)
base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java (2)
224-240: UseSqlIdentifierValidator.validatefor the table name too, instead of a parallel regex.Line 229 hand-rolls
"^[a-zA-Z_][a-zA-Z0-9_]*$"— the exact pattern already encapsulated bySqlIdentifierValidator.IDENTIFIER_PATTERN. Using the validator keeps all identifier rules in one place so future changes (e.g. allowing dotted names, reserving keywords) need only one edit.♻️ Proposed refactor
private void validateTableAndData(String tableName, Map<String, Object> data) { if (tableName == null || tableName.trim().isEmpty()) { throw new IllegalArgumentException("表名不能为空"); } - - if (!tableName.matches("^[a-zA-Z_][a-zA-Z0-9_]*$")) { - throw new IllegalArgumentException("表名格式不正确"); - } - + SqlIdentifierValidator.validate(tableName); if (data == null || data.isEmpty()) { throw new IllegalArgumentException("数据不能为空"); } - for (String field : data.keySet()) { SqlIdentifierValidator.validate(field); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java` around lines 224 - 240, The table-name regex check in validateTableAndData duplicates identifier rules; replace the manual pattern check with a call to SqlIdentifierValidator.validate for the tableName so all identifier rules are centralized. Keep the existing null/empty check (tableName == null || tableName.trim().isEmpty()) and then call SqlIdentifierValidator.validate(tableName.trim()) (or validate(tableName) after trimming) before proceeding; leave the existing data null/empty check and the loop that validates fields unchanged.
27-29: Duplicate ofDynamicModelService— extract the allowlist helper.
SYSTEM_FIELDS,getAllowedFields,extractProp, andvalidateQueryFieldshere are identical to the copies inDynamicModelService. A shared utility (e.g.ModelFieldAllowlisttakingModelServiceas a dependency) would eliminate the drift risk without changing behavior.Also applies to: 170-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java` around lines 27 - 29, The duplicate allowlist logic in DynamicService (SYSTEM_FIELDS, getAllowedFields, extractProp, validateQueryFields) is identical to DynamicModelService — extract this into a shared helper class (e.g., ModelFieldAllowlist) that accepts the existing ModelService dependency and exposes the allowlist operations; replace the duplicated methods in both DynamicService and DynamicModelService with calls to ModelFieldAllowlist.INSTANCE or an injected ModelFieldAllowlist instance, move SYSTEM_FIELDS into that helper, and update usages in DynamicService (methods named getAllowedFields, extractProp, validateQueryFields) to delegate to the new helper to eliminate duplication and prevent drift.base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java (1)
38-40:SYSTEM_FIELDS,getAllowedFields,extractProp, andvalidateQueryFieldsare verbatim duplicates ofDynamicService.Both services now carry identical allowlisting logic. Any future change (e.g. adding a system column, tweaking the allowlist rule) must be applied in two places or the two endpoints will drift. Consider extracting to a shared helper (e.g.
ModelFieldAllowlist) undercom.tinyengine.it.common.utilsand injecting it into both services.Also applies to: 323-376
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java` around lines 38 - 40, DynamicModelService duplicates allowlisting logic (SYSTEM_FIELDS, getAllowedFields, extractProp, validateQueryFields) already present in DynamicService; extract this logic into a single shared helper class (e.g., com.tinyengine.it.common.utils.ModelFieldAllowlist) that encapsulates the SYSTEM_FIELDS set and methods for getAllowedFields, extractProp, and validateQueryFields, then replace the local implementations in both DynamicModelService and DynamicService with dependency-injected usages of ModelFieldAllowlist (constructor or field injection) so both services delegate to the shared helper for future-proof, single-source maintenance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@base/src/main/java/com/tinyengine/it/dynamic/dto/DynamicQuery.java`:
- Around line 25-26: The DTO's orderType field (`@Pattern` on orderType) is
case-sensitive while SqlIdentifierValidator.validateOrderType accepts
case-insensitive values, causing valid lowercase values to be rejected; to fix,
make the DTO accept case-insensitive values (e.g., add the (?i) inline flag to
the `@Pattern` on the orderType field) so it aligns with
SqlIdentifierValidator.validateOrderType and the tests
(validateOrderTypeAsc/validateOrderTypeDesc) and does not reject "asc"/"desc" at
the `@Valid` DTO layer.
In
`@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java`:
- Around line 208-210: The code in DynamicModelService using
orderBy.replaceAll("\\s+(ASC|DESC)$", "") is case-sensitive and fails to strip
"asc"/"desc" in lowercase, causing SqlIdentifierValidator.validate to receive
the trailing order token; change the strip to be case-insensitive (e.g. use a
case-insensitive regex like "(?i)\\s+(ASC|DESC)$" or normalize case before
stripping) so that orderBy is cleaned correctly before calling
SqlIdentifierValidator.validate; ensure this change touches the orderBy handling
in the dynamicQuery path that currently uses orderBy.replaceAll and still allows
SqlIdentifierValidator.validateOrderType to accept "asc"/"desc".
In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java`:
- Around line 65-67: The call to validateTableAndData(dto.getParams()) makes
queryWithPage reject empty or null params; change the validation so
queryWithPage only validates condition keys when params are present instead of
enforcing "non-empty data". Create a new helper (e.g.,
validateConditionKeys(List<...> params)) that iterates params and validates each
condition key/format but returns silently for null/empty lists, and replace the
validateTableAndData(dto.getNameEn(), dto.getParams()) call in queryWithPage
with validateConditionKeys(dto.getParams()); keep the original
validateTableAndData or a separate non-empty-enforcing helper for operations
that must not accept empty payloads (like update if required). Add a regression
test for queryWithPage that asserts success with params == null and with params
== empty list.
---
Outside diff comments:
In
`@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java`:
- Around line 296-322: The ordering direction from DynamicQuery
(dto.getOrderType()) is validated in validateQueryFields but never applied in
queryWithPage, causing DESC requests to be ignored; update queryWithPage to
combine the validated orderBy and orderType (or extend dynamicQuery to accept an
orderType param) and pass the composed ordering into dynamicQuery (e.g., build
"orderBy + ' ' + orderType" after validation) so that dynamicQuery receives the
direction and ORDER BY is correctly emitted; refer to queryWithPage,
validateQueryFields, dynamicQuery and dto.getOrderType when making the change.
---
Nitpick comments:
In
`@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java`:
- Around line 38-40: DynamicModelService duplicates allowlisting logic
(SYSTEM_FIELDS, getAllowedFields, extractProp, validateQueryFields) already
present in DynamicService; extract this logic into a single shared helper class
(e.g., com.tinyengine.it.common.utils.ModelFieldAllowlist) that encapsulates the
SYSTEM_FIELDS set and methods for getAllowedFields, extractProp, and
validateQueryFields, then replace the local implementations in both
DynamicModelService and DynamicService with dependency-injected usages of
ModelFieldAllowlist (constructor or field injection) so both services delegate
to the shared helper for future-proof, single-source maintenance.
In `@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.java`:
- Around line 224-240: The table-name regex check in validateTableAndData
duplicates identifier rules; replace the manual pattern check with a call to
SqlIdentifierValidator.validate for the tableName so all identifier rules are
centralized. Keep the existing null/empty check (tableName == null ||
tableName.trim().isEmpty()) and then call
SqlIdentifierValidator.validate(tableName.trim()) (or validate(tableName) after
trimming) before proceeding; leave the existing data null/empty check and the
loop that validates fields unchanged.
- Around line 27-29: The duplicate allowlist logic in DynamicService
(SYSTEM_FIELDS, getAllowedFields, extractProp, validateQueryFields) is identical
to DynamicModelService — extract this into a shared helper class (e.g.,
ModelFieldAllowlist) that accepts the existing ModelService dependency and
exposes the allowlist operations; replace the duplicated methods in both
DynamicService and DynamicModelService with calls to
ModelFieldAllowlist.INSTANCE or an injected ModelFieldAllowlist instance, move
SYSTEM_FIELDS into that helper, and update usages in DynamicService (methods
named getAllowedFields, extractProp, validateQueryFields) to delegate to the new
helper to eliminate duplication and prevent drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7d48adf4-048b-4233-9ad4-80ee21fc5b4c
📒 Files selected for processing (7)
base/src/main/java/com/tinyengine/it/common/utils/SqlIdentifierValidator.javabase/src/main/java/com/tinyengine/it/dynamic/dto/DynamicQuery.javabase/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.javabase/src/main/java/com/tinyengine/it/dynamic/service/DynamicService.javabase/src/test/java/com/tinyengine/it/common/utils/SqlIdentifierValidatorTest.javabase/src/test/java/com/tinyengine/it/dynamic/service/DynamicModelServiceTest.javabase/src/test/java/com/tinyengine/it/dynamic/service/DynamicServiceSqlInjectionTest.java
2 participants
Summary
Security impact
模型数据接口
queryApi接受用户传入的fields（查询字段）、orderBy（排序字段）、orderType（排序方式）参数(前端功能暂未开放入口，但可以被直接调用接口攻击)，这些参数通过字符串拼接直接写入 SQL 语句，未做任何校验或过滤，攻击者可注入任意 SQL 表达式，足以造成敏感数据泄露与数据库结构暴露。Blocks all SQL injection vectors reported in the security audit:
fieldssubquery injection (@@version,information_schema,SELECT password FROM t_user)orderBy/orderTypeexpression injection (SLEEP(),IF(),; DROP TABLE)updatecondition column injection解决方法：
添加必要的校验，参数字段需在模型表字段中 & 正则校验输入非法字符
SqlIdentifierValidatorutility for centralized SQL identifier format validation (regex^[a-zA-Z_][a-zA-Z0-9_]*$) and order type enum check (ASC/DESC)DynamicServiceandDynamicModelService—fieldsandorderByare restricted to columns defined in the model's parameters@Patternvalidation annotations onDynamicQueryDTO (nameEn,fields,orderBy,orderType)DynamicService.update()— previously only validateddatakeys, now also validatesparams(WHERE condition) keysDynamicModelService.dynamicQuery(),createData(),updateDateById()DynamicModelServiceandModelServiceImplvia@LazyTest plan
Summary by CodeRabbit
Release Notes
New Features
ASCorDESCvalues only.Tests