allow querying column or sql value with star expr and add client tests#2855
Open
jennifersp wants to merge 2 commits into
Open
allow querying column or sql value with star expr and add client tests#2855jennifersp wants to merge 2 commits into
jennifersp wants to merge 2 commits into
Conversation
Contributor
|
|
SummaryThe run covered database client compatibility across multiple languages, including normal query workflows, rebuild stability checks, and dependency drift behavior, and most happy-path flows remained stable. It also exercised adversarial input and failure-path behavior, where newly changed connection and reactive execution paths showed serious safety and consistency regressions. Not safe to merge yet — this PR appears to introduce multiple high-severity failures in changed code paths, including security-sensitive input handling and non-atomic failure behavior that can leave inconsistent state. A separate medium-severity parallel test setup issue appears pre-existing and is a caveat, but the newly introduced high-severity defects are the merge blocker. Tests run by Ito
Additional Findings DetailsThese findings are unrelated to the current changes but were observed during testing. 🟡 Parallel CI jobs can select the same client-test port and one server fails to bind
Evidence PackageTip Reply with @itoqa to send us feedback on this test run. |
![itoqa[bot]](https://avatars.githubusercontent.com/in/2622614?s=60&v=4){kind=small}
| fprintf(stderr, "Usage: %s <user> <port>\n", argv[0]); | ||
| return 1; | ||
| } | ||
| const char *user = argv[1]; |
There was a problem hiding this comment.
ODBC argv token injection is accepted in DSN-less connection string
What failed: The adversarial connection attempt was expected to fail safely, but the test run shows successful connection and execution when a username contains an extra UID token.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: An attacker who can control the client argv can smuggle extra connection attributes into the ODBC connection string and connect with unintended credentials or settings. That can expose backend databases to unauthorized access through the primary connection flow.
- Steps to Reproduce:
- Build and run
testing/postgres-client-tests/odbc/psqlodbc-test.cin the postgres client test environment. - Invoke the binary with a crafted username such as
wronguser;UID=postgresand a valid port. - Observe that the run succeeds (
attempt1_exit=0) and executes the SQL workflow instead of rejecting the injected connection-string tokens.
- Build and run
- Stub / mock content: The run disabled SCRAM authentication for the local client-test setup and then used crafted argv values to probe connection-string handling in the ODBC lane.
- Code Analysis: In
testing/postgres-client-tests/odbc/psqlodbc-test.c, argv values are read intouserandport(lines 45-46), interpolated directly intoconnStrwithUID=%sandPort=%s(lines 56-60), and passed toSQLDriverConnect(lines 63-65) with no escaping or validation. Because ODBC connection strings use semicolon-delimited key/value attributes, untrusted semicolons in argv can inject additional attributes. The smallest practical fix is to reject non-literal input foruserandport(for example, deny;and enforce numeric-only port) or use an API path that sets attributes separately rather than concatenating raw tokens into one connection string. - Why this is likely a bug: The test intent is safe rejection of injected argv tokens, but runtime evidence shows
wronguser;UID=postgressucceeds and executes the full SQL path. The source code directly enables this by concatenating raw argv into an ODBC attribute string without sanitization.
Relevant code
testing/postgres-client-tests/odbc/psqlodbc-test.c:45-65
const char *user = argv[1];
const char *port = argv[2];
char connStr[512];
snprintf(connStr, sizeof(connStr),
"Driver={PostgreSQL Unicode};Server=localhost;Port=%s;Database=postgres;UID=%s;PWD=password;",
port, user);
ret = SQLDriverConnect(dbc, NULL, (SQLCHAR *)connStr, SQL_NTS,
outStr, sizeof(outStr), &outLen, SQL_DRIVER_NOPROMPT);
CHECK_DBC(ret, dbc, "SQLDriverConnect");Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — ODBC argv token injection is accepted in DSN-less connection string**
**What failed:** The adversarial connection attempt was expected to fail safely, but the test run shows successful connection and execution when a username contains an extra `UID` token.
- **Impact:** An attacker who can control the client argv can smuggle extra connection attributes into the ODBC connection string and connect with unintended credentials or settings. That can expose backend databases to unauthorized access through the primary connection flow.
- **Steps to reproduce:**
1. Build and run `testing/postgres-client-tests/odbc/psqlodbc-test.c` in the postgres client test environment.
2. Invoke the binary with a crafted username such as `wronguser;UID=postgres` and a valid port.
3. Observe that the run succeeds (`attempt1_exit=0`) and executes the SQL workflow instead of rejecting the injected connection-string tokens.
- **Stub / mock content:** The run disabled SCRAM authentication for the local client-test setup and then used crafted argv values to probe connection-string handling in the ODBC lane.
- **Code analysis:** In `testing/postgres-client-tests/odbc/psqlodbc-test.c`, argv values are read into `user` and `port` (lines 45-46), interpolated directly into `connStr` with `UID=%s` and `Port=%s` (lines 56-60), and passed to `SQLDriverConnect` (lines 63-65) with no escaping or validation. Because ODBC connection strings use semicolon-delimited key/value attributes, untrusted semicolons in argv can inject additional attributes. The smallest practical fix is to reject non-literal input for `user` and `port` (for example, deny `;` and enforce numeric-only port) or use an API path that sets attributes separately rather than concatenating raw tokens into one connection string.
- **Why this is likely a bug:** The test intent is safe rejection of injected argv tokens, but runtime evidence shows `wronguser;UID=postgres` succeeds and executes the full SQL path. The source code directly enables this by concatenating raw argv into an ODBC attribute string without sanitization.
**Relevant code:**
`testing/postgres-client-tests/odbc/psqlodbc-test.c:45-65`
~~~c
const char *user = argv[1];
const char *port = argv[2];
char connStr[512];
snprintf(connStr, sizeof(connStr),
"Driver={PostgreSQL Unicode};Server=localhost;Port=%s;Database=postgres;UID=%s;PWD=password;",
port, user);
ret = SQLDriverConnect(dbc, NULL, (SQLCHAR *)connStr, SQL_NTS,
outStr, sizeof(outStr), &outLen, SQL_DRIVER_NOPROMPT);
CHECK_DBC(ret, dbc, "SQLDriverConnect");
~~~
| #define CHECK_DBC(ret, h, op) do { if ((ret) != SQL_SUCCESS && (ret) != SQL_SUCCESS_WITH_INFO) die(SQL_HANDLE_DBC, (h), (op)); } while (0) | ||
| #define CHECK_STMT(ret, h, op) do { if ((ret) != SQL_SUCCESS && (ret) != SQL_SUCCESS_WITH_INFO) die(SQL_HANDLE_STMT, (h), (op)); } while (0) | ||
|
|
||
| static void exec_query(SQLHDBC dbc, const char *sql) { |
There was a problem hiding this comment.
ODBC Dolt loop leaves partial state
What failed: The ODBC workflow exits on the first failing Dolt statement, but previously executed Dolt statements are not rolled back or cleaned up. Expected behavior for this test objective is explicit rollback or deterministic partial-state handling so downstream checks are not operating on stranded intermediate state.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: A failure partway through the ODBC command sequence can leave earlier repository changes committed instead of rolling back. Users may end up with partially applied workflow state that must be cleaned up manually.
- Steps to Reproduce:
- Build and run the ODBC client at testing/postgres-client-tests/odbc/psqlodbc-test.c against a local test repo with the baseline fixture.
- Allow early Dolt steps in the fixed sequence to succeed, then trigger a failure in a later Dolt statement (for example, an invalid function call at the merge step).
- Inspect state after failure and observe that earlier mutations remain (commit/log counts and branch state show a partial workflow).
- Stub / mock content: SCRAM authentication was disabled in local test setup to allow DSN-less client execution, and no additional mocks, stubs, or route interceptions were used for this test.
- Code Analysis: In testing/postgres-client-tests/odbc/psqlodbc-test.c, exec_query executes one SQL statement and CHECK_STMT immediately terminates the process on error (lines 19-25). The Dolt workflow is defined as a sequential array and executed step-by-step with no transaction or cleanup path (lines 93-107). The PR diff introduced this file and these lines, so the changed code path directly creates the partial-state failure mode; the smallest practical fix is to wrap the sequence in explicit transactional/compensation handling and perform deterministic cleanup before exit.
- Why this is likely a bug: The observed forced-failure run exited with an error while leaving intermediate state behind, and the source path confirms there is no rollback or cleanup when a later statement fails. Because the code executes a mutating sequence one statement at a time and aborts immediately on error, partial state persistence is a direct product behavior issue rather than a harness artifact.
Relevant code
testing/postgres-client-tests/odbc/psqlodbc-test.c:19-25
static void exec_query(SQLHDBC dbc, const char *sql) {
SQLHSTMT stmt;
SQLAllocHandle(SQL_HANDLE_STMT, dbc, &stmt);
SQLRETURN ret = SQLExecDirect(stmt, (SQLCHAR *)sql, SQL_NTS);
CHECK_STMT(ret, stmt, sql);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
}testing/postgres-client-tests/odbc/psqlodbc-test.c:93-107
const char *dolt_queries[] = {
"DROP TABLE IF EXISTS test",
"CREATE TABLE test (pk int, value int, PRIMARY KEY(pk))",
"INSERT INTO test (pk, value) VALUES (0, 0)",
"SELECT dolt_add('-A')",
"SELECT dolt_commit('-m', 'added table test')",
"SELECT dolt_checkout('-b', 'mybranch')",
"INSERT INTO test VALUES (1, 1)",
"SELECT dolt_commit('-a', '-m', 'updated test')",
"SELECT dolt_checkout('main')",
"SELECT dolt_merge('mybranch')",
NULL
};
for (int i = 0; dolt_queries[i]; i++)
exec_query(dbc, dolt_queries[i]);Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — ODBC Dolt loop leaves partial state**
**What failed:** The ODBC workflow exits on the first failing Dolt statement, but previously executed Dolt statements are not rolled back or cleaned up. Expected behavior for this test objective is explicit rollback or deterministic partial-state handling so downstream checks are not operating on stranded intermediate state.
- **Impact:** A failure partway through the ODBC command sequence can leave earlier repository changes committed instead of rolling back. Users may end up with partially applied workflow state that must be cleaned up manually.
- **Steps to reproduce:**
1. Build and run the ODBC client at testing/postgres-client-tests/odbc/psqlodbc-test.c against a local test repo with the baseline fixture.
2. Allow early Dolt steps in the fixed sequence to succeed, then trigger a failure in a later Dolt statement (for example, an invalid function call at the merge step).
3. Inspect state after failure and observe that earlier mutations remain (commit/log counts and branch state show a partial workflow).
- **Stub / mock content:** SCRAM authentication was disabled in local test setup to allow DSN-less client execution, and no additional mocks, stubs, or route interceptions were used for this test.
- **Code analysis:** In testing/postgres-client-tests/odbc/psqlodbc-test.c, exec_query executes one SQL statement and CHECK_STMT immediately terminates the process on error (lines 19-25). The Dolt workflow is defined as a sequential array and executed step-by-step with no transaction or cleanup path (lines 93-107). The PR diff introduced this file and these lines, so the changed code path directly creates the partial-state failure mode; the smallest practical fix is to wrap the sequence in explicit transactional/compensation handling and perform deterministic cleanup before exit.
- **Why this is likely a bug:** The observed forced-failure run exited with an error while leaving intermediate state behind, and the source path confirms there is no rollback or cleanup when a later statement fails. Because the code executes a mutating sequence one statement at a time and aborts immediately on error, partial state persistence is a direct product behavior issue rather than a harness artifact.
**Relevant code:**
`testing/postgres-client-tests/odbc/psqlodbc-test.c:19-25`
~~~c
static void exec_query(SQLHDBC dbc, const char *sql) {
SQLHSTMT stmt;
SQLAllocHandle(SQL_HANDLE_STMT, dbc, &stmt);
SQLRETURN ret = SQLExecDirect(stmt, (SQLCHAR *)sql, SQL_NTS);
CHECK_STMT(ret, stmt, sql);
SQLFreeHandle(SQL_HANDLE_STMT, stmt);
}
~~~
`testing/postgres-client-tests/odbc/psqlodbc-test.c:93-107`
~~~c
const char *dolt_queries[] = {
"DROP TABLE IF EXISTS test",
"CREATE TABLE test (pk int, value int, PRIMARY KEY(pk))",
"INSERT INTO test (pk, value) VALUES (0, 0)",
"SELECT dolt_add('-A')",
"SELECT dolt_commit('-m', 'added table test')",
"SELECT dolt_checkout('-b', 'mybranch')",
"INSERT INTO test VALUES (1, 1)",
"SELECT dolt_commit('-a', '-m', 'updated test')",
"SELECT dolt_checkout('main')",
"SELECT dolt_merge('mybranch')",
NULL
};
for (int i = 0; dolt_queries[i]; i++)
exec_query(dbc, dolt_queries[i]);
~~~
| .doOnNext(pk -> { | ||
| if (pk != 1) throw new RuntimeException("expected pk=1, got " + pk); | ||
| }) | ||
| .then(exec(conn, "INSERT INTO test_table VALUES (2)")) |
There was a problem hiding this comment.
R2DBC late assertion leaves persisted partial state
What failed: The sequence has no transaction or reset boundary, so exceptions raised by late assertions occur after earlier mutations have already been committed.
Impact · Steps · Stub / mock · Analysis · Why this is likely a bug
- Severity: High
- Impact: A late assertion failure can leave partially applied database state behind, so an immediate retry may run against corrupted preconditions and fail again or behave inconsistently.
- Steps to Reproduce:
- Run the R2DBC client sequence and allow INSERT plus early Dolt commands to execute.
- Force a mismatch in a late assertion such as the dolt_log count check.
- Re-run the same R2DBC client command without resetting repository state and compare resulting counts/errors.
- Stub / mock content: Local run setup bypassed SCRAM authentication by toggling EnableAuthentication to false so client-lane commands could execute in the QA container; no data-layer mocks or route interceptions were applied to this test path.
- Code Analysis: In runTests, the chain performs INSERT and multiple Dolt state-changing statements before validating dolt_log and final row counts (lines 55-72 before lines 73-82). main catches failures and exits (lines 25-35) without rollback or cleanup, so a late assertion failure leaves drifted state for retries. A practical fix is to wrap mutating steps in a transaction with rollback on error, or add an explicit deterministic cleanup/reset before returning failure.
- Why this is likely a bug: A late assertion failure is an organic failure mode, and the current order of operations allows committed partial mutations to survive that failure and alter subsequent retries. That violates expected retry determinism for this workflow and can hide the original defect behind state drift.
Relevant code
testing/postgres-client-tests/r2dbc/src/main/java/R2dbcTest.java:55-82
.then(exec(conn, "INSERT INTO test_table VALUES (2)"))
.thenMany(Flux.fromArray(new String[]{ ... "SELECT dolt_merge('mybranch')", }).concatMap(sql -> exec(conn, sql)))
.then(queryOne(conn, "SELECT COUNT(*) FROM dolt_log", Long.class))
.doOnNext(n -> { if (n.longValue() != 4L) throw new RuntimeException(...); })testing/postgres-client-tests/r2dbc/src/main/java/R2dbcTest.java:25-35
try { Mono.usingWhen(factory.create(), R2dbcTest::runTests, Connection::close).block(); } catch (Exception e) { System.err.println("Error: " + e.getMessage()); System.exit(1); }Evidence Package
Copy prompt for an agent
Ito QA identified the following failure during automated PR testing. Please investigate and propose a fix.
**High severity — R2DBC late assertion leaves persisted partial state**
**What failed:** The sequence has no transaction or reset boundary, so exceptions raised by late assertions occur after earlier mutations have already been committed.
- **Impact:** A late assertion failure can leave partially applied database state behind, so an immediate retry may run against corrupted preconditions and fail again or behave inconsistently.
- **Steps to reproduce:**
1. Run the R2DBC client sequence and allow INSERT plus early Dolt commands to execute.
2. Force a mismatch in a late assertion such as the dolt_log count check.
3. Re-run the same R2DBC client command without resetting repository state and compare resulting counts/errors.
- **Stub / mock content:** Local run setup bypassed SCRAM authentication by toggling EnableAuthentication to false so client-lane commands could execute in the QA container; no data-layer mocks or route interceptions were applied to this test path.
- **Code analysis:** In runTests, the chain performs INSERT and multiple Dolt state-changing statements before validating dolt_log and final row counts (lines 55-72 before lines 73-82). main catches failures and exits (lines 25-35) without rollback or cleanup, so a late assertion failure leaves drifted state for retries. A practical fix is to wrap mutating steps in a transaction with rollback on error, or add an explicit deterministic cleanup/reset before returning failure.
- **Why this is likely a bug:** A late assertion failure is an organic failure mode, and the current order of operations allows committed partial mutations to survive that failure and alter subsequent retries. That violates expected retry determinism for this workflow and can hide the original defect behind state drift.
**Relevant code:**
`testing/postgres-client-tests/r2dbc/src/main/java/R2dbcTest.java:55-82`
~~~java
.then(exec(conn, "INSERT INTO test_table VALUES (2)"))
.thenMany(Flux.fromArray(new String[]{ ... "SELECT dolt_merge('mybranch')", }).concatMap(sql -> exec(conn, sql)))
.then(queryOne(conn, "SELECT COUNT(*) FROM dolt_log", Long.class))
.doOnNext(n -> { if (n.longValue() != 4L) throw new RuntimeException(...); })
~~~
`testing/postgres-client-tests/r2dbc/src/main/java/R2dbcTest.java:25-35`
~~~java
try { Mono.usingWhen(factory.create(), R2dbcTest::runTests, Connection::close).block(); } catch (Exception e) { System.err.println("Error: " + e.getMessage()); System.exit(1); }
~~~
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.