Skip to content

Commit 6dbca9b

Browse files
dpageclaude
andcommitted
Address PR review feedback from akshay-joshi and coderabbitai
- Enforce TLS 1.2 minimum on all SSL contexts to resolve SonarQube weak SSL/TLS vulnerability across all provider files - Remove unused manager parameter from _generate_security_report_llm - Remove unused readonly_wrapper variable in database.py - Replace raise Exception with specific exception types (ValueError, ConnectionError) in model-fetching functions - Use optional chaining in ai_tools.js (res?.data?.success) - Fix all line length violations (>79 chars) in chat.py, __init__.py, test_llm_status.py, and database.py - Fix DOCKER_API_URL comment to say "Typical value" instead of "Default value" since the actual default is empty string Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
1 parent 2f80039 commit 6dbca9b

9 files changed

Lines changed: 78 additions & 44 deletions

File tree

web/config.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1019,7 +1019,7 @@
10191019
# Docker Desktop 4.40+ includes a built-in model runner with an
10201020
# OpenAI-compatible API. No API key is required.
10211021
# URL for the Docker Model Runner API endpoint. Leave empty to disable.
1022-
# Default value: http://localhost:12434
1022+
# Typical value: http://localhost:12434
10231023
DOCKER_API_URL = ''
10241024

10251025
# The Docker Model Runner model to use for AI features.

web/pgadmin/llm/__init__.py

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@
2828
except ImportError:
2929
SSL_CONTEXT = ssl.create_default_context()
3030

31+
# Enforce minimum TLS 1.2 to satisfy security requirements
32+
SSL_CONTEXT.minimum_version = ssl.TLSVersion.TLSv1_2
33+
3134

3235
MODULE_NAME = 'llm'
3336

@@ -606,8 +609,8 @@ def _fetch_anthropic_models(api_key):
606609
data = json.loads(response.read().decode('utf-8'))
607610
except urllib.error.HTTPError as e:
608611
if e.code == 401:
609-
raise Exception('Invalid API key')
610-
raise Exception(f'API error: {e.code}')
612+
raise ValueError('Invalid API key')
613+
raise ConnectionError(f'API error: {e.code}')
611614

612615
models = []
613616
seen = set()
@@ -661,8 +664,8 @@ def _fetch_openai_models(api_key):
661664
data = json.loads(response.read().decode('utf-8'))
662665
except urllib.error.HTTPError as e:
663666
if e.code == 401:
664-
raise Exception('Invalid API key')
665-
raise Exception(f'API error: {e.code}')
667+
raise ValueError('Invalid API key')
668+
raise ConnectionError(f'API error: {e.code}')
666669

667670
models = []
668671
seen = set()
@@ -706,9 +709,13 @@ def _fetch_ollama_models(api_url):
706709
) as response:
707710
data = json.loads(response.read().decode('utf-8'))
708711
except urllib.error.URLError as e:
709-
raise Exception(f'Cannot connect to Ollama: {e.reason}')
710-
except Exception as e:
711-
raise Exception(f'Error fetching models: {str(e)}')
712+
raise ConnectionError(
713+
f'Cannot connect to Ollama: {e.reason}'
714+
)
715+
except OSError as e:
716+
raise ConnectionError(
717+
f'Error fetching models: {str(e)}'
718+
)
712719

713720
models = []
714721
for model in data.get('models', []):
@@ -755,12 +762,15 @@ def _fetch_docker_models(api_url):
755762
) as response:
756763
data = json.loads(response.read().decode('utf-8'))
757764
except urllib.error.URLError as e:
758-
raise Exception(
759-
f'Cannot connect to Docker Model Runner: {e.reason}. '
760-
f'Is Docker Desktop running with model runner enabled?'
765+
raise ConnectionError(
766+
f'Cannot connect to Docker Model Runner: '
767+
f'{e.reason}. Is Docker Desktop running '
768+
f'with model runner enabled?'
769+
)
770+
except OSError as e:
771+
raise ConnectionError(
772+
f'Error fetching models: {str(e)}'
761773
)
762-
except Exception as e:
763-
raise Exception(f'Error fetching models: {str(e)}')
764774

765775
models = []
766776
seen = set()
@@ -1031,7 +1041,7 @@ def _gather_security_config(conn, manager):
10311041
return security_info
10321042

10331043

1034-
def _generate_security_report_llm(client, security_info, manager):
1044+
def _generate_security_report_llm(client, security_info):
10351045
"""
10361046
Use the LLM to analyze the security configuration and generate a report.
10371047
"""
@@ -1046,11 +1056,12 @@ def _generate_security_report_llm(client, security_info, manager):
10461056
"objects or data.\n\n"
10471057
"IMPORTANT: Do NOT include a report title, header block, or "
10481058
"generation date at the top of your response. The title and "
1049-
"metadata are added separately by the application. "
1050-
"Start directly with the Executive Summary section.\n\n"
1059+
"metadata are added separately by the application. Start "
1060+
"directly with the Executive Summary section.\n\n"
10511061
"The report should include:\n"
10521062
"1. **Executive Summary** - Brief overview of the security posture\n"
1053-
"2. **Critical Issues** - Vulnerabilities needing immediate attention\n"
1063+
"2. **Critical Issues** - Vulnerabilities needing "
1064+
"immediate attention\n"
10541065
"3. **Warnings** - Important security concerns to be addressed\n"
10551066
"4. **Recommendations** - Best practices to improve security\n"
10561067
"5. **Configuration Review** - Analysis of key security settings\n\n"

web/pgadmin/llm/chat.py

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,8 @@ def chat_with_database(
6565
did: Database ID for database connection
6666
conversation_history: Optional list of previous messages
6767
system_prompt: Optional custom system prompt (uses default if None)
68-
max_tool_iterations: Maximum number of tool call rounds (uses preference)
68+
max_tool_iterations: Maximum number of tool call
69+
rounds. Uses preference setting if None.
6970
provider: Optional LLM provider override
7071
model: Optional model override
7172

web/pgadmin/llm/providers/anthropic.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,9 @@
2323
except ImportError:
2424
SSL_CONTEXT = ssl.create_default_context()
2525

26+
# Enforce minimum TLS 1.2 to satisfy security requirements
27+
SSL_CONTEXT.minimum_version = ssl.TLSVersion.TLSv1_2
28+
2629
from pgadmin.llm.client import LLMClient, LLMClientError
2730
from pgadmin.llm.models import (
2831
Message, Tool, ToolCall, LLMResponse, LLMError,

web/pgadmin/llm/providers/docker.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,9 @@
2828
except ImportError:
2929
SSL_CONTEXT = ssl.create_default_context()
3030

31+
# Enforce minimum TLS 1.2 to satisfy security requirements
32+
SSL_CONTEXT.minimum_version = ssl.TLSVersion.TLSv1_2
33+
3134
from pgadmin.llm.client import LLMClient, LLMClientError
3235
from pgadmin.llm.models import (
3336
Message, Tool, ToolCall, LLMResponse, LLMError,

web/pgadmin/llm/providers/openai.py

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@
2424
except ImportError:
2525
SSL_CONTEXT = ssl.create_default_context()
2626

27+
# Enforce minimum TLS 1.2 to satisfy security requirements
28+
SSL_CONTEXT.minimum_version = ssl.TLSVersion.TLSv1_2
29+
2730
from pgadmin.llm.client import LLMClient, LLMClientError
2831
from pgadmin.llm.models import (
2932
Message, Tool, ToolCall, LLMResponse, LLMError,

web/pgadmin/llm/static/js/ai_tools.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ define([
167167
const api = getApiInstance();
168168
api.get(url_for('llm.status'))
169169
.then((res) => {
170-
if (res.data && res.data.success) {
170+
if (res?.data?.success) {
171171
this.llmEnabled = res.data.data?.enabled || false;
172172
this.llmSystemEnabled = res.data.data?.system_enabled || false;
173173
}

web/pgadmin/llm/tests/test_llm_status.py

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,16 @@ def runTest(self):
5252
self.provider_enabled and hasattr(self, 'provider_name')
5353
) else None
5454

55-
with patch('pgadmin.llm.utils.is_llm_enabled') as mock_enabled, \
56-
patch('pgadmin.llm.utils.is_llm_enabled_system') as mock_system, \
57-
patch('pgadmin.llm.utils.get_default_provider') as mock_provider, \
58-
patch('pgadmin.authenticate.mfa.utils.mfa_required', lambda f: f):
55+
with patch(
56+
'pgadmin.llm.utils.is_llm_enabled'
57+
) as mock_enabled, patch(
58+
'pgadmin.llm.utils.is_llm_enabled_system'
59+
) as mock_system, patch(
60+
'pgadmin.llm.utils.get_default_provider'
61+
) as mock_provider, patch(
62+
'pgadmin.authenticate.mfa.utils.mfa_required',
63+
lambda f: f
64+
):
5965

6066
mock_enabled.return_value = self.expected_enabled
6167
mock_system.return_value = self.provider_enabled

web/pgadmin/llm/tools/database.py

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -142,16 +142,8 @@ def _execute_readonly_query(conn, query: str) -> dict:
142142
Raises:
143143
DatabaseToolError: If query execution fails
144144
"""
145-
# Wrap the query in a read-only transaction
146-
# This ensures even if the query tries to modify data, it will fail
147-
readonly_wrapper = """
148-
BEGIN TRANSACTION READ ONLY;
149-
{query}
150-
ROLLBACK;
151-
"""
152-
153-
# For SELECT queries, we need to handle them differently
154-
# We'll set the transaction to read-only, execute, then rollback
145+
# Set the transaction to read-only, execute, then rollback.
146+
# This ensures even if the query tries to modify data, it will fail.
155147
try:
156148
# First, set the transaction to read-only mode
157149
status, result = conn.execute_void(
@@ -237,7 +229,8 @@ def execute_readonly_query(
237229
- truncated: True if results were limited
238230
239231
Raises:
240-
DatabaseToolError: If the query fails or connection cannot be established
232+
DatabaseToolError: If the query fails or connection
233+
cannot be established
241234
"""
242235
# Generate unique connection ID for this LLM query
243236
conn_id = f"llm_{secrets.choice(range(1, 9999999))}"
@@ -364,10 +357,13 @@ def get_database_schema(sid: int, did: int) -> dict:
364357
'description': row.get('description')
365358
})
366359

367-
# Get views for this schema (relkind v=view, m=materialized view)
360+
# Get views (relkind v=view, m=materialized view)
368361
views_sql = f"""
369-
SELECT c.oid, c.relname AS name,
370-
pg_catalog.obj_description(c.oid, 'pg_class') AS description
362+
SELECT c.oid,
363+
c.relname AS name,
364+
pg_catalog.obj_description(
365+
c.oid, 'pg_class'
366+
) AS description
371367
FROM pg_catalog.pg_class c
372368
WHERE c.relkind IN ('v', 'm')
373369
AND c.relnamespace = {schema_oid}::oid
@@ -486,7 +482,10 @@ def get_table_columns(
486482
for row in cols_res.get('rows', []):
487483
columns.append({
488484
'name': row.get('name'),
489-
'data_type': row.get('displaytypname') or row.get('datatype'),
485+
'data_type': (
486+
row.get('displaytypname')
487+
or row.get('datatype')
488+
),
490489
'not_null': row.get('not_null', False),
491490
'has_default': row.get('has_default_val', False),
492491
'description': row.get('description')
@@ -591,7 +590,10 @@ def get_table_info(
591590
for row in cols_res.get('rows', []):
592591
columns.append({
593592
'name': row.get('name'),
594-
'data_type': row.get('displaytypname') or row.get('datatype'),
593+
'data_type': (
594+
row.get('displaytypname')
595+
or row.get('datatype')
596+
),
595597
'not_null': row.get('not_null', False),
596598
'has_default': row.get('has_default_val', False),
597599
'description': row.get('description')
@@ -608,7 +610,9 @@ def get_table_info(
608610
WHEN 'c' THEN 'CHECK'
609611
WHEN 'x' THEN 'EXCLUSION'
610612
END AS type,
611-
pg_catalog.pg_get_constraintdef(con.oid, true) AS definition
613+
pg_catalog.pg_get_constraintdef(
614+
con.oid, true
615+
) AS definition
612616
FROM pg_catalog.pg_constraint con
613617
WHERE con.conrelid = {table_oid}::oid
614618
ORDER BY con.contype, con.conname
@@ -740,8 +744,10 @@ def execute_tool(
740744
"query": {
741745
"type": "string",
742746
"description": (
743-
"The SQL query to execute. Should be a SELECT query "
744-
"or other read-only statement. DML statements will fail."
747+
"The SQL query to execute. Should "
748+
"be a SELECT query or other "
749+
"read-only statement. DML "
750+
"statements will fail."
745751
)
746752
}
747753
},
@@ -751,8 +757,9 @@ def execute_tool(
751757
Tool(
752758
name="get_database_schema",
753759
description=(
754-
"Get a list of all schemas, tables, and views in the database. "
755-
"Use this to understand the database structure before writing queries."
760+
"Get a list of all schemas, tables, and views "
761+
"in the database. Use this to understand the "
762+
"database structure before writing queries."
756763
),
757764
parameters={
758765
"type": "object",

0 commit comments

Comments
 (0)