Repository navigation
[Security] SQL injection in get_collection_info via unsanitized collection_name #287
Description
Summary
DatabasePostgreSQL.get_collection_info() interpolates its collection_name
argument directly into an SQL string, allowing SQL injection if that argument is
externally controlled.
Details
Affected component: evoagentx/tools/database_postgresql.py
Affected function: get_collection_info(self, collection_name)
Line 896 builds the query with an f-string:
:896 cur.execute(f"SELECT COUNT(*) as row_count FROM {collection_name}")Key observation — the same function already does it correctly 2 lines below:
:898 cur.execute("SELECT column_name, data_type FROM information_schema.columns "
"WHERE table_name = %s", (collection_name,))The same parameter, in the same function, is parameterized at line 898 and
interpolated at line 896. This suggests the omission rather than an intentional
design choice.
The database_* classes are exposed as tools for LLM/Agent invocation, so
collection_name originates from model output rather than a trusted caller.
PoC
db = DatabasePostgreSQL(...) # initialized per project docs
db.get_collection_info("x; DROP TABLE users; --")The injected fragment is concatenated verbatim into the statement executed at line 896.
An equivalent test that reads data without modifying it:
db.get_collection_info("information_schema.tables WHERE table_name='users'--")Impact
- Attack complexity: low — requires being able to invoke the tool, i.e. influencing
model output (indirect prompt injection, or a user of the agent itself) - Privileges required: none at the HTTP layer; the statement executes with the
database account configured for the deployment - Confidentiality impact: read access to whatever the database account can read
(credentials, user data) - Integrity / availability: depends on the database account's grants; if the
account is write-capable, this escalates to data modification and deletion
Suggested fix
Parameterize line 896:
cur.execute("SELECT COUNT(*) FROM information_schema.tables WHERE table_name = %s",
(collection_name,))Note that :898 already queries information_schema.tables. Reusing that result
avoids the second query entirely:
# :898 already fetched the row set; derive the count from it instead of
# interpolating the name into another statement.More generally, identifiers such as table names cannot be passed as bound
parameters, so if arbitrary table names must be supported, validate them against
the catalog first:
if not re.fullmatch(r"[A-Za-z_][A-Za-z0-9_]*", collection_name):
return self.format_error_result("Invalid table name")Environment
- Version/commit:
main@d77fd6b9a3e7（2026-08-27） - Verified by reading the source at that commit
-Tested only aginst the project's own code locally;no third-party system was involve
Activity
phoen1x-git commentedon Oct 8, 2026
Related: #211 reports a sandbox escape in interpreter_python.py. I re-checked
on main @ d77fd6b and the bypass still appears reproducible (the AST check
only rejects ast.Import / ast.ImportFrom, while
imp = __import__is a plainassignment). Raising separately since it is a different code path.
akushonkamen commentedon Oct 11, 2026
Confirmed:
evoagentx/tools/database_postgresql.py:896interpolatescollection_namevia f-string intocur.execute(), while :898 in the same function already uses a parameterized%splaceholder — clearly an omission. I'm preparing a fix (parameterize/validate the identifier) and will follow up. Note: this work is assisted by an AI coding agent on my behalf.akushonkamen commentedon Oct 11, 2026
Fix ready on my local branch (will follow the maintainers' preferred contribution flow):
evoagentx/tools/database_postgresql.py:897-903:collection_nameis now wrapped withpsycopg2.sql.Identifierviapsycopg2.sql.SQL(...).format(...), so the value is safely quoted as an identifier instead of being interpolated into the statement. This preserves current semantics (real row count of the table) rather than counting catalog rows.get_collection_infonow callsconn.rollback()on error, matching the existing convention inexecute_query()(:637-641). Without it, the correctly-rejected injected name leaves the transaction aborted and poisons every subsequent query on the shared connection — verified live before/after.tests/src/tools/test_database_postgresql.py): mocked-cursor tests assert the payload only appears as a quotedIdentifierand that errors roll back, plus an opt-in live-PG test gated onEVOAGENTX_TEST_PG_DSN.Adjacent surface, flagging for maintainer triage (not changed by this fix): the sibling tools
postgresql_find(:1022-1028),postgresql_update(:1057-1060) andpostgresql_delete(:1095-1098) build statements by f-string-interpolating model-controlledtable_name/where/set/sortfragments. Since these tools deliberately accept raw SQL fragments, that looks like a by-design capability — but it means those tools are injectable by design and may warrant docs/allow-listing/sandboxing guidance rather than a code change.This work was assisted by an AI coding agent on my behalf.
akushonkamen commentedon Oct 11, 2026
PR opened: #290 — root cause, fix and full test evidence are in the PR description. (Root-cause confirmation, local-fix notice and the find/update/delete extension-surface note were already reported above in comments 6112502360 / 6112649648.)
phoen1x-git commentedon Oct 12, 2026
Reporter of #287 here — thanks for picking this up.
I went through the patch statically against
evoagentx/tools/database_postgresql.py(no live PostgreSQL instance, so this is a source read, not a run):
The fix is right. A table name is an identifier, not a bindable parameter, so
psycopg2.sql.Identifieris the correct tool here — and it keeps the function'soriginal semantics (the real row count) instead of quietly falling back to
counting catalog rows. The
rollback()in the error path is a good catch aswell; it matches the convention already used in
execute_query()(:635-641),and without it a rejected name would leave the shared connection's transaction
aborted and poison the queries that follow.
One minor note on the identifier quoting (non-blocking):
psycopg2.sql.Identifier("public.users")renders as a single quoted identifier —
"public.users"— not"public"."users";it only splits when the parts are passed as separate arguments,
Identifier("public", "users"). So a schema-qualified name would now fail wherethe old f-string happened to work. Impact is low here, since the
information_schema.columns WHERE table_name = %squery two lines below alreadyrequires a bare table name — but a one-line comment there might save someone a
confusing error later.
Agreed on the adjacent surface being by design. The
postgresql_findtooldescription advertises the raw fragments outright —
where= "WHERE clause(optional, e.g., 'age > 18')" (:1008),
sort= "ORDER BY clause (optional, e.g.,'age ASC')" (:1012); same pattern in update (:1043) and delete (:1087). So :896
reads as an omission while those are the interface — treating them as a
documentation / allow-listing / sandboxing question rather than a code change
looks like the right split to me.