schalli/tessera-ctl
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 15 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 = [
{