From a41df4a127b950bfe98d29cbe44e27e5e5779380 Mon Sep 17 00:00:00 2001 From: Valentin Maerten Date: Thu, 20 Aug 2026 16:50:43 +0200 Subject: [PATCH] fix(remote): report a 401 instead of a missing Taskfile RemoteExists treated every non-200 as an absent file, so a server refusing the credentials ended up as "No Taskfile found", sending the user to check the URL rather than the token. A 401 now stops the search and reports the status code; the default names need the same credentials, so trying them would only add rejected requests. A 403 is left alone: it is also what a server without directory listing answers for a readable directory. That message being correct, the expansion no longer needs to refuse an undefined variable: os.ExpandEnv is inlined and expandEnv is gone. The `$$` escape goes with it, so a literal value can no longer hold a `$` followed by a name; a secret carried in an environment variable is unaffected, as os.Expand never rescans what it substituted. Header names are validated with httpguts.ValidHeaderFieldName, the table net/http itself uses, rather than a denylist that let X-Foo(bar) through. golang.org/x/net was already in the module graph, so tidy only moves it to the direct block. Finally, node_http_auth.go becomes http_auth.go: the node_ prefix is for files defining a Node type, and this one holds the auth concern of HTTPNode plus hostMatches, which reader.go uses for trusted hosts. --- CHANGELOG.md | 7 ++ go.mod | 2 +- taskfile/{node_http_auth.go => http_auth.go} | 51 +++-------- ...de_http_auth_test.go => http_auth_test.go} | 37 ++++---- taskfile/taskfile.go | 7 ++ taskfile/taskfile_test.go | 84 +++++++++++++++++++ website/src/latest/docs/reference/config.md | 7 +- 7 files changed, 134 insertions(+), 61 deletions(-) rename taskfile/{node_http_auth.go => http_auth.go} (62%) rename taskfile/{node_http_auth_test.go => http_auth_test.go} (86%) create mode 100644 taskfile/taskfile_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e5c90377..bf2940f7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,13 @@ where it would leak into error messages and the confirmation prompt (#2329 by @vmaerten). +### 🐛 Fixes + +- Fixed a remote Taskfile whose server refuses the credentials being reported as + a missing Taskfile. A `401` now stops the search and reports the status code, + instead of retrying every default Taskfile name and concluding that no + Taskfile exists (#2329 by @vmaerten). + ### 📦 Package API - Bumped the minimum Go version to 1.26. Task follows Go's two-latest support diff --git a/go.mod b/go.mod index c6fd21f5..c6d9e8eb 100644 --- a/go.mod +++ b/go.mod @@ -28,6 +28,7 @@ require ( github.com/stretchr/testify v1.11.1 github.com/zeebo/xxh3 v1.1.0 go.yaml.in/yaml/v3 v3.0.4 + golang.org/x/net v0.58.0 golang.org/x/sync v0.22.0 golang.org/x/term v0.45.0 mvdan.cc/sh/moreinterp v0.0.0-20260817215856-d6550df7ed8d @@ -121,7 +122,6 @@ require ( go.opentelemetry.io/otel/trace v1.45.0 // indirect golang.org/x/crypto v0.55.0 // indirect golang.org/x/exp v0.0.0-20260718201538-764159d718ef // indirect - golang.org/x/net v0.58.0 // indirect golang.org/x/oauth2 v0.36.0 // indirect golang.org/x/sys v0.47.0 // indirect golang.org/x/text v0.41.0 // indirect diff --git a/taskfile/node_http_auth.go b/taskfile/http_auth.go similarity index 62% rename from taskfile/node_http_auth.go rename to taskfile/http_auth.go index 9048b83f..b1871060 100644 --- a/taskfile/node_http_auth.go +++ b/taskfile/http_auth.go @@ -7,7 +7,8 @@ import ( "net/http" "os" "slices" - "strings" + + "golang.org/x/net/http/httpguts" ) // HostHeaders maps a host to the HTTP headers to send when fetching a remote @@ -33,8 +34,8 @@ func (t *authTransport) RoundTrip(req *http.Request) (*http.Response, error) { return t.base.RoundTrip(req) } -// authenticatedClient resolves the headers on each read, not when the node is -// built, so that a run served from the cache needs no credentials. +// authenticatedClient resolves on each read, not at build time, so a cached +// run needs no credentials. func (node *HTTPNode) authenticatedClient() (*http.Client, error) { headers, err := resolveAuthHeaders(node.authHeaders, node.url.Host) if err != nil { @@ -58,8 +59,7 @@ func withAuthHeaders(client *http.Client, host string, headers map[string]string return &authenticated } -// resolveAuthHeaders returns the expanded headers configured for host, or nil -// when no entry matches. +// resolveAuthHeaders returns the expanded headers for host, or nil if none. func resolveAuthHeaders(hostHeaders HostHeaders, host string) (map[string]string, error) { var headers map[string]string for pattern, patternHeaders := range hostHeaders { @@ -77,47 +77,16 @@ func resolveAuthHeaders(hostHeaders HostHeaders, host string) (map[string]string if err := validateHeaderName(name); err != nil { return nil, fmt.Errorf(`remote auth for host %q: %w`, host, err) } - value, err := expandEnv(headers[name]) - if err != nil { - return nil, fmt.Errorf(`remote auth for host %q: header %q: %w`, host, name, err) - } - resolved[name] = value + resolved[name] = os.ExpandEnv(headers[name]) } return resolved, nil } -// expandEnv replaces ${VAR} and $VAR references; `$$` is a literal dollar -// sign. An undefined variable is an error, not an empty header that would only -// surface as an opaque 401. -func expandEnv(value string) (string, error) { - var missing []string - expanded := os.Expand(value, func(name string) string { - if name == "$" { - return "$" - } - v, ok := os.LookupEnv(name) - if !ok { - missing = append(missing, name) - return "" - } - return v - }) - if len(missing) > 0 { - return "", fmt.Errorf("environment variable $%s is not set", strings.Join(missing, ", $")) - } - return expanded, nil -} - -// validateHeaderName reports the offending header by name, where the transport -// would only refuse the request. +// validateHeaderName names the offending header; ReadContext discards the +// transport's own error. func validateHeaderName(name string) error { - if name == "" { - return fmt.Errorf("header name cannot be empty") - } - if strings.ContainsFunc(name, func(r rune) bool { - return r <= ' ' || r == ':' || r == 0x7f - }) { - return fmt.Errorf("header name %q contains invalid characters", name) + if !httpguts.ValidHeaderFieldName(name) { + return fmt.Errorf("invalid header name %q", name) } return nil } diff --git a/taskfile/node_http_auth_test.go b/taskfile/http_auth_test.go similarity index 86% rename from taskfile/node_http_auth_test.go rename to taskfile/http_auth_test.go index 03b81444..30891cbc 100644 --- a/taskfile/node_http_auth_test.go +++ b/taskfile/http_auth_test.go @@ -55,22 +55,28 @@ func TestResolveAuthHeaders(t *testing.T) { //nolint:paralleltest // t.Setenv ca want: map[string]string{"Authorization": "Bearer s3cret"}, }, { - name: "escaped dollar sign", - hostHeaders: HostHeaders{"gitlab.com": {"PRIVATE-TOKEN": "lit$$eral"}}, - host: "gitlab.com", - want: map[string]string{"PRIVATE-TOKEN": "lit$eral"}, - }, - { - name: "undefined environment variable", + name: "undefined environment variable expands to nothing", hostHeaders: HostHeaders{"gitlab.com": {"PRIVATE-TOKEN": "${TASK_TEST_UNSET}"}}, //nolint:gosec // an env var reference, not a credential host: "gitlab.com", - wantErr: `remote auth for host "gitlab.com": header "PRIVATE-TOKEN": environment variable $TASK_TEST_UNSET is not set`, + want: map[string]string{"PRIVATE-TOKEN": ""}, }, { - name: "invalid header name", + name: "header name with a space", hostHeaders: HostHeaders{"gitlab.com": {"PRIVATE TOKEN": "token"}}, host: "gitlab.com", - wantErr: `remote auth for host "gitlab.com": header name "PRIVATE TOKEN" contains invalid characters`, + wantErr: `remote auth for host "gitlab.com": invalid header name "PRIVATE TOKEN"`, + }, + { + name: "header name outside the HTTP token grammar", + hostHeaders: HostHeaders{"gitlab.com": {"X-Foo(bar)": "token"}}, + host: "gitlab.com", + wantErr: `remote auth for host "gitlab.com": invalid header name "X-Foo(bar)"`, + }, + { + name: "empty header name", + hostHeaders: HostHeaders{"gitlab.com": {"": "token"}}, + host: "gitlab.com", + wantErr: `remote auth for host "gitlab.com": invalid header name ""`, }, } @@ -194,18 +200,17 @@ func TestHTTPNodeAuthHeadersNotSentOnRedirect(t *testing.T) { func TestHTTPNodeAuthHeadersResolvedLazily(t *testing.T) { //nolint:paralleltest // t.Setenv cannot be used in parallel tests node, err := NewHTTPNode("https://gitlab.com/Taskfile.yml", "", false, WithAuthHeaders(HostHeaders{ - "gitlab.com": {"PRIVATE-TOKEN": "${TASK_TEST_UNSET}"}, //nolint:gosec // an env var reference, not a credential + "gitlab.com": {"PRIVATE-TOKEN": "${TASK_TEST_LAZY}"}, //nolint:gosec // an env var reference, not a credential }), ) require.NoError(t, err) - _, err = node.authenticatedClient() - require.EqualError(t, err, `remote auth for host "gitlab.com": header "PRIVATE-TOKEN": environment variable $TASK_TEST_UNSET is not set`) + // Defined only after the node was built: the value must still be picked up. + t.Setenv("TASK_TEST_LAZY", "s3cret") - t.Setenv("TASK_TEST_UNSET", "s3cret") - client, err := node.authenticatedClient() + headers, err := resolveAuthHeaders(node.authHeaders, node.url.Host) require.NoError(t, err) - assert.IsType(t, &authTransport{}, client.Transport) + assert.Equal(t, map[string]string{"PRIVATE-TOKEN": "s3cret"}, headers) } type roundTripperFunc func(*http.Request) (*http.Response, error) diff --git a/taskfile/taskfile.go b/taskfile/taskfile.go index 4251a205..00e25c67 100644 --- a/taskfile/taskfile.go +++ b/taskfile/taskfile.go @@ -66,6 +66,13 @@ func RemoteExists(ctx context.Context, u url.URL, client *http.Client) (*url.URL return &u, nil } + // The default names need the same credentials, so trying them would only + // add rejected requests. A 403 is left alone: it is also what a server + // without directory listing answers for a readable directory. + if resp.StatusCode == http.StatusUnauthorized { + return nil, errors.TaskfileFetchFailedError{URI: u.Redacted(), HTTPStatusCode: resp.StatusCode} + } + // If the request was not successful, append the default Taskfile names to // the URL and return the URL of the first successful request for _, taskfile := range DefaultTaskfiles { diff --git a/taskfile/taskfile_test.go b/taskfile/taskfile_test.go new file mode 100644 index 00000000..79eb2cdd --- /dev/null +++ b/taskfile/taskfile_test.go @@ -0,0 +1,84 @@ +package taskfile + +import ( + "net/http" + "net/http/httptest" + "net/url" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/go-task/task/v3/errors" +) + +// alwaysStatus answers every request with the given status. +func alwaysStatus(t *testing.T, status int) (*url.URL, *int) { + t.Helper() + var requests int + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + requests++ + w.WriteHeader(status) + })) + t.Cleanup(srv.Close) + return mustParse(t, srv.URL), &requests +} + +func TestRemoteExistsUnauthorized(t *testing.T) { + t.Parallel() + + u, requests := alwaysStatus(t, http.StatusUnauthorized) + _, err := RemoteExists(t.Context(), *u, http.DefaultClient) + + var fetchErr errors.TaskfileFetchFailedError + require.ErrorAs(t, err, &fetchErr) + assert.Equal(t, http.StatusUnauthorized, fetchErr.HTTPStatusCode) + assert.Equal(t, 1, *requests) +} + +// A 403 is ambiguous, so it keeps the existing behaviour. +func TestRemoteExistsForbiddenEverywhere(t *testing.T) { + t.Parallel() + + u, requests := alwaysStatus(t, http.StatusForbidden) + _, err := RemoteExists(t.Context(), *u, http.DefaultClient) + + var notFoundErr errors.TaskfileNotFoundError + assert.ErrorAs(t, err, ¬FoundErr) + assert.Greater(t, *requests, 1) +} + +func TestRemoteExistsForbiddenDirectoryWithReadableTaskfile(t *testing.T) { + t.Parallel() + + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/Taskfile.yml" { + w.WriteHeader(http.StatusForbidden) + return + } + w.Header().Set("Content-Type", "text/yaml") + w.WriteHeader(http.StatusOK) + })) + defer srv.Close() + + found, err := RemoteExists(t.Context(), *mustParse(t, srv.URL), http.DefaultClient) + require.NoError(t, err) + assert.Equal(t, "/Taskfile.yml", found.Path) +} + +func TestRemoteExistsNotFound(t *testing.T) { + t.Parallel() + + u, _ := alwaysStatus(t, http.StatusNotFound) + _, err := RemoteExists(t.Context(), *u, http.DefaultClient) + + var notFoundErr errors.TaskfileNotFoundError + assert.ErrorAs(t, err, ¬FoundErr) +} + +func mustParse(t *testing.T, rawURL string) *url.URL { + t.Helper() + parsed, err := url.Parse(rawURL) + require.NoError(t, err) + return parsed +} diff --git a/website/src/latest/docs/reference/config.md b/website/src/latest/docs/reference/config.md index 26b80333..83c66f33 100644 --- a/website/src/latest/docs/reference/config.md +++ b/website/src/latest/docs/reference/config.md @@ -326,9 +326,10 @@ commit. Each entry applies to a single host, matched exactly and including the port if the URL has one — the same rule as [`remote.trusted-hosts`](#remote-trusted-hosts). Header values may reference -environment variables with `${VAR}` or `$VAR`; write `$$` for a literal dollar -sign. A variable is only read when Task actually contacts the host, and an -undefined one is reported as an error instead of being sent as an empty header. +environment variables with `${VAR}` or `$VAR`, read when Task contacts the host. +An undefined variable expands to nothing, so the header is sent empty and the +server rejects it — prefer an environment variable over a literal value, which +cannot contain a `$` followed by a name. The header your server expects depends on the service: