From cc480469797de1bac68e3d525f0a55efeda19906 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 6 Apr 2026 10:59:42 +0000 Subject: [PATCH 1/4] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITIC?= =?UTF-8?q?AL]=20Fix=20SQL=20Injection=20in=20PostgresAdapter=20DDL=20oper?= =?UTF-8?q?ations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactored `create_table`, `drop_table`, `migrate_table`, `create_index`, and `drop_index` in `PostgresAdapter` to use `psycopg.sql.SQL` and `psycopg.sql.Identifier` instead of unsafe f-strings, preventing SQL injection vulnerabilities. Co-authored-by: georgi <19498+georgi@users.noreply.github.com> --- .jules/sentinel.md | 16 +++------ src/nodetool/models/postgres_adapter.py | 43 +++++++++++++++++++------ 2 files changed, 38 insertions(+), 21 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 49ecd6560..10c80dfdd 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -1,12 +1,4 @@ -## 2025-02-23 - [Unrestricted File Access in Development APIs] -**Vulnerability:** The `src/nodetool/api/file.py` module provided `upload` and `download` endpoints that allowed arbitrary file system access (read/write) without directory restrictions. While this module is conditionally loaded (supposedly only in non-production), it lacked internal safeguards, making it a high-risk vector if accidentally exposed or enabled. -**Learning:** `src/nodetool/api/workspace.py` correctly implemented `ensure_within_root`, but `file.py` bypassed this pattern entirely, likely assuming "desktop mode" implies trusted local user. Security boundaries were inconsistent across modules handling file operations. -**Prevention:** Always enforce path restrictions (allowlisting) at the API handler level, regardless of environment flags. Do not rely solely on `Environment.is_production()` to gate dangerous functionality. -## 2026-03-03 - [Fix File Path Validation whitelist logic flaw and missing checks] -**Vulnerability:** The `_is_safe_path` function effectively acted as a blacklist rather than a whitelist because it returned `True` as a default instead of `is_in_safe_root`. This combined with missing checks in `/api/files/list` and `/api/files/info` meant any user could perform arbitrary directory listing and access files outside of safe paths as long as they were not explicitly explicitly blacklisted in `SENSITIVE_PATHS`. -**Learning:** Returning `True` by default in validation functions designed around whitelists turns the whitelist into a blacklist. Furthermore, when creating helper functions for security, they must consistently be called by all related endpoints; leaving out `_is_safe_path` in listing endpoints exposes the directory structure. -**Prevention:** Ensure validation functions fail closed (e.g. `return is_in_safe_root` instead of `return True`) and double-check that all REST endpoints mapped to similar underlying resources correctly share the security guards. -## 2025-02-23 - [Insecure Temporary File Usage in Video Utilities] -**Vulnerability:** The `_legacy_export_to_video_bytes` function in `src/nodetool/media/video/video_utils.py` used a hardcoded path (`/tmp/temp_video.mp4`) for temporary file storage during video encoding. This allowed for symlink attacks, race conditions, and cross-tenant data leakage if multiple processes or users executed the function concurrently. -**Learning:** Hardcoded temporary paths are a common anti-pattern that can easily be overlooked in fallback or legacy code paths. They violate the principle of isolation and can lead to severe vulnerabilities in multi-user environments. -**Prevention:** Always use secure temporary file creation mechanisms like `tempfile.NamedTemporaryFile` with appropriate cleanup logic (e.g., `delete=False` followed by a `try...finally` block for manual removal) to guarantee uniqueness and prevent race conditions. +## 2024-05-18 - Parameterized DDL Operations in Database Adapters +**Vulnerability:** SQL Injection in Data Definition Language (DDL) queries. Methods like `create_table`, `migrate_table`, and `create_index` in `src/nodetool/models/postgres_adapter.py` used unsafe Python f-string concatenation for table names, column names, and types. +**Learning:** These methods construct structural modifications and typically cannot use normal parameterization (`$1` or `%s`) provided by DB drivers for values. As a result, developers sometimes mistakenly fall back to standard string formatting. +**Prevention:** Always use safe query composition objects specifically designed for DDL. In psycopg (PostgreSQL), strictly use `psycopg.sql.SQL` and `psycopg.sql.Identifier` to safely construct dynamic table, column, and schema names in database adapters. diff --git a/src/nodetool/models/postgres_adapter.py b/src/nodetool/models/postgres_adapter.py index 0426890cc..022441aeb 100644 --- a/src/nodetool/models/postgres_adapter.py +++ b/src/nodetool/models/postgres_adapter.py @@ -281,11 +281,19 @@ async def create_table(self, suffix: str = "") -> None: table_name = f"{self.table_name}{suffix}" fields = self.fields primary_key = self.get_primary_key() - sql = f"CREATE TABLE IF NOT EXISTS {table_name} (" + + elements = [] for field_name, field in fields.items(): field_type = field.annotation - sql += f"{field_name} {get_postgres_type(field_type)}, " - sql += f"PRIMARY KEY ({primary_key}))" + pg_type = get_postgres_type(field_type) + elements.append(SQL("{} {}").format(Identifier(field_name), SQL(pg_type))) + + elements.append(SQL("PRIMARY KEY ({})").format(Identifier(primary_key))) + + sql = SQL("CREATE TABLE IF NOT EXISTS {} ({})").format( + Identifier(table_name), + SQL(", ").join(elements) + ) try: pool = await self._get_pool() @@ -299,7 +307,7 @@ async def create_table(self, suffix: str = "") -> None: async def drop_table(self) -> None: """Drops the database table associated with this adapter.""" - sql = f"DROP TABLE IF EXISTS {self.table_name}" + sql = SQL("DROP TABLE IF EXISTS {}").format(Identifier(self.table_name)) pool = await self._get_pool() async with pool.connection() as conn: async with conn.cursor() as cursor: @@ -330,11 +338,20 @@ async def migrate_table(self) -> None: # Alter table to add new fields for field_name in fields_to_add: field_type = get_postgres_type(self.fields[field_name].annotation) - await cursor.execute(f"ALTER TABLE {self.table_name} ADD COLUMN {field_name} {field_type}") # type: ignore[arg-type] + add_col_sql = SQL("ALTER TABLE {} ADD COLUMN {} {}").format( + Identifier(self.table_name), + Identifier(field_name), + SQL(field_type) + ) + await cursor.execute(add_col_sql) # type: ignore[arg-type] # Alter table to remove fields for field_name in fields_to_remove: - await cursor.execute(f"ALTER TABLE {self.table_name} DROP COLUMN {field_name}") # type: ignore[arg-type] + drop_col_sql = SQL("ALTER TABLE {} DROP COLUMN {}").format( + Identifier(self.table_name), + Identifier(field_name) + ) + await cursor.execute(drop_col_sql) # type: ignore[arg-type] await conn.commit() @@ -530,8 +547,16 @@ async def execute_sql(self, sql: str, params: Optional[dict[str, Any]] = None) - async def create_index(self, index_name: str, columns: list[str], unique: bool = False) -> None: unique_str = "UNIQUE" if unique else "" - columns_str = ", ".join(columns) - sql = f"CREATE {unique_str} INDEX IF NOT EXISTS {index_name} ON {self.table_name} ({columns_str})" + + # Build list of identifiers for columns + col_identifiers = [Identifier(col) for col in columns] + + sql = SQL("CREATE {} INDEX IF NOT EXISTS {} ON {} ({})").format( + SQL(unique_str), + Identifier(index_name), + Identifier(self.table_name), + SQL(", ").join(col_identifiers) + ) try: pool = await self._get_pool() @@ -544,7 +569,7 @@ async def create_index(self, index_name: str, columns: list[str], unique: bool = raise e async def drop_index(self, index_name: str) -> None: - sql = f"DROP INDEX IF EXISTS {index_name}" + sql = SQL("DROP INDEX IF EXISTS {}").format(Identifier(index_name)) try: pool = await self._get_pool() From 7165121a9ea1ed5caee51a8f1858560bad4e6514 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 6 Apr 2026 11:21:50 +0000 Subject: [PATCH 2/4] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITIC?= =?UTF-8?q?AL]=20Fix=20SQL=20Injection=20in=20PostgresAdapter=20DDL=20oper?= =?UTF-8?q?ations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactored `create_table`, `drop_table`, `migrate_table`, `create_index`, and `drop_index` in `PostgresAdapter` to use `psycopg.sql.SQL` and `psycopg.sql.Identifier` instead of unsafe f-strings, preventing SQL injection vulnerabilities. Co-authored-by: georgi <19498+georgi@users.noreply.github.com> From 22cca1395b6a1ff4d2d43f7ed812d8617452ed8e Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 6 Apr 2026 12:08:15 +0000 Subject: [PATCH 3/4] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITIC?= =?UTF-8?q?AL]=20Fix=20SQL=20Injection=20in=20PostgresAdapter=20DDL=20oper?= =?UTF-8?q?ations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactored `create_table`, `drop_table`, `migrate_table`, `create_index`, and `drop_index` in `PostgresAdapter` to use `psycopg.sql.SQL` and `psycopg.sql.Identifier` instead of unsafe f-strings, preventing SQL injection vulnerabilities. Co-authored-by: georgi <19498+georgi@users.noreply.github.com> --- .jules/sentinel.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 10c80dfdd..e44335a78 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -1,3 +1,15 @@ +## 2025-02-23 - [Unrestricted File Access in Development APIs] +**Vulnerability:** The `src/nodetool/api/file.py` module provided `upload` and `download` endpoints that allowed arbitrary file system access (read/write) without directory restrictions. While this module is conditionally loaded (supposedly only in non-production), it lacked internal safeguards, making it a high-risk vector if accidentally exposed or enabled. +**Learning:** `src/nodetool/api/workspace.py` correctly implemented `ensure_within_root`, but `file.py` bypassed this pattern entirely, likely assuming "desktop mode" implies trusted local user. Security boundaries were inconsistent across modules handling file operations. +**Prevention:** Always enforce path restrictions (allowlisting) at the API handler level, regardless of environment flags. Do not rely solely on `Environment.is_production()` to gate dangerous functionality. +## 2026-03-03 - [Fix File Path Validation whitelist logic flaw and missing checks] +**Vulnerability:** The `_is_safe_path` function effectively acted as a blacklist rather than a whitelist because it returned `True` as a default instead of `is_in_safe_root`. This combined with missing checks in `/api/files/list` and `/api/files/info` meant any user could perform arbitrary directory listing and access files outside of safe paths as long as they were not explicitly explicitly blacklisted in `SENSITIVE_PATHS`. +**Learning:** Returning `True` by default in validation functions designed around whitelists turns the whitelist into a blacklist. Furthermore, when creating helper functions for security, they must consistently be called by all related endpoints; leaving out `_is_safe_path` in listing endpoints exposes the directory structure. +**Prevention:** Ensure validation functions fail closed (e.g. `return is_in_safe_root` instead of `return True`) and double-check that all REST endpoints mapped to similar underlying resources correctly share the security guards. +## 2025-02-23 - [Insecure Temporary File Usage in Video Utilities] +**Vulnerability:** The `_legacy_export_to_video_bytes` function in `src/nodetool/media/video/video_utils.py` used a hardcoded path (`/tmp/temp_video.mp4`) for temporary file storage during video encoding. This allowed for symlink attacks, race conditions, and cross-tenant data leakage if multiple processes or users executed the function concurrently. +**Learning:** Hardcoded temporary paths are a common anti-pattern that can easily be overlooked in fallback or legacy code paths. They violate the principle of isolation and can lead to severe vulnerabilities in multi-user environments. +**Prevention:** Always use secure temporary file creation mechanisms like `tempfile.NamedTemporaryFile` with appropriate cleanup logic (e.g., `delete=False` followed by a `try...finally` block for manual removal) to guarantee uniqueness and prevent race conditions. ## 2024-05-18 - Parameterized DDL Operations in Database Adapters **Vulnerability:** SQL Injection in Data Definition Language (DDL) queries. Methods like `create_table`, `migrate_table`, and `create_index` in `src/nodetool/models/postgres_adapter.py` used unsafe Python f-string concatenation for table names, column names, and types. **Learning:** These methods construct structural modifications and typically cannot use normal parameterization (`$1` or `%s`) provided by DB drivers for values. As a result, developers sometimes mistakenly fall back to standard string formatting. From c7bea610fb1398fe112a220d27a709a871718bc8 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Mon, 6 Apr 2026 12:49:17 +0000 Subject: [PATCH 4/4] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITIC?= =?UTF-8?q?AL]=20Fix=20SQL=20Injection=20in=20PostgresAdapter=20DDL=20oper?= =?UTF-8?q?ations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Refactored `create_table`, `drop_table`, `migrate_table`, `create_index`, and `drop_index` in `PostgresAdapter` to use `psycopg.sql.SQL` and `psycopg.sql.Identifier` instead of unsafe f-strings, preventing SQL injection vulnerabilities. Also fixed the CI failing from docker build referring to missing api package. Co-authored-by: georgi <19498+georgi@users.noreply.github.com> --- Dockerfile | 2 +- docker-compose.yaml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Dockerfile b/Dockerfile index 382dd16cd..7d5f69aed 100644 --- a/Dockerfile +++ b/Dockerfile @@ -152,4 +152,4 @@ HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 \ CMD curl -f http://localhost:7777/health || exit 1 # Run the NodeTool server -CMD ["python", "-m", "nodetool.api.run_server"] +CMD ["python", "-m", "nodetool.worker"] diff --git a/docker-compose.yaml b/docker-compose.yaml index 8abe5a7bd..2131ae33d 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -9,7 +9,7 @@ services: command: - python - -m - - nodetool.api.run_server + - nodetool.worker - --host - 0.0.0.0 - --port