diff --git a/changelog/unreleased/pull-194 b/changelog/unreleased/pull-194 new file mode 100644 index 0000000..a5c3a93 --- /dev/null +++ b/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 diff --git a/handlers_test.go b/handlers_test.go index e5931d7..4ebd195 100644 --- a/handlers_test.go +++ b/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 TestResticHandler(t *testing.T) { +func createTestHandler(t *testing.T, conf Server) (http.Handler, string, string, string, func()) { 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 - 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) - } - }() - - // configure path, the race condition doesn't happen for append-only repositories - mux, err := NewHandler(&Server{ - AppendOnly: false, - Path: tempdir, + // the race condition doesn't happen for append-only repositories + mux, _, _, _, cleanup := createTestHandler(t, Server{ NoAuth: true, Debug: true, PanicOnError: true, }) - if err != nil { - t.Fatalf("error from NewHandler: %v", err) - } + defer cleanup() // create the repo checkRequest(t, mux.ServeHTTP, diff --git a/repo/repo.go b/repo/repo.go index 812bee2..e59cc37 100644 --- a/repo/repo.go +++ b/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 { - log.Print(err) - } - httpDefaultError(w, http.StatusNotFound) + h.fileAccessError(w, err) 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 { - log.Print(err) - } - httpDefaultError(w, http.StatusNotFound) + h.fileAccessError(w, err) 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 { - log.Print(err) - } - if os.IsNotExist(err) { - httpDefaultError(w, http.StatusNotFound) - } else { - h.internalServerError(w, err) + // ignore not exist errors to make deleting idempotent, which is + // necessary to properly handle request retries + if !errors.Is(err, os.ErrNotExist) { + h.fileAccessError(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 { - log.Print(err) - } - httpDefaultError(w, http.StatusNotFound) + h.fileAccessError(w, err) 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 { - log.Print(err) - } - httpDefaultError(w, http.StatusNotFound) + h.fileAccessError(w, err) 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 { - log.Print(err) - } - httpDefaultError(w, http.StatusNotFound) + h.fileAccessError(w, err) 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 { - log.Print(err) - } - httpDefaultError(w, http.StatusNotFound) + h.fileAccessError(w, err) 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 { - log.Print(err) - } - httpDefaultError(w, http.StatusNotFound) + h.fileAccessError(w, err) 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 { - log.Print(err) - } - httpDefaultError(w, http.StatusNotFound) + h.fileAccessError(w, err) 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 { - log.Print(err) - } - if os.IsNotExist(err) { - httpDefaultError(w, http.StatusNotFound) - } else { - h.internalServerError(w, err) + // ignore not exist errors to make deleting idempotent, which is + // necessary to properly handle request retries + if !errors.Is(err, os.ErrNotExist) { + h.fileAccessError(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) + } +}