diff --git a/README.md b/README.md index edb1f46..dd16a79 100644 --- a/README.md +++ b/README.md @@ -40,7 +40,7 @@ tree by URL: bin/mfer gen . # Verify the files on disk against the manifest. Exits nonzero if any file -# is missing or corrupted. +# it lists is missing or corrupted; warns about files it does not list. bin/mfer check index.mf # Download and cryptographically verify a tree published over HTTP: mfer @@ -251,6 +251,9 @@ are now tracked only in the [issues](https://git.eeqj.de/sneak/mfer/issues). - `mfer check` / `mfer check .` - verifies checksums of all files in manifest, displaying error and exiting nonzero if any files are missing or corrupted + - warns about each file under the base directory that the manifest does not + list, hidden files included; with `--no-extra-files` each one is a failure + instead - `mfer fetch https://example.com/stuff/` - fetches `/stuff/index.mf` and downloads all files listed in manifest, optionally resuming any that already exist locally, and assures diff --git a/internal/cli/check.go b/internal/cli/check.go index 520c043..acb99de 100644 --- a/internal/cli/check.go +++ b/internal/cli/check.go @@ -206,16 +206,21 @@ func countCheckFailures( } // findExtraFiles reports files present on disk but absent from the -// manifest, counting each as a failure. +// manifest, and anything the search cannot read: each is a failure under +// --no-extra-files, otherwise a warning. func findExtraFiles(ctx *cli.Context, chk *mfer.Checker, failures *int64) error { extraResults := make(chan mfer.Result, 1) extraDone := make(chan struct{}) go func() { for result := range extraResults { - *failures++ + if ctx.Bool("no-extra-files") { + *failures++ - log.Infof("%s: %s (%s)", result.Status, result.Path, result.Message) + log.Infof("%s: %s (%s)", result.Status, result.Path, result.Message) + } else { + log.Warnf("%s: %s (%s)", result.Status, result.Path, result.Message) + } } close(extraDone) @@ -270,12 +275,9 @@ func runCheck(ctx *cli.Context, chk *mfer.Checker, showProgress bool) (int64, er // Wait for results processing to complete <-done - // Check for extra files if requested - if ctx.Bool("no-extra-files") { - err = findExtraFiles(ctx, chk, &failures) - if err != nil { - return 0, err - } + err = findExtraFiles(ctx, chk, &failures) + if err != nil { + return 0, err } return failures, nil diff --git a/internal/cli/entry_test.go b/internal/cli/entry_test.go index fcfe1b7..e5e8a31 100644 --- a/internal/cli/entry_test.go +++ b/internal/cli/entry_test.go @@ -705,30 +705,155 @@ func TestNoExtraFilesWithSubdirectory(t *testing.T) { "check should fail when extra files exist in subdirectory") } -func TestCheckWithoutNoExtraFilesIgnoresExtra(t *testing.T) { +// TestCheckWarnsAboutUnlistedFiles adds one file the manifest does not list +// to a tree that had none: it gets one warning and the check passes, unless +// --no-extra-files makes it a failure. --quiet hides the warning, not the +// failure. +func TestCheckWarnsAboutUnlistedFiles(t *testing.T) { t.Parallel() - fs := afero.NewMemMapFs() + for name, unlisted := range map[string]string{ + "regular file": "extra.txt", + "dotfile": ".hidden", + "file in hidden directory": ".git/config", + } { + t.Run(name, func(t *testing.T) { + t.Parallel() - // Create test file - require.NoError(t, fs.MkdirAll(testDir, 0o755)) - writeTestFile(t, fs, testFile1, "hello") + fs := afero.NewMemMapFs() + require.NoError(t, fs.MkdirAll(testDir, 0o755)) + writeTestFile(t, fs, testFile1, "hello") - // Generate manifest - opts := testOpts([]string{testApp, cmdGenerate, "-q", "-o", testManifest, testDir}, fs) - exitCode := runCLI(opts) - require.Equal(t, 0, exitCode) + opts := testOpts([]string{ + testApp, cmdGenerate, "-q", "-o", testManifest, testDir, + }, fs) + require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) - // Add extra file - writeTestFile(t, fs, "/testdir/extra.txt", "extra") + check := func(flags ...string) (int, string) { + args := append([]string{testApp, cmdCheck, testFlagBase, testDir}, flags...) + opts := testOpts(append(args, testManifest), fs) + + return runCLI(opts), testStderr(t, opts) + } + + exitCode, stderr := check() + assert.Equal(t, 0, exitCode, "stderr: %s", stderr) + assert.NotContains(t, stderr, "not in manifest") + + writeTestFile(t, fs, filepath.Join(testDir, unlisted), "unlisted") + + exitCode, stderr = check() + assert.Equal(t, 0, exitCode, "stderr: %s", stderr) + assert.Equal(t, 1, strings.Count(stderr, "not in manifest"), stderr) + assert.Contains(t, stderr, unlisted) + + exitCode, stderr = check("-q") + assert.Equal(t, 0, exitCode, "stderr: %s", stderr) + assert.NotContains(t, stderr, "not in manifest") + + exitCode, stderr = check(testFlagNoExtra) + assert.Equal(t, 1, exitCode, "stderr: %s", stderr) + assert.Contains(t, stderr, unlisted) + + exitCode, _ = check("-q", testFlagNoExtra) + assert.Equal(t, 1, exitCode) + }) + } +} + +// TestCheckNeverReportsManifest keeps the manifest inside the checked tree, +// under several names and path spellings, and names the tree both directly +// and through a symlink: the manifest is never reported, even under +// --no-extra-files. The manifest is recognized by file identity, which needs +// the real filesystem. +func TestCheckNeverReportsManifest(t *testing.T) { + t.Parallel() + + // Manifest paths are relative to the checked tree. + for name, manifest := range map[string]string{ + "default name": defaultManifestName, + "hidden name": ".index.mf", + "other name in a subdirectory": "sub/listing.mf", + "path spelled through ..": "sub/../index.mf", + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + + // A temp dir holding data/tree and link, a symlink to data. + root := t.TempDir() + tree := filepath.Join(root, "data", "tree") + // Not filepath.Join, which would clean away a "..". + manifestPath := tree + "/" + manifest + + fs := afero.NewOsFs() + require.NoError(t, fs.MkdirAll(filepath.Join(tree, "sub"), 0o750)) + require.NoError(t, + os.Symlink(filepath.Join(root, "data"), filepath.Join(root, "link"))) + writeTestFile(t, fs, filepath.Join(tree, testFileTxt), "hello") + + opts := testOpts([]string{ + testApp, cmdGenerate, "-q", "-o", manifestPath, tree, + }, fs) + require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) + + for _, base := range []string{tree, filepath.Join(root, "link", "tree")} { + opts = testOpts([]string{ + testApp, cmdCheck, testFlagNoExtra, testFlagBase, base, manifestPath, + }, fs) + assert.Equal(t, 0, runCLI(opts), + "base %s, stderr: %s", base, testStderr(t, opts)) + assert.NotContains(t, testStderr(t, opts), "not in manifest") + } + }) + } +} + +// unlistableDirFs is a filesystem on which one directory cannot be listed. +type unlistableDirFs struct { + afero.Fs + + dir string +} + +//nolint:ireturn // Open must return afero.File to satisfy afero.Fs. +func (f unlistableDirFs) Open(name string) (afero.File, error) { + if name == f.dir { + return nil, os.ErrPermission + } + + return f.Fs.Open(name) +} + +// TestCheckWithUnlistableDirectory has a directory under the base that +// cannot be listed, followed by an unlisted file: both are warned about and +// the check still passes, unless --no-extra-files is given. +func TestCheckWithUnlistableDirectory(t *testing.T) { + t.Parallel() + + mem := afero.NewMemMapFs() + require.NoError(t, mem.MkdirAll("/testdir/locked", 0o755)) + writeTestFile(t, mem, testFile1, "hello") + + opts := testOpts([]string{ + testApp, cmdGenerate, "-q", "-o", testManifest, testDir, + }, mem) + require.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) + + // Directories are searched in name order, so this comes after "locked". + writeTestFile(t, mem, "/testdir/unlisted.txt", "unlisted") + + fs := unlistableDirFs{Fs: mem, dir: "/testdir/locked"} + + opts = testOpts([]string{testApp, cmdCheck, testFlagBase, testDir, testManifest}, fs) + assert.Equal(t, 0, runCLI(opts), "stderr: %s", testStderr(t, opts)) + assert.Contains(t, testStderr(t, opts), os.ErrPermission.Error()) + assert.Contains(t, testStderr(t, opts), "unlisted.txt") - // Check WITHOUT --no-extra-files (should pass - extra files ignored) opts = testOpts([]string{ - testApp, cmdCheck, "-q", testFlagBase, testDir, testManifest, + testApp, cmdCheck, testFlagNoExtra, testFlagBase, testDir, testManifest, }, fs) - exitCode = runCLI(opts) - assert.Equal(t, 0, exitCode, - "check without --no-extra-files should ignore extra files") + assert.Equal(t, 1, runCLI(opts), "stderr: %s", testStderr(t, opts)) + assert.Contains(t, testStderr(t, opts), "unlisted.txt") } func TestGenerateAtomicWriteNoTempFileOnSuccess(t *testing.T) { diff --git a/internal/cli/mfer.go b/internal/cli/mfer.go index 0e12299..fe848b4 100644 --- a/internal/cli/mfer.go +++ b/internal/cli/mfer.go @@ -227,7 +227,7 @@ func (mfa *CLIApp) checkCommand() *cli.Command { }, &cli.BoolFlag{ Name: "no-extra-files", - Usage: "Fail if files exist in base directory that are not in manifest", + Usage: "Fail, instead of warning, if files in base directory are not in manifest", }, &cli.StringFlag{ Name: "require-signature", diff --git a/mfer/checker.go b/mfer/checker.go index 0b4fd37..2f37203 100644 --- a/mfer/checker.go +++ b/mfer/checker.go @@ -77,9 +77,9 @@ type Checker struct { fs afero.Fs // manifestPaths is a set of paths in the manifest for quick lookup manifestPaths map[RelFilePath]struct{} - // manifestRelPath is the relative path of the manifest file from - // basePath (for exclusion) - manifestRelPath RelFilePath + // manifestInfo is the manifest file, which FindExtraFiles leaves out, + // matched with os.SameFile. + manifestInfo os.FileInfo // signature info from the manifest signature []byte signer []byte @@ -133,26 +133,20 @@ func NewChecker(opts *CheckerOptions) (*Checker, error) { manifestPaths[RelFilePath(f.GetPath())] = struct{}{} } - // Compute manifest's relative path from basePath for exclusion in FindExtraFiles - absManifest, err := filepath.Abs(opts.ManifestPath) + manifestInfo, err := fs.Stat(opts.ManifestPath) if err != nil { return nil, err } - manifestRel, err := filepath.Rel(abs, absManifest) - if err != nil { - manifestRel = "" - } - return &Checker{ - basePath: AbsFilePath(abs), - files: files, - fs: fs, - manifestPaths: manifestPaths, - manifestRelPath: RelFilePath(manifestRel), - signature: m.pbOuter.GetSignature(), - signer: m.pbOuter.GetSigner(), - signingPubKey: m.pbOuter.GetSigningPubKey(), + basePath: AbsFilePath(abs), + files: files, + fs: fs, + manifestPaths: manifestPaths, + manifestInfo: manifestInfo, + signature: m.pbOuter.GetSignature(), + signer: m.pbOuter.GetSigner(), + signingPubKey: m.pbOuter.GetSigningPubKey(), }, nil } @@ -273,20 +267,27 @@ func (c *Checker) Check( return nil } -// FindExtraFiles walks the filesystem and reports files not in the manifest. -// Results are sent to the results channel. The channel is closed when done. -// Hidden files/directories (starting with .) are skipped, as they are excluded -// from manifests by default. The manifest file itself is also skipped. +// FindExtraFiles walks the filesystem and reports files not in the manifest, +// hidden files and directories included. The manifest file itself is not +// reported. Anything the search cannot read, such as a directory that cannot +// be listed, is reported with StatusError and the search goes on. Results are +// sent to the results channel. The channel is closed when done. func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) error { if results != nil { defer close(results) } - walkFn := func(walkPath string, info os.FileInfo, err error) error { - if err != nil { - return err - } + // The search does not follow symlinks, so a base directory named + // through one is resolved first. If that fails, the base is searched as + // named and the search reports the problem. + root := string(c.basePath) + resolved, err := filepath.EvalSymlinks(root) + if err == nil { + root = resolved + } + + walkFn := func(walkPath string, info os.FileInfo, walkErr error) error { select { case <-ctx.Done(): return ctx.Err() @@ -294,15 +295,22 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err } // Get relative path - rel, err := filepath.Rel(string(c.basePath), walkPath) + rel, err := filepath.Rel(root, walkPath) if err != nil { return err } - // Skip hidden files and directories (dotfiles) - if IsHiddenPath(filepath.ToSlash(rel)) { - if info.IsDir() { - return filepath.SkipDir + relPath := RelFilePath(rel) + + // Report what cannot be read, such as a directory that cannot be + // listed, and go on with the rest. + if walkErr != nil { + if results != nil { + results <- Result{ + Path: relPath, + Status: StatusError, + Message: walkErr.Error(), + } } return nil @@ -313,10 +321,17 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err return nil } - relPath := RelFilePath(rel) + // A symlink is compared by what it points to, so a manifest reached + // through one is not reported either. + if info.Mode()&os.ModeSymlink != 0 { + target, statErr := c.fs.Stat(walkPath) + if statErr == nil { + info = target + } + } - // Skip the manifest file itself - if relPath == c.manifestRelPath { + // Skip the manifest file itself, however its path is spelled + if os.SameFile(info, c.manifestInfo) { return nil } @@ -334,7 +349,7 @@ func (c *Checker) FindExtraFiles(ctx context.Context, results chan<- Result) err return nil } - return afero.Walk(c.fs, string(c.basePath), walkFn) + return afero.Walk(c.fs, root, walkFn) } func (c *Checker) checkFile(entry *MFFilePath, checkedBytes *FileSize) Result { diff --git a/mfer/checker_test.go b/mfer/checker_test.go index d00350d..92f6faa 100644 --- a/mfer/checker_test.go +++ b/mfer/checker_test.go @@ -21,8 +21,6 @@ const ( testExistsFile = "exists.txt" testManifestPath = "/manifest.mf" testDataDir = "/data" - // testDataManifestPath is a manifest kept inside the checked tree. - testDataManifestPath = testDataDir + "/index.mf" ) func TestStatusString(t *testing.T) { @@ -459,50 +457,120 @@ func TestFindExtraFiles(t *testing.T) { assert.Equal(t, "not in manifest", extras[0].Message) } -func TestFindExtraFilesSkipsManifestAndDotfiles(t *testing.T) { +// TestFindExtraFilesReportsHiddenFilesButNotManifest keeps the manifest +// inside the checked tree: hidden files and directories are reported, the +// manifest is not. The manifest is recognized by file identity, which needs +// the real filesystem. +func TestFindExtraFilesReportsHiddenFilesButNotManifest(t *testing.T) { t.Parallel() - fs := afero.NewMemMapFs() - manifestFiles := map[string][]byte{ - testFile1: []byte("in manifest"), - } - createTestManifest(t, fs, testDataManifestPath, manifestFiles) - createFilesOnDisk(t, fs, map[string][]byte{ + dir := t.TempDir() + manifestPath := filepath.Join(dir, "index.mf") + + fs := afero.NewOsFs() + createTestManifest(t, fs, manifestPath, map[string][]byte{ testFile1: []byte("in manifest"), }) - // Create dotfile and manifest that should be skipped - require.NoError(t, afero.WriteFile(fs, "/data/.hidden", []byte("hidden"), 0o644)) - require.NoError(t, afero.WriteFile(fs, "/data/.config/settings", []byte("cfg"), 0o644)) - // Create a real extra file - require.NoError(t, fs.MkdirAll(testDataDir, 0o755)) - require.NoError(t, afero.WriteFile(fs, "/data/extra.txt", []byte("extra"), 0o644)) + + unlisted := []RelFilePath{"extra.txt", ".hidden", ".git/config"} + for _, p := range append([]RelFilePath{testFile1}, unlisted...) { + path := filepath.Join(dir, string(p)) + require.NoError(t, fs.MkdirAll(filepath.Dir(path), 0o750)) + require.NoError(t, afero.WriteFile(fs, path, []byte("x"), 0o600)) + } chk, err := NewChecker(&CheckerOptions{ - ManifestPath: testDataManifestPath, - BasePath: testDataDir, + ManifestPath: manifestPath, + BasePath: dir, Fs: fs, }) require.NoError(t, err) results := make(chan Result, 10) - err = chk.FindExtraFiles(context.Background(), results) + require.NoError(t, chk.FindExtraFiles(context.Background(), results)) + + var extras []RelFilePath + for r := range results { + extras = append(extras, r.Path) + } + + assert.ElementsMatch(t, unlisted, extras) +} + +// TestFindExtraFilesSkipsManifestReachedThroughSymlink checks a tree whose +// index.mf is a symlink to the manifest kept outside the tree: the symlink is +// not reported. +func TestFindExtraFilesSkipsManifestReachedThroughSymlink(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + tree := filepath.Join(dir, "tree") + manifestPath := filepath.Join(dir, "real.mf") + linkPath := filepath.Join(tree, "index.mf") + + fs := afero.NewOsFs() + createTestManifest(t, fs, manifestPath, map[string][]byte{testFile1: []byte("x")}) + require.NoError(t, fs.MkdirAll(tree, 0o750)) + require.NoError(t, + afero.WriteFile(fs, filepath.Join(tree, testFile1), []byte("x"), 0o600)) + require.NoError(t, os.Symlink(manifestPath, linkPath)) + + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: linkPath, + BasePath: tree, + Fs: fs, + }) require.NoError(t, err) - var extras []Result + results := make(chan Result, 10) + require.NoError(t, chk.FindExtraFiles(context.Background(), results)) + + var extras []RelFilePath for r := range results { - extras = append(extras, r) + extras = append(extras, r.Path) } - // Should only report extra.txt, not .hidden, .config/settings, or index.mf - for _, e := range extras { - t.Logf("extra: %s", e.Path) + assert.Empty(t, extras) +} + +// TestFindExtraFilesSearchesBaseNamedThroughSymlink names the checked tree +// through a symlink to it: the files in the tree are searched, and the +// symlink itself is not reported. +func TestFindExtraFilesSearchesBaseNamedThroughSymlink(t *testing.T) { + t.Parallel() + + dir := t.TempDir() + tree := filepath.Join(dir, "tree") + link := filepath.Join(dir, "link") + manifestPath := filepath.Join(dir, "index.mf") + + fs := afero.NewOsFs() + createTestManifest(t, fs, manifestPath, map[string][]byte{testFile1: []byte("x")}) + require.NoError(t, fs.MkdirAll(tree, 0o750)) + + for _, name := range []string{testFile1, testFile2} { + require.NoError(t, + afero.WriteFile(fs, filepath.Join(tree, name), []byte("x"), 0o600)) } - assert.Len(t, extras, 1) + require.NoError(t, os.Symlink(tree, link)) - if len(extras) > 0 { - assert.Equal(t, RelFilePath("extra.txt"), extras[0].Path) + chk, err := NewChecker(&CheckerOptions{ + ManifestPath: manifestPath, + BasePath: link, + Fs: fs, + }) + require.NoError(t, err) + + results := make(chan Result, 10) + require.NoError(t, chk.FindExtraFiles(context.Background(), results)) + + var extras []RelFilePath + for r := range results { + extras = append(extras, r.Path) } + + assert.Equal(t, []RelFilePath{testFile2}, extras) } func TestFindExtraFilesContextCancellation(t *testing.T) { @@ -649,77 +717,6 @@ func TestCheckMissingFileDetectedWithoutFallback(t *testing.T) { assert.Equal(t, 0, statusCounts[StatusError], "no files should be ERROR") } -func TestFindExtraFilesSkipsDotfiles(t *testing.T) { - t.Parallel() - - // Regression test for #16: FindExtraFiles should not report dotfiles - // or the manifest file itself as extra files. - fs := afero.NewMemMapFs() - files := map[string][]byte{ - testFile1: []byte("in manifest"), - } - createTestManifest(t, fs, testDataManifestPath, files) - createFilesOnDisk(t, fs, files) - - // Add dotfiles and manifest file on disk - require.NoError(t, afero.WriteFile(fs, "/data/.hidden", []byte("dotfile"), 0o644)) - require.NoError(t, fs.MkdirAll("/data/.git", 0o755)) - require.NoError(t, - afero.WriteFile(fs, "/data/.git/config", []byte("git config"), 0o644)) - - chk, err := NewChecker(&CheckerOptions{ - ManifestPath: testDataManifestPath, - BasePath: testDataDir, - Fs: fs, - }) - require.NoError(t, err) - - results := make(chan Result, 10) - err = chk.FindExtraFiles(context.Background(), results) - require.NoError(t, err) - - var extras []Result - for r := range results { - extras = append(extras, r) - } - - // Should report NO extra files — dotfiles and manifest should be skipped - assert.Empty(t, extras, - "FindExtraFiles should not report dotfiles or manifest file as extra; got: %v", - extras) -} - -func TestFindExtraFilesSkipsManifestFile(t *testing.T) { - t.Parallel() - - // The manifest file itself should never be reported as extra - fs := afero.NewMemMapFs() - files := map[string][]byte{ - testFile1: []byte("content"), - } - createTestManifest(t, fs, testDataManifestPath, files) - createFilesOnDisk(t, fs, files) - - chk, err := NewChecker(&CheckerOptions{ - ManifestPath: testDataManifestPath, - BasePath: testDataDir, - Fs: fs, - }) - require.NoError(t, err) - - results := make(chan Result, 10) - err = chk.FindExtraFiles(context.Background(), results) - require.NoError(t, err) - - var extras []Result - for r := range results { - extras = append(extras, r) - } - - assert.Empty(t, extras, - "manifest file should not be reported as extra; got: %v", extras) -} - func TestCheckEmptyManifest(t *testing.T) { t.Parallel()