Skip to content

Commit 3f581ec

Browse files
mccullsmcculls
authored
Fix trailing element separator rejecting valid dd tracestate (#12229)
Fix trailing element separator rejecting valid dd tracestate A trailing ';' at the end of the W3C tracestate 'dd' member value was incorrectly rejected as invalid, dropping the entire dd member and any decoded tags, priority, or origin instead of ignoring the separator. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Apply same fix to DatadogPTagsCodec Fix OWS before comma after trailing separator, align OWC helpers Skipping to the next comma when a trailing separator is followed by OWS (e.g. "dd=s:2;o:some; ,x=y") left the parser resuming on a space, which isAllowedKeyChar rejects and drops the whole dd member. We add skipLeadingOWC to consume that OWS, and restyle stripTrailingOWC to the same while-loop shape. Reject ';' as a dd tracestate key character isAllowedKeyChar didn't exclude the element separator, so a key scan starting right after an empty element (e.g. "dd=s:2;;o:some") would swallow the next ';' into the key instead of failing, silently dropping the origin tag rather than rejecting the malformed member. Datadog's sibling codec already excludes its element separator from key chars; align W3C's the same way. Flagged by Copilot review on PR #12229. Exclude TAG_KEY_SEPARATOR alongside TAGS_SEPARATOR, mirroring the W3C codec's isAllowedKeyChar. Note TAG_KEY_SEPARATOR is already caught by the separator check in validateCharsUntilSeparatorOrEnd, so this doesn't change behavior, but keeps the predicate decoupled from how it's used by the caller. (the similar KEY_VALUE_SEPARATOR check in the W3C codec is also a no-op given how it's used, but is also worth keeping in case the calling code changes.) Avoid exception when unknown dd tag is followed by trailing-OWS-only content cleanUpAndAppendUnknown re-scans the raw tracestate to re-append unknown dd submembers, assuming every element is at least 2 chars long so it can peek a second character to detect known s/o tags. Accepting a trailing separator followed only by OWS (e.g. "dd=x:y; ") breaks that assumption. We skip leading OWS before each element the same way the value scanner already does, so a trailing whitespace-only remainder is recognized as padding and the loop exits cleanly. Disallow interior OWS padding inside 'dd' section Ignore empty elements inside dd tracestate Drop malformed elements from dd tracestate, but retain valid elements (unless we're sure it's a bad tid/dm/ts value in which case it's safer to drop the whole section) Ensure last parent tag (p:) is not duplicated when updating Still relay elements we couldn't parse, as downstream tracers might handle them Co-authored-by: stuart.mcculloch <stuart.mcculloch@datadoghq.com>
1 parent 2d8d93d commit 3f581ec

5 files changed

Lines changed: 171 additions & 65 deletions

File tree

‎dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/DatadogPTagsCodec.java‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -176,10 +176,8 @@ private static int validateCharsUntilSeparatorOrEnd(
176176
pos++;
177177
if (pos < end) {
178178
c = s.charAt(pos);
179-
// It's not allowed to have the separator as the last character so only check
180-
// if there is something after the separator
181-
if (pos < end - 1 && c == separator) {
182-
break;
179+
if (c == separator) {
180+
break; // trailing separator allowed; caller resumes parsing from here
183181
}
184182
}
185183
} while (pos < end);
@@ -189,7 +187,10 @@ private static int validateCharsUntilSeparatorOrEnd(
189187

190188
private static boolean isAllowedKeyChar(int c) {
191189
// space (MIN_ALLOWED_CHAR) is not allowed
192-
return c > MIN_ALLOWED_CHAR && c <= MAX_ALLOWED_CHAR && c != TAGS_SEPARATOR;
190+
return c > MIN_ALLOWED_CHAR
191+
&& c <= MAX_ALLOWED_CHAR
192+
&& c != TAG_KEY_SEPARATOR
193+
&& c != TAGS_SEPARATOR;
193194
}
194195

195196
private static boolean isAllowedValueChar(int c) {

‎dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java‎

Lines changed: 58 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,10 @@ PropagationTags fromHeaderValue(PTagsFactory tagsFactory, String value) {
100100
CharSequence lastParentId = null;
101101
TagValue orgPropagationMarkerTagValue = null;
102102
while (tagPos < ddMemberValueEnd) {
103+
tagPos = skipEmptyElements(value, tagPos, ddMemberValueEnd);
104+
if (tagPos >= ddMemberValueEnd) {
105+
break;
106+
}
103107
int tagKeyEndsAt =
104108
validateCharsUntilSeparatorOrEnd(
105109
value,
@@ -108,10 +112,11 @@ PropagationTags fromHeaderValue(PTagsFactory tagsFactory, String value) {
108112
KEY_VALUE_SEPARATOR,
109113
false,
110114
W3CPTagsCodec::isAllowedKeyChar);
111-
if (tagKeyEndsAt < 0 || tagKeyEndsAt == ddMemberValueEnd) {
112-
log.warn("Invalid datadog tags header value: '{}' at {}", value, tagPos);
113-
// TODO drop parts?
114-
return empty(tagsFactory, value, firstMemberStart, ddMemberStart, ddMemberValueEnd);
115+
if (tagKeyEndsAt < 0 || tagKeyEndsAt >= ddMemberValueEnd) {
116+
int nextTagPos = skipMalformedElement(value, tagPos, ddMemberValueEnd);
117+
maxUnknownSize += (nextTagPos - tagPos); // still relay malformed elements
118+
tagPos = nextTagPos;
119+
continue;
115120
}
116121
int tagValuePos = tagKeyEndsAt + 1;
117122
int tagValueEndsAt =
@@ -123,9 +128,10 @@ PropagationTags fromHeaderValue(PTagsFactory tagsFactory, String value) {
123128
true,
124129
W3CPTagsCodec::isAllowedValueChar);
125130
if (tagValueEndsAt < 0) {
126-
log.warn("Invalid datadog tags header value: '{}' at {}", value, tagKeyEndsAt);
127-
// TODO drop parts?
128-
return empty(tagsFactory, value, firstMemberStart, ddMemberStart, ddMemberValueEnd);
131+
int nextTagPos = skipMalformedElement(value, tagValuePos, ddMemberValueEnd);
132+
maxUnknownSize += (nextTagPos - tagPos); // still relay malformed elements
133+
tagPos = nextTagPos;
134+
continue;
129135
}
130136
int nextTagPos = tagValueEndsAt + 1;
131137
if (tagValueEndsAt == ddMemberValueEnd) {
@@ -152,7 +158,6 @@ PropagationTags fromHeaderValue(PTagsFactory tagsFactory, String value) {
152158
if (tagKey.equals(TRACE_ID_TAG)) {
153159
return tagsFactory.createInvalid(PROPAGATION_ERROR_MALFORMED_TID + tagValue);
154160
}
155-
// TODO drop parts?
156161
return empty(tagsFactory, value, firstMemberStart, ddMemberStart, ddMemberValueEnd);
157162
}
158163
if (tagKey.equals(DECISION_MAKER_TAG)) {
@@ -351,10 +356,8 @@ private static int validateCharsUntilSeparatorOrEnd(
351356
pos++;
352357
if (pos < end) {
353358
c = s.charAt(pos);
354-
// It's not allowed to have the separator as the last character so only check
355-
// if there is something after the separator
356-
if (pos < end - 1 && c == separator) {
357-
break;
359+
if (c == separator) {
360+
break; // trailing separator allowed; caller resumes parsing from here
358361
}
359362
}
360363
} while (pos < end);
@@ -365,7 +368,10 @@ private static int validateCharsUntilSeparatorOrEnd(
365368
private static boolean isAllowedKeyChar(int c) {
366369
// We already know that the segments have been validated against the valid chars for
367370
// the general tracestate header
368-
return c > MIN_ALLOWED_CHAR && c <= MAX_ALLOWED_CHAR && c != KEY_VALUE_SEPARATOR;
371+
return c > MIN_ALLOWED_CHAR
372+
&& c <= MAX_ALLOWED_CHAR
373+
&& c != KEY_VALUE_SEPARATOR
374+
&& c != ELEMENT_SEPARATOR;
369375
}
370376

371377
private static boolean isAllowedValueChar(int c) {
@@ -613,12 +619,37 @@ private static boolean isOWC(char c) {
613619
return c == ' ' || c == '\t';
614620
}
615621

616-
private static int stripTrailingOWC(String original, int start, int end) {
617-
char c = original.charAt(--end);
618-
while (isOWC(c) && end > start) {
619-
c = original.charAt(--end);
622+
private static int stripTrailingOWC(String value, int start, int end) {
623+
while (end > start + 1 && isOWC(value.charAt(end - 1))) {
624+
end--;
620625
}
621-
return ++end;
626+
return end;
627+
}
628+
629+
private static int skipMalformedElement(String value, int start, int end) {
630+
log.warn(
631+
"Invalid datadog tags header value: '{}' dropping malformed element at {}", value, start);
632+
int pos = start;
633+
while (pos < end) {
634+
char c = value.charAt(pos++);
635+
if (c == ELEMENT_SEPARATOR) {
636+
return pos;
637+
}
638+
}
639+
return end;
640+
}
641+
642+
private static int skipEmptyElements(String value, int start, int end) {
643+
int pos = start;
644+
while (pos < end) {
645+
char c = value.charAt(pos++);
646+
if (c == ELEMENT_SEPARATOR) {
647+
start = pos;
648+
} else if (!isOWC(c)) {
649+
return start;
650+
}
651+
}
652+
return end;
622653
}
623654

624655
private static int cleanUpAndAppendUnknown(StringBuilder sb, W3CPTags w3CPTags, int size) {
@@ -631,16 +662,21 @@ private static int cleanUpAndAppendUnknown(StringBuilder sb, W3CPTags w3CPTags,
631662
int elementStart = w3CPTags.ddMemberStart + EMPTY_SIZE; // skip over 'dd='
632663
int okSize = size;
633664
while (elementStart < w3CPTags.ddMemberValueEnd && size < MAX_HEADER_SIZE) {
665+
elementStart = skipEmptyElements(original, elementStart, w3CPTags.ddMemberValueEnd);
666+
if (elementStart == w3CPTags.ddMemberValueEnd) {
667+
break;
668+
}
634669
okSize = size;
635670
int elementEnd = original.indexOf(ELEMENT_SEPARATOR, elementStart);
636-
if (elementEnd < 0) {
671+
if (elementEnd < 0 || elementEnd > w3CPTags.ddMemberValueEnd) {
637672
elementEnd = w3CPTags.ddMemberValueEnd;
638673
}
639674
if (!original.startsWith(Encoding.W3C.getPrefix(), elementStart)) {
640675
char first = original.charAt(elementStart);
641-
char second = original.charAt(elementStart + 1);
642-
if (second != KEY_VALUE_SEPARATOR || (first != 'o' && first != 's')) {
643-
// only append elements that we don't know about or are not tags
676+
// ignore known o:, s:, and p: elements because we always add them back in appendPrefix
677+
if ((first != 'o' && first != 's' && first != 'p')
678+
|| elementStart + 1 >= elementEnd
679+
|| original.charAt(elementStart + 1) != KEY_VALUE_SEPARATOR) {
644680
if (sb.length() > EMPTY_SIZE) {
645681
sb.append(ELEMENT_SEPARATOR);
646682
size++;

‎dd-trace-core/src/test/java/datadog/trace/core/propagation/DatadogPropagationTagsTest.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ class DatadogPropagationTagsTest extends DDJavaSpecification {
2727
"valid dm tag 2-digit | '_dd.p.dm=934086a686-10' | '_dd.p.dm=934086a686-10' | [_dd.p.dm: '934086a686-10'] ",
2828
"valid dm tag 3-digit | '_dd.p.dm=934086a686-102' | '_dd.p.dm=934086a686-102' | [_dd.p.dm: '934086a686-102'] ",
2929
"dm tag minus only | '_dd.p.dm=-1' | '_dd.p.dm=-1' | [_dd.p.dm: '-1'] ",
30+
"dm tag with trailing separator | '_dd.p.dm=-4,' | '_dd.p.dm=-4' | [_dd.p.dm: '-4'] ",
3031
"any p tag | '_dd.p.anytag=value' | '_dd.p.anytag=value' | [_dd.p.anytag: 'value'] ",
3132
"non p tag dropped | '_dd.b.somekey=value' | | [:] ",
3233
"upstream services alone dropped | '_dd.p.upstream_services=bWNudWx0eS13ZWI|0|1|0.1' | | [:] ",

‎dd-trace-core/src/test/java/datadog/trace/core/propagation/W3CHttpExtractorTest.java‎

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -108,15 +108,22 @@ void checkMaxFromW3CTraceIds(@ConvertWith(TraceIdTestConverter.class) DDTraceId
108108
}
109109

110110
@TableTest({
111-
"scenario | traceparent | tracestate | priority | decisionMaker | origin",
112-
"keep empty state | '00-00000000000000000000000000000001-123456789abcdef0-01' | '' | SAMPLER_KEEP | SamplingMechanism.DEFAULT | ",
113-
"drop empty state | '00-00000000000000000000000000000001-123456789abcdef0-00' | '' | SAMPLER_DROP | | ",
114-
"keep with user keep state | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:2;o:some' | USER_KEEP | | some ",
115-
"keep with user keep state and manual dm | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:2;o:some;t.dm:-4' | USER_KEEP | SamplingMechanism.MANUAL | some ",
116-
"drop with user keep state and manual dm | '00-00000000000000000000000000000001-123456789abcdef0-00' | 'dd=s:2;o:some;t.dm:-4' | SAMPLER_DROP | | some ",
117-
"drop with user drop state | '00-00000000000000000000000000000001-123456789abcdef0-00' | 'dd=s:-1;o:some' | USER_DROP | | some ",
118-
"drop with user drop state and manual dm | '00-00000000000000000000000000000001-123456789abcdef0-00' | 'dd=s:-1;o:some;t.dm:-4' | USER_DROP | SamplingMechanism.MANUAL | some ",
119-
"keep overrides user drop state with manual dm | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:-1;o:some;t.dm:-4' | SAMPLER_KEEP | SamplingMechanism.DEFAULT | some "
111+
"scenario | traceparent | tracestate | priority | decisionMaker | origin",
112+
"keep empty state | '00-00000000000000000000000000000001-123456789abcdef0-01' | '' | SAMPLER_KEEP | SamplingMechanism.DEFAULT | ",
113+
"drop empty state | '00-00000000000000000000000000000001-123456789abcdef0-00' | '' | SAMPLER_DROP | | ",
114+
"keep with user keep state | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:2;o:some' | USER_KEEP | | some ",
115+
"keep with trailing element separator | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:2;o:some;' | USER_KEEP | | some ",
116+
"keep with trailing separator and OWS | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:2;o:some; \t' | USER_KEEP | | some ",
117+
"keep with trailing separator OWS before comma | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:2;o:some; ,x=y' | USER_KEEP | | some ",
118+
"skip empty element in middle | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:2;;o:some' | USER_KEEP | | some ",
119+
"skip leading separator | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=;s:2;o:some' | USER_KEEP | | some ",
120+
"skip bare element in middle | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:2;flag;o:some' | USER_KEEP | | some ",
121+
"skip multiple bare elements in middle | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:2;flag1;flag2;o:some' | USER_KEEP | | some ",
122+
"keep with user keep state and manual dm | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:2;o:some;t.dm:-4' | USER_KEEP | SamplingMechanism.MANUAL | some ",
123+
"drop with user keep state and manual dm | '00-00000000000000000000000000000001-123456789abcdef0-00' | 'dd=s:2;o:some;t.dm:-4' | SAMPLER_DROP | | some ",
124+
"drop with user drop state | '00-00000000000000000000000000000001-123456789abcdef0-00' | 'dd=s:-1;o:some' | USER_DROP | | some ",
125+
"drop with user drop state and manual dm | '00-00000000000000000000000000000001-123456789abcdef0-00' | 'dd=s:-1;o:some;t.dm:-4' | USER_DROP | SamplingMechanism.MANUAL | some ",
126+
"keep overrides user drop state with manual dm | '00-00000000000000000000000000000001-123456789abcdef0-01' | 'dd=s:-1;o:some;t.dm:-4' | SAMPLER_KEEP | SamplingMechanism.DEFAULT | some "
120127
})
121128
void extractTraceparentTracestateAndHttpHeaders(
122129
String traceparent,

0 commit comments

Comments
 (0)