Skip to content

Commit 6fa097a

Browse files
mccullsmcculls
committed
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
1 parent 80b5061 commit 6fa097a

3 files changed

Lines changed: 127 additions & 107 deletions

File tree

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

Lines changed: 36 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -101,13 +101,9 @@ PropagationTags fromHeaderValue(PTagsFactory tagsFactory, String value) {
101101
TagValue orgPropagationMarkerTagValue = null;
102102
while (tagPos < ddMemberValueEnd) {
103103
tagPos = skipEmptyElements(value, tagPos, ddMemberValueEnd);
104-
if (tagPos == ddMemberValueEnd) {
104+
if (tagPos >= ddMemberValueEnd) {
105105
break;
106106
}
107-
if (isOWC(value.charAt(tagPos))) {
108-
log.warn("Invalid datadog tags header value: '{}' at {}", value, tagPos);
109-
return empty(tagsFactory, value, firstMemberStart, ddMemberStart, ddMemberValueEnd);
110-
}
111107
int tagKeyEndsAt =
112108
validateCharsUntilSeparatorOrEnd(
113109
value,
@@ -116,10 +112,9 @@ PropagationTags fromHeaderValue(PTagsFactory tagsFactory, String value) {
116112
KEY_VALUE_SEPARATOR,
117113
false,
118114
W3CPTagsCodec::isAllowedKeyChar);
119-
if (tagKeyEndsAt < 0 || tagKeyEndsAt == ddMemberValueEnd) {
120-
log.warn("Invalid datadog tags header value: '{}' at {}", value, tagPos);
121-
// TODO drop parts?
122-
return empty(tagsFactory, value, firstMemberStart, ddMemberStart, ddMemberValueEnd);
115+
if (tagKeyEndsAt < 0 || tagKeyEndsAt >= ddMemberValueEnd) {
116+
tagPos = skipMalformedElement(value, tagPos, ddMemberValueEnd);
117+
continue;
123118
}
124119
int tagValuePos = tagKeyEndsAt + 1;
125120
int tagValueEndsAt =
@@ -130,10 +125,9 @@ PropagationTags fromHeaderValue(PTagsFactory tagsFactory, String value) {
130125
ELEMENT_SEPARATOR,
131126
true,
132127
W3CPTagsCodec::isAllowedValueChar);
133-
if (tagValueEndsAt < 0) {
134-
log.warn("Invalid datadog tags header value: '{}' at {}", value, tagKeyEndsAt);
135-
// TODO drop parts?
136-
return empty(tagsFactory, value, firstMemberStart, ddMemberStart, ddMemberValueEnd);
128+
if (tagValueEndsAt < 0 || tagValueEndsAt > ddMemberValueEnd) {
129+
tagPos = skipMalformedElement(value, tagValuePos, ddMemberValueEnd);
130+
continue;
137131
}
138132
int nextTagPos = tagValueEndsAt + 1;
139133
if (tagValueEndsAt == ddMemberValueEnd) {
@@ -160,7 +154,6 @@ PropagationTags fromHeaderValue(PTagsFactory tagsFactory, String value) {
160154
if (tagKey.equals(TRACE_ID_TAG)) {
161155
return tagsFactory.createInvalid(PROPAGATION_ERROR_MALFORMED_TID + tagValue);
162156
}
163-
// TODO drop parts?
164157
return empty(tagsFactory, value, firstMemberStart, ddMemberStart, ddMemberValueEnd);
165158
}
166159
if (tagKey.equals(DECISION_MAKER_TAG)) {
@@ -629,6 +622,13 @@ private static int stripTrailingOWC(String value, int start, int end) {
629622
return end;
630623
}
631624

625+
private static int skipMalformedElement(String value, int start, int end) {
626+
log.warn(
627+
"Invalid datadog tags header value: '{}' dropping malformed element at {}", value, start);
628+
int elementEnd = value.indexOf(ELEMENT_SEPARATOR, start);
629+
return (elementEnd < 0 || elementEnd >= end) ? end : elementEnd + 1;
630+
}
631+
632632
private static int skipEmptyElements(String value, int start, int end) {
633633
int pos = start;
634634
while (pos < end) {
@@ -662,20 +662,29 @@ private static int cleanUpAndAppendUnknown(StringBuilder sb, W3CPTags w3CPTags,
662662
elementEnd = w3CPTags.ddMemberValueEnd;
663663
}
664664
if (!original.startsWith(Encoding.W3C.getPrefix(), elementStart)) {
665-
char first = original.charAt(elementStart);
666-
char second = original.charAt(elementStart + 1);
667-
if (second != KEY_VALUE_SEPARATOR || (first != 'o' && first != 's')) {
668-
// only append elements that we don't know about or are not tags
669-
if (sb.length() > EMPTY_SIZE) {
670-
sb.append(ELEMENT_SEPARATOR);
671-
size++;
672-
}
673-
int end = elementEnd;
674-
if (end == w3CPTags.ddMemberValueEnd) {
675-
end = stripTrailingOWC(original, elementStart, end);
665+
int tagKeyEndsAt =
666+
validateCharsUntilSeparatorOrEnd(
667+
original,
668+
elementStart,
669+
elementEnd,
670+
KEY_VALUE_SEPARATOR,
671+
false,
672+
W3CPTagsCodec::isAllowedKeyChar);
673+
if (tagKeyEndsAt > elementStart && tagKeyEndsAt < elementEnd) {
674+
char first = original.charAt(elementStart);
675+
// ignore known o:, s:, and p: elements because we always add them back in appendPrefix
676+
if ((first != 'o' && first != 's' && first != 'p') || tagKeyEndsAt - elementStart != 1) {
677+
if (sb.length() > EMPTY_SIZE) {
678+
sb.append(ELEMENT_SEPARATOR);
679+
size++;
680+
}
681+
int end = elementEnd;
682+
if (end == w3CPTags.ddMemberValueEnd) {
683+
end = stripTrailingOWC(original, elementStart, end);
684+
}
685+
sb.append(original, elementStart, end);
686+
size += (end - elementStart);
676687
}
677-
sb.append(original, elementStart, end);
678-
size += (end - elementStart);
679688
}
680689
}
681690
elementStart = elementEnd + 1;

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

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -108,20 +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 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-
"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 ",
121-
"drop with user keep state and manual dm | '00-00000000000000000000000000000001-123456789abcdef0-00' | 'dd=s:2;o:some;t.dm:-4' | SAMPLER_DROP | | some ",
122-
"drop with user drop state | '00-00000000000000000000000000000001-123456789abcdef0-00' | 'dd=s:-1;o:some' | USER_DROP | | some ",
123-
"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 ",
124-
"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 "
125127
})
126128
void extractTraceparentTracestateAndHttpHeaders(
127129
String traceparent,

0 commit comments

Comments
 (0)