diff --git a/cmd/model/test.go b/cmd/model/test.go index 1ac74956..82d5a37f 100644 --- a/cmd/model/test.go +++ b/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") diff --git a/cmd/model/test_test.go b/cmd/model/test_test.go new file mode 100644 index 00000000..79afdd20 --- /dev/null +++ b/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) + } +} diff --git a/cmd/model/test_unix_test.go b/cmd/model/test_unix_test.go new file mode 100644 index 00000000..26f19034 --- /dev/null +++ b/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) + } +}