baggage module fails to accept baggage produced by itself

[Lim3nius]

Description

In current implementation ([email protected]) of baggage propagation and parsing it is possible to pass value into baggage member, which is accepted, then inject given baggage into HeaderCarrier and then extract baggage from HeaderCarrier to find out, that baggage hasn't been successfully parsed.

Steps To Reproduce

• Playground link https://go.dev/play/p/Edu6AMmhD2G with demonstration of given behavior.

Or

• Try to propagate baggage.NewMember("ok-key", url.QueryEscape("messed,up%key")) through HeaderCarrier

Expected behavior

Parsed baggage contains member with key "ok-key" and value "messed,up%key"

Other issues dealing with same problem

• Error while parsing Baggage header with percent-encoded whitespace #3601
• baggage member validation in ParseMember #3467

Problem

In baggage.parseMember() value of baggage is first unescaped and then is checked whether it contains "forbidden values" by W3C Baggage wire restriction.

Solution

Most obvious solution to me is to move check that value conforms to W3C Baggage right before unescaping that value, or completely remove that check.

[Lim3nius]

@MrAlias could you take a look at this and lets finally resolve this baggage problem? Also addressing more deep problem with OpenTelemetry baggage implementation described here would be nice.

[pellared]

In current implementation ([email protected]) of baggage propagation and parsing it is possible to pass value into baggage member, which is accepted, then inject given baggage into HeaderCarrier and then extract baggage from HeaderCarrier to find out, that baggage hasn't been successfully parsed.

On parsing see https://www.w3.org/TR/baggage/#value:

Any characters outside of the baggage-octet range of characters MUST be percent-encoded. Characters which are not required to be percent-encoded MAY be percent-encoded.

Why do you expect hc.Get() to return "unecaped" value? This would be wrong.

The "semantical type" of Parse input and hc.Get() output is different. Bboth are string but they have different formatting. The first expect an "encoded value", the second returns a "decoded value".

I think that the current behavior is OK and conforms with the Baggage Specification.

[Lim3nius]

@pellared Thanks for your interest in this issue, I'd like to point out, that main problem was demonstrated in lines 24-46, that given baggage was not succefully propagated through http.Header, i.e. no baggage members were found after extract.

The part with Parse is basically to demonstrate what happens in code of Baggage.Extract()

I.e. still there is problem, that I can encode some value which is accepted by baggage, transfer it via http headers and then all baggage is lost during extraction. That part seems completely missed!

Side note: If I double url.QueryEscape baggage on line 26, then baggage is succesfuly parsed...

[pellared]

@Lim3nius

Sorry, you are correct. I needed more time to understand it.

Most obvious solution to me is to move check that value conforms to W3C Baggage right before unescaping that value

Here you are correct as well.

I will try to make a PR which fixes it on Thursday or Monday next week.

Let me know what do you think of these tests (added to propagation/baggage_test.go):

func TestBaggageInjectExtractRoundtrip(t *testing.T) {
	propagator := propagation.Baggage{}
	tests := []struct {
		name string
		mems members
	}{
		{
			name: "two simple values",
			mems: members{
				{Key: "key1", Value: "val1"},
				{Key: "key2", Value: "val2"},
			},
		},
		{
			name: "values with escaped chars",
			mems: members{
				{Key: "key1", Value: "val3=4"},
				{Key: "key2", Value: "mess,me%up"},
			},
		},
		{
			name: "with properties",
			mems: members{
				{Key: "key1", Value: "val1"},
				{
					Key:   "key2",
					Value: "val2",
					Properties: []property{
						{Key: "prop", Value: "1"},
					},
				},
			},
		},
	}
	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			b := tt.mems.Baggage(t)
			req, _ := http.NewRequest("GET", "http://example.com", nil)
			ctx := baggage.ContextWithBaggage(context.Background(), b)
			propagator.Inject(ctx, propagation.HeaderCarrier(req.Header))

			ctx = propagator.Extract(context.Background(), propagation.HeaderCarrier(req.Header))
			got := baggage.FromContext(ctx)

			assert.Equal(t, b, got)
		})
	}
}

If you want you can do the PR yourself then I will review it 😉