diff --git a/internal/flags/flags.go b/internal/flags/flags.go index 7f04fe21..1a3a791e 100644 --- a/internal/flags/flags.go +++ b/internal/flags/flags.go @@ -166,8 +166,7 @@ func init() { pflag.StringVar(&CACert, "cacert", getConfig(config, "REMOTE_CACERT", func() *string { return config.Remote.CACert }, ""), "Path to a custom CA certificate for HTTPS connections.") pflag.StringVar(&Cert, "cert", getConfig(config, "REMOTE_CERT", func() *string { return config.Remote.Cert }, ""), "Path to a client certificate for HTTPS connections.") pflag.StringVar(&CertKey, "cert-key", getConfig(config, "REMOTE_CERT_KEY", func() *string { return config.Remote.CertKey }, ""), "Path to a client certificate key for HTTPS connections.") - // Configurable through the configuration file only: a token given on the - // command line would be visible to any process listing it. + // No flag: a token on the command line is visible to any process listing it. RemoteAuth = remoteAuth(config) // Gentle force experiment will override the force flag and add a new force-all flag @@ -316,9 +315,8 @@ func (o *flagsOption) ApplyToExecutor(e *task.Executor) { ) } -// remoteAuth flattens the configured authentication entries into a lookup by -// host. A host declared twice in the same file keeps its last entry, which is -// the rule the configuration files themselves follow when they are merged. +// remoteAuth flattens the configured entries into a lookup by host, the last +// entry winning as it does when configuration files are merged. func remoteAuth(config *taskrcast.TaskRC) map[string]map[string]string { if config == nil || len(config.Remote.Auth) == 0 { return nil diff --git a/taskfile/node_base.go b/taskfile/node_base.go index 7d552e5a..9a8cafa7 100644 --- a/taskfile/node_base.go +++ b/taskfile/node_base.go @@ -77,8 +77,7 @@ func WithCertKey(certKey string) NodeOption { } } -// WithAuthHeaders sets the HTTP headers to send when the node's host matches -// one of the configured ones. +// WithAuthHeaders sets the HTTP headers to send, keyed by host. func WithAuthHeaders(authHeaders HostHeaders) NodeOption { return func(node *baseNode) { node.authHeaders = authHeaders diff --git a/taskfile/node_http_auth.go b/taskfile/node_http_auth.go index fb8530d4..9048b83f 100644 --- a/taskfile/node_http_auth.go +++ b/taskfile/node_http_auth.go @@ -11,11 +11,9 @@ import ( ) // HostHeaders maps a host to the HTTP headers to send when fetching a remote -// Taskfile from it. Values may reference environment variables using the -// `${VAR}` or `$VAR` syntax. +// Taskfile from it. Values may reference environment variables. type HostHeaders map[string]map[string]string -// authTransport adds the configured headers to every request made to host. type authTransport struct { base http.RoundTripper host string @@ -23,15 +21,11 @@ type authTransport struct { } func (t *authTransport) RoundTrip(req *http.Request) (*http.Response, error) { - // The headers are scoped to a single host. Checking here rather than once - // at build time is what keeps a redirect from carrying the credentials - // somewhere else: the client sends the redirected request through this same - // transport, and Go only strips Authorization, WWW-Authenticate and Cookie - // on its own. + // Re-checked per request: a redirect goes through this same transport, and + // Go only strips Authorization, WWW-Authenticate and Cookie on its own. if !hostMatches(t.host, req.URL.Host) { return t.base.RoundTrip(req) } - // A RoundTripper must not modify the request it is given. req = req.Clone(req.Context()) for name, value := range t.headers { req.Header.Set(name, value) @@ -39,10 +33,8 @@ func (t *authTransport) RoundTrip(req *http.Request) (*http.Response, error) { return t.base.RoundTrip(req) } -// authenticatedClient returns the node's client, wrapped so that it sends the -// configured headers. The environment variables the headers reference are read -// here rather than when the node is built, so that a run served from the cache -// does not require credentials it will never send. +// authenticatedClient resolves the headers on each read, not when the node is +// built, so that a run served from the cache needs no credentials. func (node *HTTPNode) authenticatedClient() (*http.Client, error) { headers, err := resolveAuthHeaders(node.authHeaders, node.url.Host) if err != nil { @@ -54,8 +46,7 @@ func (node *HTTPNode) authenticatedClient() (*http.Client, error) { return withAuthHeaders(node.client, node.url.Host, headers), nil } -// withAuthHeaders returns a copy of client that sends headers to host. The -// client is copied rather than mutated because buildHTTPClient returns the +// withAuthHeaders copies rather than mutates: buildHTTPClient returns the // shared http.DefaultClient when no TLS option is set. func withAuthHeaders(client *http.Client, host string, headers map[string]string) *http.Client { authenticated := *client @@ -67,9 +58,8 @@ func withAuthHeaders(client *http.Client, host string, headers map[string]string return &authenticated } -// resolveAuthHeaders returns the headers configured for host, with their -// environment variable references expanded. It returns nil when no entry -// matches, leaving the request unauthenticated. +// resolveAuthHeaders returns the expanded headers configured for host, or nil +// when no entry matches. func resolveAuthHeaders(hostHeaders HostHeaders, host string) (map[string]string, error) { var headers map[string]string for pattern, patternHeaders := range hostHeaders { @@ -96,10 +86,9 @@ func resolveAuthHeaders(hostHeaders HostHeaders, host string) (map[string]string return resolved, nil } -// expandEnv replaces ${VAR} and $VAR references with the value of the -// environment variable. An undefined variable is an error rather than an empty -// header, which would only surface later as an opaque 401. A literal dollar -// sign is written `$$`. +// 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 { @@ -119,9 +108,8 @@ func expandEnv(value string) (string, error) { return expanded, nil } -// validateHeaderName rejects names that http.Header.Set would silently accept -// but the transport would later refuse, so that the error names the offending -// header instead of the request. +// validateHeaderName reports the offending header by name, where the transport +// would only refuse the request. func validateHeaderName(name string) error { if name == "" { return fmt.Errorf("header name cannot be empty") @@ -134,8 +122,7 @@ func validateHeaderName(name string) error { return nil } -// hostMatches reports whether a host matches a configured pattern. The -// comparison is exact and includes the port, as it does for trusted hosts. +// hostMatches compares exactly, port included, as trusted hosts do. func hostMatches(pattern, host string) bool { return pattern == host } diff --git a/taskfile/node_http_auth_test.go b/taskfile/node_http_auth_test.go index 423adcc5..03b81444 100644 --- a/taskfile/node_http_auth_test.go +++ b/taskfile/node_http_auth_test.go @@ -128,9 +128,8 @@ func TestWithAuthHeadersDoesNotMutateTheDefaultClient(t *testing.T) { assert.IsType(t, &authTransport{}, client.Transport) } -// TestHTTPNodeAuthHeaders covers the whole download: RemoteExists probes the -// URL with a HEAD request before ReadContext issues the GET, and both must -// carry the headers. +// Both requests must carry the headers: RemoteExists probes with HEAD before +// ReadContext issues the GET. func TestHTTPNodeAuthHeaders(t *testing.T) { //nolint:paralleltest // t.Setenv cannot be used in parallel tests var methods []string srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -158,8 +157,7 @@ func TestHTTPNodeAuthHeaders(t *testing.T) { //nolint:paralleltest // t.Setenv c assert.Equal(t, []string{"HEAD", "GET"}, methods) } -// TestHTTPNodeAuthHeadersNotSentOnRedirect guards the credentials against a -// server that bounces the request to a host they were never meant for. +// A server bouncing the request must not get the credentials forwarded to it. func TestHTTPNodeAuthHeadersNotSentOnRedirect(t *testing.T) { t.Parallel() @@ -191,9 +189,8 @@ func TestHTTPNodeAuthHeadersNotSentOnRedirect(t *testing.T) { } } -// TestHTTPNodeAuthHeadersResolvedLazily makes sure a node can be built without -// the credentials it would need to download: a run served from the cache, or an -// offline one, never sends them. +// A node must build without the credentials it would need to download, so that +// cached and offline runs do not require them. 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{ diff --git a/taskrc/ast/taskrc.go b/taskrc/ast/taskrc.go index b0db1a26..7975446d 100644 --- a/taskrc/ast/taskrc.go +++ b/taskrc/ast/taskrc.go @@ -83,10 +83,9 @@ func (t *TaskRC) Merge(other *TaskRC) { t.TempDir = cmp.Or(other.TempDir, t.TempDir) } -// mergeAuth unions two lists of [RemoteAuth] by host. An entry from other -// replaces the entry for the same host as a whole, so that a closer -// configuration file can redefine the headers of a host without inheriting the -// ones it chose to drop. +// mergeAuth unions both lists by host. An entry from other replaces the one +// for the same host as a whole, so a closer file can drop a header rather than +// inherit it. func mergeAuth(base, other []RemoteAuth) []RemoteAuth { if len(other) == 0 { return base