From 6a97833337b2d85584a2a7e9af39266790834b44 Mon Sep 17 00:00:00 2001
From: Michael Terry <mike@mterry.name>
Date: Sat, 27 Jul 2024 18:55:00 -0400
Subject: [PATCH] restore: clean up error handling when restoring metadata

- Fix a logic error that instead of reporting the *first*
  metadata-setting error that appears, we were instead reporting the
  *last* error (and only if the lchown call failed!).
- Don't show any errors when setting metadata for files in non-root
  mode (things like timestamps, attributes). Previously, only lchown
  errors were skipped. But other kinds of attribute errors make sense
  to skip as well. The code path happened to work correctly before
  because of the above logic error. But once that was fixed, this
  change needed to happen too.
---
 changelog/unreleased/pull-4958 |  7 +++++++
 internal/restic/node.go        | 24 ++++++++++++------------
 internal/restic/node_test.go   | 12 ++++++++++++
 3 files changed, 31 insertions(+), 12 deletions(-)
 create mode 100644 changelog/unreleased/pull-4958

diff --git a/changelog/unreleased/pull-4958 b/changelog/unreleased/pull-4958
new file mode 100644
index 000000000..bbb28a97b
--- /dev/null
+++ b/changelog/unreleased/pull-4958
@@ -0,0 +1,7 @@
+Bugfix: Don't ignore metadata-setting errors during restore
+
+Restic was accidentally ignoring errors when setting timestamps,
+attributes, or file modes during restore. It will now report those
+errors (unless it's just a permission error when not running as root).
+
+https://github.com/restic/restic/pull/4958
diff --git a/internal/restic/node.go b/internal/restic/node.go
index 51c6071b7..7c1988227 100644
--- a/internal/restic/node.go
+++ b/internal/restic/node.go
@@ -229,6 +229,13 @@ func (node *Node) CreateAt(ctx context.Context, path string, repo BlobLoader) er
 func (node Node) RestoreMetadata(path string, warn func(msg string)) error {
 	err := node.restoreMetadata(path, warn)
 	if err != nil {
+		// It is common to have permission errors for folders like /home
+		// unless you're running as root, so ignore those.
+		if os.Geteuid() > 0 && errors.Is(err, os.ErrPermission) {
+			debug.Log("not running as root, ignoring permission error for %v: %v",
+				path, err)
+			return nil
+		}
 		debug.Log("restoreMetadata(%s) error %v", path, err)
 	}
 
@@ -239,33 +246,26 @@ func (node Node) restoreMetadata(path string, warn func(msg string)) error {
 	var firsterr error
 
 	if err := lchown(path, int(node.UID), int(node.GID)); err != nil {
-		// Like "cp -a" and "rsync -a" do, we only report lchown permission errors
-		// if we run as root.
-		if os.Geteuid() > 0 && os.IsPermission(err) {
-			debug.Log("not running as root, ignoring lchown permission error for %v: %v",
-				path, err)
-		} else {
-			firsterr = errors.WithStack(err)
-		}
+		firsterr = errors.WithStack(err)
 	}
 
 	if err := node.RestoreTimestamps(path); err != nil {
 		debug.Log("error restoring timestamps for dir %v: %v", path, err)
-		if firsterr != nil {
+		if firsterr == nil {
 			firsterr = err
 		}
 	}
 
 	if err := node.restoreExtendedAttributes(path); err != nil {
 		debug.Log("error restoring extended attributes for %v: %v", path, err)
-		if firsterr != nil {
+		if firsterr == nil {
 			firsterr = err
 		}
 	}
 
 	if err := node.restoreGenericAttributes(path, warn); err != nil {
 		debug.Log("error restoring generic attributes for %v: %v", path, err)
-		if firsterr != nil {
+		if firsterr == nil {
 			firsterr = err
 		}
 	}
@@ -275,7 +275,7 @@ func (node Node) restoreMetadata(path string, warn func(msg string)) error {
 	// calls above would fail.
 	if node.Type != "symlink" {
 		if err := fs.Chmod(path, node.Mode); err != nil {
-			if firsterr != nil {
+			if firsterr == nil {
 				firsterr = errors.WithStack(err)
 			}
 		}
diff --git a/internal/restic/node_test.go b/internal/restic/node_test.go
index 6e0f31e21..7991d33e0 100644
--- a/internal/restic/node_test.go
+++ b/internal/restic/node_test.go
@@ -13,6 +13,7 @@ import (
 	"time"
 
 	"github.com/google/go-cmp/cmp"
+	"github.com/restic/restic/internal/errors"
 	"github.com/restic/restic/internal/test"
 	rtest "github.com/restic/restic/internal/test"
 )
@@ -382,3 +383,14 @@ func TestSymlinkSerializationFormat(t *testing.T) {
 		test.Assert(t, n2.LinkTargetRaw == nil, "quoted link target is just a helper field and must be unset after decoding")
 	}
 }
+
+func TestNodeRestoreMetadataError(t *testing.T) {
+	tempdir := t.TempDir()
+
+	node := nodeTests[0]
+	nodePath := filepath.Join(tempdir, node.Name)
+
+	// This will fail because the target file does not exist
+	err := node.RestoreMetadata(nodePath, func(msg string) { rtest.OK(t, fmt.Errorf("Warning triggered for path: %s: %s", nodePath, msg)) })
+	test.Assert(t, errors.Is(err, os.ErrNotExist), "failed for an unexpected reason")
+}