From 9d4c27f7caec0741bb682ea81c70fadd094ceb8e Mon Sep 17 00:00:00 2001 From: Jon Petersson Date: Mon, 13 May 2024 13:41:50 +0200 Subject: [PATCH] Fix user input string validation --- .../AccessMethodRepository.swift | 4 ++++ .../CustomListRepository.swift | 23 +++++++++++++++---- .../PersistentAccessMethod.swift | 13 +++++++++++ ios/MullvadTypes/NameInputFormatter.swift | 15 ++++++++++++ ios/MullvadVPN.xcodeproj/project.pbxproj | 4 ++++ .../CustomListCellConfiguration.swift | 3 ++- .../CustomListDataSourceConfiguration.swift | 9 ++++++-- .../CustomListValidationError.swift | 14 ++++------- .../CustomListViewController.swift | 6 +++-- .../MethodSettingsCellConfiguration.swift | 2 ++ .../Models/AccessMethodValidationError.swift | 14 +++++++++++ .../AccessMethodViewModel+Persistent.swift | 5 +++- 12 files changed, 93 insertions(+), 19 deletions(-) create mode 100644 ios/MullvadTypes/NameInputFormatter.swift diff --git a/ios/MullvadSettings/AccessMethodRepository.swift b/ios/MullvadSettings/AccessMethodRepository.swift index 2df3ce7c893e..a1193f917b92 100644 --- a/ios/MullvadSettings/AccessMethodRepository.swift +++ b/ios/MullvadSettings/AccessMethodRepository.swift @@ -9,6 +9,7 @@ import Combine import Foundation import MullvadLogging +import MullvadTypes public class AccessMethodRepository: AccessMethodRepositoryProtocol { private let logger = Logger(label: "AccessMethodRepository") @@ -54,6 +55,9 @@ public class AccessMethodRepository: AccessMethodRepositoryProtocol { public func save(_ method: PersistentAccessMethod) { var methodStore = readApiAccessMethodStore() + var method = method + method.name = method.name.trimmingCharacters(in: .whitespaces) + if let index = methodStore.accessMethods.firstIndex(where: { $0.id == method.id }) { methodStore.accessMethods[index] = method } else { diff --git a/ios/MullvadSettings/CustomListRepository.swift b/ios/MullvadSettings/CustomListRepository.swift index 700a69c446dd..3a9a32dc483b 100644 --- a/ios/MullvadSettings/CustomListRepository.swift +++ b/ios/MullvadSettings/CustomListRepository.swift @@ -11,8 +11,9 @@ import Foundation import MullvadLogging import MullvadTypes -public enum CustomRelayListError: LocalizedError, Equatable { +public enum CustomRelayListError: LocalizedError, Hashable { case duplicateName + case nameTooLong public var errorDescription: String? { switch self { @@ -20,9 +21,19 @@ public enum CustomRelayListError: LocalizedError, Equatable { NSLocalizedString( "DUPLICATE_CUSTOM_LISTS_ERROR", tableName: "CustomLists", - value: "Name is already taken.", + value: "A custom list with this name exists, please choose a unique name.", comment: "" ) + case .nameTooLong: + String( + format: NSLocalizedString( + "CUSTOM_LIST_NAME_TOO_LONG_ERROR", + tableName: "CustomLists", + value: "Name should be no longer than %i characters.", + comment: "" + ), + NameInputFormatter.maxLength + ) } } } @@ -37,11 +48,15 @@ public struct CustomListRepository: CustomListRepositoryProtocol { public init() {} public func save(list: CustomList) throws { - var list = list - list.name = list.name.trimmingCharacters(in: .whitespaces) + guard list.name.count <= NameInputFormatter.maxLength else { + throw CustomRelayListError.nameTooLong + } var lists = fetchAll() + var list = list + list.name = list.name.trimmingCharacters(in: .whitespaces) + if let listWithSameName = lists.first(where: { $0.name.compare(list.name) == .orderedSame }), listWithSameName.id != list.id { throw CustomRelayListError.duplicateName diff --git a/ios/MullvadSettings/PersistentAccessMethod.swift b/ios/MullvadSettings/PersistentAccessMethod.swift index 9d67ce849f5c..2194167a8e27 100644 --- a/ios/MullvadSettings/PersistentAccessMethod.swift +++ b/ios/MullvadSettings/PersistentAccessMethod.swift @@ -40,6 +40,19 @@ public struct PersistentAccessMethod: Identifiable, Codable, Equatable { self.proxyConfiguration = proxyConfiguration } + public init(from decoder: any Decoder) throws { + let container = try decoder.container(keyedBy: CodingKeys.self) + + self.id = try container.decode(UUID.self, forKey: .id) + self.isEnabled = try container.decode(Bool.self, forKey: .isEnabled) + self.proxyConfiguration = try container.decode(PersistentProxyConfiguration.self, forKey: .proxyConfiguration) + + // Added after release of API access methods feature. There was previously no limitation on text input length, + // so this formatting has been added to prevent already stored names from being too long when displayed. + let name = try container.decode(String.self, forKey: .name) + self.name = NameInputFormatter.format(name) + } + public static func == (lhs: Self, rhs: Self) -> Bool { lhs.id == rhs.id } diff --git a/ios/MullvadTypes/NameInputFormatter.swift b/ios/MullvadTypes/NameInputFormatter.swift new file mode 100644 index 000000000000..a65c7a7438ab --- /dev/null +++ b/ios/MullvadTypes/NameInputFormatter.swift @@ -0,0 +1,15 @@ +// +// NameInputFormatter.swift +// MullvadVPN +// +// Created by Jon Petersson on 2024-05-13. +// Copyright © 2024 Mullvad VPN AB. All rights reserved. +// + +public struct NameInputFormatter { + public static let maxLength = 30 + + public static func format(_ string: String, maxLength: Int = Self.maxLength) -> String { + String(string.trimmingCharacters(in: .whitespaces).prefix(maxLength)) + } +} diff --git a/ios/MullvadVPN.xcodeproj/project.pbxproj b/ios/MullvadVPN.xcodeproj/project.pbxproj index dbef55e82506..776943004beb 100644 --- a/ios/MullvadVPN.xcodeproj/project.pbxproj +++ b/ios/MullvadVPN.xcodeproj/project.pbxproj @@ -540,6 +540,7 @@ 7A6F2FAD2AFD3DA7006D0856 /* CustomDNSViewController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A6F2FAC2AFD3DA7006D0856 /* CustomDNSViewController.swift */; }; 7A6F2FAF2AFE36E7006D0856 /* VPNSettingsInfoButtonItem.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A6F2FAE2AFE36E7006D0856 /* VPNSettingsInfoButtonItem.swift */; }; 7A7907332BC0280A00B61F81 /* InterceptibleNavigationController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A7907322BC0280A00B61F81 /* InterceptibleNavigationController.swift */; }; + 7A7AD14F2BF21EF200B30B3C /* NameInputFormatter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A7AD14D2BF21DCE00B30B3C /* NameInputFormatter.swift */; }; 7A7AD28D29DC677800480EF1 /* FirstTimeLaunch.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A7AD28C29DC677800480EF1 /* FirstTimeLaunch.swift */; }; 7A818F1F29F0305800C7F0F4 /* RootConfiguration.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A818F1E29F0305800C7F0F4 /* RootConfiguration.swift */; }; 7A83A0C62B29A750008B5CE7 /* APIAccessMethodsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 7A83A0C52B29A750008B5CE7 /* APIAccessMethodsTests.swift */; }; @@ -1850,6 +1851,7 @@ 7A6F2FAC2AFD3DA7006D0856 /* CustomDNSViewController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = CustomDNSViewController.swift; sourceTree = ""; }; 7A6F2FAE2AFE36E7006D0856 /* VPNSettingsInfoButtonItem.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = VPNSettingsInfoButtonItem.swift; sourceTree = ""; }; 7A7907322BC0280A00B61F81 /* InterceptibleNavigationController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = InterceptibleNavigationController.swift; sourceTree = ""; }; + 7A7AD14D2BF21DCE00B30B3C /* NameInputFormatter.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = NameInputFormatter.swift; sourceTree = ""; }; 7A7AD28C29DC677800480EF1 /* FirstTimeLaunch.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FirstTimeLaunch.swift; sourceTree = ""; }; 7A818F1E29F0305800C7F0F4 /* RootConfiguration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = RootConfiguration.swift; sourceTree = ""; }; 7A83A0C52B29A750008B5CE7 /* APIAccessMethodsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = APIAccessMethodsTests.swift; sourceTree = ""; }; @@ -2544,6 +2546,7 @@ 58A1AA8623F43901009F7EA6 /* Location.swift */, 5840250322B11AB700E4CFEC /* MullvadEndpoint.swift */, 58D223D7294C8E5E0029F5F8 /* MullvadTypes.h */, + 7A7AD14D2BF21DCE00B30B3C /* NameInputFormatter.swift */, A97FF54F2A0D2FFC00900996 /* NSFileCoordinator+Extensions.swift */, 58CAFA01298530DC00BE19F7 /* Promise.swift */, 449EBA242B975B7C00DFA4EB /* Protocols */, @@ -5843,6 +5846,7 @@ 58D22410294C90210029F5F8 /* Location.swift in Sources */, 58D22411294C90210029F5F8 /* MullvadEndpoint.swift in Sources */, 58D22412294C90210029F5F8 /* RelayConstraint.swift in Sources */, + 7A7AD14F2BF21EF200B30B3C /* NameInputFormatter.swift in Sources */, 58D22413294C90210029F5F8 /* RelayConstraints.swift in Sources */, 7AF9BE8C2A321D1F00DBFEDB /* RelayFilter.swift in Sources */, 58D22414294C90210029F5F8 /* RelayLocation.swift in Sources */, diff --git a/ios/MullvadVPN/Coordinators/CustomLists/CustomListCellConfiguration.swift b/ios/MullvadVPN/Coordinators/CustomLists/CustomListCellConfiguration.swift index 10f2c3ef7cb5..372a7b4e0707 100644 --- a/ios/MullvadVPN/Coordinators/CustomLists/CustomListCellConfiguration.swift +++ b/ios/MullvadVPN/Coordinators/CustomLists/CustomListCellConfiguration.swift @@ -7,6 +7,7 @@ // import Combine +import MullvadTypes import UIKit struct CustomListCellConfiguration { @@ -75,7 +76,7 @@ struct CustomListCellConfiguration { contentConfiguration.setPlaceholder(type: .required) contentConfiguration.textFieldProperties = .withSmartFeaturesDisabled() contentConfiguration.inputText = subject.value.name - contentConfiguration.maxLength = 30 + contentConfiguration.maxLength = NameInputFormatter.maxLength contentConfiguration.editingEvents.onChange = subject.bindTextAction(to: \.name) cell.accessibilityIdentifier = AccessibilityIdentifier.customListEditNameFieldCell diff --git a/ios/MullvadVPN/Coordinators/CustomLists/CustomListDataSourceConfiguration.swift b/ios/MullvadVPN/Coordinators/CustomLists/CustomListDataSourceConfiguration.swift index 77bbefd8bc6c..b5598abf3e05 100644 --- a/ios/MullvadVPN/Coordinators/CustomLists/CustomListDataSourceConfiguration.swift +++ b/ios/MullvadVPN/Coordinators/CustomLists/CustomListDataSourceConfiguration.swift @@ -92,7 +92,12 @@ extension CustomListDataSourceConfiguration: UITableViewDelegate { let errorsInSection = itemsWithErrors.filter { itemsInSection.contains($0) }.compactMap { item in switch item { case .name: - CustomListFieldValidationError.name + Array(validationErrors).filter { error in + if case .name = error { + return true + } + return false + } case .addLocations, .editLocations, .deleteList: nil } @@ -102,7 +107,7 @@ extension CustomListDataSourceConfiguration: UITableViewDelegate { case .name: let view = SettingsFieldValidationErrorContentView( configuration: SettingsFieldValidationErrorConfiguration( - errors: errorsInSection.settingsFieldValidationErrors + errors: errorsInSection.flatMap { $0.settingsFieldValidationErrors } ) ) return view diff --git a/ios/MullvadVPN/Coordinators/CustomLists/CustomListValidationError.swift b/ios/MullvadVPN/Coordinators/CustomLists/CustomListValidationError.swift index 100cff15a6f9..35880b03714d 100644 --- a/ios/MullvadVPN/Coordinators/CustomLists/CustomListValidationError.swift +++ b/ios/MullvadVPN/Coordinators/CustomLists/CustomListValidationError.swift @@ -7,19 +7,15 @@ // import Foundation +import MullvadSettings -enum CustomListFieldValidationError: LocalizedError { - case name +enum CustomListFieldValidationError: LocalizedError, Hashable { + case name(CustomRelayListError) var errorDescription: String? { switch self { - case .name: - NSLocalizedString( - "CUSTOM_LISTS_VALIDATION_ERROR_EMPTY_FIELD", - tableName: "CutstomLists", - value: "A custom list with this name exists, please choose a unique name.", - comment: "" - ) + case let .name(error): + error.errorDescription } } } diff --git a/ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift b/ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift index baf4e8949a43..6e4fe54ac573 100644 --- a/ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift +++ b/ios/MullvadVPN/Coordinators/CustomLists/CustomListViewController.swift @@ -154,8 +154,10 @@ class CustomListViewController: UIViewController { try interactor.save(viewModel: subject.value) delegate?.customListDidSave(subject.value.customList) } catch { - validationErrors.insert(.name) - dataSourceConfiguration?.set(validationErrors: validationErrors) + if let error = error as? CustomRelayListError { + validationErrors.insert(.name(error)) + dataSourceConfiguration?.set(validationErrors: validationErrors) + } } } diff --git a/ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsCellConfiguration.swift b/ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsCellConfiguration.swift index 979a0ad9c3a7..9378efef7436 100644 --- a/ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsCellConfiguration.swift +++ b/ios/MullvadVPN/Coordinators/Settings/APIAccess/Edit/MethodSettings/MethodSettingsCellConfiguration.swift @@ -7,6 +7,7 @@ // import Combine +import MullvadTypes import UIKit class MethodSettingsCellConfiguration { @@ -109,6 +110,7 @@ class MethodSettingsCellConfiguration { contentConfiguration.setPlaceholder(type: .required) contentConfiguration.textFieldProperties = .withSmartFeaturesDisabled() contentConfiguration.inputText = subject.value.name + contentConfiguration.maxLength = NameInputFormatter.maxLength contentConfiguration.editingEvents.onChange = subject.bindTextAction(to: \.name) cell.accessibilityIdentifier = .accessMethodNameTextField diff --git a/ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodValidationError.swift b/ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodValidationError.swift index 6bd2bd327448..60d247e626a3 100644 --- a/ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodValidationError.swift +++ b/ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodValidationError.swift @@ -7,6 +7,7 @@ // import Foundation +import MullvadTypes /// Access method validation error that holds an array of individual per-field validation errors. struct AccessMethodValidationError: LocalizedError, Equatable { @@ -57,6 +58,9 @@ struct AccessMethodFieldValidationError: LocalizedError, Equatable { /// Invalid port number, i.e zero. case invalidPort + + /// The name input is too long. + case nameTooLong } /// Kind of validation error. @@ -91,6 +95,16 @@ struct AccessMethodFieldValidationError: LocalizedError, Equatable { value: "Please enter a valid port.", comment: "" ) + case .nameTooLong: + String( + format: NSLocalizedString( + "VALIDATION_ERRORS_NAME_TOO_LONG", + tableName: "APIAccess", + value: "Name should be no longer than %i characters.", + comment: "" + ), + NameInputFormatter.maxLength + ) } } } diff --git a/ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel+Persistent.swift b/ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel+Persistent.swift index 3a1d938e0551..0d8182b5afb6 100644 --- a/ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel+Persistent.swift +++ b/ios/MullvadVPN/Coordinators/Settings/APIAccess/Models/AccessMethodViewModel+Persistent.swift @@ -66,10 +66,13 @@ extension AccessMethodViewModel { } private func validateName() throws -> String { + // Context doesn't matter for name field errors. if name.isEmpty { - // Context doesn't matter for name field. let fieldError = AccessMethodFieldValidationError(kind: .emptyValue, field: .name, context: .shadowsocks) throw AccessMethodValidationError(fieldErrors: [fieldError]) + } else if name.count > NameInputFormatter.maxLength { + let fieldError = AccessMethodFieldValidationError(kind: .nameTooLong, field: .name, context: .shadowsocks) + throw AccessMethodValidationError(fieldErrors: [fieldError]) } return name