fix: skip non-regular files matched by glob in model test

cmd/model/test.go

@@ -17,6 +17,7 @@ limitations under the License.
 package model
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -30,6 +31,14 @@ import (
 	"github.com/openfga/cli/internal/storetest"
 )
 
+// errAllTestFilesNonRegular is returned when a tests glob pattern matches only non-regular
+// files (e.g. FIFOs), so there is nothing safe to read.
+var errAllTestFilesNonRegular = errors.New("tests pattern matched only non-regular files (e.g. FIFOs); " +
+	"pass an explicit regular file path instead")
+
+// errNoTestFilesMatched is returned when a tests glob pattern matches no files at all.
+var errNoTestFilesMatched = errors.New("no test files matched pattern")
+
 // modelTestCmd represents the test command.
 var modelTestCmd = &cobra.Command{
 	Use:     "test",
@@ -72,18 +81,9 @@ var modelTestCmd = &cobra.Command{
 			MaxTypesPerAuthorizationModel: maxTypes,
 		}
 
-		fileNames, err := filepath.Glob(testsFileName)
+		fileNames, err := resolveTestFiles(testsFileName)
 		if err != nil {
-			return fmt.Errorf("invalid tests pattern %s due to %w", testsFileName, err)
-		}
-
-		if len(fileNames) == 0 {
-			// Check if the literal path exists
-			if _, err := os.Stat(testsFileName); err != nil {
-				return fmt.Errorf("test file %s does not exist: %w", testsFileName, err)
-			}
-
-			fileNames = []string{testsFileName}
+			return err
 		}
 
 		multipleFiles := len(fileNames) > 1
@@ -169,6 +169,59 @@ var modelTestCmd = &cobra.Command{
 	},
 }
 
+// resolveTestFiles turns testsPattern into the list of test files to read.
+//
+// A pattern with no glob metacharacters (*, ?, [) is treated as an explicit, literal path
+// and honored as-is - including non-regular paths such as process substitution
+// (--tests <(...)), which resolves to a FIFO like /dev/fd/11. The caller asked for that
+// exact path, so it is not subject to the regular-file filter below.
+//
+// A pattern with glob metacharacters is expanded via filepath.Glob, and non-regular matches
+// (FIFOs, devices, sockets) are dropped: a glob is not an explicit request for any single
+// file, and reading a FIFO with no writer via os.ReadFile blocks forever. If the glob matches
+// only non-regular files, that is an error rather than something to read.
+//
+// The metacharacter check (rather than, say, comparing the single glob match against the
+// pattern) is deliberate: a FIFO literally named "*.fga.yaml" must still be filtered out, not
+// mistaken for a literal path.
+func resolveTestFiles(testsPattern string) ([]string, error) {
+	if !strings.ContainsAny(testsPattern, `*?[`) {
+		if _, statErr := os.Stat(testsPattern); statErr != nil {
+			return nil, fmt.Errorf("test file %s does not exist: %w", testsPattern, statErr)
+		}
+
+		return []string{testsPattern}, nil
+	}
+
+	rawMatches, err := filepath.Glob(testsPattern)
+	if err != nil {
+		return nil, fmt.Errorf("invalid tests pattern %s due to %w", testsPattern, err)
+	}
+
+	if len(rawMatches) == 0 {
+		return nil, fmt.Errorf("%w: %s", errNoTestFilesMatched, testsPattern)
+	}
+
+	regularFileNames := rawMatches[:0]
+
+	for _, name := range rawMatches {
+		info, statErr := os.Stat(name)
+		if statErr != nil {
+			return nil, fmt.Errorf("failed to stat test file %s: %w", name, statErr)
+		}
+
+		if info.Mode().IsRegular() {
+			regularFileNames = append(regularFileNames, name)
+		}
+	}
+
+	if len(regularFileNames) == 0 {
+		return nil, fmt.Errorf("%w: %s", errAllTestFilesNonRegular, testsPattern)
+	}
+
+	return regularFileNames, nil
+}
+
 func init() {
 	modelTestCmd.Flags().String("store-id", "", "Store ID")
 	modelTestCmd.Flags().String("model-id", "", "Model ID")

cmd/model/test_test.go

@@ -0,0 +1,57 @@
+package model
+
+import (
+	"fmt"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+// writeRegularFile writes a minimal regular test file at path. Shared by both this file and
+// test_unix_test.go.
+func writeRegularFile(t *testing.T, path string) error {
+	t.Helper()
+
+	if err := os.WriteFile(path, []byte("name: ok\n"), 0o600); err != nil {
+		return fmt.Errorf("failed to write test file %s: %w", path, err)
+	}
+
+	return nil
+}
+
+// A glob pattern (contains metacharacters) that matches no files should fail with a
+// not-found/no-match error rather than being treated as a literal path or returning an empty
+// result.
+func TestResolveTestFilesNoMatchesFails(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	pattern := filepath.Join(dir, "*.fga.yaml")
+
+	fileNames, err := resolveTestFiles(pattern)
+	if err == nil {
+		t.Fatalf("expected an error, got fileNames=%v", fileNames)
+	}
+}
+
+// An explicitly named, existing regular file (no glob metacharacters) is honored as-is - this
+// preserves existing behavior for explicitly named paths.
+func TestResolveTestFilesLiteralExistingPath(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	path := filepath.Join(dir, "ok.fga.yaml")
+
+	if err := writeRegularFile(t, path); err != nil {
+		t.Fatalf("failed to write test file: %v", err)
+	}
+
+	fileNames, err := resolveTestFiles(path)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if len(fileNames) != 1 || fileNames[0] != path {
+		t.Fatalf("expected [%s], got %v", path, fileNames)
+	}
+}

cmd/model/test_unix_test.go

@@ -0,0 +1,177 @@
+//go:build unix
+
+package model
+
+import (
+	"path/filepath"
+	"sort"
+	"syscall"
+	"testing"
+	"time"
+)
+
+// Regression test for https://github.com/openfga/cli/issues/739
+// A glob match that is a FIFO with no writer must never be handed to
+// storetest.ReadFromFile, since os.ReadFile on such a FIFO blocks forever.
+//
+// This lives in a Unix-only file because syscall.Mkfifo does not exist on Windows, so a
+// runtime GOOS skip would not prevent a compile failure there.
+func TestResolveTestFilesSkipsNonRegularGlobMatches(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+
+	regularPath := filepath.Join(dir, "ok.fga.yaml")
+	if err := writeRegularFile(t, regularPath); err != nil {
+		t.Fatalf("failed to write regular test file: %v", err)
+	}
+
+	fifoPath := filepath.Join(dir, "evil.fga.yaml")
+	if err := syscall.Mkfifo(fifoPath, 0o600); err != nil {
+		t.Skipf("unable to create FIFO: %v", err)
+	}
+
+	pattern := filepath.Join(dir, "*.fga.yaml")
+
+	fileNames, err := resolveTestFiles(pattern)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if len(fileNames) != 1 || fileNames[0] != regularPath {
+		t.Fatalf("expected [%s], got %v", regularPath, fileNames)
+	}
+}
+
+// Multiple regular files matched by the glob should all be returned, in addition to any
+// non-regular files being dropped.
+func TestResolveTestFilesMultipleRegularFiles(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+
+	first := filepath.Join(dir, "a.fga.yaml")
+	second := filepath.Join(dir, "b.fga.yaml")
+
+	for _, p := range []string{first, second} {
+		if err := writeRegularFile(t, p); err != nil {
+			t.Fatalf("failed to write regular test file: %v", err)
+		}
+	}
+
+	if err := syscall.Mkfifo(filepath.Join(dir, "c.fga.yaml"), 0o600); err != nil {
+		t.Skipf("unable to create FIFO: %v", err)
+	}
+
+	pattern := filepath.Join(dir, "*.fga.yaml")
+
+	fileNames, err := resolveTestFiles(pattern)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	sort.Strings(fileNames)
+
+	if len(fileNames) != 2 || fileNames[0] != first || fileNames[1] != second {
+		t.Fatalf("expected [%s %s], got %v", first, second, fileNames)
+	}
+}
+
+// If every glob match is non-regular, resolution must fail with a clear error rather than
+// silently falling back to treating the glob pattern string itself as a literal path.
+func TestResolveTestFilesAllMatchesNonRegular(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+
+	if err := syscall.Mkfifo(filepath.Join(dir, "evil.fga.yaml"), 0o600); err != nil {
+		t.Skipf("unable to create FIFO: %v", err)
+	}
+
+	pattern := filepath.Join(dir, "*.fga.yaml")
+
+	fileNames, err := resolveTestFiles(pattern)
+	if err == nil {
+		t.Fatalf("expected an error, got fileNames=%v", fileNames)
+	}
+}
+
+// End-to-end regression test through the actual modelTestCmd path (not resolveTestFiles
+// directly), per review feedback. Before the fix, a FIFO picked up by the --tests glob made
+// the command block forever in os.ReadFile. Here the command must return within a short
+// deadline instead of hanging; whatever error it returns afterwards (e.g. from running the
+// tests) is irrelevant to this regression.
+func TestModelTestCmdDoesNotHangOnFifoGlobMatch(t *testing.T) { //nolint:paralleltest // mutates the shared global modelTestCmd, so it must not run in parallel
+	dir := t.TempDir()
+
+	regularPath := filepath.Join(dir, "ok.fga.yaml")
+	if err := writeRegularFile(t, regularPath); err != nil {
+		t.Fatalf("failed to write regular test file: %v", err)
+	}
+
+	if err := syscall.Mkfifo(filepath.Join(dir, "evil.fga.yaml"), 0o600); err != nil {
+		t.Skipf("unable to create FIFO: %v", err)
+	}
+
+	modelTestCmd.SetArgs([]string{"--tests", filepath.Join(dir, "*.fga.yaml")})
+
+	done := make(chan struct{})
+
+	go func() {
+		// The command may return an error (e.g. no reachable FGA server); we only care that
+		// it returns at all rather than blocking on the FIFO read.
+		_ = modelTestCmd.Execute()
+
+		close(done)
+	}()
+
+	select {
+	case <-done:
+	case <-time.After(10 * time.Second):
+		t.Fatal("modelTestCmd hung on a FIFO glob match instead of skipping it")
+	}
+}
+
+// An explicitly named non-regular path (no glob metacharacters) must be honored as-is, not
+// rejected by the regular-file filter. This is what keeps process substitution
+// (--tests <(...), which the shell turns into a FIFO like /dev/fd/11) working. The literal-
+// path test in test_test.go uses a regular file, so it cannot catch this case.
+func TestResolveTestFilesLiteralFifoPathHonored(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+	fifoPath := filepath.Join(dir, "pipe.fga.yaml")
+
+	if err := syscall.Mkfifo(fifoPath, 0o600); err != nil {
+		t.Skipf("unable to create FIFO: %v", err)
+	}
+
+	fileNames, err := resolveTestFiles(fifoPath)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+
+	if len(fileNames) != 1 || fileNames[0] != fifoPath {
+		t.Fatalf("expected [%s], got %v", fifoPath, fileNames)
+	}
+}
+
+// A FIFO literally named "*.fga.yaml" must be treated as a glob match and filtered out, not
+// mistaken for a literal path - otherwise it would be read and hang. This guards the
+// metacharacter-based branch against a naive "single match equals the pattern" shortcut.
+func TestResolveTestFilesFifoNamedLikeGlobStillFiltered(t *testing.T) {
+	t.Parallel()
+
+	dir := t.TempDir()
+
+	if err := syscall.Mkfifo(filepath.Join(dir, "*.fga.yaml"), 0o600); err != nil {
+		t.Skipf("unable to create FIFO: %v", err)
+	}
+
+	pattern := filepath.Join(dir, "*.fga.yaml")
+
+	fileNames, err := resolveTestFiles(pattern)
+	if err == nil {
+		t.Fatalf("expected an error, got fileNames=%v", fileNames)
+	}
+}