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

[Security Fix]Use Vaadin's own ObjectMapper instead of the one from Spring #8016

Merged
merged 8 commits into from
Apr 8, 2020

Conversation

haijian-vaadin
Copy link
Contributor

@haijian-vaadin haijian-vaadin commented Apr 7, 2020

Fixes #8010


This change is Reviewable

*/
@Bean
@Qualifier(VAADIN_ENDPOINT_MAPPER_BEAN_QUALIFIER)
ObjectMapper objectMapper(ApplicationContext context) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ObjectMapper objectMapper(ApplicationContext context) {
public ObjectMapper vaadinEndpointMapper(ApplicationContext context) {

Change method name to reflect the Bean that's going to be created.

Optional/Personal opinion: I would wrap the ObjectMapper in an object VaadinConnectMapper#getObjectMapper and return VaadinConnectMapper here as Bean, so it's easy to differentiate between the normal ObjectMapper and the Object Vaadin is using internally.

}
return objectMapper;
} catch (Exception e) {
throw new IllegalStateException(String.format(

This comment was marked as outdated.

@@ -136,15 +133,13 @@
* The servlet context for the controller.
*/
public VaadinConnectController(
@Autowired(required = false) @Qualifier(VAADIN_ENDPOINT_MAPPER_BEAN_QUALIFIER) ObjectMapper vaadinEndpointMapper,
@Autowired @Qualifier(VAADIN_ENDPOINT_MAPPER_BEAN_QUALIFIER) ObjectMapper vaadinEndpointMapper,

This comment was marked as outdated.

Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, knoobie (Knoobie) wrote…

Autowired could be removed - every instance in the constructor is autowired.

This has to be updated, if my comment from above is accepted.

Done.


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

Previously, knoobie (Knoobie) wrote…
    public ObjectMapper vaadinEndpointMapper(ApplicationContext context) {

Change method name to reflect the Bean that's going to be created.

Optional/Personal opinion: I would wrap the ObjectMapper in an object VaadinConnectMapper#getObjectMapper and return VaadinConnectMapper here as Bean, so it's easy to differentiate between the normal ObjectMapper and the Object Vaadin is using internally.

Thanks for the good suggestion. I think updating the method name would be enough for now.

Any specific reasons to open the method as public? Would package-private be better since it's intended for internal use?


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

Previously, knoobie (Knoobie) wrote…

The whole try/catch could be removed?

Done. Yes, good suggestion, it was a copy-paste, now it can be removed.

Copy link
Contributor

@knoobie knoobie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 4 files at r2.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @haijian-vaadin and @knoobie)


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

Previously, haijian-vaadin (Haijian Wang) wrote…

Thanks for the good suggestion. I think updating the method name would be enough for now.

Any specific reasons to open the method as public? Would package-private be better since it's intended for internal use?

I aligned the modifier with all other modifier on @Beans in this class. I would always prefer a public modifier combined with @ConditionalOnMissingBean if you are allowed to use it.


try{
String result = objectMapper.writeValueAsString(new Entity());
assertEquals("{\"name\":\"Bond\"}", result);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was thinking about this.. is this the right way to test it? I didn't have to time to look into VaadinConnect, so I'm not sure if this could break any existing usecases you wanted to handle.

What I would expect from the test:

  • Asking the ApplicationContext for two beans (normal ObjectMapper and the qualified ObjectMapper)
  • Normal ObjectMapper should have all configuration provided by the user
  • Qualified ObjectMapper should have all configuration Vaadin needs to work
  • Both ObjectMapper serialize the same object.. depending on the configuration of the normal ObjectMapper both serialized strings should not look the same. (for example one could use Getter and the other properties for Serialization)

Or am I missing something?

Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Previously, knoobie (Knoobie) wrote…

I aligned the modifier with all other modifier on @Beans in this class. I would always prefer a public modifier combined with @ConditionalOnMissingBean if you are allowed to use it.

Good point. I updated the modifier to align with others. I just checked the code, @ConditionalOnMissingBean is not used atm, so skip it for now.


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 41 at r3 (raw file):

Previously, knoobie (Knoobie) wrote…

Was thinking about this.. is this the right way to test it? I didn't have to time to look into VaadinConnect, so I'm not sure if this could break any existing usecases you wanted to handle.

What I would expect from the test:

  • Asking the ApplicationContext for two beans (normal ObjectMapper and the qualified ObjectMapper)
  • Normal ObjectMapper should have all configuration provided by the user
  • Qualified ObjectMapper should have all configuration Vaadin needs to work
  • Both ObjectMapper serialize the same object.. depending on the configuration of the normal ObjectMapper both serialized strings should not look the same. (for example one could use Getter and the other properties for Serialization)

Or am I missing something?

Good catch and thanks a lot for the good suggestion, that would be a proper test for the change. This unit test is not new but rather moved from the VaadinConnectControllerTest, since the logic is moved from there to VaaadinConnectControllerConfiguration.

In the original test, it verifies if the mocked object mapper does not call the setVisibility() if a jacksonProperties exists. Now since I cannot use the mock any more, I borrowed the example attached in the ticket to check the final result.

Basically, I wanted to roll out the fix first to make a new release, since it's a serious security issue. I will add a proper test later on.

Copy link
Contributor

@knoobie knoobie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 4 files at r2, 2 of 2 files at r3, 1 of 1 files at r4.
Reviewable status: 2 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @haijian-vaadin)


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 41 at r3 (raw file):

Previously, haijian-vaadin (Haijian Wang) wrote…

Good catch and thanks a lot for the good suggestion, that would be a proper test for the change. This unit test is not new but rather moved from the VaadinConnectControllerTest, since the logic is moved from there to VaaadinConnectControllerConfiguration.

In the original test, it verifies if the mocked object mapper does not call the setVisibility() if a jacksonProperties exists. Now since I cannot use the mock any more, I borrowed the example attached in the ticket to check the final result.

Basically, I wanted to roll out the fix first to make a new release, since it's a serious security issue. I will add a proper test later on.

Good point! LGTM!

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @haijian-vaadin)


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 27 at r4 (raw file):

        VaadinEndpointProperties endpointPropertiesMock = mock(VaadinEndpointProperties.class);
        VaadinConnectControllerConfiguration configuration = new VaadinConnectControllerConfiguration(endpointPropertiesMock);
        

extra spaces


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 39 at r4 (raw file):

        verify(contextMock, times(1)).getBean(JacksonProperties.class);

        try{

space before curly


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 42 at r4 (raw file):

                String result = objectMapper.writeValueAsString(new Entity());
                assertEquals("{\"name\":\"Bond\"}", result);
        }catch(Exception e){

spaces after and before curlis


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 55 at r4 (raw file):

        public String getName() {
                return name;

should use 4 spaces here, seems you are using 8, and the same in the below methods

@vaadin-bot vaadin-bot added +0.1.0 and removed +0.0.1 labels Apr 8, 2020
Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 6 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @knoobie, @manolo, and @vaadin-bot)


flow-server/src/main/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfiguration.java, line 157 at r4 (raw file):

Previously, vaadin-bot (Vaadin Bot) wrote…

CRITICAL Document the parameter(s): context rule

Done.


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 41 at r3 (raw file):

Previously, knoobie (Knoobie) wrote…

Good point! LGTM!

Done.


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 27 at r4 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

extra spaces

Done.


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 39 at r4 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

space before curly

Done.


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 42 at r4 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

spaces after and before curlis

Done.


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 55 at r4 (raw file):

Previously, manolo (Manuel Carrasco Moñino) wrote…

should use 4 spaces here, seems you are using 8, and the same in the below methods

Done.

@manolo manolo requested a review from vaadin-bot April 8, 2020 08:49
Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 unresolved discussions, 0 of 1 LGTMs obtained (waiting on @haijian-vaadin, @knoobie, and @vaadin-bot)


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 39 at r4 (raw file):

Previously, haijian-vaadin (Haijian Wang) wrote…

Done.

not done it should read try {


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 42 at r4 (raw file):

Previously, haijian-vaadin (Haijian Wang) wrote…

Done.

not done } catch (Excepcion e) {


flow-server/src/test/java/com/vaadin/flow/server/connect/VaadinConnectControllerConfigurationTest.java, line 55 at r4 (raw file):

Previously, haijian-vaadin (Haijian Wang) wrote…

Done.

not done should be

public String getName {
    return name;

Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@manolo manolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

crazy reviewable, some times it takes a while to reflect changes :lgtm_strong:

Reviewed 2 of 2 files at r3, 2 of 2 files at r5.
Reviewable status: 2 unresolved discussions, 1 of 1 LGTMs obtained (waiting on @knoobie and @vaadin-bot)

@haijian-vaadin haijian-vaadin changed the title Use Vaadin's own ObjectMapper instead of the one from Spring [Security Fix]Use Vaadin's own ObjectMapper instead of the one from Spring Apr 8, 2020
Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: 2 unresolved discussions, 2 of 1 LGTMs obtained (waiting on @knoobie and @vaadin-bot)

Copy link
Contributor Author

@haijian-vaadin haijian-vaadin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dismissed @knoobie and @vaadin-bot from 2 discussions.
Reviewable status: :shipit: complete! all discussions resolved, 2 of 1 LGTMs obtained

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hilla Issues related to Hilla +0.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vaadin 15 exposes private properties in JSON responses of REST api
4 participants