fix: validate traceparent before using trace context#1897
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
Walkthrough
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/helpers/context-header.ts`:
- Around line 14-20: The version field validation in the isValidTraceContext
logic currently accepts the reserved value `ff`, which violates the W3C Trace
Context specification. Add an additional condition to the isValidTraceContext
validation that explicitly checks version !== 'ff' to reject this reserved
version value, ensuring invalid trace metadata is not emitted downstream.
In `@src/helpers/context-header.unit.test.ts`:
- Around line 40-49: The test matrix in the it.each block for invalid
traceparent validation is missing a regression case for the reserved version
"ff", which means strict version validation isn't fully tested. Add a new
traceparent string to the test array that uses "ff" as the version prefix (the
first segment before the first dash) while keeping the other segments valid,
such as "ff-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01", to ensure the
reserved-version validation is properly covered by the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89b497dc-b473-459c-87c3-db43e1c32658
📒 Files selected for processing (2)
src/helpers/context-header.tssrc/helpers/context-header.unit.test.ts
| const isValidTraceContext = | ||
| /^[0-9a-f]{2}$/.test(version) && | ||
| /^[0-9a-f]{32}$/.test(traceId) && | ||
| traceId !== '00000000000000000000000000000000' && | ||
| /^[0-9a-f]{16}$/.test(spanId) && | ||
| spanId !== '0000000000000000' && | ||
| /^[0-9a-f]{2}$/.test(flags); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In the W3C Trace Context traceparent specification, is version value "ff" explicitly invalid/reserved?
💡 Result:
Yes, in the W3C Trace Context traceparent specification, the version value "ff" is explicitly defined as invalid [1][2][3]. The specification states that the version field is an 8-bit unsigned integer serialized as two hexadecimal characters, and specifically reserves "ff" (decimal 255) as an invalid value [1][2][4]. This constraint is formally documented within the header format specification, which includes the ABNF note: "; this document assumes version 00. Version ff is forbidden" [1][3][5].
Citations:
- 1: https://github.com/w3c/trace-context/blob/main/spec/20-http_request_header_format.md
- 2: https://www.w3.org/TR/trace-context-2/
- 3: https://www.w3.org/TR/trace-context/
- 4: https://www.w3.org/TR/trace-context-1/
- 5: https://www.w3.org/TR/2023/CR-trace-context-2-20230418/
Reject reserved traceparent version ff.
The version field currently accepts ff, but the W3C Trace Context specification explicitly forbids this value. Without this check, invalid trace metadata will be emitted downstream.
Suggested patch
const isValidTraceContext =
/^[0-9a-f]{2}$/.test(version) &&
+ version !== 'ff' &&
/^[0-9a-f]{32}$/.test(traceId) &&
traceId !== '00000000000000000000000000000000' &&
/^[0-9a-f]{16}$/.test(spanId) &&
spanId !== '0000000000000000' &&
/^[0-9a-f]{2}$/.test(flags);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isValidTraceContext = | |
| /^[0-9a-f]{2}$/.test(version) && | |
| /^[0-9a-f]{32}$/.test(traceId) && | |
| traceId !== '00000000000000000000000000000000' && | |
| /^[0-9a-f]{16}$/.test(spanId) && | |
| spanId !== '0000000000000000' && | |
| /^[0-9a-f]{2}$/.test(flags); | |
| const isValidTraceContext = | |
| /^[0-9a-f]{2}$/.test(version) && | |
| version !== 'ff' && | |
| /^[0-9a-f]{32}$/.test(traceId) && | |
| traceId !== '00000000000000000000000000000000' && | |
| /^[0-9a-f]{16}$/.test(spanId) && | |
| spanId !== '0000000000000000' && | |
| /^[0-9a-f]{2}$/.test(flags); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/helpers/context-header.ts` around lines 14 - 20, The version field
validation in the isValidTraceContext logic currently accepts the reserved value
`ff`, which violates the W3C Trace Context specification. Add an additional
condition to the isValidTraceContext validation that explicitly checks version
!== 'ff' to reject this reserved version value, ensuring invalid trace metadata
is not emitted downstream.
There was a problem hiding this comment.
This comment from Rabbit looks valid / worth addressing
There was a problem hiding this comment.
@sagzy, glad we're on the same page! The fix is straightforward — just add version !== 'ff' after the regex check in src/helpers/context-header.ts:
const isValidTraceContext =
/^[0-9a-f]{2}$/.test(version) &&
+ version !== 'ff' &&
/^[0-9a-f]{32}$/.test(traceId) &&
traceId !== '00000000000000000000000000000000' &&
/^[0-9a-f]{16}$/.test(spanId) &&
spanId !== '0000000000000000' &&
/^[0-9a-f]{2}$/.test(flags);Don't forget to add a corresponding test case for version === 'ff' in src/helpers/context-header.unit.test.ts as well.
| it.each([ | ||
| '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7', | ||
| '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01-extra', | ||
| 'zz-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01', | ||
| '00-xyz-00f067aa0ba902b7-01', | ||
| '00-00000000000000000000000000000000-00f067aa0ba902b7-01', | ||
| '00-4bf92f3577b34da6a3ce929d0e0e4736-xyz-01', | ||
| '00-4bf92f3577b34da6a3ce929d0e0e4736-0000000000000000-01', | ||
| '00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-zz', | ||
| ])('returns null fields for invalid traceparent %s', (traceparent) => { |
There was a problem hiding this comment.
Add a regression case for invalid version ff.
The invalid-input matrix is missing the reserved-version case, so the strict-validation objective isn’t fully pinned by tests.
Suggested patch
it.each([
'00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7',
'00-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01-extra',
+ 'ff-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01',
'zz-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01',
'00-xyz-00f067aa0ba902b7-01',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/helpers/context-header.unit.test.ts` around lines 40 - 49, The test
matrix in the it.each block for invalid traceparent validation is missing a
regression case for the reserved version "ff", which means strict version
validation isn't fully tested. Add a new traceparent string to the test array
that uses "ff" as the version prefix (the first segment before the first dash)
while keeping the other segments valid, such as
"ff-4bf92f3577b34da6a3ce929d0e0e4736-00f067aa0ba902b7-01", to ensure the
reserved-version validation is properly covered by the test.
sagzy
force-pushed
the
fix-validate-trace-context
branch
from
6e6e3c8 to
b630a49
Compare
| /^[0-9a-f]{32}$/.test(traceId) && | ||
| traceId !== '00000000000000000000000000000000' && | ||
| /^[0-9a-f]{16}$/.test(spanId) && | ||
| spanId !== '0000000000000000' && |
There was a problem hiding this comment.
nit: the 32/16 zeros are hard to read, should we simplify with
!/^0+$/.test(traceId) &&
!/^0+$/.test(spanId)
as the preceding regex already checks for hex of the right length?
| const isValidTraceContext = | ||
| /^[0-9a-f]{2}$/.test(version) && | ||
| /^[0-9a-f]{32}$/.test(traceId) && | ||
| traceId !== '00000000000000000000000000000000' && | ||
| /^[0-9a-f]{16}$/.test(spanId) && | ||
| spanId !== '0000000000000000' && | ||
| /^[0-9a-f]{2}$/.test(flags); |
There was a problem hiding this comment.
This comment from Rabbit looks valid / worth addressing
| const [_version, traceId, spanId, flags] = parts; | ||
| const [version, traceId, spanId, flags] = parts; | ||
|
|
||
| const isValidTraceContext = |
There was a problem hiding this comment.
nit: would be worth adding an inline comment here to explain why we're doing this
e.g. "Reject malformed/spoofed traceparent so we don't emit junk trace IDs to Cloud Logging"
sagzy
force-pushed
the
fix-validate-trace-context
branch
from
b630a49 to
48e4a13
Compare
|
Hey @miloquinn, thanks for your contribution 🙏 I've left some minor comments, this looks good overall! Can you ping me once you've gone through addressing those? |
sagzy
force-pushed
the
fix-validate-trace-context
branch
from
48e4a13 to
04545c1
Compare
2 participants
Summary
traceparentfields before returning Cloud Logging trace context.Testing
corepack yarn vitest run --dir src --reporter=dot src/helpers/context-header.unit.test.tscorepack yarn _test:typescorepack yarn lint src/helpers/context-header.ts src/helpers/context-header.unit.test.ts