opentelemetry-java
opentelemetry-java copied to clipboard
open-telemetry
ImmutableBaggage.isKeyValid allows non-zero-length strings that contain nothing but spaces and/or tabs
It currently is:
private static boolean isKeyValid(String name) { return name != null && !name.isEmpty() && StringUtils.isPrintableString(name); }
It needs to be:
private static boolean isKeyValid(String name) { return name != null && !name.trim().isEmpty() && StringUtils.isPrintableString(name); }
Or it needs to use something like apache StringUtils isBlank.
@jkwatson I think this should be clarified in the specification because we want to have a consistent behavior across libraries
@jkwatson I think this should be clarified in the specification because we want to have a consistent behavior across libraries
Isn't key validity defined by the w3c spec already?
https://www.w3.org/TR/baggage/#key
"Leading and trailing whitespaces (OWS) are allowed but MUST be trimmed when converting the header into a data structure."
If you trim leading and trailing whitespace on a string consisting of nothing but whitespace, you get an empty string.
So !name.trim().isEmpty() is correct, and the current code is not correct.
If we apply the w3c rules then we need to trim the keys always, for example " too" should overwrite "too". I want to clarify in the specs that we follow w3c rules which I think we don't say at the moment
The proposed change in my first post in this bug report was to change !name.isEmpty() to !name.trim().isEmpty(), which always trims the keys.
@bogdandrutu Can you shepherd a tweak to the specs so we can get #2950 merged?
Has this issue been resolved? If not I would like to take a stab at it.
Has this issue been resolved? If not I would like to take a stab at it.
cc: @alolita
I think we're still waiting on the specification issue to get resolved.
Is it still open? If yes, we can try this way.
private static boolean isKeyValid(String name) { // Trim leading and trailing whitespace name = name.trim();
// Check if the name is valid (non-empty after trimming) return !name.isEmpty(); }
Someone will need to validate what the specification currently says about baggage keys and create a PR if we're not currently in compliance.
As per this excerpt, the specification does not say anything about requirements for a key except for it being a string.
REQUIRED parameters:
Name The name for which to set the value, of type string.
