Net::FTP.default_passive, and the options a nil host was throwing away

internal/vm/net_ftp.go

@@ -77,6 +77,29 @@ func (vm *VM) registerFTPErrors(netMod, std *RClass) {
 // options) connects (and logs in when a username is supplied); open is new plus
 // the ensured-close block form.
 func (vm *VM) registerFTPClassMethods(ftp *RClass) {
+	// Net::FTP.default_passive, held in the CLASS VARIABLE the reference
+	// implementation uses (net-ftp 0.3.9 lib/net/ftp.rb:96 `@@default_passive =
+	// true`, accessors at :159-167). Storing it there rather than in a Go field
+	// is not decoration: a Ruby program can reach it through
+	// Net::FTP.class_variable_get(:@@default_passive), as it can under MRI, and
+	// there is one place holding the value instead of two that can disagree.
+	//
+	// ⛔ ruby/spec's library/net-ftp/spec_helper.rb sets this to FALSE for the
+	// whole suite, and its fixture server implements PORT/EPRT and has no `pasv`
+	// method at all. Without the accessor the helper's `if
+	// defined?(Net::FTP.default_passive)` guard was skipped in SILENCE and every
+	// spec ran against a mode the fixture cannot speak (#916).
+	ftp.cvars["@@default_passive"] = object.Bool(true)
+	ftp.smethods["default_passive"] = &Method{name: "default_passive", owner: ftp,
+		native: func(_ *VM, _ object.Value, _ []object.Value, _ *Proc) object.Value {
+			return object.Bool(ftpDefaultPassive(ftp))
+		}}
+	ftp.smethods["default_passive="] = &Method{name: "default_passive=", owner: ftp,
+		native: func(_ *VM, _ object.Value, a []object.Value, _ *Proc) object.Value {
+			ftp.cvars["@@default_passive"] = object.Bool(a[0].Truthy())
+			return a[0]
+		}}
+
 	newFn := func(vm *VM, _ object.Value, args []object.Value, _ *Proc) object.Value {
 		f := newFTPObj(vm, ftp)
 		vm.ftpInit(f, args)
@@ -109,20 +132,33 @@ func (vm *VM) ftpWithClose(f *ftpObj, blk *Proc) (res object.Value) {
 // the legacy positional user / passwd / acct, and a login when a username is
 // present.
 func (vm *VM) ftpInit(f *ftpObj, args []object.Value) {
-	if len(args) == 0 || object.IsNil(args[0]) {
-		return
-	}
-	host := strArg(args[0])
 	port := 21
 	user, passwd, acct := "", "", ""
 	userGiven, passwdGiven := false, false
 
+	// ⛔ THE OPTIONS ARE APPLIED WHETHER OR NOT A HOST WAS GIVEN. This used to
+	// return before reading them when args[0] was nil, so
+	// `Net::FTP.new(nil, passive: true)` silently ignored EVERY option -- found
+	// by running the settings against the reference rather than against itself:
+	// rbgo answered `false` where net-ftp 0.3.9 answers `true`.
+	//
+	// MRI's initialize sets @binary / @passive / @debug_mode / the timeouts
+	// first and only then, separately, connects when a host is present
+	// (lib/net/ftp.rb:285-300). ruby/spec drives exactly that shape:
+	// `@ftp.send(:initialize, nil, options)` then asserts `passive`
+	// (library/net-ftp/initialize_spec.rb:201-226).
 	if len(args) >= 2 {
 		if h, ok := args[1].(*object.Hash); ok {
 			port, user, userGiven, passwd, passwdGiven, acct = vm.ftpApplyOpts(f, h, port)
 		}
 	}
 
+	// No host: configured, not connected.
+	if len(args) == 0 || object.IsNil(args[0]) {
+		return
+	}
+	host := strArg(args[0])
+
 	vm.ftpConnect(f, host, port)
 	if userGiven {
 		vm.ftpLogin(f, user, passwd, passwdGiven, acct)
@@ -139,7 +175,13 @@ func (vm *VM) ftpApplyOpts(f *ftpObj, h *object.Hash, port int) (int, string, bo
 			port = clampInt(n)
 		}
 	}
-	if v, ok := ftpOptHash(h, "passive"); ok {
+	// ⛔ MRI tests the VALUE, not the key: `if options[:passive].nil?` falls back
+	// to @@default_passive (ftp.rb:286-290). So `passive: nil` is NOT a request
+	// for active mode — it is "no opinion". Reading the key's presence instead
+	// would make an explicit nil mean false, which is the opposite. This differs
+	// from read_timeout in the same Hash, where nil IS a request (for no bound),
+	// and the two are easy to conflate because they sit three lines apart.
+	if v, ok := ftpOptHash(h, "passive"); ok && !object.IsNil(v) {
 		f.passive = v.Truthy()
 	}
 	// open_timeout and read_timeout are set from the options Hash exactly as

internal/vm/net_ftp_bind.go

@@ -98,11 +98,28 @@ const ftpDefaultReadTimeout = 60
 
 // newFTPObj builds a Net::FTP with MRI's timeout defaults in place. Every
 // construction site goes through it so a new one cannot be born unbounded.
+// ftpDefaultPassive reads Net::FTP's @@default_passive. An absent or non-boolean
+// value reads as MRI's documented default, true: this is consulted on every
+// construction, and a class variable someone deleted must not silently flip the
+// transfer mode of the next connection.
+func ftpDefaultPassive(cls *RClass) bool {
+	if cls == nil {
+		return true
+	}
+	v, ok := cls.cvars["@@default_passive"]
+	if !ok || object.IsNil(v) {
+		return true
+	}
+	return v.Truthy()
+}
+
 func newFTPObj(vm *VM, cls *RClass) *ftpObj {
 	return &ftpObj{
-		cls:         cls,
-		binary:      true,
-		passive:     true,
+		cls:    cls,
+		binary: true,
+		// Read at CONSTRUCTION, as MRI does: Net::FTP.default_passive = false
+		// affects connections made after it, never one already open.
+		passive:     ftpDefaultPassive(cls),
 		openTimeout: object.NilV,
 		readTimeout: object.IntValue(ftpDefaultReadTimeout),
 		vm:          vm,

internal/vm/net_ftp_internal_test.go

@@ -1696,6 +1696,83 @@ func TestFTPTimeoutErrIgnoresANonTimeout(t *testing.T) {
 	ftpTimeoutErr(nil, "open")
 }
 
+// Net::FTP.default_passive, and the two things that made it worth running
+// against the reference rather than against our own expectations.
+//
+// ⛔ WHY IT MATTERS BEYOND THE ACCESSOR: ruby/spec's library/net-ftp/spec_helper.rb
+// is `if defined?(Net::FTP.default_passive) then Net::FTP.default_passive = false`,
+// and its fixture server implements PORT/EPRT with NO `pasv` method at all.
+// While the accessor was missing, that guard was skipped in SILENCE and all 56
+// files ran in a mode the fixture cannot speak (#916).
+//
+// ⛔ AND THE DEFECT THE COMPARISON FOUND: `Net::FTP.new(nil, passive: true)`
+// answered false. ftpInit returned before reading the options Hash whenever the
+// host was nil, so EVERY option was dropped on that path -- the shape of a
+// mechanism wired at only some of its doors, again. MRI applies the options
+// first and connects separately (net-ftp 0.3.9 lib/net/ftp.rb:285-300), which is
+// exactly what library/net-ftp/initialize_spec.rb:201-226 drives with
+// `@ftp.send(:initialize, nil, options)`.
+func TestFTPDefaultPassiveMatchesTheReference(t *testing.T) {
+	got := runSrc(t, `require "net/ftp"
+p Net::FTP.default_passive
+p Net::FTP.new.passive
+before = Net::FTP.new
+Net::FTP.default_passive = false
+p Net::FTP.default_passive
+p before.passive
+p Net::FTP.new.passive
+p Net::FTP.new(nil, passive: nil).passive
+p Net::FTP.new(nil, passive: true).passive
+p Net::FTP.new(nil, passive: false).passive
+Net::FTP.default_passive = true
+p Net::FTP.new(nil, passive: nil).passive
+p Net::FTP.new(nil, passive: false).passive
+p(Net::FTP.default_passive = false)
+p Net::FTP.class_variable_get(:@@default_passive)
+`)
+	// Each line is net-ftp 0.3.9's answer, read off lib/net/ftp.rb:96 (the
+	// default), :159-167 (the accessors) and :286-290 (nil means "no opinion",
+	// so it falls back to the default while false and true are requests).
+	want := "true\n" + // default_passive starts true
+		"true\n" + // so does a fresh connection
+		"false\n" + // the setter takes
+		"true\n" + // ⛔ a connection built BEFORE the change keeps its mode
+		"false\n" + // one built after follows it
+		"false\n" + // passive: nil is no opinion -> the default
+		"true\n" + // passive: true is a request, even with a nil host
+		"false\n" + // and so is passive: false
+		"true\n" + // default back to true: nil follows it
+		"false\n" + // while an explicit false still wins
+		"false\n" + // the writer returns its argument
+		"false" // and the value lives in the class variable MRI uses (runSrc trims the last newline)
+	if got != want {
+		t.Errorf("default_passive table:\ngot  %q\nwant %q", got, want)
+	}
+}
+
+// ftpDefaultPassive's fallbacks, which no Ruby program can reach: it is
+// consulted on EVERY construction, so a missing or nil class variable must read
+// as MRI's documented default rather than silently flipping the transfer mode of
+// the next connection. Asserted directly because "it cannot happen" is how a
+// default nobody chose gets shipped.
+func TestFTPDefaultPassiveFallsBackToMRIsDefault(t *testing.T) {
+	if !ftpDefaultPassive(nil) {
+		t.Error("a nil class must read as true, MRI's default")
+	}
+	cls := &RClass{cvars: map[string]object.Value{}}
+	if !ftpDefaultPassive(cls) {
+		t.Error("an absent @@default_passive must read as true")
+	}
+	cls.cvars["@@default_passive"] = object.NilV
+	if !ftpDefaultPassive(cls) {
+		t.Error("a nil @@default_passive must read as true, not as false")
+	}
+	cls.cvars["@@default_passive"] = object.Bool(false)
+	if ftpDefaultPassive(cls) {
+		t.Error("an explicit false must be honoured")
+	}
+}
+
 // ⛔ THE DEFECT SHAPE THIS REPOSITORY KEEPS REPEATING: a mechanism wired at only
 // SOME of its doors. open_timeout and read_timeout each have three doors -- the
 // constructor's options Hash, the attr writer, and the default -- and the Hash