From 8d244f7b76ba881253eb94611acaab31865c3920 Mon Sep 17 00:00:00 2001 From: Pieter-Jan Briers Date: Mon, 26 Feb 2024 03:36:38 +0100 Subject: [PATCH] Character profile sanitization improvements (#25579) Validate that job and antag prototypes can actually be set in character profiles, rather than just checking if the prototype exists. Make preferences system just call existing validation code when loading prototype from database, instead of some hacked-together stuff. Also I made the character profile validation logic take dependencies in via parameter because fuck resolves. --- .../Preferences/ClientPreferencesManager.cs | 6 +++- .../Managers/ServerPreferencesManager.cs | 31 ++----------------- .../Preferences/HumanoidCharacterProfile.cs | 29 +++++++++-------- .../Preferences/ICharacterProfile.cs | 9 +++++- 4 files changed, 29 insertions(+), 46 deletions(-) diff --git a/Content.Client/Preferences/ClientPreferencesManager.cs b/Content.Client/Preferences/ClientPreferencesManager.cs index 34b2c33140..b518493c9d 100644 --- a/Content.Client/Preferences/ClientPreferencesManager.cs +++ b/Content.Client/Preferences/ClientPreferencesManager.cs @@ -3,8 +3,10 @@ using System.Collections.Generic; using System.Linq; using Content.Shared.Preferences; using Robust.Client; +using Robust.Shared.Configuration; using Robust.Shared.IoC; using Robust.Shared.Network; +using Robust.Shared.Prototypes; using Robust.Shared.Utility; namespace Content.Client.Preferences @@ -18,6 +20,8 @@ namespace Content.Client.Preferences { [Dependency] private readonly IClientNetManager _netManager = default!; [Dependency] private readonly IBaseClient _baseClient = default!; + [Dependency] private readonly IConfigurationManager _cfg = default!; + [Dependency] private readonly IPrototypeManager _prototypes = default!; public event Action? OnServerDataLoaded; @@ -60,7 +64,7 @@ namespace Content.Client.Preferences public void UpdateCharacter(ICharacterProfile profile, int slot) { - profile.EnsureValid(); + profile.EnsureValid(_cfg, _prototypes); var characters = new Dictionary(Preferences.Characters) {[slot] = profile}; Preferences = new PlayerPreferences(characters, Preferences.SelectedCharacterIndex, Preferences.AdminOOCColor); var msg = new MsgUpdateCharacter diff --git a/Content.Server/Preferences/Managers/ServerPreferencesManager.cs b/Content.Server/Preferences/Managers/ServerPreferencesManager.cs index bcbe99edf5..e489ae28d5 100644 --- a/Content.Server/Preferences/Managers/ServerPreferencesManager.cs +++ b/Content.Server/Preferences/Managers/ServerPreferencesManager.cs @@ -99,7 +99,7 @@ namespace Content.Server.Preferences.Managers var curPrefs = prefsData.Prefs!; - profile.EnsureValid(); + profile.EnsureValid(_cfg, _protos); var profiles = new Dictionary(curPrefs.Characters) { @@ -270,34 +270,7 @@ namespace Content.Server.Preferences.Managers return new PlayerPreferences(prefs.Characters.Select(p => { - ICharacterProfile newProf; - switch (p.Value) - { - case HumanoidCharacterProfile hp: - { - var prototypeManager = IoCManager.Resolve(); - var selectedSpecies = HumanoidAppearanceSystem.DefaultSpecies; - - if (prototypeManager.TryIndex(hp.Species, out var species) && species.RoundStart) - { - selectedSpecies = hp.Species; - } - - newProf = hp - .WithJobPriorities( - hp.JobPriorities.Where(job => - _protos.HasIndex(job.Key))) - .WithAntagPreferences( - hp.AntagPreferences.Where(antag => - _protos.HasIndex(antag))) - .WithSpecies(selectedSpecies); - break; - } - default: - throw new NotSupportedException(); - } - - return new KeyValuePair(p.Key, newProf); + return new KeyValuePair(p.Key, p.Value.Validated(_cfg, _protos)); }), prefs.SelectedCharacterIndex, prefs.AdminOOCColor); } diff --git a/Content.Shared/Preferences/HumanoidCharacterProfile.cs b/Content.Shared/Preferences/HumanoidCharacterProfile.cs index 92c5b083dc..9cf45aaefd 100644 --- a/Content.Shared/Preferences/HumanoidCharacterProfile.cs +++ b/Content.Shared/Preferences/HumanoidCharacterProfile.cs @@ -371,10 +371,8 @@ namespace Content.Shared.Preferences return Appearance.MemberwiseEquals(other.Appearance); } - public void EnsureValid() + public void EnsureValid(IConfigurationManager configManager, IPrototypeManager prototypeManager) { - var prototypeManager = IoCManager.Resolve(); - if (!prototypeManager.TryIndex(Species, out var speciesPrototype) || speciesPrototype.RoundStart == false) { Species = SharedHumanoidAppearanceSystem.DefaultSpecies; @@ -390,15 +388,10 @@ namespace Content.Shared.Preferences }; // ensure the species can be that sex and their age fits the founds - var age = Age; - if (speciesPrototype != null) - { - if (!speciesPrototype.Sexes.Contains(sex)) - { - sex = speciesPrototype.Sexes[0]; - } - age = Math.Clamp(Age, speciesPrototype.MinAge, speciesPrototype.MaxAge); - } + if (!speciesPrototype.Sexes.Contains(sex)) + sex = speciesPrototype.Sexes[0]; + + var age = Math.Clamp(Age, speciesPrototype.MinAge, speciesPrototype.MaxAge); var gender = Gender switch { @@ -425,7 +418,6 @@ namespace Content.Shared.Preferences name = name.Trim(); - var configManager = IoCManager.Resolve(); if (configManager.GetCVar(CCVars.RestrictedNames)) { name = Regex.Replace(name, @"[^A-Z,a-z,0-9, -]", string.Empty); @@ -487,7 +479,7 @@ namespace Content.Shared.Preferences }; var priorities = new Dictionary(JobPriorities - .Where(p => prototypeManager.HasIndex(p.Key) && p.Value switch + .Where(p => prototypeManager.TryIndex(p.Key, out var job) && job.SetPreference && p.Value switch { JobPriority.Never => false, // Drop never since that's assumed default. JobPriority.Low => true, @@ -497,7 +489,7 @@ namespace Content.Shared.Preferences })); var antags = AntagPreferences - .Where(prototypeManager.HasIndex) + .Where(id => prototypeManager.TryIndex(id, out var antag) && antag.SetPreference) .ToList(); var traits = TraitPreferences @@ -530,6 +522,13 @@ namespace Content.Shared.Preferences _traitPreferences.AddRange(traits); } + public ICharacterProfile Validated(IConfigurationManager configManager, IPrototypeManager prototypeManager) + { + var profile = new HumanoidCharacterProfile(this); + profile.EnsureValid(configManager, prototypeManager); + return profile; + } + // sorry this is kind of weird and duplicated, /// working inside these non entity systems is a bit wack public static string GetName(string species, Gender gender) diff --git a/Content.Shared/Preferences/ICharacterProfile.cs b/Content.Shared/Preferences/ICharacterProfile.cs index a17a74498d..a9d30639bb 100644 --- a/Content.Shared/Preferences/ICharacterProfile.cs +++ b/Content.Shared/Preferences/ICharacterProfile.cs @@ -1,4 +1,6 @@ using Content.Shared.Humanoid; +using Robust.Shared.Configuration; +using Robust.Shared.Prototypes; namespace Content.Shared.Preferences { @@ -13,6 +15,11 @@ namespace Content.Shared.Preferences /// /// Makes this profile valid so there's no bad data like negative ages. /// - void EnsureValid(); + void EnsureValid(IConfigurationManager configManager, IPrototypeManager prototypeManager); + + /// + /// Gets a copy of this profile that has applied, i.e. no invalid data. + /// + ICharacterProfile Validated(IConfigurationManager configManager, IPrototypeManager prototypeManager); } } -- 2.52.0