diff --git a/api/all/src/main/java/io/opentelemetry/api/baggage/propagation/Parser.java b/api/all/src/main/java/io/opentelemetry/api/baggage/propagation/Parser.java index a96d2aebf7d..9720ca30005 100644 --- a/api/all/src/main/java/io/opentelemetry/api/baggage/propagation/Parser.java +++ b/api/all/src/main/java/io/opentelemetry/api/baggage/propagation/Parser.java @@ -147,7 +147,6 @@ private void putBaggage( String decodedValue; try { decodedValue = decodeValue(value); - metadataValue = decodeValue(metadataValue); } catch (IllegalArgumentException e) { LOGGER.log(Level.WARNING, "Skipping invalid baggage member", e); return; diff --git a/api/all/src/main/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagator.java b/api/all/src/main/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagator.java index 2b589315435..a7cc8c6d69e 100644 --- a/api/all/src/main/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagator.java +++ b/api/all/src/main/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagator.java @@ -78,20 +78,18 @@ private static String baggageToString(Baggage baggage) { } String encodedValue = encodeValue(baggageEntry.getValue()); String metadataValue = baggageEntry.getMetadata().getValue(); - String encodedMetadata = - (metadataValue != null && !metadataValue.isEmpty()) - ? encodeValue(metadataValue) - : null; + String metadata = + (metadataValue != null && !metadataValue.isEmpty()) ? metadataValue : null; // Exit early if adding this entry causes the total length to exceed the limit // encodedEntryLength includes a trailing comma; the final string trims exactly one, // so the net contribution to the final length is entryLength - 1. - if (headerContent.length() + encodedEntryLength(key, encodedValue, encodedMetadata) - 1 + if (headerContent.length() + encodedEntryLength(key, encodedValue, metadata) - 1 > MAX_BAGGAGE_BYTES) { return; } headerContent.append(key).append("=").append(encodedValue); - if (encodedMetadata != null) { - headerContent.append(";").append(encodedMetadata); + if (metadata != null) { + headerContent.append(";").append(metadata); } headerContent.append(","); entryCount[0]++; @@ -113,14 +111,13 @@ private static String encodeValue(String value) { /** * Returns the length of the serialized entry as it would appear in the baggage header, including * the trailing comma used by the trailing-comma pattern in {@link #baggageToString}. The length - * accounts for {@code "key=encodedValue,"} plus {@code ";encodedMetadata"} when metadata is - * present. + * accounts for {@code "key=encodedValue,"} plus {@code ";metadata"} when metadata is present. */ private static int encodedEntryLength( - String key, String encodedValue, @Nullable String encodedMetadata) { + String key, String encodedValue, @Nullable String metadata) { int length = key.length() + 1 + encodedValue.length() + 1; // "key=value," - if (encodedMetadata != null) { - length += 1 + encodedMetadata.length(); // ";metadata" + if (metadata != null) { + length += 1 + metadata.length(); // ";metadata" } return length; } @@ -179,7 +176,9 @@ private static int extractEntries( } private static boolean baggageIsInvalid(String key, BaggageEntry baggageEntry) { - return !isValidBaggageKey(key) || !isValidBaggageValue(baggageEntry.getValue()); + return !isValidBaggageKey(key) + || !isValidBaggageValue(baggageEntry.getValue()) + || !isValidBaggageMetadata(baggageEntry.getMetadata().getValue()); } /** @@ -202,6 +201,29 @@ private static boolean isValidBaggageValue(String value) { return value != null; } + /** + * Determines whether the given {@code String} is valid W3C baggage metadata. + * + * @param metadata the metadata to be validated. + * @return whether the metadata is valid. + */ + private static boolean isValidBaggageMetadata(@Nullable String metadata) { + if (metadata == null || metadata.isEmpty()) { + return true; + } + for (int i = 0; i < metadata.length(); i++) { + if (!isValidMetadataChar(metadata.charAt(i))) { + return false; + } + } + return true; + } + + // tchar / baggage-octet / OWS / '=' / ';' + private static boolean isValidMetadataChar(char c) { + return c == '\t' || (c >= ' ' && c <= '~' && c != '"' && c != ',' && c != '\\'); + } + @Override public String toString() { return "W3CBaggagePropagator"; diff --git a/api/all/src/test/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagatorFuzzTest.java b/api/all/src/test/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagatorFuzzTest.java index 0a95e27e83c..efca289f92b 100644 --- a/api/all/src/test/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagatorFuzzTest.java +++ b/api/all/src/test/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagatorFuzzTest.java @@ -36,11 +36,12 @@ public static class TestCases { private final W3CBaggagePropagator baggagePropagator = W3CBaggagePropagator.getInstance(); @Fuzz - public void roundTripRandomValues(String baggageValue, String metadataBlob) { + public void roundTripRandomValues( + String baggageValue, @From(MetadataGenerator.class) String metadataBlob) { + // Extract trims OWS around the metadata blob; match that so round-trip compares equal. + String metadata = metadataBlob.trim(); Baggage baggage = - Baggage.builder() - .put("b", baggageValue, BaggageEntryMetadata.create(metadataBlob)) - .build(); + Baggage.builder().put("b", baggageValue, BaggageEntryMetadata.create(metadata)).build(); Map carrier = new HashMap<>(); baggagePropagator.inject(Context.root().with(baggage), carrier, Map::put); Context extractedContext = @@ -53,11 +54,10 @@ public void roundTripRandomValues(String baggageValue, String metadataBlob) { @Fuzz public void roundTripAsciiValues( @From(AsciiGenerator.class) String baggageValue, - @From(AsciiGenerator.class) String metadataBlob) { + @From(MetadataGenerator.class) String metadataBlob) { + String metadata = metadataBlob.trim(); Baggage baggage = - Baggage.builder() - .put("b", baggageValue, BaggageEntryMetadata.create(metadataBlob)) - .build(); + Baggage.builder().put("b", baggageValue, BaggageEntryMetadata.create(metadata)).build(); Map carrier = new HashMap<>(); baggagePropagator.inject(Context.root().with(baggage), carrier, Map::put); Context extractedContext = @@ -117,6 +117,28 @@ protected boolean codePointInRange(int codePoint) { } } + public static class MetadataGenerator extends AbstractStringGenerator { + + @Override + protected int nextCodePoint(SourceOfRandomness random) { + while (true) { + char c = random.nextChar(' ', '~'); + if (c != '"' && c != ',' && c != '\\') { + return c; + } + } + } + + @Override + protected boolean codePointInRange(int codePoint) { + return codePoint >= ' ' + && codePoint <= '~' + && codePoint != '"' + && codePoint != ',' + && codePoint != '\\'; + } + } + private static class MapTextMapGetter implements TextMapGetter> { @Override public Iterable keys(Map carrier) { diff --git a/api/all/src/test/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagatorTest.java b/api/all/src/test/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagatorTest.java index 00847063952..4dfca0d4b56 100644 --- a/api/all/src/test/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagatorTest.java +++ b/api/all/src/test/java/io/opentelemetry/api/baggage/propagation/W3CBaggagePropagatorTest.java @@ -476,7 +476,11 @@ static Stream extract_member_invalidPercentEncoding_preservesValidMem Arguments.argumentSet( "multiple invalid entries", "bad1=va%lue,key1=value1,bad2=value%GG,encoded=value%202,bad3=value;meta=%GG", - Baggage.builder().put("key1", "value1").put("encoded", "value 2").build())); + Baggage.builder() + .put("key1", "value1") + .put("encoded", "value 2") + .put("bad3", "value", BaggageEntryMetadata.create("meta=%GG")) + .build())); } @Test @@ -625,7 +629,88 @@ void inject() { .containsExactlyInAnyOrderEntriesOf( singletonMap( "baggage", - "meta=meta-value;somemetadata%3B%20someother%3Dfoo,needsEncoding=blah%20blah%20blah,nometa=nometa-value")); + "meta=meta-value;somemetadata; someother=foo,needsEncoding=blah%20blah%20blah,nometa=nometa-value")); + } + + @Test + void inject_doesNotPercentEncodeMetadata() { + Baggage baggage = + Baggage.builder() + .put("SomeKey", "SomeValue", BaggageEntryMetadata.create("ValueProp \t = \t PropVal")) + .build(); + Map carrier = new HashMap<>(); + W3CBaggagePropagator.getInstance().inject(Context.root().with(baggage), carrier, Map::put); + assertThat(carrier) + .containsExactlyInAnyOrderEntriesOf( + singletonMap("baggage", "SomeKey=SomeValue;ValueProp \t = \t PropVal")); + } + + @ParameterizedTest + @MethodSource + void inject_invalidMetadata_skipsEntry(String metadata) { + Baggage baggage = + Baggage.builder() + .put("keep", "yes") + .put("drop", "no", BaggageEntryMetadata.create(metadata)) + .build(); + Map carrier = new HashMap<>(); + W3CBaggagePropagator.getInstance().inject(Context.root().with(baggage), carrier, Map::put); + assertThat(carrier).containsExactlyInAnyOrderEntriesOf(singletonMap("baggage", "keep=yes")); + } + + static Stream inject_invalidMetadata_skipsEntry() { + return Stream.of( + Arguments.argumentSet("comma", "a,b"), + Arguments.argumentSet("dquote", "a\"b"), + Arguments.argumentSet("backslash", "a\\b"), + Arguments.argumentSet("cr", "a\rb"), + Arguments.argumentSet("lf", "a\nb"), + Arguments.argumentSet("nul", "a\0b"), + Arguments.argumentSet("del", "a\u007fb"), + Arguments.argumentSet("non-ascii", "café"), + Arguments.argumentSet("obs-text", "a\u0080b")); + } + + @Test + void inject_invalidMetadataOnly_omitsHeader() { + Baggage baggage = Baggage.builder().put("k", "v", BaggageEntryMetadata.create("a,b")).build(); + Map carrier = new HashMap<>(); + W3CBaggagePropagator.getInstance().inject(Context.root().with(baggage), carrier, Map::put); + assertThat(carrier).isEmpty(); + } + + @Test + void extract_metadataNotPercentDecoded() { + W3CBaggagePropagator propagator = W3CBaggagePropagator.getInstance(); + Context result = + propagator.extract( + Context.root(), + ImmutableMap.of("baggage", "SomeKey=SomeValue;ValueProp%20%09%20%3D%20%09%20PropVal"), + getter); + + assertThat(Baggage.fromContext(result)) + .isEqualTo( + Baggage.builder() + .put( + "SomeKey", + "SomeValue", + BaggageEntryMetadata.create("ValueProp%20%09%20%3D%20%09%20PropVal")) + .build()); + } + + @Test + void roundTrip_metadataPreservedOpaque() { + W3CBaggagePropagator propagator = W3CBaggagePropagator.getInstance(); + Baggage baggage = + Baggage.builder() + .put("SomeKey", "SomeValue", BaggageEntryMetadata.create("ValueProp \t = \t PropVal")) + .build(); + Map carrier = new HashMap<>(); + propagator.inject(baggage.storeInContext(Context.root()), carrier, Map::put); + assertThat(carrier.get("baggage")).isEqualTo("SomeKey=SomeValue;ValueProp \t = \t PropVal"); + + Baggage extracted = Baggage.fromContext(propagator.extract(Context.root(), carrier, getter)); + assertThat(extracted).isEqualTo(baggage); } @Test