fix: accept legacy redirect HTTP environment variables

spikecurtis · spikecurtis · commit 30f40f10b521 · 2023-11-17T12:41:56.000+04:00
diff --git a/cli/clibase/cmd.go b/cli/clibase/cmd.go
@@ -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()
diff --git a/cli/server.go b/cli/server.go
@@ -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,24 @@ 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) {
+	if inv.Environ.Get("CODER_TLS_REDIRECT_HTTP") == "true" ||
+		inv.Environ.Get("CODER_TLS_REDIRECT_HTTP_TO_HTTPS") == "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
+	}
+}
+
 // ReadExternalAuthProvidersFromEnv is provided for compatibility purposes with
 // the viper CLI.
 func ReadExternalAuthProvidersFromEnv(environ []string) ([]codersdk.ExternalAuthConfig, error) {
diff --git a/cli/ssh_internal_test.go b/cli/ssh_internal_test.go
@@ -4,10 +4,15 @@ import (
 	"net/url"
 	"testing"
 
+	"github.com/spf13/pflag"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
+	"cdr.dev/slog/sloggers/slogtest"
+
+	"github.com/coder/coder/v2/cli/clibase"
 	"github.com/coder/coder/v2/codersdk"
+	"github.com/coder/coder/v2/testutil"
 )
 
 const (
@@ -56,3 +61,71 @@ func TestBuildWorkspaceLink(t *testing.T) {
 
 	assert.Equal(t, workspaceLink.String(), fakeServerURL+"/@"+fakeOwnerName+"/"+fakeWorkspaceName)
 }
+
+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())
+		})
+	}
+}
diff --git a/docs/admin/configure.md b/docs/admin/configure.md
@@ -29,7 +29,7 @@ export CODER_TLS_ENABLE=true
 export CODER_TLS_ADDRESS=0.0.0.0:443
 
 ## Redirect from HTTP to HTTPS
-export CODER_TLS_REDIRECT_HTTP=true
+export CODER_REDIRECT_TO_ACCESS_URL=true
 
 # Start the Coder server
 coder server
