diff --git a/backend/app/cmd/server_test.go b/backend/app/cmd/server_test.go index 2ed9dd57..3fba437d 100644 --- a/backend/app/cmd/server_test.go +++ b/backend/app/cmd/server_test.go @@ -548,6 +548,34 @@ func TestServerApp_MainSignal(t *testing.T) { assert.True(t, time.Since(st).Seconds() < 5, "should take under five sec", time.Since(st).Seconds()) } +func TestServerApp_RunCanceledBeforeRESTStart(t *testing.T) { + port := chooseRandomUnusedPort() + app, ctx, cancel := prepServerApp(t, func(o ServerCommand) ServerCommand { + o.Port = port + return o + }) + cancel() + + errCh := make(chan error, 1) + go func() { errCh <- app.run(ctx) }() + + select { + case err := <-errCh: + require.NoError(t, err) + app.Wait() + case <-time.After(time.Second): + waitForHTTPServerStart(port) + app.restSrv.Shutdown() + select { + case <-errCh: + app.Wait() + case <-time.After(time.Second): + t.Fatal("server app did not stop after forced REST shutdown") + } + t.Fatal("server app should exit when context is canceled before REST server starts") + } +} + func TestServerApp_DeprecatedArgs(t *testing.T) { s := ServerCommand{} s.SetCommon(CommonOpts{RemarkURL: "https://demo.remark42.com", SharedSecret: "123456"}) diff --git a/backend/app/rest/api/migrator.go b/backend/app/rest/api/migrator.go index 52421018..cc982264 100644 --- a/backend/app/rest/api/migrator.go +++ b/backend/app/rest/api/migrator.go @@ -74,21 +74,40 @@ func (m *Migrator) importFormCtrl(w http.ResponseWriter, r *http.Request) { } r.Body = http.MaxBytesReader(w, r.Body, 256*1024*1024) // hard cap on upload to prevent memory exhaustion - if err := r.ParseMultipartForm(20 * 1024 * 1024); err != nil { // 20M max memory, if bigger will make a file + reader, err := r.MultipartReader() + if err != nil { rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't parse multipart form", rest.ErrDecode) return } - file, _, err := r.FormFile("file") - if err != nil { - rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't get import file from the request", rest.ErrInternal) - return - } - defer func() { _ = file.Close() }() + tmpfile := "" + for { + part, err := reader.NextPart() + if err == io.EOF { + break + } + if err != nil { + rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't parse multipart form", rest.ErrDecode) + return + } + if part.FormName() != "file" { + _ = part.Close() + continue + } - tmpfile, err := m.saveTemp(file) - if err != nil { - rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't save request to temp file", rest.ErrInternal) + tmpfile, err = m.saveTemp(part) + if closeErr := part.Close(); err == nil && closeErr != nil { + err = closeErr + } + if err != nil { + rest.SendErrorJSON(w, r, http.StatusInternalServerError, err, "can't save request to temp file", rest.ErrInternal) + return + } + break + } + if tmpfile == "" { + rest.SendErrorJSON(w, r, http.StatusInternalServerError, fmt.Errorf("file field missing"), + "can't get import file from the request", rest.ErrInternal) return } diff --git a/backend/app/rest/api/rest.go b/backend/app/rest/api/rest.go index d2ae7aff..62323c84 100644 --- a/backend/app/rest/api/rest.go +++ b/backend/app/rest/api/rest.go @@ -71,10 +71,11 @@ type Rest struct { DisableFancyTextFormatting bool // disables SmartyPants in the comment text rendering of the posted comments ExternalImageProxy bool - SSLConfig SSLConfig - httpsServer *http.Server - httpServer *http.Server - lock sync.Mutex + SSLConfig SSLConfig + httpsServer *http.Server + httpServer *http.Server + shutdownRequested bool + lock sync.Mutex pubRest public privRest private @@ -117,6 +118,11 @@ func (s *Rest) Run(address string, port int) { s.lock.Lock() s.httpServer = s.makeHTTPServer(address, port, s.routes()) s.httpServer.ErrorLog = log.ToStdLogger(log.Default(), "WARN") + if s.shutdownRequested { + s.lock.Unlock() + log.Print("[WARN] rest server start canceled") + return + } s.lock.Unlock() err := s.httpServer.ListenAndServe() @@ -130,6 +136,11 @@ func (s *Rest) Run(address string, port int) { s.httpServer = s.makeHTTPServer(address, port, s.httpToHTTPSRouter()) s.httpServer.ErrorLog = log.ToStdLogger(log.Default(), "WARN") + if s.shutdownRequested { + s.lock.Unlock() + log.Print("[WARN] rest server start canceled") + return + } s.lock.Unlock() go func() { @@ -150,6 +161,11 @@ func (s *Rest) Run(address string, port int) { s.httpServer = s.makeHTTPServer(address, port, s.httpChallengeRouter(m)) s.httpServer.ErrorLog = log.ToStdLogger(log.Default(), "WARN") + if s.shutdownRequested { + s.lock.Unlock() + log.Print("[WARN] rest server start canceled") + return + } s.lock.Unlock() @@ -171,6 +187,7 @@ func (s *Rest) Shutdown() { ctx, cancel := context.WithTimeout(context.Background(), time.Second) defer cancel() s.lock.Lock() + s.shutdownRequested = true if s.httpServer != nil { if err := s.httpServer.Shutdown(ctx); err != nil { log.Printf("[DEBUG] http shutdown error, %s", err) diff --git a/backend/app/rest/api/ssl.go b/backend/app/rest/api/ssl.go index 36acffa5..f1beaeab 100644 --- a/backend/app/rest/api/ssl.go +++ b/backend/app/rest/api/ssl.go @@ -2,8 +2,10 @@ package api import ( "crypto/tls" + "fmt" "net/http" "net/url" + "strings" "time" "github.com/go-chi/chi/v5" @@ -66,14 +68,33 @@ func (s *Rest) httpChallengeRouter(m *autocert.Manager) chi.Router { func (s *Rest) redirectHandler() http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - newURL := s.RemarkURL + r.URL.Path - if r.URL.RawQuery != "" { - newURL += "?" + r.URL.RawQuery + newURL, err := s.redirectURL(r) + if err != nil { + log.Printf("[WARN] failed to build redirect URL, %s", err) + http.Error(w, "invalid redirect URL", http.StatusInternalServerError) + return } http.Redirect(w, r, newURL, http.StatusTemporaryRedirect) }) } +func (s *Rest) redirectURL(r *http.Request) (string, error) { + baseURL, err := url.Parse(s.RemarkURL) + if err != nil { + return "", fmt.Errorf("parse remark URL: %w", err) + } + if baseURL.Scheme != "http" && baseURL.Scheme != "https" || baseURL.Host == "" { + return "", fmt.Errorf("remark URL must be absolute HTTP(S) URL") + } + + basePath := strings.TrimRight(baseURL.Path, "/") + requestPath := "/" + strings.TrimLeft(r.URL.Path, "/") + baseURL.Path = basePath + requestPath + baseURL.RawQuery = r.URL.RawQuery + baseURL.Fragment = "" + return baseURL.String(), nil +} + func (s *Rest) makeAutocertManager() *autocert.Manager { return &autocert.Manager{ Prompt: autocert.AcceptTOS, diff --git a/backend/app/rest/api/ssl_test.go b/backend/app/rest/api/ssl_test.go index 3a40bada..d177c8ba 100644 --- a/backend/app/rest/api/ssl_test.go +++ b/backend/app/rest/api/ssl_test.go @@ -40,6 +40,16 @@ func TestSSL_Redirect(t *testing.T) { assert.Equal(t, "https://localhost:443/blah?param=1", resp.Header.Get("Location")) } +func TestSSL_RedirectURLKeepsConfiguredHost(t *testing.T) { + rest := Rest{RemarkURL: "https://localhost:443/base"} + req, err := http.NewRequest("GET", "http://example.com//evil.test/path?next=//evil.test", http.NoBody) + require.NoError(t, err) + + redirectURL, err := rest.redirectURL(req) + require.NoError(t, err) + assert.Equal(t, "https://localhost:443/base/evil.test/path?next=//evil.test", redirectURL) +} + func TestSSL_ACME_HTTPChallengeRouter(t *testing.T) { rest := Rest{ RemarkURL: "https://localhost:443",