Skip to content

Commit 8a9cdcb

Browse files
sumitmsftCopilotCopilotbewithgaurav
authored
FIX: Redact parameter values from executemany Decimal-conversion error (#719)
### Work Item / Issue Reference > [AB#47300](https://sqlclientdrivers.visualstudio.com/c6d89619-62de-46a0-8b46-70b92a84d85e/_workitems/edit/47300) ------------------------------------------------------------------- ### Summary The `executemany()` Decimal/NUMERIC conversion path embedded the **entire parameter row** into the raised `ValueError`: ``` ValueError(f"Failed to convert parameter at row {row}, column {i} to Decimal: {e}") ``` Here `row` is the full parameter tuple, so every column value in a failing row (potentially PII such as SSNs, emails, names, or balances) was placed into the exception message. That message propagates to caller error handlers, tracebacks, and APM/log shippers (Sentry, Splunk, App Insights) **even when driver DEBUG logging is never enabled**, because uncaught exceptions surface by default. This change makes the error **metadata-only**: it reports the row index, column index, and the value's type name, and never the value or the row. **Before** ``` Failed to convert parameter at row ('Jane Doe', '123-45-6789', 'jane@x.com', 'N/A'), column 3 to Decimal: [<class 'decimal.ConversionSyntax'>] ``` **After** ``` Failed to convert parameter to Decimal at row 0, column 3 (value type: str) ``` #### Hardening: value-bearing exception cause Redacting only the outer message is not enough. `raise ... from e` chains the original exception, which surfaces through `__cause__` and `traceback.format_exc()` -- exactly what APM/log shippers capture. For an ordinary bad string the chained cause is the value-free `decimal.InvalidOperation` (`ConversionSyntax`), but for a value whose `str()` raises with content (or any error echoing the input) the sensitive text would still leak via the cause. The conversion now splits `str(val)` from the decimal parse and chains **only** `decimal.DecimalException`, which is proven value-free (it never echoes the input). `str(val)` failures and any other unexpected error are re-raised with the chain suppressed (`from None`), so the metadata-only guarantee holds across `__cause__` and formatted tracebacks, not just `str(exc)`. #### Changes - `mssql_python/cursor.py`: enumerate rows for an index; raise a value-free, metadata-only `ValueError`; chain only the value-free `decimal.DecimalException` and suppress the cause (`from None`) for `str(val)` / unexpected failures. - `tests/test_004_cursor.py`: strengthen the unconvertible-value test to assert the sensitive value and raw row are absent from both the message and the fully formatted traceback; add `test_setinputsizes_sql_decimal_str_raises_no_leak` covering a parameter whose `str()` raises a secret (asserts `__cause__ is None` and the secret is absent from the traceback). --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: Gaurav Sharma <sharmag@microsoft.com>
1 parent 8e1f8fe commit 8a9cdcb

2 files changed

Lines changed: 211 additions & 15 deletions

File tree

‎mssql_python/cursor.py‎

Lines changed: 33 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2693,7 +2693,7 @@ def executemany( # pylint: disable=too-many-locals,too-many-branches,too-many-s
26932693
# Process parameters into column-wise format with possible type conversions
26942694
# First, convert any Decimal types as needed for NUMERIC/DECIMAL columns
26952695
processed_parameters = []
2696-
for row in seq_of_parameters:
2696+
for row_index, row in enumerate(seq_of_parameters):
26972697
processed_row = list(row)
26982698
for i, val in enumerate(processed_row):
26992699
if val is None:
@@ -2715,12 +2715,32 @@ def executemany( # pylint: disable=too-many-locals,too-many-branches,too-many-s
27152715
if isinstance(val, decimal.Decimal):
27162716
processed_row[i] = format(val, "f")
27172717
else:
2718+
# Do not embed the parameter value or the full row in the
2719+
# message: rows may contain PII (SSNs, emails, balances)
2720+
# that would leak into caller error handlers, tracebacks,
2721+
# and log/APM stores. Report metadata only (row index,
2722+
# column index, value type).
2723+
err_msg = (
2724+
f"Failed to convert parameter to Decimal at row "
2725+
f"{row_index}, column {i} (value type: {type(val).__name__})"
2726+
)
2727+
# Split str(val) from the decimal parse so we only chain a
2728+
# cause we know is value-free. decimal.DecimalException
2729+
# messages (e.g. ConversionSyntax) never echo the input, so
2730+
# they are safe to preserve for debugging. str(val) itself
2731+
# or any other error could carry the value in its message
2732+
# and surface through __cause__ / formatted tracebacks, so
2733+
# those are re-raised with the chain suppressed (from None).
2734+
try:
2735+
val_text = str(val)
2736+
except Exception: # pylint: disable=broad-exception-caught
2737+
raise ValueError(err_msg) from None
27182738
try:
2719-
processed_row[i] = format(decimal.Decimal(str(val)), "f")
2720-
except Exception as e: # pylint: disable=broad-exception-caught
2721-
raise ValueError(
2722-
f"Failed to convert parameter at row {row}, column {i} to Decimal: {e}"
2723-
) from e
2739+
processed_row[i] = format(decimal.Decimal(val_text), "f")
2740+
except decimal.DecimalException as e:
2741+
raise ValueError(err_msg) from e
2742+
except Exception: # pylint: disable=broad-exception-caught
2743+
raise ValueError(err_msg) from None
27242744
processed_parameters.append(processed_row)
27252745

27262746
# Now transpose the processed parameters
@@ -2729,14 +2749,15 @@ def executemany( # pylint: disable=too-many-locals,too-many-branches,too-many-s
27292749
# Get encoding settings
27302750
encoding_settings = self._get_encoding_settings()
27312751

2732-
# Add debug logging
2752+
# Debug logging: emit batch metadata only. Never log parameter values or
2753+
# row representations here -- rows may contain PII (SSNs, emails,
2754+
# balances) that would leak into log files and APM/log shippers even
2755+
# though this is a DEBUG-level statement. Metadata (batch size, column
2756+
# count) is sufficient for diagnostics without exposing user data.
27332757
logger.debug(
2734-
"Executing batch query with %d parameter sets:\n%s",
2758+
"Executing batch query with %d parameter sets (%d columns per row)",
27352759
len(seq_of_parameters),
2736-
"\n".join(
2737-
f" {i+1}: {tuple(p) if isinstance(p, (list, tuple)) else p}"
2738-
for i, p in enumerate(seq_of_parameters[:5])
2739-
), # Limit to first 5 rows for large batches
2760+
len(parameters_type),
27402761
)
27412762

27422763
ret = ddbc_bindings.SQLExecuteMany(

‎tests/test_004_cursor.py‎

Lines changed: 178 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
from pathlib import Path
1616
import time as time_module
1717
import decimal
18+
import traceback
1819
from contextlib import closing
1920
import threading
2021
import mssql_python
@@ -10462,7 +10463,12 @@ def test_setinputsizes_sql_decimal_null(db_connection):
1046210463

1046310464

1046410465
def test_setinputsizes_sql_decimal_unconvertible_value(db_connection):
10465-
"""Test setinputsizes with SQL_DECIMAL raises ValueError for unconvertible values (GH-503)."""
10466+
"""Test setinputsizes with SQL_DECIMAL raises ValueError for unconvertible values (GH-503).
10467+
10468+
The raised message must be metadata-only: it reports the row index, column
10469+
index, and value type, but must NOT embed the offending value or the full
10470+
parameter row (which may contain PII such as SSNs/emails/balances).
10471+
"""
1046610472
cursor = db_connection.cursor()
1046710473

1046810474
cursor.execute("DROP TABLE IF EXISTS #test_sis_dec_bad")
@@ -10471,15 +10477,184 @@ def test_setinputsizes_sql_decimal_unconvertible_value(db_connection):
1047110477

1047210478
cursor.setinputsizes([(mssql_python.SQL_DECIMAL, 18, 2)])
1047310479

10474-
with pytest.raises(ValueError, match="Failed to convert parameter"):
10480+
sensitive_value = "123-45-6789" # stand-in for PII in the failing row
10481+
with pytest.raises(ValueError) as exc_info:
1047510482
cursor.executemany(
1047610483
"INSERT INTO #test_sis_dec_bad (Price) VALUES (?)",
10477-
[("not_a_number",)],
10484+
[(sensitive_value,)],
10485+
)
10486+
10487+
message = str(exc_info.value)
10488+
# Contract: metadata is present...
10489+
assert "Failed to convert parameter" in message
10490+
assert "row 0" in message
10491+
assert "column 0" in message
10492+
assert "str" in message # value type name
10493+
# ...and the sensitive value / raw row is NOT leaked into the message.
10494+
assert sensitive_value not in message
10495+
assert repr((sensitive_value,)) not in message # no repr of the parameter tuple
10496+
# ...nor into the chained cause or the fully formatted traceback, which
10497+
# is what tracebacks and APM/log shippers actually capture.
10498+
formatted = "".join(
10499+
traceback.format_exception(
10500+
type(exc_info.value), exc_info.value, exc_info.value.__traceback__
1047810501
)
10502+
)
10503+
assert sensitive_value not in formatted
1047910504
finally:
1048010505
cursor.execute("DROP TABLE IF EXISTS #test_sis_dec_bad")
1048110506

1048210507

10508+
def test_setinputsizes_sql_decimal_str_raises_no_leak(db_connection):
10509+
"""A parameter whose str() raises must not leak the exception text (GH-503).
10510+
10511+
Exception chaining (raise ... from e) can surface a value-bearing cause
10512+
through __cause__ and formatted tracebacks. For a value whose str() raises,
10513+
the chain must be suppressed so the metadata-only guarantee holds across
10514+
tracebacks and APM/log shippers, not just str(exc).
10515+
"""
10516+
cursor = db_connection.cursor()
10517+
10518+
secret = "secret-987-65-4321"
10519+
10520+
class ExplodingStr:
10521+
def __str__(self):
10522+
raise ValueError(secret)
10523+
10524+
cursor.execute("DROP TABLE IF EXISTS #test_sis_dec_explode")
10525+
try:
10526+
cursor.execute("CREATE TABLE #test_sis_dec_explode (Price DECIMAL(18,2))")
10527+
10528+
cursor.setinputsizes([(mssql_python.SQL_DECIMAL, 18, 2)])
10529+
10530+
with pytest.raises(ValueError) as exc_info:
10531+
cursor.executemany(
10532+
"INSERT INTO #test_sis_dec_explode (Price) VALUES (?)",
10533+
[(ExplodingStr(),)],
10534+
)
10535+
10536+
# The metadata-only message must not carry the secret, and the chain
10537+
# must be suppressed so neither __cause__ nor the formatted traceback
10538+
# exposes it.
10539+
assert secret not in str(exc_info.value)
10540+
assert exc_info.value.__cause__ is None
10541+
formatted = "".join(
10542+
traceback.format_exception(
10543+
type(exc_info.value), exc_info.value, exc_info.value.__traceback__
10544+
)
10545+
)
10546+
assert secret not in formatted
10547+
finally:
10548+
cursor.execute("DROP TABLE IF EXISTS #test_sis_dec_explode")
10549+
10550+
10551+
def test_setinputsizes_sql_decimal_non_decimal_exception_no_leak(db_connection):
10552+
"""Cover the non-DecimalException conversion branch with no value leak (GH-503).
10553+
10554+
``format(decimal.Decimal("1e999999999999999999"), "f")`` raises MemoryError
10555+
(not a decimal.DecimalException) quickly and deterministically, exercising
10556+
the branch that re-raises with the chain suppressed. The resulting
10557+
ValueError must be metadata-only: no chained cause, and the offending input
10558+
must be absent from both the message and the fully formatted traceback.
10559+
"""
10560+
cursor = db_connection.cursor()
10561+
10562+
# A syntactically valid Decimal whose fixed-point expansion is astronomically
10563+
# large; format(..., "f") raises MemoryError rather than a DecimalException.
10564+
sensitive_value = "1e999999999999999999"
10565+
10566+
cursor.execute("DROP TABLE IF EXISTS #test_sis_dec_mem")
10567+
try:
10568+
cursor.execute("CREATE TABLE #test_sis_dec_mem (Price DECIMAL(18,2))")
10569+
10570+
cursor.setinputsizes([(mssql_python.SQL_DECIMAL, 18, 2)])
10571+
10572+
with pytest.raises(ValueError) as exc_info:
10573+
cursor.executemany(
10574+
"INSERT INTO #test_sis_dec_mem (Price) VALUES (?)",
10575+
[(sensitive_value,)],
10576+
)
10577+
10578+
message = str(exc_info.value)
10579+
# Metadata-only message...
10580+
assert "Failed to convert parameter" in message
10581+
assert "row 0" in message
10582+
assert "column 0" in message
10583+
# ...no chained cause (the non-DecimalException branch suppresses it)...
10584+
assert exc_info.value.__cause__ is None
10585+
# ...and the input is absent from the message and formatted traceback.
10586+
assert sensitive_value not in message
10587+
formatted = "".join(
10588+
traceback.format_exception(
10589+
type(exc_info.value), exc_info.value, exc_info.value.__traceback__
10590+
)
10591+
)
10592+
assert sensitive_value not in formatted
10593+
finally:
10594+
cursor.execute("DROP TABLE IF EXISTS #test_sis_dec_mem")
10595+
10596+
10597+
def test_executemany_debug_log_no_parameter_values(db_connection):
10598+
"""executemany() DEBUG logging must not emit parameter values or rows (GH-503).
10599+
10600+
The batch-execution debug log previously dumped the first 5 full parameter
10601+
rows, leaking the same PII the exception path now redacts. This test enables
10602+
DEBUG capture, runs a successful batch of sensitive-looking values, and
10603+
asserts the values are absent from the logs while batch metadata is present
10604+
(the metadata assertion is a positive control proving capture is working, so
10605+
the absence assertions are meaningful rather than vacuous).
10606+
"""
10607+
import logging as _logging
10608+
import io
10609+
from mssql_python.logging import logger, driver_logger
10610+
10611+
cursor = db_connection.cursor()
10612+
10613+
# Values that stand in for PII; both insert successfully into an NVARCHAR
10614+
# column so execution reaches the batch debug-log statement.
10615+
ssn = "123-45-6789"
10616+
email = "jane.doe@example.com"
10617+
10618+
log_stream = io.StringIO()
10619+
test_handler = _logging.StreamHandler(log_stream)
10620+
test_handler.setLevel(_logging.DEBUG)
10621+
10622+
# Save state we mutate so the global logger is restored afterwards.
10623+
original_cached_level = logger._cached_level
10624+
original_driver_level = driver_logger.level
10625+
10626+
cursor.execute("DROP TABLE IF EXISTS #test_dbg_no_pii")
10627+
try:
10628+
cursor.execute("CREATE TABLE #test_dbg_no_pii (Data NVARCHAR(50))")
10629+
10630+
# Enable DEBUG: bypass the wrapper's cached-level gate and lower the
10631+
# underlying stdlib logger, then attach our capturing handler.
10632+
logger._cached_level = _logging.DEBUG
10633+
driver_logger.setLevel(_logging.DEBUG)
10634+
driver_logger.addHandler(test_handler)
10635+
10636+
cursor.executemany(
10637+
"INSERT INTO #test_dbg_no_pii (Data) VALUES (?)",
10638+
[(ssn,), (email,)],
10639+
)
10640+
10641+
test_handler.flush()
10642+
log_contents = log_stream.getvalue()
10643+
10644+
# Positive control: batch metadata is logged (proves capture works).
10645+
assert "Executing batch query with 2 parameter sets" in log_contents
10646+
# Redaction: no parameter value or row representation is emitted.
10647+
assert ssn not in log_contents
10648+
assert email not in log_contents
10649+
assert repr((ssn,)) not in log_contents
10650+
assert repr((email,)) not in log_contents
10651+
finally:
10652+
driver_logger.removeHandler(test_handler)
10653+
driver_logger.setLevel(original_driver_level)
10654+
logger._cached_level = original_cached_level
10655+
cursor.execute("DROP TABLE IF EXISTS #test_dbg_no_pii")
10656+
10657+
1048310658
def test_setinputsizes_sql_decimal_high_precision(db_connection):
1048410659
"""Test setinputsizes with SQL_DECIMAL preserves full DECIMAL(38,18) precision (GH-503)."""
1048510660
cursor = db_connection.cursor()

0 commit comments

Comments
 (0)