Commands run through the terminal were only logged at debug level, and
admin_audit skipped them because the console request carried no command
in argv. Pass the command as argv so admin_audit records the full
command the way it does for occ on the command line, and log the
command name with the user as a warning, like the SQL mode does for
queries. The regular log gets only the name, because arguments can hold
secrets (config:system:set --value=...).
The console application was created with a hand-written list of eight
constructor arguments, which broke whenever Nextcloud changed its
constructor. Resolve the arguments by type from the container instead,
passing our own request so the console still gets an argv for its
ConsoleEvent.
SET PASSWORD, SET GLOBAL and SET PERSIST (also as @@GLOBAL.x) change
the server or the database account rather than the session, so they
need confirmation like other writes. Session variables stay a read.
SHOW, EXPLAIN, VALUES and TABLE count as reads now but their rows were
dropped and reported as a write; every read except SET now returns its
rows. nextval() advances a sequence, so it needs confirmation like
setval().
removeComments() used regexes that also matched "--" and "/*" inside
string literals. A literal like '--' or '/*' then removed the code after
it from the checks while PostgreSQL still ran it, e.g.
COPY (SELECT '--') TO PROGRAM 'sh' passed the forbidden-construct check,
and a DELETE in a CTE between '/*' and '*/' literals skipped the
confirmation.
Comments and literals are now found by one scanner that walks the SQL
the way the database does ('...', E'...', "...", $tag$...$tag$, -- and
block comments), shared by removeComments(), removeLiterals() and
splitStatements().
splitStatements() only knew 'strings' and "identifiers", so a DO block
or function body ($$ ... ; ... $$) was cut at its inner semicolons and
failed, and a quote inside a comment (-- don't) opened a string that
never closed. It now also skips dollar-quoted bodies, E'...' strings
with backslash escapes, and comments. It moves to SqlGuard so it is
tested with the other checks.
OccController::run() now catches Throwable, so an Error thrown by a
command is logged and returned like an Exception instead of ending in
an HTTP 500.
Not changed on purpose: showing rows for statements with RETURNING or
data-modifying CTEs, and failure handling in the Makefile release
targets. Both came up in the same review but are unrelated to the
security fixes in this release.
A security review found three issues:
- Comments were removed without a space before matching the forbidden
patterns, so "TO/**/PROGRAM" became "TOPROGRAM" and passed the
COPY ... PROGRAM check while PostgreSQL still ran it. Comments are
now replaced by a space, and the COPY file check also covers E'...',
U&'...' and dollar-quoted file names.
- Only statements starting with DELETE or UPDATE asked for
confirmation, so a leading block comment, a data-modifying CTE
(WITH x AS (DELETE ...)) or DROP/TRUNCATE/INSERT ran without it.
Every statement that is not a plain read now needs confirmation,
decided on the server.
- occ output could contain escape sequences, backspaces and URLs from
user-controlled text (display names, file names) that rewrite or hide
text or show a clickable link. Only color codes are kept now, and
URLs are no longer turned into links.
The checks move to SqlGuard so they can be tested on their own. They
remain defense in depth: dynamic SQL can always get around a text
check, so the database role should not have superuser rights,
pg_execute_server_program or pg_read/write_server_files. A read that
starts with WITH now shows its rows like a SELECT.
testbak/ (local, non-AIO docker compat rigs for NC32/33/34/35) found a real
fatal on NC35: OccOutput::setErrorOutput() didn't declare the ": void"
return type the Symfony Console version NC35 bundles now requires on
ConsoleOutputInterface. Fixed by declaring it - compatible with older
Symfony/NC versions too since it only narrows an untyped parent signature.
32/33 were also verified functional (the previously-suspected jQuery
getter-conflict console exception on those versions is real but harmless -
the app attaches via window.jQuery, not window.$).
min-version/max-version narrowed from 30/34 to 32/35: 30/31 were never
actually tested even before this session, so keeping them was an unverified
claim; 35 is now genuinely verified rather than guessed.
- js/index.js: the occ-command ajax call used the wrong jQuery .fail()
signature ((response, code) instead of (jqXHR, textStatus,
errorThrown)), so any failure printed the literal string
"[object Object]" instead of a useful message.
- js/index.js: the initial bootstrap call that builds the terminal had
no .fail() handler at all - a failure there (expired session/CSRF
token, network error, rejected admin check) left #app-content
permanently blank with zero diagnostic information. This is exactly
what made today's NC34 debugging session harder than it needed to
be; now it shows the actual HTTP status/reason in the page.
- OccController.php: translated a leftover Dutch comment (missed by
the earlier Russian->English pass since it wasn't Russian), and
documented why requireAdmin() is intentional defense-in-depth rather
than a misunderstanding of the framework: verified against
Nextcloud's own SecurityMiddleware source that any controller method
without @NoAdminRequired already rejects non-admins by default, so
this check (and the original "security fix" commit that added it)
was addressing a non-issue for THIS controller specifically.
DbController's equivalent check is still genuinely load-bearing -
that controller originally shipped with @PublicPage, which does
bypass the default protection - so it gets a note explaining the
distinction instead.
- README: added compatibility matrix rows for 0.4.0 (app id rename to
extended_occweb) and 0.4.1 (these fixes), both verified on NC 34.0.4.
Deferred to when a test instance exists (tracked, not fixed here): no
documented migration path from the old occweb id to extended_occweb
for existing installs, OccController's constructor doing expensive
work before the (redundant) admin check runs, and js/jquery.js loading
unconditionally on NC30-33 where core still provides jQuery.
Version bump to 0.4.1.
Nextcloud's app id "occweb" is effectively unavailable for a new store
listing: the original Adphi/occweb still owns that App Store slot
(stale, v0.2.3), and el-choco/occweb_v2 has since published a
same-lineage rewrite under its own id. Rather than fight over the
"occweb" name, publish this fork under its own id, matching the
<name> this app has always had: "Extended OCC Web".
Renames every place the literal app id is used: info.xml <id> and the
auto-generated navigation <route>, script()/style() registrations in
templates/index.php, the TemplateResponse app name in
OccController::index(), the /apps/... base URL in js/index.js, and the
'app' log context tag in DbController.php. The GitHub repo name and
its <website>/<bugs>/<repository>/<screenshot> links are unaffected -
those identify the repo, not the Nextcloud app.
Docs (README, INSTALL-AIO.md, install.sh) updated to clone into and
reference an "extended_occweb" directory, since Nextcloud requires the
app folder name to match its id - a plain `git clone` of this repo no
longer produces a correctly-named install directory on its own.
Version bump to 0.4.0 given the scope (breaking change for anyone who
already deployed under the old "occweb" id - requires disable+remove
of the old app and a fresh enable under the new id, not an in-place
upgrade).
query() runs arbitrary admin SQL (including DELETE/UPDATE) and was
exempted from CSRF protection — a leftover from the original upstream
code, which had this alongside @PublicPage (no auth at all) before our
admin-check hardening. The client already goes through the standard
$.ajax() flow that auto-attaches Nextcloud's CSRF token, exactly like
OccController::cmd() does without any such exemption, so removing it
has no functional cost.
Also bump nextcloud max-version to 36 to match nc01's current core
version, now that AIO auto-updates the core nightly.
Version bump to 0.3.4.
Covers OccOutput.php, DbController.php and js/index.js: docblocks,
inline comments, the forbidden-construct labels shown to admins in
error responses, and the SQL template descriptions shown in the
terminal (delete-user, list-user, rename-user).
Also backport OccController's getConfig() -> IConfig::class fix (deprecated
internal API -> stable OCP interface), spotted while comparing against
Git-Usr123/occweb.
index(), cmd() and list() were accessible to any logged-in user.
The navigation menu already restricts visibility to admins, but the
routes themselves were unprotected — any authenticated user who knows
the URL could execute occ commands or list all available commands.
Fix: inject IGroupManager + IUserSession, add requireAdmin() helper,
call it at the start of all three public methods. Matches the pattern
already used in DbController.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
BufferedOutput::fetch() declares a string return type in Symfony 5+.
PHP 8.1 enforces return type compatibility with parent methods, causing
a fatal 500 error when the occweb page loads.
Fix: add ': string' return type to OccOutput::fetch().
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- New 'rename-user' template (templates / template rename-user <old> <new>):
best-effort uid rename across core tables (oc_users, oc_preferences,
oc_group_user, oc_group_admin, oc_ldap_user_mapping, oc_share, oc_mounts,
oc_storages). Explicitly NOT a supported Nextcloud operation - the
generated script carries an in-line warning (as leading SQL comments)
that app-specific tables (Talk, Calendar, Contacts, Mail, 2FA/WebAuthn...)
are not covered and that the data directory must be renamed on disk
manually, followed by occ files:scan --all. Documented the same caveat
in both READMEs.
- Hardened the round-2 transaction wrapping: beginTransaction()/commit()/
rollBack() are now wrapped in try/catch. PostgreSQL (our backend) has
fully transactional DDL so this wasn't actually broken here, but
Nextcloud also supports MySQL/MariaDB via the same IDBConnection, where
DDL implicitly commits - on that backend a DDL statement in the batch
would make a later commit()/rollBack() throw 'no active transaction'
and previously that exception was unhandled (HTTP 500 instead of a
clean JSON response). Now: if rollBack() itself fails after an error,
we report rollbackFailed+warning instead of falsely claiming rolledBack,
since earlier statements in that batch may already be permanently
applied. If commit() fails with nothing to commit (already
auto-committed), that's logged only, since the effects are already
durably persisted.
- removeAllComments() strips -- and /* */ comments before checking
FORBIDDEN_PATTERNS, closing a bypass where a comment inserted between
a function name and '(' hid it from the regex (e.g. pg_read_file/**/(...)).
- UPDATE now requires confirm=true just like DELETE (deleteCount/updateCount).
- Batches run inside a single DB transaction; a failing statement rolls back
everything already applied in that batch instead of leaving partial writes.
- splitStatements() (PHP and JS) now tracks a single quoteChar instead of a
bool, handling both '...' strings and "..." quoted identifiers symmetrically.
- Frontend TEMPLATES no longer rely on SET/current_setting: the escaped uid
is inlined into every statement, so partial copy/paste still works.
- renderTable() renders NULL as [NULL] instead of an empty string, so it's
no longer visually identical to an empty string value.
- Added MAX_SQL_BYTES (1MB) request size limit, returned as HTTP 413.
- Added a 120s client-side AJAX timeout with a distinct timeout message,
so a hung request doesn't leave the terminal paused indefinitely.
Declaration of OCA\OCCWeb\Controller\OccOutput::getErrorOutput() must be compatible with Symfony\Component\Console\Output\ConsoleOutputInterface::getErrorOutput(): Symfony\Component\Console\Output\OutputInterface at /var/www/html/custom_apps/occweb/lib/Controller/OccOutput.php#22
dillard
Egor Bugaev