diff --git a/scripts/ci-test-plan.js b/scripts/ci-test-plan.js index 03097ce3cc..eee62b7cfb 100644 --- a/scripts/ci-test-plan.js +++ b/scripts/ci-test-plan.js @@ -226,6 +226,12 @@ const structuralTestsFor = (changedFiles, trackedSet) => { if (changedFiles.some((path) => /^server\/lib\//.test(path))) { add('server/lib/index.test.js'); } + // The socket guard readdir-scans server/sockets/ rather than importing it, so + // no import edge reaches it — a handler added there would otherwise only be + // checked on a full suite. + if (changedFiles.some((path) => /^server\/sockets\//.test(path))) { + add('server/sockets/asyncHandlerGuard.test.js'); + } if (changedFiles.some((path) => /^client\/src\/lib\//.test(path))) { add('client/src/lib/index.test.js'); } diff --git a/server/AGENTS.md b/server/AGENTS.md index f2a8aa0714..8cc90f6c0a 100644 --- a/server/AGENTS.md +++ b/server/AGENTS.md @@ -15,6 +15,10 @@ These apply to Express/service/model code. Universal constraints (no try/catch o - **An `async` timer callback must own its rejections.** Inside `setTimeout(async …)` / `setInterval(async …)`, every `await` needs either an enclosing `try { … } catch` or a `.catch(…)` on the awaited chain. A timer fires outside the request lifecycle, so nothing holds the promise the callback returns and a rejection escapes as an unhandled rejection — fatal on Node >= 15. The CoS runner shipped that at boot: its orphan-cleanup timer awaited a state-file read/write, so one truncated `agents` file killed the runner seconds after start and PM2 restart-looped it against the same file (#5668). Log and continue in the catch unless the callback genuinely owns a failure verdict. `server/timerCallbackConventions.test.js` scans the whole server tree for this and carries no allowlist; the worked example is the watchdog in `server/services/mediaJobQueue/index.js`. +## Socket handlers + +- **An `async` Socket.IO handler must own its rejections.** Same rule as the timer callbacks above, same reason: Socket.IO hands the promise a listener returns to nobody, so a rejection escapes as an unhandled rejection and Node terminates the process — taking every agent run, PTY session and media job with it. Every `await` in an `async` handler under `server/sockets/` needs an enclosing `try { … } catch` (the shape `voice.js` uses on whole handler bodies) or a `.catch(…)` on the awaited chain (the shape `logs.js` uses on the lookups it falls back from). `server/sockets/asyncHandlerGuard.test.js` scans the directory for this and carries no allowlist; it names the file and the event string of any offender. Precedent: `voice:call:detach` shipped unguarded (#5661). + ## Tests and peer fan-out - **Record-creating tests and peer fan-out.** Record create/update paths reach peer-sync only through the subscription adapter in `server/services/sharing/recordEvents.js` (`autoSubscribeRecordToAllPeers` etc.) — a silent no-op until `peerSync.js` registers the real implementation at module load. A suite that never loads `peerSync.js` therefore gets no fan-out without mocking anything. Suites that DO load the peer-sync graph (importing `peerSync.js` or `sharing/index.js`, directly or transitively) must still mock `services/instances.js` with `mockNoPeers()` from `server/lib/mockPathsDataRoot.js` and `services/sharing/peerSync.js` with `mockNoPeerSync()` — the mock keeps the real module's registration side effect from wiring live fan-out into the adapter. To assert auto-subscribe behavior in a domain test, register a test double via `registerSubscriptionAdapter({...})` and clean up with `__resetSubscriptionAdapter()`. diff --git a/server/lib/README.md b/server/lib/README.md index a37946a3b0..3d453d2c35 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -434,6 +434,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `apiAccessPolicy.js` | Shared always-public path and gated non-`/api` prefix policy consumed by both `authGate` and API discovery. | | `apiCatalog.js` | Searchable projection of the generated Express route manifest: domain, access, side-effect, contract coverage, summaries, and Express-to-OpenAPI path conversion. | | `socketEventCatalog.js` | Searchable projection of the cached Socket.IO inventory: direction, domain, and runtime-schema coverage. | +| `sourceScan.js` | Lexer-assisted primitives shared by the whole-tree source-scan guard suites, so the timer rule and the socket rule cannot drift on what "owns its rejection" means. `blankLiterals(src)` blanks comment/string/template/regex CONTENT to spaces while preserving length, so a brace inside a literal cannot skew a bracket walk and a caller can still read a literal (a socket event name) out of the original string at the same offset; `matchBracket(src, open)` returns the index past the matching `)`/`]`/`}`; `parseCallbackAt(blanked, from, limit)` parses the function expression at `from` (`async`/`function`/either arrow spelling, skipping the parameter list as a unit so a `= {}` default is not mistaken for the body) and returns `{isAsync, start, text}`; `unguardedAwaits(body)` returns the awaited chains that neither sit inside a `try`/`catch` nor END in `.catch(…)`. `blankComments(src)` is the weaker LINE-based stripper the per-line `child_process` rule needs. Callers: `childProcess.guards.test.js`, `server/timerCallbackConventions.test.js`, `server/sockets/asyncHandlerGuard.test.js`. | | `apiOperationContracts.js` | Detailed operation metadata for intentionally public APIs. It consumes the canonical route Zod contracts and feeds both public and internal OpenAPI documents. | | `apiRegistry.js` | Single source of truth for which PortOS services are externally-callable HTTP APIs (`voice`, `sdapi`). `API_REGISTRY` declares each API's `publicPrefixes` (read/compute-safe surface only) + defaults; `isRegistryPublic(settings, path)` tells `authGate` when an `exposed && !requireAuth` API re-opens its prefix; `resolveApiAccess(settings)` merges persisted `apiAccess` flags for the Settings UI + OpenAPI docs. | | `arrayUtils.js` | `shuffle(arr)` — Fisher-Yates shuffle (new array, never mutates). The canonical uniform shuffle — never `arr.sort(() => Math.random() - 0.5)`, which is biased. Shared by `meatspacePostCognitive.js` (Schulte table / mental rotation) and `meatspacePostMemory.js` (memory drill generators). | diff --git a/server/lib/childProcess.guards.test.js b/server/lib/childProcess.guards.test.js index 9f1c7ec69f..6b58ebff45 100644 --- a/server/lib/childProcess.guards.test.js +++ b/server/lib/childProcess.guards.test.js @@ -1,6 +1,7 @@ import { describe, it, expect } from 'vitest'; import { join } from 'node:path'; import { collectServerSources, readServerSource, SERVER_DIR } from './testHelper.js'; +import { blankComments } from './sourceScan.js'; // Windows hands a newly allocated console off to Windows Terminal when a // console-less parent (every PM2 fork, which is all of PortOS) spawns a console @@ -30,20 +31,6 @@ const SIBLING_PACKAGES = ['../autofixer', '../browser']; const SPAWN_FNS = ['spawn', 'spawnSync', 'fork', 'exec', 'execSync', 'execFile', 'execFileSync']; -/** - * Blank comment lines while preserving line count, so a rule can be *described* - * in a comment without the guard flagging the description as a violation - * (`cosHealthMonitor.js` explains the pm2 rule using the banned pattern). - * Line-based on purpose: every real `child_process` mention outside the wrapper - * is a JSDoc `@param {import('child_process').ChildProcess}` line, and a false - * positive here is loud and one line to fix. - * @param {string} src - * @returns {string[]} - */ -function blankComments(src) { - return src.split('\n').map((line) => (/^\s*(\/\/|\*|\/\*)/.test(line) ? '' : line)); -} - /** * Extract whole call expressions by name, brace-balanced so a call wrapped * across several lines is captured entire. A per-line scan misses exactly the diff --git a/server/lib/index.js b/server/lib/index.js index bf1bf97255..65f25da223 100644 --- a/server/lib/index.js +++ b/server/lib/index.js @@ -409,6 +409,7 @@ export * from './viteAllowedHosts.js'; export * from './apiAccessPolicy.js'; export * from './apiCatalog.js'; export * from './socketEventCatalog.js'; +export * from './sourceScan.js'; export * from './apiOperationContracts.js'; export * from './apiRegistry.js'; export * from './arrayUtils.js'; diff --git a/server/lib/sourceScan.js b/server/lib/sourceScan.js new file mode 100644 index 0000000000..7f793a61b9 --- /dev/null +++ b/server/lib/sourceScan.js @@ -0,0 +1,312 @@ +/** + * Text-level primitives shared by the whole-tree source-scan guard suites — + * `childProcess.guards.test.js`, `timerCallbackConventions.test.js` and + * `sockets/asyncHandlerGuard.test.js`. + * + * Those guards enforce structural rules no runtime test can reach ("every spawn + * goes through the wrapper", "every await outside the request lifecycle owns its + * rejection"), so each one reads source as text and each one needs the same + * three things: literals and comments neutralised, so a rule *described* in + * prose can neither satisfy nor defeat the scan and a `}` inside a string cannot + * skew a brace walk; a bracket matcher, so a construct split across lines is + * captured WHOLE rather than by a fixed character window (a window reads the + * next statement and attributes it to the current one); and one definition of + * "this await owns its rejection", so the timer rule and the socket rule cannot + * drift apart. + * + * Lexer-assisted, not an AST pass: a real parser would be more precise, but + * these guards run over ~1500 files on every `npm test`, and their false + * positives are loud and one line to fix. The limits are listed on + * `unguardedAwaits` and in `timerCallbackConventions.test.js`'s header. + */ + +/** + * Blank comment LINES while preserving line count, so a rule can be *described* + * in a comment without the guard flagging the description as a violation + * (`cosHealthMonitor.js` explains the pm2 rule using the banned pattern). + * + * Line-based, and so weaker than `blankLiterals` — it is the right tool only for + * a rule matched per line, where every real hit outside the wrapper is a JSDoc + * `@param {import('child_process').ChildProcess}` line and a false positive is + * loud and one line to fix. + * @param {string} src + * @returns {string[]} one entry per input line, comment lines replaced by '' + */ +export function blankComments(src) { + return src.split('\n').map((line) => (/^\s*(\/\/|\*|\/\*)/.test(line) ? '' : line)); +} + +/** + * True when a `/` at this position opens a regex literal rather than division. + * A regex can only follow a position where an operand cannot: an operator, an + * opening bracket, a statement boundary, or the start of the file. + */ +function regexCanStartAfter(prev) { + return prev === '' || '(,=:[!&|?{};+-*%~^<>'.includes(prev); +} + +/** + * Replace the contents of comments, string/template literals, and regex + * literals with spaces, preserving LENGTH so every index still maps back to the + * original source. Everything downstream (bracket walks, `await` matching) then + * sees code only — and a caller that needs a literal's text (a socket event + * name, say) can still read it out of the original string at the same offset. + * @param {string} src + * @returns {string} same length as `src` + */ +export function blankLiterals(src) { + const out = src.split(''); + const n = src.length; + const blank = (i) => { if (i < n && src[i] !== '\n') out[i] = ' '; }; + // Brace depths of the code regions opened by `${` inside template literals, + // so a nested template resumes correctly at its closing `}`. + const templateStack = []; + let inTemplate = false; + let braceDepth = 0; + // A `/` starts a regex only where a value cannot precede it. Tracking the last + // significant character is the standard heuristic and is what keeps a + // character class like /["']/ from being read as a string opener. + let prevSignificant = ''; + let i = 0; + + while (i < n) { + const c = src[i]; + + if (inTemplate) { + if (c === '\\') { blank(i); blank(i + 1); i += 2; continue; } + if (c === '`') { blank(i); i += 1; inTemplate = false; prevSignificant = '`'; continue; } + if (c === '$' && src[i + 1] === '{') { + blank(i); blank(i + 1); i += 2; + templateStack.push(braceDepth); + braceDepth = 0; + inTemplate = false; + prevSignificant = '{'; + continue; + } + blank(i); i += 1; continue; + } + + if (c === '/' && src[i + 1] === '/') { + while (i < n && src[i] !== '\n') { blank(i); i += 1; } + continue; + } + if (c === '/' && src[i + 1] === '*') { + while (i < n && !(src[i] === '*' && src[i + 1] === '/')) { blank(i); i += 1; } + blank(i); blank(i + 1); i += 2; + continue; + } + if (c === "'" || c === '"') { + const quote = c; + blank(i); i += 1; + while (i < n && src[i] !== quote) { + if (src[i] === '\\') { blank(i); i += 1; } + blank(i); i += 1; + } + blank(i); i += 1; + prevSignificant = quote; + continue; + } + if (c === '`') { blank(i); i += 1; inTemplate = true; continue; } + if (c === '/' && regexCanStartAfter(prevSignificant)) { + blank(i); i += 1; + let inClass = false; + while (i < n && src[i] !== '\n') { + if (src[i] === '\\') { blank(i); blank(i + 1); i += 2; continue; } + if (src[i] === '[') inClass = true; + else if (src[i] === ']') inClass = false; + else if (src[i] === '/' && !inClass) break; + blank(i); i += 1; + } + blank(i); i += 1; + // Blank the flags too so `gi` can't be read as an identifier. + while (i < n && /[a-z]/.test(src[i])) { blank(i); i += 1; } + prevSignificant = ')'; + continue; + } + + if (c === '{') braceDepth += 1; + else if (c === '}') { + if (braceDepth === 0 && templateStack.length > 0) { + blank(i); i += 1; + braceDepth = templateStack.pop(); + inTemplate = true; + continue; + } + braceDepth -= 1; + } + if (!/\s/.test(c)) prevSignificant = c; + i += 1; + } + + return out.join(''); +} + +/** + * Index just PAST the bracket matching the `(`, `[` or `{` at `open`, or -1. + * Assumes `src` has been through `blankLiterals`, so a bracket inside a string + * or comment cannot unbalance the walk. + * @param {string} src + * @param {number} open + * @returns {number} + */ +export function matchBracket(src, open) { + const pairs = { '(': ')', '[': ']', '{': '}' }; + const close = pairs[src[open]]; + if (!close) return -1; + let depth = 0; + for (let i = open; i < src.length; i += 1) { + if (src[i] === src[open]) depth += 1; + else if (src[i] === close) { + depth -= 1; + if (depth === 0) return i + 1; + } + } + return -1; +} + +/** + * Parse the function expression starting at `from` in `blanked` (already through + * `blankLiterals`), bounded by `limit`. Handles `async`, `function`, and both + * arrow spellings — parenthesized and bare-identifier parameters. + * + * The parameter list is skipped as a UNIT rather than searching for the first + * `{`, because `async (payload = {}) => {` — the shape half of `sockets/voice.js` + * uses — puts an object literal in a default value before the body ever opens. + * + * A concise arrow body (`async () => save()`) has no braces; everything up to + * `limit` is taken as the body, so an `await` in it is still seen. + * @param {string} blanked + * @param {number} from + * @param {number} limit - exclusive upper bound (the enclosing call's `)`) + * @returns {{isAsync: boolean, start: number, text: string}|null} null when the + * argument is not a function expression (an identifier reference, say) + */ +export function parseCallbackAt(blanked, from, limit) { + let i = from; + const skipSpace = () => { while (i < limit && /\s/.test(blanked[i])) i += 1; }; + skipSpace(); + + const isAsync = /^async[\s(]/.test(blanked.slice(i, limit)); + if (isAsync) { i += 'async'.length; skipSpace(); } + + if (blanked.startsWith('function', i)) { + i += 'function'.length; + skipSpace(); + // Optional name. + while (i < limit && /[\w$]/.test(blanked[i])) i += 1; + skipSpace(); + // Parameter list. Only the `function` spelling reaches this — an arrow's + // params are consumed below, and skipping a second `(` after its `=>` would + // step straight over a parenthesized concise body (`async () => (await f())`), + // hiding every await in it. + if (blanked[i] === '(') i = matchBracket(blanked, i); + if (i === -1) return null; + skipSpace(); + } else { + // Arrow: parenthesized params, or a single bare identifier. + if (blanked[i] === '(') i = matchBracket(blanked, i); + else while (i < limit && /[\w$]/.test(blanked[i])) i += 1; + if (i === -1) return null; + skipSpace(); + if (!blanked.startsWith('=>', i)) return null; + i += 2; + skipSpace(); + } + + if (blanked[i] === '{') { + const end = matchBracket(blanked, i); + if (end === -1) return null; + return { isAsync, start: i, text: blanked.slice(i, end) }; + } + return { isAsync, start: i, text: blanked.slice(i, limit) }; +} + +/** + * `[start, end)` spans of every `try { … }` block in `body` that actually has a + * `catch` clause. A `try … finally` with no `catch` runs its cleanup and then + * re-throws, so it does NOT own the rejection — counting it would let the exact + * bug these guards exist for through. + */ +function tryBlockSpans(body) { + const spans = []; + for (const match of body.matchAll(/\btry\s*\{/g)) { + const open = body.indexOf('{', match.index); + const end = matchBracket(body, open); + // Comments between `}` and `catch` are already blanked to spaces. + if (end !== -1 && /^\s*catch\b/.test(body.slice(end))) spans.push([open, end]); + } + return spans; +} + +/** + * The member/call chain awaited at `start` — e.g. for + * `await pm2.restart(name)\n .catch(err => …)` it returns the whole thing, + * newline continuation included, so a trailing `.catch(` is visible. + */ +function awaitedChain(body, start) { + let i = start; + while (i < body.length && /\s/.test(body[i])) i += 1; + const begin = i; + while (i < body.length) { + const c = body[i]; + if (/[\w$.?]/.test(c)) { i += 1; continue; } + if (c === '(' || c === '[') { + const end = matchBracket(body, i); + if (end === -1) break; + i = end; + continue; + } + if (/\s/.test(c)) { + // Only a `.` continuation may follow whitespace; anything else ends the chain. + let j = i; + while (j < body.length && /\s/.test(body[j])) j += 1; + if (body[j] === '.') { i = j; continue; } + break; + } + break; + } + return body.slice(begin, i); +} + +/** + * True when the LAST link of an awaited chain is `.catch(…)`. It has to be the + * last one: `await work().catch(recover).then(rethrow)` handles a rejection from + * `work()` and then hands the awaited promise straight back to `then`, so the + * chain as a whole can still reject. + */ +function chainEndsInCatch(chain) { + const at = chain.lastIndexOf('.catch'); + if (at === -1) return false; + let i = at + '.catch'.length; + while (i < chain.length && /\s/.test(chain[i])) i += 1; + if (chain[i] !== '(') return false; + const end = matchBracket(chain, i); + return end !== -1 && chain.slice(end).trim() === ''; +} + +/** + * Awaits in `body` (already through `blankLiterals`) that neither sit inside a + * `try`/`catch` nor end in `.catch(…)`. Returns the offending chain text for + * each, so a failure message points at the expression rather than at a line + * number that rebases away. + * + * Deliberately lexical rather than scope-aware: an `await` inside a nested + * `async` callback declared within the body is attributed to the body. That + * errs toward flagging — a fire-and-forget inner async callback has the same + * ownerless-rejection problem — but it means the fix may belong on the inner + * function. An await guarded by a helper the body calls, rather than by its own + * `try`, also reads as unguarded. + * @param {string} body + * @returns {string[]} + */ +export function unguardedAwaits(body) { + const spans = tryBlockSpans(body); + const offenders = []; + for (const match of body.matchAll(/\bawait\b/g)) { + if (spans.some(([from, to]) => match.index > from && match.index < to)) continue; + const chain = awaitedChain(body, match.index + match[0].length); + if (chainEndsInCatch(chain)) continue; + offenders.push(`await ${chain.replace(/\s+/g, ' ').trim()}`); + } + return offenders; +} diff --git a/server/sockets/asyncHandlerGuard.test.js b/server/sockets/asyncHandlerGuard.test.js new file mode 100644 index 0000000000..74116bd88a --- /dev/null +++ b/server/sockets/asyncHandlerGuard.test.js @@ -0,0 +1,251 @@ +/** + * Directory guard: an `async` Socket.IO handler may not let an `await` reject. + * + * ## The bug class + * + * socket.on('voice:call:detach', async () => { + * emitCallState(await detachHost(socket)); // ← rejects on a failed teardown + * }); + * + * Socket.IO hands the promise a listener returns to nobody, and a handler runs + * outside the Express request lifecycle, so there is no `next(err)` for a throw + * to bubble to. A rejection therefore surfaces as an unhandled rejection, which + * Node >= 15 treats as fatal — the server process dies, taking every agent run, + * PTY session and media job with it. `voice:call:detach` shipped exactly that + * (#5661), and it held across the other handlers here by review alone. + * + * `server/sockets/` is the largest population of "outside the request lifecycle" + * async code in the tree. This is the same rule and the same shape as + * `server/timerCallbackConventions.test.js` — the two share their lexer, + * callback parser and await checker via `server/lib/sourceScan.js`, so they + * cannot drift on what "owns its rejection" means. + * + * ## Scope + * + * Every `.on('', …)` in this directory, not just `socket.on` — + * `ns.on('connection', …)` and the `pm2 logs` child's `.on('data', …)` in + * `logs.js` sit in the identical blast radius, and scoping to the literal + * `socket.` would let the next one in unnoticed. `server/routes/` is out of + * scope (the centralized error middleware owns those), and so are timer and + * child-process callbacks (their own guard files). + * + * ## Allowlist + * + * None, on purpose. If you are reading this because the scan just failed, wrap + * the handler body in `try { … } catch (err) { console.error(…); }` or append + * `.catch(…)` to the awaited expression. + */ + +import { describe, it, expect } from 'vitest'; +import { readdirSync, readFileSync } from 'node:fs'; +import { dirname, join } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { blankLiterals, matchBracket, parseCallbackAt, unguardedAwaits } from '../lib/sourceScan.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); + +// `.once` as well as `.on`: a one-shot listener returns its promise to nobody +// exactly the same way, so leaving it out would be a silent hole rather than a +// narrower rule. +const REGISTRATION = /(?.on('', )` registration in `src`, with the + * callback body captured whole by a bracket-balanced walk. + * + * Bracket-balanced rather than a fixed character window: a window reads the NEXT + * statement and attributes it to this handler, so an unguarded handler followed + * by a guarded one would pass on its neighbour's `try`. + * + * The event name is read out of the ORIGINAL source at the offsets the blanked + * copy reports — `blankLiterals` preserves length, so every index still maps + * back. A registration with a computed event name is skipped rather than + * reported under a wrong name. + * @param {string} src + * @returns {{event: string, isAsync: boolean, body: string, line: number}[]} + */ +export function socketHandlers(src) { + const blanked = blankLiterals(src); + const handlers = []; + for (const match of blanked.matchAll(REGISTRATION)) { + const callOpen = match.index + match[0].length - 1; + const callEnd = matchBracket(blanked, callOpen); + if (callEnd === -1) continue; + const comma = firstArgumentEnd(blanked, callOpen + 1, callEnd - 1); + if (comma === -1) continue; + + const event = /^(['"])((?:[^'"\\]|\\.)*)\1$/.exec(src.slice(callOpen + 1, comma).trim())?.[2]; + if (event === undefined) continue; + + const callback = parseCallbackAt(blanked, comma + 1, callEnd - 1); + if (!callback) continue; + handlers.push({ + event, + isAsync: callback.isAsync, + body: callback.text, + line: blanked.slice(0, match.index).split('\n').length, + }); + } + return handlers; +} + +/** Every unguarded await in every async event handler in one file's source. */ +export function findUnguardedHandlerAwaits(src) { + return socketHandlers(src) + .filter((handler) => handler.isAsync) + .flatMap((handler) => unguardedAwaits(handler.body) + .map((chain) => `line ${handler.line} '${handler.event}': ${chain}`)); +} + +const socketFiles = readdirSync(HERE) + .filter((f) => f.endsWith('.js') && !f.includes('.test.')) + .sort(); + +describe('async socket handlers own their rejections (#5661)', () => { + it('finds the handlers it is meant to guard', () => { + // Without this, a refactor of the registration shape would leave the scan + // below iterating nothing and passing green forever. + expect(socketFiles.length).toBeGreaterThan(3); + const handlers = socketFiles.flatMap((f) => socketHandlers(readFileSync(join(HERE, f), 'utf8'))); + expect(handlers.filter((h) => h.isAsync).length).toBeGreaterThan(10); + // Sync handlers must survive extraction too, or `isAsync` is doing nothing. + expect(handlers.filter((h) => !h.isAsync).length).toBeGreaterThan(0); + expect(handlers.map((h) => h.event)).toEqual(expect.arrayContaining([ + 'voice:call:detach', 'logs:subscribe', 'shell:start', 'app:update', + ])); + }); + + it('has no unguarded await in any async socket handler', () => { + const violations = socketFiles.flatMap((file) => ( + findUnguardedHandlerAwaits(readFileSync(join(HERE, file), 'utf8')) + .map((hit) => `server/sockets/${file} ${hit}`) + )); + + expect( + violations, + 'These awaits sit in an `async` Socket.IO handler with nothing to own a rejection. ' + + 'A handler runs outside the request lifecycle and nobody holds the promise it returns, ' + + 'so a rejected await becomes an unhandled rejection — fatal on Node >= 15, which is how ' + + 'voice:call:detach could have killed the server on a failed teardown (#5661).\n' + + 'Fix: wrap the body in `try { … } catch (err) { console.error(`❌ …: ${err.message}`); }`, ' + + 'or append `.catch(…)` to the awaited expression.\n' + + `Offenders:\n ${violations.join('\n ')}`, + ).toEqual([]); + }); +}); + +// Guards the guard: if the recognizer stops seeing the broken shape, the scan +// above goes green and the bug class walks straight back in. +describe('the socket-handler recognizer', () => { + it('flags a bare await in an async handler', () => { + expect(findUnguardedHandlerAwaits("socket.on('x', async () => { await boom(); });")) + .toEqual(["line 1 'x': await boom()"]); + // An early return before the await does not make it safe — the fableLoomHosted + // handlers all open with a role check. + expect(findUnguardedHandlerAwaits(` + socket.on('hosted:mic:start', async () => { + if (socket.hostedRole !== 'audience') return; + await startListening(sessionId); + }); + `)).toEqual(["line 2 'hosted:mic:start': await startListening(sessionId)"]); + }); + + it('does not accept a try that only exists in a comment', () => { + // The comment blanking is what stops a rule being *described* in prose from + // satisfying the scan. Drop it and this fixture reads as guarded. + expect(findUnguardedHandlerAwaits(` + socket.on('x', async () => { + // try { … } catch — describing the rule, not applying it + await boom(); + }); + `)).toEqual(["line 2 'x': await boom()"]); + }); + + it('accepts the two guarded shapes', () => { + // A default-valued parameter puts an object literal before the body ever + // opens — half of voice.js is spelled this way. + expect(findUnguardedHandlerAwaits(` + socket.on('voice:text', async (payload = {}) => { + try { + await respond(payload); + } catch (err) { + console.error(\`❌ voice:text failed: \${err.message}\`); + } + }); + `)).toEqual([]); + // Chained across lines — the shape logs.js uses on its pm2-home lookups. A + // scan that ended the statement at the newline would never see the .catch. + expect(findUnguardedHandlerAwaits(` + socket.on('logs:subscribe', async ({ appId }) => { + const home = await getAppById(appId) + .then((app) => app?.pm2Home || null) + .catch((err) => { console.error(\`❌ \${err.message}\`); return null; }); + stream(home); + }); + `)).toEqual([]); + }); + + it('captures each handler body whole, not a window into the next one', () => { + const found = findUnguardedHandlerAwaits(` + socket.on('unguarded', async () => { + await boom(); + }); + socket.on('guarded', async () => { + try { await fine(); } catch (err) { console.error(err.message); } + }); + `); + // The offender must not borrow its neighbour's \`try\`, and the guarded + // handler must not be dragged down by its neighbour's bare await. + expect(found).toEqual(["line 2 'unguarded': await boom()"]); + }); + + it('leaves synchronous handlers and non-function arguments alone', () => { + expect(findUnguardedHandlerAwaits("socket.on('sync', (chunk) => { buffer.push(chunk); });")).toEqual([]); + expect(socketHandlers("emitter.on('forwarded', handlerRef);")).toEqual([]); + // A computed event name is skipped rather than reported under a wrong name. + expect(socketHandlers('socket.on(EVENT, async () => { await boom(); });')).toEqual([]); + }); + + it('reads a non-socket emitter, and a one-shot listener, in this directory too', () => { + // `ns.on('connection', …)` and the pm2-logs child's `.on('data', …)` are in + // the same blast radius; scoping the scan to the literal `socket.` would let + // the next one in unnoticed. `.once` returns its promise to nobody the same + // way `.on` does. + expect(findUnguardedHandlerAwaits("logProcess.stdout.on('data', async (chunk) => { await flush(chunk); });")) + .toEqual(["line 1 'data': await flush(chunk)"]); + expect(findUnguardedHandlerAwaits("socket.once('shell:attach', async () => { await attach(); });")) + .toEqual(["line 1 'shell:attach': await attach()"]); + }); + + it('sees a parenthesized concise body rather than stepping over it', () => { + // `parseCallbackAt` used to skip a second `(` after the `=>`, reading it as + // a parameter list and jumping the whole body — so every await inside was + // invisible and the handler passed green. + expect(findUnguardedHandlerAwaits("socket.on('x', async () => (await boom()));")) + .toEqual(["line 1 'x': await boom()"]); + }); + + it('reports the voice call handlers as guarded', () => { + // The site that motivated the rule — pinned so a revert is caught here and + // not only by the directory-wide scan. + const src = readFileSync(join(HERE, 'voice.js'), 'utf8'); + expect(src).toContain("socket.on('voice:call:detach'"); + expect(findUnguardedHandlerAwaits(src)).toEqual([]); + }); +}); diff --git a/server/timerCallbackConventions.test.js b/server/timerCallbackConventions.test.js index d20f6ed49e..53b590fa49 100644 --- a/server/timerCallbackConventions.test.js +++ b/server/timerCallbackConventions.test.js @@ -65,6 +65,14 @@ * * Tightening any of these means moving to an AST pass; the shapes above are * rare enough here that the scan earns its keep as-is. + * + * ## Where the machinery lives + * + * The lexer, bracket matcher, callback parser and the await checker are in + * `server/lib/sourceScan.js`, shared with `sockets/asyncHandlerGuard.test.js` — + * the same rule for Socket.IO handlers. Only the `setTimeout`/`setInterval` + * recognizer below is timer-specific; keeping the rest shared is what stops the + * two guards drifting on what "owns its rejection" means. */ import { describe, it, expect } from 'vitest'; @@ -72,134 +80,17 @@ import { execFileSync } from 'child_process'; import { readFileSync } from 'fs'; import { dirname, join } from 'path'; import { fileURLToPath } from 'url'; +import { blankLiterals, matchBracket, parseCallbackAt, unguardedAwaits } from './lib/sourceScan.js'; const SERVER_ROOT = dirname(fileURLToPath(import.meta.url)); -/** - * Replace the contents of comments, string/template literals, and regex - * literals with spaces, preserving length so every index still maps back to the - * original source. Everything downstream (brace walks, `await` matching) then - * sees code only. - */ -export function blankLiterals(src) { - const out = src.split(''); - const n = src.length; - const blank = (i) => { if (i < n && src[i] !== '\n') out[i] = ' '; }; - // Brace depths of the code regions opened by `${` inside template literals, - // so a nested template resumes correctly at its closing `}`. - const templateStack = []; - let inTemplate = false; - let braceDepth = 0; - // A `/` starts a regex only where a value cannot precede it. Tracking the last - // significant character is the standard heuristic and is what keeps a - // character class like /["']/ from being read as a string opener. - let prevSignificant = ''; - let i = 0; - - while (i < n) { - const c = src[i]; - - if (inTemplate) { - if (c === '\\') { blank(i); blank(i + 1); i += 2; continue; } - if (c === '`') { blank(i); i += 1; inTemplate = false; prevSignificant = '`'; continue; } - if (c === '$' && src[i + 1] === '{') { - blank(i); blank(i + 1); i += 2; - templateStack.push(braceDepth); - braceDepth = 0; - inTemplate = false; - prevSignificant = '{'; - continue; - } - blank(i); i += 1; continue; - } - - if (c === '/' && src[i + 1] === '/') { - while (i < n && src[i] !== '\n') { blank(i); i += 1; } - continue; - } - if (c === '/' && src[i + 1] === '*') { - while (i < n && !(src[i] === '*' && src[i + 1] === '/')) { blank(i); i += 1; } - blank(i); blank(i + 1); i += 2; - continue; - } - if (c === "'" || c === '"') { - const quote = c; - blank(i); i += 1; - while (i < n && src[i] !== quote) { - if (src[i] === '\\') { blank(i); i += 1; } - blank(i); i += 1; - } - blank(i); i += 1; - prevSignificant = quote; - continue; - } - if (c === '`') { blank(i); i += 1; inTemplate = true; continue; } - if (c === '/' && regexCanStartAfter(prevSignificant)) { - blank(i); i += 1; - let inClass = false; - while (i < n && src[i] !== '\n') { - if (src[i] === '\\') { blank(i); blank(i + 1); i += 2; continue; } - if (src[i] === '[') inClass = true; - else if (src[i] === ']') inClass = false; - else if (src[i] === '/' && !inClass) break; - blank(i); i += 1; - } - blank(i); i += 1; - // Blank the flags too so `gi` can't be read as an identifier. - while (i < n && /[a-z]/.test(src[i])) { blank(i); i += 1; } - prevSignificant = ')'; - continue; - } - - if (c === '{') braceDepth += 1; - else if (c === '}') { - if (braceDepth === 0 && templateStack.length > 0) { - blank(i); i += 1; - braceDepth = templateStack.pop(); - inTemplate = true; - continue; - } - braceDepth -= 1; - } - if (!/\s/.test(c)) prevSignificant = c; - i += 1; - } - - return out.join(''); -} - -/** - * True when a `/` at this position opens a regex literal rather than division. - * A regex can only follow a position where an operand cannot: an operator, an - * opening bracket, a statement boundary, or the start of the file. - */ -function regexCanStartAfter(prev) { - return prev === '' || '(,=:[!&|?{};+-*%~^<>'.includes(prev); -} - -/** Index just past the bracket matching the one at `open`, or -1. */ -function matchBracket(src, open) { - const pairs = { '(': ')', '[': ']', '{': '}' }; - const close = pairs[src[open]]; - let depth = 0; - for (let i = open; i < src.length; i += 1) { - if (src[i] === src[open]) depth += 1; - else if (src[i] === close) { - depth -= 1; - if (depth === 0) return i + 1; - } - } - return -1; -} - -const TIMER_OPEN = /\b(setTimeout|setInterval)\s*\(\s*async\b/g; +// The lookahead leaves `match.index + match[0].length` sitting ON `async`, which +// is where `parseCallbackAt` expects to start. +const TIMER_OPEN = /\b(setTimeout|setInterval)\s*\(\s*(?=async\b)/g; /** * The body text of every `setTimeout(async …)` / `setInterval(async …)` in * `blanked` (which must already have been through `blankLiterals`). - * - * A concise arrow body (`async () => save()`) has no braces; the whole remaining - * argument list is taken as the body so an `await` in it is still seen. */ export function timerCallbackBodies(blanked) { const bodies = []; @@ -207,124 +98,13 @@ export function timerCallbackBodies(blanked) { const callOpen = blanked.indexOf('(', match.index); const callEnd = matchBracket(blanked, callOpen); if (callEnd === -1) continue; - - let i = match.index + match[0].length; - const skipSpace = () => { while (i < callEnd && /\s/.test(blanked[i])) i += 1; }; - skipSpace(); - - if (blanked.startsWith('function', i)) { - i += 'function'.length; - skipSpace(); - // Optional name. - while (i < callEnd && /[\w$]/.test(blanked[i])) i += 1; - skipSpace(); - } else { - // Arrow: parenthesized params, or a single bare identifier. - if (blanked[i] === '(') i = matchBracket(blanked, i); - else while (i < callEnd && /[\w$]/.test(blanked[i])) i += 1; - if (i === -1) continue; - skipSpace(); - if (!blanked.startsWith('=>', i)) continue; - i += 2; - } - skipSpace(); - - if (blanked[i] === '(') i = matchBracket(blanked, i); // arrow with parenthesized params - if (i === -1) continue; - skipSpace(); - - if (blanked[i] === '{') { - const end = matchBracket(blanked, i); - if (end === -1) continue; - bodies.push({ start: i, text: blanked.slice(i, end) }); - } else { - // Concise body — take the rest of the call's argument list. - bodies.push({ start: i, text: blanked.slice(i, callEnd - 1) }); - } + // `callEnd - 1` is the call's `)` — the bound a concise arrow body runs to. + const callback = parseCallbackAt(blanked, match.index + match[0].length, callEnd - 1); + if (callback) bodies.push({ start: callback.start, text: callback.text }); } return bodies; } -/** - * `[start, end)` spans of every `try { … }` block in `body` that actually has a - * `catch` clause. A `try … finally` with no `catch` runs its cleanup and then - * re-throws, so it does NOT own the rejection — counting it would let the exact - * bug this guard exists for through. - */ -function tryBlockSpans(body) { - const spans = []; - for (const match of body.matchAll(/\btry\s*\{/g)) { - const open = body.indexOf('{', match.index); - const end = matchBracket(body, open); - // Comments between `}` and `catch` are already blanked to spaces. - if (end !== -1 && /^\s*catch\b/.test(body.slice(end))) spans.push([open, end]); - } - return spans; -} - -/** - * The member/call chain awaited at `start` — e.g. for - * `await pm2.restart(name)\n .catch(err => …)` it returns the whole thing, - * newline continuation included, so a trailing `.catch(` is visible. - */ -function awaitedChain(body, start) { - let i = start; - while (i < body.length && /\s/.test(body[i])) i += 1; - const begin = i; - while (i < body.length) { - const c = body[i]; - if (/[\w$.?]/.test(c)) { i += 1; continue; } - if (c === '(' || c === '[') { - const end = matchBracket(body, i); - if (end === -1) break; - i = end; - continue; - } - if (/\s/.test(c)) { - // Only a `.` continuation may follow whitespace; anything else ends the chain. - let j = i; - while (j < body.length && /\s/.test(body[j])) j += 1; - if (body[j] === '.') { i = j; continue; } - break; - } - break; - } - return body.slice(begin, i); -} - -/** - * True when the LAST link of an awaited chain is `.catch(…)`. It has to be the - * last one: `await work().catch(recover).then(rethrow)` handles a rejection from - * `work()` and then hands the awaited promise straight back to `then`, so the - * chain as a whole can still reject. - */ -function chainEndsInCatch(chain) { - const at = chain.lastIndexOf('.catch'); - if (at === -1) return false; - let i = at + '.catch'.length; - while (i < chain.length && /\s/.test(chain[i])) i += 1; - if (chain[i] !== '(') return false; - const end = matchBracket(chain, i); - return end !== -1 && chain.slice(end).trim() === ''; -} - -/** - * Awaits in `body` that neither sit inside a `try`/`catch` nor end in `.catch(…)`. - * Returns the offending chain text for each, so a failure message points at the - * expression rather than at a line number that rebases away. - */ -export function unguardedAwaits(body) { - const spans = tryBlockSpans(body); - const offenders = []; - for (const match of body.matchAll(/\bawait\b/g)) { - if (spans.some(([from, to]) => match.index > from && match.index < to)) continue; - const chain = awaitedChain(body, match.index + match[0].length); - if (chainEndsInCatch(chain)) continue; - offenders.push(`await ${chain.replace(/\s+/g, ' ').trim()}`); - } - return offenders; -} - /** Every unguarded await in every async timer callback in one file's source. */ export function findUnguardedTimerAwaits(src) { const blanked = blankLiterals(src); @@ -403,6 +183,11 @@ describe('the timer-callback recognizer', () => { // A concise body has no braces to walk, but still owns its await. expect(findUnguardedTimerAwaits('setTimeout(async () => await f(), 100);')) .toEqual(['line 1: await f()']); + // Parenthesized concise body. The parser used to read the `(` after the `=>` + // as a parameter list and jump the whole body, so the await inside was + // invisible and this passed green. + expect(findUnguardedTimerAwaits('setTimeout(async () => (await f()), 100);')) + .toEqual(['line 1: await f()']); }); it('accepts a try-wrapped body', () => {