From 5d2bbd1405af879635f030d79527b095fb67fc24 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sat, 19 Sep 2026 21:33:37 +0000 Subject: [PATCH 01/35] Fix locale-dependent failures in ETag and CSP config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On a machine whose JVM locale is not English, every /assets/* webjar request returned 200 with an empty body and no headers. Font Awesome never loaded, so icon-only controls (the ability-score reorder arrows among them) vanished with no error in the browser or the server log. Two defects combined: 1. rfc822-formatter used DateTimeFormatter/ofPattern with no Locale, so it parsed the Last-Modified header using the JVM default. HTTP dates are always English (RFC 7231), so on a Spanish or German machine "Mon" is not a day name and parse-date threw. 2. The etag-interceptor's catch returned the value of log/error rather than the context, discarding the response. Pedestal then had nothing to write and Jetty emitted a bare 200. This is why the failure was silent: the exception was logged, but the response was already gone. Only /assets/* was affected. ::http/resource-path puts Pedestal's resource interceptor ahead of the router, so /css/* is served and the chain terminated before the ETag interceptor ever runs. Fixing (2) alone would leave a working but ETag-less response; fixing (1) alone would leave the next exception silently blanking responses. Both are needed. The same class of bug appears in config.clj, where str/lower-case folds using the default locale. CSP_POLICY=STRICT on a Turkish machine becomes "strıct" (dotless i), matches nothing, and silently falls through to the permissive policy. Config tokens are ASCII, not prose, so these now use Locale/ROOT and equalsIgnoreCase. Verified under tr_TR, es_ES, ja_JP and en_US: the asset route returns 58,935 bytes with Content-Type text/css in all of them, and the ETag value is unchanged from what English-locale servers already produce, so no caches are invalidated. Confirmed fixed by the reporter on Spanish Windows. --- src/clj/orcpub/config.clj | 15 ++++++++++++--- src/clj/orcpub/pedestal.clj | 16 +++++++++++++--- 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/src/clj/orcpub/config.clj b/src/clj/orcpub/config.clj index f7cbc1064..3cdb949d5 100644 --- a/src/clj/orcpub/config.clj +++ b/src/clj/orcpub/config.clj @@ -1,7 +1,8 @@ (ns orcpub.config (:require [environ.core :refer [env]] [clojure.string :as str] - [clojure.java.io :as io])) + [clojure.java.io :as io]) + (:import [java.util Locale])) (def default-datomic-uri "datomic:dev://localhost:4334/orcpub") @@ -71,14 +72,22 @@ (let [policy (or (env :csp-policy) (System/getenv "CSP_POLICY") "strict")] - (str/lower-case policy))) + ;; Locale/ROOT, not str/lower-case: this is an ASCII config token, not + ;; prose. str/lower-case folds using the default locale, so on a Turkish + ;; machine "STRICT" becomes "strıct" (dotless i), misses every comparison + ;; below, and silently falls through to the permissive policy. + (.toLowerCase ^String policy Locale/ROOT))) (defn dev-mode? "Returns true when running in dev mode (DEV_MODE env var is 'true'). Env vars are strings — (boolean \"false\") is true in Clojure, so we must compare against the string \"true\" explicitly." [] - (= "true" (str/lower-case (or (env :dev-mode) "")))) + ;; equalsIgnoreCase compares per character rather than by locale casing + ;; rules, so it is immune to the Turkish-I problem described above. Note the + ;; receiver order: the literal is first so a nil env var returns false + ;; instead of throwing. + (.equalsIgnoreCase "true" (or (env :dev-mode) ""))) (defn strict-csp? "Returns true when CSP_POLICY=strict (regardless of dev mode). diff --git a/src/clj/orcpub/pedestal.clj b/src/clj/orcpub/pedestal.clj index ad0104f37..190b8aadd 100644 --- a/src/clj/orcpub/pedestal.clj +++ b/src/clj/orcpub/pedestal.clj @@ -11,7 +11,8 @@ [orcpub.config :as config] [orcpub.fork.integrations :as integrations]) (:import [java.io File] - [java.time.format DateTimeFormatter])) + [java.time.format DateTimeFormatter] + [java.util Locale])) (defn test? [service-map] @@ -37,7 +38,11 @@ nil) (def rfc822-formatter - (DateTimeFormatter/ofPattern "EEE, dd MMM yyyy HH:mm:ss Z")) + ;; Locale/ENGLISH is load-bearing. HTTP dates are always English (RFC 7231), + ;; but ofPattern without a locale parses using the JVM default, which follows + ;; the OS regional settings. On a non-English machine "Mon" is not a day name + ;; and parse-date throws, which used to blank the response entirely. + (DateTimeFormatter/ofPattern "EEE, dd MMM yyyy HH:mm:ss Z" Locale/ENGLISH)) (defn parse-date [date content-length] (when date @@ -100,7 +105,12 @@ (if new-etag (assoc-in context [:response :headers "etag"] new-etag) context))) - (catch Throwable t (log/error :msg "ETag interceptor error" :exception t))))})) + ;; Return the context. Without it the catch yields log/error's + ;; value, discarding the response: the client gets 200 with an + ;; empty body and no headers, and nothing reports a problem. + (catch Throwable t + (log/error :msg "ETag interceptor error" :exception t) + context)))})) (defrecord Pedestal [service-map conn service] component/Lifecycle From c27cbd32bef39c9bb3ef22535464c9ef53133334 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Fri, 18 Sep 2026 06:41:15 +0000 Subject: [PATCH 02/35] scripts: make the Windows paths work instead of silently lying MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit On Git Bash, `netstat` is Windows' netstat.exe, which has no -l flag. So the netstat branch of port_in_use ran `netstat -tln`, got "Invalid argument" on stderr (discarded) and nothing on stdout, and reported every port as free. lsof and ss are not present there, so that broken branch is the one Windows always took. find_pids_by_port was wrong the same way. One dead function, five symptoms: - start.sh's pre-flight check never warned, so the JVM was the first thing to notice, via BindException: Address already in use - stop.sh found nothing to stop and reported success while the port stayed held — no way out using the project's own tooling - wait_for_datomic never saw port 4334, so start.sh datomic printed "Failed to start Datomic" 30 seconds after starting it successfully - init_database refused with "Datomic is not running" for the same reason - --idempotent believed nothing was running and would start duplicates, which defeats the point of the flag Now Windows is asked its own way, matching the LOCAL ADDRESS column rather than the state word: netstat.exe prints "LISTENING" where Microsoft's reference says "LISTEN", and a non-English Windows translates it outright ("ABHÖREN" on a German install). Column 2 is the local address on every row and the PID is the last column, in any language. That also stops port 8890 matching a row for 18890. The PIDs that come back are native Windows PIDs, which Git Bash's `kill` cannot reliably signal — so stop.sh would still have reported success while the process lived. signal_pid/pid_alive route through taskkill/tasklist on Windows and plain kill everywhere else. Two behaviour changes beyond detection: - explain_bind_failure: when lein exits non-zero, say why in place. It names the holding PID and the command to stop it, or reports that Windows has RESERVED the port. That reserved case is invisible to the pre-flight check by definition — nothing is listening, yet bind fails. - repl_mode: Windows never gets the interactive REPL. Git Bash reports a tty, so `[[ -t 0 ]]` chose it, but that terminal is not a Windows console: the REPL prints its prompt, exits, and takes the already-bound server with it. Headless is the same server without that passenger. Unix is unchanged by construction — is_windows returns false and every path falls through to the same lsof/ss/netstat chain and the same kill calls. Verified locally: detection correct both ways, explain_bind_failure names the PID and offers `kill`, and repl_mode still returns "interactive" under a tty so Linux and macOS keep their REPL. --- scripts/common.sh | 122 ++++++++++++++++++++++++++++++++++++++++++++-- scripts/start.sh | 22 +++++++-- scripts/stop.sh | 6 +-- 3 files changed, 139 insertions(+), 11 deletions(-) diff --git a/scripts/common.sh b/scripts/common.sh index c59da0336..afaebbbf5 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -140,9 +140,28 @@ log_error() { # ----------------------------------------------------------------------------- # Check if a port is in use (returns 0 if in use, 1 if free) +# True under Git Bash / MSYS2 / Cygwin, where `netstat` is Windows' netstat.exe. +is_windows() { + case "$(uname -s 2>/dev/null)" in + MINGW*|MSYS*|CYGWIN*) return 0 ;; + *) return 1 ;; + esac +} + +# Windows netstat.exe has no -l flag, so the GNU-style `netstat -tln` below +# exits with "Invalid argument" and prints nothing to stdout. With stderr +# discarded that reads as "no match" — i.e. every port looks free, the +# pre-flight check never warns, and the JVM is the first thing to discover the +# conflict (BindException: Address already in use). Ask Windows its own way. port_in_use() { local port="$1" - if command -v lsof >/dev/null 2>&1; then + if is_windows; then + # Match on the LOCAL ADDRESS column, not the state word: netstat.exe + # prints "LISTENING" where the docs say "LISTEN", and a non-English + # Windows translates it outright. Column 2 is the local address on + # every row; the last column is the PID. + [ -n "$(netstat -ano 2>/dev/null | awk -v p="[:.]${port}\$" '$2 ~ p {print; exit}')" ] + elif command -v lsof >/dev/null 2>&1; then lsof -i ":${port}" >/dev/null 2>&1 elif command -v ss >/dev/null 2>&1; then ss -tln 2>/dev/null | grep -q ":${port}\b" @@ -216,6 +235,15 @@ find_pids_by_port() { local port="$1" local pids="" + if is_windows; then + # Last column of a LISTENING row is the owning PID. + pids=$(netstat -ano 2>/dev/null \ + | awk -v p="[:.]${port}\$" '$2 ~ p && $NF ~ /^[0-9]+$/ {print $NF}' \ + | sort -u || true) + echo "$pids" | tr '\n' ' ' | xargs + return + fi + if command -v lsof >/dev/null 2>&1; then pids=$(lsof -t -i ":${port}" 2>/dev/null || true) elif command -v ss >/dev/null 2>&1; then @@ -334,23 +362,109 @@ check_datomic_installed() { # Process Management # ----------------------------------------------------------------------------- +# Signal a process. On Windows the PIDs we discover come from netstat -ano and +# are native Windows PIDs, which Git Bash's `kill` cannot reliably signal — so +# stop.sh would report success while the process kept holding the port. The +# leading `//` stops MSYS rewriting /PID into a path. +signal_pid() { + local pid="$1" sig="${2:-TERM}" + if is_windows; then + if [[ "$sig" == "KILL" ]]; then + taskkill //PID "$pid" //F >/dev/null 2>&1 + else + taskkill //PID "$pid" >/dev/null 2>&1 + fi + else + kill "-$sig" "$pid" 2>/dev/null + fi +} + +# Is this PID still alive? +pid_alive() { + local pid="$1" + if is_windows; then + tasklist //FI "PID eq $pid" 2>/dev/null | grep -qE "[[:space:]]${pid}[[:space:]]" + else + kill -0 "$pid" 2>/dev/null + fi +} + +# When the JVM dies with "Address already in use", say why in terms the user can +# act on. This runs AFTER lein exits, which is the only moment it can: the REPL +# holds the terminal while it lives, so nothing downstream runs until it stops. +# The pre-flight check cannot cover this case — a port RESERVED by Windows reads +# as free to every listing tool, right up until bind fails. +explain_bind_failure() { + local port="$1" + echo "" + log_error "The server could not bind port $port." + + local pids + pids="$(find_pids_by_port "$port")" + if [[ -n "${pids// /}" ]]; then + log_error "Something is already listening on it (PID: $pids)." + if is_windows; then + log_error " Stop it with: taskkill /PID ${pids%% *} /F" + else + log_error " Stop it with: kill ${pids%% *}" + fi + return + fi + + if is_windows && command -v netsh >/dev/null 2>&1; then + local ranges reserved="" + ranges="$(netsh interface ipv4 show excludedportrange protocol=tcp 2>/dev/null | tr -d '\r')" + while read -r lo hi _rest; do + [[ "$lo" =~ ^[0-9]+$ ]] || continue + [[ "$hi" =~ ^[0-9]+$ ]] || continue + if (( port >= lo && port <= hi )); then reserved="$lo-$hi"; fi + done <<< "$ranges" + if [[ -n "$reserved" ]]; then + log_error "Nothing is listening, but Windows has RESERVED $port (range $reserved)." + log_error " Hyper-V/WSL2/Docker take these ranges. In an admin terminal:" + log_error " net stop winnat && net start winnat" + return + fi + fi + + log_error "Nothing appears to be listening on it, which is unusual." + log_error " For a full report run: bash scripts/diagnostics/orcpub-port-doctor.sh" +} + +# Which REPL mode should the server start in? +# +# Git Bash reports stdin as a terminal, so the old `[[ -t 0 ]]` test chose the +# interactive REPL there — but its terminal is not a Windows console. The REPL +# prints its prompt, exits immediately, and takes the already-bound server down +# with it ("Subprocess failed (exit code: 1)" / "Bye for now!"). Headless is the +# same server without that passenger, so Windows always gets headless. +repl_mode() { + if is_windows; then + echo headless + elif [[ -t 0 ]]; then + echo interactive + else + echo headless + fi +} + # Graceful shutdown with SIGKILL fallback kill_gracefully() { local pid="$1" local wait_secs="${2:-$KILL_WAIT}" # Try SIGTERM first - kill -TERM "$pid" 2>/dev/null || return 0 + signal_pid "$pid" TERM || return 0 # Wait for process to exit for ((i=0; i/dev/null || return 0 + pid_alive "$pid" || return 0 sleep 1 done # Process still running - escalate to SIGKILL log_warn "Process $pid didn't stop gracefully, sending SIGKILL" - kill -KILL "$pid" 2>/dev/null || true + signal_pid "$pid" KILL || true } # Clean up stale PID files diff --git a/scripts/start.sh b/scripts/start.sh index 8ddb5f912..599df1aba 100755 --- a/scripts/start.sh +++ b/scripts/start.sh @@ -406,13 +406,19 @@ start_server() { cd "$REPO_ROOT" # Use headless mode if not running interactively (background/nohup) - if [[ -t 0 ]]; then + local rc=0 + if [[ "$(repl_mode)" == "interactive" ]]; then log_info "Starting REPL with server (profile: +dev,+start-server)..." - lein with-profile +dev,+start-server repl + lein with-profile +dev,+start-server repl || rc=$? else log_info "Starting headless server (profile: +dev,+start-server)..." - lein with-profile +dev,+start-server repl :headless + is_windows && log_info "Headless on Windows: the Git Bash REPL exits on start and stops the server." + lein with-profile +dev,+start-server repl :headless || rc=$? fi + # The REPL held the terminal until now; this is the first chance to explain + # a bind failure, and the only place the reserved-port case is visible. + [[ $rc -ne 0 ]] && explain_bind_failure "$SERVER_PORT" + return $rc } start_figwheel() { @@ -628,7 +634,15 @@ start_all() { log_info "Starting REPL with server (profile: +dev,+start-server)..." log_info "Note: Ctrl+C will stop both server and Datomic" cd "$REPO_ROOT" - lein with-profile +dev,+start-server repl + local rc=0 + if [[ "$(repl_mode)" == "interactive" ]]; then + lein with-profile +dev,+start-server repl || rc=$? + else + is_windows && log_info "Headless on Windows: the Git Bash REPL exits on start and stops the server." + lein with-profile +dev,+start-server repl :headless || rc=$? + fi + [[ $rc -ne 0 ]] && explain_bind_failure "$SERVER_PORT" + return $rc } # ----------------------------------------------------------------------------- diff --git a/scripts/stop.sh b/scripts/stop.sh index c5064ea4d..f1845feac 100755 --- a/scripts/stop.sh +++ b/scripts/stop.sh @@ -133,7 +133,7 @@ kill_pids() { [[ "$quiet" != "true" ]] && log_info "Sending SIGTERM to PIDs: $pids" for pid in $pids; do - kill -TERM "$pid" 2>/dev/null || true + signal_pid "$pid" TERM || true done sleep "$wait_time" @@ -141,7 +141,7 @@ kill_pids() { # Check for survivors local remaining="" for pid in $pids; do - kill -0 "$pid" 2>/dev/null && remaining="$remaining $pid" + pid_alive "$pid" && remaining="$remaining $pid" done remaining=$(echo "$remaining" | xargs) @@ -149,7 +149,7 @@ kill_pids() { if [[ "$use_force" == "true" ]]; then [[ "$quiet" != "true" ]] && log_warn "Processes still running, sending SIGKILL: $remaining" for pid in $remaining; do - kill -KILL "$pid" 2>/dev/null || true + signal_pid "$pid" KILL || true done sleep 1 else From 9a0e482353c32111580cc3f05beaab71d1ce25a3 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Fri, 18 Sep 2026 06:41:15 +0000 Subject: [PATCH 03/35] ci: prove the Windows behaviour on a Windows runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Windows paths were argued, then tested against stubs written by the same hand that wrote the parser — which can agree and both be wrong. Checking one assumption against Microsoft's netstat reference already found a real defect (the state word), which is fair warning about the rest. windows-latest settles it, and asserts rather than reports, so a wrong assumption turns the build red instead of printing something reassuring: - uname says MINGW, so is_windows() fires - lsof and ss really are absent, so the netstat branch is the live one - `netstat -tln` really does fail with empty stdout - against a genuinely bound port: the fixed check says in-use, the pre-fix one is shown saying free, and find_pids_by_port returns the PID - signal_pid/taskkill stops a process found by port and frees it - repl_mode returns headless, so Windows is never handed the REPL that kills the server - check_port_available catches a busy port and names it, and explain_bind_failure identifies the holder and gives the stop command Those last two matter most: they are the existing code doing the job, which is why no separate launcher is being merged. No Java, Leiningen or Datomic needed — python binds the port — so it runs in about a minute and can sit on every push. Reserved ranges are printed, not asserted: they vary per machine, and the runner has none covering 8890. --- .github/workflows/windows-scripts.yml | 212 ++++++++++++++++++++++++++ 1 file changed, 212 insertions(+) create mode 100644 .github/workflows/windows-scripts.yml diff --git a/.github/workflows/windows-scripts.yml b/.github/workflows/windows-scripts.yml new file mode 100644 index 000000000..cc79af282 --- /dev/null +++ b/.github/workflows/windows-scripts.yml @@ -0,0 +1,212 @@ +# Validates the Windows/Git Bash assumptions the port-detection code is built +# on. Everything about the Windows path was reasoned about and tested against +# hand-written stubs — which is circular, because the same author wrote the stub +# and the parser. This runs it on a real Windows machine instead. +# +# Deliberately does NOT need Java, Leiningen or Datomic: it binds a port with +# python and asks the shell functions what they see. That keeps it ~1 minute so +# it can run on every push to the branch. +# +# It ASSERTS rather than reports. If an assumption is wrong, this job goes red. + +name: Windows scripts + +on: + push: + branches: [fix/windows-port-detection, develop, main] + pull_request: + branches: [develop, main] + workflow_dispatch: + +permissions: + contents: read + +jobs: + probe: + name: Git Bash platform facts + port detection + runs-on: windows-latest + timeout-minutes: 15 + defaults: + run: + shell: bash # on windows-latest this is Git Bash (MSYS2) + steps: + - name: Checkout + uses: actions/checkout@v4 + + # --------------------------------------------------------------------- + # FACTS — printed whether or not they match what we assumed + # --------------------------------------------------------------------- + - name: Platform and tool inventory + run: | + echo "uname -s : $(uname -s)" + echo "OSTYPE : ${OSTYPE:-}" + for t in lsof ss netstat tasklist taskkill netsh python; do + printf ' %-10s %s\n' "$t" "$(command -v "$t" 2>/dev/null || echo '(absent)')" + done + + - name: ASSERT uname reports MINGW/MSYS + run: | + case "$(uname -s)" in + MINGW*|MSYS*|CYGWIN*) echo "OK: $(uname -s)" ;; + *) echo "FAIL: is_windows() would not fire — uname says '$(uname -s)'"; exit 1 ;; + esac + + - name: ASSERT lsof and ss are absent + run: | + # If either exists, port_in_use takes a different branch than assumed + # and the Windows branch is dead code. Not fatal to users, but it means + # the whole diagnosis was wrong, so fail and make us look. + rc=0 + command -v lsof >/dev/null 2>&1 && { echo "FAIL: lsof EXISTS"; rc=1; } + command -v ss >/dev/null 2>&1 && { echo "FAIL: ss EXISTS"; rc=1; } + [ $rc -eq 0 ] && echo "OK: neither lsof nor ss present" + exit $rc + + - name: ASSERT 'netstat -tln' fails and prints nothing to stdout + run: | + # This is THE load-bearing claim: the old code ran exactly this, threw + # stderr away, and read the empty stdout as "the port is free". + set +e + out="$(netstat -tln 2>/tmp/err.txt)"; rc=$? + err="$(cat /tmp/err.txt)" + echo "exit code : $rc" + echo "stdout : [$out]" + echo "stderr : [$err]" + if [ -n "$out" ]; then + echo "FAIL: netstat -tln produced stdout — this netstat understands GNU flags" + exit 1 + fi + echo "OK: stdout empty, so the old check reads every port as free" + + - name: Show real 'netstat -ano' output shape + run: netstat -ano | head -15 + + # --------------------------------------------------------------------- + # THE ACTUAL TEST — bind a port for real, then ask both implementations + # --------------------------------------------------------------------- + - name: Old vs new detection against a real listener + run: | + set -u + python -c " + import socket, time + s = socket.socket() + s.bind(('127.0.0.1', 8890)); s.listen(5) + time.sleep(180) + " & + LISTENER=$! + sleep 5 + + # independent truth: can we actually connect? + if (exec 3<>/dev/tcp/127.0.0.1/8890) 2>/dev/null; then + echo "truth : port 8890 IS listening" + else + echo "SETUP FAIL : could not bind 8890 on the runner"; exit 1 + fi + + old_port_in_use() { # verbatim from develop, pre-fix + local port="$1" + if command -v lsof >/dev/null 2>&1; then lsof -i ":${port}" >/dev/null 2>&1 + elif command -v ss >/dev/null 2>&1; then ss -tln 2>/dev/null | grep -q ":${port}\b" + elif command -v netstat >/dev/null 2>&1; then netstat -tln 2>/dev/null | grep -q ":${port}\b" + else timeout 1 bash -c "/dev/null; fi + } + + source scripts/common.sh # the fixed implementation + + old_port_in_use 8890 && O=in-use || O=free + port_in_use 8890 && N=in-use || N=free + PIDS="$(find_pids_by_port 8890)" + echo "old check : $O" + echo "new check : $N (fixed)" + echo "pids : ${PIDS:-}" + + fail=0 + [ "$N" = "in-use" ] || { echo "FAIL: fixed port_in_use missed a real listener"; fail=1; } + [ -n "${PIDS// /}" ] || { echo "FAIL: find_pids_by_port returned nothing"; fail=1; } + port_in_use 8891 && { echo "FAIL: reported an unused port as busy"; fail=1; } + [ "$O" = "free" ] && echo "CONFIRMED: the old check reports 'free' for a port that is in use" + kill $LISTENER 2>/dev/null || true + exit $fail + + - name: ASSERT taskkill can stop a process we found by port + run: | + set -u + source scripts/common.sh + python -c " + import socket, time + s = socket.socket() + s.bind(('127.0.0.1', 8899)); s.listen(5) + time.sleep(180) + " & + sleep 5 + PIDS="$(find_pids_by_port 8899)" + echo "found pids : ${PIDS:-}" + [ -n "${PIDS// /}" ] || { echo "FAIL: could not find the listener"; exit 1; } + for p in $PIDS; do signal_pid "$p" KILL || true; done + sleep 3 + STILL="$(find_pids_by_port 8899)" + if [ -n "${STILL// /}" ]; then + echo "FAIL: taskkill did not stop it (still $STILL)"; exit 1 + fi + echo "OK: signal_pid/taskkill stopped the process and freed the port" + + - name: Reserved port ranges (informational) + run: | + netsh interface ipv4 show excludedportrange protocol=tcp || true + echo "--- is 8890 inside a reserved range on this runner? ---" + netsh interface ipv4 show excludedportrange protocol=tcp 2>/dev/null | tr -d '\r' \ + | awk '$1 ~ /^[0-9]+$/ && $2 ~ /^[0-9]+$/ && 8890 >= $1 && 8890 <= $2 {print " RESERVED: " $1 "-" $2; found=1} + END { if (!found) print " not reserved here" }' + + - name: ASSERT repl_mode picks headless on Windows + run: | + set -u + source scripts/common.sh + m="$(repl_mode)" + echo "repl_mode : $m" + if [ "$m" != "headless" ]; then + echo "FAIL: Windows must not get the interactive REPL — it exits and kills the server" + exit 1 + fi + echo "OK: Windows takes the headless path" + + - name: ASSERT start.sh's own port guard sees a busy port + run: | + # This is what replaces a bespoke launcher: with port_in_use fixed, + # check_port_available detects the conflict, names the PID, and (when + # interactive) offers to stop it. CI is non-interactive, so the + # expected outcome is a clean refusal naming the port. + set -u + python -c " + import socket, time + s = socket.socket() + s.bind(('127.0.0.1', 8890)); s.listen(5) + time.sleep(120) + " & + sleep 5 + source scripts/common.sh + set +e + out="$(check_port_available 8890 server false 2>&1)"; rc=$? + set -e + echo "$out" + echo "exit: $rc" + [ "$rc" -ne 0 ] || { echo "FAIL: guard allowed a busy port through"; exit 1; } + echo "$out" | grep -qi "in use" || { echo "FAIL: did not say the port was in use"; exit 1; } + echo "OK: the existing guard catches it — no separate launcher needed" + + - name: ASSERT explain_bind_failure names the holder + run: | + set -u + source scripts/common.sh + python -c " + import socket, time + s = socket.socket() + s.bind(('127.0.0.1', 8890)); s.listen(5) + time.sleep(60) + " & + sleep 5 + out="$(explain_bind_failure 8890 2>&1)" + echo "$out" + echo "$out" | grep -qi "already listening" || { echo "FAIL: did not identify the listener"; exit 1; } + echo "$out" | grep -qi "taskkill" || { echo "FAIL: did not give the Windows stop command"; exit 1; } + echo "OK: a bind failure is explained in place" From 5829e859d190c3ec2491b7b7815189669e2a5c88 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Fri, 18 Sep 2026 06:44:23 +0000 Subject: [PATCH 04/35] ci: source check_port_available from start.sh, where it actually lives MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new step sourced only common.sh and then called check_port_available, which is defined in start.sh — so the job failed with 'command not found' rather than on anything about the code under test. start.sh cannot simply be run end to end on the runner either: it checks for Leiningen and exits at the prereq before it ever reaches the port guard. Lift the function body out and exercise that instead. Verified locally: the extracted function refuses a busy port non-interactively with 'Port 8890 in use (non-interactive mode, exiting)' and names the PID. --- .github/workflows/windows-scripts.yml | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.github/workflows/windows-scripts.yml b/.github/workflows/windows-scripts.yml index cc79af282..e9625e7a9 100644 --- a/.github/workflows/windows-scripts.yml +++ b/.github/workflows/windows-scripts.yml @@ -185,6 +185,11 @@ jobs: " & sleep 5 source scripts/common.sh + # check_port_available lives in start.sh, and start.sh cannot be run + # end to end here (it exits at the Leiningen prereq before reaching + # the port guard), so lift the real function body out and test that. + SCRIPT_DIR="$PWD/scripts" + source <(sed -n '/^check_port_available()/,/^}/p' scripts/start.sh) set +e out="$(check_port_available 8890 server false 2>&1)"; rc=$? set -e From 6636369a405e2c5489dd284465e9b77512ee6248 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Fri, 18 Sep 2026 07:14:39 +0000 Subject: [PATCH 05/35] scripts: handle both kinds of Windows pid, and stop crying wolf MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found reviewing my own change, not by anything failing. find_service_pids checks the PID FILE before it scans ports, and that pid was written from $! — an MSYS pid, which taskkill cannot see. Only the port fallback yields a native Windows pid. Routing everything through taskkill fixed the pids that had never arrived before (detection was broken) while breaking the ones that always had: stop.sh would report success with the process still running. signal_pid and pid_alive now try kill first and fall back to taskkill, so both kinds work. explain_bind_failure ran on every non-zero exit from lein. Ctrl+C is a non-zero exit. After a normal shutdown the user would have been told "The server could not bind port 8890. Nothing appears to be listening on it, which is unusual" — an invented problem at the end of an ordinary session. It now speaks only with evidence: something holds the port, or Windows has reserved it. Otherwise it says nothing, and the header moved inside those branches so it cannot appear alone. Both are now asserted on the Windows runner: signal_pid must stop a pid that taskkill cannot see, and explain_bind_failure must print nothing for a free port. Verified on Linux too — silent when free, names the PID and offers `kill` when held. --- .github/workflows/windows-scripts.yml | 31 +++++++++++++++++++++++++++ scripts/common.sh | 22 ++++++++++++++----- 2 files changed, 48 insertions(+), 5 deletions(-) diff --git a/.github/workflows/windows-scripts.yml b/.github/workflows/windows-scripts.yml index e9625e7a9..baa4c7e86 100644 --- a/.github/workflows/windows-scripts.yml +++ b/.github/workflows/windows-scripts.yml @@ -215,3 +215,34 @@ jobs: echo "$out" | grep -qi "already listening" || { echo "FAIL: did not identify the listener"; exit 1; } echo "$out" | grep -qi "taskkill" || { echo "FAIL: did not give the Windows stop command"; exit 1; } echo "OK: a bind failure is explained in place" + + - name: ASSERT signal_pid handles an MSYS pid, not just a Windows pid + run: | + # find_service_pids checks the PID FILE first, and that pid came from + # $! — an MSYS pid, which taskkill cannot see. Using taskkill alone + # would make stop.sh report success while the process kept running. + set -u + source scripts/common.sh + sleep 120 & + MSYS_PID=$! + echo "msys pid : $MSYS_PID" + pid_alive "$MSYS_PID" || { echo "FAIL: pid_alive cannot see an MSYS pid"; exit 1; } + signal_pid "$MSYS_PID" KILL || true + sleep 2 + if pid_alive "$MSYS_PID"; then + echo "FAIL: signal_pid did not stop an MSYS pid"; exit 1 + fi + echo "OK: both pid kinds are handled" + + - name: ASSERT explain_bind_failure stays quiet when the port is free + run: | + # lein exits non-zero for ordinary reasons, Ctrl+C included. Announcing + # a bind failure after a normal shutdown would be crying wolf. + set -u + source scripts/common.sh + out="$(explain_bind_failure 8890 2>&1)" + echo "output: [$out]" + if [ -n "$out" ]; then + echo "FAIL: reported a bind failure for a port nothing is using"; exit 1 + fi + echo "OK: silent when there is no evidence of a port problem" diff --git a/scripts/common.sh b/scripts/common.sh index afaebbbf5..8edc69856 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -369,6 +369,12 @@ check_datomic_installed() { signal_pid() { local pid="$1" sig="${2:-TERM}" if is_windows; then + # A pid here can be either kind: find_service_pids checks the PID FILE + # first, which holds an MSYS pid (written from $! by start.sh), and only + # falls back to netstat -ano, which yields a native Windows pid. `kill` + # handles the first, taskkill the second, and neither handles both — so + # try kill, then taskkill. + kill "-$sig" "$pid" 2>/dev/null && return 0 if [[ "$sig" == "KILL" ]]; then taskkill //PID "$pid" //F >/dev/null 2>&1 else @@ -383,6 +389,8 @@ signal_pid() { pid_alive() { local pid="$1" if is_windows; then + # Same two-kinds-of-pid problem as signal_pid: ask both. + kill -0 "$pid" 2>/dev/null && return 0 tasklist //FI "PID eq $pid" 2>/dev/null | grep -qE "[[:space:]]${pid}[[:space:]]" else kill -0 "$pid" 2>/dev/null @@ -396,12 +404,11 @@ pid_alive() { # as free to every listing tool, right up until bind fails. explain_bind_failure() { local port="$1" - echo "" - log_error "The server could not bind port $port." - local pids pids="$(find_pids_by_port "$port")" if [[ -n "${pids// /}" ]]; then + echo "" + log_error "The server could not bind port $port." log_error "Something is already listening on it (PID: $pids)." if is_windows; then log_error " Stop it with: taskkill /PID ${pids%% *} /F" @@ -420,6 +427,8 @@ explain_bind_failure() { if (( port >= lo && port <= hi )); then reserved="$lo-$hi"; fi done <<< "$ranges" if [[ -n "$reserved" ]]; then + echo "" + log_error "The server could not bind port $port." log_error "Nothing is listening, but Windows has RESERVED $port (range $reserved)." log_error " Hyper-V/WSL2/Docker take these ranges. In an admin terminal:" log_error " net stop winnat && net start winnat" @@ -427,8 +436,11 @@ explain_bind_failure() { fi fi - log_error "Nothing appears to be listening on it, which is unusual." - log_error " For a full report run: bash scripts/diagnostics/orcpub-port-doctor.sh" + # Nothing holds the port and it is not reserved, so there is no evidence + # this was a bind failure at all — lein exits non-zero for ordinary reasons + # too, Ctrl+C among them. Saying "could not bind" here would be crying wolf + # after a normal shutdown, so say nothing. + return 0 } # Which REPL mode should the server start in? From 71c4d26b9bc6c334a4103198b02e23beb3167ec0 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Fri, 18 Sep 2026 07:17:19 +0000 Subject: [PATCH 06/35] ci: assert the quiet case on a port no other step has touched MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The new assertion checked explain_bind_failure against port 8890 and failed — correctly, because 8890 was still held. Earlier steps start python listeners that sleep long past the end of the step that created them, so by the time this ran the port was genuinely in use and the function was right to say so. The runner's own cleanup gave it away: "Terminate orphan process: pid (7592) (python)". The code was fine; the test was. Use 8123, which nothing else touches, and assert it really is free before drawing any conclusion from the silence. --- .github/workflows/windows-scripts.yml | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/.github/workflows/windows-scripts.yml b/.github/workflows/windows-scripts.yml index baa4c7e86..3ec6aaa01 100644 --- a/.github/workflows/windows-scripts.yml +++ b/.github/workflows/windows-scripts.yml @@ -240,7 +240,14 @@ jobs: # a bind failure after a normal shutdown would be crying wolf. set -u source scripts/common.sh - out="$(explain_bind_failure 8890 2>&1)" + # Use a port no other step touches. Earlier steps leave their python + # listeners alive (they sleep well past the end of the step), so 8890 + # is NOT free by the time we get here. + FREE_PORT=8123 + if port_in_use "$FREE_PORT"; then + echo "SETUP FAIL: $FREE_PORT is unexpectedly in use"; exit 1 + fi + out="$(explain_bind_failure "$FREE_PORT" 2>&1)" echo "output: [$out]" if [ -n "$out" ]; then echo "FAIL: reported a bind failure for a port nothing is using"; exit 1 From 798b126867c86bda5bd2fc254537f41aecd53220 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 00:38:25 +0000 Subject: [PATCH 07/35] ci: run the Windows job on the branch that now carries the work The push trigger still named fix/windows-port-detection. Those commits were cherry-picked onto hotfix/locale-safety, so pushes to the branch holding the fix ran no CI at all -- while the PR cited that CI as proof the fix works. The pull_request trigger (develop, main) was unaffected and would still fire for the upstream PR; this only restores the push-time signal on the branch under development. --- .github/workflows/windows-scripts.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/windows-scripts.yml b/.github/workflows/windows-scripts.yml index 3ec6aaa01..5d23ee196 100644 --- a/.github/workflows/windows-scripts.yml +++ b/.github/workflows/windows-scripts.yml @@ -13,7 +13,7 @@ name: Windows scripts on: push: - branches: [fix/windows-port-detection, develop, main] + branches: [hotfix/locale-safety, develop, main] pull_request: branches: [develop, main] workflow_dispatch: From 4e972cf7c5632e30821476fd6066f86a4a6c953d Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 01:14:16 +0000 Subject: [PATCH 08/35] Correct the CSP docs, and say which policy the server starts under Five places documented a Content-Security-Policy-Report-Only mode for dev. No code has ever emitted that header. The only header set is "Content-Security-Policy", enforcing; and in dev mode no nonce is generated, so the interceptor sets no header at all. That mattered: DEV_MODE defaults to FALSE, so a checkout with no .env runs the enforcing policy, whose connect-src omits ws://localhost:3449. Figwheel's hot reload is blocked, and anyone reading the code to find out why was told dev mode is Report-Only and therefore safe. The description and the behaviour had drifted apart, and the docs actively concealed the trap. Corrected in config.clj (strict-csp? docstring and the get-secure-headers-config comment), pedestal.clj (make-nonce-interceptor), index.clj and .env.example. The phrase "Report-Only" now appears only in the two sentences stating it does not exist, so anyone searching for it lands on the correction. Also annotates the hardcoded ":dev-mode? false" in the nonce interceptor. It looks like a bug next to a dev-mode? parameter in scope, but :enter only mints a nonce when dev-mode? is false, so the branch is unreachable in dev. Said so rather than leaving it to be rediscovered. No default is changed. Loose dev already works as designed -- .env.example ships DEV_MODE=true and common.sh sources .env under `set -a` -- so the only gap was for someone who never created a .env. Changing a security default belongs upstream, not in a bugfix. Instead, start_server now reports the policy before launching, via report_csp_mode in common.sh: unset DEV_MODE + strict -> warn, name the blocked socket, point at .env.example (nobody chose this) DEV_MODE=false + strict -> one info line (a decision, not an accident) DEV_MODE=true -> info CSP_POLICY != strict -> info The split matters: warning someone who explicitly chose prod mode is the crying-wolf pattern that trains people to ignore warnings. Verified across all four cases, and DEV_MODE matching is case-insensitive to match the server's own comparison. --- .env.example | 7 +++++-- scripts/common.sh | 31 +++++++++++++++++++++++++++++++ scripts/start.sh | 2 ++ src/clj/orcpub/config.clj | 21 +++++++++++++-------- src/clj/orcpub/index.clj | 4 ++-- src/clj/orcpub/pedestal.clj | 15 +++++++++++---- 6 files changed, 64 insertions(+), 16 deletions(-) diff --git a/.env.example b/.env.example index 9516f341e..4c98ffe0c 100644 --- a/.env.example +++ b/.env.example @@ -48,8 +48,11 @@ SIGNATURE=change-me-to-something-unique-and-long # Content Security Policy (strict|permissive|none) CSP_POLICY=strict -# Dev mode: CSP violations are logged (Report-Only) instead of blocked, -# allowing Figwheel hot-reload scripts to execute. +# Dev mode: no CSP header is sent at all, so Figwheel's scripts and its +# websocket (ws://localhost:3449) work. Leave this true for development. +# With DEV_MODE unset or false, CSP_POLICY=strict sends an ENFORCING policy +# whose connect-src does not include the Figwheel socket, and hot reload is +# blocked with no obvious cause. # Must be the string "true" (case-insensitive). Any other value is treated as false. DEV_MODE=true diff --git a/scripts/common.sh b/scripts/common.sh index 8edc69856..4beec6de8 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -31,6 +31,37 @@ if [[ -f "$REPO_ROOT/.env" ]]; then set +a fi +# Report which Content-Security-Policy the server will run under. +# +# CSP mode is invisible until something breaks, and what breaks first is +# Figwheel's websocket -- hot reload simply stops working, with the cause a +# response header nobody thought to look at. DEV_MODE defaults to false, so a +# checkout with no .env gets the enforcing policy without asking for it. +# +# Quiet when the combination is fine; loud only when it will bite. +report_csp_mode() { + local policy="${CSP_POLICY:-strict}" + local dev="${DEV_MODE:-}" + # Match the server's own comparison: case-insensitive, exactly "true". + local dev_on=false + case "$(printf '%s' "$dev" | tr '[:upper:]' '[:lower:]')" in true) dev_on=true ;; esac + + if [[ "$policy" != "strict" || "$dev_on" == "true" ]]; then + log_info "CSP: policy=$policy, DEV_MODE=${dev:-}" + elif [[ -n "$dev" ]]; then + # Explicitly set to something other than true -- a decision, not an + # accident. State the consequence once, without the lecture. + log_info "CSP: strict and ENFORCING (DEV_MODE=$dev). Figwheel hot reload will be blocked." + else + # Unset: nobody chose this, and the symptom is a dead hot-reload socket + # with no visible cause. This is the case worth interrupting for. + log_warn "CSP: strict and ENFORCING (DEV_MODE is unset, which means false)." + log_warn " Figwheel's websocket (ws://localhost:$FIGWHEEL_PORT) is not in connect-src," + log_warn " so hot reload will be blocked. Set DEV_MODE=true in .env for development." + [[ -f "$REPO_ROOT/.env" ]] || log_warn " No .env found — start from .env.example." + fi +} + # Defaults (used if not set in .env) DATOMIC_VERSION="${DATOMIC_VERSION:-1.0.7482}" DATOMIC_TYPE="${DATOMIC_TYPE:-pro}" diff --git a/scripts/start.sh b/scripts/start.sh index 599df1aba..a3a0daeb5 100755 --- a/scripts/start.sh +++ b/scripts/start.sh @@ -405,6 +405,8 @@ start_server() { cd "$REPO_ROOT" + report_csp_mode + # Use headless mode if not running interactively (background/nohup) local rc=0 if [[ "$(repl_mode)" == "interactive" ]]; then diff --git a/src/clj/orcpub/config.clj b/src/clj/orcpub/config.clj index 3cdb949d5..37ff84093 100644 --- a/src/clj/orcpub/config.clj +++ b/src/clj/orcpub/config.clj @@ -92,13 +92,18 @@ (defn strict-csp? "Returns true when CSP_POLICY=strict (regardless of dev mode). - When true, nonce-interceptor generates per-request nonces and adds them - to script tags. The header type depends on mode: - - Dev mode: Content-Security-Policy-Report-Only (violations logged, not blocked) - - Prod mode: Content-Security-Policy (violations blocked) - - This allows catching CSP issues during development while still allowing - Figwheel's document.write() scripts to execute." + When true AND dev-mode? is false, nonce-interceptor generates a per-request + nonce and sets an ENFORCING Content-Security-Policy header. + + In dev mode it generates no nonce and sets no header at all, so there is no + CSP from this application -- which is what lets Figwheel's scripts and its + websocket work. Note the consequence: DEV_MODE defaults to FALSE, so a + checkout with no .env runs enforcing CSP, and ws://localhost:3449 is absent + from connect-src. Figwheel's hot reload is then blocked with no obvious + cause. .env.example sets DEV_MODE=true for exactly this reason. + + There is no Report-Only mode. Earlier revisions of this docstring described + one; no code has ever emitted Content-Security-Policy-Report-Only." [] (= "strict" (get-csp-policy))) @@ -111,7 +116,7 @@ [] (cond ;; Strict mode - nonce-interceptor handles CSP dynamically - ;; (uses Report-Only in dev, enforcing in prod) + ;; (enforcing when DEV_MODE is not true; no header at all in dev mode) (= "strict" (get-csp-policy)) {:content-security-policy-settings nil} diff --git a/src/clj/orcpub/index.clj b/src/clj/orcpub/index.clj index be80b49ba..3bf286c61 100644 --- a/src/clj/orcpub/index.clj +++ b/src/clj/orcpub/index.clj @@ -153,8 +153,8 @@ html { [:img {:src "/image/spiral.gif" :style "height:200px;width:200px;margin-top:200px"}]])] (include-css "/css/compiled/styles.css") - ;; Dev mode uses Report-Only CSP (logs violations but doesn't block) - ;; Prod mode uses enforcing CSP with nonces + ;; Every script tag carries the per-request nonce. It is nil in dev mode, + ;; where no CSP header is set at all; enforcing otherwise. (script-tag {:src "/js/compiled/orcpub.js" :nonce nonce}) (script-tag {:src "/js/cookies.js" :nonce nonce}) (include-css "/assets/font-awesome/5.13.1/css/all.min.css") diff --git a/src/clj/orcpub/pedestal.clj b/src/clj/orcpub/pedestal.clj index 190b8aadd..740e7da1c 100644 --- a/src/clj/orcpub/pedestal.clj +++ b/src/clj/orcpub/pedestal.clj @@ -59,10 +59,14 @@ - :enter phase generates a nonce and stores it in [:request :csp-nonce] - :leave phase adds enforcing Content-Security-Policy header with the nonce - In dev mode: CSP is skipped entirely. Pedestal 0.7's default CSP is still - active, but the nonce interceptor becomes a no-op. This avoids flooding the - browser console with Report-Only violations (inline Figwheel scripts, etc.) - that obscure real issues during development." + In dev mode: no nonce is generated, so the :leave branch never fires and + this interceptor sets no CSP header. Pedestal's own CSP is disabled in the + strict branch of get-secure-headers-config, so dev runs with no CSP from + this application -- which is what lets Figwheel's scripts and its websocket + work. + + There is no Report-Only mode anywhere in this codebase, despite what earlier + comments here claimed." [dev-mode?] (interceptor/interceptor {:name :nonce-interceptor @@ -74,6 +78,9 @@ (if-let [nonce (get-in ctx [:request :csp-nonce])] (assoc-in ctx [:response :headers "Content-Security-Policy"] (csp/build-csp-header nonce + ;; Always false here, and not a mistake: :enter only + ;; makes a nonce when dev-mode? is false, so this + ;; branch is unreachable in dev mode. :dev-mode? false :extra-connect-src (:connect-src integrations/csp-domains) :extra-frame-src (:frame-src integrations/csp-domains))) From cf8a6d750624a7d0db6c0365b35e6bf71e3720d3 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 01:24:59 +0000 Subject: [PATCH 09/35] Surface the effective config when the launcher starts A launcher is the one moment the operator is present and paying attention. It is the right place to state configuration, rather than leave it to be discovered through a symptom later. start.sh now reports, once, before any target runs: - whether config came from .env or from built-in defaults - the four ports (a busy one used to be reported as free on Windows) - the CSP policy and DEV_MODE and, on a first run with no .env, offers to create one from .env.example. The offer is deliberately narrow: only when no .env exists, only when .env.example does, and only with someone at the keyboard to answer. A non-interactive run is told `cp .env.example .env` and carries on, so CI and background starts are never blocked. Default is No. An existing .env is never touched. Replaces report_csp_mode from the previous commit, which only covered CSP and only ran for the server target; this runs for every target, since ports matter to all of them. Verified: no .env + DEV_MODE unset warns and explains; DEV_MODE=true is one quiet line; an existing .env reports its source and drops the "how to configure" hint; QUIET=true emits nothing at all (log_info and log_warn already honour it). The offer was tested in both directions -- "y" writes the file and says to review SIGNATURE, "n" and an empty answer leave no .env, and an existing .env with other contents survives a "y". Also corrects a message that claimed a 30s timeout when read had actually hit EOF. The two are not distinguishable there, so it no longer asserts either. --- scripts/common.sh | 67 ++++++++++++++++++++++++++++++++++++----------- scripts/start.sh | 6 +++-- 2 files changed, 56 insertions(+), 17 deletions(-) diff --git a/scripts/common.sh b/scripts/common.sh index 4beec6de8..f75b26a68 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -31,34 +31,71 @@ if [[ -f "$REPO_ROOT/.env" ]]; then set +a fi -# Report which Content-Security-Policy the server will run under. +# Show the configuration this run will actually use, and where it came from. # -# CSP mode is invisible until something breaks, and what breaks first is -# Figwheel's websocket -- hot reload simply stops working, with the cause a -# response header nobody thought to look at. DEV_MODE defaults to false, so a -# checkout with no .env gets the enforcing policy without asking for it. +# A launcher is the one moment the operator is present and paying attention, so +# it is the right place to surface configuration rather than let it be +# discovered through a symptom. Two settings in particular fail silently: +# CSP_POLICY/DEV_MODE (blocks Figwheel's websocket with no visible cause) and +# the ports (a busy port used to be reported as free on Windows). # -# Quiet when the combination is fine; loud only when it will bite. -report_csp_mode() { +# Respects QUIET via log_info/log_warn. Never blocks a non-interactive run. +report_env_config() { + local env_file="$REPO_ROOT/.env" + local example="$REPO_ROOT/.env.example" + + if [[ -f "$env_file" ]]; then + log_info "Config: .env (edit it to change any of the below)" + else + log_info "Config: built-in defaults — no .env (see .env.example)" + fi + + log_info " ports server=$SERVER_PORT datomic=$DATOMIC_PORT figwheel=$FIGWHEEL_PORT nrepl=$NREPL_PORT" + local policy="${CSP_POLICY:-strict}" local dev="${DEV_MODE:-}" - # Match the server's own comparison: case-insensitive, exactly "true". local dev_on=false case "$(printf '%s' "$dev" | tr '[:upper:]' '[:lower:]')" in true) dev_on=true ;; esac if [[ "$policy" != "strict" || "$dev_on" == "true" ]]; then - log_info "CSP: policy=$policy, DEV_MODE=${dev:-}" + log_info " csp policy=$policy DEV_MODE=${dev:-}" elif [[ -n "$dev" ]]; then - # Explicitly set to something other than true -- a decision, not an + # Explicitly set to something other than true: a decision, not an # accident. State the consequence once, without the lecture. - log_info "CSP: strict and ENFORCING (DEV_MODE=$dev). Figwheel hot reload will be blocked." + log_info " csp strict, ENFORCING (DEV_MODE=$dev) — Figwheel hot reload will be blocked" else # Unset: nobody chose this, and the symptom is a dead hot-reload socket # with no visible cause. This is the case worth interrupting for. - log_warn "CSP: strict and ENFORCING (DEV_MODE is unset, which means false)." - log_warn " Figwheel's websocket (ws://localhost:$FIGWHEEL_PORT) is not in connect-src," - log_warn " so hot reload will be blocked. Set DEV_MODE=true in .env for development." - [[ -f "$REPO_ROOT/.env" ]] || log_warn " No .env found — start from .env.example." + log_warn " csp strict and ENFORCING (DEV_MODE unset, which means false)" + log_warn " ws://localhost:$FIGWHEEL_PORT is not in connect-src, so Figwheel" + log_warn " hot reload will be blocked. Set DEV_MODE=true in .env to develop." + fi + + # First run: offer to start a .env from the example. Only when there is + # nothing to lose -- no .env present -- and only with someone at the + # keyboard to answer. A non-interactive run is told where to look instead. + if [[ ! -f "$env_file" && -f "$example" ]]; then + if is_interactive; then + local reply="" + if read -t 30 -p "Create .env from .env.example now? [y/N] " -n 1 -r reply; then + echo + if [[ "$reply" =~ ^[Yy]$ ]]; then + if cp "$example" "$env_file"; then + log_info "Wrote $env_file — review it (SIGNATURE especially) before going further." + log_info "Re-run this script to pick it up; this run continues on defaults." + else + log_error "Could not write $env_file" + fi + fi + else + echo + # read returns non-zero for both a 30s timeout and EOF, and + # they are not distinguishable here -- so do not claim either. + log_info "No answer — continuing on defaults." + fi + else + log_info " To configure: cp .env.example .env" + fi fi } diff --git a/scripts/start.sh b/scripts/start.sh index a3a0daeb5..246b0ae56 100755 --- a/scripts/start.sh +++ b/scripts/start.sh @@ -405,8 +405,6 @@ start_server() { cd "$REPO_ROOT" - report_csp_mode - # Use headless mode if not running interactively (background/nohup) local rc=0 if [[ "$(repl_mode)" == "interactive" ]]; then @@ -765,6 +763,10 @@ main() { target="${positional[0]:-all}" + # Surface configuration once, before any target runs. Every path through + # this script benefits: ports matter to all of them, CSP to the server. + report_env_config + # Handle install flag (no prereq checks needed) if [[ "$do_install" == "true" ]]; then run_install From bcf85262d387d8bcde5fc78ee60bb604a8e6f297 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 01:32:23 +0000 Subject: [PATCH 10/35] Put config where it is read, and repair --check, which died silently Printing configuration at the top of a launcher does not work: it scrolls past before the REPL takes the terminal and nobody reads it. Attention exists in two places only -- at a prompt, and in a command whose output IS the product. The previous commit put it in neither. So: print_env_config -> run_checks (--check), where reporting is the point offer_env_file -> a first-run prompt, which is a genuine pause confirm_dev_mode -> a decision at figwheel start, where it bites confirm_dev_mode matters because starting Figwheel under an enforcing CSP produces a dev loop that looks fine and silently never reloads. That deserves a question, not a line of output. Non-interactive runs warn and continue, so CI is never blocked. The first-run prompt is now a short guided setup: create .env, choose development or production (sets DEV_MODE), and optionally replace the three change-me credentials with random values. SIGNATURE, ADMIN_PASSWORD and DATOMIC_PASSWORD otherwise ship as published strings. The file is written 0600 and the generated values are never echoed, since printing them would put credentials in scrollback. --- and while verifying the above, --check turned out to be broken --- `bash scripts/start.sh --check server` exited 0 after printing "Java (11+): " and nothing else. Two independent defects: 1. check_java ran `java -version | head -1`, assuming the version is on line 1. With JAVA_TOOL_OPTIONS or _JAVA_OPTIONS set -- normal behind a proxy and in CI images -- line 1 is "Picked up JAVA_TOOL_OPTIONS: ...". The parse then yielded that whole string, and `[[ "$str" -lt 11 ]]` evaluates its operand as ARITHMETIC, so the bare word "Picked" was read as a variable name. Under `set -u` an unset name is FATAL, not false, so the script died -- invisibly, because callers use `check_java 2>/dev/null`. Now it finds the version line wherever it is and refuses to compare anything non-numeric, reporting what it actually saw. 2. `((failed++))` returns 1 when the counter was 0, and `set -e` kills the script on the FIRST failed check. run_checks is called bare, so this was live. All bare counter increments are now `x=$((x + 1))`: five in start.sh, three in common.sh, one in stop.sh. The ones in common.sh were latent only because wait_for_datomic is always called as a condition, which suspends set -e for the whole function body. Note the asymmetry that hid defect 1: `set -u` is fatal even inside an `if` condition, while `set -e` is suspended there. That is why check_java died where the counters did not. --check now runs to completion: Java 21 detected, Leiningen correctly reported missing, ports probed, config summarised, exit 2 with a count. --- scripts/common.sh | 221 ++++++++++++++++++++++++++++++++++------------ scripts/start.sh | 20 +++-- scripts/stop.sh | 2 +- 3 files changed, 176 insertions(+), 67 deletions(-) diff --git a/scripts/common.sh b/scripts/common.sh index f75b26a68..00ab97a63 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -31,72 +31,164 @@ if [[ -f "$REPO_ROOT/.env" ]]; then set +a fi -# Show the configuration this run will actually use, and where it came from. +# Configuration reporting and the DEV_MODE decision. # -# A launcher is the one moment the operator is present and paying attention, so -# it is the right place to surface configuration rather than let it be -# discovered through a symptom. Two settings in particular fail silently: -# CSP_POLICY/DEV_MODE (blocks Figwheel's websocket with no visible cause) and -# the ports (a busy port used to be reported as free on Windows). +# Deliberately NOT printed at the top of every run. Output at the start of a +# script scrolls past before the REPL takes the terminal, and nobody reads it. +# Attention exists in two places only: at a prompt, and in a command whose +# output IS the product. So: # -# Respects QUIET via log_info/log_warn. Never blocks a non-interactive run. -report_env_config() { - local env_file="$REPO_ROOT/.env" - local example="$REPO_ROOT/.env.example" - - if [[ -f "$env_file" ]]; then - log_info "Config: .env (edit it to change any of the below)" +# print_env_config -> called from run_checks (--check), where it is the point +# offer_env_file -> a prompt, which is a genuine pause +# confirm_dev_mode -> a decision, raised at figwheel start where it bites + +# The settings that fail silently: ports (a busy one used to read as free on +# Windows) and CSP/DEV_MODE (blocks Figwheel's socket with no visible cause). +print_env_config() { + if [[ -f "$REPO_ROOT/.env" ]]; then + echo -e "Config: ${GREEN}.env${NC} (edit it to change any of the below)" else - log_info "Config: built-in defaults — no .env (see .env.example)" + echo -e "Config: ${YELLOW}built-in defaults${NC} (no .env — see .env.example)" fi + echo " ports server=$SERVER_PORT datomic=$DATOMIC_PORT figwheel=$FIGWHEEL_PORT nrepl=$NREPL_PORT" - log_info " ports server=$SERVER_PORT datomic=$DATOMIC_PORT figwheel=$FIGWHEEL_PORT nrepl=$NREPL_PORT" + local policy="${CSP_POLICY:-strict}" + local dev="${DEV_MODE:-}" + if dev_mode_blocks_figwheel; then + echo -e " csp ${YELLOW}$policy, ENFORCING${NC} (DEV_MODE=$dev) — Figwheel hot reload blocked" + else + echo " csp policy=$policy DEV_MODE=$dev" + fi +} +# True when the server will send an enforcing CSP whose connect-src omits the +# Figwheel websocket. Matches the server's own comparison: case-insensitive, +# exactly "true". +dev_mode_blocks_figwheel() { local policy="${CSP_POLICY:-strict}" - local dev="${DEV_MODE:-}" - local dev_on=false - case "$(printf '%s' "$dev" | tr '[:upper:]' '[:lower:]')" in true) dev_on=true ;; esac - - if [[ "$policy" != "strict" || "$dev_on" == "true" ]]; then - log_info " csp policy=$policy DEV_MODE=${dev:-}" - elif [[ -n "$dev" ]]; then - # Explicitly set to something other than true: a decision, not an - # accident. State the consequence once, without the lecture. - log_info " csp strict, ENFORCING (DEV_MODE=$dev) — Figwheel hot reload will be blocked" + [[ "$policy" == "strict" ]] || return 1 + case "$(printf '%s' "${DEV_MODE:-}" | tr '[:upper:]' '[:lower:]')" in + true) return 1 ;; + *) return 0 ;; + esac +} + +# Generate a random secret. Hex only, so it is safe to substitute into a file +# without quoting concerns. Empty string if no source is available. +random_secret() { + if command -v openssl >/dev/null 2>&1; then + openssl rand -hex 24 2>/dev/null && return 0 + fi + if [[ -r /dev/urandom ]] && command -v od >/dev/null 2>&1; then + od -An -tx1 -N24 /dev/urandom 2>/dev/null | tr -d ' \n' && return 0 + fi + printf '' +} + +# Replace KEY=... in a file, portably. sed -i differs between GNU and BSD, so +# write to a temp file and move it into place instead. +set_env_value() { + local file="$1" key="$2" value="$3" tmp + tmp="$(mktemp)" || return 1 + awk -v k="$key" -v v="$value" ' + $0 ~ "^" k "=" { print k "=" v; next } + { print } + ' "$file" > "$tmp" && mv "$tmp" "$file" +} + +# First run: a short guided setup. A prompt is one of the two moments an +# operator is actually reading, so this is where configuration is worth +# raising -- and while they are here, the three change-me placeholders are +# worth resolving, because each is a credential that otherwise ships as a +# known string. +# +# Only when there is nothing to lose (no .env present) and someone is at the +# keyboard. A non-interactive run is told what to do and never blocked. +offer_env_file() { + local env_file="$REPO_ROOT/.env" example="$REPO_ROOT/.env.example" + [[ -f "$env_file" || ! -f "$example" ]] && return 0 + + if ! is_interactive; then + log_info "No .env — using built-in defaults. To configure: cp .env.example .env" + return 0 + fi + + echo "" + log_warn "No .env found. Defaults will be used, including CSP_POLICY=strict with" + log_warn "DEV_MODE unset, which silently blocks Figwheel's hot reload." + + local reply="" + read -t 30 -p "Create .env from .env.example now? [y/N] " -n 1 -r reply || { echo; log_info "No answer — continuing on defaults."; return 0; } + echo + [[ "$reply" =~ ^[Yy]$ ]] || { log_info "Skipped. Continuing on defaults."; return 0; } + + cp "$example" "$env_file" || { log_error "Could not write $env_file"; return 0; } + chmod 600 "$env_file" 2>/dev/null || true + + # --- development or production ------------------------------------------- + local mode="" + read -t 30 -p "Set up for [d]evelopment or [p]roduction? [D/p] " -n 1 -r mode || mode="" + echo + if [[ "$mode" =~ ^[Pp]$ ]]; then + set_env_value "$env_file" DEV_MODE false + log_info " DEV_MODE=false — CSP enforcing. Figwheel hot reload will not work." else - # Unset: nobody chose this, and the symptom is a dead hot-reload socket - # with no visible cause. This is the case worth interrupting for. - log_warn " csp strict and ENFORCING (DEV_MODE unset, which means false)" - log_warn " ws://localhost:$FIGWHEEL_PORT is not in connect-src, so Figwheel" - log_warn " hot reload will be blocked. Set DEV_MODE=true in .env to develop." + set_env_value "$env_file" DEV_MODE true + log_info " DEV_MODE=true — no CSP header, so Figwheel works." fi - # First run: offer to start a .env from the example. Only when there is - # nothing to lose -- no .env present -- and only with someone at the - # keyboard to answer. A non-interactive run is told where to look instead. - if [[ ! -f "$env_file" && -f "$example" ]]; then - if is_interactive; then - local reply="" - if read -t 30 -p "Create .env from .env.example now? [y/N] " -n 1 -r reply; then - echo - if [[ "$reply" =~ ^[Yy]$ ]]; then - if cp "$example" "$env_file"; then - log_info "Wrote $env_file — review it (SIGNATURE especially) before going further." - log_info "Re-run this script to pick it up; this run continues on defaults." - else - log_error "Could not write $env_file" - fi - fi + # --- the three change-me credentials ------------------------------------- + local gen="" + read -t 30 -p "Generate random values for the change-me passwords/secret? [Y/n] " -n 1 -r gen || gen="" + echo + if [[ "$gen" =~ ^[Nn]$ ]]; then + log_warn " Left as-is. SIGNATURE, ADMIN_PASSWORD and DATOMIC_PASSWORD are" + log_warn " published placeholders — change them before exposing this server." + else + local key secret failed=0 + for key in SIGNATURE ADMIN_PASSWORD DATOMIC_PASSWORD; do + secret="$(random_secret)" + if [[ -n "$secret" ]]; then + set_env_value "$env_file" "$key" "$secret" else - echo - # read returns non-zero for both a 30s timeout and EOF, and - # they are not distinguishable here -- so do not claim either. - log_info "No answer — continuing on defaults." + failed=1 fi + done + if [[ $failed -eq 0 ]]; then + # Deliberately not echoed. They are in the file; printing them puts + # them in scrollback and shell history exports. + log_info " SIGNATURE, ADMIN_PASSWORD, DATOMIC_PASSWORD set to random values." else - log_info " To configure: cp .env.example .env" + log_warn " No random source (openssl / /dev/urandom) — placeholders left in place." fi fi + + log_info "Wrote $env_file (mode 600). Review it, then re-run to pick it up;" + log_info "this run continues on the values it already loaded." +} + +# Raised where it actually bites. Starting Figwheel with an enforcing CSP gives +# a dev loop that looks fine and silently never reloads, so this is a decision, +# not a line of output to scroll past. +confirm_dev_mode() { + dev_mode_blocks_figwheel || return 0 + + log_warn "CSP is strict and ENFORCING (DEV_MODE=${DEV_MODE:-})." + log_warn "ws://localhost:$FIGWHEEL_PORT is not in connect-src, so hot reload" + log_warn "will silently not work. Set DEV_MODE=true in .env to develop." + + is_interactive || { log_warn "Continuing anyway (non-interactive)."; return 0; } + + local reply="" + if read -t 30 -p "Start Figwheel anyway? [y/N] " -n 1 -r reply; then + echo + [[ "$reply" =~ ^[Yy]$ ]] && return 0 + log_info "Aborted. Set DEV_MODE=true in .env, then re-run." + return 1 + fi + echo + log_info "No answer — not starting Figwheel." + return 1 } # Defaults (used if not set in .env) @@ -252,7 +344,7 @@ wait_for_port() { return 0 fi sleep 1 - ((elapsed++)) + elapsed=$((elapsed + 1)) done return 1 } @@ -276,7 +368,7 @@ wait_for_port_or_die() { return 0 fi sleep 1 - ((elapsed++)) + elapsed=$((elapsed + 1)) done log_error "Timeout waiting for port $port (process $pid still running)" return 1 @@ -293,7 +385,7 @@ wait_for_port_free() { return 0 fi sleep 1 - ((elapsed++)) + elapsed=$((elapsed + 1)) done return 1 } @@ -367,14 +459,27 @@ get_uptime() { # ----------------------------------------------------------------------------- check_java() { - local java_version - java_version=$(java -version 2>&1 | head -1 | sed -E 's/.*"([0-9]+).*/\1/') - - if [[ -z "$java_version" ]]; then + local raw java_version + if ! raw="$(java -version 2>&1)"; then log_error "Java not found. Please install Java $JAVA_MIN_VERSION or higher." return 1 fi + # Find the version line wherever it is, rather than assuming line 1. + # JAVA_TOOL_OPTIONS and _JAVA_OPTIONS make the JVM print a "Picked up ..." + # preamble first, which is common behind a proxy and in CI images. + java_version="$(printf '%s\n' "$raw" | sed -nE 's/.*version "([0-9]+).*/\1/p' | head -n1)" + + # Guard the comparison below. [[ str -lt n ]] evaluates str as ARITHMETIC, + # so a non-numeric value is read as a variable name -- and under `set -u` + # an unset name is a FATAL error, not a false comparison. That killed this + # script outright, and silently, because callers use `check_java 2>/dev/null`. + if [[ ! "$java_version" =~ ^[0-9]+$ ]]; then + log_error "Could not read a Java version. First line of 'java -version':" + log_error " $(printf '%s\n' "$raw" | head -n1)" + return 1 + fi + if [[ "$java_version" -lt "$JAVA_MIN_VERSION" ]]; then log_error "Java $JAVA_MIN_VERSION+ required (found Java $java_version)." log_info "Use the devcontainer or install a compatible JDK." diff --git a/scripts/start.sh b/scripts/start.sh index 246b0ae56..4b1554426 100755 --- a/scripts/start.sh +++ b/scripts/start.sh @@ -174,7 +174,7 @@ run_checks() { echo -e "${GREEN}OK${NC}" else echo -e "${RED}FAILED${NC}" - ((failed++)) + failed=$((failed + 1)) fi echo -n "Leiningen: " @@ -182,7 +182,7 @@ run_checks() { echo -e "${GREEN}OK${NC}" else echo -e "${RED}FAILED${NC}" - ((failed++)) + failed=$((failed + 1)) fi # Target-specific checks @@ -193,7 +193,7 @@ run_checks() { echo -e "${GREEN}OK${NC}" else echo -e "${RED}FAILED${NC}" - ((failed++)) + failed=$((failed + 1)) fi echo -n "Datomic config: " @@ -209,7 +209,7 @@ run_checks() { fi else echo -e "${RED}FAILED${NC} (no config or template)" - ((failed++)) + failed=$((failed + 1)) fi echo -n "Datomic port ($DATOMIC_PORT): " @@ -265,6 +265,9 @@ run_checks() { ;; esac + echo "" + print_env_config + echo "" if [[ $failed -gt 0 ]]; then log_error "$failed prerequisite check(s) failed" @@ -438,6 +441,8 @@ start_figwheel() { # Clean up stale PID file cleanup_stale_pid "figwheel" + confirm_dev_mode || exit $EXIT_SUCCESS + # ── Remote dev environment detection ────────────────────────────── # Figwheel's default connect URL (ws://localhost:PORT) only works when # the browser is on the same machine. In remote environments (Codespaces, @@ -525,7 +530,7 @@ start_garden() { show_startup_failure "garden" "$LOG_DIR/garden.log" "" exit $EXIT_RUNTIME fi - ((checks++)) + checks=$((checks + 1)) done log_info "Garden is running" } @@ -763,9 +768,8 @@ main() { target="${positional[0]:-all}" - # Surface configuration once, before any target runs. Every path through - # this script benefits: ports matter to all of them, CSP to the server. - report_env_config + # A prompt is read; a banner is not. Only the first-run offer goes here. + offer_env_file # Handle install flag (no prereq checks needed) if [[ "$do_install" == "true" ]]; then diff --git a/scripts/stop.sh b/scripts/stop.sh index f1845feac..c8ddae26a 100755 --- a/scripts/stop.sh +++ b/scripts/stop.sh @@ -40,7 +40,7 @@ show_status() { # Quiet mode: just exit codes local running=0 for port in "$DATOMIC_PORT" "$SERVER_PORT" "$NREPL_PORT"; do - port_in_use "$port" && ((running++)) + port_in_use "$port" && running=$((running + 1)) done echo "$running" return From 402ea5e528d552a179ca4429673eb7e8c107f48d Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 02:33:00 +0000 Subject: [PATCH 11/35] Add tests that pin both defects, and prove they fail without the fix The suite had no config-test or pedestal-test, so a green run showed only that nothing broke -- it could not show the fix works. These pin the two defects directly. config-test runs under a Turkish locale and asserts CSP_POLICY=STRICT still resolves to strict, and that DEV_MODE matching is case-insensitive either way. pedestal-test asserts the ETag interceptor returns the CONTEXT when it throws, keeping status and body intact -- that is the defect that turned a logged exception into a bare 200 with nothing in it. It also pins the exact ETag value (1594046507000-58935), because changing it would invalidate every cached ETag in the wild. Verified to discriminate, not merely pass. Against upstream's unfixed files these produce 5 failures; with the fix, 0. One test does NOT discriminate, and says so rather than pretending: rfc822-formatter is a top-level def, so its locale is fixed when the namespace loads, before any test can call Locale/setDefault. A unit test in this JVM cannot reproduce the original parse failure, and one claiming to would pass with or without the fix. It is replaced by a test that demonstrates the hazard on a locally-built unlocalised formatter, so nobody simplifies Locale/ENGLISH back out. The real guard is running the suite under a non-English JVM. Suite on this branch: 220 tests, 985 assertions, 0 failures -- green at the default locale and under es_ES. --- test/clj/orcpub/config_test.clj | 56 ++++++++++++++++ test/clj/orcpub/pedestal_test.clj | 106 ++++++++++++++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 test/clj/orcpub/config_test.clj create mode 100644 test/clj/orcpub/pedestal_test.clj diff --git a/test/clj/orcpub/config_test.clj b/test/clj/orcpub/config_test.clj new file mode 100644 index 000000000..ca579574e --- /dev/null +++ b/test/clj/orcpub/config_test.clj @@ -0,0 +1,56 @@ +(ns orcpub.config-test + "Pins the locale-independence of the CSP config tokens. + + CSP_POLICY and DEV_MODE are ASCII protocol tokens, not prose. Folding their + case with the default locale means a Turkish machine reads STRICT as + \"strıct\" (dotless i), matches no branch, and silently falls through to the + PERMISSIVE policy -- a security downgrade nobody asked for and nothing + reports. These tests run under a Turkish locale on purpose." + (:require [clojure.test :refer [deftest testing is use-fixtures]] + [orcpub.config :as config]) + (:import [java.util Locale])) + +(def ^:private turkish (Locale/forLanguageTag "tr-TR")) + +(defn- with-locale + "Run f with the JVM default locale set to loc, then restore it. Restoring + matters: a leaked default would silently change how every later test in the + same JVM folds case." + [^Locale loc f] + (let [saved (Locale/getDefault)] + (try (Locale/setDefault loc) (f) + (finally (Locale/setDefault saved))))) + +(use-fixtures :once (fn [t] (let [saved (Locale/getDefault)] + (try (t) (finally (Locale/setDefault saved)))))) + +(deftest turkish-locale-does-not-break-csp-policy + (testing "an uppercase CSP_POLICY still resolves to strict under tr-TR" + (with-locale turkish + (fn [] + (with-redefs [environ.core/env {:csp-policy "STRICT"}] + (is (= "strict" (config/get-csp-policy)) + "STRICT lowercased with the Turkish locale yields \"strıct\" (dotless i)") + (is (true? (config/strict-csp?)) + "a mis-folded token falls through to the PERMISSIVE policy")))))) + +(deftest turkish-locale-does-not-break-dev-mode + (testing "DEV_MODE comparison is case-insensitive and locale-independent" + (with-locale turkish + (fn [] + (doseq [v ["true" "TRUE" "True"]] + (with-redefs [environ.core/env {:dev-mode v}] + (is (true? (config/dev-mode?)) (str "DEV_MODE=" v " should be true")))) + (doseq [v ["false" "FALSE" "" "yes" "1"]] + (with-redefs [environ.core/env {:dev-mode v}] + (is (false? (config/dev-mode?)) (str "DEV_MODE=" v " should be false")))))))) + +(deftest dev-mode-is-false-when-unset + (testing "an unset DEV_MODE is false, and does not throw" + (with-redefs [environ.core/env {}] + (is (false? (config/dev-mode?)))))) + +(deftest csp-policy-defaults-to-strict + (testing "no CSP_POLICY set means strict" + (with-redefs [environ.core/env {}] + (is (= "strict" (config/get-csp-policy)))))) diff --git a/test/clj/orcpub/pedestal_test.clj b/test/clj/orcpub/pedestal_test.clj new file mode 100644 index 000000000..8a2d798ed --- /dev/null +++ b/test/clj/orcpub/pedestal_test.clj @@ -0,0 +1,106 @@ +(ns orcpub.pedestal-test + "Pins the two defects that made every /assets/* request return 200 with an + empty body on a non-English machine. + + 1. parse-date must read the English HTTP date regardless of JVM locale. + HTTP dates are always English (RFC 7231); an unlocalised + DateTimeFormatter parses them with the default locale, so \"Mon\" is not + a day name on a Spanish machine and it throws. + + 2. The ETag interceptor's catch must return the CONTEXT. Returning + log/error's value instead discards the response: Pedestal then has + nothing to write and Jetty emits a bare 200. That is what made defect 1 + silent -- the exception was logged, but the response was already gone." + (:require [clojure.test :refer [deftest testing is use-fixtures]] + [orcpub.pedestal :as pedestal]) + (:import [java.time ZonedDateTime] + [java.time.format DateTimeFormatter] + [java.util Locale])) + +(def ^:private hostile-locales + [(Locale/forLanguageTag "es-ES") + (Locale/forLanguageTag "de-DE") + (Locale/forLanguageTag "tr-TR") + (Locale/forLanguageTag "ja-JP")]) + +;; A real Last-Modified from the Font Awesome webjar, and the ETag the English +;; locale has always produced for it. Pinning the exact value matters: if it +;; ever changes, every cached ETag in the wild is invalidated. +(def ^:private sample-date "Mon, 06 Jul 2020 14:41:47 GMT") +(def ^:private expected-etag "1594046507000-58935") + +(use-fixtures :once (fn [t] (let [saved (Locale/getDefault)] + (try (t) (finally (Locale/setDefault saved)))))) + +(deftest parse-date-produces-the-expected-etag + (testing "the value is exactly what English-locale servers have always produced" + ;; A value regression guard, NOT a guard against the locale defect. See + ;; the next test for why an in-process test cannot catch that one. + (is (= expected-etag (pedestal/parse-date sample-date 58935))))) + +(deftest the-locale-hazard-this-fix-removes + (testing "an unlocalised formatter really does throw on a valid HTTP date" + ;; Why this is demonstrated rather than asserted against rfc822-formatter: + ;; that formatter is a top-level def, so its locale is fixed when the + ;; namespace LOADS -- before any test can call Locale/setDefault. A unit + ;; test in this JVM therefore cannot reproduce the original failure, and a + ;; test claiming to would pass whether or not the fix were present. + ;; + ;; The real guard is running this suite under a non-English JVM: + ;; lein test with -Duser.language=es -Duser.country=ES + ;; which is green. What this test pins is that the HAZARD is real, so + ;; nobody "simplifies" Locale/ENGLISH back out of the formatter. + (let [saved (Locale/getDefault)] + (try + (Locale/setDefault (Locale/forLanguageTag "es-ES")) + (let [unlocalised (DateTimeFormatter/ofPattern "EEE, dd MMM yyyy HH:mm:ss Z") + localised (DateTimeFormatter/ofPattern "EEE, dd MMM yyyy HH:mm:ss Z" Locale/ENGLISH) + text "Mon, 06 Jul 2020 14:41:47 +0000"] + (is (thrown? Exception (ZonedDateTime/parse text unlocalised)) + "if this stops throwing, the hazard is gone and this test can go") + (is (some? (ZonedDateTime/parse text localised)) + "pinning the locale is what makes it parse")) + (finally (Locale/setDefault saved)))))) + +(deftest parse-date-passes-through-nil + (testing "no Last-Modified header means no etag, not an exception" + (is (nil? (pedestal/parse-date nil 123))))) + +(defn- leave [ctx] + ((:leave pedestal/etag-interceptor) ctx)) + +(deftest etag-interceptor-returns-context-when-it-throws + (testing "a response is never discarded, whatever the interceptor hits" + ;; An unparseable Last-Modified makes parse-date throw, which is exactly + ;; what a non-English locale used to do with a perfectly valid header. + (let [ctx {:request {:headers {}} + :response {:status 200 + :body "hello" + :headers {"Last-Modified" "not a date at all" + "Content-Length" "5"}}} + out (leave ctx)] + (is (map? out) "the catch must return the context, not log/error's value") + (is (= 200 (get-in out [:response :status]))) + (is (= "hello" (get-in out [:response :body])) + "the body survived: discarding it is what produced an empty 200")))) + +(deftest etag-interceptor-sets-an-etag-on-a-good-response + (testing "the normal path still works" + (let [ctx {:request {:headers {}} + :response {:status 200 + :body "hello" + :headers {"Last-Modified" sample-date + "Content-Length" "58935"}}} + out (leave ctx)] + (is (= expected-etag (get-in out [:response :headers "etag"])))))) + +(deftest etag-interceptor-honours-if-none-match + (testing "a matching if-none-match yields 304 with no body" + (let [ctx {:request {:headers {"if-none-match" expected-etag}} + :response {:status 200 + :body "hello" + :headers {"Last-Modified" sample-date + "Content-Length" "58935"}}} + out (leave ctx)] + (is (= 304 (get-in out [:response :status]))) + (is (nil? (get-in out [:response :body])))))) From b86d3d1287228edf586c58922e6ad21f6f0cdd56 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 02:41:34 +0000 Subject: [PATCH 12/35] Pin the locale on name-to-kw and the two name filters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Running the suite under a Turkish JVM -- something worth doing on a branch about locale -- produced three failures that had nothing to do with this branch's changes and everything to do with its subject. The serious one: name-to-kw builds a keyword from a content name by lowercasing it. clojure.string/lower-case takes no Locale on the JVM, so on a Turkish or Azerbaijani machine "Illusory Script" becomes :ıllusory-script (dotless i) rather than :illusory-script. That is a different key for the same content, on the server only, which is why warlock-test could not find an invocation spell. Content keys that differ by operating-system language are a data problem, not a display one. The other two were name searches: "GIM" finds no Gimli, "FIRE" finds neither Fireball nor Fire Bolt, because the query folds to "gım" and "fıre". Adds orcpub.common/ascii-lower-case and uses it at the four sites that fold machine tokens: both name-to-kw implementations (common.cljc and entity.cljc), the character name filter, and the spell name filter. aloof-sort-by too, so a list does not reorder itself by server locale. ClojureScript needed no guard and gets none: JS toLowerCase() is already locale-invariant -- toLocaleLowerCase() is the one that is not -- so the browser was always correct and only the JVM side diverged. The reader conditional says so at the definition. Narrow but real: only Turkish and Azerbaijani fold ASCII differently, which is why es_ES was green throughout and tr_TR was not. The stronger reason to fix it is that a permanently red locale meant the matrix could never be used as a gate. Suite now: 220 tests, 985 assertions, 0 failures under tr_TR, es_ES and en_US alike. Before this commit, tr_TR had 3 failures. --- src/cljc/orcpub/common.cljc | 27 ++++++++++++++++++++++--- src/cljc/orcpub/dnd/e5/char_filter.cljc | 7 +++++-- src/cljc/orcpub/dnd/e5/compute.cljc | 6 ++++-- src/cljc/orcpub/entity.cljc | 7 +++++-- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/cljc/orcpub/common.cljc b/src/cljc/orcpub/common.cljc index 572a0ff6d..6660ddc84 100644 --- a/src/cljc/orcpub/common.cljc +++ b/src/cljc/orcpub/common.cljc @@ -5,10 +5,30 @@ (def dot-char "•") +(defn ascii-lower-case + "Lowercase without asking the operating system what language it is in. + + clojure.string/lower-case calls .toLowerCase() with no Locale on the JVM, + which follows the JVM default. On a Turkish or Azerbaijani machine that + folds \"I\" to the DOTLESS \"ı\", so \"Illusory Script\" becomes + :ıllusory-script instead of :illusory-script -- a different key, on the + server only, for the same content. Names, keys and search terms are + machine tokens here, not prose. + + ClojureScript needs no guard: JS toLowerCase() is already locale-invariant + (toLocaleLowerCase() is the one that is not), so the browser was always + correct and only the JVM side diverged. + + See docs/kb/locale-safety.md." + [x] + (let [t (str x)] + #?(:clj (.toLowerCase ^String t java.util.Locale/ROOT) + :cljs (.toLowerCase t)))) + (defn- name-to-kw-aux [name ns] (when (string? name) (as-> name $ - (s/lower-case $) + (ascii-lower-case $) (s/replace $ #"'" "") (s/replace $ #"\W" "-") (s/replace $ #"\-+" "-") @@ -204,8 +224,9 @@ ;; Case Insensitive `sort-by` (defn aloof-sort-by [sorter coll] - (sort-by (comp s/lower-case sorter) coll) - ) + ;; ascii-lower-case, so a list does not reorder itself depending on the + ;; server's locale. + (sort-by (comp ascii-lower-case sorter) coll)) (defn ->kebab-case [s] (-> s diff --git a/src/cljc/orcpub/dnd/e5/char_filter.cljc b/src/cljc/orcpub/dnd/e5/char_filter.cljc index bf73eaadd..7b1761321 100644 --- a/src/cljc/orcpub/dnd/e5/char_filter.cljc +++ b/src/cljc/orcpub/dnd/e5/char_filter.cljc @@ -1,5 +1,6 @@ (ns orcpub.dnd.e5.char-filter (:require [clojure.string :as s] + [orcpub.common :as common] [orcpub.dnd.e5.character :as char5e])) (defn char-matches? @@ -14,8 +15,10 @@ [char name-filter level-filters class-filters has-portrait? has-faction-pic?] (and (or (s/blank? name-filter) - (s/includes? (s/lower-case (or (::char5e/character-name char) "")) - (s/lower-case name-filter))) + ;; ascii-lower-case: on a Turkish JVM "GIM" folds to "gım" and matches + ;; no character called Gimli. + (s/includes? (common/ascii-lower-case (or (::char5e/character-name char) "")) + (common/ascii-lower-case name-filter))) (or (empty? level-filters) (some #(level-filters (::char5e/level %)) (::char5e/classes char))) (or (empty? class-filters) diff --git a/src/cljc/orcpub/dnd/e5/compute.cljc b/src/cljc/orcpub/dnd/e5/compute.cljc index 7aa8fc083..808c35085 100644 --- a/src/cljc/orcpub/dnd/e5/compute.cljc +++ b/src/cljc/orcpub/dnd/e5/compute.cljc @@ -64,10 +64,12 @@ (defn filter-by-name-xform "Returns a transducer that filters items by name matching filter-text." [filter-text name-key] - (let [pattern (re-pattern (str ".*" (s/lower-case filter-text) ".*"))] + ;; ascii-lower-case: on a Turkish JVM "FIRE" folds to "fıre" and matches + ;; neither Fireball nor Fire Bolt. + (let [pattern (re-pattern (str ".*" (common/ascii-lower-case filter-text) ".*"))] (filter (fn [x] - (re-matches pattern (s/lower-case (name-key x))))))) + (re-matches pattern (common/ascii-lower-case (name-key x))))))) (defn filter-spells "Filters and sorts spells whose :name matches filter-text." diff --git a/src/cljc/orcpub/entity.cljc b/src/cljc/orcpub/entity.cljc index 9412a8fa0..a6f065514 100644 --- a/src/cljc/orcpub/entity.cljc +++ b/src/cljc/orcpub/entity.cljc @@ -698,9 +698,12 @@ :args (spec/cat :raw-entity ::raw-entity :modifier-map ::t/template) :ret any?) -(defn name-to-kw [name] +(defn name-to-kw + "Second implementation of name->keyword, alongside orcpub.common/name-to-kw. + Locale-pinned for the same reason: see orcpub.common/ascii-lower-case." + [name] (-> name - s/lower-case + common/ascii-lower-case (s/replace #"\W" "-") keyword)) From 55adc9eca6951295cd77a4bd4194394130539ba9 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 16:25:53 +0000 Subject: [PATCH 13/35] Address the review on #695: six findings, each reproduced first Copilot raised seven findings. The comment bodies are not reachable from this environment, but the titles are, and each one named something specific enough to check against the code. All six actionable ones reproduced. (1) Port diagnostics ignore the configured PORT. .env.example documents PORT and the server honours it (system.clj, System/getenv "PORT"), but common.sh used SERVER_PORT alone. Set PORT=9000 and the server moved while every port check, explain_bind_failure and the config report still looked at 8890. SERVER_PORT now falls back to PORT before the literal, so an explicit SERVER_PORT still wins. (2) Figwheel CSP predicate misclassifies policy branches. permissive-csp-settings sets default-src 'self' and NO connect-src, so connect-src falls back and ws://localhost:3449 is blocked exactly as under strict. dev_mode_blocks_figwheel only warned for strict. Now strict and permissive both warn; only "none" is silent. (3) Env template overrides the Datomic URL with a Docker hostname. .env.example ships datomic:dev://datomic:4334/orcpub -- "datomic" is the compose SERVICE name and resolves nowhere else -- which overrides config.clj's localhost default. This mattered more after the first-run prompt added here started encouraging people to copy the template. The line now carries the localhost alternative and says the default is already correct if left commented. (4) Windows port parser misidentifies non-listening sockets. The awk matched the local-address column in ANY state, so a TIME_WAIT from a connection that closed seconds ago read as "in use" and start.sh refused to start. Listening rows are now identified by the WILDCARD FOREIGN ADDRESS rather than the state word, because the state word is localised and the foreign address is not -- same reasoning that made the original parser use column positions. (5) Help command performs environment setup side effects. --help was already safe (it exits during argument parsing), but --check was not: a read-only status command offered to write a .env. It no longer prompts. (7) CSP documentation incorrectly describes DEV_MODE. pedestal.clj still said "In prod (dev-mode?=false)", conflating production with the DEFAULT. That conflation is the whole trap: DEV_MODE is false unless set, so an ordinary checkout gets the enforcing path. (6) CI does not test hostile locales -- the one I had written down and not acted on. Adds .github/workflows/locale.yml running the JVM suite under tr_TR and es_ES. Split into its own workflow rather than bolted onto windows-scripts.yml, which is about Windows. tr_TR is the high-value entry: Turkish and Azerbaijani are the only locales whose ASCII case folding differs. es_ES catches date parsing, which tr_TR also catches, but it is where the original report came from so it stays as a named guard. Verified: PORT/SERVER_PORT precedence across three combinations; permissive now warns and none stays silent; a TIME_WAIT row reads free while a LISTENING row reads in use; lein test 220/985/0 under tr_TR and en_US; both workflow files parse; shellcheck finds nothing new. --- .env.example | 5 +++ .github/workflows/locale.yml | 59 ++++++++++++++++++++++++++++++++++++ scripts/common.sh | 25 ++++++++++++--- scripts/start.sh | 6 ++-- src/clj/orcpub/pedestal.clj | 3 +- 5 files changed, 91 insertions(+), 7 deletions(-) create mode 100644 .github/workflows/locale.yml diff --git a/.env.example b/.env.example index 4c98ffe0c..bf65d2a17 100644 --- a/.env.example +++ b/.env.example @@ -28,6 +28,11 @@ TZ=America/Chicago # Old URLs with ?password= still work — the embedded password takes priority. ADMIN_PASSWORD=change-me-admin DATOMIC_PASSWORD=change-me-datomic +# Hostname "datomic" is the docker-compose SERVICE name and only resolves +# inside that network. Running outside Docker? Use localhost instead: +# DATOMIC_URL=datomic:dev://localhost:4334/orcpub +# Leaving this line commented out is also fine -- the application already +# defaults to localhost (config.clj/default-datomic-uri). DATOMIC_URL=datomic:dev://datomic:4334/orcpub # --- Transactor Tuning --- diff --git a/.github/workflows/locale.yml b/.github/workflows/locale.yml new file mode 100644 index 000000000..6dbc56f6f --- /dev/null +++ b/.github/workflows/locale.yml @@ -0,0 +1,59 @@ +# Runs the JVM suite under non-English locales. +# +# Everything the locale-safety work fixed was invisible to an English-locale CI: +# an unlocalised date formatter that blanked every webjar asset, CSP_POLICY +# folding to "strıct", name-to-kw producing :ıllusory-script, and two name +# searches returning nothing for an uppercase query. None of them fail under +# en_US, so en_US alone can never catch the next one. +# +# tr_TR is the high-value entry: Turkish and Azerbaijani are the only locales +# whose ASCII case folding differs, so they catch that whole class. es_ES +# catches date parsing, which tr_TR also catches -- one non-English entry would +# do, but es_ES is where the original bug report came from, so it stays as a +# named regression guard. + +name: Locale + +on: + push: + branches: [hotfix/locale-safety, develop, main] + pull_request: + branches: [develop, main] + workflow_dispatch: + +permissions: + contents: read + +jobs: + jvm-suite: + name: JVM suite under ${{ matrix.locale }} + runs-on: ubuntu-latest + timeout-minutes: 20 + strategy: + fail-fast: false + matrix: + include: + - locale: tr_TR + java_opts: -Duser.language=tr -Duser.country=TR + - locale: es_ES + java_opts: -Duser.language=es -Duser.country=ES + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-java@v4 + with: + distribution: temurin + java-version: '21' + + - name: Install Leiningen + run: | + curl -fsSL -o "$HOME/lein" \ + https://raw.githubusercontent.com/technomancy/leiningen/stable/bin/lein + chmod +x "$HOME/lein" + echo "$HOME" >> "$GITHUB_PATH" + "$HOME/lein" version + + - name: lein test + env: + JAVA_TOOL_OPTIONS: ${{ matrix.java_opts }} + run: lein test diff --git a/scripts/common.sh b/scripts/common.sh index 00ab97a63..826208add 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -66,7 +66,11 @@ print_env_config() { # exactly "true". dev_mode_blocks_figwheel() { local policy="${CSP_POLICY:-strict}" - [[ "$policy" == "strict" ]] || return 1 + # "permissive" is not permissive about this: permissive-csp-settings sets + # default-src 'self' and NO connect-src, so connect-src falls back to 'self' + # and ws://localhost:3449 is blocked exactly as under strict. Only "none" + # sends no policy at all. + case "$policy" in strict|permissive) ;; *) return 1 ;; esac case "$(printf '%s' "${DEV_MODE:-}" | tr '[:upper:]' '[:lower:]')" in true) return 1 ;; *) return 0 ;; @@ -199,7 +203,11 @@ LOG_DIR="${LOG_DIR:-$REPO_ROOT/logs}" # Port configuration DATOMIC_PORT="${DATOMIC_PORT:-4334}" -SERVER_PORT="${SERVER_PORT:-8890}" +# PORT is what the SERVER reads (system.clj, System/getenv "PORT") and what +# .env.example documents. Honour it here too, or the scripts check 8890 while +# the server listens somewhere else -- and every port check, explain_bind_failure +# and the config report are then confidently wrong. +SERVER_PORT="${SERVER_PORT:-${PORT:-8890}}" NREPL_PORT="${NREPL_PORT:-7888}" FIGWHEEL_PORT="${FIGWHEEL_PORT:-3449}" GARDEN_PORT="${GARDEN_PORT:-3000}" @@ -320,7 +328,15 @@ port_in_use() { # prints "LISTENING" where the docs say "LISTEN", and a non-English # Windows translates it outright. Column 2 is the local address on # every row; the last column is the PID. - [ -n "$(netstat -ano 2>/dev/null | awk -v p="[:.]${port}\$" '$2 ~ p {print; exit}')" ] + # A bound port is one with a LISTENING socket. Matching the address + # column alone also matches TIME_WAIT and ESTABLISHED rows, so a + # connection that closed seconds ago reads as "in use" and start.sh + # refuses to start. Identify listening rows by the WILDCARD FOREIGN + # ADDRESS rather than the state word -- the state word is localised + # (LISTENING/LISTEN/translated), the foreign address is not. + [ -n "$(netstat -ano 2>/dev/null \ + | awk -v p="[:.]${port}\$" \ + '$2 ~ p && $3 ~ /^(0\.0\.0\.0:0|\[::\]:0|\*:\*)$/ {print; exit}')" ] elif command -v lsof >/dev/null 2>&1; then lsof -i ":${port}" >/dev/null 2>&1 elif command -v ss >/dev/null 2>&1; then @@ -398,7 +414,8 @@ find_pids_by_port() { if is_windows; then # Last column of a LISTENING row is the owning PID. pids=$(netstat -ano 2>/dev/null \ - | awk -v p="[:.]${port}\$" '$2 ~ p && $NF ~ /^[0-9]+$/ {print $NF}' \ + | awk -v p="[:.]${port}\$" \ + '$2 ~ p && $3 ~ /^(0\.0\.0\.0:0|\[::\]:0|\*:\*)$/ && $NF ~ /^[0-9]+$/ {print $NF}' \ | sort -u || true) echo "$pids" | tr '\n' ' ' | xargs return diff --git a/scripts/start.sh b/scripts/start.sh index 4b1554426..5fc3502b5 100755 --- a/scripts/start.sh +++ b/scripts/start.sh @@ -768,8 +768,10 @@ main() { target="${positional[0]:-all}" - # A prompt is read; a banner is not. Only the first-run offer goes here. - offer_env_file + # A prompt is read; a banner is not. Only the first-run offer goes here -- + # and not for --check, which is a read-only status command: offering to + # write a file is a side effect nobody asked that question for. + [[ "$do_check" == "true" ]] || offer_env_file # Handle install flag (no prereq checks needed) if [[ "$do_install" == "true" ]]; then diff --git a/src/clj/orcpub/pedestal.clj b/src/clj/orcpub/pedestal.clj index 740e7da1c..05705dc4e 100644 --- a/src/clj/orcpub/pedestal.clj +++ b/src/clj/orcpub/pedestal.clj @@ -55,7 +55,8 @@ (defn make-nonce-interceptor "Creates an interceptor that generates per-request CSP nonces. - In prod (dev-mode?=false) with CSP_POLICY=strict: + When dev-mode? is false -- which is the DEFAULT, not just production -- + and CSP_POLICY=strict: - :enter phase generates a nonce and stores it in [:request :csp-nonce] - :leave phase adds enforcing Content-Security-Policy header with the nonce From 89f8d1778ebaefff3c00404ad240ac970375eb2d Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 16:34:41 +0000 Subject: [PATCH 14/35] Stop the env template from dictating the Datomic URL to both runtimes Asked for: default to localhost, let compose override. Done slightly differently, because the literal version does not work. docker-compose.yaml:52 is DATOMIC_URL: ${DATOMIC_URL:-datomic:dev://datomic:4334/orcpub} and compose reads .env for substitution. So an uncommented localhost in .env.example would NOT be overridden by compose -- it would override COMPOSE and break the container, which is the same defect as before with the sign flipped. DATOMIC_URL is therefore commented out entirely, and each way of running keeps its own correct default: bare metal config.clj/default-datomic-uri -> localhost docker compose's own :- fallback -> the datomic service host Neither inherits the other's, which is the behaviour asked for. The template explains both and shows when to uncomment: a remote transactor, a non-default port, or SQL storage. Checked what else reads it. migrate-db.sh:238 takes it from .env and already fails with an actionable message when unset ("Set DATOMIC_URL in .env or use --source-uri"). swarm.sh:351 carries its own :- fallback like compose. Nothing silently misbehaves. lein test 220/985/0. --- .env.example | 20 +++++++++++++++----- 1 file changed, 15 insertions(+), 5 deletions(-) diff --git a/.env.example b/.env.example index bf65d2a17..70c9f0cf3 100644 --- a/.env.example +++ b/.env.example @@ -28,12 +28,22 @@ TZ=America/Chicago # Old URLs with ?password= still work — the embedded password takes priority. ADMIN_PASSWORD=change-me-admin DATOMIC_PASSWORD=change-me-datomic -# Hostname "datomic" is the docker-compose SERVICE name and only resolves -# inside that network. Running outside Docker? Use localhost instead: +# Left COMMENTED OUT on purpose, so that each way of running picks its own +# correct value instead of inheriting the other's: +# +# bare metal -> config.clj/default-datomic-uri = datomic:dev://localhost:4334/orcpub +# docker -> docker-compose.yaml already defaults it to the "datomic" +# service hostname, which only resolves inside that network +# +# Setting it here would defeat both. Compose reads this .env for substitution, +# so an uncommented localhost would override compose's own default and break +# the container; an uncommented "datomic" hostname breaks every non-Docker +# install, which is how it used to ship. +# +# Uncomment ONLY for a database somewhere else -- a remote transactor, a +# non-default port, or Datomic SQL storage: # DATOMIC_URL=datomic:dev://localhost:4334/orcpub -# Leaving this line commented out is also fine -- the application already -# defaults to localhost (config.clj/default-datomic-uri). -DATOMIC_URL=datomic:dev://datomic:4334/orcpub +# DATOMIC_URL=datomic:sql://orcpub?jdbc:postgresql://db:5432/datomic # --- Transactor Tuning --- # These rarely need changing. See docker/transactor.properties.template. From dcd04833e5c747b263306ccaae5ef0f2ae30e8df Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 20:19:00 +0000 Subject: [PATCH 15/35] Address the second review on #695: eight findings, each reproduced first Every one was confirmed against the code before it was touched. Three of the eight are defects this branch introduced while fixing the first review, which is the honest reason a second pass found more than the first. INTRODUCED HERE, now fixed: port_in_use and find_pids_by_port matched a netstat -ano row on the local address and a wildcard foreign address, with no protocol column check. That was the fix for the localised state word (LISTENING/ESCUCHANDO), and it threw away the protocol discrimination the state word had been providing. A UDP row prints four columns with a literal "*:*" foreign address, so it satisfies both predicates: a UDP socket made a free TCP port read as busy and refuse startup, and in find_pids_by_port it handed stop.sh the PID of an unrelated UDP process to kill. Both parsers now require $1 == "TCP". Proven against realistic rows -- UDP-only now reads free, a real TCP listener still reads busy, TIME_WAIT still reads free. SERVER_PORT gained a ${PORT:-8890} fallback so the scripts would "honour what the server reads". The scripts launch the DEV service map, and dev-service-map-overrides pins ::http/port to a literal 8890 (system.clj:13). With PORT=9000 every check, report and stop targeted 9000 while the server sat on 8890. Reverted to ${SERVER_PORT:-8890}. Making the dev map read PORT is the better end state but moves where a dev server binds, which is not hotfix-sized. offer_env_file wrote .env and carried on, and said so in its own log line. .env is sourced before it exists and every default is computed from what was loaded then, so the DEV_MODE just chosen and the credentials just generated were not in the shell. It now returns 10 and start.sh exits successfully. ALREADY WRONG, now fixed: dev_mode_blocks_figwheel treated an unset DEV_MODE as false. Every server path runs `lein with-profile +dev,+start-server`, and :dev sets :env {:dev-mode "true"} (project.clj:244), so on a fresh checkout the server has dev-mode ON and skips CSP -- and the script warned that CSP would block Figwheel and offered to abort. Only an explicit false-y DEV_MODE flips it now, since environ lets a real environment variable override the profile. The same wrong premise was in offer_env_file's warning text; that is rewritten to talk about the placeholder credentials, which is the real risk on a fresh .env. The first-run .env offer ran above the `help`, `--install` and `--check` branches, so non-startup commands stopped to ask whether to write a file. Moved below all three. (--help already exits during argument parsing.) CI, which was green for the wrong reasons: "ASSERT lsof and ss are absent" pinned the runner image, not our code. port_in_use tests is_windows first, so those tools appearing would not make the Windows branch dead code -- the assertion's own rationale was wrong, and an image update would have failed a healthy build. Removed. Three steps bound 8890 and never released it. The later two therefore failed to bind, invisibly behind `&`, and then asserted against the FIRST step's listener -- passing without exercising the process they claim to create. A later step's comment already recorded the leak as a fact to work around rather than fix. Every listener step now traps EXIT and kills its own, the port-guard step moved to 8891, and both steps prove something is listening before asserting on it. Verified: bash -n and shellcheck clean (the 4 remaining warnings predate this branch), the workflow parses, `./start.sh help` and `--install` no longer reach offer_env_file where they previously did, the write-then-stop path stops where it previously started a server, the CSP predicate is correct across 7 DEV_MODE and CSP_POLICY combinations, and lein test is 220 tests / 985 assertions / 0 failures. --- .github/workflows/windows-scripts.yml | 49 +++++++++++++++-------- scripts/common.sh | 56 ++++++++++++++++++++------- scripts/start.sh | 23 ++++++++--- 3 files changed, 94 insertions(+), 34 deletions(-) diff --git a/.github/workflows/windows-scripts.yml b/.github/workflows/windows-scripts.yml index 5d23ee196..755c35dcb 100644 --- a/.github/workflows/windows-scripts.yml +++ b/.github/workflows/windows-scripts.yml @@ -51,16 +51,13 @@ jobs: *) echo "FAIL: is_windows() would not fire — uname says '$(uname -s)'"; exit 1 ;; esac - - name: ASSERT lsof and ss are absent - run: | - # If either exists, port_in_use takes a different branch than assumed - # and the Windows branch is dead code. Not fatal to users, but it means - # the whole diagnosis was wrong, so fail and make us look. - rc=0 - command -v lsof >/dev/null 2>&1 && { echo "FAIL: lsof EXISTS"; rc=1; } - command -v ss >/dev/null 2>&1 && { echo "FAIL: ss EXISTS"; rc=1; } - [ $rc -eq 0 ] && echo "OK: neither lsof nor ss present" - exit $rc + # There was an "ASSERT lsof and ss are absent" step here. It was wrong on + # its own terms: port_in_use tests is_windows FIRST, so the Windows branch + # is taken whether or not those tools exist, and their presence would not + # make it dead code. All the assertion really pinned was the contents of + # the runner image, so a routine image update would have failed a build + # that had nothing wrong with it. The platform assertion above and the + # real-listener tests below cover what actually matters. - name: ASSERT 'netstat -tln' fails and prints nothing to stdout run: | @@ -94,6 +91,10 @@ jobs: time.sleep(180) " & LISTENER=$! + # trap, not a kill at the bottom: the SETUP FAIL below exits early, and + # a listener leaked from here is what made three later steps test the + # wrong process. + trap 'kill $LISTENER 2>/dev/null || true' EXIT sleep 5 # independent truth: can we actually connect? @@ -125,7 +126,6 @@ jobs: [ -n "${PIDS// /}" ] || { echo "FAIL: find_pids_by_port returned nothing"; fail=1; } port_in_use 8891 && { echo "FAIL: reported an unused port as busy"; fail=1; } [ "$O" = "free" ] && echo "CONFIRMED: the old check reports 'free' for a port that is in use" - kill $LISTENER 2>/dev/null || true exit $fail - name: ASSERT taskkill can stop a process we found by port @@ -138,6 +138,8 @@ jobs: s.bind(('127.0.0.1', 8899)); s.listen(5) time.sleep(180) " & + LISTENER=$! + trap 'kill $LISTENER 2>/dev/null || true' EXIT sleep 5 PIDS="$(find_pids_by_port 8899)" echo "found pids : ${PIDS:-}" @@ -177,13 +179,23 @@ jobs: # interactive) offers to stop it. CI is non-interactive, so the # expected outcome is a clean refusal naming the port. set -u + # Port 8891, not 8890: an earlier step in this job binds 8890 and holds + # it for 180s without stopping it. This listener would have failed to + # bind, the failure would have been invisible behind `&`, and the test + # would then have passed against THAT listener -- green without ever + # exercising the process it claims to create. python -c " import socket, time s = socket.socket() - s.bind(('127.0.0.1', 8890)); s.listen(5) + s.bind(('127.0.0.1', 8891)); s.listen(5) time.sleep(120) " & + LISTENER=$! + trap 'kill $LISTENER 2>/dev/null || true' EXIT sleep 5 + # Prove the listener is actually ours before asserting anything on it. + (exec 3<>/dev/tcp/127.0.0.1/8891) 2>/dev/null \ + || { echo "SETUP FAIL: nothing listening on 8891"; exit 1; } source scripts/common.sh # check_port_available lives in start.sh, and start.sh cannot be run # end to end here (it exits at the Leiningen prereq before reaching @@ -191,7 +203,7 @@ jobs: SCRIPT_DIR="$PWD/scripts" source <(sed -n '/^check_port_available()/,/^}/p' scripts/start.sh) set +e - out="$(check_port_available 8890 server false 2>&1)"; rc=$? + out="$(check_port_available 8891 server false 2>&1)"; rc=$? set -e echo "$out" echo "exit: $rc" @@ -209,7 +221,11 @@ jobs: s.bind(('127.0.0.1', 8890)); s.listen(5) time.sleep(60) " & + LISTENER=$! + trap 'kill $LISTENER 2>/dev/null || true' EXIT sleep 5 + (exec 3<>/dev/tcp/127.0.0.1/8890) 2>/dev/null \ + || { echo "SETUP FAIL: nothing listening on 8890"; exit 1; } out="$(explain_bind_failure 8890 2>&1)" echo "$out" echo "$out" | grep -qi "already listening" || { echo "FAIL: did not identify the listener"; exit 1; } @@ -240,9 +256,10 @@ jobs: # a bind failure after a normal shutdown would be crying wolf. set -u source scripts/common.sh - # Use a port no other step touches. Earlier steps leave their python - # listeners alive (they sleep well past the end of the step), so 8890 - # is NOT free by the time we get here. + # A port no other step touches. Every listener step now traps EXIT and + # kills its own, so 8890 should in fact be free here -- but this + # assertion is about silence on a free port, and it should not also be + # an implicit test of someone else's cleanup. FREE_PORT=8123 if port_in_use "$FREE_PORT"; then echo "SETUP FAIL: $FREE_PORT is unexpectedly in use"; exit 1 diff --git a/scripts/common.sh b/scripts/common.sh index 826208add..4690ed66e 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -71,9 +71,19 @@ dev_mode_blocks_figwheel() { # and ws://localhost:3449 is blocked exactly as under strict. Only "none" # sends no policy at all. case "$policy" in strict|permissive) ;; *) return 1 ;; esac + # An UNSET DEV_MODE does not mean false here. Every server path in start.sh + # launches `lein with-profile +dev,+start-server`, and the :dev profile sets + # :env {:dev-mode "true"} (project.clj:244), which environ reads. So on a + # fresh checkout with nothing exported, the server we are about to start has + # dev-mode ON and skips CSP entirely -- and warning that CSP will block + # Figwheel would be false, and would talk the user out of a working setup. + # + # Only an explicit false-y DEV_MODE flips it: environ lets a real environment + # variable override the profile's value, so DEV_MODE=false does reach the + # server and does re-enable CSP. case "$(printf '%s' "${DEV_MODE:-}" | tr '[:upper:]' '[:lower:]')" in - true) return 1 ;; - *) return 0 ;; + false|0|no|off) return 0 ;; + *) return 1 ;; esac } @@ -118,8 +128,9 @@ offer_env_file() { fi echo "" - log_warn "No .env found. Defaults will be used, including CSP_POLICY=strict with" - log_warn "DEV_MODE unset, which silently blocks Figwheel's hot reload." + log_warn "No .env found. Defaults will be used: CSP_POLICY=strict, and the" + log_warn "placeholder SIGNATURE / ADMIN_PASSWORD / DATOMIC_PASSWORD from the" + log_warn "template, which are published values and must not face a network." local reply="" read -t 30 -p "Create .env from .env.example now? [y/N] " -n 1 -r reply || { echo; log_info "No answer — continuing on defaults."; return 0; } @@ -167,8 +178,14 @@ offer_env_file() { fi fi - log_info "Wrote $env_file (mode 600). Review it, then re-run to pick it up;" - log_info "this run continues on the values it already loaded." + log_info "Wrote $env_file (mode 600). Review it, then re-run to start." + # Returning 10 means "written, caller should stop". Continuing is not an + # option: .env was sourced near the top of this file, before it existed, and + # every default and derived path was computed from what was loaded then. The + # DEV_MODE just chosen and the credentials just generated are not in this + # shell and cannot be, so carrying on would launch the server on exactly the + # configuration the user was asked about and answered. + return 10 } # Raised where it actually bites. Starting Figwheel with an enforcing CSP gives @@ -203,11 +220,16 @@ LOG_DIR="${LOG_DIR:-$REPO_ROOT/logs}" # Port configuration DATOMIC_PORT="${DATOMIC_PORT:-4334}" -# PORT is what the SERVER reads (system.clj, System/getenv "PORT") and what -# .env.example documents. Honour it here too, or the scripts check 8890 while -# the server listens somewhere else -- and every port check, explain_bind_failure -# and the config report are then confidently wrong. -SERVER_PORT="${SERVER_PORT:-${PORT:-8890}}" +# Deliberately NOT ${PORT:-8890}. PORT is read by the PRODUCTION service map; +# these scripts launch the DEV one, and orcpub.system/dev-service-map-overrides +# pins ::http/port to a literal 8890 (system.clj:13). Honouring PORT here made +# the scripts probe, report and stop 9000 while the server sat on 8890 -- the +# checks confidently describing a port nothing was listening on. +# +# The alternative fix is to make the dev service map read PORT. That is the +# better end state, but it changes where a dev server binds, which is not a +# hotfix-sized change; SERVER_PORT still overrides if you need to move it. +SERVER_PORT="${SERVER_PORT:-8890}" NREPL_PORT="${NREPL_PORT:-7888}" FIGWHEEL_PORT="${FIGWHEEL_PORT:-3449}" GARDEN_PORT="${GARDEN_PORT:-3000}" @@ -334,9 +356,13 @@ port_in_use() { # refuses to start. Identify listening rows by the WILDCARD FOREIGN # ADDRESS rather than the state word -- the state word is localised # (LISTENING/LISTEN/translated), the foreign address is not. + # $1 == "TCP" is load-bearing. A UDP row is printed with four columns + # and a literal "*:*" foreign address, so it satisfies the wildcard test + # above; without the protocol check a UDP socket on this port makes a + # free TCP port read as busy and start.sh refuses to start. [ -n "$(netstat -ano 2>/dev/null \ | awk -v p="[:.]${port}\$" \ - '$2 ~ p && $3 ~ /^(0\.0\.0\.0:0|\[::\]:0|\*:\*)$/ {print; exit}')" ] + '$1 == "TCP" && $2 ~ p && $3 ~ /^(0\.0\.0\.0:0|\[::\]:0|\*:\*)$/ {print; exit}')" ] elif command -v lsof >/dev/null 2>&1; then lsof -i ":${port}" >/dev/null 2>&1 elif command -v ss >/dev/null 2>&1; then @@ -413,9 +439,13 @@ find_pids_by_port() { if is_windows; then # Last column of a LISTENING row is the owning PID. + # $1 == "TCP" for the same reason as port_in_use, and it matters more + # here: stop.sh feeds these PIDs to kill, so a UDP row passing the + # wildcard test would terminate an unrelated process that merely shares + # the port number. pids=$(netstat -ano 2>/dev/null \ | awk -v p="[:.]${port}\$" \ - '$2 ~ p && $3 ~ /^(0\.0\.0\.0:0|\[::\]:0|\*:\*)$/ && $NF ~ /^[0-9]+$/ {print $NF}' \ + '$1 == "TCP" && $2 ~ p && $3 ~ /^(0\.0\.0\.0:0|\[::\]:0|\*:\*)$/ && $NF ~ /^[0-9]+$/ {print $NF}' \ | sort -u || true) echo "$pids" | tr '\n' ' ' | xargs return diff --git a/scripts/start.sh b/scripts/start.sh index 5fc3502b5..362dc2122 100755 --- a/scripts/start.sh +++ b/scripts/start.sh @@ -768,11 +768,6 @@ main() { target="${positional[0]:-all}" - # A prompt is read; a banner is not. Only the first-run offer goes here -- - # and not for --check, which is a read-only status command: offering to - # write a file is a side effect nobody asked that question for. - [[ "$do_check" == "true" ]] || offer_env_file - # Handle install flag (no prereq checks needed) if [[ "$do_install" == "true" ]]; then run_install @@ -791,6 +786,24 @@ main() { exit $? fi + # First-run .env offer. A prompt is read; a banner is not -- so this is the + # one interactive thing that happens before startup. + # + # It goes HERE, below every non-startup branch, not above them: `help`, + # `--install` and `--check` are not requests to start a server, and none of + # them should stop to ask whether to write a file. (`--help` already exits + # during argument parsing.) It used to sit above all three. + # + # Return 10 means the file was written. Stop there rather than start: .env + # was sourced before it existed, so the answers just given are not in this + # shell and the server would launch on the configuration the user was asked + # about and answered. + offer_env_file || { + rc=$? + [[ $rc -eq 10 ]] && exit $EXIT_SUCCESS + exit $rc + } + # Check prerequisites for runtime targets check_java || exit $EXIT_PREREQ check_lein || exit $EXIT_PREREQ From b6df8098f20b65744e55a7e7119c8228e9e01ec0 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 20:25:23 +0000 Subject: [PATCH 16/35] Make the dev service map honour PORT, so the scripts and the server agree The two review rounds contradict each other on this, and the previous commit followed the wrong one. Round 1: "port diagnostics ignore the application's configured PORT" -- so scripts/common.sh was changed to read PORT. Round 2: that is wrong, because dev-service-map-overrides pinned a literal 8890, and it offered either/or -- honour PORT in the dev map, or keep the launcher on the fixed port. The launcher was reverted, which walks straight back into round 1's complaint. Round 2's own title for the finding is "Make development service honor the configured PORT", so that is what this does. It settles both rounds instead of alternating between them. PORT was read by prod and ignored by dev, while .env.example documents it with no hint that it applies to only one of them. A self-hoster setting PORT=9000 and running ./start.sh got a server on 8890 and no indication why. Both maps now call one configured-port, and scripts/common.sh follows PORT again. Found while testing this, and fixed here rather than carried: the existing prod parser treated an empty PORT as a value. (or (System/getenv "PORT") "8890") returns "" for a bare `PORT=` line, because the empty string is truthy in Clojure, and Integer/parseInt then threw. Moving that code into the dev path would have spread a latent prod bug onto the common one -- and since both service maps are top-level defs, it throws while the NAMESPACE LOADS rather than when the server starts. A config template with an empty value left in it is an ordinary thing to find. Blank now counts as unset, and the value is trimmed. Verified against a real JVM, dev and prod agreeing in every case: PORT unset dev 8890 prod 8890 PORT=9000 dev 9000 prod 9000 PORT= dev 8890 prod 8890 (previously: threw at load) PORT=" 9001 " dev 9001 prod 9001 PORT=abc still raises :invalid-port, so the guard still discriminates lein test: 220 tests, 985 assertions, 0 failures. --- scripts/common.sh | 17 +++++++---------- src/clj/orcpub/system.clj | 38 ++++++++++++++++++++++++++++---------- 2 files changed, 35 insertions(+), 20 deletions(-) diff --git a/scripts/common.sh b/scripts/common.sh index 4690ed66e..38bc5556a 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -220,16 +220,13 @@ LOG_DIR="${LOG_DIR:-$REPO_ROOT/logs}" # Port configuration DATOMIC_PORT="${DATOMIC_PORT:-4334}" -# Deliberately NOT ${PORT:-8890}. PORT is read by the PRODUCTION service map; -# these scripts launch the DEV one, and orcpub.system/dev-service-map-overrides -# pins ::http/port to a literal 8890 (system.clj:13). Honouring PORT here made -# the scripts probe, report and stop 9000 while the server sat on 8890 -- the -# checks confidently describing a port nothing was listening on. -# -# The alternative fix is to make the dev service map read PORT. That is the -# better end state, but it changes where a dev server binds, which is not a -# hotfix-sized change; SERVER_PORT still overrides if you need to move it. -SERVER_PORT="${SERVER_PORT:-8890}" +# PORT is what BOTH service maps now read (orcpub.system/configured-port) and +# what .env.example documents, so the scripts follow it too. They have to agree: +# when the dev map pinned a literal 8890 and this read PORT, every port check, +# explain_bind_failure and config report described a port nothing was listening +# on. SERVER_PORT still overrides, for moving the scripts without moving the +# server. +SERVER_PORT="${SERVER_PORT:-${PORT:-8890}}" NREPL_PORT="${NREPL_PORT:-7888}" FIGWHEEL_PORT="${FIGWHEEL_PORT:-3449}" GARDEN_PORT="${GARDEN_PORT:-3000}" diff --git a/src/clj/orcpub/system.clj b/src/clj/orcpub/system.clj index c221c047f..e8317f0ab 100644 --- a/src/clj/orcpub/system.clj +++ b/src/clj/orcpub/system.clj @@ -1,5 +1,6 @@ (ns orcpub.system - (:require [com.stuartsierra.component :as component] + (:require [clojure.string :as s] + [com.stuartsierra.component :as component] [reloaded.repl :as rrepl] [io.pedestal.http :as http] [orcpub.pedestal :as pedestal] @@ -9,8 +10,32 @@ [environ.core :as environ]) (:import (org.eclipse.jetty.server.handler.gzip GzipHandler))) +(defn- configured-port + "The port to bind, from the PORT environment variable, defaulting to 8890. + + Shared by both service maps on purpose. The dev map used to pin a literal + 8890 while prod read PORT, so PORT=9000 in .env moved the production server + and silently did nothing in dev -- and scripts/common.sh, which reads the + same PORT, then probed, reported and stopped a port nothing was listening on. + .env.example documents PORT with no hint that it only applies to one of them." + [] + ;; Blank counts as unset. An (or ...) on getenv is not enough: the empty + ;; string is truthy in Clojure, so a .env carrying a bare `PORT=` yielded it and + ;; threw here -- and because both service maps are top-level defs, that throws + ;; while the namespace LOADS, not when the server starts. An empty value in a + ;; config template is a normal thing for a user to leave behind. + (let [raw (System/getenv "PORT") + port-str (if (s/blank? raw) "8890" (s/trim raw))] + (try + (Integer/parseInt port-str) + (catch NumberFormatException e + (throw (ex-info "Invalid PORT environment variable. Expected a number." + {:error :invalid-port + :port port-str} + e)))))) + (def dev-service-map-overrides - {::http/port 8890 + {::http/port (configured-port) ;; Bind to loopback only in dev (don't expose to LAN) ::http/host "localhost" ;; do not block thread that starts web server @@ -34,14 +59,7 @@ ::http/host "0.0.0.0" ;; Pedestal 0.7+ requires explicit interceptor coercion for maps/functions ::http/enable-session false ; Disable default session handling if not needed - ::http/port (let [port-str (or (System/getenv "PORT") "8890")] - (try - (Integer/parseInt port-str) - (catch NumberFormatException e - (throw (ex-info "Invalid PORT environment variable. Expected a number." - {:error :invalid-port - :port port-str} - e))))) + ::http/port (configured-port) ::http/join false ::http/resource-path "/public" ;; CSP configured via CSP_POLICY env var (strict|permissive|none) From c9fd5a5ba073b49c596b1d4cc53817b71f00fe23 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Sun, 20 Sep 2026 20:53:15 +0000 Subject: [PATCH 17/35] Finish the CSP docs fix: three user-facing docs still described Report-Only The earlier commit corrected the docstrings in config.clj and pedestal.clj and stopped there, so the claim survived in the three places a self-hoster actually reads: docs/ENVIRONMENT.md DEV_MODE "enables dev-mode CSP (Report-Only instead of enforcing)", and "Dev mode uses Report-Only header (logs violations but doesn't block)" docs/DOCKER.md "Set to true for CSP Report-Only mode" docs/migration/pedestal-0.7.md "avoids flooding the browser console with Report-Only violations" There is no Report-Only mode and there never has been; nothing in this codebase emits Content-Security-Policy-Report-Only, and csp_test.clj asserts its absence in both dev and prod. DEV_MODE=true does not soften the policy, it sends no CSP header at all -- which is why Figwheel's ws://localhost:3449 works under it and why the distinction matters to someone debugging a blocked hot reload. Fixing only the docstrings was the wrong half: the docstrings are read by people already in the source, and the docs are read by people trying to stay out of it. lein test: 220 tests, 985 assertions, 0 failures. --- docs/DOCKER.md | 2 +- docs/ENVIRONMENT.md | 4 ++-- docs/migration/pedestal-0.7.md | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/docs/DOCKER.md b/docs/DOCKER.md index ca988d12a..c94d5cfb3 100644 --- a/docs/DOCKER.md +++ b/docs/DOCKER.md @@ -210,7 +210,7 @@ These are the variables you'll actually touch. Full reference in | `ALT_HOST` | No | `127.0.0.1` | Transactor peer fallback host. Change to `datomic` for Swarm. | | `EMAIL_SERVER_URL` | No | *(empty)* | SMTP server. Leave empty to disable email (registration still works, just no verification emails). | | `CSP_POLICY` | No | `strict` | Content Security Policy: `strict`, `permissive`, or `none`. | -| `DEV_MODE` | No | *(empty)* | Set to `true` for CSP Report-Only mode (allows Figwheel hot-reload). | +| `DEV_MODE` | No | *(empty)* | Set to `true` to send no CSP header at all, which is what allows Figwheel hot-reload. Not a Report-Only mode — that does not exist. | | `LOAD_HOMEBREW_URL` | No | *(empty)* | URL to fetch `.orcbrew` plugins on first page load. | `run` generates `DATOMIC_PASSWORD`, `ADMIN_PASSWORD`, and diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index 99ebff0eb..154781861 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -47,10 +47,10 @@ All configuration is managed via a `.env` file at the repository root. Copy `.en | Variable | Default | Description | |----------|---------|-------------| | `CSP_POLICY` | `strict` | Content Security Policy mode: `strict`, `permissive`, or `none` | -| `DEV_MODE` | `"true"` (in :dev profile) | Enables dev-mode CSP (Report-Only instead of enforcing). Must be the string `"true"` (case-insensitive) -- any other value (including `"1"`, `"yes"`, or empty) is treated as false. | +| `DEV_MODE` | `"true"` (in :dev profile) | When true, **no CSP header is sent at all** — that is what lets Figwheel's `ws://localhost:3449` through. Must be the string `"true"` (case-insensitive); any other value, including `"1"`, `"yes"` or empty, is false. | CSP modes: -- **strict** — nonce-based CSP with `strict-dynamic`. Dev mode uses `Report-Only` header (logs violations but doesn't block). Prod uses enforcing header. +- **strict** — nonce-based CSP with `strict-dynamic`, always as an enforcing `Content-Security-Policy` header. **There is no Report-Only mode**; no code has ever emitted `Content-Security-Policy-Report-Only`. Dev mode does not soften the policy, it skips it. - **permissive** — allows `unsafe-inline` and `unsafe-eval`. Legacy fallback. - **none** — disables CSP entirely. Not recommended for production. diff --git a/docs/migration/pedestal-0.7.md b/docs/migration/pedestal-0.7.md index 22c183413..f2062d827 100644 --- a/docs/migration/pedestal-0.7.md +++ b/docs/migration/pedestal-0.7.md @@ -51,7 +51,7 @@ In CSP Level 3 browsers (all modern browsers), `strict-dynamic` causes the brows 1. `nonce-interceptor` (in `pedestal.clj`) generates a 128-bit nonce per request 2. The nonce is stored in `[:request :csp-nonce]` for templates to read 3. On `:leave`, the interceptor sets the `Content-Security-Policy` header with the nonce -4. **Dev mode**: The nonce interceptor is a no-op — no nonce is generated, no CSP header is added. Pedestal 0.7's built-in `secure-headers` still applies its own defaults, but the custom nonce header is skipped entirely. This avoids flooding the browser console with Report-Only violations from Figwheel's inline scripts. +4. **Dev mode**: The nonce interceptor is a no-op — no nonce is generated, no CSP header is added. Pedestal 0.7's built-in `secure-headers` still applies its own defaults, but the custom nonce header is skipped entirely. This is what lets Figwheel's inline scripts and its `ws://localhost:3449` connection through: the policy is not softened, it is simply not sent. (Earlier revisions of this file called that a "Report-Only" mode. No such mode exists here — nothing has ever emitted `Content-Security-Policy-Report-Only`.) 5. **Prod mode**: Enforcing `Content-Security-Policy` with per-request nonces **Configuration** via `CSP_POLICY` env var (see `.env.example`): From 7703e367ab016027d3c216920d1ff6f90ed17963 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Mon, 21 Sep 2026 00:19:00 +0000 Subject: [PATCH 18/35] Mirror the server exactly in dev_mode_blocks_figwheel, including empty DEV_MODE The previous revision overcorrected. It was right that an unset DEV_MODE must not count as false -- the :dev profile supplies "true" -- but it then classified every value that was not literally false-y as non-blocking, so yes, 1, an empty string and typos all read as "Figwheel will work". The server disagrees: config/dev-mode? is (.equalsIgnoreCase "true"), so all four leave CSP enforcing and the websocket blocked, with the script staying quiet. That is the exact silent failure the warning exists for, reintroduced from the other side. Measured against `lein with-profile +dev` rather than reasoned about, because the interesting case is not guessable: DEV_MODE unset -> env :dev-mode "true" dev-mode? true does not block DEV_MODE=true -> "true" dev-mode? true does not block DEV_MODE=TRUE -> "TRUE" dev-mode? true does not block DEV_MODE=yes -> "yes" dev-mode? FALSE blocks DEV_MODE=1 -> "1" dev-mode? FALSE blocks DEV_MODE= -> "" dev-mode? FALSE blocks DEV_MODE=tru -> "tru" dev-mode? FALSE blocks DEV_MODE=false -> "false" dev-mode? FALSE blocks An explicitly empty DEV_MODE overrides the profile and yields false. Unset and empty therefore disagree, so the test is ${DEV_MODE+x} -- ${DEV_MODE:-} collapses them and would get the empty case wrong. The predicate now matches that table in all eight cases, verified by running it against each. Not changed, and why: the review also asks for Datomic Pro to be installed in locale.yml, on the grounds that com.datomic/peer 1.0.7482 cannot resolve without bin/maven-install and both jobs "will therefore fail during dependency resolution". They do not. Both have passed on every push -- most recently c9fd5a5b, where each ran the full suite to completion: Ran 220 tests containing 985 assertions. 0 failures, 0 errors. under tr_TR and es_ES. The dependency that needs the local install is com.datomic/datomic-pro, and it is commented out (project.clj:81); the live one is com.datomic/peer (project.clj:82), which resolves remotely. project.clj:275 records the same thing: "datomic-pro dependency removed - peer is already in main deps". lib/ contains no com/ directory, so nothing came from file:lib either. Adding a download step would add about a minute to a job that already works. Also correcting an earlier commit message here: it said shellcheck leaves "4 remaining warnings". There are 9. The list had been truncated with head -20 when it was counted. The substantive half of the claim holds -- all 9 are present at the branch base d42e05d1, and this branch has added none. lein test: 220 tests, 985 assertions, 0 failures. --- scripts/common.sh | 41 +++++++++++++++++++++++++++++------------ 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/scripts/common.sh b/scripts/common.sh index 38bc5556a..fa1b8f92b 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -71,19 +71,36 @@ dev_mode_blocks_figwheel() { # and ws://localhost:3449 is blocked exactly as under strict. Only "none" # sends no policy at all. case "$policy" in strict|permissive) ;; *) return 1 ;; esac - # An UNSET DEV_MODE does not mean false here. Every server path in start.sh - # launches `lein with-profile +dev,+start-server`, and the :dev profile sets - # :env {:dev-mode "true"} (project.clj:244), which environ reads. So on a - # fresh checkout with nothing exported, the server we are about to start has - # dev-mode ON and skips CSP entirely -- and warning that CSP will block - # Figwheel would be false, and would talk the user out of a working setup. + # Mirror the SERVER exactly. config/dev-mode? is (.equalsIgnoreCase "true"), + # so ONLY the literal "true" turns CSP off; every other value leaves it + # enforcing and blocks ws://localhost:3449. Guessing at other truthy-looking + # spellings here is what makes the warning lie -- an earlier revision treated + # yes/1/empty/typos as non-blocking, so the server sent enforcing CSP while + # this stayed quiet, which is the silent hot-reload failure it exists to warn + # about. # - # Only an explicit false-y DEV_MODE flips it: environ lets a real environment - # variable override the profile's value, so DEV_MODE=false does reach the - # server and does re-enable CSP. - case "$(printf '%s' "${DEV_MODE:-}" | tr '[:upper:]' '[:lower:]')" in - false|0|no|off) return 0 ;; - *) return 1 ;; + # UNSET is the one case that is not a value: every server path launches + # `lein with-profile +dev,+start-server`, and :dev supplies + # :env {:dev-mode "true"} (project.clj:244). An EXPLICIT empty string IS a + # value, and it overrides the profile. + # + # Measured against `lein with-profile +dev`, not reasoned about: + # + # DEV_MODE unset -> env :dev-mode "true" dev-mode? true does not block + # DEV_MODE=true -> "true" dev-mode? true does not block + # DEV_MODE=TRUE -> "TRUE" dev-mode? true does not block + # DEV_MODE=yes -> "yes" dev-mode? FALSE blocks + # DEV_MODE=1 -> "1" dev-mode? FALSE blocks + # DEV_MODE= -> "" dev-mode? FALSE blocks + # DEV_MODE=tru -> "tru" dev-mode? FALSE blocks + # DEV_MODE=false -> "false" dev-mode? FALSE blocks + # + # ${DEV_MODE+x}, not ${DEV_MODE:-}: the latter collapses unset and empty, + # and the table above shows those two disagree. + [ -z "${DEV_MODE+x}" ] && return 1 + case "$(printf '%s' "$DEV_MODE" | tr '[:upper:]' '[:lower:]')" in + true) return 1 ;; + *) return 0 ;; esac } From c0a6fa77f332e4efd6cfabaab79ac7cc89e37cb8 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Mon, 21 Sep 2026 02:16:56 +0000 Subject: [PATCH 19/35] Drop the commented-out datomic-pro coordinate and the comment that outlived it ;[com.datomic/datomic-pro "1.0.7482" ...] had been commented out, but the five lines of comment above it stayed and were read as describing the LIVE dependency below. They said the jar is "installed to lib/com/datomic/datomic-pro/1.0.7482/ during Docker build/postCreateCommand" and "uses existing file:lib repository pattern (same as pdfbox)". Neither is true of com.datomic/peer, which resolves from Maven Central -- the local cache records peer-1.0.7482.jar>central= -- and lib/ has no com/ directory at all. That stale comment has already cost something: it is why a review concluded locale.yml must install Datomic Pro before lein test or fail at dependency resolution. The workflow has passed on every push; the inference was reasonable and the comment is what made it wrong. Replaced with what is actually true, including the part that is easy to delete by mistake: lib/com/datomic/datomic-pro// IS still populated, by .devcontainer/post-create.sh and docker/Dockerfile, but for the TRANSACTOR BINARY that scripts/common.sh:257, start.sh:702 and migrate-db.sh:186 invoke. The download is live infrastructure; only the jar resolution moved. Also drops the ";; datomic-pro dependency removed - peer is already in main deps" note on the :prod profile. It was a changelog entry about a line that no longer exists to be compared against. Deliberately NOT touched: the Install Datomic Pro step in continuous-integration.yml. For `lein test` it now looks vestigial -- the jar comes from Central and test-datomic-pro-basic-connectivity uses datomic:mem://, so no transactor is required -- but it is gated on a stack-detection flag and may serve other jobs, and removing CI infrastructure is not this branch's business. Worth a look separately. lein deps :tree resolves; lein test 220 tests, 985 assertions, 0 failures. --- project.clj | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/project.clj b/project.clj index f60454c33..ca59d47b6 100644 --- a/project.clj +++ b/project.clj @@ -73,12 +73,17 @@ ;; datomock fork with Datomic Pro 1.0.6527+ compatibility (new transact signature) ;; Original vvvvalvalval/datomock 0.2.0 causes AbstractMethodError with Datomic Pro [org.clojars.favila/datomock "0.2.2-favila1"] - ;; Datomic Pro: Free under Apache 2.0, supports Java 11/17/21, actively maintained. + ;; The peer library, free under Apache 2.0, Java 11/17/21. ;; Exclude slf4j-nop to avoid duplicate SLF4J binding warnings. - ;; Installed to lib/com/datomic/datomic-pro/1.0.7482/ during Docker build/postCreateCommand - ;; Uses existing file:lib repository pattern (same as pdfbox) + ;; Resolves from MAVEN CENTRAL -- not from file:lib, and no local + ;; install is needed to build or test. The commented-out + ;; com.datomic/datomic-pro coordinate that used to sit here did need + ;; one, and its comment kept implying this dependency still does. + ;; lib/com/datomic/datomic-pro// is still populated, but by + ;; .devcontainer/post-create.sh and docker/Dockerfile, and for the + ;; TRANSACTOR BINARY that scripts/common.sh:257 and start.sh run -- + ;; not for this jar. ;; Latest version: https://docs.datomic.com/releases-pro.html - ;[com.datomic/datomic-pro "1.0.7482" :exclusions [org.slf4j/slf4j-nop]] [com.datomic/peer "1.0.7482" :exclusions [org.slf4j/slf4j-nop]] ;; cuerdas 026.415: Latest release on Clojars... does not match GH release versioning. [funcool/cuerdas "2026.415"] @@ -272,7 +277,6 @@ :optimizations :none}}]} :repl-options {:nrepl-middleware [cemerick.piggieback/wrap-cljs-repl]}} ;; NOTE: :prod was for React Native builds (legacy, may be unused) - ;; datomic-pro dependency removed - peer is already in main deps :prod {:cljsbuild {:builds [{:id "main" :source-paths ["src/cljs" "native/cljs" "src/cljc" "env/prod"] :compiler {:output-to "main.js" From 813fdbb8e568ad13fded5e78c9c48fb6d6d7a925 Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Mon, 21 Sep 2026 04:17:36 +0000 Subject: [PATCH 20/35] Mirror the server's three-way CSP cond, and stop the warning claiming "strict" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both findings reproduced, and a third fell out of testing them. get-secure-headers-config is a three-way cond, and the helper treated it as two-way: none -> settings nil, no CSP at all strict -> settings nil; the NONCE INTERCEPTOR emits an enforcing header, but only when dev-mode? is false. The ONLY policy DEV_MODE affects. ANY OTHER -> permissive-csp-settings, applied STATICALLY by Pedestal. That covers "permissive" and every unrecognised value. default-src 'self', no connect-src, so the Figwheel socket is blocked, and DEV_MODE cannot change it because the nonce interceptor is inert. The old `case "$policy" in strict|permissive)` then ran the DEV_MODE logic over both, so CSP_POLICY=permissive with DEV_MODE=true reported "fine" while the server blocked, and an unrecognised value reported "fine" unconditionally. The case was also raw, so CSP_POLICY=STRICT missed it entirely although the server lowercases with Locale/ROOT. Measured against a real JVM rather than reasoned about: CSP=strict DEV=unset server blocks: no CSP=strict DEV=true no CSP=strict DEV=false YES CSP=STRICT DEV=false YES (server normalises; helper did not) CSP=permissive DEV=true YES (helper said no) CSP=permissive DEV=false YES CSP=none DEV=false no CSP=bogus DEV=true YES (helper said no) CSP= DEV=true YES (helper said no -- see below) CSP= DEV=false YES The helper now agrees on all twelve combinations tested. THIRD DEFECT, found while testing the first two: ${CSP_POLICY:-strict} is wrong for the same reason ${DEV_MODE:-} was. The server does (or (env :csp-policy) (System/getenv "CSP_POLICY") "strict"), and the empty string is TRUTHY in Clojure, so CSP_POLICY= resolves to "" and falls through to the permissive fallback -- it does NOT default to strict. Bash's :- collapses unset and empty and would call it strict, which is precisely the policy DEV_MODE can switch off, so CSP_POLICY= with DEV_MODE=true was reported as safe while the server blocked. Both readers now go through one _csp_policy_token using ${CSP_POLICY+x}, so they cannot drift apart again. LC_ALL=C on both tr calls, which is this branch's own subject matter: tr '[:upper:]' '[:lower:]' folds using the shell's locale, so on a Turkish machine "STRICT" becomes "strıct" and misses every comparison -- the exact failure config.clj avoids with Locale/ROOT. The message, which was the second finding: confirm_dev_mode hardcoded "CSP is strict and ENFORCING" whatever the policy was, and closed with "Set DEV_MODE=true in .env to develop" -- advice that cannot work under permissive or a fallback, where DEV_MODE has no effect. It now reports the effective policy and gives the remedy that fits it. An unrecognised value is reported as what it becomes: permissive (fallback from "bogus") print_env_config reports the same effective policy instead of the raw string. shellcheck adds nothing new; lein test 220 tests, 985 assertions, 0 failures. --- scripts/common.sh | 121 +++++++++++++++++++++++++++++++--------------- 1 file changed, 83 insertions(+), 38 deletions(-) diff --git a/scripts/common.sh b/scripts/common.sh index fa1b8f92b..36f3b5178 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -52,8 +52,9 @@ print_env_config() { fi echo " ports server=$SERVER_PORT datomic=$DATOMIC_PORT figwheel=$FIGWHEEL_PORT nrepl=$NREPL_PORT" - local policy="${CSP_POLICY:-strict}" - local dev="${DEV_MODE:-}" + local policy dev + policy="$(effective_csp_policy)" + dev="${DEV_MODE:-}" if dev_mode_blocks_figwheel; then echo -e " csp ${YELLOW}$policy, ENFORCING${NC} (DEV_MODE=$dev) — Figwheel hot reload blocked" else @@ -61,44 +62,76 @@ print_env_config() { fi } -# True when the server will send an enforcing CSP whose connect-src omits the -# Figwheel websocket. Matches the server's own comparison: case-insensitive, -# exactly "true". +# The policy the SERVER will actually use, normalised the way +# config/get-csp-policy does (Locale/ROOT lowercase). An unrecognised value is +# not an error there: get-secure-headers-config cond-falls through to +# permissive-csp-settings, so report what it BECOMES, not what was typed. +# +# LC_ALL=C on the tr is load-bearing, and is the same defect this branch exists +# for: tr '[:upper:]' '[:lower:]' uses the shell's locale, so on a Turkish +# machine "STRICT" folds to "strıct" (dotless i), matches nothing, and gets +# reported as the fallback. The server avoids this with Locale/ROOT. +# The policy token exactly as the server resolves it, before any interpretation. +# +# ${CSP_POLICY+x}, not ${CSP_POLICY:-strict}. The server does +# (or (env :csp-policy) (System/getenv "CSP_POLICY") "strict") +# and the empty string is TRUTHY in Clojure, so CSP_POLICY= resolves to "" -- +# not to "strict". Only a genuinely absent variable defaults. Bash's :- collapses +# unset and empty, which would call an empty CSP_POLICY "strict"; strict is the +# one policy DEV_MODE can switch off, so an empty value would be reported as +# safe while the server applied the static permissive policy. Measured: with +# CSP_POLICY= and DEV_MODE=true the server BLOCKS and the :- version said it did +# not. +_csp_policy_token() { + if [ -z "${CSP_POLICY+x}" ]; then + printf 'strict' + else + printf '%s' "$CSP_POLICY" | LC_ALL=C tr 'A-Z' 'a-z' + fi +} + +effective_csp_policy() { + local p + p="$(_csp_policy_token)" + case "$p" in + strict|permissive|none) printf '%s' "$p" ;; + *) printf 'permissive (fallback from "%s")' "${CSP_POLICY-}" ;; + esac +} + +# True when the server will send a CSP whose connect-src omits the Figwheel +# websocket, so hot reload fails silently. dev_mode_blocks_figwheel() { - local policy="${CSP_POLICY:-strict}" - # "permissive" is not permissive about this: permissive-csp-settings sets - # default-src 'self' and NO connect-src, so connect-src falls back to 'self' - # and ws://localhost:3449 is blocked exactly as under strict. Only "none" - # sends no policy at all. - case "$policy" in strict|permissive) ;; *) return 1 ;; esac - # Mirror the SERVER exactly. config/dev-mode? is (.equalsIgnoreCase "true"), - # so ONLY the literal "true" turns CSP off; every other value leaves it - # enforcing and blocks ws://localhost:3449. Guessing at other truthy-looking - # spellings here is what makes the warning lie -- an earlier revision treated - # yes/1/empty/typos as non-blocking, so the server sent enforcing CSP while - # this stayed quiet, which is the silent hot-reload failure it exists to warn - # about. - # - # UNSET is the one case that is not a value: every server path launches - # `lein with-profile +dev,+start-server`, and :dev supplies - # :env {:dev-mode "true"} (project.clj:244). An EXPLICIT empty string IS a - # value, and it overrides the profile. - # - # Measured against `lein with-profile +dev`, not reasoned about: + local policy + policy="$(_csp_policy_token)" + + # Mirror config/get-secure-headers-config, which is a three-way cond and not + # a two-way one: # - # DEV_MODE unset -> env :dev-mode "true" dev-mode? true does not block - # DEV_MODE=true -> "true" dev-mode? true does not block - # DEV_MODE=TRUE -> "TRUE" dev-mode? true does not block - # DEV_MODE=yes -> "yes" dev-mode? FALSE blocks - # DEV_MODE=1 -> "1" dev-mode? FALSE blocks - # DEV_MODE= -> "" dev-mode? FALSE blocks - # DEV_MODE=tru -> "tru" dev-mode? FALSE blocks - # DEV_MODE=false -> "false" dev-mode? FALSE blocks + # none -> :content-security-policy-settings nil, no CSP at all + # strict -> settings nil; the NONCE INTERCEPTOR sets an enforcing + # header, but only when dev-mode? is false. The one policy + # DEV_MODE affects. + # ANY OTHER -> permissive-csp-settings, applied STATICALLY by Pedestal. + # That includes "permissive" and every unrecognised value. + # It has default-src 'self' and no connect-src, so the + # Figwheel websocket is blocked -- and DEV_MODE cannot + # change it, because the nonce interceptor is inert here. # - # ${DEV_MODE+x}, not ${DEV_MODE:-}: the latter collapses unset and empty, - # and the table above shows those two disagree. + # An earlier revision tested `strict|permissive` and then applied the + # DEV_MODE logic to both, so CSP_POLICY=permissive with DEV_MODE=true was + # reported as fine while the server blocked the socket. + case "$policy" in + none) return 1 ;; + strict) ;; + *) return 0 ;; + esac + + # Strict only, from here. See the measured table in the commit that added + # this: unset means the :dev profile supplies "true"; an explicit empty + # string is a value and overrides it; only the literal "true" disables CSP. [ -z "${DEV_MODE+x}" ] && return 1 - case "$(printf '%s' "$DEV_MODE" | tr '[:upper:]' '[:lower:]')" in + case "$(printf '%s' "$DEV_MODE" | LC_ALL=C tr 'A-Z' 'a-z')" in true) return 1 ;; *) return 0 ;; esac @@ -211,9 +244,21 @@ offer_env_file() { confirm_dev_mode() { dev_mode_blocks_figwheel || return 0 - log_warn "CSP is strict and ENFORCING (DEV_MODE=${DEV_MODE:-})." + local policy + policy="$(effective_csp_policy)" + log_warn "CSP policy is $policy and ENFORCING (DEV_MODE=${DEV_MODE:-})." log_warn "ws://localhost:$FIGWHEEL_PORT is not in connect-src, so hot reload" - log_warn "will silently not work. Set DEV_MODE=true in .env to develop." + log_warn "will silently not work." + # The remedy differs by policy, and the old text gave the strict one for all + # of them. Under permissive (and any unrecognised value, which becomes + # permissive) the policy is applied statically by Pedestal and DEV_MODE has + # no effect at all, so "set DEV_MODE=true" is advice that cannot work. + case "$policy" in + strict) log_warn "Set DEV_MODE=true in .env to develop." ;; + *) log_warn "DEV_MODE does not affect this policy -- it is applied" + log_warn "statically. Use CSP_POLICY=strict with DEV_MODE=true," + log_warn "or CSP_POLICY=none." ;; + esac is_interactive || { log_warn "Continuing anyway (non-interactive)."; return 0; } From db2f7ee7203877d716e325c52ae050c67db4570b Mon Sep 17 00:00:00 2001 From: codeGlaze Date: Mon, 21 Sep 2026 05:16:48 +0000 Subject: [PATCH 21/35] Add orcpub.env, convert every env read to it, and make the old pattern fail lint Three review rounds found the same defect in three different disguises, so this stops patching instances and names the rule: A BLANK ENVIRONMENT VALUE IS AN ABSENT ONE. The wrong version reads correctly, which is why it kept being written: (or (env :app-name) "OrcPub") Environ returns "" for an exported-but-empty variable, "" is TRUTHY in Clojure, so the default is never reached. .env.example ships NINE keys with empty values, so this is the documented state of an unset optional setting. What it cost, each measured before being fixed: SIGNATURE= tokens SIGNED AND VERIFIED against the empty string. The guard for exactly this was (when-not jwt-secret ...), a nil check, so a blank secret walked past it, check-auth's (if-not jwt-secret ...) never fired, and forged tokens were accepted. Demonstrated end to end: a token signed with "" was admitted as :username "admin"; it now gets a 500. DATOMIC_URL= get-datomic-uri returned "?password=" -- not a URI. DATOMIC_PASSWORD= "?password=" appended to an otherwise valid URI. CSP_POLICY= silently selected the static permissive policy instead of the documented strict default. EMAIL_SERVER_PORT= (Integer/parseInt "") threw at send time. APP_FIELD_LIMIT_* same, three more parseInt sites in branding.clj. Several sites already guarded with not-empty -- on the System/getenv branch, while leaving the (env ...) branch bare. Environ reads environment variables itself and answers first, so the guard sat on the path that never runs. Being careful was not enough; the care went to the wrong line. Those dead getenv branches are removed rather than fixed. A HELPER ALONE DOES NOT WORK, and this codebase is the evidence: orcpub.config already had a `signature` accessor and routes.clj read (environ/env :signature) raw anyway, four times. That is how the token bug survived. So .clj-kondo/config.edn now marks environ.core/env and System/getenv as :discouraged-var at :level :error, with orcpub.env as the only exempt namespace (plus the two test namespaces that stub environ with with-redefs, exempted individually rather than exempting all of test/). `lein lint` runs with --fail-level error, so reintroducing the pattern fails the build -- verified by reintroducing it and watching lint go to errors: 1. Not an HOF: there is no varying behaviour to parameterise, just one rule. A two-arity function and a flag? predicate cover all 46 call sites. Converted: config.clj, routes.clj, system.clj, email.clj, index.clj, branding.clj (22 sites), integrations.clj, privacy_content.clj, and two tests. SEPARATE BUG, found in the same sweep and fixed here: privacy_content.clj rendered (env :email-access-key) as the public contact address on the privacy page, four times -- and email.clj:81 uses that same variable as the SMTP USERNAME, paired with EMAIL_SECRET_KEY. The privacy page was printing half a credential. Now uses branding/support-email (APP_SUPPORT_EMAIL), which exists for precisely this and was already wired into the branding map. Also reverts scripts/common.sh's ${CSP_POLICY+x} back to ${CSP_POLICY:-strict}. That distinction existed to mirror the server resolving an empty CSP_POLICY to the permissive fallback -- a faithful mirror of a server bug. The server is fixed, so the mirror gets simpler. The shell predicate is re-verified against eleven combinations measured from a live JVM, not against a remembered table: the previous check had hard-coded expectations that this change invalidated. Verified: lein test 225 tests / 1011 assertions / 0 failures (up from 220/985 -- env_test.clj adds 5 tests, 26 assertions). Each of those five FAILS against the naive implementation, checked by reinstating it: 8 failures across all five groups. lein lint errors: 0, warnings: 7, all seven present before this change. shellcheck adds nothing new. --- .clj-kondo/config.edn | 32 +++++++++- scripts/common.sh | 38 +++++------- src/clj/orcpub/config.clj | 24 ++++---- src/clj/orcpub/email.clj | 20 +++---- src/clj/orcpub/env.clj | 64 ++++++++++++++++++++ src/clj/orcpub/fork/branding.clj | 48 ++++++++------- src/clj/orcpub/fork/integrations.clj | 4 +- src/clj/orcpub/fork/privacy_content.clj | 9 ++- src/clj/orcpub/index.clj | 4 +- src/clj/orcpub/routes.clj | 21 +++++-- src/clj/orcpub/system.clj | 5 +- test/clj/orcpub/config_test.clj | 3 +- test/clj/orcpub/env_test.clj | 78 +++++++++++++++++++++++++ test/clj/orcpub/routes_test.clj | 3 +- 14 files changed, 262 insertions(+), 91 deletions(-) create mode 100644 src/clj/orcpub/env.clj create mode 100644 test/clj/orcpub/env_test.clj diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn index af4647fb4..aaa673a37 100644 --- a/.clj-kondo/config.edn +++ b/.clj-kondo/config.edn @@ -3,7 +3,25 @@ :exclude-files "resources/public/js/compiled" :output {:exclude-files [".*resources/public/js/compiled.*" ".*docker/scripts/.*"]} - :linters {;; Enabled at :warning so NEW accidental shadows are caught. + :linters {;; A BLANK ENVIRONMENT VALUE IS AN ABSENT ONE, and (or (env :k) default) + ;; does not implement that: environ returns "" for an exported-but-empty + ;; variable and "" is truthy, so the default never applies. That shape + ;; was written independently at five sites and cost, among others, JWTs + ;; signed and verified against the empty string. + ;; + ;; orcpub.config already had a correct accessor and routes.clj read + ;; (environ/env :signature) raw anyway, four times -- which is why this + ;; is a lint rule and not just a helper. Read through orcpub.env/value. + ;; :error, not :warning -- `lein lint` runs with --fail-level error, so + ;; this is the difference between a rule and a suggestion. The helper + ;; it points at already existed once and was bypassed. + :discouraged-var + {:level :error + environ.core/env + {:message "Use orcpub.env/value -- (or (env :k) d) treats an empty value as set. See orcpub.env."} + java.lang.System/getenv + {:message "Use orcpub.env/value -- environ already reads env vars, and this skips the blank check."}} + ;; Enabled at :warning so NEW accidental shadows are caught. ;; :exclude covers established patterns: core vars used as param names ;; (name, key, type, etc.) and domain terms used as both defs and params ;; (level, ability, armor, etc.) across modifier/option/character code. @@ -110,7 +128,17 @@ (user/with-db)]}} ;; native/cljs and web/cljs are separate source roots; kondo doesn't know ;; about them so ns names appear to mismatch their file paths. - :config-in-ns {orcpub.core {:linters {:namespace-name-mismatch {:level :off}}} + ;; orcpub.env is the one place allowed to read the environment; the whole + ;; point of the namespace is to be the single exception. + :config-in-ns {orcpub.env {:linters {:discouraged-var {:level :off}}} + ;; These two STUB environ.core/env with with-redefs, which is how + ;; you test the accessor and the locale behaviour at all -- kondo + ;; counts the var reference as a use. Exempted by namespace rather + ;; than for all of test/, so an ordinary bare read in a test is + ;; still an error (routes_test.clj had one). + orcpub.env-test {:linters {:discouraged-var {:level :off}}} + orcpub.config-test {:linters {:discouraged-var {:level :off}}} + orcpub.core {:linters {:namespace-name-mismatch {:level :off}}} orcpub.views {:linters {:namespace-name-mismatch {:level :off}}} orcpub.dnd.e5.native-views {:linters {:namespace-name-mismatch {:level :off}}}} ;; with-conn macros use bare symbol bindings — handled via diff --git a/scripts/common.sh b/scripts/common.sh index 36f3b5178..8bb8ea6ef 100755 --- a/scripts/common.sh +++ b/scripts/common.sh @@ -62,34 +62,26 @@ print_env_config() { fi } -# The policy the SERVER will actually use, normalised the way -# config/get-csp-policy does (Locale/ROOT lowercase). An unrecognised value is -# not an error there: get-secure-headers-config cond-falls through to -# permissive-csp-settings, so report what it BECOMES, not what was typed. +# The policy token as the server resolves it: config/get-csp-policy is now +# (env/value :csp-policy "strict"), and orcpub.env/value treats blank as absent. +# So ${CSP_POLICY:-strict} is correct again -- unset and empty both mean strict. # -# LC_ALL=C on the tr is load-bearing, and is the same defect this branch exists -# for: tr '[:upper:]' '[:lower:]' uses the shell's locale, so on a Turkish -# machine "STRICT" folds to "strıct" (dotless i), matches nothing, and gets -# reported as the fallback. The server avoids this with Locale/ROOT. -# The policy token exactly as the server resolves it, before any interpretation. +# This briefly used ${CSP_POLICY+x} to distinguish them, because the server used +# to resolve an empty CSP_POLICY to "" and fall through to the static permissive +# policy. That was a faithful mirror of a server bug; the server was the thing to +# fix, and the mirror got simpler when it was. # -# ${CSP_POLICY+x}, not ${CSP_POLICY:-strict}. The server does -# (or (env :csp-policy) (System/getenv "CSP_POLICY") "strict") -# and the empty string is TRUTHY in Clojure, so CSP_POLICY= resolves to "" -- -# not to "strict". Only a genuinely absent variable defaults. Bash's :- collapses -# unset and empty, which would call an empty CSP_POLICY "strict"; strict is the -# one policy DEV_MODE can switch off, so an empty value would be reported as -# safe while the server applied the static permissive policy. Measured: with -# CSP_POLICY= and DEV_MODE=true the server BLOCKS and the :- version said it did -# not. +# LC_ALL=C on the tr is load-bearing, and is this branch's own subject matter: +# tr '[:upper:]' '[:lower:]' folds using the shell's locale, so on a Turkish +# machine "STRICT" becomes "strıct" and matches nothing. config/get-csp-policy +# avoids the same trap with Locale/ROOT. _csp_policy_token() { - if [ -z "${CSP_POLICY+x}" ]; then - printf 'strict' - else - printf '%s' "$CSP_POLICY" | LC_ALL=C tr 'A-Z' 'a-z' - fi + printf '%s' "${CSP_POLICY:-strict}" | LC_ALL=C tr 'A-Z' 'a-z' } +# The policy the SERVER will actually USE. An unrecognised value is not an error +# there: get-secure-headers-config cond-falls through to permissive-csp-settings, +# so report what it BECOMES, not what was typed. effective_csp_policy() { local p p="$(_csp_policy_token)" diff --git a/src/clj/orcpub/config.clj b/src/clj/orcpub/config.clj index 37ff84093..b62e6e007 100644 --- a/src/clj/orcpub/config.clj +++ b/src/clj/orcpub/config.clj @@ -1,5 +1,5 @@ (ns orcpub.config - (:require [environ.core :refer [env]] + (:require [orcpub.env :as env] [clojure.string :as str] [clojure.java.io :as io]) (:import [java.util Locale])) @@ -15,23 +15,20 @@ (not-empty (str/trim (slurp f)))))) (defn datomic-env - "Return the raw DATOMIC_URL environment value or nil if unset." [] - (or (env :datomic-url) - (some-> (System/getenv "DATOMIC_URL") not-empty))) + "Return the raw DATOMIC_URL environment value or nil if unset or blank." [] + (env/value :datomic-url)) (defn datomic-password "Return DATOMIC_PASSWORD from Docker secret, env var, or nil. Resolution order: /run/secrets/datomic_password > DATOMIC_PASSWORD env var." [] (or (read-secret "datomic_password") - (env :datomic-password) - (some-> (System/getenv "DATOMIC_PASSWORD") not-empty))) + (env/value :datomic-password))) (defn signature "Return SIGNATURE from Docker secret, env var, or nil. Resolution order: /run/secrets/signature > SIGNATURE env var." [] (or (read-secret "signature") - (env :signature) - (some-> (System/getenv "SIGNATURE") not-empty))) + (env/value :signature))) (defn get-datomic-uri "Return the Datomic URI from the environment or the default. @@ -69,9 +66,12 @@ (defn get-csp-policy "Return the CSP policy from CSP_POLICY env var. Defaults to 'strict'." [] - (let [policy (or (env :csp-policy) - (System/getenv "CSP_POLICY") - "strict")] + ;; env-value, so an EMPTY CSP_POLICY means unset and therefore "strict". + ;; It used to mean "": not strict, not none, so get-secure-headers-config + ;; fell through to the static permissive policy. An empty setting silently + ;; selecting a DIFFERENT and less strict policy than the documented default + ;; is the opposite of what the blank was meant to express. + (let [policy (env/value :csp-policy "strict")] ;; Locale/ROOT, not str/lower-case: this is an ASCII config token, not ;; prose. str/lower-case folds using the default locale, so on a Turkish ;; machine "STRICT" becomes "strıct" (dotless i), misses every comparison @@ -87,7 +87,7 @@ ;; rules, so it is immune to the Turkish-I problem described above. Note the ;; receiver order: the literal is first so a nil env var returns false ;; instead of throwing. - (.equalsIgnoreCase "true" (or (env :dev-mode) ""))) + (env/flag? :dev-mode)) (defn strict-csp? "Returns true when CSP_POLICY=strict (regardless of dev mode). diff --git a/src/clj/orcpub/email.clj b/src/clj/orcpub/email.clj index c6e542e62..3317cdc61 100644 --- a/src/clj/orcpub/email.clj +++ b/src/clj/orcpub/email.clj @@ -6,7 +6,7 @@ handling to prevent silent failures when the SMTP server is unavailable." (:require [hiccup2.core :as hiccup] [postal.core :as postal] - [environ.core :as environ] + [orcpub.env :as env] [clojure.pprint :as pprint] [clojure.string :as s] [orcpub.route-map :as routes] @@ -77,16 +77,16 @@ (defn email-cfg [] (try - {:user (environ/env :email-access-key) - :pass (environ/env :email-secret-key) - :host (environ/env :email-server-url) - :port (Integer/parseInt (or (environ/env :email-server-port) "587")) - :ssl (or (str/to-bool (environ/env :email-ssl)) nil) - :tls (or (str/to-bool (environ/env :email-tls)) nil)} + {:user (env/value :email-access-key) + :pass (env/value :email-secret-key) + :host (env/value :email-server-url) + :port (Integer/parseInt (env/value :email-server-port "587")) + :ssl (or (str/to-bool (env/value :email-ssl)) nil) + :tls (or (str/to-bool (env/value :email-tls)) nil)} (catch NumberFormatException e (throw (ex-info "Invalid email server port configuration. Expected a number." {:error :invalid-port - :port (environ/env :email-server-port)} + :port (env/value :email-server-port)} e))))) (defn emailfrom @@ -363,7 +363,7 @@ - Throttles: one email per unique error fingerprint per 5 minutes - Extracts Pedestal interceptor metadata as a separate section" [context exception] - (when (not-empty (environ/env :email-errors-to)) + (when (env/value :email-errors-to) (let [data-map (ex-data exception) pedestal? (pedestal-wrapper? data-map) real-ex (if pedestal? (:exception data-map) exception) @@ -378,7 +378,7 @@ (let [result (postal/send-message (email-cfg) {:from (str branding/app-name " Errors <" (emailfrom) ">") - :to (str (environ/env :email-errors-to)) + :to (str (env/value :email-errors-to)) :subject (email-subject real-ex request) :body [{:type "text/plain" :content (build-body request real-ex pedestal-meta)}]})] diff --git a/src/clj/orcpub/env.clj b/src/clj/orcpub/env.clj new file mode 100644 index 000000000..9bd5965a0 --- /dev/null +++ b/src/clj/orcpub/env.clj @@ -0,0 +1,64 @@ +(ns orcpub.env + "The one way to read an environment value. + + Exists because the same defect was found independently at five sites, which + is what a missing abstraction looks like. The rule is one line -- A BLANK + VALUE IS AN ABSENT VALUE -- and it is not the obvious thing to write: + + (or (env :app-name) \"OrcPub\") + + reads correctly and is wrong. Environ returns \"\" for a variable that is + exported but empty, and \"\" is TRUTHY in Clojure, so it wins the `or` and the + default never applies. .env.example ships nine keys with empty values, so + this is the documented state of an unset optional setting, not an edge case. + + What that cost, measured before this namespace existed: + + SIGNATURE= tokens signed and VERIFIED against the empty string. The + guard meant to catch it was (when-not jwt-secret ...), a + nil check, so a blank secret walked past it and forged + tokens were accepted. See routes.clj. + DATOMIC_URL= get-datomic-uri returned \"?password=\" -- not a URI. + DATOMIC_PASSWORD= \"?password=\" appended to an otherwise valid URI. + CSP_POLICY= silently selected the permissive fallback rather than the + documented strict default. + EMAIL_SERVER_PORT= (Integer/parseInt \"\") threw at send time. + + Several of those sites already guarded with not-empty -- on the System/getenv + branch, while leaving the (env ...) branch bare. Environ reads environment + variables itself and answers first, so the guard sat on the path that never + runs. Being careful was not enough; the care went to the wrong line. + + A helper alone does not fix this. orcpub.config already had a `signature` + accessor and routes.clj read (environ/env :signature) raw anyway, four times, + which is how the token bug survived. So .clj-kondo/config.edn marks + environ.core/env and System/getenv as discouraged everywhere except here, and + `lein lint` fails the build on them. The rule is the enforcement; this + namespace is only where the exception lives. + + Values are trimmed, matching config/read-secret: a value that is only + whitespace is not one." + (:require [clojure.string :as str] + [environ.core :as environ])) + +(defn value + "The environment value for `k`, or nil when unset, empty or whitespace. + + With `default`, returns it in place of nil. Prefer this over (or (value k) d) + so the blank rule is applied before the default, not after." + ([k] + (some-> (environ/env k) str/trim not-empty)) + ([k default] + (or (value k) default))) + +(defn flag? + "True when `k` is exactly \"true\", case-insensitively. Everything else -- \"yes\", + \"1\", blank, a typo -- is false. + + Matches the server's own comparison so callers cannot invent their own + truthiness. equalsIgnoreCase compares per character rather than by locale + casing rules, so it is immune to the Turkish dotless-i that this branch + exists to fix; the literal goes first so a nil value returns false rather + than throwing." + [k] + (.equalsIgnoreCase "true" (or (value k) ""))) diff --git a/src/clj/orcpub/fork/branding.clj b/src/clj/orcpub/fork/branding.clj index 18e6a4fb2..d226e6480 100644 --- a/src/clj/orcpub/fork/branding.clj +++ b/src/clj/orcpub/fork/branding.clj @@ -5,69 +5,67 @@ Server-side (.clj) is the source of truth. Client-side branding is delivered via the config bridge: index.clj injects client-config as window.__BRANDING__ JSON in , and branding.cljs reads it." - (:require [environ.core :refer [env]]) + (:require [orcpub.env :as env]) (:import [java.time Year])) ;; ─── App Identity ────────────────────────────────────────────────── (def app-name "Full display name. Used in emails, OG tags, page titles." - (or (env :app-name) "OrcPub")) + (env/value :app-name "OrcPub")) (def app-tagline "One-line description for OG/meta tags." - (or (env :app-tagline) - "D&D 5e character builder/generator and digital character sheet far beyond any other in the multiverse.")) + (env/value :app-tagline "D&D 5e character builder/generator and digital character sheet far beyond any other in the multiverse.")) (def app-url "Primary application URL for legal pages and external references. Empty = hidden." - (or (env :app-url) "")) + (env/value :app-url "")) (def default-page-title "Default and og:title when no page-specific title is set." - (or (env :app-page-title) - (str app-name ": D&D 5e Character Builder/Generator"))) + (env/value :app-page-title (str app-name ": D&D 5e Character Builder/Generator"))) ;; ─── Logos & Images ──────────────────────────────────────────────── (def logo-path "Path to the main SVG logo (splash page, header, privacy page)." - (or (env :app-logo-path) "/image/orcpub-logo.svg")) + (env/value :app-logo-path "/image/orcpub-logo.svg")) (def og-image-filename "Filename for the OG meta image (social sharing preview). Combined with the request host to form the full URL." - (or (env :app-og-image) "/image/orcpub-logo.png")) + (env/value :app-og-image "/image/orcpub-logo.png")) ;; ─── Copyright ───────────────────────────────────────────────────── (def copyright-holder "Entity name shown in legal footer." - (or (env :app-copyright-holder) "OrcPub")) + (env/value :app-copyright-holder "OrcPub")) (def copyright-year "Copyright year string. Defaults to the current year." - (or (env :app-copyright-year) (str (.getValue (Year/now))))) + (env/value :app-copyright-year (str (.getValue (Year/now))))) ;; ─── Email ───────────────────────────────────────────────────────── (def email-sender-name "Display name for outbound emails (verification, password reset)." - (or (env :app-email-sender-name) (str app-name " Team"))) + (env/value :app-email-sender-name (str app-name " Team"))) (def email-from-address "From address for outbound emails. Falls back to env EMAIL_FROM_ADDRESS." - (or (env :email-from-address) "no-reply@orcpub.com")) + (env/value :email-from-address "no-reply@orcpub.com")) ;; ─── Support & Help ────────────────────────────────────────────── (def support-email "Contact email shown on privacy page, error messages, etc. Empty = hidden." - (or (env :app-support-email) "")) + (env/value :app-support-email "")) (def help-url "URL for the help/FAQ page. Empty string = hidden." - (or (env :app-help-url) "")) + (env/value :app-help-url "")) ;; ─── Social Links ────────────────────────────────────────────────── ;; Each link appears in the header/footer when non-empty. @@ -77,18 +75,18 @@ (def social-links "Map of social platform links. Empty string = hidden." - {:patreon (or (env :app-social-patreon) "") - :facebook (or (env :app-social-facebook) "") - :bluesky (or (env :app-social-bluesky) "") - :twitter (or (env :app-social-twitter) "") - :reddit (or (env :app-social-reddit) "") - :discord (or (env :app-social-discord) "")}) + {:patreon (env/value :app-social-patreon "") + :facebook (env/value :app-social-facebook "") + :bluesky (env/value :app-social-bluesky "") + :twitter (env/value :app-social-twitter "") + :reddit (env/value :app-social-reddit "") + :discord (env/value :app-social-discord "")}) ;; ─── Footer ───────────────────────────────────────────────────── (def copyright-url "URL for copyright holder name in footer. Empty string = plain text." - (or (env :app-copyright-url) "")) + (env/value :app-copyright-url "")) ;; ─── UI Behavior ──────────────────────────────────────────────── @@ -105,9 +103,9 @@ (def field-limits "Max-length constraints for form input fields." - {:notes (or (some-> (env :app-field-limit-notes) Integer/parseInt) 50000) - :text (or (some-> (env :app-field-limit-text) Integer/parseInt) 255) - :number (or (some-> (env :app-field-limit-number) Integer/parseInt) 7)}) + {:notes (or (some-> (env/value :app-field-limit-notes) Integer/parseInt) 50000) + :text (or (some-> (env/value :app-field-limit-text) Integer/parseInt) 255) + :number (or (some-> (env/value :app-field-limit-number) Integer/parseInt) 7)}) ;; ─── Client-Side Config Bridge ─────────────────────────────────── ;; index.clj injects this as window.__BRANDING__ JSON in <head>. diff --git a/src/clj/orcpub/fork/integrations.clj b/src/clj/orcpub/fork/integrations.clj index 2ac4269b0..c93353a64 100644 --- a/src/clj/orcpub/fork/integrations.clj +++ b/src/clj/orcpub/fork/integrations.clj @@ -2,12 +2,12 @@ "Optional third-party <head> integrations. Configure via environment variables; disabled when unset. Fork overrides: uncomment examples and add real service config." - (:require [environ.core :refer [env]])) + (:require [orcpub.env :as env])) ;; ─── How to add an integration ─────────────────────────────────────── ;; ;; 1. Define env-var-gated config: -;; (def my-service-id (env :my-service-id)) +;; (def my-service-id (env/value :my-service-id)) ;; ;; 2. Write a tag function that returns hiccup (or nil when disabled): ;; (defn- my-service-tag [nonce] diff --git a/src/clj/orcpub/fork/privacy_content.clj b/src/clj/orcpub/fork/privacy_content.clj index f49e08ea7..51e65eebf 100644 --- a/src/clj/orcpub/fork/privacy_content.clj +++ b/src/clj/orcpub/fork/privacy_content.clj @@ -2,7 +2,6 @@ "Fork-specific privacy policy content. Public/community edition: standard privacy policy." (:require [clojure.string :as s] - [environ.core :as environ] [orcpub.fork.branding :as branding])) (def privacy-policy-section @@ -59,8 +58,8 @@ {:title "What choices do you have about your information?" :font-size 32 :paragraphs - (if (not (s/blank? (environ/env :email-access-key))) - ["You may close your account at any time by emailing " (environ/env :email-access-key) (str "We will then inactivate your account and remove your content from " branding/app-name ". We may retain archived copies of you information as required by law or for legitimate business purposes (including to help address fraud and spam). ")] + (if (not (s/blank? branding/support-email)) + ["You may close your account at any time by emailing " branding/support-email (str "We will then inactivate your account and remove your content from " branding/app-name ". We may retain archived copies of you information as required by law or for legitimate business purposes (including to help address fraud and spam). ")] [(str "You may remove any content you create from " branding/app-name " at any time, although we may retain archived copies of the information. You may also disable sharing of content you create at any time, whether publicly shared or privately shared with specific users.") "Also, we support the Do Not Track browser setting."])} {:title "Our policy on children's information" @@ -71,8 +70,8 @@ :font-size 32 :paragraphs [(str "We may change this policy from time to time, and if we do we'll post any changes on this page. If you continue to use " branding/app-name " after those changes are in effect, you agree to the revised policy. If the changes are significant, we may provide more prominent notice or get your consent as required by law.")]} - (when (not (s/blank? (environ/env :email-access-key))) + (when (not (s/blank? branding/support-email)) {:title "How can you contact us?" :font-size 32 :paragraphs - ["You can contact us by emailing " (environ/env :email-access-key) ]})]}) + ["You can contact us by emailing " branding/support-email ]})]}) diff --git a/src/clj/orcpub/index.clj b/src/clj/orcpub/index.clj index 3bf286c61..4523da422 100644 --- a/src/clj/orcpub/index.clj +++ b/src/clj/orcpub/index.clj @@ -6,13 +6,13 @@ [orcpub.dnd.e5.views-2 :as views-2] [orcpub.favicon :as fi] [orcpub.fork.integrations :as integrations] - [environ.core :refer [env]])) + [orcpub.env :as env])) (def homebrew-url "URL to fetch server-hosted .orcbrew plugins from on first load. Set LOAD_HOMEBREW_URL to enable (e.g. \"/homebrew.orcbrew\" or a full URL). When unset, no fetch is attempted — plugins come only from local imports." - (env :load-homebrew-url)) + (env/value :load-homebrew-url)) (defn meta-tag [property content] (when content diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index 3e104a68c..ad1109848 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -46,6 +46,7 @@ [orcpub.routes.folder :as folder] [hiccup.page :as page] [environ.core :as environ] + [orcpub.env :as env] [clojure.set :as sets] [ring.middleware.head :as head] [ring.util.codec :as codec] @@ -66,8 +67,18 @@ (def ^:private jwt-secret "JWT signing secret from SIGNATURE env var. - nil when unset — check-auth returns 500 with a diagnostic message." - (environ/env :signature)) + nil when unset OR BLANK — check-auth returns 500 with a diagnostic message. + + The blank check is load-bearing, and its absence defeated the warning below. + An exported-but-empty SIGNATURE= yields \"\", which is TRUTHY in Clojure, so + it sailed past `when-not jwt-secret` — the guard written for exactly this — + and was handed to buddy. Measured: buddy signs AND verifies with \"\" without + complaint, so the app issued and accepted tokens signed with a publicly + known empty secret, and anyone could forge one for any user. + + Clearing a line in .env is an ordinary thing to do; .env.example ships nine + keys with empty values." + (env/value :signature)) (when-not jwt-secret (println "WARNING: SIGNATURE env var is not set — all authenticated API calls will fail")) @@ -230,7 +241,7 @@ (defn create-token [username exp] (jwt/sign {:user username :exp exp} - (environ/env :signature))) + jwt-secret)) (defn following-usernames [db ids] (map :orcpub.user/username @@ -457,7 +468,7 @@ Stateless — no DB storage needed. Verified by checking JWT signature." [email] (jwt/sign {:email (s/lower-case email) :action "unsubscribe"} - (environ/env :signature))) + jwt-secret)) (defn unsubscribe "GET handler for /unsubscribe?token=<jwt>. @@ -468,7 +479,7 @@ (if (s/blank? token) {:status 400 :body "Missing token"} (try - (let [{:keys [email action]} (jwt/unsign token (environ/env :signature))] + (let [{:keys [email action]} (jwt/unsign token jwt-secret)] (if (not= "unsubscribe" action) {:status 400 :body "Invalid token"} (let [{:keys [:db/id]} (user-for-email (d/db conn) email)] diff --git a/src/clj/orcpub/system.clj b/src/clj/orcpub/system.clj index e8317f0ab..d1cc240b2 100644 --- a/src/clj/orcpub/system.clj +++ b/src/clj/orcpub/system.clj @@ -1,5 +1,5 @@ (ns orcpub.system - (:require [clojure.string :as s] + (:require [orcpub.env :as env] [com.stuartsierra.component :as component] [reloaded.repl :as rrepl] [io.pedestal.http :as http] @@ -24,8 +24,7 @@ ;; threw here -- and because both service maps are top-level defs, that throws ;; while the namespace LOADS, not when the server starts. An empty value in a ;; config template is a normal thing for a user to leave behind. - (let [raw (System/getenv "PORT") - port-str (if (s/blank? raw) "8890" (s/trim raw))] + (let [port-str (env/value :port "8890")] (try (Integer/parseInt port-str) (catch NumberFormatException e diff --git a/test/clj/orcpub/config_test.clj b/test/clj/orcpub/config_test.clj index ca579574e..12dcff46f 100644 --- a/test/clj/orcpub/config_test.clj +++ b/test/clj/orcpub/config_test.clj @@ -7,7 +7,8 @@ PERMISSIVE policy -- a security downgrade nobody asked for and nothing reports. These tests run under a Turkish locale on purpose." (:require [clojure.test :refer [deftest testing is use-fixtures]] - [orcpub.config :as config]) + [orcpub.config :as config] + [environ.core]) (:import [java.util Locale])) (def ^:private turkish (Locale/forLanguageTag "tr-TR")) diff --git a/test/clj/orcpub/env_test.clj b/test/clj/orcpub/env_test.clj new file mode 100644 index 000000000..ded2b4bae --- /dev/null +++ b/test/clj/orcpub/env_test.clj @@ -0,0 +1,78 @@ +(ns orcpub.env-test + "Pins the one rule orcpub.env exists to enforce: A BLANK VALUE IS ABSENT. + + Worth pinning rather than trusting, because the wrong version reads correctly. + (or (env :k) default) looks like it applies the default when the variable is + not set, and does not: environ returns \"\" for an exported-but-empty variable + and \"\" is truthy in Clojure, so the default is never reached. That shape was + written independently at five sites in this codebase. + + Each test below fails against the naive implementation. That is the point -- + a test that passes either way would not have caught any of the five." + (:require [clojure.test :refer [deftest testing is]] + [environ.core] + [orcpub.env :as env])) + +(defn- with-env + "Run f with environ/env redefined to m. Redefining environ rather than the + process environment because Java cannot set its own env vars." + [m f] + (with-redefs [environ.core/env m] (f))) + +(deftest blank-counts-as-absent + (testing "unset" + (with-env {} #(is (nil? (env/value :missing))))) + + (testing "empty string -- the case (or (env :k) d) gets wrong" + (with-env {:k ""} #(is (nil? (env/value :k))))) + + (testing "whitespace only, matching config/read-secret's trim" + (with-env {:k " "} #(is (nil? (env/value :k)))) + (with-env {:k "\t\n"} #(is (nil? (env/value :k)))))) + +(deftest real-values-survive + (testing "a value is returned unchanged" + (with-env {:k "hunter2"} #(is (= "hunter2" (env/value :k))))) + + (testing "surrounding whitespace is trimmed, inner whitespace is not" + (with-env {:k " a b "} #(is (= "a b" (env/value :k))))) + + (testing "a value that merely looks empty is still a value" + (with-env {:k "0"} #(is (= "0" (env/value :k)))) + (with-env {:k "false"} #(is (= "false" (env/value :k)))))) + +(deftest default-applies-to-blank-not-just-unset + (testing "unset takes the default" + (with-env {} #(is (= "8890" (env/value :port "8890"))))) + + (testing "EMPTY takes the default too -- the whole reason this namespace exists" + (with-env {:port ""} #(is (= "8890" (env/value :port "8890")))) + (with-env {:port " "} #(is (= "8890" (env/value :port "8890"))))) + + (testing "a real value beats the default" + (with-env {:port "9000"} #(is (= "9000" (env/value :port "8890")))))) + +(deftest flag-matches-the-servers-own-comparison + (testing "only the literal true, any case" + (doseq [v ["true" "TRUE" "True" " true "]] + (with-env {:f v} #(is (true? (env/flag? :f)) (str "expected true for " (pr-str v)))))) + + (testing "every other value is false, including the truthy-LOOKING ones" + (doseq [v ["yes" "1" "on" "tru" "false" "" " "]] + (with-env {:f v} #(is (false? (env/flag? :f)) (str "expected false for " (pr-str v)))))) + + (testing "unset is false" + (with-env {} #(is (false? (env/flag? :f)))))) + +(deftest turkish-locale-does-not-change-the-answer + ;; The branch this lands on exists because of locale-dependent case folding. + ;; flag? uses equalsIgnoreCase, which compares per character rather than by + ;; locale casing rules, so "TRUE" must not fold to "tr�e" or similar under tr-TR. + (let [saved (java.util.Locale/getDefault)] + (try + (java.util.Locale/setDefault (java.util.Locale/forLanguageTag "tr-TR")) + (with-env {:f "TRUE"} #(is (true? (env/flag? :f)) + "TRUE must still read as true on a Turkish JVM")) + (with-env {:k " STRICT "} #(is (= "STRICT" (env/value :k)) + "value trims but must not case-fold at all")) + (finally (java.util.Locale/setDefault saved))))) diff --git a/test/clj/orcpub/routes_test.clj b/test/clj/orcpub/routes_test.clj index 843ae5216..7e2697129 100644 --- a/test/clj/orcpub/routes_test.clj +++ b/test/clj/orcpub/routes_test.clj @@ -8,6 +8,7 @@ [buddy.sign.jwt :as jwt] [environ.core :as environ] [orcpub.routes :as routes] + [orcpub.env :as env] [orcpub.dnd.e5.magic-items :as mi] [orcpub.dnd.e5.character :as char5e] [orcpub.modifiers :as mod] @@ -257,7 +258,7 @@ (deftest test-unsubscribe-token-roundtrip (testing "Token encodes email and action, verifiable with signature" (let [token (routes/unsubscribe-token "Test@Example.com") - claims (jwt/unsign token (environ/env :signature))] + claims (jwt/unsign token (env/value :signature))] (is (= "test@example.com" (:email claims)) "Email should be lowercased") (is (= "unsubscribe" (:action claims)))))) From f412602c74dcc60a3133615e6d361ea71025f93e Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Mon, 21 Sep 2026 06:09:40 +0000 Subject: [PATCH 22/35] Keep postal's inputs as empty strings: orcpub.env regressed the Docker default docker-compose.yaml passes seven variables as ${VAR:-} -- explicitly EMPTY -- whenever the operator configures nothing, which is the default deployment. Three of them feed straight into postal, and turning "" into nil there made things worse rather than better. Measured: :host "" -> MailConnectException: Couldn't connect to host, port: localhost, 587 :host nil -> NullPointerException Both fail, as they should when email is unconfigured, but one says why. The nil also changes what postal does with an empty :user/:pass versus nil for SMTP AUTH, which is not this branch's business to alter. email-cfg now defaults those three to "" explicitly, which is byte-identical to the pre-orcpub.env behaviour. The blank rule is right everywhere else; here the old value was load-bearing for a third-party library, and the comment says so. Recorded, not fixed here: nothing checks whether email is configured at all. .env.example says "Leave EMAIL_SERVER_URL empty to disable email functionality" and no code implements it -- "disabled" currently means "sending throws". The error-report path is gated on EMAIL_ERRORS_TO, so it is unaffected either way. Verified by running the full config surface under the EXACT environment docker-compose.yaml provides, against 813fdbb8 (the commit before orcpub.env). Twelve of fourteen values are now identical, including datomic-uri, datomic-password, signature, csp-policy, strict-csp?, dev-mode?, secure-headers, both ports, email-cfg, app-name and field-limits. The two that differ are both fixes, and both only reachable in Docker: email-from "" -> "no-reply@orcpub.com" EMAIL_FROM_ADDRESS is passed empty, so the branding fallback never applied and every Docker deployment sent mail with an EMPTY From address. homebrew-url "" -> nil index.clj:163 is (when homebrew-url ...) and "" is truthy, so the rendered page carried fetch('') -- an empty URL resolves to the current page, meaning every page load re-fetched itself and tried to parse the HTML as an .orcbrew pack. Confirmed by rendering the index at both commits: before: fetch('') after: none Also confirmed the containers cannot be affected by this branch's shell work: the app image is ENTRYPOINT java -jar with no scripts/ copied in, the transactor runs deploy/start.sh, and deploy/ and docker/ are untouched by the entire branch. scripts/common.sh and scripts/start.sh are host-side dev tooling only. lein test 225 tests / 1011 assertions / 0 failures; lein lint errors 0. --- src/clj/orcpub/email.clj | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/src/clj/orcpub/email.clj b/src/clj/orcpub/email.clj index 3317cdc61..4f176c7ac 100644 --- a/src/clj/orcpub/email.clj +++ b/src/clj/orcpub/email.clj @@ -77,9 +77,23 @@ (defn email-cfg [] (try - {:user (env/value :email-access-key) - :pass (env/value :email-secret-key) - :host (env/value :email-server-url) + ;; "" defaults on purpose, NOT nil. docker-compose.yaml passes all three as + ;; ${VAR:-} -- explicitly empty -- whenever email is unconfigured, which is + ;; the default deployment. Handing postal nil there changes the failure from + ;; MailConnectException ("Couldn't connect to host, port: localhost, 587") + ;; to a bare NullPointerException, measured. Both fail, but one says why. + ;; + ;; Keeping "" also preserves whatever postal does with an empty :user/:pass + ;; versus nil, which differs for SMTP AUTH. This is deliberately byte-identical + ;; to the pre-orcpub.env behaviour; the blank rule is right everywhere else, + ;; and here the old value was load-bearing for a third-party library. + ;; + ;; The real gap is that nothing checks whether email is configured at all -- + ;; .env.example says "Leave EMAIL_SERVER_URL empty to disable email + ;; functionality" and no code implements that. Worth doing, not here. + {:user (env/value :email-access-key "") + :pass (env/value :email-secret-key "") + :host (env/value :email-server-url "") :port (Integer/parseInt (env/value :email-server-port "587")) :ssl (or (str/to-bool (env/value :email-ssl)) nil) :tls (or (str/to-bool (env/value :email-tls)) nil)} From e4a69649c3bb214ddb4412546da3b4e86166f5e0 Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Mon, 21 Sep 2026 13:22:15 +0000 Subject: [PATCH 23/35] Roll back the account when the verification email fails Registration transacted the user and THEN sent the email. Datomic does not roll back, so a failed send left a committed, unverified account -- and `register` validates against existing username/email, so the retry this very function tells the user to make then failed with "already taken". The address was locked out and the account could never be verified, because no email could ever be sent. That is the default state of any instance without SMTP, including the default docker-compose deployment, where EMAIL_SERVER_URL is passed as ${VAR:-}. The first person to sign up on a fresh self-hosted instance hits it -- usually the operator registering themselves. It went unnoticed because developers create users with `./menu add user`, which calls dev/user.clj's create-user! directly and never touches the email path. Not a design decision: request-email-change already does exactly this, retracting pending-email, verification-key and verification-sent when the send throws, with a test (email-change-test/test-email-send-failure-rolls-back). Registration was simply never brought up to match. This is that pattern, applied to the flow that was missing it. The send still happens AFTER the write, because the emailed link only resolves if the key is already stored. What changes is that the write is undone when the send fails. THE TWO CALLERS NEED DIFFERENT ROLLBACKS, and getting this wrong would be worse than the bug: register passes no :db/id -> new entity -> :db/retractEntity re-verify passes {:db/id id} -> EXISTING USER -> retract only the two attributes this attempt set. Retracting the entity here would delete a real account, turning a failed resend into data loss. A rollback that itself fails is reported but does not mask the original cause. registration_rollback_test.clj covers four cases, and each was verified to fail against the code it is meant to catch: - a failed send leaves no account (failed: found the committed user) - the address can be reused afterwards (failed: retry got 400) - the happy path still creates the account (passed throughout -- the control) - A FAILED RESEND DOES NOT DELETE THE USER (failed with 3 assertions when the rollback was made an unconditional retractEntity) That last sabotage is the one worth keeping: the naive fix passes the first three tests and destroys user accounts. Still open, and a product decision rather than a bug: an instance with no SMTP now fails registration cleanly instead of locking the address, but still cannot register anyone. Whether such an instance should auto-verify, refuse up front with a clear message, or offer an admin path is not something to decide inside a hotfix. .env.example already claims "Leave EMAIL_SERVER_URL empty to disable email functionality" and no code implements that. lein test 229 tests / 1025 assertions / 0 failures (up from 225/1011). lein lint errors 0 -- with-conn added to the :unresolved-symbol excludes beside the two existing test macros of the same shape. --- .clj-kondo/config.edn | 1 + src/clj/orcpub/routes.clj | 64 ++++++-- .../clj/orcpub/registration_rollback_test.clj | 141 ++++++++++++++++++ 3 files changed, 194 insertions(+), 12 deletions(-) create mode 100644 test/clj/orcpub/registration_rollback_test.clj diff --git a/.clj-kondo/config.edn b/.clj-kondo/config.edn index aaa673a37..f0070b8dd 100644 --- a/.clj-kondo/config.edn +++ b/.clj-kondo/config.edn @@ -125,6 +125,7 @@ (orcpub.routes-test/with-conn) (orcpub.routes.folder-test/with-conn) (orcpub.email-change-test/with-conn) + (orcpub.registration-rollback-test/with-conn) (user/with-db)]}} ;; native/cljs and web/cljs are separate source roots; kondo doesn't know ;; about them so ns names appear to mismatch their file paths. diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index ad1109848..9fd86f50f 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -334,23 +334,63 @@ params verification-key)) -(defn do-verification [request params conn & [tx-data]] +(defn do-verification + "Create or refresh a pending verification, then email the link. + + The write has to happen BEFORE the send, because the emailed link only + resolves if the key is already stored. Datomic does not roll back, so the + send is wrapped and the write undone if it fails. + + Without that rollback a failed email left a committed, unverified account, + and `register` validates against existing username/email -- so the retry this + very function tells the user to make then failed with \"already taken\". The + address was locked out and the account could never be verified, because no + email could ever be sent. That is the default state of any instance without + SMTP, including the default docker-compose deployment. + + `request-email-change` already did exactly this (retracting pending-email on + send failure, covered by email-change-test/test-email-send-failure-rolls-back). + Registration was never brought up to match. See + registration_rollback_test.clj and docs/kb/blank-env-values.md." + [request params conn & [tx-data]] (let [verification-key (str (java.util.UUID/randomUUID)) - now (java.util.Date.)] + now (java.util.Date.) + ;; re-verify passes an existing {:db/id id}; register does not. The two + ;; need different rollbacks -- never retract the ENTITY for a user who + ;; already existed, only the attributes this attempt set. + existing-id (:db/id tx-data) + tempid "verification-subject" + report (try + @(d/transact + conn + [(merge + tx-data + {:db/id (or existing-id tempid) + :orcpub.user/verified? false + :orcpub.user/verification-key verification-key + :orcpub.user/verification-sent now})]) + (catch Exception e + (println "ERROR: Failed to create verification record:" (.getMessage e)) + (throw (ex-info "Unable to complete registration. Please try again or contact support." + {:error :verification-failed} + e)))) + eid (or existing-id (get (:tempids report) tempid))] (try - @(d/transact - conn - [(merge - tx-data - {:orcpub.user/verified? false - :orcpub.user/verification-key verification-key - :orcpub.user/verification-sent now})]) (send-verification-email request params verification-key) {:status 200} - (catch Exception e - (println "ERROR: Failed to create verification record:" (.getMessage e)) + (catch Throwable e + (println "ERROR: Verification email failed, rolling back:" (.getMessage e)) + (try + @(d/transact conn (if existing-id + [[:db/retract eid :orcpub.user/verification-key verification-key] + [:db/retract eid :orcpub.user/verification-sent now]] + [[:db/retractEntity eid]])) + (catch Exception re + ;; Report the rollback failure, but surface the original cause. + (println "ERROR: Rollback ALSO failed; a partial account may remain:" + (.getMessage re)))) (throw (ex-info "Unable to complete registration. Please try again or contact support." - {:error :verification-failed} + {:error :verification-email-failed} e)))))) (defn register [{:keys [json-params db conn] :as request}] diff --git a/test/clj/orcpub/registration_rollback_test.clj b/test/clj/orcpub/registration_rollback_test.clj new file mode 100644 index 000000000..1de82e397 --- /dev/null +++ b/test/clj/orcpub/registration_rollback_test.clj @@ -0,0 +1,141 @@ +(ns orcpub.registration-rollback-test + "Registration must not leave an account behind when the verification email fails. + + do-verification transacts the user and THEN sends the email. Datomic does not + roll back, so a failed send left a committed, unverified account -- and since + register validates against existing username/email, the retry it tells the + user to make then fails with \"already taken\". The address is locked out and + the account can never be verified, because no email can ever be sent. + + That is the default state of any instance without SMTP, including the default + docker-compose deployment, where EMAIL_SERVER_URL is passed as ${VAR:-}. + + The sibling flow already solved this: request-email-change transacts, sends, + and retracts on failure, covered by email_change_test/test-email-send-failure- + rolls-back. These tests are that one's mirror for registration. + + See docs/kb/blank-env-values.md." + (:require + [clojure.test :refer [deftest is testing use-fixtures]] + [datomic.api :as d] + [datomock.core :as dm] + [orcpub.errors :as errors] + [orcpub.routes :as routes] + [orcpub.db.schema :as schema]) + (:import [java.util UUID])) + +(use-fixtures :each + (fn [f] + (binding [errors/*error-prefix* "TEST_ERROR:"] + (f)))) + +(defmacro with-conn [conn-binding & body] + `(let [uri# (str "datomic:mem:registration-rollback-test-" (UUID/randomUUID)) + ~conn-binding (do + (d/create-database uri#) + (d/connect uri#))] + (try ~@body + (finally (d/delete-database uri#))))) + +(defn- seed-schema [conn] + @(d/transact conn schema/all-schemas)) + +(defn- find-user [db username] + (when-let [e (d/q '[:find ?e . :in $ ?u :where [?e :orcpub.user/username ?u]] db username)] + (d/pull db '[*] e))) + +(defn- register-request [conn] + {:conn conn + :db (d/db conn) + :scheme :https + :headers {"host" "example.test"} + ;; verify-email is required by registration/validate-registration and must + ;; match :email, or register returns 400 before reaching do-verification. + :json-params {:username "newcomer" + :email "newcomer@test.com" + :verify-email "newcomer@test.com" + :password "hunter2hunter2" + :send-updates? false}}) + +(deftest failed-verification-email-leaves-no-account + (with-conn conn + (let [mocked-conn (dm/fork-conn conn)] + (seed-schema mocked-conn) + (testing "an SMTP failure must not commit a half-created user" + (with-redefs [routes/send-verification-email + (fn [& _] (throw (Exception. "SMTP down")))] + ;; register rethrows; what matters is the DB state afterwards, not + ;; which exception surfaced. + (try (routes/register (register-request mocked-conn)) + (catch Throwable _ nil)) + (let [user (find-user (d/db mocked-conn) "newcomer")] + (is (nil? user) + (str "A failed verification email left an account behind. " + "The user is now locked out: retrying registration fails " + "validation because the username and email are taken, and " + "the account can never be verified. Found: " (pr-str user))))))))) + +(deftest the-address-can-be-reused-after-a-failed-send + (with-conn conn + (let [mocked-conn (dm/fork-conn conn)] + (seed-schema mocked-conn) + (testing "after a failed send, the same details still validate as available" + (with-redefs [routes/send-verification-email + (fn [& _] (throw (Exception. "SMTP down")))] + (try (routes/register (register-request mocked-conn)) + (catch Throwable _ nil))) + ;; Second attempt, this time with a working mailer. + (with-redefs [routes/send-verification-email (fn [& _] nil)] + (let [resp (routes/register (register-request mocked-conn))] + (is (= 200 (:status resp)) + (str "Retrying after a failed send must succeed -- this is the " + "advice the error message gives the user. Got: " (pr-str resp))) + (let [user (find-user (d/db mocked-conn) "newcomer")] + (is (some? user) "the retry should create the account") + (is (false? (:orcpub.user/verified? user)) + "and it should be awaiting verification")))))))) + +(deftest a-successful-send-still-creates-the-account + (with-conn conn + (let [mocked-conn (dm/fork-conn conn)] + (seed-schema mocked-conn) + (testing "the happy path is unchanged by the rollback" + (with-redefs [routes/send-verification-email (fn [& _] nil)] + (let [resp (routes/register (register-request mocked-conn)) + user (find-user (d/db mocked-conn) "newcomer")] + (is (= 200 (:status resp))) + (is (some? user)) + (is (= "newcomer@test.com" (:orcpub.user/email user))) + (is (false? (:orcpub.user/verified? user))) + (is (some? (:orcpub.user/verification-key user)) + "a verification key must be stored for the emailed link to resolve"))))))) + +(deftest re-verify-rollback-must-not-delete-an-existing-user + ;; The dangerous case. re-verify calls do-verification with an EXISTING + ;; {:db/id id}, so rolling back with :db/retractEntity would delete a real + ;; account -- turning a failed resend into data loss. Only the attributes this + ;; attempt set may be retracted. + (with-conn conn + (let [mocked-conn (dm/fork-conn conn)] + (seed-schema mocked-conn) + (with-redefs [routes/send-verification-email (fn [& _] nil)] + (routes/register (register-request mocked-conn))) + (let [before (find-user (d/db mocked-conn) "newcomer")] + (is (some? before) "precondition: the account exists") + (testing "a failed re-send leaves the account intact" + (with-redefs [routes/send-verification-email + (fn [& _] (throw (Exception. "SMTP down")))] + (try (routes/re-verify {:conn mocked-conn + :db (d/db mocked-conn) + :scheme :https + :headers {"host" "example.test"} + :query-params {:email "newcomer@test.com"}}) + (catch Throwable _ nil))) + (let [after (find-user (d/db mocked-conn) "newcomer")] + (is (some? after) + "THE USER WAS DELETED by a failed verification resend") + (is (= (:orcpub.user/email before) (:orcpub.user/email after))) + (is (= (:orcpub.user/password before) (:orcpub.user/password after)) + "credentials must survive a failed resend") + (is (nil? (:orcpub.user/verification-key after)) + "the key from the failed attempt should be retracted"))))))) From 67cb98e08e8577e5b6d5b9bdf7231830e5b0bcc8 Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Mon, 21 Sep 2026 14:48:46 +0000 Subject: [PATCH 24/35] Correct the severity claim: the stuck account is recoverable via resend The previous commit and docs/kb/blank-env-values.md both said a failed verification email left an account that "can never be verified". That is wrong, and it was wrong because I reasoned about the failure instead of running it. Measured against the pre-fix code (f412602c), in-memory Datomic: register -> threw account left behind -> true retry registration -> 400 (the confusing part, as described) RESEND verification -> 200 (recovery WORKS) fresh key stored -> true re-verify operates on exactly the orphaned state and succeeds, and it is wired to a UI button (views.cljs:653 dispatches :re-verify). So the symptom is a confusing dead end with an escape hatch, not a permanent lockout. That also answers why seven years of production never surfaced it: live SMTP works, so the branch only runs on a transient send failure, and the few users it reaches report "it says my email already exists" -- which looks exactly like someone who forgot they had an account. Misattributed, not invisible. The fix stands: a failed send should not leave a half-created account, and "please try again" is the wrong advice when the retry cannot succeed. But it is an ordinary bug, not the emergency the previous commit message implied. lein test 229 tests / 1025 assertions / 0 failures. --- .../clj/orcpub/registration_rollback_test.clj | 21 +++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/test/clj/orcpub/registration_rollback_test.clj b/test/clj/orcpub/registration_rollback_test.clj index 1de82e397..fb7bd27d6 100644 --- a/test/clj/orcpub/registration_rollback_test.clj +++ b/test/clj/orcpub/registration_rollback_test.clj @@ -4,11 +4,24 @@ do-verification transacts the user and THEN sends the email. Datomic does not roll back, so a failed send left a committed, unverified account -- and since register validates against existing username/email, the retry it tells the - user to make then fails with \"already taken\". The address is locked out and - the account can never be verified, because no email can ever be sent. + user to make then fails with \"already taken\". - That is the default state of any instance without SMTP, including the default - docker-compose deployment, where EMAIL_SERVER_URL is passed as ${VAR:-}. + SCOPE, measured rather than assumed. The account is NOT unrecoverable: the + resend-verification route works on exactly this state (verified against the + pre-fix code -- resend returns 200 and stores a fresh key), and it is wired to + a button in the UI. So the real symptom is a confusing dead end that a user + can escape if they find the resend link, not a permanent lockout. An earlier + version of this docstring and of docs/kb/blank-env-values.md said \"can never + be verified\"; that was wrong. + + Which is why seven years of production never surfaced it: live SMTP works, so + this branch only runs on a transient send failure, and the handful of users it + hits report \"it says my email already exists\" -- indistinguishable from + someone who forgot they had an account. + + It is still worth fixing. A failed send should not leave a half-created + account, and \"please try again\" is the wrong advice when retrying cannot + work. The sibling flow already solved this: request-email-change transacts, sends, and retracts on failure, covered by email_change_test/test-email-send-failure- From 50e4c4cd1b054ff40210466999dd8782a80d2b00 Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Mon, 21 Sep 2026 17:57:03 +0000 Subject: [PATCH 25/35] Finish the severity correction: two copies were still wrong The previous commit fixed the ns docstring but left the same overstated claim in two other places, which is how a correction half-lands: routes.clj do-verification docstring -- "the address was locked out and the account could never be verified" registration_rollback_test.clj:89 -- same wording in the assertion message a failure would actually print Both now say what was measured: re-verify operates on the orphaned state, is wired to a UI button, and recovers the account. It is a dead end from the registration form with a non-obvious escape, not a lockout -- and that is why seven years of production never surfaced it. Swept the tree for the phrasing; nothing else carries it. lein test 229 / 1025 / 0, lein lint errors 0. --- src/clj/orcpub/routes.clj | 13 +++++++++---- test/clj/orcpub/registration_rollback_test.clj | 8 +++++--- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index 9fd86f50f..c9c153068 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -343,10 +343,15 @@ Without that rollback a failed email left a committed, unverified account, and `register` validates against existing username/email -- so the retry this - very function tells the user to make then failed with \"already taken\". The - address was locked out and the account could never be verified, because no - email could ever be sent. That is the default state of any instance without - SMTP, including the default docker-compose deployment. + very function tells the user to make then failed with \"already taken\". + + Not a lockout: `re-verify` works on exactly that orphaned state and is wired + to a button in the UI, so a user who finds it recovers. Measured, because an + earlier version of this docstring claimed otherwise. It is a dead end from the + registration form with a non-obvious escape, which is also why seven years of + production never surfaced it -- live SMTP works, so this runs only on a + transient send failure, and the few users it reaches report \"it says my email + already exists\", indistinguishable from forgetting an account. `request-email-change` already did exactly this (retracting pending-email on send failure, covered by email-change-test/test-email-send-failure-rolls-back). diff --git a/test/clj/orcpub/registration_rollback_test.clj b/test/clj/orcpub/registration_rollback_test.clj index fb7bd27d6..44de41684 100644 --- a/test/clj/orcpub/registration_rollback_test.clj +++ b/test/clj/orcpub/registration_rollback_test.clj @@ -84,9 +84,11 @@ (let [user (find-user (d/db mocked-conn) "newcomer")] (is (nil? user) (str "A failed verification email left an account behind. " - "The user is now locked out: retrying registration fails " - "validation because the username and email are taken, and " - "the account can never be verified. Found: " (pr-str user))))))))) + "Retrying registration then fails validation because the " + "username and email are taken -- which is exactly the " + "advice the error message gives. (Recoverable via resend, " + "see the ns docstring, but a dead end from the form.) " + "Found: " (pr-str user))))))))) (deftest the-address-can-be-reused-after-a-failed-send (with-conn conn From 5d7b4b2f6345c7a2b77791aaa3ad09191879d8f0 Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Tue, 22 Sep 2026 14:16:39 +0000 Subject: [PATCH 26/35] Verify on creation when no SMTP is configured, making the .env promise true .env.example offered an empty EMAIL_SERVER_URL as the way to run without email. Nothing implemented it: the variable was read in exactly one place, as postal's :host, so leaving it blank did not disable email -- it made every send FAIL. Registration sends a verification mail, and login-response refuses an unverified account (routes.clj:301), so the documented way to turn email off was also the way to make the site unusable. Nobody could register, and any account that did exist could not sign in. do-verification now checks email/configured? and, when there is no SMTP host, transacts the account already verified and sends nothing. The operator of a mail-less instance is handing out the accounts themselves, so nobody was proving address ownership in that configuration either way. The response carries {:verified? true} so the client can tell the difference: :register-success now routes to :verify-success ("Registration is complete, you can now log in") instead of :verify-sent ("check your email"), which would have pointed the user at a mail that is never coming. .env.example says what actually happens now, including the caveat that it is only sensible for a private instance. Tests, each verified to fail against the old behaviour by forcing the email path with (if-not true ...): no-smtp-verifies-on-creation -> ERROR: the stub asserts no send is attempted, and one was an-auto-verified-account-can-actually-log-in -> FAIL: {:status 401, :body {:error :unverified}} -- the literal symptom with-smtp-configured-nothing-changes -> passes (the control) The login test is the one that matters. The rest is bookkeeping; signing in is what was broken. registration_rollback_test now stubs email/configured? true at all six with-redefs sites. Those tests cover the path taken when a deployment HAS SMTP and the send fails, and they had been relying implicitly on the old behaviour -- the test environment sets no EMAIL_SERVER_URL, so without the stub they silently started exercising the auto-verify branch instead. Five of them failed when this landed, which is the tests doing their job. lein test 232 tests / 1035 assertions / 0 failures. lein lint errors 0. lein fig:build compiles clean (note: AGENTS.md still says `lein cljsbuild once dev`, which is not a task in this project -- it moved to figwheel-main). --- .env.example | 5 +- src/clj/orcpub/email.clj | 12 +++ src/clj/orcpub/routes.clj | 24 ++++- src/cljs/orcpub/dnd/e5/events.cljs | 10 +- .../clj/orcpub/registration_no_email_test.clj | 102 ++++++++++++++++++ .../clj/orcpub/registration_rollback_test.clj | 24 +++-- 6 files changed, 165 insertions(+), 12 deletions(-) create mode 100644 test/clj/orcpub/registration_no_email_test.clj diff --git a/.env.example b/.env.example index 70c9f0cf3..9a9e3231f 100644 --- a/.env.example +++ b/.env.example @@ -90,7 +90,10 @@ LOG_DIR= # FIGWHEEL_CONNECT_URL= # --- Email (SMTP) --- -# Leave EMAIL_SERVER_URL empty to disable email functionality +# Leave EMAIL_SERVER_URL empty to run without email. Registration then verifies +# accounts on creation instead of mailing a link, and no mail is sent at all. +# Only sensible for a private instance where you hand out the accounts -- +# nobody proves they own the address they typed. EMAIL_SERVER_URL= EMAIL_ACCESS_KEY= EMAIL_SECRET_KEY= diff --git a/src/clj/orcpub/email.clj b/src/clj/orcpub/email.clj index 4f176c7ac..42c9a5514 100644 --- a/src/clj/orcpub/email.clj +++ b/src/clj/orcpub/email.clj @@ -75,6 +75,18 @@ [{:type "text/html" :content (str (hiccup/html (email-change-verification-html username verification-url)))}]) +(defn configured? + "True when an SMTP host is set, i.e. when this deployment can send mail. + + .env.example says \"Leave EMAIL_SERVER_URL empty to disable email + functionality\". Nothing implemented that: the variable was read in exactly one + place, as postal's :host, so leaving it blank did not disable email -- it made + every send FAIL. Since registration sends a verification mail, the documented + way to turn email off also turned registration off. This is the predicate that + makes the promise true." + [] + (some? (env/value :email-server-url))) + (defn email-cfg [] (try ;; "" defaults on purpose, NOT nil. docker-compose.yaml passes all three as diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index c9c153068..8945ea89f 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -358,7 +358,27 @@ Registration was never brought up to match. See registration_rollback_test.clj and docs/kb/blank-env-values.md." [request params conn & [tx-data]] - (let [verification-key (str (java.util.UUID/randomUUID)) + (if-not (email/configured?) + ;; No SMTP: verify on the spot rather than promising a mail that cannot be + ;; sent. .env.example offers an empty EMAIL_SERVER_URL as the way to run + ;; without email; before this, that made registration impossible instead -- + ;; the send failed, so every attempt died. The operator of a mail-less + ;; instance is handing out accounts themselves, so address ownership is not + ;; being proved by anyone anyway. + (do + (println "INFO: EMAIL_SERVER_URL is unset — verifying" (:username params) + "on creation instead of sending a verification email.") + (try + @(d/transact conn [(merge tx-data {:orcpub.user/verified? true})]) + ;; The client branches on this to show "registration complete, you can + ;; log in" rather than "check your email". + {:status 200 :body {:verified? true}} + (catch Exception e + (println "ERROR: Failed to create account:" (.getMessage e)) + (throw (ex-info "Unable to complete registration. Please try again or contact support." + {:error :verification-failed} + e))))) + (let [verification-key (str (java.util.UUID/randomUUID)) now (java.util.Date.) ;; re-verify passes an existing {:db/id id}; register does not. The two ;; need different rollbacks -- never retract the ENTITY for a user who @@ -396,7 +416,7 @@ (.getMessage re)))) (throw (ex-info "Unable to complete registration. Please try again or contact support." {:error :verification-email-failed} - e)))))) + e))))))) (defn register [{:keys [json-params db conn] :as request}] (let [{:keys [username email password send-updates?]} json-params diff --git a/src/cljs/orcpub/dnd/e5/events.cljs b/src/cljs/orcpub/dnd/e5/events.cljs index 98bea8c97..fa645d775 100644 --- a/src/cljs/orcpub/dnd/e5/events.cljs +++ b/src/cljs/orcpub/dnd/e5/events.cljs @@ -1857,9 +1857,13 @@ (reg-event-db :register-success (fn [db [_ backtrack? response]] - (-> db - (update :user-data merge (:body response)) - (assoc :route :verify-sent)))) + ;; A deployment with no EMAIL_SERVER_URL verifies on creation and says so + ;; with :verified? -- sending such a user to "check your email" would point + ;; them at a mail that is never coming. + (let [verified? (get-in response [:body :verified?])] + (-> db + (update :user-data merge (:body response)) + (assoc :route (if verified? :verify-success :verify-sent)))))) (reg-event-fx :register-failure diff --git a/test/clj/orcpub/registration_no_email_test.clj b/test/clj/orcpub/registration_no_email_test.clj new file mode 100644 index 000000000..0b747ff2c --- /dev/null +++ b/test/clj/orcpub/registration_no_email_test.clj @@ -0,0 +1,102 @@ +(ns orcpub.registration-no-email-test + "Registration on a deployment with no SMTP configured. + + .env.example offers an empty EMAIL_SERVER_URL as the way to run without email. + Nothing implemented that: the variable was read in exactly one place, as + postal's :host, so leaving it blank did not disable email -- it made every + send FAIL. Registration sends a verification mail, and login refuses an + unverified account (routes.clj:301), so the documented way to turn email off + was also the way to make the site unusable: nobody could register, and any + account that did exist could not log in. + + do-verification now verifies on creation when email is unconfigured. The + operator of a mail-less instance is handing out accounts themselves, so + nobody was proving address ownership either way. + + The decisive test here is the login one. Everything else is bookkeeping; + being able to sign in is the thing that was broken." + (:require + [clojure.test :refer [deftest is testing use-fixtures]] + [datomic.api :as d] + [datomock.core :as dm] + [orcpub.email :as email] + [orcpub.errors :as errors] + [orcpub.routes :as routes] + [orcpub.db.schema :as schema]) + (:import [java.util UUID])) + +(use-fixtures :each + (fn [f] + (binding [errors/*error-prefix* "TEST_ERROR:"] + (f)))) + +(defn- fresh-conn [] + (let [uri (str "datomic:mem:rne-" (UUID/randomUUID))] + (d/create-database uri) + (let [c (dm/fork-conn (d/connect uri))] + @(d/transact c schema/all-schemas) + c))) + +(defn- register-request [conn] + {:conn conn + :db (d/db conn) + :scheme :https + :headers {"host" "example.test"} + :json-params {:username "newcomer" + :email "newcomer@test.com" + :verify-email "newcomer@test.com" + :password "hunter2hunter2" + :send-updates? false}}) + +(defn- find-user [db] + (when-let [e (d/q '[:find ?e . :where [?e :orcpub.user/username "newcomer"]] db)] + (d/pull db '[*] e))) + +(deftest no-smtp-verifies-on-creation + (let [conn (fresh-conn)] + (testing "registration succeeds and the account is already verified" + (with-redefs [email/configured? (constantly false) + ;; If this is ever called the test should fail loudly: the + ;; whole point is that no send is attempted. + routes/send-verification-email + (fn [& _] (throw (AssertionError. "tried to send mail with no SMTP configured")))] + (let [resp (routes/register (register-request conn)) + user (find-user (d/db conn))] + (is (= 200 (:status resp))) + (is (true? (get-in resp [:body :verified?])) + "the client branches on this to show \"you can log in\" instead of \"check your email\"") + (is (true? (:orcpub.user/verified? user))) + (is (nil? (:orcpub.user/verification-key user)) + "no key should be stored for a link that is never sent")))))) + +(deftest an-auto-verified-account-can-actually-log-in + ;; The one that matters. login-response refuses an unverified account, so + ;; before this change a mail-less instance produced accounts nobody could use. + (let [conn (fresh-conn)] + (with-redefs [email/configured? (constantly false) + routes/send-verification-email (fn [& _] nil)] + (routes/register (register-request conn))) + (testing "the account registered without SMTP can sign in" + (let [resp (routes/login-response + {:conn conn + :db (d/db conn) + :remote-addr "127.0.0.1" + :json-params {:username "newcomer" :password "hunter2hunter2"}})] + (is (= 200 (:status resp)) + (str "Login was refused for an account created on a mail-less " + "instance. Got: " (pr-str (select-keys resp [:status :body])))))))) + +(deftest with-smtp-configured-nothing-changes + (let [conn (fresh-conn) + sent (atom 0)] + (testing "the normal flow still creates an unverified account and mails a link" + (with-redefs [email/configured? (constantly true) + routes/send-verification-email (fn [& _] (swap! sent inc) nil)] + (let [resp (routes/register (register-request conn)) + user (find-user (d/db conn))] + (is (= 200 (:status resp))) + (is (nil? (get-in resp [:body :verified?])) + "no :verified? flag, so the client shows \"check your email\"") + (is (false? (:orcpub.user/verified? user))) + (is (some? (:orcpub.user/verification-key user))) + (is (= 1 @sent) "exactly one verification email")))))) diff --git a/test/clj/orcpub/registration_rollback_test.clj b/test/clj/orcpub/registration_rollback_test.clj index 44de41684..4f8ae1ecb 100644 --- a/test/clj/orcpub/registration_rollback_test.clj +++ b/test/clj/orcpub/registration_rollback_test.clj @@ -27,11 +27,17 @@ and retracts on failure, covered by email_change_test/test-email-send-failure- rolls-back. These tests are that one's mirror for registration. + Every test here stubs email/configured? true: these cover the path taken when + a deployment HAS SMTP and the send fails. With it unconfigured, registration + verifies on creation instead and never sends -- that is + registration_no_email_test. + See docs/kb/blank-env-values.md." (:require [clojure.test :refer [deftest is testing use-fixtures]] [datomic.api :as d] [datomock.core :as dm] + [orcpub.email :as email] [orcpub.errors :as errors] [orcpub.routes :as routes] [orcpub.db.schema :as schema]) @@ -75,7 +81,8 @@ (let [mocked-conn (dm/fork-conn conn)] (seed-schema mocked-conn) (testing "an SMTP failure must not commit a half-created user" - (with-redefs [routes/send-verification-email + (with-redefs [email/configured? (constantly true) + routes/send-verification-email (fn [& _] (throw (Exception. "SMTP down")))] ;; register rethrows; what matters is the DB state afterwards, not ;; which exception surfaced. @@ -95,12 +102,14 @@ (let [mocked-conn (dm/fork-conn conn)] (seed-schema mocked-conn) (testing "after a failed send, the same details still validate as available" - (with-redefs [routes/send-verification-email + (with-redefs [email/configured? (constantly true) + routes/send-verification-email (fn [& _] (throw (Exception. "SMTP down")))] (try (routes/register (register-request mocked-conn)) (catch Throwable _ nil))) ;; Second attempt, this time with a working mailer. - (with-redefs [routes/send-verification-email (fn [& _] nil)] + (with-redefs [email/configured? (constantly true) + routes/send-verification-email (fn [& _] nil)] (let [resp (routes/register (register-request mocked-conn))] (is (= 200 (:status resp)) (str "Retrying after a failed send must succeed -- this is the " @@ -115,7 +124,8 @@ (let [mocked-conn (dm/fork-conn conn)] (seed-schema mocked-conn) (testing "the happy path is unchanged by the rollback" - (with-redefs [routes/send-verification-email (fn [& _] nil)] + (with-redefs [email/configured? (constantly true) + routes/send-verification-email (fn [& _] nil)] (let [resp (routes/register (register-request mocked-conn)) user (find-user (d/db mocked-conn) "newcomer")] (is (= 200 (:status resp))) @@ -133,12 +143,14 @@ (with-conn conn (let [mocked-conn (dm/fork-conn conn)] (seed-schema mocked-conn) - (with-redefs [routes/send-verification-email (fn [& _] nil)] + (with-redefs [email/configured? (constantly true) + routes/send-verification-email (fn [& _] nil)] (routes/register (register-request mocked-conn))) (let [before (find-user (d/db mocked-conn) "newcomer")] (is (some? before) "precondition: the account exists") (testing "a failed re-send leaves the account intact" - (with-redefs [routes/send-verification-email + (with-redefs [email/configured? (constantly true) + routes/send-verification-email (fn [& _] (throw (Exception. "SMTP down")))] (try (routes/re-verify {:conn mocked-conn :db (d/db mocked-conn) From fcaf894eb09d61907d2a341e5caf9ca5e0935e6f Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Wed, 23 Sep 2026 13:05:08 +0000 Subject: [PATCH 27/35] Fail closed when email config goes missing: auto-verify now needs an opt-in Answering "can a malicious party use the no-email guard against us on a properly configured instance?" -- no, but the guard failed OPEN, which is worse in practice than the attack it does not permit. WHAT IS NOT REACHABLE, verified: - The guard keys on CONFIGURATION, never on send outcome. A failed send goes to the rollback path, never to verified? true. Knocking the mail server over does not auto-verify anybody. - environ.core/env is a static PersistentHashMap built once at namespace load. Confirmed by System/setProperty after load: configured? was unchanged. No request, input or runtime manipulation can flip it. WHAT WAS REACHABLE, and is the actual problem. Keying auto-verify on "no SMTP" alone means every way of LOSING the config reads as "the operator wanted no email". Measured, all three silently produced auto-verify: EMAIL_SERVER_URL=" " whitespace, e.g. a copy-paste artifact EMAIL_SERVER_URL= a value dropped by a deploy or a failed secret mount (absent entirely) a typo'd variable name, a renamed compose key A public site would have switched from verified registration to OPEN registration with no error and no alarm -- the only signal was a println on a running server that nobody reads. Not attacker-triggered, but precisely the shape that gets found by someone scanning for it: no need to break the guard, just wait for one operator slip. So the weaker mode is now ASKED FOR. ALLOW_UNVERIFIED_REGISTRATION=true plus an empty EMAIL_SERVER_URL gives the intended mail-less behaviour. Empty EMAIL_SERVER_URL WITHOUT it now refuses to register anyone, with a specific error naming both remedies, so losing your SMTP config breaks loudly instead of quietly downgrading security. losing-smtp-config-fails-closed pins it, and was verified to fail against the previous design by reinstating it: 3 assertions fail, including "registration must FAIL when email config is missing and nothing opted out" and the account being created anyway. .env.example and docker-compose.yaml document and pass the flag. The template no longer describes empty-as-disable, because empty alone is now a refusal. lein test 233 / 1038 / 0, lein lint errors 0. --- .env.example | 16 +++++++--- docker-compose.yaml | 3 ++ src/clj/orcpub/email.clj | 20 +++++++++++++ src/clj/orcpub/routes.clj | 30 ++++++++++++++----- .../clj/orcpub/registration_no_email_test.clj | 28 +++++++++++++++++ 5 files changed, 86 insertions(+), 11 deletions(-) diff --git a/.env.example b/.env.example index 9a9e3231f..970ab2a50 100644 --- a/.env.example +++ b/.env.example @@ -90,10 +90,14 @@ LOG_DIR= # FIGWHEEL_CONNECT_URL= # --- Email (SMTP) --- -# Leave EMAIL_SERVER_URL empty to run without email. Registration then verifies -# accounts on creation instead of mailing a link, and no mail is sent at all. -# Only sensible for a private instance where you hand out the accounts -- -# nobody proves they own the address they typed. +# EMAIL_SERVER_URL empty means this deployment cannot send mail, and registration +# then REFUSES to create accounts -- deliberately, because a dropped or typo'd +# value would otherwise silently turn a public site into open registration. +# +# To actually run without email, say so: ALLOW_UNVERIFIED_REGISTRATION=true. +# Accounts are then verified on creation and no mail is ever sent. Only sensible +# for a private instance where you hand out the accounts, since nobody proves +# they own the address they typed. EMAIL_SERVER_URL= EMAIL_ACCESS_KEY= EMAIL_SECRET_KEY= @@ -103,6 +107,10 @@ EMAIL_ERRORS_TO= EMAIL_SSL=FALSE EMAIL_TLS=FALSE +# Accept registrations without verifying the address. Requires EMAIL_SERVER_URL +# to be empty; ignored otherwise. Private instances only. +ALLOW_UNVERIFIED_REGISTRATION=false + # --- Branding (optional) --- # Override app identity for forks. All have sensible defaults in fork/branding.clj. # APP_NAME=Dungeon Master's Vault diff --git a/docker-compose.yaml b/docker-compose.yaml index be568a6d9..2d4992e1a 100644 --- a/docker-compose.yaml +++ b/docker-compose.yaml @@ -54,6 +54,9 @@ services: SIGNATURE: ${SIGNATURE:-change-me-to-something-unique} CSP_POLICY: ${CSP_POLICY:-strict} DEV_MODE: ${DEV_MODE:-} + # Empty EMAIL_SERVER_URL makes registration refuse rather than silently + # accept unverified accounts; this is how you opt into the weaker mode. + ALLOW_UNVERIFIED_REGISTRATION: ${ALLOW_UNVERIFIED_REGISTRATION:-} LOAD_HOMEBREW_URL: ${LOAD_HOMEBREW_URL:-} depends_on: datomic: diff --git a/src/clj/orcpub/email.clj b/src/clj/orcpub/email.clj index 42c9a5514..abd1c15cc 100644 --- a/src/clj/orcpub/email.clj +++ b/src/clj/orcpub/email.clj @@ -87,6 +87,26 @@ [] (some? (env/value :email-server-url))) +(defn unverified-registration-allowed? + "True when this deployment has DELIBERATELY opted out of email verification. + + Keying auto-verification on \"no SMTP configured\" alone fails OPEN: a typo in + the variable name, a value dropped by a deploy, a failed secrets mount, or a + stray space all read as \"no email\", and the site silently stops requiring + verification. Measured -- EMAIL_SERVER_URL as \" \", as empty, and absent + entirely all produced auto-verify, with no signal beyond a println nobody + reads on a running server. + + An attacker cannot flip this: environ.core/env is a static map built once at + namespace load, so no request can change it. The risk is an operator slip + downgrading the site from verified to open registration, which is exactly the + kind of mistake that gets found by someone scanning for it. + + So the weaker mode has to be ASKED FOR. Losing your SMTP config now breaks + registration loudly instead of quietly accepting unverified accounts." + [] + (env/flag? :allow-unverified-registration)) + (defn email-cfg [] (try ;; "" defaults on purpose, NOT nil. docker-compose.yaml passes all three as diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index 8945ea89f..4854adffe 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -358,13 +358,28 @@ Registration was never brought up to match. See registration_rollback_test.clj and docs/kb/blank-env-values.md." [request params conn & [tx-data]] - (if-not (email/configured?) - ;; No SMTP: verify on the spot rather than promising a mail that cannot be - ;; sent. .env.example offers an empty EMAIL_SERVER_URL as the way to run - ;; without email; before this, that made registration impossible instead -- - ;; the send failed, so every attempt died. The operator of a mail-less - ;; instance is handing out accounts themselves, so address ownership is not - ;; being proved by anyone anyway. + (cond + ;; SMTP is gone but nobody asked for unverified registration. FAIL CLOSED. + ;; Keying auto-verify on "no SMTP" alone would silently turn a production + ;; site into open registration the moment a variable is typo'd, dropped by a + ;; deploy, or mounted empty -- a downgrade with no error and no alarm. + ;; Measured: " ", "" and absent entirely all read as "no email". Make the + ;; operator say so, and make the accident loud instead. + (and (not (email/configured?)) + (not (email/unverified-registration-allowed?))) + (do + (println "ERROR: registration unavailable — EMAIL_SERVER_URL is unset, so no" + "verification email can be sent. Set it, or set" + "ALLOW_UNVERIFIED_REGISTRATION=true to accept accounts without" + "verifying the address.") + (throw (ex-info "Registration is temporarily unavailable. Please contact the site administrator." + {:error :email-not-configured}))) + + ;; Deliberately running without email: verify on the spot rather than + ;; promising a mail that cannot be sent. The operator of a mail-less + ;; instance is handing out the accounts themselves, so address ownership is + ;; not being proved by anyone anyway. + (not (email/configured?)) (do (println "INFO: EMAIL_SERVER_URL is unset — verifying" (:username params) "on creation instead of sending a verification email.") @@ -378,6 +393,7 @@ (throw (ex-info "Unable to complete registration. Please try again or contact support." {:error :verification-failed} e))))) + :else (let [verification-key (str (java.util.UUID/randomUUID)) now (java.util.Date.) ;; re-verify passes an existing {:db/id id}; register does not. The two diff --git a/test/clj/orcpub/registration_no_email_test.clj b/test/clj/orcpub/registration_no_email_test.clj index 0b747ff2c..dc03ac0c2 100644 --- a/test/clj/orcpub/registration_no_email_test.clj +++ b/test/clj/orcpub/registration_no_email_test.clj @@ -56,6 +56,7 @@ (let [conn (fresh-conn)] (testing "registration succeeds and the account is already verified" (with-redefs [email/configured? (constantly false) + email/unverified-registration-allowed? (constantly true) ;; If this is ever called the test should fail loudly: the ;; whole point is that no send is attempted. routes/send-verification-email @@ -74,6 +75,7 @@ ;; before this change a mail-less instance produced accounts nobody could use. (let [conn (fresh-conn)] (with-redefs [email/configured? (constantly false) + email/unverified-registration-allowed? (constantly true) routes/send-verification-email (fn [& _] nil)] (routes/register (register-request conn))) (testing "the account registered without SMTP can sign in" @@ -91,6 +93,7 @@ sent (atom 0)] (testing "the normal flow still creates an unverified account and mails a link" (with-redefs [email/configured? (constantly true) + email/unverified-registration-allowed? (constantly false) routes/send-verification-email (fn [& _] (swap! sent inc) nil)] (let [resp (routes/register (register-request conn)) user (find-user (d/db conn))] @@ -100,3 +103,28 @@ (is (false? (:orcpub.user/verified? user))) (is (some? (:orcpub.user/verification-key user))) (is (= 1 @sent) "exactly one verification email")))))) + +(deftest losing-smtp-config-fails-closed + ;; The security case. Auto-verify keyed on "no SMTP" ALONE fails open: a + ;; typo'd variable name, a value dropped by a deploy, a failed secrets mount + ;; or a stray space all read as "no email", and a production site silently + ;; stops requiring verification. Measured before this guard: " ", "" and + ;; absent all produced auto-verify. + ;; + ;; Not attacker-triggerable -- environ.core/env is a static map built once at + ;; namespace load, so no request can flip it. The risk is one operator slip + ;; downgrading the site to open registration with no alarm. + (let [conn (fresh-conn)] + (testing "no SMTP and no explicit opt-in refuses to register anyone" + (with-redefs [email/configured? (constantly false) + email/unverified-registration-allowed? (constantly false) + routes/send-verification-email + (fn [& _] (throw (AssertionError. "must not attempt a send")))] + (let [thrown (try (routes/register (register-request conn)) nil + (catch Throwable e e))] + (is (some? thrown) + "registration must FAIL when email config is missing and nothing opted out") + (is (= :email-not-configured (:error (ex-data thrown))) + (str "expected a specific, actionable error. Got: " (pr-str (ex-data thrown)))) + (is (nil? (find-user (d/db conn))) + "and no account may be created, verified or otherwise")))))) From 896d186fb0c463445c2dd76129fc653f3cfb1304 Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Wed, 23 Sep 2026 13:26:24 +0000 Subject: [PATCH 28/35] Say at startup what registration will actually do Every wrong answer about registration policy is silent, so the three states that are not "normal" now announce themselves at namespace load, beside the existing SIGNATURE warning. There is no boot report on this branch to hang it on; if one is added, move it there with the rest of the effective config. no SMTP + ALLOW_UNVERIFIED_REGISTRATION=true "running WITHOUT EMAIL VERIFICATION" -- the intended private-instance mode, stated plainly rather than inferred from silence. no SMTP + no opt-in "registration is DISABLED", naming both remedies. Existing users are unaffected, and the message says so, because an admin seeing this needs to know the site is not down. SMTP configured + ALLOW_UNVERIFIED_REGISTRATION=true The loaded gun. Inert today; the day the SMTP value is lost to a typo, a dropped deploy variable or a failed secret mount, this flag turns registration into OPEN registration instead of failing. Says to remove it unless the instance is private. A correct configuration prints nothing. Verified across all four combinations. NOT made fatal, deliberately. The inert-flag case is a latent risk, not an active one, and refusing to boot over "registration is disabled" would take down character building, exports and every existing user over a feature they are not using. Loud and running beats silent, and beats dead. lein test 233 / 1038 / 0; lein lint errors 0; lein fig:build clean. --- src/clj/orcpub/routes.clj | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index 4854adffe..bb44b605b 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -83,6 +83,40 @@ (when-not jwt-secret (println "WARNING: SIGNATURE env var is not set — all authenticated API calls will fail")) +(defn- report-registration-mode! + "Say at startup what registration will actually do, because every wrong answer + here is silent. + + Printed at namespace load, beside the SIGNATURE warning above, because this + branch has no boot report to hang it on. If one is added later, move it there + -- it belongs with the rest of the effective configuration." + [] + (let [smtp? (email/configured?) + opted-out? (email/unverified-registration-allowed?)] + (cond + (and (not smtp?) opted-out?) + (do (println "WARNING: registration is running WITHOUT EMAIL VERIFICATION.") + (println " EMAIL_SERVER_URL is unset and ALLOW_UNVERIFIED_REGISTRATION=true,") + (println " so anyone who registers is verified on the spot and nobody proves") + (println " they own the address they typed. Intended for a private instance.") + (println " Set EMAIL_SERVER_URL to restore verification.")) + + (not smtp?) + (do (println "WARNING: registration is DISABLED — EMAIL_SERVER_URL is unset, so no") + (println " verification email can be sent. Existing users are unaffected.") + (println " Set EMAIL_SERVER_URL, or ALLOW_UNVERIFIED_REGISTRATION=true to") + (println " accept accounts without verifying the address.")) + + opted-out? + ;; The loaded gun: inert today, decides policy the day SMTP goes missing. + (do (println "WARNING: ALLOW_UNVERIFIED_REGISTRATION is set but has no effect right now,") + (println " because EMAIL_SERVER_URL is configured. If that value is ever lost") + (println " — a typo, a dropped deploy variable, a failed secret mount — this") + (println " flag silently turns registration into OPEN registration instead of") + (println " failing. Remove it unless this is a private instance."))))) + +(report-registration-mode!) + (def backend (backends/jws {:secret jwt-secret})) (defn first-user-by [db query value] From 8b33c26e41614bd5d65d58b2da6d21a488c70a59 Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Thu, 24 Sep 2026 00:04:02 +0000 Subject: [PATCH 29/35] Set EMAIL_FROM_ADDRESS explicitly in the template Blank now falls back to branding/email-from-address (no-reply@orcpub.com), which a real SMTP provider will refuse to send as -- SendGrid, SES and the like check the sender against an authenticated domain. A template that ships blank invites exactly that failure, and the previous behaviour hid it by sending an EMPTY From instead. FOLLOW-UP for whoever merges integration down: config/print-report! exists there and not on this branch, which is why report-registration-mode! in routes.clj prints at namespace load instead. Fold these into the boot report when the code meets it -- registration mode belongs with the rest of the effective config, not three lines above the SIGNATURE warning. The comment on that fn says so too. --- .env.example | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/.env.example b/.env.example index 970ab2a50..4081a1d4c 100644 --- a/.env.example +++ b/.env.example @@ -102,7 +102,11 @@ EMAIL_SERVER_URL= EMAIL_ACCESS_KEY= EMAIL_SECRET_KEY= EMAIL_SERVER_PORT=587 -EMAIL_FROM_ADDRESS= +# MUST be an address your SMTP provider is authorised to send as (SPF/DKIM), +# not just any address you own. Left blank it falls back to +# branding/email-from-address (no-reply@orcpub.com), which your provider +# almost certainly will not accept -- set it. +EMAIL_FROM_ADDRESS=no-reply@example.com EMAIL_ERRORS_TO= EMAIL_SSL=FALSE EMAIL_TLS=FALSE From 4909ea1e3b352039d32e5b51941183e1b5685a4e Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Thu, 24 Sep 2026 00:51:49 +0000 Subject: [PATCH 30/35] Document ALLOW_UNVERIFIED_REGISTRATION, and make the KB references resolvable Both review findings were right, and both were mine. 1. The opt-in existed in .env.example and docker-compose.yaml and in no operator documentation at all -- verified, zero hits across docs/. Worse, DOCKER.md:211 said an empty SMTP host means "registration still works, just no verification emails", which is now precisely backwards: registration is REFUSED. An operator following that line would have taken the site's signup offline and had no reason to suspect the doc. DOCKER.md and ENVIRONMENT.md now carry the variable and correct the empty-host claim. email-system.md's registration flow gains the two branches it never described: the no-SMTP paths (auto-verify with the opt-in, refusal without it) and the send-failure rollback, including that re-verify retracts only the attributes its attempt set and never the existing user. 2. docs/kb/blank-env-values.md was cited three times from source and tests on this branch, and the KB lives on agents/develop -- by design, since code branches gitignore it. So those were dead paths from the moment I wrote them. Rewritten as `git show agents/develop:docs/kb/blank-env-values.md`, which tells the reader how to actually read it and is verified to resolve. This is the form CLAUDE.md already uses for e2e-logged-in-sessions.md. The general trap, worth stating: a code branch cannot link into the KB with a relative path. Any cross-branch reference has to name the branch or it is born broken, and nothing on the code branch will ever fail to warn you. lein test 233 / 1038 / 0; lein lint errors 0. --- docs/DOCKER.md | 3 ++- docs/ENVIRONMENT.md | 3 ++- docs/email-system.md | 10 ++++++++++ src/clj/orcpub/routes.clj | 2 +- test/clj/orcpub/registration_rollback_test.clj | 4 ++-- 5 files changed, 17 insertions(+), 5 deletions(-) diff --git a/docs/DOCKER.md b/docs/DOCKER.md index c94d5cfb3..6a94dbe86 100644 --- a/docs/DOCKER.md +++ b/docs/DOCKER.md @@ -208,7 +208,8 @@ These are the variables you'll actually touch. Full reference in | `DATOMIC_URL` | Yes | `datomic:dev://datomic:4334/orcpub` | Database connection URI. No `?password=` — the app adds it from `DATOMIC_PASSWORD`. | | `PORT` | No | `8890` | App server port. Nginx and healthcheck adapt automatically. | | `ALT_HOST` | No | `127.0.0.1` | Transactor peer fallback host. Change to `datomic` for Swarm. | -| `EMAIL_SERVER_URL` | No | *(empty)* | SMTP server. Leave empty to disable email (registration still works, just no verification emails). | +| `EMAIL_SERVER_URL` | No | *(empty)* | SMTP server. **Empty means registration is REFUSED**, not that it works without verification — a verification email cannot be sent, and failing closed stops a dropped value silently turning the site into open registration. To run without email, also set `ALLOW_UNVERIFIED_REGISTRATION`. | +| `ALLOW_UNVERIFIED_REGISTRATION` | No | `false` | `true` **and** an empty `EMAIL_SERVER_URL` verifies accounts on creation and sends no mail. Inert when SMTP is configured. Private instances only — nobody proves they own the address they typed. The server warns at startup in every abnormal combination. | | `CSP_POLICY` | No | `strict` | Content Security Policy: `strict`, `permissive`, or `none`. | | `DEV_MODE` | No | *(empty)* | Set to `true` to send no CSP header at all, which is what allows Figwheel hot-reload. Not a Report-Only mode — that does not exist. | | `LOAD_HOMEBREW_URL` | No | *(empty)* | URL to fetch `.orcbrew` plugins on first page load. | diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index 154781861..45a691bac 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -69,10 +69,11 @@ See `docker/transactor.properties.template` for the full transactor configuratio | Variable | Default | Description | |----------|---------|-------------| -| `EMAIL_SERVER_URL` | — | SMTP server hostname. Leave empty to disable email. | +| `EMAIL_SERVER_URL` | — | SMTP server hostname. **Empty does not mean "email off, site fine": registration is refused**, because no verification mail can be sent. Pair with `ALLOW_UNVERIFIED_REGISTRATION` to run without email. | | `EMAIL_ACCESS_KEY` | — | SMTP username | | `EMAIL_SECRET_KEY` | — | SMTP password | | `EMAIL_SERVER_PORT` | `587` | SMTP port | +| `ALLOW_UNVERIFIED_REGISTRATION` | `false` | With an empty `EMAIL_SERVER_URL`, accounts are verified on creation and no mail is sent. Ignored when SMTP is configured. Deliberately an opt-in: keying this on "no SMTP" alone would let a typo'd or dropped variable silently disable verification. | | `EMAIL_FROM_ADDRESS` | `no-reply@dungeonmastersvault.com` | Sender email address | | `EMAIL_ERRORS_TO` | — | Error notification recipient | | `EMAIL_SSL` | `FALSE` | Enable SSL for SMTP | diff --git a/docs/email-system.md b/docs/email-system.md index a6b39384d..55664f66a 100644 --- a/docs/email-system.md +++ b/docs/email-system.md @@ -55,6 +55,16 @@ User attributes related to email and verification (`src/clj/orcpub/db/schema.clj **Re-verify:** `GET /re-verify?email=...` (`routes/re-verify`) re-sends the verification email for unverified accounts. +**No SMTP configured:** `do-verification` checks `email/configured?` first. With no SMTP host and +`ALLOW_UNVERIFIED_REGISTRATION=true`, the account is transacted `verified? true`, no mail is sent, +and the response carries `{:verified? true}` so the client shows "you can log in" rather than +"check your email". With no SMTP host and **no** opt-in, registration is refused with +`:email-not-configured` — fail closed, so a lost SMTP value cannot quietly become open +registration. Startup warns in all three abnormal combinations; a correct config is silent. + +**Send failure:** the account is rolled back. `register` retracts the new entity; `re-verify` +retracts only the attributes that attempt set, never the existing user. + **Login gate:** Unverified users cannot log in. If the verification has expired, the login error tells them to re-register. **Files:** `routes.clj:register`, `routes.clj:do-verification`, `routes.clj:verify`, `email.clj:send-verification-email` diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index bb44b605b..8f6ee2862 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -390,7 +390,7 @@ `request-email-change` already did exactly this (retracting pending-email on send failure, covered by email-change-test/test-email-send-failure-rolls-back). Registration was never brought up to match. See - registration_rollback_test.clj and docs/kb/blank-env-values.md." + registration_rollback_test.clj and `git show agents/develop:docs/kb/blank-env-values.md`." [request params conn & [tx-data]] (cond ;; SMTP is gone but nobody asked for unverified registration. FAIL CLOSED. diff --git a/test/clj/orcpub/registration_rollback_test.clj b/test/clj/orcpub/registration_rollback_test.clj index 4f8ae1ecb..647e39582 100644 --- a/test/clj/orcpub/registration_rollback_test.clj +++ b/test/clj/orcpub/registration_rollback_test.clj @@ -11,7 +11,7 @@ pre-fix code -- resend returns 200 and stores a fresh key), and it is wired to a button in the UI. So the real symptom is a confusing dead end that a user can escape if they find the resend link, not a permanent lockout. An earlier - version of this docstring and of docs/kb/blank-env-values.md said \"can never + version of this docstring and of `git show agents/develop:docs/kb/blank-env-values.md` said \"can never be verified\"; that was wrong. Which is why seven years of production never surfaced it: live SMTP works, so @@ -32,7 +32,7 @@ verifies on creation instead and never sends -- that is registration_no_email_test. - See docs/kb/blank-env-values.md." + See `git show agents/develop:docs/kb/blank-env-values.md`." (:require [clojure.test :refer [deftest is testing use-fixtures]] [datomic.api :as d] From 9ca0560303f3bd7e027bf475712b0f2731ac8fc7 Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Thu, 24 Sep 2026 00:56:37 +0000 Subject: [PATCH 31/35] Write the new settings for humans, and point the code at the doc that mirrors it The first pass at documenting ALLOW_UNVERIFIED_REGISTRATION was written like release notes -- "fail closed", "inert when SMTP is configured", "keying this on". An operator reading DOCKER.md wants to know what happens if they leave a field blank, not the reasoning behind the guard. Rewritten plainly: EMAIL_SERVER_URL "Leave it empty and nobody can sign up: the site can't send the confirmation email, so it turns registration off rather than letting people in unchecked." ALLOW_UNVERIFIED_REGISTRATION "Set this to true, and leave EMAIL_SERVER_URL empty, to let people sign up without confirming their email. Only do this on a private site -- anyone can sign up using an address that isn't theirs. It does nothing if you have SMTP set up." Two more human-facing tables were missing the setting entirely, found by grepping for EMAIL_SERVER_URL rather than assuming the three the review named were all of them: README.md:276 and docs/docker-user-management.md:91. Both now carry it, and both had the same "leave empty to skip email" phrasing that is no longer true. do-verification's docstring now names docs/email-system.md as the doc that mirrors it, because that file walks the same flow branch by branch and is what an operator reads instead of the code. A change in one needs a change in the other, and nothing else would say so. lein test 233 / 1038 / 0; lein lint errors 0. --- README.md | 3 ++- docs/DOCKER.md | 4 ++-- docs/ENVIRONMENT.md | 4 ++-- docs/docker-user-management.md | 3 ++- src/clj/orcpub/routes.clj | 6 +++++- 5 files changed, 13 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index 04d216652..cc59bf2a2 100644 --- a/README.md +++ b/README.md @@ -273,7 +273,8 @@ Key variables: | `DATOMIC_URL` | Database connection string | `datomic:dev://localhost:4334/orcpub` | | `SIGNATURE` | JWT signing secret (**required**) | dev default in `.lein-env` | | `PORT` | Web server port | `8890` | -| `EMAIL_SERVER_URL` | SMTP server | (optional) | +| `EMAIL_SERVER_URL` | SMTP server — leave empty and nobody can sign up (see `ALLOW_UNVERIFIED_REGISTRATION`) | (optional) | +| `ALLOW_UNVERIFIED_REGISTRATION` | Let people sign up without confirming their email. Private sites only | `false` | | `CSP_POLICY` | Content Security Policy mode | `strict` | | `DEV_MODE` | Enable dev features | `true` in dev | diff --git a/docs/DOCKER.md b/docs/DOCKER.md index 6a94dbe86..ca1c9bd64 100644 --- a/docs/DOCKER.md +++ b/docs/DOCKER.md @@ -208,8 +208,8 @@ These are the variables you'll actually touch. Full reference in | `DATOMIC_URL` | Yes | `datomic:dev://datomic:4334/orcpub` | Database connection URI. No `?password=` — the app adds it from `DATOMIC_PASSWORD`. | | `PORT` | No | `8890` | App server port. Nginx and healthcheck adapt automatically. | | `ALT_HOST` | No | `127.0.0.1` | Transactor peer fallback host. Change to `datomic` for Swarm. | -| `EMAIL_SERVER_URL` | No | *(empty)* | SMTP server. **Empty means registration is REFUSED**, not that it works without verification — a verification email cannot be sent, and failing closed stops a dropped value silently turning the site into open registration. To run without email, also set `ALLOW_UNVERIFIED_REGISTRATION`. | -| `ALLOW_UNVERIFIED_REGISTRATION` | No | `false` | `true` **and** an empty `EMAIL_SERVER_URL` verifies accounts on creation and sends no mail. Inert when SMTP is configured. Private instances only — nobody proves they own the address they typed. The server warns at startup in every abnormal combination. | +| `EMAIL_SERVER_URL` | No | *(empty)* | Your SMTP server. Leave it empty and nobody can sign up: the site can't send the confirmation email, so it turns registration off rather than letting people in unchecked. To run without email, see the next setting. | +| `ALLOW_UNVERIFIED_REGISTRATION` | No | `false` | Set this to `true`, and leave `EMAIL_SERVER_URL` empty, to let people sign up without confirming their email. Their accounts work right away. Only do this on a private site — anyone can sign up using an address that isn't theirs. It does nothing if you have SMTP set up. | | `CSP_POLICY` | No | `strict` | Content Security Policy: `strict`, `permissive`, or `none`. | | `DEV_MODE` | No | *(empty)* | Set to `true` to send no CSP header at all, which is what allows Figwheel hot-reload. Not a Report-Only mode — that does not exist. | | `LOAD_HOMEBREW_URL` | No | *(empty)* | URL to fetch `.orcbrew` plugins on first page load. | diff --git a/docs/ENVIRONMENT.md b/docs/ENVIRONMENT.md index 45a691bac..293d37cf2 100644 --- a/docs/ENVIRONMENT.md +++ b/docs/ENVIRONMENT.md @@ -69,11 +69,11 @@ See `docker/transactor.properties.template` for the full transactor configuratio | Variable | Default | Description | |----------|---------|-------------| -| `EMAIL_SERVER_URL` | — | SMTP server hostname. **Empty does not mean "email off, site fine": registration is refused**, because no verification mail can be sent. Pair with `ALLOW_UNVERIFIED_REGISTRATION` to run without email. | +| `EMAIL_SERVER_URL` | — | Your SMTP server. If it's empty, nobody can sign up — the site can't send a confirmation email, so it turns registration off instead of letting people in unchecked. | | `EMAIL_ACCESS_KEY` | — | SMTP username | | `EMAIL_SECRET_KEY` | — | SMTP password | | `EMAIL_SERVER_PORT` | `587` | SMTP port | -| `ALLOW_UNVERIFIED_REGISTRATION` | `false` | With an empty `EMAIL_SERVER_URL`, accounts are verified on creation and no mail is sent. Ignored when SMTP is configured. Deliberately an opt-in: keying this on "no SMTP" alone would let a typo'd or dropped variable silently disable verification. | +| `ALLOW_UNVERIFIED_REGISTRATION` | `false` | Set to `true`, with `EMAIL_SERVER_URL` empty, to let people sign up without confirming their email. For private sites only. Does nothing if SMTP is set up. You have to ask for this on purpose, so that losing your SMTP settings by accident can't quietly stop the site checking addresses. | | `EMAIL_FROM_ADDRESS` | `no-reply@dungeonmastersvault.com` | Sender email address | | `EMAIL_ERRORS_TO` | — | Error notification recipient | | `EMAIL_SSL` | `FALSE` | Enable SSL for SMTP | diff --git a/docs/docker-user-management.md b/docs/docker-user-management.md index de4c33512..a47acc56d 100644 --- a/docs/docker-user-management.md +++ b/docs/docker-user-management.md @@ -88,7 +88,8 @@ The setup script creates a `.env` file used by `docker-compose.yaml`. You can al | `ADMIN_PASSWORD` | Datomic admin interface password | generated | | `DATOMIC_PASSWORD` | Datomic application password | generated | | `SIGNATURE` | JWT signing secret (20+ chars) | generated | -| `EMAIL_SERVER_URL` | SMTP server (leave empty to skip email) | empty | +| `EMAIL_SERVER_URL` | SMTP server. Leave it empty and sign-ups are turned off, unless you also set the next one | empty | +| `ALLOW_UNVERIFIED_REGISTRATION` | Allow sign-ups without an email confirmation. Private sites only | `false` | | `EMAIL_ACCESS_KEY` | SMTP username | empty | | `EMAIL_SECRET_KEY` | SMTP password | empty | | `EMAIL_SERVER_PORT` | SMTP port | `587` | diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index 8f6ee2862..e38a4e138 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -390,7 +390,11 @@ `request-email-change` already did exactly this (retracting pending-email on send failure, covered by email-change-test/test-email-send-failure-rolls-back). Registration was never brought up to match. See - registration_rollback_test.clj and `git show agents/develop:docs/kb/blank-env-values.md`." + registration_rollback_test.clj and `git show agents/develop:docs/kb/blank-env-values.md`. + + docs/email-system.md describes this flow for operators, branch by branch. It + mirrors this function, so a change here needs a change there -- that doc is + what someone reads instead of this code." [request params conn & [tx-data]] (cond ;; SMTP is gone but nobody asked for unverified registration. FAIL CLOSED. From 969cf64496ebeffae4fa8311fe620a1c86241e3f Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Thu, 24 Sep 2026 01:43:29 +0000 Subject: [PATCH 32/35] Read the JWT secret through config/signature so Docker secrets work routes.clj read SIGNATURE from the environment directly, bypassing config/signature -- the accessor that checks /run/secrets/signature first. So a deployment that followed the Docker-secrets instructions in docker-compose.yaml (mount the secret, drop the environment variable) had a perfectly good secret on disk and never looked at it. In a container there is no .lein-env to fall back on, so jwt-secret is nil, check-auth returns 500, and every authenticated call and token operation fails. Correct behaviour verified with a real /run/secrets/signature file: new code, secret mounted, SIGNATURE unset -> "secret-from-the-mounted-file" new code, no secret file, SIGNATURE set -> "from-the-env" And the old code, same mounted secret, returned "dev-secret-do-not-use-in-production" -- the :dev profile's value from .lein-env, not the file. It ignored the secret. That is the bug; the nil only appears in a container, where no profile supplies a fallback. Worth stating precisely, because the local reproduction shows the wrong symptom. I had the chance to fix this when converting these reads to orcpub.env and did not -- config/signature already existed and I routed around it, which is the same mistake this branch documented earlier: a correct accessor is worth nothing if callers reach past it. The blank-value check is unchanged; it now lives in config/signature, which routes both the file and the variable through orcpub.env/value. The reviewer also cited line 406, which in the current revision is the email cond -- their line numbers predate these commits. Grepped the whole file: 81 was the only direct read. lein test 233 / 1038 / 0; lein lint errors 0. --- src/clj/orcpub/routes.clj | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index e38a4e138..06e0289b6 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -32,6 +32,7 @@ [orcpub.route-map :as route-map] [orcpub.errors :as errors] [orcpub.privacy :as privacy] + [orcpub.config :as config] [orcpub.email :as email] [orcpub.index :refer [index-page]] [orcpub.pdf :as pdf] @@ -69,7 +70,17 @@ "JWT signing secret from SIGNATURE env var. nil when unset OR BLANK — check-auth returns 500 with a diagnostic message. + Read through config/signature, NOT the environment directly. That accessor + checks /run/secrets/signature before SIGNATURE, which is what the Docker + secrets setup in docker-compose.yaml promises -- that comment says the app + checks /run/secrets/ first and falls back to the environment. Reading the + env var here meant a deployment that followed those instructions -- mount the + secret, drop the variable -- got nil, and every authenticated call and token + operation returned 500 while a perfectly good secret sat on disk. + The blank check is load-bearing, and its absence defeated the warning below. + It now lives in config/signature, which routes both sources through + orcpub.env/value. An exported-but-empty SIGNATURE= yields \"\", which is TRUTHY in Clojure, so it sailed past `when-not jwt-secret` — the guard written for exactly this — and was handed to buddy. Measured: buddy signs AND verifies with \"\" without @@ -78,7 +89,7 @@ Clearing a line in .env is an ordinary thing to do; .env.example ships nine keys with empty values." - (env/value :signature)) + (config/signature)) (when-not jwt-secret (println "WARNING: SIGNATURE env var is not set — all authenticated API calls will fail")) From 7b16f2e3872866c04b9c6d84813f31dd8da2e117 Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Thu, 24 Sep 2026 06:47:44 +0000 Subject: [PATCH 33/35] Guard the two secret-backed settings against being read from the env directly SIGNATURE and DATOMIC_PASSWORD can come from a mounted file as well as the environment, and config/signature and config/datomic-password are the only readers that know that. Reading env/value directly still compiles, still passes every other test, and still works on any deployment using environment variables -- which is why routes.clj did it until 969cf644, and why review caught it rather than the suite. Scans src/ for (env/value :signature) and (env/value :datomic-password) outside orcpub/config.clj, where both accessors live. Verified to catch the exact bug by reintroducing it: src/clj/orcpub/routes.clj:92 reads :signature - use config/signature NOT a clj-kondo rule, and worth saying why. :discouraged-var matches VARS, and (env/value :signature) is an approved var with a particular argument. Catching that needs a custom analyze-call hook; a source scan is smaller, and the JVM suite already runs in CI, which is the only place enforcement counts. Comments are stripped before matching, because these key names appear in prose throughout the codebase including this test's own docstring. The self-test asserts the strip rather than claiming the pattern ignores comments -- the pattern matches prose either way, and asserting otherwise would be asserting something false. A third test resolves both accessors, so renaming one cannot leave the failure message pointing at a function nobody can find. This is the third layer of the same lesson: orcpub.env exists because callers wrote (or (env :k) default); the clj-kondo rule exists because a correct accessor did not stop callers reaching past it; this exists because that rule cannot see one approved accessor used where another was required. 236 tests / 1046 assertions / 0 failures; lint errors 0, warnings 7 -- the same seven that predate this branch. The first push of this commit added two "Shadowed var: clojure.core/accessor" warnings from a local binding and a destructuring key; both renamed. --- test/clj/orcpub/secret_accessors_test.clj | 94 +++++++++++++++++++++++ 1 file changed, 94 insertions(+) create mode 100644 test/clj/orcpub/secret_accessors_test.clj diff --git a/test/clj/orcpub/secret_accessors_test.clj b/test/clj/orcpub/secret_accessors_test.clj new file mode 100644 index 000000000..122998b74 --- /dev/null +++ b/test/clj/orcpub/secret_accessors_test.clj @@ -0,0 +1,94 @@ +(ns orcpub.secret-accessors-test + "Two settings can come from a mounted file as well as the environment, and must + only ever be read through the accessor that knows that. + + SIGNATURE -> config/signature (/run/secrets/signature first) + DATOMIC_PASSWORD -> config/datomic-password (/run/secrets/datomic_password first) + + Reading the environment directly still compiles, still passes every other + test, and still works on any deployment that uses environment variables -- + which is why it survived review twice. It breaks only for the deployment that + followed the Docker-secrets instructions in docker-compose.yaml: the secret is + mounted, the variable is deliberately absent, and the direct read returns nil. + In a container there is no .lein-env to mask it, so check-auth returns 500 and + every authenticated call fails with a good secret sitting on disk. + + That is exactly what routes.clj did until 969cf644. orcpub.config already had + the right accessor; the caller reached past it. A correct accessor nobody is + obliged to use is worth nothing -- the lesson this branch learned about + environ.core/env and then repeated one layer up. + + clj-kondo cannot catch this the way it catches bare environ reads: + :discouraged-var matches VARS, and (env/value :signature) is an approved var + with a particular argument. Distinguishing that needs a custom hook, so this + scans the source instead. `lein test` already runs in CI, which is the only + place enforcement matters." + (:require [clojure.test :refer [deftest is testing]] + [clojure.java.io :as io] + [clojure.string :as str])) + +(def ^:private secret-keys + {":signature" "config/signature" + ":datomic-password" "config/datomic-password"}) + +(def ^:private allowed-file + "orcpub/config.clj is where both accessors live, so it reads the keys legitimately." + "config.clj") + +(defn- clj-sources [] + (->> (file-seq (io/file "src")) + (filter #(and (.isFile %) (str/ends-with? (.getName %) ".clj"))) + (remove #(= allowed-file (.getName %))))) + +(defn- offences + "[{:file :line :text :key}] for every direct env read of a secret-backed key. + Comments are skipped: these keys are named in prose all over the codebase, + including in the docstring above." + [] + (for [f (clj-sources) + [n line] (map-indexed vector (str/split-lines (slurp f))) + [k reader] secret-keys + :let [code (str/replace line #";.*$" "")] + :when (re-find (re-pattern (str "\\(env/value\\s+" k "\\b")) code)] + {:file (.getPath f) :line (inc n) :key k :accessor reader :text (str/trim line)})) + +(deftest secrets-are-not-read-straight-from-the-environment + (testing "every secret-backed key goes through its accessor, not env/value" + (let [bad (offences)] + (is (empty? bad) + (str "These read a secret-backed setting directly, so a deployment using " + "Docker secrets gets nil and all authentication fails:\n" + (str/join "\n" + (for [{:keys [file line key] :as o} bad] + (format " %s:%d reads %s — use %s\n %s" + file line key (:accessor o) (:text o))))))))) + +(deftest the-check-can-still-find-something + ;; A scan that cannot match is a scan that passes forever. Prove the pattern + ;; fires on the exact text it exists to reject, without needing a real offence + ;; in the tree. + (testing "the pattern matches a direct read" + (is (re-find #"\(env/value\s+:signature\b" "(def x (env/value :signature))")) + (is (re-find #"\(env/value\s+:datomic-password\b" "(env/value :datomic-password)"))) + + (testing "and does not match the accessor it points people at" + (is (nil? (re-find #"\(env/value\s+:signature\b" "(config/signature)")))) + + (testing "comments are stripped BEFORE matching, not ignored by the pattern" + ;; The pattern matches prose either way. It is the strip in `offences` that + ;; spares it, so that is what gets asserted -- claiming the pattern ignores + ;; comments would be asserting something false. + (let [line ";; never write (env/value :signature) here"] + (is (re-find #"\(env/value\s+:signature\b" line) + "the raw pattern does match, which is why the strip exists") + (is (nil? (re-find #"\(env/value\s+:signature\b" + (str/replace line #";.*$" ""))) + "with the comment removed there is nothing left to match")))) + + +(deftest both-accessors-still-exist + ;; If one is renamed, the message above would name a function nobody can find. + (testing "the accessors this test points people at are real" + (require 'orcpub.config) + (is (some? (resolve 'orcpub.config/signature))) + (is (some? (resolve 'orcpub.config/datomic-password))))) From b29d89efcbc4e21b8ef7a7256e9ec87f3bb37f20 Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Fri, 25 Sep 2026 13:50:35 +0000 Subject: [PATCH 34/35] Fix the three registration paths Greptile flagged, and pin lein in the locale job From the first Greptile review on the codeGlaze/orcpub#35 mirror of #695. Each finding was checked against the code before anything was changed; four were real, one was not. 1. A failed resend invalidated the link already in the user's inbox (P1). verification-key is cardinality-one, so a resend's transaction REPLACES the existing key before the send is attempted. My rollback then retracted the new key and restored nothing, leaving the account with no working link at all -- an email that was still valid stopped verifying anything because a later resend failed. do-verification now pulls the previous key and sent-time before the write and puts them back on failure; a fresh registration still retracts the whole entity as before. re-verify-rollback-must-not-delete-an-existing-user previously asserted the key was nil afterwards, which encoded the bug. It now asserts the original key and expiry survive. Verified by reinstating the retract-only rollback: both new assertions fail. 2. A resend on a mail-less instance promised an email that never comes (P1). With no SMTP and ALLOW_UNVERIFIED_REGISTRATION set, re-verify verifies on the spot and returns {:verified? true}, but :re-verify-success ignored its response and always routed to "check your email". It now routes to the "registration complete, you can log in" page when the flag is present, the same branch :register-success already had. 3. Registration being switched off was invisible to the user (P1). Two gaps: the server THREW, which reached the client as a bare 500 indistinguishable from any other failure; and :register-failure only cleared login state, while register-form never rendered the message area at all. The refusal is now a 503 with {:error :email-not-configured}, :register-failure shows a message for it and a generic one for every other failure, and the form renders the same message block the login form uses. Previously EVERY server-side registration failure was silent, not just this one. 5. The locale job fetched Leiningen from the moving `stable` ref (P2). Pinned to 2.11.2, the version continuous-integration.yml already requests. NOT CHANGED -- 4. "Mirror PR checks do not run" (P2). Both workflows run on the head commit through their push trigger, and #35's head shows all three jobs green. Adding mirror/upstream-develop to the pull_request filter would put review-mirror plumbing into the upstream PR's workflows. lein test 236 tests / 1047 assertions / 0 failures; lein lint errors 0, warnings 7 (all predate the branch); lein fig:build compiles; locale.yml parses. --- .github/workflows/locale.yml | 4 ++- src/clj/orcpub/routes.clj | 25 ++++++++++++++++--- src/cljs/orcpub/dnd/e5/events.cljs | 17 ++++++++++--- src/cljs/orcpub/dnd/e5/views.cljs | 6 +++++ .../clj/orcpub/registration_no_email_test.clj | 13 +++++----- .../clj/orcpub/registration_rollback_test.clj | 12 +++++++-- 6 files changed, 61 insertions(+), 16 deletions(-) diff --git a/.github/workflows/locale.yml b/.github/workflows/locale.yml index 6dbc56f6f..9fa24cef1 100644 --- a/.github/workflows/locale.yml +++ b/.github/workflows/locale.yml @@ -45,10 +45,12 @@ jobs: distribution: temurin java-version: '21' + # Pinned to the version continuous-integration.yml uses. `stable` is a + # moving ref, so an upstream release could change or break this check. - name: Install Leiningen run: | curl -fsSL -o "$HOME/lein" \ - https://raw.githubusercontent.com/technomancy/leiningen/stable/bin/lein + https://raw.githubusercontent.com/technomancy/leiningen/2.11.2/bin/lein chmod +x "$HOME/lein" echo "$HOME" >> "$GITHUB_PATH" "$HOME/lein" version diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index 06e0289b6..82b131ca4 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -421,8 +421,9 @@ "verification email can be sent. Set it, or set" "ALLOW_UNVERIFIED_REGISTRATION=true to accept accounts without" "verifying the address.") - (throw (ex-info "Registration is temporarily unavailable. Please contact the site administrator." - {:error :email-not-configured}))) + ;; A response, not a throw. A throw reaches the client as a bare 500 it + ;; cannot tell apart from any other failure, so the form showed nothing. + {:status 503 :body {:error :email-not-configured}}) ;; Deliberately running without email: verify on the spot rather than ;; promising a mail that cannot be sent. The operator of a mail-less @@ -449,6 +450,15 @@ ;; need different rollbacks -- never retract the ENTITY for a user who ;; already existed, only the attributes this attempt set. existing-id (:db/id tx-data) + ;; What a resend is about to overwrite. verification-key is + ;; cardinality-one, so the new transaction REPLACES the link already + ;; sitting in the user's inbox. If this send then fails, retracting the + ;; new key is not enough -- the old one has to come back, or a link that + ;; was still valid stops working because a later resend failed. + previous (when existing-id + (d/pull (d/db conn) + [:orcpub.user/verification-key :orcpub.user/verification-sent] + existing-id)) tempid "verification-subject" report (try @(d/transact @@ -472,8 +482,15 @@ (println "ERROR: Verification email failed, rolling back:" (.getMessage e)) (try @(d/transact conn (if existing-id - [[:db/retract eid :orcpub.user/verification-key verification-key] - [:db/retract eid :orcpub.user/verification-sent now]] + ;; Put back what was there, or remove what we added. + (let [{old-key :orcpub.user/verification-key + old-sent :orcpub.user/verification-sent} previous] + [(if old-key + [:db/add eid :orcpub.user/verification-key old-key] + [:db/retract eid :orcpub.user/verification-key verification-key]) + (if old-sent + [:db/add eid :orcpub.user/verification-sent old-sent] + [:db/retract eid :orcpub.user/verification-sent now])]) [[:db/retractEntity eid]])) (catch Exception re ;; Report the rollback failure, but surface the original cause. diff --git a/src/cljs/orcpub/dnd/e5/events.cljs b/src/cljs/orcpub/dnd/e5/events.cljs index fa645d775..3edecbf28 100644 --- a/src/cljs/orcpub/dnd/e5/events.cljs +++ b/src/cljs/orcpub/dnd/e5/events.cljs @@ -1868,7 +1868,13 @@ (reg-event-fx :register-failure (fn [cofx [_ response]] - {:dispatch [:clear-login]})) + ;; This used to clear login state and nothing else, so every server-side + ;; failure -- including registration being switched off -- left the form + ;; looking as if nothing had happened. + (dispatch-login-failure + (if (= :email-not-configured (get-in response [:body :error])) + "Registration is currently unavailable. Please contact the site administrator." + "Registration failed. Please try again.")))) #_ ;; dead stub — real impl is orcpub.registration/validate-registration (defn validate-registration []) @@ -1952,8 +1958,13 @@ (reg-event-db :re-verify-success - (fn [db []] - (assoc db :route routes/verify-sent-route))) + (fn [db [_ response]] + ;; With no SMTP and ALLOW_UNVERIFIED_REGISTRATION set, a resend verifies the + ;; account on the spot and says so -- "check your email" would leave the user + ;; waiting for a message that is never sent. + (assoc db :route (if (get-in response [:body :verified?]) + routes/verify-success-route + routes/verify-sent-route)))) (reg-event-fx :re-verify diff --git a/src/cljs/orcpub/dnd/e5/views.cljs b/src/cljs/orcpub/dnd/e5/views.cljs index f233ca2e8..aec4d38c4 100644 --- a/src/cljs/orcpub/dnd/e5/views.cljs +++ b/src/cljs/orcpub/dnd/e5/views.cljs @@ -920,6 +920,12 @@ [:span "Already have an account?"] (login-link)] [:div.m-t-10.m-b-20 [:span "After clicking JOIN A validation email will be sent to the above email address."]] + (when @(subscribe [:login-message-shown?]) + [:div.m-t-5.p-r-5.p-l-5 + [message + :error + @(subscribe [:login-message]) + hide-login-message]]) [:button.form-button {:style {:height "40px" :width "174px" diff --git a/test/clj/orcpub/registration_no_email_test.clj b/test/clj/orcpub/registration_no_email_test.clj index dc03ac0c2..104bc27bf 100644 --- a/test/clj/orcpub/registration_no_email_test.clj +++ b/test/clj/orcpub/registration_no_email_test.clj @@ -120,11 +120,12 @@ email/unverified-registration-allowed? (constantly false) routes/send-verification-email (fn [& _] (throw (AssertionError. "must not attempt a send")))] - (let [thrown (try (routes/register (register-request conn)) nil - (catch Throwable e e))] - (is (some? thrown) - "registration must FAIL when email config is missing and nothing opted out") - (is (= :email-not-configured (:error (ex-data thrown))) - (str "expected a specific, actionable error. Got: " (pr-str (ex-data thrown)))) + (let [resp (routes/register (register-request conn))] + (is (= 503 (:status resp)) + "registration must be REFUSED when email config is missing and nothing opted out") + ;; A response rather than a throw: a throw reached the client as a bare + ;; 500 and the form showed nothing. The client matches on this key. + (is (= :email-not-configured (get-in resp [:body :error])) + (str "expected a specific, actionable error. Got: " (pr-str resp))) (is (nil? (find-user (d/db conn))) "and no account may be created, verified or otherwise")))))) diff --git a/test/clj/orcpub/registration_rollback_test.clj b/test/clj/orcpub/registration_rollback_test.clj index 647e39582..1d6e7e510 100644 --- a/test/clj/orcpub/registration_rollback_test.clj +++ b/test/clj/orcpub/registration_rollback_test.clj @@ -164,5 +164,13 @@ (is (= (:orcpub.user/email before) (:orcpub.user/email after))) (is (= (:orcpub.user/password before) (:orcpub.user/password after)) "credentials must survive a failed resend") - (is (nil? (:orcpub.user/verification-key after)) - "the key from the failed attempt should be retracted"))))))) + ;; Not nil: the resend REPLACED the key in the user's inbox before + ;; the send failed. Retracting the new one left the account with no + ;; working link at all, so an email that was still valid stopped + ;; verifying anything. The rollback has to put the old key back. + (is (= (:orcpub.user/verification-key before) + (:orcpub.user/verification-key after)) + "the link already in the user's inbox must still work after a failed resend") + (is (= (:orcpub.user/verification-sent before) + (:orcpub.user/verification-sent after)) + "and its expiry clock must be the original one, not the failed attempt's"))))))) From 94a71ea0aa6aa32405ecaa91dae624c2d37198fa Mon Sep 17 00:00:00 2001 From: codeGlaze <github@codeglaze.com> Date: Fri, 25 Sep 2026 14:29:00 +0000 Subject: [PATCH 35/35] Only restore the previous verification key if it is still ours From Greptile's second review on the codeGlaze/orcpub#35 mirror of #695: my round-1 fix introduced a race. That fix restored the pre-resend key when a send failed, using :db/add, which overwrites unconditionally. Two resends can overlap: A writes its key, B writes a newer key and emails it successfully, then A's send fails -- and A's rollback writes the ORIGINAL key back over B's. B's link, the one actually sitting in the user's inbox, stops verifying anything. The same happens to the expiry time. The restore now uses :db/cas, so it only happens while the stored value is still the one this attempt wrote; the key and sent-time are in one transaction, so it is all or nothing. If a newer resend has taken over, the cas fails, and that is logged as an INFO skip rather than as a rollback failure, because the newer state is the right state to keep. The retract branches needed no change: retracting a value that is no longer current is already a no-op in Datomic. a-failed-resend-must-not-clobber-a-newer-successful-one reproduces the race deterministically: the send stub writes a newer key and time (standing in for B landing between A's write and A's failure), then throws. Against the :db/add version both assertions fail; with :db/cas both pass, the uncontended restore test from round 1 still passes, and the run logs the INFO skip rather than the ERROR path. --- src/clj/orcpub/routes.clj | 23 ++++++++---- .../clj/orcpub/registration_rollback_test.clj | 36 +++++++++++++++++++ 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/src/clj/orcpub/routes.clj b/src/clj/orcpub/routes.clj index 82b131ca4..cf8fa3d8e 100644 --- a/src/clj/orcpub/routes.clj +++ b/src/clj/orcpub/routes.clj @@ -482,20 +482,31 @@ (println "ERROR: Verification email failed, rolling back:" (.getMessage e)) (try @(d/transact conn (if existing-id - ;; Put back what was there, or remove what we added. + ;; Put back what was there, or remove what we added -- + ;; but ONLY while the values are still ours. Two + ;; resends can overlap: if a newer one wrote its key + ;; and emailed it after we wrote ours, restoring the + ;; old key would kill the link actually sitting in + ;; the inbox. :db/cas makes the restore conditional + ;; and the whole transaction atomic; a retract of a + ;; value that is no longer current is already a no-op. (let [{old-key :orcpub.user/verification-key old-sent :orcpub.user/verification-sent} previous] [(if old-key - [:db/add eid :orcpub.user/verification-key old-key] + [:db/cas eid :orcpub.user/verification-key verification-key old-key] [:db/retract eid :orcpub.user/verification-key verification-key]) (if old-sent - [:db/add eid :orcpub.user/verification-sent old-sent] + [:db/cas eid :orcpub.user/verification-sent now old-sent] [:db/retract eid :orcpub.user/verification-sent now])]) [[:db/retractEntity eid]])) (catch Exception re - ;; Report the rollback failure, but surface the original cause. - (println "ERROR: Rollback ALSO failed; a partial account may remain:" - (.getMessage re)))) + (if (re-find #"cas-failed" (str re (some-> re .getCause))) + ;; Not a failure: a newer resend superseded this attempt, so its + ;; state is the right state to keep. + (println "INFO: verification rollback skipped — a newer resend owns the key now.") + ;; Report the rollback failure, but surface the original cause. + (println "ERROR: Rollback ALSO failed; a partial account may remain:" + (.getMessage re))))) (throw (ex-info "Unable to complete registration. Please try again or contact support." {:error :verification-email-failed} e))))))) diff --git a/test/clj/orcpub/registration_rollback_test.clj b/test/clj/orcpub/registration_rollback_test.clj index 1d6e7e510..31a9b3ffa 100644 --- a/test/clj/orcpub/registration_rollback_test.clj +++ b/test/clj/orcpub/registration_rollback_test.clj @@ -174,3 +174,39 @@ (is (= (:orcpub.user/verification-sent before) (:orcpub.user/verification-sent after)) "and its expiry clock must be the original one, not the failed attempt's"))))))) + +(deftest a-failed-resend-must-not-clobber-a-newer-successful-one + ;; Two resends overlap. A writes its key, B writes a newer one and emails it, + ;; then A's send fails. A's rollback must NOT put the original key back over + ;; B's: B's link is the one sitting in the user's inbox. Restore only if the + ;; key is still the one this attempt wrote -- otherwise a newer resend owns it. + (with-conn conn + (let [mocked-conn (dm/fork-conn conn)] + (seed-schema mocked-conn) + (with-redefs [email/configured? (constantly true) + routes/send-verification-email (fn [& _] nil)] + (routes/register (register-request mocked-conn))) + (let [uid (:db/id (find-user (d/db mocked-conn) "newcomer")) + newer-key "key-from-a-concurrent-successful-resend" + newer-sent (java.util.Date.)] + (testing "the newer resend's key survives the older one's rollback" + (with-redefs [email/configured? (constantly true) + routes/send-verification-email + (fn [& _] + ;; Simulate B landing between A's write and A's failure. + @(d/transact mocked-conn + [{:db/id uid + :orcpub.user/verification-key newer-key + :orcpub.user/verification-sent newer-sent}]) + (throw (Exception. "SMTP down")))] + (try (routes/re-verify {:conn mocked-conn + :db (d/db mocked-conn) + :scheme :https + :headers {"host" "example.test"} + :query-params {:email "newcomer@test.com"}}) + (catch Throwable _ nil))) + (let [after (find-user (d/db mocked-conn) "newcomer")] + (is (= newer-key (:orcpub.user/verification-key after)) + "a failed resend overwrote the link a newer resend had already emailed") + (is (= newer-sent (:orcpub.user/verification-sent after)) + "and reset that link's expiry clock to the stale one")))))))