opentelemetry: validate JSON container value types#12131
Conversation
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
📝 WalkthroughWalkthroughOpenTelemetry JSON wrapped-value parsing now validates nested container shapes and dispatches arrays and maps by their actual msgpack types. Conversion helpers reject invalid arguments, and integration coverage exercises malformed AnyValue containers. ChangesOpenTelemetry JSON validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
Signed-off-by: Eduardo Silva <eduardo@chronosphere.io>
edsiper
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1253cb3949
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (internal_type == MSGPACK_OBJECT_ARRAY && | ||
| kv_value->type != MSGPACK_OBJECT_ARRAY) { | ||
| return -2; |
There was a problem hiding this comment.
When an OTLP/JSON client sends an empty ArrayValue as {"arrayValue": {}}, the protobuf JSON mapping omits the repeated values field, so this is a valid empty array. This new check returns -2 whenever the resolved array wrapper is a map rather than an array, so the log body path no longer unwraps it as [] and instead falls back to serializing the raw wrapper map; the same wrapper is also rejected by shared AnyValue callers. Please keep the empty-map wrapper case for arrayValue and only reject scalar/non-container values payloads.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/opentelemetry/flb_opentelemetry_utils.c`:
- Around line 184-191: Update the nested-wrapper handling around
flb_otel_utils_json_payload_append_unwrapped_value() and
flb_otel_utils_json_payload_append_converted_map() so malformed wrapper shapes
rejected by the internal_type checks propagate a distinct error instead of being
treated as ordinary maps. Preserve the separate “not wrapped” result for valid
non-wrapper maps, and add a regression covering an arrayValue containing an
invalid values field that must be rejected rather than emitted verbatim.
- Around line 178-180: Update the key validation around the values-wrapper check
to require both an exact length of 6 and a case-insensitive match against
“values”; return -3 for shorter, longer, or otherwise different keys while
preserving acceptance of the exact key.
In
`@tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py`:
- Around line 854-900: Update
test_in_opentelemetry_handles_json_logs_with_invalid_any_value_container to
distinguish scalar invalid containers from the map-form kvlist case. Assert that
arrayValue and scalar kvlistValue inputs are dropped or emit no records (or
verify the relevant drop metric), while map-form kvlistValue produces the
expected emitted structure; retain the existing status and process-liveness
checks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61d61ef7-760e-42ef-bc7b-3f1f4d4aa340
📒 Files selected for processing (4)
plugins/in_opentelemetry/opentelemetry_utils.cplugins/in_opentelemetry/opentelemetry_utils.hsrc/opentelemetry/flb_opentelemetry_utils.ctests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py
💤 Files with no reviewable changes (2)
- plugins/in_opentelemetry/opentelemetry_utils.h
- plugins/in_opentelemetry/opentelemetry_utils.c
| if (strncasecmp(kv_key->ptr, "values", kv_key->size) != 0) { | ||
| return -3; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require an exact values key match.
Line 178 compares only kv_key->size bytes, so "", "v", or "value" match the prefix of "values" and are accepted as valid container wrappers.
Proposed fix
- if (strncasecmp(kv_key->ptr, "values", kv_key->size) != 0) {
+ if (kv_key->size != sizeof("values") - 1 ||
+ strncasecmp(kv_key->ptr, "values",
+ sizeof("values") - 1) != 0) {
return -3;
}📝 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.
| if (strncasecmp(kv_key->ptr, "values", kv_key->size) != 0) { | |
| return -3; | |
| } | |
| if (kv_key->size != sizeof("values") - 1 || | |
| strncasecmp(kv_key->ptr, "values", | |
| sizeof("values") - 1) != 0) { | |
| return -3; | |
| } |
🤖 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/opentelemetry/flb_opentelemetry_utils.c` around lines 178 - 180, Update
the key validation around the values-wrapper check to require both an exact
length of 6 and a case-insensitive match against “values”; return -3 for
shorter, longer, or otherwise different keys while preserving acceptance of the
exact key.
| if (internal_type == MSGPACK_OBJECT_ARRAY && | ||
| kv_value->type != MSGPACK_OBJECT_ARRAY) { | ||
| return -2; | ||
| } | ||
| else if (internal_type == MSGPACK_OBJECT_MAP && | ||
| kv_value->type != MSGPACK_OBJECT_ARRAY && | ||
| kv_value->type != MSGPACK_OBJECT_MAP) { | ||
| return -2; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Propagate malformed nested-wrapper errors instead of encoding them as maps.
When Line 186/191 rejects a nested wrapper, flb_otel_utils_json_payload_append_unwrapped_value() reduces that failure to -1; flb_otel_utils_json_payload_append_converted_map() then falls back to generic-map encoding. Thus a nested {"arrayValue":{"values":"bad"}} can be emitted verbatim instead of rejected. Preserve a distinct “not wrapped” status for ordinary maps and propagate malformed-wrapper errors; add a nested-container regression.
🤖 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/opentelemetry/flb_opentelemetry_utils.c` around lines 184 - 191, Update
the nested-wrapper handling around
flb_otel_utils_json_payload_append_unwrapped_value() and
flb_otel_utils_json_payload_append_converted_map() so malformed wrapper shapes
rejected by the internal_type checks propagate a distinct error instead of being
treated as ordinary maps. Preserve the separate “not wrapped” result for valid
non-wrapper maps, and add a regression covering an arrayValue containing an
invalid values field that must be rejected rather than emitted verbatim.
| @pytest.mark.parametrize( | ||
| "body", | ||
| [ | ||
| pytest.param( | ||
| {"arrayValue": {"values": "not-an-array"}}, | ||
| id="array-value", | ||
| ), | ||
| pytest.param( | ||
| {"kvlistValue": {"values": "not-a-kvlist"}}, | ||
| id="kvlist-value", | ||
| ), | ||
| pytest.param( | ||
| {"kvlistValue": {}}, | ||
| id="map-form-kvlist-value", | ||
| ), | ||
| ], | ||
| ) | ||
| def test_in_opentelemetry_handles_json_logs_with_invalid_any_value_container(body): | ||
| payload = { | ||
| "resourceLogs": [ | ||
| { | ||
| "scopeLogs": [ | ||
| { | ||
| "logRecords": [ | ||
| { | ||
| "body": body, | ||
| } | ||
| ], | ||
| } | ||
| ], | ||
| } | ||
| ], | ||
| } | ||
|
|
||
| service = Service("003-stdout-otlp-json.yaml") | ||
| service.start() | ||
|
|
||
| try: | ||
| response = service.send_raw_request( | ||
| "/v1/logs", | ||
| json.dumps(payload).encode("utf-8"), | ||
| content_type="application/json", | ||
| ) | ||
|
|
||
| assert response.status_code == 201 | ||
| assert service.flb.process is not None | ||
| assert service.flb.process.poll() is None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert conversion outcomes for both invalid and map-form payloads.
This only verifies 201 and worker liveness. It passes if malformed containers are serialized as ordinary maps, and it does not prove that the map-form kvlist remains correctly encoded. Assert a drop/no-emitted-record (or the relevant drop metric) for scalar containers, and assert the expected emitted structure for the map-form case.
As per coding guidelines, “Validate both success and failure paths, including invalid payloads, boundary sizes, and null or missing fields.”
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 893-893: use jsonify instead of json.dumps for JSON output
Context: json.dumps(payload)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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
`@tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py`
around lines 854 - 900, Update
test_in_opentelemetry_handles_json_logs_with_invalid_any_value_container to
distinguish scalar invalid containers from the map-form kvlist case. Assert that
arrayValue and scalar kvlistValue inputs are dropped or emit no records (or
verify the relevant drop metric), while map-form kvlistValue produces the
expected emitted structure; retain the existing status and process-liveness
checks.
Source: Coding guidelines
1 participant
Summary
arrayValueandkvlistValuecontainers before conversionvaluesfields before any MessagePack array/map union accessRoot cause
flb_otel_utils_json_payload_get_wrapped_value()assigned the semantic AnyValue type from the wrapper key, then returned the nestedvaluesobject without consistently validating its actual MessagePack type. The downstream array and kvlist conversion paths could therefore interpret a scalar object's union as a container.Related-path audit
The same missing validation affected
kvlistValue.values, so this PR fixes both container paths and adds defensive checks at all conversion entry points. The metrics and traces callers share the corrected Fluent Bit utility. The cmetrics and ctraces bundled decoders use different conversion paths and do not require changes.Compatibility
Valid OTLP/JSON arrays and kvlists continue to decode as before. Invalid scalar container payloads are rejected without terminating the worker. Existing map-form kvlists remain supported.
Validation
cmake --build build -j8— passedctest --test-dir build -R flb-it-opentelemetry --output-on-failure— passedtests/integration/.venv/bin/python -m pytest tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py::test_in_opentelemetry_handles_json_logs_with_invalid_any_value_container -q— 3 passedVALGRIND=1 VALGRIND_STRICT=1 tests/integration/.venv/bin/python -m pytest tests/integration/scenarios/in_opentelemetry/tests/test_in_opentelemetry_001.py::test_in_opentelemetry_handles_json_logs_with_invalid_any_value_container -q— 3 passedGITHUB_EVENT_NAME=pull_request GITHUB_BASE_REF=master tests/integration/.venv/bin/python .github/scripts/commit_prefix_check.py— passedSummary by CodeRabbit
Bug Fixes
Tests
AnyValuecontainer shapes.