fix: accept legacy redirect HTTP environment variables (#10748)

> Can someone help me understand the differences between these env variables: > > CODER_REDIRECT_TO_ACCESS_URL > CODER_TLS_REDIRECT_HTTP_TO_HTTPS > CODER_TLS_REDIRECT_HTTP Oh man, what a mess. It looks like `CODER_TLS_REDIRECT_HTTP ` appears in our config docs. Maybe that was the initial name for the environment variable? At some point, both the flag and the environment variable were `--tls-redirect-http-to-https` and `CODER_TLS_REDIRECT_HTTP_TO_HTTPS`. `CODER_TLS_REDIRECT_HTTP` did nothing. However, then we introduced `CODER_REDIRECT_TO_ACCESS_URL`, we put in some deprecation code that was maybe fat-fingered such that we accept the environment variable `CODER_TLS_REDIRECT_HTTP` but the flag `--tls-redirect-http-to-https`. Our docs still refer to `CODER_TLS_REDIRECT_HTTP` at https://coder.com/docs/v2/latest/admin/configure#address So, I think what we gotta do is still accept `CODER_TLS_REDIRECT_HTTP` since it was working and in an example doc, but also fix the deprecation code to accept `CODER_TLS_REDIRECT_HTTP_TO_HTTPS` environment variable.
2026-09-24 15:04:27 +08:00 · 2023-11-17 15:09:29 +04:00
parent fc249fab1e
commit 71f87d054f
5 changed files with 113 additions and 11 deletions
@@ -226,6 +226,15 @@ func (inv *Invocation) SignalNotifyContext(parent context.Context, signals ...os
 	return inv.signalNotifyContext(parent, signals...)
 }

+func (inv *Invocation) WithTestParsedFlags(
+	_ testing.TB, // ensure we only call this from tests
+	parsedFlags *pflag.FlagSet,
+) *Invocation {
+	return inv.with(func(i *Invocation) {
+		i.parsedFlags = parsedFlags
+	})
+}
+
 func (inv *Invocation) Context() context.Context {
 	if inv.ctx == nil {
 		return context.Background()
@@ -2319,12 +2319,7 @@ func ConfigureHTTPServers(logger slog.Logger, inv *clibase.Invocation, cfg *code
 			return nil, xerrors.New("tls address must be set if tls is enabled")
 		}

-		// DEPRECATED: This redirect used to default to true.
-		// It made more sense to have the redirect be opt-in.
-		if inv.Environ.Get("CODER_TLS_REDIRECT_HTTP") == "true" || inv.ParsedFlags().Changed("tls-redirect-http-to-https") {
-			logger.Warn(ctx, "--tls-redirect-http-to-https is deprecated, please use --redirect-to-access-url instead")
-			cfg.RedirectToAccessURL = cfg.TLS.RedirectHTTP
-		}
+		redirectHTTPToHTTPSDeprecation(ctx, logger, inv, cfg)

 		tlsConfig, err := configureServerTLS(
 			ctx,
@@ -2374,6 +2369,31 @@ func ConfigureHTTPServers(logger slog.Logger, inv *clibase.Invocation, cfg *code
 	return httpServers, nil
 }

+// redirectHTTPToHTTPSDeprecation handles deprecation of the --tls-redirect-http-to-https flag and
+// "related" environment variables.
+//
+// --tls-redirect-http-to-https used to default to true.
+// It made more sense to have the redirect be opt-in.
+//
+// Also, for a while we have been accepting the environment variable (but not the
+// corresponding flag!) "CODER_TLS_REDIRECT_HTTP", and it appeared in a configuration
+// example, so we keep accepting it to not break backward compat.
+func redirectHTTPToHTTPSDeprecation(ctx context.Context, logger slog.Logger, inv *clibase.Invocation, cfg *codersdk.DeploymentValues) {
+	truthy := func(s string) bool {
+		b, err := strconv.ParseBool(s)
+		if err != nil {
+			return false
+		}
+		return b
+	}
+	if truthy(inv.Environ.Get("CODER_TLS_REDIRECT_HTTP")) ||
+		truthy(inv.Environ.Get("CODER_TLS_REDIRECT_HTTP_TO_HTTPS")) ||
+		inv.ParsedFlags().Changed("tls-redirect-http-to-https") {
+		logger.Warn(ctx, "⚠️ --tls-redirect-http-to-https is deprecated, please use --redirect-to-access-url instead")
+		cfg.RedirectToAccessURL = cfg.TLS.RedirectHTTP
+	}
+}
+
 // ReadExternalAuthProvidersFromEnv is provided for compatibility purposes with
 // the viper CLI.
 func ReadExternalAuthProvidersFromEnv(environ []string) ([]codersdk.ExternalAuthConfig, error) {
@@ -6,11 +6,17 @@ import (
 	"crypto/tls"
 	"testing"

+	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"

 	"cdr.dev/slog"
 	"cdr.dev/slog/sloggers/sloghuman"
+	"cdr.dev/slog/sloggers/slogtest"
+
+	"github.com/coder/coder/v2/cli/clibase"
+	"github.com/coder/coder/v2/codersdk"
+	"github.com/coder/coder/v2/testutil"
 )

 func Test_configureCipherSuites(t *testing.T) {
@@ -169,3 +175,71 @@ func Test_configureCipherSuites(t *testing.T) {
 		})
 	}
 }
+
+func TestRedirectHTTPToHTTPSDeprecation(t *testing.T) {
+	t.Parallel()
+
+	testcases := []struct {
+		name     string
+		environ  clibase.Environ
+		flags    []string
+		expected bool
+	}{
+		{
+			name:     "AllUnset",
+			environ:  clibase.Environ{},
+			flags:    []string{},
+			expected: false,
+		},
+		{
+			name:     "CODER_TLS_REDIRECT_HTTP=true",
+			environ:  clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP", Value: "true"}},
+			flags:    []string{},
+			expected: true,
+		},
+		{
+			name:     "CODER_TLS_REDIRECT_HTTP_TO_HTTPS=true",
+			environ:  clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP_TO_HTTPS", Value: "true"}},
+			flags:    []string{},
+			expected: true,
+		},
+		{
+			name:     "CODER_TLS_REDIRECT_HTTP=false",
+			environ:  clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP", Value: "false"}},
+			flags:    []string{},
+			expected: false,
+		},
+		{
+			name:     "CODER_TLS_REDIRECT_HTTP_TO_HTTPS=false",
+			environ:  clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP_TO_HTTPS", Value: "false"}},
+			flags:    []string{},
+			expected: false,
+		},
+		{
+			name:     "--tls-redirect-http-to-https",
+			environ:  clibase.Environ{},
+			flags:    []string{"--tls-redirect-http-to-https"},
+			expected: true,
+		},
+	}
+
+	for _, tc := range testcases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			ctx := testutil.Context(t, testutil.WaitShort)
+			logger := slogtest.Make(t, nil)
+			flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
+			_ = flags.Bool("tls-redirect-http-to-https", true, "")
+			err := flags.Parse(tc.flags)
+			require.NoError(t, err)
+			inv := (&clibase.Invocation{Environ: tc.environ}).WithTestParsedFlags(t, flags)
+			cfg := &codersdk.DeploymentValues{}
+			opts := cfg.Options()
+			err = opts.SetDefaults()
+			require.NoError(t, err)
+			redirectHTTPToHTTPSDeprecation(ctx, logger, inv, cfg)
+			require.Equal(t, tc.expected, cfg.RedirectToAccessURL.Value())
+		})
+	}
+}
@@ -5,16 +5,15 @@ import (
 	"net/url"
 	"testing"

+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	"golang.org/x/xerrors"

 	"cdr.dev/slog"
 	"cdr.dev/slog/sloggers/slogtest"
-	"github.com/coder/coder/v2/testutil"
-
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"

 	"github.com/coder/coder/v2/codersdk"
+	"github.com/coder/coder/v2/testutil"
 )

 const (