Register filters & servlets later + CSP improvements

Author: labkey-adam

api/src/org/labkey/api/mcp/McpService.java

@@ -29,11 +29,11 @@
 /// ### MCP Development Guide
 /// `McpService` lets you expose functionality over the MCP protocol (only simple http for now). This allows external
 /// chat sessions to pull information from LabKey Server. Exposed functionality is also made available to chat sessions
-/// hosted by LabKey (see `AbstractAgentAction``).
+/// hosted by LabKey (see `AbstractAgentAction`).
 ///
 /// ### Adding a new MCP class
 /// 1. Create a new class that implements `McpImpl` (see below) in the appropriate module
-/// 2. Register that class in your module `init()` method: `McpService.get().register(new MyMcp())`
+/// 2. Register that class in your module's `startup()` method: `McpService.get().register(new MyMcp())`
 /// 3. Add tools and resources
 ///
 /// ### Adding a new MCP tool
@@ -44,13 +44,13 @@
 ///     permission annotation is required, otherwise your tool will not be registered.**
 /// 4.  Add `ToolContext` as the first parameter to the method
 /// 5.  Add additional required or optional parameters to the method signature, as needed. Note that "required" is the
-///     default. Again here, the parameter descriptions are very important. Provide examples.
+///     default. Again here, the parameter descriptions are very important. Provide examples of parameter values.
 /// 6.  Use the helper method `getContext(ToolContext)` to retrieve the current `Container` and `User`
 /// 7.  Use the helper method `getUser(ToolContext)` in the rare cases where you need just a `User`
 /// 8.  Perform additional permissions checking (beyond what the annotations offer), where appropriate
 /// 9.  Filter all results to the current container, of course
 /// 10. For any error conditions, throw exceptions with detailed information. These will get translated into appropriate
-///     failure responses and the LLM client will attempt to correct the problem.
+///     failure responses and the LLM client will attempt to correct any problems (hopefully).
 /// 11. For success cases, return a String with a message or JSON content, for example, `JSONObject.toString()`. Spring
 ///     has some limited ability to convert other objects into JSON strings, but we haven't experimented with that. See
 ///     `DefaultToolCallResultConverter` and the ability to provide a custom result converter via the `@Tool` annotation.
@@ -126,6 +126,7 @@ static void setInstance(McpService service)
 
     boolean isReady();
 
+    // Register MCPs in Module.startup()
     default void register(McpImpl mcp)
     {
         try

api/src/org/labkey/api/module/ModuleLoader.java

@@ -580,19 +580,6 @@ private void doInit(Execution execution) throws ServletException
             throw new IllegalStateException("Core module was not first or could not find the Core module. Ensure that Tomcat user can create directories under the <LABKEY_HOME>/modules directory.");
         setProjectRoot(coreModule);
 
-        for (Module module : modules)
-        {
-            module.registerFilters(_servletContext);
-        }
-        for (Module module : modules)
-        {
-            module.registerServlets(_servletContext);
-        }
-        for (Module module : modules)
-        {
-            module.registerFinalServlets(_servletContext);
-        }
-
         // Do this after we've checked to see if we can find the core module. See issue 22797.
         verifyProductionModeMatchesBuild();
 
@@ -790,12 +777,34 @@ public void addStaticWarnings(@NotNull Warnings warnings, boolean showAllWarning
         if (!modulesRequiringUpgrade.isEmpty() || !additionalSchemasRequiringUpgrade.isEmpty())
             setUpgradeState(UpgradeState.UpgradeRequired);
 
-        // Don't accept any requests if we're bootstrapping empty schemas or migrating from SQL Server
+        // Don't accept any requests if we're bootstrapping empty schemas or doing a database migration
         if (!shouldInsertData())
             execution = Execution.Synchronous;
 
         startNonCoreUpgradeAndStartup(execution, lockFile);
 
+        // Register filters and servlets at the last minute, just before Tomcat starts. At this point, the list of
+        // modules is final. Also, we've had one case where the CSP filter was getting called before modules were
+        // initialized, GitHub Issue 1008. We have no idea how that happened, but registering late won't hurt.
+
+        _log.info("Registering filters");
+
+        for (Module module : _modules)
+        {
+            module.registerFilters(_servletContext);
+        }
+
+        _log.info("Registering servlets");
+
+        for (Module module : _modules)
+        {
+            module.registerServlets(_servletContext);
+        }
+        for (Module module : _modules)
+        {
+            module.registerFinalServlets(_servletContext);
+        }
+
         _log.info("LabKey Server startup is complete; {}", execution.getLogMessage());
     }
 

api/src/org/labkey/filters/ContentSecurityPolicyFilter.java

@@ -12,7 +12,6 @@
 import org.apache.commons.collections4.multimap.HashSetValuedHashMap;
 import org.apache.commons.lang3.EnumUtils;
 import org.apache.commons.lang3.StringUtils;
-import org.apache.commons.lang3.Strings;
 import org.apache.logging.log4j.Logger;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -30,7 +29,6 @@
 import org.labkey.api.util.StringExpressionFactory;
 import org.labkey.api.util.StringExpressionFactory.AbstractStringExpression.NullValueBehavior;
 import org.labkey.api.util.logging.LogHelper;
-import org.labkey.api.view.ActionURL;
 
 import java.io.IOException;
 import java.security.SecureRandom;
@@ -40,13 +38,11 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Set;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
 import java.util.stream.Collectors;
 
-
 /**
  * Content Security Policies (CSPs) are loaded from the csp.enforce and csp.report properties in application.properties.
  */
@@ -57,6 +53,9 @@ public class ContentSecurityPolicyFilter implements Filter
     private static final String NONCE_SUBST = "REQUEST.SCRIPT.NONCE";
     private static final String UPGRADE_INSECURE_REQUESTS_SUBSTITUTION = "UPGRADE.INSECURE.REQUESTS";
     private static final String HEADER_NONCE = "org.labkey.filters.ContentSecurityPolicyFilter#NONCE";  // needs to match PageConfig.HEADER_NONCE
+    private static final String REPORTING_ENDPOINTS_HEADER = "Reporting-Endpoints";
+    @SuppressWarnings("DataFlowIssue")
+    private static final String REPORTING_ENDPOINTS_HEADER_VALUE = "csp-report=\"" + PageFlowUtil.urlProvider(AdminUrls.class).getCspReportToURL().getLocalURIString() + "\"";;
 
     private static final Map<ContentSecurityPolicyType, ContentSecurityPolicyFilter> CSP_FILTERS = new CopyOnWriteHashMap<>();
 
@@ -76,11 +75,10 @@ public class ContentSecurityPolicyFilter implements Filter
     private @NotNull String _cspVersion = "Unknown";
     // These two are effectively @NotNull since they are set to non-null values in init() and never changed
     private String _stashedTemplate = null;
-    private String _reportToEndpointName = null;
 
-    // Per-filter-instance settings are initialized on first request and reset when base server URL or allowed sources
-    // change. Don't reference this directly; always use ensureSettings().
-    private volatile @Nullable CspFilterSettings _settings = null;
+    // Initialized on first request and reset when allowed sources change. Don't reference this directly; always use
+    // ensurePolicyExpression().
+    private volatile StringExpression _policyExpression;
 
     public enum ContentSecurityPolicyType
     {
@@ -118,7 +116,7 @@ public String getHeaderName()
     @Override
     public void init(FilterConfig filterConfig) throws ServletException
     {
-        LogHelper.getLogger(ContentSecurityPolicyFilter.class, "CSP filter initialization").info("Initializing {}", filterConfig.getFilterName());
+        LOG.info("Initializing {}", filterConfig.getFilterName());
         Enumeration<String> paramNames = filterConfig.getInitParameterNames();
         while (paramNames.hasMoreElements())
         {
@@ -146,9 +144,6 @@ else if ("disposition".equalsIgnoreCase(paramName))
 
         if (CSP_FILTERS.put(getType(), this) != null)
             throw new ServletException("ContentSecurityPolicyFilter is misconfigured, duplicate policies of type: " + getType());
-
-        // configure a different endpoint for each type. TODO: We only need one CSP violation reporting endpoint now, so one header would do
-        _reportToEndpointName = "csp-" + getType().name().toLowerCase();
     }
 
     /** Filter out block comments and replace special characters in the provided policy */
@@ -197,18 +192,21 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
     {
         if (request instanceof HttpServletRequest req && response instanceof HttpServletResponse resp)
         {
-            CspFilterSettings settings = ensureSettings();
+            StringExpression expression = ensurePolicyExpression();
 
             if (getType() != ContentSecurityPolicyType.Enforce || !OptionalFeatureService.get().isFeatureEnabled(FEATURE_FLAG_DISABLE_ENFORCE_CSP))
             {
                 Map<String, String> map = Map.of(NONCE_SUBST, getScriptNonceHeader(req));
-                var csp = settings.getPolicyExpression().eval(map);
-                resp.setHeader(getType().getHeaderName(), csp);
+                String csp = expression.eval(map);
+
+                if ("https".equals(req.getScheme()))
+                {
+                    if (resp.getHeader(REPORTING_ENDPOINTS_HEADER) == null)
+                        resp.addHeader(REPORTING_ENDPOINTS_HEADER, REPORTING_ENDPOINTS_HEADER_VALUE);
+                    csp = csp + " report-to csp-report ;";
+                }
 
-                // null if https: is not configured on this server
-                String reportingEndpointsHeaderValue = settings.getReportingEndpointsHeaderValue();
-                if (reportingEndpointsHeaderValue != null)
-                    resp.addHeader("Reporting-Endpoints", reportingEndpointsHeaderValue);
+                resp.setHeader(getType().getHeaderName(), csp);
             }
         }
         chain.doFilter(request, response);
@@ -229,91 +227,26 @@ public String getStashedTemplate()
         return _stashedTemplate;
     }
 
-    public String getReportToEndpointName()
-    {
-        return _reportToEndpointName;
-    }
-
-    private void clearSettings()
+    private void clearPolicyExpression()
     {
-        _settings = null;
+        _policyExpression = null;
     }
 
-    private @NotNull CspFilterSettings ensureSettings()
+    private @NotNull StringExpression ensurePolicyExpression()
     {
-        String baseServerUrl = AppProps.getInstance().getBaseServerUrl();
-        CspFilterSettings settings = _settings; // Stash a local copy to ensure consistency in the checks below
+        StringExpression expression = _policyExpression;
 
-        // Reset settings if null or if base server URL has changed
-        if (null == settings || !Objects.equals(baseServerUrl, settings.getPreviousBaseServerUrl()))
+        if (expression == null)
         {
-            settings = _settings = new CspFilterSettings(this, baseServerUrl);
-        }
-
-        return settings;
-    }
-
-    // Hold all the mutable per-filter settings in a single object so they can be set atomically
-    private static class CspFilterSettings
-    {
-        private final String _policyTemplate;
-        private final String _reportingEndpointsHeaderValue;
-        private final String _previousBaseServerUrl;
-        private final StringExpression _policyExpression;
-
-        private CspFilterSettings(ContentSecurityPolicyFilter filter, String baseServerUrl)
-        {
-            // Add "Reporting-Endpoints" header and "report-to" directive only if https: is configured on this
-            // server. This ensures that browsers fall-back on report-uri if https: isn't configured.
-            if (Strings.CI.startsWith(baseServerUrl, "https://"))
-            {
-                // Each filter adds its own "Reporting-Endpoints" header since we want to convey the correct version (eXX vs. rXX)
-                @SuppressWarnings("DataFlowIssue")
-                ActionURL violationUrl = PageFlowUtil.urlProvider(AdminUrls.class).getCspReportToURL();
-                // Use an absolute URL so we always post to https:, even if the violating request uses http:
-                _reportingEndpointsHeaderValue = filter.getReportToEndpointName() + "=\"" + violationUrl.getURIString() + "\"";
-
-                // Add "report-to" directive to the policy
-                _policyTemplate = filter.getStashedTemplate() + " report-to " + filter.getReportToEndpointName() + " ;";
-            }
-            else
-            {
-                _policyTemplate = filter.getStashedTemplate();
-                _reportingEndpointsHeaderValue = null;
-            }
-
-            _previousBaseServerUrl = baseServerUrl;
-
-            final String substitutedPolicy;
-
             synchronized (SUBSTITUTION_LOCK)
             {
-                substitutedPolicy = StringExpressionFactory.create(_policyTemplate, false, NullValueBehavior.KeepSubstitution)
+                var substitutedPolicy = StringExpressionFactory.create(getStashedTemplate(), false, NullValueBehavior.KeepSubstitution)
                     .eval(SUBSTITUTION_MAP);
+                expression = _policyExpression = StringExpressionFactory.create(substitutedPolicy, false, NullValueBehavior.ReplaceNullAndMissingWithBlank);
             }
-
-            _policyExpression = StringExpressionFactory.create(substitutedPolicy, false, NullValueBehavior.ReplaceNullAndMissingWithBlank);
-        }
-
-        public String getPolicyTemplate()
-        {
-            return _policyTemplate;
         }
 
-        public String getReportingEndpointsHeaderValue()
-        {
-            return _reportingEndpointsHeaderValue;
-        }
-
-        public String getPreviousBaseServerUrl()
-        {
-            return _previousBaseServerUrl;
-        }
-
-        public StringExpression getPolicyExpression()
-        {
-            return _policyExpression;
-        }
+        return expression;
     }
 
     public static String getScriptNonceHeader(HttpServletRequest request)
@@ -362,7 +295,7 @@ public static void unregisterAllowedSources(String key, Directive directive)
 
     /**
      * Regenerate the substitution map on every register/unregister. The policy expression will be regenerated on the
-     * next request (see {@link #ensureSettings()}).
+     * next request (see {@link #ensurePolicyExpression()}).
      */
     public static void regenerateSubstitutionMap()
     {
@@ -389,7 +322,7 @@ public static void regenerateSubstitutionMap()
 
             // Tell each registered ContentSecurityPolicyFilter to clear its settings so the next request recreates them
             // using the new substitution map
-            CSP_FILTERS.values().forEach(ContentSecurityPolicyFilter::clearSettings);
+            CSP_FILTERS.values().forEach(ContentSecurityPolicyFilter::clearPolicyExpression);
         }
     }
 
@@ -436,7 +369,7 @@ public static List<String> getMissingSubstitutions(ContentSecurityPolicyType typ
         }
         else
         {
-            String template = filter.ensureSettings().getPolicyTemplate();
+            String template = filter.getStashedTemplate();
             ret = Arrays.stream(Directive.values())
                 .map(dir -> "${" + dir.getSubstitutionKey() + "}")
                 .filter(key -> !template.contains(key))
@@ -451,13 +384,15 @@ public static void registerMetricsProvider()
         UsageMetricsService.get().registerUsageMetrics("API", () -> Map.of("cspFilters", CSP_FILTERS.values().stream()
             .collect(Collectors.toMap(ContentSecurityPolicyFilter::getType,
                 filter -> {
-                    CspFilterSettings settings = filter.ensureSettings();
+                    StringExpression expression = filter.ensurePolicyExpression();
                     return Map.of(
                         "version", filter.getCspVersion(),
-                        "csp", settings.getPolicyTemplate(),
-                        "cspSubstituted", settings.getPolicyExpression().getSource()
+                        "csp", filter.getStashedTemplate(),
+                        "cspSubstituted", expression.getSource()
                     );
-                }))));
+                }))
+            )
+        );
     }
 
     public static class TestCase extends Assert
@@ -630,7 +565,7 @@ public void testSubstitutionMap()
         private void verifySubstitutionInPolicyExpressions(String value, int expectedCount)
         {
             List<String> failures = CSP_FILTERS.values().stream()
-                .map(filter -> filter.ensureSettings().getPolicyExpression().eval(Map.of()))
+                .map(filter -> filter.ensurePolicyExpression().eval(Map.of()))
                 .filter(policy -> StringUtils.countMatches(policy, value) != expectedCount)
                 .toList();
 

core/src/org/labkey/core/CoreModule.java

@@ -559,7 +559,6 @@ public QuerySchema createSchema(DefaultSchema schema, Module module)
 
         ScriptEngineManagerImpl.registerEncryptionMigrationHandler();
 
-        McpService.get().register(new CoreMcp());
         PostgreSqlService.setInstance(PostgreSqlDialectFactory::getLatestSupportedDialect);
 
         deleteTempFiles();
@@ -1283,6 +1282,8 @@ public void moduleStartupComplete(ServletContext servletContext)
         UserManager.addUserListener(new EmailPreferenceUserListener());
 
         Encryption.checkMigration();
+
+        McpService.get().register(new CoreMcp());
     }
 
     // Issue 7527: Auto-detect missing SQL views and attempt to recreate