Disallow mutating SQL in GET requests

api/src/org/labkey/api/ApiModule.java

@@ -219,6 +219,7 @@ public class ApiModule extends CodeOnlyModule
 
     public static final String EXTJS_3_REQUIRED = "ExtJs3Required";
     public static final String EXTJS_3_API_REQUIRED = "ExtJs3ApiRequired";
+    public static final String ALLOW_MUTATING_SQL_VIA_GET = "AllowMutatingSqlViaGet";
 
     @Override
     protected void init()
@@ -264,6 +265,14 @@ protected void init()
             false,
             FeatureType.Deprecated
         ));
+        OptionalFeatureService.get().addFeatureFlag(new OptionalFeatureFlag(
+            ALLOW_MUTATING_SQL_VIA_GET,
+            "Allow GET requests to execute mutating SQL",
+            "We strongly recommend leaving this off since it bypasses a critical security check (CSRF). This option will be removed in LabKey Server 26.11",
+            false,
+            false,
+            FeatureType.Deprecated
+        ));
     }
 
     @NotNull

api/src/org/labkey/api/action/SpringActionController.java

@@ -21,11 +21,10 @@
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
 import org.apache.commons.lang3.StringUtils;
-import org.apache.logging.log4j.LogManager;
 import org.apache.logging.log4j.Logger;
+import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
 import org.json.JSONObject;
-import org.jetbrains.annotations.NotNull;
 import org.labkey.api.action.ApiResponseWriter.Format;
 import org.labkey.api.admin.AdminUrls;
 import org.labkey.api.collections.CaseInsensitiveHashMap;
@@ -47,6 +46,7 @@
 import org.labkey.api.security.User;
 import org.labkey.api.security.UserManager;
 import org.labkey.api.settings.AppProps;
+import org.labkey.api.settings.OptionalFeatureService;
 import org.labkey.api.util.ConfigurationException;
 import org.labkey.api.util.ExceptionUtil;
 import org.labkey.api.util.FileUtil;
@@ -55,6 +55,7 @@
 import org.labkey.api.util.PageFlowUtil;
 import org.labkey.api.util.Path;
 import org.labkey.api.util.URLHelper;
+import org.labkey.api.util.logging.LogHelper;
 import org.labkey.api.view.ActionURL;
 import org.labkey.api.view.BadRequestException;
 import org.labkey.api.view.HttpView;
@@ -97,9 +98,11 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArraySet;
+import java.util.function.Supplier;
 
 import static java.lang.Boolean.FALSE;
 import static java.lang.Boolean.TRUE;
+import static org.labkey.api.ApiModule.ALLOW_MUTATING_SQL_VIA_GET;
 import static org.labkey.api.view.template.PageConfig.Template.Dialog;
 
 /**
@@ -128,7 +131,7 @@ public abstract class SpringActionController implements Controller, HasViewConte
 
     private static final Map<Class<? extends Controller>, ActionDescriptor> _classToDescriptor = new HashMap<>();
 
-    private static final Logger _log = LogManager.getLogger(SpringActionController.class);
+    private static final Logger _log = LogHelper.getLogger(SpringActionController.class, "Problems with actions and template");
 
     public void setActionResolver(ActionResolver actionResolver)
     {
@@ -143,7 +146,8 @@ public ActionResolver getActionResolver()
     protected static void registerAction(ActionDescriptor ad)
     {
         ActionDescriptor prev = _classToDescriptor.put(ad.getActionClass(), ad);
-        assert null == prev || prev == ad;
+        if (null != prev)
+            throw new IllegalStateException("Action class '" + prev.getActionClass() + "' has already registered with a controller");
     }
 
     @Nullable
@@ -748,16 +752,17 @@ protected void addNavTrail(Controller action, NavTree root)
         if (action instanceof NavTrailAction)
         {
             ((NavTrailAction)action).addNavTrail(root);
-            assert isValidNavTree(root, action) : action.getClass().getName() + " is generating a malformed NavTree";
+            if (!isValidNavTree(root))
+                throw new IllegalStateException(action.getClass().getName() + " is generating a malformed NavTree");
         }
     }
 
     // NavTrail renders only the first level children, so flag the NavTree as invalid if any child has children. This
     // most likely means that addNavTrail() chained calls to addChild(), which adds nodes that will never render.
-    private boolean isValidNavTree(NavTree tree, Controller action)
+    private boolean isValidNavTree(NavTree tree)
     {
-        return tree.getChildren().stream()   // Very temporary exception for RespondAction. TODO: Remove once 20.7.2 changes are merged to develop.
-            .noneMatch(NavTree::hasChildren) || "org.labkey.announcements.AnnouncementsController$RespondAction".equals(action.getClass().getName());
+        return tree.getChildren().stream()
+            .noneMatch(NavTree::hasChildren);
     }
 
     protected void beforeAction(Controller action) throws ServletException
@@ -1214,10 +1219,7 @@ public static void setActionForThread(Controller c)
 
     public static void setActionForThread(Class<?> c)
     {
-        if (assertsEnabled() && AppProps.getInstance().isDevMode())
-        {
-            currentAction.get().add(c);
-        }
+        currentAction.get().add(c);
     }
 
     public static void clearActionForThread(Controller c)
@@ -1228,72 +1230,69 @@ public static void clearActionForThread(Controller c)
 
     public static void clearActionForThread(Class<?> c)
     {
-        if (assertsEnabled() && AppProps.getInstance().isDevMode())
-        {
-            ArrayList<Class<?>> list = currentAction.get();
-            assert !list.isEmpty();
-            assert list.getLast() == c;
-            list.removeLast();
-        }
-    }
-
-    private static boolean assertsEnabled()
-    {
-        boolean enableasserts = false;
-        //noinspection AssertWithSideEffects
-        assert (enableasserts = true);
-        return enableasserts;
+        ArrayList<Class<?>> list = currentAction.get();
+        Class<?> last = list.removeLast();
+        if (last != c)
+            throw new IllegalStateException("Unexpected last action class: " + last.getName() + " vs. " + c.getName());
     }
 
     @Nullable
     public static Class<?> getActionForThread()
     {
-        if (assertsEnabled() && AppProps.getInstance().isDevMode())
-        {
-            ArrayList<Class<?>> list = currentAction.get();
-            if (!list.isEmpty())
-                return list.getLast();
-        }
+        ArrayList<Class<?>> list = currentAction.get();
+        if (!list.isEmpty())
+            return list.getLast();
         return null;
     }
 
-    public static void executingMutatingSql(String sql)
+    public static void checkForMutatingSql(Supplier<String> mutatingSqlSupplier)
     {
-        if (!assertsEnabled())
-            return;
         if (ignoreUpdates.get())
             return;
-        Class<?> c = getActionForThread();
-        if (null == c)
+        Class<?> actionClass = getActionForThread();
+        if (null == actionClass)
             return;
 
         ViewContext vc = HttpView.currentContext();
         boolean readonly = false;
 
-        if (ReadOnlyApiAction.class.isAssignableFrom(c))
+        if (ReadOnlyApiAction.class.isAssignableFrom(actionClass))
         {
             readonly = true;
         }
-        else if (SimpleRedirectAction.class.isAssignableFrom(c) || SimpleViewAction.class.isAssignableFrom(c))
+        else if (SimpleRedirectAction.class.isAssignableFrom(actionClass) || SimpleViewAction.class.isAssignableFrom(actionClass))
         {
             readonly = true;
         }
-        else if (null != vc && "GET".equals(vc.getRequest().getMethod()))
+        else
         {
-            readonly = true;
-            _log.warn("Action {} accepted GET unexpectedly... might need to update executingMutatingSql()", c.getName());
+            if (null != vc && "GET".equals(vc.getRequest().getMethod()))
+            {
+                readonly = true;
+                if (!FormViewAction.class.isAssignableFrom(actionClass) && AppProps.getInstance().isDevMode())
+                    _log.warn("Action {} accepted GET unexpectedly... might need to update checkForMutatingSql()", actionClass.getName());
+            }
         }
 
         if (readonly)
         {
-            if (c.getName().contains("JunitController"))
+            if (actionClass.getName().contains("JunitController"))
+                return;
+
+            // Checking this late in the game to ensure OptionalFeatureService has been initialized.
+            if (OptionalFeatureService.get().isFeatureEnabled(ALLOW_MUTATING_SQL_VIA_GET))
                 return;
 
-            boolean verbose = _log.isDebugEnabled() || mutatingActionsWarned.add(c.getName());
-            String message = "MUTATING SQL executed as part of handling action: " +
-                    (null == vc ? "" : vc.getRequest().getMethod()) + " " +
-                    c.getName() + (verbose ? ("\n" + sql) : "");
-            throw new IllegalStateException(message);
+            // Note: This defers the mutating SQL parsing & detection until after the quick checks above
+            String mutatingSql = mutatingSqlSupplier.get();
+            if (mutatingSql != null)
+            {
+                boolean verbose = _log.isDebugEnabled() || mutatingActionsWarned.add(actionClass.getName());
+                String message = "MUTATING SQL executed as part of handling action: " +
+                        (null == vc ? "" : vc.getRequest().getMethod()) + " " +
+                        actionClass.getName() + (verbose ? ("\n" + mutatingSql) : "");
+                throw new IllegalStateException(message);
+            }
         }
     }
 

api/src/org/labkey/api/data/PropertyManager.java

@@ -501,7 +501,7 @@ public void save()
             // Flag all property map saves here to catch the unmodified case (those code paths are likely to mutate in
             // other scenarios). Also, we want to flag updates to existing property maps, but that path invokes a stored
             // procedure that StatementWrapper doesn't recognize as mutating SQL.
-            SpringActionController.executingMutatingSql("Saving a PropertyMap");
+            SpringActionController.checkForMutatingSql(() -> "Saving a PropertyMap");
 
             if (!isModified())
             {

api/src/org/labkey/api/data/SqlSelectorTestCase.java

@@ -223,7 +223,7 @@ public void testJdbcUncached() throws SQLException
         }
     }
 
-    // Passing in a Connections and calling setJdbcCaching() should throw
+    // Passing in a Connection and calling setJdbcCaching() should throw
     @Test(expected = IllegalStateException.class)
     public void testJdbcUncachedTrue() throws SQLException
     {
@@ -234,7 +234,7 @@ public void testJdbcUncachedTrue() throws SQLException
         }
     }
 
-    // Passing in a Connections and calling setJdbcCaching() should throw
+    // Passing in a Connection and calling setJdbcCaching() should throw
     @Test(expected = IllegalStateException.class)
     public void testJdbcUncachedFalse() throws SQLException
     {

api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java

@@ -18,6 +18,7 @@
 import org.apache.logging.log4j.Logger;
 import org.labkey.api.collections.CaseInsensitiveHashMap;
 import org.labkey.api.data.SqlScanner;
+import org.labkey.api.settings.AppProps;
 import org.labkey.api.util.logging.LogHelper;
 
 import java.util.Map;
@@ -65,7 +66,7 @@ State getNextState(char c, StringBuilder firstWord, String sql)
                     if (mutatingWord == null && sql.startsWith("? = CALL"))
                         mutatingWord = Boolean.TRUE;
 
-                    if (null == mutatingWord)
+                    if (null == mutatingWord && AppProps.getInstance().isDevMode())
                         LOG.warn("Unrecognized keyword: {} for SQL: {}", word, sql);
 
                     if (Boolean.TRUE == mutatingWord)
@@ -131,7 +132,8 @@ State getNextState(char c, StringBuilder firstWord, String sql)
             "ANALYZE", true,   // Typically executed after UPDATE, CREATE INDEX, et al
             "LOCK", true,      // Not technically mutating. However, it is currently only used in mutating TX.
             "VACUUM", true,    // VACUUM is mutating
-            "{call", true      // Execute a stored procedure, which is likely to be mutating
+            "{call", true,     // Execute a stored procedure, which is likely to be mutating
+            "REFRESH", true    // REFRESH MATERIALIZED VIEW - see JdbcMetaDataTest
         ));
 
         // Needed for SQL Server

api/src/org/labkey/api/data/dialect/StatementWrapper.java

@@ -2827,11 +2827,8 @@ private void afterExecute(Query query, @Nullable SQLException x, int rowsAffecte
     private void _logStatement(String sql, @Nullable SQLException x, int rowsAffected, QueryLogging queryLogging)
     {
         long elapsed = System.currentTimeMillis() - _msStart;
-        boolean isAssertEnabled = false;
-        assert isAssertEnabled = true;
 
-        if (isAssertEnabled && AppProps.getInstance().isDevMode() && isMutatingSql(sql))
-            SpringActionController.executingMutatingSql(sql);
+        SpringActionController.checkForMutatingSql(() -> isMutatingSql(sql) ? sql : null);
 
         // Hold on to this stack trace so that we can reuse it later (if collection has been enabled)
         Query query = QueryProfiler.getInstance().track(_conn.getScope(), sql, translateParametersForQueryTracking(), elapsed, _stackTrace, isRequestThread(), queryLogging);
@@ -2841,6 +2838,9 @@ private void _logStatement(String sql, @Nullable SQLException x, int rowsAffecte
             _conn.logAndCheckException(x);
         }
 
+        boolean isAssertEnabled = false;
+        assert isAssertEnabled = true;
+
         //noinspection ConstantConditions
         if (!_log.isEnabled(Level.DEBUG) && !isAssertEnabled)
             return;

api/src/org/labkey/api/settings/OptionalFeatureService.java

@@ -37,6 +37,11 @@ public interface OptionalFeatureService
         return svc;
     }
 
+    static boolean isAvailable()
+    {
+        return null != ServiceRegistry.get().getService(OptionalFeatureService.class);
+    }
+
     static void setInstance(OptionalFeatureService impl)
     {
         ServiceRegistry.get().registerService(OptionalFeatureService.class, impl);

devtools/src/org/labkey/devtools/TestController.java

@@ -38,6 +38,9 @@
 import org.labkey.api.action.SpringActionController;
 import org.labkey.api.data.Container;
 import org.labkey.api.data.ContainerManager;
+import org.labkey.api.data.DbScope;
+import org.labkey.api.data.SQLFragment;
+import org.labkey.api.data.SqlExecutor;
 import org.labkey.api.mcp.AbstractAgentAction;
 import org.labkey.api.mcp.McpService;
 import org.labkey.api.security.AdminConsoleAction;
@@ -1407,6 +1410,59 @@ public boolean handlePost(Object o, BindException errors)
         }
     }
 
+    @RequiresPermission(UpdatePermission.class)
+    public static class ExecuteMutatingSqlGetAction extends SimpleViewAction<Object>
+    {
+        @Override
+        public ModelAndView getView(Object o, BindException errors)
+        {
+            setTitle("Execute Mutating SQL via GET");
+
+            new SqlExecutor(DbScope.getLabKeyScope()).execute(new SQLFragment("UPDATE core.Containers SET Name = 'xxx' WHERE 1 = 0"));
+
+            return new HtmlView(HtmlString.of("UPDATE via GET was allowed!"));
+        }
+
+        @Override
+        public void addNavTrail(NavTree root)
+        {
+        }
+    }
+
+    @RequiresPermission(UpdatePermission.class)
+    public class ExecuteMutatingSqlPostAction extends FormViewAction<Object>
+    {
+        @Override
+        public void validateCommand(Object target, Errors errors)
+        {
+        }
+
+        @Override
+        public ModelAndView getView(Object o, boolean reshow, BindException errors)
+        {
+            setTitle("Execute Mutating SQL via POST");
+            return new HtmlView(HtmlString.of(reshow ? "UPDATE via POST was allowed!" : "Need to POST to this action"));
+        }
+
+        @Override
+        public boolean handlePost(Object o, BindException errors)
+        {
+            new SqlExecutor(DbScope.getLabKeyScope()).execute(new SQLFragment("UPDATE core.Containers SET Name = 'xxx' WHERE 1 = 0"));
+            return false;
+        }
+
+        @Override
+        public URLHelper getSuccessURL(Object o)
+        {
+            return actionURL(BeginAction.class);
+        }
+
+        @Override
+        public void addNavTrail(NavTree root)
+        {
+        }
+    }
+
     @AdminConsoleAction(AdminOperationsPermission.class)
     @JsonInputLimit(50)
     public static class TestJsonObjectInputLimitAction extends MutatingApiAction<SimpleApiJsonForm>

experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java

@@ -2118,7 +2118,7 @@ public ExpExperiment createHiddenRunGroup(Container container, User user, ExpRun
         if (!exp.isEmpty())
         {
             // We're not actually mutating in this case, but we would be if some action hadn't already cached this run group. Flag it as if we're mutating.
-            SpringActionController.executingMutatingSql("Creating an experiment run group");
+            SpringActionController.checkForMutatingSql(() -> "Creating an experiment run group");
 
             // We don't care which one we use. It's possible to have multiple matches if a run was deleted that was
             // already part of a hidden run group.