You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Fix locale-dependent asset, CSP and content-key failures, and the Windows port checks - #695
Three locale defects and one Windows defect, all of which fail silently. Found by chasing one bug report: a self-hoster on Spanish Windows whose ability-score reorder arrows had vanished.
1. Every /assets/* webjar request returns 200 with a zero-byte body on a non-English machine
Font Awesome never loads, so icon-only controls disappear — zero width, no hit target, absent from the accessibility tree — with no error in the browser console or the server log.
Two defects combine, and it takes both to make it silent:
rfc822-formatter has no Locale (pedestal.clj). HTTP dates are always English (RFC 7231), but ofPattern without a Locale parses with the JVM default:
DateTimeParseException: Text 'Mon, 06 Jul 2020 14:41:47 +0000' could not be parsed at index 0
Index 0 is "Mon" — Spanish days are lun/mar/mié.
The ETag interceptor's catch discards the response.(catch Throwable t (log/error ...)) returns log/error's value, not context. Pedestal then has nothing to write and Jetty emits a bare 200. The exception is logged — but the response is already gone.
Why only /assets/*:::http/resource-path "/public" puts the resource interceptor ahead of the router, so /css/* is served and the chain terminated before ETag runs. The app's own CSS loads fine while every webjar asset is blank — which is why it reads as a Font Awesome problem.
2. CSP_POLICY and content keys fold case with the operator's locale
str/lower-case takes no Locale on the JVM. On a Turkish or Azerbaijani machine:
CSP_POLICY=STRICT becomes strıct (dotless i), matches no branch, and falls through to the permissive policy — a silent security downgrade.
name-to-kw turns "Illusory Script" into :ıllusory-script. A different key for the same content, server-side only. That is a data problem, not a display one — warlock-test could not find an invocation spell under tr_TR.
Character and spell name search return nothing for an uppercase query: "FIRE" folds to "fıre".
Adds orcpub.common/ascii-lower-case and uses it at the sites that fold machine tokens. ClojureScript needs no guard and gets none — JS toLowerCase() is already locale-invariant, so only the JVM side ever diverged.
3. The CSP docs described a mode that does not exist
Five places documented a Content-Security-Policy-Report-Only dev mode. No code has ever emitted that header — in dev no nonce is generated so no header is set at all, otherwise it is enforcing. That mattered because DEV_MODE defaults to false: a checkout with no .env runs enforcing CSP whose connect-src omits ws://localhost:3449, Figwheel's hot reload is blocked, and the docs told anyone investigating that dev mode was safe.
No default is changed — .env.example already ships DEV_MODE=true. Instead start.sh surfaces the policy where it is read: --check output, a first-run .env prompt, and a decision before starting Figwheel.
4. The Windows port checks never worked, and reported success either way
port_in_use falls through to netstat -tln. On Git Bash that is Windows' netstat.exe, which errors, writes nothing to stdout, and the check reads that as "port is free". So start.sh launched into an occupied port and died with BindException, stop.sh found nothing to stop, and start.sh datomic reported failure ~30s after a successful start.
Fixed by detecting Git Bash/MSYS/Cygwin and parsing netstat -anoby column position, not status text — Windows prints LISTENING where Microsoft's reference documents LISTEN, and a non-English Windows translates it. Adds signal_pid (kill, then taskkill, covering both PID kinds), explain_bind_failure, and a headless REPL mode. .github/workflows/windows-scripts.yml asserts 14 facts on a real windows-latest runner in ~1 minute.
Also repaired here:start.sh --check exited silently after one line. check_java assumed java -version puts the version on line 1, but JAVA_TOOL_OPTIONS prints a preamble there; [[ "$str" -lt 11 ]] then evaluated that string as arithmetic, and under set -u the unset bare word was fatal. Separately ((failed++)) returns 1 when the counter is 0, so set -e killed the script on the first failed check.
Checklist:
The code change is tested and works locally. lein test: 220 tests / 985 assertions / 0 failures, green under en_US, es_ESand tr_TR. lein fig:build compiles clean (the .cljc changes reach the frontend). Windows CI green on this branch. Reporter confirmed the asset fix on their own Spanish Windows machine.
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation if necessary — no docs/ change; these are invisible failure modes rather than documented behaviour. The misleading CSP comments are corrected in place.
There is no commented out code in this PR.
My changes generate no new warnings. orcpub.config loads under *warn-on-reflection* with none. shellcheck 0.11.0 on all three scripts: zero new finding types vs the upstream baseline (11 before, 11 after).
Testing
Asset route against the real Font Awesome webjar, before → after:
locale
before
after
es_ES / de_DE
200, no Content-Type, 0 bytes
200, text/css, 58,935 bytes
ja_JP / tr_TR
—
200, text/css, 58,935 bytes
en_US
already worked
unchanged
ETag values are identical to what English-locale servers already produce (1594046507000-58935), so no caches are invalidated.
New config-test and pedestal-test pin the defects and were verified to fail without the fix — 5 failures against the unfixed files, 0 with them. One deliberately does not assert against rfc822-formatter: it is a top-level def, so its locale is fixed at namespace load and no in-process test can reproduce the original failure; it demonstrates the hazard instead and says so.
tr_TR had 3 pre-existing failures before item 2; now green, which makes a locale matrix usable as a CI gate.
Notes for reviewers
DateTimeFormatter/RFC_1123_DATE_TIME would be cleaner still — locale-independent by construction and it removes the (s/replace …) surgery, for an identical value. Worth a follow-up.
Nothing changes on an English-locale Linux server: identical ETags, identical keys, Windows code behind is_windows().
The four items are independent and can be split on request.
Not done: the app was never booted (no transactor here). JVM suite, CLJS build and Windows runner all pass.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
Pausing mid-flight, so the ledger now opens with a WHERE WE ARE section
rather than burying the state below the history.
PR Orcpub#695 is OPEN and carries a Copilot review -- 6 medium,
1 low -- that nobody has triaged. That review is the next piece of work.
Nothing is merged.
The review comments could not be read verbatim from here: GitHub renders
them client-side so they are absent from the static HTML, the
unauthenticated API returns 403, and this session's GitHub tools are
scoped to codeglaze/orcpub. The ledger records the seven topics as
LEADS, explicitly not quotations, and says to read the real comments
before acting.
Two were spot-checked and are real defects in code written during this
session:
PORT in .env never reaches the scripts. .env.example documents
PORT=8890 and the server honours it, but common.sh uses SERVER_PORT.
Set PORT=9000 and the server moves while every port check,
explain_bind_failure and the new report_env_config still look at 8890
-- so the config reporting added here makes the mismatch more visible
and still wrong.
permissive CSP blocks Figwheel too. permissive-csp-settings sets
default-src 'self' and no connect-src, so connect-src falls back and
the websocket is blocked. dev_mode_blocks_figwheel only warns for
strict, staying silent on a case that is equally broken.
Also fixes kb.py printing a BrokenPipeError traceback when its output is
piped to head -- a tool that looks broken while working is the exact
thing this KB argues against.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DraEeSebsmHp5AYJacsQGW
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.
codeGlaze
added a commit
to codeGlaze/orcpub
that referenced
this pull request
Sep 20, 2026
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.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DraEeSebsmHp5AYJacsQGW
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.
Pausing mid-flight, so the ledger now opens with a WHERE WE ARE section
rather than burying the state below the history.
PR Orcpub#695 is OPEN and carries a Copilot review -- 6 medium,
1 low -- that nobody has triaged. That review is the next piece of work.
Nothing is merged.
The review comments could not be read verbatim from here: GitHub renders
them client-side so they are absent from the static HTML, the
unauthenticated API returns 403, and this session's GitHub tools are
scoped to codeglaze/orcpub. The ledger records the seven topics as
LEADS, explicitly not quotations, and says to read the real comments
before acting.
Two were spot-checked and are real defects in code written during this
session:
PORT in .env never reaches the scripts. .env.example documents
PORT=8890 and the server honours it, but common.sh uses SERVER_PORT.
Set PORT=9000 and the server moves while every port check,
explain_bind_failure and the new report_env_config still look at 8890
-- so the config reporting added here makes the mismatch more visible
and still wrong.
permissive CSP blocks Figwheel too. permissive-csp-settings sets
default-src 'self' and no connect-src, so connect-src falls back and
the websocket is blocked. dev_mode_blocks_figwheel only warns for
strict, staying silent on a case that is equally broken.
Also fixes kb.py printing a BrokenPipeError traceback when its output is
piped to head -- a tool that looks broken while working is the exact
thing this KB argues against.
The reason will be displayed to describe this comment to others. Learn more.
Copilot review overview
🟡 Changes recommended
Launcher configuration and Windows port handling can still target incorrect ports or processes, and parts of the workflow do not test their intended behavior.
Get a fresh assessment by requesting another Copilot review.
…ced 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.
…gree
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.
…t-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.
… reviews
Three review rounds on PR Orcpub#695 found the same defect in different disguises, and
the tangent is long enough that the next agent would have to re-derive it from
commit messages. One document instead.
The rule: environ returns "" for an exported-but-empty variable, "" is truthy in
Clojure, so the default in (or (env :k) "default") is never reached. .env.example
ships nine keys empty and docker-compose.yaml passes seven as ${VAR:-}, so blank
is the documented, default, shipped state -- not an edge case.
Records, each measured rather than inferred: JWTs signed AND verified against ""
(with the nil-check guard written for exactly that case walked past, because ""
is truthy); get-datomic-uri returning "?password="; CSP_POLICY= silently
selecting permissive over the documented strict; three parseInt crashes; Docker
sending mail with an empty From; and index.clj emitting fetch('') on every page
load, which re-fetched the current page and tried to parse it as an .orcbrew
pack.
Three things the next person needs and would not guess:
- A HELPER IS NOT ENOUGH. orcpub.config already had a correct `signature`
accessor and routes.clj read (environ/env :signature) raw anyway, four
times. That is how the token bug survived. The fix is the clj-kondo
:discouraged-var rule at :error; the namespace is only where the exception
lives.
- ONE PLACE KEEPS "" ON PURPOSE. postal answers :host "" with a
MailConnectException naming localhost and :host nil with a bare NPE. The
blank rule is right where the APPLICATION reads the value, not where a
library has already assigned "" a meaning. Check the consumer.
- HOW TO VERIFY AGAINST DOCKER: git archive the before-commit to /tmp, run the
whole config surface under the exact compose environment in both trees, and
diff. That is what caught the postal regression. Also: the shell scripts
cannot affect Docker at all -- the app image is ENTRYPOINT java -jar with no
scripts/ copied in.
Carries one OPEN finding that is worse than the bypass and is not fixed
anywhere: nothing checks whether email is configured, and do-verification
transacts the user BEFORE attempting the email. On any instance without SMTP --
the default Docker state -- registration creates the account, throws, tells the
user to try again, and the retry fails validation because the name and address
are now taken. The account can never be verified. Reachable by default rather
than by misconfiguration.
Also corrects two docs from inside this branch's own KB: AGENTS.md claims twice
that Clojure reads .env via "environ or dotenv" -- there is no dotenv dependency
on any branch and Clojure never reads .env, which is likely why more than one
person remembers a dotenv approach being tried and abandoned. And
env-and-auth.md still describes a CSP "report-only" mode that has never existed.
Linked from the README index and findable: kb find "empty string", "dotenv",
"discouraged-var" and "registration lockout" all return it. kb lint clean at 139
documents, check-docs CLEAN, topic-index regenerated.
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.
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 (f412602), 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.
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.
…e 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).
…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.
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.
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.
This sentence still says Pedestal's built-in CSP defaults apply in dev, which contradicts both the rest of this paragraph and get-secure-headers-config (strict mode sets :content-security-policy-settings to nil). The documented behavior is that no application CSP header is emitted in strict dev mode.
Fix missing locale safety documentation
src/cljc/orcpub/common.cljc:22
The referenced docs/kb/locale-safety.md is not present in the repository, so this public helper docstring points readers to a dead path. Add that document in this PR or remove/replace the reference with an existing source.
…solvable
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.
… 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.
codeGlaze
added a commit
to codeGlaze/orcpub
that referenced
this pull request
Sep 24, 2026
The existing rule covers a doc you discover is wrong. This is the one that keeps
getting through review: a doc that was CORRECT until your change landed. You are
the only person who can know, and nothing tells you.
Both misses on PR Orcpub#695 were this shape, and both were in files the change never
touched:
Correcting the CSP docstrings left the same false "Report-Only" claim in
ENVIRONMENT.md, DOCKER.md and migration/pedestal-0.7.md -- the three files a
self-hoster actually reads.
Adding ALLOW_UNVERIFIED_REGISTRATION turned DOCKER.md's "registration still
works, just no verification emails" from true into backwards. An operator
following it would have taken sign-ups offline with no reason to doubt the doc.
So the practice is one command before committing: grep for the NAMES you changed,
not the files you edited. A new env var, a renamed fn, a changed default.
Two habits recorded with it. Search for the OLD BEHAVIOUR as well as the new name
-- the review named three docs, and grepping EMAIL_SERVER_URL found two more
(README.md:276, docker-user-management.md:91) with the same now-false "leave empty
to skip email". And when a doc mirrors a function, say so in the docstring, which
do-verification now does for docs/email-system.md.
Also records the register the operator docs should be in. The first pass at the
new setting said "fail closed" and "inert when SMTP is configured"; someone
reading DOCKER.md wants to know what happens if they leave a field blank. The
reasoning belongs in the code comment, where the next maintainer needs it.
kb lint clean at 139 documents, check-docs CLEAN.
This does not mirror env/value for quoted values containing surrounding whitespace. For example, CSP_POLICY=" STRICT " is trimmed to strict by the server but reported here as a permissive fallback; whitespace-only input is likewise strict on the server but permissive here. Trim first, then apply the empty-value default and ASCII case fold.
This issue also appears on line 126 of the same file.
Propagate setup copy failures
scripts/common.sh:182
A failed copy is reported but returned as success, so main continues and starts the requested services with the very defaults this setup step was meant to replace. This is especially misleading after the user chose production setup. Propagate failure so the caller aborts.
Reject unknown users during re-verification
src/clj/orcpub/routes.clj:447
re-verify passes {:db/id id} even when user-for-email found no account, so existing-id is nil and this fallback now allocates a fresh entity containing only verification state, then emails an unusable link. A successful request for an unknown address consequently leaves an orphan Datomic entity. Reject a missing re-verification subject before choosing the registration tempid (for example, distinguish a tx map with a nil :db/id from a new registration map).
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.
…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 969cf64, 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.
…e locale job
From the first Greptile review on the #35 mirror of Orcpub#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.
From Greptile's second review on the #35 mirror of Orcpub#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.
…nd replaced
re-verify passed {:db/id nil} for an address with no account, so do-verification registered a
key-only record and emailed a link to any address typed. It now answers like a resend and creates
nothing.
A failed resend restored the key it had read before writing. If an overlapping resend wrote and
emailed in between, that stale key replaced the live link. The previous values now come from the
failed attempt's own :db-before.
Both from Greptile on #46; each new test fails without its fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description:
Three locale defects and one Windows defect, all of which fail silently. Found by chasing one bug report: a self-hoster on Spanish Windows whose ability-score reorder arrows had vanished.
1. Every
/assets/*webjar request returns 200 with a zero-byte body on a non-English machineFont Awesome never loads, so icon-only controls disappear — zero width, no hit target, absent from the accessibility tree — with no error in the browser console or the server log.
Two defects combine, and it takes both to make it silent:
rfc822-formatterhas noLocale(pedestal.clj). HTTP dates are always English (RFC 7231), butofPatternwithout aLocaleparses with the JVM default:Index 0 is
"Mon"— Spanish days arelun/mar/mié.The ETag interceptor's
catchdiscards the response.(catch Throwable t (log/error ...))returnslog/error's value, notcontext. Pedestal then has nothing to write and Jetty emits a bare200. The exception is logged — but the response is already gone.Why only
/assets/*:::http/resource-path "/public"puts the resource interceptor ahead of the router, so/css/*is served and the chain terminated before ETag runs. The app's own CSS loads fine while every webjar asset is blank — which is why it reads as a Font Awesome problem.2.
CSP_POLICYand content keys fold case with the operator's localestr/lower-casetakes noLocaleon the JVM. On a Turkish or Azerbaijani machine:CSP_POLICY=STRICTbecomesstrıct(dotless i), matches no branch, and falls through to the permissive policy — a silent security downgrade.name-to-kwturns"Illusory Script"into:ıllusory-script. A different key for the same content, server-side only. That is a data problem, not a display one —warlock-testcould not find an invocation spell undertr_TR."FIRE"folds to"fıre".Adds
orcpub.common/ascii-lower-caseand uses it at the sites that fold machine tokens. ClojureScript needs no guard and gets none — JStoLowerCase()is already locale-invariant, so only the JVM side ever diverged.3. The CSP docs described a mode that does not exist
Five places documented a
Content-Security-Policy-Report-Onlydev mode. No code has ever emitted that header — in dev no nonce is generated so no header is set at all, otherwise it is enforcing. That mattered becauseDEV_MODEdefaults to false: a checkout with no.envruns enforcing CSP whoseconnect-srcomitsws://localhost:3449, Figwheel's hot reload is blocked, and the docs told anyone investigating that dev mode was safe.No default is changed —
.env.examplealready shipsDEV_MODE=true. Insteadstart.shsurfaces the policy where it is read:--checkoutput, a first-run.envprompt, and a decision before starting Figwheel.4. The Windows port checks never worked, and reported success either way
port_in_usefalls through tonetstat -tln. On Git Bash that is Windows'netstat.exe, which errors, writes nothing to stdout, and the check reads that as "port is free". Sostart.shlaunched into an occupied port and died withBindException,stop.shfound nothing to stop, andstart.sh datomicreported failure ~30s after a successful start.Fixed by detecting Git Bash/MSYS/Cygwin and parsing
netstat -anoby column position, not status text — Windows printsLISTENINGwhere Microsoft's reference documentsLISTEN, and a non-English Windows translates it. Addssignal_pid(kill, thentaskkill, covering both PID kinds),explain_bind_failure, and a headless REPL mode..github/workflows/windows-scripts.ymlasserts 14 facts on a realwindows-latestrunner in ~1 minute.Also repaired here:
start.sh --checkexited silently after one line.check_javaassumedjava -versionputs the version on line 1, butJAVA_TOOL_OPTIONSprints a preamble there;[[ "$str" -lt 11 ]]then evaluated that string as arithmetic, and underset -uthe unset bare word was fatal. Separately((failed++))returns 1 when the counter is 0, soset -ekilled the script on the first failed check.Checklist:
lein test: 220 tests / 985 assertions / 0 failures, green underen_US,es_ESandtr_TR.lein fig:buildcompiles clean (the.cljcchanges reach the frontend). Windows CI green on this branch. Reporter confirmed the asset fix on their own Spanish Windows machine.docs/change; these are invisible failure modes rather than documented behaviour. The misleading CSP comments are corrected in place.orcpub.configloads under*warn-on-reflection*with none.shellcheck0.11.0 on all three scripts: zero new finding types vs the upstream baseline (11 before, 11 after).Testing
Asset route against the real Font Awesome webjar, before → after:
es_ES/de_DEtext/css, 58,935 bytesja_JP/tr_TRtext/css, 58,935 bytesen_USETag values are identical to what English-locale servers already produce (
1594046507000-58935), so no caches are invalidated.New
config-testandpedestal-testpin the defects and were verified to fail without the fix — 5 failures against the unfixed files, 0 with them. One deliberately does not assert againstrfc822-formatter: it is a top-leveldef, so its locale is fixed at namespace load and no in-process test can reproduce the original failure; it demonstrates the hazard instead and says so.tr_TRhad 3 pre-existing failures before item 2; now green, which makes a locale matrix usable as a CI gate.Notes for reviewers
DateTimeFormatter/RFC_1123_DATE_TIMEwould be cleaner still — locale-independent by construction and it removes the(s/replace …)surgery, for an identical value. Worth a follow-up.is_windows().Issues
Closes #697
Closes #699
Closes #700
Closes #701