fix(rest): Filter sensitive headers from error logs (#2117)#2130
Conversation
6e69dd0 to
6a2bda1
Compare
crates/catalog/rest/src/client.rs
Outdated
| .filter(|(name, _)| !is_sensitive_header(name.as_str())) | ||
| .filter_map(|(name, value)| value.to_str().ok().map(|v| (name.as_str(), v))) | ||
| .collect(); | ||
| format!("{filtered:?}") |
There was a problem hiding this comment.
When we say redacting sensitive header, I think we replace actually content with things like xxxx rather than removing them?
There was a problem hiding this comment.
The reason we want to keep them rather than removing them directly is that if maybe confusing when missing these headers when debugging. Also, I would prefer to add a config to disable the redaction. We could redact these header by default, but we should allow user to disable it.
There was a problem hiding this comment.
Thanks for the feedback @blackmwk! Updated to redact instead of filter using [REDACTED] as a value and made it configurable.
blackmwk
left a comment
There was a problem hiding this comment.
Thanks @cmackenzie1 for this pr, this is quite useful, just one comment.
Sensitive headers like Set-Cookie, Authorization, and Cookie were being included in error messages, potentially exposing authentication tokens and session information in logs. This change replaces sensitive headers with `[REDACTED]` from error context rather than logging their values, preventing credential leakage. This option can be disabled using `REST_CATALOG_PROP_DISABLE_HEADER_REDACTION=true`
6a2bda1 to
d2abcab
Compare
blackmwk
left a comment
There was a problem hiding this comment.
The method name is a little odd, but it's an internal method, so I think it's fine to go. Thanks @cmackenzie1 for this pr!
| /// | ||
| /// If `disable_redaction` is true, returns all headers without redaction. | ||
| /// Otherwise, replaces sensitive header values with "[REDACTED]". | ||
| fn format_headers_redacted(headers: &HeaderMap, disable_redaction: bool) -> String { |
There was a problem hiding this comment.
| fn format_headers_redacted(headers: &HeaderMap, disable_redaction: bool) -> String { | |
| fn format_headers(headers: &HeaderMap, disable_redaction: bool) -> String { |
* Use uv instead of pip for python packages (apache#2129) * refactor(storage): Reorganize storage code into a new module (apache#2109) ## Which issue does this PR close? - Closes #. ## What changes are included in this PR? - Add a new module `storage` within `io` and put traits there ## Are these changes tested? Relying on the existing tests * fix: Interpret s3tables warehouse as table_location not metadata loca… (apache#2115) ## Which issue does this PR close? AWS [Docs](https://docs.aws.amazon.com/AmazonS3/latest/userguide/s3-tables-tables.html) state: > When you create a table, Amazon S3 automatically generates a warehouse location for the table. This is a unique S3 location that stores objects associated with the table. The following example shows the format of a warehouse location: ``` s3://63a8e430-6e0b-46f5-k833abtwr6s8tmtsycedn8s4yc3xhuse1b--table-s3 ``` We were previously interpreting this as as a metadata location (i.e. the path to the metadata.json file), this changes the code use it as a table location. - Closes apache#2114 ## What changes are included in this PR? Change how we construct the MetadataLocation object. ## Are these changes tested? There never appears to have been a test, and I don't have an AWS account to verify this. Note the test was initially developed by Claude Code and refined by me. * fix(rest): Filter sensitive headers from error logs (apache#2117) (apache#2130) * Merge upstream * . * . * Fix merge mistakes * . --------- Co-authored-by: blackmwk <liurenjie1024@outlook.com> Co-authored-by: Shawn Chang <yxchang@amazon.com> Co-authored-by: emkornfield <emkornfield@gmail.com> Co-authored-by: Cole Mackenzie <colemackenzie1@gmail.com>
Which issue does this PR close?
What changes are included in this PR?
Sensitive headers like Set-Cookie, Authorization, and Cookie were being included in error messages, potentially exposing authentication tokens and session information in logs.
This change replaces sensitive headers with
[REDACTED]from error context rather than logging their values, preventing credential leakage.This option can be disabled using
REST_CATALOG_PROP_DISABLE_HEADER_REDACTION=trueAre these changes tested?
Covered by unit tests.