FIX: Redact parameter values from executemany Decimal-conversion error - #719
Open
Sumit Sarabhai (sumitmsft) wants to merge 3 commits into
Open
FIX: Redact parameter values from executemany Decimal-conversion error#719Sumit Sarabhai (sumitmsft) wants to merge 3 commits into
Sumit Sarabhai (sumitmsft) wants to merge 3 commits into
Conversation
…r (AB#47300) The executemany() Decimal/NUMERIC conversion path embedded the full parameter row into the raised ValueError, so every column value in a failing row (potentially PII such as SSNs, emails, or balances) leaked into caller error handlers, tracebacks, and log/APM stores even without DEBUG logging enabled. Report metadata only (row index, column index, value type). The original decimal error is preserved via exception chaining. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Sumit Sarabhai (sumitmsft)
August 13, 2026 17:47
View session
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the executemany() DECIMAL/NUMERIC conversion error path to avoid leaking full parameter rows (potentially containing PII) into exception messages, while preserving debugging detail via exception chaining.
Changes:
- Redacts parameter values/rows from the
ValueErrorraised during DECIMAL conversion inCursor.executemany(), replacing them with row/column index + value type metadata. - Strengthens the integration test to assert that sensitive values and raw parameter-row representations are not present in the raised error message.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
mssql_python/cursor.py |
Changes DECIMAL/NUMERIC conversion failure message to include only row/column indices and value type (no value/row), using exception chaining. |
tests/test_004_cursor.py |
Adds assertions that the error message includes metadata and does not include the sensitive value / raw row. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…or (AB#47300) The prior redaction cleaned the outer ValueError message but chained the raw exception via 'from e'. For a parameter whose str() raises with content (or any exception echoing the input), that cause surfaces through __cause__ and traceback.format_exc(), which APM/log shippers capture -- defeating the metadata-only guarantee the threat model requires. Now only decimal.DecimalException (proven value-free, e.g. ConversionSyntax) is chained for debuggability; str(val) failures and other unexpected errors are re-raised with the chain suppressed (from None). Adds a test asserting the formatted traceback leaks neither the value nor a str()-raised secret. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
📊 Code Coverage Report
Diff CoverageDiff: main...HEAD, staged and unstaged changes
Summary
mssql_python/cursor.pyLines 2692-2701 2692 try:
2693 processed_row[i] = format(decimal.Decimal(val_text), "f")
2694 except decimal.DecimalException as e:
2695 raise ValueError(err_msg) from e
! 2696 except Exception: # pylint: disable=broad-exception-caught
! 2697 raise ValueError(err_msg) from None
2698 processed_parameters.append(processed_row)
2699
2700 # Now transpose the processed parameters
2701 columnwise_params, row_count = self._transpose_rowwise_to_columnwise(processed_parameters)📋 Files Needing Attention📉 Files with overall lowest coverage (click to expand)mssql_python.pybind.logger_bridge.cpp: 59.2%
mssql_python.pybind.ddbc_bindings.h: 59.9%
mssql_python.pybind.logger_bridge.hpp: 70.8%
mssql_python.pybind.ddbc_bindings.cpp: 76.6%
mssql_python.__init__.py: 77.6%
mssql_python.row.py: 77.6%
mssql_python.ddbc_bindings.py: 79.6%
mssql_python.pybind.connection.connection_pool.cpp: 81.4%
mssql_python.pybind.connection.connection.cpp: 84.3%
mssql_python.logging.py: 85.5%🔗 Quick Links
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Work Item / Issue Reference
Summary
The
executemany()Decimal/NUMERIC conversion path embedded the entire parameter row into the raisedValueError:Here
rowis 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
After
Hardening: value-bearing exception cause
Redacting only the outer message is not enough.
raise ... from echains the original exception, which surfaces through__cause__andtraceback.format_exc()-- exactly what APM/log shippers capture. For an ordinary bad string the chained cause is the value-freedecimal.InvalidOperation(ConversionSyntax), but for a value whosestr()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 onlydecimal.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 juststr(exc).Changes
mssql_python/cursor.py: enumerate rows for an index; raise a value-free, metadata-onlyValueError; chain only the value-freedecimal.DecimalExceptionand suppress the cause (from None) forstr(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; addtest_setinputsizes_sql_decimal_str_raises_no_leakcovering a parameter whosestr()raises a secret (asserts__cause__ is Noneand the secret is absent from the traceback).