diff --git a/api/src/org/labkey/api/mcp/McpService.java b/api/src/org/labkey/api/mcp/McpService.java index 63bfcaabfa6..6ad04872537 100644 --- a/api/src/org/labkey/api/mcp/McpService.java +++ b/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 diff --git a/api/src/org/labkey/api/module/ModuleLoader.java b/api/src/org/labkey/api/module/ModuleLoader.java index a43af596db6..6248d1b0e75 100644 --- a/api/src/org/labkey/api/module/ModuleLoader.java +++ b/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 /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. We've had one case where the CSP filter was getting initialized before the core module + // was 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()); } diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index 90a9b1c5437..dc077525b6b 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/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,7 @@ 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"; private static final Map CSP_FILTERS = new CopyOnWriteHashMap<>(); @@ -76,11 +73,14 @@ 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; + // We can't set this statically because the class is referenced before URLProviders are available + @SuppressWarnings("DataFlowIssue") + private final String _reportingEndpointsHeaderValue = "csp-report=\"" + PageFlowUtil.urlProvider(AdminUrls.class).getCspReportToURL().getLocalURIString() + "\""; + + // 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 +118,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 paramNames = filterConfig.getInitParameterNames(); while (paramNames.hasMoreElements()) { @@ -146,9 +146,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 +194,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 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, _reportingEndpointsHeaderValue); + 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 +229,26 @@ public String getStashedTemplate() return _stashedTemplate; } - public String getReportToEndpointName() + private void clearPolicyExpression() { - return _reportToEndpointName; + _policyExpression = null; } - private void clearSettings() + private @NotNull StringExpression ensurePolicyExpression() { - _settings = null; - } + StringExpression expression = _policyExpression; - private @NotNull CspFilterSettings ensureSettings() - { - String baseServerUrl = AppProps.getInstance().getBaseServerUrl(); - CspFilterSettings settings = _settings; // Stash a local copy to ensure consistency in the checks below - - // 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 +297,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 +324,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 +371,7 @@ public static List 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 +386,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 +567,7 @@ public void testSubstitutionMap() private void verifySubstitutionInPolicyExpressions(String value, int expectedCount) { List 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(); diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 2312ec48f5f..fd2fd568258 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/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 diff --git a/query/src/org/labkey/query/QueryModule.java b/query/src/org/labkey/query/QueryModule.java index 0630786a1a2..15301c414c9 100644 --- a/query/src/org/labkey/query/QueryModule.java +++ b/query/src/org/labkey/query/QueryModule.java @@ -247,8 +247,6 @@ public QuerySchema createSchema(DefaultSchema schema, Module module) "Allow for lookup fields in product folders to query across all folders within the top-level folder.", false); OptionalFeatureService.get().addExperimentalFeatureFlag(QueryServiceImpl.EXPERIMENTAL_PRODUCT_PROJECT_DATA_LISTING_SCOPED, "Product folders display folder-specific data", "Only list folder-specific data within product folders.", false); - - McpService.get().register(new QueryMcp()); } @@ -350,11 +348,14 @@ public void doStartup(ModuleContext moduleContext) trustedAnalystRole.addPermission(EditQueriesPermission.class); OptionalFeatureService.get().addFeatureFlag(new OptionalFeatureFlag(R_REPORT_CUSTOM_SHARING, - "Restore custom R report sharing", - "Allows R reports to be shared on a per user basis. This option will be removed in LabKey Server 26.7.", - false, - false, - OptionalFeatureService.FeatureType.Deprecated)); + "Restore custom R report sharing", + "Allows R reports to be shared on a per user basis. This option will be removed in LabKey Server 26.7.", + false, + false, + OptionalFeatureService.FeatureType.Deprecated) + ); + + McpService.get().register(new QueryMcp()); } @Override @@ -390,13 +391,13 @@ public Set getSchemaNames() return Set.of( ModuleReportCache.TestCase.class, OlapController.TestCase.class, - QueryController.TestCase.class, QueryController.SaveRowsTestCase.class, + QueryController.TestCase.class, QueryServiceImpl.TestCase.class, RolapReader.RolapTest.class, RolapTestCase.class, - ServerManager.TestCase.class, - SelectRowsStreamHack.TestCase.class + SelectRowsStreamHack.TestCase.class, + ServerManager.TestCase.class ); }