fix(relay): reject invalid stored device IDs (#161)

CHANGELOG.md

@@ -12,6 +12,7 @@ All notable changes to VaultSync are documented here.
 
 ### Fixed
 
+- **Cloud Relay no longer continues from malformed saved Relay device IDs** ([#161](https://github.com/psimaker/vaultsync/issues/161)) — VaultSync reports that Cloud Relay provisioning did not complete and sends no provisioning request, without rewriting or deleting the stored value. Valid existing JSON and legacy records remain unchanged.
 - **Push and Cloud Relay registration details now survive failed secure-storage updates** ([#148](https://github.com/psimaker/vaultsync/issues/148)) — VaultSync keeps the last valid value when a replacement cannot be saved and reports the failure instead of continuing as if registration succeeded.
 - **Folder access now stays intact when reconnecting or syncing in the background** ([#147](https://github.com/psimaker/vaultsync/issues/147)) — reselecting the same Obsidian folder no longer accumulates access claims. Switching folders takes effect only after the new location is readable, scanned, and its permission is saved; any failure keeps the previous folder connected. Background runs release only their own access on completion, restart, or cancellation.
 - **Background sync no longer reports unfinished work as completed** ([#146](https://github.com/psimaker/vaultsync/issues/146)) — continued processing now reports success only after every expected vault is confirmed fully idle. If the sync engine stops, vault status cannot be read, a vault reports an error, the run expires or is cancelled, or the app returns to the foreground, the background run reports failure instead; conflict checks happen only after idle is proven.

ios/VaultSync/Services/SubscriptionManager.swift

@@ -117,14 +117,23 @@ enum RelayDeviceIDStorage {
     }
 
     static func decodeStoredValue(_ stored: String) -> LoadResult {
-        if stored.hasPrefix("[") {
+        let ids: [String]
+        if stored.trimmingCharacters(in: .whitespacesAndNewlines).hasPrefix("[") {
             guard let data = stored.data(using: .utf8),
-                  let ids = try? JSONDecoder().decode([String].self, from: data) else {
+                  let decoded = try? JSONDecoder().decode([String].self, from: data) else {
                 return .failed
             }
-            return .loaded(ids)
+            ids = decoded
+        } else {
+            ids = stored.components(separatedBy: ",")
+        }
+
+        guard ids.allSatisfy({ SyncthingDeviceID.canonicalize($0) != nil }) else {
+            return .failed
         }
-        return .loaded(stored.components(separatedBy: ",").filter { !$0.isEmpty })
+        // Validation must not become an implicit Keychain migration: preserve
+        // every valid stored value exactly as read.
+        return .loaded(ids)
     }
 
     static func remainingFailedIDs(

ios/VaultSyncTests/KeychainServiceTests.swift

@@ -301,6 +301,25 @@ struct APNsDeviceTokenRegistrationTests {
 @MainActor
 @Suite("Relay device ID persistence (#148)")
 struct RelayDeviceIDStorageTests {
+    private static let firstValidDeviceID = TestSupport.samplePeerDeviceID
+    private static let secondValidDeviceID =
+        "P56IOI7-MZJNU2Y-IQGDREY-DM2MGTI-MGL3BXN-PQ6W5BM-TBBZ4TJ-XZWICQ2"
+    private static let invalidDeviceID =
+        "P56IOI7-MZJNU2Y-IQGDREY-DM2MGTI-MGL3BXN-PQ6W5BM-TBBZ4TJ-XZWICQ3"
+
+    @MainActor
+    private final class ProvisionRecorder {
+        var deviceIDs: [String] = []
+
+        func provision(deviceID: String, token: String, signedTransaction: String) async throws {
+            deviceIDs.append(deviceID)
+        }
+    }
+
+    private static func json(_ deviceIDs: [String]) throws -> String {
+        String(decoding: try JSONEncoder().encode(deviceIDs), as: UTF8.self)
+    }
+
     @Test("Encoding failure never calls the Keychain writer")
     func issue148RelayEncodingFailureSkipsKeychain() {
         var writeCalls: [String] = []
@@ -373,6 +392,127 @@ struct RelayDeviceIDStorageTests {
         #expect(RelayDeviceIDStorage.decodeStoredValue("[not-json") == .failed)
     }
 
+    @Test("Valid JSON storage loads every device ID unchanged (#161)")
+    func issue161ValidJSONStorageLoads() throws {
+        let stored = try Self.json([Self.firstValidDeviceID, Self.secondValidDeviceID])
+
+        #expect(RelayDeviceIDStorage.decodeStoredValue(stored) == .loaded([
+            Self.firstValidDeviceID,
+            Self.secondValidDeviceID,
+        ]))
+    }
+
+    @Test("Valid JSON with leading whitespace loads unchanged (#161)")
+    func issue161WhitespacePrefixedJSONStorageLoads() throws {
+        let stored = " \n\t" + (try Self.json([
+            Self.firstValidDeviceID,
+            Self.secondValidDeviceID,
+        ]))
+
+        #expect(RelayDeviceIDStorage.decodeStoredValue(stored) == .loaded([
+            Self.firstValidDeviceID,
+            Self.secondValidDeviceID,
+        ]))
+    }
+
+    @Test("A JSON array containing only an invalid device ID fails closed (#161)")
+    func issue161InvalidJSONMemberFailsClosed() throws {
+        let stored = try Self.json([Self.invalidDeviceID])
+
+        #expect(RelayDeviceIDStorage.decodeStoredValue(stored) == .failed)
+    }
+
+    @Test("A mixed-validity JSON array fails as one indivisible read (#161)")
+    func issue161MixedJSONFailsClosed() throws {
+        let stored = try Self.json([Self.firstValidDeviceID, Self.invalidDeviceID])
+
+        #expect(RelayDeviceIDStorage.decodeStoredValue(stored) == .failed)
+    }
+
+    @Test("Valid legacy comma-separated storage loads every device ID unchanged (#161)")
+    func issue161ValidLegacyStorageLoads() {
+        let stored = [Self.firstValidDeviceID, Self.secondValidDeviceID].joined(separator: ",")
+
+        #expect(RelayDeviceIDStorage.decodeStoredValue(stored) == .loaded([
+            Self.firstValidDeviceID,
+            Self.secondValidDeviceID,
+        ]))
+    }
+
+    @Test("An invalid legacy member fails the entire read closed (#161)")
+    func issue161InvalidLegacyMemberFailsClosed() {
+        let stored = [Self.firstValidDeviceID, Self.invalidDeviceID].joined(separator: ",")
+
+        #expect(RelayDeviceIDStorage.decodeStoredValue(stored) == .failed)
+    }
+
+    @Test("Any empty legacy member fails the entire read closed (#161)")
+    func issue161EmptyLegacyMemberFailsClosed() {
+        let malformedValues = [
+            "",
+            ",\(Self.firstValidDeviceID)",
+            "\(Self.firstValidDeviceID),",
+            "\(Self.firstValidDeviceID),,\(Self.secondValidDeviceID)",
+            ",,,",
+        ]
+
+        for stored in malformedValues {
+            #expect(RelayDeviceIDStorage.decodeStoredValue(stored) == .failed)
+        }
+    }
+
+    @Test("Invalid stored IDs cannot be rewritten or provisioned (#161)")
+    func issue161InvalidStorageSkipsWriteAndProvisioning() async throws {
+        let stored = try Self.json([Self.firstValidDeviceID, Self.invalidDeviceID])
+        var encodeCount = 0
+        var writeCount = 0
+        let environment = RelayDeviceIDStorage.Environment(
+            load: { RelayDeviceIDStorage.decodeStoredValue(stored) },
+            encode: { _ in
+                encodeCount += 1
+                return "encoded"
+            },
+            write: { _ in
+                writeCount += 1
+                return true
+            }
+        )
+
+        let persistence = RelayDeviceIDStorage.persist(
+            [Self.secondValidDeviceID],
+            environment: environment
+        )
+        let preparation = RelayDeviceIDStorage.prepareTarget(
+            source: .storedDeviceIDs,
+            persist: { _ in
+                Issue.record("A stored-device-ID target must not invoke persistence")
+                return .failed(.keychain)
+            },
+            load: environment.load
+        )
+
+        let provisionRecorder = ProvisionRecorder()
+        if case .ready(let deviceIDs) = preparation {
+            let entitlement = try #require(RelayVerifiedEntitlement(
+                signedTransaction: "header.payload.signature"
+            ))
+            _ = await RelayReprovisioning.run(
+                trigger: .tokenRotation,
+                deviceIDs: deviceIDs,
+                statuses: [:],
+                entitlement: .verified(entitlement),
+                apnsToken: "token",
+                provision: provisionRecorder.provision
+            )
+        }
+
+        #expect(persistence == .failed(.read))
+        #expect(preparation == .skip(nil))
+        #expect(encodeCount == 0)
+        #expect(writeCount == 0)
+        #expect(provisionRecorder.deviceIDs.isEmpty)
+    }
+
     @Test("Every caller context blocks its production target after storage failure")
     func issue148EveryRelayCallerHandlesPersistenceFailure() {
         #expect(RelayDeviceIDStorage.Context.allCases.count == 5)

ios/VaultSyncTests/SubscriptionManagerTests.swift

@@ -5,6 +5,9 @@ import Testing
 @MainActor
 @Suite("Relay Provision State Machine", .serialized)
 struct SubscriptionManagerTests {
+    private static let invalidStoredDeviceJSON =
+        "[\"P56IOI7-MZJNU2Y-IQGDREY-DM2MGTI-MGL3BXN-PQ6W5BM-TBBZ4TJ-XZWICQ3\"]"
+
     @Test("RelayProvisionStatus exposes stable summary contract")
     func relayProvisionStatusContract() {
         #expect(RelayProvisionStatus.notAttempted.stateKey == "not_attempted")
@@ -66,4 +69,39 @@ struct SubscriptionManagerTests {
         )
         #expect(manager.relayProvisioningNeedsAttention)
     }
+
+    @Test("Invalid stored IDs use the existing generic storage failure without rewrite (#161)")
+    func issue161InvalidStoredIDsSurfaceGenericFailure() async {
+        TestSupport.resetRelayState()
+        defer { TestSupport.resetRelayState() }
+
+        var encodeCount = 0
+        var writeCount = 0
+        let manager = SubscriptionManager(
+            relayDeviceIDStorageEnvironment: RelayDeviceIDStorage.Environment(
+                load: {
+                    RelayDeviceIDStorage.decodeStoredValue(Self.invalidStoredDeviceJSON)
+                },
+                encode: { _ in
+                    encodeCount += 1
+                    return "encoded"
+                },
+                write: { _ in
+                    writeCount += 1
+                    return true
+                }
+            ),
+            startsLiveWork: false
+        )
+
+        await manager.retryRelayProvisioning(homeserverDeviceIDs: [])
+
+        #expect(
+            manager.relayDeviceIDStorageErrorMessage
+                == L10n.tr("Cloud Relay provisioning did not complete.")
+        )
+        #expect(manager.relayProvisioningNeedsAttention)
+        #expect(encodeCount == 0)
+        #expect(writeCount == 0)
+    }
 }