cache_bust() in web/includes/functions.php symlinks every css and js file to a
name with the source file's mtime in it, so each time a source changes a new
link appears and the old one is left behind. Nothing has ever removed them.
The install this was written on had 8346 entries going back to April 2020,
1562 of them pointing at files that no longer exist -- one of which was the
log.js deleted in the log panel refactor, still being served because the skin
auto-includes views/js/<view>.js whenever that file is present. The directory
inode had grown to 823KB, which every lookup in that directory has to scan.
misc/zoneminder-tmpfiles.conf.in did not mention the directory at all, so
systemd neither created nor cleaned it. Add it as a d entry with a 30d age,
which the daily systemd-tmpfiles-clean.timer then acts on.
On the age, and why it is generous rather than a day or two: a symlink's mtime
and ctime are fixed when it is created, and neither being served nor having
its target read refreshes them -- verified here that traversing a symlink does
not update its atime either. So an age removes entries whether or not they are
in use, rather than only the unused ones. That is safe, because the directory
is derived and not authoritative: a link removed while still wanted is
recreated by the next page render, so the worst case is one missing asset on a
page that was already open. 30d keeps about a month of source versions for a
few recreated symlinks a month, where a short age would churn the whole set
repeatedly for no further benefit.
d and not D: D empties the directory at boot, which would throw away every
current entry on every reboot.
Tests: tests/php/test_tmpfiles_conf.php, 12 checks. Nothing exercises this
file during a build or a test run, and a mistake in it is quiet in both
directions -- a malformed line is skipped with a log message nobody reads, and
a missing age field just means a directory grows forever, which is how this
one got to 8346 entries. So the fields are parsed and checked, and the entry
is checked to name the same cmake variable that config.php.in defines
ZM_DIR_CACHE from, since if those two drift systemd would clean a directory
nothing writes to and nothing would report it.
The last check hands the file to systemd-tmpfiles itself, with every path
rewritten into a temporary directory first so that running a clean cannot
touch anything real. That check skips where systemd-tmpfiles is not installed.
Confirmed separately that a d entry with an age does remove symlinks, which is
what this directory is made of.
Verified the checks fail when the age is dropped, when d becomes D, when the
entry names a variable config.php.in does not use, and on an age systemd would
reject.
Ran: tests/php/test_tmpfiles_conf.php (12 passed), the rest of tests/php and
tests/js, npx eslint . (clean), utils/check-homoglyphs.py (clean).
test_bandwidth_clamp.php fails on this box for an unrelated reason: it cannot
read /etc/zm/conf.d/03custom.conf and the permission warning lands in its
captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
The known-issues list recorded each finding as path:line, and a rebase onto a
master carrying 1677470af1 inserted four lines above one of them in
monitor.php. The finding moved from 1483 to 1487, so it read as both a new
offender and a missing list entry, and the test failed on code nobody had
touched.
A line number is not a stable identity for a finding. Key on the file and the
variable being assigned instead, which survives anything inserted above and
still names the offender uniquely -- there is no file here assigning the same
variable twice with a cut-short expression. The line is still reported, since
it is what someone needs in order to go and look, but it is not what gets
compared.
Verified the test still catches a new instance, and that shifting every line
in a listed file no longer breaks it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
A saved time based action listed its schedule as "0 21 * * *" rather than
"Every day at 21:00". The cron parser was not at fault; cronToParts('0 21 * *
*') returns the right parts. The fault was one operator:
$drawable = ($action->Schedule() === '') or ($schedule_parts !== null);
"or" binds looser than "=", so this parses as ($drawable = ($action->Schedule()
=== '')) or (...). The right-hand clause is evaluated and thrown away, and
$drawable gets only the left one -- false for any schedule that is actually
set, which is every saved action. So the editor took every saved schedule for
one it could not draw and fell back to the read-only cron field, which is the
branch that exists for genuinely undrawable specs like "*/5 * * * *".
Changed to ||, which binds tighter than "=" and does what the line looks like
it does.
Tests: tests/php/test_assignment_precedence.php, 21 checks. It tokenizes every
php file under web/ and reports assignments whose right-hand side has an
unbracketed "and", "or" or "xor" -- tokenized rather than grepped so that the
many safely bracketed ones, in conditions and ternaries, are not reported.
There is a self-check for each distinction it has to make, since a scan that
quietly matched nothing would pass the tree-wide check on its own. Verified it
fails when the reported line is put back, and when a new instance is added
elsewhere.
The scan found six more in the rest of the tree, which the test carries as a
list of open questions rather than as exemptions. Two are the deliberate
"$fp = fopen(...) or error_log(...)" idiom and are correct. The other four are
this same bug, and each changes behaviour to fix, so none is swept up here --
in particular filter.php:401 assigns only $filter->Id() to $canDelete, so
delete is offered for any existing filter whether or not the user may edit it,
and that is access control rather than something to guess at. Anything not on
the list fails the test, so a new one cannot be added, and the list is checked
in both directions so it shrinks as the entries are dealt with.
Ran: tests/php/test_assignment_precedence.php (21 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
Looking for other instances of the CronTab.php fatal in the previous commit
found eleven, in ten files, dating from 2019 to 2023. Each is a call to a
logger function from the global namespace without the \ZM\ prefix, so each is
a fatal the moment its branch runs:
web/ajax/console.php, events.php, reports.php, add_monitors.php, status.php
web/includes/actions/snapshot.php (two), donate.php
web/includes/functions.php, session.php
web/views/image.php
They have survived this long because of where they sit. Every one is on an
error or diagnostic path -- an invalid sort order, a zero byte download, a
failed read lock -- so the call is only reached when something has already
gone wrong, and then it turns a handled problem into a blank 500. That is the
shape of the bug on the monitor save: the write had worked, and the thing that
broke was the attempt to log.
Verified before changing any of them that each file is in the global
namespace, does not define the function itself and does not import it, so the
call really does not resolve.
Tests: tests/php/test_logger_calls_qualified.php, 16 checks. It tokenizes
every php file under web/ and reports unqualified calls, with the function
names read out of logger.php rather than listed, so a new one is covered the
day it is added. web/api is skipped as cakephp with its own logging, and
assets as third party.
Php's own tokenizer rather than a regex, because the distinctions matter: a
method call on an object that happens to have an Error() method is fine, as
is a definition, a mention, a qualified call, the same call from inside
namespace ZM, and the name appearing in a comment or a string. There is a
check for each of those, since a scan that quietly matched nothing would pass
the tree-wide check on its own.
Verified the scan fails when the Debug() in CronTab.php is put back.
Ran: tests/php/test_logger_calls_qualified.php (16 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
Pin When to 11em and raise Schedule from 24em to 28em.
Both columns are sized by a select, and a select is as wide as its longest
option whether or not the column is. When's is "Manually triggered", which
needs about 11em; it had been sharing the leftover space three ways with
Device and Does and taking more than that. Schedule has to fit "Every
<frequency> at <time>" on one line, which comes to about 25em -- so the 24em
it was given was short, and the frequency select was the thing being
truncated.
The space comes out of Device and Does, which now divide the remainder
between them. Device is left loose on purpose: monitor names are the only
contents in this table of unpredictable length, so it is the right column to
absorb whatever is spare.
Worth knowing for later: Schedule is wide almost entirely because of one
frequency option, "weekend day (Sat & Sun)", which sets that select's width
by itself. Rewording just that label would give several em back to the rest
of the table. Not done here, since it is a visible label and not mine to
change.
Tests: tests/php/test_monitor_actions_columns.php, now 30 checks. Added a
check per select column that it fits its longest option, with the option keys
read out of the view and their text out of the language file so neither is
restated. That is the measurement these widths were chosen from, so it is the
one worth keeping: a label reworded longer would start truncating with
nothing to say it had.
The estimate for Schedule initially cleared 24em, which would have made it
too generous to catch what was reported. The two terms it was missing are
real and identifiable -- .table-sm's padding, and the collapsed space between
four controls written on separate source lines -- and together they come to
about an em and a half, which is roughly the difference complained of. There
is a check that the estimate still fails 24em, labelled as the calibration it
is rather than as a property of the css.
Verified the checks fail with Schedule back at 24em (1), with When too narrow
for its longest option (1), and with Device pinned so nothing is left loose
(3).
Ran: tests/php/test_monitor_actions_columns.php (30 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
4em was set from what the cells hold -- one checkbox -- and that is the wrong
thing to measure. A fixed layout will not widen a column to fit its header,
it lets the text run on into the next one, and skin.css renders every th
uppercase at 0.8em. So what has to fit is "ENABLED" plus .table-sm's padding,
which at 4em fit so exactly that it touched the When header. 5.5em leaves a
clear gap.
Tests: tests/php/test_monitor_actions_columns.php, now 21 checks. Dropped the
check that Enabled was the narrowest column "being one checkbox", since that
is the reasoning that produced the bug: "ENABLED" is a wider word than
"SOUND" whatever sits in the cells below them, so there is no ordering to
assert between the two. In its place each narrow column is checked against an
estimate of its own header's width, calibrated so that the 4em this started
with fails and 5.5em passes. The header text is read out of the language file
rather than written into the test, so a translation long enough to break the
layout fails too -- under a fixed layout it really would break it.
Ran: tests/php/test_monitor_actions_columns.php (21 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
ul.form indents every row by 300px to make a gutter for its label, and pulls
the label back into that gutter with an equal negative margin. It is a neat
trick for a labelled field and wrong for anything else: the actions tab's
rows have no label, so the table started 300px in with the space beside it
left empty.
Add a noLabel class that cancels the indent, and put it on the four rows of
the tab that have no label -- the table, the help text above and below it,
and the no-capable-devices warning that replaces the lot. The mobile rules
already drop the indent for every row, so there is no counterpart to add
there.
This is also why the Schedule column felt cramped. The fixed widths in
6eb390007a divide whatever the table is given, and the table was being given
300px less than the page; those widths are unchanged and now divide the whole
of it. Only one table on this view sits in a ul.form, so nothing else was
affected.
Tests: tests/php/test_monitor_actions_columns.php grows to 19 checks. Which
class cancels the indent is read out of the css rather than named in the test,
since a drift between the two would bring the indent back without erroring.
It also checks that no row carrying the class has a label, because a label in
a row with no gutter to be pulled into would hang off the left edge -- the
same mistake in the other direction. Verified the checks fail when the
table's row loses the class (2), when the css renames it and the view does
not (2), and when the class is put on a labelled row (1).
Ran: tests/php/test_monitor_actions_columns.php (19 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
The Enabled column already asked for 5em, but under the default automatic
table layout a width on a cell is only a suggestion: the browser sizes each
column from its contents and may hand one more than it asked for. So Enabled
kept claiming space on the strength of its header text while holding a single
checkbox, and Schedule -- which has to fit "Every weekend day (Sat & Sun) at
08:00" across a label, a select and a time input -- was the column that got
squeezed.
Make the layout fixed, which binds the declared widths exactly. Enabled and
Sound are pinned to what a checkbox and a number need, Schedule is pinned to
what its sentence needs, and When, Device and Does divide what is left: those
three are selects, and a select that runs short truncates its option text
rather than breaking the row. zone.css already lays a table out this way, so
it is not a new approach in the skin.
A fixed layout will not shrink a control to fit its column, so the selects
and inputs are capped at the column width. Without that a long monitor name
in Device would spill over Does.
Tests: tests/php/test_monitor_actions_columns.php, 13 checks. The widths are
addressed positionally, as :nth-child(1), (3) and (6), because the cells carry
no classes -- which is fine until a column is inserted, when every pin after
it silently moves one column right and nothing errors. So the css is read for
which positions it pins, the view for what is in each position, and the two
are compared. Verified the checks fail when table-layout is removed (1
failure) and when a column is inserted before Schedule (6).
Not verified in a browser: the install under /usr/share/zoneminder/www is a
copy rather than a symlink of this tree, so the em values are a judgement
about what each column holds rather than something measured on screen.
Ran: tests/php/test_monitor_actions_columns.php (13 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
The scheduled tasks view carried an inline form above the list, so the page
was two things at once: the form was always on screen whether or not anyone
was editing, and the list it belonged to was pushed below the fold.
Make the view the list, and move adding and editing into a modal built by
web/ajax/modals/cronjob.php, following the pattern the Options modals use.
The Add button and the task name both open it; the modal posts back to
?view=cron, so includes/actions/cron.php still does the validating and the
crontab rebuild and did not have to change.
Two things follow from the form being built over ajax rather than with the
page. The modal emits getCSRFinputHTML() itself, since nothing else wraps it.
And cron.js binds the schedule pickers after the html is inserted instead of
on DOMContentLoaded: the controls do not exist until then, and a fresh set
arrives each time the modal opens.
While moving the When row, give the pickers a placeholder when they cannot
draw the stored schedule. Previously an existing task set to something like
"*/5 * * * *" rendered the pickers at their default, so the page showed
"Every day at 03:00" in large friendly controls beside a cron field reading
"*/5 * * * *" -- two different answers at once, with the wrong one the more
prominent. Adding a task still defaults to daily at 03:00, which is a default
rather than a contradiction.
Tests: tests/php/test_cron_modal.php, 42 checks. The field names are now a
contract between files that do not mention each other, so each side is parsed
out of its own source and compared rather than restated. The three states of
the When row are checked by rendering the modal against stubs, since which
picker is selected and whether the cron field is open are decisions the php
makes. Verified the new checks fail when the placeholder, a field name or the
csrf input is removed.
Ran: tests/php/test_cron_modal.php (42 passed), test_crontab.php (88 passed),
test_allowed_commands.php (31 passed), tests/js/cron-schedule.test.js (12
passed), the rest of tests/php and tests/js, npx eslint . (clean),
utils/check-homoglyphs.py (clean). test_bandwidth_clamp.php fails on this box
for an unrelated reason: it cannot read /etc/zm/conf.d/03custom.conf and the
permission warning lands in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two places in ZoneMinder run a command the operator typed: a filter with
"Execute command", and the scheduled tasks added for #4957. Both run it as the
web user through a shell, which is close to shell access on the server. Until
now the only control over that was who may edit, and the answer was anyone
with System rights.
ZM_ALLOWED_COMMANDS names the programs that may be run, comma separated.
Empty is the default and places no restriction, so an upgrade changes nothing
and the filters people already have keep working; setting it narrows both
features at once.
Only the program is matched, because an exact whole-command match could never
work for filters: zmfilter.pl appends the event path to whatever the filter
stored, so no two runs have the same command line. Arguments are therefore
free, which means the program being on the list has to be the whole of the
guarantee -- so while a list is in force a command may not contain the
characters that start another program: semicolon, pipe, ampersand, backtick
and dollar-paren. Without that, "/usr/local/bin/backup.sh; anything" would
pass on the strength of its first word. Redirection is deliberately allowed;
it starts no program, and refusing it would break the ordinary
"... >> /var/log/mine.log".
Refused in three places, because each answers a different question. The
filter and scheduled task editors refuse on save, so the operator is told at
the time rather than by a log line days later. zmfilter.pl refuses at the
moment it runs, because a row can reach the database without passing through
the web ui and because the setting may have been narrowed since. And
cronRebuild leaves a newly disallowed task out of the crontab, so tightening
the setting takes jobs away rather than waiting for somebody to edit each one.
The rule exists twice, in web/includes/Command.php and ZoneMinder::General,
since one is PHP and the other Perl. Rather than hope they stay in step,
tests/php/test_allowed_commands.php puts every case to both and fails if they
disagree. That earned itself immediately: both split the command on
whitespace before handling quotes, so a program path containing a space was
cut in half and compared as the wrong string. The cross-check found it in
both at once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This is the rest of what #4957 asked for -- run Perl, PHP, bash and binaries
on a flexible schedule, from a visual interface -- and it needs no daemon,
because cron is the daemon. 1.39.37 already taught ZoneMinder to rewrite a
marked block in the web user's crontab for scheduled monitor actions while
leaving every line outside that block alone. A CronJobs row is another line in
the same block.
So the parts that would otherwise have to be written and then supervised --
timing, catch-up, restart on boot, running as a known user -- are cron's, and
have been since 1975. What ZoneMinder adds is the table, the page, and the
rule that the crontab is a projection of the table: rebuilt whenever a row
changes, never the other way round, so the two cannot drift.
The schedule uses the same pickers the monitor actions got, reading "Every
Monday at 7pm", with the cron field beside them for anything the pickers
cannot say. The pickers build the spec in the browser so the field fills in as
you choose, and the server builds and validates it again on save, because the
browser is not what keeps a crontab line honest. The two mappings are checked
against each other in tests/js/cron-schedule.test.js, which reads the PHP map
rather than restating it.
The command is not judged on what it does; running things is the point. It is
judged on whether it stays on its own line and means in cron what the operator
meant: a newline is refused because it would end the entry and let the rest
become a line with a schedule of its own, and an unescaped % is refused
because cron stops the command there and feeds the remainder in as stdin,
which would silently run less than was typed. Cron's own escape, \%, is
accepted.
Editing takes System edit rights, and every add, change and delete is logged
with the command. Anyone who can edit here can run commands as the web user,
which is worth saying plainly -- though it is the same trust Filters'
AutoExecuteCmd has carried for years, not a new one.
ServerId NULL means every server writes the job into its own crontab, which
is what a single server install wants without having to say so; naming a
server restricts it to that one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five cron fields are a reasonable thing to store and an unreasonable thing to
put in front of an operator. The editor now reads as the sentence it means:
Every [day v] at [08:00]
Every [Monday v] at [19:00]
Every [weekday (Mon-Fri) v] at [07:30]
Cron stays the stored form, because it is what goes in the crontab and it can
say things the two controls cannot. cronFromParts turns what the editor
collected into a spec and cronToParts turns one back, so a saved schedule
loads into the same controls that made it.
cronToParts is deliberately partial. A spec the controls cannot draw -- every
fifteen minutes, the first of the month, hourly, a pair of weekdays -- comes
back null, and the editor shows it as it stands and saves it back untouched
through a read-only field. Rounding someone's schedule to the nearest thing
the pickers understand would be worse than admitting the editor cannot draw
it.
The time comes from <input type="time">, so it is shown in whatever format
the operator's locale uses; 8am and 08:00 are the same control. The schedule
is still built and validated server side, since the browser is not what keeps
a crontab line honest.
Tested for the round trip over every frequency and a spread of times: what
the editor can build must survive being saved and loaded back, or editing a
row would quietly change when it runs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
that already exist -- lights on, lights off, play a sound -- at set times.
MonitorActions already says what to do and to which device, with triggers for
EventStart, Alarm, Manual and so on. So this adds a trigger rather than a
subsystem: an action whose TriggerOn is 'Schedule' carries a five field cron
spec, and cron runs it.
Cron, rather than a daemon of our own, because the web user owns a crontab it
can rewrite without privilege, and because the line cron runs,
30 22 * * * /usr/bin/zmcontrol.pl --id 24 --command=lightOff
is the same invocation web/includes/Monitor.php already falls back to when no
zmcontrol daemon is listening. A scheduled action therefore takes the path a
manual one does rather than a parallel one of its own. It also means no new
code runs as root, nothing new has to be supervised, and the feature is a
table column plus a crontab block.
Nothing a form can set reaches a shell. The schedule is refused unless
cronScheduleIsValid() accepts it -- five fields, numbers and the usual
*/,-/ only, with newline, carriage return and % rejected outright because
each would end the line or the command. The command itself comes from
MonitorAction::commandFor()'s fixed list and the ids are integers, so the
whole line is assembled from constrained parts. That is a narrower surface
than Filters.AutoExecuteCmd, which has taken an arbitrary command for years.
The crontab is not ours to own, so ZoneMinder rewrites only a block between
two markers and leaves every other line byte for byte. A block whose end
marker is missing, which is what an interrupted write leaves, is treated as
no block at all rather than swallowing everything after it: losing our own
stale block is the safe failure, losing the admin's entries is not. The block
is rebuilt from the table on every save, so the crontab is a projection of
the database rather than a second place to keep the truth.
zmc recognises the new trigger and skips it, so having one does not warn on
every reload, and nothing fires a scheduled action from the capture process.
An action targeting a monitor on another server is left out and logged: cron
runs zmcontrol.pl locally, and the web ui's proxy through the API has no cron
equivalent. Single server installs are unaffected.
The two pieces that can lose data or run the wrong thing are pure, and
tests/php/test_crontab.php covers them: schedule validation including the
injection cases, and splicing a block in, out and over without disturbing the
lines around it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
monitors/alarm/command:on now takes an optional cause (query or POST data)
and passes it to zmu --cause, so the event records it in place of
"Forced Web". Without a cause nothing changes.
The cause is stored as the event's Cause and in its Notes, so alarmCause()
keeps it to plain text: letters, digits, spaces and plain punctuation, cut
to the 31 bytes zmu stores on a character boundary, falling back to the
default when nothing is left. The value is quoted with escapeshellarg()
for the zmu command line.
Test: tests/php/test_monitor_alarm_cause.php
fixes#5348
Follow-up hardening to the multi-server control forwarding added in
#5343:
- remoteControlUrl() now forwards an allowlist of known control
parameters (id, control, xge, yge, x, y, scale, preset, newLabel,
action, aid, response) instead of passing everything that is not on a
denylist, so no stray key or cookie-folded value reaches the other
server.
- forwardControlRequest() sets a 15s timeout so an unreachable recording
server can't hang an interactive control click for default_socket_timeout,
and, with ignore_errors, treats a non-2xx response (e.g. an auth-relay
login redirect's HTML) as a failure.
- relayControlReply() refuses to pass a non-JSON body back to the browser
under a JSON content-type.
Extends the test to assert unknown parameters are not forwarded.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
In a multi-server setup a PTZ move, preset or light command sent to one
server for a monitor that another server records never reached the
camera. Monitor::sendControlCommand() passed the zmcontrol option string
to the other server's monitors/daemonControl API, which only starts and
stops zmcontrol.pl, so the request built a URL like
daemonControl/4/ --panspeed=51 ... --command=moveConLeft/zmcontrol.pl.json
and answered "Failed". The web UI's own PTZ buttons send to the
monitor's server and were not affected; clients that send every request
to the server they log in to (zm-py, pyzmNg, zmNinjaNg, aiozoneminder)
and monitor actions whose target is on another server were.
ajax/control.php now hands such a request, as received, to the index.php
of the server that records the monitor (or the action's target), with
the auth relay in place of the caller's credentials, and answers with
that server's reply. That server checks the permissions and builds the
command itself. A request is forwarded at most once.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#5288 added an isDefaultPassword() call to validateUser(). The comparison
test from #5294 lifts validateUser() out of auth.php into its own
namespace without it, so since both merged the test stops at its first
check: "Call to undefined function ValidateUserTest\isDefaultPassword()".
Each PR passed on its own branch.
Lift isDefaultPassword() and the ZM_DEFAULT_PASSWORD* constants from
auth.php as well, rather than copying them, and check what #5288 added to
validateUser(): the default password is refused with its own message,
plain or MySQL-hashed, and a wrong password still reads as one. Test only;
no change to ZoneMinder.
refs #5287
Add ZM_PASSWORD_MIN_LENGTH (Options, System; default 8, 0 turns it off).
passwordProblem() refuses a shorter password, so the user page, the API
and both login-page forms apply it alike. Existing passwords keep
working; only a password being set is checked. The length counts
characters, not bytes.
There is no setting for character classes: they push people to
predictable variations (Password1!) and add less than length does.
A database whose Config table doesn't have the option yet (an upgrade
not run with zmupdate.pl -f) gets no minimum. The password inputs carry
minlength so the browser says so before the form is sent; the server
check is the one that counts. The API answers with the English text, as
for the other reasons.
Tests: tests/php/test_default_password.php.
The login page's change form and the first-run page refused admin as a
new password, but the user page (Options, Users, and a user's own page
with ZM_USER_SELF_EDIT) and the API (users/add, users/edit) still stored
it. Every login then refused that account.
Check a new password in one place, passwordProblem() in auth.php, and
call it from all four:
- includes/actions/user.php, for an admin's edit and a self edit: the
save stops with the reason shown on the page. A Password sent as an
array is refused too (it reached password_hash() before).
- api/app/Model/User.php: an "acceptable" validation rule, so add and
edit answer with the reason as the validation message. The API loads
no language file, so passwordProblemText() gives it the English.
- newPasswordProblem() (both login-page forms) checks the confirmation,
then passwordProblem().
Tests: tests/php/test_default_password.php covers passwordProblem() and
passwordProblemText().
A new install starts with authentication on and the admin password admin,
which is refused everywhere. Asking for that password just to replace it is
an extra step, and some installs don't want a login at all. Ask instead.
While the first run is pending (authentication on, the privacy page not
answered yet, builtin auth, and admin still has the password admin), the
login page shows "Set up ZoneMinder" instead of the login form:
- Require a login: choose the admin password; the account is then logged
in and the privacy page follows, as before.
- Don't require a login: ZM_OPT_USE_AUTH is set to 0 in the database.
Not offered when ZM_OPT_USE_AUTH is set in zm.conf or conf.d: the owner
of the box has decided then, and only the password is asked for.
Until one is chosen the web interface, API and streams can't be used: the
password admin is still refused. Whoever opens the page first makes the
choice; before this, whoever typed admin/admin did. The form carries a
nonce kept in the session, like the default-password form, and is exempt
from csrf-magic as action=login is (the login regenerates the session id).
Both choices are audited. Upgrades are not affected: ZM_SHOW_PRIVACY is 0
once the privacy page was answered, and zmupdate.pl doesn't reset it.
Docs: a First Run section in the getting-started guide, and the FAQ and
system options updated to match.
Tests: tests/php/test_first_run.php.
db/zm_create.sql seeds the admin account with the password the user
guide documents (admin), so with authentication on a new install is
still open to anyone who reads the guide.
Refuse that password as a credential and have the web login replace it:
- validateUser() returns false with "This account uses the password
admin, which ZoneMinder does not accept. Log in to the web interface
to set a new one." when the password is right but is admin. The API (login.json and
user=/pass= on any request) answers 401 with that message; the
user=/pass= web parameters are refused the same way.
- On the login page (action=login) the same login shows a form that
asks for the current password again and a new one, with a nonce kept
in the session so another site can't submit it for the user. The new
password must be set, confirmed and not the default. The account is
logged in only once it has been replaced; the change is audited.
That POST skips reCAPTCHA when it carries the form's nonce: the form
follows a login that passed it, and doesn't render the widget.
With ZM_AUTH_RELAY=plain the session keeps the new password.
- zmLoadUser() refuses the default password, so zms, zmu and the RTSP
server don't accept it in user/password parameters either.
The check is on the password typed, so it needs no schema change, covers
existing installs whose admin still has the default password, and
follows the account if it is renamed. A password set to the default
again later is caught the same way. Sessions, tokens and auth hashes
issued before the upgrade keep working until they expire; with
ZM_AUTH_TYPE=remote the web server's login is not affected.
The FAQ now says to force authentication on in conf.d: since 4d7609d36
the daemons apply zm.conf and conf.d values over the database too.
Tests: tests/php/test_default_password.php (the helpers, and that the
seeded hash is the documented default), tests/zm_user_default_password.cpp.
A boolean option set in zm.conf or conf.d wins over the Config table, and a
person editing a file writes yes or off rather than 1 or 0. The C++ daemons
now read those words (ConfigFileValue, 4d7609d36), but PHP and Perl took the
value as written, and a non-empty string is true in both. So
ZM_STRICT_VIDEO_CONFIG=off in conf.d left the option on in the web UI and the
Perl scripts while zmc turned it off.
Read a file value for a boolean option the same way in all three: yes, true
and on are 1; no, false and off are 0; anything else stays as written. Only
booleans are read this way, so a string option such as a title keeps "Yes".
PHP defines the file values as constants before it reads the Config table,
since it needs the database settings to connect, and only the table says
which options are booleans. So a word now waits until the table has been
read, apart from ZM_DB_* settings, whose words database.php reads itself.
Perl reads the type with each row.
Tested on 1.38's zm138 database with a conf.d file setting
ZM_OPT_FAST_DELETE=yes (0 in the table), ZM_STRICT_VIDEO_CONFIG=off (1)
and ZM_WEB_TITLE=Yes. The installed PHP and Perl gave 'yes', 'off' and
'Yes'; these give '1', '0' and 'Yes'. tests/php/test_config_file_booleans.php
checks the rule.
refs #5289
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
validateUser() compared a plain-text Password column and a MySQL
PASSWORD() hash with ==. Two strings that look like numbers compare as
numbers under ==, so a plain-text password stored as "100" also accepted
"1e2", "100.0" and " 100". Use hash_equals(), which compares the bytes in
constant time; a password that isn't a string is refused.
ZoneMinder stores new passwords as bcrypt, so only accounts whose
Password column was set by hand or carried over from old installs use
these branches. bcrypt still goes through password_verify().
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With ZM_OPT_USE_GOOG_RECAPTCHA on, a password login was refused only when
siteverify answered success == false together with an error-codes list.
An answer without a success field, one that wasn't JSON, success:false
without error-codes, or success:"false" all let the login go ahead as if
the challenge had been solved.
Move the decision into recaptchaAllowsLogin() and allow only
success === true. The existing allowance for a wrong secret key
(invalid-input-secret) is kept, so a mistyped key still logs an Error
instead of locking everyone out.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
get_auth_relay() builds the query string the web UI appends to stream,
AJAX and multi-server URLs. It encoded the plain relay's password but put
the username in as typed, under all three relays. A username containing
& or = split into extra parameters, and zms ends each value at the next =.
Under plain and hashed that user's streams were refused; under none they
ran as whichever user the rest of the name spelled out.
Encode the username as well. zms decodes user=/username= (UriDecode) and
PHP decodes $_REQUEST, so names without reserved characters are unchanged
on the wire and names with them now arrive whole.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getAuthUser() compared the auth= hash a client presents with the one it
builds using ==. When both strings look like numbers PHP compares them as
numbers, so a computed md5 of the form 0e<digits> equalled any other
0e<digits> string (and "00"), and == also returns at the first byte that
differs. hash_equals() compares the bytes in constant time.
A hash of that form comes up about once in 3*10^8 user-hours, so this is
hardening; ordinary hashes compare exactly as before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dbQuery()'s two error paths for a prepare() or query() that returns false
logged $pdo->errorInfo. $pdo is not defined there, so PHP warned twice and
the message ended without the error. They also read dbLastError() from the
connection only after ZM\Error() had logged; with a database log target that
inserts into Logs through the same connection, which replaces its errorInfo,
so dbLastError() came back as "00000" instead of the error.
Read the error once, before logging, and use it for both. The query() path
also said "Error preparing" for a statement it ran directly.
These paths run when PDO's error mode doesn't throw; dbConnect() sets
ERRMODE_EXCEPTION, so normally the catch below handles the failure.
tests/php/test_database_error_paths.php drives both paths with
ERRMODE_SILENT and checks for no PHP warning and the real error text.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dbConnect() turns emulated prepares off, so every query of the web UI, the
ajax calls and the includes the API runs (config, servers, users,
permissions) is a server-side prepare: prepare, execute and close, three
round trips to the database where one would do. From PHP 8.1, pdo_mysql
returns integers and floats as PHP ints and floats with emulated prepares
too, so values keep the types they have now. Emulate prepares there and keep
server-side prepares on older PHP, where every value would become a string.
Emulated prepares quote parameters bound as strings, which breaks LIMIT ?.
dbQuery() now binds integer values as integers (everything else as strings,
as execute() did), and the log and watch ajax calls cast their validated
offset and limit to int. With emulation, PDO would also run several
statements given in one string; MYSQL_ATTR_MULTI_STATEMENTS is turned off so
that is refused as before. CakePHP's own connection is not changed: it
already uses PDO's default, emulated prepares.
tests/php/test_database_emulated_prepares.php compares every column of every
table read through both kinds of prepare (types and values), checks
aggregates, LIMIT with positional and named integers, string and null
parameters, and that two statements in one string are refused.
UsersController::view() and edit() assigned their permission check with
`$canX = A or B;`. `=` binds tighter than `or`, so only A was stored and the
self check was dropped: with ZM_USER_SELF_EDIT on, a user without System=Edit
got 401 on PUT users/<own id>.json. beforeFilter() also refused every user
with System=None before view() or edit() ran.
Let beforeFilter() through view and edit of the user's own record while
ZM_USER_SELF_EDIT is on, use || in both checks, and limit what such a user
saves to the fields the web UI allows for self edit
(includes/actions/user.php): Password, Language and HomeView. The request
data is rebuilt from those three fields, so Id, System, permissions and the
rest are never saved; a Language without a lang file is refused as in the UI.
Users with System=Edit save as before.
Self view follows the web UI too: a user without System permission sees their
own record only while ZM_USER_SELF_EDIT is on.
fixes#5242
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ZM_Object's constructor treats an array as the object's row, and validInt()
and validCardinal() returned an array as they got it (preg_replace() does).
So wherever a request id reached `new ZM\X(...)`, raw or through those
validators, a crafted array built an object whose Id, UserId or MonitorId
the sender chose. The permission check then looked at those, and the delete
or update that followed used the Id. A user with Events=View deleted another
user's filter by posting Id[Id]=<filter>&Id[UserId]=<self> to the filter
delete action; the same shape reached events (MonitorId), users, reports,
snapshots and storage.
validInt() and validCardinal() now give '' for anything that isn't a single
value. The places that built an object from a raw $_REQUEST value (the
control and event ajax handlers, the event, filter, report, snapshot and
storage actions, view_hls, and the download and event detail modals) take
the id through validCardinal().
tests/php/test_request_ids_scalar.php covers the validators with arrays and
fails if web code builds an object straight from $_REQUEST, $_GET or $_POST.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After saving a zone, actions/zone.php redirected to the REFERER the zone form
posted, which the form fills from the Referer its page was loaded with. A
zone editor who reached the zone page through a link on another site was sent
to that site after saving: an open redirect, usable to lead someone from a
trusted ZoneMinder page to a look-alike login. Forging the POST itself is
blocked by the CSRF token, so it needs an editor who followed such a link.
localRedirectUrl() returns the target only if it is a query-only URL or an
absolute http(s) URL on the host and port of the current request, and the
fallback otherwise: another host, //host, backslashes, user@host, other
schemes and control characters all give the fallback. The zone save falls
back to the zones list, as it already does with no Referer (#5207).
tests/php/test_local_redirect_url.php covers 21 cases.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
requestField() read Model[field] whenever the request's Model entry was an
array, empty or not. CakePHP's Model::set() treats the bare top-level fields
as the model's data when that entry is empty (_setAliasData), so
{"Monitor":[],"EventEndCommand":"x"} passed the System-edit check on the
monitor command fields, which found no value, while save() stored x. A
Monitors=Edit user without System=Edit could set EventStartCommand or
EventEndCommand, which zmc runs through /bin/sh.
Every other check built on requestField() had the same gap: the target
MonitorId when adding or editing an event, the EventId when adding a frame,
and the ParentId when editing a group.
Read Model[field] only when the Model entry is non-empty, as set() does, and
otherwise the bare field; a non-empty entry that is not an array yields
nothing, since set() skips it.
tests/php/test_api_request_field.php runs the real method against each
request shape.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FOSCAMR2C presetGoto and presetSet interpolated the preset into
"SELECT Label FROM ControlPresets WHERE Preset = $preset" and sent the selected
Label to the camera as name=. For presetGoto the preset came straight from the
web request, so any user who could see the monitor could run a SELECT against
the ZoneMinder database and have the value (the auth hash secret, password
hashes) delivered to the camera link, or use it as a boolean oracle.
- FOSCAMR2C: bind the preset in every statement, refuse a preset that is not a
number, and look ControlPresets up by MonitorId as Dahua and PSIA do.
- buildControlCommand(): refuse a non-numeric presetGoto preset, and a control
name that is not a plain method name. zmcontrol calls the name as a method and
the options are split back out of the command string on spaces.
- zmcontrol.pl: refuse a command that is not a plain method name, so a qualified
Package::sub name can't call a sub outside the control module.
- canView()/canEdit() with a monitor id returned only the monitor check and
ignored the area, so canView('Control', $mid) passed for anyone who could see
the monitor. Require the area permission as well; Monitors stays decided by
visibleMonitor()/editableMonitor().
tests/php/test_control_command.php covers buildControlCommand() and the
monitor-scoped canView()/canEdit(); tests/perl/test_foscamr2c_preset_sql.pl
checks the statements FOSCAMR2C prepares and what it sends to the camera.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Filter::canView(), canEdit() and canDelete() ask the user object
$u->canEdit('System'), $u->canView('Events') and so on, but ZM\User had no such
methods. The calls fell through to ZM_Object::__call(), which stored the argument
as a property and returned it, so every check returned a truthy area name. Any
account with Events=View passed as a System editor and could save and run a
filter with AutoExecute, an OS command run by zmfilter.pl. The same fall-through
made the filter ownership and filterdebug checks pass for everyone.
Add canView(), canEdit() and canCreate() to ZM\User. They take the level from the
user's own field and from their role, the way the global functions in auth.php do
for the logged-in user, and refuse anything that is not a permission area.
Monitor::commandFieldChangesNotAllowed() now uses canEdit('System') so a role's
System=Edit counts there too.
In the filter action, require edit on a stored filter before applying the
submitted fields when saving over it, since the fields can name a new owner;
stop after a refused save instead of starting the filter in the background; and
require edit on the filter for start/stop/restart, which had no check.
tests/php/test_user_permissions.php drives the real ZM\User, ZM\User_Role and
ZM\Filter classes. The older filter tests stubbed the user with its own
canView/canEdit, which is why they passed while the real class had neither.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
view_video.php took the end of the range straight from the request and never
checked it against the file:
if (!empty($matches[2])) $end = intval($matches[2]);
$length = $end - $begin + 1;
so on a 1000 byte file:
bytes=0-99999 206, Content-Length 100000, body 1000 bytes
bytes=5000-6000 206, Content-Range naming bytes that do not exist
bytes=500-100 206, Content-Length -399
bytes=1000- 206, Content-Length 0
bytes=-100 200 with the whole file, not the last 100 bytes
A client that is told to expect 100000 bytes and gets 1000 does not see a bad
request, it sees a truncated file, and reports the video as broken. The suffix
form was not recognised at all because the pattern required a digit before the
dash.
This is reached once per fragment by the byte-range HLS manifest VideoStore
writes, every fragment being a Range against the one mp4, so a player that
asks for anything the file cannot supply gets a body that does not match its
own Content-Length rather than an answer it can act on.
Parse the header properly: clamp a range that runs past the end, because a
client may ask for more than is there and is entitled to what is there;
answer 416 with "Content-Range: bytes */size" when the range cannot be
satisfied at all, so the client learns the real length; and read "-N" as the
last N bytes. Length is now derived from the range being served rather than
the one requested, and the send loop counts down by the bytes it actually
read, so Content-Length and the body cannot disagree.
Only the first range of a multi-range request is served, as before. A
multipart/byteranges body is not worth building for this, and falling back to
sending the whole representation is not an option when these are event videos
of hundreds of megabytes; Content-Range names exactly what was sent.
The parsing is its own dependency-free include so it can be tested without a
database, and tests/php/test_http_range.php covers each case above plus a
sweep asserting that every range it ever returns lies inside the file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ajax/frames.php collected Frames.Id values from the event's rows and then
filtered with a FrameId IN (...) term, which FilterTerm translated to
Id IN (...). The search query had no EventId condition of its own. Once
the term matches on FrameId, which is only unique within an event, it
returns rows from every event, so the query is now anchored on the event
with the same WHERE EventId clause as the unfiltered list.
FilterTerm now maps FrameId to the FrameId column. The frames list
thumbnail links address the image by eid and fid instead of Frames.Id.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With ZM_AUTH_HASH_IPS on, the client address bound into the auth hash was
taken from the left-most X-Forwarded-For value whenever the header was
present, both when PHP generated the hash (getRemoteAddr()) and when PHP or
zms validated it (getAuthUser(), zmLoadAuthUser()). The header is client
controlled, so anyone holding a leaked hash could replay it from anywhere by
sending the address it was bound to.
Add ZM_AUTH_TRUSTED_PROXIES, a list of exact reverse proxy addresses.
X-Forwarded-For is now used only when REMOTE_ADDR is one of them, and is read
from the right, skipping hops that are themselves listed proxies, so values a
client prepends are never chosen. With the option empty, the default, the
header is ignored and REMOTE_ADDR is used.
PHP (web/includes/Network.php getRemoteAddr()) and C++ (ClientAddress() in
zm_utils, used by zmLoadAuthUser()) implement the same rule so generation and
validation continue to agree. Every PHP caller already routes through
getRemoteAddr(), so session.php and auth.php need no change.
Reverse proxy users who enable ZM_AUTH_HASH_IPS must list their proxy in the
new option; until they do, hashes bind to the proxy address, which still
validates but no longer distinguishes clients. This is the behaviour change
that the #4921 work avoided by trusting the header.
The option is added through ConfigData only, like other recent options;
zmupdate.pl --freshen inserts it, zms falls back to the compiled-in default and
PHP treats an undefined constant as empty, so no schema migration is needed.
Tests: ClientAddress Catch2 case; tests/php/test_remote_addr.php updated for
the trusted-proxy rule.
refs GHSA-72rf-54rm-798c
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit acea5889dec0826f595cb736147a5fdc9c94a2a9)
Saving a user from the web ui took the whole auth path down:
PHP Fatal error: Uncaught TypeError: strcasecmp(): Argument #1 ($string1)
must be of type string, array given in includes/auth.php:197
#0 auth.php(197): strcasecmp()
#1 auth.php(528): getAuthUser()
#2 auth.php(687): userFromSession()
reached from ?view=user&uid=2. That page's form posts user[Username],
user[Password], user[Name] and the rest, so $_REQUEST['user'] is an array on
every save from it, and getAuthUser() read that parameter as the username to
filter on and handed it to strcasecmp(). Under PHP 8 a string function given an
array is a TypeError rather than a warning, so the request died with a 500.
The same shape arrives from anyone who cares to send it, and not only on a page
that needs a session. userFromSession() reads user, pass, username, password
and auth straight out of the request, and the credential branches run before
anyone is logged in, so ?username[]=x&password[]=y reaches validateUser() with
arrays on an install that has never seen the caller before.
requestString() returns a parameter only when it is a string and null
otherwise, which is what the callers already do with a parameter that was not
sent. An array is not a username, a password or an auth hash.
master no longer has the strcasecmp line the report names, so it does not fatal
in that exact spot, but it reads the same unvalidated values: $filterUser is
bound as a query parameter and the credentials still reach validateUser(). This
fixes the class rather than the one line, and backports to 1.38 where the
reported line lives.
The test lifts requestString() out of auth.php and evaluates it alone, because
including auth.php needs a database; test_auth_no_include_side_effects.php
sidesteps the same dependency the same way. 8 cases, covering the form's array,
the login parameters, a nested array and the strings that must still pass
through. 5 of them fail without this change.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 1f4aa1a16c1b63fdf34a6f2c6aef6590dbd45acd)
The merged mp4 export is named '<Monitor> <start> to <end>.mp4', so it contains
spaces and colons, and download.php emitted it as a bare unquoted filename=
parameter. That is not a valid RFC 6266 token, so browsers that parse
Content-Disposition strictly find no filename and fall back to naming the
download after the last path segment of the URL - index.php. Firefox is lenient
and accepted it, which is why the report was Chrome-on-Windows only.
Add contentDispositionAttachment(), which emits a quoted ASCII filename with the
Windows-illegal characters folded to '_', plus the untouched name as RFC 5987
filename* whenever that folding changed anything, so unicode monitor names still
arrive intact.
Also in that path:
- urlencode the file and export_root query parameters; a monitor name containing
'&' or '+' would otherwise split or mis-decode the download URL. Read them back
in export.js with URLSearchParams so the link text shows the decoded name.
- drop the stray ';' from Content-Length, which made the value unparseable.
- silence the shutdown unlink()s, whose warnings would be appended to the body
of a download that had already started.
- log $this->filenamePath, not an undefined local, on the unreadable-file path.
Tests: tests/php/test_download_content_disposition.php, 12 assertions, all pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nr76CednxtDt2nPuq6WrbL
The three profiles' settings were aliased by a switch with one arm per
profile, each repeating the same 18 define() calls against a different
ZM_WEB_<H|M|L>_ prefix. Sixty three lines in which only a single letter
differed, and nothing held the arms in step: a setting added to one and not
the others is undefined for two thirds of users, which is the same blank page
the missing default case caused, just narrower.
Name the settings once and build both sides from the prefix. The two settings
that carried a defined() guard keep it, as a separate list with the fallback
each one uses, so a genuinely absent setting is still distinguishable from a
mistyped one - the rest go through constant() and fail loudly.
Looking up an unknown profile now yields the low prefix instead of skipping
every define, so the skin config no longer depends on skin.php having clamped
the cookie first; that clamp remains the place a bad value is corrected.
Verified by diffing every resulting ZM_WEB_ constant against the previous
implementation for each of the three profiles: 49 constants, identical values.
The test drops the checks that only made sense against the switch and gains
ones for the new shape. It loads the skin config in a child process, once per
profile probed, because constants cannot be redefined and loading it is itself
what can fail. It now catches a mistyped setting name, a removed fallback, a
profile the whitelist does not know, and a setting dropped from the alias list
- the last by way of the per-profile config options, which are the authority
on which settings exist and are independent of the lists under test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
web/skins/classic/includes/config.php defines all 18 ZM_WEB_* constants inside
a switch on $_COOKIE['zmBandwidth'] with cases for high, medium and low and no
default. On any other value none of them are defined, and skin.js.php - emitted
in the footer of every page - reads ZM_WEB_VIEWING_TIMEOUT, ZM_WEB_AJAX_TIMEOUT
and ZM_WEB_REFRESH_NAVBAR. On PHP 8 an undefined constant is a fatal Error, so
every page including login stops rendering until the cookie is cleared, which
cannot be done from inside the interface.
skin.php only tested the value for empty, and nothing else validated it:
- the cookie is set client side by skin.js, so any value survives
- action=bandwidth put $_REQUEST['newBandwidth'] through validStr, which is
only strip_tags, and persisted it
- ZM_BANDWIDTH_DEFAULT is a free-form string in ConfigData. The Options UI
renders it as a select, but loadConfig lets a conf.d file override the
database, so a typo there locks out everyone with no cookie yet
skin.php now validates both the cookie and ZM_BANDWIDTH_DEFAULT before falling
back to low, and the action rejects a value it does not recognise rather than
storing it.
tests/php/test_bandwidth_clamp.php checks the whitelist against the switch it
guards - the two must name the same profiles, since a value in one and not the
other reopens this - and that no arm of the switch defines a constant the
others do not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
database.php called dbConnect() at file scope, so including it opened a socket,
and on failure rendered views/no_database_connection.php and exit()ed from
inside a library include. Every model in web/includes requires this file, so
merely loading a class did both.
Connect on first use instead. $dbConn becomes tri-state - false for "not
attempted", null for "attempt failed", a PDO for connected - and two accessors
sit on top of it:
zmDbConn() opens if needed; on failure renders the error view and
stops, which is what the include used to do, just at the
point a query is actually attempted.
zmDbConnOrNull() opens if needed but returns null instead of ending the
request, for callers with a fallback.
dbQuery() is the funnel every fetch helper goes through, so routing it plus
dbEscape(), dbError() and dbInsertId() through the accessors covers the library.
The five callers that reached for the raw global are updated: config.php.in,
Event.php and ajax/console.php need a connection and take zmDbConn(); logger.php
takes zmDbConnOrNull() and falls through to its error_log target, so a logging
call can no longer end the request or open a connection by itself.
ZMSessionHandler captured $dbConn in its constructor. It is constructed while
session.php is being included, before anything has needed the database, so with
a lazy connection that captured false. It now resolves per call and its methods
return "no session" rather than dereferencing a bool.
Two smaller fixes fall out. The error view was included by a relative path that
only resolved when the cwd was web/, so it never worked for requests served out
of web/api/; it is now anchored with __DIR__. And dbDisconnect() set $dbConn to
null, which in the new tri-state means "connecting failed" and would send the
next query to the error page; it sets false so a later query can reconnect.
Nothing calls dbDisconnect() today.
This does NOT make database.php includable without a database. It requires
logger.php, which requires config.php, which reads ZoneMinder's configuration
out of the Config table at include time. Until that cycle is broken the
connection still happens during bootstrap, just from config.php rather than from
here.
Tests: tests/php/test_database_lazy_connect.php, 7 assertions, all pass. It
tokenises database.php and asserts nothing runs at include time, that dbQuery()
goes through the accessor, and that only the connection plumbing touches the
global. Verified it reports the pre-refactor file's `if ( !dbConnect() )` - an
earlier version of the check skipped tokens inside parentheses and so passed on
exactly the code it exists to reject.
Not covered by tests: behaviour when the database is genuinely unreachable, and
the session handler against a live database. Needs manual testing on an
installed tree, including stopping mysql to confirm the error view still renders
for both a web request and an API request.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01477mR97vfnK6zczbHgzq6T
