zhangshenghang commented on PR #10327:
URL: https://github.com/apache/seatunnel/pull/10327#issuecomment-3754095938
<!-- code-pr-reviewer -->
This PR moves in the right direction by adding square-bracket quoting for
SQL Server database names, but there are **2 CRITICAL security issues** and **2
MAJOR correctness issues** that should be addressed before merge.
## CRITICAL - Must Fix
### 1. SQL Injection in `getDatabaseWithConditionSql()` -
SqlServerCatalog.java:73-75
The method uses single-quoted string concatenation instead of square
brackets:
```java
return String.format(getListDatabaseSql() + " where name = '%s'",
databaseName);
```
**Problem**: When `databaseName` contains special characters (e.g.,
`test-db-name` or `test'db`), this causes SQL syntax errors or SQL injection.
This directly contradicts the PR's goal of supporting special characters.
**Impact**: Affects `databaseExists()` checks in `AbstractJdbcCatalog:352`.
All operations depending on this check (dropDatabaseInternal, createTable,
dropTable) may fail. Attackers could bypass existence checks using malicious
names like `test' OR '1'='1`.
**Fix**: Use `where name = [%s]` to match other methods in the same class
(getListTableSql:93-95, getCreateDatabaseSql:147, getDropDatabaseSql:152).
---
### 2. SQL Injection in `getTableWithConditionSql()` -
SqlServerCatalog.java:78-84
The method directly concatenates schemaName and tableName without escaping:
```java
TABLE_SCHEMA = '%s' and TABLE_NAME = '%s'
```
**Problem**: Uses single quotes instead of square brackets. Inconsistent
with the databaseName quoting strategy fixed in this PR.
**Impact**: Affects `tableExists()` checks in `AbstractJdbcCatalog:422`. All
operations depending on this check (getTable, createTable, dropTable, open) are
vulnerable to SQL injection. Schema names with special characters (e.g.,
`my-schema`) cause runtime errors.
**Fix**: Change to `TABLE_SCHEMA = '[%s]'and TABLE_NAME = '[%s]'` for
consistency.
---
## MAJOR - Strongly Recommended
### 3. Missing Square Brackets in `getDropTableSql()` -
SqlServerCatalog.java:141-143
Uses `tablePath.getFullName()` without quoting, while other methods
(`getExistDataSql:174`, `getTruncateTableSql:179`) use
`getFullNameWithQuoted("[", "]")`.
**Impact**: Tables/schemas with special characters cannot be dropped.
Inconsistent quoting strategy within the same class creates confusion and
maintenance burden.
**Fix**: Change to `tablePath.getFullNameWithQuoted("[", "]")` and update
`PreviewActionTest` assertions.
---
### 4. Asymmetric Fix - Schema/Table Special Characters Not Supported
The PR only adds square brackets for databaseName, but schemaName and
tableName remain unquoted in `getTableWithConditionSql`. The E2E test uses a
fixed `dbo` schema without special characters.
**Impact**: Production environments using custom schema names (e.g.,
`app-v2`) will encounter runtime errors. Incomplete implementation relative to
PR title.
**Fix**: Apply consistent quoting strategy to all identifiers, or document
current limitations.
---
## Recommendation
Please address the 2 CRITICAL security issues before merging to complete the
SQL injection prevention coverage. The MAJOR issues can be tracked as
follow-ups but would ideally be fixed together for consistency.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
