From d5b2b36df543ad387284a9d170f539a101a7c584 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 13 May 2025 12:27:12 -0700 Subject: [PATCH 1/3] Conditionally add upgrade-secure-requests CSP directive --- .../filters/ContentSecurityPolicyFilter.java | 64 +++++++++---------- .../labkey/core/admin/AdminController.java | 4 ++ 2 files changed, 36 insertions(+), 32 deletions(-) diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index a89a69013de..e8a4a1cfcbe 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -49,16 +49,17 @@ public class ContentSecurityPolicyFilter implements Filter private static final String NONCE_SUBST = "REQUEST.SCRIPT.NONCE"; private static final String REPORT_PARAMETER_SUBSTITUTION = "CSP.REPORT.PARAMS"; + private static final String UPGRADE_SECURITY_REQUESTS_SUBSTITUTION = "UPGRADE.SECURE.REQUESTS"; private static final String HEADER_NONCE = "org.labkey.filters.ContentSecurityPolicyFilter#NONCE"; // needs to match PageConfig.HEADER_NONCE private static final Map CSP_FILTERS = new CopyOnWriteHashMap<>(); // Lock that protects the static data structures below - private static final Object ALLOWED_SOURCES_LOCK = new Object(); + private static final Object SUBSTITUTION_LOCK = new Object(); private static final Map> ALLOWED_SOURCES = new HashMap<>(); // Regenerate and stash on every "allowed source" change as a convenience (so every filter doesn't need to recalculate // it on every init() and change) - private static Map ALLOWED_SOURCES_SUBSTITUTION_MAP = Collections.emptyMap(); + private static Map SUBSTITUTION_MAP = Collections.emptyMap(); // Per-filter-instance parameters that are set in init() and never changed private ContentSecurityPolicyType _type = ContentSecurityPolicyType.Enforce; @@ -89,7 +90,7 @@ public String getHeaderName() static { - // ReactJS hot reload uses localhost port 3001. If in dev mode, allow browser to access that port for fonts + // ReactJS hot reload uses localhost port 3001. If in dev mode, allow the browser to access that port for fonts // and connections. if (AppProps.getInstance().isDevMode()) { @@ -203,10 +204,10 @@ private void regeneratePolicyExpression() { final String allowSubstitutedPolicy; - synchronized (ALLOWED_SOURCES_LOCK) + synchronized (SUBSTITUTION_LOCK) { allowSubstitutedPolicy = StringExpressionFactory.create(_policyTemplate, false, NullValueBehavior.KeepSubstitution) - .eval(ALLOWED_SOURCES_SUBSTITUTION_MAP); + .eval(SUBSTITUTION_MAP); } _policyExpression = StringExpressionFactory.create(allowSubstitutedPolicy, false, NullValueBehavior.ReplaceNullAndMissingWithBlank); @@ -250,7 +251,7 @@ public static void registerAllowedSources(Directive directive, String key, Strin public static void registerAllowedSources(String key, Directive directive, String... allowedSources) { - synchronized (ALLOWED_SOURCES_LOCK) + synchronized (SUBSTITUTION_LOCK) { if (allowedSources.length == 0) throw new IllegalStateException("Registering no sources is not allowed"); @@ -263,7 +264,7 @@ public static void registerAllowedSources(String key, Directive directive, Strin public static void unregisterAllowedSources(Directive directive, String key) { - synchronized (ALLOWED_SOURCES_LOCK) + synchronized (SUBSTITUTION_LOCK) { LOG.debug("Unregistering {} for {}", directive, key); SetValuedMap multiMap = ALLOWED_SOURCES.get(directive); @@ -278,11 +279,11 @@ public static void unregisterAllowedSources(Directive directive, String key) } // Regenerate the substitution map and all policy expressions on every register/unregister - private static void regenerateSubstitutionMap() + public static void regenerateSubstitutionMap() { - synchronized (ALLOWED_SOURCES_LOCK) + synchronized (SUBSTITUTION_LOCK) { - ALLOWED_SOURCES_SUBSTITUTION_MAP = ALLOWED_SOURCES.entrySet().stream() + SUBSTITUTION_MAP = ALLOWED_SOURCES.entrySet().stream() .filter(e -> !e.getValue().isEmpty()) .collect(Collectors.toMap( e -> e.getKey().getSubstitutionKey(), @@ -294,12 +295,9 @@ private static void regenerateSubstitutionMap() // Add an empty substitution for sources that lack registrations. This strips them from the stashed policy, // meaning less work on every request. Arrays.stream(Directive.values()) - .forEach(dir -> ALLOWED_SOURCES_SUBSTITUTION_MAP.putIfAbsent(dir.getSubstitutionKey(), "")); + .forEach(dir -> SUBSTITUTION_MAP.putIfAbsent(dir.getSubstitutionKey(), "")); - // Backward compatibility for CSPs using old substitution key - // TODO: Remove in 25.4 and adjust the junit test below - if (ALLOWED_SOURCES_SUBSTITUTION_MAP.containsKey(Directive.Connection.getSubstitutionKey())) - ALLOWED_SOURCES_SUBSTITUTION_MAP.put("LABKEY.ALLOWED.CONNECTIONS", ALLOWED_SOURCES_SUBSTITUTION_MAP.get(Directive.Connection.getSubstitutionKey())); + SUBSTITUTION_MAP.put(UPGRADE_SECURITY_REQUESTS_SUBSTITUTION, AppProps.getInstance().isSSLRequired() ? "upgrade-secure-requests;" : ""); // Tell each registered ContentSecurityPolicyFilter to refresh its policy template based on the new substitution map CSP_FILTERS.values().forEach(ContentSecurityPolicyFilter::regeneratePolicyExpression); @@ -378,13 +376,13 @@ public void testPolicyFiltering() @Test public void testSubstitutionMap() { - synchronized (ALLOWED_SOURCES_LOCK) + synchronized (SUBSTITUTION_LOCK) { - // Ensure substitution map has been initialized, otherwise the finally block asserts will fail + // Ensure the substitution map has been initialized; otherwise the finally block asserts will fail regenerateSubstitutionMap(); // Make a deep copy of ALLOWED_SOURCES so we can restore it after testing int sourceMapSize = ALLOWED_SOURCES.size(); - int substitutionMapSize = ALLOWED_SOURCES_SUBSTITUTION_MAP.size(); + int substitutionMapSize = SUBSTITUTION_MAP.size(); Map> savedSources = ALLOWED_SOURCES.entrySet().stream() .collect(Collectors.toMap(Map.Entry::getKey, e -> new HashSetValuedHashMap<>(e.getValue()))); @@ -408,53 +406,53 @@ public void testSubstitutionMap() verifySubstitutionMapSize(0); registerAllowedSources("foo", Directive.Connection, "MySource"); assertEquals(1, ALLOWED_SOURCES.size()); - verifySubstitutionMapSize(2); // Old connection substitution key should be added as well + verifySubstitutionMapSize(1); verifySubstitutionInPolicyExpressions("MySource", 1); registerAllowedSources("bar", Directive.Connection, "MySource"); assertEquals(1, ALLOWED_SOURCES.size()); - verifySubstitutionMapSize(2); + verifySubstitutionMapSize(1); verifySubstitutionInPolicyExpressions("MySource", 1); // Duplicate source should be filtered out unregisterAllowedSources(Directive.Font, "font"); registerAllowedSources("font", Directive.Font, "MySource"); assertEquals(2, ALLOWED_SOURCES.size()); - verifySubstitutionMapSize(3); + verifySubstitutionMapSize(2); verifySubstitutionInPolicyExpressions("MySource", 2); registerAllowedSources("font2", Directive.Font, "MyFontSource"); assertEquals(2, ALLOWED_SOURCES.size()); - verifySubstitutionMapSize(3); + verifySubstitutionMapSize(2); verifySubstitutionInPolicyExpressions("MySource", 2); verifySubstitutionInPolicyExpressions("MyFontSource", 1); unregisterAllowedSources(Directive.Font, "font2"); assertEquals(2, ALLOWED_SOURCES.size()); - verifySubstitutionMapSize(3); + verifySubstitutionMapSize(2); verifySubstitutionInPolicyExpressions("MySource", 2); verifySubstitutionInPolicyExpressions("MyFontSource", 0); unregisterAllowedSources(Directive.Font, "font"); assertEquals(2, ALLOWED_SOURCES.size()); // Font entry still exists, but should be empty assertTrue(ALLOWED_SOURCES.get(Directive.Font).isEmpty()); - verifySubstitutionMapSize(2);// Back to the way it was + verifySubstitutionMapSize(1);// Back to the way it was verifySubstitutionInPolicyExpressions("MySource", 1); verifySubstitutionInPolicyExpressions("MyFontSource", 0); unregisterAllowedSources(Directive.Frame, "frame"); registerAllowedSources("frame", Directive.Frame, "FrameSource", "FrameStore"); assertEquals(3, ALLOWED_SOURCES.size()); - verifySubstitutionMapSize(3); + verifySubstitutionMapSize(2); verifySubstitutionInPolicyExpressions("FrameSource", 1); verifySubstitutionInPolicyExpressions("FrameStore", 1); unregisterAllowedSources(Directive.Style, "style"); registerAllowedSources("style", Directive.Style, "StyleSource", "MoreStylishStore"); assertEquals(4, ALLOWED_SOURCES.size()); - verifySubstitutionMapSize(4); + verifySubstitutionMapSize(3); verifySubstitutionInPolicyExpressions("StyleSource", 1); verifySubstitutionInPolicyExpressions("MoreStylishStore", 1); unregisterAllowedSources(Directive.Image, "image"); registerAllowedSources("image", Directive.Image, "ImageSource", "BetterImageStore"); assertEquals(5, ALLOWED_SOURCES.size()); - verifySubstitutionMapSize(5); + verifySubstitutionMapSize(4); verifySubstitutionInPolicyExpressions("ImageSource", 1); verifySubstitutionInPolicyExpressions("BetterImageStore", 1); } @@ -465,7 +463,7 @@ public void testSubstitutionMap() ALLOWED_SOURCES.putAll(savedSources); regenerateSubstitutionMap(); assertEquals(sourceMapSize, ALLOWED_SOURCES.size()); - assertEquals(substitutionMapSize, ALLOWED_SOURCES_SUBSTITUTION_MAP.size()); + assertEquals(substitutionMapSize, SUBSTITUTION_MAP.size()); } } } @@ -485,11 +483,13 @@ private void verifySubstitutionInPolicyExpressions(String value, int expectedCou private void verifySubstitutionMapSize(long expectedNonEmptyValues) { - // Actual map size should stay static throughout test - int expectedSubstitutionMapSize = Directive.values().length + 1; // One extra for old "connections" key + // Actual map size should stay static throughout the test + int expectedSubstitutionMapSize = Directive.values().length + 1; // One extra for UPDATE.SECURE.REQUESTS + if (AppProps.getInstance().isSSLRequired()) + expectedNonEmptyValues++; - assertEquals(expectedSubstitutionMapSize, ALLOWED_SOURCES_SUBSTITUTION_MAP.size()); - long nonEmptyValues = ALLOWED_SOURCES_SUBSTITUTION_MAP.entrySet().stream().filter(e -> !e.getValue().isEmpty()).count(); + assertEquals(expectedSubstitutionMapSize, SUBSTITUTION_MAP.size()); + long nonEmptyValues = SUBSTITUTION_MAP.entrySet().stream().filter(e -> !e.getValue().isEmpty()).count(); assertEquals(expectedNonEmptyValues, nonEmptyValues); } } diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 8fbd09b5553..e2207f411f2 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -327,6 +327,7 @@ import org.labkey.core.security.BlockListFilter; import org.labkey.core.security.SecurityController; import org.labkey.data.xml.TablesDocument; +import org.labkey.filters.ContentSecurityPolicyFilter; import org.labkey.security.xml.GroupEnumType; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.validation.BindException; @@ -1341,6 +1342,7 @@ public boolean handlePost(SiteSettingsForm form, BindException errors) throws Ex props.setPipelineToolsDir(form.getPipelineToolsDirectory()); props.setNavAccessOpen(form.isNavAccessOpen()); props.setSSLRequired(form.isSslRequired()); + boolean sslSettingChanged = AppProps.getInstance().isSSLRequired() != form.isSslRequired(); props.setSSLPort(form.getSslPort()); props.setMemoryUsageDumpInterval(form.getMemoryUsageDumpInterval()); props.setReadOnlyHttpRequestTimeout(form.getReadOnlyHttpRequestTimeout()); @@ -1415,6 +1417,8 @@ public boolean handlePost(SiteSettingsForm form, BindException errors) throws Ex props.save(getViewContext().getUser()); UsageReportingLevel.reportNow(); + if (sslSettingChanged) + ContentSecurityPolicyFilter.regenerateSubstitutionMap(); return true; } From 61ddd7d751b3ccf77f477f533b1d56a93196c58f Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Tue, 13 May 2025 17:39:41 -0700 Subject: [PATCH 2/3] Fix typo in upgrade-insecure-requests --- api/src/org/labkey/filters/ContentSecurityPolicyFilter.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index e8a4a1cfcbe..e28879561ce 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -49,7 +49,7 @@ public class ContentSecurityPolicyFilter implements Filter private static final String NONCE_SUBST = "REQUEST.SCRIPT.NONCE"; private static final String REPORT_PARAMETER_SUBSTITUTION = "CSP.REPORT.PARAMS"; - private static final String UPGRADE_SECURITY_REQUESTS_SUBSTITUTION = "UPGRADE.SECURE.REQUESTS"; + 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 Map CSP_FILTERS = new CopyOnWriteHashMap<>(); @@ -297,7 +297,7 @@ public static void regenerateSubstitutionMap() Arrays.stream(Directive.values()) .forEach(dir -> SUBSTITUTION_MAP.putIfAbsent(dir.getSubstitutionKey(), "")); - SUBSTITUTION_MAP.put(UPGRADE_SECURITY_REQUESTS_SUBSTITUTION, AppProps.getInstance().isSSLRequired() ? "upgrade-secure-requests;" : ""); + SUBSTITUTION_MAP.put(UPGRADE_INSECURE_REQUESTS_SUBSTITUTION, AppProps.getInstance().isSSLRequired() ? "upgrade-insecure-requests;" : ""); // Tell each registered ContentSecurityPolicyFilter to refresh its policy template based on the new substitution map CSP_FILTERS.values().forEach(ContentSecurityPolicyFilter::regeneratePolicyExpression); From 0e25eb170a6da45757ae2d73d90f740f3e6b9006 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Wed, 14 May 2025 08:01:52 -0700 Subject: [PATCH 3/3] Update (upgrade?) comment --- 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 e28879561ce..787c1b3702d 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -484,7 +484,7 @@ private void verifySubstitutionInPolicyExpressions(String value, int expectedCou private void verifySubstitutionMapSize(long expectedNonEmptyValues) { // Actual map size should stay static throughout the test - int expectedSubstitutionMapSize = Directive.values().length + 1; // One extra for UPDATE.SECURE.REQUESTS + int expectedSubstitutionMapSize = Directive.values().length + 1; // One extra for UPGRADE.INSECURE.REQUESTS if (AppProps.getInstance().isSSLRequired()) expectedNonEmptyValues++;