sneak/webhooker
1
0
Fork 0
Code Issues 62 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

Two config paths fail silently: an unparseable SENTRY_DSN starts anyway, and a malformed .env is discarded whole #283

New Issue
Closed
opened 2026-08-24 03:17:05 +02:00 by clawbot · 2 comments
clawbot commented 2026-08-24 03:17:05 +02:00
Collaborator
Copy Link

Found by a config-matrix audit that tested all 13 environment variables across absent / set-and-valid / set-but-invalid. Eleven variables and the METRICS_* pair all abort correctly. These two do not.

Both violate the same rule, stated by the owner on #78: "this should fail loudly and prevent server startup if it cant parse the duration string in the env var. your implementation silently fails. bad."

1. SENTRY_DSN — set but unparseable, starts anyway

SENTRY_DSN=not-a-dsn, %%%, and https://example.invalid/1 each log {"level":"ERROR","msg":"sentry init failure","error":"[Sentry] DsnParseError: invalid scheme"} and then keep running, serving traffic with error reporting silently off.

internal/server/server.go:161-164, with the intent stated in the code:

	if err != nil {
		s.log.Error("sentry init failure", "error", err)
		// Don't use fatal since we still want the service to run
		return
	}

The aggravating part: the startup summary logs "hasSentryDSN":true in exactly this case. The operator is told Sentry is configured while it is inert. That is the identical failure mode MetricsAuthEnabled() was written to eliminate — its doc comment at config.go:161-174 argues specifically that the log must not be able to disagree with reality. hasSentryDSN is that same bug, unfixed.

This is the "typo in the DSN and you lose all production error reporting, forever, silently" case. The variable exists solely to make failures visible.

2. A malformed .env is silently discarded in full

A .env containing bad syntax (PORT 19423, this is not = valid ! syntax, "unclosed) results in: server starts, every value in the file ignored, defaults applied, and not one line of log output mentioning the file.

github.com/joho/godotenv/autoload (imported at config.go:20) calls godotenv.Load() in its init() and discards the returned error, so config.go never sees it.

Blast radius is wider than #1: one fat-fingered line loses the ENTIRE file, reverting every variable in it to defaults, with no signal. That defeats the loud-failure guarantee for all 13 variables at once whenever .env is the delivery mechanism — and README line 73 tells developers to use .env.

Why one issue

Both are the same defect class in the same subsystem, and fixing them separately would put two workers in internal/config. One coherent change.

Definition of done

  • An unparseable SENTRY_DSN aborts startup, naming the variable, matching the existing envDuration/envPort pattern. Validate in loadFromEnv() rather than at Sentry-init time, so the failure happens where every other config failure happens.
  • enableSentry's error branch becomes a hard failure, and the comment justifying the current behaviour goes with it.
  • hasSentryDSN is replaced by something that cannot disagree with reality — follow MetricsAuthEnabled(), which already solved this exact problem.
  • A malformed .env fails loudly. This means dropping the autoload import for an explicit godotenv.Load() whose error is handled. Decide and state whether a MISSING .env remains fine (it must — it is optional) while a malformed one aborts.
  • README's "Invalid values abort startup" section currently asserts, without qualification, that a set-but-unparseable variable is fatal. That sentence is false today. Make the code true rather than weakening the sentence, and add SENTRY_DSN to the per-variable enumeration, which omits it entirely.
  • Document .env parse behaviour where .env is introduced.

Verification

  • SENTRY_DSN=not-a-dsn exits nonzero naming the variable. A valid DSN still starts and activates.
  • A malformed .env exits nonzero naming the file; a missing .env still starts; a valid .env still takes effect.
  • No log field can report a subsystem as configured when it is inert.
  • make check green.
Found by a config-matrix audit that tested all 13 environment variables across absent / set-and-valid / set-but-invalid. Eleven variables and the `METRICS_*` pair all abort correctly. These two do not. Both violate the same rule, stated by the owner on https://git.eeqj.de/sneak/webhooker/pulls/78: "this should fail loudly and prevent server startup if it cant parse the duration string in the env var. your implementation silently fails. bad." ## 1. `SENTRY_DSN` — set but unparseable, starts anyway `SENTRY_DSN=not-a-dsn`, `%%%`, and `https://example.invalid/1` each log `{"level":"ERROR","msg":"sentry init failure","error":"[Sentry] DsnParseError: invalid scheme"}` and then **keep running**, serving traffic with error reporting silently off. `internal/server/server.go:161-164`, with the intent stated in the code: ```go if err != nil { s.log.Error("sentry init failure", "error", err) // Don't use fatal since we still want the service to run return } ``` **The aggravating part:** the startup summary logs `"hasSentryDSN":true` in exactly this case. The operator is told Sentry is configured while it is inert. That is the identical failure mode `MetricsAuthEnabled()` was written to eliminate — its doc comment at `config.go:161-174` argues specifically that the log must not be able to disagree with reality. `hasSentryDSN` is that same bug, unfixed. This is the "typo in the DSN and you lose all production error reporting, forever, silently" case. The variable exists solely to make failures visible. ## 2. A malformed `.env` is silently discarded in full A `.env` containing bad syntax (`PORT 19423`, `this is not = valid ! syntax`, `"unclosed`) results in: server starts, every value in the file ignored, defaults applied, and **not one line of log output** mentioning the file. `github.com/joho/godotenv/autoload` (imported at `config.go:20`) calls `godotenv.Load()` in its `init()` and discards the returned error, so `config.go` never sees it. Blast radius is wider than #1: one fat-fingered line loses the ENTIRE file, reverting every variable in it to defaults, with no signal. That defeats the loud-failure guarantee for all 13 variables at once whenever `.env` is the delivery mechanism — and README line 73 tells developers to use `.env`. ## Why one issue Both are the same defect class in the same subsystem, and fixing them separately would put two workers in `internal/config`. One coherent change. ## Definition of done - An unparseable `SENTRY_DSN` aborts startup, naming the variable, matching the existing `envDuration`/`envPort` pattern. Validate in `loadFromEnv()` rather than at Sentry-init time, so the failure happens where every other config failure happens. - `enableSentry`'s error branch becomes a hard failure, and the comment justifying the current behaviour goes with it. - `hasSentryDSN` is replaced by something that cannot disagree with reality — follow `MetricsAuthEnabled()`, which already solved this exact problem. - A malformed `.env` fails loudly. This means dropping the `autoload` import for an explicit `godotenv.Load()` whose error is handled. Decide and state whether a MISSING `.env` remains fine (it must — it is optional) while a malformed one aborts. - README's "Invalid values abort startup" section currently asserts, without qualification, that a set-but-unparseable variable is fatal. That sentence is false today. Make the code true rather than weakening the sentence, and add `SENTRY_DSN` to the per-variable enumeration, which omits it entirely. - Document `.env` parse behaviour where `.env` is introduced. ## Verification - `SENTRY_DSN=not-a-dsn` exits nonzero naming the variable. A valid DSN still starts and activates. - A malformed `.env` exits nonzero naming the file; a missing `.env` still starts; a valid `.env` still takes effect. - No log field can report a subsystem as configured when it is inert. - `make check` green.
clawbot added this to the 1.0.0 milestone 2026-08-24 03:17:08 +02:00
clawbot commented 2026-08-24 03:54:42 +02:00
Author
Collaborator
Copy Link

Both reproduced on unmodified next (8d64259) before touching anything: SENTRY_DSN=not-a-dsn logged sentry init failure plus "hasSentryDSN":true and served until killed; a .env of PORT 19611 / this is not = valid ! syntax / "unclosed started on the default port 8080 with no log line naming the file.

Plan:

  1. SENTRY_DSN validated in loadFromEnv() via a new envSentryDSN helper shaped like envPort/envBindAddress, wrapping a new ErrInvalidSentryDSN. It parses with sentry.NewDsn, which is literally the call sentry.Init makes on the DSN — so the config-time check and the init-time check cannot disagree. Cost: internal/config imports github.com/getsentry/sentry-go. Choosing that over a hand-rolled syntactic check because a syntactic check is a second, drifting definition of "valid DSN", and the SDK is already a module dependency already linked into this binary, so it adds nothing to the build. Unset stays unset — Sentry off, normal start.

  2. hasSentryDSN replaced by Config.SentryEnabled(), following MetricsAuthEnabled(): one method, read by both the startup log field (now sentryEnabled) and by enableSentry, so the log cannot say configured while the subsystem is inert.

  3. enableSentry's error branch becomes fatal, comment and all. It runs on the serving goroutine after fx has already reported RUNNING, so "fatal" there means the same thing a listen failure already means in this file: Shutdowner.Shutdown(fx.ExitCode(1)) — non-zero exit through fx's normal stop sequence, not a panic and not a bare os.Exit. I will factor the existing shutdownOnListenFailure body into a shared helper both call.

  4. .env: drop the godotenv/autoload blank import for an exported config.LoadDotEnv(). Missing file (fs.ErrNotExist) returns nil — optional, and the common case. Anything else — parse error, unreadable, a directory — is a wrapped error naming .env.

Scope disclosure on (4): this needs a few lines in cmd/webhooker/main.go as well, calling LoadDotEnv() at the top of dispatch(). autoload ran in init(), i.e. before run() calls config.DataDir() to take the DATA_DIR lock and before resetpw calls it. Loading only inside loadFromEnv() would move that after the lock, so a .env setting DATA_DIR would lock one directory while the config opened databases in another, and resetpw would never see the file at all. dispatch() is the one point that precedes every reader of the environment on both subcommand paths.

Verification will cover: not-a-dsn, %%%, https://example.invalid/1 each exiting non-zero naming SENTRY_DSN; a valid DSN starting and activating; absent starting with Sentry off; malformed .env exiting non-zero naming the file; missing .env starting; a valid .env value provably reaching the config; and sentryEnabled correct in all three states.

Both reproduced on unmodified `next` (8d64259) before touching anything: `SENTRY_DSN=not-a-dsn` logged `sentry init failure` plus `"hasSentryDSN":true` and served until killed; a `.env` of `PORT 19611` / `this is not = valid ! syntax` / `"unclosed` started on the default port 8080 with no log line naming the file. Plan: 1. **`SENTRY_DSN` validated in `loadFromEnv()`** via a new `envSentryDSN` helper shaped like `envPort`/`envBindAddress`, wrapping a new `ErrInvalidSentryDSN`. It parses with `sentry.NewDsn`, which is literally the call `sentry.Init` makes on the DSN — so the config-time check and the init-time check cannot disagree. Cost: `internal/config` imports `github.com/getsentry/sentry-go`. Choosing that over a hand-rolled syntactic check because a syntactic check is a second, drifting definition of "valid DSN", and the SDK is already a module dependency already linked into this binary, so it adds nothing to the build. Unset stays unset — Sentry off, normal start. 2. **`hasSentryDSN` replaced by `Config.SentryEnabled()`**, following `MetricsAuthEnabled()`: one method, read by both the startup log field (now `sentryEnabled`) and by `enableSentry`, so the log cannot say configured while the subsystem is inert. 3. **`enableSentry`'s error branch becomes fatal**, comment and all. It runs on the serving goroutine after fx has already reported RUNNING, so "fatal" there means the same thing a listen failure already means in this file: `Shutdowner.Shutdown(fx.ExitCode(1))` — non-zero exit through fx's normal stop sequence, not a panic and not a bare `os.Exit`. I will factor the existing `shutdownOnListenFailure` body into a shared helper both call. 4. **`.env`**: drop the `godotenv/autoload` blank import for an exported `config.LoadDotEnv()`. Missing file (`fs.ErrNotExist`) returns nil — optional, and the common case. Anything else — parse error, unreadable, a directory — is a wrapped error naming `.env`. **Scope disclosure on (4):** this needs a few lines in `cmd/webhooker/main.go` as well, calling `LoadDotEnv()` at the top of `dispatch()`. `autoload` ran in `init()`, i.e. before `run()` calls `config.DataDir()` to take the `DATA_DIR` lock and before `resetpw` calls it. Loading only inside `loadFromEnv()` would move that after the lock, so a `.env` setting `DATA_DIR` would lock one directory while the config opened databases in another, and `resetpw` would never see the file at all. `dispatch()` is the one point that precedes every reader of the environment on both subcommand paths. Verification will cover: `not-a-dsn`, `%%%`, `https://example.invalid/1` each exiting non-zero naming `SENTRY_DSN`; a valid DSN starting and activating; absent starting with Sentry off; malformed `.env` exiting non-zero naming the file; missing `.env` starting; a valid `.env` value provably reaching the config; and `sentryEnabled` correct in all three states.
clawbot commented 2026-08-24 04:18:20 +02:00
Author
Collaborator
Copy Link

Built as #289, branch issue-283-fail-loud-config, base next.

Both defects fixed as planned. Two things worth reading in the PR rather than repeating here: the Sentry SDK import into internal/config (taken deliberately, reasoning stated), and the cmd/webhooker/main.go change, which was outside my fence but is what keeps .env ahead of the DATA_DIR lock.

Verified against a built binary, all eight states: not-a-dsn, %%% and https://example.invalid/1 each exit 1 naming SENTRY_DSN and quoting the value; a valid DSN starts with "sentryEnabled":true and sentry error reporting activated; an absent one starts with "sentryEnabled":false. In the rejected state no sentryEnabled field is emitted at all — the process never reaches the startup summary — so no field can report a subsystem as configured while it is inert. A malformed .env exits 1 with one stderr line naming the file, before any log output and before the lock; a missing one starts; a valid one takes effect, shown by a DATA_DIR set only in .env producing webhooker.lock and webhooker.db in that directory, which also proves the load still precedes the lock. resetpw is refused on the same terms.

Both defects were reproduced on unmodified next first, and make check is green with GOFLAGS=-count=1 after rebasing onto current next (48cf93e), with lint running uncached in Docker.

Built as https://git.eeqj.de/sneak/webhooker/pulls/289, branch `issue-283-fail-loud-config`, base `next`. Both defects fixed as planned. Two things worth reading in the PR rather than repeating here: the Sentry SDK import into `internal/config` (taken deliberately, reasoning stated), and the `cmd/webhooker/main.go` change, which was outside my fence but is what keeps `.env` ahead of the `DATA_DIR` lock. Verified against a built binary, all eight states: `not-a-dsn`, `%%%` and `https://example.invalid/1` each exit 1 naming `SENTRY_DSN` and quoting the value; a valid DSN starts with `"sentryEnabled":true` and `sentry error reporting activated`; an absent one starts with `"sentryEnabled":false`. In the rejected state no `sentryEnabled` field is emitted at all — the process never reaches the startup summary — so no field can report a subsystem as configured while it is inert. A malformed `.env` exits 1 with one stderr line naming the file, before any log output and before the lock; a missing one starts; a valid one takes effect, shown by a `DATA_DIR` set only in `.env` producing `webhooker.lock` and `webhooker.db` in that directory, which also proves the load still precedes the lock. `resetpw` is refused on the same terms. Both defects were reproduced on unmodified `next` first, and `make check` is green with `GOFLAGS=-count=1` after rebasing onto current `next` (48cf93e), with lint running uncached in Docker.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
needs-checks
needs-rebase
needs-review
needs-rework
No Label
Milestone
No items
No Milestone 1.0.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/webhooker#283
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?