From a7689d7023611924863c8f3d2b93d3f725b079f2 Mon Sep 17 00:00:00 2001 From: Nick Craig-Wood Date: Sun, 10 Jan 2021 12:24:22 +0000 Subject: [PATCH] rcserver: fix 500 error when marshalling errors from core/command Before this change attempting to return an error from core/command failed with a 500 error and a message about unmarshable types. This is because it was attempting to marshal the input parameters which get _response added to them which contains an unmarshalable field. This was fixed by using the original parameters in the error response rather than the one modified during the error handling. This also adds end to end tests for the streaming facilities as used in core/command. --- fs/rc/rcserver/rcserver.go | 13 +++-- fs/rc/rcserver/rcserver_test.go | 84 +++++++++++++++++++++++++++++++++ 2 files changed, 92 insertions(+), 5 deletions(-) diff --git a/fs/rc/rcserver/rcserver.go b/fs/rc/rcserver/rcserver.go index 2067a59ce..3c6945ebc 100644 --- a/fs/rc/rcserver/rcserver.go +++ b/fs/rc/rcserver/rcserver.go @@ -186,7 +186,7 @@ func writeError(path string, in rc.Params, w http.ResponseWriter, err error, sta }) if err != nil { // can't return the error at this point - fs.Errorf(nil, "rc: failed to write JSON output: %v", err) + fs.Errorf(nil, "rc: writeError: failed to write JSON output from %#v: %v", in, err) } } @@ -270,6 +270,9 @@ func (s *Server) handlePost(w http.ResponseWriter, r *http.Request, path string) writeError(path, in, w, errors.Errorf("authentication must be set up on the rc server to use %q or the --rc-no-auth flag must be in use", path), http.StatusForbidden) return } + + inOrig := in.Copy() + if call.NeedsRequest { // Add the request to RC in["_request"] = r @@ -282,7 +285,7 @@ func (s *Server) handlePost(w http.ResponseWriter, r *http.Request, path string) // Check to see if it is async or not isAsync, err := in.GetBool("_async") if rc.NotErrParamNotFound(err) { - writeError(path, in, w, err, http.StatusBadRequest) + writeError(path, inOrig, w, err, http.StatusBadRequest) return } delete(in, "_async") // remove the async parameter after parsing so vfs operations don't get confused @@ -297,7 +300,7 @@ func (s *Server) handlePost(w http.ResponseWriter, r *http.Request, path string) w.Header().Add("x-rclone-jobid", fmt.Sprintf("%d", jobID)) } if err != nil { - writeError(path, in, w, err, http.StatusInternalServerError) + writeError(path, inOrig, w, err, http.StatusInternalServerError) return } if out == nil { @@ -308,8 +311,8 @@ func (s *Server) handlePost(w http.ResponseWriter, r *http.Request, path string) err = rc.WriteJSON(w, out) if err != nil { // can't return the error at this point - but have a go anyway - writeError(path, in, w, err, http.StatusInternalServerError) - fs.Errorf(nil, "rc: failed to write JSON output: %v", err) + writeError(path, inOrig, w, err, http.StatusInternalServerError) + fs.Errorf(nil, "rc: handlePost: failed to write JSON output: %v", err) } } diff --git a/fs/rc/rcserver/rcserver_test.go b/fs/rc/rcserver/rcserver_test.go index 678157b5d..9ada37a92 100644 --- a/fs/rc/rcserver/rcserver_test.go +++ b/fs/rc/rcserver/rcserver_test.go @@ -8,6 +8,7 @@ import ( "io/ioutil" "net/http" "net/http/httptest" + "os" "regexp" "strings" "testing" @@ -17,6 +18,7 @@ import ( "github.com/stretchr/testify/require" _ "github.com/rclone/rclone/backend/local" + "github.com/rclone/rclone/fs" "github.com/rclone/rclone/fs/accounting" "github.com/rclone/rclone/fs/rc" ) @@ -28,6 +30,21 @@ const ( remoteURL = "[" + testFs + "]/" // initial URL path to fetch from that remote ) +func TestMain(m *testing.M) { + // Pretend to be rclone version if we have a version string parameter + if os.Args[len(os.Args)-1] == "version" { + fmt.Printf("rclone %s\n", fs.Version) + os.Exit(0) + } + // Pretend to error if we have an unknown command + if os.Args[len(os.Args)-1] == "unknown_command" { + fmt.Printf("rclone %s\n", fs.Version) + fmt.Fprintf(os.Stderr, "Unknown command\n") + os.Exit(1) + } + os.Exit(m.Run()) +} + // Test the RC server runs and we can do HTTP fetches from it. // We'll do the majority of the testing with the httptest framework func TestRcServer(t *testing.T) { @@ -456,6 +473,73 @@ func TestRC(t *testing.T) { testServer(t, tests, &opt) } +func TestRCWithAuth(t *testing.T) { + tests := []testRun{{ + Name: "core-command", + URL: "core/command", + Method: "POST", + Body: `command=version`, + ContentType: "application/x-www-form-urlencoded", + Status: http.StatusOK, + Expected: fmt.Sprintf(`{ + "error": false, + "result": "rclone %s\n" +} +`, fs.Version), + }, { + Name: "core-command-bad-returnType", + URL: "core/command", + Method: "POST", + Body: `command=version&returnType=POTATO`, + ContentType: "application/x-www-form-urlencoded", + Status: http.StatusInternalServerError, + Expected: `{ + "error": "Unknown returnType \"POTATO\"", + "input": { + "command": "version", + "returnType": "POTATO" + }, + "path": "core/command", + "status": 500 +} +`, + }, { + Name: "core-command-stream", + URL: "core/command", + Method: "POST", + Body: `command=version&returnType=STREAM`, + ContentType: "application/x-www-form-urlencoded", + Status: http.StatusOK, + Expected: fmt.Sprintf(`rclone %s +{} +`, fs.Version), + }, { + Name: "core-command-stream-error", + URL: "core/command", + Method: "POST", + Body: `command=unknown_command&returnType=STREAM`, + ContentType: "application/x-www-form-urlencoded", + Status: http.StatusOK, + Expected: fmt.Sprintf(`rclone %s +Unknown command +{ + "error": "exit status 1", + "input": { + "command": "unknown_command", + "returnType": "STREAM" + }, + "path": "core/command", + "status": 500 +} +`, fs.Version), + }} + opt := newTestOpt() + opt.Serve = true + opt.Files = testFs + opt.NoAuth = true + testServer(t, tests, &opt) +} + func TestMethods(t *testing.T) { tests := []testRun{{ Name: "options",