fix(common): skip unknown TLV tags instead of failing#181
Merged
michaelbeutler merged 7 commits intotruvami:mainfrom Mar 3, 2026
Merged
fix(common): skip unknown TLV tags instead of failing#181michaelbeutler merged 7 commits intotruvami:mainfrom
michaelbeutler merged 7 commits intotruvami:mainfrom
Conversation
The TLV parser returned a hard error on unknown tags, which broke forward-compatibility when new config settings were added to payloads. Unknown tags are now skipped using their length field, with a warning logged via slog. Closes truvami#180
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe TLV parser in Changes
Sequence Diagram(s)(No diagrams generated — changes adjust parser error handling and metric emission without adding new multi-component control flow that requires visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 the current code and only fix it if needed.
Inline comments:
In `@pkg/common/helpers.go`:
- Around line 205-206: The TLV parsing loop uses uint8 for index and
payloadLength causing overflow in the guard (index+2<payloadLength) and when
doing index += length; change the types of index, payloadLength (and any
intermediate length variable) from uint8 to int in the parsing function so
arithmetic is done in signed integers, update the loop condition to use int math
(e.g., index+2 < payloadLength), and before advancing index (where the code does
index += length) validate that length is non-negative and that index+int(length)
<= payloadLength (or < depending on semantics) to prevent reads past
payloadBytes; also check bounds before accessing payloadBytes[index] and on
subsequent slices to return a parse error instead of panicking.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
pkg/common/helpers.gopkg/common/helpers_test.gopkg/decoder/tagxl/v1/decoder_test.go
Validates that known tags still decode correctly when an unknown tag appears in-between, complementing the existing unknown-only test.
JanickFischer
commented
Mar 2, 2026
The TLV parsing loop declared index and payloadLength as uint8, which could wrap around on large payloads and cause out-of-bounds access. Also adds a bounds check before advancing index past a TLV value. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/helpers_test.go`:
- Around line 144-150: The table-driven tests in pkg/common/helpers_test.go use
a shared error assertion that can silently pass for the new TLV cases; update
the assertion to explicitly compare the returned error against each case's
expectedErr (e.g., tc.expectedErr) instead of a generic check so failures
surface correctly, and apply the same change for the other occurrence noted (the
assertion around the second block mentioned). Locate the test loop (references
to payload, config, expected, expectedErr) and replace the generic error check
at the shared assertion sites with a conditional that asserts
equality/containment with tc.expectedErr and only proceeds to compare
tc.expected when expectedErr is nil.
The error assertions in TestDecode, TestExtractFieldValue, TestConvertFieldValue, and TestInsertFieldBytes would silently pass when expectedErr was set but no error was returned. Now each loop explicitly fails on missing expected errors and on unexpected errors. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
michaelbeutler
requested changes
Mar 3, 2026
Contributor
michaelbeutler
left a comment
michaelbeutler
left a comment
There was a problem hiding this comment.
I would also suggest to create a new metric for this so we can track and alert.
// pkg/common/metrics.go
package common
import (
"github.com/prometheus/client_golang/prometheus"
"github.com/prometheus/client_golang/prometheus/promauto"
)
var (
unknownTLVTagsTotal = promauto.NewCounterVec(prometheus.CounterOpts{
Name: "truvami_common_unknown_tlv_tags_total",
Help: "The total number of unknown TLV tags encountered during decoding",
}, []string{"tag"})
)Then you can call like this:
unknownTLVTagsTotal.WithLabelValues(fmt.Sprintf("0x%02x", tag)).Inc()Replace slog with logger.Logger (zap) and add a truvami_common_unknown_tlv_tags_total counter metric to track unknown TLV tags encountered during decoding. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
michaelbeutler
requested changes
Mar 3, 2026
Contributor
michaelbeutler
left a comment
michaelbeutler
left a comment
There was a problem hiding this comment.
Please fix lint and CI checks.
Satisfies staticcheck QF1011: omit type int from declaration as it is inferred from the right-hand side. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/common/helpers.go`:
- Around line 159-169: The loop currently uses "index+2 < payloadLength" which
silently skips the case when exactly 2 bytes remain or when 1 byte remains;
change the parsing loop so it iterates while index < payloadLength and
explicitly check that at least 2 bytes remain before reading a TLV header (e.g.
if payloadLength-index < 2 return an error about incomplete TLV header), then
read tag/length from payloadBytes using index and proceed to validate the value
length as you already do; update any error messages to reference tag, offset
(index-2 or current index), and remaining bytes consistently (symbols:
payloadBytes, index, payloadLength).
Codecov Report❌ Patch coverage is
... and 4 files with indirect coverage changes 🚀 New features to boost your workflow:
|
The loop condition `index+2 < payloadLength` required 3 bytes remaining, silently dropping trailing bytes when only 1 or 2 remained. Changed to iterate while `index < payloadLength` with an explicit 2-byte header check that returns a descriptive error. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The TLV parser returned a hard error on unknown tags, which broke forward-compatibility when new config settings were added to payloads. Unknown tags are now skipped using their length field, with a warning logged via slog. Closes #180
Summary by CodeRabbit
Bug Fixes
Tests
Chores