Fix Accept header charset parameter ignored in content negotiation#5474
Open
tranhoangtu-it wants to merge 2 commits intomicrosoft:mainfrom
Open
Fix Accept header charset parameter ignored in content negotiation#5474tranhoangtu-it wants to merge 2 commits intomicrosoft:mainfrom
tranhoangtu-it wants to merge 2 commits intomicrosoft:mainfrom
Conversation
…crosoft#4676) When the Accept header includes extra parameters like charset=UTF-8 (e.g., "Accept: application/fhir+xml;charset=UTF-8"), ASP.NET Core's content negotiation may fail to match the registered output formatter because the formatter only declares the base media type without parameters. This causes fallback to JSON even when XML was requested. Explicitly set Response.ContentType when the Accept header contains non-quality parameters (charset, etc.), matching the behavior already used for the _format query parameter override.
Author
|
Required metadata labels for CI:
Thank you! |
Author
@microsoft-github-policy-service agree |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses an ASP.NET Core content-negotiation edge case where Accept header media type parameters (e.g., charset=UTF-8) can prevent matching an output formatter, causing the server to fall back to JSON instead of honoring the requested FHIR XML/JSON media type.
Changes:
- Updates
FormatParametersValidator.CheckRequestedContentTypeAsyncto track the first supportedAcceptmedia type. - When the request
Acceptheader includes non-quality parameters, explicitly setsHttpResponse.ContentTypeusingGetClosestClientMediaType(...)to force the intended formatter selection.
Comment on lines
+74
to
84
| string matchedMediaType = null; | ||
|
|
||
| foreach (MediaTypeHeaderValue acceptHeader in acceptHeaders) | ||
| { | ||
| var headerValue = acceptHeader.MediaType.ToString(); | ||
| isAcceptHeaderValid = await IsFormatSupportedAsync(headerValue); | ||
|
|
||
| if (isAcceptHeaderValid) | ||
| if (await IsFormatSupportedAsync(headerValue)) | ||
| { | ||
| matchedMediaType = headerValue; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
The selected matchedMediaType is the first supported media type in the raw Accept header order. When this method later sets Response.ContentType, it can override ASP.NET Core’s normal quality-factor (q) precedence and return a less-preferred type (e.g., xml;charset;q=0.1 listed before json;q=1.0). Consider choosing the supported Accept entry with the highest Quality (and then using header order as a tiebreaker) before forcing Response.ContentType.
Comment on lines
+92
to
+100
| // When the Accept header contains extra parameters (e.g., charset=UTF-8), | ||
| // ASP.NET Core's content negotiation may fail to match the output formatter. | ||
| // Explicitly set the content type so the correct formatter is selected. | ||
| if (acceptHeaders.Any(a => a.Parameters.Count > 0 && a.Parameters.Any(p => p.Name != "q"))) | ||
| { | ||
| string closestClientMediaType = _outputFormatters.GetClosestClientMediaType( | ||
| matchedMediaType, | ||
| acceptHeaders.Select(x => x.MediaType.Value).ToList()); | ||
|
|
There was a problem hiding this comment.
The override condition checks whether any Accept entry has non-q parameters, but the override uses matchedMediaType (which may come from a different Accept entry). This can change behavior even when the chosen/most-preferred supported media type has no extra parameters (and would have negotiated correctly). Suggest only forcing Response.ContentType when the selected (best) supported Accept entry itself contains non-quality parameters.
| // When the Accept header contains extra parameters (e.g., charset=UTF-8), | ||
| // ASP.NET Core's content negotiation may fail to match the output formatter. | ||
| // Explicitly set the content type so the correct formatter is selected. | ||
| if (acceptHeaders.Any(a => a.Parameters.Count > 0 && a.Parameters.Any(p => p.Name != "q"))) |
There was a problem hiding this comment.
p.Name != "q" is a case-sensitive comparison; parameter names are case-insensitive in HTTP, and some parsers may normalize differently. Use an ordinal-ignore-case comparison (or, if q is surfaced via MediaTypeHeaderValue.Quality, simplify the check to just a.Parameters.Count > 0).
Suggested change
| if (acceptHeaders.Any(a => a.Parameters.Count > 0 && a.Parameters.Any(p => p.Name != "q"))) | |
| if (acceptHeaders.Any(a => a.Parameters.Count > 0 && a.Parameters.Any(p => !string.Equals(p.Name, "q", StringComparison.OrdinalIgnoreCase)))) |
Comment on lines
+92
to
+105
| // When the Accept header contains extra parameters (e.g., charset=UTF-8), | ||
| // ASP.NET Core's content negotiation may fail to match the output formatter. | ||
| // Explicitly set the content type so the correct formatter is selected. | ||
| if (acceptHeaders.Any(a => a.Parameters.Count > 0 && a.Parameters.Any(p => p.Name != "q"))) | ||
| { | ||
| string closestClientMediaType = _outputFormatters.GetClosestClientMediaType( | ||
| matchedMediaType, | ||
| acceptHeaders.Select(x => x.MediaType.Value).ToList()); | ||
|
|
||
| if (!string.IsNullOrEmpty(closestClientMediaType)) | ||
| { | ||
| httpContext.Response.ContentType = closestClientMediaType; | ||
| } | ||
| } |
There was a problem hiding this comment.
This change introduces new content-negotiation behavior for Accept parameters (e.g., charset). There are existing unit tests around format negotiation/overrides, but none appear to cover Accept: application/fhir+xml;charset=UTF-8 (and mixed q+parameter cases). Add a unit test to prevent regressions and to validate q precedence is still respected when non-quality parameters are present.
LTA-Thinking
added
the
External Author
…se-insensitive q - Only check non-quality params on the MATCHED Accept entry (not any) - Use OrdinalIgnoreCase for 'q' parameter name comparison - Track matchedHeader to avoid cross-entry confusion
Author
|
Addressed all Copilot review points:
|
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.
Summary
Fixes #4676.
When the
Acceptheader includes acharsetparameter (e.g.,Accept: application/fhir+xml;charset=UTF-8), the server ignores the requested media type and falls back to JSON. This happens because ASP.NET Core's content negotiation doesn't match the output formatter when extra parameters are present, since the formatters only declare the base media type (e.g.,application/fhir+xml) without charset parameters.Per the IANA media type registration for application/fhir+xml,
charsetis a valid parameter.Changes
In
FormatParametersValidator.CheckRequestedContentTypeAsync, when the Accept header validation passes and the header contains non-quality parameters (e.g.,charset), explicitly setResponse.ContentTypeto ensure the correct output formatter is selected. This matches the existing behavior used for the_formatquery parameter override.Impact
Accept: application/fhir+xml;charset=UTF-8now correctly returns XML (was returning JSON)Accept: application/fhir+xml;q=0.8continues to work as before (quality parameters already handled)_formatquery parameter behaviorTest plan
GET /PatientwithAccept: application/fhir+xml;charset=UTF-8→ returns XMLGET /PatientwithAccept: application/fhir+xml→ returns XML (unchanged)GET /PatientwithAccept: application/fhir+json;charset=UTF-8→ returns JSONGET /PatientwithAccept: application/fhir+xml;q=0.8→ returns XML (unchanged)As a certified HL7 FHIR Implementer, this is my second contribution to the FHIR server. Also building FHIRBridge — an open-source FHIR integration toolkit.