Fail loudly on an unparseable SENTRY_DSN and a malformed .env (closes #283)

internal/config/config.go

@@ -4,6 +4,7 @@ package config
 import (
 	"errors"
 	"fmt"
+	"io/fs"
 	"log/slog"
 	"net/netip"
 	"os"
@@ -11,13 +12,11 @@ import (
 	"strings"
 	"time"
 
+	"github.com/getsentry/sentry-go"
+	"github.com/joho/godotenv"
 	"go.uber.org/fx"
 	"sneak.berlin/go/webhooker/internal/globals"
 	"sneak.berlin/go/webhooker/internal/logger"
-
-	// Populates the environment from a ./.env file automatically for
-	// development configuration. Kept in one place only (here).
-	_ "github.com/joho/godotenv/autoload"
 )
 
 const (
@@ -84,6 +83,12 @@ const (
 	// IPv6 prefix spends on the ::ffff:0:0/96 wrapper, so a /104
 	// covers the same addresses as an IPv4 /8.
 	mappedV4Offset = 96
+
+	// DotEnvPath is the optional file of KEY=value lines read into the
+	// environment at startup, relative to the process working
+	// directory. Exported so that documentation and tests name the
+	// same path the loader opens.
+	DotEnvPath = ".env"
 )
 
 // ErrInvalidEnvironment is returned when WEBHOOKER_ENVIRONMENT
@@ -107,6 +112,15 @@ var ErrInvalidCIDR = errors.New("invalid CIDR")
 // something that is not an IP address literal.
 var ErrInvalidBindAddress = errors.New("invalid bind address")
 
+// ErrInvalidSentryDSN is returned when SENTRY_DSN is set to something
+// the Sentry SDK cannot parse as a DSN.
+var ErrInvalidSentryDSN = errors.New("invalid Sentry DSN")
+
+// ErrDotEnvUnreadable is returned when the optional .env file exists
+// but cannot be read or parsed. A file that is not there is not an
+// error; a file that is there and broken is.
+var ErrDotEnvUnreadable = errors.New("unreadable .env file")
+
 // ErrIncompleteMetricsAuth is returned when exactly one of
 // METRICS_USERNAME and METRICS_PASSWORD carries a value. Neither
 // fallback is acceptable: serving /metrics on the username alone
@@ -212,12 +226,62 @@ func (c *Config) MetricsAuthEnabled() bool {
 	return c.MetricsUsername != "" && c.MetricsPassword != ""
 }
 
+// SentryEnabled reports whether error reporting is shipped to Sentry.
+// It is the only answer to that question in the codebase: the SDK
+// initialisation, the sentryhttp middleware registration and the
+// startup log's sentryEnabled field all read this one method, so the
+// log cannot report reporting as on while nothing is sending.
+//
+// A non-empty DSN is enough because loadFromEnv already parsed it with
+// the SDK's own parser and refused to build a Config around one the
+// SDK would reject, and because initialising the SDK with a DSN that
+// parsed and failed anyway aborts the process rather than leaving this
+// true and the client absent.
+func (c *Config) SentryEnabled() bool {
+	return c.SentryDSN != ""
+}
+
 // envString returns the value of the named environment variable,
 // or an empty string if not set.
 func envString(key string) string {
 	return os.Getenv(key)
 }
 
+// LoadDotEnv reads DotEnvPath into the environment when that file is
+// present, and reports a file that is present but broken.
+//
+// It has to run before anything reads the environment, so that every
+// reader agrees on what the environment holds — the DATA_DIR lock
+// taken before the fx graph exists as much as loadFromEnv itself. A
+// variable already set in the real environment wins: godotenv never
+// overwrites one.
+//
+// A missing file is not an error. It is a development convenience and
+// most deployments set the environment directly.
+//
+// Any other failure is. godotenv parses the whole file before setting
+// anything, so a single malformed line applies none of it: every
+// variable in the file silently reverts to its default, which defeats
+// the fail-loud guarantee for all of them at once.
+func LoadDotEnv() error {
+	return loadDotEnvFile(DotEnvPath)
+}
+
+// loadDotEnvFile is LoadDotEnv over a named file, so tests can point
+// at a temporary one instead of the process working directory.
+func loadDotEnvFile(path string) error {
+	err := godotenv.Load(path)
+	if err == nil || errors.Is(err, fs.ErrNotExist) {
+		return nil
+	}
+
+	return fmt.Errorf(
+		"%w: %s: %w; nothing in it was applied, so fix the file or "+
+			"remove it",
+		ErrDotEnvUnreadable, path, err,
+	)
+}
+
 // DataDir resolves DATA_DIR, applying DefaultDataDir when it is unset
 // or empty. It is exported so that entry points which must act on the
 // data directory before the fx graph exists — taking the exclusive
@@ -462,6 +526,41 @@ func envBindAddress(key, defaultValue string) (string, error) {
 	return addr.String(), nil
 }
 
+// envSentryDSN returns the value of the named environment variable
+// checked as a Sentry DSN. An unset (or empty, or whitespace-only)
+// value yields "", which means error reporting stays off — the common
+// case, and a normal start.
+//
+// A set value is parsed with sentry.NewDsn, which is the call
+// sentry.Init makes on the DSN it is handed, so what passes here is
+// exactly what the SDK will accept later and the two cannot disagree.
+// Reproducing the check by hand instead would cost this package its
+// dependency on the SDK — already a module dependency, already linked
+// into the binary — in exchange for a second definition of "valid DSN"
+// free to drift from the one that decides.
+//
+// A set value that does not parse is a hard error naming the key, so
+// startup fails loudly. Losing error reporting is the failure this
+// variable exists to prevent, and a typo in a DSN is silent forever:
+// nothing later in the process can notice that reports are going
+// nowhere. The bad value is quoted because it is a URL to a public
+// endpoint carrying a public key, not a secret.
+func envSentryDSN(key string) (string, error) {
+	v := strings.TrimSpace(os.Getenv(key))
+	if v == "" {
+		return "", nil
+	}
+
+	_, err := sentry.NewDsn(v)
+	if err != nil {
+		return "", fmt.Errorf(
+			"%w: %s: %q: %w", ErrInvalidSentryDSN, key, v, err,
+		)
+	}
+
+	return v, nil
+}
+
 // resolveMetricsAuth reads the /metrics basic-auth credentials and
 // rejects a half-set pair, naming both variables either way. The
 // error carries neither value: the password is a secret.
@@ -594,6 +693,11 @@ func loadFromEnv() (*Config, error) {
 		return nil, err
 	}
 
+	sentryDSN, err := envSentryDSN("SENTRY_DSN")
+	if err != nil {
+		return nil, err
+	}
+
 	return &Config{
 		DataDir:                DataDir(),
 		Debug:                  debug,
@@ -603,7 +707,7 @@ func loadFromEnv() (*Config, error) {
 		MetricsPassword:        metricsPassword,
 		Port:                   port,
 		BindAddress:            bindAddress,
-		SentryDSN:              envString("SENTRY_DSN"),
+		SentryDSN:              sentryDSN,
 		RetentionSweepInterval: retentionSweepInterval,
 		SessionIdleTimeout:     sessionIdleTimeout,
 		ReceiverRateLimit:      receiverRateLimit,
@@ -738,7 +842,7 @@ func New(lc fx.Lifecycle, params ConfigParams) (*Config, error) {
 		"receiverRateLimit", s.ReceiverRateLimit,
 		"trustedProxies", len(s.TrustedProxies),
 		"allowedEgressCIDRs", len(s.AllowedEgressCIDRs),
-		"hasSentryDSN", s.SentryDSN != "",
+		"sentryEnabled", s.SentryEnabled(),
 		"hasMetricsAuth", s.MetricsAuthEnabled(),
 	)
 

internal/config/dotenv_test.go

@@ -0,0 +1,158 @@
+package config_test
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/config"
+)
+
+// dotEnvKey is a throwaway variable name the .env tests write and
+// read, so they cannot disturb real configuration.
+const dotEnvKey = "WEBHOOKER_TEST_DOTENV_VALUE"
+
+// malformedDotEnv is a file godotenv cannot parse. The first line is
+// the realistic typo — a space where the `=` belongs — and the rest
+// make sure nothing downstream treats the file as salvageable line by
+// line.
+const malformedDotEnv = "PORT 19615\n" +
+	"this is not = valid ! syntax\n" +
+	"\"unclosed\n"
+
+// unsetDotEnvKey makes dotEnvKey genuinely absent for the duration of
+// the test and restores it afterwards. t.Setenv registers the restore;
+// the Unsetenv that follows is what the test actually needs, because a
+// variable set to the empty string is still present in os.Environ and
+// godotenv would refuse to overwrite it.
+func unsetDotEnvKey(t *testing.T) {
+	t.Helper()
+	t.Setenv(dotEnvKey, "placeholder")
+	require.NoError(t, os.Unsetenv(dotEnvKey))
+}
+
+// writeDotEnv writes contents to a .env file in a fresh temporary
+// directory and returns its path.
+func writeDotEnv(t *testing.T, contents string) string {
+	t.Helper()
+
+	path := filepath.Join(t.TempDir(), config.DotEnvPath)
+	require.NoError(t, os.WriteFile(path, []byte(contents), 0o600))
+
+	return path
+}
+
+// TestLoadDotEnv_MissingFileIsFine pins the case most deployments are
+// in. The file is optional: it is a development convenience, and a
+// deployment that configures the environment directly must start
+// normally rather than be refused for a file it was never meant to
+// have.
+//
+//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
+func TestLoadDotEnv_MissingFileIsFine(t *testing.T) {
+	unsetDotEnvKey(t)
+
+	absent := filepath.Join(t.TempDir(), config.DotEnvPath)
+	require.NoError(t, config.LoadDotEnvFileForTest(absent))
+
+	_, present := os.LookupEnv(dotEnvKey)
+	assert.False(t, present, "nothing may be set from an absent file")
+}
+
+// TestLoadDotEnv_AppliesValues pins that a well-formed file still
+// reaches the environment, which is the whole reason the file is read
+// at all.
+//
+//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
+func TestLoadDotEnv_AppliesValues(t *testing.T) {
+	unsetDotEnvKey(t)
+
+	path := writeDotEnv(t, "# a comment\n"+dotEnvKey+"=from-dot-env\n")
+
+	require.NoError(t, config.LoadDotEnvFileForTest(path))
+	assert.Equal(t, "from-dot-env", os.Getenv(dotEnvKey))
+}
+
+// TestLoadDotEnv_RealEnvironmentWins pins that the file cannot
+// override a variable the process was actually started with. A
+// deployment that sets DATA_DIR in its unit file must not have it
+// silently replaced by a stale .env left in the working directory.
+func TestLoadDotEnv_RealEnvironmentWins(t *testing.T) {
+	t.Setenv(dotEnvKey, "from-environment")
+
+	path := writeDotEnv(t, dotEnvKey+"=from-dot-env\n")
+
+	require.NoError(t, config.LoadDotEnvFileForTest(path))
+	assert.Equal(t, "from-environment", os.Getenv(dotEnvKey))
+}
+
+// TestLoadDotEnv_MalformedFileAborts is the defect this fixes. One bad
+// line makes godotenv apply none of the file, so every variable in it
+// reverts to its default; the process used to start that way with no
+// log line naming the file at all.
+//
+//nolint:paralleltest // unsetDotEnvKey uses t.Setenv.
+func TestLoadDotEnv_MalformedFileAborts(t *testing.T) {
+	unsetDotEnvKey(t)
+
+	path := writeDotEnv(
+		t, malformedDotEnv+dotEnvKey+"=from-dot-env\n",
+	)
+
+	err := config.LoadDotEnvFileForTest(path)
+
+	require.Error(t, err)
+	require.ErrorIs(t, err, config.ErrDotEnvUnreadable)
+	assert.Contains(
+		t, err.Error(), config.DotEnvPath,
+		"the failure must name the file it could not read",
+	)
+
+	_, present := os.LookupEnv(dotEnvKey)
+	assert.False(
+		t, present,
+		"a rejected file must apply nothing, not part of itself",
+	)
+}
+
+// TestLoadDotEnv_UnreadableFileAborts pins that only absence is
+// tolerated. A .env that exists but cannot be read is a file the
+// operator meant to be applied, so it fails like a malformed one
+// rather than being treated as though it were not there.
+func TestLoadDotEnv_UnreadableFileAborts(t *testing.T) {
+	t.Parallel()
+
+	// A directory in the file's place: open succeeds and the read
+	// fails, which no umask or root-ness can turn back into success
+	// the way a chmod could.
+	path := filepath.Join(t.TempDir(), config.DotEnvPath)
+	require.NoError(t, os.Mkdir(path, 0o750))
+
+	err := config.LoadDotEnvFileForTest(path)
+
+	require.Error(t, err)
+	require.ErrorIs(t, err, config.ErrDotEnvUnreadable)
+}
+
+// TestLoadDotEnv_ReadsTheWorkingDirectory pins the path LoadDotEnv
+// itself opens, which the tests above bypass. It is relative to the
+// process working directory, as it was under godotenv/autoload and as
+// the README documents.
+//
+//nolint:paralleltest // t.Chdir moves the whole process.
+func TestLoadDotEnv_ReadsTheWorkingDirectory(t *testing.T) {
+	unsetDotEnvKey(t)
+
+	dir := t.TempDir()
+	require.NoError(t, os.WriteFile(
+		filepath.Join(dir, config.DotEnvPath),
+		[]byte(dotEnvKey+"=from-working-directory\n"),
+		0o600,
+	))
+	t.Chdir(dir)
+
+	require.NoError(t, config.LoadDotEnv())
+	assert.Equal(t, "from-working-directory", os.Getenv(dotEnvKey))
+}

internal/config/env_test.go

@@ -518,8 +518,20 @@ type badEnvValueCase struct {
 }
 
 // badEnvValueCases is the config.New table, kept out of the test body
-// so the test itself stays readable.
+// so the test itself stays readable. It is assembled from per-variable
+// groups because one literal covering every variable outgrew the
+// function-length budget.
 func badEnvValueCases() []badEnvValueCase {
+	cases := listenerEnvValueCases()
+	cases = append(cases, flagEnvValueCases()...)
+	cases = append(cases, sentryEnvValueCases()...)
+
+	return cases
+}
+
+// listenerEnvValueCases covers the two variables that describe the
+// HTTP listener.
+func listenerEnvValueCases() []badEnvValueCase {
 	return []badEnvValueCase{
 		{
 			name:  "valid PORT is used",
@@ -542,27 +554,6 @@ func badEnvValueCases() []badEnvValueCase {
 			value:       "70000",
 			expectError: true,
 		},
-		{
-			name:  "valid DEBUG is used",
-			key:   envKeyDebug,
-			value: "true",
-			check: func(t *testing.T, cfg *config.Config) {
-				t.Helper()
-				assert.True(t, cfg.Debug)
-			},
-		},
-		{
-			name:        "unparseable DEBUG aborts startup",
-			key:         envKeyDebug,
-			value:       "ture",
-			expectError: true,
-		},
-		{
-			name:        "unparseable MAINTENANCE_MODE aborts startup",
-			key:         envKeyMaintenanceMode,
-			value:       "sometimes",
-			expectError: true,
-		},
 		{
 			name:  "valid BIND_ADDRESS is used",
 			key:   envKeyBindAddress,
@@ -595,6 +586,69 @@ func badEnvValueCases() []badEnvValueCase {
 	}
 }
 
+// flagEnvValueCases covers the boolean variables.
+func flagEnvValueCases() []badEnvValueCase {
+	return []badEnvValueCase{
+		{
+			name:  "valid DEBUG is used",
+			key:   envKeyDebug,
+			value: "true",
+			check: func(t *testing.T, cfg *config.Config) {
+				t.Helper()
+				assert.True(t, cfg.Debug)
+			},
+		},
+		{
+			name:        "unparseable DEBUG aborts startup",
+			key:         envKeyDebug,
+			value:       "ture",
+			expectError: true,
+		},
+		{
+			name:        "unparseable MAINTENANCE_MODE aborts startup",
+			key:         envKeyMaintenanceMode,
+			value:       "sometimes",
+			expectError: true,
+		},
+	}
+}
+
+// sentryEnvValueCases covers SENTRY_DSN. The three rejected values are
+// the ones measured on the defect: each initialised the SDK with an
+// error and left the process serving with error reporting off.
+func sentryEnvValueCases() []badEnvValueCase {
+	return []badEnvValueCase{
+		{
+			name:  "valid SENTRY_DSN is used",
+			key:   envKeySentryDSN,
+			value: validSentryDSN,
+			check: func(t *testing.T, cfg *config.Config) {
+				t.Helper()
+				assert.Equal(t, validSentryDSN, cfg.SentryDSN)
+				assert.True(t, cfg.SentryEnabled())
+			},
+		},
+		{
+			name:        "unparseable SENTRY_DSN aborts startup",
+			key:         envKeySentryDSN,
+			value:       "not-a-dsn",
+			expectError: true,
+		},
+		{
+			name:        "SENTRY_DSN that is not a URL aborts startup",
+			key:         envKeySentryDSN,
+			value:       "%%%",
+			expectError: true,
+		},
+		{
+			name:        "keyless SENTRY_DSN aborts startup",
+			key:         envKeySentryDSN,
+			value:       "https://example.invalid/1",
+			expectError: true,
+		},
+	}
+}
+
 // TestNewUsesDefaultsWhenUnset proves the fail-loud behaviour did not
 // break the legitimate unset case: absent variables still get their
 // documented defaults.
@@ -603,7 +657,7 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) {
 
 	for _, key := range []string{
 		envKeyPort, envKeyDebug, envKeyMaintenanceMode,
-		envKeyBindAddress,
+		envKeyBindAddress, envKeySentryDSN,
 	} {
 		require.NoError(t, os.Unsetenv(key))
 	}
@@ -626,4 +680,9 @@ func TestNewUsesDefaultsWhenUnset(t *testing.T) {
 		t, config.DefaultBindAddressForTest, cfg.BindAddress,
 	)
 	assert.Equal(t, bindAddressDefault, cfg.BindAddress)
+
+	// An absent SENTRY_DSN is the common case and must stay a normal
+	// start with error reporting off, not a refusal.
+	assert.Empty(t, cfg.SentryDSN)
+	assert.False(t, cfg.SentryEnabled())
 }

internal/config/export_test.go

@@ -51,6 +51,18 @@ func EnvPortForTest(key string, defaultValue int) (int, error) {
 	return envPort(key, defaultValue)
 }
 
+// EnvSentryDSNForTest exposes envSentryDSN.
+func EnvSentryDSNForTest(key string) (string, error) {
+	return envSentryDSN(key)
+}
+
+// LoadDotEnvFileForTest exposes the loader LoadDotEnv runs, over a
+// caller-named file rather than the process working directory, so
+// each .env state can be covered without moving the test process.
+func LoadDotEnvFileForTest(path string) error {
+	return loadDotEnvFile(path)
+}
+
 // EnvBindAddressForTest exposes envBindAddress.
 func EnvBindAddressForTest(key, defaultValue string) (string, error) {
 	return envBindAddress(key, defaultValue)

internal/config/sentry_test.go

@@ -0,0 +1,141 @@
+package config_test
+
+import (
+	"os"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"sneak.berlin/go/webhooker/internal/config"
+)
+
+// envKeySentryDSN is the variable envSentryDSN reads in production.
+const envKeySentryDSN = "SENTRY_DSN"
+
+// validSentryDSN is a syntactically complete DSN. The host is under
+// .invalid (RFC 2606), so nothing a test builds around it can reach a
+// real Sentry installation.
+const validSentryDSN = "https://abc123@sentry.invalid/42"
+
+// envSentryDSNCase is one row of the envSentryDSN table.
+type envSentryDSNCase struct {
+	name        string
+	set         bool
+	value       string
+	expectError bool
+	expected    string
+}
+
+// envSentryDSNCases is the envSentryDSN table. The three invalid
+// values are the ones measured on the defect: each initialised the SDK
+// with an error and left the process serving with reporting off.
+func envSentryDSNCases() []envSentryDSNCase {
+	return []envSentryDSNCase{
+		{
+			name:     "unset means reporting off",
+			expected: "",
+		},
+		{
+			name:     "empty means reporting off",
+			set:      true,
+			value:    "",
+			expected: "",
+		},
+		{
+			name:     "whitespace means reporting off",
+			set:      true,
+			value:    "   ",
+			expected: "",
+		},
+		{
+			name:     "a valid DSN is kept",
+			set:      true,
+			value:    validSentryDSN,
+			expected: validSentryDSN,
+		},
+		{
+			name:     "surrounding whitespace is trimmed",
+			set:      true,
+			value:    "  " + validSentryDSN + "\t",
+			expected: validSentryDSN,
+		},
+		{
+			name:        "a value that is not a URL is rejected",
+			set:         true,
+			value:       "not-a-dsn",
+			expectError: true,
+		},
+		{
+			name:        "an unparseable URL is rejected",
+			set:         true,
+			value:       "%%%",
+			expectError: true,
+		},
+		{
+			name:        "a DSN without a public key is rejected",
+			set:         true,
+			value:       "https://example.invalid/1",
+			expectError: true,
+		},
+		{
+			name:        "a DSN without a project id is rejected",
+			set:         true,
+			value:       "https://abc123@sentry.invalid/",
+			expectError: true,
+		},
+		{
+			name:        "a non-HTTP scheme is rejected",
+			set:         true,
+			value:       "ftp://abc123@sentry.invalid/42",
+			expectError: true,
+		},
+	}
+}
+
+// TestEnvSentryDSN covers the helper directly. What it pins beyond the
+// value is the failure shape: a set-but-unparseable DSN names the
+// variable and the value, exactly as the other fail-loud helpers do,
+// so an operator reads the fix off the message.
+func TestEnvSentryDSN(t *testing.T) {
+	for _, tt := range envSentryDSNCases() {
+		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(envKeySentryDSN, tt.value)
+			} else {
+				require.NoError(t, os.Unsetenv(envKeySentryDSN))
+			}
+
+			got, err := config.EnvSentryDSNForTest(envKeySentryDSN)
+
+			if tt.expectError {
+				require.Error(t, err)
+				require.ErrorIs(t, err, config.ErrInvalidSentryDSN)
+				assert.Contains(t, err.Error(), envKeySentryDSN)
+				assert.Contains(t, err.Error(), tt.value)
+				assert.Empty(t, got)
+
+				return
+			}
+
+			require.NoError(t, err)
+			assert.Equal(t, tt.expected, got)
+		})
+	}
+}
+
+// TestSentryEnabled_TracksTheDSN pins that the one method answering
+// "is anything being reported" agrees with the DSN in every state. The
+// startup log, the SDK initialisation and the sentryhttp middleware
+// all read it, so a log field cannot report reporting as on while
+// nothing is sending.
+func TestSentryEnabled_TracksTheDSN(t *testing.T) {
+	t.Parallel()
+
+	assert.False(t, (&config.Config{}).SentryEnabled())
+	assert.True(
+		t,
+		(&config.Config{SentryDSN: validSentryDSN}).SentryEnabled(),
+	)
+}