Merge pull request #195 from MichaelEischer/internal-error-for-inaccessible-files

Return "internal server error" if files cannot be read

changelog/unreleased/pull-194

@@ -0,0 +1,13 @@
+Bugfix: Return "internal server error" if files cannot be read
+
+When files in a repository cannot be read by rest-server, for example after
+running `restic prune` directly on the server hosting the repositories, then
+rest-server returned "Not Found" as status code. This is extremely confusing
+for users.
+
+The error handling has been fixed to only return "Not Found" if the file
+actually does not exist. Otherwise an internal server error is reported to the
+user and the underlying error is logged at the server side.
+
+https://github.com/restic/restic/issues/1871
+https://github.com/restic/rest-server/pull/194

handlers_test.go

@@ -10,6 +10,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"os"
+	"path"
 	"path/filepath"
 	"reflect"
 	"strings"
@@ -164,8 +165,7 @@ func createOverwriteDeleteSeq(t testing.TB, path string, data string) []TestRequ
 	return req
 }
 
-// TestResticHandler runs tests on the restic handler code, especially in append-only mode.
+func createTestHandler(t *testing.T, conf Server) (http.Handler, string, string, string, func()) {
-func TestResticHandler(t *testing.T) {
 	buf := make([]byte, 32)
 	_, err := io.ReadFull(rand.Reader, buf)
 	if err != nil {
@@ -175,6 +175,38 @@ func TestResticHandler(t *testing.T) {
 	dataHash := sha256.Sum256([]byte(data))
 	fileID := hex.EncodeToString(dataHash[:])
 
+	// setup the server with a local backend in a temporary directory
+	tempdir, err := ioutil.TempDir("", "rest-server-test-")
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// make sure the tempdir is properly removed
+	cleanup := func() {
+		err := os.RemoveAll(tempdir)
+		if err != nil {
+			t.Fatal(err)
+		}
+	}
+
+	conf.Path = tempdir
+	mux, err := NewHandler(&conf)
+	if err != nil {
+		t.Fatalf("error from NewHandler: %v", err)
+	}
+	return mux, data, fileID, tempdir, cleanup
+}
+
+// TestResticAppendOnlyHandler runs tests on the restic handler code, especially in append-only mode.
+func TestResticAppendOnlyHandler(t *testing.T) {
+	mux, data, fileID, _, cleanup := createTestHandler(t, Server{
+		AppendOnly:   true,
+		NoAuth:       true,
+		Debug:        true,
+		PanicOnError: true,
+	})
+	defer cleanup()
+
 	var tests = []struct {
 		seq []TestRequest
 	}{
@@ -226,32 +258,6 @@ func TestResticHandler(t *testing.T) {
 		{createOverwriteDeleteSeq(t, "/parent2/data/"+fileID, data)},
 	}
 
-	// setup the server with a local backend in a temporary directory
-	tempdir, err := ioutil.TempDir("", "rest-server-test-")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// make sure the tempdir is properly removed
-	defer func() {
-		err := os.RemoveAll(tempdir)
-		if err != nil {
-			t.Fatal(err)
-		}
-	}()
-
-	// set append-only mode and configure path
-	mux, err := NewHandler(&Server{
-		AppendOnly:   true,
-		Path:         tempdir,
-		NoAuth:       true,
-		Debug:        true,
-		PanicOnError: true,
-	})
-	if err != nil {
-		t.Fatalf("error from NewHandler: %v", err)
-	}
-
 	// create the repos
 	for _, path := range []string{"/", "/parent1/sub1/", "/parent1/", "/parent2/"} {
 		checkRequest(t, mux.ServeHTTP,
@@ -269,6 +275,110 @@ func TestResticHandler(t *testing.T) {
 	}
 }
 
+// createOverwriteDeleteSeq returns a sequence which will create a new file at
+// path, and then deletes it twice.
+func createIdempotentDeleteSeq(t testing.TB, path string, data string) []TestRequest {
+	return []TestRequest{
+		{
+			req:  newRequest(t, "POST", path, strings.NewReader(data)),
+			want: []wantFunc{wantCode(http.StatusOK)},
+		},
+		{
+			req:  newRequest(t, "DELETE", path, nil),
+			want: []wantFunc{wantCode(http.StatusOK)},
+		},
+		{
+			req:  newRequest(t, "GET", path, nil),
+			want: []wantFunc{wantCode(http.StatusNotFound)},
+		},
+		{
+			req:  newRequest(t, "DELETE", path, nil),
+			want: []wantFunc{wantCode(http.StatusOK)},
+		},
+	}
+}
+
+// TestResticHandler runs tests on the restic handler code, especially in append-only mode.
+func TestResticHandler(t *testing.T) {
+	mux, data, fileID, _, cleanup := createTestHandler(t, Server{
+		NoAuth:       true,
+		Debug:        true,
+		PanicOnError: true,
+	})
+	defer cleanup()
+
+	var tests = []struct {
+		seq []TestRequest
+	}{
+		{createIdempotentDeleteSeq(t, "/config", data)},
+		{createIdempotentDeleteSeq(t, "/data/"+fileID, data)},
+	}
+
+	// create the repo
+	checkRequest(t, mux.ServeHTTP,
+		newRequest(t, "POST", "/?create=true", nil),
+		[]wantFunc{wantCode(http.StatusOK)})
+
+	for _, test := range tests {
+		t.Run("", func(t *testing.T) {
+			for i, seq := range test.seq {
+				t.Logf("request %v: %v %v", i, seq.req.Method, seq.req.URL.Path)
+				checkRequest(t, mux.ServeHTTP, seq.req, seq.want)
+			}
+		})
+	}
+}
+
+// TestResticErrorHandler runs tests on the restic handler error handling.
+func TestResticErrorHandler(t *testing.T) {
+	mux, _, _, tempdir, cleanup := createTestHandler(t, Server{
+		AppendOnly: true,
+		NoAuth:     true,
+		Debug:      true,
+	})
+	defer cleanup()
+
+	var tests = []struct {
+		seq []TestRequest
+	}{
+		// Test inaccessible file
+		{
+			[]TestRequest{{
+				req:  newRequest(t, "GET", "/config", nil),
+				want: []wantFunc{wantCode(http.StatusInternalServerError)},
+			}},
+		},
+		{
+			[]TestRequest{{
+				req:  newRequest(t, "GET", "/parent4/config", nil),
+				want: []wantFunc{wantCode(http.StatusNotFound)},
+			}},
+		},
+	}
+
+	// create the repo
+	checkRequest(t, mux.ServeHTTP,
+		newRequest(t, "POST", "/?create=true", nil),
+		[]wantFunc{wantCode(http.StatusOK)})
+	// create inaccessible config
+	checkRequest(t, mux.ServeHTTP,
+		newRequest(t, "POST", "/config", strings.NewReader("example")),
+		[]wantFunc{wantCode(http.StatusOK)})
+	err := os.Chmod(path.Join(tempdir, "config"), 0o000)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	for _, test := range tests {
+		t.Run("", func(t *testing.T) {
+			for i, seq := range test.seq {
+				t.Logf("request %v: %v %v", i, seq.req.Method, seq.req.URL.Path)
+				checkRequest(t, mux.ServeHTTP, seq.req, seq.want)
+			}
+		})
+	}
+}
+
 func TestSplitURLPath(t *testing.T) {
 	var tests = []struct {
 		// Params
@@ -359,31 +469,13 @@ func (d *delayErrorReader) Read(p []byte) (int, error) {
 
 // TestAbortedRequest runs tests with concurrent upload requests for the same file.
 func TestAbortedRequest(t *testing.T) {
-	// setup the server with a local backend in a temporary directory
+	// the race condition doesn't happen for append-only repositories
-	tempdir, err := ioutil.TempDir("", "rest-server-test-")
+	mux, _, _, _, cleanup := createTestHandler(t, Server{
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	// make sure the tempdir is properly removed
-	defer func() {
-		err := os.RemoveAll(tempdir)
-		if err != nil {
-			t.Fatal(err)
-		}
-	}()
-
-	// configure path, the race condition doesn't happen for append-only repositories
-	mux, err := NewHandler(&Server{
-		AppendOnly:   false,
-		Path:         tempdir,
 		NoAuth:       true,
 		Debug:        true,
 		PanicOnError: true,
 	})
-	if err != nil {
+	defer cleanup()
-		t.Fatalf("error from NewHandler: %v", err)
-	}
 
 	// create the repo
 	checkRequest(t, mux.ServeHTTP,

repo/repo.go

@@ -257,10 +257,7 @@ func (h *Handler) checkConfig(w http.ResponseWriter, r *http.Request) {
 
 	st, err := os.Stat(cfg)
 	if err != nil {
-		if h.opt.Debug {
+		h.fileAccessError(w, err)
-			log.Print(err)
-		}
-		httpDefaultError(w, http.StatusNotFound)
 		return
 	}
 
@@ -276,10 +273,7 @@ func (h *Handler) getConfig(w http.ResponseWriter, r *http.Request) {
 
 	bytes, err := ioutil.ReadFile(cfg)
 	if err != nil {
-		if h.opt.Debug {
+		h.fileAccessError(w, err)
-			log.Print(err)
-		}
-		httpDefaultError(w, http.StatusNotFound)
 		return
 	}
 
@@ -331,13 +325,10 @@ func (h *Handler) deleteConfig(w http.ResponseWriter, r *http.Request) {
 	cfg := h.getSubPath("config")
 
 	if err := os.Remove(cfg); err != nil {
-		if h.opt.Debug {
+		// ignore not exist errors to make deleting idempotent, which is
-			log.Print(err)
+		// necessary to properly handle request retries
-		}
+		if !errors.Is(err, os.ErrNotExist) {
-		if os.IsNotExist(err) {
+			h.fileAccessError(w, err)
-			httpDefaultError(w, http.StatusNotFound)
-		} else {
-			h.internalServerError(w, err)
 		}
 		return
 	}
@@ -378,10 +369,7 @@ func (h *Handler) listBlobsV1(w http.ResponseWriter, r *http.Request) {
 
 	items, err := ioutil.ReadDir(path)
 	if err != nil {
-		if h.opt.Debug {
+		h.fileAccessError(w, err)
-			log.Print(err)
-		}
-		httpDefaultError(w, http.StatusNotFound)
 		return
 	}
 
@@ -392,10 +380,7 @@ func (h *Handler) listBlobsV1(w http.ResponseWriter, r *http.Request) {
 			var subitems []os.FileInfo
 			subitems, err = ioutil.ReadDir(subpath)
 			if err != nil {
-				if h.opt.Debug {
+				h.fileAccessError(w, err)
-					log.Print(err)
-				}
-				httpDefaultError(w, http.StatusNotFound)
 				return
 			}
 			for _, f := range subitems {
@@ -439,10 +424,7 @@ func (h *Handler) listBlobsV2(w http.ResponseWriter, r *http.Request) {
 
 	items, err := ioutil.ReadDir(path)
 	if err != nil {
-		if h.opt.Debug {
+		h.fileAccessError(w, err)
-			log.Print(err)
-		}
-		httpDefaultError(w, http.StatusNotFound)
 		return
 	}
 
@@ -453,10 +435,7 @@ func (h *Handler) listBlobsV2(w http.ResponseWriter, r *http.Request) {
 			var subitems []os.FileInfo
 			subitems, err = ioutil.ReadDir(subpath)
 			if err != nil {
-				if h.opt.Debug {
+				h.fileAccessError(w, err)
-					log.Print(err)
-				}
-				httpDefaultError(w, http.StatusNotFound)
 				return
 			}
 			for _, f := range subitems {
@@ -493,10 +472,7 @@ func (h *Handler) checkBlob(w http.ResponseWriter, r *http.Request) {
 
 	st, err := os.Stat(path)
 	if err != nil {
-		if h.opt.Debug {
+		h.fileAccessError(w, err)
-			log.Print(err)
-		}
-		httpDefaultError(w, http.StatusNotFound)
 		return
 	}
 
@@ -519,10 +495,7 @@ func (h *Handler) getBlob(w http.ResponseWriter, r *http.Request) {
 
 	file, err := os.Open(path)
 	if err != nil {
-		if h.opt.Debug {
+		h.fileAccessError(w, err)
-			log.Print(err)
-		}
-		httpDefaultError(w, http.StatusNotFound)
 		return
 	}
 
@@ -721,13 +694,10 @@ func (h *Handler) deleteBlob(w http.ResponseWriter, r *http.Request) {
 	}
 
 	if err := os.Remove(path); err != nil {
-		if h.opt.Debug {
+		// ignore not exist errors to make deleting idempotent, which is
-			log.Print(err)
+		// necessary to properly handle request retries
-		}
+		if !errors.Is(err, os.ErrNotExist) {
-		if os.IsNotExist(err) {
+			h.fileAccessError(w, err)
-			httpDefaultError(w, http.StatusNotFound)
-		} else {
-			h.internalServerError(w, err)
 		}
 		return
 	}
@@ -770,7 +740,7 @@ func (h *Handler) createRepo(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-// internalServerError is called to repot an internal server error.
+// internalServerError is called to report an internal server error.
 // The error message will be reported in the server logs. If PanicOnError
 // is set, this will panic instead, which makes debugging easier.
 func (h *Handler) internalServerError(w http.ResponseWriter, err error) {
@@ -780,3 +750,18 @@ func (h *Handler) internalServerError(w http.ResponseWriter, err error) {
 	}
 	httpDefaultError(w, http.StatusInternalServerError)
 }
+
+// internalServerError is called to report an error that occurred while
+// accessing a file. If the does not exist, the corresponding http status code
+// will be returned to the client. All other errors are passed on to
+// internalServerError
+func (h *Handler) fileAccessError(w http.ResponseWriter, err error) {
+	if h.opt.Debug {
+		log.Print(err)
+	}
+	if errors.Is(err, os.ErrNotExist) {
+		httpDefaultError(w, http.StatusNotFound)
+	} else {
+		h.internalServerError(w, err)
+	}
+}