fix(remote): refuse a redirect that drops TLS

The scheme was only checked on the URL the user wrote. A server answering
an https URL with a redirect to http was followed by the client without
any further check, so both the HEAD probe and the download travelled in
the clear, and a network attacker could substitute the Taskfile that is
about to be executed.

CheckRedirect now refuses an https to http hop. --insecure does not
loosen it: requesting an http entrypoint is the user's decision, being
sent to one is the server's. Setting CheckRedirect also replaces Go's
default cap, so the ten-hop limit is kept explicitly.

The three call sites turned almost every client error into a generic
download failure, which would have hidden the reason; TaskfileNotSecureError
is now passed through, with wording of its own for the redirect case since
--insecure is not a way out of it.

taskfile/node_http.go

@@ -32,9 +32,12 @@ func buildHTTPClient(insecure bool, caCert, cert, certKey string) (*http.Client,
 		return nil, fmt.Errorf("both --cert and --cert-key must be provided together")
 	}
 
-	// If no TLS customization is needed, return the default client
+	// If no TLS customization is needed, copy the default client rather than
+	// hand it out: setting CheckRedirect on it would apply process-wide.
 	if !insecure && caCert == "" && cert == "" {
-		return http.DefaultClient, nil
+		client := *http.DefaultClient
+		client.CheckRedirect = checkRedirect
+		return &client, nil
 	}
 
 	tlsConfig := &tls.Config{
@@ -67,9 +70,26 @@ func buildHTTPClient(insecure bool, caCert, cert, certKey string) (*http.Client,
 		Transport: &http.Transport{
 			TLSClientConfig: tlsConfig,
 		},
+		CheckRedirect: checkRedirect,
 	}, nil
 }
 
+// checkRedirect refuses a redirect that would drop TLS. --insecure does not
+// loosen it: an http:// entrypoint is the user's choice, a redirect is not.
+func checkRedirect(req *http.Request, via []*http.Request) error {
+	// Setting CheckRedirect replaces the default cap, so it has to be kept.
+	if len(via) >= 10 {
+		return fmt.Errorf("stopped after 10 redirects")
+	}
+	if len(via) == 0 {
+		return nil
+	}
+	if via[len(via)-1].URL.Scheme == "https" && req.URL.Scheme == "http" {
+		return &errors.TaskfileNotSecureError{URI: req.URL.Redacted(), Redirect: true}
+	}
+	return nil
+}
+
 func NewHTTPNode(
 	entrypoint string,
 	dir string,
@@ -120,6 +140,9 @@ func (node *HTTPNode) ReadContext(ctx context.Context) ([]byte, error) {
 		if ctx.Err() != nil {
 			return nil, err
 		}
+		if notSecure, ok := errors.AsType[*errors.TaskfileNotSecureError](err); ok {
+			return nil, notSecure
+		}
 		return nil, errors.TaskfileFetchFailedError{URI: node.Location()}
 	}
 	defer resp.Body.Close()

taskfile/node_http_test.go

@@ -9,6 +9,7 @@ import (
 	"encoding/pem"
 	"math/big"
 	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
 	"testing"
@@ -16,6 +17,8 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+
+	"github.com/go-task/task/v3/errors"
 )
 
 func TestHTTPNode_CacheKey(t *testing.T) {
@@ -62,10 +65,14 @@ func TestHTTPNode_CacheKey(t *testing.T) {
 func TestBuildHTTPClient_Default(t *testing.T) {
 	t.Parallel()
 
-	// When no TLS customization is needed, should return http.DefaultClient
+	// When no TLS customization is needed, should copy http.DefaultClient
 	client, err := buildHTTPClient(false, "", "", "")
 	require.NoError(t, err)
-	assert.Equal(t, http.DefaultClient, client)
+	assert.NotSame(t, http.DefaultClient, client)
+	assert.Equal(t, http.DefaultClient.Transport, client.Transport)
+	assert.NotNil(t, client.CheckRedirect)
+	// The shared client must keep following redirects as before.
+	assert.Nil(t, http.DefaultClient.CheckRedirect)
 }
 
 func TestBuildHTTPClient_Insecure(t *testing.T) {
@@ -282,3 +289,80 @@ func generateTestCACert(t *testing.T) []byte {
 		Bytes: certDER,
 	})
 }
+
+func TestCheckRedirect(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name    string
+		from    string
+		to      string
+		wantErr bool
+	}{
+		{name: "https to http is refused", from: "https://example.com", to: "http://example.com", wantErr: true},
+		{name: "https to http on another host is refused", from: "https://example.com", to: "http://evil.test", wantErr: true},
+		{name: "https to https is allowed", from: "https://example.com", to: "https://other.example.com"},
+		{name: "http to https is allowed", from: "http://example.com", to: "https://example.com"},
+		{name: "http to http is allowed, the entrypoint already opted in", from: "http://example.com", to: "http://other.example.com"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			via := []*http.Request{mustGet(t, tt.from)}
+			err := checkRedirect(mustGet(t, tt.to), via)
+			if !tt.wantErr {
+				require.NoError(t, err)
+				return
+			}
+			var notSecure *errors.TaskfileNotSecureError
+			require.ErrorAs(t, err, &notSecure)
+		})
+	}
+}
+
+func TestCheckRedirectFirstRequest(t *testing.T) {
+	t.Parallel()
+
+	require.NoError(t, checkRedirect(mustGet(t, "http://example.com"), nil))
+}
+
+func TestCheckRedirectStopsAfterTenHops(t *testing.T) {
+	t.Parallel()
+
+	via := make([]*http.Request, 10)
+	for i := range via {
+		via[i] = mustGet(t, "https://example.com")
+	}
+	require.Error(t, checkRedirect(mustGet(t, "https://example.com"), via))
+}
+
+// The downgrade is refused even with --insecure, which here also makes the
+// client accept the test server's self-signed certificate.
+func TestBuildHTTPClientRefusesDowngradeWithInsecure(t *testing.T) {
+	t.Parallel()
+
+	plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.WriteHeader(http.StatusOK)
+	}))
+	defer plain.Close()
+
+	secure := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, plain.URL+"/Taskfile.yml", http.StatusFound)
+	}))
+	defer secure.Close()
+
+	client, err := buildHTTPClient(true, "", "", "")
+	require.NoError(t, err)
+
+	_, err = client.Do(mustGet(t, secure.URL+"/Taskfile.yml")) //nolint:bodyclose // the request never completes
+	var notSecure *errors.TaskfileNotSecureError
+	require.ErrorAs(t, err, &notSecure)
+}
+
+func mustGet(t *testing.T, rawURL string) *http.Request {
+	t.Helper()
+	req, err := http.NewRequestWithContext(t.Context(), http.MethodGet, rawURL, nil)
+	require.NoError(t, err)
+	return req
+}

taskfile/taskfile.go

@@ -51,6 +51,9 @@ func RemoteExists(ctx context.Context, u url.URL, client *http.Client) (*url.URL
 		if ctx.Err() != nil {
 			return nil, fmt.Errorf("checking remote file: %w", ctx.Err())
 		}
+		if notSecure, ok := errors.AsType[*errors.TaskfileNotSecureError](err); ok {
+			return nil, notSecure
+		}
 		return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()}
 	}
 	defer resp.Body.Close()
@@ -80,6 +83,9 @@ func RemoteExists(ctx context.Context, u url.URL, client *http.Client) (*url.URL
 		// Try the alternative URL
 		resp, err = client.Do(req)
 		if err != nil {
+			if notSecure, ok := errors.AsType[*errors.TaskfileNotSecureError](err); ok {
+				return nil, notSecure
+			}
 			return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()}
 		}
 		defer resp.Body.Close()