Bug #89350: media type validation #1462
Conversation
|
|
shivakumar-ms
requested review from
bcarthic,
alishahahmed,
wsugarman,
shivakumar-ms and
jovinson-ms
src/Microsoft.Health.Dicom.Api/Features/Filters/AcceptContentFilterAttribute.cs
Outdated
Show resolved
Hide resolved
src/Microsoft.Health.Dicom.Api.UnitTests/Features/Filters/AcceptContentFilterAttributeTests.cs
Outdated
Show resolved
Hide resolved
|
As discussed, the Dicom standard requires the As an example, this is not a valid Accept header: Enforcing this constraint allows us to leverage the built in header parsing libraries in ASP.NET Core and greatly simplify our code. |
…ving allowSingle and allowMultiple, as allowSingle was always true and allowMultiple was always false.
|
Committed code for updated PR scope. New code simplifies AcceptContentFilterAttribute so it no longer handles multipart Accept headers as it was never used in the codebase. |
|
I am surprised none of our E2E tests are broken. Should we add a test? |
|
That's a good idea. I can put them under Microsoft.Health.Dicom.Web.Tests.E2E -> Rest -> AcceptHeaderTests.cs. Should only need a couple E2E tests to validate everything past the updated unit tests. EDIT: After standup discussion, adding E2E test is not necessary. |
|
|
||
| private readonly HashSet<MediaTypeHeaderValue> _mediaTypes; | ||
|
|
||
| public AcceptContentFilterAttribute(string[] mediaTypes, bool allowSingle, bool allowMultiple) | ||
| public AcceptContentFilterAttribute(string[] mediaTypes) |
There was a problem hiding this comment.
Can we confirm this change also works down stream? There are places like dicom-server\src\Microsoft.Health.Dicom.Api\Extensions\MediaTypeHeaderValueExtensions.cs, where the acceptHeader gets converted to internal objects and passed into service, is the type with and without quotes passed in as same value downstream?
There was a problem hiding this comment.
The service currently uses two code paths to handle media type content negotiation - AcceptContentFilterAttribute is used only for single part media types like application/dicom (example), and the GetAcceptHeaders extension method is only used on controller actions that use the Produces attribute for multipart/related media types (example), and then parses to find the media type from the type parameter.
Once we realized that the AcceptContentFilterAttribute doesn't handle multipart/related, we decided to explicitly handle only single part media types there, and consider a future refactor to handle multipart media type content negotiation at the API layer as well.
|
|
||
| foreach (MediaTypeHeaderValue acceptHeader in acceptHeaders) | ||
| { | ||
| if (_mediaTypes.Any(x => x.MatchesMediaType(acceptHeader.MediaType))) |
There was a problem hiding this comment.
As discussed, we will flip this check - the defined media types for this action should be a subset of the Accept header media range, e.g. application/dicom is a subset of application/* and so will match.
…pt header, like application/*. Also removed a now-redundant E2E test.
Description
PS3.18 (nema.org):
"An mtp-value can be transmitted either as a token or quoted-string. The quoted and unquoted values are equivalent."
Currently, WADO with
Accept: multipart/related; type=application/dicomwill throw an error. It will acceptAccept: multipart/related; type="application/dicom", but they should be equivalent.##Changes
Altered AcceptContentFilterAttribute.cs to account for media type values with missing quotes such as "multipart/related; type=application/dicom" instead of the usual "multipart/related; type='application/dicom'".
Addresses
#1396 and AB#89350
Testing
Added new tests to AcceptContentFilterAttributeTests.cs.