Looking for other instances of the CronTab.php fatal in the previous commit
found eleven, in ten files, dating from 2019 to 2023. Each is a call to a
logger function from the global namespace without the \ZM\ prefix, so each is
a fatal the moment its branch runs:
web/ajax/console.php, events.php, reports.php, add_monitors.php, status.php
web/includes/actions/snapshot.php (two), donate.php
web/includes/functions.php, session.php
web/views/image.php
They have survived this long because of where they sit. Every one is on an
error or diagnostic path -- an invalid sort order, a zero byte download, a
failed read lock -- so the call is only reached when something has already
gone wrong, and then it turns a handled problem into a blank 500. That is the
shape of the bug on the monitor save: the write had worked, and the thing that
broke was the attempt to log.
Verified before changing any of them that each file is in the global
namespace, does not define the function itself and does not import it, so the
call really does not resolve.
Tests: tests/php/test_logger_calls_qualified.php, 16 checks. It tokenizes
every php file under web/ and reports unqualified calls, with the function
names read out of logger.php rather than listed, so a new one is covered the
day it is added. web/api is skipped as cakephp with its own logging, and
assets as third party.
Php's own tokenizer rather than a regex, because the distinctions matter: a
method call on an object that happens to have an Error() method is fine, as
is a definition, a mention, a qualified call, the same call from inside
namespace ZM, and the name appearing in a comment or a string. There is a
check for each of those, since a scan that quietly matched nothing would pass
the tree-wide check on its own.
Verified the scan fails when the Debug() in CronTab.php is put back.
Ran: tests/php/test_logger_calls_qualified.php (16 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
Saving a time based action on a monitor died with
PHP Fatal error: Uncaught Error: Call to undefined function Debug() in
includes/CronTab.php:280
and the browser showed only "An error occurred while saving data", because
the save goes over ajax and a fatal gives it a 500 with nothing to report.
The action itself had already been written to the database; what failed was
the crontab rebuild that follows it.
Debug(), Info(), Warning() and Error() are defined in includes/logger.php,
which is namespace ZM. CronTab.php is in the global namespace, so they have to
be called as \ZM\Debug(...) -- php resolves a function name against the
current namespace and then the global one, and these are in neither from
there. Four calls were bare: the Debug at the end of cronRebuild and three
Error/Warning calls on failure paths.
Nothing catches this at include time, php -l is happy with it, and the file
works until the one branch holding the call is taken. The Error calls here are
on paths that only run when crontab itself fails, so they could have sat
unnoticed for much longer than the Debug did.
A follow-up commit adds a test that scans for this, and fixes the ten other
places in the tree that have the same bug.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
Two places in ZoneMinder run a command the operator typed: a filter with
"Execute command", and the scheduled tasks added for #4957. Both run it as the
web user through a shell, which is close to shell access on the server. Until
now the only control over that was who may edit, and the answer was anyone
with System rights.
ZM_ALLOWED_COMMANDS names the programs that may be run, comma separated.
Empty is the default and places no restriction, so an upgrade changes nothing
and the filters people already have keep working; setting it narrows both
features at once.
Only the program is matched, because an exact whole-command match could never
work for filters: zmfilter.pl appends the event path to whatever the filter
stored, so no two runs have the same command line. Arguments are therefore
free, which means the program being on the list has to be the whole of the
guarantee -- so while a list is in force a command may not contain the
characters that start another program: semicolon, pipe, ampersand, backtick
and dollar-paren. Without that, "/usr/local/bin/backup.sh; anything" would
pass on the strength of its first word. Redirection is deliberately allowed;
it starts no program, and refusing it would break the ordinary
"... >> /var/log/mine.log".
Refused in three places, because each answers a different question. The
filter and scheduled task editors refuse on save, so the operator is told at
the time rather than by a log line days later. zmfilter.pl refuses at the
moment it runs, because a row can reach the database without passing through
the web ui and because the setting may have been narrowed since. And
cronRebuild leaves a newly disallowed task out of the crontab, so tightening
the setting takes jobs away rather than waiting for somebody to edit each one.
The rule exists twice, in web/includes/Command.php and ZoneMinder::General,
since one is PHP and the other Perl. Rather than hope they stay in step,
tests/php/test_allowed_commands.php puts every case to both and fails if they
disagree. That earned itself immediately: both split the command on
whitespace before handling quotes, so a program path containing a space was
cut in half and compared as the wrong string. The cross-check found it in
both at once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This is the rest of what #4957 asked for -- run Perl, PHP, bash and binaries
on a flexible schedule, from a visual interface -- and it needs no daemon,
because cron is the daemon. 1.39.37 already taught ZoneMinder to rewrite a
marked block in the web user's crontab for scheduled monitor actions while
leaving every line outside that block alone. A CronJobs row is another line in
the same block.
So the parts that would otherwise have to be written and then supervised --
timing, catch-up, restart on boot, running as a known user -- are cron's, and
have been since 1975. What ZoneMinder adds is the table, the page, and the
rule that the crontab is a projection of the table: rebuilt whenever a row
changes, never the other way round, so the two cannot drift.
The schedule uses the same pickers the monitor actions got, reading "Every
Monday at 7pm", with the cron field beside them for anything the pickers
cannot say. The pickers build the spec in the browser so the field fills in as
you choose, and the server builds and validates it again on save, because the
browser is not what keeps a crontab line honest. The two mappings are checked
against each other in tests/js/cron-schedule.test.js, which reads the PHP map
rather than restating it.
The command is not judged on what it does; running things is the point. It is
judged on whether it stays on its own line and means in cron what the operator
meant: a newline is refused because it would end the entry and let the rest
become a line with a schedule of its own, and an unescaped % is refused
because cron stops the command there and feeds the remainder in as stdin,
which would silently run less than was typed. Cron's own escape, \%, is
accepted.
Editing takes System edit rights, and every add, change and delete is logged
with the command. Anyone who can edit here can run commands as the web user,
which is worth saying plainly -- though it is the same trust Filters'
AutoExecuteCmd has carried for years, not a new one.
ServerId NULL means every server writes the job into its own crontab, which
is what a single server install wants without having to say so; naming a
server restricts it to that one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five cron fields are a reasonable thing to store and an unreasonable thing to
put in front of an operator. The editor now reads as the sentence it means:
Every [day v] at [08:00]
Every [Monday v] at [19:00]
Every [weekday (Mon-Fri) v] at [07:30]
Cron stays the stored form, because it is what goes in the crontab and it can
say things the two controls cannot. cronFromParts turns what the editor
collected into a spec and cronToParts turns one back, so a saved schedule
loads into the same controls that made it.
cronToParts is deliberately partial. A spec the controls cannot draw -- every
fifteen minutes, the first of the month, hourly, a pair of weekdays -- comes
back null, and the editor shows it as it stands and saves it back untouched
through a read-only field. Rounding someone's schedule to the nearest thing
the pickers understand would be worse than admitting the editor cannot draw
it.
The time comes from <input type="time">, so it is shown in whatever format
the operator's locale uses; 8am and 08:00 are the same control. The schedule
is still built and validated server side, since the browser is not what keeps
a crontab line honest.
Tested for the round trip over every frequency and a spread of times: what
the editor can build must survive being saved and loaded back, or editing a
row would quietly change when it runs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
that already exist -- lights on, lights off, play a sound -- at set times.
MonitorActions already says what to do and to which device, with triggers for
EventStart, Alarm, Manual and so on. So this adds a trigger rather than a
subsystem: an action whose TriggerOn is 'Schedule' carries a five field cron
spec, and cron runs it.
Cron, rather than a daemon of our own, because the web user owns a crontab it
can rewrite without privilege, and because the line cron runs,
30 22 * * * /usr/bin/zmcontrol.pl --id 24 --command=lightOff
is the same invocation web/includes/Monitor.php already falls back to when no
zmcontrol daemon is listening. A scheduled action therefore takes the path a
manual one does rather than a parallel one of its own. It also means no new
code runs as root, nothing new has to be supervised, and the feature is a
table column plus a crontab block.
Nothing a form can set reaches a shell. The schedule is refused unless
cronScheduleIsValid() accepts it -- five fields, numbers and the usual
*/,-/ only, with newline, carriage return and % rejected outright because
each would end the line or the command. The command itself comes from
MonitorAction::commandFor()'s fixed list and the ids are integers, so the
whole line is assembled from constrained parts. That is a narrower surface
than Filters.AutoExecuteCmd, which has taken an arbitrary command for years.
The crontab is not ours to own, so ZoneMinder rewrites only a block between
two markers and leaves every other line byte for byte. A block whose end
marker is missing, which is what an interrupted write leaves, is treated as
no block at all rather than swallowing everything after it: losing our own
stale block is the safe failure, losing the admin's entries is not. The block
is rebuilt from the table on every save, so the crontab is a projection of
the database rather than a second place to keep the truth.
zmc recognises the new trigger and skips it, so having one does not warn on
every reload, and nothing fires a scheduled action from the capture process.
An action targeting a monitor on another server is left out and logged: cron
runs zmcontrol.pl locally, and the web ui's proxy through the API has no cron
equivalent. Single server installs are unaffected.
The two pieces that can lose data or run the wrong thing are pure, and
tests/php/test_crontab.php covers them: schedule validation including the
injection cases, and splicing a block in, out and over without disturbing the
lines around it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Follow-up hardening to the multi-server control forwarding added in
#5343:
- remoteControlUrl() now forwards an allowlist of known control
parameters (id, control, xge, yge, x, y, scale, preset, newLabel,
action, aid, response) instead of passing everything that is not on a
denylist, so no stray key or cookie-folded value reaches the other
server.
- forwardControlRequest() sets a 15s timeout so an unreachable recording
server can't hang an interactive control click for default_socket_timeout,
and, with ignore_errors, treats a non-2xx response (e.g. an auth-relay
login redirect's HTML) as a failure.
- relayControlReply() refuses to pass a non-JSON body back to the browser
under a JSON content-type.
Extends the test to assert unknown parameters are not forwarded.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
For a monitor recorded by another server, sendControlCommand() put any
zmcontrol command into a monitors/daemonControl URL, which only takes
start, stop and quit. Those still go there; other commands now log why
they were not sent and return false instead of requesting a malformed
URL.
fixes#5342
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
In a multi-server setup a PTZ move, preset or light command sent to one
server for a monitor that another server records never reached the
camera. Monitor::sendControlCommand() passed the zmcontrol option string
to the other server's monitors/daemonControl API, which only starts and
stops zmcontrol.pl, so the request built a URL like
daemonControl/4/ --panspeed=51 ... --command=moveConLeft/zmcontrol.pl.json
and answered "Failed". The web UI's own PTZ buttons send to the
monitor's server and were not affected; clients that send every request
to the server they log in to (zm-py, pyzmNg, zmNinjaNg, aiozoneminder)
and monitor actions whose target is on another server were.
ajax/control.php now hands such a request, as received, to the index.php
of the server that records the monitor (or the action's target), with
the auth relay in place of the caller's credentials, and answers with
that server's reply. That server checks the permissions and builds the
command itself. A request is forwarded at most once.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ZM_Object's constructor takes either an id (which it loads from the database)
or an array, which it trusts as the record's own fields. Several request-fed
call sites passed the raw request value straight to new ZM\Event(), so a request
that sends an array instead of a scalar -- eid[Id]=...&eid[MonitorId]=... -- had
its fields adopted as the event row. The request then chose the MonitorId that
canView() checks and the StorageId/Scheme/Id that Path() is built from.
Impact: view_video.php turned this into an arbitrary file read (forge a viewable
MonitorId, point Path() at any directory, append file=); ajax/events.php let a
forged row pass the per-event permission check so archive/delete acted on a row
the user could not see, and delete ran unlink on the forged Path().
validInt()/validCardinal() already return '' for a non-scalar, so route the
request value through them first; an array becomes '' and ZM\Event loads the
real row (or none). Fixed at the two affected sinks and centrally in
getAffectedIds(), which several action handlers use to build the id list.
Addresses GHSA-6h8f-gx4m-qqw4.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add ZM_PASSWORD_MIN_LENGTH (Options, System; default 8, 0 turns it off).
passwordProblem() refuses a shorter password, so the user page, the API
and both login-page forms apply it alike. Existing passwords keep
working; only a password being set is checked. The length counts
characters, not bytes.
There is no setting for character classes: they push people to
predictable variations (Password1!) and add less than length does.
A database whose Config table doesn't have the option yet (an upgrade
not run with zmupdate.pl -f) gets no minimum. The password inputs carry
minlength so the browser says so before the form is sent; the server
check is the one that counts. The API answers with the English text, as
for the other reasons.
Tests: tests/php/test_default_password.php.
The login page's change form and the first-run page refused admin as a
new password, but the user page (Options, Users, and a user's own page
with ZM_USER_SELF_EDIT) and the API (users/add, users/edit) still stored
it. Every login then refused that account.
Check a new password in one place, passwordProblem() in auth.php, and
call it from all four:
- includes/actions/user.php, for an admin's edit and a self edit: the
save stops with the reason shown on the page. A Password sent as an
array is refused too (it reached password_hash() before).
- api/app/Model/User.php: an "acceptable" validation rule, so add and
edit answer with the reason as the validation message. The API loads
no language file, so passwordProblemText() gives it the English.
- newPasswordProblem() (both login-page forms) checks the confirmation,
then passwordProblem().
Tests: tests/php/test_default_password.php covers passwordProblem() and
passwordProblemText().
A new install starts with authentication on and the admin password admin,
which is refused everywhere. Asking for that password just to replace it is
an extra step, and some installs don't want a login at all. Ask instead.
While the first run is pending (authentication on, the privacy page not
answered yet, builtin auth, and admin still has the password admin), the
login page shows "Set up ZoneMinder" instead of the login form:
- Require a login: choose the admin password; the account is then logged
in and the privacy page follows, as before.
- Don't require a login: ZM_OPT_USE_AUTH is set to 0 in the database.
Not offered when ZM_OPT_USE_AUTH is set in zm.conf or conf.d: the owner
of the box has decided then, and only the password is asked for.
Until one is chosen the web interface, API and streams can't be used: the
password admin is still refused. Whoever opens the page first makes the
choice; before this, whoever typed admin/admin did. The form carries a
nonce kept in the session, like the default-password form, and is exempt
from csrf-magic as action=login is (the login regenerates the session id).
Both choices are audited. Upgrades are not affected: ZM_SHOW_PRIVACY is 0
once the privacy page was answered, and zmupdate.pl doesn't reset it.
Docs: a First Run section in the getting-started guide, and the FAQ and
system options updated to match.
Tests: tests/php/test_first_run.php.
db/zm_create.sql seeds the admin account with the password the user
guide documents (admin), so with authentication on a new install is
still open to anyone who reads the guide.
Refuse that password as a credential and have the web login replace it:
- validateUser() returns false with "This account uses the password
admin, which ZoneMinder does not accept. Log in to the web interface
to set a new one." when the password is right but is admin. The API (login.json and
user=/pass= on any request) answers 401 with that message; the
user=/pass= web parameters are refused the same way.
- On the login page (action=login) the same login shows a form that
asks for the current password again and a new one, with a nonce kept
in the session so another site can't submit it for the user. The new
password must be set, confirmed and not the default. The account is
logged in only once it has been replaced; the change is audited.
That POST skips reCAPTCHA when it carries the form's nonce: the form
follows a login that passed it, and doesn't render the widget.
With ZM_AUTH_RELAY=plain the session keeps the new password.
- zmLoadUser() refuses the default password, so zms, zmu and the RTSP
server don't accept it in user/password parameters either.
The check is on the password typed, so it needs no schema change, covers
existing installs whose admin still has the default password, and
follows the account if it is renamed. A password set to the default
again later is caught the same way. Sessions, tokens and auth hashes
issued before the upgrade keep working until they expire; with
ZM_AUTH_TYPE=remote the web server's login is not affected.
The FAQ now says to force authentication on in conf.d: since 4d7609d36
the daemons apply zm.conf and conf.d values over the database too.
Tests: tests/php/test_default_password.php (the helpers, and that the
seeded hash is the documented default), tests/zm_user_default_password.cpp.
zm.conf and conf.d are set by the system owner and win over the Config
table in PHP, Perl and, since 4d7609d36, the daemons. The Options page
still offered those options for editing and showed the database value,
with a note only when the two differed.
An option a file sets is now shown with the value in effect (a boolean
word read as in configFileValue()), its input disabled, and one line
naming the file ('OptionSetInConfigFile', through translate()). That
line replaces the old "overriden via configuration files" note. A row
whose option a file sets gets no data-requires, so the page script
doesn't enable its input again, and the Requires check of other options
reads the value in effect.
actions/options.php skips options a file sets. A disabled checkbox is
left out of the form, and the action saved a missing boolean as 0, so
without the skip a save of the tab would have reset the Config row; a
hand-made POST can't change it either. The Config row keeps whatever the
admin last saved, which applies again once the file line is removed.
loadConfigFiles() is split out of loadConfig() so the view and the
action can tell which names come from a file and which file. Each entry
carries that file; loadConfig() drops the key before returning, so
configs/viewByName answers as before.
docs/userguide/configfiles.rst describes setting an option in a file.
A boolean option set in zm.conf or conf.d wins over the Config table, and a
person editing a file writes yes or off rather than 1 or 0. The C++ daemons
now read those words (ConfigFileValue, 4d7609d36), but PHP and Perl took the
value as written, and a non-empty string is true in both. So
ZM_STRICT_VIDEO_CONFIG=off in conf.d left the option on in the web UI and the
Perl scripts while zmc turned it off.
Read a file value for a boolean option the same way in all three: yes, true
and on are 1; no, false and off are 0; anything else stays as written. Only
booleans are read this way, so a string option such as a title keeps "Yes".
PHP defines the file values as constants before it reads the Config table,
since it needs the database settings to connect, and only the table says
which options are booleans. So a word now waits until the table has been
read, apart from ZM_DB_* settings, whose words database.php reads itself.
Perl reads the type with each row.
Tested on 1.38's zm138 database with a conf.d file setting
ZM_OPT_FAST_DELETE=yes (0 in the table), ZM_STRICT_VIDEO_CONFIG=off (1)
and ZM_WEB_TITLE=Yes. The installed PHP and Perl gave 'yes', 'off' and
'Yes'; these give '1', '0' and 'Yes'. tests/php/test_config_file_booleans.php
checks the rule.
refs #5289
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
validateUser() compared a plain-text Password column and a MySQL
PASSWORD() hash with ==. Two strings that look like numbers compare as
numbers under ==, so a plain-text password stored as "100" also accepted
"1e2", "100.0" and " 100". Use hash_equals(), which compares the bytes in
constant time; a password that isn't a string is refused.
ZoneMinder stores new passwords as bcrypt, so only accounts whose
Password column was set by hand or carried over from old installs use
these branches. bcrypt still goes through password_verify().
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
do_post_request() throws when Google's siteverify can't be reached or
answers with an HTTP error. The login code didn't catch it, so the login
form got a 500 page and the reason went only to the web server's error
log. Catch it, log an Error naming the URL, and refuse the login the way
a failed challenge is refused. Logins were refused in this case before
too; only the page and the log line change.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With ZM_OPT_USE_GOOG_RECAPTCHA on, a password login was refused only when
siteverify answered success == false together with an error-codes list.
An answer without a success field, one that wasn't JSON, success:false
without error-codes, or success:"false" all let the login go ahead as if
the challenge had been solved.
Move the decision into recaptchaAllowsLogin() and allow only
success === true. The existing allowance for a wrong secret key
(invalid-input-secret) is kept, so a mistyped key still logs an Error
instead of locking everyone out.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
get_auth_relay() builds the query string the web UI appends to stream,
AJAX and multi-server URLs. It encoded the plain relay's password but put
the username in as typed, under all three relays. A username containing
& or = split into extra parameters, and zms ends each value at the next =.
Under plain and hashed that user's streams were refused; under none they
ran as whichever user the rest of the name spelled out.
Encode the username as well. zms decodes user=/username= (UriDecode) and
PHP decodes $_REQUEST, so names without reserved characters are unchanged
on the wire and names with them now arrive whole.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getAuthUser() compared the auth= hash a client presents with the one it
builds using ==. When both strings look like numbers PHP compares them as
numbers, so a computed md5 of the form 0e<digits> equalled any other
0e<digits> string (and "00"), and == also returns at the first byte that
differs. hash_equals() compares the bytes in constant time.
A hash of that form comes up about once in 3*10^8 user-hours, so this is
hardening; ordinary hashes compare exactly as before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dbQuery()'s two error paths for a prepare() or query() that returns false
logged $pdo->errorInfo. $pdo is not defined there, so PHP warned twice and
the message ended without the error. They also read dbLastError() from the
connection only after ZM\Error() had logged; with a database log target that
inserts into Logs through the same connection, which replaces its errorInfo,
so dbLastError() came back as "00000" instead of the error.
Read the error once, before logging, and use it for both. The query() path
also said "Error preparing" for a statement it ran directly.
These paths run when PDO's error mode doesn't throw; dbConnect() sets
ERRMODE_EXCEPTION, so normally the catch below handles the failure.
tests/php/test_database_error_paths.php drives both paths with
ERRMODE_SILENT and checks for no PHP warning and the real error text.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dbConnect() turns emulated prepares off, so every query of the web UI, the
ajax calls and the includes the API runs (config, servers, users,
permissions) is a server-side prepare: prepare, execute and close, three
round trips to the database where one would do. From PHP 8.1, pdo_mysql
returns integers and floats as PHP ints and floats with emulated prepares
too, so values keep the types they have now. Emulate prepares there and keep
server-side prepares on older PHP, where every value would become a string.
Emulated prepares quote parameters bound as strings, which breaks LIMIT ?.
dbQuery() now binds integer values as integers (everything else as strings,
as execute() did), and the log and watch ajax calls cast their validated
offset and limit to int. With emulation, PDO would also run several
statements given in one string; MYSQL_ATTR_MULTI_STATEMENTS is turned off so
that is refused as before. CakePHP's own connection is not changed: it
already uses PDO's default, emulated prepares.
tests/php/test_database_emulated_prepares.php compares every column of every
table read through both kinds of prepare (types and values), checks
aggregates, LIMIT with positional and named integers, string and null
parameters, and that two statements in one string are refused.
ZM_Object's constructor treats an array as the object's row, and validInt()
and validCardinal() returned an array as they got it (preg_replace() does).
So wherever a request id reached `new ZM\X(...)`, raw or through those
validators, a crafted array built an object whose Id, UserId or MonitorId
the sender chose. The permission check then looked at those, and the delete
or update that followed used the Id. A user with Events=View deleted another
user's filter by posting Id[Id]=<filter>&Id[UserId]=<self> to the filter
delete action; the same shape reached events (MonitorId), users, reports,
snapshots and storage.
validInt() and validCardinal() now give '' for anything that isn't a single
value. The places that built an object from a raw $_REQUEST value (the
control and event ajax handlers, the event, filter, report, snapshot and
storage actions, view_hls, and the download and event detail modals) take
the id through validCardinal().
tests/php/test_request_ids_scalar.php covers the validators with arrays and
fails if web code builds an object straight from $_REQUEST, $_GET or $_POST.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
Storage.DiskSpace defaults to NULL on fresh installs, and zmc's
DiskSpace = DiskSpace + N leaves it NULL. Storage::event_disk_space()
treated NULL as uncached and ran SUM(DiskSpace) over Events on every
navbar render and status refresh, taking seconds per page on large
Events tables.
Write the computed sum back to the Storage row (only while it is still
NULL, so a value set by zmaudit is not overwritten). Later page loads use
the stored value and zmc keeps it current. COALESCE the sum so a storage
area with no events stores 0 instead of NULL.
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>
