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.

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

baggage: Fix escaping in Member.String #4756

Merged
merged 11 commits into from Dec 21, 2023

Conversation

pellared
Copy link
Member

@pellared pellared commented Dec 14, 2023

Why

%20 (whitespace) is improperly encoded as +.

Test TestMemberString result before the fix:

--- FAIL: TestMemberString (0.00s)
    /home/rpajak/repos/opentelemetry-go/baggage/baggage_test.go:878: 
        	Error Trace:	/home/rpajak/repos/opentelemetry-go/baggage/baggage_test.go:878
        	Error:      	Not equal: 
        	            	expected: "key=%3B+"
        	            	actual  : "key=%3B%20"

I noticed the bug when working on #4755.

What

Only CTLs, whitespace, DQUOTE, comma, semicolon, backslash have to be escaped.

Characters like +, =, - do NOT need to be escaped - they are acceptable. Therefore, some tests are updated as they are no longer escaped.

Reference: https://www.w3.org/TR/baggage/#definition

Take notice that url.PathUnescape is used for decoding then it also makes more sense to use url.PathEscape to encode. Not mentioning that using url.QueryEscape simply produces invalid value.

Copy link

codecov bot commented Dec 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e3bf787) 82.2% compared to head (bde4937) 82.2%.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #4756   +/-   ##
=====================================
  Coverage   82.2%   82.2%           
=====================================
  Files        226     226           
  Lines      18342   18344    +2     
=====================================
+ Hits       15085   15087    +2     
  Misses      2975    2975           
  Partials     282     282           
Files Coverage Δ
baggage/baggage.go 100.0% <100.0%> (ø)

baggage/baggage.go Outdated Show resolved Hide resolved
baggage/baggage_test.go Outdated Show resolved Hide resolved
baggage/baggage_test.go Outdated Show resolved Hide resolved
baggage/baggage_test.go Outdated Show resolved Hide resolved
@pellared
Copy link
Member Author

I created:

I am waiting until tomorrow before merging.

@pellared pellared merged commit 885210b into open-telemetry:main Dec 21, 2023
25 checks passed
@pellared pellared deleted the fix-baggage-string branch December 21, 2023 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants