From 14f939a3b59e85a2b0cfbfde16bf4624ead747b1 Mon Sep 17 00:00:00 2001 From: Max Weber Date: Tue, 9 Feb 2021 11:52:58 -0700 Subject: [PATCH] ConfigManager: don't allow access to partially loaded configs If you did a getConfiguration while the config was being loaded from the client you could get a value from the new config or the old config. Since some plugins would check profile values from the ConfigChange events being posted it could cause the ConfigManager to observe an unset rsprofile.loginSalt, which would make it generate a new one, overwriting the one the server had, causing all profiles to be lost. --- .../runelite/client/config/ConfigManager.java | 186 ++++++++---------- 1 file changed, 84 insertions(+), 102 deletions(-) diff --git a/runelite-client/src/main/java/net/runelite/client/config/ConfigManager.java b/runelite-client/src/main/java/net/runelite/client/config/ConfigManager.java index 549ef0b123..b4bc89c2da 100644 --- a/runelite-client/src/main/java/net/runelite/client/config/ConfigManager.java +++ b/runelite-client/src/main/java/net/runelite/client/config/ConfigManager.java @@ -27,7 +27,6 @@ package net.runelite.client.config; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Strings; import com.google.common.collect.ComparisonChain; -import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; import com.google.common.hash.Hasher; import com.google.common.hash.Hashing; @@ -128,9 +127,10 @@ public class ConfigManager private final Client client; private final ConfigInvocationHandler handler = new ConfigInvocationHandler(this); - private final Properties properties = new Properties(); private final Map pendingChanges = new HashMap<>(); + private Properties properties = new Properties(); + // null => we need to make a new profile @Nullable private String rsProfileKey; @@ -225,35 +225,14 @@ public class ConfigManager return; } - handler.invalidate(); - properties.clear(); - + Properties newProperties = new Properties(); for (ConfigEntry entry : configuration.getConfig()) { - log.debug("Loading configuration value from client {}: {}", entry.getKey(), entry.getValue()); - - String[] split = splitKey(entry.getKey()); - if (split == null) - { - continue; - } - - final String groupName = split[KEY_SPLITTER_GROUP]; - final String profile = split[KEY_SPLITTER_PROFILE]; - final String key = split[KEY_SPLITTER_KEY]; - final String value = entry.getValue(); - final String oldValue = (String) properties.setProperty(entry.getKey(), value); - - ConfigChanged configChanged = new ConfigChanged(); - configChanged.setGroup(groupName); - configChanged.setProfile(profile); - configChanged.setKey(key); - configChanged.setOldValue(oldValue); - configChanged.setNewValue(value); - eventBus.post(configChanged); + newProperties.setProperty(entry.getKey(), entry.getValue()); } - migrateConfig(); + log.debug("Loading in config from server"); + swapProperties(newProperties, false); try { @@ -267,7 +246,64 @@ public class ConfigManager } } - private synchronized void syncPropertiesFromFile(File propertiesFile) + private void swapProperties(Properties newProperties, boolean saveToServer) + { + Set allKeys = new HashSet<>(newProperties.keySet()); + + Properties oldProperties; + synchronized (this) + { + handler.invalidate(); + oldProperties = properties; + this.properties = newProperties; + } + + updateRSProfile(); + + allKeys.addAll(oldProperties.keySet()); + + for (Object wholeKey : allKeys) + { + String[] split = splitKey((String) wholeKey); + if (split == null) + { + continue; + } + + String groupName = split[KEY_SPLITTER_GROUP]; + String profile = split[KEY_SPLITTER_PROFILE]; + String key = split[KEY_SPLITTER_KEY]; + String oldValue = (String) oldProperties.get(wholeKey); + String newValue = (String) newProperties.get(wholeKey); + + if (Objects.equals(oldValue, newValue)) + { + continue; + } + + log.debug("Loading configuration value {}: {}", wholeKey, newValue); + + ConfigChanged configChanged = new ConfigChanged(); + configChanged.setGroup(groupName); + configChanged.setProfile(profile); + configChanged.setKey(key); + configChanged.setOldValue(oldValue); + configChanged.setNewValue(newValue); + eventBus.post(configChanged); + + if (saveToServer) + { + synchronized (pendingChanges) + { + pendingChanges.put((String) wholeKey, newValue); + } + } + } + + migrateConfig(); + } + + private void syncPropertiesFromFile(File propertiesFile) { final Properties properties = new Properties(); try (FileInputStream in = new FileInputStream(propertiesFile)) @@ -276,44 +312,12 @@ public class ConfigManager } catch (Exception e) { - log.debug("Malformed properties, skipping update"); + log.warn("Malformed properties, skipping update"); return; } - final Map copy = (Map) ImmutableMap.copyOf(this.properties); - copy.forEach((wholeKey, value) -> - { - if (!properties.containsKey(wholeKey)) - { - String[] split = splitKey(wholeKey); - if (split == null) - { - return; - } - - String groupName = split[KEY_SPLITTER_GROUP]; - String profile = split[KEY_SPLITTER_PROFILE]; - String key = split[KEY_SPLITTER_KEY]; - unsetConfiguration(groupName, profile, key); - } - }); - - properties.forEach((wholeKey, objValue) -> - { - String[] split = splitKey((String) wholeKey); - if (split == null) - { - return; - } - - String groupName = split[KEY_SPLITTER_GROUP]; - String profile = split[KEY_SPLITTER_PROFILE]; - String key = split[KEY_SPLITTER_KEY]; - String value = String.valueOf(objValue); - setConfiguration(groupName, profile, key, value); - }); - - migrateConfig(); + log.debug("Loading in config from disk for upload"); + swapProperties(properties, true); } public Future importLocal() @@ -343,12 +347,10 @@ public class ConfigManager private synchronized void loadFromFile() { - handler.invalidate(); - properties.clear(); - + Properties newProperties = new Properties(); try (FileInputStream in = new FileInputStream(propertiesFile)) { - properties.load(new InputStreamReader(in, StandardCharsets.UTF_8)); + newProperties.load(new InputStreamReader(in, StandardCharsets.UTF_8)); } catch (FileNotFoundException ex) { @@ -359,38 +361,8 @@ public class ConfigManager log.warn("Unable to load settings", ex); } - try - { - Map copy = (Map) ImmutableMap.copyOf(properties); - copy.forEach((wholeKey, value) -> - { - String[] split = splitKey(wholeKey); - if (split == null) - { - log.debug("Properties key malformed!: {}", wholeKey); - properties.remove(wholeKey); - return; - } - - String groupName = split[KEY_SPLITTER_GROUP]; - String profile = split[KEY_SPLITTER_PROFILE]; - String key = split[KEY_SPLITTER_KEY]; - - ConfigChanged configChanged = new ConfigChanged(); - configChanged.setGroup(groupName); - configChanged.setProfile(profile); - configChanged.setKey(key); - configChanged.setOldValue(null); - configChanged.setNewValue(value); - eventBus.post(configChanged); - }); - } - catch (Exception ex) - { - log.warn("Error posting config events", ex); - } - - migrateConfig(); + log.debug("Loading in config from disk"); + swapProperties(newProperties, false); } private void saveToFile(final File propertiesFile) throws IOException @@ -519,7 +491,11 @@ public class ConfigManager assert !key.startsWith(RSPROFILE_GROUP + "."); String wholeKey = getWholeKey(groupName, profile, key); - String oldValue = (String) properties.setProperty(wholeKey, value); + String oldValue; + synchronized (this) + { + oldValue = (String) properties.setProperty(wholeKey, value); + } if (Objects.equals(oldValue, value)) { @@ -599,7 +575,11 @@ public class ConfigManager { assert !key.startsWith(RSPROFILE_GROUP + "."); String wholeKey = getWholeKey(groupName, profile, key); - String oldValue = (String) properties.remove(wholeKey); + String oldValue; + synchronized (this) + { + oldValue = (String) properties.remove(wholeKey); + } if (oldValue == null) { @@ -977,6 +957,7 @@ public class ConfigManager salt = new byte[15]; new SecureRandom() .nextBytes(salt); + log.info("creating new salt as there is no existing one {}", Base64.getUrlEncoder().encodeToString(salt)); setConfiguration(RSPROFILE_GROUP, RSPROFILE_LOGIN_SALT, salt); } @@ -1013,7 +994,7 @@ public class ConfigManager String keyStr = RSPROFILE_GROUP + "." + Base64.getUrlEncoder().encodeToString(key); if (!keys.contains(keyStr)) { - log.info("creating new profile {} for user {}", key, username); + log.info("creating new profile {} for user {} ({}) salt {}", keyStr, username, type, Base64.getUrlEncoder().encodeToString(salt)); setConfiguration(RSPROFILE_GROUP, keyStr, RSPROFILE_LOGIN_HASH, loginHash); setConfiguration(RSPROFILE_GROUP, keyStr, RSPROFILE_TYPE, type); @@ -1071,6 +1052,7 @@ public class ConfigManager /** * Split a config key into (group, profile, key) + * * @param key in form group.(rsprofile.profile.)?key * @return an array of {group, profile, key} */