When ZM_AUTH_HASH_SECRET is unusable zmu refuses tokens, so
zmuCredentials() falls back to what zmu got before. With
ZM_AUTH_RELAY=plain the API's alarm action used to pass the password
from the request (pass= or password=); the fallback read only the
session, which an API call without a stateful login doesn't have, so
zmu refused the call (rc 255). zmuCredentials() takes the caller's
password first again. With a usable secret it is never used.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 5ca19c2703ea9211fe040991305ce9b0a873faff)
getZmuCommand() and the API's alarm action put the user's credentials on
zmu's command line, where every local user can read them (ps,
/proc/<pid>/cmdline): an auth hash with the default hashed relay, the
plaintext password with ZM_AUTH_RELAY=plain. The API also wrote the
whole command to the debug log. With the default ZM_AUTH_HASH_IPS=0 that
hash is accepted as an auth= login from any address for ZM_AUTH_HASH_TTL
hours.
zmuCredentials() builds them in one place: with a usable
ZM_AUTH_HASH_SECRET, an access token for the user that expires after a
minute (zmu -T, accepted with every relay). Without the secret zmu
refuses tokens, so it gets what it did before. The API logs the command
without the credentials.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 32bc2b3fee87b752590ba6f210403afd9222d9ff)
ZmErrorHandler::redact() matched token=, auth=, pass= and password=
only. A client that sends one of them as an array (token[]=x, which
Cake logs as token%5B0%5D=x) still puts the value in the log, so the
name may now carry [..] or %5B..%5D before the =.
An empty value is no longer replaced: token= stays token=, so the log
still shows a client that sent no credential.
Tests: prefixes (authority, tokenize, password2, pass_hint, token_type,
passwd) are not touched, arrays are redacted, empty stays empty.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 4789df0a0433d51e12e19cd21df5ac18e7c6305f)
Cake logs every uncaught exception, 4xx included, with "Request URL:"
and the full query string. API clients may authenticate with ?token=,
?auth= or ?user=&pass=, so a 401, 404 or 500 wrote a working access
token, auth hash or password to cake_error.log and error.log in
ZM_LOGDIR, both world-readable.
ZmErrorHandler extends Cake's ErrorHandler and replaces the values of
token, auth, pass and password with [redacted] in the logged message.
core.php uses it for both errors and exceptions.
Credentials in the query string still reach web server and proxy
access logs; that is outside the API code.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit b80568847d1af9eb88ed3eefee31691229c245b7)
The snapshot action's create step had no permission check: any
logged-in user could POST view=snapshot&action=create and have the
server force an alarm (zmu -a) on any monitor id, record an event and
archive it, and add a Snapshots row. zmu itself only asks for Stream
view on a monitor it can access, so a stock viewer forced alarms; a
user without Stream still added Snapshots rows.
Create now needs Snapshots edit, uses only monitors the user can view,
and creates nothing when none is left. An unknown or non-numeric
monitor id used to call Error(), which is PHP's Error class, not a
function, and ended the request with a fatal error; it is now logged
with ZM\Error and skipped.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 910961d744b26da577f393ec17ef084ddcb474a5)
The ajax/event.php actions rename, eventdetail, archive, unarchive and
delete checked Events edit only, and took any event id: a user with
Events edit restricted to some monitors could rename, annotate,
archive, unarchive or delete events of monitors they can't see. The
web event action and the API already check the event's monitor.
Load the event once for every Edit-level action in ajax/event.php and
require visibleMonitor() on its monitor, as addtag/removetag do; a
missing and a hidden event get the same answer (delete keeps its "Event
not found." reply). getselectedtags lists no tags of an event on a
hidden monitor.
includes/actions/eventdetail.php checks each event's monitor since
13138a0af, but passes markEids entries through validInt(), which keeps
an array an array, and new ZM\Event() takes an array as the row itself
without loading it: markEids[0][Id]=<hidden event>&markEids[0][MonitorId]=
<visible monitor> passes canEdit() and updates the hidden event. Take
only scalar ids there.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit afe6452e421b10a9738d243bb5092cbbb2c4d0a6)
includes/actions/settings.php ran for Control view, while the settings
modal only enables its sliders with Control edit. zmu itself refuses
users without Monitors edit and then prints nothing; the action still
wrote that output into Monitors (Brightness, Contrast, Hue, Colour),
which strict SQL mode rejects with an error and a non-strict server
would store as 0.
Require Control edit (with the monitor checked by canView, since
canEdit('Control', $mid) also demands Monitors edit), and update
Monitors only when zmu reports four integers.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 49247d3bf89a78aeef3624bb33827d0b93e4a9b6)
The video view generates videos (Events view, ajax/event.php action
video) and lists them under Existing Video Files with a Delete link.
Requiring Events edit for every delete also took that from viewers,
although only the event's recording needs protecting: a generated
video can be made again from it.
Treat as the recording the event's DefaultVideo, incomplete.<ext> (zmc's
name while recording) and <Id>-video.* (its final name), and require
Events edit for those only. Show the Delete link per file by the same
rule.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit a7c4b01a254bf006eec40cf1c9aa8bc82c92e6f3)
The file list numbered its rows with a counter that skipped empty
files, while deleteIndex, downloadIndex and showIndex look the number up
as the key in $videoFiles. With an empty file earlier in the directory
(for example a video generation that failed) every later row's Download,
Show and Delete acted on another file, Delete included. Use the array
key.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit ca1b9311edab1a6eca6d61864a0e03f2796f3b77)
addtag and removetag took any event id: a user with Events edit
restricted to some monitors could tag or untag events on monitors they
can't see. Filters select events by tag (a purge filter can keep tagged
events, or delete them), so that reaches events the user has no access
to. Load the event and require visibleMonitor() on its monitor; a
missing and a hidden event get the same error. removetag now validates
its ids like addtag.
ajax/tags.php listed tags only for Events view since the previous
commit, but the event view also serves Snapshots view users and loads
the tag list; accept either, as ajax/event.php does.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit 5b9df458d55d3491086dbbe25dbac2d3f3db1264)
Three event write paths were reachable with Events view only:
- ajax/event.php handled addtag and removetag in its canView('Events')
block. removetag also deletes the Tag row once no event uses it.
- ajax/tags.php had no permission check, so createtag inserted Tags for
any logged-in user, and getavailabletags listed them.
- views/video.php opens with canView('Events') and unlinks
$videoFiles[deleteIndex]. ZM_FFMPEG_FORMATS defaults to "mp4* ...", so
that list includes the event's own recording: a viewer could delete
the recorded video of every event on the monitors they can see.
Move addtag and removetag into the canEdit('Events') block next to
rename and archive. In tags.php require canView('Events') for the file
and canEdit('Events') for createtag. In video.php require
canEdit('Events') for deleteIndex, skip indexes that are not in the
list, and only show the Delete link to users who can use it. The event
view disables the tag input and the tag-and-step buttons for users
without Events edit.
tests/php/test_event_writes_require_events_edit.php drives the three
files with Events None, View and Edit and checks the SQL they send and
whether the video file survives.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
(cherry picked from commit d935293c527c4543a3afbdad1afdddc774c63ea2)
Two ways a System=View account could read secrets through the Config API.
The oracle: ConfigsController::index() passed the caller's named filter to
buildFilter() and returned the matched rows. redactSecret() blanked a secret
Value in the response but not in the WHERE clause, so a filter such as
Name:ZM_AUTH_HASH_SECRET/Value LIKE:a% returned the row for a correct prefix
and nothing otherwise - the row count leaked the secret a character at a time,
and that secret signs auth tokens. index() now detects a filter on the Value
column and drops the secret rows from the result, so their presence reveals
nothing; a filter on any other column is unaffected.
The flags: ZM_JANUS_SECRET, ZM_UPLOAD_PASS and ZM_MQTT_PASSWORD held secrets
but were not marked Private, so the API returned them to System=View and
skin.js.php wrote them into every page as JavaScript constants. Mark them
private in ConfigData.pm.in (fresh installs) and add a Config UPDATE to the
untagged 1.39.41 migration (existing installs). No web page reads the three;
zmfilter, zmc and the Janus integration read them server side.
Tested: tests/php/test_config_value_filter.php (11 cases) covers the Value
detection, including Model.Value, case, and the DefaultValue non-match.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cache_bust() in web/includes/functions.php symlinks every css and js file to a
name with the source file's mtime in it, so each time a source changes a new
link appears and the old one is left behind. Nothing has ever removed them.
The install this was written on had 8346 entries going back to April 2020,
1562 of them pointing at files that no longer exist -- one of which was the
log.js deleted in the log panel refactor, still being served because the skin
auto-includes views/js/<view>.js whenever that file is present. The directory
inode had grown to 823KB, which every lookup in that directory has to scan.
misc/zoneminder-tmpfiles.conf.in did not mention the directory at all, so
systemd neither created nor cleaned it. Add it as a d entry with a 30d age,
which the daily systemd-tmpfiles-clean.timer then acts on.
On the age, and why it is generous rather than a day or two: a symlink's mtime
and ctime are fixed when it is created, and neither being served nor having
its target read refreshes them -- verified here that traversing a symlink does
not update its atime either. So an age removes entries whether or not they are
in use, rather than only the unused ones. That is safe, because the directory
is derived and not authoritative: a link removed while still wanted is
recreated by the next page render, so the worst case is one missing asset on a
page that was already open. 30d keeps about a month of source versions for a
few recreated symlinks a month, where a short age would churn the whole set
repeatedly for no further benefit.
d and not D: D empties the directory at boot, which would throw away every
current entry on every reboot.
Tests: tests/php/test_tmpfiles_conf.php, 12 checks. Nothing exercises this
file during a build or a test run, and a mistake in it is quiet in both
directions -- a malformed line is skipped with a log message nobody reads, and
a missing age field just means a directory grows forever, which is how this
one got to 8346 entries. So the fields are parsed and checked, and the entry
is checked to name the same cmake variable that config.php.in defines
ZM_DIR_CACHE from, since if those two drift systemd would clean a directory
nothing writes to and nothing would report it.
The last check hands the file to systemd-tmpfiles itself, with every path
rewritten into a temporary directory first so that running a clean cannot
touch anything real. That check skips where systemd-tmpfiles is not installed.
Confirmed separately that a d entry with an age does remove symlinks, which is
what this directory is made of.
Verified the checks fail when the age is dropped, when d becomes D, when the
entry names a variable config.php.in does not use, and on an age systemd would
reject.
Ran: tests/php/test_tmpfiles_conf.php (12 passed), the rest of tests/php and
tests/js, npx eslint . (clean), utils/check-homoglyphs.py (clean).
test_bandwidth_clamp.php fails on this box for an unrelated reason: it cannot
read /etc/zm/conf.d/03custom.conf and the permission warning lands in its
captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
The known-issues list recorded each finding as path:line, and a rebase onto a
master carrying 1677470af1 inserted four lines above one of them in
monitor.php. The finding moved from 1483 to 1487, so it read as both a new
offender and a missing list entry, and the test failed on code nobody had
touched.
A line number is not a stable identity for a finding. Key on the file and the
variable being assigned instead, which survives anything inserted above and
still names the offender uniquely -- there is no file here assigning the same
variable twice with a cut-short expression. The line is still reported, since
it is what someone needs in order to go and look, but it is not what gets
compared.
Verified the test still catches a new instance, and that shifting every line
in a listed file no longer breaks it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
A saved time based action listed its schedule as "0 21 * * *" rather than
"Every day at 21:00". The cron parser was not at fault; cronToParts('0 21 * *
*') returns the right parts. The fault was one operator:
$drawable = ($action->Schedule() === '') or ($schedule_parts !== null);
"or" binds looser than "=", so this parses as ($drawable = ($action->Schedule()
=== '')) or (...). The right-hand clause is evaluated and thrown away, and
$drawable gets only the left one -- false for any schedule that is actually
set, which is every saved action. So the editor took every saved schedule for
one it could not draw and fell back to the read-only cron field, which is the
branch that exists for genuinely undrawable specs like "*/5 * * * *".
Changed to ||, which binds tighter than "=" and does what the line looks like
it does.
Tests: tests/php/test_assignment_precedence.php, 21 checks. It tokenizes every
php file under web/ and reports assignments whose right-hand side has an
unbracketed "and", "or" or "xor" -- tokenized rather than grepped so that the
many safely bracketed ones, in conditions and ternaries, are not reported.
There is a self-check for each distinction it has to make, since a scan that
quietly matched nothing would pass the tree-wide check on its own. Verified it
fails when the reported line is put back, and when a new instance is added
elsewhere.
The scan found six more in the rest of the tree, which the test carries as a
list of open questions rather than as exemptions. Two are the deliberate
"$fp = fopen(...) or error_log(...)" idiom and are correct. The other four are
this same bug, and each changes behaviour to fix, so none is swept up here --
in particular filter.php:401 assigns only $filter->Id() to $canDelete, so
delete is offered for any existing filter whether or not the user may edit it,
and that is access control rather than something to guess at. Anything not on
the list fails the test, so a new one cannot be added, and the list is checked
in both directions so it shrinks as the entries are dealt with.
Ran: tests/php/test_assignment_precedence.php (21 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
Looking for other instances of the CronTab.php fatal in the previous commit
found eleven, in ten files, dating from 2019 to 2023. Each is a call to a
logger function from the global namespace without the \ZM\ prefix, so each is
a fatal the moment its branch runs:
web/ajax/console.php, events.php, reports.php, add_monitors.php, status.php
web/includes/actions/snapshot.php (two), donate.php
web/includes/functions.php, session.php
web/views/image.php
They have survived this long because of where they sit. Every one is on an
error or diagnostic path -- an invalid sort order, a zero byte download, a
failed read lock -- so the call is only reached when something has already
gone wrong, and then it turns a handled problem into a blank 500. That is the
shape of the bug on the monitor save: the write had worked, and the thing that
broke was the attempt to log.
Verified before changing any of them that each file is in the global
namespace, does not define the function itself and does not import it, so the
call really does not resolve.
Tests: tests/php/test_logger_calls_qualified.php, 16 checks. It tokenizes
every php file under web/ and reports unqualified calls, with the function
names read out of logger.php rather than listed, so a new one is covered the
day it is added. web/api is skipped as cakephp with its own logging, and
assets as third party.
Php's own tokenizer rather than a regex, because the distinctions matter: a
method call on an object that happens to have an Error() method is fine, as
is a definition, a mention, a qualified call, the same call from inside
namespace ZM, and the name appearing in a comment or a string. There is a
check for each of those, since a scan that quietly matched nothing would pass
the tree-wide check on its own.
Verified the scan fails when the Debug() in CronTab.php is put back.
Ran: tests/php/test_logger_calls_qualified.php (16 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
Pin When to 11em and raise Schedule from 24em to 28em.
Both columns are sized by a select, and a select is as wide as its longest
option whether or not the column is. When's is "Manually triggered", which
needs about 11em; it had been sharing the leftover space three ways with
Device and Does and taking more than that. Schedule has to fit "Every
<frequency> at <time>" on one line, which comes to about 25em -- so the 24em
it was given was short, and the frequency select was the thing being
truncated.
The space comes out of Device and Does, which now divide the remainder
between them. Device is left loose on purpose: monitor names are the only
contents in this table of unpredictable length, so it is the right column to
absorb whatever is spare.
Worth knowing for later: Schedule is wide almost entirely because of one
frequency option, "weekend day (Sat & Sun)", which sets that select's width
by itself. Rewording just that label would give several em back to the rest
of the table. Not done here, since it is a visible label and not mine to
change.
Tests: tests/php/test_monitor_actions_columns.php, now 30 checks. Added a
check per select column that it fits its longest option, with the option keys
read out of the view and their text out of the language file so neither is
restated. That is the measurement these widths were chosen from, so it is the
one worth keeping: a label reworded longer would start truncating with
nothing to say it had.
The estimate for Schedule initially cleared 24em, which would have made it
too generous to catch what was reported. The two terms it was missing are
real and identifiable -- .table-sm's padding, and the collapsed space between
four controls written on separate source lines -- and together they come to
about an em and a half, which is roughly the difference complained of. There
is a check that the estimate still fails 24em, labelled as the calibration it
is rather than as a property of the css.
Verified the checks fail with Schedule back at 24em (1), with When too narrow
for its longest option (1), and with Device pinned so nothing is left loose
(3).
Ran: tests/php/test_monitor_actions_columns.php (30 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
4em was set from what the cells hold -- one checkbox -- and that is the wrong
thing to measure. A fixed layout will not widen a column to fit its header,
it lets the text run on into the next one, and skin.css renders every th
uppercase at 0.8em. So what has to fit is "ENABLED" plus .table-sm's padding,
which at 4em fit so exactly that it touched the When header. 5.5em leaves a
clear gap.
Tests: tests/php/test_monitor_actions_columns.php, now 21 checks. Dropped the
check that Enabled was the narrowest column "being one checkbox", since that
is the reasoning that produced the bug: "ENABLED" is a wider word than
"SOUND" whatever sits in the cells below them, so there is no ordering to
assert between the two. In its place each narrow column is checked against an
estimate of its own header's width, calibrated so that the 4em this started
with fails and 5.5em passes. The header text is read out of the language file
rather than written into the test, so a translation long enough to break the
layout fails too -- under a fixed layout it really would break it.
Ran: tests/php/test_monitor_actions_columns.php (21 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
ul.form indents every row by 300px to make a gutter for its label, and pulls
the label back into that gutter with an equal negative margin. It is a neat
trick for a labelled field and wrong for anything else: the actions tab's
rows have no label, so the table started 300px in with the space beside it
left empty.
Add a noLabel class that cancels the indent, and put it on the four rows of
the tab that have no label -- the table, the help text above and below it,
and the no-capable-devices warning that replaces the lot. The mobile rules
already drop the indent for every row, so there is no counterpart to add
there.
This is also why the Schedule column felt cramped. The fixed widths in
6eb390007a divide whatever the table is given, and the table was being given
300px less than the page; those widths are unchanged and now divide the whole
of it. Only one table on this view sits in a ul.form, so nothing else was
affected.
Tests: tests/php/test_monitor_actions_columns.php grows to 19 checks. Which
class cancels the indent is read out of the css rather than named in the test,
since a drift between the two would bring the indent back without erroring.
It also checks that no row carrying the class has a label, because a label in
a row with no gutter to be pulled into would hang off the left edge -- the
same mistake in the other direction. Verified the checks fail when the
table's row loses the class (2), when the css renames it and the view does
not (2), and when the class is put on a labelled row (1).
Ran: tests/php/test_monitor_actions_columns.php (19 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
The Enabled column already asked for 5em, but under the default automatic
table layout a width on a cell is only a suggestion: the browser sizes each
column from its contents and may hand one more than it asked for. So Enabled
kept claiming space on the strength of its header text while holding a single
checkbox, and Schedule -- which has to fit "Every weekend day (Sat & Sun) at
08:00" across a label, a select and a time input -- was the column that got
squeezed.
Make the layout fixed, which binds the declared widths exactly. Enabled and
Sound are pinned to what a checkbox and a number need, Schedule is pinned to
what its sentence needs, and When, Device and Does divide what is left: those
three are selects, and a select that runs short truncates its option text
rather than breaking the row. zone.css already lays a table out this way, so
it is not a new approach in the skin.
A fixed layout will not shrink a control to fit its column, so the selects
and inputs are capped at the column width. Without that a long monitor name
in Device would spill over Does.
Tests: tests/php/test_monitor_actions_columns.php, 13 checks. The widths are
addressed positionally, as :nth-child(1), (3) and (6), because the cells carry
no classes -- which is fine until a column is inserted, when every pin after
it silently moves one column right and nothing errors. So the css is read for
which positions it pins, the view for what is in each position, and the two
are compared. Verified the checks fail when table-layout is removed (1
failure) and when a column is inserted before Schedule (6).
Not verified in a browser: the install under /usr/share/zoneminder/www is a
copy rather than a symlink of this tree, so the em values are a judgement
about what each column holds rather than something measured on screen.
Ran: tests/php/test_monitor_actions_columns.php (13 passed), the rest of
tests/php and tests/js, npx eslint . (clean), utils/check-homoglyphs.py
(clean). test_bandwidth_clamp.php fails on this box for an unrelated reason:
it cannot read /etc/zm/conf.d/03custom.conf and the permission warning lands
in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JpiSWBmtQkR5bcgpHWY4ME
The scheduled tasks view carried an inline form above the list, so the page
was two things at once: the form was always on screen whether or not anyone
was editing, and the list it belonged to was pushed below the fold.
Make the view the list, and move adding and editing into a modal built by
web/ajax/modals/cronjob.php, following the pattern the Options modals use.
The Add button and the task name both open it; the modal posts back to
?view=cron, so includes/actions/cron.php still does the validating and the
crontab rebuild and did not have to change.
Two things follow from the form being built over ajax rather than with the
page. The modal emits getCSRFinputHTML() itself, since nothing else wraps it.
And cron.js binds the schedule pickers after the html is inserted instead of
on DOMContentLoaded: the controls do not exist until then, and a fresh set
arrives each time the modal opens.
While moving the When row, give the pickers a placeholder when they cannot
draw the stored schedule. Previously an existing task set to something like
"*/5 * * * *" rendered the pickers at their default, so the page showed
"Every day at 03:00" in large friendly controls beside a cron field reading
"*/5 * * * *" -- two different answers at once, with the wrong one the more
prominent. Adding a task still defaults to daily at 03:00, which is a default
rather than a contradiction.
Tests: tests/php/test_cron_modal.php, 42 checks. The field names are now a
contract between files that do not mention each other, so each side is parsed
out of its own source and compared rather than restated. The three states of
the When row are checked by rendering the modal against stubs, since which
picker is selected and whether the cron field is open are decisions the php
makes. Verified the new checks fail when the placeholder, a field name or the
csrf input is removed.
Ran: tests/php/test_cron_modal.php (42 passed), test_crontab.php (88 passed),
test_allowed_commands.php (31 passed), tests/js/cron-schedule.test.js (12
passed), the rest of tests/php and tests/js, npx eslint . (clean),
utils/check-homoglyphs.py (clean). test_bandwidth_clamp.php fails on this box
for an unrelated reason: it cannot read /etc/zm/conf.d/03custom.conf and the
permission warning lands in its captured output.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two places in ZoneMinder run a command the operator typed: a filter with
"Execute command", and the scheduled tasks added for #4957. Both run it as the
web user through a shell, which is close to shell access on the server. Until
now the only control over that was who may edit, and the answer was anyone
with System rights.
ZM_ALLOWED_COMMANDS names the programs that may be run, comma separated.
Empty is the default and places no restriction, so an upgrade changes nothing
and the filters people already have keep working; setting it narrows both
features at once.
Only the program is matched, because an exact whole-command match could never
work for filters: zmfilter.pl appends the event path to whatever the filter
stored, so no two runs have the same command line. Arguments are therefore
free, which means the program being on the list has to be the whole of the
guarantee -- so while a list is in force a command may not contain the
characters that start another program: semicolon, pipe, ampersand, backtick
and dollar-paren. Without that, "/usr/local/bin/backup.sh; anything" would
pass on the strength of its first word. Redirection is deliberately allowed;
it starts no program, and refusing it would break the ordinary
"... >> /var/log/mine.log".
Refused in three places, because each answers a different question. The
filter and scheduled task editors refuse on save, so the operator is told at
the time rather than by a log line days later. zmfilter.pl refuses at the
moment it runs, because a row can reach the database without passing through
the web ui and because the setting may have been narrowed since. And
cronRebuild leaves a newly disallowed task out of the crontab, so tightening
the setting takes jobs away rather than waiting for somebody to edit each one.
The rule exists twice, in web/includes/Command.php and ZoneMinder::General,
since one is PHP and the other Perl. Rather than hope they stay in step,
tests/php/test_allowed_commands.php puts every case to both and fails if they
disagree. That earned itself immediately: both split the command on
whitespace before handling quotes, so a program path containing a space was
cut in half and compared as the wrong string. The cross-check found it in
both at once.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This is the rest of what #4957 asked for -- run Perl, PHP, bash and binaries
on a flexible schedule, from a visual interface -- and it needs no daemon,
because cron is the daemon. 1.39.37 already taught ZoneMinder to rewrite a
marked block in the web user's crontab for scheduled monitor actions while
leaving every line outside that block alone. A CronJobs row is another line in
the same block.
So the parts that would otherwise have to be written and then supervised --
timing, catch-up, restart on boot, running as a known user -- are cron's, and
have been since 1975. What ZoneMinder adds is the table, the page, and the
rule that the crontab is a projection of the table: rebuilt whenever a row
changes, never the other way round, so the two cannot drift.
The schedule uses the same pickers the monitor actions got, reading "Every
Monday at 7pm", with the cron field beside them for anything the pickers
cannot say. The pickers build the spec in the browser so the field fills in as
you choose, and the server builds and validates it again on save, because the
browser is not what keeps a crontab line honest. The two mappings are checked
against each other in tests/js/cron-schedule.test.js, which reads the PHP map
rather than restating it.
The command is not judged on what it does; running things is the point. It is
judged on whether it stays on its own line and means in cron what the operator
meant: a newline is refused because it would end the entry and let the rest
become a line with a schedule of its own, and an unescaped % is refused
because cron stops the command there and feeds the remainder in as stdin,
which would silently run less than was typed. Cron's own escape, \%, is
accepted.
Editing takes System edit rights, and every add, change and delete is logged
with the command. Anyone who can edit here can run commands as the web user,
which is worth saying plainly -- though it is the same trust Filters'
AutoExecuteCmd has carried for years, not a new one.
ServerId NULL means every server writes the job into its own crontab, which
is what a single server install wants without having to say so; naming a
server restricts it to that one.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Five cron fields are a reasonable thing to store and an unreasonable thing to
put in front of an operator. The editor now reads as the sentence it means:
Every [day v] at [08:00]
Every [Monday v] at [19:00]
Every [weekday (Mon-Fri) v] at [07:30]
Cron stays the stored form, because it is what goes in the crontab and it can
say things the two controls cannot. cronFromParts turns what the editor
collected into a spec and cronToParts turns one back, so a saved schedule
loads into the same controls that made it.
cronToParts is deliberately partial. A spec the controls cannot draw -- every
fifteen minutes, the first of the month, hourly, a pair of weekdays -- comes
back null, and the editor shows it as it stands and saves it back untouched
through a read-only field. Rounding someone's schedule to the nearest thing
the pickers understand would be worse than admitting the editor cannot draw
it.
The time comes from <input type="time">, so it is shown in whatever format
the operator's locale uses; 8am and 08:00 are the same control. The schedule
is still built and validated server side, since the browser is not what keeps
a crontab line honest.
Tested for the round trip over every frequency and a spread of times: what
the editor can build must survive being saved and loaded back, or editing a
row would quietly change when it runs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
that already exist -- lights on, lights off, play a sound -- at set times.
MonitorActions already says what to do and to which device, with triggers for
EventStart, Alarm, Manual and so on. So this adds a trigger rather than a
subsystem: an action whose TriggerOn is 'Schedule' carries a five field cron
spec, and cron runs it.
Cron, rather than a daemon of our own, because the web user owns a crontab it
can rewrite without privilege, and because the line cron runs,
30 22 * * * /usr/bin/zmcontrol.pl --id 24 --command=lightOff
is the same invocation web/includes/Monitor.php already falls back to when no
zmcontrol daemon is listening. A scheduled action therefore takes the path a
manual one does rather than a parallel one of its own. It also means no new
code runs as root, nothing new has to be supervised, and the feature is a
table column plus a crontab block.
Nothing a form can set reaches a shell. The schedule is refused unless
cronScheduleIsValid() accepts it -- five fields, numbers and the usual
*/,-/ only, with newline, carriage return and % rejected outright because
each would end the line or the command. The command itself comes from
MonitorAction::commandFor()'s fixed list and the ids are integers, so the
whole line is assembled from constrained parts. That is a narrower surface
than Filters.AutoExecuteCmd, which has taken an arbitrary command for years.
The crontab is not ours to own, so ZoneMinder rewrites only a block between
two markers and leaves every other line byte for byte. A block whose end
marker is missing, which is what an interrupted write leaves, is treated as
no block at all rather than swallowing everything after it: losing our own
stale block is the safe failure, losing the admin's entries is not. The block
is rebuilt from the table on every save, so the crontab is a projection of
the database rather than a second place to keep the truth.
zmc recognises the new trigger and skips it, so having one does not warn on
every reload, and nothing fires a scheduled action from the capture process.
An action targeting a monitor on another server is left out and logged: cron
runs zmcontrol.pl locally, and the web ui's proxy through the API has no cron
equivalent. Single server installs are unaffected.
The two pieces that can lose data or run the wrong thing are pure, and
tests/php/test_crontab.php covers them: schedule validation including the
injection cases, and splicing a block in, out and over without disturbing the
lines around it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
monitors/alarm/command:on now takes an optional cause (query or POST data)
and passes it to zmu --cause, so the event records it in place of
"Forced Web". Without a cause nothing changes.
The cause is stored as the event's Cause and in its Notes, so alarmCause()
keeps it to plain text: letters, digits, spaces and plain punctuation, cut
to the 31 bytes zmu stores on a character boundary, falling back to the
default when nothing is left. The value is quoted with escapeshellarg()
for the zmu command line.
Test: tests/php/test_monitor_alarm_cause.php
fixes#5348
Follow-up hardening to the multi-server control forwarding added in
#5343:
- remoteControlUrl() now forwards an allowlist of known control
parameters (id, control, xge, yge, x, y, scale, preset, newLabel,
action, aid, response) instead of passing everything that is not on a
denylist, so no stray key or cookie-folded value reaches the other
server.
- forwardControlRequest() sets a 15s timeout so an unreachable recording
server can't hang an interactive control click for default_socket_timeout,
and, with ignore_errors, treats a non-2xx response (e.g. an auth-relay
login redirect's HTML) as a failure.
- relayControlReply() refuses to pass a non-JSON body back to the browser
under a JSON content-type.
Extends the test to assert unknown parameters are not forwarded.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
In a multi-server setup a PTZ move, preset or light command sent to one
server for a monitor that another server records never reached the
camera. Monitor::sendControlCommand() passed the zmcontrol option string
to the other server's monitors/daemonControl API, which only starts and
stops zmcontrol.pl, so the request built a URL like
daemonControl/4/ --panspeed=51 ... --command=moveConLeft/zmcontrol.pl.json
and answered "Failed". The web UI's own PTZ buttons send to the
monitor's server and were not affected; clients that send every request
to the server they log in to (zm-py, pyzmNg, zmNinjaNg, aiozoneminder)
and monitor actions whose target is on another server were.
ajax/control.php now hands such a request, as received, to the index.php
of the server that records the monitor (or the action's target), with
the auth relay in place of the caller's credentials, and answers with
that server's reply. That server checks the permissions and builds the
command itself. A request is forwarded at most once.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#5288 added an isDefaultPassword() call to validateUser(). The comparison
test from #5294 lifts validateUser() out of auth.php into its own
namespace without it, so since both merged the test stops at its first
check: "Call to undefined function ValidateUserTest\isDefaultPassword()".
Each PR passed on its own branch.
Lift isDefaultPassword() and the ZM_DEFAULT_PASSWORD* constants from
auth.php as well, rather than copying them, and check what #5288 added to
validateUser(): the default password is refused with its own message,
plain or MySQL-hashed, and a wrong password still reads as one. Test only;
no change to ZoneMinder.
refs #5287
Add ZM_PASSWORD_MIN_LENGTH (Options, System; default 8, 0 turns it off).
passwordProblem() refuses a shorter password, so the user page, the API
and both login-page forms apply it alike. Existing passwords keep
working; only a password being set is checked. The length counts
characters, not bytes.
There is no setting for character classes: they push people to
predictable variations (Password1!) and add less than length does.
A database whose Config table doesn't have the option yet (an upgrade
not run with zmupdate.pl -f) gets no minimum. The password inputs carry
minlength so the browser says so before the form is sent; the server
check is the one that counts. The API answers with the English text, as
for the other reasons.
Tests: tests/php/test_default_password.php.
The login page's change form and the first-run page refused admin as a
new password, but the user page (Options, Users, and a user's own page
with ZM_USER_SELF_EDIT) and the API (users/add, users/edit) still stored
it. Every login then refused that account.
Check a new password in one place, passwordProblem() in auth.php, and
call it from all four:
- includes/actions/user.php, for an admin's edit and a self edit: the
save stops with the reason shown on the page. A Password sent as an
array is refused too (it reached password_hash() before).
- api/app/Model/User.php: an "acceptable" validation rule, so add and
edit answer with the reason as the validation message. The API loads
no language file, so passwordProblemText() gives it the English.
- newPasswordProblem() (both login-page forms) checks the confirmation,
then passwordProblem().
Tests: tests/php/test_default_password.php covers passwordProblem() and
passwordProblemText().
A new install starts with authentication on and the admin password admin,
which is refused everywhere. Asking for that password just to replace it is
an extra step, and some installs don't want a login at all. Ask instead.
While the first run is pending (authentication on, the privacy page not
answered yet, builtin auth, and admin still has the password admin), the
login page shows "Set up ZoneMinder" instead of the login form:
- Require a login: choose the admin password; the account is then logged
in and the privacy page follows, as before.
- Don't require a login: ZM_OPT_USE_AUTH is set to 0 in the database.
Not offered when ZM_OPT_USE_AUTH is set in zm.conf or conf.d: the owner
of the box has decided then, and only the password is asked for.
Until one is chosen the web interface, API and streams can't be used: the
password admin is still refused. Whoever opens the page first makes the
choice; before this, whoever typed admin/admin did. The form carries a
nonce kept in the session, like the default-password form, and is exempt
from csrf-magic as action=login is (the login regenerates the session id).
Both choices are audited. Upgrades are not affected: ZM_SHOW_PRIVACY is 0
once the privacy page was answered, and zmupdate.pl doesn't reset it.
Docs: a First Run section in the getting-started guide, and the FAQ and
system options updated to match.
Tests: tests/php/test_first_run.php.
db/zm_create.sql seeds the admin account with the password the user
guide documents (admin), so with authentication on a new install is
still open to anyone who reads the guide.
Refuse that password as a credential and have the web login replace it:
- validateUser() returns false with "This account uses the password
admin, which ZoneMinder does not accept. Log in to the web interface
to set a new one." when the password is right but is admin. The API (login.json and
user=/pass= on any request) answers 401 with that message; the
user=/pass= web parameters are refused the same way.
- On the login page (action=login) the same login shows a form that
asks for the current password again and a new one, with a nonce kept
in the session so another site can't submit it for the user. The new
password must be set, confirmed and not the default. The account is
logged in only once it has been replaced; the change is audited.
That POST skips reCAPTCHA when it carries the form's nonce: the form
follows a login that passed it, and doesn't render the widget.
With ZM_AUTH_RELAY=plain the session keeps the new password.
- zmLoadUser() refuses the default password, so zms, zmu and the RTSP
server don't accept it in user/password parameters either.
The check is on the password typed, so it needs no schema change, covers
existing installs whose admin still has the default password, and
follows the account if it is renamed. A password set to the default
again later is caught the same way. Sessions, tokens and auth hashes
issued before the upgrade keep working until they expire; with
ZM_AUTH_TYPE=remote the web server's login is not affected.
The FAQ now says to force authentication on in conf.d: since 4d7609d36
the daemons apply zm.conf and conf.d values over the database too.
Tests: tests/php/test_default_password.php (the helpers, and that the
seeded hash is the documented default), tests/zm_user_default_password.cpp.
A boolean option set in zm.conf or conf.d wins over the Config table, and a
person editing a file writes yes or off rather than 1 or 0. The C++ daemons
now read those words (ConfigFileValue, 4d7609d36), but PHP and Perl took the
value as written, and a non-empty string is true in both. So
ZM_STRICT_VIDEO_CONFIG=off in conf.d left the option on in the web UI and the
Perl scripts while zmc turned it off.
Read a file value for a boolean option the same way in all three: yes, true
and on are 1; no, false and off are 0; anything else stays as written. Only
booleans are read this way, so a string option such as a title keeps "Yes".
PHP defines the file values as constants before it reads the Config table,
since it needs the database settings to connect, and only the table says
which options are booleans. So a word now waits until the table has been
read, apart from ZM_DB_* settings, whose words database.php reads itself.
Perl reads the type with each row.
Tested on 1.38's zm138 database with a conf.d file setting
ZM_OPT_FAST_DELETE=yes (0 in the table), ZM_STRICT_VIDEO_CONFIG=off (1)
and ZM_WEB_TITLE=Yes. The installed PHP and Perl gave 'yes', 'off' and
'Yes'; these give '1', '0' and 'Yes'. tests/php/test_config_file_booleans.php
checks the rule.
refs #5289
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
validateUser() compared a plain-text Password column and a MySQL
PASSWORD() hash with ==. Two strings that look like numbers compare as
numbers under ==, so a plain-text password stored as "100" also accepted
"1e2", "100.0" and " 100". Use hash_equals(), which compares the bytes in
constant time; a password that isn't a string is refused.
ZoneMinder stores new passwords as bcrypt, so only accounts whose
Password column was set by hand or carried over from old installs use
these branches. bcrypt still goes through password_verify().
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With ZM_OPT_USE_GOOG_RECAPTCHA on, a password login was refused only when
siteverify answered success == false together with an error-codes list.
An answer without a success field, one that wasn't JSON, success:false
without error-codes, or success:"false" all let the login go ahead as if
the challenge had been solved.
Move the decision into recaptchaAllowsLogin() and allow only
success === true. The existing allowance for a wrong secret key
(invalid-input-secret) is kept, so a mistyped key still logs an Error
instead of locking everyone out.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
get_auth_relay() builds the query string the web UI appends to stream,
AJAX and multi-server URLs. It encoded the plain relay's password but put
the username in as typed, under all three relays. A username containing
& or = split into extra parameters, and zms ends each value at the next =.
Under plain and hashed that user's streams were refused; under none they
ran as whichever user the rest of the name spelled out.
Encode the username as well. zms decodes user=/username= (UriDecode) and
PHP decodes $_REQUEST, so names without reserved characters are unchanged
on the wire and names with them now arrive whole.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
getAuthUser() compared the auth= hash a client presents with the one it
builds using ==. When both strings look like numbers PHP compares them as
numbers, so a computed md5 of the form 0e<digits> equalled any other
0e<digits> string (and "00"), and == also returns at the first byte that
differs. hash_equals() compares the bytes in constant time.
A hash of that form comes up about once in 3*10^8 user-hours, so this is
hardening; ordinary hashes compare exactly as before.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dbQuery()'s two error paths for a prepare() or query() that returns false
logged $pdo->errorInfo. $pdo is not defined there, so PHP warned twice and
the message ended without the error. They also read dbLastError() from the
connection only after ZM\Error() had logged; with a database log target that
inserts into Logs through the same connection, which replaces its errorInfo,
so dbLastError() came back as "00000" instead of the error.
Read the error once, before logging, and use it for both. The query() path
also said "Error preparing" for a statement it ran directly.
These paths run when PDO's error mode doesn't throw; dbConnect() sets
ERRMODE_EXCEPTION, so normally the catch below handles the failure.
tests/php/test_database_error_paths.php drives both paths with
ERRMODE_SILENT and checks for no PHP warning and the real error text.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dbConnect() turns emulated prepares off, so every query of the web UI, the
ajax calls and the includes the API runs (config, servers, users,
permissions) is a server-side prepare: prepare, execute and close, three
round trips to the database where one would do. From PHP 8.1, pdo_mysql
returns integers and floats as PHP ints and floats with emulated prepares
too, so values keep the types they have now. Emulate prepares there and keep
server-side prepares on older PHP, where every value would become a string.
Emulated prepares quote parameters bound as strings, which breaks LIMIT ?.
dbQuery() now binds integer values as integers (everything else as strings,
as execute() did), and the log and watch ajax calls cast their validated
offset and limit to int. With emulation, PDO would also run several
statements given in one string; MYSQL_ATTR_MULTI_STATEMENTS is turned off so
that is refused as before. CakePHP's own connection is not changed: it
already uses PDO's default, emulated prepares.
tests/php/test_database_emulated_prepares.php compares every column of every
table read through both kinds of prepare (types and values), checks
aggregates, LIMIT with positional and named integers, string and null
parameters, and that two statements in one string are refused.
UsersController::view() and edit() assigned their permission check with
`$canX = A or B;`. `=` binds tighter than `or`, so only A was stored and the
self check was dropped: with ZM_USER_SELF_EDIT on, a user without System=Edit
got 401 on PUT users/<own id>.json. beforeFilter() also refused every user
with System=None before view() or edit() ran.
Let beforeFilter() through view and edit of the user's own record while
ZM_USER_SELF_EDIT is on, use || in both checks, and limit what such a user
saves to the fields the web UI allows for self edit
(includes/actions/user.php): Password, Language and HomeView. The request
data is rebuilt from those three fields, so Id, System, permissions and the
rest are never saved; a Language without a lang file is refused as in the UI.
Users with System=Edit save as before.
Self view follows the web UI too: a user without System permission sees their
own record only while ZM_USER_SELF_EDIT is on.
fixes#5242
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ZM_Object's constructor treats an array as the object's row, and validInt()
and validCardinal() returned an array as they got it (preg_replace() does).
So wherever a request id reached `new ZM\X(...)`, raw or through those
validators, a crafted array built an object whose Id, UserId or MonitorId
the sender chose. The permission check then looked at those, and the delete
or update that followed used the Id. A user with Events=View deleted another
user's filter by posting Id[Id]=<filter>&Id[UserId]=<self> to the filter
delete action; the same shape reached events (MonitorId), users, reports,
snapshots and storage.
validInt() and validCardinal() now give '' for anything that isn't a single
value. The places that built an object from a raw $_REQUEST value (the
control and event ajax handlers, the event, filter, report, snapshot and
storage actions, view_hls, and the download and event detail modals) take
the id through validCardinal().
tests/php/test_request_ids_scalar.php covers the validators with arrays and
fails if web code builds an object straight from $_REQUEST, $_GET or $_POST.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
After saving a zone, actions/zone.php redirected to the REFERER the zone form
posted, which the form fills from the Referer its page was loaded with. A
zone editor who reached the zone page through a link on another site was sent
to that site after saving: an open redirect, usable to lead someone from a
trusted ZoneMinder page to a look-alike login. Forging the POST itself is
blocked by the CSRF token, so it needs an editor who followed such a link.
localRedirectUrl() returns the target only if it is a query-only URL or an
absolute http(s) URL on the host and port of the current request, and the
fallback otherwise: another host, //host, backslashes, user@host, other
schemes and control characters all give the fallback. The zone save falls
back to the zones list, as it already does with no Referer (#5207).
tests/php/test_local_redirect_url.php covers 21 cases.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
requestField() read Model[field] whenever the request's Model entry was an
array, empty or not. CakePHP's Model::set() treats the bare top-level fields
as the model's data when that entry is empty (_setAliasData), so
{"Monitor":[],"EventEndCommand":"x"} passed the System-edit check on the
monitor command fields, which found no value, while save() stored x. A
Monitors=Edit user without System=Edit could set EventStartCommand or
EventEndCommand, which zmc runs through /bin/sh.
Every other check built on requestField() had the same gap: the target
MonitorId when adding or editing an event, the EventId when adding a frame,
and the ParentId when editing a group.
Read Model[field] only when the Model entry is non-empty, as set() does, and
otherwise the bare field; a non-empty entry that is not an array yields
nothing, since set() skips it.
tests/php/test_api_request_field.php runs the real method against each
request shape.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
FOSCAMR2C presetGoto and presetSet interpolated the preset into
"SELECT Label FROM ControlPresets WHERE Preset = $preset" and sent the selected
Label to the camera as name=. For presetGoto the preset came straight from the
web request, so any user who could see the monitor could run a SELECT against
the ZoneMinder database and have the value (the auth hash secret, password
hashes) delivered to the camera link, or use it as a boolean oracle.
- FOSCAMR2C: bind the preset in every statement, refuse a preset that is not a
number, and look ControlPresets up by MonitorId as Dahua and PSIA do.
- buildControlCommand(): refuse a non-numeric presetGoto preset, and a control
name that is not a plain method name. zmcontrol calls the name as a method and
the options are split back out of the command string on spaces.
- zmcontrol.pl: refuse a command that is not a plain method name, so a qualified
Package::sub name can't call a sub outside the control module.
- canView()/canEdit() with a monitor id returned only the monitor check and
ignored the area, so canView('Control', $mid) passed for anyone who could see
the monitor. Require the area permission as well; Monitors stays decided by
visibleMonitor()/editableMonitor().
tests/php/test_control_command.php covers buildControlCommand() and the
monitor-scoped canView()/canEdit(); tests/perl/test_foscamr2c_preset_sql.pl
checks the statements FOSCAMR2C prepares and what it sends to the camera.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Filter::canView(), canEdit() and canDelete() ask the user object
$u->canEdit('System'), $u->canView('Events') and so on, but ZM\User had no such
methods. The calls fell through to ZM_Object::__call(), which stored the argument
as a property and returned it, so every check returned a truthy area name. Any
account with Events=View passed as a System editor and could save and run a
filter with AutoExecute, an OS command run by zmfilter.pl. The same fall-through
made the filter ownership and filterdebug checks pass for everyone.
Add canView(), canEdit() and canCreate() to ZM\User. They take the level from the
user's own field and from their role, the way the global functions in auth.php do
for the logged-in user, and refuse anything that is not a permission area.
Monitor::commandFieldChangesNotAllowed() now uses canEdit('System') so a role's
System=Edit counts there too.
In the filter action, require edit on a stored filter before applying the
submitted fields when saving over it, since the fields can name a new owner;
stop after a refused save instead of starting the filter in the background; and
require edit on the filter for start/stop/restart, which had no check.
tests/php/test_user_permissions.php drives the real ZM\User, ZM\User_Role and
ZM\Filter classes. The older filter tests stubbed the user with its own
canView/canEdit, which is why they passed while the real class had neither.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Isaac Connor