diff --git a/CHANGELOG.md b/CHANGELOG.md index 059ad11b3..a3ed1916f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ ## Next Version +### Fixed +- Added validation to ensure that all values in `settings.configs` are mappings. Previously, passing non-mapping values did not raise an error, making it difficult to detect misconfigurations. Now, `SpecParsingError.invalidConfigsMappingFormat` is thrown if misused. #1547 @Ryu0118 + ## 2.43.0 ### Added diff --git a/Sources/ProjectSpec/AggregateTarget.swift b/Sources/ProjectSpec/AggregateTarget.swift index 1a3225c40..9bea7098c 100644 --- a/Sources/ProjectSpec/AggregateTarget.swift +++ b/Sources/ProjectSpec/AggregateTarget.swift @@ -60,7 +60,7 @@ extension AggregateTarget: NamedJSONDictionaryConvertible { public init(name: String, jsonDictionary: JSONDictionary) throws { self.name = jsonDictionary.json(atKeyPath: "name") ?? name targets = jsonDictionary.json(atKeyPath: "targets") ?? [] - settings = jsonDictionary.json(atKeyPath: "settings") ?? .empty + settings = try BuildSettingsParser(jsonDictionary: jsonDictionary).parse() configFiles = jsonDictionary.json(atKeyPath: "configFiles") ?? [:] buildScripts = jsonDictionary.json(atKeyPath: "buildScripts") ?? [] buildToolPlugins = jsonDictionary.json(atKeyPath: "buildToolPlugins") ?? [] diff --git a/Sources/ProjectSpec/BuildSettingsExtractor.swift b/Sources/ProjectSpec/BuildSettingsExtractor.swift new file mode 100644 index 000000000..1dfeb93de --- /dev/null +++ b/Sources/ProjectSpec/BuildSettingsExtractor.swift @@ -0,0 +1,37 @@ +import Foundation +import JSONUtilities + +/// A helper for extracting and validating the `Settings` object from a JSON dictionary. +struct BuildSettingsParser { + let jsonDictionary: JSONDictionary + + /// Attempts to extract and parse the `Settings` from the dictionary. + /// + /// - Returns: A valid `Settings` object + func parse() throws -> Settings { + do { + return try jsonDictionary.json(atKeyPath: "settings") + } catch let specParsingError as SpecParsingError { + // Re-throw `SpecParsingError` to prevent the misuse of settings.configs. + throw specParsingError + } catch { + // Ignore all errors except `SpecParsingError` + return .empty + } + } + + /// Attempts to extract and parse setting groups from the dictionary with fallback defaults. + /// + /// - Returns: Parsed setting groups or default groups if parsing fails + func parseSettingGroups() throws -> [String: Settings] { + do { + return try jsonDictionary.json(atKeyPath: "settingGroups", invalidItemBehaviour: .fail) + } catch let specParsingError as SpecParsingError { + // Re-throw `SpecParsingError` to prevent the misuse of settingGroups. + throw specParsingError + } catch { + // Ignore all errors except `SpecParsingError` + return jsonDictionary.json(atKeyPath: "settingPresets") ?? [:] + } + } +} diff --git a/Sources/ProjectSpec/Project.swift b/Sources/ProjectSpec/Project.swift index 72daf0324..70fb9bc33 100644 --- a/Sources/ProjectSpec/Project.swift +++ b/Sources/ProjectSpec/Project.swift @@ -171,11 +171,13 @@ extension Project { self.basePath = basePath let jsonDictionary = Project.resolveProject(jsonDictionary: jsonDictionary) + let buildSettingsParser = BuildSettingsParser(jsonDictionary: jsonDictionary) name = try jsonDictionary.json(atKeyPath: "name") - settings = jsonDictionary.json(atKeyPath: "settings") ?? .empty - settingGroups = jsonDictionary.json(atKeyPath: "settingGroups") - ?? jsonDictionary.json(atKeyPath: "settingPresets") ?? [:] + + settings = try buildSettingsParser.parse() + settingGroups = try buildSettingsParser.parseSettingGroups() + let configs: [String: String] = jsonDictionary.json(atKeyPath: "configs") ?? [:] self.configs = configs.isEmpty ? Config.defaultConfigs : configs.map { Config(name: $0, type: ConfigType(rawValue: $1)) }.sorted { $0.name < $1.name } diff --git a/Sources/ProjectSpec/Settings.swift b/Sources/ProjectSpec/Settings.swift index f59006a72..b3b366a86 100644 --- a/Sources/ProjectSpec/Settings.swift +++ b/Sources/ProjectSpec/Settings.swift @@ -28,7 +28,8 @@ public struct Settings: Equatable, JSONObjectConvertible, CustomStringConvertibl groups = jsonDictionary.json(atKeyPath: "groups") ?? jsonDictionary.json(atKeyPath: "presets") ?? [] let buildSettingsDictionary: JSONDictionary = jsonDictionary.json(atKeyPath: "base") ?? [:] buildSettings = buildSettingsDictionary - configSettings = jsonDictionary.json(atKeyPath: "configs") ?? [:] + + self.configSettings = try Self.extractValidConfigs(from: jsonDictionary) } else { buildSettings = jsonDictionary configSettings = [:] @@ -36,6 +37,26 @@ public struct Settings: Equatable, JSONObjectConvertible, CustomStringConvertibl } } + /// Extracts and validates the `configs` mapping from the given JSON dictionary. + /// - Parameter jsonDictionary: The JSON dictionary to extract `configs` from. + /// - Returns: A dictionary mapping configuration names to `Settings` objects. + private static func extractValidConfigs(from jsonDictionary: JSONDictionary) throws -> [String: Settings] { + guard let configSettings = jsonDictionary["configs"] as? JSONDictionary else { + return [:] + } + + let invalidConfigKeys = Set( + configSettings.filter { !($0.value is JSONDictionary) } + .map(\.key) + ) + + guard invalidConfigKeys.isEmpty else { + throw SpecParsingError.invalidConfigsMappingFormat(keys: invalidConfigKeys) + } + + return try jsonDictionary.json(atKeyPath: "configs") + } + public static func == (lhs: Settings, rhs: Settings) -> Bool { NSDictionary(dictionary: lhs.buildSettings).isEqual(to: rhs.buildSettings) && lhs.configSettings == rhs.configSettings && diff --git a/Sources/ProjectSpec/SpecParsingError.swift b/Sources/ProjectSpec/SpecParsingError.swift index b875eadce..57db99f2e 100644 --- a/Sources/ProjectSpec/SpecParsingError.swift +++ b/Sources/ProjectSpec/SpecParsingError.swift @@ -15,6 +15,7 @@ public enum SpecParsingError: Error, CustomStringConvertible { case unknownBreakpointActionType(String) case unknownBreakpointActionConveyanceType(String) case unknownBreakpointActionSoundName(String) + case invalidConfigsMappingFormat(keys: Set) public var description: String { switch self { @@ -46,6 +47,8 @@ public enum SpecParsingError: Error, CustomStringConvertible { return "Unknown Breakpoint Action conveyance type: \(type)" case let .unknownBreakpointActionSoundName(name): return "Unknown Breakpoint Action sound name: \(name)" + case let .invalidConfigsMappingFormat(keys): + return "Invalid format: The value for \"\(keys.sorted().joined(separator: ", "))\" in `configs` must be mapping format" } } } diff --git a/Sources/ProjectSpec/Target.swift b/Sources/ProjectSpec/Target.swift index 3b9f2c535..781994f42 100644 --- a/Sources/ProjectSpec/Target.swift +++ b/Sources/ProjectSpec/Target.swift @@ -316,7 +316,7 @@ extension Target: NamedJSONDictionaryConvertible { deploymentTarget = nil } - settings = jsonDictionary.json(atKeyPath: "settings") ?? .empty + settings = try BuildSettingsParser(jsonDictionary: jsonDictionary).parse() configFiles = jsonDictionary.json(atKeyPath: "configFiles") ?? [:] if let source: String = jsonDictionary.json(atKeyPath: "sources") { sources = [TargetSource(path: source)] diff --git a/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_aggregate_targets.yml b/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_aggregate_targets.yml new file mode 100644 index 000000000..293e384df --- /dev/null +++ b/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_aggregate_targets.yml @@ -0,0 +1,24 @@ +name: InvalidConfigsValueNonMappingAggregateTargets + +# This fixture tests validation of `settings.configs` under an aggregate target. +# Here, `invalid_key0` and `invalid_key1` are scalar values (not mappings), +# so parsing should throw SpecParsingError.invalidConfigsMappingFormat. +targets: + valid_target1: + type: application + platform: iOS + valid_target2: + type: application + platform: iOS + +aggregateTargets: + invalid_target: + targets: + - valid_target1 + - valid_target2 + settings: + configs: + invalid_key0: value0 + debug: + valid_key: value1 + invalid_key1: value2 diff --git a/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_setting_groups.yml b/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_setting_groups.yml new file mode 100644 index 000000000..90822c24f --- /dev/null +++ b/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_setting_groups.yml @@ -0,0 +1,19 @@ +name: InvalidConfigsValueNonMappingSettingGroups + +# This fixture tests validation of `settings.configs` under an aggregate target. +# Here, `invalid_key0` and `invalid_key1` are scalar values (not mappings), +# so parsing should throw SpecParsingError.invalidConfigsMappingFormat. +settingGroups: + invalid_preset: + configs: + invalid_key0: value0 + debug: + valid_key: value1 + invalid_key1: value2 +targets: + invalid_target: + type: application + platform: iOS + settings: + groups: + - invalid_preset diff --git a/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_settings.yml b/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_settings.yml new file mode 100644 index 000000000..431804b70 --- /dev/null +++ b/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_settings.yml @@ -0,0 +1,11 @@ +name: InvalidConfigsValueNonMappingSettings + +# This fixture tests validation of `settings.configs` at the top level. +# Here, `invalid_key0` and `invalid_key1` are scalar values (not mappings), +# so parsing should throw SpecParsingError.invalidConfigsMappingFormat. +settings: + configs: + invalid_key0: value0 + debug: + valid_key: value1 + invalid_key1: value2 diff --git a/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_targets.yml b/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_targets.yml new file mode 100644 index 000000000..84a803bce --- /dev/null +++ b/Tests/Fixtures/invalid_configs/invalid_configs_value_non_mapping_targets.yml @@ -0,0 +1,15 @@ +name: InvalidConfigsValueNonMappingTargets + +# This fixture tests nested validation of `settings.configs` under a target. +# Here, `invalid_key0` and `invalid_key1` are scalar values (not mappings), +# so parsing should throw SpecParsingError.invalidConfigsMappingFormat. +targets: + invalid_target: + type: application + platform: iOS + settings: + configs: + invalid_key0: value0 + debug: + valid_key: value1 + invalid_key1: value2 diff --git a/Tests/ProjectSpecTests/InvalidConfigsFormatTests.swift b/Tests/ProjectSpecTests/InvalidConfigsFormatTests.swift new file mode 100644 index 000000000..e10236088 --- /dev/null +++ b/Tests/ProjectSpecTests/InvalidConfigsFormatTests.swift @@ -0,0 +1,43 @@ +import ProjectSpec +import Testing +import TestSupport +import PathKit + +struct invalidConfigsMappingFormatTests { + struct InvalidConfigsTestArguments { + var fixturePath: Path + var expectedError: SpecParsingError + } + + private static var testArguments: [InvalidConfigsTestArguments] { + let invalidConfigsFixturePath: Path = fixturePath + "invalid_configs" + return [ + InvalidConfigsTestArguments( + fixturePath: invalidConfigsFixturePath + "invalid_configs_value_non_mapping_settings.yml", + expectedError: SpecParsingError.invalidConfigsMappingFormat(keys: ["invalid_key0", "invalid_key1"]) + ), + InvalidConfigsTestArguments( + fixturePath: invalidConfigsFixturePath + "invalid_configs_value_non_mapping_targets.yml", + expectedError: SpecParsingError.invalidConfigsMappingFormat(keys: ["invalid_key0", "invalid_key1"]) + ), + InvalidConfigsTestArguments( + fixturePath: invalidConfigsFixturePath + "invalid_configs_value_non_mapping_aggregate_targets.yml", + expectedError: SpecParsingError.invalidConfigsMappingFormat(keys: ["invalid_key0", "invalid_key1"]) + ), + InvalidConfigsTestArguments( + fixturePath: invalidConfigsFixturePath + "invalid_configs_value_non_mapping_setting_groups.yml", + expectedError: SpecParsingError.invalidConfigsMappingFormat(keys: ["invalid_key0", "invalid_key1"]) + ) + ] + } + + @Test("throws invalidConfigsMappingFormat for non-mapping configs entries", arguments: testArguments) + func testInvalidConfigsMappingFormat(_ arguments: InvalidConfigsTestArguments) throws { + #expect { + try Project(path: arguments.fixturePath) + } throws: { actualError in + (actualError as any CustomStringConvertible).description + == arguments.expectedError.description + } + } +}