CVE-2020-36319: inherit the default behaviour of spring for internal object mapper by haijian-vaadin · Pull Request #8051 · vaadin/flow

Reviewable status: 3 unresolved discussions, 0 of 1 LGTMs obtained, and 1 stale (waiting on @knoobie)

flow-server/src/main/java/com/vaadin/flow/server/connect/VaadinConnectController.java, line 164 at r1 (raw file):

Previously, knoobie (Knoobie) wrote…

    private ObjectMapper createVaadinConnectObjectMapper(ApplicationContext context) {

• I would rename the get to create, because we create a new instance
• I would rename the method to show that we create the default ObjectMapper for VaadinConnect, not the default ObjectMapper

Done.

flow-server/src/main/java/com/vaadin/flow/server/connect/VaadinConnectController.java, line 165 at r1 (raw file):

Previously, knoobie (Knoobie) wrote…

Edit: This could be all wrong - let me check it… just saw that it is a static method call.

Edit2: It is working like this (it inherits in my test spring’s config) - this is surprising… but I it feels wrong. I would still be in favor ofcontext.getBean(Jackson2ObjectMapperBuilder.class) - just in case.

This won’t work Because you create a completely new instance again (this time from the Jackson2ObjectMapperBuilder) - so all spring related defaults are gone again.

This can be fixed in two ways:

• add Jackson2ObjectMapperBuilder to the constructor of the class and in this method as argument
• context.getBean(Jackson2ObjectMapperBuilder.class) could be used to get the current impl of spring.

See here for more information what is configured: https://github.com/spring-projects/spring-boot/blob/master/spring-boot-project/spring-boot-autoconfigure/src/main/java/org/springframework/boot/autoconfigure/jackson/JacksonAutoConfiguration.java#L172

hmm, I think I would prefer to static way since it’s a static method. Otherwise, I get code warnings.

flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerTest.java, line 822 at r1 (raw file):

Previously, knoobie (Knoobie) wrote…

Not blocking, just my personal opinion: I feel this needs a lot more tests.

• no mocking - a real test with Spring / Spring Boot’s Applicationcontext to verify that multiple instances are created with different qualifier (default and vaadin) - example: Can be done with ApplicationContextRunner
• a test that verifies that the bug described by the user is fixed
• my comment in the last PR: #8016 (comment)

Done. Now added tests for the reported cases.