fix code check #292
fix code check #292
Conversation
WalkthroughThe changes extend DynamicModelService with new CRUD methods for dynamic table operations, add SQL injection prevention validation to table name generation, update error handling to use generic exceptions, improve locale-safe string normalization, and remove a post-create initialization call from model creation flow. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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://d.iamgay.cn/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java (8)
72-85:⚠️ Potential issue | 🟠 MajorSame silent failure pattern as
createDynamicTable.The exception is caught and logged but not propagated. Callers cannot distinguish between successful drop and failed drop.
Proposed fix
public void dropDynamicTable(Model modelMetadata) { if (modelMetadata == null || modelMetadata.getNameEn() == null || modelMetadata.getNameEn().isEmpty()) { throw new IllegalArgumentException("Model metadata or table name cannot be null or empty"); } String tableName = getTableName(modelMetadata.getNameEn()); String sql = generateDropTableSQL(tableName); - try { - jdbcTemplate.execute(sql); - log.info("Successfully dropped table: {}", tableName); - } catch (Exception e) { - log.error("Failed to drop table: {}", tableName, e); - } + jdbcTemplate.execute(sql); + log.info("Successfully dropped table: {}", tableName); }
130-142:⚠️ Potential issue | 🔴 CriticalHardcoded default value "1" overwrites all parameter defaults.
Line 133 sets
param.setDefaultValue("1")for every parameter, which:
- Overwrites any legitimate default values defined in the model
- Causes type conversion failures for non-integer fields (Boolean, Date, etc.)
- Makes the null check on line 136 always pass (value is always "1")
This appears to be debug code that was accidentally committed.
Proposed fix to remove hardcoded value
for (ParametersDto param : parameters) { String columnName = param.getProp(); String fieldType = param.getType(); - param.setDefaultValue("1"); String value = param.getDefaultValue(); if (value == null && Boolean.TRUE.equals(param.getRequired())) {
169-209:⚠️ Potential issue | 🔴 CriticalSQL injection vulnerabilities in
fieldsandorderByparameters.User-controlled values are directly concatenated into SQL:
- Line 179:
fieldslist joined without validation- Line 195:
orderBydirectly appendedWhile
tableNameis validated viagetTableName(), an attacker could inject SQL through fields like"id; DROP TABLE users; --"ororderBy = "id; DELETE FROM users; --".Proposed fix to validate fields and orderBy
public List<Map<String, Object>> dynamicQuery(String tableName, List<String> fields, Map<String, Object> conditions, String orderBy, Integer limit) { + // Validate field names + if (fields != null) { + for (String field : fields) { + if (!field.matches("^[a-zA-Z_][a-zA-Z0-9_]*$")) { + throw new IllegalArgumentException("Invalid field name: " + field); + } + } + } + + // Validate orderBy + if (orderBy != null && !orderBy.isEmpty()) { + // Allow only column name with optional ASC/DESC + if (!orderBy.matches("^[a-zA-Z_][a-zA-Z0-9_]*(\\s+(ASC|DESC))?$")) { + throw new IllegalArgumentException("Invalid orderBy clause: " + orderBy); + } + } + // 1. 构建SQL StringBuilder sql = new StringBuilder("SELECT ");
267-276:⚠️ Potential issue | 🔴 CriticalPagination is broken - offset is never applied.
The code sets
limit = pageSizebut never calculates or passes an offset. The comment on line 271 acknowledges this ("如果需要偏移量，可以在这里处理") but the fix was never implemented. Every page request will return the same first N records.Proposed fix to implement proper pagination
// 计算分页 - Integer limit = null; + Integer offset = null; + Integer limit = null; if (pageNum != null && pageSize != null) { limit = pageSize; - // 如果需要偏移量，可以在这里处理 + offset = (pageNum - 1) * pageSize; } // 执行查询 List<Map<String, Object>> data = dynamicQuery( - tableName, fields, conditions, orderBy, limit); + tableName, fields, conditions, orderBy, limit, offset);Note: You'll also need to update
dynamicQueryto accept and apply the offset parameter.
447-452:⚠️ Potential issue | 🔴 CriticalSQL injection via unescaped
defaultValueanddescriptionfields.Values are interpolated directly into DDL without escaping single quotes. A malicious
defaultValuelike"'; DROP TABLE users; --"would execute arbitrary SQL.Proposed fix to escape single quotes
if (field.getDefaultValue() != null) { - sb.append(" DEFAULT '").append(field.getDefaultValue()).append("'"); + sb.append(" DEFAULT '").append(escapeSingleQuotes(field.getDefaultValue())).append("'"); } if(field.getDescription()!=null && !field.getDescription().isEmpty()){ - sb.append(" COMMENT '").append(field.getDescription()).append("'"); + sb.append(" COMMENT '").append(escapeSingleQuotes(field.getDescription())).append("'"); }Add helper method:
private String escapeSingleQuotes(String value) { return value.replace("'", "''"); }
457-468:⚠️ Potential issue | 🟠 MajorSQL injection via unescaped enum option values.
Enum values from JSON are wrapped in single quotes without escaping. Malicious JSON like
[{"value": "'; DROP TABLE x; --"}]would execute arbitrary SQL.Proposed fix
private String getEnumOptions(String optionStr) { List<String> options= new ArrayList<>(); JSONArray jsonlist = JSON.parseArray(optionStr); for (int i = 0; i < jsonlist.size(); i++) { String value = jsonlist.getJSONObject(i).getString("value"); - options.add(value); + options.add(value.replace("'", "''")); } return options.stream() .map(opt -> "'" + opt + "'") .collect(Collectors.joining(", ")); }
587-593:⚠️ Potential issue | 🔴 CriticalSQL injection via unvalidated field names in UPDATE statement.
Field names from
dto.getData()are directly concatenated into SQL without validation. A malicious key like"id = 1; DROP TABLE users; --"would execute arbitrary SQL.Proposed fix to validate field names
+ // Validate field names + for (String field : updateFields.keySet()) { + if (!field.matches("^[a-zA-Z_][a-zA-Z0-9_]*$")) { + throw new IllegalArgumentException("Invalid field name: " + field); + } + } + StringBuilder sql = new StringBuilder("UPDATE " + tableName + " SET "); List<Object> params = new ArrayList<>();
500-519:⚠️ Potential issue | 🔴 CriticalSQL injection via unvalidated column names in INSERT statement.
Column names from
dataDto.getParams()keys are directly joined into SQL (line 511) without validation. The existingvalidateTableAndDatamethod could be reused here but is not called.Proposed fix to validate column names
public Map<String, Object> createData(DynamicInsert dataDto) { String tableName = getTableName(dataDto.getNameEn()); Map<String, Object> record = new HashMap<>(dataDto.getParams()); + + // Validate column names + for (String column : record.keySet()) { + if (!column.matches("^[a-zA-Z_][a-zA-Z0-9_]*$")) { + throw new IllegalArgumentException("Invalid column name: " + column); + } + } + String userId = loginUserContext.getLoginUserId();
🤖 Fix all issues with AI agents
In
`@base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java`:
- Around line 43-57: The createDynamicTable method in DynamicModelService
currently catches all exceptions and only logs them, which swallows errors and
prevents transactional rollback; change this by removing the broad try/catch or
rethrowing after logging so failures propagate: in createDynamicTable (which
calls getTableName, generateCreateTableSQL and jdbcTemplate.execute) either let
jdbcTemplate.execute throw its DataAccessException naturally or catch Exception
e, log it with log.error(..., e) and then rethrow it (or wrap and throw a
RuntimeException/DataAccessException) so callers and `@Transactional` can see the
failure and rollback.
In `@base/src/main/java/com/tinyengine/it/login/config/SSOInterceptor.java`:
- Around line 115-119: The catch block in SSOInterceptor currently throws a new
generic Exception, losing the original ServiceException type and cause; update
the catch to either rethrow the original exception (throw e;) or wrap it
preserving type and cause (throw new
ServiceException(e.getCodeOrAppropriateCode(), e.getMessage(), e);) so that the
original stack trace and structured error code are retained; keep the
DefaultLoginUserContext.clear() call before rethrowing to preserve cleanup.
🧹 Nitpick comments (2)
base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java (2)
350-357:addCommonFieldsmutates the input list - consider returning a new list instead.
addCommonFields(parameters)modifies theparameterslist from theModelobject, which could have unintended side effects if the caller expects the model to remain unchanged. Consider returning a new list or working with a copy.
314-320: Consider using varargs instead of deprecatednew Object[]{}pattern.The
new Object[]{tableName}syntax works but modern Spring JDBC allows varargs directly.Minor improvement
- List<Map<String, String>> existingColumns = jdbcTemplate.query(fetchColumnsSql, new Object[]{tableName}, (rs, rowNum) -> { + List<Map<String, String>> existingColumns = jdbcTemplate.query(fetchColumnsSql, (rs, rowNum) -> { Map<String, String> column = new HashMap<>(); column.put("COLUMN_NAME", rs.getString("COLUMN_NAME")); column.put("DATA_TYPE", rs.getString("DATA_TYPE")); return column; - }); + }, tableName);
base/src/main/java/com/tinyengine/it/dynamic/service/DynamicModelService.java
Show resolved
Hide resolved
2 participants
English | 简体中文
PR
PR Checklist
Please check if your PR fulfills the following requirements:
PR Type
What kind of change does this PR introduce?
Background and solution
What is the current behavior?
Issue Number: N/A
What is the new behavior?
Does this PR introduce a breaking change?
Other information
Summary by CodeRabbit
New Features
Improvements
Changes
✏️ Tip: You can customize this high-level summary in your review settings.