Skip to content

A couple more CSP enhancements #6809

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: release25.7-SNAPSHOT
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion api/src/org/labkey/api/action/SimpleViewAction.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package org.labkey.api.action;

import org.jetbrains.annotations.NotNull;
import org.labkey.api.miniprofiler.MiniProfiler;
import org.labkey.api.miniprofiler.Timing;
import org.labkey.api.view.BadRequestException;
Expand Down Expand Up @@ -134,7 +135,7 @@ public ModelAndView getPrintView(FORM form, BindException errors) throws Excepti
}

@Override
public void validate(Object target, Errors errors)
public void validate(@NotNull Object target, @NotNull Errors errors)
{
}
}
2 changes: 1 addition & 1 deletion api/src/org/labkey/api/data/ColumnInfo.java
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ static int findColumn(ResultSet rs, String name)
* new SQLFragment().append(col.getValueSql("R")).append(" AS ").appendIdentifier(col.getAlias())
* The returned ResultSet will contain a column named col.getAlias()
*
* NOTE: if you directly bind your results using BeanObjectFactory (e.g. TableSelector.getArrayList(MyClass.class))
* NOTE: if you directly bind your results using BeanObjectFactory (e.g., TableSelector.getArrayList(MyClass.class))
* you should
* a) match your column aliases to the bean properties you want to populate
* b) prefer using TableSelector vs SqlSelector. TableSelector will use ColumnInfo.getAlias().
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public HtmlOutputView(ParamReplacement param, String label)
protected String renderInternalAsString(File file) throws Exception
{
if (exists(file))
return PageFlowUtil.getFileContentsAsString(file);
return PageFlowUtil.addScriptNonces(PageFlowUtil.getFileContentsAsString(file));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems fine. I'm curious about all the R caching scenarios, but I think this happens "late".


return null;
}
Expand Down
3 changes: 2 additions & 1 deletion api/src/org/labkey/api/security/Directive.java
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@

/**
* 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.
* any changes. If adding a Directive, make sure to add the corresponding substitutions in LabKeyServer baseCsp.
*/
public enum Directive implements StartupProperty, SafeToRenderEnum
{
Connection("connect-src", "Sources for fetch/XHR requests"),
Font("font-src", "Sources for fonts"),
Frame("frame-src", "Sources for iframes"),
Image("image-src", "Sources for images"),
Object("object-src", "Sources for objects"), // Issue 53226
Style("style-src", "Sources for stylesheets");

private final String _cspDirective;
Expand Down
2 changes: 1 addition & 1 deletion api/src/org/labkey/api/security/SecurityManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ public static void registerAllowedConnectionSource(String key, String serviceURL
{
if (StringUtils.trimToNull(serviceURL) == null)
{
ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Connection, key);
ContentSecurityPolicyFilter.unregisterAllowedSources(key, Directive.Connection);
LOG.trace(String.format("Unregistered [%1$s] as an allowed connection source", key));
return;
}
Expand Down
47 changes: 47 additions & 0 deletions api/src/org/labkey/api/util/PageFlowUtil.java
Original file line number Diff line number Diff line change
Expand Up @@ -3156,4 +3156,51 @@ public static HtmlString getDataRegionHtmlForPropertyValues(Map<String, String>
return HtmlString.unsafe(sb.toString());
}

/**
* Convert String containing HTML into a Document, add nonces to all {@code <script>} tags, and turn the Document
* back into a String.
*/
public static String addScriptNonces(String html)
{
Document doc = JSoupUtil.convertHtmlToDocument(StringUtils.trimToEmpty(html), false, new LinkedList<>());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course this will do various transformations to the code if it is not well-formed. I guess I'm alright with that.

String ret = "";
if (doc != null)
{
addScriptNonces(doc);
try
{
ret = convertNodeToHtml(doc);
}
catch (TransformerException | IOException e)
{
throw new RuntimeException(e);
}
}

return ret;
}

/*
* Add nonce attribute to <script> tags in the Document.
* We don't require the <%=scriptNonce%> syntax (as with module HTML view), because we already parsed the page.
* ModuleHtmlView does not parse the page and does a raw regexp substitution.
*/
public static int addScriptNonces(Document doc)
{
NodeList nl = doc.getElementsByTagName("script");

if (nl.getLength() > 0)
{
// If rendering outside a request (e.g., search crawler), substitute blank
String nonce = HttpView.hasCurrentView() ? HttpView.currentPageConfig().getScriptNonce().toString() : "";

for (int i = 0, length = nl.getLength(); i < length; i++)
{
Element script = (Element) nl.item(i);
script.setAttribute("nonce", nonce);
}
}

return nl.getLength();
}
}
34 changes: 23 additions & 11 deletions api/src/org/labkey/filters/ContentSecurityPolicyFilter.java
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ public static void registerAllowedSources(String key, Directive directive, Strin
}
}

public static void unregisterAllowedSources(Directive directive, String key)
public static void unregisterAllowedSources(String key, Directive directive)
{
synchronized (SUBSTITUTION_LOCK)
{
Expand Down Expand Up @@ -293,6 +293,9 @@ public static void regenerateSubstitutionMap()
.collect(Collectors.joining(" ")))
);

// Special case for object-src: default to 'none' if no admin overrides exist, Issue 53226
SUBSTITUTION_MAP.putIfAbsent(Directive.Object.getSubstitutionKey(), "'none'");

// 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())
Expand Down Expand Up @@ -395,14 +398,14 @@ public void testSubstitutionMap()
// Initial checks
assertTrue(ALLOWED_SOURCES.isEmpty());
verifySubstitutionMapSize(0);
// All "allowed sources" substitutions should be replaced with empty strings
// All "allowed sources" substitutions should be replaced with empty string or 'none' (object-src)
verifySubstitutionInPolicyExpressions(".SOURCES}", 0);
// Should have been substitution in init()
verifySubstitutionInPolicyExpressions("${CSP.REPORT.PARAMS}", 0);
verifySubstitutionInPolicyExpressions("${", 0);

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

unregisterAllowedSources(Directive.Font, "font");
unregisterAllowedSources("font", Directive.Font);
registerAllowedSources("font", Directive.Font, "MySource");
assertEquals(2, ALLOWED_SOURCES.size());
verifySubstitutionMapSize(2);
Expand All @@ -424,38 +427,47 @@ public void testSubstitutionMap()
verifySubstitutionMapSize(2);
verifySubstitutionInPolicyExpressions("MySource", 2);
verifySubstitutionInPolicyExpressions("MyFontSource", 1);
unregisterAllowedSources(Directive.Font, "font2");
unregisterAllowedSources("font2", Directive.Font);
assertEquals(2, ALLOWED_SOURCES.size());
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
unregisterAllowedSources("font", Directive.Font);
assertEquals(2, ALLOWED_SOURCES.size()); // Font entry still exists but should be empty
assertTrue(ALLOWED_SOURCES.get(Directive.Font).isEmpty());
verifySubstitutionMapSize(1);// Back to the way it was
verifySubstitutionInPolicyExpressions("MySource", 1);
verifySubstitutionInPolicyExpressions("MyFontSource", 0);

unregisterAllowedSources(Directive.Frame, "frame");
unregisterAllowedSources("frame", Directive.Frame);
registerAllowedSources("frame", Directive.Frame, "FrameSource", "FrameStore");
assertEquals(3, ALLOWED_SOURCES.size());
verifySubstitutionMapSize(2);
verifySubstitutionInPolicyExpressions("FrameSource", 1);
verifySubstitutionInPolicyExpressions("FrameStore", 1);

unregisterAllowedSources(Directive.Style, "style");
unregisterAllowedSources("style", Directive.Style);
registerAllowedSources("style", Directive.Style, "StyleSource", "MoreStylishStore");
assertEquals(4, ALLOWED_SOURCES.size());
verifySubstitutionMapSize(3);
verifySubstitutionInPolicyExpressions("StyleSource", 1);
verifySubstitutionInPolicyExpressions("MoreStylishStore", 1);

unregisterAllowedSources(Directive.Image, "image");
unregisterAllowedSources("image", Directive.Image);
registerAllowedSources("image", Directive.Image, "ImageSource", "BetterImageStore");
assertEquals(5, ALLOWED_SOURCES.size());
verifySubstitutionMapSize(4);
verifySubstitutionInPolicyExpressions("ImageSource", 1);
verifySubstitutionInPolicyExpressions("BetterImageStore", 1);

unregisterAllowedSources("object", Directive.Object);
verifySubstitutionInPolicyExpressions("'none'", 1);
registerAllowedSources("object", Directive.Object, "ObjectSource", "BetterObjectStore");
assertEquals(6, ALLOWED_SOURCES.size());
verifySubstitutionMapSize(4); // Adding to object-src doesn't change the non-empty count
verifySubstitutionInPolicyExpressions("'none'", 0);
verifySubstitutionInPolicyExpressions("ObjectSource", 1);
verifySubstitutionInPolicyExpressions("BetterObjectStore", 1);
}
finally
{
Expand Down Expand Up @@ -491,7 +503,7 @@ private void verifySubstitutionMapSize(long expectedNonEmptyValues)

assertEquals(expectedSubstitutionMapSize, SUBSTITUTION_MAP.size());
long nonEmptyValues = SUBSTITUTION_MAP.entrySet().stream().filter(e -> !e.getValue().isEmpty()).count();
assertEquals(expectedNonEmptyValues, nonEmptyValues);
assertEquals(expectedNonEmptyValues + 1, nonEmptyValues); // One extra expected because of OBJECT.SOURCES default value
}
}
}
2 changes: 1 addition & 1 deletion core/src/org/labkey/core/CoreModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -537,7 +537,7 @@ public QuerySchema createSchema(DefaultSchema schema, Module module)
false);
AdminConsole.addOptionalFeatureFlag(new OptionalFeatureFlag(AppProps.DEPRECATED_OBJECT_LEVEL_DISCUSSIONS,
"Restore Object-Level Discussions",
"This option and all support for Object-Level Discussions will be removed in LabKey Server v25.7.",
"This option and all support for Object-Level Discussions will be removed in LabKey Server v25.11.",
false, false, FeatureType.Deprecated));
AdminConsole.addOptionalFeatureFlag(new OptionalFeatureFlag(SimpleTranslator.DEPRECATED_NULL_MISSING_VALUE_RESOLUTION,
"Resolve Missing Lookup Values to Null",
Expand Down
8 changes: 4 additions & 4 deletions core/src/org/labkey/core/analytics/AnalyticsServiceImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -118,13 +118,13 @@ public String getDescription()

public void resetCSP()
{
ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Connection, ANALYTICS_CSP_KEY);
ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Image, ANALYTICS_CSP_KEY);
ContentSecurityPolicyFilter.unregisterAllowedSources(ANALYTICS_CSP_KEY, Directive.Connection);
ContentSecurityPolicyFilter.unregisterAllowedSources(ANALYTICS_CSP_KEY, Directive.Image);

if (getTrackingStatus().contains(TrackingStatus.ga4FullUrl))
{
ContentSecurityPolicyFilter.registerAllowedSources(Directive.Connection, ANALYTICS_CSP_KEY, "https://*.googletagmanager.com", "https://*.google-analytics.com", "https://*.analytics.google.com");
ContentSecurityPolicyFilter.registerAllowedSources(Directive.Image, ANALYTICS_CSP_KEY, "https://www.googletagmanager.com");
ContentSecurityPolicyFilter.registerAllowedSources(ANALYTICS_CSP_KEY, Directive.Connection, "https://*.googletagmanager.com", "https://*.google-analytics.com", "https://*.analytics.google.com");
ContentSecurityPolicyFilter.registerAllowedSources(ANALYTICS_CSP_KEY, Directive.Image, "https://www.googletagmanager.com");
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ public static void saveAllowedHosts(@Nullable Collection<AllowedHost> allowedHos

// Unregister all supported directives then register the directives that have at least one allowed host
Arrays.stream(Directive.values()).forEach(dir -> {
ContentSecurityPolicyFilter.unregisterAllowedSources(dir, ALLOWED_EXTERNAL_RESOURCES);
ContentSecurityPolicyFilter.unregisterAllowedSources(ALLOWED_EXTERNAL_RESOURCES, dir);
List<String> list = map.get(dir);
if (list != null)
ContentSecurityPolicyFilter.registerAllowedSources(ALLOWED_EXTERNAL_RESOURCES, dir, list.toArray(new String[0]));
Expand Down
17 changes: 4 additions & 13 deletions core/src/org/labkey/core/wiki/HtmlRenderer.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@
import org.labkey.api.util.HtmlString;
import org.labkey.api.util.JSoupUtil;
import org.labkey.api.util.PageFlowUtil;
import org.labkey.api.view.HttpView;
import org.labkey.api.view.template.ClientDependency;
import org.labkey.api.wiki.FormattedHtml;
import org.labkey.api.wiki.WikiRenderer;
Expand Down Expand Up @@ -97,7 +96,7 @@ public FormattedHtml format(String text)
Document doc = JSoupUtil.convertHtmlToDocument("<html><body>" + StringUtils.trimToEmpty(formattedHtml.getHtml().toString()) + "</body></html>", false, errors);
if (!errors.isEmpty() || doc == null)
{
StringBuilder innerHtml = new StringBuilder("<div class=\"labkey-error\"><b>An exception occurred while generating the HTML. Please correct this content.</b></div><br>The error message was: ");
StringBuilder innerHtml = new StringBuilder("<div class=\"labkey-error\"><b>An exception occurred while generating the HTML. Please correct this content.</b></div><br>The error message was: ");
if (!errors.isEmpty())
{
for (String error : errors)
Expand Down Expand Up @@ -169,18 +168,10 @@ public FormattedHtml format(String text)
}
}

/* Add nonce attribute to <script> tags in the page.
* We don't require the <%=scriptNonce%> syntax (as with module html view), because we're already parsing the page.
* ModuleHtmlView does not parse the page and does a raw regexp substitution.
*/
nl = doc.getElementsByTagName("script");
for (int i = 0, length = nl.getLength(); i < length; i++)
{
Element script = (Element)nl.item(i);
script.setAttribute("nonce", HttpView.currentPageConfig().getScriptNonce().toString());
// Add nonces to all script elements. Mark the page as volatile if any script elements are present. This is a
// little heavy-handed. We could add a "post-render" step instead.
if (PageFlowUtil.addScriptNonces(doc) > 0)
volatilePage = true;
/* NOTE marking the page as volatile is a little heavy-handed. We could add a "post-render" step, or detect that there is not CSP */
}

// back to html
StringBuilder innerHtml;
Expand Down
2 changes: 1 addition & 1 deletion pipeline/src/org/labkey/pipeline/PipelineModule.java
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ protected void startupAfterSpringConfig(ModuleContext moduleContext)

AdminConsole.addOptionalFeatureFlag(new AdminConsole.OptionalFeatureFlag(ADVANCED_IMPORT_FLAG,
"Restore 'Advanced Import Options' during Folder import",
"This option will be removed in LabKey Server v25.7.",
"This option will be removed in LabKey Server v25.11.",
false, false, OptionalFeatureService.FeatureType.Deprecated
));

Expand Down
1 change: 1 addition & 0 deletions study/src/org/labkey/study/query/StudyQuerySchema.java
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

package org.labkey.study.query;

import com.google.common.collect.Iterables;
import org.apache.commons.lang3.StringUtils;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
Expand Down