mirror of
https://github.com/ether/etherpad-lite.git
synced 2026-07-18 00:57:55 +00:00
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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
* 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) <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
86c56cf827
commit
7ea9970648
6 changed files with 84 additions and 17 deletions
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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]!
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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': {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue