fix: handle REST shutdown before server start

2026-06-01 19:55:14 -05:00
parent a21044738d
commit 589e956ade
5 changed files with 112 additions and 17 deletions
@@ -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"})
@@ -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
 	}

@@ -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)
@@ -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,
@@ -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",