diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index ff196ee1ef1..83170e3529d 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..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; @@ -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, 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> 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(); + 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> 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 (!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); + } } } 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/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 { diff --git a/api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java b/api/src/org/labkey/api/data/dialect/MutatingSqlDetector.java index 02f743a2a78..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) @@ -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 diff --git a/api/src/org/labkey/api/data/dialect/StatementWrapper.java b/api/src/org/labkey/api/data/dialect/StatementWrapper.java index d764be2c44f..3c092bcab87 100644 --- a/api/src/org/labkey/api/data/dialect/StatementWrapper.java +++ b/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; 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/devtools/src/org/labkey/devtools/TestController.java b/devtools/src/org/labkey/devtools/TestController.java index 6c49f6572c5..5de66de2919 100644 --- a/devtools/src/org/labkey/devtools/TestController.java +++ b/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 + { + @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 + { + @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 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.