Fix SQL vulnerability in /webapp/findQueryHistory endpoint#999
Conversation
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
I've submited CLA a few hours ago. |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis pull request refactors the query history filtering mechanism from dynamically constructed SQL condition strings to individual parameterized filter parameters. The Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
gateway-ha/src/test/java/io/trino/gateway/ha/router/BaseTestQueryHistoryManager.java (2)
39-40: Prefer cleanup over ordered stateful tests.
@TestMethodOrdermakes this deterministic, but with@TestInstance(Lifecycle.PER_CLASS)every new test now inherits rows written by previous ones. Resettingquery_historybetween tests would keep assertions independent and avoid hidden ordering constraints.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-ha/src/test/java/io/trino/gateway/ha/router/BaseTestQueryHistoryManager.java` around lines 39 - 40, The tests use `@TestInstance`(Lifecycle.PER_CLASS) and `@TestMethodOrder` on BaseTestQueryHistoryManager which causes state (rows in query_history) to leak between tests; add a cleanup that resets query_history after each test instead of relying on ordering. Implement an `@AfterEach` method (e.g., cleanupQueryHistory or resetQueryHistory) inside BaseTestQueryHistoryManager that truncates or deletes all rows from the query_history table via the same database client/QueryHistoryManager used by the tests, and ensure it runs for every test to keep assertions independent (alternatively remove PER_CLASS or TestMethodOrder if per-test instance behavior is acceptable).
149-173: Cover the remaining changed query paths in this regression.This test only inspects
getRows(), but the production change also replacedQueryHistoryDao.count(...)ingateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/QueryHistoryDao.javaLines 136-147 and parameterizeduserNametoo. Please add one injectedusercase and assert the reported total/count so a regression there cannot slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-ha/src/test/java/io/trino/gateway/ha/router/BaseTestQueryHistoryManager.java` around lines 149 - 173, Add a test that injects SQL into the user field and asserts the returned total/count from the paged response so the DAO.count regression is covered: create a QueryHistoryRequest with a malicious user value (e.g. "' OR '1'='1") and call queryHistoryManager.findQueryHistory(...) and assert the TableData returned has total/count == 0 (use TableData.getTotal() or its equivalent), then also assert the normalRequest still reports total/count == 1; this ensures both QueryHistoryRequest handling in queryHistoryManager.findQueryHistory and the QueryHistoryDao.count(...) parameterization are exercised.gateway-ha/src/main/java/io/trino/gateway/ha/router/HaQueryHistoryManager.java (1)
127-132: Normalize the page size once and reuse for both offset and limit calculations.
QueryHistoryRequesthas no validation annotations and does not enforce thatsizeis positive. Line 127 clamps bad sizes insidegetStart(), but line 132 still passes the rawquery.size()to the DAO as the limit. This creates an inconsistency: when size is non-positive, the offset becomes 0 (normalized), but the limit remains unchanged (not normalized), leading to backend-dependent behavior. Extract a normalized size value and pass it to bothgetStart()andpageQueryHistory().Suggested change
- int start = getStart(query.page(), query.size()); + int size = Math.max(query.size(), 0); + int start = getStart(query.page(), size); String userName = Strings.emptyToNull(query.user()); String externalUrl = Strings.emptyToNull(query.externalUrl()); String queryId = Strings.emptyToNull(query.queryId()); String source = Strings.emptyToNull(query.source()); - List<QueryHistory> histories = dao.pageQueryHistory(userName, externalUrl, queryId, source, query.size(), start, isOracleBackend); + List<QueryHistory> histories = dao.pageQueryHistory(userName, externalUrl, queryId, source, size, start, isOracleBackend);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@gateway-ha/src/main/java/io/trino/gateway/ha/router/HaQueryHistoryManager.java` around lines 127 - 132, Normalize the requested page size once and reuse it for both offset and limit: compute an int normalizedSize = getValidSize(query.size()) (or call getStart/getSize helper) and pass normalizedSize into getStart(normalizedSize) and dao.pageQueryHistory(..., normalizedSize, start, ...) instead of using query.size() directly; update references around getStart, QueryHistoryRequest, and dao.pageQueryHistory so the same validated size value is used for both offset and limit to avoid backend-dependent behavior when size is non-positive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@gateway-ha/src/main/java/io/trino/gateway/ha/router/HaQueryHistoryManager.java`:
- Around line 127-132: Normalize the requested page size once and reuse it for
both offset and limit: compute an int normalizedSize =
getValidSize(query.size()) (or call getStart/getSize helper) and pass
normalizedSize into getStart(normalizedSize) and dao.pageQueryHistory(...,
normalizedSize, start, ...) instead of using query.size() directly; update
references around getStart, QueryHistoryRequest, and dao.pageQueryHistory so the
same validated size value is used for both offset and limit to avoid
backend-dependent behavior when size is non-positive.
In
`@gateway-ha/src/test/java/io/trino/gateway/ha/router/BaseTestQueryHistoryManager.java`:
- Around line 39-40: The tests use `@TestInstance`(Lifecycle.PER_CLASS) and
`@TestMethodOrder` on BaseTestQueryHistoryManager which causes state (rows in
query_history) to leak between tests; add a cleanup that resets query_history
after each test instead of relying on ordering. Implement an `@AfterEach` method
(e.g., cleanupQueryHistory or resetQueryHistory) inside
BaseTestQueryHistoryManager that truncates or deletes all rows from the
query_history table via the same database client/QueryHistoryManager used by the
tests, and ensure it runs for every test to keep assertions independent
(alternatively remove PER_CLASS or TestMethodOrder if per-test instance behavior
is acceptable).
- Around line 149-173: Add a test that injects SQL into the user field and
asserts the returned total/count from the paged response so the DAO.count
regression is covered: create a QueryHistoryRequest with a malicious user value
(e.g. "' OR '1'='1") and call queryHistoryManager.findQueryHistory(...) and
assert the TableData returned has total/count == 0 (use TableData.getTotal() or
its equivalent), then also assert the normalRequest still reports total/count ==
1; this ensures both QueryHistoryRequest handling in
queryHistoryManager.findQueryHistory and the QueryHistoryDao.count(...)
parameterization are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2f30fa1a-ab7b-4995-9764-46bf0f7cd007
📒 Files selected for processing (3)
gateway-ha/src/main/java/io/trino/gateway/ha/persistence/dao/QueryHistoryDao.javagateway-ha/src/main/java/io/trino/gateway/ha/router/HaQueryHistoryManager.javagateway-ha/src/test/java/io/trino/gateway/ha/router/BaseTestQueryHistoryManager.java
|
Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla |
|
This pull request has gone a while without any activity. Ask for help on #trino-gateway-dev on Trino slack. |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
@ebyhr @trinodb/maintainers-gateway .. good to go? |
byungnam
force-pushed
the
fix-sql-injection
branch
2 times, most recently
from
cbbe436 to
7c273d5
Compare
|
Can you resolve conflicts? |
byungnam
force-pushed
the
fix-sql-injection
branch
from
7c273d5 to
4237dcc
Compare
done |
4 participants
Description
Fix SQL injection vulnerability described in #991
Additional context and related issues
Replaced raw string concatenation with parameters. User input is no longer interpolated into the SQL string.
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required, with the following suggested text:
* Fix SQL injection vulnerability in /webapp/findQueryHistory endpoint