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

Override maxAliasesForCollections at LoaderOptions #30505

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

azexcy
Copy link
Contributor

@azexcy azexcy commented Mar 15, 2024

Default maxAliasesForCollections is 50, if over it, will throw org.yaml.snakeyaml.error.YAMLException: Number of aliases for non-scalar nodes exceeds the specified max=50

Changes proposed in this pull request:

  • Set it to Integer.MAX

Before committing this PR, I'm sure that I have checked the following options:

  • My code follows the code of conduct of this project.
  • I have self-reviewed the commit code.
  • I have (or in comment I request) added corresponding labels for the pull request.
  • I have passed maven check locally : ./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.
  • I have made corresponding changes to the documentation.
  • I have added corresponding unit tests for my changes.

@azexcy azexcy added this to the 5.5.0 milestone Mar 15, 2024
@azexcy azexcy marked this pull request as draft March 15, 2024 10:44
@@ -49,6 +49,7 @@ public ShardingSphereYamlConstructor(final Class<?> rootClass) {

private static LoaderOptions createLoaderOptions() {
LoaderOptions result = new LoaderOptions();
result.setMaxAliasesForCollections(Integer.MAX_VALUE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add unit test for YamlEngine.unmarshal, when maxAliasesForCollections is not overwritten, it'll throw exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

And could we set maxAliasesForCollections value to limited value? e.g. 1000. Since

  1. It's enough for now.
  2. Use Integer.MAX_VALUE might have vulnerability, see:

Copy link
Contributor

Choose a reason for hiding this comment

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

And could we disable anchor and alias in snakeyaml?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And could we set maxAliasesForCollections value to limited value? e.g. 1000. Since

  1. It's enough for now.
  2. Use Integer.MAX_VALUE might have vulnerability, see:

And LoaderOptions.allowRecursiveKeys maybe could avoid the problem
image

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@azexcy azexcy marked this pull request as ready for review March 18, 2024 02:16
@sandynz sandynz merged commit eeac1c7 into apache:master Mar 18, 2024
139 checks passed
@azexcy azexcy deleted the improve branch March 18, 2024 06:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants