From 76e53375a4ca26f1437e141b6d109ae23d3c6a62 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 20 Feb 2025 19:48:16 -0800 Subject: [PATCH 01/23] Admin UI for adding external hosts to a variety of directives --- core/src/org/labkey/core/CoreModule.java | 6 +- .../labkey/core/admin/AdminController.java | 149 +++++++++--------- ...rnalServerType.java => AllowListType.java} | 82 +++++----- .../labkey/core/admin/AuthorityValidator.java | 18 +++ ...ewExternalHost.jsp => addNewListValue.jsp} | 16 +- ...ternalHosts.jsp => existingListValues.jsp} | 40 ++--- 6 files changed, 164 insertions(+), 147 deletions(-) rename core/src/org/labkey/core/admin/{ExternalServerType.java => AllowListType.java} (77%) create mode 100644 core/src/org/labkey/core/admin/AuthorityValidator.java rename core/src/org/labkey/core/admin/{addNewExternalHost.jsp => addNewListValue.jsp} (80%) rename core/src/org/labkey/core/admin/{existingExternalHosts.jsp => existingListValues.jsp} (67%) diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 525eb1f7e40..5941903d163 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -213,7 +213,7 @@ import org.labkey.core.admin.CustomizeMenuForm; import org.labkey.core.admin.DisplayFormatAnalyzer; import org.labkey.core.admin.DisplayFormatValidationProviderFactory; -import org.labkey.core.admin.ExternalServerType; +import org.labkey.core.admin.AllowListType; import org.labkey.core.admin.FilesSiteSettingsAction; import org.labkey.core.admin.MenuViewFactory; import org.labkey.core.admin.importer.FolderTypeImporterFactory; @@ -1244,8 +1244,8 @@ public void moduleStartupComplete(ServletContext servletContext) private void registerAllowedConnectionSources() { ContentSecurityPolicyFilter.registerAllowedConnectionSource( - ExternalServerType.getExternalSourceHostsKey(), - ExternalServerType.Source.getHosts().toArray(new String[0]) + AllowListType.getExternalSourceHostsKey(), + AllowListType.Source.getValues().toArray(new String[0]) ); LOG.debug("Registered [{}] as an allowed connection source", () -> String.join(", ", AppProps.getInstance().getExternalSourceHosts())); diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index edbdcba81f1..da1b31bc8a8 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -452,9 +452,9 @@ public static void registerAdminConsoleLinks() AdminConsole.addLink(Configuration, "short urls", new ActionURL(ShortURLAdminAction.class, root), AdminPermission.class); AdminConsole.addLink(Configuration, "site settings", new AdminUrlsImpl().getCustomizeSiteURL()); AdminConsole.addLink(Configuration, "system maintenance", new ActionURL(ConfigureSystemMaintenanceAction.class, root)); - AdminConsole.addLink(Configuration, "external redirect hosts", new ActionURL(ExternalHostsAdminAction.class, root).addParameter("type", ExternalServerType.Redirect.name()), TroubleshooterPermission.class); - AdminConsole.addLink(Configuration, "external allowed sources", new ActionURL(ExternalHostsAdminAction.class, root).addParameter("type", ExternalServerType.Source.name()), TroubleshooterPermission.class); - AdminConsole.addLink(Configuration, "allowed file extensions", new ActionURL(ExternalHostsAdminAction.class, root).addParameter("type", ExternalServerType.FileExtension.name()), TroubleshooterPermission.class); + AdminConsole.addLink(Configuration, "allowed external redirect hosts", new ActionURL(AllowListAction.class, root).addParameter("type", AllowListType.Redirect.name()), TroubleshooterPermission.class); + AdminConsole.addLink(Configuration, "allowed external sources", new ActionURL(AllowListAction.class, root).addParameter("type", AllowListType.Source.name()), TroubleshooterPermission.class); + AdminConsole.addLink(Configuration, "allowed file extensions", new ActionURL(AllowListAction.class, root).addParameter("type", AllowListType.FileExtension.name()), TroubleshooterPermission.class); // Diagnostics AdminConsole.addLink(Diagnostics, "actions", new ActionURL(ActionsAction.class, root)); @@ -10910,25 +10910,25 @@ private static class IgnorePasswordMixIn } @AdminConsoleAction() - public class ExternalHostsAdminAction extends FormViewAction + public class AllowListAction extends FormViewAction { - private ExternalServerType _type; + private AllowListType _type; @Override - public void validateCommand(ExternalHostsForm target, Errors errors) + public void validateCommand(AllowListForm target, Errors errors) { } @Override - public ModelAndView getView(ExternalHostsForm form, boolean reshow, BindException errors) + public ModelAndView getView(AllowListForm form, boolean reshow, BindException errors) { _type = form.getTypeEnum(); - form.setExistingHostsList(form.getTypeEnum().getHosts()); + form.setExistingValuesList(form.getTypeEnum().getValues()); - JspView newView = new JspView<>("/org/labkey/core/admin/addNewExternalHost.jsp", form, errors); + JspView newView = new JspView<>("/org/labkey/core/admin/addNewListValue.jsp", form, errors); newView.setTitle("Register New " + form.getTypeEnum().getTitle()); newView.setFrame(WebPartView.FrameType.PORTAL); - JspView existingView = new JspView<>("/org/labkey/core/admin/existingExternalHosts.jsp", form, errors); + JspView existingView = new JspView<>("/org/labkey/core/admin/existingListValues.jsp", form, errors); existingView.setTitle("Existing " + form.getTypeEnum().getTitle() + "s"); existingView.setFrame(WebPartView.FrameType.PORTAL); @@ -10936,20 +10936,20 @@ public ModelAndView getView(ExternalHostsForm form, boolean reshow, BindExceptio } @Override - public boolean handlePost(ExternalHostsForm form, BindException errors) throws Exception + public boolean handlePost(AllowListForm form, BindException errors) throws Exception { - ExternalServerType hostType = form.getTypeEnum(); - //handle delete of existing external redirect host + AllowListType allowListType = form.getTypeEnum(); + //handle delete of existing value if (form.isDelete()) { - String urlToDelete = form.getExistingExternalHost(); - List hosts = hostType.getHosts(); - for (String externalHost : hosts) + String urlToDelete = form.getExistingValue(); + List values = allowListType.getValues(); + for (String value : values) { - if (null != urlToDelete && urlToDelete.trim().equalsIgnoreCase(externalHost.trim())) + if (null != urlToDelete && urlToDelete.trim().equalsIgnoreCase(value.trim())) { - hosts.remove(externalHost); - hostType.setHosts(hosts, getUser()); + values.remove(value); + allowListType.setValues(values, getUser()); break; } } @@ -10957,27 +10957,27 @@ public boolean handlePost(ExternalHostsForm form, BindException errors) throws E //handle updates - clicking on Save button under Existing will save the updated urls else if (form.isSaveAll()) { - Set validatedHosts = form.validateHostList(errors); + Set validatedValues = form.validateValues(errors); if (errors.hasErrors()) return false; - hostType.setHosts(validatedHosts.stream().toList(), getUser()); + allowListType.setValues(validatedValues.stream().toList(), getUser()); } - //save new external host + //save new external value else if (form.isSaveNew()) { - Set hostSet = form.validateNewExternalHost(errors); + Set valueSet = form.validateNewValue(errors); if (errors.hasErrors()) return false; - hostType.setHosts(hostSet, getUser()); + allowListType.setValues(valueSet, getUser()); } return true; } @Override - public URLHelper getSuccessURL(ExternalHostsForm form) + public URLHelper getSuccessURL(AllowListForm form) { return form.getTypeEnum().getSuccessURL(getContainer()); } @@ -10990,36 +10990,38 @@ public void addNavTrail(NavTree root) } } - public static class ExternalHostsForm + public static class AllowListForm { - private String _newExternalHost; - private String _existingExternalHost; + private String _newValue; + private String _existingValue; private boolean _delete; - private String _existingExternalHosts; + private String _existingValues; private boolean _saveAll; private boolean _saveNew; private String _type; - private List _existingHostURLList; + private List _existingValuesList; - public String getNewExternalHost() + public String getNewValue() { - return _newExternalHost; + return _newValue; } - public void setNewExternalHost(String newExternalHost) + @SuppressWarnings("unused") + public void setNewValue(String newValue) { - _newExternalHost = newExternalHost; + _newValue = newValue; } - public String getExistingExternalHost() + public String getExistingValue() { - return _existingExternalHost; + return _existingValue; } - public void setExistingExternalHost(String existingExternalHost) + @SuppressWarnings("unused") + public void setExistingValue(String existingValue) { - _existingExternalHost = existingExternalHost; + _existingValue = existingValue; } public boolean isDelete() @@ -11027,19 +11029,21 @@ public boolean isDelete() return _delete; } + @SuppressWarnings("unused") public void setDelete(boolean delete) { _delete = delete; } - public String getExistingExternalHosts() + public String getExistingValues() { - return _existingExternalHosts; + return _existingValues; } - public void setExistingExternalHosts(String existingExternalHosts) + @SuppressWarnings("unused") + public void setExistingValues(String existingValues) { - _existingExternalHosts = existingExternalHosts; + _existingValues = existingValues; } public boolean isSaveAll() @@ -11047,6 +11051,7 @@ public boolean isSaveAll() return _saveAll; } + @SuppressWarnings("unused") public void setSaveAll(boolean saveAll) { _saveAll = saveAll; @@ -11057,24 +11062,25 @@ public boolean isSaveNew() return _saveNew; } + @SuppressWarnings("unused") public void setSaveNew(boolean saveNew) { _saveNew = saveNew; } - public List getExistingHostList() + public List getExistingValuesList() { //for updated urls that comes in as String values from the jsp/html form - if (null != getExistingExternalHosts()) + if (null != getExistingValues()) { - return new ArrayList<>(Arrays.asList(getExistingExternalHosts().split("\n"))); + return new ArrayList<>(Arrays.asList(getExistingValues().split("\n"))); } - return _existingHostURLList; + return _existingValuesList; } - public void setExistingHostsList(List urlList) + public void setExistingValuesList(List valuesList) { - _existingHostURLList = urlList; + _existingValuesList = valuesList; } public String getType() @@ -11089,58 +11095,57 @@ public void setType(String type) } @NotNull - public ExternalServerType getTypeEnum() + public AllowListType getTypeEnum() { - return EnumUtils.getEnum(ExternalServerType.class, getType(), ExternalServerType.Redirect); + return EnumUtils.getEnum(AllowListType.class, getType(), AllowListType.Redirect); } @JsonIgnore - public Set validateNewExternalHost(BindException errors) + public Set validateNewValue(BindException errors) { - String newExternalHost = StringUtils.trimToEmpty(getNewExternalHost()); - getTypeEnum().validateHostFormat(newExternalHost, errors); + String value = StringUtils.trimToEmpty(getNewValue()); + getTypeEnum().validateValueFormat(value, errors); if (errors.hasErrors()) return null; - Set hostSet = new CaseInsensitiveHashSet(getTypeEnum().getHosts()); - checkDuplicatesByAddition(newExternalHost, hostSet, errors); - return hostSet; + Set valueSet = new CaseInsensitiveHashSet(getTypeEnum().getValues()); + checkDuplicatesByAddition(value, valueSet, errors); + return valueSet; } @JsonIgnore - public Set validateHostList(BindException errors) + public Set validateValues(BindException errors) { - List hosts = getExistingHostList(); //get hosts from the form, this includes updated hosts - Set hostSet = new CaseInsensitiveHashSet(); + List values = getExistingValuesList(); //get values from the form, this includes updated values + Set valueSet = new CaseInsensitiveHashSet(); - if (null != hosts && !hosts.isEmpty()) + if (null != values && !values.isEmpty()) { - for (String host : hosts) + for (String value : values) { - getTypeEnum().validateHostFormat(host, errors); + getTypeEnum().validateValueFormat(value, errors); if (errors.hasErrors()) continue; - checkDuplicatesByAddition(host, hostSet, errors); + checkDuplicatesByAddition(value, valueSet, errors); } } - return hostSet; + return valueSet; } /** - * Adds host to host set unless it is a duplicate, in which case it adds an error - * Note: Attempts to validate host string using URLHelper - * @param host to check - * @param hostSet of existing hosts + * Adds value to value set unless it is a duplicate, in which case it adds an error + * @param value to check + * @param valueSet of existing values * @param errors collections of errors observed */ @JsonIgnore - private void checkDuplicatesByAddition(String host, Set hostSet, BindException errors) + private void checkDuplicatesByAddition(String value, Set valueSet, BindException errors) { - String trimHost = StringUtils.trimToEmpty(host); - if (!hostSet.add(trimHost)) - errors.addError(new LabKeyError(String.format("'%1$s' already exists. Duplicate hosts not allowed.", trimHost))); + String trimValue = StringUtils.trimToEmpty(value); + if (!valueSet.add(trimValue)) + errors.addError(new LabKeyError(String.format("'%1$s' already exists. Duplicate values not allowed.", trimValue))); } } diff --git a/core/src/org/labkey/core/admin/ExternalServerType.java b/core/src/org/labkey/core/admin/AllowListType.java similarity index 77% rename from core/src/org/labkey/core/admin/ExternalServerType.java rename to core/src/org/labkey/core/admin/AllowListType.java index d7e4ed5258a..df9b99a2611 100644 --- a/core/src/org/labkey/core/admin/ExternalServerType.java +++ b/core/src/org/labkey/core/admin/AllowListType.java @@ -18,7 +18,7 @@ import java.util.Collection; import java.util.List; -public enum ExternalServerType +public enum AllowListType { Source { @Override @@ -39,13 +39,13 @@ public HtmlString getDescription() } @Override - public List getHosts() + public List getValues() { return AppProps.getInstance().getExternalSourceHosts(); } @Override - public void setHosts(Collection hosts, User user) + public void setValues(Collection hosts, User user) { WriteableAppProps props = AppProps.getWriteableInstance(); props.setExternalSourceHosts(hosts); @@ -53,7 +53,13 @@ public void setHosts(Collection hosts, User user) // Refresh the CSP with new values. ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Connection, EXTERNAL_SOURCE_HOSTS_KEY); - ContentSecurityPolicyFilter.registerAllowedSources(Directive.Connection, EXTERNAL_SOURCE_HOSTS_KEY, getHosts().toArray(new String[0])); + ContentSecurityPolicyFilter.registerAllowedSources(Directive.Connection, EXTERNAL_SOURCE_HOSTS_KEY, getValues().toArray(new String[0])); + } + + @Override + public void validateValueFormat(String value, BindException errors) + { + } @Override @@ -69,6 +75,8 @@ public HtmlString getLabel() } }, Redirect { + private static final AuthorityValidator AUTHORITY_VALIDATOR = new AuthorityValidator(UrlValidator.ALLOW_LOCAL_URLS); + @Override public HtmlString getDescription() { @@ -90,19 +98,33 @@ public HtmlString getDescription() } @Override - public List getHosts() + public List getValues() { return AppProps.getInstance().getExternalRedirectHosts(); } @Override - public void setHosts(Collection hosts, User user) + public void setValues(Collection hosts, User user) { WriteableAppProps props = AppProps.getWriteableInstance(); props.setExternalRedirectHosts(hosts); props.save(user); } + @Override + @JsonIgnore + public void validateValueFormat(String host, BindException errors) + { + if (StringUtils.isEmpty(host)) + { + errors.addError(new LabKeyError("Redirect host name must not be blank.")); + } + else if (!AUTHORITY_VALIDATOR.isValidAuthority(host)) + { + errors.addError(new LabKeyError(String.format("Redirect host name %1$s is not formatted correctly", host))); + } + } + @Override public HtmlString getTitle() { @@ -132,13 +154,13 @@ public HtmlString getDescription() } @Override - public List getHosts() + public List getValues() { return AppProps.getInstance().getAllowedExtensions(); } @Override - public void setHosts(Collection allowedExtensions, User user) + public void setValues(Collection allowedExtensions, User user) { WriteableAppProps props = AppProps.getWriteableInstance(); props.setAllowedFileExtensions(allowedExtensions); @@ -146,11 +168,12 @@ public void setHosts(Collection allowedExtensions, User user) } @Override - public void validateHostFormat(String externalHost, BindException errors) + @JsonIgnore + public void validateValueFormat(String value, BindException errors) { - if (StringUtils.isEmpty(externalHost)) + if (StringUtils.isEmpty(value)) errors.addError(new LabKeyError("File extension must not be blank.")); - else if (!externalHost.startsWith(".")) + else if (!value.startsWith(".")) errors.addError(new LabKeyError("File extension must start with a '.'")); } @@ -167,7 +190,6 @@ public HtmlString getLabel() } }; - private static final AuthorityValidator AUTHORITY_VALIDATOR = new AuthorityValidator(UrlValidator.ALLOW_LOCAL_URLS); private static final String EXTERNAL_SOURCE_HOSTS_KEY = "External Sources"; public static String getExternalSourceHostsKey() { @@ -175,12 +197,12 @@ public static String getExternalSourceHostsKey() } public abstract HtmlString getDescription(); - public abstract List getHosts(); - public abstract void setHosts(Collection redirectHosts, User user); + public abstract List getValues(); + public abstract void setValues(Collection redirectHosts, User user); + public abstract void validateValueFormat(String value, BindException errors); public abstract HtmlString getTitle(); public abstract HtmlString getLabel(); - public String getHelpTopic() { return "externalHosts#" + name().toLowerCase(); @@ -188,34 +210,6 @@ public String getHelpTopic() public URLHelper getSuccessURL(Container container) { - return new ActionURL(AdminController.ExternalHostsAdminAction .class, container).addParameter("type", name()); + return new ActionURL(AdminController.AllowListAction.class, container).addParameter("type", name()); } - - @JsonIgnore - public void validateHostFormat(String externalHost, BindException errors) - { - if (StringUtils.isEmpty(externalHost)) - { - errors.addError(new LabKeyError("External host name must not be blank.")); - } - else if (!AUTHORITY_VALIDATOR.isValidAuthority(externalHost)) - { - errors.addError(new LabKeyError(String.format("External host name %1$s is not formatted correctly", externalHost))); - } - } - - private static class AuthorityValidator extends UrlValidator - { - public AuthorityValidator(long options) - { - super(options); - } - - @Override - public boolean isValidAuthority(String authority) - { - String base = authority.startsWith("*.") ? authority.substring(2) : authority; - return super.isValidAuthority(base); - } - }; } diff --git a/core/src/org/labkey/core/admin/AuthorityValidator.java b/core/src/org/labkey/core/admin/AuthorityValidator.java new file mode 100644 index 00000000000..3a18d4f3bc8 --- /dev/null +++ b/core/src/org/labkey/core/admin/AuthorityValidator.java @@ -0,0 +1,18 @@ +package org.labkey.core.admin; + +import org.apache.commons.validator.routines.UrlValidator; + +class AuthorityValidator extends UrlValidator +{ + public AuthorityValidator(long options) + { + super(options); + } + + @Override + public boolean isValidAuthority(String authority) + { + String base = authority.startsWith("*.") ? authority.substring(2) : authority; + return super.isValidAuthority(base); + } +} diff --git a/core/src/org/labkey/core/admin/addNewExternalHost.jsp b/core/src/org/labkey/core/admin/addNewListValue.jsp similarity index 80% rename from core/src/org/labkey/core/admin/addNewExternalHost.jsp rename to core/src/org/labkey/core/admin/addNewListValue.jsp index 5e76e16ece2..3a2d11dbbdd 100644 --- a/core/src/org/labkey/core/admin/addNewExternalHost.jsp +++ b/core/src/org/labkey/core/admin/addNewListValue.jsp @@ -1,8 +1,3 @@ -<%@ page import="org.labkey.api.admin.AdminUrls" %> -<%@ page import="org.labkey.api.data.Container" %> -<%@ page import="org.labkey.api.security.permissions.AdminOperationsPermission" %> -<%@ page import="org.labkey.core.admin.AdminController" %> -<%@ page import="org.labkey.api.view.HttpView" %> <% /* * Copyright (c) 2019 LabKey Corporation @@ -20,12 +15,17 @@ * limitations under the License. */ %> +<%@ page import="org.labkey.api.admin.AdminUrls" %> +<%@ page import="org.labkey.api.data.Container" %> +<%@ page import="org.labkey.api.security.permissions.AdminOperationsPermission" %> +<%@ page import="org.labkey.api.view.HttpView" %> +<%@ page import="org.labkey.core.admin.AdminController.AllowListForm" %> <%@ page extends="org.labkey.api.jsp.JspBase" %> <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <% Container c = getContainer(); boolean isTroubleshooter = c.isRoot() && !c.hasPermission(getUser(), AdminOperationsPermission.class); - AdminController.ExternalHostsForm bean = (AdminController.ExternalHostsForm) HttpView.currentModel(); + AllowListForm bean = (AllowListForm) HttpView.currentModel(); %> <%=bean.getTypeEnum().getDescription()%> @@ -43,8 +43,8 @@ - - + + diff --git a/core/src/org/labkey/core/admin/existingExternalHosts.jsp b/core/src/org/labkey/core/admin/existingListValues.jsp similarity index 67% rename from core/src/org/labkey/core/admin/existingExternalHosts.jsp rename to core/src/org/labkey/core/admin/existingListValues.jsp index bf2275da22d..24bb0829374 100644 --- a/core/src/org/labkey/core/admin/existingExternalHosts.jsp +++ b/core/src/org/labkey/core/admin/existingListValues.jsp @@ -20,7 +20,7 @@ <%@ page import="org.labkey.api.security.permissions.AdminOperationsPermission" %> <%@ page import="org.labkey.api.util.HtmlString" %> <%@ page import="org.labkey.api.view.HttpView" %> -<%@ page import="org.labkey.core.admin.AdminController" %> +<%@ page import="org.labkey.core.admin.AdminController.AllowListForm" %> <%@ page extends="org.labkey.api.jsp.JspBase" %> <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <% @@ -29,58 +29,58 @@ %> - + <% - AdminController.ExternalHostsForm bean = (AdminController.ExternalHostsForm) HttpView.currentModel(); + AllowListForm bean = (AllowListForm) HttpView.currentModel(); %>

<%= button("Save").submit(true) %>
- <% if (bean.getExistingHostList().isEmpty()) { %> + <% if (bean.getExistingValuesList().isEmpty()) { %> <% } %> <% int num = 1; - for (String externalHost : bean.getExistingHostList()) { - String inputNameExisting = "existingExternalHost" + num; + for (String value : bean.getExistingValuesList()) { + String inputNameExisting = "existingValue" + num; %> - + - @@ -89,10 +89,10 @@ } %>
<%=h(bean.getTypeEnum().getTitle() + "s")%>
No <%=h(bean.getTypeEnum().getTitle())%>s have been configured.
<%=isTroubleshooter ? HtmlString.EMPTY_STRING : button("Delete").primary(true).onClick("return deleteExisting(\"" + h(externalHost) + "\");") %> + <%=isTroubleshooter ? HtmlString.EMPTY_STRING : button("Delete").primary(true).onClick("return deleteExisting(\"" + h(value) + "\");") %>
- <% if (!bean.getExistingHostList().isEmpty()) { %> + <% if (!bean.getExistingValuesList().isEmpty()) { %> - - + +
<%=isTroubleshooter ? button("Done").href(urlProvider(AdminUrls.class).getAdminConsoleURL()) : button("Save").primary(true).onClick("return saveAll();")%> From 83aa1cb583579eb26a3b4094f4ab811a0a29237b Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 21 Feb 2025 12:04:22 -0800 Subject: [PATCH 02/23] Reduce per-request work for CSP filter to a single substitution. Improve junit test to interrogate the policy itself, not just related data structures. --- .../filters/ContentSecurityPolicyFilter.java | 296 +++++++++++------- 1 file changed, 190 insertions(+), 106 deletions(-) diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index 1879f54c690..3162229d24f 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -10,6 +10,7 @@ import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.collections4.SetValuedMap; import org.apache.commons.collections4.multimap.HashSetValuedHashMap; +import org.apache.commons.lang3.StringUtils; import org.junit.Assert; import org.junit.Test; import org.labkey.api.security.Directive; @@ -28,8 +29,10 @@ import java.util.Collections; import java.util.Enumeration; import java.util.HashMap; +import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.CopyOnWriteArrayList; import java.util.stream.Collectors; @@ -43,14 +46,22 @@ 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 HEADER_NONCE = "org.labkey.filters.ContentSecurityPolicyFilter#NONCE"; // needs to match PageConfig.HEADER_NONCE + + private static final List CSP_FILTERS = new CopyOnWriteArrayList<>(); + + // Lock that protects the static data structures below private static final Object ALLOWED_SOURCES_LOCK = new Object(); private static final Map> ALLOWED_SOURCES = new HashMap<>(); - - private static Map _substitutionMap = Collections.emptyMap(); + // Regenerate and stash on every "allowed source" change as a convenience (so every filter don't need to recalculate + // it on every init() and change) + private static Map ALLOWED_SOURCES_SUBSTITUTION_MAP = Collections.emptyMap(); // Per-filter-instance parameters that are set in init() and never changed - private StringExpression policyExpression = null; - private ContentSecurityPolicyType type = ContentSecurityPolicyType.Enforce; + private ContentSecurityPolicyType _type = ContentSecurityPolicyType.Enforce; + private String _policyTemplate = null; + + // Updated after every change to "allowed sources" + private StringExpression _policyExpression = null; public enum ContentSecurityPolicyType { @@ -98,7 +109,7 @@ public void init(FilterConfig filterConfig) throws ServletException s = StringExpressionFactory.create(s, false, NullValueBehavior.KeepSubstitution) .eval(Map.of(REPORT_PARAMETER_SUBSTITUTION, "labkeyVersion=" + PageFlowUtil.encodeURIComponent(AppProps.getInstance().getReleaseVersion()))); - policyExpression = StringExpressionFactory.create(s, false, NullValueBehavior.ReplaceNullAndMissingWithBlank); + _policyTemplate = s; } else if ("disposition".equalsIgnoreCase(paramName)) { @@ -106,13 +117,31 @@ else if ("disposition".equalsIgnoreCase(paramName)) if (!"report".equalsIgnoreCase(s) && !"enforce".equalsIgnoreCase(s)) throw new ServletException("ContentSecurityPolicyFilter is misconfigured, unexpected disposition value: " + s); if ("report".equalsIgnoreCase(s)) - type = ContentSecurityPolicyType.Report; + _type = ContentSecurityPolicyType.Report; } else { throw new ServletException("ContentSecurityPolicyFilter is misconfigured, unexpected parameter name: " + paramName); } } + + regeneratePolicyExpression(); + CSP_FILTERS.add(this); + } + + // Make all the "allowed sources" substitutions at init() and whenever the allowed sources map changes. With this, + // the only substitution needed on a per-request basis is the nonce value. + private void regeneratePolicyExpression() + { + final String allowSubstitutedPolicy; + + synchronized (ALLOWED_SOURCES_LOCK) + { + allowSubstitutedPolicy = StringExpressionFactory.create(_policyTemplate, false, NullValueBehavior.KeepSubstitution) + .eval(ALLOWED_SOURCES_SUBSTITUTION_MAP); + } + + _policyExpression = StringExpressionFactory.create(allowSubstitutedPolicy, false, NullValueBehavior.KeepSubstitution); } /** Filter out block comments and replace special characters in the provided policy */ @@ -139,14 +168,13 @@ public static String filterPolicy(String policy) @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { - if (request instanceof HttpServletRequest req && response instanceof HttpServletResponse resp && null != policyExpression) + if (request instanceof HttpServletRequest req && response instanceof HttpServletResponse resp && null != _policyExpression) { - if (type != ContentSecurityPolicyType.Enforce || !OptionalFeatureService.get().isFeatureEnabled(FEATURE_FLAG_DISABLE_ENFORCE_CSP)) + if (_type != ContentSecurityPolicyType.Enforce || !OptionalFeatureService.get().isFeatureEnabled(FEATURE_FLAG_DISABLE_ENFORCE_CSP)) { - Map map = new HashMap<>(_substitutionMap); - map.put(NONCE_SUBST, getScriptNonceHeader(req)); - var csp = policyExpression.eval(map); - resp.setHeader(type.getHeaderName(), csp); + Map map = Map.of(NONCE_SUBST, getScriptNonceHeader(req)); + var csp = _policyExpression.eval(map); + resp.setHeader(_type.getHeaderName(), csp); } } chain.doFilter(request, response); @@ -201,19 +229,30 @@ public static void unregisterAllowedSources(Directive directive, String key) // Pre-generate the substitution map on every register/unregister private static void regenerateSubstitutionMap() { - _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())); + synchronized (ALLOWED_SOURCES_LOCK) + { + ALLOWED_SOURCES_SUBSTITUTION_MAP = 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(" "))) + ); + + // 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(), "")); + + // 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())); + + // Tell each registered ContentSecurityPolicyFilter to refresh its policy template based on the new substitution map + CSP_FILTERS.forEach(ContentSecurityPolicyFilter::regeneratePolicyExpression); + } } public static class TestCase extends Assert @@ -222,33 +261,33 @@ public static class TestCase extends Assert public void testPolicyFiltering() { String fakePolicyForTesting = """ - /* Beginning of line comment should be removed */default-src\t'self' https: http: ; - connect-src 'self' http://www.labkey.org /* this is a mistake! */ localhost:* ws: ${LABKEY.ALLOWED.CONNECTIONS} ; - object-src https://* ‘none’ ; /* Hard to see, but there are curly quotes surrounding "none" on this line */\r - style-src 'self'\rhttps: 'unsafe-inline' ; - img-src 'self'\thttps: data: ; - font-src 'self' http://www.labkey.com https://* http: /* I don't know why we're doing this! */ https: data: ; - script-src 'unsafe-eval' 'strict-dynamic' 'nonce-${REQUEST.SCRIPT.NONCE}' ; - base-uri 'self' ; /* what in the world?! */ - frame-ancestors 'self' ; /* This here comment spans - multiple lines - for testing purposes - */ - report-uri /* Whoa! */ /admin-contentsecuritypolicyreport.api?${CSP.REPORT.PARAMS} https://*; - """; + /* Beginning of line comment should be removed */default-src\t'self' https: http: ; + connect-src 'self' http://www.labkey.org /* this is a mistake! */ localhost:* ws: ${LABKEY.ALLOWED.CONNECTIONS} ; + object-src https://* ‘none’ ; /* Hard to see, but there are curly quotes surrounding "none" on this line */\r + style-src 'self'\rhttps: 'unsafe-inline' ; + img-src 'self'\thttps: data: ; + font-src 'self' http://www.labkey.com https://* http: /* I don't know why we're doing this! */ https: data: ; + script-src 'unsafe-eval' 'strict-dynamic' 'nonce-${REQUEST.SCRIPT.NONCE}' ; + base-uri 'self' ; /* what in the world?! */ + frame-ancestors 'self' ; /* This here comment spans + multiple lines + for testing purposes + */ + report-uri /* Whoa! */ /admin-contentsecuritypolicyreport.api?${CSP.REPORT.PARAMS} https://*; + """; // Multi-line for readability, but notice that newlines are replaced before assignment String expected = """ - default-src 'self' https: http: ; - connect-src 'self' http://www.labkey.org localhost:* ws: ${LABKEY.ALLOWED.CONNECTIONS} ; - object-src https://* 'none' ; - style-src 'self' https: 'unsafe-inline' ; - img-src 'self' https: data: ; - font-src 'self' http://www.labkey.com https://* http: https: data: ; - script-src 'unsafe-eval' 'strict-dynamic' 'nonce-${REQUEST.SCRIPT.NONCE}' ; - base-uri 'self' ; - frame-ancestors 'self' ; - report-uri /admin-contentsecuritypolicyreport.api?${CSP.REPORT.PARAMS} https://*;""".replace('\n', ' '); + default-src 'self' https: http: ; + connect-src 'self' http://www.labkey.org localhost:* ws: ${LABKEY.ALLOWED.CONNECTIONS} ; + object-src https://* 'none' ; + style-src 'self' https: 'unsafe-inline' ; + img-src 'self' https: data: ; + font-src 'self' http://www.labkey.com https://* http: https: data: ; + script-src 'unsafe-eval' 'strict-dynamic' 'nonce-${REQUEST.SCRIPT.NONCE}' ; + base-uri 'self' ; + frame-ancestors 'self' ; + report-uri /admin-contentsecuritypolicyreport.api?${CSP.REPORT.PARAMS} https://*;""".replace('\n', ' '); Assert.assertEquals(expected, filterPolicy(fakePolicyForTesting)); } @@ -256,74 +295,119 @@ public void testPolicyFiltering() @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() + // 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(); + Map> savedSources = ALLOWED_SOURCES.entrySet().stream() .collect(Collectors.toMap(Map.Entry::getKey, e -> new HashSetValuedHashMap<>(e.getValue()))); - ALLOWED_SOURCES.clear(); - regenerateSubstitutionMap(); - } - try - { - 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()); - - registerAllowedSources(Directive.Image, "image", "ImageSource", "BetterImageStore"); - assertEquals(5, ALLOWED_SOURCES.size()); - assertEquals(5, _substitutionMap.size()); - } - finally - { - // Restore the previous ALLOWED_SOURCES - synchronized (ALLOWED_SOURCES_LOCK) + try { + ALLOWED_SOURCES.clear(); + regenerateSubstitutionMap(); + + // Initial checks + assertTrue(ALLOWED_SOURCES.isEmpty()); + verifySubstitutionMapSize(0); + // All "allowed sources" substitutions should be replaced with empty strings + verifySubstitutionInPolicyExpressions(".SOURCES}", 0); + // Should have been substitution in init() + verifySubstitutionInPolicyExpressions("${CSP.REPORT.PARAMS}", 0); + // A single substitution parameter (${REQUEST.SCRIPT.NONCE}) should remain + verifySubstitutionInPolicyExpressions("${REQUEST.SCRIPT.NONCE}", 1); + verifySubstitutionInPolicyExpressions("${", 1); + + // Now unregister and register sources for each Directive, testing expectations along the way + unregisterAllowedSources(Directive.Connection, "foo"); + assertTrue(ALLOWED_SOURCES.isEmpty()); + verifySubstitutionMapSize(0); + registerAllowedSources(Directive.Connection, "foo", "MySource"); + assertEquals(1, ALLOWED_SOURCES.size()); + verifySubstitutionMapSize(2); // Old connection substitution key should be added as well + verifySubstitutionInPolicyExpressions("MySource", 1); + registerAllowedSources(Directive.Connection, "bar", "MySource"); + assertEquals(1, ALLOWED_SOURCES.size()); + verifySubstitutionMapSize(2); + verifySubstitutionInPolicyExpressions("MySource", 1); // Duplicate source should be filtered out + + unregisterAllowedSources(Directive.Font, "font"); + registerAllowedSources(Directive.Font, "font", "MySource"); + assertEquals(2, ALLOWED_SOURCES.size()); + verifySubstitutionMapSize(3); + verifySubstitutionInPolicyExpressions("MySource", 2); + registerAllowedSources(Directive.Font, "font2", "MyFontSource"); + assertEquals(2, ALLOWED_SOURCES.size()); + verifySubstitutionMapSize(3); + verifySubstitutionInPolicyExpressions("MySource", 2); + verifySubstitutionInPolicyExpressions("MyFontSource", 1); + unregisterAllowedSources(Directive.Font, "font2"); + assertEquals(2, ALLOWED_SOURCES.size()); + verifySubstitutionMapSize(3); + 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 + verifySubstitutionInPolicyExpressions("MySource", 1); + verifySubstitutionInPolicyExpressions("MyFontSource", 0); + + unregisterAllowedSources(Directive.Frame, "frame"); + registerAllowedSources(Directive.Frame, "frame", "FrameSource", "FrameStore"); + assertEquals(3, ALLOWED_SOURCES.size()); + verifySubstitutionMapSize(3); + verifySubstitutionInPolicyExpressions("FrameSource", 1); + verifySubstitutionInPolicyExpressions("FrameStore", 1); + + unregisterAllowedSources(Directive.Style, "style"); + registerAllowedSources(Directive.Style, "style", "StyleSource", "MoreStylishStore"); + assertEquals(4, ALLOWED_SOURCES.size()); + verifySubstitutionMapSize(4); + verifySubstitutionInPolicyExpressions("StyleSource", 1); + verifySubstitutionInPolicyExpressions("MoreStylishStore", 1); + + unregisterAllowedSources(Directive.Image, "image"); + registerAllowedSources(Directive.Image, "image", "ImageSource", "BetterImageStore"); + assertEquals(5, ALLOWED_SOURCES.size()); + verifySubstitutionMapSize(5); + verifySubstitutionInPolicyExpressions("ImageSource", 1); + verifySubstitutionInPolicyExpressions("BetterImageStore", 1); + } + finally + { + // Restore the previous ALLOWED_SOURCES ALLOWED_SOURCES.clear(); ALLOWED_SOURCES.putAll(savedSources); regenerateSubstitutionMap(); assertEquals(sourceMapSize, ALLOWED_SOURCES.size()); - assertEquals(substitutionMapSize, _substitutionMap.size()); + assertEquals(substitutionMapSize, ALLOWED_SOURCES_SUBSTITUTION_MAP.size()); } } } + + private void verifySubstitutionInPolicyExpressions(String value, int expectedCount) + { + List failures = CSP_FILTERS.stream() + .map(filter -> filter._policyExpression.eval(Map.of())) + .filter(policy -> StringUtils.countMatches(policy, value) != expectedCount) + .toList(); + + if (!failures.isEmpty()) + { + fail("Occurrences of value \"" + value + "\" was not the expected count (" + expectedCount + ") in policies [\"" + String.join("\", \"", failures) + "\"]"); + } + } + + 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 + + assertEquals(expectedSubstitutionMapSize, ALLOWED_SOURCES_SUBSTITUTION_MAP.size()); + long nonEmptyValues = ALLOWED_SOURCES_SUBSTITUTION_MAP.entrySet().stream().filter(e -> !e.getValue().isEmpty()).count(); + assertEquals(expectedNonEmptyValues, nonEmptyValues); + } } } From 32d43fa976f63fd80b3118352bb3ce4629140e61 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 21 Feb 2025 16:24:41 -0800 Subject: [PATCH 03/23] Checkpoint - persistence, initialization, and add new hosts --- api/src/org/labkey/api/settings/AppProps.java | 6 +- .../org/labkey/api/settings/AppPropsImpl.java | 28 ++- .../api/settings/RandomStartupProperties.java | 8 - .../api/settings/WriteableAppProps.java | 10 +- .../filters/ContentSecurityPolicyFilter.java | 11 +- core/src/org/labkey/core/CoreModule.java | 14 +- .../labkey/core/admin/AdminController.java | 233 +++++++++++++++--- .../org/labkey/core/admin/AllowListType.java | 8 - .../org/labkey/core/admin/externalSources.jsp | 69 ++++++ .../AllowedExternalResourceHosts.java | 83 +++++++ 10 files changed, 385 insertions(+), 85 deletions(-) create mode 100644 core/src/org/labkey/core/admin/externalSources.jsp create mode 100644 core/src/org/labkey/core/security/AllowedExternalResourceHosts.java diff --git a/api/src/org/labkey/api/settings/AppProps.java b/api/src/org/labkey/api/settings/AppProps.java index 485cba78e88..cfdac77443f 100644 --- a/api/src/org/labkey/api/settings/AppProps.java +++ b/api/src/org/labkey/api/settings/AppProps.java @@ -47,6 +47,7 @@ public interface AppProps String EXPERIMENTAL_BLOCKER = "blockMaliciousClients"; String EXPERIMENTAL_RESOLVE_PROPERTY_URI_COLUMNS = "resolve-property-uri-columns"; String DEPRECATED_OBJECT_LEVEL_DISCUSSIONS = "deprecatedObjectLevelDiscussions"; + String ALLOWED_EXTERNAL_RESOURCES = "allowedExternalResources"; String UNKNOWN_VERSION = "Unknown Release Version"; @@ -237,16 +238,15 @@ static WriteableAppProps getWriteableInstance() boolean isIncludeServerHttpHeader(); /** - * * @return List of configured external redirect hosts */ @NotNull List getExternalRedirectHosts(); /** - * * @return List of configured external resource hosts */ + @Deprecated // Left for backward compatibility @NotNull List getExternalSourceHosts(); @@ -259,4 +259,6 @@ static WriteableAppProps getWriteableInstance() @NotNull Set getDistributionSupportedDatabases(); @NotNull List getAllowedExtensions(); + + @NotNull String getAllowedExternalResources(); } diff --git a/api/src/org/labkey/api/settings/AppPropsImpl.java b/api/src/org/labkey/api/settings/AppPropsImpl.java index 8768a2a912e..e69c1989c02 100644 --- a/api/src/org/labkey/api/settings/AppPropsImpl.java +++ b/api/src/org/labkey/api/settings/AppPropsImpl.java @@ -86,6 +86,8 @@ class AppPropsImpl extends AbstractWriteableSettingsGroup implements AppProps private static final String DISTRIBUTION_FILENAME; private static final Set DISTRIBUTION_SUPPORTED_DATABASES; + public static final String ALLOWED_EXTERNAL_RESOURCES = "allowedExternalResources"; + private static final Logger LOG = LogHelper.getLogger(AppPropsImpl.class, "Site settings startup properties"); static @@ -667,13 +669,6 @@ public List getExternalRedirectHosts() return getExternalHosts(externalRedirectHostURLs); } - @Override - @NotNull - public List getExternalSourceHosts() - { - return getExternalHosts(externalSourceHostURLs); - } - @Override @NotNull public List getAllowedExtensions() @@ -732,4 +727,23 @@ public Map getStashedStartupProp { return DISTRIBUTION_SUPPORTED_DATABASES; } + + @Deprecated + @Override + @NotNull + public List getExternalSourceHosts() + { + String urls = lookupStringValue("externalSourceHostURLs", ""); + if (StringUtils.isNotBlank(urls)) + { + return new ArrayList<>(Arrays.asList(urls.split(EXTERNAL_HOST_DELIMITER))); + } + return new ArrayList<>(); + } + + @Override + public @NotNull String getAllowedExternalResources() + { + return lookupStringValue(ALLOWED_EXTERNAL_RESOURCES, "[]"); + } } diff --git a/api/src/org/labkey/api/settings/RandomStartupProperties.java b/api/src/org/labkey/api/settings/RandomStartupProperties.java index 5af8d7b4da7..0363a54c538 100644 --- a/api/src/org/labkey/api/settings/RandomStartupProperties.java +++ b/api/src/org/labkey/api/settings/RandomStartupProperties.java @@ -28,14 +28,6 @@ public void setValue(WriteableAppProps writeable, String value) writeable.setExternalRedirectHosts(Arrays.asList(StringUtils.split(value, AppPropsImpl.EXTERNAL_HOST_DELIMITER))); } }, - externalSourceHostURLs("Allowed external source hosts") - { - @Override - public void setValue(WriteableAppProps writeable, String value) - { - writeable.setExternalSourceHosts(Arrays.asList(StringUtils.split(value, AppPropsImpl.EXTERNAL_HOST_DELIMITER))); - } - }, allowedFileExtensions("Allowed file extensions") { @Override diff --git a/api/src/org/labkey/api/settings/WriteableAppProps.java b/api/src/org/labkey/api/settings/WriteableAppProps.java index 83051137173..0b5e4e8ae8d 100644 --- a/api/src/org/labkey/api/settings/WriteableAppProps.java +++ b/api/src/org/labkey/api/settings/WriteableAppProps.java @@ -232,11 +232,6 @@ public void setExternalRedirectHosts(@NotNull Collection externalRedirec setExternalHosts(externalRedirectHostURLs, externalRedirectHosts); } - public void setExternalSourceHosts(@NotNull Collection externalSourceHosts) - { - setExternalHosts(externalSourceHostURLs, externalSourceHosts); - } - private void setExternalHosts(RandomStartupProperties propName, @NotNull Collection externalSourceHosts) { storeStringValue(propName, String.join(EXTERNAL_HOST_DELIMITER, externalSourceHosts)); @@ -247,4 +242,9 @@ public void setAllowedFileExtensions(Collection allowedFileExtensions) setExternalHosts(RandomStartupProperties.allowedFileExtensions, allowedFileExtensions); FileUtil.setExtensionChecker(AppProps.getInstance()); } + + public void setAllowedExternalSources(String jsonArray) + { + storeStringValue(ALLOWED_EXTERNAL_RESOURCES, jsonArray); + } } diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index 3162229d24f..dfbbe774b5d 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -11,6 +11,7 @@ import org.apache.commons.collections4.SetValuedMap; import org.apache.commons.collections4.multimap.HashSetValuedHashMap; import org.apache.commons.lang3.StringUtils; +import org.apache.logging.log4j.Logger; import org.junit.Assert; import org.junit.Test; import org.labkey.api.security.Directive; @@ -63,6 +64,8 @@ public class ContentSecurityPolicyFilter implements Filter // Updated after every change to "allowed sources" private StringExpression _policyExpression = null; + private static final Logger LOG = LogHelper.getLogger(ContentSecurityPolicyFilter.class, "Register/unregister allowed resource hosts"); + public enum ContentSecurityPolicyType { Report("Content-Security-Policy-Report-Only"), Enforce("Content-Security-Policy"); @@ -195,16 +198,11 @@ 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) - { - registerAllowedSources(Directive.Connection, key, allowedUrls); - } - public static void registerAllowedSources(Directive directive, String key, String... allowedSources) { synchronized (ALLOWED_SOURCES_LOCK) { + LOG.debug("Registering {} for {}: {}", directive, key, Arrays.toString(allowedSources)); SetValuedMap multiMap = ALLOWED_SOURCES.computeIfAbsent(directive, d -> new HashSetValuedHashMap<>()); Arrays.stream(allowedSources).forEach(s -> multiMap.put(key, s)); regenerateSubstitutionMap(); @@ -215,6 +213,7 @@ public static void unregisterAllowedSources(Directive directive, String key) { synchronized (ALLOWED_SOURCES_LOCK) { + LOG.debug("Unregistering {} for {}", directive, key); SetValuedMap multiMap = ALLOWED_SOURCES.get(directive); if (multiMap != null) { diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 5941903d163..9ca0ddb1344 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -213,7 +213,6 @@ import org.labkey.core.admin.CustomizeMenuForm; import org.labkey.core.admin.DisplayFormatAnalyzer; import org.labkey.core.admin.DisplayFormatValidationProviderFactory; -import org.labkey.core.admin.AllowListType; import org.labkey.core.admin.FilesSiteSettingsAction; import org.labkey.core.admin.MenuViewFactory; import org.labkey.core.admin.importer.FolderTypeImporterFactory; @@ -276,6 +275,7 @@ import org.labkey.core.reports.DocumentConversionServiceImpl; import org.labkey.core.reports.ScriptEngineManagerImpl; import org.labkey.core.script.RhinoService; +import org.labkey.core.security.AllowedExternalResourceHosts; import org.labkey.core.security.ApiKeyViewProvider; import org.labkey.core.security.SecurityApiActions; import org.labkey.core.security.SecurityController; @@ -1050,7 +1050,7 @@ public void shutdownStarted() // populate look and feel settings and site settings with values read from startup properties as appropriate for not bootstrap populateLookAndFeelResourcesWithStartupProps(); - registerAllowedConnectionSources(); + AllowedExternalResourceHosts.registerHosts(); WriteableLookAndFeelProperties.populateLookAndFeelWithStartupProps(); WriteableAppProps.populateSiteSettingsWithStartupProps(); // create users and groups and assign roles with values read from startup properties as appropriate for not bootstrap @@ -1241,16 +1241,6 @@ public void moduleStartupComplete(ServletContext servletContext) UserManager.addUserListener(new EmailPreferenceUserListener()); } - private void registerAllowedConnectionSources() - { - ContentSecurityPolicyFilter.registerAllowedConnectionSource( - AllowListType.getExternalSourceHostsKey(), - AllowListType.Source.getValues().toArray(new String[0]) - ); - - LOG.debug("Registered [{}] as an allowed connection source", () -> String.join(", ", AppProps.getInstance().getExternalSourceHosts())); - } - // Issue 7527: Auto-detect missing sql views and attempt to recreate private void checkForMissingDbViews() { diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index da1b31bc8a8..c3d77ad7f64 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -17,6 +17,7 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonIgnoreProperties; +import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; import com.google.common.base.Joiner; import com.google.common.util.concurrent.UncheckedExecutionException; @@ -181,6 +182,7 @@ import org.labkey.api.security.ActionNames; import org.labkey.api.security.AdminConsoleAction; import org.labkey.api.security.CSRF; +import org.labkey.api.security.Directive; import org.labkey.api.security.Group; import org.labkey.api.security.GroupManager; import org.labkey.api.security.IgnoresTermsOfUse; @@ -317,6 +319,8 @@ import org.labkey.core.query.CoreQuerySchema; import org.labkey.core.query.PostgresUserSchema; import org.labkey.core.reports.ExternalScriptEngineDefinitionImpl; +import org.labkey.core.security.AllowedExternalResourceHosts; +import org.labkey.core.security.AllowedExternalResourceHosts.Substitution; import org.labkey.core.security.BlockListFilter; import org.labkey.core.security.SecurityController; import org.labkey.data.xml.TablesDocument; @@ -453,7 +457,7 @@ public static void registerAdminConsoleLinks() AdminConsole.addLink(Configuration, "site settings", new AdminUrlsImpl().getCustomizeSiteURL()); AdminConsole.addLink(Configuration, "system maintenance", new ActionURL(ConfigureSystemMaintenanceAction.class, root)); AdminConsole.addLink(Configuration, "allowed external redirect hosts", new ActionURL(AllowListAction.class, root).addParameter("type", AllowListType.Redirect.name()), TroubleshooterPermission.class); - AdminConsole.addLink(Configuration, "allowed external sources", new ActionURL(AllowListAction.class, root).addParameter("type", AllowListType.Source.name()), TroubleshooterPermission.class); + AdminConsole.addLink(Configuration, "allowed external sources", new ActionURL(ExternalSourcesAction.class, root), TroubleshooterPermission.class); AdminConsole.addLink(Configuration, "allowed file extensions", new ActionURL(AllowListAction.class, root).addParameter("type", AllowListType.FileExtension.name()), TroubleshooterPermission.class); // Diagnostics @@ -10841,30 +10845,30 @@ public class SuspiciousAction extends SimpleViewAction public ModelAndView getView(Object o, BindException errors) { Collection list = BlockListFilter.reportSuspicious(); - HtmlStringBuilder sb = HtmlStringBuilder.of(); + HtmlStringBuilder html = HtmlStringBuilder.of(); if (list.isEmpty()) { - sb.append("No suspicious activity.\n"); + html.append("No suspicious activity.\n"); } else { - sb.unsafeAppend(""); - sb.unsafeAppend("\n"); + html.unsafeAppend("
host (user)user-agentcount
"); + html.unsafeAppend("\n"); for (BlockListFilter.Suspicious s : list) { - sb.unsafeAppend("\n"); + html.append(HtmlString.NBSP).append("(" + s.user + ")"); + html.unsafeAppend("\n"); } - sb.unsafeAppend("
host (user)user-agentcount
") - .append(s.host); + html.unsafeAppend("
") + .append(s.host); if (!isBlank(s.user)) - sb.append(HtmlString.NBSP).append("(" + s.user + ")"); - sb.unsafeAppend("") - .append(s.userAgent) - .unsafeAppend("") - .append(s.count) - .unsafeAppend("
") + .append(s.userAgent) + .unsafeAppend("") + .append(s.count) + .unsafeAppend("
"); + html.unsafeAppend(""); } - return new HtmlView(sb); + return new HtmlView(html); } @Override @@ -10880,7 +10884,7 @@ public void addNavTrail(NavTree root) @Marshal(Marshaller.Jackson) @RequiresNoPermission @AllowedBeforeInitialUserIsSet - public class ConfigurationSummaryAction extends ReadOnlyApiAction + public static class ConfigurationSummaryAction extends ReadOnlyApiAction { @Override public Object execute(Object o, BindException errors) @@ -10902,6 +10906,37 @@ protected ObjectMapper createResponseObjectMapper() result.addMixIn(ExternalScriptEngineDefinitionImpl.class, IgnorePasswordMixIn.class); return result; } + + /* returns a jackson serializable object that reports superset of information returned in admin console */ + private JSONObject getConfigurationJson() + { + JSONObject res = new JSONObject(); + + res.put("server", AdminBean.getPropertyMap()); + + final Map> sets = new TreeMap<>(); + new SqlSelector(CoreSchema.getInstance().getScope(), + new SQLFragment("SELECT category, name, value FROM prop.propertysets PS inner join prop.properties P on PS.\"set\" = P.\"set\"\n" + + "WHERE objectid = ? AND category IN ('SiteConfig') AND encryption='None' AND LOWER(name) NOT LIKE '%password%'", ContainerManager.getRoot())).forEachMap(m -> + { + String category = (String)m.get("category"); + String name = (String)m.get("name"); + Object value = m.get("value"); + if (!sets.containsKey(category)) + sets.put(category, new TreeMap<>()); + sets.get(category).put(name,value); + } + ); + res.put("siteSettings", sets); + + HealthCheck.Result result = HealthCheckRegistry.get().checkHealth(Arrays.asList("all")); + res.put("health", result); + + LabKeyScriptEngineManager mgr = LabKeyScriptEngineManager.get(); + res.put("scriptEngines", mgr.getEngineDefinitions()); + + return res; + } } @JsonIgnoreProperties(value = { "password", "changePassword", "configuration" }) @@ -10913,6 +10948,7 @@ private static class IgnorePasswordMixIn public class AllowListAction extends FormViewAction { private AllowListType _type; + @Override public void validateCommand(AllowListForm target, Errors errors) { @@ -11149,35 +11185,158 @@ private void checkDuplicatesByAddition(String value, Set valueSet, BindE } } - /* returns a jackson serializable object that reports superset of information returned in admin console */ - JSONObject getConfigurationJson() + public static class ExternalSourcesForm { - JSONObject res = new JSONObject(); + private boolean _saveNew; + private String _newDirective; + private String _newHost; + + public boolean isSaveNew() + { + return _saveNew; + } - res.put("server", AdminBean.getPropertyMap()); + @SuppressWarnings("unused") + public void setSaveNew(boolean saveNew) + { + _saveNew = saveNew; + } + + public String getNewDirective() + { + return _newDirective; + } + + @SuppressWarnings("unused") + public void setNewDirective(String newDirective) + { + _newDirective = newDirective; + } + + public String getNewHost() + { + return _newHost; + } - final Map> sets = new TreeMap<>(); - new SqlSelector(CoreSchema.getInstance().getScope(), - new SQLFragment("SELECT category, name, value FROM prop.propertysets PS inner join prop.properties P on PS.\"set\" = P.\"set\"\n" + - "WHERE objectid = ? AND category IN ('SiteConfig') AND encryption='None' AND LOWER(name) NOT LIKE '%password%'", ContainerManager.getRoot())).forEachMap(m -> + @SuppressWarnings("unused") + public void setNewHost(String newHost) + { + _newHost = newHost; + } + + private Collection validateNewSubstitution(BindException errors) throws JsonProcessingException + { + Directive directive = EnumUtils.getEnum(Directive.class, getNewDirective()); + String host = StringUtils.trimToEmpty(getNewHost()); + + if (getNewDirective() == null) { - String category = (String)m.get("category"); - String name = (String)m.get("name"); - Object value = m.get("value"); - if (!sets.containsKey(category)) - sets.put(category, new TreeMap<>()); - sets.get(category).put(name,value); + errors.addError(new LabKeyError("Directive must not be blank.")); } - ); - res.put("siteSettings", sets); + else if (null == directive) + { + errors.addError(new LabKeyError("Unrecognized directive.")); + } + + if (StringUtils.isEmpty(host)) + { + errors.addError(new LabKeyError("Redirect host name must not be blank.")); + } + + // TODO: Validate host - set errors + // TODO: Validate no duplicates - set errors + + if (errors.hasErrors()) + return null; + + List ret = AllowedExternalResourceHosts.readSubstitutions(); + ret.add(new Substitution(directive, host)); + + return ret; + } + } + + @AdminConsoleAction() + public class ExternalSourcesAction extends FormViewAction + { + @Override + public void validateCommand(ExternalSourcesForm form, Errors errors) + { + } + + @Override + public ModelAndView getView(ExternalSourcesForm form, boolean reshow, BindException errors) + { + boolean isTroubleshooter = !getContainer().hasPermission(getUser(), AdminOperationsPermission.class); + + JspView newView = new JspView<>("/org/labkey/core/admin/externalSources.jsp", null, errors); + newView.setTitle(isTroubleshooter ? "Overview" : "Register New External Source"); + newView.setFrame(WebPartView.FrameType.PORTAL); +// JspView existingView = new JspView<>("/org/labkey/core/admin/existingListValues.jsp", form, errors); +// existingView.setTitle("Existing " + form.getTypeEnum().getTitle() + "s"); +// existingView.setFrame(WebPartView.FrameType.PORTAL); + + return new VBox(newView); + } + + @Override + public boolean handlePost(ExternalSourcesForm form, BindException errors) throws Exception + { + Collection substitutions = null; + + if (false) + { + substitutions = null; +// //handle delete of existing value +// if (form.isDelete()) +// { +// String urlToDelete = form.getExistingValue(); +// List values = allowListType.getValues(); +// for (String value : values) +// { +// if (null != urlToDelete && urlToDelete.trim().equalsIgnoreCase(value.trim())) +// { +// values.remove(value); +// allowListType.setValues(values, getUser()); +// break; +// } +// } +// } +// //handle updates - clicking on Save button under Existing will save the updated urls +// else if (form.isSaveAll()) +// { +// Set validatedValues = form.validateValues(errors); +// if (errors.hasErrors()) +// return false; +// +// allowListType.setValues(validatedValues.stream().toList(), getUser()); + } + //save new external value + else if (form.isSaveNew()) + { + substitutions = form.validateNewSubstitution(errors); + } + + if (errors.hasErrors()) + return false; - HealthCheck.Result result = HealthCheckRegistry.get().checkHealth(Arrays.asList("all")); - res.put("health", result); + AllowedExternalResourceHosts.saveSubstitutions(substitutions, getUser()); - LabKeyScriptEngineManager mgr = LabKeyScriptEngineManager.get(); - res.put("scriptEngines", mgr.getEngineDefinitions()); + return true; + } - return res; + @Override + public URLHelper getSuccessURL(ExternalSourcesForm form) + { + return null; + } + + @Override + public void addNavTrail(NavTree root) + { + setHelpTopic("externalHosts"); + addAdminNavTrail(root, "Allowed External Resource Hosts", getClass()); + } } @RequiresPermission(AdminPermission.class) diff --git a/core/src/org/labkey/core/admin/AllowListType.java b/core/src/org/labkey/core/admin/AllowListType.java index df9b99a2611..302b91cabc9 100644 --- a/core/src/org/labkey/core/admin/AllowListType.java +++ b/core/src/org/labkey/core/admin/AllowListType.java @@ -47,19 +47,11 @@ public List getValues() @Override public void setValues(Collection hosts, User user) { - WriteableAppProps props = AppProps.getWriteableInstance(); - props.setExternalSourceHosts(hosts); - props.save(user); - - // Refresh the CSP with new values. - ContentSecurityPolicyFilter.unregisterAllowedSources(Directive.Connection, EXTERNAL_SOURCE_HOSTS_KEY); - ContentSecurityPolicyFilter.registerAllowedSources(Directive.Connection, EXTERNAL_SOURCE_HOSTS_KEY, getValues().toArray(new String[0])); } @Override public void validateValueFormat(String value, BindException errors) { - } @Override diff --git a/core/src/org/labkey/core/admin/externalSources.jsp b/core/src/org/labkey/core/admin/externalSources.jsp new file mode 100644 index 00000000000..a6b5c86917d --- /dev/null +++ b/core/src/org/labkey/core/admin/externalSources.jsp @@ -0,0 +1,69 @@ +<% +/* + * Copyright (c) 2019 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +%> +<%@ page import="org.labkey.api.collections.LabKeyCollectors" %> +<%@ page import="org.labkey.api.data.Container" %> +<%@ page import="org.labkey.api.security.Directive" %> +<%@ page import="org.labkey.api.security.permissions.AdminOperationsPermission" %> +<%@ page import="java.util.Arrays" %> +<%@ page extends="org.labkey.api.jsp.JspBase" %> +<%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> +<% + Container c = getContainer(); + boolean isTroubleshooter = c.isRoot() && !c.hasPermission(getUser(), AdminOperationsPermission.class); +%> + +
+

+ For security reasons, the standard LabKey Content Security Policy (CSP) restricts the hosts that browsers can + use as resource origins. By default, only sources from this server are allowed; other server hosts must be + configured below to enable them to be used as external sources. All provided hosts are added into the CSP + using the \${} substitution key shown next to each directive. +<% + if (!isTroubleshooter) + { +%> + To allow a resource, pick a directive and add the associated hostname or IP address, for example: www.myexternalhost.com or 1.2.3.4. +<% + } +%> +

+

+ For more information on the security concern, please refer to the + <%=link("OWASP cheat sheet", "https://cheatsheetseries.owasp.org/cheatsheets/HTML5_Security_Cheat_Sheet.html#cross-origin-resource-sharing").clearClasses()%> +

+
+<% + if (!isTroubleshooter) + { +%> + + + + + + + + + +
<%=select().name("newDirective").addOptions( + Arrays.stream(Directive.values()).collect(LabKeyCollectors.toLinkedMap(Enum::name, d->d.getCspDirective() + " ${" + d.getSubstitutionKey() + "}")) + )%> 

<%= button("Save").submit(true) %>
+ +<% + } +%> diff --git a/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java b/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java new file mode 100644 index 00000000000..2c8ed46689b --- /dev/null +++ b/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java @@ -0,0 +1,83 @@ +package org.labkey.core.security; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.core.type.TypeReference; +import org.apache.logging.log4j.Logger; +import org.jetbrains.annotations.Nullable; +import org.labkey.api.security.Directive; +import org.labkey.api.security.User; +import org.labkey.api.settings.AppProps; +import org.labkey.api.settings.WriteableAppProps; +import org.labkey.api.util.ExceptionUtil; +import org.labkey.api.util.JsonUtil; +import org.labkey.api.util.logging.LogHelper; +import org.labkey.filters.ContentSecurityPolicyFilter; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.Map; +import java.util.stream.Collectors; + +import static org.labkey.api.settings.AppProps.ALLOWED_EXTERNAL_RESOURCES; + +public class AllowedExternalResourceHosts +{ + private static final Logger LOG = LogHelper.getLogger(AllowedExternalResourceHosts.class, "Exceptions reading persisted external resource hosts"); + + private AllowedExternalResourceHosts() + { + } + + public record Substitution(Directive directive, String host) { } + + public static void saveSubstitutions(@Nullable Collection substitutions, User user) throws JsonProcessingException + { + if (null != substitutions) + { + String json = JsonUtil.createDefaultMapper().writeValueAsString(substitutions); + + WriteableAppProps props = AppProps.getWriteableInstance(); + props.setAllowedExternalSources(json); + props.save(user); + + // Group the substitutions by directive + Map> map = substitutions.stream() + .collect(Collectors.groupingBy(Substitution::directive, Collectors.mapping(Substitution::host, Collectors.toCollection(ArrayList::new)))); + + // Unregister all supported directives then register the directives with substitutions + Arrays.stream(Directive.values()).forEach(dir -> { + ContentSecurityPolicyFilter.unregisterAllowedSources(dir, ALLOWED_EXTERNAL_RESOURCES); + List list = map.get(dir); + if (list != null) + ContentSecurityPolicyFilter.registerAllowedSources(dir, ALLOWED_EXTERNAL_RESOURCES, list.toArray(new String[0])); + }); + } + } + + // Returns a mutable list (mutating it won't affect any cached values) + public static List readSubstitutions() throws JsonProcessingException + { + String json = AppProps.getInstance().getAllowedExternalResources(); + return JsonUtil.createDefaultMapper().readValue(json, new TypeReference<>() {}); + } + + public static void registerHosts() + { + final List list; + + try + { + list = AllowedExternalResourceHosts.readSubstitutions(); + } + catch (JsonProcessingException e) + { + ExceptionUtil.logExceptionToMothership(null, e); + return; + } + + list.forEach(sub -> ContentSecurityPolicyFilter.registerAllowedSources(sub.directive(), sub.host())); + LOG.debug("Registered [{}] as allowed external sources", list); + } +} From b4802e3207ea95189372e8dd42d1a64584ca6f15 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 21 Feb 2025 16:50:30 -0800 Subject: [PATCH 04/23] Checkpoint - list existing hosts, upgrade code --- core/src/org/labkey/core/CoreUpgradeCode.java | 26 +++++ .../labkey/core/admin/AdminController.java | 17 +-- .../org/labkey/core/admin/AllowListType.java | 54 ---------- ...alSources.jsp => addNewExternalSource.jsp} | 0 .../core/admin/existingExternalSources.jsp | 102 ++++++++++++++++++ 5 files changed, 139 insertions(+), 60 deletions(-) rename core/src/org/labkey/core/admin/{externalSources.jsp => addNewExternalSource.jsp} (100%) create mode 100644 core/src/org/labkey/core/admin/existingExternalSources.jsp diff --git a/core/src/org/labkey/core/CoreUpgradeCode.java b/core/src/org/labkey/core/CoreUpgradeCode.java index 7c10438dbee..ce34a5f9b95 100644 --- a/core/src/org/labkey/core/CoreUpgradeCode.java +++ b/core/src/org/labkey/core/CoreUpgradeCode.java @@ -15,6 +15,7 @@ */ package org.labkey.core; +import com.fasterxml.jackson.core.JsonProcessingException; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.NotNull; import org.labkey.api.collections.CaseInsensitiveHashSet; @@ -29,8 +30,13 @@ import org.labkey.api.data.UpgradeCode; import org.labkey.api.module.ModuleContext; import org.labkey.api.module.ModuleLoader; +import org.labkey.api.security.Directive; +import org.labkey.api.settings.AppProps; +import org.labkey.api.util.ExceptionUtil; import org.labkey.api.util.GUID; import org.labkey.api.util.logging.LogHelper; +import org.labkey.core.security.AllowedExternalResourceHosts; +import org.labkey.core.security.AllowedExternalResourceHosts.Substitution; import java.util.HashSet; import java.util.List; @@ -167,4 +173,24 @@ public static void makeWithCounterCaseInsensitive(ModuleContext context) LOG.info("** finished upgrade withCounter DBSequences for container: " + container.getPath()); } } + + public static void migrateAllowedExternalConnectionHosts(ModuleContext context) + { + if (context.isNewInstall()) + return; + + List hosts = AppProps.getInstance().getExternalSourceHosts(); + List substitutions = hosts.stream() + .map(host -> new Substitution(Directive.Connection, host)) + .toList(); + + try + { + AllowedExternalResourceHosts.saveSubstitutions(substitutions, context.getUpgradeUser()); + } + catch (JsonProcessingException e) + { + ExceptionUtil.logExceptionToMothership(null, e); + } + } } \ No newline at end of file diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index c3d77ad7f64..897acd7a8b8 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -11249,11 +11249,16 @@ else if (null == directive) if (errors.hasErrors()) return null; - List ret = AllowedExternalResourceHosts.readSubstitutions(); + List ret = getExistingSubstitutions(); ret.add(new Substitution(directive, host)); return ret; } + + public List getExistingSubstitutions() throws JsonProcessingException + { + return AllowedExternalResourceHosts.readSubstitutions(); + } } @AdminConsoleAction() @@ -11269,14 +11274,14 @@ public ModelAndView getView(ExternalSourcesForm form, boolean reshow, BindExcept { boolean isTroubleshooter = !getContainer().hasPermission(getUser(), AdminOperationsPermission.class); - JspView newView = new JspView<>("/org/labkey/core/admin/externalSources.jsp", null, errors); + JspView newView = new JspView<>("/org/labkey/core/admin/addNewExternalSource.jsp", null, errors); newView.setTitle(isTroubleshooter ? "Overview" : "Register New External Source"); newView.setFrame(WebPartView.FrameType.PORTAL); -// JspView existingView = new JspView<>("/org/labkey/core/admin/existingListValues.jsp", form, errors); -// existingView.setTitle("Existing " + form.getTypeEnum().getTitle() + "s"); -// existingView.setFrame(WebPartView.FrameType.PORTAL); + JspView existingView = new JspView<>("/org/labkey/core/admin/existingExternalSources.jsp", form, errors); + existingView.setTitle("Existing External Sources"); + existingView.setFrame(WebPartView.FrameType.PORTAL); - return new VBox(newView); + return new VBox(newView, existingView); } @Override diff --git a/core/src/org/labkey/core/admin/AllowListType.java b/core/src/org/labkey/core/admin/AllowListType.java index 302b91cabc9..e5a9eea79ae 100644 --- a/core/src/org/labkey/core/admin/AllowListType.java +++ b/core/src/org/labkey/core/admin/AllowListType.java @@ -6,13 +6,11 @@ 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; import org.labkey.api.util.URLHelper; import org.labkey.api.view.ActionURL; -import org.labkey.filters.ContentSecurityPolicyFilter; import org.springframework.validation.BindException; import java.util.Collection; @@ -20,52 +18,6 @@ public enum AllowListType { - Source { - @Override - public HtmlString getDescription() - { - return HtmlString.unsafe(""" -
-

- For security reasons, LabKey Server restricts the hosts that can be used as resource origins. By default, only LabKey sources are allowed, other server URLs must be configured below to enable them to be used as script sources. - For more information on the security concern, please refer to the OWASP cheat sheet. -

-

- Add allowed source URLs or IP address as they will be referenced in script source values. - For example: www.myexternalhost.com or 1.2.3.4 -

-
- """); - } - - @Override - public List getValues() - { - return AppProps.getInstance().getExternalSourceHosts(); - } - - @Override - public void setValues(Collection hosts, User user) - { - } - - @Override - public void validateValueFormat(String value, BindException errors) - { - } - - @Override - public HtmlString getTitle() - { - return HtmlString.of(String.format("External %1$s Host", name())); - } - - @Override - public HtmlString getLabel() - { - return HtmlString.of("Host"); - } - }, Redirect { private static final AuthorityValidator AUTHORITY_VALIDATOR = new AuthorityValidator(UrlValidator.ALLOW_LOCAL_URLS); @@ -182,12 +134,6 @@ public HtmlString getLabel() } }; - private static final String EXTERNAL_SOURCE_HOSTS_KEY = "External Sources"; - public static String getExternalSourceHostsKey() - { - return EXTERNAL_SOURCE_HOSTS_KEY; - } - public abstract HtmlString getDescription(); public abstract List getValues(); public abstract void setValues(Collection redirectHosts, User user); diff --git a/core/src/org/labkey/core/admin/externalSources.jsp b/core/src/org/labkey/core/admin/addNewExternalSource.jsp similarity index 100% rename from core/src/org/labkey/core/admin/externalSources.jsp rename to core/src/org/labkey/core/admin/addNewExternalSource.jsp diff --git a/core/src/org/labkey/core/admin/existingExternalSources.jsp b/core/src/org/labkey/core/admin/existingExternalSources.jsp new file mode 100644 index 00000000000..2c85d2dba29 --- /dev/null +++ b/core/src/org/labkey/core/admin/existingExternalSources.jsp @@ -0,0 +1,102 @@ +<% +/* + * Copyright (c) 2019 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +%> +<%@ page import="org.labkey.api.admin.AdminUrls" %> +<%@ page import="org.labkey.api.data.Container" %> +<%@ page import="org.labkey.api.security.permissions.AdminOperationsPermission" %> +<%@ page import="org.labkey.api.util.HtmlString" %> +<%@ page import="org.labkey.api.view.HttpView" %> +<%@ page import="org.labkey.core.admin.AdminController.ExternalSourcesForm" %> +<%@ page import="org.labkey.core.security.AllowedExternalResourceHosts.Substitution" %> +<%@ page import="java.util.List" %> +<%@ page import="java.util.Comparator" %> +<%@ page extends="org.labkey.api.jsp.JspBase" %> +<%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> +<% + Container c = getContainer(); + boolean isTroubleshooter = c.isRoot() && !c.hasPermission(getUser(), AdminOperationsPermission.class); +%> + + + + + <% + ExternalSourcesForm bean = (ExternalSourcesForm) HttpView.currentModel(); + List existingSubstitutions = bean.getExistingSubstitutions(); + existingSubstitutions.sort(Comparator.comparing(Substitution::directive).thenComparing(Substitution::host)); + %> + + <% if (existingSubstitutions.isEmpty()) { %> + + <% } %> + + <% + int num = 1; + for (Substitution value : existingSubstitutions) { + String inputNameExisting = "existingValue" + num; + %> + + + + + + + <% + num++; + } + %> +
No External Sources have been configured.
<%=isTroubleshooter ? HtmlString.EMPTY_STRING : button("Delete").primary(true).onClick("return deleteExisting(\"" + h(value) + "\");") %> + +
+ <% if (!existingSubstitutions.isEmpty()) { %> + + + + + +
<%=isTroubleshooter ? button("Done").href(urlProvider(AdminUrls.class).getAdminConsoleURL()) : button("Save").primary(true).onClick("return saveAll();")%> + + <% } %> + From 41519f38e24efe2332a084301b03513e2cf33e0d Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 21 Feb 2025 17:28:27 -0800 Subject: [PATCH 05/23] Checkpoint - delete --- .../labkey/core/admin/AdminController.java | 75 ++++++++++++++----- .../core/admin/existingExternalSources.jsp | 9 ++- 2 files changed, 60 insertions(+), 24 deletions(-) diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 897acd7a8b8..ec70d40211b 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -11187,9 +11187,22 @@ private void checkDuplicatesByAddition(String value, Set valueSet, BindE public static class ExternalSourcesForm { + private boolean _delete; private boolean _saveNew; private String _newDirective; private String _newHost; + private String _existingValue; + + public boolean isDelete() + { + return _delete; + } + + @SuppressWarnings("unused") + public void setDelete(boolean delete) + { + _delete = delete; + } public boolean isSaveNew() { @@ -11224,7 +11237,18 @@ public void setNewHost(String newHost) _newHost = newHost; } - private Collection validateNewSubstitution(BindException errors) throws JsonProcessingException + public String getExistingValue() + { + return _existingValue; + } + + @SuppressWarnings("unused") + public void setExistingValue(String existingValue) + { + _existingValue = existingValue; + } + + private List validateNewSubstitution(BindException errors) throws JsonProcessingException { Directive directive = EnumUtils.getEnum(Directive.class, getNewDirective()); String host = StringUtils.trimToEmpty(getNewHost()); @@ -11287,26 +11311,37 @@ public ModelAndView getView(ExternalSourcesForm form, boolean reshow, BindExcept @Override public boolean handlePost(ExternalSourcesForm form, BindException errors) throws Exception { - Collection substitutions = null; + List substitutions = null; - if (false) + //handle delete of existing value + if (form.isDelete()) { - substitutions = null; -// //handle delete of existing value -// if (form.isDelete()) -// { -// String urlToDelete = form.getExistingValue(); -// List values = allowListType.getValues(); -// for (String value : values) -// { -// if (null != urlToDelete && urlToDelete.trim().equalsIgnoreCase(value.trim())) -// { -// values.remove(value); -// allowListType.setValues(values, getUser()); -// break; -// } -// } -// } + String subToDelete = form.getExistingValue(); + String[] parts = subToDelete.split(":", 2); + if (parts.length != 2) + { + errors.addError(new LabKeyError("Can't parse substitution.")); + return false; + } + Directive dir = EnumUtils.getEnum(Directive.class, parts[0], null); + if (null == dir) + { + errors.addError(new LabKeyError("Unknown directive.")); + return false; + } + Substitution delete = new Substitution(dir, parts[1]); + substitutions = form.getExistingSubstitutions(); + var iter = substitutions.listIterator(); + while (iter.hasNext()) + { + Substitution sub = iter.next(); + if (sub.equals(delete)) + { + iter.remove(); + break; + } + } + } // //handle updates - clicking on Save button under Existing will save the updated urls // else if (form.isSaveAll()) // { @@ -11315,7 +11350,7 @@ public boolean handlePost(ExternalSourcesForm form, BindException errors) throws // return false; // // allowListType.setValues(validatedValues.stream().toList(), getUser()); - } +// } //save new external value else if (form.isSaveNew()) { diff --git a/core/src/org/labkey/core/admin/existingExternalSources.jsp b/core/src/org/labkey/core/admin/existingExternalSources.jsp index 2c85d2dba29..42a7c3e4c73 100644 --- a/core/src/org/labkey/core/admin/existingExternalSources.jsp +++ b/core/src/org/labkey/core/admin/existingExternalSources.jsp @@ -22,8 +22,8 @@ <%@ page import="org.labkey.api.view.HttpView" %> <%@ page import="org.labkey.core.admin.AdminController.ExternalSourcesForm" %> <%@ page import="org.labkey.core.security.AllowedExternalResourceHosts.Substitution" %> -<%@ page import="java.util.List" %> <%@ page import="java.util.Comparator" %> +<%@ page import="java.util.List" %> <%@ page extends="org.labkey.api.jsp.JspBase" %> <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <% @@ -74,14 +74,15 @@ <% int num = 1; - for (Substitution value : existingSubstitutions) { + for (Substitution sub : existingSubstitutions) { String inputNameExisting = "existingValue" + num; %> - + /> + /> - <%=isTroubleshooter ? HtmlString.EMPTY_STRING : button("Delete").primary(true).onClick("return deleteExisting(\"" + h(value) + "\");") %> + <%=isTroubleshooter ? HtmlString.EMPTY_STRING : button("Delete").primary(true).onClick("return deleteExisting(" + q(sub.directive() + ":" + sub.host()) + ");") %> From 91fdc7523ac7e79d5f34b439f42b3aa2f889a160 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 21 Feb 2025 20:15:55 -0800 Subject: [PATCH 06/23] Checkpoint - edit --- .../org/labkey/api/security/Directive.java | 4 +- .../labkey/core/admin/AdminController.java | 92 +++++++++++++------ .../core/admin/addNewExternalSource.jsp | 2 +- .../core/admin/existingExternalSources.jsp | 31 ++++--- 4 files changed, 90 insertions(+), 39 deletions(-) diff --git a/api/src/org/labkey/api/security/Directive.java b/api/src/org/labkey/api/security/Directive.java index 2fff4323bf7..d7cd60fad19 100644 --- a/api/src/org/labkey/api/security/Directive.java +++ b/api/src/org/labkey/api/security/Directive.java @@ -1,10 +1,12 @@ package org.labkey.api.security; +import org.labkey.api.util.SafeToRenderEnum; + /** * 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 +public enum Directive implements SafeToRenderEnum { Connection("connect-src"), Font("font-src"), diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index ec70d40211b..38cdf6c00a7 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -11189,9 +11189,12 @@ public static class ExternalSourcesForm { private boolean _delete; private boolean _saveNew; + private boolean _saveAll; + private String _newDirective; private String _newHost; private String _existingValue; + private String _existingValues; public boolean isDelete() { @@ -11215,6 +11218,16 @@ public void setSaveNew(boolean saveNew) _saveNew = saveNew; } + public boolean isSaveAll() + { + return _saveAll; + } + + public void setSaveAll(boolean saveAll) + { + _saveAll = saveAll; + } + public String getNewDirective() { return _newDirective; @@ -11248,6 +11261,46 @@ public void setExistingValue(String existingValue) _existingValue = existingValue; } + public String getExistingValues() + { + return _existingValues.replace("\r", ""); + } + + @SuppressWarnings("unused") + public void setExistingValues(String existingValues) + { + _existingValues = existingValues; + } + + private Substitution getExistingSubstitution(BindException errors) + { + return getSubstitution(getExistingValue(), errors); + } + + private Substitution getSubstitution(String value, BindException errors) + { + String[] parts = value.split("\\|", 2); // Stop after the first bar to produce two parts + if (parts.length != 2) + { + errors.addError(new LabKeyError("Can't parse substitution.")); + return null; + } + Directive dir = EnumUtils.getEnum(Directive.class, parts[0], null); + if (null == dir) + { + errors.addError(new LabKeyError("Unknown directive.")); + return null; + } + return new Substitution(dir, parts[1]); + } + + private List getExistingSubstitutions(BindException errors) + { + return Arrays.stream(getExistingValues().split("\n")) + .map(value->getSubstitution(value, errors)) + .toList(); + } + private List validateNewSubstitution(BindException errors) throws JsonProcessingException { Directive directive = EnumUtils.getEnum(Directive.class, getNewDirective()); @@ -11273,13 +11326,13 @@ else if (null == directive) if (errors.hasErrors()) return null; - List ret = getExistingSubstitutions(); + List ret = getSavedSubstitutions(); ret.add(new Substitution(directive, host)); return ret; } - public List getExistingSubstitutions() throws JsonProcessingException + public List getSavedSubstitutions() throws JsonProcessingException { return AllowedExternalResourceHosts.readSubstitutions(); } @@ -11316,41 +11369,28 @@ public boolean handlePost(ExternalSourcesForm form, BindException errors) throws //handle delete of existing value if (form.isDelete()) { - String subToDelete = form.getExistingValue(); - String[] parts = subToDelete.split(":", 2); - if (parts.length != 2) - { - errors.addError(new LabKeyError("Can't parse substitution.")); - return false; - } - Directive dir = EnumUtils.getEnum(Directive.class, parts[0], null); - if (null == dir) - { - errors.addError(new LabKeyError("Unknown directive.")); + Substitution subToDelete = form.getExistingSubstitution(errors); + if (errors.hasErrors()) return false; - } - Substitution delete = new Substitution(dir, parts[1]); - substitutions = form.getExistingSubstitutions(); + substitutions = form.getSavedSubstitutions(); var iter = substitutions.listIterator(); while (iter.hasNext()) { Substitution sub = iter.next(); - if (sub.equals(delete)) + if (sub.equals(subToDelete)) { iter.remove(); break; } } } -// //handle updates - clicking on Save button under Existing will save the updated urls -// else if (form.isSaveAll()) -// { -// Set validatedValues = form.validateValues(errors); -// if (errors.hasErrors()) -// return false; -// -// allowListType.setValues(validatedValues.stream().toList(), getUser()); -// } + //handle updates - clicking on Save button under Existing will save the updated urls + else if (form.isSaveAll()) + { + substitutions = form.getExistingSubstitutions(errors); + if (errors.hasErrors()) + return false; + } //save new external value else if (form.isSaveNew()) { diff --git a/core/src/org/labkey/core/admin/addNewExternalSource.jsp b/core/src/org/labkey/core/admin/addNewExternalSource.jsp index a6b5c86917d..879aa1539a7 100644 --- a/core/src/org/labkey/core/admin/addNewExternalSource.jsp +++ b/core/src/org/labkey/core/admin/addNewExternalSource.jsp @@ -60,7 +60,7 @@   -
<%= button("Save").submit(true) %> +
<%= button("Add").submit(true) %> diff --git a/core/src/org/labkey/core/admin/existingExternalSources.jsp b/core/src/org/labkey/core/admin/existingExternalSources.jsp index 42a7c3e4c73..abdb745958d 100644 --- a/core/src/org/labkey/core/admin/existingExternalSources.jsp +++ b/core/src/org/labkey/core/admin/existingExternalSources.jsp @@ -43,15 +43,17 @@ function saveAll() { //clicking on save will save all the values - changed and unchanged values - var num = 1; - var inputNameExisting = "existingValue" + num; - var values = ""; + let num = 1; + let directiveId = "directive" + num; + let hostId = "host" + num; + let values = ""; - while (null != document.getElementById(inputNameExisting)) + while (null != document.getElementById(directiveId)) { - values += (document.getElementById(inputNameExisting).value + "\n"); + values += (document.getElementById(directiveId).getAttribute('data-directive') + "|" + document.getElementById(hostId).value) + "\n"; num++; - inputNameExisting = "existingValue" + num; + directiveId = "directive" + num; + hostId = "host" + num; } document.getElementById("saveAll").value = true; @@ -64,7 +66,7 @@ <% ExternalSourcesForm bean = (ExternalSourcesForm) HttpView.currentModel(); - List existingSubstitutions = bean.getExistingSubstitutions(); + List existingSubstitutions = bean.getSavedSubstitutions(); existingSubstitutions.sort(Comparator.comparing(Substitution::directive).thenComparing(Substitution::host)); %> @@ -75,14 +77,21 @@ <% int num = 1; for (Substitution sub : existingSubstitutions) { - String inputNameExisting = "existingValue" + num; + String directiveId = "directive" + num; + String hostId = "host" + num; %> - - + + - From 627d94ed3b04f94aa0c2b2f6aacdf02df1bd92a0 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 21 Feb 2025 20:32:11 -0800 Subject: [PATCH 07/23] Substitution -> AllowedHost --- api/src/org/labkey/api/settings/AppProps.java | 4 +- .../org/labkey/api/settings/AppPropsImpl.java | 2 +- .../api/settings/WriteableAppProps.java | 2 +- core/src/org/labkey/core/CoreUpgradeCode.java | 8 ++-- .../labkey/core/admin/AdminController.java | 47 ++++++++++--------- .../core/admin/existingExternalSources.jsp | 14 +++--- .../AllowedExternalResourceHosts.java | 26 +++++----- 7 files changed, 52 insertions(+), 51 deletions(-) diff --git a/api/src/org/labkey/api/settings/AppProps.java b/api/src/org/labkey/api/settings/AppProps.java index cfdac77443f..5eb3a6d52d8 100644 --- a/api/src/org/labkey/api/settings/AppProps.java +++ b/api/src/org/labkey/api/settings/AppProps.java @@ -246,7 +246,7 @@ static WriteableAppProps getWriteableInstance() /** * @return List of configured external resource hosts */ - @Deprecated // Left for backward compatibility + @Deprecated // Left for upgrade code only @NotNull List getExternalSourceHosts(); @@ -260,5 +260,5 @@ static WriteableAppProps getWriteableInstance() @NotNull List getAllowedExtensions(); - @NotNull String getAllowedExternalResources(); + @NotNull String getAllowedExternalResourceHosts(); } diff --git a/api/src/org/labkey/api/settings/AppPropsImpl.java b/api/src/org/labkey/api/settings/AppPropsImpl.java index e69c1989c02..c86406ec38a 100644 --- a/api/src/org/labkey/api/settings/AppPropsImpl.java +++ b/api/src/org/labkey/api/settings/AppPropsImpl.java @@ -742,7 +742,7 @@ public List getExternalSourceHosts() } @Override - public @NotNull String getAllowedExternalResources() + public @NotNull String getAllowedExternalResourceHosts() { return lookupStringValue(ALLOWED_EXTERNAL_RESOURCES, "[]"); } diff --git a/api/src/org/labkey/api/settings/WriteableAppProps.java b/api/src/org/labkey/api/settings/WriteableAppProps.java index 0b5e4e8ae8d..aba9b5503bf 100644 --- a/api/src/org/labkey/api/settings/WriteableAppProps.java +++ b/api/src/org/labkey/api/settings/WriteableAppProps.java @@ -243,7 +243,7 @@ public void setAllowedFileExtensions(Collection allowedFileExtensions) FileUtil.setExtensionChecker(AppProps.getInstance()); } - public void setAllowedExternalSources(String jsonArray) + public void setAllowedExternalResourceHosts(String jsonArray) { storeStringValue(ALLOWED_EXTERNAL_RESOURCES, jsonArray); } diff --git a/core/src/org/labkey/core/CoreUpgradeCode.java b/core/src/org/labkey/core/CoreUpgradeCode.java index ce34a5f9b95..234ff50b0f5 100644 --- a/core/src/org/labkey/core/CoreUpgradeCode.java +++ b/core/src/org/labkey/core/CoreUpgradeCode.java @@ -36,7 +36,7 @@ import org.labkey.api.util.GUID; import org.labkey.api.util.logging.LogHelper; import org.labkey.core.security.AllowedExternalResourceHosts; -import org.labkey.core.security.AllowedExternalResourceHosts.Substitution; +import org.labkey.core.security.AllowedExternalResourceHosts.AllowedHost; import java.util.HashSet; import java.util.List; @@ -180,13 +180,13 @@ public static void migrateAllowedExternalConnectionHosts(ModuleContext context) return; List hosts = AppProps.getInstance().getExternalSourceHosts(); - List substitutions = hosts.stream() - .map(host -> new Substitution(Directive.Connection, host)) + List allowedHosts = hosts.stream() + .map(host -> new AllowedHost(Directive.Connection, host)) .toList(); try { - AllowedExternalResourceHosts.saveSubstitutions(substitutions, context.getUpgradeUser()); + AllowedExternalResourceHosts.saveAllowedHosts(allowedHosts, context.getUpgradeUser()); } catch (JsonProcessingException e) { diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 38cdf6c00a7..893240462f6 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -320,7 +320,7 @@ import org.labkey.core.query.PostgresUserSchema; import org.labkey.core.reports.ExternalScriptEngineDefinitionImpl; import org.labkey.core.security.AllowedExternalResourceHosts; -import org.labkey.core.security.AllowedExternalResourceHosts.Substitution; +import org.labkey.core.security.AllowedExternalResourceHosts.AllowedHost; import org.labkey.core.security.BlockListFilter; import org.labkey.core.security.SecurityController; import org.labkey.data.xml.TablesDocument; @@ -11272,17 +11272,17 @@ public void setExistingValues(String existingValues) _existingValues = existingValues; } - private Substitution getExistingSubstitution(BindException errors) + private AllowedHost getExistingAllowedHost(BindException errors) { - return getSubstitution(getExistingValue(), errors); + return getAllowedHost(getExistingValue(), errors); } - private Substitution getSubstitution(String value, BindException errors) + private AllowedHost getAllowedHost(String value, BindException errors) { String[] parts = value.split("\\|", 2); // Stop after the first bar to produce two parts if (parts.length != 2) { - errors.addError(new LabKeyError("Can't parse substitution.")); + errors.addError(new LabKeyError("Can't parse allowed host.")); return null; } Directive dir = EnumUtils.getEnum(Directive.class, parts[0], null); @@ -11291,17 +11291,17 @@ private Substitution getSubstitution(String value, BindException errors) errors.addError(new LabKeyError("Unknown directive.")); return null; } - return new Substitution(dir, parts[1]); + return new AllowedHost(dir, parts[1]); } - private List getExistingSubstitutions(BindException errors) + private List getExistingAllowedHosts(BindException errors) { return Arrays.stream(getExistingValues().split("\n")) - .map(value->getSubstitution(value, errors)) + .map(value-> getAllowedHost(value, errors)) .toList(); } - private List validateNewSubstitution(BindException errors) throws JsonProcessingException + private List validateNewAllowedHost(BindException errors) throws JsonProcessingException { Directive directive = EnumUtils.getEnum(Directive.class, getNewDirective()); String host = StringUtils.trimToEmpty(getNewHost()); @@ -11326,15 +11326,16 @@ else if (null == directive) if (errors.hasErrors()) return null; - List ret = getSavedSubstitutions(); - ret.add(new Substitution(directive, host)); + List ret = getSavedAllowedHosts(); + ret.add(new AllowedHost(directive, host)); return ret; } - public List getSavedSubstitutions() throws JsonProcessingException + // Returns a mutable list + public List getSavedAllowedHosts() throws JsonProcessingException { - return AllowedExternalResourceHosts.readSubstitutions(); + return AllowedExternalResourceHosts.readAllowedHosts(); } } @@ -11352,10 +11353,10 @@ public ModelAndView getView(ExternalSourcesForm form, boolean reshow, BindExcept boolean isTroubleshooter = !getContainer().hasPermission(getUser(), AdminOperationsPermission.class); JspView newView = new JspView<>("/org/labkey/core/admin/addNewExternalSource.jsp", null, errors); - newView.setTitle(isTroubleshooter ? "Overview" : "Register New External Source"); + newView.setTitle(isTroubleshooter ? "Overview" : "Register New External Resource Host"); newView.setFrame(WebPartView.FrameType.PORTAL); JspView existingView = new JspView<>("/org/labkey/core/admin/existingExternalSources.jsp", form, errors); - existingView.setTitle("Existing External Sources"); + existingView.setTitle("Existing External Resource Hosts"); existingView.setFrame(WebPartView.FrameType.PORTAL); return new VBox(newView, existingView); @@ -11364,19 +11365,19 @@ public ModelAndView getView(ExternalSourcesForm form, boolean reshow, BindExcept @Override public boolean handlePost(ExternalSourcesForm form, BindException errors) throws Exception { - List substitutions = null; + List allowedHosts = null; //handle delete of existing value if (form.isDelete()) { - Substitution subToDelete = form.getExistingSubstitution(errors); + AllowedHost subToDelete = form.getExistingAllowedHost(errors); if (errors.hasErrors()) return false; - substitutions = form.getSavedSubstitutions(); - var iter = substitutions.listIterator(); + allowedHosts = form.getSavedAllowedHosts(); + var iter = allowedHosts.listIterator(); while (iter.hasNext()) { - Substitution sub = iter.next(); + AllowedHost sub = iter.next(); if (sub.equals(subToDelete)) { iter.remove(); @@ -11387,20 +11388,20 @@ public boolean handlePost(ExternalSourcesForm form, BindException errors) throws //handle updates - clicking on Save button under Existing will save the updated urls else if (form.isSaveAll()) { - substitutions = form.getExistingSubstitutions(errors); + allowedHosts = form.getExistingAllowedHosts(errors); if (errors.hasErrors()) return false; } //save new external value else if (form.isSaveNew()) { - substitutions = form.validateNewSubstitution(errors); + allowedHosts = form.validateNewAllowedHost(errors); } if (errors.hasErrors()) return false; - AllowedExternalResourceHosts.saveSubstitutions(substitutions, getUser()); + AllowedExternalResourceHosts.saveAllowedHosts(allowedHosts, getUser()); return true; } diff --git a/core/src/org/labkey/core/admin/existingExternalSources.jsp b/core/src/org/labkey/core/admin/existingExternalSources.jsp index abdb745958d..f29a934a363 100644 --- a/core/src/org/labkey/core/admin/existingExternalSources.jsp +++ b/core/src/org/labkey/core/admin/existingExternalSources.jsp @@ -21,7 +21,7 @@ <%@ page import="org.labkey.api.util.HtmlString" %> <%@ page import="org.labkey.api.view.HttpView" %> <%@ page import="org.labkey.core.admin.AdminController.ExternalSourcesForm" %> -<%@ page import="org.labkey.core.security.AllowedExternalResourceHosts.Substitution" %> +<%@ page import="org.labkey.core.security.AllowedExternalResourceHosts.AllowedHost" %> <%@ page import="java.util.Comparator" %> <%@ page import="java.util.List" %> <%@ page extends="org.labkey.api.jsp.JspBase" %> @@ -66,17 +66,17 @@ <% ExternalSourcesForm bean = (ExternalSourcesForm) HttpView.currentModel(); - List existingSubstitutions = bean.getSavedSubstitutions(); - existingSubstitutions.sort(Comparator.comparing(Substitution::directive).thenComparing(Substitution::host)); + List existingAllowedHosts = bean.getSavedAllowedHosts(); + existingAllowedHosts.sort(Comparator.comparing(AllowedHost::directive).thenComparing(AllowedHost::host)); %>
/>/>/><%=isTroubleshooter ? HtmlString.EMPTY_STRING : button("Delete").primary(true).onClick("return deleteExisting(" + q(sub.directive() + ":" + sub.host()) + ");") %> + <%=isTroubleshooter ? + HtmlString.EMPTY_STRING : + button("Delete") + .primary(true) + .onClick("return deleteExisting(" + + q(sub.directive() + "|" + sub.host()) + // Using | separator is safe because directive name never contains it + ");") %>
- <% if (existingSubstitutions.isEmpty()) { %> - + <% if (existingAllowedHosts.isEmpty()) { %> + <% } %> <% int num = 1; - for (Substitution sub : existingSubstitutions) { + for (AllowedHost sub : existingAllowedHosts) { String directiveId = "directive" + num; String hostId = "host" + num; %> @@ -100,7 +100,7 @@ } %>
No External Sources have been configured.
No External Resource Hosts have been configured.
- <% if (!existingSubstitutions.isEmpty()) { %> + <% if (!existingAllowedHosts.isEmpty()) { %> diff --git a/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java b/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java index 2c8ed46689b..be33e3758c6 100644 --- a/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java +++ b/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java @@ -30,23 +30,23 @@ private AllowedExternalResourceHosts() { } - public record Substitution(Directive directive, String host) { } + public record AllowedHost(Directive directive, String host) { } - public static void saveSubstitutions(@Nullable Collection substitutions, User user) throws JsonProcessingException + public static void saveAllowedHosts(@Nullable Collection allowedHosts, User user) throws JsonProcessingException { - if (null != substitutions) + if (null != allowedHosts) { - String json = JsonUtil.createDefaultMapper().writeValueAsString(substitutions); + String json = JsonUtil.createDefaultMapper().writeValueAsString(allowedHosts); WriteableAppProps props = AppProps.getWriteableInstance(); - props.setAllowedExternalSources(json); + props.setAllowedExternalResourceHosts(json); props.save(user); - // Group the substitutions by directive - Map> map = substitutions.stream() - .collect(Collectors.groupingBy(Substitution::directive, Collectors.mapping(Substitution::host, Collectors.toCollection(ArrayList::new)))); + // Group the allowed hosts by directive + Map> map = allowedHosts.stream() + .collect(Collectors.groupingBy(AllowedHost::directive, Collectors.mapping(AllowedHost::host, Collectors.toCollection(ArrayList::new)))); - // Unregister all supported directives then register the directives with substitutions + // Unregister all supported directives then register the directives with that have allowed hosts Arrays.stream(Directive.values()).forEach(dir -> { ContentSecurityPolicyFilter.unregisterAllowedSources(dir, ALLOWED_EXTERNAL_RESOURCES); List list = map.get(dir); @@ -57,19 +57,19 @@ public static void saveSubstitutions(@Nullable Collection substitu } // Returns a mutable list (mutating it won't affect any cached values) - public static List readSubstitutions() throws JsonProcessingException + public static List readAllowedHosts() throws JsonProcessingException { - String json = AppProps.getInstance().getAllowedExternalResources(); + String json = AppProps.getInstance().getAllowedExternalResourceHosts(); return JsonUtil.createDefaultMapper().readValue(json, new TypeReference<>() {}); } public static void registerHosts() { - final List list; + final List list; try { - list = AllowedExternalResourceHosts.readSubstitutions(); + list = AllowedExternalResourceHosts.readAllowedHosts(); } catch (JsonProcessingException e) { From a10d0b9cdda8b706e918cfd615ffb21d04b24162 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 21 Feb 2025 20:41:34 -0800 Subject: [PATCH 08/23] Upgrade code to migrate old properties --- core/module.properties | 2 +- .../schemas/dbscripts/postgresql/core-25.000-25.001.sql | 1 + .../schemas/dbscripts/sqlserver/core-25.000-25.001.sql | 1 + core/src/org/labkey/core/CoreUpgradeCode.java | 4 ++++ 4 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 core/resources/schemas/dbscripts/postgresql/core-25.000-25.001.sql create mode 100644 core/resources/schemas/dbscripts/sqlserver/core-25.000-25.001.sql diff --git a/core/module.properties b/core/module.properties index 0d75d4beb95..0c1eb10334b 100644 --- a/core/module.properties +++ b/core/module.properties @@ -1,6 +1,6 @@ Name: Core ModuleClass: org.labkey.core.CoreModule -SchemaVersion: 25.000 +SchemaVersion: 25.001 Label: Administration and Essential Services Description: The Core module provides central services such as login, \ security, administration, folder management, user management, \ diff --git a/core/resources/schemas/dbscripts/postgresql/core-25.000-25.001.sql b/core/resources/schemas/dbscripts/postgresql/core-25.000-25.001.sql new file mode 100644 index 00000000000..d2fc4660427 --- /dev/null +++ b/core/resources/schemas/dbscripts/postgresql/core-25.000-25.001.sql @@ -0,0 +1 @@ +SELECT core.executeJavaUpgradeCode('migrateAllowedExternalConnectionHosts'); diff --git a/core/resources/schemas/dbscripts/sqlserver/core-25.000-25.001.sql b/core/resources/schemas/dbscripts/sqlserver/core-25.000-25.001.sql new file mode 100644 index 00000000000..e4af0fa1909 --- /dev/null +++ b/core/resources/schemas/dbscripts/sqlserver/core-25.000-25.001.sql @@ -0,0 +1 @@ +EXEC core.executeJavaUpgradeCode 'migrateAllowedExternalConnectionHosts'; diff --git a/core/src/org/labkey/core/CoreUpgradeCode.java b/core/src/org/labkey/core/CoreUpgradeCode.java index 234ff50b0f5..cf8a6ff3fdf 100644 --- a/core/src/org/labkey/core/CoreUpgradeCode.java +++ b/core/src/org/labkey/core/CoreUpgradeCode.java @@ -174,6 +174,10 @@ public static void makeWithCounterCaseInsensitive(ModuleContext context) } } + /** + * Called from core-25.000-25.001.sql + */ + @SuppressWarnings("unused") public static void migrateAllowedExternalConnectionHosts(ModuleContext context) { if (context.isNewInstall()) From 212c71d23a135c1d53f6d3b0aa75beaf354f033c Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Fri, 21 Feb 2025 21:37:55 -0800 Subject: [PATCH 09/23] Startup properties --- .../org/labkey/api/security/Directive.java | 23 +++++++++---- core/src/org/labkey/core/CoreModule.java | 1 + core/src/org/labkey/core/CoreUpgradeCode.java | 21 ++++-------- .../AllowedExternalResourceHosts.java | 34 +++++++++++++++++-- 4 files changed, 56 insertions(+), 23 deletions(-) diff --git a/api/src/org/labkey/api/security/Directive.java b/api/src/org/labkey/api/security/Directive.java index d7cd60fad19..22c9d645f50 100644 --- a/api/src/org/labkey/api/security/Directive.java +++ b/api/src/org/labkey/api/security/Directive.java @@ -1,24 +1,27 @@ package org.labkey.api.security; +import org.labkey.api.settings.StartupProperty; import org.labkey.api.util.SafeToRenderEnum; /** * 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 implements SafeToRenderEnum +public enum Directive implements StartupProperty, SafeToRenderEnum { - Connection("connect-src"), - Font("font-src"), - Frame("frame-src"), - Image("image-src"), - Style("style-src"); + 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"), + Style("style-src", "Sources for stylesheets"); private final String _cspDirective; + private final String _description; - Directive(String cspDirective) + Directive(String cspDirective, String description) { _cspDirective = cspDirective; + _description = description; } public String getCspDirective() @@ -30,4 +33,10 @@ public String getSubstitutionKey() { return name().toUpperCase() + ".SOURCES"; } + + @Override + public String getDescription() + { + return _description + " (" + _cspDirective + "). Multiple hosts, separated by a space, can be provided."; + } } diff --git a/core/src/org/labkey/core/CoreModule.java b/core/src/org/labkey/core/CoreModule.java index 9ca0ddb1344..ace4488d70a 100644 --- a/core/src/org/labkey/core/CoreModule.java +++ b/core/src/org/labkey/core/CoreModule.java @@ -1050,6 +1050,7 @@ public void shutdownStarted() // populate look and feel settings and site settings with values read from startup properties as appropriate for not bootstrap populateLookAndFeelResourcesWithStartupProps(); + AllowedExternalResourceHosts.registerStartupProperties(); AllowedExternalResourceHosts.registerHosts(); WriteableLookAndFeelProperties.populateLookAndFeelWithStartupProps(); WriteableAppProps.populateSiteSettingsWithStartupProps(); diff --git a/core/src/org/labkey/core/CoreUpgradeCode.java b/core/src/org/labkey/core/CoreUpgradeCode.java index cf8a6ff3fdf..be1d5204592 100644 --- a/core/src/org/labkey/core/CoreUpgradeCode.java +++ b/core/src/org/labkey/core/CoreUpgradeCode.java @@ -128,9 +128,9 @@ private static void toLowerCaseWithCounterSeqs(Container container) { TableInfo tableInfo = CoreSchema.getInstance().getTableInfoDbSequences(); SQLFragment toLowerSql = new SQLFragment("UPDATE ").append(tableInfo) - .append(" SET Name = LOWER(Name) ") - .append(" WHERE Container = ? AND NAME LIKE 'SampleNameGenCounter-%'") - .add(container); + .append(" SET Name = LOWER(Name) ") + .append(" WHERE Container = ? AND NAME LIKE 'SampleNameGenCounter-%'") + .add(container); new SqlExecutor(tableInfo.getSchema()).execute(toLowerSql); } @@ -147,9 +147,9 @@ public static void makeWithCounterCaseInsensitive(ModuleContext context) TableInfo tableInfo = CoreSchema.getInstance().getTableInfoDbSequences(); SQLFragment sql = new SQLFragment() - .append("SELECT DISTINCT Container\n") - .append("FROM ").append(tableInfo, "seq") - .append(" WHERE seq.NAME LIKE 'SampleNameGenCounter-%'"); + .append("SELECT DISTINCT Container\n") + .append("FROM ").append(tableInfo, "seq") + .append(" WHERE seq.NAME LIKE 'SampleNameGenCounter-%'"); @NotNull List containers = new SqlSelector(tableInfo.getSchema(), sql).getArrayList(String.class); if (containers.isEmpty()) @@ -188,13 +188,6 @@ public static void migrateAllowedExternalConnectionHosts(ModuleContext context) .map(host -> new AllowedHost(Directive.Connection, host)) .toList(); - try - { - AllowedExternalResourceHosts.saveAllowedHosts(allowedHosts, context.getUpgradeUser()); - } - catch (JsonProcessingException e) - { - ExceptionUtil.logExceptionToMothership(null, e); - } + AllowedExternalResourceHosts.saveAllowedHosts(allowedHosts, context.getUpgradeUser()); } } \ No newline at end of file diff --git a/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java b/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java index be33e3758c6..988ff295090 100644 --- a/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java +++ b/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java @@ -4,9 +4,12 @@ import com.fasterxml.jackson.core.type.TypeReference; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; +import org.labkey.api.module.ModuleLoader; import org.labkey.api.security.Directive; import org.labkey.api.security.User; import org.labkey.api.settings.AppProps; +import org.labkey.api.settings.StandardStartupPropertyHandler; +import org.labkey.api.settings.StartupPropertyEntry; import org.labkey.api.settings.WriteableAppProps; import org.labkey.api.util.ExceptionUtil; import org.labkey.api.util.JsonUtil; @@ -32,11 +35,21 @@ private AllowedExternalResourceHosts() public record AllowedHost(Directive directive, String host) { } - public static void saveAllowedHosts(@Nullable Collection allowedHosts, User user) throws JsonProcessingException + public static void saveAllowedHosts(@Nullable Collection allowedHosts, User user) { if (null != allowedHosts) { - String json = JsonUtil.createDefaultMapper().writeValueAsString(allowedHosts); + final String json; + + try + { + json = JsonUtil.createDefaultMapper().writeValueAsString(allowedHosts); + } + catch (JsonProcessingException e) + { + ExceptionUtil.logExceptionToMothership(null, e); + return; + } WriteableAppProps props = AppProps.getWriteableInstance(); props.setAllowedExternalResourceHosts(json); @@ -80,4 +93,21 @@ public static void registerHosts() list.forEach(sub -> ContentSecurityPolicyFilter.registerAllowedSources(sub.directive(), sub.host())); LOG.debug("Registered [{}] as allowed external sources", list); } + + public static void registerStartupProperties() + { + ModuleLoader.getInstance().handleStartupProperties(new StandardStartupPropertyHandler<>("AllowedExternalResourceHosts", Directive.class) + { + @Override + public void handle(Map properties) + { + List allowedHosts = properties.entrySet().stream() + .flatMap(e -> Arrays.stream(e.getValue().getValue().split(" ")) + .map(host -> new AllowedHost(e.getKey(), host)) + ) + .toList(); + saveAllowedHosts(allowedHosts, User.getAdminServiceUser()); + } + }); + } } From 06ca71a314822f8615d1e23a6e698c54b3339541 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 24 Feb 2025 11:01:54 -0800 Subject: [PATCH 10/23] Report CSP status (enforce CSP & expected substitutions in place). Handle cspVersion in report action. Labels and clean up HTML. --- .../filters/ContentSecurityPolicyFilter.java | 25 ++++++++ .../labkey/core/admin/AdminController.java | 7 +- .../core/admin/addNewExternalSource.jsp | 51 +++++++++++++-- .../core/admin/existingExternalSources.jsp | 64 +++++++++++-------- 4 files changed, 112 insertions(+), 35 deletions(-) diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index dfbbe774b5d..3bf9f957564 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -254,6 +254,31 @@ private static void regenerateSubstitutionMap() } } + public static boolean hasCsp(ContentSecurityPolicyType type) + { + return CSP_FILTERS.stream().anyMatch(filter -> type.equals(filter._type)); + } + + public static List getMissingSubstitutions(ContentSecurityPolicyType type) + { + ContentSecurityPolicyFilter filter = CSP_FILTERS.stream().filter(f -> type.equals(f._type)).findFirst().orElse(null); + final List ret; + if (filter == null) + { + ret = Collections.emptyList(); + } + else + { + String template = filter._policyTemplate; + ret = Arrays.stream(Directive.values()) + .map(dir -> "${" + dir.getSubstitutionKey() + "}") + .filter(key -> !template.contains(key)) + .toList(); + } + + return ret; + } + public static class TestCase extends Assert { @Test diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 893240462f6..1d1c5e8a903 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -11109,7 +11109,8 @@ public List getExistingValuesList() //for updated urls that comes in as String values from the jsp/html form if (null != getExistingValues()) { - return new ArrayList<>(Arrays.asList(getExistingValues().split("\n"))); + // The JavaScript delimits with "\n". Not sure where these "\r"s are coming from, but we need to strip them. + return new ArrayList<>(Arrays.asList(getExistingValues().replace("\r", "").split("\n"))); } return _existingValuesList; } @@ -11263,6 +11264,7 @@ public void setExistingValue(String existingValue) public String getExistingValues() { + // The JSP JavaScript delimits with "\n". Not sure where these "\r"s are coming from, but we need to strip them. return _existingValues.replace("\r", ""); } @@ -11953,6 +11955,9 @@ public Object execute(SimpleApiJsonForm form, BindException errors) throws Excep String labkeyVersion = request.getParameter("labkeyVersion"); if (null != labkeyVersion) jsonObj.put("labkeyVersion", labkeyVersion); + String cspVersion = request.getParameter("cspVersion"); + if (null != cspVersion) + jsonObj.put("cspVersion", cspVersion); var jsonStr = jsonObj.toString(2); _log.warn("ContentSecurityPolicy warning on page: " + urlString + "\n" + jsonStr); } diff --git a/core/src/org/labkey/core/admin/addNewExternalSource.jsp b/core/src/org/labkey/core/admin/addNewExternalSource.jsp index 879aa1539a7..051e347cc2e 100644 --- a/core/src/org/labkey/core/admin/addNewExternalSource.jsp +++ b/core/src/org/labkey/core/admin/addNewExternalSource.jsp @@ -19,20 +19,55 @@ <%@ page import="org.labkey.api.data.Container" %> <%@ page import="org.labkey.api.security.Directive" %> <%@ page import="org.labkey.api.security.permissions.AdminOperationsPermission" %> +<%@ page import="org.labkey.api.settings.OptionalFeatureService" %> +<%@ page import="org.labkey.filters.ContentSecurityPolicyFilter" %> <%@ page import="java.util.Arrays" %> +<%@ page import="java.util.List" %> +<%@ page import="static org.labkey.filters.ContentSecurityPolicyFilter.FEATURE_FLAG_DISABLE_ENFORCE_CSP" %> <%@ page extends="org.labkey.api.jsp.JspBase" %> <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <% Container c = getContainer(); boolean isTroubleshooter = c.isRoot() && !c.hasPermission(getUser(), AdminOperationsPermission.class); + + String noEffect = "External resource hosts can be configured below, but they'll have no effect until "; + String message; + + boolean hasEnforce = ContentSecurityPolicyFilter.hasCsp(ContentSecurityPolicyFilter.ContentSecurityPolicyType.Enforce); + if (hasEnforce) + { + message = "This server is configured with an enforce Content Security Policy (CSP) "; + boolean disabled = OptionalFeatureService.get().isFeatureEnabled(FEATURE_FLAG_DISABLE_ENFORCE_CSP); + + if (disabled) + { + message += "but it's currently disabled via an experimental feature flag! " + noEffect + "the enforce CSP is re-enabled."; + } + else + { + List missing = ContentSecurityPolicyFilter.getMissingSubstitutions(ContentSecurityPolicyFilter.ContentSecurityPolicyType.Enforce); + int count = missing.size(); + message += missing.isEmpty() ? + "that includes all the expected substitutions." : + "but the following substitution" + (count > 1 ? "s are" : " is") + " missing: " + String.join(", ", missing) + + ". External resource hosts for " + (count > 1 ? "these directives" : "this directive") + " can be configured below, but they'll have no effect."; + } + } + else + { + message = "This server is not configured with an enforce Content Security Policy (CSP); LabKey strongly recommends " + + "configuring a strict enforce CSP. " + noEffect + "an enforce CSP is configured."; + } %>

- For security reasons, the standard LabKey Content Security Policy (CSP) restricts the hosts that browsers can - use as resource origins. By default, only sources from this server are allowed; other server hosts must be - configured below to enable them to be used as external sources. All provided hosts are added into the CSP - using the \${} substitution key shown next to each directive. + <%=h(message)%> +

+

+ The standard LabKey CSP restricts the hosts that browsers can use as resource origins. By default, only sources + from this server are allowed; other server hosts must be configured below to enable them to be used as external + sources. All provided hosts are added into the CSP using the \${} substitution key shown next to each directive. <% if (!isTroubleshooter) { @@ -54,10 +89,14 @@ - + - + + + + diff --git a/core/src/org/labkey/core/admin/existingExternalSources.jsp b/core/src/org/labkey/core/admin/existingExternalSources.jsp index f29a934a363..679bcf709d0 100644 --- a/core/src/org/labkey/core/admin/existingExternalSources.jsp +++ b/core/src/org/labkey/core/admin/existingExternalSources.jsp @@ -64,24 +64,30 @@ - <% - ExternalSourcesForm bean = (ExternalSourcesForm) HttpView.currentModel(); - List existingAllowedHosts = bean.getSavedAllowedHosts(); - existingAllowedHosts.sort(Comparator.comparing(AllowedHost::directive).thenComparing(AllowedHost::host)); - %> +<% + ExternalSourcesForm bean = (ExternalSourcesForm) HttpView.currentModel(); + List existingAllowedHosts = bean.getSavedAllowedHosts(); + existingAllowedHosts.sort(Comparator.comparing(AllowedHost::directive).thenComparing(AllowedHost::host)); +%>
<%=select().name("newDirective").addOptions( + <%=select().name("newDirective").id("newDirective").addStyle("width:300px").addOptions( Arrays.stream(Directive.values()).collect(LabKeyCollectors.toLinkedMap(Enum::name, d->d.getCspDirective() + " ${" + d.getSubstitutionKey() + "}")) )%> 

<%= button("Add").submit(true) %>
- <% if (existingAllowedHosts.isEmpty()) { %> - - <% } %> - - <% - int num = 1; - for (AllowedHost sub : existingAllowedHosts) { - String directiveId = "directive" + num; - String hostId = "host" + num; - %> +<% + if (existingAllowedHosts.isEmpty()) + { +%> + +<% + } + else + { +%> + +<% + int num = 1; + for (AllowedHost sub : existingAllowedHosts) { + String directiveId = "directive" + num; + String hostId = "host" + num; +%> - @@ -95,18 +101,20 @@ - <% +<% num++; - } - %> + } + + if (!existingAllowedHosts.isEmpty()) + { +%>
No External Resource Hosts have been configured.
No External Resource Hosts have been configured.
DirectiveHost
/>
- <% if (!existingAllowedHosts.isEmpty()) { %> - - - - - -
<%=isTroubleshooter ? button("Done").href(urlProvider(AdminUrls.class).getAdminConsoleURL()) : button("Save").primary(true).onClick("return saveAll();")%> - - <% } %> + + + +
<%=isTroubleshooter ? button("Done").href(urlProvider(AdminUrls.class).getAdminConsoleURL()) : button("Save").primary(true).onClick("return saveAll();")%> +<% + } + } +%> From 49e0a0cdb32547d652d4b119dc64e11caa659220 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 24 Feb 2025 12:47:16 -0800 Subject: [PATCH 11/23] Turn CSP_FILTERS into a Map. Attempt to determine cspVersion of each CSP and report them via metrics. --- api/src/org/labkey/api/ApiModule.java | 1 + .../filters/ContentSecurityPolicyFilter.java | 93 ++++++++++++++----- 2 files changed, 71 insertions(+), 23 deletions(-) diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index e6029ffb627..a5b92e03c19 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -254,6 +254,7 @@ protected void doStartup(ModuleContext moduleContext) { SystemMaintenance.addTask(new ApiKeyMaintenanceTask()); AuthenticationManager.registerMetricsProvider(); + ContentSecurityPolicyFilter.registerMetricsProvider(); ApiKeyManager.get().handleStartupProperties(); MailHelper.init(); // Handle optional feature and system maintenance startup properties as late as possible; we want all optional diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index 3bf9f957564..703d610ad69 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -14,15 +14,18 @@ import org.apache.logging.log4j.Logger; import org.junit.Assert; import org.junit.Test; +import org.labkey.api.collections.CopyOnWriteHashMap; import org.labkey.api.security.Directive; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.OptionalFeatureService; +import org.labkey.api.usageMetrics.UsageMetricsService; import org.labkey.api.util.CspCommentScanner; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.StringExpression; import org.labkey.api.util.StringExpressionFactory; import org.labkey.api.util.StringExpressionFactory.AbstractStringExpression.NullValueBehavior; import org.labkey.api.util.logging.LogHelper; +import org.labkey.api.view.ActionURL; import java.io.IOException; import java.security.SecureRandom; @@ -33,7 +36,6 @@ import java.util.List; import java.util.Map; import java.util.Set; -import java.util.concurrent.CopyOnWriteArrayList; import java.util.stream.Collectors; @@ -48,7 +50,7 @@ public class ContentSecurityPolicyFilter implements Filter 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 List CSP_FILTERS = new CopyOnWriteArrayList<>(); + 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(); @@ -60,6 +62,7 @@ public class ContentSecurityPolicyFilter implements Filter // Per-filter-instance parameters that are set in init() and never changed private ContentSecurityPolicyType _type = ContentSecurityPolicyType.Enforce; private String _policyTemplate = null; + private String _cspVersion = "Unknown"; // Updated after every change to "allowed sources" private StringExpression _policyExpression = null; @@ -110,9 +113,11 @@ public void init(FilterConfig filterConfig) throws ServletException // Replace REPORT_PARAMETER_SUBSTITUTION now since its value is static s = StringExpressionFactory.create(s, false, NullValueBehavior.KeepSubstitution) - .eval(Map.of(REPORT_PARAMETER_SUBSTITUTION, "labkeyVersion=" + PageFlowUtil.encodeURIComponent(AppProps.getInstance().getReleaseVersion()))); + .eval(Map.of(REPORT_PARAMETER_SUBSTITUTION, "labkeyVersion=" + PageFlowUtil.encodeURIComponent(AppProps.getInstance().getReleaseVersion()))); _policyTemplate = s; + + extractCspVersion(s); } else if ("disposition".equalsIgnoreCase(paramName)) { @@ -128,23 +133,10 @@ else if ("disposition".equalsIgnoreCase(paramName)) } } - regeneratePolicyExpression(); - CSP_FILTERS.add(this); - } + if (CSP_FILTERS.put(_type, this) != null) + throw new ServletException("ContentSecurityPolicyFilter is misconfigured, duplicate policies of type: " + _type); - // Make all the "allowed sources" substitutions at init() and whenever the allowed sources map changes. With this, - // the only substitution needed on a per-request basis is the nonce value. - private void regeneratePolicyExpression() - { - final String allowSubstitutedPolicy; - - synchronized (ALLOWED_SOURCES_LOCK) - { - allowSubstitutedPolicy = StringExpressionFactory.create(_policyTemplate, false, NullValueBehavior.KeepSubstitution) - .eval(ALLOWED_SOURCES_SUBSTITUTION_MAP); - } - - _policyExpression = StringExpressionFactory.create(allowSubstitutedPolicy, false, NullValueBehavior.KeepSubstitution); + regeneratePolicyExpression(); } /** Filter out block comments and replace special characters in the provided policy */ @@ -168,6 +160,55 @@ public static String filterPolicy(String policy) return s; } + /** + * Extract the cspVersion parameter value from the report-uri directive, if possible. Otherwise, cspVersion is left + * as "Unknown". This value is reported as part of usage metrics. + */ + private void extractCspVersion(String s) + { + String directive = "report-uri"; + int dirIdx = StringUtils.indexOfIgnoreCase(s, directive); + + if (dirIdx != -1) + { + int start = StringUtils.indexOfIgnoreCase(s, directive, dirIdx); + int end = s.indexOf(';', start); + + if (end == -1) + end = s.length(); + + try + { + ActionURL reportUrl = new ActionURL(s.substring(start, end)); + String cspVersion = reportUrl.getParameter("cspVersion"); + + if (null != cspVersion) + _cspVersion = cspVersion; + } + catch (IllegalArgumentException e) + { + LOG.warn("Unable to parse {} URI", directive, e); + } + } + + LOG.debug("CspVersion: {}", _cspVersion); + } + + // Make all the "allowed sources" substitutions at init() and whenever the allowed sources map changes. With this, + // the only substitution needed on a per-request basis is the nonce value. + private void regeneratePolicyExpression() + { + final String allowSubstitutedPolicy; + + synchronized (ALLOWED_SOURCES_LOCK) + { + allowSubstitutedPolicy = StringExpressionFactory.create(_policyTemplate, false, NullValueBehavior.KeepSubstitution) + .eval(ALLOWED_SOURCES_SUBSTITUTION_MAP); + } + + _policyExpression = StringExpressionFactory.create(allowSubstitutedPolicy, false, NullValueBehavior.KeepSubstitution); + } + @Override public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { @@ -250,18 +291,18 @@ private static void regenerateSubstitutionMap() ALLOWED_SOURCES_SUBSTITUTION_MAP.put("LABKEY.ALLOWED.CONNECTIONS", ALLOWED_SOURCES_SUBSTITUTION_MAP.get(Directive.Connection.getSubstitutionKey())); // Tell each registered ContentSecurityPolicyFilter to refresh its policy template based on the new substitution map - CSP_FILTERS.forEach(ContentSecurityPolicyFilter::regeneratePolicyExpression); + CSP_FILTERS.values().forEach(ContentSecurityPolicyFilter::regeneratePolicyExpression); } } public static boolean hasCsp(ContentSecurityPolicyType type) { - return CSP_FILTERS.stream().anyMatch(filter -> type.equals(filter._type)); + return CSP_FILTERS.get(type) != null; } public static List getMissingSubstitutions(ContentSecurityPolicyType type) { - ContentSecurityPolicyFilter filter = CSP_FILTERS.stream().filter(f -> type.equals(f._type)).findFirst().orElse(null); + ContentSecurityPolicyFilter filter = CSP_FILTERS.get(type); final List ret; if (filter == null) { @@ -279,6 +320,12 @@ public static List getMissingSubstitutions(ContentSecurityPolicyType typ return ret; } + public static void registerMetricsProvider() + { + UsageMetricsService.get().registerUsageMetrics("API", () -> Map.of("cspFilters", CSP_FILTERS.values().stream() + .collect(Collectors.toMap(filter -> filter._type, filter -> filter._cspVersion)))); + } + public static class TestCase extends Assert { @Test @@ -413,7 +460,7 @@ public void testSubstitutionMap() private void verifySubstitutionInPolicyExpressions(String value, int expectedCount) { - List failures = CSP_FILTERS.stream() + List failures = CSP_FILTERS.values().stream() .map(filter -> filter._policyExpression.eval(Map.of())) .filter(policy -> StringUtils.countMatches(policy, value) != expectedCount) .toList(); From 6e09508e8b7921d3aa776f6aa7ba3e25c4410c49 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 24 Feb 2025 12:51:23 -0800 Subject: [PATCH 12/23] Update 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 703d610ad69..2b4352f91ef 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -266,7 +266,7 @@ public static void unregisterAllowedSources(Directive directive, String key) } } - // Pre-generate the substitution map on every register/unregister + // Regenerate the substitution map and all policy expressions on every register/unregister private static void regenerateSubstitutionMap() { synchronized (ALLOWED_SOURCES_LOCK) From 5b1f3d8d507642e3d4c4997a6309c6b00f304711 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 24 Feb 2025 15:07:27 -0800 Subject: [PATCH 13/23] Clarify ambiguous name to CaseInsensitiveKeyedHashSetValuedMap. Add new CaseInsensitiveHashSetValuedMap (with case-insensitive String values). Add validation. --- .../CaseInsensitiveHashSetValuedMap.java | 12 +-- .../CaseInsensitiveKeyedHashSetValuedMap.java | 38 +++++++++ .../org/labkey/api/module/ModuleLoader.java | 4 +- .../labkey/core/admin/AdminController.java | 80 +++++++++++++------ 4 files changed, 103 insertions(+), 31 deletions(-) create mode 100644 api/src/org/labkey/api/collections/CaseInsensitiveKeyedHashSetValuedMap.java diff --git a/api/src/org/labkey/api/collections/CaseInsensitiveHashSetValuedMap.java b/api/src/org/labkey/api/collections/CaseInsensitiveHashSetValuedMap.java index 079c8c6cc4b..bbfe5a3fefe 100644 --- a/api/src/org/labkey/api/collections/CaseInsensitiveHashSetValuedMap.java +++ b/api/src/org/labkey/api/collections/CaseInsensitiveHashSetValuedMap.java @@ -17,22 +17,22 @@ import org.apache.commons.collections4.multimap.AbstractSetValuedMap; -import java.util.HashSet; +import java.util.HashMap; import java.util.Set; /** - * A case-insensitive version of org.apache.commons.collections4.multimap.HashSetValuedHashMap + * An org.apache.commons.collections4.multimap.HashSetValuedHashMap with case-insensitive String values */ -public class CaseInsensitiveHashSetValuedMap extends AbstractSetValuedMap implements CaseInsensitiveCollection +public class CaseInsensitiveHashSetValuedMap extends AbstractSetValuedMap implements CaseInsensitiveCollection { public CaseInsensitiveHashSetValuedMap() { - super(new CaseInsensitiveHashMap<>()); + super(new HashMap<>()); } @Override - protected Set createCollection() + protected Set createCollection() { - return new HashSet<>(); + return new CaseInsensitiveHashSet(); } } diff --git a/api/src/org/labkey/api/collections/CaseInsensitiveKeyedHashSetValuedMap.java b/api/src/org/labkey/api/collections/CaseInsensitiveKeyedHashSetValuedMap.java new file mode 100644 index 00000000000..400d4384b87 --- /dev/null +++ b/api/src/org/labkey/api/collections/CaseInsensitiveKeyedHashSetValuedMap.java @@ -0,0 +1,38 @@ +/* + * Copyright (c) 2016 LabKey Corporation + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.labkey.api.collections; + +import org.apache.commons.collections4.multimap.AbstractSetValuedMap; + +import java.util.HashSet; +import java.util.Set; + +/** + * An org.apache.commons.collections4.multimap.HashSetValuedHashMap with case-insensitive String keys + */ +public class CaseInsensitiveKeyedHashSetValuedMap extends AbstractSetValuedMap implements CaseInsensitiveCollection +{ + public CaseInsensitiveKeyedHashSetValuedMap() + { + super(new CaseInsensitiveHashMap<>()); + } + + @Override + protected Set createCollection() + { + return new HashSet<>(); + } +} diff --git a/api/src/org/labkey/api/module/ModuleLoader.java b/api/src/org/labkey/api/module/ModuleLoader.java index 3fb89f2c21e..7c39b7ea772 100644 --- a/api/src/org/labkey/api/module/ModuleLoader.java +++ b/api/src/org/labkey/api/module/ModuleLoader.java @@ -30,7 +30,7 @@ import org.labkey.api.action.UrlProviderService; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.CaseInsensitiveHashSet; -import org.labkey.api.collections.CaseInsensitiveHashSetValuedMap; +import org.labkey.api.collections.CaseInsensitiveKeyedHashSetValuedMap; import org.labkey.api.collections.CaseInsensitiveTreeMap; import org.labkey.api.collections.CaseInsensitiveTreeSet; import org.labkey.api.collections.CopyOnWriteHashMap; @@ -229,7 +229,7 @@ public enum ModuleState // Allow multiple StartupPropertyHandlers with the same scope as long as the StartupProperty impl class is different. private final Set> _startupPropertyHandlers = new ConcurrentSkipListSet<>(Comparator.comparing((StartupPropertyHandler sph)->sph.getScope(), String.CASE_INSENSITIVE_ORDER).thenComparing(StartupPropertyHandler::getStartupPropertyClassName)); - private final MultiValuedMap _startupPropertyMap = new CaseInsensitiveHashSetValuedMap<>(); + private final MultiValuedMap _startupPropertyMap = new CaseInsensitiveKeyedHashSetValuedMap<>(); private ModuleLoader() { diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 1d1c5e8a903..47d3e301434 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -26,6 +26,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.beanutils.ConversionException; +import org.apache.commons.collections4.MultiValuedMap; import org.apache.commons.collections4.map.LRUMap; import org.apache.commons.io.FileUtils; import org.apache.commons.lang3.ArrayUtils; @@ -93,6 +94,7 @@ import org.labkey.api.cloud.CloudStoreService; import org.labkey.api.collections.CaseInsensitiveHashMap; import org.labkey.api.collections.CaseInsensitiveHashSet; +import org.labkey.api.collections.CaseInsensitiveHashSetValuedMap; import org.labkey.api.compliance.ComplianceFolderSettings; import org.labkey.api.compliance.ComplianceService; import org.labkey.api.compliance.PhiColumnBehavior; @@ -11224,6 +11226,7 @@ public boolean isSaveAll() return _saveAll; } + @SuppressWarnings("unused") public void setSaveAll(boolean saveAll) { _saveAll = saveAll; @@ -11287,51 +11290,82 @@ private AllowedHost getAllowedHost(String value, BindException errors) errors.addError(new LabKeyError("Can't parse allowed host.")); return null; } - Directive dir = EnumUtils.getEnum(Directive.class, parts[0], null); - if (null == dir) - { - errors.addError(new LabKeyError("Unknown directive.")); - return null; - } - return new AllowedHost(dir, parts[1]); + return validateHost(parts[0], parts[1], errors); } private List getExistingAllowedHosts(BindException errors) { - return Arrays.stream(getExistingValues().split("\n")) + List existing = Arrays.stream(getExistingValues().split("\n")) .map(value-> getAllowedHost(value, errors)) .toList(); + + if (errors.hasErrors()) + return null; + + return checkDuplicates(existing, errors); } private List validateNewAllowedHost(BindException errors) throws JsonProcessingException { - Directive directive = EnumUtils.getEnum(Directive.class, getNewDirective()); - String host = StringUtils.trimToEmpty(getNewHost()); + AllowedHost newAllowedHost = validateHost(getNewDirective(), getNewHost(), errors); - if (getNewDirective() == null) + if (errors.hasErrors()) + return null; + + List hosts = getSavedAllowedHosts(); + hosts.add(newAllowedHost); + + return checkDuplicates(hosts, errors); + } + + // Lenient for now: no blanks or unknown directives + private AllowedHost validateHost(String directiveString, String host, BindException errors) + { + AllowedHost ret = null; + + if (StringUtils.isEmpty(directiveString)) { errors.addError(new LabKeyError("Directive must not be blank.")); } - else if (null == directive) + else if (StringUtils.isEmpty(host)) { - errors.addError(new LabKeyError("Unrecognized directive.")); + errors.addError(new LabKeyError("Host must not be blank.")); } - - if (StringUtils.isEmpty(host)) + else { - errors.addError(new LabKeyError("Redirect host name must not be blank.")); + Directive directive = EnumUtils.getEnum(Directive.class, directiveString); + + if (null == directive) + { + errors.addError(new LabKeyError("Unknown directive: " + directiveString)); + } + else + { + ret = new AllowedHost(directive, host.trim()); + } } - // TODO: Validate host - set errors - // TODO: Validate no duplicates - set errors + return ret; + } - if (errors.hasErrors()) - return null; + /** + * Check for duplicates in hosts: within each Directive, hosts are checked using case-sensitive comparisons + * @param hosts a list of AllowedHost objects to check for duplicates + * @param errors errors to populate + * @return hosts if there are no duplicates, otherwise {@code null} + */ + private @Nullable List checkDuplicates(List hosts, BindException errors) + { + // Not a simple Set check since we want host check to be case-insensitive + MultiValuedMap map = new CaseInsensitiveHashSetValuedMap<>(); - List ret = getSavedAllowedHosts(); - ret.add(new AllowedHost(directive, host)); + hosts.forEach(allowedHost -> { + String host = allowedHost.host().trim(); + if (!map.put(allowedHost.directive(), host)) + errors.addError(new LabKeyError(String.format("'%1$s' already exists. Duplicate values are not allowed.", allowedHost))); + }); - return ret; + return errors.hasErrors() ? null : hosts; } // Returns a mutable list From 5ad6d3222ba617d07bce7b156e8fa5f85b8e7368 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 24 Feb 2025 15:18:13 -0800 Subject: [PATCH 14/23] When adding new hosts, persist the most recent directive in the drop-down list --- .../labkey/core/admin/AdminController.java | 2 +- .../core/admin/addNewExternalSource.jsp | 21 ++++++++++++++----- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 47d3e301434..64df2ba03f8 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -11388,7 +11388,7 @@ public ModelAndView getView(ExternalSourcesForm form, boolean reshow, BindExcept { boolean isTroubleshooter = !getContainer().hasPermission(getUser(), AdminOperationsPermission.class); - JspView newView = new JspView<>("/org/labkey/core/admin/addNewExternalSource.jsp", null, errors); + JspView newView = new JspView<>("/org/labkey/core/admin/addNewExternalSource.jsp", form, errors); newView.setTitle(isTroubleshooter ? "Overview" : "Register New External Resource Host"); newView.setFrame(WebPartView.FrameType.PORTAL); JspView existingView = new JspView<>("/org/labkey/core/admin/existingExternalSources.jsp", form, errors); diff --git a/core/src/org/labkey/core/admin/addNewExternalSource.jsp b/core/src/org/labkey/core/admin/addNewExternalSource.jsp index 051e347cc2e..c955e049160 100644 --- a/core/src/org/labkey/core/admin/addNewExternalSource.jsp +++ b/core/src/org/labkey/core/admin/addNewExternalSource.jsp @@ -15,15 +15,17 @@ * limitations under the License. */ %> +<%@ page import="org.apache.commons.lang3.EnumUtils" %> <%@ page import="org.labkey.api.collections.LabKeyCollectors" %> <%@ page import="org.labkey.api.data.Container" %> <%@ page import="org.labkey.api.security.Directive" %> <%@ page import="org.labkey.api.security.permissions.AdminOperationsPermission" %> <%@ page import="org.labkey.api.settings.OptionalFeatureService" %> +<%@ page import="org.labkey.core.admin.AdminController.ExternalSourcesForm" %> <%@ page import="org.labkey.filters.ContentSecurityPolicyFilter" %> +<%@ page import="static org.labkey.filters.ContentSecurityPolicyFilter.FEATURE_FLAG_DISABLE_ENFORCE_CSP" %> <%@ page import="java.util.Arrays" %> <%@ page import="java.util.List" %> -<%@ page import="static org.labkey.filters.ContentSecurityPolicyFilter.FEATURE_FLAG_DISABLE_ENFORCE_CSP" %> <%@ page extends="org.labkey.api.jsp.JspBase" %> <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <% @@ -65,8 +67,8 @@ <%=h(message)%>

- The standard LabKey CSP restricts the hosts that browsers can use as resource origins. By default, only sources - from this server are allowed; other server hosts must be configured below to enable them to be used as external + The standard LabKey CSP restricts hosts that browsers can use as resource origins. By default, only sources from + this server are allowed; other server hosts must be configured below to enable them to be used as external sources. All provided hosts are added into the CSP using the \${} substitution key shown next to each directive. <% if (!isTroubleshooter) @@ -85,13 +87,22 @@ <% if (!isTroubleshooter) { + ExternalSourcesForm form = (ExternalSourcesForm)getModelBean(); + Directive directive = EnumUtils.getEnum(Directive.class, form.getNewDirective()); %> - From 4ed570334ad007b4d8722d6fec9b6213bca71995 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 24 Feb 2025 17:34:08 -0800 Subject: [PATCH 15/23] Add the actual policy (substituted and not) to metrics --- api/src/org/labkey/filters/ContentSecurityPolicyFilter.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index 2b4352f91ef..3ea14e66f75 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -323,7 +323,8 @@ public static List getMissingSubstitutions(ContentSecurityPolicyType typ public static void registerMetricsProvider() { UsageMetricsService.get().registerUsageMetrics("API", () -> Map.of("cspFilters", CSP_FILTERS.values().stream() - .collect(Collectors.toMap(filter -> filter._type, filter -> filter._cspVersion)))); + .collect(Collectors.toMap(filter -> filter._type, + filter -> Map.of("version", filter._cspVersion, "csp", filter._policyTemplate, "cspSubstituted", filter._policyExpression.getSource()))))); } public static class TestCase extends Assert From 7d270ee663b5eedafba6583cd8c3179907a8d922 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Mon, 24 Feb 2025 18:18:23 -0800 Subject: [PATCH 16/23] Behold the super sophisticated CSP parser --- .../filters/ContentSecurityPolicyFilter.java | 23 +++++++++++-------- .../core/admin/addNewExternalSource.jsp | 2 +- .../AllowedExternalResourceHosts.java | 17 +++++++++----- 3 files changed, 25 insertions(+), 17 deletions(-) diff --git a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java index 3ea14e66f75..b7c96edf984 100644 --- a/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java +++ b/api/src/org/labkey/filters/ContentSecurityPolicyFilter.java @@ -15,6 +15,7 @@ import org.junit.Assert; import org.junit.Test; import org.labkey.api.collections.CopyOnWriteHashMap; +import org.labkey.api.collections.LabKeyCollectors; import org.labkey.api.security.Directive; import org.labkey.api.settings.AppProps; import org.labkey.api.settings.OptionalFeatureService; @@ -113,7 +114,7 @@ public void init(FilterConfig filterConfig) throws ServletException // Replace REPORT_PARAMETER_SUBSTITUTION now since its value is static s = StringExpressionFactory.create(s, false, NullValueBehavior.KeepSubstitution) - .eval(Map.of(REPORT_PARAMETER_SUBSTITUTION, "labkeyVersion=" + PageFlowUtil.encodeURIComponent(AppProps.getInstance().getReleaseVersion()))); + .eval(Map.of(REPORT_PARAMETER_SUBSTITUTION, "labkeyVersion=" + PageFlowUtil.encodeURIComponent(AppProps.getInstance().getReleaseVersion()))); _policyTemplate = s; @@ -166,20 +167,22 @@ public static String filterPolicy(String policy) */ private void extractCspVersion(String s) { + // Simple parser that should be compliant with https://www.w3.org/TR/CSP3/#parse-serialized-policy + Map cspMap = Arrays.stream(s.split(";")) + .map(String::trim) + .filter(line -> !line.isEmpty()) + .map(line -> line.split("\\s", 2)) + .filter(parts -> parts.length == 2) + .collect(LabKeyCollectors.toCaseInsensitiveLinkedMap(parts -> parts[0], parts -> parts[1])); + String directive = "report-uri"; - int dirIdx = StringUtils.indexOfIgnoreCase(s, directive); + String reportUri = cspMap.get(directive); - if (dirIdx != -1) + if (reportUri != null) { - int start = StringUtils.indexOfIgnoreCase(s, directive, dirIdx); - int end = s.indexOf(';', start); - - if (end == -1) - end = s.length(); - try { - ActionURL reportUrl = new ActionURL(s.substring(start, end)); + ActionURL reportUrl = new ActionURL(reportUri); String cspVersion = reportUrl.getParameter("cspVersion"); if (null != cspVersion) diff --git a/core/src/org/labkey/core/admin/addNewExternalSource.jsp b/core/src/org/labkey/core/admin/addNewExternalSource.jsp index c955e049160..e335d3ec215 100644 --- a/core/src/org/labkey/core/admin/addNewExternalSource.jsp +++ b/core/src/org/labkey/core/admin/addNewExternalSource.jsp @@ -23,9 +23,9 @@ <%@ page import="org.labkey.api.settings.OptionalFeatureService" %> <%@ page import="org.labkey.core.admin.AdminController.ExternalSourcesForm" %> <%@ page import="org.labkey.filters.ContentSecurityPolicyFilter" %> -<%@ page import="static org.labkey.filters.ContentSecurityPolicyFilter.FEATURE_FLAG_DISABLE_ENFORCE_CSP" %> <%@ page import="java.util.Arrays" %> <%@ page import="java.util.List" %> +<%@ page import="static org.labkey.filters.ContentSecurityPolicyFilter.FEATURE_FLAG_DISABLE_ENFORCE_CSP" %> <%@ page extends="org.labkey.api.jsp.JspBase" %> <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <% diff --git a/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java b/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java index 988ff295090..dd322b1eb8a 100644 --- a/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java +++ b/core/src/org/labkey/core/security/AllowedExternalResourceHosts.java @@ -101,12 +101,17 @@ public static void registerStartupProperties() @Override public void handle(Map properties) { - List allowedHosts = properties.entrySet().stream() - .flatMap(e -> Arrays.stream(e.getValue().getValue().split(" ")) - .map(host -> new AllowedHost(e.getKey(), host)) - ) - .toList(); - saveAllowedHosts(allowedHosts, User.getAdminServiceUser()); + // If any allowed-hosts startup properties are provided, they completely replace whatever values were + // previously configured + if (!properties.isEmpty()) + { + List allowedHosts = properties.entrySet().stream() + .flatMap(e -> Arrays.stream(e.getValue().getValue().split(" ")) + .map(host -> new AllowedHost(e.getKey(), host)) + ) + .toList(); + saveAllowedHosts(allowedHosts, User.getAdminServiceUser()); + } } }); } From 7e69abc0669a2f40a2fb469db5d324a7370adf41 Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 25 Feb 2025 16:17:16 -0800 Subject: [PATCH 17/23] No semicolons --- core/src/org/labkey/core/admin/AdminController.java | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 64df2ba03f8..d2d92f6c2e0 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -11318,18 +11318,22 @@ private List validateNewAllowedHost(BindException errors) throws Js return checkDuplicates(hosts, errors); } - // Lenient for now: no blanks or unknown directives + // Lenient for now: no unknown directives, no blank hosts or hosts with semicolons private AllowedHost validateHost(String directiveString, String host, BindException errors) { AllowedHost ret = null; if (StringUtils.isEmpty(directiveString)) { - errors.addError(new LabKeyError("Directive must not be blank.")); + errors.addError(new LabKeyError("Directive must not be blank")); } else if (StringUtils.isEmpty(host)) { - errors.addError(new LabKeyError("Host must not be blank.")); + errors.addError(new LabKeyError("Host must not be blank")); + } + else if (host.contains(";")) + { + errors.addError(new LabKeyError("Semicolons are not allowed in host names")); } else { From ce04d6c6da1476274fa7856969423002c0c01d2a Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Tue, 25 Feb 2025 17:03:05 -0800 Subject: [PATCH 18/23] App admins can edit allowed external resource hosts, allowed redirect hosts, and allowed file extensions --- core/src/org/labkey/core/admin/AdminController.java | 4 ++-- core/src/org/labkey/core/admin/addNewExternalSource.jsp | 9 +++++---- core/src/org/labkey/core/admin/addNewListValue.jsp | 4 ++-- .../org/labkey/core/admin/existingExternalSources.jsp | 4 ++-- core/src/org/labkey/core/admin/existingListValues.jsp | 4 ++-- 5 files changed, 13 insertions(+), 12 deletions(-) diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index d2d92f6c2e0..9577d50aaff 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -459,7 +459,7 @@ public static void registerAdminConsoleLinks() AdminConsole.addLink(Configuration, "site settings", new AdminUrlsImpl().getCustomizeSiteURL()); AdminConsole.addLink(Configuration, "system maintenance", new ActionURL(ConfigureSystemMaintenanceAction.class, root)); AdminConsole.addLink(Configuration, "allowed external redirect hosts", new ActionURL(AllowListAction.class, root).addParameter("type", AllowListType.Redirect.name()), TroubleshooterPermission.class); - AdminConsole.addLink(Configuration, "allowed external sources", new ActionURL(ExternalSourcesAction.class, root), TroubleshooterPermission.class); + AdminConsole.addLink(Configuration, "allowed external resource hosts", new ActionURL(ExternalSourcesAction.class, root), TroubleshooterPermission.class); AdminConsole.addLink(Configuration, "allowed file extensions", new ActionURL(AllowListAction.class, root).addParameter("type", AllowListType.FileExtension.name()), TroubleshooterPermission.class); // Diagnostics @@ -11390,7 +11390,7 @@ public void validateCommand(ExternalSourcesForm form, Errors errors) @Override public ModelAndView getView(ExternalSourcesForm form, boolean reshow, BindException errors) { - boolean isTroubleshooter = !getContainer().hasPermission(getUser(), AdminOperationsPermission.class); + boolean isTroubleshooter = !getContainer().hasPermission(getUser(), ApplicationAdminPermission.class); JspView newView = new JspView<>("/org/labkey/core/admin/addNewExternalSource.jsp", form, errors); newView.setTitle(isTroubleshooter ? "Overview" : "Register New External Resource Host"); diff --git a/core/src/org/labkey/core/admin/addNewExternalSource.jsp b/core/src/org/labkey/core/admin/addNewExternalSource.jsp index e335d3ec215..f0fd5e17706 100644 --- a/core/src/org/labkey/core/admin/addNewExternalSource.jsp +++ b/core/src/org/labkey/core/admin/addNewExternalSource.jsp @@ -19,10 +19,11 @@ <%@ page import="org.labkey.api.collections.LabKeyCollectors" %> <%@ page import="org.labkey.api.data.Container" %> <%@ page import="org.labkey.api.security.Directive" %> -<%@ page import="org.labkey.api.security.permissions.AdminOperationsPermission" %> +<%@ page import="org.labkey.api.security.permissions.ApplicationAdminPermission" %> <%@ page import="org.labkey.api.settings.OptionalFeatureService" %> <%@ page import="org.labkey.core.admin.AdminController.ExternalSourcesForm" %> <%@ page import="org.labkey.filters.ContentSecurityPolicyFilter" %> +<%@ page import="org.labkey.filters.ContentSecurityPolicyFilter.ContentSecurityPolicyType" %> <%@ page import="java.util.Arrays" %> <%@ page import="java.util.List" %> <%@ page import="static org.labkey.filters.ContentSecurityPolicyFilter.FEATURE_FLAG_DISABLE_ENFORCE_CSP" %> @@ -30,12 +31,12 @@ <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <% Container c = getContainer(); - boolean isTroubleshooter = c.isRoot() && !c.hasPermission(getUser(), AdminOperationsPermission.class); + boolean isTroubleshooter = !c.hasPermission(getUser(), ApplicationAdminPermission.class); String noEffect = "External resource hosts can be configured below, but they'll have no effect until "; String message; - boolean hasEnforce = ContentSecurityPolicyFilter.hasCsp(ContentSecurityPolicyFilter.ContentSecurityPolicyType.Enforce); + boolean hasEnforce = ContentSecurityPolicyFilter.hasCsp(ContentSecurityPolicyType.Enforce); if (hasEnforce) { message = "This server is configured with an enforce Content Security Policy (CSP) "; @@ -47,7 +48,7 @@ } else { - List missing = ContentSecurityPolicyFilter.getMissingSubstitutions(ContentSecurityPolicyFilter.ContentSecurityPolicyType.Enforce); + List missing = ContentSecurityPolicyFilter.getMissingSubstitutions(ContentSecurityPolicyType.Enforce); int count = missing.size(); message += missing.isEmpty() ? "that includes all the expected substitutions." : diff --git a/core/src/org/labkey/core/admin/addNewListValue.jsp b/core/src/org/labkey/core/admin/addNewListValue.jsp index 3a2d11dbbdd..d592738ad66 100644 --- a/core/src/org/labkey/core/admin/addNewListValue.jsp +++ b/core/src/org/labkey/core/admin/addNewListValue.jsp @@ -17,14 +17,14 @@ %> <%@ page import="org.labkey.api.admin.AdminUrls" %> <%@ page import="org.labkey.api.data.Container" %> -<%@ page import="org.labkey.api.security.permissions.AdminOperationsPermission" %> +<%@ page import="org.labkey.api.security.permissions.ApplicationAdminPermission" %> <%@ page import="org.labkey.api.view.HttpView" %> <%@ page import="org.labkey.core.admin.AdminController.AllowListForm" %> <%@ page extends="org.labkey.api.jsp.JspBase" %> <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <% Container c = getContainer(); - boolean isTroubleshooter = c.isRoot() && !c.hasPermission(getUser(), AdminOperationsPermission.class); + boolean isTroubleshooter = !c.hasPermission(getUser(), ApplicationAdminPermission.class);; AllowListForm bean = (AllowListForm) HttpView.currentModel(); %> diff --git a/core/src/org/labkey/core/admin/existingExternalSources.jsp b/core/src/org/labkey/core/admin/existingExternalSources.jsp index 679bcf709d0..e39ecf11c2e 100644 --- a/core/src/org/labkey/core/admin/existingExternalSources.jsp +++ b/core/src/org/labkey/core/admin/existingExternalSources.jsp @@ -17,7 +17,7 @@ %> <%@ page import="org.labkey.api.admin.AdminUrls" %> <%@ page import="org.labkey.api.data.Container" %> -<%@ page import="org.labkey.api.security.permissions.AdminOperationsPermission" %> +<%@ page import="org.labkey.api.security.permissions.ApplicationAdminPermission" %> <%@ page import="org.labkey.api.util.HtmlString" %> <%@ page import="org.labkey.api.view.HttpView" %> <%@ page import="org.labkey.core.admin.AdminController.ExternalSourcesForm" %> @@ -28,7 +28,7 @@ <%@ taglib prefix="labkey" uri="http://www.labkey.org/taglib" %> <% Container c = getContainer(); - boolean isTroubleshooter = c.isRoot() && !c.hasPermission(getUser(), AdminOperationsPermission.class); + boolean isTroubleshooter = !c.hasPermission(getUser(), ApplicationAdminPermission.class); %>
<%=select().name("newDirective").id("newDirective").addStyle("width:300px").addOptions( - Arrays.stream(Directive.values()).collect(LabKeyCollectors.toLinkedMap(Enum::name, d->d.getCspDirective() + " ${" + d.getSubstitutionKey() + "}")) + <%=select() + .name("newDirective") + .id("newDirective") + .addStyle("width:300px") + .selected(directive) + .addOptions( + Arrays.stream(Directive.values()) + .collect(LabKeyCollectors.toLinkedMap(Enum::name, d->d.getCspDirective() + " ${" + d.getSubstitutionKey() + "}") + ) )%>