From f45349e0cd535e2839599ea7917cc186c796dc80 Mon Sep 17 00:00:00 2001 From: William Mantly Date: Tue, 14 Jul 2026 23:03:59 -0400 Subject: [PATCH] Fix uid/gidNumber allocation crash, add a configurable id floor (#44) Reported: creating any user via the API failed with {"name":"InvalidSyntaxError","message":"gidNumber: value #0 invalid per syntax Code: 0x15"} Root cause: addPosixGroup() computes the next gidNumber as `Math.max(...groups.map(i => i.gidNumber)) + 1`. theta-env's bootstrap.js creates the first admin via raw ldapadd with a hardcoded uidNumber/gidNumber (10000) directly on the user entry, but never creates a matching posixGroup entry -- so on a theta-env-bootstrapped directory there are zero posixGroup entries, `Math.max()` on an empty array is `-Infinity` in JS (not 0), and `-Infinity + 1` stringifies to "-Infinity" -- an invalid LDAP integer, rejected by the directory. This broke every single user creation, not just this one. Separately: the reporter's intended scheme is for organically-created users to start at uidNumber/gidNumber 1500, distinct from the bootstrap admin's reserved 10000. Fixing the crash with a bare "floor of 1500" alone wouldn't achieve that, since addPosixAccount's own Math.max() would still find the admin's posixAccount entry (uidNumber 10000, found via a different, correctly-indexed search) and allocate 10001 for the next user. Added a shared nextPosixId(entries, key) helper: takes the highest existing value strictly below conf.ldap.uidGidReservedFloor (default 9000) plus one, or conf.ldap.uidGidMin (default 1500) if there are no such entries. Ids at/above the reserved floor -- like the bootstrap admin's 10000 -- are ignored entirely when computing the next available number, so real users always start at 1500 and grow upward regardless of the admin's reserved id. Verified against a real theta-env deployment end to end: - Reproduced the exact reported crash on a fresh bootstrap - After the fix: first real user gets uidNumber/gidNumber "1500", second gets "1501" -- admin's 10000 never enters the calculation - New unit tests (nodejs/tests/posix_id.test.js, no LDAP required): 6/6 pass, covering the empty-array case, the reserved-floor exclusion, and the NaN-from-missing-value case - npm test: 18/18 passing tests still pass (unchanged); the other 155 failures are pre-existing/environmental (no LDAP server in this sandbox) -- confirmed via git stash before starting this fix --- DEPLOYMENT.md | 1 + docs/configuration.md | 2 ++ nodejs/conf/base.js | 11 +++++++++- nodejs/models/user_ldap.js | 23 ++++++++++++++++++--- nodejs/tests/posix_id.test.js | 39 +++++++++++++++++++++++++++++++++++ 5 files changed, 72 insertions(+), 4 deletions(-) create mode 100644 nodejs/tests/posix_id.test.js diff --git a/DEPLOYMENT.md b/DEPLOYMENT.md index 53e1702..ecf7c1c 100644 --- a/DEPLOYMENT.md +++ b/DEPLOYMENT.md @@ -23,6 +23,7 @@ as raw strings otherwise. Examples: |---------|------|------| | `app_ldap__url=ldap://host:389` | `conf.ldap.url` | string | | `app_ldap__bindPassword=secret` | `conf.ldap.bindPassword` | string | +| `app_ldap__uidGidMin=1500` | `conf.ldap.uidGidMin` | number (new-user id floor) | | `app_oauth__jwtSecret=...` | `conf.oauth.jwtSecret` | string | | `app_smtp__secure=false` | `conf.smtp.secure` | boolean | | `app_oauth__token_lifetime__access_token=3600` | `conf.oauth.token_lifetime.access_token` | number | diff --git a/docs/configuration.md b/docs/configuration.md index 977068b..15a6765 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -29,6 +29,8 @@ raw strings otherwise. | `app_ldap__url=ldap://host:389` | `conf.ldap.url` | string | | `app_ldap__bindPassword=secret` | `conf.ldap.bindPassword` | string | | `app_ldap__userBase=ou=people,dc=…` | `conf.ldap.userBase` | string | +| `app_ldap__uidGidMin=1500` | `conf.ldap.uidGidMin` | number (new-user id floor) | +| `app_ldap__uidGidReservedFloor=9000` | `conf.ldap.uidGidReservedFloor` | number (ids at/above this are ignored when allocating) | | `app_oauth__jwtSecret=...` | `conf.oauth.jwtSecret` | string | | `app_oauth__issuer=https://sso.example.com` | `conf.oauth.issuer` | string | | `app_oauth__token_lifetime__access_token=3600` | `conf.oauth.token_lifetime.access_token` | number | diff --git a/nodejs/conf/base.js b/nodejs/conf/base.js index 7b2ae4f..244c303 100644 --- a/nodejs/conf/base.js +++ b/nodejs/conf/base.js @@ -20,7 +20,16 @@ module.exports = { userBase: 'ou=people,dc=example,dc=com', groupBase: 'ou=groups,dc=example,dc=com', userFilter: '(objectClass=posixAccount)', - userNameAttribute: 'uid' + userNameAttribute: 'uid', + // New users/personal groups (see addPosixAccount/addPosixGroup in + // models/user_ldap.js) get the next uid/gidNumber >= uidGidMin. + // Existing entries >= uidGidReservedFloor are ignored when computing + // that "next available" number, so a deliberately high, easily + // recognizable id (e.g. the bootstrap admin at 10000 — see + // theta-env's bootstrap.js) doesn't drag every real user's id up + // into that same range. + uidGidMin: 1500, + uidGidReservedFloor: 9000, }, oauth: { issuer: '', // falls back to the request host at runtime (routes/index.js) diff --git a/nodejs/models/user_ldap.js b/nodejs/models/user_ldap.js index 6ee7ee3..988b403 100644 --- a/nodejs/models/user_ldap.js +++ b/nodejs/models/user_ldap.js @@ -45,6 +45,23 @@ function escapeLDAPSearchValue(val) { .replace(/\0/g, '\\00'); } +// Compute the next available uid/gidNumber: the highest existing value below +// conf.uidGidReservedFloor, plus one -- or conf.uidGidMin if there are no +// such entries yet. Entries at/above the reserved floor (e.g. a bootstrap +// admin deliberately given a high, easily-recognizable id -- see +// theta-env's bootstrap.js) are ignored, so they don't drag every real +// user's id up into that same range. Math.max() on an empty array is +// -Infinity in JS, not 0 -- without the explicit floor here, a fresh +// directory with zero existing entries produces an invalid ("-Infinity") +// LDAP attribute value and the add fails with InvalidSyntaxError. +function nextPosixId(entries, key){ + const existing = entries + .map(i => Number(i[key])) + .filter(n => Number.isFinite(n) && n < conf.uidGidReservedFloor); + + return String(Math.max(conf.uidGidMin - 1, ...existing) + 1); +} + async function addPosixGroup(client, data){ try{ @@ -53,7 +70,7 @@ async function addPosixGroup(client, data){ filter: '(&(objectClass=posixGroup))', })).searchEntries; - data.gidNumber = (Math.max(...groups.map(i => i.gidNumber))+1)+''; + data.gidNumber = nextPosixId(groups, 'gidNumber'); await client.add(`cn=${data.cn},${conf.groupBase}`, { cn: data.cn, @@ -75,7 +92,7 @@ async function addPosixAccount(client, data){ filter: conf.userFilter, })).searchEntries; - data.uidNumber = (Math.max(...people.map(i => i.uidNumber))+1)+''; + data.uidNumber = nextPosixId(people, 'uidNumber'); const entry = { cn: data.cn, @@ -742,7 +759,7 @@ User.login = async function(data){ }; -module.exports = {User, hashPasswordSSHA512}; +module.exports = {User, hashPasswordSSHA512, nextPosixId}; // (async function(){ diff --git a/nodejs/tests/posix_id.test.js b/nodejs/tests/posix_id.test.js new file mode 100644 index 0000000..9c0fe96 --- /dev/null +++ b/nodejs/tests/posix_id.test.js @@ -0,0 +1,39 @@ +'use strict'; + +const { nextPosixId } = require('../models/user_ldap'); + +// Pure logic, no LDAP/Redis needed -- regression coverage for the bug where +// an empty directory (Math.max() on an empty array is -Infinity in JS, not +// 0) produced an invalid "-Infinity" uid/gidNumber and every user creation +// failed with InvalidSyntaxError. Uses the real conf/base.js defaults +// (uidGidMin: 1500, uidGidReservedFloor: 9000). +describe('nextPosixId', () => { + test('starts at uidGidMin (1500) when there are no existing entries', () => { + expect(nextPosixId([], 'uidNumber')).toBe('1500'); + }); + + test('continues from the highest existing value below the reserved floor', () => { + const entries = [{ uidNumber: '1500' }, { uidNumber: '1501' }]; + expect(nextPosixId(entries, 'uidNumber')).toBe('1502'); + }); + + test('ignores entries at/above uidGidReservedFloor (e.g. the bootstrap admin at 10000)', () => { + const entries = [{ uidNumber: '10000' }]; + expect(nextPosixId(entries, 'uidNumber')).toBe('1500'); + }); + + test('a reserved high entry does not affect allocation once real users exist', () => { + const entries = [{ uidNumber: '10000' }, { uidNumber: '1500' }, { uidNumber: '1501' }]; + expect(nextPosixId(entries, 'uidNumber')).toBe('1502'); + }); + + test('ignores non-numeric/missing values instead of producing NaN', () => { + const entries = [{ uidNumber: undefined }, { someOtherField: '1' }]; + expect(nextPosixId(entries, 'uidNumber')).toBe('1500'); + }); + + test('works the same way for gidNumber', () => { + expect(nextPosixId([], 'gidNumber')).toBe('1500'); + expect(nextPosixId([{ gidNumber: '1500' }], 'gidNumber')).toBe('1501'); + }); +});