fix: enforce w3c trace context value validation #777
Conversation
validate length and characters of the w3c trace context value. see https://www.w3.org/TR/trace-context/#traceparent-header-field-values refactor unit testing to bring invalid w3c trace context tests to separate file Fixes #774
google-cla
bot
added
the
cla: yes
product-auto-label
bot
added
the
api: logging
google-cloud-logging/src/main/java/com/google/cloud/logging/Context.java
Outdated
Show resolved
Hide resolved
| throw new IllegalArgumentException("Not supporting versionFormat other than \"00\""); | ||
| } | ||
| } else { | ||
| Matcher validator = W3C_TRACE_CONTEXT_FORMAT.matcher(traceParent.toLowerCase()); |
There was a problem hiding this comment.
I'm not sure if toLowerCase() is needed. The spec says "Vendors MUST ignore the traceparent when the parent-id is invalid (for example, if it contains non-lowercase hex characters)."
There was a problem hiding this comment.
I do not know whether we should be so strict about it. A chance that two different trace ids will be produced that differ only by a specific letter register seems low to me. But this way we can avoid exceptions that interrupt the metadata collection in the library.
There was a problem hiding this comment.
Should we use then toUpperCase() to be safe?
There was a problem hiding this comment.
My vote would be to just ignore any traceparent headers with invalid characters as the spec suggests, that upper-case "MUST" in the docs seems pretty serious haha.
My interpretation is that header parsing is meant to be farily strict, we're not supposed to alter the fields to make the data fit our expectations. But I'll leave the decision up to you.
There was a problem hiding this comment.
I agree with you about the standard. My concern is that ignoring it (or throwing and catching exception on the same account) would be very hard to troubleshoot for application developers. To be a little more liberal on this might prove to be closer to the spirit of the shared library.
change length of the span field from 12 to 16 according to the standard
…logging into minherz/fix-774
enforce w3c trace context value validation (#777) (0150655) java: add -ntp flag to native image testing command (#1299) (#780) (3f70b62) Rename LogDestinationName.getId() to LogDestinationName.getDestinationId() (#797) (62e7838) Rename staleness.critical config parameter to staleness.extraold (#781) (3083bca) Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
Validates length and characters of the w3c trace context value. See W3C standard for more details.
Refactor unit testing to bring invalid w3c trace context tests to separate file.
Fixes #774