diff --git a/CHANGELOG.md b/CHANGELOG.md index 428b516660..d988895ca7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,11 @@ - Fixed task names containing regex characters (e.g. `c++` or `deploy.prod`) crashing Task or matching the wrong task when calling a task that has no exact match (#2916 by @vsaraikin). +- 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 (#3091 by @vmaerten). +- Fixed `remote.headers` being sent over plain `http` when an `https` remote + Taskfile is redirected to `http` on the same host (#3091 by @vmaerten). ## v3.54.0 - 2026-10-01 diff --git a/errors/errors_taskfile.go b/errors/errors_taskfile.go index 3b2b3795f8..725b865c0e 100644 --- a/errors/errors_taskfile.go +++ b/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), diff --git a/taskfile/http_headers.go b/taskfile/http_headers.go index 4cdbfaab5f..d10cb2c129 100644 --- a/taskfile/http_headers.go +++ b/taskfile/http_headers.go @@ -5,6 +5,7 @@ import ( "fmt" "maps" "net/http" + "net/url" "slices" "golang.org/x/net/http/httpguts" @@ -18,13 +19,16 @@ type HeadersByHost map[string]map[string]string type headersTransport struct { base http.RoundTripper + scheme string host string headers map[string]string } func (t *headersTransport) RoundTrip(req *http.Request) (*http.Response, error) { - // Scope all configured headers to this host, including after redirects. - if !hostMatches(t.host, req.URL.Host) { + // Scope all configured headers to this origin, including after redirects: + // an https URL redirected to http on the same host keeps its Host string, + // so the scheme must match too or the headers would travel in plaintext. + if req.URL.Scheme != t.scheme || !hostMatches(t.host, req.URL.Host) { return t.base.RoundTrip(req) } req = req.Clone(req.Context()) @@ -43,15 +47,16 @@ func (node *HTTPNode) clientWithHeaders() (*http.Client, error) { if len(headers) == 0 { return node.client, nil } - return withHeaders(node.client, node.url.Host, headers), nil + return withHeaders(node.client, node.url, headers), nil } -func withHeaders(client *http.Client, host string, headers map[string]string) *http.Client { +func withHeaders(client *http.Client, origin *url.URL, headers map[string]string) *http.Client { // The client may be http.DefaultClient; leave it unchanged. configured := *client configured.Transport = &headersTransport{ base: cmp.Or(client.Transport, http.DefaultTransport), - host: host, + scheme: origin.Scheme, + host: origin.Host, headers: headers, } return &configured diff --git a/taskfile/http_headers_test.go b/taskfile/http_headers_test.go index b6dde36443..c44dcc2657 100644 --- a/taskfile/http_headers_test.go +++ b/taskfile/http_headers_test.go @@ -127,6 +127,7 @@ func TestHeadersTransport(t *testing.T) { transport := &headersTransport{ base: roundTripperFunc(func(req *http.Request) (*http.Response, error) { return newResponse(req), nil }), + scheme: "https", host: "gitlab.com", headers: map[string]string{"PRIVATE-TOKEN": "token"}, } @@ -147,12 +148,20 @@ func TestHeadersTransport(t *testing.T) { require.NoError(t, err) assert.Empty(t, resp.Request.Header.Get("PRIVATE-TOKEN")) }) + + t.Run("leaves the same host over http alone", func(t *testing.T) { + t.Parallel() + req := newRequest(t, "http://gitlab.com/api/v4/Taskfile.yml") + resp, err := transport.RoundTrip(req) + require.NoError(t, err) + assert.Empty(t, resp.Request.Header.Get("PRIVATE-TOKEN")) + }) } func TestWithHeadersDoesNotMutateTheDefaultClient(t *testing.T) { t.Parallel() - client := withHeaders(http.DefaultClient, "gitlab.com", map[string]string{"PRIVATE-TOKEN": "token"}) + client := withHeaders(http.DefaultClient, &url.URL{Scheme: "https", Host: "gitlab.com"}, map[string]string{"PRIVATE-TOKEN": "token"}) assert.NotSame(t, http.DefaultClient, client) assert.Nil(t, http.DefaultClient.Transport) diff --git a/taskfile/node_http.go b/taskfile/node_http.go index f5ba91d83f..6d44e14a53 100644 --- a/taskfile/node_http.go +++ b/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, @@ -124,7 +149,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 { diff --git a/taskfile/node_http_test.go b/taskfile/node_http_test.go index 359ec798bf..f1e4c553bb 100644 --- a/taskfile/node_http_test.go +++ b/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, ¬Secure) + }) + } +} + +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, ¬Secure) + 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 +} diff --git a/taskfile/taskfile.go b/taskfile/taskfile.go index 00e25c679c..ab03919db2 100644 --- a/taskfile/taskfile.go +++ b/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() @@ -87,7 +87,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() @@ -99,3 +99,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} +} diff --git a/website/src/next/docs/reference/config.md b/website/src/next/docs/reference/config.md index 98376bc342..7131d23a6d 100644 --- a/website/src/next/docs/reference/config.md +++ b/website/src/next/docs/reference/config.md @@ -369,9 +369,10 @@ the command line would be visible to any process listing it. ::: warning -Headers are only sent to the host they are configured for. If that host answers -with a redirect to another one, the request follows the redirect **without** -them, and will likely fail — point the URL at the final host instead. Headers +Headers are only sent to the host and scheme they are configured for. If that +host answers with a redirect to another host, or to `http`, the request follows +the redirect **without** them, and will likely fail — point the URL at the final +location instead. Headers are also HTTP-only: a Taskfile fetched over `git` should authenticate with SSH or a git credential helper. diff --git a/website/src/next/docs/remote-taskfiles.md b/website/src/next/docs/remote-taskfiles.md index b75b57b7e7..64310a0199 100644 --- a/website/src/next/docs/remote-taskfiles.md +++ b/website/src/next/docs/remote-taskfiles.md @@ -238,6 +238,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. + #### Add certificates {#custom-certificates} If your remote Taskfiles are hosted on a server that uses a custom CA