Conversation
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.
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.
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.
…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.
…y 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
c9fd5a5, 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 d42e05d, and this branch has added none.
lein test: 220 tests, 985 assertions, 0 failures.
…tlived 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/<version>/ 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.
… "strict"
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.
…n 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.
…r 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 813fdbb (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.
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.
…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.
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.
Owner
Author
|
@greptileai review Generated by Claude Code |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…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.
Owner
Author
|
@greptileai review Generated by Claude Code |
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.
Owner
Author
|
@greptileai review Generated by Claude Code |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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:
Review mirror of Orcpub#695 — do not merge. The base branch
mirror/upstream-developis pinned to upstreamdevelop's tip (d42e05d1), so this diff is identical to Orcpub#695. Review rounds happen here, where findings can be read and fixed directly; Orcpub#695 is updated only when a round comes back clean.Related issue (if applicable): Orcpub#695
What it contains:
/assets/*came back empty on Spanish, German and other non-English systems (no icon font). Plus the Turkish dotless-i fixes (Locale/ROOT,equalsIgnoreCase) and a CI matrix undertr_TR/es_ES.netstatparser now works on localised Windows and ignores UDP rows;SERVER_PORTfollowsPORT, which both service maps now read.orcpub.env/valuetreats an exported-but-empty variable as unset. A clj-kondo rule stops directenviron.core/envreads, and a test makes the Docker-secret-backed settings (SIGNATURE,DATOMIC_PASSWORD) go throughconfig/signatureandconfig/datomic-password.ALLOW_UNVERIFIED_REGISTRATION=true, in which case accounts are verified on creation. The server warns at startup when the configuration is in any of the abnormal states.README,DOCKER.md,ENVIRONMENT.md,docker-user-management.md,email-system.md) updated for the new settings; the false CSP "Report-Only" claims removed.Checklist:
lein test236 tests / 1046 assertions / 0 failures;lein fig:buildcompiles; CI green on Windows and Locale jobslein linterrors 0, warnings 7, all present at the baseGenerated by Claude Code