Closes #354 - Ignores B3 headers if invalid values are provided #355
Conversation
| @@ -60,8 +60,8 @@ public void inject(SpanContext spanContext, TextMap carrier) { | |||
|
|
|||
| @Override | |||
| public SpanContext extract(TextMap carrier) { | |||
| Long traceId = null; | |||
| Long spanId = null; | |||
| long traceId = -1; | |||
There was a problem hiding this comment.
HexCodec.lowerHexToUnsignedLong("ffffffffffffffffffffffffffffffff") returns -1
There was a problem hiding this comment.
Why does a method named lowerHexToUnsignedLong returns a signed long as a valid response? O_O
Signed-off-by: Tomasz Adamski <tadamski@wikia-inc.com>
ced4349 to
b84f4a6
Compare
|
@pavolloffay , I changed the PR to use Could you please re-review and merge? Looks like the debug flag is safe, but I couldn't find where the baggage might be vulnerable (see #354 (comment)) |
Codecov Report
@@ Coverage Diff @@
## master #355 +/- ##
============================================
+ Coverage 84.5% 84.83% +0.32%
- Complexity 590 597 +7
============================================
Files 92 92
Lines 2388 2387 -1
Branches 272 272
============================================
+ Hits 2018 2025 +7
+ Misses 270 266 -4
+ Partials 100 96 -4
Continue to review full report at Codecov.
|
| assertEquals(HexCodec.lowerHexToUnsignedLong(lower64Bits), context.getTraceId()); | ||
| assertEquals(HexCodec.lowerHexToUnsignedLong(lower64Bits), context.getSpanId()); | ||
| assertNotNull(HexCodec.lowerHexToUnsignedLong(lower64Bits)); | ||
| assertEquals((long) HexCodec.lowerHexToUnsignedLong(lower64Bits), context.getTraceId()); |
There was a problem hiding this comment.
nit there is Long.longValue()
| import static org.junit.Assert.assertNotNull; | ||
| import static org.junit.Assert.assertNull; | ||
| import static org.junit.Assert.assertThat; | ||
| import static org.mockito.ArgumentMatchers.isNull; |
There was a problem hiding this comment.
nit unused and the above too
…rovided Signed-off-by: Juraci Paixão Kröhling <juraci@kroehling.de>
b84f4a6 to
0847ca9
Compare
There was a problem hiding this comment.
It covers test cases I've prepared but I am still concerned about bugs we didn't detect yet. I would sleep better if there would be some try-catch-all block that fallbacks to "no context" in case of failure.
It can be also implemented as a decorator for composability.
| */ | ||
| static long lowerHexToUnsignedLong(String lowerHex) { | ||
| static Long lowerHexToUnsignedLong(String lowerHex) { |
There was a problem hiding this comment.
Generally, Optional would be more obvious. However I understand that some like to "optimise" code with nulls
There was a problem hiding this comment.
I agree, this tradeoff was made to maintain Java 6 compatibility without adding/writing an optional back port.
There was a problem hiding this comment.
I get it. I didn't expect to have different lang level in test and main. Makes sense ;)
| maliciousCarrier.put(B3TextMapCodec.PARENT_SPAN_ID_NAME, validInput); | ||
|
|
||
| // everything is valid, except for one of the headers at a time: | ||
| for (String header : Arrays.asList( |
There was a problem hiding this comment.
for test case failure readability it should be implemented as second test method parameter + cartesian product in @DataProvidrer
|
Why is this a good change? There's another PR #359 that catches and logs all exceptions in the codecs. I think it's better to log an error saying that the inbound trace context was malformed than to silently ignore it, which makes it much harder for the user to figure out what's going on. |
No description provided.