Expose load settings #613

headius · 2023-01-13T14:50:46Z

This builds off #612, adding access to some key DOS-related settings in the parser. See #579.

The PR in #612 should be merged before this one (and I will rebase to master when that happens).

These values are often set to mitigate DOS attacks, so we want to expose them for JRuby users. See ruby#579

headius · 2023-01-26T21:03:16Z

@hsbt @tenderlove I would like to start shipping this feature, so users can configure the limits in the SnakeYAML parser. Is what I have here acceptable? Does libyaml have any similar configurations we could expose in the same way?

hsbt · 2023-01-27T00:39:14Z

It's reasonable to expose CodePointLimit for jruby/jruby#7543 and AllowDuplicateKeys for #426 (comment)

But I'm not sure we should allow to set AllowRecursiveKeys and MaxAliasesForCollections. Can you point why we should allow them?

headius · 2023-01-31T18:10:20Z

@hsbt I believe all four of these were requested by users. Those additional two (recursive keys, max aliases for collections) were mentioned in #579.

hsbt · 2023-01-31T22:06:57Z

Ah, I see. Thanks.

headius · 2023-02-06T15:28:13Z

I guess this is approved so I'm going to merge. If we find that these settings are configurable in libyaml we can deal with making the API the same at that point.

yvesll · 2023-07-12T05:58:16Z

What about this option in high-level API like Psych.load? Now I need to add a monkey patch to set the code point limit in Psych module method..., but this seems a bad way.

headius · 2023-07-13T18:16:53Z

@yvesll That's not a bad idea. Perhaps you could put together a PR that allows passing these values through as options?

oliverbarnes · 2023-09-05T21:06:23Z

How can these load settings be defined in a rails app? (JRuby newbie here)

I tried implementing the monkey patch as per @yvesll's comment, but that isn't working for me:

#Rails initializer
module Psych
  class Parser
    def code_point_limit(_context)
      20_000_000
    end
  end
end

We're on Rails 6 running on JRuby 9.4.3.0, and we're hitting the code point limit issue when loading big yaml fixtures for rspec:

Failure/Error: Unable to find org.snakeyaml.engine.v2.scanner.ScannerImpl.fetchMoreTokens(ScannerImpl.java to read failed line

     Java::OrgSnakeyamlEngineV2Exceptions::YamlEngineException:
       The incoming YAML document exceeds the limit: 3145728 code points.
     # org.snakeyaml.engine.v2.scanner.ScannerImpl.fetchMoreTokens(ScannerImpl.java:289)
     # org.snakeyaml.engine.v2.scanner.ScannerImpl.checkToken(ScannerImpl.java:192)
     # org.snakeyaml.engine.v2.parser.ParserImpl$ParseBlockMappingKey.produce(ParserImpl.java:710)
     # org.snakeyaml.engine.v2.parser.ParserImpl.lambda$produce$1(ParserImpl.java:232)
     # java.base/java.util.Optional.ifPresent(Optional.java:178)
     # org.snakeyaml.engine.v2.parser.ParserImpl.produce(ParserImpl.java:232)
     # org.snakeyaml.engine.v2.parser.ParserImpl.hasNext(ParserImpl.java:226)
     # org.jruby.ext.psych.PsychParser.parse(PsychParser.java:245)
     # org.jruby.ext.psych.PsychParser$INVOKER$i$3$0$parse.call(PsychParser$INVOKER$i$3$0$parse.gen)

Is there a way to set it in a .jrubyrc config? I didn't find a code_point_limit option in jruby --properties output

yvesll · 2023-09-06T05:53:24Z

HI, @oliverbarnes, here is my monkey patch which works fine for me, hope it can help you:

module Psych
  def self.parse_stream yaml, filename: nil, &block
    if block_given?
      parser = Psych::Parser.new(Handlers::DocumentStream.new(&block))
      parser.code_point_limit = 20_000_000
      parser.parse yaml, filename
    else
      parser = self.parser
      parser.parse yaml, filename
      parser.code_point_limit = 20_000_000
      parser.handler.root
    end
  end if defined?(RUBY_ENGINE) && RUBY_ENGINE == "jruby"
end

oliverbarnes · 2023-09-06T23:07:37Z

Thanks @yvesll! I'm getting a new error now, but I think it's enough to make progress:

the leading empty lines contain more spaces (8) than the first non-empty line. while scanning a block scalar at line 32385 column 5
     # ./config/initializers/psych.rb:6:in `parse_stream'

headius · 2023-09-07T08:09:09Z

I had not considered that many applications won't control the construction of the psych parser! I think we should add class methods to psych for setting the default values that then will be passed in for every parser creation. It may also be worth using higher defaults to begin with. Perhaps one of you could try to create a PR for that feature?

oliverbarnes · 2023-09-07T21:52:48Z

@headius I'd be happy to do that (after some grokking), but I'm still encountering issues related to psych.

Using @yvesll's monkey patch got me past the code point limitation, but now I'm stuck with vcr not recognizing http requests previously recorded in yaml fixtures. I have a hunch this is due to Psych now being compliant with YAML 1.2, as mentioned in other issues here on the repo.

Being totally honest, I'm spiking migrating a project to jruby for a client, and before I can dedicate time contributing to jruby, I need to show some progress :)

headius · 2023-09-07T22:08:29Z

due to Psych now being compliant with YAML 1.2

@oliverbarnes That is certainly possible, but there's always the possibility of a bug in SnakeYAML Engine. If you can show this parsing with another 1.2-compliant parser, we will know.

Keep me posted...I want to help your experiment to succeed!

headius · 2023-09-10T10:07:21Z

@oliverbarnes Is there an issue filed for your YAML 1.2 problem? I want to make sure we track it but I'm not sure what other issues you were referring to. I found only one recent issue in the psych tracker since the upgrade.

Given your report and the other I found I'm recommending that we not upgrade psych in the JRuby 9.3 line (jruby/jruby#7600), so having a complete bug report would be really helpful!

This adds class-level accessors for the following SnakeYAML- specific parser settings: * max_aliases_for_collections * allow_duplicate_keys * allow_recursive_keys * code_point_limit The initial values are based on SnakeYAML Engine's defaults. This PR does not modify those default values. Using these accessors, it should be possible for users to globally change them for all future Psych parser instances without resorting to monkey patches as described in ruby#613 (comment).

headius · 2023-09-10T10:43:21Z

@oliverbarnes Here, I'll trade you this for your YAML 1.2 issue report: #647

oliverbarnes · 2023-09-11T15:13:05Z

Here, I'll trade you this for your YAML 1.2 issue report: #647

:D fair trade! Thanks, and will do @headius 👍

Keep me posted...I want to help your experiment to succeed!

For sure! Appreciate the support :)

This adds class-level accessors for the following SnakeYAML- specific parser settings: * max_aliases_for_collections * allow_duplicate_keys * allow_recursive_keys * code_point_limit The initial values are based on SnakeYAML Engine's defaults. This PR does not modify those default values. Using these accessors, it should be possible for users to globally change them for all future Psych parser instances without resorting to monkey patches as described in ruby#613 (comment).

These settings are not enforced at the parser level in SnakeYAML Engine, instead being enforced during the construction of the YAML node graph. Because the JRuby Psych wrapper around SnakeYAML only uses the parser directly, the settings have no effect. This commit removes the ineffective settings until we can decide what to do about them. See ruby#613, ruby#647, and ruby#649.

headius mentioned this pull request Jan 13, 2023

org.yaml.snakeyaml.error.YAMLException: The incoming YAML document exceeds the limit: 3145728 code points. jruby/jruby#7543

Closed

headius force-pushed the expose_load_settings branch from 7bbe013 to caac676 Compare January 13, 2023 16:12

Expose a few key LoadSettings values

a0e5247

These values are often set to mitigate DOS attacks, so we want to expose them for JRuby users. See ruby#579

headius force-pushed the expose_load_settings branch from caac676 to a0e5247 Compare January 13, 2023 16:12

Merge branch 'master' into expose_load_settings

5b5d945

headius marked this pull request as ready for review January 26, 2023 21:03

headius requested review from hsbt and tenderlove January 26, 2023 21:05

hsbt approved these changes Jan 31, 2023

View reviewed changes

headius merged commit 104fbe9 into ruby:master Feb 6, 2023

headius deleted the expose_load_settings branch February 6, 2023 15:28

headius mentioned this pull request Feb 6, 2023

JRuby: Provide some sort of ability to configure SnakeYAML LoaderOptions #579

Closed

headius mentioned this pull request Sep 10, 2023

Update 9.3 to Psych 5.1 jruby/jruby#7600

Closed

headius mentioned this pull request Sep 10, 2023

Allow setting defaults for SnakeYAML limits #647

Merged

oliverbarnes mentioned this pull request Sep 12, 2023

YAML 1.2 compliance possibly breaking VCR #648

Open

headius mentioned this pull request Sep 19, 2023

Add settings for alias limits, recursion, and duplicate keys #649

Open

headius mentioned this pull request Oct 5, 2023

Remove SnakeYAML settings that don't work #653

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose load settings #613

Expose load settings #613

headius commented Jan 13, 2023

headius commented Jan 26, 2023

hsbt commented Jan 27, 2023

headius commented Jan 31, 2023

hsbt commented Jan 31, 2023

headius commented Feb 6, 2023

yvesll commented Jul 12, 2023 •

edited

Loading

headius commented Jul 13, 2023

oliverbarnes commented Sep 5, 2023 •

edited

Loading

yvesll commented Sep 6, 2023

oliverbarnes commented Sep 6, 2023

headius commented Sep 7, 2023

oliverbarnes commented Sep 7, 2023 •

edited

Loading

headius commented Sep 7, 2023

headius commented Sep 10, 2023

headius commented Sep 10, 2023

oliverbarnes commented Sep 11, 2023

Expose load settings #613

Expose load settings #613

Conversation

headius commented Jan 13, 2023

headius commented Jan 26, 2023

hsbt commented Jan 27, 2023

headius commented Jan 31, 2023

hsbt commented Jan 31, 2023

headius commented Feb 6, 2023

yvesll commented Jul 12, 2023 • edited Loading

headius commented Jul 13, 2023

oliverbarnes commented Sep 5, 2023 • edited Loading

yvesll commented Sep 6, 2023

oliverbarnes commented Sep 6, 2023

headius commented Sep 7, 2023

oliverbarnes commented Sep 7, 2023 • edited Loading

headius commented Sep 7, 2023

headius commented Sep 10, 2023

headius commented Sep 10, 2023

oliverbarnes commented Sep 11, 2023

yvesll commented Jul 12, 2023 •

edited

Loading

oliverbarnes commented Sep 5, 2023 •

edited

Loading

oliverbarnes commented Sep 7, 2023 •

edited

Loading