Skip to content

FIX: Redact parameter values from executemany Decimal-conversion error - #719

Open
Sumit Sarabhai (sumitmsft) wants to merge 3 commits into
mainfrom
sumitmsft-argus-cf-024
Open

FIX: Redact parameter values from executemany Decimal-conversion error#719
Sumit Sarabhai (sumitmsft) wants to merge 3 commits into
mainfrom
sumitmsft-argus-cf-024

Conversation

@sumitmsft

@sumitmsftSumit Sarabhai (sumitmsft) commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

Work Item / Issue Reference

AB#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 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 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).

…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>
CopilotAI lite review requested due to automatic review settings August 13, 2026 17:46
@github-actionsgithub-actionsBot added the pr-size: small Minimal code update label Aug 13, 2026

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ValueError raised during DECIMAL conversion in Cursor.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.

FileDescription
mssql_python/cursor.pyChanges DECIMAL/NUMERIC conversion failure message to include only row/column indices and value type (no value/row), using exception chaining.
tests/test_004_cursor.pyAdds 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.

Comment threadtests/test_004_cursor.py Outdated
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>
@github-actionsgithub-actionsBot added pr-size: medium Moderate update size and removed pr-size: small Minimal code update labels Aug 13, 2026
@github-actions

Copy link
Copy Markdown

📊 Code Coverage Report

🔥 Diff Coverage

81%


🎯 Overall Coverage

82%


📈 Total Lines Covered:7373 out of 8970
📁 Project:mssql-python


Diff Coverage

Diff: main...HEAD, staged and unstaged changes

  • mssql_python/cursor.py (81.8%): Missing lines 2696-2697

Summary

  • Total: 11 lines
  • Missing: 2 lines
  • Coverage: 81%

mssql_python/cursor.py

Lines 2692-2701

2692try:
2693processed_row[i] =format(decimal.Decimal(val_text), "f")
2694exceptdecimal.DecimalExceptionase:
2695raiseValueError(err_msg) frome
! 2696exceptException: # pylint: disable=broad-exception-caught
! 2697raiseValueError(err_msg) fromNone2698processed_parameters.append(processed_row)
26992700# Now transpose the processed parameters2701columnwise_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

⚙️ Build Summary📋 Coverage Details

View Azure DevOps Build

Browse Full Coverage Report

Comment on lines 2706 to 2714
# Add debug logging
logger.debug(
"Executing batch query with %d parameter sets:\n%s",
len(seq_of_parameters),
"\n".join(
f" {i+1}: {tuple(p) if isinstance(p, (list, tuple)) else p}"
for i, p in enumerate(seq_of_parameters[:5])
), # Limit to first 5 rows for large batches
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block also still logs the first 5 full parameter rows ([seq_of_parameters[:5]])- the same PII this PR redacts from the exception. Redact or gate this too

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no blockers as such, added a test suggestion - requesting changes

except decimal.DecimalException as e:
raise ValueError(err_msg) from e
except Exception: # pylint: disable=broad-exception-caught
raise ValueError(err_msg) from None

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

test suggestion: this is an imp branch, and adding tests would be highly beneficial here
the coverage bot flags 2696-2697 as the only miss on the diff

asking since it is reachable cheaply and deterministically.

format(Decimal("1e999999999999999999"), "f") raises MemoryError, not a DecimalException, in 0.1ms. the fixed-point expansion length is rejected up front so nothing is allocated.

suggesting adding the below - will take the diff from 81% to 100%):

deftest_setinputsizes_sql_decimal_non_decimal_error_no_leak(db_connection):
"""A non-DecimalException during conversion must not chain a cause (GH-503)."""cursor=db_connection.cursor()
huge_exponent="1e999999999999999999"cursor.execute("DROP TABLE IF EXISTS #test_sis_dec_nondec")
try:
cursor.execute("CREATE TABLE #test_sis_dec_nondec (Price DECIMAL(18,2))")
cursor.setinputsizes([(mssql_python.SQL_DECIMAL, 18, 2)])
withpytest.raises(ValueError) asexc_info:
cursor.executemany(
"INSERT INTO #test_sis_dec_nondec (Price) VALUES (?)",
[(huge_exponent,)],
)
message=str(exc_info.value)
assert"Failed to convert parameter"inmessageassert"row 0"inmessageassert"column 0"inmessageasserthuge_exponentnotinmessageassertexc_info.value.__cause__isNoneformatted="".join(
traceback.format_exception(
type(exc_info.value), exc_info.value, exc_info.value.__traceback__
)
)
asserthuge_exponentnotinformattedfinally:
cursor.execute("DROP TABLE IF EXISTS #test_sis_dec_nondec")

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

pr-size: mediumModerate update size

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@sumitmsft@bewithgaurav@gargsaumya