From 7f2c71299fb0ef023b73070acf22ea0eef718464 Mon Sep 17 00:00:00 2001 From: William Mantly Date: Mon, 27 Jul 2026 21:03:34 -0400 Subject: [PATCH 1/2] Fix DELETE /api/oauth/client/:id: client.remove is not a function OAuthClient wraps @simpleworkjs/orm's Resource model, whose instance delete method is .delete() -- not .remove(), which is what model-redis's Table instances (e.g. this app's ApiToken, AuthToken) use. The DELETE route called the wrong one, so every delete silently 500'd; the route's try/catch turned it into a plain JSON error response rather than a thrown exception, and the existing tests' cleanup-only delete calls (afterAll, end of the rotate test) never checked the response status, so the bug shipped unnoticed. The Directory Management UI was never affected -- routes/api_directory_admin.js's DELETE routes already used .delete() correctly throughout. Found and root-caused live against a real deployment's SSO API, then reproduced and fixed against a local docker stack with a rebuilt image: confirmed DELETE returned a genuine 500 before the fix and a real 200 + 404-on-subsequent-GET after. Adds two dedicated tests (PUT and DELETE persistence, each verified by a follow-up GET rather than trusting the mutating response alone), and hardens the existing rotate test's incidental delete call with real assertions. Verified the new DELETE test fails on the old code and passes on the fix. Full suite (189 tests, real LDAP + Redis) passes. Co-Authored-By: Claude Sonnet 5 --- nodejs/routes/oauth_client.js | 2 +- nodejs/tests/oauth.test.js | 51 ++++++++++++++++++++++++++++++++++- 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/nodejs/routes/oauth_client.js b/nodejs/routes/oauth_client.js index 18569cc..b35213f 100644 --- a/nodejs/routes/oauth_client.js +++ b/nodejs/routes/oauth_client.js @@ -97,7 +97,7 @@ router.delete('/:client_id', async function(req, res, next) { await permission.byGroup(req.user, [ADMIN_GROUP]); const client = await OAuthClient.get(req.params.client_id); - await client.remove(); + await client.delete(); return res.json({ client_id: req.params.client_id, diff --git a/nodejs/tests/oauth.test.js b/nodejs/tests/oauth.test.js index 5613a1f..ffd0764 100644 --- a/nodejs/tests/oauth.test.js +++ b/nodejs/tests/oauth.test.js @@ -69,6 +69,51 @@ describe('OAuth client management API — /api/oauth/client', () => { expect(res.body.results).not.toHaveProperty('client_secret_hash'); }); + test('PUT persists — a changed name survives a fresh GET', async () => { + const created = await request(app) + .post('/api/oauth/client/') + .set('auth-token', token) + .send({ name: 'put-persist-test', redirect_uris: REDIRECT_URI }); + expect(created.status).toBe(200); + const id = created.body.results.client_id; + + const updated = await request(app) + .put(`/api/oauth/client/${id}`) + .set('auth-token', token) + .send({ name: 'put-persist-test-renamed' }); + expect(updated.status).toBe(200); + expect(updated.body.results.name).toBe('put-persist-test-renamed'); + + const fetched = await request(app).get(`/api/oauth/client/${id}`).set('auth-token', token); + expect(fetched.status).toBe(200); + expect(fetched.body.results.name).toBe('put-persist-test-renamed'); + + await request(app).delete(`/api/oauth/client/${id}`).set('auth-token', token); + }); + + // Regression: this route called client.remove(), but OAuthClient wraps + // @simpleworkjs/orm's Resource model, whose instance method is .delete() + // — .remove() doesn't exist on it (unlike the model-redis Tables + // elsewhere in this app, e.g. api_token.js, which really do have + // .remove()). The route's try/catch turned the resulting TypeError into + // a plain 500 JSON response rather than a thrown exception, so every + // prior DELETE call in this file's cleanup hooks silently "succeeded" + // from Jest's point of view while leaving the client un-deleted. + test('DELETE persists — the client is actually gone, not just a 200', async () => { + const created = await request(app) + .post('/api/oauth/client/') + .set('auth-token', token) + .send({ name: 'delete-persist-test', redirect_uris: REDIRECT_URI }); + expect(created.status).toBe(200); + const id = created.body.results.client_id; + + const deleted = await request(app).delete(`/api/oauth/client/${id}`).set('auth-token', token); + expect(deleted.status).toBe(200); + + const fetched = await request(app).get(`/api/oauth/client/${id}`).set('auth-token', token); + expect(fetched.status).toBe(404); + }); + test('list then rotate a client by its returned client_id (the bootstrap path)', async () => { // Reproduces exactly what the theta-env bootstrap does: create, list, // find by name, rotate by the client_id from the list response. Uses a @@ -90,7 +135,11 @@ describe('OAuth client management API — /api/oauth/client', () => { expect(rotated.status).toBe(200); expect(rotated.body.client_secret).toBeTruthy(); - await request(app).delete(`/api/oauth/client/${found.client_id}`).set('auth-token', token); + const deleted = await request(app).delete(`/api/oauth/client/${found.client_id}`).set('auth-token', token); + expect(deleted.status).toBe(200); + + const afterDelete = await request(app).get(`/api/oauth/client/${found.client_id}`).set('auth-token', token); + expect(afterDelete.status).toBe(404); }); test('GET /:id unknown id returns 404, not 500', async () => { From 3852e9ba6278d7730a6ac100050d45898f3eea22 Mon Sep 17 00:00:00 2001 From: William Mantly Date: Mon, 27 Jul 2026 21:10:49 -0400 Subject: [PATCH 2/2] Add regression test: no native alert()/confirm()/prompt() Native confirm() blocks all further browser events on the page (found live, mid browser-automation testing, on directory.ejs's "Rotate Client Secret" -- it froze the tab). Every call site across the app was removed in favor of app.messages.action/confirm/toast and app.modal.open; this static check (scans views/ and public/js|lib/js for bare alert(/confirm(/ prompt() calls) keeps a regression from shipping unnoticed the way the oauth_client.js DELETE bug just did. Co-Authored-By: Claude Sonnet 5 --- nodejs/tests/no_native_dialogs.test.js | 45 ++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) create mode 100644 nodejs/tests/no_native_dialogs.test.js diff --git a/nodejs/tests/no_native_dialogs.test.js b/nodejs/tests/no_native_dialogs.test.js new file mode 100644 index 0000000..ac394ad --- /dev/null +++ b/nodejs/tests/no_native_dialogs.test.js @@ -0,0 +1,45 @@ +'use strict'; + +// Regression guard: native alert()/confirm()/prompt() calls block all further +// browser events on the page (found live, mid browser-automation testing, on +// directory.ejs's "Rotate Client Secret" — it froze the tab entirely) and are +// visually inconsistent with the rest of the UI. Every call site was removed +// in favor of app.messages.action/confirm/toast and app.modal.open; this test +// keeps it that way. + +const fs = require('fs'); +const path = require('path'); + +const ROOTS = ['views', 'public/js', 'public/lib/js'].map((d) => path.join(__dirname, '..', d)); + +// Matches a bare alert(/confirm(/prompt( call, but not app.messages.*, +// app.modal.*, or identifiers merely containing these words (e.g. +// "confirmation", ".confirmed"). +const NATIVE_DIALOG_RE = /(^|[^.\w$])(alert|confirm|prompt)\s*\(/g; + +function walk(dir) { + let files = []; + if (!fs.existsSync(dir)) return files; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) files = files.concat(walk(full)); + else if (/\.(ejs|js)$/.test(entry.name)) files.push(full); + } + return files; +} + +test('no view or client-side script calls native alert()/confirm()/prompt()', () => { + const offenders = []; + for (const root of ROOTS) { + for (const file of walk(root)) { + const src = fs.readFileSync(file, 'utf8'); + let m; + NATIVE_DIALOG_RE.lastIndex = 0; + while ((m = NATIVE_DIALOG_RE.exec(src))) { + const line = src.slice(0, m.index).split('\n').length; + offenders.push(`${path.relative(path.join(__dirname, '..'), file)}:${line} — ${m[2]}(`); + } + } + } + expect(offenders).toEqual([]); +});