From 23f04264f9bbd403ec9c123a9f5541a62c15d006 Mon Sep 17 00:00:00 2001 From: Syed Osama Ali Shah <86572800+Osamaali313@users.noreply.github.com> Date: Fri, 17 Jul 2026 02:36:31 +0300 Subject: [PATCH] fix(knowledge_sql): match write keywords on word boundaries (allow valid SELECTs) (#640) * fix(knowledge_sql): match write keywords on word boundaries The read-only guard rejected a query if any of DROP/DELETE/INSERT/UPDATE/ ALTER/CREATE/ATTACH appeared as a bare substring of the uppercased text. That wrongly blocks valid SELECTs whose column/alias/literal merely contains one -- e.g. "deleted_at" (DELETE), "created_at" (CREATE), "updated_content" (UPDATE). The knowledge_chunks table actually has deleted_at/created_at columns and the store's own retrieval filters on "WHERE deleted_at IS NULL", so realistic read queries were refused. Match on word boundaries with a compiled regex, mirroring the sibling tool db_query.py. Add a regression test. * fix(knowledge_sql): ignore string literals in keyword scan, broaden error handling - Strip single-quoted literals before the forbidden-keyword scan so SELECTs whose data merely mentions a write keyword (e.g. LIKE '%delete%') are not rejected. - Catch sqlite3.Error instead of only OperationalError so multi- statement strings return a failed ToolResult instead of raising. - Document created_at/deleted_at in the tool's schema description. --------- Co-authored-by: Elliot Slusky --- src/openjarvis/tools/knowledge_sql.py | 41 ++++++++++++++++++--------- tests/tools/test_knowledge_sql.py | 35 +++++++++++++++++++++++ 2 files changed, 63 insertions(+), 13 deletions(-) diff --git a/src/openjarvis/tools/knowledge_sql.py b/src/openjarvis/tools/knowledge_sql.py index 15001de9..67e66bc1 100644 --- a/src/openjarvis/tools/knowledge_sql.py +++ b/src/openjarvis/tools/knowledge_sql.py @@ -6,6 +6,7 @@ and filtering operations that BM25 search cannot handle. from __future__ import annotations +import re import sqlite3 from typing import Any, Optional @@ -16,10 +17,25 @@ from openjarvis.tools._stubs import BaseTool, ToolSpec _MAX_ROWS = 50 +# Write keywords are matched on word boundaries (mirroring db_query.py) so that +# a read-only SELECT is not rejected just because a column/alias/literal happens +# to contain one as a substring (e.g. "deleted_at", "created_at"). +_FORBIDDEN_RE = re.compile( + r"\b(DROP|DELETE|INSERT|UPDATE|ALTER|CREATE|TRUNCATE|ATTACH)\b", + re.IGNORECASE, +) + +# String literals are stripped before the keyword scan so that data mentioning +# a write keyword (e.g. WHERE content LIKE '%delete%') is not rejected. A write +# "hidden" in a literal still cannot execute: the query must start with SELECT +# and sqlite3 refuses multi-statement strings. +_STRING_LITERAL_RE = re.compile(r"'[^']*'") + _SCHEMA_DESCRIPTION = ( "Table: knowledge_chunks\n" "Columns: id, content, source, doc_type, doc_id, title, author, " - "participants, timestamp, thread_id, url, metadata, chunk_index" + "participants, timestamp, thread_id, url, metadata, chunk_index, " + "created_at, deleted_at (NULL for active rows)" ) @@ -84,21 +100,20 @@ class KnowledgeSQLTool(BaseTool): success=False, ) - _FORBIDDEN = ("DROP", "DELETE", "INSERT", "UPDATE", "ALTER", "CREATE", "ATTACH") - for forbidden in _FORBIDDEN: - if forbidden in normalized: - return ToolResult( - tool_name="knowledge_sql", - content=( - f"Query contains forbidden keyword: {forbidden}." - " Only SELECT queries allowed." - ), - success=False, - ) + forbidden = _FORBIDDEN_RE.search(_STRING_LITERAL_RE.sub("''", query)) + if forbidden: + return ToolResult( + tool_name="knowledge_sql", + content=( + f"Query contains forbidden keyword: {forbidden.group(1).upper()}." + " Only SELECT queries allowed." + ), + success=False, + ) try: rows = self._store._conn.execute(query).fetchmany(_MAX_ROWS) - except sqlite3.OperationalError as exc: + except sqlite3.Error as exc: return ToolResult( tool_name="knowledge_sql", content=f"SQL error: {exc}", diff --git a/tests/tools/test_knowledge_sql.py b/tests/tools/test_knowledge_sql.py index 7f434e7b..1b3f8d70 100644 --- a/tests/tools/test_knowledge_sql.py +++ b/tests/tools/test_knowledge_sql.py @@ -64,6 +64,41 @@ def test_rejects_drop(store: KnowledgeStore) -> None: assert not result.success +def test_allows_select_with_keyword_substring(store: KnowledgeStore) -> None: + """A read-only SELECT must not be rejected because a column/alias merely + contains a write keyword as a substring (e.g. 'created' -> CREATE).""" + from openjarvis.tools.knowledge_sql import KnowledgeSQLTool + + tool = KnowledgeSQLTool(store=store) + result = tool.execute( + query="SELECT author AS created_author FROM knowledge_chunks" + ) + assert result.success, result.content + assert "Alice" in result.content + + +def test_allows_keyword_inside_string_literal(store: KnowledgeStore) -> None: + """A write keyword appearing only inside a string literal must not be + treated as a forbidden statement.""" + from openjarvis.tools.knowledge_sql import KnowledgeSQLTool + + tool = KnowledgeSQLTool(store=store) + result = tool.execute( + query="SELECT content FROM knowledge_chunks WHERE content LIKE '%delete%'" + ) + assert result.success, result.content + + +def test_rejects_multi_statement(store: KnowledgeStore) -> None: + """Multi-statement strings fail with a ToolResult, not an exception.""" + from openjarvis.tools.knowledge_sql import KnowledgeSQLTool + + tool = KnowledgeSQLTool(store=store) + result = tool.execute(query="SELECT 1; VACUUM") + assert not result.success + assert "error" in result.content.lower() + + def test_handles_bad_sql(store: KnowledgeStore) -> None: from openjarvis.tools.knowledge_sql import KnowledgeSQLTool