fix(remote): refuse a redirect that drops TLS

CHANGELOG.md

@@ -2,6 +2,12 @@
 
 ## Unreleased
 
+### 🐛 Fixes
+
+- Fixed an `https` remote Taskfile being downloaded over an unencrypted
+  connection when the server redirects to `http`. Such a redirect now requires
+  `--insecure`, like an `http` entrypoint (by @vmaerten).
+
 ### 📦 Package API
 
 - Bumped the minimum Go version to 1.26. Task follows Go's two-latest support

errors/errors_taskfile.go

@@ -102,9 +102,18 @@ func (err *TaskfileNotTrustedError) Code() int {
 // remote Taskfile over an insecure connection.
 type TaskfileNotSecureError struct {
 	URI string
+	// Redirect reports that the insecure URI was reached through a redirect
+	// rather than requested, in which case --insecure does not allow it.
+	Redirect bool
 }
 
 func (err *TaskfileNotSecureError) Error() string {
+	if err.Redirect {
+		return fmt.Sprintf(
+			`task: Taskfile %q was redirected to over an insecure connection. You can override this by using the --insecure flag`,
+			filepath.ToSlash(err.URI),
+		)
+	}
 	return fmt.Sprintf(
 		`task: Taskfile %q cannot be downloaded over an insecure connection. You can override this by using the --insecure flag`,
 		filepath.ToSlash(err.URI),

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(insecure)
+		return &client, nil
 	}
 
 	tlsConfig := &tls.Config{
@@ -67,9 +70,31 @@ func buildHTTPClient(insecure bool, caCert, cert, certKey string) (*http.Client,
 		Transport: &http.Transport{
 			TLSClientConfig: tlsConfig,
 		},
+		CheckRedirect: checkRedirect(insecure),
 	}, nil
 }
 
+// checkRedirect refuses a redirect that would drop TLS, unless --insecure was
+// given: that flag also disables certificate verification, so refusing the
+// plaintext hop would guard nothing an attacker could not walk around.
+func checkRedirect(insecure bool) func(*http.Request, []*http.Request) error {
+	return func(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 !insecure &&
+			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,7 +145,7 @@ func (node *HTTPNode) ReadContext(ctx context.Context) ([]byte, error) {
 		if ctx.Err() != nil {
 			return nil, err
 		}
-		return nil, errors.TaskfileFetchFailedError{URI: node.Location()}
+		return nil, taskfileFetchError(err, node.Location())
 	}
 	defer resp.Body.Close()
 	if resp.StatusCode != http.StatusOK {

taskfile/node_http_test.go

@@ -9,13 +9,17 @@ import (
 	"encoding/pem"
 	"math/big"
 	"net/http"
+	"net/http/httptest"
 	"os"
 	"path/filepath"
+	"sync/atomic"
 	"testing"
 	"time"
 
 	"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 +66,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 +290,115 @@ func generateTestCACert(t *testing.T) []byte {
 		Bytes: certDER,
 	})
 }
+
+func TestCheckRedirect(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name     string
+		from     string
+		to       string
+		insecure bool
+		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 http is allowed with insecure", from: "https://example.com", to: "http://example.com", insecure: 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(tt.insecure)(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(false)(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(false)(mustGet(t, "https://example.com"), via))
+}
+
+// downgradeServers returns a TLS server redirecting to a plaintext one, and a
+// flag reporting whether the plaintext one was ever reached.
+func downgradeServers(t *testing.T) (*httptest.Server, *atomic.Bool) {
+	t.Helper()
+
+	var plainReached atomic.Bool
+	plain := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		plainReached.Store(true)
+		w.WriteHeader(http.StatusOK)
+	}))
+	t.Cleanup(plain.Close)
+
+	secure := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		http.Redirect(w, r, plain.URL+"/Taskfile.yml", http.StatusFound)
+	}))
+	t.Cleanup(secure.Close)
+
+	return secure, &plainReached
+}
+
+func TestBuildHTTPClientRefusesDowngrade(t *testing.T) {
+	t.Parallel()
+
+	secure, plainReached := downgradeServers(t)
+
+	// The test server's certificate has to be trusted explicitly, so that the
+	// refusal is the redirect and not the handshake.
+	caCert := filepath.Join(t.TempDir(), "ca.crt")
+	require.NoError(t, os.WriteFile(caCert, pem.EncodeToMemory(&pem.Block{
+		Type: "CERTIFICATE", Bytes: secure.Certificate().Raw,
+	}), 0o600))
+
+	client, err := buildHTTPClient(false, caCert, "", "")
+	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)
+	assert.False(t, plainReached.Load(), "the plaintext server must never be contacted")
+}
+
+func TestBuildHTTPClientFollowsDowngradeWithInsecure(t *testing.T) {
+	t.Parallel()
+
+	secure, plainReached := downgradeServers(t)
+
+	client, err := buildHTTPClient(true, "", "", "")
+	require.NoError(t, err)
+
+	resp, err := client.Do(mustGet(t, secure.URL+"/Taskfile.yml"))
+	require.NoError(t, err)
+	defer resp.Body.Close()
+	assert.Equal(t, http.StatusOK, resp.StatusCode)
+	assert.True(t, plainReached.Load())
+}
+
+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,7 +51,7 @@ 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())
 		}
-		return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()}
+		return nil, taskfileFetchError(err, u.Redacted())
 	}
 	defer resp.Body.Close()
 
@@ -80,7 +80,7 @@ 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 {
-			return nil, errors.TaskfileFetchFailedError{URI: u.Redacted()}
+			return nil, taskfileFetchError(err, u.Redacted())
 		}
 		defer resp.Body.Close()
 
@@ -92,3 +92,12 @@ func RemoteExists(ctx context.Context, u url.URL, client *http.Client) (*url.URL
 
 	return nil, errors.TaskfileNotFoundError{URI: u.Redacted(), Walk: false}
 }
+
+// taskfileFetchError preserves redirect-policy errors wrapped by http.Client.
+// Other transport errors remain generic download failures.
+func taskfileFetchError(err error, uri string) error {
+	if notSecure, ok := errors.AsType[*errors.TaskfileNotSecureError](err); ok {
+		return notSecure
+	}
+	return errors.TaskfileFetchFailedError{URI: uri}
+}

website/src/next/docs/remote-taskfiles.md

@@ -260,6 +260,10 @@ Taskfile that is downloaded via an unencrypted connection. Sources that are not
 protected by TLS are vulnerable to man-in-the-middle attacks and should be
 avoided unless you know what you are doing.
 
+A server answering an `https` URL with a redirect to `http` is refused too,
+unless you pass `--insecure` — the flag covers the whole download, not just the
+URL you wrote.
+
 #### Custom Certificates
 
 If your remote Taskfiles are hosted on a server that uses a custom CA