[fix](fe) Mask sensitive headers in stream load logs#62108
[fix](fe) Mask sensitive headers in stream load logs#62108gavinchou merged 2 commits intoapache:masterfrom
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
There was a problem hiding this comment.
Pull request overview
This PR reduces the risk of credential leakage by masking selected sensitive HTTP request headers when FE logs stream load REST requests.
Changes:
- Mask values for a small set of sensitive headers (e.g.,
Authorization,token) ingetAllHeaders(). - Add
isSensitiveHeader()helper to centralize the masking decision.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
### What problem does this PR solve? Issue Number: None Related PR: None Problem Summary: FE stream load REST logs printed full request headers, which could leak Authorization and token values into INFO logs. ### Release note None ### Check List (For Author) - Test: No need to test (log sanitization only; no completed automated test run in this environment) - Behavior changed: No - Does this need documentation: No
c55096e to
012d589
Compare
|
run buildall |
TPC-H: Total hot run time: 29072 ms |
### What problem does this PR solve? Issue Number: None Related PR: apache#62108 Problem Summary: Expand stream load header masking to cover cookie headers and add a regression test for sensitive header masking. ### Release note Mask Cookie and Set-Cookie headers in FE stream load logs. ### Check List (For Author) - Test: FE unit test - ./run-fe-ut.sh --run org.apache.doris.httpv2.rest.LoadActionTest - Behavior changed: Yes (Cookie and Set-Cookie headers are now masked in logs) - Does this need documentation: No
|
run buildall |
|
/review |
There was a problem hiding this comment.
One blocking issue remains, so I cannot approve this PR yet.
Critical checkpoint conclusions:
- Goal of the task: Partially accomplished. The patch masks sensitive headers in
getAllHeaders()and adds a focused FE unit test, but it does not fully prevent sensitive credential exposure because the invalid cluster-token path still echoes the raw token back in theUnauthorizedExceptionmessage. - Modification size/focus: Yes. The change is small and focused on stream-load header logging.
- Concurrency: Not applicable. No shared-state or locking changes are introduced here.
- Lifecycle/static initialization: Not applicable.
- Configuration changes: None.
- Compatibility/incompatible change: None.
- Functionally parallel code paths: Not fully handled. The same raw-token echo pattern still exists in this controller flow and in the matching
StreamingJobActionpath. - Special conditional checks: The new sensitive-header filter is straightforward, but because this is security-sensitive, partial masking is insufficient when another path still exposes the same secret.
- Test coverage: Improved but incomplete. The added FE unit test verifies header masking, but there is no coverage for the invalid-token/error-response path that still leaks the credential.
- Observability: Sufficient for this scope; no extra metrics/logging needed.
- Transaction/persistence: Not applicable.
- Data write/modification correctness: Not applicable.
- FE-BE variable passing: Not applicable.
- Performance: No meaningful concern in this change.
- Other issues: Blocking security issue described inline.
Please remove raw token values from unauthorized/error messages before this is approved.
invalid token don't need to mask.
|
PR approved by at least one committer and no changes requested. |
|
PR approved by anyone and no changes requested. |
FE stream load REST logs printed full request headers, which could leak Authorization and token values into INFO logs. Changes: - Mask values for a small set of sensitive headers (e.g., Authorization, token) in getAllHeaders() - Add isSensitiveHeader() helper to centralize the masking decision
FE stream load REST logs printed full request headers, which could leak Authorization and token values into INFO logs. Changes: - Mask values for a small set of sensitive headers (e.g., Authorization, token) in getAllHeaders() - Add isSensitiveHeader() helper to centralize the masking decision
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: FE stream load REST logs printed full request headers, which could leak Authorization and token values into INFO logs.
Release note
None
Check List (For Author)