Skip to content

Commit 71f87d0

Browse files
spikecurtisspikecurtis
authored
fix: accept legacy redirect HTTP environment variables (coder#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.
1 parent fc249fa commit 71f87d0

File tree

5 files changed

+113
-11
lines changed

5 files changed

+113
-11
lines changed

‎cli/clibase/cmd.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -226,6 +226,15 @@ func (inv *Invocation) SignalNotifyContext(parent context.Context, signals ...os
226226
return inv.signalNotifyContext(parent, signals...)
227227
}
228228

229+
func (inv *Invocation) WithTestParsedFlags(
230+
_ testing.TB, // ensure we only call this from tests
231+
parsedFlags *pflag.FlagSet,
232+
) *Invocation {
233+
return inv.with(func(i *Invocation) {
234+
i.parsedFlags = parsedFlags
235+
})
236+
}
237+
229238
func (inv *Invocation) Context() context.Context {
230239
if inv.ctx == nil {
231240
return context.Background()

‎cli/server.go

Lines changed: 26 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2319,12 +2319,7 @@ func ConfigureHTTPServers(logger slog.Logger, inv *clibase.Invocation, cfg *code
23192319
return nil, xerrors.New("tls address must be set if tls is enabled")
23202320
}
23212321

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

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

2372+
// redirectHTTPToHTTPSDeprecation handles deprecation of the --tls-redirect-http-to-https flag and
2373+
// "related" environment variables.
2374+
//
2375+
// --tls-redirect-http-to-https used to default to true.
2376+
// It made more sense to have the redirect be opt-in.
2377+
//
2378+
// Also, for a while we have been accepting the environment variable (but not the
2379+
// corresponding flag!) "CODER_TLS_REDIRECT_HTTP", and it appeared in a configuration
2380+
// example, so we keep accepting it to not break backward compat.
2381+
func redirectHTTPToHTTPSDeprecation(ctx context.Context, logger slog.Logger, inv *clibase.Invocation, cfg *codersdk.DeploymentValues) {
2382+
truthy := func(s string) bool {
2383+
b, err := strconv.ParseBool(s)
2384+
if err != nil {
2385+
return false
2386+
}
2387+
return b
2388+
}
2389+
if truthy(inv.Environ.Get("CODER_TLS_REDIRECT_HTTP")) ||
2390+
truthy(inv.Environ.Get("CODER_TLS_REDIRECT_HTTP_TO_HTTPS")) ||
2391+
inv.ParsedFlags().Changed("tls-redirect-http-to-https") {
2392+
logger.Warn(ctx, "⚠️ --tls-redirect-http-to-https is deprecated, please use --redirect-to-access-url instead")
2393+
cfg.RedirectToAccessURL = cfg.TLS.RedirectHTTP
2394+
}
2395+
}
2396+
23772397
// ReadExternalAuthProvidersFromEnv is provided for compatibility purposes with
23782398
// the viper CLI.
23792399
func ReadExternalAuthProvidersFromEnv(environ []string) ([]codersdk.ExternalAuthConfig, error) {

‎cli/server_internal_test.go

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,11 +6,17 @@ import (
66
"crypto/tls"
77
"testing"
88

9+
"github.com/spf13/pflag"
910
"github.com/stretchr/testify/assert"
1011
"github.com/stretchr/testify/require"
1112

1213
"cdr.dev/slog"
1314
"cdr.dev/slog/sloggers/sloghuman"
15+
"cdr.dev/slog/sloggers/slogtest"
16+
17+
"github.com/coder/coder/v2/cli/clibase"
18+
"github.com/coder/coder/v2/codersdk"
19+
"github.com/coder/coder/v2/testutil"
1420
)
1521

1622
func Test_configureCipherSuites(t *testing.T) {
@@ -169,3 +175,71 @@ func Test_configureCipherSuites(t *testing.T) {
169175
})
170176
}
171177
}
178+
179+
func TestRedirectHTTPToHTTPSDeprecation(t *testing.T) {
180+
t.Parallel()
181+
182+
testcases := []struct {
183+
name string
184+
environ clibase.Environ
185+
flags []string
186+
expected bool
187+
}{
188+
{
189+
name: "AllUnset",
190+
environ: clibase.Environ{},
191+
flags: []string{},
192+
expected: false,
193+
},
194+
{
195+
name: "CODER_TLS_REDIRECT_HTTP=true",
196+
environ: clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP", Value: "true"}},
197+
flags: []string{},
198+
expected: true,
199+
},
200+
{
201+
name: "CODER_TLS_REDIRECT_HTTP_TO_HTTPS=true",
202+
environ: clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP_TO_HTTPS", Value: "true"}},
203+
flags: []string{},
204+
expected: true,
205+
},
206+
{
207+
name: "CODER_TLS_REDIRECT_HTTP=false",
208+
environ: clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP", Value: "false"}},
209+
flags: []string{},
210+
expected: false,
211+
},
212+
{
213+
name: "CODER_TLS_REDIRECT_HTTP_TO_HTTPS=false",
214+
environ: clibase.Environ{{Name: "CODER_TLS_REDIRECT_HTTP_TO_HTTPS", Value: "false"}},
215+
flags: []string{},
216+
expected: false,
217+
},
218+
{
219+
name: "--tls-redirect-http-to-https",
220+
environ: clibase.Environ{},
221+
flags: []string{"--tls-redirect-http-to-https"},
222+
expected: true,
223+
},
224+
}
225+
226+
for _, tc := range testcases {
227+
tc := tc
228+
t.Run(tc.name, func(t *testing.T) {
229+
t.Parallel()
230+
ctx := testutil.Context(t, testutil.WaitShort)
231+
logger := slogtest.Make(t, nil)
232+
flags := pflag.NewFlagSet("test", pflag.ContinueOnError)
233+
_ = flags.Bool("tls-redirect-http-to-https", true, "")
234+
err := flags.Parse(tc.flags)
235+
require.NoError(t, err)
236+
inv := (&clibase.Invocation{Environ: tc.environ}).WithTestParsedFlags(t, flags)
237+
cfg := &codersdk.DeploymentValues{}
238+
opts := cfg.Options()
239+
err = opts.SetDefaults()
240+
require.NoError(t, err)
241+
redirectHTTPToHTTPSDeprecation(ctx, logger, inv, cfg)
242+
require.Equal(t, tc.expected, cfg.RedirectToAccessURL.Value())
243+
})
244+
}
245+
}

‎cli/ssh_internal_test.go

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,15 @@ import (
55
"net/url"
66
"testing"
77

8+
"github.com/stretchr/testify/assert"
9+
"github.com/stretchr/testify/require"
810
"golang.org/x/xerrors"
911

1012
"cdr.dev/slog"
1113
"cdr.dev/slog/sloggers/slogtest"
12-
"github.com/coder/coder/v2/testutil"
13-
14-
"github.com/stretchr/testify/assert"
15-
"github.com/stretchr/testify/require"
1614

1715
"github.com/coder/coder/v2/codersdk"
16+
"github.com/coder/coder/v2/testutil"
1817
)
1918

2019
const (

‎docs/admin/configure.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ export CODER_TLS_ENABLE=true
2929
export CODER_TLS_ADDRESS=0.0.0.0:443
3030

3131
## Redirect from HTTP to HTTPS
32-
export CODER_TLS_REDIRECT_HTTP=true
32+
export CODER_REDIRECT_TO_ACCESS_URL=true
3333

3434
# Start the Coder server
3535
coder server

0 commit comments

Comments
 (0)