schalli/tessera-ctl
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 14 Wiki Activity

fix(ldap): search objectGUID by raw bytes, not an escaped filter string

Browse Source 
The existence sweep in syncBoundGroupsForTenant() built its filter by
interpolating a byte-wise \xx escape of the stored objectGUID into a filter
string. ldapts parses that string before encoding it and does not turn the
escape sequences back into the bytes they stand for, so the assertion value
that reached the directory was a different value and matched nothing.

Measured read-only against a real Active Directory on 2026-08-11, probing a
group whose GUID had just been read from that same directory:

  (objectGUID=\1e\4b...)                          0 hits
  (objectGUID=\1E\4B...)                          0 hits
  EqualityFilter{attribute, value: <16 bytes>}    1 hit, correct DN
  (cn=Domain Admins)  [control]                   1 hit

Both the narrow base-DN sweep and the wider WR-03 move-detection sweep shared
that filter, so neither could ever hit: every AD-bound group looked deleted and
would have been removed together with its GroupMembership and ModuleGrant rows
on the first real sync, after handing off the default-group marker.

Build the filter as an EqualityFilter over the raw Buffer instead, and drop
escapeLdapFilterBuffer() -- it has no remaining caller and is the trap the code
walked into. escapeLdapFilterValue() is untouched: escaping STRING values into
a filter is correct and still in use.

The existing spec mocks matched on the escaped string, which is how the broken
shape passed review. They now match on the filter object's Buffer value, and
two added tests fail if a stringly-typed objectGUID filter ever comes back.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Schalli
2026-08-11 11:05:11 +02:00
parent ba21b0c74a
commit d2019dc527
2 changed files with 175 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
apps/api/src/ldap/ldap.service.spec.ts
+148 -18
View File
@@ -6,14 +6,51 @@ const mockBind = vi.fn().mockResolvedValue(undefined);
const mockSearch = vi.fn();
const mockUnbind = vi.fn().mockResolvedValue(undefined);
// EqualityFilter is mocked as a plain carrier of { attribute, value } — that is
// exactly the contract the existence sweep depends on: the raw GUID Buffer is
// handed to the filter object instead of being escaped into a filter string
// (quick task 260811-f9i). The class is declared INSIDE the factory because
// vi.mock is hoisted above every top-level binding in this file.
vi.mock('ldapts', () => ({
Client: vi.fn().mockImplementation(() => ({
bind: mockBind,
search: mockSearch,
unbind: mockUnbind,
})),
EqualityFilter: class {
attribute: string;
value: Buffer | string;
constructor({
attribute,
value,
}: {
attribute: string;
value: Buffer | string;
}) {
this.attribute = attribute;
this.value = value;
}
},
}));
/**
* The existence sweep passes a FILTER OBJECT carrying the raw GUID bytes, never
* an escaped filter string. Returns the Buffer when a search call is that sweep,
* null otherwise — a string filter of the form "(objectGUID=...)" deliberately
* returns null, so the old broken shape can never satisfy a mock again.
*/
const sweptGuid = (filter: unknown): Buffer | null => {
if (
filter &&
typeof filter === 'object' &&
(filter as { attribute?: unknown }).attribute === 'objectGUID'
) {
const value = (filter as { value?: unknown }).value;
return Buffer.isBuffer(value) ? value : null;
}
return null;
};
// forTenant just returns the same client in these tests (tenant scoping is not
// under test here).
vi.mock('../prisma/prisma-tenant.extension', () => ({
@@ -565,7 +602,7 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1
return Buffer.from(hex, 'hex');
};
const resolve5aNoOp = (baseDn: string, filter: string) => {
const resolve5aNoOp = (baseDn: string, filter: unknown) => {
if (filter === '(objectClass=group)') {
// 5a legacy-binding backfill probe: the probed base DN IS the
// group's own stored ldapDn (scope 'base').
@@ -576,14 +613,12 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1
: [],
};
}
if (filter.startsWith('(objectGUID=')) {
// 5a existence sweep: match whichever fixture group's DN-derived
// GUID produced this escaped filter.
const guid = sweptGuid(filter);
if (guid) {
// 5a existence sweep: match whichever fixture group's DN-derived GUID
// the filter object carries.
const g = groups.find(
(x) =>
x.ldapDn &&
filter ===
`(objectGUID=${LdapService.escapeLdapFilterBuffer(guidForDn(x.ldapDn))})`,
(x) => x.ldapDn && guidForDn(x.ldapDn as string).equals(guid),
);
return { searchEntries: g ? [{ dn: g.ldapDn, cn: g.name }] : [] };
}
@@ -609,7 +644,7 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1
if (filter.includes('memberOf=')) {
return Promise.resolve({ searchEntries: groupSearchEntries });
}
const fivea = resolve5aNoOp(baseDn, filter);
const fivea = resolve5aNoOp(baseDn, opts.filter);
if (fivea) {
return Promise.resolve(fivea);
}
@@ -741,7 +776,7 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1
);
const memberOfCalls = mockSearch.mock.calls.filter(([, opts]) =>
(opts.filter as string).includes('memberOf='),
typeof opts.filter === 'string' && opts.filter.includes('memberOf='),
);
expect(memberOfCalls).toHaveLength(2);
expect(memberOfCalls[0][0]).toBe('dc=a,dc=com');
@@ -764,10 +799,11 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1
await service.syncUsersForTenant(cfg as any, 't1');
const userSyncCall = mockSearch.mock.calls.find(
([, opts]) => !(opts.filter as string).includes('memberOf='),
([, opts]) =>
typeof opts.filter === 'string' && !opts.filter.includes('memberOf='),
);
const groupSyncCall = mockSearch.mock.calls.find(([, opts]) =>
(opts.filter as string).includes('memberOf='),
typeof opts.filter === 'string' && opts.filter.includes('memberOf='),
);
expect(userSyncCall).toBeDefined();
expect(groupSyncCall).toBeDefined();
@@ -790,7 +826,7 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1
await service.syncUsersForTenant(cfg as any, 't1');
const groupSyncCall = mockSearch.mock.calls.find(([, opts]) =>
(opts.filter as string).includes('memberOf='),
typeof opts.filter === 'string' && opts.filter.includes('memberOf='),
);
expect(groupSyncCall![1].filter).toBe(
'(&(objectClass=person)(memberOf=CN=Sales \\28EMEA\\29\\2a\\5c,DC=ctl,DC=local))',
@@ -916,7 +952,7 @@ describe('LdapService.syncUsersForTenant — AD-bound group membership sync (D-1
searchEntries: [{ dn: 'cn=alice,dc=example,dc=com', sAMAccountName: 'alice' }],
});
}
const fivea = resolve5aNoOp(baseDn, filter);
const fivea = resolve5aNoOp(baseDn, opts.filter);
if (fivea) {
return Promise.resolve(fivea);
}
@@ -1056,8 +1092,7 @@ describe('LdapService.syncUsersForTenant — Schrittreihenfolge Gruppen vor Mitg
});
mockSearch.mockImplementation((_baseDn: string, opts: any) => {
const filter = typeof opts.filter === 'string' ? opts.filter : '';
if (filter.startsWith('(objectGUID=')) {
if (sweptGuid(opts.filter)) {
// 5a's existence sweep: AD reports the group renamed — new cn/dn.
return Promise.resolve({
searchEntries: [
@@ -1076,7 +1111,7 @@ describe('LdapService.syncUsersForTenant — Schrittreihenfolge Gruppen vor Mitg
expect(groups[0].ldapDn).toBe('cn=Vertrieb,dc=example,dc=com');
const memberOfCall = mockSearch.mock.calls.find(([, opts]: any) =>
(opts.filter as string).includes('memberOf='),
typeof opts.filter === 'string' && opts.filter.includes('memberOf='),
);
expect(memberOfCall).toBeDefined();
expect(memberOfCall![1].filter).toContain('cn=Vertrieb,dc=example,dc=com');
@@ -1584,7 +1619,8 @@ describe('LdapService.syncBoundGroupsForTenant — Rekonziliation gegen das Verz
},
];
mockSearch.mockImplementation((_baseDn: string, opts: any) => {
if (opts.filter.includes(LdapService.escapeLdapFilterBuffer(guidBuffer))) {
const swept = sweptGuid(opts.filter);
if (swept && swept.equals(guidBuffer)) {
return Promise.reject(new Error('directory unavailable'));
}
return Promise.resolve({
@@ -1600,6 +1636,100 @@ describe('LdapService.syncBoundGroupsForTenant — Rekonziliation gegen das Verz
expect(groups.find((g) => g.id === 'g-ok')).toBeDefined();
});
// Quick task 260811-f9i. The sweep used to interpolate a byte-wise `\xx`
// escape into a filter STRING. Measured read-only against a real AD on
// 2026-08-11, that filter returned zero hits for an object whose GUID had
// just been read from that same directory — so every bound group looked
// deleted and would have been removed with its memberships and module grants
// on the first real sync. These two tests pin the shape that actually works.
it('passes the GUID as raw Buffer bytes on a filter object, never as an escaped filter string', async () => {
groups = [
{
id: 'g1',
tenantId: 't1',
name: 'Sales',
ldapDn: 'cn=Sales,dc=example,dc=com',
ldapObjectGuid: guidHex,
isDefault: false,
},
];
mockSearch.mockResolvedValue({
searchEntries: [{ dn: 'cn=Sales,dc=example,dc=com', cn: 'Sales' }],
});
await run(makeResult());
const sweepCalls = mockSearch.mock.calls.filter(([, opts]: any) =>
sweptGuid(opts.filter),
);
expect(sweepCalls.length).toBeGreaterThan(0);
for (const [, opts] of sweepCalls as any[]) {
const value = opts.filter.value;
expect(Buffer.isBuffer(value)).toBe(true);
// Byte-for-byte the stored hex, not a re-encoding of it.
expect((value as Buffer).equals(guidBuffer)).toBe(true);
}
// No search in this run may carry a stringly-typed objectGUID filter —
// that is the exact shape the directory refused to match.
const stringGuidFilters = mockSearch.mock.calls.filter(
([, opts]: any) =>
typeof opts.filter === 'string' && opts.filter.includes('objectGUID='),
);
expect(stringGuidFilters).toEqual([]);
});
it('reuses the same raw-Buffer filter for the wider WR-03 move sweep', async () => {
groups = [
{
id: 'g1',
tenantId: 't1',
name: 'Sales',
ldapDn: 'cn=Sales,ou=Vertrieb,dc=example,dc=com',
ldapObjectGuid: guidHex,
isDefault: false,
},
];
// A COPY — `cfg` is shared across this describe block and never reset in
// beforeEach, so mutating it here would leak into later tests.
const narrowCfg = { ...cfg, baseDn: 'ou=Vertrieb,dc=example,dc=com' };
// Narrow sweep misses, wide sweep over the domain root finds it —
// the move case WR-03 protects. Both must carry the same Buffer, or the
// protection inherits a broken filter and deletes the group anyway.
mockSearch.mockImplementation((baseDn: string) =>
Promise.resolve(
baseDn === 'dc=example,dc=com'
? {
searchEntries: [
{ dn: 'cn=Sales,ou=Anders,dc=example,dc=com', cn: 'Sales' },
],
}
: { searchEntries: [] },
),
);
const result = makeResult();
await (service as any).syncBoundGroupsForTenant(
client,
narrowCfg,
't1',
result,
);
const sweepCalls = mockSearch.mock.calls.filter(([, opts]: any) =>
sweptGuid(opts.filter),
);
// Narrow (configured base DN) plus wide (domain root).
expect(sweepCalls.length).toBe(2);
for (const [, opts] of sweepCalls as any[]) {
expect((opts.filter.value as Buffer).equals(guidBuffer)).toBe(true);
}
expect(prisma.group.delete).not.toHaveBeenCalled();
expect(result.errors.length).toBe(1);
expect(result.errors[0]).toContain('Nicht geloescht');
});
it('is idempotent: a second run over an unchanged AD state issues no group.update or group.delete call', async () => {
groups = [
{
apps/api/src/ldap/ldap.service.ts
+27 -19
View File
@@ -1,5 +1,5 @@
import { Injectable, Logger } from '@nestjs/common';
import { Client, Entry } from 'ldapts';
import { Client, EqualityFilter, Entry } from 'ldapts';
import { PrismaService } from '../prisma/prisma.service';
import { forTenant } from '../prisma/prisma-tenant.extension';
import { GroupsService } from '../groups/groups.service';
@@ -1179,9 +1179,9 @@ export class LdapService {
* 2. Existence sweep: the stored hex ldapObjectGuid is validated as
* exactly 32 [0-9a-f] characters BEFORE it is ever turned into a
* filter (T-16-01) — an invalid value is an error line, never a filter
* interpolation. A valid value is turned back into a Buffer and
* byte-wise escaped via escapeLdapFilterBuffer() into an
* (objectGUID=...) filter, searched across every configured base DN.
* interpolation. A valid value is turned back into a Buffer and handed
* to an EqualityFilter over objectGUID — raw bytes, never an escaped
* filter string — searched across every configured base DN.
* 3. A hit whose cn/dn differ from the stored name/ldapDn is a rename
* (SC-3): Group.name/ldapDn are updated to the AD state,
* groupsRenamed++. internalName is NEVER written here (D-04). A
@@ -1315,7 +1315,21 @@ export class LdapService {
continue;
}
const guidBuffer = Buffer.from(ldapObjectGuid, 'hex');
const filter = `(objectGUID=${LdapService.escapeLdapFilterBuffer(guidBuffer)})`;
// The GUID goes onto the wire as raw bytes via an EqualityFilter, NOT
// as a `\xx`-escaped filter string. A string filter is parsed by ldapts
// before it is encoded, and the parser does not turn `\1e\4b...` back
// into the 16 bytes it stands for — the assertion value that reaches the
// directory is then a different value entirely and matches nothing.
// Measured read-only against a real AD on 2026-08-11 (see the quick task
// 260811-f9i): the escaped string returned 0 hits for an object whose
// GUID had just been read from that same directory, while this
// EqualityFilter returned exactly that object. Both sweeps below share
// this value, so the WR-03 move-detection cannot silently inherit a
// broken filter again.
const filter = new EqualityFilter({
attribute: 'objectGUID',
value: guidBuffer,
});
let hit: Entry | null = null;
for (const baseDn of baseDns) {
@@ -1503,20 +1517,14 @@ export class LdapService {
.replace(/\x00/g, '\\00');
}
/**
* Escape a binary value (e.g. a stored objectGUID) for use in an LDAP
* search filter per RFC 4515 — a byte-wise `\XX` hex escape, distinct from
* escapeLdapFilterValue() which escapes a STRING value. Not yet called
* anywhere in this plan (Plan 16-01 only WRITES ldapObjectGuid); Plan
* 16-03's existence sweep is the first caller, reading it back via a
* binary (objectGUID=...) filter. [ASSUMED — RFC 4515-Praxis, nicht gegen
* ein echtes AD verifiziert, siehe RESEARCH.md Pattern 3/A2.]
*/
static escapeLdapFilterBuffer(buf: Buffer): string {
return Array.from(buf)
.map((b) => '\\' + b.toString(16).padStart(2, '0'))
.join('');
}
// NOTE: escapeLdapFilterBuffer() used to live here — a byte-wise `\XX` hex
// escape for binary values, written under the assumption (RESEARCH.md A2)
// that ldapts would pass such a string through to the directory unchanged.
// It does not, and the resulting filter matched nothing; the existence sweep
// in syncBoundGroupsForTenant() now builds an EqualityFilter over the raw
// Buffer instead. Do not reintroduce it: escapeLdapFilterValue() below is for
// STRING values and stays correct, but binary values belong in a filter
// object, never in an interpolated filter string.
/**
* Split a DN into its individual RDN components, respecting a