Skip to content

Exclude HTTP/2 pseudo-headers from sanitization#575

Merged
trentm merged 1 commit intomainfrom
trentm/sanitize-not-http2-pseudo-headers
Jan 5, 2022
Merged

Exclude HTTP/2 pseudo-headers from sanitization#575
trentm merged 1 commit intomainfrom
trentm/sanitize-not-http2-pseudo-headers

Conversation

@trentm
Copy link
Member

@trentm trentm commented Dec 6, 2021

(This separates out the HTTP/2 pseudo-header discussion on #573. See some earlier discussion on options there.)

This is to avoid conflict between the ':authority' HTTP/2 pseudo-header
and the addition of *auth* to the sanitize_field_names default patterns in #573.

The Node.js HTTP/2 core lib includes the HTTP/2 pseudo-headers in $requestObject.headers and $responseObject.headers, so its normal capturing of headers includes them. Are other agents that have HTTP/2 support the same or different?

This is a small enhancement

  • Create PR as draft
  • Approval by at least one other agent
  • Mark as Ready for Review (automatically requests reviews from all agents and PM via CODEOWNERS)
    • Remove PM from reviewers if impact on product is negligible
    • Remove agents from reviewers if the change is not relevant for them
  • Merge after 2 business days passed without objections
This is to avoid conflict between the ':authority' HTTP/2 pseudo-header
and the addition of "*auth*" to the `sanitize_field_names` default patterns.
@trentm trentm self-assigned this Dec 6, 2021
@trentm trentm mentioned this pull request Dec 6, 2021
14 tasks
@ghost
Copy link

ghost commented Dec 6, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-01-03T04:19:00.538+0000

  • Duration: 3 min 17 sec

  • Commit: 7079b78

Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. If we want to capture these pseudo-headers at all, I think having a hard-coded exception for them is the way to go rather than adding config (re #573 (comment))

@trentm trentm marked this pull request as ready for review December 7, 2021 16:27
@trentm trentm requested review from a team as code owners December 7, 2021 16:27
@trentm trentm removed the request for review from a team December 7, 2021 16:28
@trentm trentm merged commit fe36cdb into main Jan 5, 2022
@trentm trentm deleted the trentm/sanitize-not-http2-pseudo-headers branch January 5, 2022 17:22
stuartnelson3 added a commit to stuartnelson3/apm-agent-go that referenced this pull request Mar 23, 2022
stuartnelson3 added a commit to elastic/apm-agent-go that referenced this pull request Mar 23, 2022
* sanitize *auth* instead of authorization

closes #1166

* add exception for http2 :authority pseudo-header

as discussed in elastic/apm#575
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

5 participants