Bind the app port deliberately and document the proxy deployment (closes #268)
All checks were successful
check / check (push) Successful in 3m46s
All checks were successful
check / check (push) Successful in 3m46s
The plaintext listener bound `:PORT`, so it answered on every interface with no way to say otherwise. That published the admin UI and the unauthenticated receiver in cleartext beside whatever TLS proxy was in front of them, reachable from any host that could route to the machine. BIND_ADDRESS now selects the address. The binary defaults to 127.0.0.1, which is the safe answer for a bare host: reaching webhooker from elsewhere becomes a deliberate act. The image sets 0.0.0.0, which is the correct answer inside a container, where the network namespace is already the boundary and exposure is decided by the publish flag instead — so `-p 127.0.0.1:8080:8080` is what the README shows. Existing container deployments are unaffected. Only IP address literals are accepted: hostnames, host:port and CIDR blocks abort startup naming the variable and the value, and a literal that is not an address of this host fails at listen and exits non-zero. The http.Server is now built in New rather than in the serving goroutine, and sentryEnabled is atomic. Both fields were written by the serving goroutine and read by the fx stop hook with nothing ordering them, and the OnStart hook returns before that goroutine has necessarily run: cleanShutdown could dereference a nil httpServer on an early SIGTERM, and both reads raced. No test started and stopped the server, so nothing observed it. Closes #226. README gains a "Deployment behind a reverse proxy" section: a working nginx server block, and the five things that are silent when wrong — bind or firewall the app port, WEBHOOKER_ENVIRONMENT=prod, TRUSTED_PROXIES, Host as $http_host rather than $host, and keeping the proxy's access log because webhooker's own records only the proxy.
This commit is contained in:
@@ -21,6 +21,22 @@ const (
|
||||
envKeyPort = "PORT"
|
||||
envKeyDebug = "DEBUG"
|
||||
envKeyMaintenanceMode = "MAINTENANCE_MODE"
|
||||
envKeyBindAddress = "BIND_ADDRESS"
|
||||
)
|
||||
|
||||
// Sample BIND_ADDRESS values used by the tables below.
|
||||
const (
|
||||
// bindAddressDefault is the shipped default. It is asserted
|
||||
// against the package's own constant in
|
||||
// TestNewUsesDefaultsWhenUnset, so the two cannot drift.
|
||||
bindAddressDefault = "127.0.0.1"
|
||||
|
||||
// bindAddressWildcard is the value a container deployment sets.
|
||||
bindAddressWildcard = "0.0.0.0"
|
||||
|
||||
// bindAddressSample is an arbitrary specific address, standing
|
||||
// for "one interface of several".
|
||||
bindAddressSample = "10.1.2.3"
|
||||
)
|
||||
|
||||
// envBoolCase is one row of the envBool table.
|
||||
@@ -291,6 +307,160 @@ func TestEnvPort(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestEnvBindAddress covers BIND_ADDRESS parsing.
|
||||
//
|
||||
// Only IP address literals are accepted. Every rejection below is a
|
||||
// value an operator plausibly writes — a hostname, a host:port, a
|
||||
// CIDR block — and each has to abort startup rather than fall back to
|
||||
// the default, because falling back would bind an address other than
|
||||
// the one asked for and, in the wildcard-default case this setting
|
||||
// exists to end, publish cleartext on every interface.
|
||||
func TestEnvBindAddress(t *testing.T) {
|
||||
for _, tt := range envBindAddressCases() {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
// Cannot use t.Parallel() here because t.Setenv
|
||||
// is incompatible with parallel subtests.
|
||||
if tt.set {
|
||||
t.Setenv(testEnvKey, tt.value)
|
||||
} else {
|
||||
require.NoError(t, os.Unsetenv(testEnvKey))
|
||||
}
|
||||
|
||||
got, err := config.EnvBindAddressForTest(
|
||||
testEnvKey, bindAddressDefault,
|
||||
)
|
||||
|
||||
if tt.expectError {
|
||||
require.Error(t, err)
|
||||
require.ErrorIs(t, err, config.ErrInvalidBindAddress)
|
||||
assert.Contains(t, err.Error(), testEnvKey)
|
||||
assert.Contains(t, err.Error(), tt.value)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
require.NoError(t, err)
|
||||
assert.Equal(t, tt.expected, got)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// envBindAddressCase is one row of the envBindAddress table.
|
||||
type envBindAddressCase struct {
|
||||
name string
|
||||
set bool
|
||||
value string
|
||||
expectError bool
|
||||
expected string
|
||||
}
|
||||
|
||||
// envBindAddressCases is the envBindAddress table, kept out of the
|
||||
// test body so the test itself stays readable.
|
||||
func envBindAddressCases() []envBindAddressCase {
|
||||
return append(
|
||||
envBindAddressAcceptedCases(),
|
||||
envBindAddressRejectedCases()...,
|
||||
)
|
||||
}
|
||||
|
||||
// envBindAddressAcceptedCases are the values that parse: the three
|
||||
// spellings of "unset" that take the default, and the literals.
|
||||
func envBindAddressAcceptedCases() []envBindAddressCase {
|
||||
return []envBindAddressCase{
|
||||
{
|
||||
name: "unset returns the default",
|
||||
expected: bindAddressDefault,
|
||||
},
|
||||
{
|
||||
name: "empty returns the default",
|
||||
set: true,
|
||||
value: "",
|
||||
expected: bindAddressDefault,
|
||||
},
|
||||
{
|
||||
name: "whitespace returns the default",
|
||||
set: true,
|
||||
value: " ",
|
||||
expected: bindAddressDefault,
|
||||
},
|
||||
{
|
||||
name: "ipv4 wildcard is parsed",
|
||||
set: true,
|
||||
value: bindAddressWildcard,
|
||||
expected: bindAddressWildcard,
|
||||
},
|
||||
{
|
||||
name: "ipv4 literal is parsed",
|
||||
set: true,
|
||||
value: bindAddressSample,
|
||||
expected: bindAddressSample,
|
||||
},
|
||||
{
|
||||
name: "surrounding whitespace is trimmed",
|
||||
set: true,
|
||||
value: " " + bindAddressSample + " ",
|
||||
expected: bindAddressSample,
|
||||
},
|
||||
{
|
||||
name: "ipv6 wildcard is parsed",
|
||||
set: true,
|
||||
value: "::",
|
||||
expected: "::",
|
||||
},
|
||||
{
|
||||
name: "ipv6 literal is parsed",
|
||||
set: true,
|
||||
value: "2001:db8::5",
|
||||
expected: "2001:db8::5",
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
// envBindAddressRejectedCases are the values that abort startup.
|
||||
// Each is something an operator plausibly writes, and none may fall
|
||||
// back to the default: the default is loopback, so a silent fallback
|
||||
// would bind somewhere other than what was asked for.
|
||||
func envBindAddressRejectedCases() []envBindAddressCase {
|
||||
return []envBindAddressCase{
|
||||
{
|
||||
name: "garbage is rejected",
|
||||
set: true,
|
||||
value: "not-an-address",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "hostname is rejected",
|
||||
set: true,
|
||||
value: "localhost",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "unresolvable hostname is rejected",
|
||||
set: true,
|
||||
value: "no-such-host.invalid",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "host and port is rejected",
|
||||
set: true,
|
||||
value: bindAddressDefault + ":8080",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "bracketed ipv6 is rejected",
|
||||
set: true,
|
||||
value: "[::1]",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "CIDR block is rejected",
|
||||
set: true,
|
||||
value: "10.0.0.0/8",
|
||||
expectError: true,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
// buildConfig constructs a Config through fx exactly as the
|
||||
// application does, returning the config and any construction error.
|
||||
func buildConfig(t *testing.T) (*config.Config, error) {
|
||||
@@ -312,13 +482,45 @@ func buildConfig(t *testing.T) (*config.Config, error) {
|
||||
}
|
||||
|
||||
func TestNewRejectsBadEnvValues(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
key string
|
||||
value string
|
||||
expectError bool
|
||||
check func(t *testing.T, cfg *config.Config)
|
||||
}{
|
||||
for _, tt := range badEnvValueCases() {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
// Cannot use t.Parallel() here because t.Setenv
|
||||
// is incompatible with parallel subtests.
|
||||
t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
|
||||
t.Setenv(tt.key, tt.value)
|
||||
|
||||
cfg, err := buildConfig(t)
|
||||
|
||||
if tt.expectError {
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), tt.key)
|
||||
assert.Contains(t, err.Error(), tt.value)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, cfg)
|
||||
tt.check(t, cfg)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// badEnvValueCase is one row of the config.New table: a variable, the
|
||||
// value it is set to, and either the assertion that startup fails
|
||||
// naming both, or a check on the Config that resulted.
|
||||
type badEnvValueCase struct {
|
||||
name string
|
||||
key string
|
||||
value string
|
||||
expectError bool
|
||||
check func(t *testing.T, cfg *config.Config)
|
||||
}
|
||||
|
||||
// badEnvValueCases is the config.New table, kept out of the test body
|
||||
// so the test itself stays readable.
|
||||
func badEnvValueCases() []badEnvValueCase {
|
||||
return []badEnvValueCase{
|
||||
{
|
||||
name: "valid PORT is used",
|
||||
key: envKeyPort,
|
||||
@@ -361,29 +563,35 @@ func TestNewRejectsBadEnvValues(t *testing.T) {
|
||||
value: "sometimes",
|
||||
expectError: true,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
// Cannot use t.Parallel() here because t.Setenv
|
||||
// is incompatible with parallel subtests.
|
||||
t.Setenv("WEBHOOKER_ENVIRONMENT", "dev")
|
||||
t.Setenv(tt.key, tt.value)
|
||||
|
||||
cfg, err := buildConfig(t)
|
||||
|
||||
if tt.expectError {
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), tt.key)
|
||||
assert.Contains(t, err.Error(), tt.value)
|
||||
|
||||
return
|
||||
}
|
||||
|
||||
require.NoError(t, err)
|
||||
require.NotNil(t, cfg)
|
||||
tt.check(t, cfg)
|
||||
})
|
||||
{
|
||||
name: "valid BIND_ADDRESS is used",
|
||||
key: envKeyBindAddress,
|
||||
value: bindAddressWildcard,
|
||||
check: func(t *testing.T, cfg *config.Config) {
|
||||
t.Helper()
|
||||
assert.Equal(
|
||||
t, bindAddressWildcard, cfg.BindAddress,
|
||||
)
|
||||
},
|
||||
},
|
||||
{
|
||||
name: "unparseable BIND_ADDRESS aborts startup",
|
||||
key: envKeyBindAddress,
|
||||
value: "not-an-address",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "hostname BIND_ADDRESS aborts startup",
|
||||
key: envKeyBindAddress,
|
||||
value: "localhost",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "BIND_ADDRESS with a port aborts startup",
|
||||
key: envKeyBindAddress,
|
||||
value: bindAddressDefault + ":8080",
|
||||
expectError: true,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
@@ -395,6 +603,7 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) {
|
||||
|
||||
for _, key := range []string{
|
||||
envKeyPort, envKeyDebug, envKeyMaintenanceMode,
|
||||
envKeyBindAddress,
|
||||
} {
|
||||
require.NoError(t, os.Unsetenv(key))
|
||||
}
|
||||
@@ -406,4 +615,15 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) {
|
||||
assert.Equal(t, 8080, cfg.Port)
|
||||
assert.False(t, cfg.Debug)
|
||||
assert.False(t, cfg.MaintenanceMode)
|
||||
|
||||
// Loopback, not the wildcard: the default must not publish the
|
||||
// cleartext admin UI and the unauthenticated receiver on every
|
||||
// interface of a host that configured nothing. The value is read
|
||||
// from the package rather than repeated, so the README's
|
||||
// documented default and the compiled-in one are pinned to the
|
||||
// same constant.
|
||||
assert.Equal(
|
||||
t, config.DefaultBindAddressForTest, cfg.BindAddress,
|
||||
)
|
||||
assert.Equal(t, bindAddressDefault, cfg.BindAddress)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user