Repository navigation
[release/17.0] Return 400 for OTLP JSON requests with malformed ids - #22131
Closed
Sébastien Ros (sebastienros) wants to merge 1 commit into
Closed
Sébastien Ros (sebastienros) wants to merge 1 commit into
Sébastien Ros (sebastienros) wants to merge 1 commit into
Conversation
## Description The OTLP HTTP **JSON** ingestion path returns an unhandled **500** (instead of **400**) when a request contains a syntactically valid JSON body with a malformed field value, e.g. a `traceId`/`spanId` that is not valid hex, or an invalid base64 `bytesValue`. `MessageBindable<TMessage>.BindAsync` (`src/Aspire.Dashboard/Otlp/Http/OtlpHttpEndpointsBuilder.cs`) only catches `JsonException` and `BadHttpRequestException`. The JSON → protobuf conversion in `OtlpJsonProtobufConverter` runs **after** the JSON has parsed successfully; converting a bad hex id throws `FormatException` (`byte.Parse`) or `ArgumentException` (odd length), and `ByteString.FromBase64` throws `FormatException`. These escape the bind handler and surface as a 500. A malformed id is a bad request, so it should be a 400 like the existing invalid-JSON case. ## Fix Catch `FormatException`/`ArgumentException` in the JSON bind case and return `400 Bad Request`, mirroring the existing `JsonException` handling (debug-logged, no 500). ## Tests Added `CallService_Traces_JsonContentType_InvalidTraceId_BadRequest` to `OtlpHttpJsonTests`, covering a non-hex id (`FormatException` path) and an odd-length id (`ArgumentException` path). Both assert `400`. Without the fix they return `500`. (cherry picked from commit 9a926d8)
Sébastien Ros (sebastienros)
requested review from
James Newton-King (JamesNK) and
Adam Ratzman (adamint)
as code owners
November 11, 2026 04:30
This was referenced Nov 11, 2026
James Newton-King (JamesNK)
approved these changes
Nov 11, 2026
Adam Ratzman (adamint)
approved these changes
Nov 11, 2026
Member
Jose Perez Rodriguez (joperezr)
added a commit
that referenced
this pull request
Nov 12, 2026
Combines the stacked dashboard backports into one PR for `release/17.0`. The commits are cherry-picked in stack order, one per source PR, onto the current `release/17.0` tip: 1. #22125: Store dashboard tags, resource order and metric pins in browser local storage (#21840) 2. #22126: Add dashboard resource-selector dropdown (#21861) 3. #22127: Rename the WithTags trait from Aspire_Dashboard_Tag to Aspire_Tag (#21881) 4. #22128: Restore single-line compact rows in the dashboard resource list (#21880) 5. #22129: Allocate the next dashboard run ID when two dashboards start in the same millisecond (#21913) 6. #22130: Simplify the dashboard around its two main jobs (#21943) 7. #22131: Return 400 for OTLP JSON requests with malformed ids (#21436) 8. #22132: Address deferred review feedback for the modern dashboard (#21766) ## Conflict resolution The stack was cut from an older `release/17.0`. Since then, #22123 (Harden Go SDK runtime) landed and regenerated `TwoPassScanningGeneratedAspire.verified.go`. That conflicted with #21766, which replaces `WithDashboardPosition` with `WithDisplayOrder`. The resolution drops `WithDashboardPosition` and regenerates the snapshot with the current Go generator, so `WithDisplayOrder` now uses the hardened `s.Err()` pattern. The other seven commits applied cleanly. ## Validation All `Aspire.Hosting.CodeGeneration.*.Tests` projects pass locally (Go, Java, Python, Rust, TypeScript). --------- Co-authored-by: Sébastien Ros <1165805+sebastienros@users.noreply.github.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Co-authored-by: James Newton-King <james@newtonking.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: David Fowler <davidfowl@gmail.com> Co-authored-by: davidfowl <95136+davidfowl@users.noreply.github.com> Co-authored-by: Maddy Montaquila <maddyleger1@gmail.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com> Co-authored-by: Maddy Montaquila <maddy@MadBook-Pro-20.local> Co-authored-by: Taha Koçal <tahakocalgs@gmail.com> Co-authored-by: Jose Perez Rodriguez <joperezr@microsoft.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
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.
Backport of #21436 to release/17.0
/cc Sébastien Ros (@sebastienros)
Note
Part of a chain of dashboard backports. This PR targets the branch of #22130, so review only its own commit. Merge the chain in order; GitHub retargets each PR as its base merges.
Customer Impact
Testing
Risk
Regression?