CHANGELOG.md

@@ -5,6 +5,11 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
 and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).
 
+## [Unreleased]
+
+### Fixed
+- **Runtime ENV handling** – Empty or invalid environment variables (e.g. `SMTP_PORT=`, `PORT=`, `POOL_SIZE=`, `DATABASE_PORT=`) no longer cause `ArgumentError` at boot. Instead raises clear errors for required vars set but empty (e.g. DATABASE_HOST, PHX_HOST/DOMAIN, SECRET_KEY_BASE).
+
 ## [1.1.1] - 2026-03-16
 
 ### Added

CODE_GUIDELINES.md

@@ -1286,7 +1286,7 @@ mix hex.outdated
 - `SMTP_SSL` values: `tls` (default, port 587), `ssl` (port 465), `none` (port 25).
 - When `SMTP_HOST` ENV is present at boot, `runtime.exs` configures `Swoosh.Adapters.SMTP` automatically.
 - When SMTP is configured only via Settings, `Mv.Mailer.smtp_config/0` builds the adapter config per-send.
-- In test environment, `Swoosh.Adapters.Test` is used regardless of SMTP config.
+- In test environment, `Swoosh.Adapters.Test` is used regardless of SMTP config. `smtp_config/0` returns `[]` when the mailer adapter is `Swoosh.Adapters.Test`, so per-send SMTP opts never bypass the test mailbox. Port 587/465 sockopts are unit-tested on `Mv.Smtp.ConfigBuilder.build_opts/1` (`test/mv/smtp/config_builder_test.exs`); `test/mv/mailer_smtp_config_test.exs` covers the Test-adapter guard and temporarily sets the adapter to `Swoosh.Adapters.Local` to assert `smtp_config/0` wiring from ENV. Use `Mv.DataCase` for those tests (not plain `ExUnit.Case`) because `smtp_config/0` pulls `Mv.Config` fields that may read Settings from the DB when SMTP user/password ENV vars are unset.
 - **TLS in OTP 27:** Verify mode defaults to `verify_none` for self-signed/internal certs. Set `SMTP_VERIFY_PEER=true` (or `1`/`yes`) in prod when using public SMTP (Gmail, Mailgun). Config key `:smtp_verify_peer` is set in `runtime.exs` and read by `Mv.Mailer.smtp_config/0`.
 - **Test email:** `Mv.Mailer.send_test_email(to_email)` sends a transactional test email; returns `{:ok, email}` or `{:error, classified_reason}`. Classified errors: `:sender_rejected`, `:auth_failed`, `:recipient_rejected`, `:tls_failed`, `:connection_failed`, `{:smtp_error, message}`. Each shows a specific message in the UI.
 - **Production warning:** When SMTP is not configured in production, a warning is shown in the Settings UI. Use `Application.get_env(:mv, :environment, :dev)` (or assign in mount) for environment checks in LiveView/templates; do not use `Mix.env()` at runtime (it is not available in releases).
@@ -1712,6 +1712,8 @@ mix test test/membership/member_test.exs:42
 
 ### 4.7 Testing Best Practices
 
+**Process environment (`test/test_helper.exs`):** Vereinfacht and OIDC-related `System.get_env/1` keys are cleared at test startup so configuration comes from the test database (Membership settings) unless a test explicitly sets variables in `setup` and restores them with `on_exit`. This matches production priority (ENV over settings) while keeping the suite deterministic when `.env` is loaded (e.g. via `just`).
+
 **Testing Philosophy: Focus on Business Logic, Not Framework Functionality**
 
 We test our business logic and domain-specific behavior, not core framework features. Framework features (Ash validations, Ecto relationships, etc.) are already tested by their respective libraries.
@@ -2177,6 +2179,14 @@ mix phx.gen.secret
 mix phx.gen.secret
 ```
 
+**Runtime configuration (config/runtime.exs):**
+
+- Production config is loaded from `config/runtime.exs` at boot (releases and `mix phx.server`). Environment variables are read via helpers so that **empty or invalid values do not cause cryptic crashes** (e.g. `ArgumentError` from `String.to_integer("")`).
+- **Helpers used:** `get_env_or_file` / `get_env_or_file!` (with `_FILE` support); `get_env_required` (required vars: raises if missing or empty after trim); `get_env_non_empty` (optional string: empty treated as unset, returns default); `parse_positive_integer` (PORT, POOL_SIZE, SMTP_PORT: empty or invalid → default).
+- **Required vars** (e.g. DATABASE_HOST, PHX_HOST/DOMAIN, SECRET_KEY_BASE): if set but empty, the app raises at boot with a clear message including “(Variable X is set but empty.)”.
+- **Optional numeric vars** (PORT, POOL_SIZE, SMTP_PORT, DATABASE_PORT): empty or invalid value is treated as “unset” and the documented default is used (e.g. PORT=4000, SMTP_PORT=587).
+- When adding new ENV in `runtime.exs`, use these helpers instead of raw `System.get_env(...)` and `String.to_integer(...)` so that misconfigured or empty variables fail fast with clear errors.
+
 ### 5.6 Security Headers
 
 **Configure Security Headers:**

config/runtime.exs

@@ -32,11 +32,91 @@ get_env_or_file = fn var_name, default ->
   end
 end
 
-# Same as get_env_or_file but raises if the value is not set
+# Same as get_env_or_file but raises if the value is not set or empty (after trim).
+# Empty values lead to unclear runtime errors; failing at boot with a clear message is preferred.
 get_env_or_file! = fn var_name, error_message ->
   case get_env_or_file.(var_name, nil) do
-    nil -> raise error_message
-    value -> value
+    nil ->
+      raise error_message
+
+    value when is_binary(value) ->
+      trimmed = String.trim(value)
+
+      if trimmed == "" do
+        raise """
+        #{error_message}
+        (Variable #{var_name} or #{var_name}_FILE is set but the value is empty.)
+        """
+      else
+        trimmed
+      end
+
+    value ->
+      value
+  end
+end
+
+# Returns default when env_value is nil, empty after trim, or not a valid positive integer.
+# Used for PORT, POOL_SIZE, SMTP_PORT to avoid ArgumentError on empty or invalid values.
+parse_positive_integer = fn env_value, default ->
+  case env_value do
+    nil ->
+      default
+
+    v when is_binary(v) ->
+      case String.trim(v) do
+        "" ->
+          default
+
+        trimmed ->
+          case Integer.parse(trimmed) do
+            {n, _} when n > 0 -> n
+            _ -> default
+          end
+      end
+
+    _ ->
+      default
+  end
+end
+
+# Returns default when the key is missing or the value is empty (after trim).
+# Use for optional string ENV vars (e.g. DATABASE_PORT) so empty string is treated as "unset".
+get_env_non_empty = fn key, default ->
+  case System.get_env(key) do
+    nil ->
+      default
+
+    v when is_binary(v) ->
+      trimmed = String.trim(v)
+      if trimmed == "", do: default, else: trimmed
+
+    v ->
+      v
+  end
+end
+
+# Returns the trimmed value when set and non-empty; otherwise raises with error_message.
+# Use for required vars (DATABASE_HOST, etc.) so "set but empty" fails at boot with a clear message.
+get_env_required = fn key, error_message ->
+  case System.get_env(key) do
+    nil ->
+      raise error_message
+
+    v when is_binary(v) ->
+      trimmed = String.trim(v)
+
+      if trimmed == "" do
+        raise """
+        #{error_message}
+        (Variable #{key} is set but empty.)
+        """
+      else
+        trimmed
+      end
+
+    v ->
+      v
   end
 end
 
@@ -49,12 +129,14 @@ build_database_url = fn ->
     nil ->
       # Build URL from separate components
       host =
-        System.get_env("DATABASE_HOST") ||
-          raise "DATABASE_HOST is required when DATABASE_URL is not set"
+        get_env_required.("DATABASE_HOST", """
+        DATABASE_HOST is required when DATABASE_URL is not set.
+        """)
 
       user =
-        System.get_env("DATABASE_USER") ||
-          raise "DATABASE_USER is required when DATABASE_URL is not set"
+        get_env_required.("DATABASE_USER", """
+        DATABASE_USER is required when DATABASE_URL is not set.
+        """)
 
       password =
         get_env_or_file!.("DATABASE_PASSWORD", """
@@ -62,10 +144,11 @@ build_database_url = fn ->
         """)
 
       database =
-        System.get_env("DATABASE_NAME") ||
-          raise "DATABASE_NAME is required when DATABASE_URL is not set"
+        get_env_required.("DATABASE_NAME", """
+        DATABASE_NAME is required when DATABASE_URL is not set.
+        """)
 
-      port = System.get_env("DATABASE_PORT", "5432")
+      port = get_env_non_empty.("DATABASE_PORT", "5432")
 
       # URL-encode the password to handle special characters
       encoded_password = URI.encode_www_form(password)
@@ -102,7 +185,7 @@ if config_env() == :prod do
   config :mv, Mv.Repo,
     # ssl: true,
     url: database_url,
-    pool_size: String.to_integer(System.get_env("POOL_SIZE") || "10"),
+    pool_size: parse_positive_integer.(System.get_env("POOL_SIZE"), 10),
     socket_options: maybe_ipv6
 
   # The secret key base is used to sign/encrypt cookies and other secrets.
@@ -120,11 +203,14 @@ if config_env() == :prod do
   # PHX_HOST or DOMAIN can be used to set the host for the application.
   # DOMAIN is commonly used in deployment environments (e.g., Portainer templates).
   host =
-    System.get_env("PHX_HOST") ||
-      System.get_env("DOMAIN") ||
-      raise "Please define the PHX_HOST or DOMAIN environment variable."
+    get_env_non_empty.("PHX_HOST", nil) ||
+      get_env_non_empty.("DOMAIN", nil) ||
+      raise """
+      Please define the PHX_HOST or DOMAIN environment variable.
+      (Variable may be set but empty.)
+      """
 
-  port = String.to_integer(System.get_env("PORT") || "4000")
+  port = parse_positive_integer.(System.get_env("PORT"), 4000)
 
   config :mv, :dns_cluster_query, System.get_env("DNS_CLUSTER_QUERY")
 
@@ -230,11 +316,7 @@ if config_env() == :prod do
   smtp_host_env = System.get_env("SMTP_HOST")
 
   if smtp_host_env && String.trim(smtp_host_env) != "" do
-    smtp_port_env =
-      case System.get_env("SMTP_PORT") do
-        nil -> 587
-        v -> String.to_integer(String.trim(v))
-      end
+    smtp_port_env = parse_positive_integer.(System.get_env("SMTP_PORT"), 587)
 
     smtp_password_env =
       case System.get_env("SMTP_PASSWORD") do

docs/development-progress-log.md

@@ -710,6 +710,10 @@ end
 
 ## Testing Strategy
 
+### Test process environment
+
+`test/test_helper.exs` clears Vereinfacht and OIDC-related environment variables at startup (same rationale as not hitting real APIs when `.env` is loaded). `Mv.Config` prefers ENV over database settings; without this, OIDC sign-in redirect tests would depend on the developer shell and become flaky. Tests that need specific OIDC env values set them in `setup` and restore with `on_exit`.
+
 ### Test Coverage Areas
 
 #### 1. Unit Tests (Domain Logic)

docs/smtp-configuration-concept.md

@@ -42,6 +42,8 @@ When an ENV variable is set, the corresponding Settings field is read-only in th
 | Sender name    | `MAIL_FROM_NAME`       | `smtp_from_name`    | Display name in "From" header (default: Mila)|
 | Sender email   | `MAIL_FROM_EMAIL`      | `smtp_from_email`   | Address in "From" header; must match SMTP user on most servers |
 
+**Boot-time ENV handling:** In `config/runtime.exs`, if `SMTP_PORT` is set but empty or invalid, it is treated as unset and default 587 is used. This avoids startup crashes (e.g. `ArgumentError` from `String.to_integer("")`) when variables are misconfigured in deployment.
+
 **Important:** On most SMTP servers (e.g. Postfix with strict relay policies) the sender email (`smtp_from_email`) must be the same address as `smtp_username` or an alias that is owned by that account.
 
 **Settings UI:** The form uses three rows on wide viewports: host, port, TLS/SSL | username, password | sender email, sender name. Content width is limited by the global settings wrapper (see `DESIGN_GUIDELINES.md` §6.4).
@@ -107,6 +109,8 @@ By default, TLS certificate verification is relaxed (`verify_none`) so self-sign
 
 Verify mode is set in `tls_options` for port 587 (STARTTLS). For port 465 (implicit SSL), the initial connection is `ssl:connect`, so we also pass `sockopts: [verify: verify_mode]` so the SSL handshake uses the same mode. For 587 we must not pass `verify` in sockopts—gen_tcp is used first and rejects it (ArgumentError). The logic lives in `Mv.Smtp.ConfigBuilder.build_opts/1` (single source of truth), used by `config/runtime.exs` (boot) and `Mv.Mailer.smtp_config/0` (Settings-only).
 
+**Tests:** `Mv.Smtp.ConfigBuilderTest` asserts sockopts/TLS shape. `Mv.Mailer.smtp_config/0` returns `[]` when the mailer adapter is `Swoosh.Adapters.Test`; `test/mv/mailer_smtp_config_test.exs` asserts that guard and, with the adapter temporarily set to `Swoosh.Adapters.Local`, wiring from ENV. Those mailer tests use `Mv.DataCase` so Settings fallbacks in `Mv.Config` (e.g. SMTP username/password when ENV is unset) stay under the SQL sandbox.
+
 ---
 
 ## 12. Summary Checklist

lib/mv/mailer.ex

@@ -100,7 +100,11 @@ defmodule Mv.Mailer do
   """
   @spec smtp_config() :: keyword()
   def smtp_config do
-    if Mv.Config.smtp_configured?() and not boot_smtp_configured?() do
+    # In test we use Swoosh.Adapters.Test; do not override with SMTP opts or emails would not land in the test mailbox.
+    adapter = Application.get_env(:mv, __MODULE__, []) |> Keyword.get(:adapter)
+
+    if Mv.Config.smtp_configured?() and not boot_smtp_configured?() and
+         adapter != Swoosh.Adapters.Test do
       verify_mode =
         if Application.get_env(:mv, :smtp_verify_peer, false),
           do: :verify_peer,

test/mv/mailer_smtp_config_test.exs

@@ -1,18 +1,22 @@
 defmodule Mv.MailerSmtpConfigTest do
   @moduledoc """
-  Unit tests for Mv.Mailer.smtp_config/0.
+  Integration-style tests for `Mv.Mailer.smtp_config/0`.
 
-  Ensures both port 587 (STARTTLS) and 465 (implicit SSL) work:
-  - 587: sockopts must NOT contain :verify (gen_tcp:connect would raise ArgumentError).
-  - 465: sockopts MUST contain :verify so initial ssl:connect uses verify_none/verify_peer.
-  Uses ENV to drive config; async: false.
+  With the default test mailer adapter (`Swoosh.Adapters.Test`), `smtp_config/0`
+  must return `[]` so per-send SMTP opts never bypass the test mailbox.
+
+  Port 587 vs 465 / `:verify` in `sockopts` are covered by
+  `Mv.Smtp.ConfigBuilderTest`; here we assert `smtp_config/0` wiring from
+  `Mv.Config` when the mailer adapter is temporarily set to
+  `Swoosh.Adapters.Local` (`async: false`, global Application env).
+
+  Uses `Mv.DataCase` so `Mv.Config.smtp_username/0` and `smtp_password/0` (Settings
+  fallback when ENV is unset) run under the SQL sandbox like the rest of the suite.
   """
   use Mv.DataCase, async: false
 
   alias Mv.Mailer
 
-  defp set_smtp_env(key, value), do: System.put_env(key, value)
-
   defp clear_smtp_env do
     System.delete_env("SMTP_HOST")
     System.delete_env("SMTP_PORT")
@@ -22,31 +26,64 @@ defmodule Mv.MailerSmtpConfigTest do
     System.delete_env("SMTP_SSL")
   end
 
-  describe "smtp_config/0" do
+  describe "smtp_config/0 with Swoosh.Adapters.Test" do
+    setup do
+      previous = Application.get_env(:mv, Mv.Mailer)
+      Application.put_env(:mv, Mv.Mailer, adapter: Swoosh.Adapters.Test)
+
+      on_exit(fn ->
+        Application.put_env(:mv, Mv.Mailer, previous)
+      end)
+
+      :ok
+    end
+
+    test "returns empty list when SMTP_* ENV is set so the test adapter is not overridden" do
+      System.put_env("SMTP_HOST", "smtp.example.com")
+      System.put_env("SMTP_PORT", "587")
+      System.put_env("SMTP_SSL", "tls")
+      on_exit(fn -> clear_smtp_env() end)
+
+      assert Mailer.smtp_config() == []
+    end
+  end
+
+  describe "smtp_config/0 with a non-Test mailer adapter" do
+    setup do
+      previous = Application.get_env(:mv, Mv.Mailer)
+      Application.put_env(:mv, Mv.Mailer, adapter: Swoosh.Adapters.Local)
+
+      on_exit(fn ->
+        Application.put_env(:mv, Mv.Mailer, previous)
+      end)
+
+      :ok
+    end
+
     test "port 587 (TLS): does not include :verify in sockopts so gen_tcp:connect does not crash" do
-      set_smtp_env("SMTP_HOST", "smtp.example.com")
-      set_smtp_env("SMTP_PORT", "587")
-      set_smtp_env("SMTP_SSL", "tls")
+      System.put_env("SMTP_HOST", "smtp.example.com")
+      System.put_env("SMTP_PORT", "587")
+      System.put_env("SMTP_SSL", "tls")
+      on_exit(fn -> clear_smtp_env() end)
 
       config = Mailer.smtp_config()
 
-      assert config != [], "expected non-empty config when SMTP_HOST is set"
+      assert config != [],
+             "expected non-empty config when SMTP_HOST is set and adapter is not Test"
 
       sockopts = Keyword.get(config, :sockopts, [])
 
       refute Keyword.has_key?(sockopts, :verify),
              "for 587 gen_tcp is used first; sockopts must not contain :verify"
-    after
-      clear_smtp_env()
     end
 
     test "port 465 (SSL): includes :verify in sockopts so initial ssl:connect accepts verify mode" do
-      set_smtp_env("SMTP_HOST", "smtp.example.com")
-      set_smtp_env("SMTP_PORT", "465")
-      set_smtp_env("SMTP_SSL", "ssl")
+      System.put_env("SMTP_HOST", "smtp.example.com")
+      System.put_env("SMTP_PORT", "465")
+      System.put_env("SMTP_SSL", "ssl")
+      on_exit(fn -> clear_smtp_env() end)
 
       config = Mailer.smtp_config()
-
       assert config != []
       sockopts = Keyword.get(config, :sockopts, [])
 
@@ -54,36 +91,32 @@ defmodule Mv.MailerSmtpConfigTest do
              "for 465 initial connection is ssl:connect; sockopts must contain :verify"
 
       assert Keyword.get(sockopts, :verify) in [:verify_none, :verify_peer]
-    after
-      clear_smtp_env()
     end
 
     test "builds TLS mode for port 587 (STARTTLS)" do
-      set_smtp_env("SMTP_HOST", "smtp.example.com")
-      set_smtp_env("SMTP_PORT", "587")
-      set_smtp_env("SMTP_SSL", "tls")
+      System.put_env("SMTP_HOST", "smtp.example.com")
+      System.put_env("SMTP_PORT", "587")
+      System.put_env("SMTP_SSL", "tls")
+      on_exit(fn -> clear_smtp_env() end)
 
       config = Mailer.smtp_config()
 
       assert config != []
       assert Keyword.get(config, :tls) == :always
       assert Keyword.get(config, :ssl) == false
-    after
-      clear_smtp_env()
     end
 
     test "builds SSL mode for port 465 (implicit SSL)" do
-      set_smtp_env("SMTP_HOST", "smtp.example.com")
-      set_smtp_env("SMTP_PORT", "465")
-      set_smtp_env("SMTP_SSL", "ssl")
+      System.put_env("SMTP_HOST", "smtp.example.com")
+      System.put_env("SMTP_PORT", "465")
+      System.put_env("SMTP_SSL", "ssl")
+      on_exit(fn -> clear_smtp_env() end)
 
       config = Mailer.smtp_config()
 
       assert config != []
       assert Keyword.get(config, :ssl) == true
       assert Keyword.get(config, :tls) == :never
-    after
-      clear_smtp_env()
     end
   end
 end

test/mv_web/controllers/auth_controller_test.exs

@@ -333,9 +333,11 @@ defmodule MvWeb.AuthControllerTest do
 
         # When OIDC-only is enabled, password sign-in must not succeed (no redirect to sign_in_with_token).
         case result do
-          {:error, {:redirect, %{to: to}}} ->
-            refute to =~ "sign_in_with_token",
-                   "Expected password sign-in to be rejected when OIDC-only, got redirect to: #{to}"
+          {:error, {:redirect, opts}} when is_map(opts) ->
+            to_path = Map.get(opts, :to) || Map.get(opts, "to") || ""
+
+            refute to_path =~ "sign_in_with_token",
+                   "Expected password sign-in to be rejected when OIDC-only, got redirect to: #{to_path}"
 
           _ ->
             # LiveView re-rendered (e.g. with flash error) instead of redirecting to success
@@ -426,6 +428,7 @@ defmodule MvWeb.AuthControllerTest do
         oidc_client_secret: settings.oidc_client_secret
       }
 
+      # Set OIDC-only but leave OIDC unconfigured so the plug does not redirect.
       {:ok, _} =
         Membership.update_settings(settings, %{
           oidc_only: true,

test/test_helper.exs

@@ -4,6 +4,13 @@ System.delete_env("VEREINFACHT_API_URL")
 System.delete_env("VEREINFACHT_API_KEY")
 System.delete_env("VEREINFACHT_CLUB_ID")
 
+# Mv.Config.oidc_* reads ENV before database Settings (see lib/mv/config.ex). If a developer
+# or CI loads .env with OIDC variables, OidcOnlySignInRedirect and oidc_configured?/0 would
+# ignore Membership settings and tests become flaky. Tests that need OIDC env (e.g. group
+# claim) set it in setup and restore via on_exit.
+~w(OIDC_CLIENT_ID OIDC_BASE_URL OIDC_REDIRECT_URI OIDC_CLIENT_SECRET OIDC_CLIENT_SECRET_FILE OIDC_ONLY OIDC_ADMIN_GROUP_NAME OIDC_GROUPS_CLAIM)
+|> Enum.each(&System.delete_env/1)
+
 ExUnit.start(
   # shows 10 slowest tests at the end of the test run
   # slowest: 10