actions/eventdetail.php checked only the global Events=Edit permission and
then updated the Name, Cause and Notes of every id in markEids, so a user
denied some monitors could rewrite those monitors' events. The event detail
modal likewise showed the name, cause and notes of any event id it was given.
Load each event and require $event->canEdit(), which adds the per-monitor
view check, as archiving and unarchiving in actions/events.php already do.
The action skips events that fail it; the modal leaves them out, and shows
nothing for a single event that fails.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
actions/control.php and actions/eventdetail.php redirected to
$_SERVER['HTTP_REFERER'] as is. That is the Referer of the POST itself, which
normally comes from a ZoneMinder page, and the CSRF token keeps other sites
from making the POST, so it isn't the open redirect the zone save had. But it
is the same pattern, and a missing Referer warned about an undefined index and
rendered the action's view instead of redirecting.
Use localRedirectUrl() as the zone save does, falling back to the monitor's
control view and to the events list.
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>
The zone form posts its Referer back as REFERER, and posts it empty when
the form was opened from a typed URL or a bookmark. The action used it
whenever it was set, so $redirect was empty, index.php rendered the zone
view with the action's $zone (a database row, or array() for a new zone,
neither with Points) and ksort() at views/zone.php:105 failed with
'Argument #1 must be of type array, null given'. The zone was saved but
the browser got a 500.
Use REFERER only when it is not empty and fall back to the monitor's
zones list.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Controls.Type looks like the monitor type a control applies to, and has
been kept in step with the Monitors enum since 1.27, but nothing has
ever read it. The control dropdown on the monitor page lists every row
by Name, no query filters on it, and no code compares it to the
monitor's own Type. It was a field the edit form asked for and the
control list displayed back.
The column appears to be MonitorPresets.Type copied by analogy, where
the idea is real: that one decides what a preset creates. That also
explains why every migration altering one altered the other.
Removes the column from the schema, adds the 1.39.39 migration, the edit form row
and its type list, the control list column, the defaults in Control.php
and Control.pm, and the value from all 53 shipped rows in controls.sql.
This supersedes the 1.39.38 retyping of three libvlc controls to Ffmpeg:
the column they were being retyped into is gone.
zmcamtool needs no change -- its import regex rewrites the Id and
captures the Name, so it is not positional past that, and its export is
a mysqldump that follows the live schema.
Dropping Type left the two shipped Pelco rows byte-identical to their
twins, which existed only as a Local and an Ffmpeg copy of the same
control, so the redundant copies are removed: 53 shipped rows become
51, with both Pelco-D and Pelco-P still present once.
Existing installs are deliberately left with their duplicates. The same
pair collapses there too, but deleting rows a monitor's ControlId may
point at, to tidy a repeated name in a dropdown, is not a trade a
migration should make silently.
Verified by loading the old and rewritten controls.sql into scratch
databases and diffing every remaining column across all 53 rows: no
value shifted, which is the failure mode when a positional INSERT loses
a field. The file mixes positional and explicit-column INSERTs, so both
forms were handled. Fresh-install and migrated Controls schemas compare
equal, the shipped data loads into both, and Monitors and MonitorPresets
keep their Type. Build clean, 147 test cases pass, php -l and perl -c
clean on the edited files.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
Follows the daemons: no monitor can be type Libvlc any more, so the ui
should not offer it or carry text describing it.
Drops the source-type option and its deprecation notice, the Libvlc arm
of the Source Path and console source-column branches, the entry in the
control-capability type list, and the value from the api's Type
validation list. The OPTIONS_LIBVLC help block goes from all 25 language
files: monitor.php builds that key as OPTIONS_ plus the monitor type, so
it became unreachable the moment the type did. The Libvlc source-type
label goes from the 22 files that carried it.
SourceTypeDeprecated and Deprecated lose their last caller and are
removed. RemoteRtspDeprecated and Removed stay -- the rtsp protocol
option is still shown to monitors already set to it.
The WEB_FILTER_SOURCE help listed the affected source types; corrected
in ConfigData.pm.in and in the one translation that had localised it.
All 25 language files and the four edited php files parse clean under
php -l, and the config module parses under perl -c. No javascript
changed, so eslint has nothing to say here -- it cannot run in this
worktree anyway, which has no node_modules.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
RTSPDescribe chose whether to take the media url from the DESCRIBE
response rather than the configured path. That was a quirk of
ZoneMinder's own rtsp implementation; ffmpeg decides it itself. Removing
that client left the setting with nothing behind it -- the monitor form
still drew the checkbox and the save path still wrote the column, but no
code read it, so the only thing it could do was mislead whoever ticked
it.
Drops the form row and the Remote-only block that existed to hold it,
the javascript that showed and hid it as the protocol changed, the field
in the php and perl monitor models, the default in the save action, the
column from the schema, and the label and help text from all the
language files carrying them.
The 1.39.37 migration drops the column behind an information_schema
guard scoped to DATABASE(), so the file stays re-runnable.
Verified by building a pre-upgrade database from upstream/master's own
schema and applying the migration: the column goes, and Monitors matches a fresh install of this
branch column for column. Migration applied three times with no error.
php -l clean on the four edited files and all 28 language files, perl -c
clean, build clean, 154 test cases pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
With the rtsp client gone, Type=Remote with Protocol=rtsp cannot capture, so
the monitor form no longer offers it and the camera probes no longer suggest
it.
A monitor already saved as rtsp still sees the option, because dropping it
outright would mean opening such a monitor to read the warning silently
re-saved it as http -- swapping one broken configuration for another. For those
the option is labelled removed and the warning now says the monitor will not
capture until it is changed, rather than that it will stop working in a future
release.
The three probes that suggested Remote/rtsp -- ACTI in both probe
implementations, and Vivotek -- now suggest Type Ffmpeg with the equivalent
rtsp:// Source Path, which is the same stream by a maintained route.
Type=Remote keeps http. mjpeg and snapshot-jpeg cameras are what it is for.
php -l clean on all four files; eslint clean; the three pre-existing duplicate
lang keys are unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
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>
zmc runs a monitor's EventStartCommand and EventEndCommand at the start
and end of each event. Both the classic save and bulk edit actions and
the API monitor add and edit authorized only on the monitor, so a user
with edit on one monitor and no System permission could set a command
for zmc to run as the ZoneMinder service user.
Changing either field now needs System=Edit, as Filter::canEdit()
already requires for AutoExecuteCmd. Monitor::commandFieldChangesNotAllowed()
reports the command fields a change would alter; submitting the current
value is not a change, so other users can still save the rest of the
monitor. The monitor form shows the fields read-only to them. Also
close the EventEndCommand list item.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Since b2861d5cf loadConfig() returns the Config table merged with the
zm.conf and conf.d values that have no Config row. Those carry no
Private flag, and skin.js.php writes every config entry not marked
Private into each page as a JavaScript const, for any logged-in user
whatever their permissions. ZM_DB_USER and ZM_DB_PASS were in every
page, the permission error page included. The API's Configs listing
redacts by the same flag.
Mark values that exist only in the config files Private, apart from
ZM_DIR_SOUNDS, the one the browser uses (for the alarm sound).
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>
8c35190b0 moved the Servers lookup into a foreach over $s but kept
define('ZM_SERVER_ID', $Server->Id()), so every request on a node with
ZM_SERVER_NAME in zm.conf died with 'Call to a member function Id() on
null'. Use $s like the ZM_SERVER_HOST branch.
Also make the 'no corresponding entry in Servers table' errors reachable
($thisServer starts as an empty Server object, which is always truthy,
so test its Id) and concatenate the ZM_SERVER_ID error with '.' instead
of '+', which throws a TypeError on PHP 8.
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
* fix: give each migrated password its own random bcrypt salt
migratePasswords() stored the Bytes::Random::Secure object rather than
bytes from it, so en_base64() encoded the string
"Bytes::Random::Secure=HASH(0x...)". bcrypt only reads the first 22
characters of the salt, all from the constant class name, so every user
on every install got the same salt. With neither Bytes::Random::Secure
nor Data::Entropy installed the salt was empty and bcrypt() died with
"bad bcrypt settings", aborting zmupdate.pl. Even the Data::Entropy path
reused a single salt for every user in the run.
Read 16 bytes per user from /dev/urandom, as generateAuthHashSecret()
in ZoneMinder::Config already does. If it can't be read, leave that
user's legacy hash in place, which auth.php still verifies, instead of
writing a weak one. This drops the need for Bytes::Random::Secure and
the deprecated Data::Entropy. refs #4333
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* chore: drop the Data::Entropy dependency from packaging
zmupdate.pl no longer uses Data::Entropy, which upstream has deprecated
(CVE-2025-1860). refs #4333
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: rehash legacy passwords on every password login
Only the login form called migrateHash(), so accounts that sign in with
user=/pass= or through the API kept their mysql or zmupdate.pl
(mysql+bcrypt) hash, including those with the fixed salt. Call it from
those paths too, and have it check the password type itself.
migrateHash() also regenerated the auth hash from the in-memory user,
which still held the old password, so getAuthUser(), which checks
against the new one in the database, rejected it. Update the in-memory
password first, and update the row by Id rather than by the username
as typed.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
* fix: note passwords still on a zmupdate.pl-migrated hash
Every salt migratePasswords() used before this branch was shared by all
users in a run: from Data::Entropy, which before 0.008 keys its generator
from rand() (CVE-2025-1860), or, since 38c0f743 with Bytes::Random::Secure
installed, a constant. The original hash input is gone, so zmupdate.pl
can't rehash these, and nothing distinguishes them from properly salted
ones. Log a warning listing every user still on a -ZM- hash, noting that
it is upgraded at their next login or password reset, and that unused
accounts can be disabled or deleted.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Every query on Frames filters on EventId and orders or ranges by FrameId,
and nothing needs a frame by Id. The Id primary key was a clustered index
nothing read, and EventId_FrameId_idx was a secondary index every query
used. On a copy of a local table that secondary index was half the
table's size. With (EventId, FrameId) as the primary key the index is
gone, and an event's rows are stored together, so deleting an event is a
range delete.
zm_update-1.39.36.sql:
- converts AI_Detections.FrameId from Frames.Id to the per-event frame
number, drops its foreign key to Frames and indexes (EventId, FrameId).
A composite foreign key cannot replace it: ON DELETE SET NULL would
have to null the NOT NULL EventId, and Frames rows are written in
batches, so a detection can be recorded before its frame row.
- removes duplicate (EventId, FrameId) rows, keeping the earliest.
- rebuilds Frames with the new primary key.
- removes ON UPDATE CURRENT_TIMESTAMP from Frames.TimeStamp. Any UPDATE
of a frame row was overwriting its capture time.
Each step checks the current schema first, so the migration can be
re-run.
REST API: view, edit and delete take /frames/<action>/<EventId>/<FrameId>.json.
The old single-Id URLs return 404. CakePHP 2 has no composite keys, so
the model's primaryKey is EventId. That keeps Event's dependent cascade
delete limited to the event's own frames. The controller writes with
explicit (EventId, FrameId) conditions instead of save(), which would
match rows on EventId alone. Edit no longer changes EventId or FrameId.
view=image with fid but no eid used to look up Frames.Id. It now returns
404.
The Perl Frame class is identified by (EventId, FrameId).
Co-Authored-By: Claude Opus 5.5 <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>
Group membership feeds per-monitor access through Groups_Permissions, but
the classic group actions and the Groups API checked only the global
Groups permission. A Groups editor could add a monitor they are denied to
a group they have access through, or remove it from, or delete, the group
that denies it.
Add Group::canEditMembership() and require it, in both the classic UI and
the API, for every monitor added to or removed from a group, for all of a
group's monitors when it is re-parented, and for all of them when it is
deleted. The API also pins the record id and omits monitors the user may
not view from the groups it lists.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ZM_AUTH_HASH_SECRET signs the JWT access and refresh tokens, and its
default is a fixed string in the public source. Nothing generated a
per-install value and tokens signed with the default verified normally,
so on an install with auth on and the secret untouched anyone could sign
an admin token and be accepted by the web UI, the API and zms.
- ZoneMinder::Config::saveConfigToDB() now replaces an empty or default
secret with 32 random bytes from /dev/urandom, hex encoded. Package
installs and upgrades run zmupdate.pl -f, which saves the config, so
existing installs get a secret on upgrade. A secret the admin set is
left alone.
- validateToken() in PHP and zmLoadTokenUser() in C++ refuse to verify
tokens while the secret is empty or the default, and the API refuses to
issue them, as it already did for an empty secret.
- zmLoadTokenUser() no longer writes the signing key to the debug log.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The classic event actions checked only the global Events permission and
then acted on whatever event ids the request supplied, so a user denied a
monitor could still change that monitor's events:
- deleteEvent() deleted when the user had Events=Edit. It now requires
Event::canEdit(), which also requires access to the event's monitor.
This covers the events form, the event form and monitor deletion.
- The events form archive/unarchive updated Events by id. Each event now
needs canEdit().
- ajax events archiveRequest() updated by id under the page-wide Events
view check. Archive now needs canView() on the event and unarchive
canEdit(), keeping the intent that viewers may archive.
- actions/event.php returned early whenever an eid was supplied, so none
of its actions ran. Fix that inverted test, and require canEdit() on the
event for rename, detail edits, archive, unarchive and delete, rather
than the global permission.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ZM_Object::set() called any method whose name matched a key in its data,
and changes() called it as a getter. That data is usually a request array
(filter[...], newMonitor[...], user[...]), so a request could invoke
save(), delete(), execute() and the like. filterdebug with fid=0 did
exactly that before its authorization check: filter[save][...] stored an
AutoExecute filter with a chosen command and filter[execute] ran
zmfilter.pl on it, giving command execution to any logged-in user. The
filter and events views pass filter[...] to set() the same way.
set() and changes() now dispatch a key to a method only when the key is
a field in $defaults or is listed in the class's new static $setters, the
accessors outside $defaults that take a value (Filter's query accessors,
Monitor::Model/Manufacturer/Groups, User::Role, and so on). Other method
names are refused with a warning.
filterdebug also requires Events view before it builds a filter from the
request.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Event::GenerateVideo() built the ffmpeg command as one string and ran it
through escapeshellcmd(), which leaves spaces alone. Any value in the string
could therefore add ffmpeg arguments. The event Name is already reduced to a
safe filename, but DefaultVideo and the transforms were not.
The command is now a list with every argument passed through escapeshellarg(),
the same approach as the Perl GenerateVideo. The configured input and output
option strings are split on whitespace, as the Perl side does. ffmpeg output
goes to ffmpeg.log through proc_open descriptors: escapeshellcmd() was escaping
the old '> ffmpeg.log 2>&1' redirect, so it was passed to ffmpeg as arguments.
An empty transforms string no longer adds a bare -vf.
Checked with a stub ffmpeg that records its argv: a Name and a DefaultVideo
carrying shell and option payloads arrive as single literal arguments and
nothing runs.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DefaultVideo can be set by any user with Events=Edit through the API
(EventsController add/edit), and readers join it onto the event path:
view_video.php streams it, image.php extracts frames from it,
findVideoEventFile, Event::getStreamSrc/FileSize and the API fileExists
check all use Path().'/'.DefaultVideo. A value such as ../../x pointed
those reads at arbitrary files readable by the web account.
Reject DefaultVideo values on save in the API model unless they are a
bare filename (no / or \, no NUL, not . or ..). Make the PHP Event
DefaultVideo() accessor return basename() so every web reader only ever
looks inside the event directory, even for rows written before this
check, and apply basename() to the raw array read in the API model's
fileExists.
Event::GenerateVideo built its output filename from Name with only
whitespace replaced; Name is editable through the event rename and
eventdetail actions. Replace anything outside [-A-Za-z0-9_.] and a
leading dot so the output stays inside the event directory.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit f3f679b644db28b4b1bff79d5dd98eca24395fc4)
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)
sortHeader() concatenated $_REQUEST['filter']['query'] into the href of
every export view column header without escaping, allowing reflected
HTML injection (iframe, meta refresh, style) via a crafted link. Pass it
through validHtmlStr() like the other request values in the same string
are passed through validInt()/validCardinal().
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 7517018879f2be9ba1f9439e3364686ccee226ed)
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)
Reverts the previous commit's always-on measurement. Decoding audio for every
monitor that has it spends CPU on a number almost nothing reads, which is the
wrong trade even though it did solve the chicken and egg of picking a
threshold without ever seeing a level.
Measure when something is actually going to use the reading instead:
- AudioDetection is on, as before, so nothing changes for a monitor that
scores on audio; or
- somebody asked. SharedData gains audio_level_until, a wall clock second
the capture thread keeps measuring up to. The monitor editor's new level
meter pushes it forward while it is on screen and the measurement lapses a
few seconds after the page is left, so nothing has to send a stop and a
crashed browser cannot leave a monitor decoding forever.
When the reading stops being wanted the decoder is released and the published
level and peak are cleared, so a stale number is not left looking current and
an old peak does not land on the next frame row written.
audio_level_until is carved out of analysis_pad rather than appended, so
SharedData stays 888 bytes and no existing offset moves; the static_asserts,
Memory.pm and Monitor.php are updated together and all three now agree the
field is at +880.
The meter itself is on the audio settings, shown whether or not
AudioDetection is checked, because the level is what you need in order to
choose a threshold. It draws the threshold currently in the input as a mark on
the bar so a reading can be judged against it before saving, and a monitor
whose zmc is not running reads "no reading" rather than a confident 0, which
would be indistinguishable from silence.
Frames.AudioLevel is therefore 0 again on monitors that do not score on audio.
That is what the graph already treats as "no audio data", so it draws no line
rather than a flat one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
The audio_fifo_path field was retired alongside video_fifo_path when the
media FIFOs were replaced by the stream socket. video_fifo_path was
repurposed as stream_socket_path, but a single socket carries both
streams so there is no separate audio path to publish; reserved_path2
stays reserved.
Spell out the convention where the field is defined, so a future reader
does not remove or reorder it (which would shift every later field for
out-of-tree shm readers) and knows the slot is free to reuse at the same
64-byte width. Align the PHP Monitor object and ZoneMinder::Memory notes.
refs #5143
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T4UcdJLt1bxwdpcigGxZRD
Address monitor-side stream socket review findings:
- zmc starts the stream socket before the first connect attempt, and
SendStreamHealthEvent records the health state even when the socket is
not up yet, so connection/prime faults during startup are observable to
a consumer instead of being lost until the first successful prime.
- PrimeCapture announces audio whenever the camera has a decodable audio
stream (Capture forwards audio packets unconditionally), and clears a
previously announced stream when a re-prime no longer sees it.
- Publish the media stream socket path in the monitor shared-memory
block (reusing the retired video_fifo_path field as stream_socket_path,
same offset and size) so consumers discover it without hard-coding the
convention or reading the producer's zm.conf; the PHP Monitor object
and the ZoneMinder::Memory Perl module expose the renamed field.
- Move the wall-clock microseconds helper out of zm_monitor.cpp into
zm_time.h as SystemClockMicros(), where time helpers belong.
refs #5143
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01T4UcdJLt1bxwdpcigGxZRD
Filter::widget() opened <table id="fieldsTable"><tbody> and returned
without closing either. On the filter view the parser recovered because
the next sibling was another <table>, but on the events page the
following </div> closed the toolbar column while the table stayed open,
so the remaining toolbar columns were foster-parented out of place.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rename video_fifo_path/audio_fifo_path to reserved_path1/2, keeping the
struct layout and 864-byte size unchanged so existing shared memory
mappers (and out-of-tree shm readers) keep working through the
transition. The fields are zeroed at startup as before, scrubbing any
stale path strings left by a pre-upgrade zmc.
The stream socket deliberately publishes nothing via shared memory: its
path is the documented convention PATH_SOCKS/stream_{monitor_id}.sock,
derivable from static config, and codec parameters travel in the HELLO
handshake instead of shm fields.
Perl (Memory.pm) and PHP shared memory maps updated to match by name;
offsets are unchanged.
Tests: full suite passes via ctest; php -l clean on Monitor.php.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ZoneMinder has carried audio for years without ever listening to it: the
packets go into the event and nothing reads them. AudioDetector decodes
them in the capture thread and reports a 0-100 level, which Analyse()
turns into score alongside motion, ONVIF and Amcrest.
The level is dBFS-derived, not a raw amplitude ratio. Linear RMS is
unusable as a setting: ordinary speech sits at 1-3% of full scale, so
every sound worth catching would be crammed into the bottom two points of
the range and no operator could tune it. Mapping -60..0 dBFS onto 0..100
puts speech around 43 instead.
Three columns on Monitors: AudioDetection to enable it, AudioThreshold
for the level to alarm at, and AudioAlarmScore for what it contributes.
AudioAlarmScore defaults to 9, matching what an ONVIF or Amcrest alarm
already adds. A threshold of 0 means off, so enabling detection without
choosing a threshold cannot alarm on silence -- a plain level >= threshold
test would alarm on every packet in that state.
The level and the alarm flag go in SharedData's two spare bytes, renamed
from reserved1/reserved2. Offsets and sizes are unchanged, so the
888-byte cross-process layout and the Memory.pm and Monitor.php offset
tables all still agree; the readers are renamed in the same commit so the
level is available to the web UI.
The decoder is opened lazily on the first audio packet rather than at
camera setup, so a monitor with detection off never carries one and a
stream that gains audio on reconnect still gets picked up. Scoring reads
the capture thread's most recent level rather than scoring per audio
packet, because the score belongs to a video frame and audio packets do
not arrive in step with them.
Tested: 242 assertions over the pure helpers - the RMS of both sample
formats, the dB scale's monotonicity and endpoints, full-scale clamping
of decoder overshoot, and the threshold-0 case. The repo's Catch2 harness
needs v3 and only v2 is installed here, so tests/zm_audio_detector.cpp
was compiled against a v2 shim to check it, and an identical set of
assertions was run standalone.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvTCzCbvGt8xKQRNCSA7o8
A camera that detects motion should be able to sound a speaker, and the
speaker is rarely the camera: it is a separate device with its own address,
credentials, stream and Controls entry. Model it as a monitor and let a
monitor's alarm drive actions on other monitors.
Add Monitors.DeviceClass enum('Camera','Speaker'). This is what the device
is, as distinct from Type, which selects the capture backend - an IP speaker
still captures over Ffmpeg like any other RTSP device, so Type could not
carry the distinction.
Add the MonitorActions table: MonitorId is the monitor that triggers,
TargetMonitorId the device acted on, and the two are frequently different.
TriggerOn covers EventStart, EventEnd, Alarm and Manual.
Which actions a device is offered is decided by its Controls row - CanLight,
CanIndicatorLight, CanAudioPlay - so a device can only be asked to do what it
has been measured to do. The editor filters on this and the save path
re-checks it, because the request is not to be trusted.
Execution goes straight to the target's zmcontrol socket rather than forking
zmcontrol.pl per action: the daemon already accepts a line of JSON there, and
it is the same path the control panel uses. ActionCommandName maps the DB
enum onto a method name as a whitelist, so nothing out of the database
reaches the control daemon uninspected. Actions are fire-and-forget - a
speaker that is offline is logged and skipped, never allowed to hold up event
handling.
Alarm actions fire only on the genuine entry into alarm, not on the
ALERT->ALARM re-entry, which would re-sound a speaker within one incident.
EventEnd runs on the calling thread before the event is handed to the closing
thread, which does not capture `this`.
Manual actions appear as buttons on the watch page, and are the reason the
control panel is now shown for a monitor that has actions but no control of
its own. Firing one sends only the action id; the command and target are
rebuilt server side, and Control rights are required on the target device and
not merely on the monitor the action hangs off.
Also add --file to zmcontrol.pl, without which audioPlay could not be driven
from the command line. The web path was unaffected as it bypasses GetOptions.
Tests: tests/zm_monitor_action.cpp covers the command whitelist, the message
format (including that file id 0 is a real id and that a stale AudioFile is
never passed to a command that takes none), trigger names, and that every
value of the ActionType enum maps to a command.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvTCzCbvGt8xKQRNCSA7o8
(cherry picked from commit b561341e988af69c3a46db854da410309f7cfa82)
An IP speaker is controllable but has none of the capabilities the Controls
table models: nothing moves, focuses or lights up. It plays a sound file it
already holds, selected by a numeric id, and carries an output volume.
Add CanAudioPlay, MinAudioFile, MaxAudioFile and CanAudioVolume to Controls,
with migration zm_update-1.39.21.sql. CanAudioPlay renders one button per
sound id plus a stop button; MinAudioFile/MaxAudioFile bound that range;
CanAudioVolume renders the volume pair. The sound id travels in the command
name (audioPlay12), the way presets already do, because a control button has
no way to attach a separate parameter.
Add ZoneMinder::Control::IPSpeaker driving the device's own HTTP interface,
and a Controls entry for it. Playback goes over the vendor interface rather
than ONVIF because the ONVIF audio output service on this firmware only
describes the output and offers no way to start a stored file.
Measured against a device reporting ONVIF Manufacturer "IPSpeaker" and
firmware CS20-V3.3.45N:
- File ids fall in two windows, 10-14 built-in and 20-30 operator uploads.
Anything else is refused with result -2, and an id in a window with no file
uploaded with result -3, so valid_fileid refuses only the former: an empty
slot is a legitimate id the operator may fill later.
- config=audio.set replaces the whole audio section, so a partial write
reverts the microphone, codec list and echo-cancellation settings. Every
volume change reads the section and echoes it back with the new level.
audio.get also returns outmute, which audio.set rejects, so it is excluded
from the field list.
- The volume reads back and round-trips, but the device's RTSP audio is a
pre-volume tap of the playback signal: it is unchanged with outvolume at 0,
so it cannot confirm the loudspeaker's acoustic output. CanAudioVolume is
set on the strength of the setting persisting, and both the POD and the
seed row say the acoustic effect is unconfirmed.
The seed row covers the built-in window only, because a fresh device has
nothing uploaded and the firmware refuses an empty slot.
Adding columns to Controls extends the 43 positional INSERTs in
db/controls.sql, which carry no column list and so must match the column
count exactly.
Tests: scripts/ZoneMinder/t/ip_speaker.t, 32 assertions covering file id
windows, volume clamping and stepping, request building and the whole-section
write. All pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01UvTCzCbvGt8xKQRNCSA7o8
(cherry picked from commit d07e552a3c58373f7f0ccebf9aa28adb4d51baa3)
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
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
The monitor attribute filters and the event filter terms were unrelated
pieces of UI that happened to sit next to each other. They are not
independent: Name, Capturing, Analysing, Recording, Status and Source do
not filter events at all, they narrow which monitors exist to choose
between - which is to say they decide what the Monitor term should offer.
Anything they exclude cannot appear in the events either, so offering it
in the term is offering a choice that returns nothing.
Filter carries the restriction rather than the widget taking it as an
argument: monitor_options() is a get/set in the same shape as terms(), so a
view states the rule on the filter and then asks that filter for a widget.
Unset, the Monitor term offers every monitor the user can view, so
events.php and any other caller is unchanged.
montagereview sets the monitors that survived its filter bar.
Verified on the install this repo matches: the events view still lists all
monitors in its Monitor term, montage review renders its event filters as
Monitor, Date Time, Date Time, Archive Status, Tags, Notes with Name,
Capturing, Analysing, Recording, Status and Source collapsed, and the
events, console and filter views all still load.
logState() counted every row with Level < INFO and folded anything at or
below PANIC into the FATAL bucket. AUDIT is -5, so audit entries were
counted as fatals and pushed the state to alert/alarm.
Bound the count query at PANIC so AUDIT and NOLOG are left out.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GAFKf86P78WqniPEP2b45J
Two cases produced a Warning for a request that needs no CORS headers at
all, so the log filled with noise that pointed at nothing wrong.
An empty Origin header satisfies isset(), so CORSHeaders() walked the
servers list, matched nothing, and logged " is not found in servers list."
with no value to print. Treat an empty Origin the same as no Origin.
Browsers also send Origin on same-origin POST and fetch. Such a request
needs no headers, but not finding the host in the Servers table still
warned, so any install reached on a hostname the Servers table does not
list warned on ordinary use. Log that at Debug instead. Genuine
cross-origin requests still warn.
Headers are unchanged; this only affects logging and the empty-Origin
short circuit. The comparison ignores the scheme, so http:// against an
https-served HTTP_HOST counts as same-origin - that only suppresses a log
line, no header is emitted either way.
Checked with a standalone assert script over same-origin with and without
a port on both schemes, differing port, differing host, a suffix near-miss
(hamburg.local vs hamburg) and a missing HTTP_HOST; only the first three
go quiet.
Every request through index.php starts a session and always dirties it:
zm_session_set_remote_addr() writes remoteAddr, and index.php stores skin,
css and navbar_type. ZMSessionHandler::write() then persisted that session
unconditionally, so any request arriving without a ZMSESSID cookie left a
Sessions row behind that nothing would ever load again.
Viewing an event polls the event's server every ZM_WEB_REFRESH_STATUS
seconds via monitorUrl, which is absolute when the monitor has a Server
row. Those cross-origin ajax polls carry auth in the URL and no cookie, so
each one added a Sessions row every few seconds. Bot scans of the login
page did the same.
Persist a session only when the client presented our cookie, or when
zm_session_persist() marks it as one we are issuing: login, and the
postLoginQuery stashed before redirecting to the login page.
Verified on a live install by logging row counts from the save handler:
three cookieless requests skipped the write and left the count unchanged,
while a cookie-jar run wrote on the request that returned the cookie.
php -l clean on both files.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GAFKf86P78WqniPEP2b45J
SIGHUP means reload. zmc responds by closing its events, disconnecting the
camera and reconnecting; the perl daemons respond by exiting so zmdc restarts
them. zmdc.pl logrot hupped every managed process, so the nightly logrotate
run cost about 8 seconds of capture on a default install.
Rotating a log file only needs the daemon to drop its file handle, so use a
separate signal for it. SIGWINCH is otherwise unused, is ignored by default and
exists on every supported platform.
- Logger (C++) installs a SIGWINCH handler beside its USR1/USR2 handler. The
handler only sets a flag; the next logPrint closes the file and the write
reopens it at the original path. This covers every C++ binary without
touching any daemon's main loop.
- Logger.pm registers WINCH alongside HUP in logSetSignal, which logInit
already calls, so the scripts that install their own HUP handler still
rotate.
- zmdc.pl logrot sends WINCH. The logrotate config is unchanged - it still
calls zmpkg.pl logrot.
Filter.pm and FilterTerm.php justified MAX_EVENT_DAYS by events not outliving
the nightly HUP, which is no longer what bounds them; cite SectionLength.
Tests: tests/zm_logger_rotate.cpp and tests/perl/test_log_rotate_signal.pl both
log, rename the file out from under the process, confirm writes still land in
the renamed file, signal WINCH and confirm the original path is written again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDhTBPj9xEaT52pRmAufaP
A DateTime lower bound was written as
COALESCE(E.EndDateTime, '9999-12-31 23:59:59') >= T1
reading "an event that has not ended yet never ends". EndDateTime is NULL for
an event that is still recording, but also for one zmc was killed part way
through, and that stays NULL forever - so every abandoned event matched every
window from then on. Wrapping the column in a function also meant no index
could range over it: with the only MonitorId key being MonitorId alone, every
montage review request read all of that monitor's events and filtered them in
memory, however narrow the window.
Length tells the two cases apart. It is flushed during recording, so a live
event's effective end keeps advancing while an abandoned one's is frozen at
whatever was recorded. Use the same CASE the SELECT list in ajax/events.php
already uses, and bound it below by StartDateTime.
The lower bound now emits three conjuncts:
E.StartDateTime >= DATE_SUB(T1, INTERVAL 1 DAY)
AND (E.EndDateTime IS NULL OR E.EndDateTime >= T1)
AND <effective end> >= T1
The floor bounds the scan for a window in the past and drops events abandoned
more than a day earlier - events do not outlive the nightly logrotate SIGHUP,
which stops and restarts them, so a day is comfortably beyond any real event.
The middle conjunct is implied by the third and exists only to give the
optimiser a second indexable handle: it is narrow exactly when the floor is
wide, so between them a window at either end of the retention period has
something cheap to range over. Verified the optimiser picks correctly for both,
unprompted. The third is the residual that gets the semantics right.
Add the two composite keys those conjuncts need. Two range columns cannot both
narrow one B-tree, and these are mirror images - EndDateTime >= T1 is
open-ended upwards, StartDateTime <= T2 downwards:
Events_MonitorId_StartDateTime_idx (MonitorId, StartDateTime)
Events_EndDateTime_MonitorId_idx (EndDateTime, MonitorId)
MonitorId leads the first because it is the equality; past a range column later
columns can no longer narrow the scan, and (StartDateTime, MonitorId) measured
3.5x slower for the same rows. EndDateTime leads the second so it also covers
zmaudit's hunt for events that were never closed, which has no monitor to scope
it - 9 rows with a key against a 22,948 row table scan without.
Both replacements are added before the keys they supersede are dropped:
Events_MonitorId_idx (MonitorId) is now a leftmost prefix of the new key.
Events_EndDateTime_DiskSpace (EndDateTime, DiskSpace) existed for the scan
that hunted events with no DiskSpace set; DiskSpace is set when the event is
finalised in C++, so nothing scans for it any more.
Net index count on Events is unchanged.
Measured on a 7,167 event monitor. Default one hour window: 7,055 rows
examined / 25.4ms -> 173 rows / 4ms. Window scrubbed a week back, which
persists in the zmFilter_StartDateTime cookie: 6,995 rows / 28ms -> 19 rows /
0.15ms.
Adds migration db/zm_update-1.39.22.sql. Extends t/filter_sql.t; the PHP and Perl were
checked to emit identical SQL, and the semantics checked against a table of
seven event shapes - abandoned long ago, abandoned recently, overlapping,
inside, before, still recording, and still recording for 17 hours.
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
