Skip to content

Commit be8ecbb

Browse files
labkey-adamlabkey-adam
authored
A couple more CSP enhancements (#6809)
1 parent 473df44 commit be8ecbb

File tree

13 files changed

+89
-36
lines changed

13 files changed

+89
-36
lines changed

‎api/src/org/labkey/api/action/SimpleViewAction.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616

1717
package org.labkey.api.action;
1818

19+
import org.jetbrains.annotations.NotNull;
1920
import org.labkey.api.miniprofiler.MiniProfiler;
2021
import org.labkey.api.miniprofiler.Timing;
2122
import org.labkey.api.view.BadRequestException;
@@ -134,7 +135,7 @@ public ModelAndView getPrintView(FORM form, BindException errors) throws Excepti
134135
}
135136

136137
@Override
137-
public void validate(Object target, Errors errors)
138+
public void validate(@NotNull Object target, @NotNull Errors errors)
138139
{
139140
}
140141
}

‎api/src/org/labkey/api/data/ColumnInfo.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,7 @@ static int findColumn(ResultSet rs, String name)
134134
* new SQLFragment().append(col.getValueSql("R")).append(" AS ").appendIdentifier(col.getAlias())
135135
* The returned ResultSet will contain a column named col.getAlias()
136136
*
137-
* NOTE: if you directly bind your results using BeanObjectFactory (e.g. TableSelector.getArrayList(MyClass.class))
137+
* NOTE: if you directly bind your results using BeanObjectFactory (e.g., TableSelector.getArrayList(MyClass.class))
138138
* you should
139139
* a) match your column aliases to the bean properties you want to populate
140140
* b) prefer using TableSelector vs SqlSelector. TableSelector will use ColumnInfo.getAlias().

‎api/src/org/labkey/api/reports/report/r/view/HtmlOutput.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ public HtmlOutputView(ParamReplacement param, String label)
9494
protected String renderInternalAsString(File file) throws Exception
9595
{
9696
if (exists(file))
97-
return PageFlowUtil.getFileContentsAsString(file);
97+
return PageFlowUtil.addScriptNonces(PageFlowUtil.getFileContentsAsString(file));
9898

9999
return null;
100100
}

‎api/src/org/labkey/api/security/Directive.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,15 @@
55

66
/**
77
* All CSP directives that support substitutions. These constant names are persisted to the database, so be careful with
8-
* any changes. If adding a Directive, make sure to add the corresponding substitutions to application.properties.
8+
* any changes. If adding a Directive, make sure to add the corresponding substitutions in LabKeyServer baseCsp.
99
*/
1010
public enum Directive implements StartupProperty, SafeToRenderEnum
1111
{
1212
Connection("connect-src", "Sources for fetch/XHR requests"),
1313
Font("font-src", "Sources for fonts"),
1414
Frame("frame-src", "Sources for iframes"),
1515
Image("image-src", "Sources for images"),
16+
Object("object-src", "Sources for objects"), // Issue 53226
1617
Style("style-src", "Sources for stylesheets");
1718

1819
private final String _cspDirective;

‎api/src/org/labkey/api/security/SecurityManager.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ public static void registerAllowedConnectionSource(String key, String serviceURL
236236
{
237237
if (StringUtils.trimToNull(serviceURL) == null)
238238
{
239-
ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Connection, key);
239+
ContentSecurityPolicyFilter.unregisterAllowedSources(key, Directive.Connection);
240240
LOG.trace(String.format("Unregistered [%1$s] as an allowed connection source", key));
241241
return;
242242
}

‎api/src/org/labkey/api/util/PageFlowUtil.java

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3156,4 +3156,51 @@ public static HtmlString getDataRegionHtmlForPropertyValues(Map<String, String>
31563156
return HtmlString.unsafe(sb.toString());
31573157
}
31583158

3159+
/**
3160+
* Convert String containing HTML into a Document, add nonces to all {@code <script>} tags, and turn the Document
3161+
* back into a String.
3162+
*/
3163+
public static String addScriptNonces(String html)
3164+
{
3165+
Document doc = JSoupUtil.convertHtmlToDocument(StringUtils.trimToEmpty(html), false, new LinkedList<>());
3166+
String ret = "";
3167+
if (doc != null)
3168+
{
3169+
addScriptNonces(doc);
3170+
try
3171+
{
3172+
ret = convertNodeToHtml(doc);
3173+
}
3174+
catch (TransformerException | IOException e)
3175+
{
3176+
throw new RuntimeException(e);
3177+
}
3178+
}
3179+
3180+
return ret;
3181+
}
3182+
3183+
/*
3184+
* Add nonce attribute to <script> tags in the Document.
3185+
* We don't require the <%=scriptNonce%> syntax (as with module HTML view), because we already parsed the page.
3186+
* ModuleHtmlView does not parse the page and does a raw regexp substitution.
3187+
*/
3188+
public static int addScriptNonces(Document doc)
3189+
{
3190+
NodeList nl = doc.getElementsByTagName("script");
3191+
3192+
if (nl.getLength() > 0)
3193+
{
3194+
// If rendering outside a request (e.g., search crawler), substitute blank
3195+
String nonce = HttpView.hasCurrentView() ? HttpView.currentPageConfig().getScriptNonce().toString() : "";
3196+
3197+
for (int i = 0, length = nl.getLength(); i < length; i++)
3198+
{
3199+
Element script = (Element) nl.item(i);
3200+
script.setAttribute("nonce", nonce);
3201+
}
3202+
}
3203+
3204+
return nl.getLength();
3205+
}
31593206
}

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

Lines changed: 23 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -263,7 +263,7 @@ public static void registerAllowedSources(String key, Directive directive, Strin
263263
}
264264
}
265265

266-
public static void unregisterAllowedSources(Directive directive, String key)
266+
public static void unregisterAllowedSources(String key, Directive directive)
267267
{
268268
synchronized (SUBSTITUTION_LOCK)
269269
{
@@ -293,6 +293,9 @@ public static void regenerateSubstitutionMap()
293293
.collect(Collectors.joining(" ")))
294294
);
295295

296+
// Special case for object-src: default to 'none' if no admin overrides exist, Issue 53226
297+
SUBSTITUTION_MAP.putIfAbsent(Directive.Object.getSubstitutionKey(), "'none'");
298+
296299
// Add an empty substitution for sources that lack registrations. This strips them from the stashed policy,
297300
// meaning less work on every request.
298301
Arrays.stream(Directive.values())
@@ -395,14 +398,14 @@ public void testSubstitutionMap()
395398
// Initial checks
396399
assertTrue(ALLOWED_SOURCES.isEmpty());
397400
verifySubstitutionMapSize(0);
398-
// All "allowed sources" substitutions should be replaced with empty strings
401+
// All "allowed sources" substitutions should be replaced with empty string or 'none' (object-src)
399402
verifySubstitutionInPolicyExpressions(".SOURCES}", 0);
400403
// Should have been substitution in init()
401404
verifySubstitutionInPolicyExpressions("${CSP.REPORT.PARAMS}", 0);
402405
verifySubstitutionInPolicyExpressions("${", 0);
403406

404407
// Now unregister and register sources for each Directive, testing expectations along the way
405-
unregisterAllowedSources(Directive.Connection, "foo");
408+
unregisterAllowedSources("foo", Directive.Connection);
406409
assertTrue(ALLOWED_SOURCES.isEmpty());
407410
verifySubstitutionMapSize(0);
408411
registerAllowedSources("foo", Directive.Connection, "MySource");
@@ -414,7 +417,7 @@ public void testSubstitutionMap()
414417
verifySubstitutionMapSize(1);
415418
verifySubstitutionInPolicyExpressions("MySource", 1); // Duplicate source should be filtered out
416419

417-
unregisterAllowedSources(Directive.Font, "font");
420+
unregisterAllowedSources("font", Directive.Font);
418421
registerAllowedSources("font", Directive.Font, "MySource");
419422
assertEquals(2, ALLOWED_SOURCES.size());
420423
verifySubstitutionMapSize(2);
@@ -424,38 +427,47 @@ public void testSubstitutionMap()
424427
verifySubstitutionMapSize(2);
425428
verifySubstitutionInPolicyExpressions("MySource", 2);
426429
verifySubstitutionInPolicyExpressions("MyFontSource", 1);
427-
unregisterAllowedSources(Directive.Font, "font2");
430+
unregisterAllowedSources("font2", Directive.Font);
428431
assertEquals(2, ALLOWED_SOURCES.size());
429432
verifySubstitutionMapSize(2);
430433
verifySubstitutionInPolicyExpressions("MySource", 2);
431434
verifySubstitutionInPolicyExpressions("MyFontSource", 0);
432-
unregisterAllowedSources(Directive.Font, "font");
433-
assertEquals(2, ALLOWED_SOURCES.size()); // Font entry still exists, but should be empty
435+
unregisterAllowedSources("font", Directive.Font);
436+
assertEquals(2, ALLOWED_SOURCES.size()); // Font entry still exists but should be empty
434437
assertTrue(ALLOWED_SOURCES.get(Directive.Font).isEmpty());
435438
verifySubstitutionMapSize(1);// Back to the way it was
436439
verifySubstitutionInPolicyExpressions("MySource", 1);
437440
verifySubstitutionInPolicyExpressions("MyFontSource", 0);
438441

439-
unregisterAllowedSources(Directive.Frame, "frame");
442+
unregisterAllowedSources("frame", Directive.Frame);
440443
registerAllowedSources("frame", Directive.Frame, "FrameSource", "FrameStore");
441444
assertEquals(3, ALLOWED_SOURCES.size());
442445
verifySubstitutionMapSize(2);
443446
verifySubstitutionInPolicyExpressions("FrameSource", 1);
444447
verifySubstitutionInPolicyExpressions("FrameStore", 1);
445448

446-
unregisterAllowedSources(Directive.Style, "style");
449+
unregisterAllowedSources("style", Directive.Style);
447450
registerAllowedSources("style", Directive.Style, "StyleSource", "MoreStylishStore");
448451
assertEquals(4, ALLOWED_SOURCES.size());
449452
verifySubstitutionMapSize(3);
450453
verifySubstitutionInPolicyExpressions("StyleSource", 1);
451454
verifySubstitutionInPolicyExpressions("MoreStylishStore", 1);
452455

453-
unregisterAllowedSources(Directive.Image, "image");
456+
unregisterAllowedSources("image", Directive.Image);
454457
registerAllowedSources("image", Directive.Image, "ImageSource", "BetterImageStore");
455458
assertEquals(5, ALLOWED_SOURCES.size());
456459
verifySubstitutionMapSize(4);
457460
verifySubstitutionInPolicyExpressions("ImageSource", 1);
458461
verifySubstitutionInPolicyExpressions("BetterImageStore", 1);
462+
463+
unregisterAllowedSources("object", Directive.Object);
464+
verifySubstitutionInPolicyExpressions("'none'", 1);
465+
registerAllowedSources("object", Directive.Object, "ObjectSource", "BetterObjectStore");
466+
assertEquals(6, ALLOWED_SOURCES.size());
467+
verifySubstitutionMapSize(4); // Adding to object-src doesn't change the non-empty count
468+
verifySubstitutionInPolicyExpressions("'none'", 0);
469+
verifySubstitutionInPolicyExpressions("ObjectSource", 1);
470+
verifySubstitutionInPolicyExpressions("BetterObjectStore", 1);
459471
}
460472
finally
461473
{
@@ -491,7 +503,7 @@ private void verifySubstitutionMapSize(long expectedNonEmptyValues)
491503

492504
assertEquals(expectedSubstitutionMapSize, SUBSTITUTION_MAP.size());
493505
long nonEmptyValues = SUBSTITUTION_MAP.entrySet().stream().filter(e -> !e.getValue().isEmpty()).count();
494-
assertEquals(expectedNonEmptyValues, nonEmptyValues);
506+
assertEquals(expectedNonEmptyValues + 1, nonEmptyValues); // One extra expected because of OBJECT.SOURCES default value
495507
}
496508
}
497509
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -537,7 +537,7 @@ public QuerySchema createSchema(DefaultSchema schema, Module module)
537537
false);
538538
AdminConsole.addOptionalFeatureFlag(new OptionalFeatureFlag(AppProps.DEPRECATED_OBJECT_LEVEL_DISCUSSIONS,
539539
"Restore Object-Level Discussions",
540-
"This option and all support for Object-Level Discussions will be removed in LabKey Server v25.7.",
540+
"This option and all support for Object-Level Discussions will be removed in LabKey Server v25.11.",
541541
false, false, FeatureType.Deprecated));
542542
AdminConsole.addOptionalFeatureFlag(new OptionalFeatureFlag(SimpleTranslator.DEPRECATED_NULL_MISSING_VALUE_RESOLUTION,
543543
"Resolve Missing Lookup Values to Null",

‎core/src/org/labkey/core/analytics/AnalyticsServiceImpl.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,13 +118,13 @@ public String getDescription()
118118

119119
public void resetCSP()
120120
{
121-
ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Connection, ANALYTICS_CSP_KEY);
122-
ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Image, ANALYTICS_CSP_KEY);
121+
ContentSecurityPolicyFilter.unregisterAllowedSources(ANALYTICS_CSP_KEY, Directive.Connection);
122+
ContentSecurityPolicyFilter.unregisterAllowedSources(ANALYTICS_CSP_KEY, Directive.Image);
123123

124124
if (getTrackingStatus().contains(TrackingStatus.ga4FullUrl))
125125
{
126-
ContentSecurityPolicyFilter.registerAllowedSources(Directive.Connection, ANALYTICS_CSP_KEY, "https://*.googletagmanager.com", "https://*.google-analytics.com", "https://*.analytics.google.com");
127-
ContentSecurityPolicyFilter.registerAllowedSources(Directive.Image, ANALYTICS_CSP_KEY, "https://www.googletagmanager.com");
126+
ContentSecurityPolicyFilter.registerAllowedSources(ANALYTICS_CSP_KEY, Directive.Connection, "https://*.googletagmanager.com", "https://*.google-analytics.com", "https://*.analytics.google.com");
127+
ContentSecurityPolicyFilter.registerAllowedSources(ANALYTICS_CSP_KEY, Directive.Image, "https://www.googletagmanager.com");
128128
}
129129
}
130130

‎core/src/org/labkey/core/security/AllowedExternalResourceHosts.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ public static void saveAllowedHosts(@Nullable Collection<AllowedHost> allowedHos
6363

6464
// Unregister all supported directives then register the directives that have at least one allowed host
6565
Arrays.stream(Directive.values()).forEach(dir -> {
66-
ContentSecurityPolicyFilter.unregisterAllowedSources(dir, ALLOWED_EXTERNAL_RESOURCES);
66+
ContentSecurityPolicyFilter.unregisterAllowedSources(ALLOWED_EXTERNAL_RESOURCES, dir);
6767
List<String> list = map.get(dir);
6868
if (list != null)
6969
ContentSecurityPolicyFilter.registerAllowedSources(ALLOWED_EXTERNAL_RESOURCES, dir, list.toArray(new String[0]));

0 commit comments

Comments
 (0)