The merged mp4 export is named '<Monitor> <start> to <end>.mp4', so it contains
spaces and colons, and download.php emitted it as a bare unquoted filename=
parameter. That is not a valid RFC 6266 token, so browsers that parse
Content-Disposition strictly find no filename and fall back to naming the
download after the last path segment of the URL - index.php. Firefox is lenient
and accepted it, which is why the report was Chrome-on-Windows only.
Add contentDispositionAttachment(), which emits a quoted ASCII filename with the
Windows-illegal characters folded to '_', plus the untouched name as RFC 5987
filename* whenever that folding changed anything, so unicode monitor names still
arrive intact.
Also in that path:
- urlencode the file and export_root query parameters; a monitor name containing
'&' or '+' would otherwise split or mis-decode the download URL. Read them back
in export.js with URLSearchParams so the link text shows the decoded name.
- drop the stray ';' from Content-Length, which made the value unparseable.
- silence the shutdown unlink()s, whose warnings would be appended to the body
of a download that had already started.
- log $this->filenamePath, not an undefined local, on the unreadable-file path.
Tests: tests/php/test_download_content_disposition.php, 12 assertions, all pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Nr76CednxtDt2nPuq6WrbL
The three profiles' settings were aliased by a switch with one arm per
profile, each repeating the same 18 define() calls against a different
ZM_WEB_<H|M|L>_ prefix. Sixty three lines in which only a single letter
differed, and nothing held the arms in step: a setting added to one and not
the others is undefined for two thirds of users, which is the same blank page
the missing default case caused, just narrower.
Name the settings once and build both sides from the prefix. The two settings
that carried a defined() guard keep it, as a separate list with the fallback
each one uses, so a genuinely absent setting is still distinguishable from a
mistyped one - the rest go through constant() and fail loudly.
Looking up an unknown profile now yields the low prefix instead of skipping
every define, so the skin config no longer depends on skin.php having clamped
the cookie first; that clamp remains the place a bad value is corrected.
Verified by diffing every resulting ZM_WEB_ constant against the previous
implementation for each of the three profiles: 49 constants, identical values.
The test drops the checks that only made sense against the switch and gains
ones for the new shape. It loads the skin config in a child process, once per
profile probed, because constants cannot be redefined and loading it is itself
what can fail. It now catches a mistyped setting name, a removed fallback, a
profile the whitelist does not know, and a setting dropped from the alias list
- the last by way of the per-profile config options, which are the authority
on which settings exist and are independent of the lists under test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
web/skins/classic/includes/config.php defines all 18 ZM_WEB_* constants inside
a switch on $_COOKIE['zmBandwidth'] with cases for high, medium and low and no
default. On any other value none of them are defined, and skin.js.php - emitted
in the footer of every page - reads ZM_WEB_VIEWING_TIMEOUT, ZM_WEB_AJAX_TIMEOUT
and ZM_WEB_REFRESH_NAVBAR. On PHP 8 an undefined constant is a fatal Error, so
every page including login stops rendering until the cookie is cleared, which
cannot be done from inside the interface.
skin.php only tested the value for empty, and nothing else validated it:
- the cookie is set client side by skin.js, so any value survives
- action=bandwidth put $_REQUEST['newBandwidth'] through validStr, which is
only strip_tags, and persisted it
- ZM_BANDWIDTH_DEFAULT is a free-form string in ConfigData. The Options UI
renders it as a select, but loadConfig lets a conf.d file override the
database, so a typo there locks out everyone with no cookie yet
skin.php now validates both the cookie and ZM_BANDWIDTH_DEFAULT before falling
back to low, and the action rejects a value it does not recognise rather than
storing it.
tests/php/test_bandwidth_clamp.php checks the whitelist against the switch it
guards - the two must name the same profiles, since a value in one and not the
other reopens this - and that no arm of the switch defines a constant the
others do not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Y6FieTwEXuLhhR4e2yiax
database.php called dbConnect() at file scope, so including it opened a socket,
and on failure rendered views/no_database_connection.php and exit()ed from
inside a library include. Every model in web/includes requires this file, so
merely loading a class did both.
Connect on first use instead. $dbConn becomes tri-state - false for "not
attempted", null for "attempt failed", a PDO for connected - and two accessors
sit on top of it:
zmDbConn() opens if needed; on failure renders the error view and
stops, which is what the include used to do, just at the
point a query is actually attempted.
zmDbConnOrNull() opens if needed but returns null instead of ending the
request, for callers with a fallback.
dbQuery() is the funnel every fetch helper goes through, so routing it plus
dbEscape(), dbError() and dbInsertId() through the accessors covers the library.
The five callers that reached for the raw global are updated: config.php.in,
Event.php and ajax/console.php need a connection and take zmDbConn(); logger.php
takes zmDbConnOrNull() and falls through to its error_log target, so a logging
call can no longer end the request or open a connection by itself.
ZMSessionHandler captured $dbConn in its constructor. It is constructed while
session.php is being included, before anything has needed the database, so with
a lazy connection that captured false. It now resolves per call and its methods
return "no session" rather than dereferencing a bool.
Two smaller fixes fall out. The error view was included by a relative path that
only resolved when the cwd was web/, so it never worked for requests served out
of web/api/; it is now anchored with __DIR__. And dbDisconnect() set $dbConn to
null, which in the new tri-state means "connecting failed" and would send the
next query to the error page; it sets false so a later query can reconnect.
Nothing calls dbDisconnect() today.
This does NOT make database.php includable without a database. It requires
logger.php, which requires config.php, which reads ZoneMinder's configuration
out of the Config table at include time. Until that cycle is broken the
connection still happens during bootstrap, just from config.php rather than from
here.
Tests: tests/php/test_database_lazy_connect.php, 7 assertions, all pass. It
tokenises database.php and asserts nothing runs at include time, that dbQuery()
goes through the accessor, and that only the connection plumbing touches the
global. Verified it reports the pre-refactor file's `if ( !dbConnect() )` - an
earlier version of the check skipped tokens inside parentheses and so passed on
exactly the code it exists to reject.
Not covered by tests: behaviour when the database is genuinely unreachable, and
the session handler against a live database. Needs manual testing on an
installed tree, including stopping mysql to confirm the error view still renders
for both a web request and an API request.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01477mR97vfnK6zczbHgzq6T
The side-effect detector skipped tokens inside parentheses, so a call in a
condition - `if ( !dbConnect() )`, the shape database.php had - was invisible to
it. Only the enclosing control-flow keyword was reported, and a file whose sole
include-time work sat inside a condition would have passed.
Drop the parenthesis rule and add a fixture for that shape.
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
Deleted monitors are excluded from every monitor listing, so once a monitor
is deleted there is no way to find it again from the ui - which matters
because deleting is reversible, the monitor edit form has an undelete
checkbox for exactly that.
Add Deleted as a pseudo status in the Status filter. Selected on its own it
lists only the deleted monitors; selected alongside real statuses it adds
them to that selection rather than intersecting with it, which would always
be empty; not selected, listings stay restricted to live monitors as before.
Deleted is deliberately not matched against Monitor_Status. Whatever row a
deleted monitor left behind is stale - its daemons were stopped when it was
deleted - so filtering on it would drop the monitors we are trying to find.
For the same reason a deleted monitor is reported as Deleted rather than the
status on that row, is drawn with the error dot, is labelled in the list,
and does not get a link to a stream that is not running.
The three queries that hardcoded Deleted=false now share one function, so
the console page, the console ajax endpoint and getFilteredMonitorIds()
cannot disagree about what the filter means. Each passes its own status
column expression, which differ: the ajax endpoint coalesces a WebSite
monitor to Running.
tests/php/test_monitor_status_filter.php covers the sql and the bind value
ordering for all four cases, including a bare string from a cookie written
before the filter became a multi-select. Verified against a live install:
13 deleted and 19 live monitors return 19 with no filter, 13 for Deleted,
and 20 for Deleted plus NotRunning.
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.
Adds a web config option whose contents are rendered on the login view,
between the title and the username and password fields. Useful for a site
notice, an acceptable use or legal warning, or a note identifying which
installation this is when running more than one.
Defaults to empty, and nothing is emitted when it is empty or whitespace, so
existing installs look exactly as they do now.
The text is escaped rather than interpreted. The login page is served before
anyone has authenticated, so it is not somewhere to emit admin-supplied markup,
and no other config value in the skin is output unescaped either. Escaping runs
before nl2br so the only tags reaching the browser are the line breaks we add
ourselves; reversing that order would turn the setting into stored XSS.
Uses the text type, so the Options UI renders a textarea and the value can span
lines. The Config.Value column is already text and options.php normalises CRLF
to LF on save, so no schema change is needed and nl2br sees consistent
newlines.
Guarded with defined() to match the surrounding code, so the view still renders
where the database predates the option.
Styled in base, classic and dark. Text contrast is 7.0:1 light and 7.5:1 dark,
both above WCAG AA, and long unbroken tokens wrap rather than widening the
fixed-width form.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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>
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
zmfilter and the web filter UI generated SQL like
to_days(E.StartDateTime) = to_days('2026-05-06 09:42:56')
which prevents MySQL from using the StartDateTime index, forcing a
full table scan. With many filter daemons against a large Events
table this saturates mysqld and makes the system unresponsive.
Rewrite the SQL generation in ZoneMinder::Filter (Perl) and
ZM\FilterTerm (PHP) so Date/StartDate/EndDate attrs emit range
expressions against the underlying datetime column:
E.StartDateTime >= '2026-05-06 00:00:00'
AND E.StartDateTime < '2026-05-07 00:00:00'
Covers =, !=, >, >=, <, <=, IS, IS NOT, IN, NOT IN, and the
CURDATE()/NOW() values (which use INTERVAL 1 DAY for the upper
bound). EXPLAIN now reports type=range on Events_StartDateTime_idx
where it previously reported type=ALL.
CurrentDate (the constant left-hand expression to_days(NOW()))
keeps its existing form since it does not touch the indexed column.
Add Perl and PHP unit tests under tests/perl/ and tests/php/
exercising the generated SQL across operators.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
singhharsh1708