Repository navigation
[fix](be) Redact HTTP query details from request diagnostics - #69506
Merged
mrhhsg merged 1 commit intoOct 26, 2026
Merged
Conversation
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: BE HTTP request diagnostics rendered the full URI and each parsed parameter, so query credentials could appear in request logs. Render only the parsed path and parameter count, and omit raw query text from the parse-failure warning. Request accessors retain their original values.
### Release note
BE HTTP request diagnostic logs no longer include query strings or parameter details.
### Check List (For Author)
- Test:
- Unit Test: HttpRequestTest.* (9 cases, using a temporary exclusion for an unrelated pre-existing test compile error)
- Regression test: Not applicable; only diagnostic log rendering changes
- Behavior changed: Yes (diagnostic log text only)
- Does this need documentation: No
Member
Author
|
/review |
Contributor
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed the complete patch at 9a9ffcbfe4cd36a7b29b7216b05a332f43da7b4b. No actionable new findings or existing blocking inline comments were identified.
Critical checkpoint conclusions:
- Goal and proof: diagnostics render the parsed path and parameter count; the parse-failure warning omits the query. The added tests exercise real query parsing and preserved accessors.
- Scope: the three production rendering edits and three focused unit tests are small and consistent with the stated logging-only goal.
- Concurrency: no new shared mutation, thread, lock, lock-held work, or lock-order dependency. Initialization and route insertion precede handler logging.
- Lifecycle: no new production ownership or static initialization. Fixtures detach the borrowed URI and freed components before libevent request destruction; the later wrapper destructor has no handler context (pinned libevent implementation).
- Configuration: no new configuration or dynamic-update behavior.
- Compatibility: diagnostic text intentionally changes; public interfaces, HTTP parsing/responses, wire/storage formats, and rolling-upgrade behavior are unchanged. No new libc API is introduced.
- Thrift/protobuf: no definitions or field IDs change.
- Parallel paths: auth, action, web-page, and async-timeout callers share the updated renderer. Header-only rendering needs no query change. Separate action-specific logs remain outside the declared scope.
- Conditional/error handling: no condition changes; query-parse failures still return the original error before handler dispatch.
- Test coverage: plain credentials, encoded/case-varied query parameters, an inserted route-map entry, no-query requests, path/count output, and unchanged URI/parameter getters are covered by the added assertions. Existing header-masking tests remain.
- Test results: no golden files change; expected decoding, counts, and absence assertions match the code. Tests were inspected, not executed.
- Observability: method, path, masked headers, and parameter count remain available. Reduced parameter detail is intentional; no new execution path requires metrics.
- Transactions/persistence/failover: unaffected; no visibility, version, bitmap, or EditLog changes.
- Data writes/atomicity: no data-write or crash-recovery behavior changes, and no new retained allocation.
- FE/BE variables: no transmitted variables or sending paths change.
- Performance: parameter iteration and full-query output are removed; map size is constant-time. No new expensive operation was found.
- Further issues: URI component separation and fixture cleanup were independently rechecked; no unresolved candidate remains.
User focus: none supplied. Validation limits: this was a static review with no builds, tests, or live HTTP requests. The parse-failure warning has no dedicated log-capture test; its constant rendering was inspected directly. Path contents, existing header behavior, and separate action-specific logs are outside the query-diagnostic change.
Review converged after 2 round(s).
Member
Author
|
run buildall |
Contributor
BE UT Coverage ReportIncrement line coverage Increment coverage report
|
Contributor
TPC-H: Total hot run time: 28802 ms |
Contributor
TPC-DS: Total hot run time: 150550 ms |
Contributor
ClickBench: Total hot run time: 24.17 s |
Contributor
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
mrhhsg
marked this pull request as ready for review
October 24, 2026 03:12
jacktengg
approved these changes
Oct 26, 2026
github-actions Bot
pushed a commit
that referenced
this pull request
Oct 26, 2026
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: `HttpRequest::debug_string()` rendered the full request
URI and every parsed parameter key and value, so query credentials could
appear in BE diagnostics. The query-parse failure warning also included
the raw query. Render the parsed path and parameter count instead, and
omit query text from that warning. Request parsing and URI/parameter
accessors retain their existing behavior. This change is limited to
these logging paths; action-specific HTTP logs are unchanged.
### Release note
BE `HttpRequest::debug_string()` no longer emits query strings or
parameter keys and values. Query-parse failure warnings no longer emit
the raw query.
### Check List (For Author)
- Test:
- Unit Test: `HttpRequestTest.*` passed (9/9). The unmodified full BE UT
build was blocked by a pre-existing compile error in an unrelated test
translation unit; the focused suite was run with that unit temporarily
excluded from the UT build.
- Build: `BUILD_TYPE=ASAN ./build.sh --be` passed.
- Static and format checks: BE build hygiene, clang-format,
check-format, and changed-line clang-tidy passed.
- Regression test: Not run; this changes BE diagnostic log text, not SQL
or HTTP responses.
- Behavior changed: Yes (diagnostic log text only).
- Does this need documentation: No.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
HttpRequest::debug_string()rendered the full request URI and every parsed parameter key and value, so query credentials could appear in BE diagnostics. The query-parse failure warning also included the raw query. Render the parsed path and parameter count instead, and omit query text from that warning. Request parsing and URI/parameter accessors retain their existing behavior. This change is limited to these logging paths; action-specific HTTP logs are unchanged.Release note
BE
HttpRequest::debug_string()no longer emits query strings or parameter keys and values. Query-parse failure warnings no longer emit the raw query.Check List (For Author)
HttpRequestTest.*passed (9/9). The unmodified full BE UT build was blocked by a pre-existing compile error in an unrelated test translation unit; the focused suite was run with that unit temporarily excluded from the UT build.BUILD_TYPE=ASAN ./build.sh --bepassed.