aiai-bot commented on PR #10327:
URL: https://github.com/apache/seatunnel/pull/10327#issuecomment-3753631750
<!-- code-pr-reviewer -->
This PR adds support for database names with special characters in SQL
Server CDC, but **critical SQL injection vulnerabilities must be addressed**
before merging.
## Blocking Issues
### 1. SQL Injection in `getDatabaseWithConditionSql()` - CRITICAL
**File**: `SqlServerCatalog.java:73-75`
The method uses single quotes around the database name without escaping:
```java
return String.format(getListDatabaseSql() + " where name = '%s'",
databaseName);
```
When the database name contains a single quote (e.g., `test'db` or `test' OR
'1'='1`), it results in SQL injection, allowing attackers to bypass checks or
execute arbitrary SQL.
**Fix**: Escape single quotes: `databaseName.replace("'", "''")` or use
`QUOTENAME()`.
---
### 2. Bracket Escaping Incomplete - `]` Character Not Handled - CRITICAL
**File**: `SqlServerCatalog.java:93-95`,
`SqlServerCreateTableSqlBuilder.java:156,168`
Database names are wrapped with brackets `[databaseName]`, but internal `]`
characters are not escaped to `]]`.
A database name containing `]` (e.g., `test]db`) generates invalid SQL like
`[test]db]`, causing syntax errors. Worse, names like `test]; DROP TABLE
users--` enable SQL injection.
**Fix**: Escape `]` to `]]` using `databaseName.replace("]", "]]")` or
`QUOTENAME()`. As a short-term fix, validate and reject database names
containing `]` in `SqlServerSourceConfigFactory`.
---
### 3. Schema/Table Names Not Escaped in `getTableWithConditionSql()` - MAJOR
**File**: `SqlServerCatalog.java:78-84`
Schema and table names are wrapped with single quotes without escaping:
```java
TABLE_SCHEMA = '%s' and TABLE_NAME = '%s'
```
Schema or table names containing single quotes lead to SQL injection.
**Fix**: Escape single quotes for both schema and table names.
---
### 4. JDBC URL Parameters Not Encoded - MAJOR
**File**: `SqlServerCatalog.java:162-164`
`getUrlFromDatabaseName()` directly concatenates the database name into the
JDBC URL parameter without URL encoding.
Database names containing URL special characters (`;`, `&`, `=`) cause
connection failures.
**Fix**: Verify if SQL Server JDBC driver requires URL encoding for the
`databaseName` parameter. If so, use `URLEncoder.encode()`.
---
## Strong Recommendations
### 5. Missing Unit Tests for SQL Generation - MEDIUM
**File**: `SqlServerCatalog.java` (lines 92-153)
No unit tests for `getListTableSql()`, `getCreateDatabaseSql()`,
`getDropDatabaseSql()`, etc. to verify special character handling.
**Fix**: Add `SqlServerCatalogSqlBuilderTest.java` with tests for various
special characters (hyphen, space, dot, brackets, quotes, semicolon, Unicode).
### 6. Missing JDBC Sink Test - MEDIUM
**File**: `SqlServerCreateTableSqlBuilder.java` (lines 155-173)
No test verifies `CREATE TABLE` statements when database names contain
special characters.
**Fix**: Add `testCreateTableWithDatabaseNameWithHyphen()` verifying correct
bracket wrapping in generated SQL.
### 7. Missing Catalog API Integration Tests - MEDIUM
**File**: `AbstractJdbcCatalog.java` (lines 347-364)
No tests for direct Catalog API operations (`createDatabase()`,
`dropDatabase()`, `listTables()`, `tableExists()`) with special character
database names.
**Fix**: Add `testCatalogOperationsWithSpecialCharacters()` to
`SqlServerCDCIT.java`.
### 8. Missing Documentation - MEDIUM
No documentation explaining special character support and the `]` character
limitation.
**Fix**: Update `docs/en/connector-v2/source/SqlServer-CDC.md` with examples
of valid/invalid database names.
---
## Minor Issues
- Test only covers hyphens, not spaces or other special characters
- `PreviewActionTest` updates incomplete; `TRUNCATE_TABLE`/`DROP_TABLE`
methods not tested with special characters
- Inconsistent escaping strategy (some brackets, some quotes)
- No input validation beyond `isBlank()` check
- No negative SQL injection tests
- No backward compatibility check (though SQL Server bracket syntax has
long-standing support)
---
## Verification Checklist
- Construct database name `test']; DROP TABLE--` and confirm query throws
exception
- Verify `databaseExists()` returns correct result for `test-db-name`
- Verify JDBC connection succeeds with `test-db-name`
- Verify table creation and metadata queries work in special-character
databases
- Confirm backward compatibility with normal database names
- Verify `getColumn()` method safety
- Run full CDC Source → JDBC Sink E2E test
- Test all Catalog API operations
This PR addresses a real problem but requires security fixes before merging.
--
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]
