Skip to content

acllist: reject a new record with no applicable content - #795

Merged
cheggaaa merged 4 commits into
mainfrom
go-7544-acl-empty-content
Sep 29, 2026
Merged

cheggaaa merged 4 commits into
mainfrom
go-7544-acl-empty-content

Conversation

@requilence

@requilence requilence commented Sep 28, 2026 •

Copy link
Copy Markdown
Contributor

What

ACL permission checks run inside the per-content-type apply handlers. A record whose content reaches none of them was applied without any check on its author:

  • a content value with the oneof unset, or of a type this build does not know, fell through applyChangeContent's default and returned nil;
  • an AclData with no content values (empty Data, or only unknown fields) looped zero times;
  • a permissionChanges value with no changes reached its handler, which checks the author per change, so it checked nothing.

The existing ErrEmptyAclRecordData guard never runs on live paths: both Unmarshall and UnmarshallWithId always set Model.

Fix

Reject both cases at admission only: ValidateRawRecord (used by the coordinator before handing a record to consensus) and the record builder's preflight. Both now use a dedicated admission validator.

Everything that replays records already in the log stays lenient, whatever the verifier:

  • BuildAclListWithIdentity (clients, and node stats / anytype-heart paths that build with ValidateFull)
  • storage migration (aclmigrator re-adds records via AddRawRecords with ValidateFull)
  • sync ingest (AddRawRecord)

A record already in the log was accepted by the network, and a content type added in a later release reaches old clients the same way, so failing there would stop the ACL at that record.

Compatibility

  • Existing ACLs containing such records keep building, migrating and syncing (covered by tests for both verifiers).
  • BuildBatchRequest with an empty payload now fails locally with ErrNoAclContent instead of sending an empty record. RevokeAllInvites therefore returns early when there is nothing to revoke (it previously sent an empty record), so a retry after the revocation already landed still succeeds. Likewise ChangePermissions with no changes (anytype-heart drops changes a member already has, so a retried change can arrive empty) returns without sending.
  • A new content type must reach the coordinator before clients start sending it (already the required deploy order, since the coordinator validates content).

Tests

  • TestAclList_ValidateRawRecordRejectsNoApplicableContent: all four shapes rejected at admission.
  • TestAclList_StoredNoApplicableContentStillBuilds: the same shapes, once stored, still ingest, rebuild, accept new owner records on top, and migrate into a fresh list, under validating and non-validating verifiers.
  • TestAclList_ValidateRawRecordRejectsMixedContent: a valid change does not carry an inapplicable one past admission in either order, and afterValid never runs.
  • TestAclRecordBuilder_EmptyBatchRequest, TestAclSpaceClient_RevokeAllInvites, TestAclSpaceClient_ChangePermissionsWithoutChanges.

go test ./... passes.

Linear: GO-7544

Permission checks live in the per-type apply handlers, so a record whose
content reaches none of them was applied without checking its author: an
unset oneof or unknown content type fell through applyChangeContent's
default, and an AclData with no content values looped zero times. The
ErrEmptyAclRecordData guard never ran on live paths, since both unmarshal
paths always set Model.

Reject both at admission only (ValidateRawRecord and the builder's
preflight). A record already in the log was accepted by the network, and a
content type added later reaches old clients the same way, so build,
migration and sync ingest keep applying it as a no-op whatever the
verifier.

GO-7544
@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

New Coverage 62.0% of statements
Patch Coverage 100.0% of changed statements (32/32)

Coverage provided by https://github.com/seriousben/go-patch-cover-action

A permissionChanges value checks its author per change, so one with no
changes reached its handler and checked nothing, like the no-content
records. ValidateAclData now rejects it at admission; stored ones still
replay.

RevokeAllInvites returns early when there is nothing to revoke instead of
building a record the preflight now refuses, so a retry after the
revocation already landed still succeeds.

GO-7544
A caller that drops the changes members already have may be left with
none, e.g. retrying a change that already landed. Building that record now
fails admission for carrying no content, so return early instead.

GO-7544
@requilence
requilence marked this pull request as ready for review September 29, 2026 09:17
@requilence
requilence requested a review from cheggaaa September 29, 2026 09:17
@cheggaaa
cheggaaa merged commit aa8fac7 into main Sep 29, 2026
4 checks passed
@cheggaaa
cheggaaa deleted the go-7544-acl-empty-content branch September 29, 2026 13:08
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 29, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants