fix: isolate SSO callbacks and fail safely when the local port is occupied

cmd/login.go

@@ -24,6 +24,7 @@ import (
 	"fmt"
 	"html"
 	"log"
+	"net"
 	"net/http"
 	"os"
 	"strconv"
@@ -228,7 +229,7 @@ func oauth2login(
 	// handledRequests ensures we do not handle more requests than necessary
 	handledRequests := 0
 	// completionChan is to signal flow completed. Non-empty string indicates error
-	completionChan := make(chan string)
+	completionChan := make(chan string, 1)
 
 	stateNonce, err := rand.String(24)
 	if err != nil {
@@ -304,13 +305,6 @@ func oauth2login(
 		completionChan <- ""
 	}
 
-	// G112: Set ReadHeaderTimeout to mitigate Slowloris DoS attack.
-	srv := &http.Server{
-		Addr:              "localhost:" + strconv.Itoa(port),
-		ReadHeaderTimeout: 3 * time.Second,
-	}
-	http.HandleFunc("/auth/callback", callbackHandler)
-
 	var url string
 	opts := []oauth2.AuthCodeOption{}
 	opts = append(opts, oauth2.SetAuthURLParam("client_id", "microcks-app-js"))
@@ -322,15 +316,20 @@ func oauth2login(
 	authBaseURL := strings.SplitN(url, "?", 2)[0]
 	fmt.Printf("Performing %s flow login: %s\n", "authorization_code", authBaseURL)
 	time.Sleep(1 * time.Second)
-	if err := ssoAuthFlow(url, ssoLaunchBrowser); err != nil {
+	srv, listener, err := newOAuthCallbackServer("localhost:"+strconv.Itoa(port), callbackHandler)
+	if err != nil {
 		return "", "", err
 	}
+	defer srv.Close()
 	go func() {
 		log.Printf("Listen: %s\n", srv.Addr)
-		if err := srv.ListenAndServe(); err != http.ErrServerClosed {
+		if err := srv.Serve(listener); err != http.ErrServerClosed {
 			completionChan <- fmt.Sprintf("temporary HTTP server failed: %s", err)
 		}
 	}()
+	if err := ssoAuthFlow(url, ssoLaunchBrowser); err != nil {
+		return "", "", err
+	}
 	errMsg := <-completionChan
 	if errMsg != "" {
 		return "", "", errors.Wrapf(errors.KindGeneric, "%s", errMsg)
@@ -345,6 +344,21 @@ func oauth2login(
 	return tokenString, refreshToken, nil
 }
 
+func newOAuthCallbackServer(addr string, callbackHandler http.HandlerFunc) (*http.Server, net.Listener, error) {
+	mux := http.NewServeMux()
+	mux.HandleFunc("/auth/callback", callbackHandler)
+	srv := &http.Server{
+		Addr:              addr,
+		Handler:           mux,
+		ReadHeaderTimeout: 3 * time.Second,
+	}
+	listener, err := net.Listen("tcp", srv.Addr)
+	if err != nil {
+		return nil, nil, errors.Wrap(errors.KindEnvironment, fmt.Errorf("starting temporary HTTP server on %s: %w", srv.Addr, err))
+	}
+	return srv, listener, nil
+}
+
 func ssoAuthFlow(url string, ssoLaunchBrowser bool) error {
 	if ssoLaunchBrowser {
 		fmt.Printf("Opening system default browser for authentication\n")

cmd/login_test.go

@@ -0,0 +1,103 @@
+/*
+ * Copyright The Microcks Authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *  http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package cmd
+
+import (
+	stderrors "errors"
+	"net"
+	"net/http"
+	"testing"
+
+	clierrors "github.com/microcks/microcks-cli/pkg/errors"
+)
+
+func TestNewOAuthCallbackServerUsesIsolatedMuxes(t *testing.T) {
+	firstStatus := startOAuthCallbackTestServer(t, http.StatusCreated)
+	secondStatus := startOAuthCallbackTestServer(t, http.StatusAccepted)
+
+	if firstStatus != http.StatusCreated {
+		t.Fatalf("first callback server returned status %d, want %d", firstStatus, http.StatusCreated)
+	}
+	if secondStatus != http.StatusAccepted {
+		t.Fatalf("second callback server returned status %d, want %d", secondStatus, http.StatusAccepted)
+	}
+}
+
+func TestNewOAuthCallbackServerReturnsErrorWhenPortIsOccupied(t *testing.T) {
+	occupiedListener, err := net.Listen("tcp", "127.0.0.1:0")
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Cleanup(func() {
+		if err := occupiedListener.Close(); err != nil {
+			t.Errorf("closing occupied listener: %v", err)
+		}
+	})
+
+	server, listener, err := newOAuthCallbackServer(occupiedListener.Addr().String(), http.NotFound)
+	if err == nil {
+		if server != nil {
+			if closeErr := server.Close(); closeErr != nil {
+				t.Errorf("closing unexpected callback server: %v", closeErr)
+			}
+		}
+		if listener != nil {
+			if closeErr := listener.Close(); closeErr != nil {
+				t.Errorf("closing unexpected callback listener: %v", closeErr)
+			}
+		}
+		t.Fatal("expected an error when the callback port is already occupied")
+	}
+	if got := clierrors.KindOf(err); got != clierrors.KindEnvironment {
+		t.Fatalf("error kind = %v, want %v", got, clierrors.KindEnvironment)
+	}
+}
+
+func startOAuthCallbackTestServer(t *testing.T, status int) int {
+	t.Helper()
+
+	server, listener, err := newOAuthCallbackServer("127.0.0.1:0", func(w http.ResponseWriter, _ *http.Request) {
+		w.WriteHeader(status)
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	serveErrors := make(chan error, 1)
+	go func() {
+		serveErrors <- server.Serve(listener)
+	}()
+	t.Cleanup(func() {
+		if err := server.Close(); err != nil {
+			t.Errorf("closing callback server: %v", err)
+		}
+		if err := <-serveErrors; !stderrors.Is(err, http.ErrServerClosed) {
+			t.Errorf("callback server returned unexpected error: %v", err)
+		}
+	})
+
+	response, err := http.Get("http://" + listener.Addr().String() + "/auth/callback")
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer func() {
+		if err := response.Body.Close(); err != nil {
+			t.Errorf("closing callback response body: %v", err)
+		}
+	}()
+	return response.StatusCode
+}