Fix review findings from PR #6029

Security
- Redact certbot output before it reaches the API: strip the
  certificate's DNS credential values, URL query strings and userinfo,
  secret-looking key=value pairs and Bearer/Basic tokens. DNS plugins
  can echo request URLs carrying API keys. The raw CommandError is no
  longer attached as `previous`, since the error handler returns
  `previous` to the client on certificate routes.
- Rate limit login/2FA on the admin nginx's real TCP peer, passed as
  X-NPM-Peer-Addr ($realip_remote_addr), instead of X-Forwarded-For.
  nginx trusts X-Real-IP from private ranges, so a client on the LAN or
  Docker network could get a fresh bucket on every request. Behind
  another proxy, limits now apply per account through that proxy.
- Bound rate limiter memory: hash keys over 128 chars and cap each store
  at 50k entries, removing expired entries first and then the oldest.

Correctness
- Proxy host modal: send location advanced configs unchanged for
  non-admins instead of looking them up by path, which dropped the
  config from a renamed location.
- Location advanced config: locations that omit advanced_config now get
  a config only after explicit ones are matched, so a new location at
  an old path can't take the config from the renamed location.
- Certificate upload: check that the certificate matches the private
  key (uploaded or stored) before writing, so a mismatched pair can't
  break nginx -t.
- Certificate delete: keep the Let's Encrypt files when revoke fails and
  hosts still use the certificate, so their configs still load.

Cleanup
- Make the rate-limit certbot hint first in the list, as its comment
  says.
- Proxy host getCount uses the permissions access has already loaded
  instead of querying user_permission again.
- Rate limiter: register the success refund listener in one place and
  call it a fixed window, not a sliding one.

backend/internal/certificate.js

@@ -46,6 +46,11 @@ const certbotNoise = [
 // provider, so a generic message (e.g. "Authentication error" from another DNS plugin) isn't
 // reported as a Cloudflare problem.
 const certbotHints = [
+	// Checked first: a rate limit is unambiguous and the most useful thing to report
+	[
+		/too many (certificates|failed authorizations|new orders)|rateLimited/i,
+		() => "Let's Encrypt rate limit reached. Wait before retrying (see https://letsencrypt.org/docs/rate-limits/).",
+	],
 	[
 		/expected \S+ to be a symlink/i,
 		() =>
@@ -56,11 +61,6 @@ const certbotHints = [
 		() =>
 			"This certificate's files in /etc/letsencrypt are damaged or missing. Restore /etc/letsencrypt from a backup, or delete this certificate and create it again.",
 	],
-	// Checked first: a rate limit is unambiguous and the most useful thing to report
-	[
-		/too many (certificates|failed authorizations|new orders)|rateLimited/i,
-		() => "Let's Encrypt rate limit reached. Wait before retrying (see https://letsencrypt.org/docs/rate-limits/).",
-	],
 	[
 		/\b9109\b.*?location: ([0-9a-f.:]+)/i,
 		(m) => {
@@ -89,15 +89,54 @@ const certbotHints = [
 ];
 
 /**
- * Turns certbot's stderr into a message the UI can show: a hint for common causes plus certbot's own reason.
- * Certbot never prints DNS provider credentials.
+ * The values in a DNS credentials file ("key = value" lines), for redacting from certbot output.
  *
- * @param   {String} stderr
- * @param   {String} [dnsProvider]  The certificate's DNS provider, if it uses a DNS challenge
+ * @param   {String} [credentials]
+ * @returns {String[]}
+ */
+const credentialValues = (credentials) =>
+	`${credentials || ""}`
+		.split("\n")
+		.map((line) => line.replace(/^[^=:]*[=:]/, "").trim())
+		.filter((value) => value.length >= 4);
+
+/**
+ * Removes anything that could be a secret from certbot output before it leaves the server.
+ * DNS plugins can echo provider errors verbatim, including request URLs that carry API keys in
+ * the query string (e.g. "401 Client Error ... for url: https://...?token=..."), so this strips
+ * the given credential values, URL query strings and userinfo, and key=value pairs whose name
+ * looks secret.
+ *
+ * @param   {String}   text
+ * @param   {String[]} [secrets]
  * @returns {String}
  */
-const certbotFailureMessage = (stderr, dnsProvider) => {
-	const reason = `${stderr || ""}`
+const redactCertbotOutput = (text, secrets = []) => {
+	let out = `${text || ""}`;
+	for (const secret of [...secrets].sort((a, b) => b.length - a.length)) {
+		out = out.split(secret).join("[REDACTED]");
+	}
+	return out
+		.replace(/\b(Bearer|Basic)\s+[A-Za-z0-9._~+/=-]+/gi, "$1 [REDACTED]")
+		.replace(/(\b[a-z][a-z0-9+.-]*:\/\/)[^/\s@]*@/gi, "$1[REDACTED]@")
+		.replace(/(\b[a-z][a-z0-9+.-]*:\/\/[^\s?#]*)\?[^\s#]*/gi, "$1?[REDACTED]")
+		.replace(
+			/\b([\w-]*(?:token|key|secret|password|passwd|pass|auth|credential|signature|sig)[\w-]*)(["']?\s*[=:]\s*["']?)[^\s"'&,;]+/gi,
+			"$1$2[REDACTED]",
+		);
+};
+
+/**
+ * Turns certbot's stderr into a message the UI can show: a hint for common causes plus certbot's own reason.
+ * The output is redacted first (see redactCertbotOutput), since DNS plugins can echo credentials.
+ *
+ * @param   {String}   stderr
+ * @param   {String}   [dnsProvider]  The certificate's DNS provider, if it uses a DNS challenge
+ * @param   {String[]} [secrets]      Values that must never appear in the message
+ * @returns {String}
+ */
+const certbotFailureMessage = (stderr, dnsProvider, secrets) => {
+	const reason = redactCertbotOutput(stderr, secrets)
 		.split("\n")
 		.map((line) => line.trim())
 		.filter((line) => line && !certbotNoise.some((re) => re.test(line)))
@@ -122,14 +161,19 @@ const certbotFailureMessage = (stderr, dnsProvider) => {
  * @param   {Array}  args
  * @param   {Object} [opts]
  * @param   {String} [dnsProvider]  Passed through so provider-specific hints only show for that provider
+ * @param   {String} [credentials]  The DNS credentials file contents, so its values are redacted from errors
  * @returns {Promise}
  */
-const runCertbot = async (args, opts, dnsProvider) => {
+const runCertbot = async (args, opts, dnsProvider, credentials) => {
 	try {
 		return await utils.execFile(certbotCommand, args, opts);
 	} catch (err) {
 		if (err instanceof error.CommandError) {
-			throw new error.ValidationError(certbotFailureMessage(err.message, dnsProvider), err);
+			const secrets = credentialValues(credentials);
+			logger.error(redactCertbotOutput(err.message, secrets));
+			// The raw error is not attached as `previous`: the API error handler returns
+			// `previous` to the client for certificate routes, and it holds unredacted output.
+			throw new error.ValidationError(certbotFailureMessage(err.message, dnsProvider, secrets));
 		}
 		throw err;
 	}
@@ -515,12 +559,18 @@ const internalCertificate = {
 	 */
 	delete: async (access, data) => {
 		await access.can("certificates:delete", data.id);
-		const row = await internalCertificate.get(access, { id: data.id });
+		const hostRelations = ["proxy_hosts", "redirection_hosts", "dead_hosts", "streams"];
+		const row = await internalCertificate.get(access, { id: data.id, expand: hostRelations });
 
 		if (!row?.id) {
 			throw new error.ItemNotFoundError(data.id);
 		}
 
+		const inUse = hostRelations.some((rel) => row[rel]?.length);
+		for (const rel of hostRelations) {
+			delete row[rel];
+		}
+
 		await certificateModel.query().where("id", row.id).patch({
 			is_deleted: 1,
 		});
@@ -537,8 +587,19 @@ const internalCertificate = {
 
 		if (row.provider === "letsencrypt") {
 			// Revoke the cert
-			await internalCertificate.revokeLetsEncryptSsl(row);
-			// Certbot leaves the files behind when it can't revoke (e.g. a broken renewal config); remove them anyway.
+			const revoked = await internalCertificate.revokeLetsEncryptSsl(row, true).then(
+				() => true,
+				() => false,
+			);
+			// Certbot leaves the files behind when it can't revoke (e.g. a broken renewal config).
+			// Remove them anyway, unless a host still uses this certificate: its rendered config
+			// includes these files, and removing them would fail nginx -t on the next reload.
+			if (!revoked && inUse) {
+				logger.warn(
+					`Certificate #${row.id} was deleted but could not be revoked, and hosts still use it; its files in /etc/letsencrypt were kept`,
+				);
+				return true;
+			}
 			// The row is already deleted and audited, so leftover files are logged, not thrown:
 			// an error here would report a completed delete as failed, and a retry would 404.
 			const leftover = internalCertificate.removeLetsEncryptFiles(row.id);
@@ -780,6 +841,9 @@ const internalCertificate = {
 		if (!meta.certificate_key) {
 			throw new error.ValidationError("Certificate Key file was not provided");
 		}
+		// The key may be the stored one (a certificate-only upload), so check the pair belongs
+		// together; a mismatch would make nginx -t fail for every host on the next reload.
+		await internalCertificate.checkKeyMatchesCertificate(meta.certificate, meta.certificate_key);
 
 		const certificate = await internalCertificate.update(access, {
 			id: data.id,
@@ -794,6 +858,36 @@ const internalCertificate = {
 		return _.pick(meta, uploaded);
 	},
 
+	/**
+	 * Throws unless the private key is the one the certificate was issued for, by comparing the
+	 * public key derived from each.
+	 *
+	 * @param   {String}  certificate  PEM certificate (the leaf comes first)
+	 * @param   {String}  privateKey   PEM private key
+	 * @returns {Promise}
+	 */
+	checkKeyMatchesCertificate: async (certificate, privateKey) => {
+		const certPath = await tempWrite(certificate);
+		const keyPath = await tempWrite(privateKey);
+		try {
+			const [certPub, keyPub] = await Promise.all([
+				utils.execFile("openssl", ["x509", "-in", certPath, "-pubkey", "-noout"]),
+				utils.execFile("openssl", ["pkey", "-in", keyPath, "-pubout"]),
+			]);
+			if (!certPub.trim() || certPub.trim() !== keyPub.trim()) {
+				throw new error.ValidationError("Certificate does not match the Certificate Key");
+			}
+		} catch (err) {
+			if (err instanceof error.ValidationError) {
+				throw err;
+			}
+			throw new error.ValidationError("Could not check that the Certificate matches the Certificate Key");
+		} finally {
+			fs.rmSync(certPath, { force: true });
+			fs.rmSync(keyPath, { force: true });
+		}
+	},
+
 	/**
 	 * Uses the openssl command to validate the private key.
 	 * It will save the file to disk first, then run commands on it, then delete the file.
@@ -1050,7 +1144,12 @@ const internalCertificate = {
 		logger.info(`Command: ${certbotCommand} ${args ? args.join(" ") : ""}`);
 
 		try {
-			const result = await runCertbot(args, adds.opts, certificate.meta.dns_provider);
+			const result = await runCertbot(
+				args,
+				adds.opts,
+				certificate.meta.dns_provider,
+				certificate.meta.dns_provider_credentials,
+			);
 			logger.info(result);
 			return result;
 		} finally {
@@ -1251,7 +1350,7 @@ const internalCertificate = {
 		logger.info(`Command: ${certbotCommand} ${args ? args.join(" ") : ""}`);
 
 		try {
-			const result = await runCertbot(args, adds.opts, certificate.meta.dns_provider);
+			const result = await runCertbot(args, adds.opts, certificate.meta.dns_provider, credentials);
 			logger.info(result);
 			return result;
 		} finally {

backend/internal/host.js

@@ -49,7 +49,8 @@ const internalHost = {
 	 * host. Matching is by value, not path, so renaming or reordering a location that carries
 	 * admin-written config is allowed, but new or edited config is not. Each existing config can
 	 * be used once (no duplicating it onto other paths), and same-path matches are taken first.
-	 * A location that omits advanced_config keeps the one at its path. Mutates `locations`.
+	 * A location that omits advanced_config keeps the one at its path, unless another location
+	 * explicitly carries it. Mutates `locations`.
 	 *
 	 * @param   {Array}  locations          incoming locations
 	 * @param   {Array}  [existingLocations] the host's current locations
@@ -71,12 +72,13 @@ const internalHost = {
 			throw new errs.PermissionError("You do not have permission to modify advanced configuration");
 		};
 
-		// Pass 1: locations that keep (or omit) the config at their own path
+		// Pass 1: locations that keep the config at their own path
 		const unresolved = [];
+		const omitted = [];
 		for (const loc of locations) {
 			const atPath = advAtPath(loc.path);
 			if (typeof loc.advanced_config === "undefined") {
-				loc.advanced_config = atPath && take(atPath) ? atPath : "";
+				omitted.push(loc);
 			} else if ((loc.advanced_config || "") === atPath && (!atPath || take(atPath))) {
 				// unchanged
 			} else {
@@ -91,6 +93,14 @@ const internalHost = {
 				deny();
 			}
 		}
+
+		// Pass 3: locations that omit advanced_config keep the one at their path, if no location
+		// that names it explicitly has claimed it. Running this last stops a new location at an
+		// old path taking the config away from the location it was renamed to.
+		for (const loc of omitted) {
+			const atPath = advAtPath(loc.path);
+			loc.advanced_config = atPath && take(atPath) ? atPath : "";
+		}
 	},
 
 	/**

backend/internal/proxy-host.js

@@ -3,7 +3,6 @@ import errs from "../lib/error.js";
 import { castJsonIfNeed } from "../lib/helpers.js";
 import utils from "../lib/utils.js";
 import proxyHostModel, { scopeToOwnerOrAllowed } from "../models/proxy_host.js";
-import userPermissionModel from "../models/user_permission.js";
 import internalAuditLog from "./audit-log.js";
 import internalCertificate from "./certificate.js";
 import internalHost from "./host.js";
@@ -455,14 +454,14 @@ const internalProxyHost = {
 	 *
 	 * @param   {Number}  user_id
 	 * @param   {String}  visibility
+	 * @param   {Object}  [permissionsMeta]  the user's permissions meta, as already loaded by access
 	 * @returns {Promise}
 	 */
-	getCount: async (user_id, visibility) => {
+	getCount: async (user_id, visibility, permissionsMeta) => {
 		const query = proxyHostModel.query().count("id as count").where("is_deleted", 0);
 
 		if (visibility !== "all") {
-			const perm = await userPermissionModel.query().where("user_id", user_id).first();
-			scopeToOwnerOrAllowed(query, user_id, perm?.meta);
+			scopeToOwnerOrAllowed(query, user_id, permissionsMeta);
 		}
 
 		const row = await query.first();

backend/internal/report.js

@@ -15,7 +15,11 @@ const internalReport = {
 				const userId = access.token.getUserId(1);
 
 				const promises = [
-					internalProxyHost.getCount(userId, access_data.permission_visibility),
+					internalProxyHost.getCount(
+						userId,
+						access_data.permission_visibility,
+						access.getPermissions?.()?.meta,
+					),
 					internalRedirectionHost.getCount(userId, access_data.permission_visibility),
 					internalStream.getCount(userId, access_data.permission_visibility),
 					internalDeadHost.getCount(userId, access_data.permission_visibility),

backend/lib/express/rate-limit.js

@@ -1,9 +1,17 @@
+import crypto from "node:crypto";
 import { isCI } from "../config.js";
 
 /**
- * In-memory sliding window rate limiter for authentication endpoints
+ * In-memory fixed window rate limiter for authentication endpoints
  */
 
+// Keys longer than this are hashed, so a client can't make the store hold arbitrarily large
+// strings (keys can include request body values such as the login identity).
+const MAX_KEY_LENGTH = 128;
+// Entries per limiter. A flood of unique keys sweeps expired entries, then evicts the oldest,
+// instead of growing the store until the periodic cleanup runs.
+const MAX_STORE_SIZE = 50000;
+
 // One store per limiter, so separate endpoints never share buckets
 const stores = new Set();
 
@@ -21,9 +29,12 @@ const cleanupTimer = setInterval(() => {
 cleanupTimer.unref();
 
 /**
- * The client IP as seen by the backend. X-Forwarded-For is only honoured when the socket is
- * loopback, which is the admin nginx in this container; it sets the header to its own
- * $remote_addr after realip processing, so the first entry is the resolved client.
+ * The address to rate limit on: the TCP peer of the admin nginx, which it passes as
+ * X-NPM-Peer-Addr ($realip_remote_addr), only honoured when the socket is loopback (that nginx,
+ * in this container). The realip-resolved client address is deliberately not used: nginx trusts
+ * X-Real-IP from private ranges, so a client on the LAN or Docker network could pick a new
+ * address, and a fresh bucket, on every request. Behind another proxy this resolves to that
+ * proxy's address, which is why limiters also key on the target account.
  *
  * @param   {Object} req
  * @returns {String}
@@ -31,10 +42,28 @@ cleanupTimer.unref();
 export const getClientIp = (req) => {
 	const socketAddr = req.socket?.remoteAddress || "unknown";
 	const isTrustedLocalProxy = socketAddr === "127.0.0.1" || socketAddr === "::1" || socketAddr === "::ffff:127.0.0.1";
-	const forwardedFor = isTrustedLocalProxy ? req.headers["x-forwarded-for"]?.split(",")[0].trim() : null;
-	return forwardedFor || socketAddr;
+	const peerAddr = isTrustedLocalProxy ? req.headers["x-npm-peer-addr"]?.trim() : null;
+	return peerAddr || socketAddr;
 };
 
+const makeRoom = (store, now) => {
+	for (const [k, r] of store.entries()) {
+		if (now - r.startTime > r.windowMs) {
+			store.delete(k);
+		}
+	}
+	// Maps iterate in insertion order, so this drops the oldest windows first
+	for (const k of store.keys()) {
+		if (store.size < MAX_STORE_SIZE) {
+			break;
+		}
+		store.delete(k);
+	}
+};
+
+const boundedKey = (key) =>
+	key.length > MAX_KEY_LENGTH ? `sha256:${crypto.createHash("sha256").update(key).digest("hex")}` : key;
+
 /**
  * @param {Object}   [options]
  * @param {Number}   [options.windowMs]
@@ -67,29 +96,21 @@ export default function createRateLimiter(options = {}) {
 			return next();
 		}
 
-		const key = keyGenerator(req, getClientIp(req));
+		const key = boundedKey(String(keyGenerator(req, getClientIp(req))));
 
 		const now = Date.now();
 		let record = store.get(key);
 
 		if (!record || now - record.startTime > windowMs) {
 			record = {
-				count: 1,
+				count: 0,
 				startTime: now,
 				windowMs,
 			};
-			store.set(key, record);
-
-			if (skipSuccessfulRequests) {
-				const currentRecord = record;
-				res.on("finish", () => {
-					if (res.statusCode < 400 && currentRecord.count > 0) {
-						currentRecord.count--;
-					}
-				});
+			if (!store.has(key) && store.size >= MAX_STORE_SIZE) {
+				makeRoom(store, now);
 			}
-
-			return next();
+			store.set(key, record);
 		}
 
 		record.count++;

backend/routes/tokens.js

@@ -9,7 +9,7 @@ import { getValidationSchema } from "../schema/index.js";
 // Keyed on IP + target account rather than IP alone: when the admin UI sits behind another
 // proxy every client can resolve to the same IP, and an IP-only bucket would let anyone lock
 // out every user with a handful of bad logins. Brute force against one account is still
-// capped per IP.
+// capped per connecting address (see getClientIp).
 const tokenRateLimiter = createRateLimiter({
 	windowMs: 60 * 1000,
 	max: 10,

docker/rootfs/etc/nginx/conf.d/dev.conf

@@ -17,6 +17,9 @@ server {
 		proxy_set_header      X-Forwarded-Scheme $scheme;
 		proxy_set_header      X-Forwarded-Proto  $scheme;
 		proxy_set_header      X-Forwarded-For    $remote_addr;
+		# The TCP peer before realip processing; the backend rate limits on this because
+		# X-Real-IP can be set by any client in the trusted private ranges
+		proxy_set_header      X-NPM-Peer-Addr    $realip_remote_addr;
 		proxy_pass            http://127.0.0.1:3000/;
 
 		proxy_read_timeout 15m;

docker/rootfs/etc/nginx/conf.d/production.conf.template

@@ -17,6 +17,9 @@ server {
 		proxy_set_header      X-Forwarded-Scheme $scheme;
 		proxy_set_header      X-Forwarded-Proto  $scheme;
 		proxy_set_header      X-Forwarded-For    $remote_addr;
+		# The TCP peer before realip processing; the backend rate limits on this because
+		# X-Real-IP can be set by any client in the trusted private ranges
+		proxy_set_header      X-NPM-Peer-Addr    $realip_remote_addr;
 		proxy_pass            http://127.0.0.1:3000/;
 
 		proxy_read_timeout 15m;

frontend/src/modals/ProxyHostModal.tsx

@@ -53,14 +53,10 @@ const ProxyHostModal = EasyModal.create(({ id, visible, remove }: Props) => {
 		if (!isAdmin) {
 			delete payload.advanced_config;
 			delete payload.advancedConfig;
-			const existingLocations = data?.locations || [];
-			payload.locations = payload.locations.map((loc: any) => {
-				const existingLoc = existingLocations.find((l: any) => l.path === loc.path);
-				return {
-					...loc,
-					advancedConfig: existingLoc?.advancedConfig || "",
-				};
-			});
+			// Location advanced configs are sent as-is. Non-admins can't edit them (the editor is
+			// hidden), so each one is still the admin-written value and moves with its location
+			// when the path is renamed; the backend matches them by value. Looking them up by
+			// path here would silently drop the config from a renamed location.
 		}
 
 		setProxyHost(payload, {