From 80866305c2e2473d33beaa9d7150e59f4cffa6ff Mon Sep 17 00:00:00 2001
From: Scott Duensing <scott@duensing.com>
Date: Wed, 1 Jul 2026 20:05:17 -0500
Subject: [PATCH] More security issues addressed.

---
 lib/ldap/search.js   | 20 ++++++++++++++------
 lib/oidc/provider.js | 14 ++++++++++++++
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/lib/ldap/search.js b/lib/ldap/search.js
index 57122d8..bfd4783 100644
--- a/lib/ldap/search.js
+++ b/lib/ldap/search.js
@@ -96,13 +96,21 @@ const handler = async (req, res, next) => {
             // no full-directory load, correct even for directories larger than the
             // cap. A presence filter like (uid=*) is NOT equality (no .value) and
             // falls through to the capped scan.
-            const f    = req.filter;
-            const attr = f && f.attribute ? String(f.attribute).toLowerCase() : null;
-            // A leaf equality on uid/mail (same structural test as the existing
-            // single-equality fast path). Reused to classify OR children below.
+            const f = req.filter;
+            // A leaf filter (equality) exposes .clauses/.filters as an EMPTY array
+            // (the same array instance in @ldapjs/filter), which is TRUTHY -- so the
+            // old `!node.clauses` guard was ALWAYS false, making both fast paths dead
+            // code and forcing every search into the capped full scan (a user past
+            // the cap could then never be resolved). Test emptiness by length.
+            const noChildren = (node) => {
+                const kids = (node && (node.clauses || node.filters)) || [];
+                return kids.length === 0;
+            };
+            // A leaf equality on uid/mail (same structural test as the single-
+            // equality fast path). Reused to classify OR children below.
             const isUidMailEquality = (node) => {
                 const a = node && node.attribute ? String(node.attribute).toLowerCase() : null;
-                return !!node && (a === "uid" || a === "mail") && node.value !== undefined && node.value !== null && !node.filters && !node.clauses;
+                return !!node && (a === "uid" || a === "mail") && node.value !== undefined && node.value !== null && noChildren(node);
             };
             // OR children expose siblings via .filters (ldapjs) or .clauses (alias).
             const orChildren = (node) => {
@@ -117,7 +125,7 @@ const handler = async (req, res, next) => {
                 return Array.isArray(kids) ? kids : null;
             };
             let users;
-            if (f && (attr === "uid" || attr === "mail") && f.value !== undefined && f.value !== null && !f.filters && !f.clauses) {
+            if (isUidMailEquality(f)) {
                 const one = await User.findOne({ email: String(f.value) });
                 users = one ? [one] : [];
             } else if (orChildren(f) && orChildren(f).length > 0 && orChildren(f).every(isUidMailEquality)) {
diff --git a/lib/oidc/provider.js b/lib/oidc/provider.js
index 00abdcf..e4e38a0 100644
--- a/lib/oidc/provider.js
+++ b/lib/oidc/provider.js
@@ -22,6 +22,13 @@ const { issuerForReq } = require("./discovery");
 const COOKIE_KEY_INFO  = "saltcorn-idp:oidc-cookie-keys:v1";
 const COOKIE_KEY_BYTES = 32;
 
+// Hard cap on distinct cached Providers. The issuer is derived from the request
+// Host header when base_url is unset (see issuerForReq), so without a bound an
+// anonymous attacker spraying distinct Host values could grow this Map -- each
+// entry a full Provider -- until the worker OOMs. A real deployment has one issuer
+// per tenant, far below this; past the cap we evict the oldest (insertion order).
+const MAX_PROVIDERS = 256;
+
 let providerClassPromise = null;
 const providersByIssuer = new Map();
 
@@ -116,6 +123,13 @@ const getProviderEntry = async (req) => {
         const provider = await buildProvider(issuer);
         entry = { provider: provider, handler: provider.callback(), fingerprint: fingerprint };
         providersByIssuer.set(issuer, entry);
+        // Bound the cache: evict the oldest entry once we exceed the cap. A brand
+        // new issuer is inserted last, so the oldest key is always a different,
+        // older entry -- never the one just built.
+        if (providersByIssuer.size > MAX_PROVIDERS) {
+            const oldest = providersByIssuer.keys().next().value;
+            providersByIssuer.delete(oldest);
+        }
     }
     return entry;
 };