Fix trailing element separator rejecting valid dd tracestate - #12229
Fix trailing element separator rejecting valid dd tracestate#12229mcculls wants to merge 9 commits into
Conversation
mcculls
added
type: bug fix
This comment has been minimized.
This comment has been minimized.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18d90ea4f7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR makes W3C propagation parsing more tolerant by accepting a trailing element separator in the dd= member (e.g., dd=...;) and adds a regression test to ensure the behavior is preserved.
Changes:
- Add a test case covering
dd=state values with a trailing;. - Relax parsing/validation logic to treat a trailing separator as a valid terminator.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| dd-trace-core/src/test/java/datadog/trace/core/propagation/W3CHttpExtractorTest.java | Adds coverage for dd= tracestate values that end with a trailing element separator. |
| dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java | Updates validation to stop at the separator even when it is the final character, enabling acceptance of trailing separators. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![datadog-datadog-us1-prod[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
A trailing ; is now accepted only when it is the literal final character. If valid optional whitespace follows it, the parser mistakes that whitespace for another tag key and still drops the decoded Datadog sampling, origin, and propagation tags.
🤖 Datadog Autotest · Commit 18d90ea · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
🤖 Claude (via automated review) Nice fix for the trailing-separator rejection in Repro: So an upstream producer emitting a trailing separator on the |
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>
afb3f3b to
4853d64
Compare
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.
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.
… 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.)
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java:357
- This change makes
validateCharsUntilSeparatorOrEndaccept a trailingseparatorfor all separators passed to it. If this helper is also used with key/value separators (e.g.,:), it can broaden accepted inputs (e.g.,key:with an empty value) beyond the intended relaxation (trailing element separator only). Consider adding anallowTrailingSeparatorparameter (or a second helper) so only element/tag-list separators allow trailing separators, while key/value separators keep the stricter behavior.
pos++;
if (pos < end) {
c = s.charAt(pos);
if (c == separator) {
break; // trailing separator allowed; caller resumes parsing from here
}
}
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/DatadogPTagsCodec.java:182
- Same concern as in
W3CPTagsCodec: this helper now permits trailing separators for any separator it is invoked with. If the method is reused for validating segments split by the key/value separator, this can unintentionally allow empty values. Consider scoping the relaxation to list separators only (e.g., via a boolean flag or dedicated method) to avoid widening the accepted grammar more than required.
pos++;
if (pos < end) {
c = s.charAt(pos);
if (c == separator) {
break; // trailing separator allowed; caller resumes parsing from here
}
}
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java:359
- The updated validation/parsing behavior (including the trailing-separator relaxation) appears duplicated across
W3CPTagsCodecandDatadogPTagsCodec. To reduce the risk of the two codecs drifting over time, consider extracting the common validation routine(s) into a shared utility (or a small internal base/helper) with codec-specific configuration for separators/allowed-char predicates.
pos++;
if (pos < end) {
c = s.charAt(pos);
if (c == separator) {
break; // trailing separator allowed; caller resumes parsing from here
}
}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d11ce6a2e9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…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.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Suppressed comments (2)
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java:139
- This change introduces a new
warnlog path for interior OWS cases, which can be triggered by untrusted inbound headers and potentially cause warning-log noise at scale. Consider reducing the level (e.g.,debug) and/or applying rate-limiting/sampling to this specific message, while still returningempty(...)for correctness.
int afterOWC = skipLeadingOWC(value, nextTagPos, ddMemberValueEnd);
if (afterOWC > nextTagPos && afterOWC < ddMemberValueEnd) {
// OWS was skipped but real content still follows - interior OWS, not trailing padding
log.warn("Invalid datadog tags header value: '{}' at {}", value, nextTagPos);
return empty(tagsFactory, value, firstMemberStart, ddMemberStart, ddMemberValueEnd);
}
dd-trace-core/src/main/java/datadog/trace/core/propagation/ptags/W3CPTagsCodec.java:650
- Using assignment inside the
whilecondition makes the loop harder to read and debug. Consider movingelementStart = skipLeadingOWC(...)to the end of the previous iteration (or to the top of the loop body with an earlybreak) so the loop condition remains a simple boolean expression.
while ((elementStart = skipLeadingOWC(original, elementStart, w3CPTags.ddMemberValueEnd))
< w3CPTags.ddMemberValueEnd
&& size < MAX_HEADER_SIZE) {
c8fbd7e to
b5bbd60
Compare
Summary
;at the end of the W3C tracestateddmember value was rejected as invalid, dropping the entireddmember (and its decoded tags/priority/origin) instead of just ignoring the harmless trailing separator.;at the end, as the preceding content is still parseable.W3CPTagsCodec.validateCharsUntilSeparatorOrEndto always terminate on the element separator, regardless of position, matching the intent of the surrounding parser. The still-invalid case of a trailing:(key with no value) remains rejected via an existing downstream check.DatadogPTagsCodec(the_dd.p.*header codec), so_dd.p.dm=-4,is accepted the same way.;(element separator) fromisAllowedKeyCharin the W3C codec, and=/,(TAG_KEY_SEPARATOR/TAGS_SEPARATOR) in the Datadog codec, so an empty element (e.g.dd=s:2;;o:some) is rejected instead of the separator being silently swallowed into the next key.ddsection (whitespace between submembers that isn't trailing padding, e.g.dd=s:0;t.dm:934086a686-4; t.x:y) is now correctly rejected rather than silently accepted.Test plan
W3CHttpExtractorTest,W3CPropagationTagsTest, andDatadogPropagationTagsTestcases covering: trailing element separator, trailing separator + OWS (space/tab), trailing separator + OWS before the next list-member comma, empty/leading element separators (rejected), interior OWS padding (rejected), and trailing separator on the Datadog_dd.p.*codec../gradlew :dd-trace-core:test --tests "datadog.trace.core.propagation.*"passes.🤖 Generated with Claude Code