Repository navigation
fix: parameterize table name in PostgreSQL get_collection_info - #290
Open
akushonkamen wants to merge 1 commit into
Open
akushonkamen wants to merge 1 commit into
akushonkamen wants to merge 1 commit into
Conversation
get_collection_info() interpolated the LLM-controlled collection_name directly into an f-string SQL statement while the same function already parameterized the same argument two lines below (issue ANative-Lab#287). Table names are identifiers and cannot be bound parameters, so wrap them with psycopg2.sql.Identifier, which safely quotes the value. Also roll back on error in get_collection_info so a rejected name does not leave the session's transaction aborted for subsequent queries, matching the existing convention in execute_query(). Add regression tests: mocked-cursor tests assert the payload only rides as a quoted Identifier and that errors roll back, plus an opt-in live PostgreSQL test (EVOAGENTX_TEST_PG_DSN).
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This branch has not been deployed
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.
Fixes #287
Root cause
get_collection_info()built its row-count query with an f-string, interpolating the LLM-controlledcollection_namedirectly into SQL (SELECT COUNT(*) as row_count FROM {collection_name}, pre-fixevoagentx/tools/database_postgresql.py:896) — while the same function already passed the very same argument as a bound parameter to theinformation_schema.columnsquery two lines below (:898), so this one call site is an omission. A name likeinformation_schema.tables WHERE table_name='users'--escapes the statement and executes against arbitrary SQL.Fix
psycopg2.sql.SQL("SELECT COUNT(*) as row_count FROM {}").format(psycopg2.sql.Identifier(collection_name)), which safely quotes the value while preserving the function's semantics of returning the real row count of the named table.conn.rollback()in the function'sexceptblock, aligning with the existing convention inexecute_query()(:637-641): without it, a correctly rejected injection leaves the session's transaction aborted and poisons subsequent queries on the same connection — reproduced and verified during testing.Tests
New regression tests: mocked-cursor tests assert the payload only rides as a quoted
Identifierand that errors trigger rollback, plus an opt-in live PostgreSQL test (enabled viaEVOAGENTX_TEST_PG_DSN).postgres:16, non-destructive read-only): on unfixed code, payloadinformation_schema.tables WHERE table_name='users'--returnedsuccess=True, row_count=1, proving execution against the catalog. After the fix, the injection returnssuccess=False(quoted-identifier error), the legitimateuserstable reportsrow_count=2, and ax; DROP TABLE users; --payload only produces a quoting error — the table stays intact.pytest tests/src/tools/test_database_postgresql.py -k "not live_postgres" -q: 3 failed before the fix, 3 passed after.-m live_postgreswithEVOAGENTX_TEST_PG_DSN): 1 passed.-k databasesubset cannot fully collect in our environment: 18–24 pre-existingModuleNotFoundErrorcollection errors from heavy deps (the repo-pinnedfaiss-cpu==1.8.0.post1has no Python 3.14 wheel, verified via pip). Degraded faithful run with--continue-on-collection-errors: 3 passed, 1 skipped, 99 deselected, 18 errors — none of the errored modules matchdatabase.pytest tests/src/tools/test_alita_generated_code_tool.py→ 7 passed.Note
While verifying, we noticed the sibling find/update/delete tools in the same file also splice model-controllable fragments into SQL via f-strings — possibly by design. Flagged on the issue for maintainer triage: #287 (comment)