[issue-15259] upgrade snakeyaml due to cve

[pjfanning]

Fixes #15259

Changes proposed in this pull request:

• pom change

[pjfanning]

Needs investigation - a yaml unmarshall is not working as it should

[terrymanu]

Please fix CI

[terrymanu]

Needs investigation - a yaml unmarshall is not working as it should

Yes, it is better to investigate, the API is changed

[linghengqian]

I noticed after testing that I changed snakeyaml.version to 1.18 and modified NoneYamlTupleProcessor.java and YamlTupleProcessorFixture.java as in the PR, the result of executing ./mvnw -T 1C clean install is normal pass Yes, the build fails starting from 1.19 (two classes seem to have to be changed, and only changing snakeyaml.version to 1.18 still fails the build). Maybe an important API change happened in org.yaml:snakeyaml: 1.19 (For example, I pointed out java.lang.ClassCastException: class java.util.LinkedHashMap cannot be cast to class org.apache.shardingsphere.infra.executor.sql.process.model.yaml.YamlExecuteProcessUnit (java. util.LinkedHashMap is in module java.base of loader 'bootstrap'; org.apache.shardingsphere.infra.executor.sql.process.model.yaml.YamlExecuteProcessUnit is in unnamed module of loader 'app') ), I am in https://bitbucket.org/snakeyaml/snakeyaml/wiki/Changes also see the instructions for backward-incompatible changes.

• snakeyaml.version=1.18
• 
• snakeyaml.version=1.19
• 

[pjfanning]

Thanks @linghengqian - I haven't had much time to have a look. I think there are 2 possibilities - 1. that snakeyaml introduced a bug or 2. something about mockito is causing snakeyaml to misbehave (the tests that fail use mocking)

[linghengqian]

@pjfanning

• After upgrading to 1.20 version, I passed org.yaml.snakeyaml.events.ScalarEvent and org.yaml.snakeyaml.DumperOptions by referring to 1.16 version.
  Finally I determined that the changes to the org.apache.shardingsphere.sharding.yaml.engine.representer.processor.NoneYamlTupleProcessor and org.apache.shardingsphere.infra.yaml.engine.fixture.YamlTupleProcessorFixture classes in this PR are Wrong, the style of org.yaml.snakeyaml.nodes.ScalarNode must specify a value, in 1.16 it is the enumeration class of org.yaml.snakeyaml.DumperOptions.ScalarStyle.PLAIN mentioned on the official website ( https://yaml.org/spec/1.1/#id903915 ).
• Maybe I'll help you by providing another PR to improve org.yaml:snakeyaml to a transitional version, but I'm not familiar with the components of org.yaml:snakeyaml and it might take some time.
• 

[pjfanning]

thanks @linghengqian - I've applied the change you suggested and will check later to see if the CI build passes

[linghengqian]

• I am still testing here, I am sure that this PR still has problems in org.apache.shardingsphere.infra.yaml.engine.representer.ShardingSphereYamlRepresenter, the final Boolean flowStyle property of protected Node representMapping() should be final DumperOptions.FlowStyle flowStyle property.
• I'm testing mockito and honestly I've never touched this component. The main effect is in org.apache.shardingsphere.mode.manager.cluster.coordinator.registry.process.subscriber.ProcessRegistrySubscriberTest

[linghengqian]

• Apparently inside org.apache.shardingsphere.mode.manager.cluster.coordinator.registry.process.subscriber.ProcessRegistrySubscriber#reportExecuteProcessUnit the call to org.apache.shardingsphere.infra.yaml.engine.YamlEngine#unmarshal exists problem, but its encapsulation is so multi-layered that I don't quite understand why this step builds the wrong object.
• snakeyaml.version=1.30
• 
• snakeyaml.version=1.16
• 

[linghengqian]

• Well, first exclusion method, at least org.apache.shardingsphere.infra.yaml.engine.YamlEngine#marshal has not changed before and after replacing snakeyaml.version.
• snakeyaml.version=1.30
• 
• snakeyaml.version=1.16
• 

[pjfanning]

@linghengqian thanks for looking at this - I don't want to interfere too much with your investigation but shardingsphere uses an old version of mockito and it wouldn't shock me if the bytecode manipulation it does could be contributing to this - I don't have time this afternoon but over next few days I can help out by trying to use the Yaml unmarshalling in shardingsphere when no mocking is involved to see if this issue is caused by snakeyaml, the shardingsphere code wrapping snakeyaml or with the mockito-based test itself.

[linghengqian]

To a certain extent, I don't think this is a problem with mockito, after all, the mockito.version currently used by ShardingSphere is 3.4.2, which was launched on Jul 16, 2020 (refer to https://mvnrepository.com/artifact/org.mockito/mockito-core/3.4.2 ). And after raising snakeyaml.version to 1.20, the ProcessRegistrySubscriberTest test class fails, and snakeyaml's 1.20 is released earlier than org. mockito:mockito-core:3.4.2 is too early (refer to https://mvnrepository.com/artifact/org.yaml/snakeyaml/1.20 )

Update: I noticed that occur in the discussion of spring-projects/spring-boot#13191, I will continue to explore on the 1.23 version for the disappearance of a generic, This may be about https://bitbucket.org/snakeyaml/snakeyaml/issues/387/support-for-generic-types-when-serializing has not yet been closed.

[linghengqian]

• Traced to org.apache.shardingsphere.infra.yaml.engine.constructor.ShardingSphereYamlConstructor, there is a step in its constructor YamlRuleConfigurationSwapperEngine.getYamlShortcuts().forEach((key, value) -> addTypeDescription(new TypeDescription(value, key))) ; , at this step, generic information is lost due to post-compilation generic erasure. This causes org.apache.shardingsphere.infra.yaml.engine.YamlEngine#unmarshal to have an incorrect reference to it.
  What should be done to handle it? I still don't understand why this step is fine on snakeyaml 1.16, generic information should be additionally tagged.

• 

• The relevant class whose generics are erased is org.apache.shardingsphere.infra.executor.sql.process.model.yaml.YamlExecuteProcessContext .

• 

[linghengqian]

@pjfanning Just a request for help. I noticed that version 1.16-1.18 of snakeyaml, org.yaml.snakeyaml.constructor.Constructor contained two properties, typeTags and typeDefinitions. But in version 1.19 these two properties were removed. Can you point out the reason for this modification of snakeyaml? I can't find a description of it in the changelog, which makes org.apache.shardingsphere.infra.yaml.engine.constructor.ShardingSphereYamlConstructor useless after upgrading the version.

• snakeyaml.version <= 1.18

• 

• 

• snakeyaml.version >= 1.19

• 

• 

[pjfanning]

@linghengqian I don't know much about the internals of snakeyaml - maybe you could raise an issue with the snakeyaml team to describe the problem to them?

[linghengqian]

The design pattern (strategy pattern) adopted by shardingsphere in this piece is quite complicated for me, and my time does not allow me to organize materials. 🤣

[pjfanning]

typeTags and typeDefinitions are in the super class 'BaseConstructor' - so Constructor inherits from BaseConstructor which means those fields are still there (looking at v1.30 snakeyaml code)

[pjfanning]

what I don't understand is mockYamlExecuteProcessContext and YamlEngine.marshal

For YamlExecuteProcessContext it returns:

unitStatuses: !!set
  ? status: EXECUTE_STATUS_DONE
    unitID: '159917166'
  : null

Wouldn't yaml like this be a better representation (generated using jackson-dataformat-yaml)?:

executionID: null
schemaName: null
username: null
hostname: null
sql: null
unitStatuses:
- unitID: "159917166"
  status: "EXECUTE_STATUS_DONE"
startTimeMillis: null

The custom yaml code in shardingsphere confuses me - I just think it would be easier to use jackson-dataformat-yaml instead. jackson-dataformat-yaml uses snakeyaml but recent versions of jackson-dataformat-yaml use up to date versions of snakeyaml

[linghengqian]

The custom yaml code in shardingsphere confuses me - I just think it would be easier to use jackson-dataformat-yaml instead. jackson-dataformat-yaml uses snakeyaml but recent versions of jackson-dataformat-yaml use up to date versions of snakeyaml

Honestly I've never tried customizing the classes org.yaml.snakeyaml.representer.Representer and org.yaml.snakeyaml.constructor.Constructor , the friend last modified org.apache.shardingsphere.infra.yaml.engine.YamlEngine of class is @sandynz , can you please explain it ? I didn't see much information in https://shardingsphere.apache.org/document/current/en/dev-manual/configuration/ .

[linghengqian]

Thanks @linghengqian - I haven't had much time to have a look. I think there are 2 possibilities - 1. that snakeyaml introduced a bug or 2. something about mockito is causing snakeyaml to misbehave (the tests that fail use mocking)

I have opened a git repository, https://github.com/linghengqian/snakeyaml-update-test, to confirm that this is not a mockito problem.

I've found a minimally reproducible demo and asked for help at https://bitbucket.org/snakeyaml/snakeyaml/issues/387/support-for-generic-types-when-serializing .

[linghengqian]

Wouldn't yaml like this be a better representation (generated using jackson-dataformat-yaml)?:

executionID: null
schemaName: null
username: null
hostname: null
sql: null
unitStatuses:
- unitID: "159917166"
  status: "EXECUTE_STATUS_DONE"
startTimeMillis: null

I'm guessing you're referring to the link in https://github.com/linghengqian/snakeyaml-update-test/blob/master/snakeyaml-new-version-test/src/test/java/com/lingh/JunitTest.java , snakeYamlTest4() parses like this. Unfortunately, it still works fine on snakeyaml 1.16, but fails on snakeyaml 1.30.

[pjfanning]

@linghengqian the alternate yaml I showed in #15260 (comment) is not generated using snakeyaml - it was generated using jackson-dataformat-yaml.

I would suggest that you dump snakeyaml because it is not well maintained. The yaml that jackson-dataformat-yaml generates in much more readable and portable.

• https://bitbucket.org/snakeyaml/snakeyaml/issues/387/support-for-generic-types-when-serializing is open for over 4 years.

I extended your sample in a fork I made - see https://github.com/pjfanning/snakeyaml-update-test/blob/master/jackson-test/src/test/java/org/example/jackson/JacksonTest.java

[sandynz]

Hi @pjfanning , I tested with your PR. In fact, the unit test code could be improved to solve the exception.

In ProcessRegistrySubscriberTest.mockYamlExecuteProcessContext(), change

Collection<YamlExecuteProcessUnit> unitStatuses = Collections.singleton(yamlExecuteProcessUnit);

to

Collection<YamlExecuteProcessUnit> unitStatuses = Collections.singletonList(yamlExecuteProcessUnit);

Then marshaled YamlExecuteProcessContext yaml is:

unitStatuses:
- status: EXECUTE_STATUS_DONE
  unitID: '159917166'

And ProcessRegistrySubscriberTest could pass.

In fact, in YamlExecuteProcessContext constructor:

unitStatuses = executeProcessContext.getUnitStatuses().stream().map(YamlExecuteProcessUnit::new).collect(Collectors.toList());

Though unitStatuses is type of Collection, it use List, but not Set.

[sandynz]

@pjfanning More snakeyaml versions could be updated, e.g. examples/pom.xml, main/release-docs/LICENSE. You could search 1.16 in project.

[pjfanning]

Thanks @sandynz - I've added the changes you suggested to this PR

[linghengqian]

• A minimal reproducible unit test is now in the master branch of snakeyaml, see https://bitbucket.org/snakeyaml/snakeyaml/commits/23fbcef0e952adbea3fc147c6b17eb8ab7e3432a . This issue was fixed in snakeyaml 1.31 and I followed up on Update snakeyaml to 1.31 or 1.32 to fix unit tests #21043 .