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

\n in form model when using Jetty 12 client and multipart/form-data #31361

Closed
mhalbritter opened this issue Oct 4, 2023 · 6 comments
Closed
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@mhalbritter
Copy link
Contributor

Hello,

I'm using Jetty 12.0.1 and observe the control character \r in my populated model attribute. With Jetty I have this controller:

@RestController
public class FormSubmissionController {

	@PostMapping(path = "/", consumes = MediaType.MULTIPART_FORM_DATA_VALUE, produces = MediaType.APPLICATION_JSON_VALUE)
	public OutputForm greetingSubmit(@ModelAttribute InputForm form) {
		return new OutputForm(form.firstName, form.lastName);
	}

	public record InputForm(String firstName, String lastName) {
	}

	public record OutputForm(String firstName, String lastName) {
	}

}

and i expect this test to pass:

@SpringBootTest(webEnvironment =  SpringBootTest.WebEnvironment.RANDOM_PORT)
class JettyFormSubmissionApplicationTests {

	@Autowired
	private TestRestTemplate testRestTemplate;

	@Test
	void test() {
		MultiValueMap<String, String> formData = new LinkedMultiValueMap<>();
		formData.add("firstName", "first-name");
		formData.add("lastName", "last-name");

		HttpHeaders headers = new HttpHeaders();
		headers.add("Content-Type", MediaType.MULTIPART_FORM_DATA_VALUE);
		RequestEntity<?> requestEntity = new RequestEntity<>(formData, headers, HttpMethod.POST, URI.create("/"));
		ResponseEntity<FormSubmissionController.OutputForm> result = this.testRestTemplate.exchange(requestEntity, FormSubmissionController.OutputForm.class);
		assertThat(result.getBody().firstName()).isEqualTo("first-name");
		assertThat(result.getBody().lastName()).isEqualTo("last-name");
	}
}

This fails with Jetty 12, giving me this error message:

org.opentest4j.AssertionFailedError: 
expected: "last-name"
 but was: "
last-name"

With Jetty 11.0.15, it works.

I have prepared a small reproducer. You can switch back to Jetty 11 by editing the build.gradle, i've put some comments in.

jetty-form-submission.zip

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 4, 2023
@rstoyanchev
Copy link
Contributor

This is likely down to Jetty as we don't parse ourselves but rather access request params and parts. Do those look okay if accessed directly through the HttpServletRequest?

@mhalbritter
Copy link
Contributor Author

I get the same failure if I use the getParts() API from HttpServletRequest. Looks like a Jetty bug to me then:

	@PostMapping(path = "/", consumes = MediaType.MULTIPART_FORM_DATA_VALUE, produces = MediaType.APPLICATION_JSON_VALUE)
	public OutputForm greetingSubmit(HttpServletRequest servletRequest, @ModelAttribute InputForm form) throws ServletException, IOException {
		Map<String, String> parts = new HashMap<>();
		for (Part part : servletRequest.getParts()) {
			try (InputStream inputStream = part.getInputStream()) {
				String content = new String(inputStream.readAllBytes(), StandardCharsets.UTF_8);
				parts.put(part.getName(), content);
			}
		}
		return new OutputForm(parts.get("firstName"), parts.get("lastName"));
	}

@mhalbritter mhalbritter closed this as not planned Won't fix, can't repro, duplicate, stale Oct 20, 2023
@mhalbritter
Copy link
Contributor Author

mhalbritter commented Oct 20, 2023

Hm. It looks like it's related to the sender side. I have this code:

        MultiValueMap<String, String> formData = new LinkedMultiValueMap<>();
        formData.add("firstName", "first-name");
        formData.add("lastName", "last-name");
        HttpHeaders headers = new HttpHeaders();
        headers.add("Content-Type", MediaType.MULTIPART_FORM_DATA_VALUE);
        RequestEntity<?> requestEntity = new RequestEntity<>(formData, headers, HttpMethod.POST, URI.create("/"));
        ResponseEntity<FormSubmissionController.OutputForm> result = this.testRestTemplate.exchange(requestEntity, FormSubmissionController.OutputForm.class);
        assertThat(result.getStatusCode()).isEqualTo(HttpStatusCode.valueOf(200));
        assertThat(result.getBody().firstName()).isEqualTo("first-name");
        assertThat(result.getBody().lastName()).isEqualTo("last-name");

When the RestTemplate is using JettyClientHttpRequestFactory it fails. If it's using CustomHttpComponentsClientHttpRequestFactory (or JdkClientHttpRequestFactory, or SimpleClientHttpRequestFactory), it works. It works with curl, too:

curl -v -F firstName=First -F lastName=Last http://localhost:8080

@mhalbritter mhalbritter reopened this Oct 20, 2023
@mhalbritter mhalbritter changed the title Control characters in form model when using Jetty 12 and multipart/form-data Control characters in form model when using Jetty 12 client and multipart/form-data Oct 20, 2023
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Oct 20, 2023

Thanks for the further investigation. Part writing happens in FormHttpMessageConverter and is the same for any client, and has been in place for a very long time (hasn't changed recently). If the issue is reproducible with direct use of Jetty HttpClient, then it is for the Jetty issue tracker. It could be that some of the changes in jetty/jetty.project#9076 are related to this.

@poutsma poutsma self-assigned this Nov 7, 2023
poutsma added a commit that referenced this issue Nov 7, 2023
@poutsma
Copy link
Contributor

poutsma commented Nov 7, 2023

I did some investigation of this, and—in short—still don't understand what's going on. I'm pretty sure that it is a Jetty 12 client-side bug, because of the reasons mentioned by @rstoyanchev (i.e. the FormHttpMessageConverter has not changed, and is used by all clients). I have added some tests (0839f5b) to verify the behavior of the FormHttpMessageConverter when used with a RestClient, and reproduce the behavior of the sample app, but that seems to work fine, also for Jetty.

I even went as far as to see what is exactly posted by your JettyFormSubmissionApplicationTests, by running the test against netcat, dumping the posted bytes in a file, and comparing the bytes using a hex viewer. The results look good.

The only thing that I can think of is that it's off-by-one error, because the byte that precedes last-name is \n. Somehow, this character either gets duplicated, sent twice, or mixed up.

@poutsma poutsma closed this as completed in 486503b Nov 7, 2023
@poutsma
Copy link
Contributor

poutsma commented Nov 7, 2023

I fixed the issue, by buffering OutputStream.write calls with a BufferedOutputStream.

By looking at the dump from netcat, I determined that each write to Jetty's OutputStreamContentSource results in a separate HTTP chunk written. The FormHttpMessageConverter does many writes, some even as short as 1 byte, resulting in a lot of HTTP chunks. My guess is that these chunks collided somewhere along the wire. By buffering the content into a BufferedOutputStream, we ensure that Jetty only writes a new chunk every 1024 bytes.

@poutsma poutsma added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Nov 7, 2023
@poutsma poutsma added this to the 6.1.0 milestone Nov 7, 2023
@poutsma poutsma changed the title Control characters in form model when using Jetty 12 client and multipart/form-data \n in form model when using Jetty 12 client and multipart/form-data Nov 7, 2023
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) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants