Skip to content

fix(log): redact secrets from the request payload stored in the event log - #45353

Open
hughhhh wants to merge 2 commits into
apache:masterfrom
hughhhh:hughhhh/redact-secrets-in-event-log
Open

hughhhh wants to merge 2 commits into
apache:masterfrom
hughhhh:hughhhh/redact-secrets-in-event-log

Conversation

@hughhhh

@hughhhh hughhhh commented Oct 15, 2026 •

Copy link
Copy Markdown
Member

SUMMARY

The event logger stores each request's form, args and JSON body, so the Action Log kept every secret typed into the database dialog, a connection test or an import in clear (database and SSH tunnel passwords, OAuth2 client secrets, embedded fallback credentials, import password / encrypted_extra_secrets maps). collect_request_payload now masks the value of any sensitive key at any depth and keeps sqlalchemy_uri readable with its password and query values masked.

TESTING INSTRUCTIONS

pytest tests/unit_tests/utils/log_tests.py tests/unit_tests/databases/api_test.py -k "redact or never_carries or never_records". The two end-to-end tests fail without the change.

ADDITIONAL INFORMATION

  • Has associated issue:
  • Changes UI

🤖 Generated with Claude Code

Comment thread superset/utils/log.py Resolved
@codeant-ai-for-open-source

codeant-ai-for-open-source Bot commented Oct 15, 2026 •

Copy link
Copy Markdown
Contributor

CodeAnt  CodeAnt PR Risk

🟢  Low Risk

  • The PR appears safe to merge; URI passwords and query values are now masked before request payloads reach the event log.
  • Tests cover URI query credentials, form parameters, and the request logging path.

Assessed commit: 8363b4d4ba41

@sadpandajoe sadpandajoe added the review:checkpoint Last PR reviewed during the daily review standup label Oct 15, 2026
@codecov

codecov Bot commented Oct 15, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.69767% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.93%. Comparing base (730016b) to head (8363b4d).
⚠️ Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
superset/utils/log.py 90.69% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #45353   +/-   ##
=======================================
  Coverage   84.93%   84.93%           
=======================================
  Files        3077     3077           
  Lines      208202   208243   +41     
  Branches    49105    49118   +13     
=======================================
+ Hits       176831   176868   +37     
- Misses      28002    28004    +2     
- Partials     3369     3371    +2     
Flag Coverage Δ
hive 34.19% <27.90%> (-0.01%) ⬇️
mysql 52.18% <76.74%> (+<0.01%) ⬆️
postgres 52.19% <76.74%> (+<0.01%) ⬆️
presto 35.89% <27.90%> (-0.01%) ⬇️
python 87.88% <90.69%> (+<0.01%) ⬆️
sqlite 51.94% <74.41%> (+<0.01%) ⬆️
unit 82.72% <90.69%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sadpandajoe sadpandajoe removed the review:checkpoint Last PR reviewed during the daily review standup label Oct 16, 2026
@github-actions github-actions Bot added the requires:rebase Requires rebasing on top of current master label Nov 2, 2026
hughhhh and others added 2 commits November 3, 2026 12:01
… log

The event logger stores each request's form, args and JSON body, so the
Action Log kept every secret typed into the database dialog, a connection
test or an import in clear: database passwords, SSH tunnel credentials,
OAuth2 client secrets, embedded fallback credentials, and the import
password and encrypted_extra_secrets maps. collect_request_payload now
masks the value of any sensitive key at any depth, and keeps
sqlalchemy_uri readable with only its password masked.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A SQLAlchemy URI can carry its credential in the query string
(motherduck_token, azure_ad_client_secret), under names particular to each
engine, and such a URI was logged unchanged. Mask every query value, in the
URI and in the form's parameters.query that builds it, rather than keep a
list of names that would leak the next one.

Also mask whole a URI whose parsed host holds an "@", which is what remains
of an unencoded "@" in the password, and any non-string sqlalchemy_uri.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@hughhhh
hughhhh force-pushed the hughhhh/redact-secrets-in-event-log branch from 97c7714 to 8363b4d Compare November 3, 2026 17:04

@aminghadersohi aminghadersohi 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.

Some supported connection credentials still reach the Action Log.

Comment thread superset/utils/log.py
"private_key",
"private_key_password",
"privatekey_body",
"privatekey_pass",

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.

query_input duplicates the Additional Parameters text beside parameters.query, but only the latter is masked. Entering sslpassword=... still records the credential on validation/save; mask query_input too.

Suggested change
"privatekey_pass",
"privatekey_pass",
"query_input",

Comment thread superset/utils/log.py
Comment on lines +181 to +182
if url.password is None and not url.query:
return uri

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.

Arc API keys and Dremio ODBC tokens occupy the URI username, so they still reach Action Log records verbatim, with or without query parameters. Mask these credential-bearing usernames before returning the URI.

Suggested change
if url.password is None and not url.query:
return uri
if url.username and (
url.drivername.split("+", 1)[0] == "arc"
or url.drivername == "dremio+pyodbc"
):
return PASSWORD_MASK
if url.password is None and not url.query:
return uri

Comment thread superset/utils/log.py
Comment on lines +179 to +180
if url.host and "@" in url.host:
return PASSWORD_MASK

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.

Nit: An unencoded '@' followed by '/' or '?' can leave part of a password outside the parsed host, bypassing this check. For example, postgresql://user:p@ss/word@host/db still logs ss/word; mask the whole ambiguous URI.

Suggested change
if url.host and "@" in url.host:
return PASSWORD_MASK
if (url.host and "@" in url.host) or (
url.password is not None and uri.count("@") > 1
):
return PASSWORD_MASK

@github-actions github-actions Bot removed the requires:rebase Requires rebasing on top of current master label Nov 3, 2026

This branch has not been deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants