From 61952f986d02eef2dd5e489ce7e787d5850b0768 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 23 Mar 2026 12:55:05 +0100 Subject: [PATCH] feat: provide clear errors for missing required envs --- CHANGELOG.md | 5 + CODE_GUIDELINES.md | 8 ++ config/runtime.exs | 122 +++++++++++++++--- docs/smtp-configuration-concept.md | 2 + lib/mv/mailer.ex | 6 +- .../controllers/auth_controller_test.exs | 30 ++++- 6 files changed, 146 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b94ce50..cd65692 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index f84c5ad..efa1678 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -2177,6 +2177,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:** diff --git a/config/runtime.exs b/config/runtime.exs index 6a434fa..d5ba574 100644 --- a/config/runtime.exs +++ b/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 diff --git a/docs/smtp-configuration-concept.md b/docs/smtp-configuration-concept.md index 13b0d17..3cc01df 100644 --- a/docs/smtp-configuration-concept.md +++ b/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). diff --git a/lib/mv/mailer.ex b/lib/mv/mailer.ex index 41a77cd..ec8f357 100644 --- a/lib/mv/mailer.ex +++ b/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, diff --git a/test/mv_web/controllers/auth_controller_test.exs b/test/mv_web/controllers/auth_controller_test.exs index f1fbd76..98f6f23 100644 --- a/test/mv_web/controllers/auth_controller_test.exs +++ b/test/mv_web/controllers/auth_controller_test.exs @@ -316,9 +316,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 @@ -398,8 +400,24 @@ defmodule MvWeb.AuthControllerTest do test "returns 200 when OIDC-only but OIDC not configured", %{conn: authenticated_conn} do {:ok, settings} = Membership.get_settings() - original_oidc_only = Map.get(settings, :oidc_only, false) - {:ok, _} = Membership.update_settings(settings, %{oidc_only: true}) + + prev = %{ + oidc_only: settings.oidc_only, + oidc_client_id: settings.oidc_client_id, + oidc_base_url: settings.oidc_base_url, + oidc_redirect_uri: settings.oidc_redirect_uri, + 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, + oidc_client_id: nil, + oidc_base_url: nil, + oidc_redirect_uri: nil, + oidc_client_secret: nil + }) try do conn = build_unauthenticated_conn(authenticated_conn) @@ -407,7 +425,7 @@ defmodule MvWeb.AuthControllerTest do assert conn.status == 200 after {:ok, s} = Membership.get_settings() - Membership.update_settings(s, %{oidc_only: original_oidc_only}) + Membership.update_settings(s, prev) end end