From 6cc71199dadb87fc71153094a5c26cfa22f1b904 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 23 Jun 2026 15:34:04 -0700 Subject: [PATCH 1/5] Disallow mutating SQL in GET requests --- api/src/org/labkey/api/ApiModule.java | 9 +++++++++ .../api/action/SpringActionController.java | 4 +++- .../api/data/dialect/MutatingSqlDetector.java | 3 ++- .../api/data/dialect/StatementWrapper.java | 7 ++++--- .../org/labkey/devtools/TestController.java | 20 +++++++++++++++++++ 5 files changed, 38 insertions(+), 5 deletions(-) diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index def6dcec40c..9c35c2c2a0b 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/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 diff --git a/api/src/org/labkey/api/action/SpringActionController.java b/api/src/org/labkey/api/action/SpringActionController.java index 0ad8524273c..b873b4317bd 100644 --- a/api/src/org/labkey/api/action/SpringActionController.java +++ b/api/src/org/labkey/api/action/SpringActionController.java @@ -47,6 +47,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; @@ -100,6 +101,7 @@ 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; /** @@ -1259,7 +1261,7 @@ public static Class getActionForThread() public static void executingMutatingSql(String sql) { - if (!assertsEnabled()) + if (OptionalFeatureService.get().isFeatureEnabled(ALLOW_MUTATING_SQL_VIA_GET)) return; if (ignoreUpdates.get()) return; diff --git a/api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java b/api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java index 02f743a2a78..641fab60a89 100644 --- a/api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java +++ b/api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java @@ -131,7 +131,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 diff --git a/api/src/org/labkey/api/data/dialect/StatementWrapper.java b/api/src/org/labkey/api/data/dialect/StatementWrapper.java index d764be2c44f..4b483f1056d 100644 --- a/api/src/org/labkey/api/data/dialect/StatementWrapper.java +++ b/api/src/org/labkey/api/data/dialect/StatementWrapper.java @@ -2827,10 +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)) + if (isMutatingSql(sql)) SpringActionController.executingMutatingSql(sql); // Hold on to this stack trace so that we can reuse it later (if collection has been enabled) @@ -2841,6 +2839,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; diff --git a/devtools/src/org/labkey/devtools/TestController.java b/devtools/src/org/labkey/devtools/TestController.java index 1240c01fb0e..bf6dda5c1ff 100644 --- a/devtools/src/org/labkey/devtools/TestController.java +++ b/devtools/src/org/labkey/devtools/TestController.java @@ -37,6 +37,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; @@ -1404,6 +1407,23 @@ public boolean handlePost(Object o, BindException errors) } } + @RequiresPermission(UpdatePermission.class) + public static class ExecuteMutatingSqlAction extends SimpleViewAction + { + @Override + public ModelAndView getView(Object o, BindException errors) + { + 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) + { + } + } + @AdminConsoleAction(AdminOperationsPermission.class) @JsonInputLimit(50) public static class TestJsonObjectInputLimitAction extends MutatingApiAction From 34f8cda9aa6863bb1f9803a1240ae77a16adf54f Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 23 Jun 2026 17:23:37 -0700 Subject: [PATCH 2/5] Claude feedback --- .../api/action/SpringActionController.java | 57 ++++++++----------- .../org/labkey/api/data/PropertyManager.java | 2 +- .../api/data/dialect/MutatingSqlDetector.java | 3 +- .../api/data/dialect/StatementWrapper.java | 3 +- .../api/settings/OptionalFeatureService.java | 5 ++ .../admin/OptionalFeatureServiceImpl.java | 17 +++++- .../experiment/api/ExperimentServiceImpl.java | 2 +- 7 files changed, 50 insertions(+), 39 deletions(-) diff --git a/api/src/org/labkey/api/action/SpringActionController.java b/api/src/org/labkey/api/action/SpringActionController.java index b873b4317bd..c559d30ccaa 100644 --- a/api/src/org/labkey/api/action/SpringActionController.java +++ b/api/src/org/labkey/api/action/SpringActionController.java @@ -98,6 +98,7 @@ 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; @@ -1216,10 +1217,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) @@ -1230,44 +1228,32 @@ public static void clearActionForThread(Controller c) public static void clearActionForThread(Class c) { - if (assertsEnabled() && AppProps.getInstance().isDevMode()) - { - ArrayList> 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> list = currentAction.get(); + assert !list.isEmpty(); + assert list.getLast() == c; + list.removeLast(); } @Nullable public static Class getActionForThread() { - if (assertsEnabled() && AppProps.getInstance().isDevMode()) - { - ArrayList> list = currentAction.get(); - if (!list.isEmpty()) - return list.getLast(); - } + ArrayList> list = currentAction.get(); + if (!list.isEmpty()) + return list.getLast(); return null; } - public static void executingMutatingSql(String sql) + public static void checkForMutatingSql(Supplier mutatingSqlSupplier) { - if (OptionalFeatureService.get().isFeatureEnabled(ALLOW_MUTATING_SQL_VIA_GET)) - return; if (ignoreUpdates.get()) return; Class c = getActionForThread(); if (null == c) return; + // Order is important: this method is called very early during startup. The getActionForThread() check above + // ensures the service is ready. + if (OptionalFeatureService.get().isFeatureEnabled(ALLOW_MUTATING_SQL_VIA_GET)) + return; ViewContext vc = HttpView.currentContext(); boolean readonly = false; @@ -1291,11 +1277,16 @@ else if (null != vc && "GET".equals(vc.getRequest().getMethod())) if (c.getName().contains("JunitController")) 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(c.getName()); + String message = "MUTATING SQL executed as part of handling action: " + + (null == vc ? "" : vc.getRequest().getMethod()) + " " + + c.getName() + (verbose ? ("\n" + mutatingSql) : ""); + throw new IllegalStateException(message); + } } } diff --git a/api/src/org/labkey/api/data/PropertyManager.java b/api/src/org/labkey/api/data/PropertyManager.java index f025dfbe8a0..669be93fa45 100644 --- a/api/src/org/labkey/api/data/PropertyManager.java +++ b/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()) { diff --git a/api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java b/api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java index 641fab60a89..0e034d31573 100644 --- a/api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java +++ b/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) diff --git a/api/src/org/labkey/api/data/dialect/StatementWrapper.java b/api/src/org/labkey/api/data/dialect/StatementWrapper.java index 4b483f1056d..3c092bcab87 100644 --- a/api/src/org/labkey/api/data/dialect/StatementWrapper.java +++ b/api/src/org/labkey/api/data/dialect/StatementWrapper.java @@ -2828,8 +2828,7 @@ private void _logStatement(String sql, @Nullable SQLException x, int rowsAffecte { long elapsed = System.currentTimeMillis() - _msStart; - if (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); diff --git a/api/src/org/labkey/api/settings/OptionalFeatureService.java b/api/src/org/labkey/api/settings/OptionalFeatureService.java index 7fcfeccb88d..5444d834240 100644 --- a/api/src/org/labkey/api/settings/OptionalFeatureService.java +++ b/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); diff --git a/core/src/org/labkey/core/admin/OptionalFeatureServiceImpl.java b/core/src/org/labkey/core/admin/OptionalFeatureServiceImpl.java index 17d1b93ca03..540c088dc99 100644 --- a/core/src/org/labkey/core/admin/OptionalFeatureServiceImpl.java +++ b/core/src/org/labkey/core/admin/OptionalFeatureServiceImpl.java @@ -15,6 +15,7 @@ */ package org.labkey.core.admin; +import org.labkey.api.action.SpringActionController; import org.labkey.api.security.User; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.OptionalFeatureFlag; @@ -30,6 +31,8 @@ import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.CopyOnWriteArrayList; +import static org.labkey.api.ApiModule.ALLOW_MUTATING_SQL_VIA_GET; + public class OptionalFeatureServiceImpl implements OptionalFeatureService { private final Set _optionalFlags = new ConcurrentSkipListSet<>(); @@ -88,7 +91,19 @@ public void setFeatureEnabled(String feature, boolean enabled, User user) { WriteableAppProps props = AppProps.getWriteableInstance(); setFeatureEnabled(feature, enabled, props); - props.save(user); + + // Hack to avoid "Caller is already loading this object!" exception from the cache due to reentrancy + if (ALLOW_MUTATING_SQL_VIA_GET.equals(feature)) + { + try (var ignore = SpringActionController.ignoreSqlUpdates()) + { + props.save(user); + } + } + else + { + props.save(user); + } } private void setFeatureEnabled(String feature, boolean enabled, WriteableAppProps props) diff --git a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java b/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java index e62e713182c..eab9554b7c6 100644 --- a/experiment/src/org/labkey/experiment/api/ExperimentServiceImpl.java +++ b/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. From 65094661b11de0be843afd706df53358f5259239 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 23 Jun 2026 18:37:23 -0700 Subject: [PATCH 3/5] More Claude feedback --- .../api/action/SpringActionController.java | 10 +++++----- .../core/admin/OptionalFeatureServiceImpl.java | 17 +---------------- 2 files changed, 6 insertions(+), 21 deletions(-) diff --git a/api/src/org/labkey/api/action/SpringActionController.java b/api/src/org/labkey/api/action/SpringActionController.java index c559d30ccaa..e9fac12dcef 100644 --- a/api/src/org/labkey/api/action/SpringActionController.java +++ b/api/src/org/labkey/api/action/SpringActionController.java @@ -1250,10 +1250,6 @@ public static void checkForMutatingSql(Supplier mutatingSqlSupplier) Class c = getActionForThread(); if (null == c) return; - // Order is important: this method is called very early during startup. The getActionForThread() check above - // ensures the service is ready. - if (OptionalFeatureService.get().isFeatureEnabled(ALLOW_MUTATING_SQL_VIA_GET)) - return; ViewContext vc = HttpView.currentContext(); boolean readonly = false; @@ -1269,7 +1265,7 @@ else if (SimpleRedirectAction.class.isAssignableFrom(c) || SimpleViewAction.clas else if (null != vc && "GET".equals(vc.getRequest().getMethod())) { readonly = true; - _log.warn("Action {} accepted GET unexpectedly... might need to update executingMutatingSql()", c.getName()); + _log.warn("Action {} accepted GET unexpectedly... might need to update checkForMutatingSql()", c.getName()); } if (readonly) @@ -1277,6 +1273,10 @@ else if (null != vc && "GET".equals(vc.getRequest().getMethod())) if (c.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; + // Note: This defers the mutating SQL parsing & detection until after the quick checks above String mutatingSql = mutatingSqlSupplier.get(); if (mutatingSql != null) diff --git a/core/src/org/labkey/core/admin/OptionalFeatureServiceImpl.java b/core/src/org/labkey/core/admin/OptionalFeatureServiceImpl.java index 540c088dc99..17d1b93ca03 100644 --- a/core/src/org/labkey/core/admin/OptionalFeatureServiceImpl.java +++ b/core/src/org/labkey/core/admin/OptionalFeatureServiceImpl.java @@ -15,7 +15,6 @@ */ package org.labkey.core.admin; -import org.labkey.api.action.SpringActionController; import org.labkey.api.security.User; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.OptionalFeatureFlag; @@ -31,8 +30,6 @@ import java.util.concurrent.ConcurrentSkipListSet; import java.util.concurrent.CopyOnWriteArrayList; -import static org.labkey.api.ApiModule.ALLOW_MUTATING_SQL_VIA_GET; - public class OptionalFeatureServiceImpl implements OptionalFeatureService { private final Set _optionalFlags = new ConcurrentSkipListSet<>(); @@ -91,19 +88,7 @@ public void setFeatureEnabled(String feature, boolean enabled, User user) { WriteableAppProps props = AppProps.getWriteableInstance(); setFeatureEnabled(feature, enabled, props); - - // Hack to avoid "Caller is already loading this object!" exception from the cache due to reentrancy - if (ALLOW_MUTATING_SQL_VIA_GET.equals(feature)) - { - try (var ignore = SpringActionController.ignoreSqlUpdates()) - { - props.save(user); - } - } - else - { - props.save(user); - } + props.save(user); } private void setFeatureEnabled(String feature, boolean enabled, WriteableAppProps props) From 14264d14b15edf970bdfb799cfb0b4983d578709 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 24 Jun 2026 13:47:19 -0700 Subject: [PATCH 4/5] Deassertification --- .../api/action/SpringActionController.java | 48 +++++++++++-------- .../labkey/api/data/SqlSelectorTestCase.java | 4 +- 2 files changed, 29 insertions(+), 23 deletions(-) diff --git a/api/src/org/labkey/api/action/SpringActionController.java b/api/src/org/labkey/api/action/SpringActionController.java index e9fac12dcef..793152567e1 100644 --- a/api/src/org/labkey/api/action/SpringActionController.java +++ b/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; @@ -56,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; @@ -131,7 +131,7 @@ public abstract class SpringActionController implements Controller, HasViewConte private static final Map, 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) { @@ -146,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 @@ -751,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 @@ -1229,9 +1231,9 @@ public static void clearActionForThread(Controller c) public static void clearActionForThread(Class c) { ArrayList> list = currentAction.get(); - assert !list.isEmpty(); - assert list.getLast() == c; - list.removeLast(); + Class last = list.removeLast(); + if (last != c) + throw new IllegalStateException("Unexpected last action class: " + last.getName() + " vs. " + c.getName()); } @Nullable @@ -1247,30 +1249,34 @@ public static void checkForMutatingSql(Supplier mutatingSqlSupplier) { 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 checkForMutatingSql()", 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. @@ -1281,10 +1287,10 @@ else if (null != vc && "GET".equals(vc.getRequest().getMethod())) String mutatingSql = mutatingSqlSupplier.get(); if (mutatingSql != null) { - boolean verbose = _log.isDebugEnabled() || mutatingActionsWarned.add(c.getName()); + boolean verbose = _log.isDebugEnabled() || mutatingActionsWarned.add(actionClass.getName()); String message = "MUTATING SQL executed as part of handling action: " + (null == vc ? "" : vc.getRequest().getMethod()) + " " + - c.getName() + (verbose ? ("\n" + mutatingSql) : ""); + actionClass.getName() + (verbose ? ("\n" + mutatingSql) : ""); throw new IllegalStateException(message); } } diff --git a/api/src/org/labkey/api/data/SqlSelectorTestCase.java b/api/src/org/labkey/api/data/SqlSelectorTestCase.java index 24245672849..e77de43d433 100644 --- a/api/src/org/labkey/api/data/SqlSelectorTestCase.java +++ b/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 { From 3c94dd76163bfd5785c6b2bd76661e6aa1016b10 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 24 Jun 2026 15:21:04 -0700 Subject: [PATCH 5/5] Test POST as well --- .../org/labkey/devtools/TestController.java | 38 ++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/devtools/src/org/labkey/devtools/TestController.java b/devtools/src/org/labkey/devtools/TestController.java index 8a49f45c280..5de66de2919 100644 --- a/devtools/src/org/labkey/devtools/TestController.java +++ b/devtools/src/org/labkey/devtools/TestController.java @@ -1411,11 +1411,13 @@ public boolean handlePost(Object o, BindException errors) } @RequiresPermission(UpdatePermission.class) - public static class ExecuteMutatingSqlAction extends SimpleViewAction + public static class ExecuteMutatingSqlGetAction extends SimpleViewAction { @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!")); @@ -1427,6 +1429,40 @@ public void addNavTrail(NavTree root) } } + @RequiresPermission(UpdatePermission.class) + public class ExecuteMutatingSqlPostAction extends FormViewAction + { + @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