Repository navigation
[BUG] PasswordMasker's urlparse fallback drops the query/fragment and invents credentials #12241
Description
What is wrong
PasswordMasker has two ways to mask a database URL. When SQLAlchemy cannot build an engine for the URL - no driver installed for that dialect is enough - it falls back to _mask_db_url_no_sa (great_expectations/data_context/util.py:260-283), which re-assembles the URL from the parsed components it happens to name:
masked_url = (
f"{parsed_url.scheme}://{parsed_url.username}:{cls.MASKED_PASSWORD_STRING}"
f"@{parsed_url.hostname}{colon}{parsed_url.port or ''}{parsed_url.path or ''}"
)Everything that is not in that list is discarded, and any component that is absent is stringified as None. Measured on develop (8bfb2b2), Python 3.11.15, SQLAlchemy 2.0.54, alongside what SQLAlchemy itself renders for the same URL:
input : mssql+pyodbc://scott:tiger@h:1433/db?driver=ODBC+Driver+17&TrustServerCertificate=yes
sqlalchemy: mssql+pyodbc://scott:***@h:1433/db?TrustServerCertificate=yes&driver=ODBC+Driver+17
fallback : mssql+pyodbc://scott:***@h:1433/db <- the driver is gone
input : postgresql://scott:tiger@h:5432/db?sslmode=require
sqlalchemy: postgresql://scott:***@h:5432/db?sslmode=require
fallback : postgresql://scott:***@h:5432/db <- sslmode=require is gone
input : bigquery://my-project/dataset
sqlalchemy: bigquery://my-project/dataset
fallback : bigquery://None:***@my-project/dataset <- invents a masked password
input : postgresql://scott@h:5432/db
sqlalchemy: postgresql://scott@h:5432/db
fallback : postgresql://scott:***@h:5432/db <- claims a password there isn't one
input : postgresql://scott:tiger@[::1]:5432/db?sslmode=require
sqlalchemy: postgresql://scott:***@[::1]:5432/db?sslmode=require
fallback : postgresql://scott:***@::1:5432/db <- brackets lost, host/port ambiguous
input : mysql+pymysql://scott:tiger@/db?unix_socket=/tmp/mysql.sock
sqlalchemy: mysql+pymysql://scott:***@/db?unix_socket=%2Ftmp%2Fmysql.sock
fallback : mysql+pymysql://scott:***@None/db <- empty host becomes "None"
input : postgresql://scott:tiger@h:5432/db#section
sqlalchemy: postgresql://scott:***@h:5432/db#section
fallback : postgresql://scott:***@h:5432/db
The masked string is what ends up in log records, in the exception_traceback of a failed Validation, and in a serialized DataContext, so the practical harm is diagnosis rather than a leak: the password is still replaced in every case above. But a URL that silently drops ?driver= or ?sslmode=require describes a different connection than the one configured, and "no credentials at all" being rendered as None:***@ misstates whether a secret exists.
The repo already states the intended property, in tests/data_context/test_data_context_utils.py:69: "The output of mask_db_url should be the same whether user_urlparse is set to True or False." For any URL with a query string that cannot hold today, because only one of the two paths keeps the query.
Reproduction
from great_expectations.data_context.util import PasswordMasker
PasswordMasker.mask_db_url(
"mssql+pyodbc://scott:tiger@h:1433/db?driver=ODBC+Driver+17&TrustServerCertificate=yes",
use_urlparse=True,
)
# 'mssql+pyodbc://scott:***@h:1433/db'
PasswordMasker.mask_db_url("bigquery://my-project/dataset", use_urlparse=True)
# 'bigquery://None:***@my-project/dataset'use_urlparse=True forces the branch; it is also taken automatically whenever sa.create_engine(url) raises, which is the ordinary situation for a dialect whose DBAPI is not installed in the process that happens to be serializing the config.
Expected behaviour
Replace the password and nothing else: keep the query, the fragment, the host exactly as written (including IPv6 brackets), and add no credentials that the URL did not carry.
Activity
akashmalbari commentedon Sep 24, 2026
Thanks for the detailed reproduction. I verified the issue is still open, unassigned, and has no overlapping open PR. Would a focused PR be welcome that makes the URL-parser fallback replace only an existing password while preserving all other URL components (including query, fragment, IPv6 brackets, and host-less URLs), with regression coverage for the fallback path? I will wait for maintainer confirmation before beginning implementation.
feiiiiii5 commentedon Sep 24, 2026
Thanks for checking. An open PR now covers the same
PasswordMaskerfallback issue: #12262. Please follow that PR rather than start a duplicate; maintainers have not yet reviewed or confirmed the approach.joshua-stauffer commentedon Sep 29, 2026
Thanks for the thorough report. The masked URL should describe the connection that was actually configured, and this issue shows several ways it doesn't.
There are two open PRs for this issue. We'll go ahead with #12242, since it was opened first alongside the report. Thank you @akashmalbari for #12262; please follow #12242 rather than continuing it separately. Before #12242 can merge, it needs to meet the requirements below.
Requirements
Output fidelity (urlparse fallback)
***. Everything else comes through exactly as written: the scheme including any+driversuffix (e.g.oracle+cx_oracle), the username, the host (including case and IPv6 brackets), the port, the path, and the fragment. That includes the empty authority in URLs such asmssql+pyodbc:///?odbc_connect=....bigquery://my-project/datasetstays as-is). A URL with a username but no password gets no:***. An empty host stays empty rather than becomingNone.Query-string credentials (both paths)
***unless the name is on an explicit allowlist of parameters known to carry no credential. This follows the existing Azure connection-string masking (AZURE_SAFE_TO_DISPLAY_FIELDS). A reasonable starting list, matched case-insensitively, issslmode,driver,charset,encoding,connect_timeout,application_name,unix_socket,warehouse,role,schema,database,encrypt,trustservercertificate. Both paths must share one implementation of this rule. An unrecognised parameter, including a wholeodbc_connectvalue, is masked.Consistency and safety
test_password_masker_mask_db_urlclaims both paths give the same output; the new tests should actually check it.sqliteURLs, config-substitution strings (${VAR}), and Azure connection strings.Tests
unit-marked tests that fail ondevelopand pass with the fix. They should cover every example in this issue, the cases in item 4 on both paths (force the fallback withuse_urlparse=True), and the unparseable-URL case in item 6. For item 4, include at least one allowlisted parameter left visible,password=masked, and anodbc_connectvalue masked.Where #12242 stands (head
ce4fcd8)oracle+cx_oracleURL whose host containsoracleis rewritten:oracle-db.example.combecomesoracle+cx_oracle-db.example.com. The scheme restore at the end of_mask_db_url_no_sais an unboundedstr.replace. This predates the PR, but it falls under item 1, and keeping the query gives it more text to corrupt.mssql+pyodbc:///?odbc_connect=...comes out asmssql+pyodbc:/?odbc_connect=..., becauseurlunparsedrops an empty netloc.check-actor-permissions,ci-required) are the fork-PR approval gate, not test failures. I'll start CI once the changes above are pushed.[BUGFIX] Keep the URL intact when masking a database password (fivetr…
Mask credentials passed as URL query parameters
feiiiiii5 commentedon Sep 29, 2026
Thanks for the detailed acceptance criteria, @joshua-stauffer. #12242 is now at
62aa0a1; the update addresses items 1–8, including shared query-parameter masking for both paths and tests for the URL shapes you listed. I put the item-by-item results and local verification in the PR update so the review can use one checklist. Please let me know if any case still misses your intent.github-actions commentedon Oct 2, 2026
⏰ Assignment Reminder
Hi @feiiiiii5, this is a friendly reminder about your assignment to this issue.
Warning
This issue will be automatically unassigned in 4 days if there's no activity.
How to keep your assignment
You can prevent automatic unassignment by:
We appreciate your contribution and are here to help if needed!
joshua-stauffer commentedon Oct 2, 2026
@akashmalbari thanks for being willing to pick this up - our contributor guidelines state that an issue needs the
ready-for-worklabel prior to being picked up. I've closed your PR, but we would welcome a contribution in the future!