From ef2c5a199870bc3cfdb0b854576c7749350c7aa7 Mon Sep 17 00:00:00 2001
From: Michael Eischer <michael.eischer@gmx.de>
Date: Mon, 29 Jan 2024 18:54:23 +0100
Subject: [PATCH] serve restic: fix error handling

* serve restic: return internal error if listing failed

If listing a remote failed, then rclone returned http status "not
found". This has become a problem since restic 0.16.0 which ignores "not
found"-errors while listing a directory.

Just return internal server error, if something unexpected happens while
listing a directory.

* serve restic: fix error handling if getting a file fails

If the call to `newObject` in `serveObject` fails, then rclone always
returned a "not found" error. This prevents restic from distinguishing
permanent "not found" errors from everything else.

Thus, only return "not found" if the object is not found and an internal
server error otherwise.
---
 cmd/serve/restic/restic.go      | 10 ++++--
 cmd/serve/restic/restic_test.go | 56 +++++++++++++++++++++++++++++++++
 2 files changed, 63 insertions(+), 3 deletions(-)

diff --git a/cmd/serve/restic/restic.go b/cmd/serve/restic/restic.go
index be56fe947..396ef1ebf 100644
--- a/cmd/serve/restic/restic.go
+++ b/cmd/serve/restic/restic.go
@@ -341,7 +341,11 @@ func (s *server) serveObject(w http.ResponseWriter, r *http.Request) {
 	o, err := s.newObject(r.Context(), remote)
 	if err != nil {
 		fs.Debugf(remote, "%s request error: %v", r.Method, err)
-		http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound)
+		if errors.Is(err, fs.ErrorObjectNotFound) {
+			http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound)
+		} else {
+			http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
+		}
 		return
 	}
 	serve.Object(w, r, o)
@@ -404,7 +408,7 @@ func (s *server) deleteObject(w http.ResponseWriter, r *http.Request) {
 
 	if err := o.Remove(r.Context()); err != nil {
 		fs.Errorf(remote, "Delete request remove error: %v", err)
-		if err == fs.ErrorObjectNotFound {
+		if errors.Is(err, fs.ErrorObjectNotFound) {
 			http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound)
 		} else {
 			http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
@@ -466,7 +470,7 @@ func (s *server) listObjects(w http.ResponseWriter, r *http.Request) {
 	if err != nil {
 		if !errors.Is(err, fs.ErrorDirNotFound) {
 			fs.Errorf(remote, "list failed: %#v %T", err, err)
-			http.Error(w, http.StatusText(http.StatusNotFound), http.StatusNotFound)
+			http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError)
 			return
 		}
 	}
diff --git a/cmd/serve/restic/restic_test.go b/cmd/serve/restic/restic_test.go
index 3db040bdd..36d09f93c 100644
--- a/cmd/serve/restic/restic_test.go
+++ b/cmd/serve/restic/restic_test.go
@@ -5,6 +5,7 @@ package restic
 
 import (
 	"context"
+	"errors"
 	"net/http"
 	"net/http/httptest"
 	"os"
@@ -12,6 +13,8 @@ import (
 	"testing"
 
 	_ "github.com/rclone/rclone/backend/all"
+	"github.com/rclone/rclone/cmd"
+	"github.com/rclone/rclone/fs"
 	"github.com/rclone/rclone/fstest"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -114,3 +117,56 @@ func TestMakeRemote(t *testing.T) {
 		got.ServeHTTP(w, r)
 	}
 }
+
+type listErrorFs struct {
+	fs.Fs
+}
+
+func (f *listErrorFs) List(ctx context.Context, dir string) (entries fs.DirEntries, err error) {
+	return fs.DirEntries{}, errors.New("oops")
+}
+
+func TestListErrors(t *testing.T) {
+	ctx := context.Background()
+	// setup rclone with a local backend in a temporary directory
+	tempdir := t.TempDir()
+	opt := newOpt()
+
+	// make a new file system in the temp dir
+	f := &listErrorFs{Fs: cmd.NewFsSrc([]string{tempdir})}
+	s, err := newServer(ctx, f, &opt)
+	require.NoError(t, err)
+	router := s.Server.Router()
+
+	req := newRequest(t, "GET", "/test/snapshots/", nil)
+	checkRequest(t, router.ServeHTTP, req, []wantFunc{wantCode(http.StatusInternalServerError)})
+}
+
+type newObjectErrorFs struct {
+	fs.Fs
+	err error
+}
+
+func (f *newObjectErrorFs) NewObject(ctx context.Context, remote string) (fs.Object, error) {
+	return nil, f.err
+}
+
+func TestServeErrors(t *testing.T) {
+	ctx := context.Background()
+	// setup rclone with a local backend in a temporary directory
+	tempdir := t.TempDir()
+	opt := newOpt()
+
+	// make a new file system in the temp dir
+	f := &newObjectErrorFs{Fs: cmd.NewFsSrc([]string{tempdir})}
+	s, err := newServer(ctx, f, &opt)
+	require.NoError(t, err)
+	router := s.Server.Router()
+
+	f.err = errors.New("oops")
+	req := newRequest(t, "GET", "/test/config", nil)
+	checkRequest(t, router.ServeHTTP, req, []wantFunc{wantCode(http.StatusInternalServerError)})
+
+	f.err = fs.ErrorObjectNotFound
+	checkRequest(t, router.ServeHTTP, req, []wantFunc{wantCode(http.StatusNotFound)})
+}