diff --git a/api/src/org/labkey/api/security/Directive.java b/api/src/org/labkey/api/security/Directive.java new file mode 100644 index 00000000000..5dd30647415 --- /dev/null +++ b/api/src/org/labkey/api/security/Directive.java @@ -0,0 +1,30 @@ +package org.labkey.api.security; + +/** + * All CSP directives that support substitutions. These constant names are persisted to the database, so be careful with + * any changes. If adding a Directive, make sure to add the corresponding substitutions to application.properties. + */ +public enum Directive +{ + Connection("connect-src"), + Font("font-src"), + Frame("frame-src"), + Style("style-src"); + + private final String _cspDirective; + + Directive(String cspDirective) + { + _cspDirective = cspDirective; + } + + public String getCspDirective() + { + return _cspDirective; + } + + public String getSubstitutionKey() + { + return name().toUpperCase() + ".SOURCES"; + } +} diff --git a/api/src/org/labkey/api/security/SecurityManager.java b/api/src/org/labkey/api/security/SecurityManager.java index 3b6040fcfea..673f36a3a35 100644 --- a/api/src/org/labkey/api/security/SecurityManager.java +++ b/api/src/org/labkey/api/security/SecurityManager.java @@ -233,12 +233,12 @@ public static void registerAllowedConnectionSource(String key, String serviceURL { if (StringUtils.trimToNull(serviceURL) == null) { - ContentSecurityPolicyFilter.unregisterAllowedConnectionSource(key); + ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Connection, key); LOG.trace(String.format("Unregistered [%1$s] as an allowed connection source", key)); return; } - ContentSecurityPolicyFilter.registerAllowedConnectionSource(key, serviceURL); + ContentSecurityPolicyFilter.registerAllowedSources(Directive.Connection, key, serviceURL); LOG.trace(String.format("Registered [%1$s] as an allowed connection source", serviceURL)); } diff --git a/api/src/org/labkey/api/settings/AppPropsImpl.java b/api/src/org/labkey/api/settings/AppPropsImpl.java index db49974162e..8768a2a912e 100644 --- a/api/src/org/labkey/api/settings/AppPropsImpl.java +++ b/api/src/org/labkey/api/settings/AppPropsImpl.java @@ -28,6 +28,7 @@ import org.labkey.api.module.ModuleLoader; import org.labkey.api.module.SupportedDatabase; import org.labkey.api.portal.ProjectUrls; +import org.labkey.api.security.Directive; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; import org.labkey.api.security.UserPrincipal; @@ -595,7 +596,7 @@ public String getStaticFilesPrefix() { var url = new URLHelper(s).setPath(""); if (StringUtils.isNotEmpty(url.getHost())) - ContentSecurityPolicyFilter.registerAllowedConnectionSource("static.files.prefix", url.toString()); + ContentSecurityPolicyFilter.registerAllowedSources(Directive.Connection, "static.files.prefix", url.toString()); prefix = s; } catch (URISyntaxException ignore) diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index e5f9abfb365..69862d8b68e 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -8,8 +8,11 @@ import jakarta.servlet.ServletResponse; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.apache.commons.collections4.SetValuedMap; +import org.apache.commons.collections4.multimap.HashSetValuedHashMap; import org.junit.Assert; import org.junit.Test; +import org.labkey.api.security.Directive; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.OptionalFeatureService; import org.labkey.api.util.CspCommentScanner; @@ -22,31 +25,28 @@ import java.io.IOException; import java.security.SecureRandom; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.Enumeration; -import java.util.List; +import java.util.HashMap; import java.util.Map; -import java.util.concurrent.ConcurrentHashMap; +import java.util.Set; import java.util.stream.Collectors; /** - * For example CSPs, see csp.enforce and csp.report property examples in application.properties - * Do not use those examples for any production environment without understanding the meaning of each directive! + * Content Security Policies (CSPs) are loaded from the csp.enforce and csp.report properties in application.properties. */ - public class ContentSecurityPolicyFilter implements Filter { public static final String FEATURE_FLAG_DISABLE_ENFORCE_CSP = "disableEnforceCsp"; private static final String NONCE_SUBST = "REQUEST.SCRIPT.NONCE"; - private static final String ALLOWED_CONNECT_SUBSTITUTION = "LABKEY.ALLOWED.CONNECTIONS"; private static final String REPORT_PARAMETER_SUBSTITUTION = "CSP.REPORT.PARAMS"; private static final String HEADER_NONCE = "org.labkey.filters.ContentSecurityPolicyFilter#NONCE"; // needs to match PageConfig.HEADER_NONCE - private static final Map> ALLOWED_CONNECTION_SOURCES = new ConcurrentHashMap<>(); + private static final Object ALLOWED_SOURCES_LOCK = new Object(); + private static final Map> ALLOWED_SOURCES = new HashMap<>(); - private static String connectionSrc = ""; + private static Map _substitutionMap = Collections.emptyMap(); // Per-filter-instance parameters that are set in init() and never changed private StringExpression policyExpression = null; @@ -71,9 +71,13 @@ public String getHeaderName() static { - // ReactJS hot reload uses localhost port 3001. If in dev mode, allow browser to access that port. + // ReactJS hot reload uses localhost port 3001. If in dev mode, allow browser to access that port for fonts + // and connections. if (AppProps.getInstance().isDevMode()) - registerAllowedConnectionSource("reactjs.hot.reload", "localhost:3001"); + { + registerAllowedSources(Directive.Connection, "reactjs.hot.reload", "localhost:3001 ws://localhost:3001"); + registerAllowedSources(Directive.Font, "reactjs.hot.reload", "localhost:3001"); + } } @Override @@ -136,10 +140,8 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha { if (type != ContentSecurityPolicyType.Enforce || !OptionalFeatureService.get().isFeatureEnabled(FEATURE_FLAG_DISABLE_ENFORCE_CSP)) { - Map map = Map.of( - NONCE_SUBST, getScriptNonceHeader(req), - ALLOWED_CONNECT_SUBSTITUTION, connectionSrc - ); + Map map = new HashMap<>(_substitutionMap); + map.put(NONCE_SUBST, getScriptNonceHeader(req)); var csp = policyExpression.eval(map); resp.setHeader(type.getHeaderName(), csp); } @@ -147,18 +149,6 @@ NONCE_SUBST, getScriptNonceHeader(req), chain.doFilter(request, response); } - /** - * Return concatenated list of allowed connection hosts - */ - private static String getAllowedConnectionsHeader(Collection> allowedConnectionSources) - { - //Remove substitution parameter if no sources are registered - if (allowedConnectionSources.isEmpty()) - return ""; - - return allowedConnectionSources.stream().flatMap(Collection::stream).distinct().collect(Collectors.joining(" ")); - } - public static String getScriptNonceHeader(HttpServletRequest request) { String nonce = (String)request.getAttribute(HEADER_NONCE); @@ -174,16 +164,53 @@ public static String getScriptNonceHeader(HttpServletRequest request) private static final SecureRandom rand = new SecureRandom(); + @Deprecated // Use registerAllowedSources(Directive.Connection...) public static void registerAllowedConnectionSource(String key, String... allowedUrls) { - ALLOWED_CONNECTION_SOURCES.put(key, Collections.unmodifiableList(Arrays.asList(allowedUrls))); - connectionSrc = getAllowedConnectionsHeader(ALLOWED_CONNECTION_SOURCES.values()); + registerAllowedSources(Directive.Connection, key, allowedUrls); + } + + public static void registerAllowedSources(Directive directive, String key, String... allowedSources) + { + synchronized (ALLOWED_SOURCES_LOCK) + { + SetValuedMap multiMap = ALLOWED_SOURCES.computeIfAbsent(directive, d -> new HashSetValuedHashMap<>()); + Arrays.stream(allowedSources).forEach(s -> multiMap.put(key, s)); + regenerateSubstitutionMap(); + } + } + + public static void unregisterAllowedSources(Directive directive, String key) + { + synchronized (ALLOWED_SOURCES_LOCK) + { + SetValuedMap multiMap = ALLOWED_SOURCES.get(directive); + if (multiMap != null) + { + Set previous = multiMap.remove(key); + // Empty set means no previous mappings were removed, so no need to regenerate the substitution map + if (!previous.isEmpty()) + regenerateSubstitutionMap(); + } + } } - public static void unregisterAllowedConnectionSource(String key) + // Pre-generate the substitution map on every register/unregister + private static void regenerateSubstitutionMap() { - ALLOWED_CONNECTION_SOURCES.remove(key); - connectionSrc = getAllowedConnectionsHeader(ALLOWED_CONNECTION_SOURCES.values()); + _substitutionMap = ALLOWED_SOURCES.entrySet().stream() + .filter(e -> !e.getValue().isEmpty()) + .collect(Collectors.toMap( + e -> e.getKey().getSubstitutionKey(), + e -> e.getValue().values().stream() + .distinct() + .collect(Collectors.joining(" "))) + ); + + // Backward compatibility for CSPs using old substitution key + // TODO: Remove in 25.4 and adjust the junit test below + if (_substitutionMap.containsKey(Directive.Connection.getSubstitutionKey())) + _substitutionMap.put("LABKEY.ALLOWED.CONNECTIONS", _substitutionMap.get(Directive.Connection.getSubstitutionKey())); } public static class TestCase extends Assert @@ -222,5 +249,69 @@ public void testPolicyFiltering() Assert.assertEquals(expected, filterPolicy(fakePolicyForTesting)); } + + @Test + public void testSubstitutionMap() + { + // Make a deep copy of ALLOWED_SOURCES so we can restore it after testing + final Map> savedSources; + final int sourceMapSize; + final int substitutionMapSize; + synchronized (ALLOWED_SOURCES_LOCK) + { + sourceMapSize = ALLOWED_SOURCES.size(); + substitutionMapSize = _substitutionMap.size(); + savedSources = ALLOWED_SOURCES.entrySet().stream() + .collect(Collectors.toMap(Map.Entry::getKey, e -> new HashSetValuedHashMap<>(e.getValue()))); + ALLOWED_SOURCES.clear(); + regenerateSubstitutionMap(); + } + + assertTrue(ALLOWED_SOURCES.isEmpty()); + assertTrue(_substitutionMap.isEmpty()); + unregisterAllowedSources(Directive.Connection, "foo"); + assertTrue(ALLOWED_SOURCES.isEmpty()); + assertTrue(_substitutionMap.isEmpty()); + registerAllowedSources(Directive.Connection, "foo", "MySource"); + assertEquals(1, ALLOWED_SOURCES.size()); + assertEquals(2, _substitutionMap.size()); // Old connection substitution key should be added as well + registerAllowedSources(Directive.Connection, "bar", "MySource"); + assertEquals(1, ALLOWED_SOURCES.size()); + assertEquals(2, _substitutionMap.size()); // Duplicate source should be filtered out + + registerAllowedSources(Directive.Font, "font", "MySource"); + assertEquals(2, ALLOWED_SOURCES.size()); + assertEquals(3, _substitutionMap.size()); + registerAllowedSources(Directive.Font, "font2", "MyOtherSource"); + assertEquals(2, ALLOWED_SOURCES.size()); + assertEquals(3, _substitutionMap.size()); + String value = _substitutionMap.get("FONT.SOURCES"); + assertEquals("! !", value.replace("MyOtherSource", "!").replace("MySource", "!")); + unregisterAllowedSources(Directive.Font, "font2"); + assertEquals(2, ALLOWED_SOURCES.size()); + assertEquals(3, _substitutionMap.size()); + unregisterAllowedSources(Directive.Font, "font"); + assertEquals(2, ALLOWED_SOURCES.size()); // Font entry still exists, but should be empty + assertTrue(ALLOWED_SOURCES.get(Directive.Font).isEmpty()); + assertEquals(2, _substitutionMap.size()); // Back to the way it was + + registerAllowedSources(Directive.Frame, "frame", "FrameSource", "FrameStore"); + assertEquals(3, ALLOWED_SOURCES.size()); + assertEquals(3, _substitutionMap.size()); + + registerAllowedSources(Directive.Style, "style", "StyleSource", "MoreStylishStore"); + assertEquals(4, ALLOWED_SOURCES.size()); + assertEquals(4, _substitutionMap.size()); + + // Restore the previous ALLOWED_SOURCES + synchronized (ALLOWED_SOURCES_LOCK) + { + ALLOWED_SOURCES.clear(); + ALLOWED_SOURCES.putAll(savedSources); + regenerateSubstitutionMap(); + assertEquals(sourceMapSize, ALLOWED_SOURCES.size()); + assertEquals(substitutionMapSize, _substitutionMap.size()); + } + } } } diff --git a/core/src/org/labkey/core/admin/ExternalServerType.java b/core/src/org/labkey/core/admin/ExternalServerType.java index 3a13552d972..d7e4ed5258a 100644 --- a/core/src/org/labkey/core/admin/ExternalServerType.java +++ b/core/src/org/labkey/core/admin/ExternalServerType.java @@ -6,6 +6,7 @@ import org.labkey.api.action.LabKeyError; import org.labkey.api.data.Container; import org.labkey.api.security.User; +import org.labkey.api.security.Directive; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.WriteableAppProps; import org.labkey.api.util.HtmlString; @@ -51,8 +52,8 @@ public void setHosts(Collection hosts, User user) props.save(user); // Refresh the CSP with new values. - ContentSecurityPolicyFilter.unregisterAllowedConnectionSource(EXTERNAL_SOURCE_HOSTS_KEY); - ContentSecurityPolicyFilter.registerAllowedConnectionSource(EXTERNAL_SOURCE_HOSTS_KEY, getHosts().toArray(new String[0])); + ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Connection, EXTERNAL_SOURCE_HOSTS_KEY); + ContentSecurityPolicyFilter.registerAllowedSources(Directive.Connection, EXTERNAL_SOURCE_HOSTS_KEY, getHosts().toArray(new String[0])); } @Override diff --git a/core/src/org/labkey/core/analytics/AnalyticsServiceImpl.java b/core/src/org/labkey/core/analytics/AnalyticsServiceImpl.java index 412a91a09d0..b7b244cae88 100644 --- a/core/src/org/labkey/core/analytics/AnalyticsServiceImpl.java +++ b/core/src/org/labkey/core/analytics/AnalyticsServiceImpl.java @@ -23,6 +23,7 @@ import org.labkey.api.data.PropertyManager; import org.labkey.api.data.PropertyManager.WritablePropertyMap; import org.labkey.api.module.ModuleLoader; +import org.labkey.api.security.Directive; import org.labkey.api.security.User; import org.labkey.api.settings.AbstractWriteableSettingsGroup; import org.labkey.api.settings.AppProps; @@ -115,11 +116,11 @@ public String getDescription() public void resetCSP() { - ContentSecurityPolicyFilter.unregisterAllowedConnectionSource(ANALYTICS_CSP_KEY); + ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Connection, ANALYTICS_CSP_KEY); if (getTrackingStatus().contains(TrackingStatus.ga4FullUrl)) { - ContentSecurityPolicyFilter.registerAllowedConnectionSource(ANALYTICS_CSP_KEY, "https://*.googletagmanager.com", "https://*.google-analytics.com", "https://*.analytics.google.com"); + ContentSecurityPolicyFilter.registerAllowedSources(Directive.Connection, ANALYTICS_CSP_KEY, "https://*.googletagmanager.com", "https://*.google-analytics.com", "https://*.analytics.google.com"); } }