From 64047d2f41d552aa87820424b425facec59332b1 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 19 Feb 2025 10:54:44 -0800 Subject: [PATCH 1/3] Support adding module-specific resource hosts to more CSP directives --- .../org/labkey/api/security/Directive.java | 30 ++++++ .../labkey/api/security/SecurityManager.java | 4 +- .../org/labkey/api/settings/AppPropsImpl.java | 3 +- .../filters/ContentSecurityPolicyFilter.java | 99 +++++++++++++------ .../labkey/core/admin/ExternalServerType.java | 5 +- .../core/analytics/AnalyticsServiceImpl.java | 5 +- 6 files changed, 107 insertions(+), 39 deletions(-) create mode 100644 api/src/org/labkey/api/security/Directive.java 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..b5c50e41d68 --- /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 "LABKEY.ALLOWED." + 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..3ac068ead61 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"); + 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 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 name + // TODO: Remove in 25.4 + 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,13 @@ public void testPolicyFiltering() Assert.assertEquals(expected, filterPolicy(fakePolicyForTesting)); } + +// @Test +// public void testSubstitutions() +// { +// // TODO: Save away existing ALLOWED_SOURCES +// unregisterAllowedSources(Directive.Connection, "foo"); +// registerAllowedSources(Directive.Connection, "foo", "bar"); +// } } } 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"); } } From 8837e88fac87850a5a14fcb140a553dcb68a9cbf Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 19 Feb 2025 13:51:48 -0800 Subject: [PATCH 2/3] Shorten the CSP substitution keys. Add substitutions junit test. --- .../org/labkey/api/security/Directive.java | 2 +- .../filters/ContentSecurityPolicyFilter.java | 76 ++++++++++++++++--- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/api/src/org/labkey/api/security/Directive.java b/api/src/org/labkey/api/security/Directive.java index b5c50e41d68..5dd30647415 100644 --- a/api/src/org/labkey/api/security/Directive.java +++ b/api/src/org/labkey/api/security/Directive.java @@ -25,6 +25,6 @@ public String getCspDirective() public String getSubstitutionKey() { - return "LABKEY.ALLOWED." + name().toUpperCase() + ".SOURCES"; + return name().toUpperCase() + ".SOURCES"; } } diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index 3ac068ead61..44238fb8535 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -164,7 +164,7 @@ public static String getScriptNonceHeader(HttpServletRequest request) private static final SecureRandom rand = new SecureRandom(); - @Deprecated + @Deprecated // Use registerAllowedSources(Directive.Connection...) public static void registerAllowedConnectionSource(String key, String... allowedUrls) { registerAllowedSources(Directive.Connection, key, allowedUrls); @@ -207,8 +207,8 @@ private static void regenerateSubstitutionMap() .collect(Collectors.joining(" "))) ); - // Backward compatibility for CSPs using old name - // TODO: Remove in 25.4 + // 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())); } @@ -250,12 +250,68 @@ public void testPolicyFiltering() Assert.assertEquals(expected, filterPolicy(fakePolicyForTesting)); } -// @Test -// public void testSubstitutions() -// { -// // TODO: Save away existing ALLOWED_SOURCES -// unregisterAllowedSources(Directive.Connection, "foo"); -// registerAllowedSources(Directive.Connection, "foo", "bar"); -// } + @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()); + } + } } } From 7939bec57932af9c5072d99d6444acaf26ff2584 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 19 Feb 2025 14:43:20 -0800 Subject: [PATCH 3/3] Support ws: as well for hot reload --- api/src/org/labkey/filters/ContentSecurityPolicyFilter.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index 44238fb8535..69862d8b68e 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -75,7 +75,7 @@ public String getHeaderName() // and connections. if (AppProps.getInstance().isDevMode()) { - registerAllowedSources(Directive.Connection, "reactjs.hot.reload", "localhost:3001"); + registerAllowedSources(Directive.Connection, "reactjs.hot.reload", "localhost:3001 ws://localhost:3001"); registerAllowedSources(Directive.Font, "reactjs.hot.reload", "localhost:3001"); } }