From cc6b92e4641a6a81f9365d2e9712e60e48d994e3 Mon Sep 17 00:00:00 2001 From: Umut Erdem Date: Thu, 27 Aug 2026 19:54:47 +0200 Subject: [PATCH 1/2] fix(relay): reject invalid stored device IDs (#161) Validate every JSON and legacy Relay device ID before storage-dependent work. Treat any malformed member, including empty legacy fields, as a failed read so VaultSync neither rewrites secure storage nor starts provisioning. What could go wrong and why this is safe: accepting one malformed identifier could rewrite corrupt state or trigger paid Relay work with an invalid target. Valid values remain unchanged, the existing generic error is reused, and failed reads perform no migration, deletion, write, or provisioning. Not verified: physical-device Keychain corruption and production Relay E2E remain part of the isolated release-candidate campaign. Fixes #161 --- CHANGELOG.md | 1 + .../Services/SubscriptionManager.swift | 15 ++- ios/VaultSyncTests/KeychainServiceTests.swift | 127 ++++++++++++++++++ .../SubscriptionManagerTests.swift | 38 ++++++ 4 files changed, 178 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 212c018..8e6cdb0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ All notable changes to VaultSync are documented here. ### Fixed +- **Cloud Relay no longer continues from invalid saved server identities** ([#161](https://github.com/psimaker/vaultsync/issues/161)) — if secure storage contains a malformed server identifier, VaultSync reports that registration did not complete and sends no registration 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. diff --git a/ios/VaultSync/Services/SubscriptionManager.swift b/ios/VaultSync/Services/SubscriptionManager.swift index d09ed0f..9746b24 100644 --- a/ios/VaultSync/Services/SubscriptionManager.swift +++ b/ios/VaultSync/Services/SubscriptionManager.swift @@ -117,14 +117,23 @@ enum RelayDeviceIDStorage { } static func decodeStoredValue(_ stored: String) -> LoadResult { + let ids: [String] if stored.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( diff --git a/ios/VaultSyncTests/KeychainServiceTests.swift b/ios/VaultSyncTests/KeychainServiceTests.swift index f330b80..64e7d15 100644 --- a/ios/VaultSyncTests/KeychainServiceTests.swift +++ b/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,114 @@ 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("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) diff --git a/ios/VaultSyncTests/SubscriptionManagerTests.swift b/ios/VaultSyncTests/SubscriptionManagerTests.swift index 2d45232..c127c5d 100644 --- a/ios/VaultSyncTests/SubscriptionManagerTests.swift +++ b/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) + } } From dad92edd68480ea713d2fe443e3bbb1da9f6723d Mon Sep 17 00:00:00 2001 From: Umut Erdem Date: Thu, 27 Aug 2026 20:15:48 +0200 Subject: [PATCH 2/2] fix(relay): accept whitespace-prefixed stored JSON (#161) Select the stored format from a whitespace-trimmed view while decoding the original JSON value, and align the changelog with Relay device-ID and provisioning terminology. What could go wrong and why this is safe: valid JSON with leading whitespace could otherwise be mistaken for a legacy identifier and block Relay work. The change affects format selection only; every decoded member still passes canonical validation, malformed reads remain fail-closed, and no rewrite, delete, or provisioning path is added. --- CHANGELOG.md | 2 +- ios/VaultSync/Services/SubscriptionManager.swift | 2 +- ios/VaultSyncTests/KeychainServiceTests.swift | 13 +++++++++++++ 3 files changed, 15 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8e6cdb0..cba2a58 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ All notable changes to VaultSync are documented here. ### Fixed -- **Cloud Relay no longer continues from invalid saved server identities** ([#161](https://github.com/psimaker/vaultsync/issues/161)) — if secure storage contains a malformed server identifier, VaultSync reports that registration did not complete and sends no registration request, without rewriting or deleting the stored value. Valid existing JSON and legacy records remain unchanged. +- **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. diff --git a/ios/VaultSync/Services/SubscriptionManager.swift b/ios/VaultSync/Services/SubscriptionManager.swift index 9746b24..d7743ac 100644 --- a/ios/VaultSync/Services/SubscriptionManager.swift +++ b/ios/VaultSync/Services/SubscriptionManager.swift @@ -118,7 +118,7 @@ enum RelayDeviceIDStorage { static func decodeStoredValue(_ stored: String) -> LoadResult { let ids: [String] - if stored.hasPrefix("[") { + if stored.trimmingCharacters(in: .whitespacesAndNewlines).hasPrefix("[") { guard let data = stored.data(using: .utf8), let decoded = try? JSONDecoder().decode([String].self, from: data) else { return .failed diff --git a/ios/VaultSyncTests/KeychainServiceTests.swift b/ios/VaultSyncTests/KeychainServiceTests.swift index 64e7d15..8441346 100644 --- a/ios/VaultSyncTests/KeychainServiceTests.swift +++ b/ios/VaultSyncTests/KeychainServiceTests.swift @@ -402,6 +402,19 @@ struct RelayDeviceIDStorageTests { ])) } + @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])