Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Error while parsing Baggage header with percent-encoded whitespace #3601

Closed
tonyo opened this issue Jan 18, 2023 · 3 comments
Closed

Error while parsing Baggage header with percent-encoded whitespace #3601

tonyo opened this issue Jan 18, 2023 · 3 comments
Labels
bug Something isn't working question Further information is requested
Milestone
untracked

Comments

@tonyo
Copy link

tonyo commented Jan 18, 2023

Description

At the moment baggage.Parse fails when trying to decode a value with percent-encoded whitespace "%20" (and potentially others?).

Related issue in "opentelemetry-python" that has a pending fix: open-telemetry/opentelemetry-python#2934

A test case in "opentelemetry-js" that hints that whitespaces should be properly handled when encoding and decoding baggage: https://github.com/open-telemetry/opentelemetry-js/blob/main/packages/opentelemetry-core/test/baggage/W3CBaggagePropagator.test.ts#L60

Might be also related to #3467.

Environment

  • Go Version: 1.19
  • opentelemetry-go version: v1.11.2

Steps To Reproduce

The following code returns the error: invalid value: "val1 val2"

val, err := baggage.Parse("key1=val1%20val2")

https://go.dev/play/p/2kLO3RFzH_R

Expected behavior

key1=val1%20val2 should be properly parsed into a baggage item with key "key1" and value "val1 val2" (at least that's my understanding).
@tonyo tonyo added the bug Something isn't working label Jan 18, 2023
This was referenced Jan 30, 2023
Merged
Closed
@tonyo
Copy link
Author

tonyo commented Feb 3, 2023 •
edited
Loading

After looking into this a bit more, I believe here are some issues with the current implementation:

  1. Baggage string value one%20two should be properly parsed as "one two"
  2. Baggage string value one+two should be parsed as "one+two"
  3. Go string value "one two" should be encoded as one%20two (percent encoding), and NOT as one+two (URL query encoding).
  4. Go string value "1=1" might be encoded as 1=1 (the spec says: "Note, value MAY contain any number of the equal sign (=) characters. Parsers MUST NOT assume that the equal sign is only used to separate key and value."). 1%3D1 is also valid, but to simplify the implementation we don't have to do extra encoding.

This is our take on fixing these issues: getsentry/sentry-go#568

@Lim3nius Lim3nius mentioned this issue Jun 20, 2023
Closed
n-oden added a commit to odenio/opentelemetry-go that referenced this issue Oct 24, 2023
@n-oden
Use url.PathUnescape rather than url.QueryUnescape
870534d 
I believe this addresses the majority of the cases described in
open-telemetry#3601

Golang's url.QueryUnescape will render url _path_ elements (e.g. /, +)
as spaces: `foo+bar` is rendered as `foo bar`.  Path elements are (as I
read the spec) legal W3C baggage values, and replacing them with spaces
fails the value validation regex.

url.PathEscape allows path elements through unmolested.

Signed-off-by: Nathan J. Mehl <[email protected]>
n-oden added a commit to odenio/opentelemetry-go that referenced this issue Oct 24, 2023
@n-oden
Use url.PathUnescape rather than url.QueryUnescape
f22a4f2 
I believe this addresses the majority of the cases described in
open-telemetry#3601

Golang's url.QueryUnescape will render url _path_ elements (e.g. /, +)
as spaces: `foo+bar` is rendered as `foo bar`.  Path elements are (as I
read the spec) legal W3C baggage values, and replacing them with spaces
fails the value validation regex.

url.PathEscape allows path elements through unmolested.

Signed-off-by: Nathan J. Mehl <[email protected]>
@n-oden n-oden mentioned this issue Oct 24, 2023
Merged
@n-oden
Copy link
Contributor

n-oden commented Oct 24, 2023 •
edited
Loading

@tonyo I believe I've figured out at least the decoding side of this: #4667

I suspect you're correct about the encoding side as well, but swapping out url.QueryEncode with url.PathEncode broke a bunch of the unit tests, and I'm leery of breaking existing behavior.

MrAlias pushed a commit that referenced this issue Oct 31, 2023
@n-oden @pellared
Use url.PathUnescape rather than url.QueryUnescape in baggage parsing (…
bdb9322 
…#4667)

* Use url.PathUnescape rather than url.QueryUnescape

I believe this addresses the majority of the cases described in
#3601

Golang's url.QueryUnescape will render url _path_ elements (e.g. /, +)
as spaces: `foo+bar` is rendered as `foo bar`.  Path elements are (as I
read the spec) legal W3C baggage values, and replacing them with spaces
fails the value validation regex.

url.PathEscape allows path elements through unmolested.

Signed-off-by: Nathan J. Mehl <[email protected]>

* Update CHANGELOG.md

address comments

Co-authored-by: Robert Pająk <[email protected]>

---------

Signed-off-by: Nathan J. Mehl <[email protected]>
Co-authored-by: Robert Pająk <[email protected]>
@pellared
Copy link
Member

pellared commented Dec 6, 2023

@tonyo Can you please double-check if releases starting from https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.20.0 are solving the issue you described?

@pellared pellared added the question Further information is requested label Dec 6, 2023
@XSAM XSAM added this to the untracked milestone Nov 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working question Further information is requested
Projects
None yet
Milestone
untracked
Development

No branches or pull requests
4 participants
@tonyo @pellared @XSAM @n-oden