ColeMurray opened a new pull request, #36722:
URL: https://github.com/apache/superset/pull/36722
## Summary
This PR adds defense-in-depth input validation for the `cancel_query_id`
parameter in all database engine specs that use string interpolation in
SQL/command execution.
While `cancel_query_id` typically comes from trusted database sources (e.g.,
`CONNECTION_ID()`), adding validation ensures safety even if the data source is
compromised or if future code changes introduce user-controlled input.
## Changes
- Add `validate_cancel_query_id()` static method to `BaseEngineSpec` for
reusable validation
- Add input validation to `cancel_query()` in:
- **MySQLEngineSpec** - numeric validation
- **SingleStoreSpec** - space-separated numerics (CONNECTION_ID +
AGGREGATOR_ID)
- **PostgresEngineSpec** - numeric validation (PID)
- **RedshiftEngineSpec** - numeric validation (PID)
- **SnowflakeEngineSpec** - numeric validation (session ID)
- **TrinoEngineSpec** - alphanumeric with underscores (query ID format)
- **ImpalaEngineSpec** - hex format with colon (GUID format)
- **OcientEngineSpec** - alphanumeric with dashes
- Add comprehensive test suite with 22 test cases covering:
- Valid input acceptance
- SQL injection payload rejection
- URL injection payload rejection (for Impala HTTP-based cancellation)
- Edge cases (None, empty string, special characters)
## Security Impact
This addresses potential SQL injection vulnerabilities in `cancel_query`
implementations by ensuring `cancel_query_id` matches expected format before
use in:
- `KILL CONNECTION {id}` (MySQL, SingleStore)
- `SELECT SYSTEM$CANCEL_ALL_QUERIES({id})` (Snowflake)
- `SELECT pg_terminate_backend(pid) WHERE pid='{id}'` (PostgreSQL)
- `SELECT pg_cancel_backend(procpid) WHERE procpid='{id}'` (Redshift)
- `CALL system.runtime.kill_query(query_id => '{id}', ...)` (Trino)
- HTTP requests with `query_id={id}` (Impala)
- `CANCEL {id}` (Ocient)
## Test Plan
- [x] Added unit tests for `validate_cancel_query_id()` base method
- [x] Added unit tests for each affected engine spec
- [x] All 22 tests pass
- [ ] Manual testing with actual database connections
## Related Issues
This is a defense-in-depth security improvement. While there is no currently
known exploitable path to inject malicious `cancel_query_id` values (as they
come from database functions), this validation provides protection against:
1. Future code changes that might introduce user input
2. Compromised data sources
3. Security audit findings for using f-string interpolation in SQL
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
