fix: identify RFC 4533 sync control members by tag, not by count

[youdie006]

On disclosure, first

I read SECURITY.md before opening this. I am sending it as a normal PR because the repository's own precedent for this exact class is public PRs -- #611, #612, #614 (guarding malformed decode input), and #617 and #618, both of which you authored. #617's description says in as many words that "a conformant server that omits it panics the caller with a nil-pointer dereference". If you would rather have had this privately, say so and I will close this immediately and resend it to the addresses in SECURITY.md.

The bug

NewControlSyncDone and NewControlSyncInfo decode RFC 4533 sync sequences by switching on len(pkt.Children) and treating Children[0] as the cookie. RFC 4533 §2.4/§2.5 mark cookie as OPTIONAL and the refresh flags as DEFAULT, so the number of members present does not identify which members they are.

With the cookie omitted:

control	current result
syncIdSet	panic -- Children[1] is the SET, whose Value is nil, and Children[1].Value.(bool) asserts on it
syncIdSet, flags also omitted	every UUID silently dropped
refreshDelete / refreshPresent	the boolean is read as the cookie, and refreshDone comes out inverted
syncDone	Cookie gets the flag's byte, RefreshDeletes is lost

--- FAIL: TestControlSyncInfoSyncIdSetOptionalCookie/cookie_omitted
panic: interface conversion: interface {} is nil, not bool
	github.com/go-ldap/ldap/v3.NewControlSyncInfo(...) v3/control.go:1484

Cookie = "\x00", want empty
RefreshDone = true, want false
Cookie = "\x01", want empty
RefreshDeletes = false, want true

All reproduced through the exported constructors, no network.

Your own NewControlServerSideSorting already does this

v3/control.go:961 decodes the same shape -- a SEQUENCE with OPTIONAL and context-tagged members -- by iterating children and switching on ClassType/Tag, and it guards the type assertion:

// A constructed-form OCTET STRING matches this case but leaves
// Value nil; guard the assertion so a malformed attributeType is
// rejected below rather than panicking.
if attrType, ok := child.Value.(string); ok {

That comment describes the exact failure in NewControlSyncInfo, and it is your #618. This change applies the same shape to the sync controls: one parseSyncMembers helper that dispatches on tag, with the boolean assertion guarded.

What I did not change

NewControlSyncState (v3/control.go:1246) also switches on the child count, and it is correct -- there the cookie is the trailing optional, so position is unambiguous. I left it alone rather than churn it.

Verification

gofmt -l . clean, go vet . clean -- CI runs exactly those, then go test -v -cover -race -count=1 .. The offline half of that suite: ok github.com/go-ldap/ldap/v3 on both master and this branch, coverage 32.3% either way. I did not run the integration half, which needs make local-server and a Docker OpenLDAP; the network tests hang in my sandbox and I would rather say so than imply I ran them.

There was no test coverage at all for these constructors before this -- grep for NewControlSyncInfo|NewControlSyncDone|SyncInfo in *_test.go returns nothing -- which is why this went unnoticed. The new tests include an all_members_present row that passes on both sides, as a regression guard on the path that already worked.

I mutation-checked each site separately, anchored to its own case block since several are textual twins. All eight fail when weakened:

mutation	result
cookie case tag	Cookie = "", want "csn=1"
drop the boolean capture	RefreshDeletes = false, want true
uuid set case tag	SyncUUIDs = [], want 1 entry
syncDone flag apply	RefreshDeletes = false, want true
refreshDelete flag	RefreshDone = true, want false
refreshPresent flag	RefreshDone = true, want false
syncIdSet flag	RefreshDeletes = false, want true
uuidSet nil guard	panic: nil pointer dereference

Two notes on that, because both changed the diff:

• A ninth mutation survived -- a if child.ClassType != ber.ClassUniversal { continue } guard I had copied from the sorting sibling. It is load-bearing there (SortKey really does have context-tagged members) but unreachable here, since no syncInfoValue member is context-tagged. I removed those lines rather than ship code no test can pin.
• The uuidSet nil guard also survived at first. That one I kept and covered instead, with TestControlSyncInfoSyncIdSetWithoutUUIDs, because a server omitting the mandatory syncUUIDs would otherwise nil-deref.

Observable output

Identical for every input the old code handled -- the all_members_present row passes unmodified on both sides. It changes only the cases that were wrong. No existing test row had to change, since there were none.

---

Disclosure: AI-assisted. I found and prepared this with an AI assistant, and I ran and verified everything above myself.

[cpuschma]

LGTM. I'm currently on vacation and I've started to work on a restructuring of how server responses are handled. There's atleast a dozen potential problems with no proper bound checking.