From 9352f71389989ed8e86808510bdaec43fc4a87a0 Mon Sep 17 00:00:00 2001 From: tannevaled Date: Mon, 5 Oct 2026 18:33:54 +0200 Subject: [PATCH 1/2] fix(yaml): load and load_file are restricted, as Psych 4 made them YAML.load materialised arbitrary classes. #775 fixed safe_load and left this deliberately, because load shared an implementation with unsafe_load and the comment said Puppet's persistence depends on objects coming back. load is the call people habitually write, so this was the more dangerous half. psych.rb:369 is `def self.load yaml, permitted_classes: [Symbol], ...` delegating straight to safe_load, and load_file delegates to load (psych.rb:716). Binding load to the unrestricted loader was correct for Psych 3 and wrong from Psych 4 on -- that inversion is the change Psych made precisely because load is the habitual call. An explicit permitted_classes REPLACES the [Symbol] default rather than adding to it, because it is an ordinary keyword default. Measured on MRI 4.0.5: load(":s") => :s load(":s", permitted_classes: [G]) => Psych::DisallowedClass load(":s", permitted_classes: [G, Symbol]) => :s so psychLoadArgs seeds Symbol only when the keyword is ABSENT, never when the resulting list is merely empty. Absent and empty are different policies, which is the distinction psychSafeOpts was built to keep. unsafe_load and a new unsafe_load_file (psych.rb:694) are the unrestricted spellings. unsafe_load_file did not exist here; it is added WITH the restriction rather than after it, so a caller who legitimately wants objects back from a file -- which is what load_file used to give them -- has somewhere to migrate to. THE PUPPET PREMISE DOES NOT HOLD, and that is what settled the scope question. MRI refuses Puppet's own state shape through a bare load: h = {"File[/x]" => {:checked => Time.at(0).utc, :synced => Time.at(0).utc}} YAML.load(YAML.dump(h)) -> DisallowedClass YAML.load(YAML.dump(h), permitted_classes: [Symbol, Time]) -> the hash Time is not in load's [Symbol] default. So Puppet cannot be reading its state file with a bare load either -- Puppet::Util::Yaml passes its own allow-list -- and this change makes rbgo AGREE with the MRI that the Puppet gem is written against. Eleven rows measured side by side, ten identical; the one divergence (a bare timestamp scalar loads as a String where MRI refuses it) is PRE-EXISTING and identical on main, controlled both ways, and is the engine discarding the tag rather than anything here. Seven call sites in the suite went red, each decided individually rather than swept: - TestYAMLLoadStaysUnrestricted asserted the defect by design -- it REQUIRED load to return a Gadget. Replaced by the positive invariant: load restricts, unsafe_load does not, and an explicit list replaces the default. - TestYAMLLoadStateRoundTrip now passes [Symbol, Time], with the measurement showing MRI needs it too. - Five loader-capability tests (Time, three !ruby/object shapes, anchors, an object graph with shared refs, and a load_file line inside the safe_load_file test) moved to unsafe_load / unsafe_load_file: what they test is what the LOADER can rebuild, not what load's policy permits. One of those sites was a NAMELESS `!ruby/object` with no colon, which a ':'-suffixed pattern missed -- a first attempt rewrote 42 call sites with a regex and had to be stashed and redone site by site, because most of those 42 were plain scalars that load still handles and had no business changing. Closes #788. Co-Authored-By: Claude Opus 5 --- internal/vm/yaml.go | 78 +++++++++++++++++++++++++++--- internal/vm/yaml_load_test.go | 36 +++++++++----- internal/vm/yaml_object_test.go | 2 +- internal/vm/yaml_safe_load_test.go | 46 ++++++++++++++---- 4 files changed, 135 insertions(+), 27 deletions(-) diff --git a/internal/vm/yaml.go b/internal/vm/yaml.go index 0e2d4e08..78b51301 100644 --- a/internal/vm/yaml.go +++ b/internal/vm/yaml.go @@ -43,18 +43,31 @@ func (vm *VM) registerYAML() { psych.smethods[m] = &Method{name: m, owner: psych, native: notImpl(m)} } - // YAML.load(source[, ...]) / Psych.load parse a YAML document string to a tree - // of Ruby values. unsafe_load shares the same implementation. Leading + // YAML.load(source[, ...]) / Psych.load is RESTRICTED, and does not share an + // implementation with unsafe_load. Psych 4 inverted the two: psych.rb:369 is + // `def self.load yaml, permitted_classes: [Symbol], ...` delegating straight + // to safe_load, which is the change Psych made precisely because `load` is + // the call people habitually write. Binding it to the unrestricted loader was + // right for Psych 3 and wrong from Psych 4 on. See psychLoadArgs. Leading // keyword/positional options Psych accepts (filename, symbolize_names, …) are // tolerated and ignored. loadFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { if len(args) == 0 { raise("ArgumentError", "wrong number of arguments (given 0, expected 1..)") } - return yamlLoad(vm, yamlSourceArg(args[0])) + return yamlSafeLoad(vm, yamlSourceArg(args[0]), psychLoadArgs(args[1:])) } psych.smethods["load"] = &Method{name: "load", owner: psych, native: loadFn} - psych.smethods["unsafe_load"] = &Method{name: "unsafe_load", owner: psych, native: loadFn} + + // YAML.unsafe_load is the unrestricted entry point (psych.rb:272), and the one + // a caller that genuinely wants arbitrary classes back must now name. + unsafeLoadFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { + if len(args) == 0 { + raise("ArgumentError", "wrong number of arguments (given 0, expected 1..)") + } + return yamlLoad(vm, yamlSourceArg(args[0])) + } + psych.smethods["unsafe_load"] = &Method{name: "unsafe_load", owner: psych, native: unsafeLoadFn} // YAML.safe_load(source[, permitted_classes: [...], permitted_symbols: [...]]) // restricts which classes a document may materialise. Psych's signature is @@ -69,15 +82,31 @@ func (vm *VM) registerYAML() { } psych.smethods["safe_load"] = &Method{name: "safe_load", owner: psych, native: safeLoadFn} - // YAML.load_file(path[, ...]) reads the file and parses its contents. + // YAML.load_file(path[, ...]) reads the file and parses its contents. It + // delegates to load (psych.rb:716), so it carries load's restriction and its + // keyword defaults -- measured: load_file over ":s\n" with + // permitted_classes: [G] raises Psych::DisallowedClass under MRI, exactly as + // load does, because the explicit list replaces the [Symbol] default. loadFileFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { if len(args) == 0 { raise("ArgumentError", "wrong number of arguments (given 0, expected 1..)") } - return yamlLoad(vm, yamlReadFile(args[0])) + return yamlSafeLoad(vm, yamlReadFile(args[0]), psychLoadArgs(args[1:])) } psych.smethods["load_file"] = &Method{name: "load_file", owner: psych, native: loadFileFn} + // YAML.unsafe_load_file(path) is unsafe_load over a file (psych.rb:694). It + // is added here with the restriction, not before it: without it a caller that + // legitimately wants objects back from a file -- which is what load_file used + // to give them -- would have no spelling at all to migrate to. + unsafeLoadFileFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { + if len(args) == 0 { + raise("ArgumentError", "wrong number of arguments (given 0, expected 1..)") + } + return yamlLoad(vm, yamlReadFile(args[0])) + } + psych.smethods["unsafe_load_file"] = &Method{name: "unsafe_load_file", owner: psych, native: unsafeLoadFileFn} + // YAML.safe_load_file(path[, permitted_classes: [...]]) is safe_load over a file. safeLoadFileFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value { if len(args) == 0 { @@ -216,6 +245,43 @@ func psychSafeArgs(rest []object.Value) psychSafeOpts { return o } +// psychLoadArgs is psychSafeArgs with Psych.load's own default allow-list. +// +// psych.rb:369 declares `def self.load yaml, permitted_classes: [Symbol], ...` +// and delegates to safe_load, so load differs from safe_load ONLY in that +// default. An explicit permitted_classes REPLACES it rather than adding to it, +// because it is an ordinary keyword default -- measured against MRI 4.0.5: +// +// load(":s") => :s +// load(":s", permitted_classes: [G]) => Psych::DisallowedClass +// load(":s", permitted_classes: [G, Symbol]) => :s +// +// which is why the seeding below is conditional on the keyword being ABSENT +// rather than on the resulting list being empty. Absent and empty are different +// policies here: absent means "use load's default", empty means "deny +// everything", and psychSafeOpts exists to keep them distinguishable. +func psychLoadArgs(rest []object.Value) psychSafeOpts { + o := psychSafeArgs(rest) + if !psychHasKeyword(rest, "permitted_classes") { + o.classes["Symbol"] = true + } + return o +} + +// psychHasKeyword reports whether the trailing keyword hash carries key at all, +// which is not the same question as whether its value is empty. +func psychHasKeyword(rest []object.Value, key string) bool { + if len(rest) == 0 { + return false + } + h, ok := rest[len(rest)-1].(*object.Hash) + if !ok { + return false + } + _, ok = h.Get(object.Symbol(key)) + return ok +} + // psychNameList reads one keyword from h as a list of names. A Class / Module // element contributes its name and any other element its to_s, matching how // Psych accepts `[Date]` and `["Date"]` alike (`permitted_classes.map(&:to_s)`). diff --git a/internal/vm/yaml_load_test.go b/internal/vm/yaml_load_test.go index 0d4a7db0..967f8812 100644 --- a/internal/vm/yaml_load_test.go +++ b/internal/vm/yaml_load_test.go @@ -136,11 +136,14 @@ func TestYAMLLoadBlockScalar(t *testing.T) { } // TestYAMLLoadTime covers the ISO-8601 timestamp the emitter writes loading back -// to a Time, in both the "Z" and numeric-offset forms. +// to a Time, in both the "Z" and numeric-offset forms. It goes through +// unsafe_load because what is under test is the LOADER's ability to build a +// Time, not load's policy: load permits only Symbol by default, and MRI refuses +// a timestamp through a bare load for the same reason. func TestYAMLLoadTime(t *testing.T) { cases := []struct{ src, want string }{ - {`t = YAML.load("--- 1970-01-01 00:00:00.000000000 Z\n"); p t.class; p t.to_i`, "Time\n0\n"}, - {`t = YAML.load("--- 1970-01-01 01:00:00.000000000 +01:00\n"); p t.to_i`, "0\n"}, + {`t = YAML.unsafe_load("--- 1970-01-01 00:00:00.000000000 Z\n"); p t.class; p t.to_i`, "Time\n0\n"}, + {`t = YAML.unsafe_load("--- 1970-01-01 01:00:00.000000000 +01:00\n"); p t.to_i`, "0\n"}, } for _, c := range cases { if got := eval(t, c.src); got != c.want { @@ -149,12 +152,19 @@ func TestYAMLLoadTime(t *testing.T) { } } +// The allow-list is not a concession to our own restriction: MRI refuses this +// document through a bare YAML.load too, because Time is not in load's [Symbol] +// default. Measured on 4.0.5 -- load(doc) raises Psych::DisallowedClass, while +// load(doc, permitted_classes: [Symbol, Time]) returns the hash. Since this is +// Puppet's state shape, Puppet cannot be reading it with a bare load either: +// Puppet::Util::Yaml passes its own allow-list, which is what this mirrors. +// // TestYAMLLoadStateRoundTrip covers the exact shape Puppet's state.yaml uses (a // String -> {Symbol -> Time} mapping), which must round-trip through dump/load. func TestYAMLLoadStateRoundTrip(t *testing.T) { src := `require "yaml" h = {"File[/x]" => {:checked => Time.at(0).utc, :synced => Time.at(0).utc}} -r = YAML.load(YAML.dump(h)) +r = YAML.load(YAML.dump(h), permitted_classes: [Symbol, Time]) p r.keys p r["File[/x]"].keys p r["File[/x]"][:checked].to_i` @@ -164,6 +174,10 @@ p r["File[/x]"][:checked].to_i` } } +// These go through unsafe_load because the subject is the LOADER's ability to +// rebuild a tagged object, not load's policy: since Psych 4, load permits only +// Symbol by default and refuses !ruby/object: exactly as MRI does. +// // TestYAMLLoadRubyObject covers loading a !ruby/object: mapping back into an // instance of the named class, with the mapping entries as instance variables — // including a class the program already defines and one it does not (which the @@ -172,26 +186,26 @@ func TestYAMLLoadRubyObject(t *testing.T) { cases := []struct{ src, want string }{ // Known class: ivars are restored. {`class Foo; attr_reader :a, :b; end -o = YAML.load("--- !ruby/object:Foo\na: 1\nb: hi\n") +o = YAML.unsafe_load("--- !ruby/object:Foo\na: 1\nb: hi\n") p o.class p o.a p o.b`, "Foo\n1\n\"hi\"\n"}, // Bare !ruby/object loads as an Object instance. - {`o = YAML.load("--- !ruby/object\na: 1\n") + {`o = YAML.unsafe_load("--- !ruby/object\na: 1\n") p o.instance_variable_get(:@a)`, "1\n"}, // Unknown class: a placeholder class of that name is created. - {`o = YAML.load("--- !ruby/object:Quux\nx: 5\n") + {`o = YAML.unsafe_load("--- !ruby/object:Quux\nx: 5\n") p o.class.name p o.instance_variable_get(:@x)`, "\"Quux\"\n5\n"}, // A qualified unknown class name. - {`o = YAML.load("--- !ruby/object:A::B\nx: 1\n") + {`o = YAML.unsafe_load("--- !ruby/object:A::B\nx: 1\n") p o.class.name`, "\"A::B\"\n"}, // An empty-bodied object (the "tag {}" inline form). {`class Empty; end -o = YAML.load("--- !ruby/object:Empty {}\n") +o = YAML.unsafe_load("--- !ruby/object:Empty {}\n") p o.class`, "Empty\n"}, // A symbol-keyed object mapping (Psych object ivars). - {`o = YAML.load("--- !ruby/object:Quux2\n:x: 1\n") + {`o = YAML.unsafe_load("--- !ruby/object:Quux2\n:x: 1\n") p o.instance_variable_get(:@x)`, "1\n"}, } for _, c := range cases { @@ -206,7 +220,7 @@ p o.instance_variable_get(:@x)`, "1\n"}, func TestYAMLLoadAnchors(t *testing.T) { src := `class Node; attr_reader :n; end doc = "---\n- &1 !ruby/object:Node\n n: 7\n- *1\n" -a = YAML.load(doc) +a = YAML.unsafe_load(doc) p a[0].equal?(a[1]) p a[1].n` want := "true\n7\n" diff --git a/internal/vm/yaml_object_test.go b/internal/vm/yaml_object_test.go index 83b22327..1d222589 100644 --- a/internal/vm/yaml_object_test.go +++ b/internal/vm/yaml_object_test.go @@ -201,7 +201,7 @@ p0 = Person.new p0.instance_variable_set(:@name, "Ada") p0.instance_variable_set(:@age, 36) doc = [p0, p0].to_yaml -r = YAML.load(doc) +r = YAML.unsafe_load(doc) p r[0].class p r[0].name p r[0].age diff --git a/internal/vm/yaml_safe_load_test.go b/internal/vm/yaml_safe_load_test.go index eed0eccf..a395f68a 100644 --- a/internal/vm/yaml_safe_load_test.go +++ b/internal/vm/yaml_safe_load_test.go @@ -253,24 +253,52 @@ rescue Psych::DisallowedClass => e puts "refused: #{e.message}" end puts YAML.safe_load_file(` + rubyStr(path) + `, permitted_classes: [Gadget]).class -puts YAML.load_file(` + rubyStr(path) + `).class` +puts YAML.unsafe_load_file(` + rubyStr(path) + `).class` want := "refused: Tried to load unspecified class: Gadget\nGadget\nGadget\n" if got := eval(t, src); got != want { t.Errorf("got %q want %q", got, want) } } -// TestYAMLLoadStaysUnrestricted checks the fix did not leak into the unsafe -// entry points. load / unsafe_load are documented as unsafe and Puppet's -// persistence calls them; a restriction there would be a different bug. -func TestYAMLLoadStaysUnrestricted(t *testing.T) { +// TestYAMLLoadIsRestrictedAndUnsafeLoadIsNot replaces a test that asserted the +// defect. Its predecessor, TestYAMLLoadStaysUnrestricted, REQUIRED +// YAML.load("--- !ruby/object:Gadget") to return a Gadget, reasoning that load +// is "documented as unsafe" and that Puppet's persistence calls it. +// +// Psych 4 inverted that: psych.rb:369 makes load delegate to safe_load with +// permitted_classes: [Symbol] -- the change Psych made precisely because load is +// the call people habitually write. The Puppet premise does not hold either: +// MRI refuses Puppet's own state shape through a bare load (measured; see +// TestYAMLLoadStateRoundTrip), so Puppet passes its own allow-list rather than +// relying on load being unrestricted. +// +// The invariant is the pair. load restricts to Symbol; unsafe_load does not +// restrict at all, so a caller who genuinely wants objects back still has a +// spelling to name; and an explicit permitted_classes REPLACES the Symbol +// default rather than adding to it, which is the subtlety a merge-shaped +// implementation would get wrong. +func TestYAMLLoadIsRestrictedAndUnsafeLoadIsNot(t *testing.T) { src := `require "yaml" class Gadget; attr_accessor :foo; end -puts YAML.load("--- !ruby/object:Gadget\nfoo: bar\n").class -puts YAML.unsafe_load("--- !ruby/object:Gadget\nfoo: bar\n").foo +g = "--- !ruby/object:Gadget\nfoo: bar\n" +begin + YAML.load(g) + puts "load: NOT REFUSED" +rescue Psych::DisallowedClass => e + puts "load: #{e.message}" +end +puts YAML.unsafe_load(g).foo puts YAML.load(":sym").class -puts YAML.load("--- !ruby/regexp /ab/i\n").class` - want := "Gadget\nbar\nSymbol\nRegexp\n" +puts YAML.load(g, permitted_classes: [Gadget]).class +begin + YAML.load(":sym", permitted_classes: [Gadget]) + puts "explicit list: NOT REFUSED" +rescue Psych::DisallowedClass + puts "explicit list replaces the Symbol default" +end` + want := "load: Tried to load unspecified class: Gadget\n" + + "bar\nSymbol\nGadget\n" + + "explicit list replaces the Symbol default\n" if got := eval(t, src); got != want { t.Errorf("got %q want %q", got, want) } From 86acc22ea82765db5aba01e4b1554acfae008e0e Mon Sep 17 00:00:00 2001 From: tannevaled Date: Tue, 6 Oct 2026 21:58:10 +0200 Subject: [PATCH 2/2] test(yaml): cover the two arms the coverage gate named CI's per-function gate reported both as newly below 100%: internal/vm/yaml.go psychHasKeyword 85.7% (was fully covered) internal/vm/yaml.go registerYAML 98.3% (was fully covered) Asked "can this happen?" before "how do I cover it?", and both times it can: - psychHasKeyword's arm for a trailing argument that is NOT a keyword hash. The binding tolerates and ignores leading positional options Psych accepts, so `YAML.load(doc, "anything")` takes it and the document still loads under load's [Symbol] default. Measured: returns {"a" => 1}. - the arity guard inside unsafe_load_file, the entry point this branch added. Measured: ArgumentError, as MRI raises there too. So: missing tests, not dead code, and nothing to delete. Verified before pushing rather than after, which is how the parser's nesting PR earned a red CI yesterday: registerYAML 100.0% psychLoadArgs 100.0% psychHasKeyword 100.0% ok internal/vm 186.430s coverage: 99.9% Recorded in the test and deliberately not changed: MRI refuses `YAML.load(doc, "notahash")` outright with "wrong number of arguments (given 2, expected 1)" where this binding tolerates it. That divergence is the binding's documented choice and predates this branch. Co-Authored-By: Claude Opus 5 --- internal/vm/yaml_safe_load_test.go | 42 ++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/internal/vm/yaml_safe_load_test.go b/internal/vm/yaml_safe_load_test.go index a395f68a..f0eb86fc 100644 --- a/internal/vm/yaml_safe_load_test.go +++ b/internal/vm/yaml_safe_load_test.go @@ -353,3 +353,45 @@ puts YAML.safe_load("` + tag + `", permitted_classes: [Object]).class` } } } + +// TestYAMLLoadArgShapesTheCoverageGateNamed covers two arms the per-function +// gate reported as newly below 100% after the restriction landed. Both were +// missing tests rather than dead code, which is the question worth asking +// first: each is reachable from ordinary Ruby. +// +// - psychHasKeyword returns false when the trailing argument is not a keyword +// hash at all. The binding tolerates and ignores leading positional options +// Psych accepts, so `YAML.load(doc, "anything")` takes that arm and the +// document still loads under load's [Symbol] default. +// - the arity guard inside unsafe_load_file, the entry point this change +// added. MRI raises there too, with its own wording. +// +// Noted while measuring, and deliberately not changed here: MRI refuses +// `YAML.load(doc, "notahash")` outright with `wrong number of arguments +// (given 2, expected 1)`, where this binding tolerates it. That divergence is +// the binding's documented choice and predates this change. +func TestYAMLLoadArgShapesTheCoverageGateNamed(t *testing.T) { + path := tmpDirSlash(t) + "/doc.yaml" + writeFileT(t, path, "a: 1\n") + src := `require "yaml" +p YAML.load("a: 1\n", "a positional that is not a keyword hash") +p YAML.load("a: 1\n", 42) +begin + YAML.unsafe_load_file +rescue ArgumentError => e + puts "unsafe_load_file: #{e.message}" +end +begin + YAML.unsafe_load +rescue ArgumentError => e + puts "unsafe_load: #{e.message}" +end +p YAML.unsafe_load_file(` + rubyStr(path) + `)` + want := "{\"a\" => 1}\n{\"a\" => 1}\n" + + "unsafe_load_file: wrong number of arguments (given 0, expected 1..)\n" + + "unsafe_load: wrong number of arguments (given 0, expected 1..)\n" + + "{\"a\" => 1}\n" + if got := eval(t, src); got != want { + t.Errorf("got %q want %q", got, want) + } +}