Skip to content

Commit b4aad39

Browse files
committed
Resolving comments
1 parent 9d8f37f commit b4aad39

File tree

6 files changed

+165
-253
lines changed

6 files changed

+165
-253
lines changed

mssql_python/__init__.py

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -53,37 +53,17 @@
5353
SQL_WMETADATA = -99
5454

5555
# Export connection attribute constants for set_attr()
56-
# NOTE: Some attributes are only supported when using an ODBC Driver Manager.
57-
# Attributes marked with [NO-OP] are not supported directly by the SQL Server ODBC driver
58-
# and will have no effect in this implementation.
56+
# Only include driver-level attributes that the SQL Server ODBC driver can handle directly
5957

58+
# Core driver-level attributes
6059
SQL_ATTR_ACCESS_MODE = ConstantsDDBC.SQL_ATTR_ACCESS_MODE.value
6160
SQL_ATTR_AUTOCOMMIT = ConstantsDDBC.SQL_ATTR_AUTOCOMMIT.value
6261
SQL_ATTR_CONNECTION_TIMEOUT = ConstantsDDBC.SQL_ATTR_CONNECTION_TIMEOUT.value
6362
SQL_ATTR_CURRENT_CATALOG = ConstantsDDBC.SQL_ATTR_CURRENT_CATALOG.value
6463
SQL_ATTR_LOGIN_TIMEOUT = ConstantsDDBC.SQL_ATTR_LOGIN_TIMEOUT.value
65-
SQL_ATTR_ODBC_CURSORS = ConstantsDDBC.SQL_ATTR_ODBC_CURSORS.value
6664
SQL_ATTR_PACKET_SIZE = ConstantsDDBC.SQL_ATTR_PACKET_SIZE.value
6765
SQL_ATTR_TXN_ISOLATION = ConstantsDDBC.SQL_ATTR_TXN_ISOLATION.value
6866

69-
# The following attributes are [NO-OP] in this implementation (require Driver Manager):
70-
# SQL_ATTR_QUIET_MODE
71-
# SQL_ATTR_TRACE
72-
# SQL_ATTR_TRACEFILE
73-
# SQL_ATTR_TRANSLATE_LIB
74-
# SQL_ATTR_TRANSLATE_OPTION
75-
# SQL_ATTR_CONNECTION_POOLING
76-
# SQL_ATTR_CP_MATCH
77-
# SQL_ATTR_ASYNC_ENABLE
78-
# SQL_ATTR_ENLIST_IN_DTC
79-
# SQL_ATTR_ENLIST_IN_XA
80-
# SQL_ATTR_CONNECTION_DEAD
81-
# SQL_ATTR_ASYNC_DBC_FUNCTIONS_ENABLE
82-
# SQL_ATTR_ASYNC_DBC_EVENT
83-
# SQL_ATTR_SERVER_NAME
84-
# SQL_ATTR_RESET_CONNECTION
85-
# SQL_RESET_CONNECTION_YES
86-
8767
# Transaction Isolation Level Constants
8868
SQL_TXN_READ_UNCOMMITTED = ConstantsDDBC.SQL_TXN_READ_UNCOMMITTED.value
8969
SQL_TXN_READ_COMMITTED = ConstantsDDBC.SQL_TXN_READ_COMMITTED.value
@@ -94,11 +74,6 @@
9474
SQL_MODE_READ_WRITE = ConstantsDDBC.SQL_MODE_READ_WRITE.value
9575
SQL_MODE_READ_ONLY = ConstantsDDBC.SQL_MODE_READ_ONLY.value
9676

97-
# ODBC Cursors Constants
98-
SQL_CUR_USE_IF_NEEDED = ConstantsDDBC.SQL_CUR_USE_IF_NEEDED.value
99-
SQL_CUR_USE_ODBC = ConstantsDDBC.SQL_CUR_USE_ODBC.value
100-
SQL_CUR_USE_DRIVER = ConstantsDDBC.SQL_CUR_USE_DRIVER.value
101-
10277

10378
# GLOBALS
10479
# Read-Only

mssql_python/connection.py

Lines changed: 18 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,7 @@
1515
import codecs
1616
from functools import lru_cache
1717
from mssql_python.cursor import Cursor
18-
from mssql_python.helpers import add_driver_to_connection_str, sanitize_connection_string, sanitize_user_input, log
18+
from mssql_python.helpers import add_driver_to_connection_str, sanitize_connection_string, log, validate_attribute_value, sanitize_user_input
1919
from mssql_python import ddbc_bindings
2020
from mssql_python.pooling import PoolingManager
2121
from mssql_python.exceptions import InterfaceError, ProgrammingError
@@ -534,8 +534,8 @@ def set_attr(self, attribute, value):
534534
attribute (int): The connection attribute to set. Should be one of the
535535
SQL_ATTR_* constants (e.g., SQL_ATTR_AUTOCOMMIT,
536536
SQL_ATTR_TXN_ISOLATION).
537-
value: The value to set for the attribute. Can be an integer or bytes/bytearray
538-
depending on the attribute type.
537+
value: The value to set for the attribute. Can be an integer, string,
538+
bytes, or bytearray depending on the attribute type.
539539
540540
Raises:
541541
InterfaceError: If the connection is closed or attribute is invalid.
@@ -548,35 +548,27 @@ def set_attr(self, attribute, value):
548548
if self._closed:
549549
raise InterfaceError("Cannot set attribute on closed connection", "Connection is closed")
550550

551-
# Validate attribute type and range
552-
if not isinstance(attribute, int) or attribute < 0:
553-
raise ProgrammingError("Connection attribute must be a non-negative integer", f"Invalid attribute: {attribute}")
554-
555-
# Validate value type - must be integer, bytes, bytearray, or string
556-
if not isinstance(value, (int, bytes, bytearray, str)):
557-
raise ProgrammingError("Attribute value must be an integer, bytes, bytearray, or string",
558-
f"Invalid value type: {type(value)}")
559-
560-
# For integer values
561-
if isinstance(value, int):
562-
if value < 0:
563-
raise ProgrammingError(f"Attribute value must be non-negative", f"Invalid value: {value}")
564-
565-
# Sanitize user input for security
566-
try:
567-
sanitized_input = sanitize_user_input(str(attribute))
568-
log('debug', f"Setting connection attribute: {sanitized_input}")
569-
except Exception:
570-
# If sanitization fails, log without user input
571-
log('debug', "Setting connection attribute")
551+
# Use the integrated validation helper function
552+
is_valid, error_message, sanitized_attr, sanitized_val = validate_attribute_value(attribute, value)
553+
554+
if not is_valid:
555+
# Use the already sanitized values for logging
556+
log('warning', f"Invalid attribute or value: {sanitized_attr}={sanitized_val}, {error_message}")
557+
raise ProgrammingError(
558+
driver_error=f"Invalid attribute or value: {error_message}",
559+
ddbc_error=error_message
560+
)
561+
562+
# Log with sanitized values
563+
log('debug', f"Setting connection attribute: {sanitized_attr}={sanitized_val}")
572564

573565
try:
574566
# Call the underlying C++ method
575567
self._conn.set_attr(attribute, value)
576-
log('info', f"Connection attribute {attribute} set successfully")
568+
log('info', f"Connection attribute {sanitized_attr} set successfully")
577569

578570
except Exception as e:
579-
error_msg = f"Failed to set connection attribute {attribute}: {str(e)}"
571+
error_msg = f"Failed to set connection attribute {sanitized_attr}: {str(e)}"
580572
log('error', error_msg)
581573

582574
# Determine appropriate exception type based on error content

mssql_python/helpers.py

Lines changed: 132 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,8 @@
77
from mssql_python import ddbc_bindings
88
from mssql_python.exceptions import raise_exception
99
from mssql_python.logging_config import get_logger
10-
import platform
11-
from pathlib import Path
10+
import re
11+
from mssql_python.constants import ConstantsDDBC
1212
from mssql_python.ddbc_bindings import normalize_architecture
1313

1414
logger = get_logger()
@@ -155,6 +155,136 @@ def sanitize_user_input(user_input: str, max_length: int = 50) -> str:
155155
# Return placeholder if nothing remains after sanitization
156156
return sanitized if sanitized else "<invalid>"
157157

158+
def validate_attribute_value(attribute, value, sanitize_logs=True, max_log_length=50):
159+
"""
160+
Validates attribute and value pairs for connection attributes and optionally
161+
sanitizes values for safe logging.
162+
163+
This function performs comprehensive validation of ODBC connection attributes
164+
and their values to ensure they are safe and valid before passing to the C++ layer.
165+
166+
Args:
167+
attribute (int): The connection attribute to validate (SQL_ATTR_*)
168+
value: The value to set for the attribute (int, str, bytes, or bytearray)
169+
sanitize_logs (bool): Whether to include sanitized versions for logging
170+
max_log_length (int): Maximum length of sanitized output for logging
171+
172+
Returns:
173+
tuple: (is_valid, error_message, sanitized_attribute, sanitized_value) where:
174+
- is_valid is a boolean
175+
- error_message is None if valid, otherwise validation error message
176+
- sanitized_attribute is attribute as a string safe for logging
177+
- sanitized_value is value as a string safe for logging
178+
179+
Note:
180+
This validation acts as a security layer to prevent SQL injection, buffer
181+
overflows, and other attacks by validating all inputs before they reach C++ code.
182+
"""
183+
184+
# Sanitize a value for logging
185+
def _sanitize_for_logging(input_val, max_length=max_log_length):
186+
if not isinstance(input_val, str):
187+
try:
188+
input_val = str(input_val)
189+
except:
190+
return "<non-string>"
191+
192+
# Remove control characters and non-printable characters
193+
# Allow alphanumeric, dash, underscore, and dot (common in encoding names)
194+
sanitized = re.sub(r'[^\w\-\.]', '', input_val)
195+
196+
# Limit length to prevent log flooding
197+
if len(sanitized) > max_length:
198+
sanitized = sanitized[:max_length] + "..."
199+
200+
# Return placeholder if nothing remains after sanitization
201+
return sanitized if sanitized else "<invalid>"
202+
203+
# Create sanitized versions for logging regardless of validation result
204+
sanitized_attr = _sanitize_for_logging(attribute) if sanitize_logs else str(attribute)
205+
sanitized_val = _sanitize_for_logging(value) if sanitize_logs else str(value)
206+
207+
# Attribute must be a non-negative integer
208+
if not isinstance(attribute, int):
209+
return False, f"Attribute must be an integer, got {type(attribute).__name__}", sanitized_attr, sanitized_val
210+
211+
if attribute < 0:
212+
return False, f"Attribute value cannot be negative: {attribute}", sanitized_attr, sanitized_val
213+
214+
# Define attribute limits based on SQL specifications
215+
MAX_STRING_SIZE = 8192 # 8KB maximum for string values
216+
MAX_BINARY_SIZE = 32768 # 32KB maximum for binary data
217+
218+
# Attribute-specific validation
219+
if isinstance(value, int):
220+
# General integer validation
221+
if value < 0 and attribute not in [
222+
# List of attributes that can accept negative values (very few)
223+
]:
224+
return False, f"Integer value cannot be negative: {value}", sanitized_attr, sanitized_val
225+
226+
# Attribute-specific integer validation
227+
if attribute == ConstantsDDBC.SQL_ATTR_CONNECTION_TIMEOUT.value:
228+
# Connection timeout has a maximum of UINT_MAX (4294967295)
229+
if value > 4294967295:
230+
return False, f"Connection timeout cannot exceed 4294967295: {value}", sanitized_attr, sanitized_val
231+
232+
elif attribute == ConstantsDDBC.SQL_ATTR_LOGIN_TIMEOUT.value:
233+
# Login timeout has a maximum of UINT_MAX (4294967295)
234+
if value > 4294967295:
235+
return False, f"Login timeout cannot exceed 4294967295: {value}", sanitized_attr, sanitized_val
236+
237+
elif attribute == ConstantsDDBC.SQL_ATTR_AUTOCOMMIT.value:
238+
# Autocommit can only be 0 or 1
239+
if value not in [0, 1]:
240+
return False, f"Autocommit value must be 0 or 1: {value}", sanitized_attr, sanitized_val
241+
242+
elif attribute == ConstantsDDBC.SQL_ATTR_TXN_ISOLATION.value:
243+
# Transaction isolation must be one of the predefined values
244+
valid_isolation_levels = [
245+
ConstantsDDBC.SQL_TXN_READ_UNCOMMITTED.value,
246+
ConstantsDDBC.SQL_TXN_READ_COMMITTED.value,
247+
ConstantsDDBC.SQL_TXN_REPEATABLE_READ.value,
248+
ConstantsDDBC.SQL_TXN_SERIALIZABLE.value
249+
]
250+
if value not in valid_isolation_levels:
251+
return False, f"Invalid transaction isolation level: {value}", sanitized_attr, sanitized_val
252+
253+
elif isinstance(value, str):
254+
# String validation
255+
if len(value) > MAX_STRING_SIZE:
256+
return False, f"String value too large: {len(value)} bytes (max {MAX_STRING_SIZE})", sanitized_attr, sanitized_val
257+
258+
# SQL injection pattern detection for strings
259+
sql_injection_patterns = [
260+
'--', ';', '/*', '*/', 'UNION', 'SELECT', 'INSERT', 'UPDATE',
261+
'DELETE', 'DROP', 'EXEC', 'EXECUTE', '@@', 'CHAR(', 'CAST('
262+
]
263+
264+
# Case-insensitive check for SQL injection patterns
265+
value_upper = value.upper()
266+
for pattern in sql_injection_patterns:
267+
if pattern.upper() in value_upper:
268+
return False, f"String value contains potentially unsafe SQL pattern: {pattern}", sanitized_attr, sanitized_val
269+
270+
elif isinstance(value, (bytes, bytearray)):
271+
# Binary data validation
272+
if len(value) > MAX_BINARY_SIZE:
273+
return False, f"Binary value too large: {len(value)} bytes (max {MAX_BINARY_SIZE})", sanitized_attr, sanitized_val
274+
275+
# Check for suspicious binary patterns
276+
# Count null bytes (could indicate manipulation)
277+
null_count = value.count(0)
278+
# Too many nulls might indicate padding attack
279+
if null_count > len(value) // 4: # More than 25% nulls
280+
return False, "Binary data contains suspicious patterns", sanitized_attr, sanitized_val
281+
282+
else:
283+
return False, f"Unsupported attribute value type: {type(value).__name__}", sanitized_attr, sanitized_val
284+
285+
# If we got here, all validations passed
286+
return True, None, sanitized_attr, sanitized_val
287+
158288

159289
def log(level: str, message: str, *args) -> None:
160290
"""

0 commit comments

Comments
 (0)