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
auth.php ended in a 130-line block at file scope, so merely including the file
authenticated the current request: it read $_REQUEST, opened a session, queried
the database, and on a login could rewrite the user's stored password hash and
populate $_SESSION. Any caller that wanted one of the functions in the file got
all of that as a side effect, and the order of includes decided when it ran.
HostController requires auth.php twice purely to reach generateAuthHash() and
validateToken().
Move the block into zm_authenticate_request() and call it explicitly from the
two places that want it, web/index.php and AppController::beforeFilter(). The
function returns the ZM\User or null and still sets the global $user, so the
views, ajax handlers and API controllers that read that global are unaffected.
HostController now gets only the function definitions from its requires, which
is all it ever wanted.
Inside a function the five `unset($user)` calls would drop the local binding
and leave the global set, so they become `$user = null` - the idiom the rest of
the file already uses for this, and one that keeps isset($user) false for the
gate at index.php:255. The block's other locals ($ret, $username, $password,
$sql) no longer leak into the caller's scope, which in beforeFilter() means they
can no longer collide with the variables of the same name it assigns just after.
The body is otherwise unchanged; `git diff -w` shows only the wrapper, those
five assignments and the return.
Tests: tests/php/test_auth_no_include_side_effects.php tokenises auth.php and
asserts nothing executes at file scope, with a fixture check so a broken
detector cannot pass vacuously. 4 assertions, all pass. Verified it reports the
pre-refactor file's file-scope block, so it would have caught this.
Not covered by tests: the login, logout, auth-hash and API token flows this
touches. auth.php cannot be included without a database (User.php pulls in
database.php, which connects at include time), so the check is structural.
Needs manual testing on an installed tree before merging.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01477mR97vfnK6zczbHgzq6T
ZM_AUTH_HASH_IPS binds the auth hash to the client address. When that address
changes mid-session - a phone moving between wifi and cellular is the common
case - the hash the browser is still holding no longer matches the address we
now see, and the user is bounced to the login page. The usual workaround is to
turn ZM_AUTH_HASH_IPS off entirely.
Accept the address the request arrives from plus the one it arrived from
immediately before, so an in-flight hash validates once and generateAuthHash()
then reissues against the new address. Addresses are matched exactly. A netmask
was considered and rejected: accepting a whole subnet would let any other host
on the client's network replay a stolen hash, which on a home LAN includes the
cameras themselves.
The previous address is only accepted for as long as a hash issued to it would
itself still be valid (ZM_AUTH_HASH_TTL), so this widens which address is
accepted without extending how long any hash lives. A login clears it, since
nothing from before a privilege boundary should stay acceptable, and only one
previous address is ever retained.
userFromSession() needed the same treatment: it looks the cached hash up by the
live address, so after a change the slot does not exist yet and the user was
reported as not logged in regardless of what getAuthUser() would have accepted.
Also centralises the X-Forwarded-For/REMOTE_ADDR handling in getRemoteAddr(),
replacing four duplicated copies across session.php and auth.php. Those copies
sat on both the generation and validation sides, so any drift between them broke
authentication outright behind a reverse proxy. Network.php holds only that
address parsing; which addresses an auth hash is accepted from is auth policy
and lives in auth.php.
This is web-side only; zms has no session, so a stream request still fails once
on an address change and recovers through the existing auth-refresh path in
MonitorStream.js.
Tests: tests/php/test_remote_addr.php covers getRemoteAddr() parsing and the
session address rotation, and needs no config or database - 18 assertions, all
pass. tests/php/test_auth_hash_candidate_addrs.php covers the acceptance window
including both sides of the TTL boundary; it bootstraps config.php as the other
tests in that directory do and so needs an installed tree to run.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01477mR97vfnK6zczbHgzq6T
Applying a named filter and clicking LIST MATCHES landed on Events showing
"No matching records found" until a manual refresh. Two independent defects
produce that, and both are fixed here.
Stored filter selections overriding the applied filter
------------------------------------------------------
The zmFilter_* cookies remember what was last selected in console, montage and
montagereview. They are a convenience for the default, filter-less page, but
they were also applied on top of a filter the request had specified, silently
widening or narrowing it: the applied filter was ANDed with a date range left
over from an earlier visit to another view, so it matched nothing. Refreshing
appeared to fix it because the bar's reset control blanks those inputs.
The cause was a layering one. Filter::simple_widget() refilled any empty term
value from that term's cookie as it rendered the input, so it overrode the value
addTerm() had been given, from below the layer that knows what the request asked
for. An earlier attempt to fix this in events.php could not hold for that
reason: it blanked the values and they were refilled during rendering, and
ajaxRequest() sends the live inputs rather than the URL.
Filter now resolves nothing. It renders the value it was handed, and the six
cookie fallbacks are gone; terms keep their cookie name so edits still persist
client-side. The callers that build those terms decide instead, next to
getFilterSelection(), which already resolved request-before-cookie this way.
Each resolves its value into a local before the addTerm() calls, testing
$use_stored in the same statement that reads the cookie, so the rule is visible
at the point it applies.
montagereview needs a window to draw whatever happens, so with a filter present
it derives one from the filter's own terms and otherwise defaults to the last
hour, rather than reaching for the stored window; its Notes term no longer seeds
from a cookie inside the branch that already has an explicit filter.
Tables never retrying a request skipped while hidden
----------------------------------------------------
The table views skip their ajax request while the page is hidden so a background
tab does not poll. Bootstrap-table calls that function on init as well as on
refresh, though, and a skipped request was never re-issued: the table rendered
"No matching records found" over a result it never asked for, with nothing to
bring it back. A page can be hidden for the whole of its load - opened in a
background tab, restored, or behind another window - and the same guard is in
seven views: events, console, log, frames, reports, snapshots and watch.
deferTableRequestWhileHidden() skips the request as before and records the table,
so it is refreshed the moment the page becomes visible. The queue is drained
before refreshing, because refresh() calls the ajax function synchronously and
would otherwise re-add a table that is still hidden.
Tests
-----
tests/audit-filter-cookies.php checks both halves of the first rule: that Filter
resolves nothing, and that every stored-selection read in a caller tests
$use_stored. It works a statement at a time, joining continuation lines, since a
value and its guard often span a line break. Anything wider is too coarse: the
enclosing block holds other guarded reads and would mask one that lost its own.
tests/js/table-helpers.test.js covers the deferral queue, including a table
re-deferred during its own refresh.
Verified against a live instance with the stale cookies still set: a "last hour"
named filter queried 0 of 4 matching events before and 12 of 12 after; the
filter-less page still restores the stored date range; and a table deferred while
hidden repaints on becoming visible.
Nothing enforced uniqueness, but every reader already assumed one row per
name: User::Preferences() hashes the rows by Name, montagereview.php and
User_Preference::find_one() take the first match, and TagOrder::load()
reads a single Value. With duplicates present those pick an arbitrary
row, so a write could land on one row while reads returned another --
the tag order would appear to stop updating.
Replace the UserId-only index with UNIQUE (UserId, Name) and make Name
NOT NULL. The unique index still serves UserId-only lookups and the
UserId foreign key as a leftmost prefix, so the old index is redundant
and is dropped. Name has to be NOT NULL for the constraint to mean
anything, since a unique index permits any number of NULLs; a NULL-named
row is unreachable regardless, as a preference is only ever looked up by
name.
zm_update-1.39.19.sql discards NULL-named rows, makes the column NOT
NULL, collapses existing duplicates keeping the highest Id (the most
recently inserted), adds the unique index, then drops the old one. The
index is added before the old one is dropped so the foreign key is never
left without a usable index. Each schema change is guarded against
INFORMATION_SCHEMA so re-running is a no-op. db/User_Preferences.sql
gets the same shape for fresh installs.
With the constraint in place, TagOrder::recordUsage() replaces its
SELECT-then-INSERT-or-UPDATE with a single INSERT ... ON DUPLICATE KEY
UPDATE. That drops a query and closes the race where two concurrent tag
additions by the same user both see no row and both insert.
Verified on MariaDB 11.8.8 against a seeded table: duplicate rows
collapse to the expected survivors, NULL-named rows are removed, a
second run is a clean no-op, NULL/duplicate/bad-UserId inserts are
rejected (1048/1062/1452, so the foreign key survives losing its index),
and mysqldump of a fresh install matches a migrated database exactly.
Bump version.txt and the redhat spec Version to 1.39.19.
Follow-up to review comments on PR #5022.
addtag passed $_REQUEST['tid'] and $_REQUEST['id'] straight into the
Events_Tags INSERT and the Tags UPDATE, while only the TagOrder call
sanitized tid. A non-numeric tid therefore still inserted a row with the
value coerced to 0, and only the recency bookkeeping was skipped.
Validate both ids once at the top of the case, reject non-scalar values
(array-form parameters such as tid[]=1, which validCardinal would return
as an array and bind as one), and reuse the sanitized values for all
three statements.
TagOrder.php calls dbFetchOne/dbQuery but did not pull in the database
helpers itself, relying on the caller having done so. Both current call
sites happen to, but that is incidental; require_once('database.php') to
match Tag.php and User_Preference.php.
Three points raised on #5038 after it was merged.
ajaxError() documents the reason field as included "only when set", but
tested it for truthiness, which would also drop '' and '0'. None of the
four STREAM_ERR_ constants are falsy so nothing changed behaviour, but
the check now matches the documented contract. The client already treats
an empty reason as fatal, so a caller that does pass one still gets the
old handling.
The case 0 branch called ajaxError() twice in sequence and relied on the
first one exiting to keep the second from running on a timeout. Made the
two paths mutually exclusive so it no longer depends on that.
The test's "every ajaxError call is classified" assertion compared the
number of call sites to the number of STREAM_ERR_ occurrences anywhere in
the file. The four define()s are part of that count, so up to four calls
could lose their classification and the test would still pass - verified
by dropping the argument from one call, which the old assertion accepted.
It now matches each call to the end of its statement and requires every
one to carry a constant.
Saving a deleted monitor undeleted it even though the undelete checkbox was
never ticked.
Two causes. The $types map, whose entries exist so that unticked checkboxes
(which submit nothing) get forced to a default, listed Deleted => 0. But the
Deleted checkbox is not a state toggle, it is an undelete action: it is only
rendered for a deleted monitor and only ever submits 0. Forcing it to 0 when
absent put Deleted => 0 into every $changes set. Remove it from $types so an
absent Deleted simply is not a change.
Second, the guard meant to catch the "new monitor reusing a deleted record's
Id" case tested isset($_REQUEST['newMonitor[Deleted]']), a literal-bracket key
that never exists, so $monitor->Deleted(false) ran on every save of a deleted
record. Since save() writes every field, that persisted. Test empty(mid)
instead, which is exactly the case the comment describes: mid is absent when
adding a new monitor whose requested Id collides with a deleted row. Editing an
existing monitor by mid now only undeletes via the checkbox, which arrives as
newMonitor[Deleted]=0 and shows up in $changes on its own.
Manual test on a deleted monitor: editing and saving with the checkbox
unticked keeps Deleted set and persists the other edits; ticking the checkbox
and saving clears Deleted as before. The restart block was already guarded by
!$monitor->Deleted(), so no capture daemon is started for a monitor that stays
deleted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01NDhTBPj9xEaT52pRmAufaP
getStreamCmdResponse() responded to every ajax/stream.php failure the same way:
mint a fresh connkey and reload the img src. ajaxError() returns HTTP 200 with
result=Error, so these arrive in jQuery's done() rather than fail(), and all
twelve error paths in stream.php took that branch.
Only one of them means zms is gone. For the rest the process is still running
and streaming, and replacing the connkey makes it unaddressable: CMD_STOP,
CMD_QUIT and mode=single all then go to the new key, so nothing can reach the
old process and only SIGPIPE can stop it, which we know is unreliable. That is
why the reports of lingering zms after switching monitors were unaffected by
changes to what the stop path sends.
The timeout path made this routine rather than rare. On select() expiry
ajaxError is commented out, so the script carries on to socket_recvfrom() on a
now non-blocking socket. That returns false, and false == 0 under switch's loose
comparison, so a merely slow zms was reported as 'No data to read from socket'
and torn down.
stream.php now classifies each failure as no_socket, timeout, transient or
invalid, and sends it as 'reason'. The client restarts the stream only for
no_socket. A missing reason is still treated as fatal, so a php that predates
this keeps the old behaviour.
Before replacing the connkey the client now sends CMD_QUIT to the old one, so
the process we are about to lose track of is asked to exit. That is deliberately
not routed through streamCommand(): it must name its target explicitly, since
this.connKey is about to change, and its response must not feed back into
getStreamCmdResponse(), or a QUIT that also failed would re-enter the error path
and loop.
ajaxError() takes the classification as a third argument, named $reason because
$code is already the HTTP status, and only includes it when set, so the other
131 callers are unaffected.
Tests: tests/js covers the fatal/non-fatal decision including the no-reason
fallback, tests/php pins the classification mapping and the switch(false)
semantics the timeout branch depends on. Both verified to fail when the
behaviour is reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The filterdebug modal (web/ajax/modals/filterdebug.php) builds and EXPLAINs a
filter's events query but enforced no authorization beyond the global login
check, so any authenticated user could inspect the MySQL EXPLAIN for an
arbitrary stored filter (including one they don't own).
Add ZM\Filter::canView() mirroring canDelete()/canEdit(): System viewers can
inspect any filter, otherwise the user must own it; an unsaved/transient
filter (no Id, built from the requester's own request in this modal) is
viewable by the requester. Construct the filter up front in filterdebug.php
and return early when the current user can't view it.
Add tests/php/test_filter_canview.php covering owner/non-owner/system/unsaved
cases.
refs GHSA-28mv-hqxw-qw84
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The Filter `limit` value is user-controlled (populated from the request
via set()/Query()) and was returned verbatim by ZM\Filter::limit(), then
concatenated straight into SQL by two callers:
- web/ajax/modals/filterdebug.php (EXPLAIN ... LIMIT <limit>)
- ZM\Filter::Events() (SELECT ... LIMIT <limit>)
An authenticated, view-only user could submit filter[Query][limit] with
an error-based/subquery payload after the LIMIT keyword and read arbitrary
database contents (e.g. Users password hashes). The parallel path in
web/ajax/events.php already cast the value with (int); these two sites did
not.
Coerce to int at the source in limit() so every caller is safe, and add
defense-in-depth (int) casts at both concatenation sites to match the
events.php pattern. sort_field is already validated by
isValidSortExpression(); sort_asc/skip_locked are boolean-guarded.
Add tests/php/test_filter_limit_sqli.php exercising the real ZM\Filter to
prove the payload is coerced to 1 and benign values round-trip.
See GHSA-28mv-hqxw-qw84.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
06801955a bound $this->Id() directly in the stats lookup. On a
single-server install the Server object's Id() is empty/undef and
zmstats.pl writes the rows under ServerId=0 (ZM_SERVER_ID ? ZM_SERVER_ID
: 0), so the query matched nothing and CpuLoad/load reported as -1.
Coerce a falsy id to 0 so the reader mirrors the writer; a real
(multi-server) id still passes through unchanged.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Server.php's ReadStats() query (SELECT ... WHERE ServerId=? ORDER BY
TimeStamp DESC LIMIT 1) had no supporting index, forcing a full table
scan on every page load that shows server stats. Add a composite
(ServerId, TimeStamp) index via zm_update-1.39.18.sql (idempotent,
checked against INFORMATION_SCHEMA.STATISTICS) and zm_create.sql.in
for fresh installs. Keep the existing TimeStamp-only index, since
zmstats.pl's prune DELETE filters by TimeStamp alone.
Also query ReadStats() with the server's actual Id() instead of
coercing Id() <= 1 to 0.
Bump version.txt and the redhat spec Version to 1.39.18.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q7jeoG8ptXRpXxif2JqkD7
Filter::canEdit() checked view-only users with an `and` chain, so it only
denied a filter when every auto-action was enabled at once; a filter with
only AutoExecute set passed the check. Because AutoExecuteCmd is run as a
shell command by zmfilter.pl (qx($command)), a user with Events=View could
run arbitrary OS commands via a temporary filter.
Rework canEdit():
- enforce ownership before any per-flag checks
- require System edit permission for AutoExecute; running an arbitrary OS
command is a System-level capability, not event editing
- deny view-only users when ANY auto side-effect is enabled (and -> or),
covering AutoArchive/Video/Upload/Email/Message as well
Hide the AutoExecute/AutoExecuteCmd inputs in the classic filter view from
non-System users, preserving existing values in hidden fields so unrelated
edits do not alter them.
Add tests/php/test_filter_canedit_autoexecute.php exercising the real
canEdit() across the permission matrix.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019pVdHJvR87bvPMu1EFDN7M
setCookie() JSON-encodes arrays, so the Monitor/Notes/Group filter cookies are
stored as JSON while simple_widget() read them with explode(','), breaking
round-trips (montagereview already writes JSON). Add Filter::decode_multi()
which json_decodes first and falls back to explode(',') for legacy comma
values, and route the Monitor and Notes term/cookie reads through it. Also add
the missing cookie-default handling to the Group term so a persisted group
selection is applied.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A DateTime filter term now expresses "event overlaps this instant/window"
rather than a plain StartDateTime comparison. A lower bound (>=/>) compares
COALESCE(EndDateTime,'9999-12-31 23:59:59') so events still running at that
time (NULL EndDateTime) or ending after it are included; an upper bound
(<=/</=) still compares StartDateTime. Composed across the two montagereview
bounds this selects events overlapping the timeline window, so events that
began before the window but are still recording are no longer omitted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The delete action computed $is_ok_path to confirm the requested path sits
below a configured Storage area, but never consulted it, so the check was
dead code and unlink() ran on whatever path was supplied. The path comes
from detaintPathAllowAbsolute($_REQUEST['path']), which deliberately
permits absolute paths, so nothing else constrained the target.
Return with an error when the path is not below a Storage area. Deleting
already requires System Edit, so this is not reachable by a low privilege
user, but the containment check should do what it was written to do.
The adjacent $path_parts assignment is also unused; left in place as it
predates this change.
Refs GHSA-g355-3rf6-f38v.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
detaintPath() and detaintPathAllowAbsolute() removed '://' with a single
str_replace() while the '../' removal below them already looped. One
pass is not enough, because removing a match can join its neighbours
into a fresh match: '::////' collapses to '://'. So
'php::////filter/read=string.rot13/resource=/etc/passwd' came back out
of the filter as 'php://filter/read=string.rot13/resource=/etc/passwd',
reinstating exactly the wrapper the strip exists to remove.
Loop the '://' removal the same way the '../' removal is looped. These
functions guard $view, $request, $action, the modal name and skin file
paths.
Refs GHSA-wgqf-6fjf-7gxw.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Zone.Name, Event.Name, Event.Notes and Server.Name are persisted
user-controllable strings that were emitted without escaping, so a user
with edit rights on the object could store markup that runs in the
browser of anyone who later views the page, including an administrator.
Zone and Server save through getFormChanges + raw SQL, so they never
pass the object filter_regexp layer that strips markup elsewhere.
HTML and SVG sinks now use validHtmlStr():
- Zone::svg_polygon() escapes the <title>, covering every caller
(event view, montage and stream).
- views/zone.php and views/plugin.php headings.
- ajax/modals/server.php modal title.
The two inline-JS sinks in views/js/event.js.php are require_once'd
inside a <script nonce> block, so a </script> in Event.Name or Notes
broke out of the element and the nonce did not help. Notes was also
interpolated into a template literal, making backtick and ${} live.
Both now emit json_encode(..., JSON_HEX_TAG|JSON_HEX_APOS|
JSON_HEX_QUOT|JSON_HEX_AMP), which supplies its own double quotes.
Refs GHSA-72c3-g86w-f587.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
validateToken() returns array(false, $errorMessage) when a token is
invalid or its signature fails. The token branch in auth.php assigned
$user = $ret[0] unconditionally, leaving $user as boolean false on
failure. Because isset($false) is true in PHP, the ZM_OPT_USE_AUTH gate
in index.php (!isset($user)) was skipped, allowing unauthenticated
access via any malformed ?token= value. It also permitted an
unauthenticated DoS: downstream $user->Username() on a bool fatals.
Check !$ret[0] and unset($user) on failure, mirroring the existing
validateUser branch, so $user stays undefined and the auth gate blocks
the request. The API call sites already throw UnauthorizedException on
!$user and are unaffected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the single alarm_image slot with an analysis_image_buffer ring of
image_buffer_count Images living in the already-reserved alarm_images SHM
region. Successive WriteAlarmImage calls rotate through the ring and
publish last_analysis_index last (after the bytes and per-slot format),
so a reader sampling last_analysis_index always sees a fully written
slot. GetAlarmImage returns that slot, syncing its AVPixelFormat from the
per-slot analysis_image_pixelformats array.
SharedData gains last_analysis_index and analysis_image_count (plus 8
bytes of padding to keep the 16-byte-multiple layout), making it 888
bytes. The Perl (Memory.pm) and PHP (Monitor.php) SHM readers are updated
in lockstep, and a static_assert(sizeof(SharedData)==888) in zm_monitor.h
guards the layout against silent drift.
This lets multiple in-flight analysis/annotated frames be buffered and
streamed in sync rather than always overwriting one slot, and gives the
AI object-detection work a place to publish annotated frames.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The C++ SharedData struct is naturally aligned, not packed, so the
compiler inserts a 4-byte pad before capture_fps (after state) and
another before the startup_time union (after audio_channels). Monitor.php
used the naive packed offsets, so every field from capture_fps onward
(capture_fps/analysis_fps, latitude/longitude, the time fields,
alarm_cause and all of TriggerData) was read from an address 4-8 bytes
too low, yielding garbage.
Correct the offsets to the real aligned layout (SharedData is 872 bytes,
TriggerData starts at 872), matching what ZoneMinder::Memory computes and
what the C++ writes. No struct change; this is a reader-side fix.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add three Options tabs (AI Datasets, AI Models, AI Classes) with full
CRUD, backed by the AI_* tables:
- List views (_options_ai_{datasets,models,classes}.php), edit modals
(ajax/modals/ai_{dataset,model,class}.php) and action handlers
(actions/ai_{dataset,model,class}.php).
- options.js loads the modals over ajax, wires the Add/edit buttons and
the AI Classes dataset filter.
- options.php dispatches the new tab includes; functions.php registers
the three tabs in the Options sub-menu with readable labels (they are
not Config categories, so they need explicit entries).
- actions/options.php routes object=ai_* deletes to the matching
handler; saves post directly to view=ai_*.
All tabs and actions are gated on System permission.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Port the menu customization work from the ai_server branch onto master:
- Add/edit/delete custom navbar/sidebar menu entries via Options > Menu
- Per-entry Link column (new Menu_Items.Link) with ?view= fallback derived
the same way as built-in items; custom entries render via buildMenuItem
- Live icon preview (material/font awesome) with fixed-width preview cell
- Per-row delete icon; add/reset buttons with tooltips
- Surface DB errors when saving options config: capture the swallowed PDO
error via dbLastError() and show it in the page error banner instead of
redirecting past it
Menu_Items.Link is added to zm_create.sql.in and as an idempotent ALTER in
zm_update-1.39.16.sql.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The C++ and PHP parsers for zm.conf and /etc/zm/conf.d/*.conf used fgets
with a 512-byte buffer, silently truncating any line longer than that.
There was also no way to split a value across multiple lines, so editors
hit the cap with no workaround.
Accept a trailing backslash (with optional whitespace before the newline)
as a line-continuation marker. Leading whitespace on continuation lines
is stripped so users can indent for readability without it leaking into
the value. Three parsers all read the same files and must agree:
- src/zm_config.cpp: switch to std::ifstream + std::getline so a single
physical line is no longer capped at 512 bytes, then join continuation
lines before running the existing pointer-based parser
- scripts/ZoneMinder/lib/ZoneMinder/Config.pm.in: accumulate a logical
line across trailing-backslash physical lines
- web/includes/config.php.in: drop the 512-byte fgets cap and accumulate
in the same way
tests/zm_config.cpp covers three cases against the C++ parser: a
three-segment continuation joins to "firstsecondthird" with leading
whitespace stripped, a bare backslash inside a value is preserved
(C:\Users\zm), and a 1500-byte single line survives intact.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The insert branch only logged via ZM\Error and returned, leaving
$error_message empty. The dispatcher then re-rendered views/monitor.php
with no banner and the user saw an apparent success that silently
discarded their new monitor.
Append $monitor->get_last_error() to $error_message so the existing
<div id="error"> in getBodyTopHTML() shows the actual DB message, and
keep the early return so the user stays on the edit view with the form
values preserved by views/monitor.php's $_REQUEST['newMonitor'] handling.
Matches the pattern already used on the update path at line 273.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
generateAuthHash() cached every hash under the IP-keyed slot
'AuthHash'.$remoteAddr, but the value was IP-bound only when $useRemoteAddr was
set. getZmuCommand() calls generateAuthHash(false, true), which wrote an
IP-less hash into the IP-bound slot and reset AuthHashGeneratedAt. Because the
status poll runs getZmuCommand (web/ajax/status.php) right before emitting the
auth hash, the poll then served the IP-less hash to the browser; the next
IP-bound request was rejected by the validator, redirecting the user to login
roughly every poll. This happens with a completely stable client IP, so it is
distinct from the IP-rotation case.
Key the cache slot by the address actually baked into the value: only use the
session address when this caller asked for it (and AUTH_HASH_IPS is on). IP-less
and IP-bound hashes now occupy separate slots and can no longer clobber each
other.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The tablename field of a filter term was copied verbatim into SQL by
sql_attr(), while attr, op, val and collate were all sanitized. An
authenticated user with Events View permission could inject SQL via the
filter[Query][terms][N][tablename] request parameter, enabling blind
read access to the whole database (password hashes, camera credentials).
Restrict tablename to the table aliases actually used by the filter
queries (E, M, S, F, T, ET, Snapshots); reject anything else, log it,
and fall back to 'E'.
Refs GHSA-q2w3-h644-f8xq
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- API (database.php.default): only set the PDO verify flag when SSL is
actually configured (ZM_DB_SSL_CA_CERT set), matching the web/Perl/C++
layers. Previously a fresh install's default (1) would set the flag on a
non-SSL connection, since the CakePHP datasource merges 'flags' uncondi-
tionally.
- Both PHP layers: cast to string and trim before parsing the value, and use
strict in_array, to avoid type-juggling and stray-whitespace edge cases.
- zm_db.cpp: use my_bool (not char) for the MYSQL_OPT_SSL_VERIFY_SERVER_CERT
fallback argument, the type libmysqlclient expects. That branch only
compiles on older clients without MYSQL_OPT_SSL_MODE, where my_bool exists.
refs #3816
Addresses review feedback on #4223:
- The IS NOT Odd/Even branch emitted '% 2 = ' (identical to IS), so
'MaxScore IS NOT Odd' matched odd values instead of even. Emit '% 2 != '
so the negation is correct.
- Reword the comment to reflect that IS is only preserved for NULL here;
any other value (including TRUE/FALSE) compares for equality.
refs #4223
Jash