From 7ea99706483443239bbbc0f2df9aff8ab5de4805 Mon Sep 17 00:00:00 2001 From: John McLear Date: Sun, 7 Jun 2026 18:17:39 +0100 Subject: [PATCH] Hardening: API request handling, random IDs, and plugin loading (#7906) * Hardening: API request handling, token generation, and plugin loading - pad_utils.randomString: generate the random IDs via crypto.getRandomValues (CSPRNG) instead of Math.random. - OAuth2Provider: constant-time password comparison and a uniform failure delay on the OIDC interaction login; own-property user lookup. - API.appendChatMessage: require the pad to already exist (getPadSafe), consistent with the other content API methods. - RestAPI /api/2: forward only the authorization header rather than merging all request headers into the API field set. - LinkInstaller: validate plugin dependency names before building filesystem paths from them. - admin file server: return a generic error message and log details server-side. Co-Authored-By: Claude Opus 4.8 (1M context) * Address review: timingSafeEqual on raw bytes; align /api/2 auth fallback - OAuth2Provider.constantTimeEquals: compare raw UTF-8 bytes with crypto.timingSafeEqual instead of hashing them first. Resolves the CodeQL "password hash with insufficient computational effort" alert while keeping a content-independent comparison (length difference is covered by the uniform failure delay). - RestAPI /api/2: fall back to the authorization header whenever the field is falsy (not only null), matching the openapi.ts handler so the two routers authenticate identically (Qodo). Co-Authored-By: Claude Opus 4.8 (1M context) * appendChatMessage: throw explicit error instead of getPadSafe (review) Per review: replace the getPadSafe(padID, true) existence check with an explicit `throw new CustomError('padID does not exist', 'apierror')` so chat messages can't create pads, without fetching the pad. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- src/node/db/API.ts | 8 ++++- src/node/handler/RestAPI.ts | 10 +++++- src/node/hooks/express/admin.ts | 5 ++- src/node/security/OAuth2Provider.ts | 42 +++++++++++++++++++------ src/static/js/pad_utils.ts | 19 ++++++++--- src/static/js/pluginfw/LinkInstaller.ts | 17 ++++++++++ 6 files changed, 84 insertions(+), 17 deletions(-) diff --git a/src/node/db/API.ts b/src/node/db/API.ts index 68c953f28..bdc659b05 100644 --- a/src/node/db/API.ts +++ b/src/node/db/API.ts @@ -414,7 +414,13 @@ exports.appendChatMessage = async (padID: string, text: string|object, authorID: time = Date.now(); } - // @TODO - missing getPadSafe() call ? + // Reject messages addressed to a pad that doesn't exist. Without this check + // the downstream padManager.getPad() would create the pad on demand with + // default content, so the documented {code:1,"padID does not exist"} result + // would never be returned. + if (!await padManager.doesPadExists(padID)) { + throw new CustomError('padID does not exist', 'apierror'); + } // save chat message to database and send message to all connected clients await padMessageHandler.sendChatMessageToPadClients(new ChatMessage(text, authorID, time), padID); diff --git a/src/node/handler/RestAPI.ts b/src/node/handler/RestAPI.ts index 066c2d596..4bae9895c 100644 --- a/src/node/handler/RestAPI.ts +++ b/src/node/handler/RestAPI.ts @@ -1471,7 +1471,15 @@ export const expressCreateServer = async (hookName: string, {app}: ArgsExpressTy } } - const fields = Object.assign({}, headers, params, query, formData); + // Merge with clear precedence: body > query > path params, matching the + // openapi.ts handler. Forward only the authorization header explicitly + // instead of merging all request headers into the field set. + const fields = Object.assign({}, params, query, formData); + if (headers && headers.authorization) { + // Match the openapi.ts handler: fall back to the header whenever the + // field value is falsy (absent or empty), not only when it is null. + fields.authorization = fields.authorization || headers.authorization; + } if (mapping.has(method) && pathToFunction in mapping.get(method)!) { const {apiVersion, functionName} = mapping.get(method)![pathToFunction]! diff --git a/src/node/hooks/express/admin.ts b/src/node/hooks/express/admin.ts index fb6cbe69c..d264534a1 100644 --- a/src/node/hooks/express/admin.ts +++ b/src/node/hooks/express/admin.ts @@ -66,8 +66,11 @@ exports.expressCreateServer = (hookName: string, args: ArgsExpressType, cb: Func // read file from file system fs.readFile(pathname, function (err, data) { if (err) { + // Log the detailed error server-side; return a generic message to the + // client rather than echoing the filesystem error. + console.error(`admin: error reading ${pathname}: ${err}`); res.statusCode = 500; - res.end(`Error getting the file: ${err}.`); + res.end('Error getting the file.'); } else { let dataToSend:Buffer|string = data // if the file is found, set Content-type and send data diff --git a/src/node/security/OAuth2Provider.ts b/src/node/security/OAuth2Provider.ts index 6c069359d..8bfb46831 100644 --- a/src/node/security/OAuth2Provider.ts +++ b/src/node/security/OAuth2Provider.ts @@ -9,6 +9,21 @@ import express from 'express'; import {format} from 'url' import {ParsedUrlQuery} from "node:querystring"; import {MapArrayType} from "../types/MapType"; +import crypto from "node:crypto"; + +// Small fixed delay applied to every failed interactive login, mirroring +// webaccess.authnFailureDelayMs, so failures take a consistent amount of time. +const OAUTH_LOGIN_FAILURE_DELAY_MS = 1000; + +// Constant-time string comparison using crypto.timingSafeEqual. Unequal-length +// inputs short-circuit to false (that length difference is covered by the +// uniform failure delay below). The raw bytes are compared directly — the +// values are not hashed/stored, this only avoids a content-dependent compare. +const constantTimeEquals = (a: string, b: string): boolean => { + const ba = Buffer.from(String(a), 'utf8'); + const bb = Buffer.from(String(b), 'utf8'); + return ba.length === bb.length && crypto.timingSafeEqual(ba, bb); +}; const configuration: Configuration = { scopes: ['openid', 'profile', 'email'], @@ -171,21 +186,28 @@ export const expressCreateServer = async (hookName: string, args: ArgsExpressTyp admin: boolean; } } - const usersArray1 = Object.keys(users).map((username) => ({ - username, - ...users[username] - })); - const account = usersArray1.find((user) => user.username === login as unknown as string && user.password === password as unknown as string); + const loginStr = String(login ?? ''); + const passwordStr = String(password ?? ''); + // Look up by own property only, then compare the password + // with constantTimeEquals. + const user = Object.prototype.hasOwnProperty.call(users, loginStr) + ? users[loginStr] : undefined; + const passwordOk = user != null && + constantTimeEquals(passwordStr, String(user.password)); + const account = passwordOk ? {username: loginStr, ...user} : undefined; if (!account) { + // Apply the failure delay and stop here (explicit break) + // so a failed login never reaches the grant branch. + await new Promise((resolve) => + setTimeout(resolve, OAUTH_LOGIN_FAILURE_DELAY_MS)); res.setHeader('Content-Type', 'application/json'); res.end(JSON.stringify({error: "Invalid login"})); + break; } - if (account) { - await oidc.interactionFinished(req, res, { - login: {accountId: account.username} - }, {mergeWithLastSubmission: false}); - } + await oidc.interactionFinished(req, res, { + login: {accountId: account.username} + }, {mergeWithLastSubmission: false}); break; } case 'consent': { diff --git a/src/static/js/pad_utils.ts b/src/static/js/pad_utils.ts index 194974523..997dd2be6 100644 --- a/src/static/js/pad_utils.ts +++ b/src/static/js/pad_utils.ts @@ -33,11 +33,22 @@ import jsCookie, {CookiesStatic} from 'js-cookie' */ export const randomString = (len?: number) => { const chars = '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'; - let randomstring = ''; len = len || 20; - for (let i = 0; i < len; i++) { - const rnum = Math.floor(Math.random() * chars.length); - randomstring += chars.substring(rnum, rnum + 1); + // Generate these IDs from crypto.getRandomValues (a CSPRNG) rather than + // Math.random. getRandomValues is available in browsers and in Node >= 20 + // (Node 24 is required) and, unlike crypto.subtle, needs no secure context. + // Rejection-sample to the largest multiple of chars.length (62*4=248) to + // avoid modulo bias. + const maxUnbiased = 256 - (256 % chars.length); // 248 + let randomstring = ''; + while (randomstring.length < len) { + const bytes = new Uint8Array(len - randomstring.length); + globalThis.crypto.getRandomValues(bytes); + for (const b of bytes) { + if (b >= maxUnbiased) continue; // drop biased samples, keep going + randomstring += chars[b % chars.length]; + if (randomstring.length === len) break; + } } return randomstring; }; diff --git a/src/static/js/pluginfw/LinkInstaller.ts b/src/static/js/pluginfw/LinkInstaller.ts index 1c56a5c46..430e1fa91 100644 --- a/src/static/js/pluginfw/LinkInstaller.ts +++ b/src/static/js/pluginfw/LinkInstaller.ts @@ -177,7 +177,20 @@ export class LinkInstaller { } } + // A dependency name comes from a plugin's package.json and is used to build + // filesystem paths (readFileSync / symlinkSync), so validate it is a plain npm + // package name (optional scope, no path separators or "..") before use. + private isValidDependencyName(dependency: string): boolean { + return typeof dependency === 'string' && + !dependency.includes('..') && + /^(@[a-z0-9-._~]+\/)?[a-z0-9-._~]+$/i.test(dependency); + } + private async addSubDependency(plugin: string, dependency: string) { + if (!this.isValidDependencyName(dependency)) { + console.error(`Skipping plugin dependency with invalid name: ${dependency}`) + return + } if (this.dependenciesMap.has(dependency)) { // We already added the sub dependency this.dependenciesMap.get(dependency)?.add(plugin) @@ -201,6 +214,10 @@ export class LinkInstaller { } private linkDependency(dependency: string) { + if (!this.isValidDependencyName(dependency)) { + console.error(`Skipping plugin dependency with invalid name: ${dependency}`) + return + } try { // Check if the dependency is already installed accessSync(path.join(node_modules, dependency), constants.F_OK)