diff --git a/CHANGELOG.md b/CHANGELOG.md index cd65692..b94ce50 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,11 +5,6 @@ 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 efa1678..f84c5ad 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -2177,14 +2177,6 @@ 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 d5ba574..6a434fa 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -32,91 +32,11 @@ get_env_or_file = fn var_name, default -> end end -# 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. +# Same as get_env_or_file but raises if the value is not set get_env_or_file! = fn var_name, error_message -> case get_env_or_file.(var_name, nil) do - 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 + nil -> raise error_message + value -> value end end @@ -129,14 +49,12 @@ build_database_url = fn -> nil -> # Build URL from separate components host = - get_env_required.("DATABASE_HOST", """ - DATABASE_HOST is required when DATABASE_URL is not set. - """) + System.get_env("DATABASE_HOST") || + raise "DATABASE_HOST is required when DATABASE_URL is not set" user = - get_env_required.("DATABASE_USER", """ - DATABASE_USER is required when DATABASE_URL is not set. - """) + System.get_env("DATABASE_USER") || + raise "DATABASE_USER is required when DATABASE_URL is not set" password = get_env_or_file!.("DATABASE_PASSWORD", """ @@ -144,11 +62,10 @@ build_database_url = fn -> """) database = - get_env_required.("DATABASE_NAME", """ - DATABASE_NAME is required when DATABASE_URL is not set. - """) + System.get_env("DATABASE_NAME") || + raise "DATABASE_NAME is required when DATABASE_URL is not set" - port = get_env_non_empty.("DATABASE_PORT", "5432") + port = System.get_env("DATABASE_PORT", "5432") # URL-encode the password to handle special characters encoded_password = URI.encode_www_form(password) @@ -185,7 +102,7 @@ if config_env() == :prod do config :mv, Mv.Repo, # ssl: true, url: database_url, - pool_size: parse_positive_integer.(System.get_env("POOL_SIZE"), 10), + pool_size: String.to_integer(System.get_env("POOL_SIZE") || "10"), socket_options: maybe_ipv6 # The secret key base is used to sign/encrypt cookies and other secrets. @@ -203,14 +120,11 @@ 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 = - 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.) - """ + System.get_env("PHX_HOST") || + System.get_env("DOMAIN") || + raise "Please define the PHX_HOST or DOMAIN environment variable." - port = parse_positive_integer.(System.get_env("PORT"), 4000) + port = String.to_integer(System.get_env("PORT") || "4000") config :mv, :dns_cluster_query, System.get_env("DNS_CLUSTER_QUERY") @@ -316,7 +230,11 @@ 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 = parse_positive_integer.(System.get_env("SMTP_PORT"), 587) + smtp_port_env = + case System.get_env("SMTP_PORT") do + nil -> 587 + v -> String.to_integer(String.trim(v)) + end 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 3cc01df..13b0d17 100644 --- a/docs/smtp-configuration-concept.md +++ b/docs/smtp-configuration-concept.md @@ -42,8 +42,6 @@ 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 ec8f357..41a77cd 100644 --- a/lib/mv/mailer.ex +++ b/lib/mv/mailer.ex @@ -100,11 +100,7 @@ defmodule Mv.Mailer do """ @spec smtp_config() :: keyword() def smtp_config 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 + if Mv.Config.smtp_configured?() and not boot_smtp_configured?() 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 98f6f23..f1fbd76 100644 --- a/test/mv_web/controllers/auth_controller_test.exs +++ b/test/mv_web/controllers/auth_controller_test.exs @@ -316,11 +316,9 @@ 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, 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}" + {:error, {:redirect, %{to: to}}} -> + refute to =~ "sign_in_with_token", + "Expected password sign-in to be rejected when OIDC-only, got redirect to: #{to}" _ -> # LiveView re-rendered (e.g. with flash error) instead of redirecting to success @@ -400,24 +398,8 @@ defmodule MvWeb.AuthControllerTest do test "returns 200 when OIDC-only but OIDC not configured", %{conn: authenticated_conn} do {:ok, settings} = Membership.get_settings() - - 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 - }) + original_oidc_only = Map.get(settings, :oidc_only, false) + {:ok, _} = Membership.update_settings(settings, %{oidc_only: true}) try do conn = build_unauthenticated_conn(authenticated_conn) @@ -425,7 +407,7 @@ defmodule MvWeb.AuthControllerTest do assert conn.status == 200 after {:ok, s} = Membership.get_settings() - Membership.update_settings(s, prev) + Membership.update_settings(s, %{oidc_only: original_oidc_only}) end end