Give the two logs and the buckets a resource type, so a conflict can be written

AadResourceTypes.For maps a syncable type onto the AAD resource type its cache
records bind to, and it had no arm for ConnectionLogEntry, ActivityLogEntry or
ObjectStore. All three are on both enums, in the reconciler registry and in the
cipher pinning; only this switch was missed, and it throws rather than falling
back — so a merge conflict on a connection log, an activity log or a bucket
raised ArgumentOutOfRangeException on the path that records what the merge
discarded. The conflict log is the whole reason the merge is allowed to pick a
winner, so the one item kind whose conflicts could not be recorded was a bucket:
an editable item two machines can genuinely disagree about.

Worth writing down why it lasted two phases. Of the three callers, ItemStore and
OutboxStore reach the mapping only when an item carries plaintext fields, and
none of these three kinds does — so they never touched the gap. ConflictStore
calls it unconditionally, but a test only reaches that by causing a real merge
conflict, and every existing one raised its conflict against a Host. Three arms
missing, and no path in the suite crossed any of them.

So the tests are the point of this commit as much as the arms are. The guard is
AadResourceTypeTests.EverySyncableType_HasAnArmInTheStorageMapping: it walks the
whole wire enum, and for each type asserts both that there is an arm and that the
arm returns the same-named resource type, which is the mistake the file's cipher
half already guards against on the server side. Written over the full enum rather
than over ItemKinds.SyncedTypes, because that is the stronger claim and the one
the switch really makes — the two reserved association types have arms too.
Beside it, CacheStoreTests.AConflict_CanBeRecordedForEveryKindOfItem records a
conflict per kind and reads the detail back, since an arm returning the wrong
resource type seals under one AAD and opens under another, which surfaces as an
empty detail rather than as a throw.

Both were confirmed to fail with the arms removed: the theory fails on exactly
ConnectionLogEntry, ActivityLogEntry and ObjectStore and passes on the other
three, and the guard names those three and no others.

The note in docs/adding-hosts-on-the-phone.md that recorded this as out of scope
is marked fixed, with what let it survive, since that is the part worth knowing
next time an item kind is added.

1529 tests pass, seven of them new.
This commit is contained in:
2026-08-04 10:24:47 +02:00
parent 27bb1deb5d
commit 6ae1912c34
4 changed files with 134 additions and 3 deletions
+8 -3
View File
@@ -325,9 +325,14 @@ because-string used as prose.
`ConflictStore.Record` throws `ArgumentOutOfRangeException` on a conflict for any of those three. Pre-existing, `ConflictStore.Record` throws `ArgumentOutOfRangeException` on a conflict for any of those three. Pre-existing,
unrelated to any of this, and `Tag` is already in that switch. Worth a separate fix. unrelated to any of this, and `Tag` is already in that switch. Worth a separate fix.
> **Still open.** All three are on `SyncEntityType` and on `CryptoSpec.AadResourceType`, and all three are > **Fixed 2026-08-04**, having outlived the phases that shipped the logs and the buckets — which is exactly
> still absent from `LocalCacheProtector.For` — so this outlived the phases that shipped the logs and the > the drift a note like this is meant to prevent, so it is worth saying what let it last. Of the three callers
> buckets, which is exactly the drift a note like this is meant to prevent. > of `AadResourceTypes.For`, two reach it only when an item carries plaintext fields and none of these three
> does; the third, `ConflictStore.RecordAsync`, calls it unconditionally but is reached only by a real merge
> conflict, which every existing test raised against a `Host`. The arms are in, and two tests now hold them
> there: `CacheStoreTests.AConflict_CanBeRecordedForEveryKindOfItem` records one per kind, and
> `AadResourceTypeTests.EverySyncableType_HasAnArmInTheStorageMapping` fails on the *next* item type added
> without one, by name rather than by a list kept by hand.
## Prose that becomes false ## Prose that becomes false
@@ -98,10 +98,27 @@ public sealed class LocalCacheProtector : IDisposable
/// Maps a syncable entity type onto the resource type its AAD binds. /// Maps a syncable entity type onto the resource type its AAD binds.
/// </summary> /// </summary>
/// <remarks> /// <remarks>
/// <para>
/// A switch rather than a cast, even though the two enums happen to be adjacent. They are not the /// A switch rather than a cast, even though the two enums happen to be adjacent. They are not the
/// same list: <see cref="CryptoSpec.AadResourceType"/> also covers users, devices and vaults, so the /// same list: <see cref="CryptoSpec.AadResourceType"/> also covers users, devices and vaults, so the
/// numbers do not line up, and a cast would bind an item's ciphertext to the wrong resource type /// numbers do not line up, and a cast would bind an item's ciphertext to the wrong resource type
/// without failing anywhere a test would notice. /// without failing anywhere a test would notice.
/// </para>
/// <para>
/// <b>It has to name every syncable type, and the throw is not a safety net.</b>
/// <c>ConflictStore.RecordAsync</c> calls this unconditionally, so a type with no arm here is a type
/// whose <em>conflicts</em> cannot be recorded — and the conflict log is the only reason the merge is
/// allowed to pick a winner at all. Three types went two phases without one: the two logs and the
/// buckets were added to both enums and to the reconciler registry, and this switch was not touched,
/// so a merge over any of them turned a recorded conflict into an
/// <see cref="ArgumentOutOfRangeException"/>. The two other callers hid it —
/// <c>ItemStore</c> and <c>OutboxStore</c> only reach this when an item carries plaintext fields, and
/// none of those three does.
/// </para>
/// <para>
/// <c>AadResourceTypeTests.EverySyncableType_HasAnArmInTheStorageMapping</c> is what says so now, and it
/// is a name comparison rather than a list to keep by hand.
/// </para>
/// </remarks> /// </remarks>
internal static class AadResourceTypes internal static class AadResourceTypes
{ {
@@ -117,6 +134,14 @@ internal static class AadResourceTypes
SyncEntityType.Snippet => CryptoSpec.AadResourceType.Snippet, SyncEntityType.Snippet => CryptoSpec.AadResourceType.Snippet,
SyncEntityType.PortForward => CryptoSpec.AadResourceType.PortForward, SyncEntityType.PortForward => CryptoSpec.AadResourceType.PortForward,
SyncEntityType.KnownHostKey => CryptoSpec.AadResourceType.KnownHostKey, SyncEntityType.KnownHostKey => CryptoSpec.AadResourceType.KnownHostKey,
// Appended in the order the enums grew, which is why these three are not beside their
// neighbours by number. See docs/crypto.md §4.3 on why 1416 are not one-behind their wire
// counterparts the way the arms above are.
SyncEntityType.ConnectionLogEntry => CryptoSpec.AadResourceType.ConnectionLogEntry,
SyncEntityType.ActivityLogEntry => CryptoSpec.AadResourceType.ActivityLogEntry,
SyncEntityType.ObjectStore => CryptoSpec.AadResourceType.ObjectStore,
_ => throw new ArgumentOutOfRangeException( _ => throw new ArgumentOutOfRangeException(
nameof(entityType), entityType, "No AAD resource type is defined for this entity type."), nameof(entityType), entityType, "No AAD resource type is defined for this entity type."),
}; };
@@ -394,6 +394,49 @@ public sealed class CacheStoreTests : IAsyncLifetime
all.Detail.ShouldBe(new byte[] { 1, 2, 3 }); all.Detail.ShouldBe(new byte[] { 1, 2, 3 });
} }
/// <summary>
/// A conflict can be recorded against any kind of item, not only the kinds that were here first.
/// </summary>
/// <remarks>
/// <para>
/// Every other test in this section uses <see cref="SyncEntityType.Host"/>, and that is how three item
/// kinds shipped with no way to record a conflict at all: the two logs and the buckets were added to both
/// enums, to the reconciler registry and to the cipher pinning, while <c>AadResourceTypes.For</c> — which
/// <c>ConflictStore.RecordAsync</c> calls unconditionally — kept throwing for them. The two other callers
/// of that mapping only reach it when an item carries plaintext fields, which none of the three does, so
/// nothing else so much as touched the gap.
/// </para>
/// <para>
/// A theory over the types rather than one more <c>Host</c> case, because the failure was never about
/// conflicts and always about which types the layer below had been taught. Recording is asserted through
/// a read-back rather than by "it did not throw": an arm returning the wrong resource type would seal
/// under one AAD and open under another, which is a null detail rather than an exception.
/// </para>
/// </remarks>
[Theory]
[InlineData(SyncEntityType.Host)]
[InlineData(SyncEntityType.HostGroup)]
[InlineData(SyncEntityType.Snippet)]
[InlineData(SyncEntityType.ConnectionLogEntry)]
[InlineData(SyncEntityType.ActivityLogEntry)]
[InlineData(SyncEntityType.ObjectStore)]
public async Task AConflict_CanBeRecordedForEveryKindOfItem(SyncEntityType entityType)
{
var detail = System.Text.Encoding.UTF8.GetBytes($$"""{"kind":"{{entityType}}"}""");
var id = await harness.Conflicts.RecordAsync(
VaultId, entityType, Guid.CreateVersion7(), ConflictKind.FieldOverridden, detail, Token);
var listed = (await harness.Conflicts.ListAsync(VaultId, false, Token)).ShouldHaveSingleItem();
listed.Id.ShouldBe(id);
listed.EntityType.ShouldBe(entityType);
listed.Detail.ShouldBe(
detail,
"an empty detail here means the record was sealed under one resource type and opened under "
+ "another, which ListAsync reports as nothing rather than as a failure");
}
[Fact] [Fact]
public async Task AnUnacknowledgedConflict_CannotBeDiscarded() public async Task AnUnacknowledgedConflict_CannotBeDiscarded()
{ {
@@ -1,5 +1,6 @@
using System.Security.Cryptography; using System.Security.Cryptography;
using DodoSSH.Client.Domain; using DodoSSH.Client.Domain;
using DodoSSH.Client.Storage;
using DodoSSH.Client.Sync; using DodoSSH.Client.Sync;
using DodoSSH.Contracts; using DodoSSH.Contracts;
using DodoSSH.Crypto; using DodoSSH.Crypto;
@@ -151,6 +152,63 @@ public sealed class AadResourceTypeTests
.ShouldBe(ItemKinds.SyncedTypes, ignoreOrder: true); .ShouldBe(ItemKinds.SyncedTypes, ignoreOrder: true);
} }
/// <summary>
/// The same pairing, made a second time in the storage layer, and every type must be in it.
/// </summary>
/// <remarks>
/// <para>
/// <c>AadResourceTypes.For</c> is the cache's copy of the table above: the ciphers seal an item for the
/// <em>server</em>, and this seals the two things the local cache holds in the clear — a relay host's
/// address, and the values a merge overrode. A type missing from it throws rather than mis-seals, which
/// sounds like the safe failure and is not: <c>ConflictStore.RecordAsync</c> calls it unconditionally, so
/// the exception lands on the path that records what a merge discarded.
/// </para>
/// <para>
/// This is written after finding three types missing from it — <c>ConnectionLogEntry</c>,
/// <c>ActivityLogEntry</c> and <c>ObjectStore</c> went two shipping phases without an arm, because the
/// only unconditional caller is one a test suite reaches solely by causing a real merge conflict. Asserted
/// over the whole wire enum rather than over <c>ItemKinds.SyncedTypes</c>, which is the stronger claim and
/// the one the switch actually makes: the two reserved association types have arms too.
/// </para>
/// </remarks>
[Fact]
public void EverySyncableType_HasAnArmInTheStorageMapping()
{
var missing = new List<SyncEntityType>();
foreach (var wire in Enum.GetValues<SyncEntityType>())
{
if (wire == SyncEntityType.Unspecified)
{
continue;
}
CryptoSpec.AadResourceType resource;
try
{
resource = AadResourceTypes.For(wire);
}
catch (ArgumentOutOfRangeException)
{
missing.Add(wire);
continue;
}
// Same name, as the cipher table demands — a wrong-but-present arm is the failure this half
// would otherwise wave through.
Enum.GetName(resource).ShouldBe(
Enum.GetName(wire),
$"AadResourceTypes.For({wire}) returns {resource}, which binds this type's cache records "
+ "to another type's resource.");
}
missing.ShouldBeEmpty(
"every syncable type needs an arm in AadResourceTypes.For, or a conflict recorded against one "
+ "of these throws instead of being written — and the conflict log is what justifies the merge "
+ "picking a winner.");
}
private static EncryptedPayload SealSample( private static EncryptedPayload SealSample(
SyncEntityType wire, SyncEntityType wire,
byte[] vaultKey, byte[] vaultKey,