Fix code-review findings: ajax error handling, translated comment, docs
- 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.
This commit is contained in:
@@ -146,6 +146,9 @@ was verified, so a rollback target is obvious if a newer major causes problems.
|
||||
| 0.3.4 | `v0.3.4` | 34.0.4 | ❌ Blank terminal - global jQuery gone on NC34 (`jQuery is not defined`), then a 412 "CSRF check failed" on every ajax call once jQuery was vendored. Fixed in 0.3.5. |
|
||||
| 0.3.5 | `v0.3.5-nc34` | 34.0.4 | ✅ Verified (nc01, 2026-09-26) - vendors its own jQuery + sends the CSRF `requesttoken` header explicitly on every ajax call |
|
||||
| 0.3.5 | `v0.3.5-nc34` | 33.0.8 | ⚠️ Believed compatible (the 0.3.5 change is additive-only, nothing removed that NC30-33 depended on) but not re-verified - nc01 no longer runs NC33 to test against |
|
||||
| 0.4.0 | (untagged) | 34.0.4 | ✅ Verified (nc01, 2026-09-26) - app id renamed `occweb` → `extended_occweb` (App Store prep), no functional changes to the 0.3.5 fixes otherwise |
|
||||
| 0.4.1 | `v0.4.1-nc34` | 34.0.4 | ✅ Verified (nc01, 2026-09-26) - code review fixes: correct jQuery `.fail()` signature, added a missing `.fail()` handler on the initial terminal-load request |
|
||||
| 0.4.1 | `v0.4.1-nc34` | 33.0.8 | ⚠️ Same caveat as 0.3.5/0.4.0 - not re-verified, no NC33 test environment available. Also unresolved: `js/jquery.js` now loads unconditionally on every supported version, even NC30-33 where core still provides jQuery - flagged in code review, needs an NC33 environment to confirm it's harmless there. |
|
||||
|
||||
If a future major breaks something, `git checkout v0.3.4` is the last version confirmed
|
||||
working on NC33 and older.
|
||||
|
||||
+1
-1
@@ -5,7 +5,7 @@
|
||||
<name>Extended OCC Web</name>
|
||||
<summary>OCC Commands in a web terminal</summary>
|
||||
<description><![CDATA[Run OCC Commands in a web terminal]]></description>
|
||||
<version>0.4.0</version>
|
||||
<version>0.4.1</version>
|
||||
<licence>agpl</licence>
|
||||
<author mail="dillard.blom@ensembia.com" >Dillard Blom</author>
|
||||
<namespace>OCCWeb</namespace>
|
||||
|
||||
+12
-2
@@ -387,8 +387,8 @@
|
||||
data: JSON.stringify(occCommand)
|
||||
}).done(function (response) {
|
||||
term.echo('\n' + response).resume();
|
||||
}).fail(function (response, code) {
|
||||
term.echo('\n' + response).resume();
|
||||
}).fail(function (xhr, status) {
|
||||
term.echo('\n[[;#ff5555;]Request failed: ]' + $.terminal.escape_formatting(xhr.status + ' ' + xhr.statusText)).resume();
|
||||
});
|
||||
}
|
||||
}, {
|
||||
@@ -410,6 +410,16 @@
|
||||
scrollToBottom()
|
||||
}
|
||||
});
|
||||
}).fail(function (xhr, status) {
|
||||
// Without this, a failure here (expired session/CSRF token, network
|
||||
// error, a rejected admin check) leaves #app-content permanently
|
||||
// empty with no visible error and nothing in the console pointing
|
||||
// at the cause - exactly what happened during NC34 testing before
|
||||
// this handler existed.
|
||||
$('#app-content').text(
|
||||
'Failed to load the terminal: ' + xhr.status + ' ' + xhr.statusText +
|
||||
'. Try reloading the page; if that keeps happening, check the server logs.'
|
||||
);
|
||||
});
|
||||
$('html').keypress(function(){
|
||||
scrollToBottom()
|
||||
|
||||
@@ -162,7 +162,12 @@ class DbController extends Controller
|
||||
|
||||
public function query()
|
||||
{
|
||||
// Check admin privileges
|
||||
// Deliberate defense-in-depth (see the equivalent check in
|
||||
// OccController::requireAdmin() for why it's kept even though
|
||||
// Nextcloud's SecurityMiddleware already enforces this by default
|
||||
// absent @NoAdminRequired) - this one is also the direct fix for
|
||||
// this controller having shipped with @PublicPage originally,
|
||||
// which did bypass that default protection entirely.
|
||||
$user = $this->userSession->getUser();
|
||||
if (!$user) {
|
||||
return new JSONResponse(['error' => 'Not authenticated'], 401);
|
||||
|
||||
@@ -50,8 +50,17 @@ class OccController extends Controller
|
||||
|
||||
private function requireAdmin(): ?JSONResponse
|
||||
{
|
||||
// Services via OC::$server zodat de constructor-signatuur ongewijzigd blijft
|
||||
// en NC's DI-container (zonder application.php) de class kan resolven.
|
||||
// Deliberate defense-in-depth, not a workaround for a missing framework
|
||||
// check: Nextcloud's own SecurityMiddleware already rejects non-admins
|
||||
// here by default (no method below has @NoAdminRequired), so this is
|
||||
// redundant today. Kept anyway given what this app can do (arbitrary
|
||||
// occ/SQL execution) - if the framework check ever regresses, this
|
||||
// still holds. If you ever want to intentionally open one of these
|
||||
// routes to non-admins, remove the call below AND add
|
||||
// @NoAdminRequired, or this will keep silently blocking it.
|
||||
//
|
||||
// Services via OC::$server so the constructor signature stays unchanged
|
||||
// and NC's DI container (without an application.php) can resolve the class.
|
||||
$userSession = OC::$server->get(\OCP\IUserSession::class);
|
||||
$user = $userSession->getUser();
|
||||
if ($user === null) {
|
||||
|
||||
Reference in New Issue
Block a user