Fix delete of allowed external resource hosts (#6841)

Author: labkey-adam

core/src/org/labkey/core/CoreUpgradeCode.java

@@ -174,6 +174,7 @@ public static void migrateAllowedExternalConnectionHosts(ModuleContext context)
             .map(host -> new AllowedHost(Directive.Connection, host))
             .toList();
 
+        // No need to synchronize since upgrade is single-threaded
         AllowedExternalResourceHosts.saveAllowedHosts(allowedHosts, context.getUpgradeUser());
     }
 }

core/src/org/labkey/core/admin/AdminController.java

@@ -11380,46 +11380,52 @@ public ModelAndView getView(ExternalSourcesForm form, boolean reshow, BindExcept
             return new VBox(newView, existingView);
         }
 
+        private static final Object HOST_LOCK = new Object();
+
         @Override
         public boolean handlePost(ExternalSourcesForm form, BindException errors) throws Exception
         {
             List<AllowedHost> allowedHosts = null;
 
-            //handle delete of existing value
-            if (form.isDelete())
+            // Multiple requests could access this in parallel, so synchronize access, Issue 53457
+            synchronized (HOST_LOCK)
             {
-                AllowedHost subToDelete = form.getExistingAllowedHost(errors);
-                if (errors.hasErrors())
-                    return false;
-                allowedHosts = form.getSavedAllowedHosts();
-                var iter = allowedHosts.listIterator();
-                while (iter.hasNext())
+                //handle delete of an existing value
+                if (form.isDelete())
                 {
-                    AllowedHost sub = iter.next();
-                    if (sub.equals(subToDelete))
+                    AllowedHost subToDelete = form.getExistingAllowedHost(errors);
+                    if (errors.hasErrors())
+                        return false;
+                    allowedHosts = form.getSavedAllowedHosts();
+                    var iter = allowedHosts.listIterator();
+                    while (iter.hasNext())
                     {
-                        iter.remove();
-                        break;
+                        AllowedHost sub = iter.next();
+                        if (sub.equals(subToDelete))
+                        {
+                            iter.remove();
+                            break;
+                        }
                     }
                 }
-            }
-            //handle updates - clicking on Save button under Existing will save the updated urls
-            else if (form.isSaveAll())
-            {
-                allowedHosts = form.getExistingAllowedHosts(errors);
+                //handle updates - clicking on Save button under Existing will save the updated hosts
+                else if (form.isSaveAll())
+                {
+                    allowedHosts = form.getExistingAllowedHosts(errors);
+                    if (errors.hasErrors())
+                        return false;
+                }
+                //save new external value
+                else if (form.isSaveNew())
+                {
+                    allowedHosts = form.validateNewAllowedHost(errors);
+                }
+
                 if (errors.hasErrors())
                     return false;
-            }
-            //save new external value
-            else if (form.isSaveNew())
-            {
-                allowedHosts = form.validateNewAllowedHost(errors);
-            }
 
-            if (errors.hasErrors())
-                return false;
-
-            AllowedExternalResourceHosts.saveAllowedHosts(allowedHosts, getUser());
+                AllowedExternalResourceHosts.saveAllowedHosts(allowedHosts, getUser());
+            }
 
             return true;
         }

core/src/org/labkey/core/security/AllowedExternalResourceHosts.java

@@ -37,6 +37,7 @@ private AllowedExternalResourceHosts()
 
     public record AllowedHost(Directive directive, String host) { }
 
+    // Callers must ensure thread-safe access to this method
     public static void saveAllowedHosts(@Nullable Collection<AllowedHost> allowedHosts, User user)
     {
         if (null != allowedHosts)
@@ -63,14 +64,24 @@ public static void saveAllowedHosts(@Nullable Collection<AllowedHost> allowedHos
 
             // Unregister all supported directives then register the directives that have at least one allowed host
             Arrays.stream(Directive.values()).forEach(dir -> {
-                ContentSecurityPolicyFilter.unregisterAllowedSources(ALLOWED_EXTERNAL_RESOURCES, dir);
+                unregister(dir);
                 List<String> list = map.get(dir);
                 if (list != null)
-                    ContentSecurityPolicyFilter.registerAllowedSources(ALLOWED_EXTERNAL_RESOURCES, dir, list.toArray(new String[0]));
+                    register(dir, list.toArray(new String[0]));
             });
         }
     }
 
+    private static void register(Directive dir, String... hosts)
+    {
+        ContentSecurityPolicyFilter.registerAllowedSources(ALLOWED_EXTERNAL_RESOURCES, dir, hosts);
+    }
+
+    private static void unregister(Directive dir)
+    {
+        ContentSecurityPolicyFilter.unregisterAllowedSources(ALLOWED_EXTERNAL_RESOURCES, dir);
+    }
+
     // Returns a mutable list (mutating it won't affect any cached values)
     public static List<AllowedHost> readAllowedHosts() throws JsonProcessingException
     {
@@ -92,7 +103,7 @@ public static void registerHosts()
             return;
         }
 
-        list.forEach(sub -> ContentSecurityPolicyFilter.registerAllowedSources("External Sources", sub.directive(), sub.host()));
+        list.forEach(sub -> register(sub.directive(), sub.host()));
         LOG.debug("Registered [{}] as allowed external sources", list);
     }
 
@@ -125,6 +136,7 @@ public void handle(Map<Directive, StartupPropertyEntry> properties)
                     }
                     else
                     {
+                        // No need to synchronize since startup property handling is single-threaded
                         saveAllowedHosts(allowedHosts, User.getAdminServiceUser());
                     }
                 }