Skip to content

fix: redact secret property values in SessionContext Debug - #2960

Closed
plusplusjiajia wants to merge 1 commit into
apache:mainfrom
plusplusjiajia:fix/session-context-debug-redaction
Closed

fix: redact secret property values in SessionContext Debug#2960
plusplusjiajia wants to merge 1 commit into
apache:mainfrom
plusplusjiajia:fix/session-context-debug-redaction

Conversation

@plusplusjiajia

@plusplusjiajia plusplusjiajia commented Aug 5, 2026

Copy link
Copy Markdown
Member

What changes are included in this PR?

SessionContext's derived Debug printed properties verbatim, while session properties routinely carry secrets. Redaction now happens on two levels:

  • By key: values of sensitive keys are redacted, matched case-insensitively and separator-agnostically (token, s3.secret-access-key, AWS_SECRET_ACCESS_KEY, clientSecret, Authorization, Cookie, ...) since property keys carry no casing contract.
  • By value: values that are themselves credential-bearing are redacted even under innocuous keys — URIs with userinfo (e.g. the SQL catalog's uri holding postgres://user:pass@host/db) and signed URLs (X-Amz-Signature=, SAS sig=).

Plain values (warehouse names, credential-free URIs) stay visible, so Credential's redaction isn't undone by the map next to it. Same approach as the REST catalog config redaction in #2838.

Are these changes tested?

Table-driven tests pin both matchers (key casings/separators, URI/signed-URL values, plus negative cases), and a Debug formatting test asserts secret values are redacted while plain values remain visible.

@plusplusjiajia
plusplusjiajia marked this pull request as ready for review August 5, 2026 09:14
@plusplusjiajia
plusplusjiajia force-pushed the fix/session-context-debug-redaction branch 10 times, most recently from 767c783 to 36f0722 Compare August 6, 2026 01:43
@plusplusjiajia
plusplusjiajia force-pushed the fix/session-context-debug-redaction branch from 36f0722 to f7ceb7e Compare August 6, 2026 01:48
@CTTY

CTTY commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

Do we really need this? The sessionContext will be fully controlled by users, and they can set them in credentials if they want to keep them redacted

@plusplusjiajia

Copy link
Copy Markdown
Member Author

Do we really need this? The sessionContext will be fully controlled by users, and they can set them in credentials if they want to keep them redacted

Fair question — credentials didn't exist when I opened this (it came with #2971), and with it there, redacting by key name is largely redundant.
What it doesn't cover is a secret embedded in a value that is legitimately a property: the SQL catalog's uri is postgres://user:pass@host/db, and SessionContext still derives Debug. Happy to cut this down to just that case, or close it.

@blackmwk

Copy link
Copy Markdown
Contributor

I also don't think this added much value. It should be user's responsibility to put sensitive properties in credentials, and it's hard to figure out what property is sensitive from keys.

@blackmwk

Copy link
Copy Markdown
Contributor

I'll close this pr for now, feel free to reopen it if necessary.

@blackmwk blackmwk closed this Aug 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants