Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HeaderContentNegotiationStrategy.resolveMediaTypes() throws unexpected InvalidMimeTypeException #32483

Closed
sbrannen opened this issue Mar 18, 2024 · 0 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Milestone

Comments

@sbrannen
Copy link
Member

sbrannen commented Mar 18, 2024

Overview

In #31769 (comment), @jandroalvarez brought it to our attention that the fix for #31254 resulted in an InvalidMimeTypeException being thrown by MimeTypeUtils.sortBySpecificity() instead of an IllegalArgumentException.

However, InvalidMimeTypeException extends IllegalArgumentException.

Consequently, the change from IllegalArgumentException to InvalidMimeTypeException did not result in the desired effect in HeaderContentNegotiationStrategy.resolveMediaTypes.

HeaderContentNegotiationStrategy.resolveMediaTypes() still allows the InvalidMimeTypeException to propagate as-is without wrapping it in an HttpMediaTypeNotAcceptableException.

We should therefore catch both InvalidMediaTypeException and InvalidMimeTypeException in HeaderContentNegotiationStrategy.resolveMediaTypes() and wrap the exception in an HttpMediaTypeNotAcceptableException.

Proposed Fix and Test

catch (InvalidMediaTypeException | InvalidMimeTypeException ex) in HeaderContentNegotiationStrategy.resolveMediaTypes().

@Test
void resolveMediaTypesWithTooManyElements() throws Exception {
	String acceptHeaderValue = "text/plain,".repeat(50);
	this.servletRequest.addHeader("Accept", acceptHeaderValue);
	assertThat(this.strategy.resolveMediaTypes(this.webRequest)).hasSize(50);

	acceptHeaderValue = "text/plain,".repeat(51);
	this.servletRequest.addHeader("Accept", acceptHeaderValue);
	assertThatExceptionOfType(HttpMediaTypeNotAcceptableException.class)
			.isThrownBy(() -> this.strategy.resolveMediaTypes(this.webRequest))
			.withMessageStartingWith("Could not parse 'Accept' header")
			.withMessageEndingWith("Too many elements");
}
@sbrannen sbrannen added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug labels Mar 18, 2024
@sbrannen sbrannen added this to the 6.1.6 milestone Mar 18, 2024
@sbrannen sbrannen self-assigned this Mar 18, 2024
@sbrannen sbrannen added the for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x label Mar 18, 2024
@github-actions github-actions bot added status: backported An issue that has been backported to maintenance branches and removed for: backport-to-6.0.x Marks an issue as a candidate for backport to 6.0.x labels Mar 18, 2024
sbrannen added a commit that referenced this issue Mar 19, 2024
The fix for #31254 resulted in an InvalidMimeTypeException being thrown
by MimeTypeUtils.sortBySpecificity() instead of an
IllegalArgumentException. However, InvalidMimeTypeException extends
IllegalArgumentException. Consequently, the change from
IllegalArgumentException to InvalidMimeTypeException did not result in
the desired effect in HeaderContentNegotiationStrategy.

HeaderContentNegotiationStrategy.resolveMediaTypes() still allows the
InvalidMimeTypeException to propagate as-is without wrapping it in an
HttpMediaTypeNotAcceptableException.

To address this issue, this commit catches InvalidMediaTypeException
and InvalidMimeTypeException in HeaderContentNegotiationStrategy and
wraps the exception in an HttpMediaTypeNotAcceptableException.

See gh-31254
See gh-31769
Closes gh-32483

(cherry picked from commit ef02f0b)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) status: backported An issue that has been backported to maintenance branches type: bug A general bug
Projects
None yet
Development

No branches or pull requests

1 participant