diff --git a/announcements/src/org/labkey/announcements/AnnouncementModule.java b/announcements/src/org/labkey/announcements/AnnouncementModule.java index 7daa6d907a8..ba9c68000d2 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementModule.java +++ b/announcements/src/org/labkey/announcements/AnnouncementModule.java @@ -242,7 +242,8 @@ public void startBackgroundThreads() public @NotNull Set> getIntegrationTests() { return Set.of( - AnnouncementManager.TestCase.class + AnnouncementManager.TestCase.class, + AnnouncementsController.ContainerScopingTestCase.class ); } diff --git a/announcements/src/org/labkey/announcements/AnnouncementsController.java b/announcements/src/org/labkey/announcements/AnnouncementsController.java index 236fb8df9c3..fe47452e09c 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementsController.java +++ b/announcements/src/org/labkey/announcements/AnnouncementsController.java @@ -25,6 +25,8 @@ import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; import org.json.JSONObject; +import org.junit.Before; +import org.junit.Test; import org.labkey.announcements.model.AnnouncementDigestProvider; import org.labkey.announcements.model.AnnouncementFullModel; import org.labkey.announcements.model.AnnouncementManager; @@ -94,12 +96,14 @@ import org.labkey.api.security.SecurityManager; import org.labkey.api.security.User; import org.labkey.api.security.UserManager; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; import org.labkey.api.security.permissions.AdminPermission; import org.labkey.api.security.permissions.DeletePermission; import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.security.roles.ReaderRole; import org.labkey.api.security.roles.Role; import org.labkey.api.security.roles.RoleManager; import org.labkey.api.util.DateUtil; @@ -2447,9 +2451,15 @@ public boolean handlePost(SubscriptionBean bean, BindException errors) { throw new NotFoundException("No such message thread: " + id); } - // Make sure they have permission to see the container for the specific message they're - // requesting - if (!ann.lookupContainer().hasPermission(getUser(), ReadPermission.class)) + // Resolve permissions against the thread's own container since this action resolves threads cross-container. + // Use allowRead() not a plain container ReadPermission check: secure boards additionally enforce member-list membership. + Container threadContainer = ann.lookupContainer(); + if (threadContainer == null) + { + throw new UnauthorizedException(); + } + Permissions perm = getPermissions(threadContainer, getUser(), getSettings(threadContainer)); + if (!perm.allowRead(ann)) { throw new UnauthorizedException(); } @@ -2883,4 +2893,79 @@ public Object execute(ThreadForm form, BindException errors) return success(updatedThread); } } + + public static class ContainerScopingTestCase extends AbstractContainerScopingTest + { + private Container _folderA; + private Container _folderB; + private AnnouncementModel _thread; + + @Before + public void createThread() throws Exception + { + // A message thread that lives in folder B. SubscribeThreadAction resolves threads by global entityId + // (cross-container by design), so each test addresses B's thread through a folder-A request. + _folderA = createContainer("A"); + _folderB = createContainer("B"); + AnnouncementModel insert = new AnnouncementModel(); + insert.setTitle("Container scoping test thread"); + insert.setBody("body"); + _thread = AnnouncementManager.insertAnnouncement(_folderB, getAdmin(), insert, null, false); + } + + @Test + public void testSubscribeThreadRequiresReadOnThreadContainer() throws Exception + { + ActionURL url = new ActionURL(SubscribeThreadAction.class, _folderA) + .addParameter("threadId", _thread.getEntityId()); + + // Negative: the @RequiresPermission(ReadPermission) gate only proves read on folder A; a caller who + // cannot read the thread's own folder must not be able to subscribe to it. + User readerA = createUserInRole(_folderA, ReaderRole.class); + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, readerA)); + + // Positive control: the same subscription succeeds (302 to the success URL) once the caller can also + // read the thread's folder. + User readerAB = createUserInRole(_folderA, ReaderRole.class); + grantRole(readerAB, _folderB, ReaderRole.class); + assertStatus(HttpServletResponse.SC_FOUND, post(url, readerAB)); + } + + @Test + public void testSubscribeThreadEnforcesSecureBoardMemberList() throws Exception + { + // A secure board scopes read to its member list, so allowRead() rejects a board reader who is not on the + // thread's member list. The old plain container-ReadPermission check missed exactly this: it let any + // reader subscribe to a secure thread they cannot read. Address the request to the thread's own folder so + // only member-list membership varies. + Container secure = createContainer("Secure"); + Settings settings = AnnouncementManager.getMessageBoardSettings(secure); + settings.setSecure(Settings.SECURE_WITHOUT_EMAIL); + settings.setMemberList(true); + AnnouncementManager.saveMessageBoardSettings(secure, settings); + + // Both are plain readers (ReaderRole lacks SecureMessageBoardReadPermission, so neither is an editor who + // could read every thread). Create them before insert: the member-list validation checks that each listed + // member can read the thread. + User member = createUserInRole(secure, ReaderRole.class); + User nonMember = createUserInRole(secure, ReaderRole.class); + + AnnouncementModel insert = new AnnouncementModel(); + insert.setTitle("Secure member-list test thread"); + insert.setBody("body"); + // insertAnnouncement rebuilds memberListIds from memberListInput, so set the input, not the ids directly. + insert.setMemberListInput(String.valueOf(member.getUserId())); + AnnouncementModel secureThread = AnnouncementManager.insertAnnouncement(secure, getAdmin(), insert, null, false); + + ActionURL url = new ActionURL(SubscribeThreadAction.class, secure) + .addParameter("threadId", secureThread.getEntityId()); + + // Negative: a reader of the secure board who is not on the member list cannot read the thread, so cannot + // subscribe. Under the old container-ReadPermission check this incorrectly succeeded. + assertStatus(HttpServletResponse.SC_FORBIDDEN, post(url, nonMember)); + + // Positive control: a reader who is on the member list can read the thread, so the subscription succeeds. + assertStatus(HttpServletResponse.SC_FOUND, post(url, member)); + } + } } diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index 07f6e775e7c..24238916980 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -58,6 +58,7 @@ import org.labkey.api.data.DbSchema; import org.labkey.api.data.DbScope; import org.labkey.api.data.DbSequenceManager; +import org.labkey.api.data.DisplayColumn; import org.labkey.api.data.ExcelColumn; import org.labkey.api.data.ExcelWriter; import org.labkey.api.data.InlineInClauseGenerator; @@ -526,6 +527,7 @@ public void registerServlets(ServletContext servletCtx) DbScope.SchemaNameTestCase.class, DbScope.TransactionTestCase.class, DbSequenceManager.TestCase.class, + DisplayColumn.TestCase.class, DomTestCase.class, DomainTemplateGroup.TestCase.class, Encryption.TestCase.class, diff --git a/api/src/org/labkey/api/cache/Throttle.java b/api/src/org/labkey/api/cache/Throttle.java index 585af71ee66..8c26b4d8c43 100644 --- a/api/src/org/labkey/api/cache/Throttle.java +++ b/api/src/org/labkey/api/cache/Throttle.java @@ -15,6 +15,7 @@ */ package org.labkey.api.cache; +import java.util.concurrent.atomic.AtomicLong; import java.util.function.Consumer; /** @@ -36,15 +37,21 @@ * THROTTLE.execute(user); * } * + * + *

Tracks how many times the consumer actually ran ({@link #getExecutionCount()}) versus was throttled + * ({@link #getThrottledCount()}), useful for metrics and for tests asserting the throttling behavior.

*/ public class Throttle { private final BlockingCache _cache; + private final AtomicLong _executionCount = new AtomicLong(); + private final AtomicLong _requestCount = new AtomicLong(); public Throttle(String name, int limit, long timeToLive, Consumer consumer) { _cache = CacheManager.getBlockingCache(limit, timeToLive, "Throttle for " + name, (key, argument) -> { + _executionCount.incrementAndGet(); consumer.accept(key); return key; }); @@ -52,6 +59,19 @@ public Throttle(String name, int limit, long timeToLive, Consumer consumer) public void execute(K key) { + _requestCount.incrementAndGet(); _cache.get(key); } + + /** @return the number of times the consumer actually ran (i.e., calls that were not throttled) */ + public long getExecutionCount() + { + return _executionCount.get(); + } + + /** @return the number of {@link #execute} calls that were throttled (the consumer did not run) */ + public long getThrottledCount() + { + return _requestCount.get() - _executionCount.get(); + } } diff --git a/api/src/org/labkey/api/data/DisplayColumn.java b/api/src/org/labkey/api/data/DisplayColumn.java index 659a1b69dda..c46fbd12b20 100644 --- a/api/src/org/labkey/api/data/DisplayColumn.java +++ b/api/src/org/labkey/api/data/DisplayColumn.java @@ -24,7 +24,11 @@ import org.apache.poi.ss.usermodel.Workbook; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.action.HasViewContext; +import org.labkey.api.cache.CacheManager; +import org.labkey.api.cache.Throttle; import org.labkey.api.collections.NullPreventingSet; import org.labkey.api.compliance.PhiTransformedColumnInfo; import org.labkey.api.ontology.Concept; @@ -64,6 +68,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; import static org.apache.commons.lang3.StringUtils.isBlank; import static org.apache.commons.lang3.StringUtils.trimToEmpty; @@ -112,6 +117,10 @@ public abstract class DisplayColumn extends RenderColumn private String _description = null; private String _displayClass; + // GH Issue 1332: Throttle to one warning per column + value type per hour, across all renders. + private static final Throttle FORMAT_MISMATCH_THROTTLE = new Throttle<>("DisplayColumn format mismatch", 1000, CacheManager.HOUR, key -> + LOG.warn("Unable to apply format to {}, likely a SQL type mismatch between XML metadata and actual ResultSet", key)); + private final List _analyticsProviders = new ArrayList<>(); /** Handles spanning multiple rows in a grid. A separate interface to allow for easier mixing and matching with DisplayColumn implementations. */ @@ -488,7 +497,7 @@ public String getFormattedText(RenderContext ctx) } catch (IllegalArgumentException e) { - LOG.warn("Unable to apply format to {} value \"{}\" for column \"{}\", likely a SQL type mismatch between XML metadata and actual ResultSet", value.getClass().getName(), value, getName()); + warnFormatMismatch(value); return value.toString(); } } @@ -497,6 +506,13 @@ public String getFormattedText(RenderContext ctx) } + private void warnFormatMismatch(Object value) + { + ColumnInfo col = getColumnInfo(); + String key = "column \"" + (col != null ? col.getFieldKey() : getName()) + "\" (value type " + value.getClass().getName() + ")"; + FORMAT_MISMATCH_THROTTLE.execute(key); + } + /** * Render the value as text using the expr or format if provided without * any html encoding. @@ -540,7 +556,7 @@ else if (null != format) } catch (IllegalArgumentException e) { - LOG.warn("Unable to apply format to {} value \"{}\" for column \"{}\", likely a SQL type mismatch between XML metadata and actual ResultSet", value.getClass().getName(), value, getName()); + warnFormatMismatch(value); formattedString = ConvertUtils.convert(value); } } @@ -1344,4 +1360,61 @@ else if (null == formatString && col.isNumericType()) } } } + + // GH Issue 1332: a type-mismatched column logs in format() on every cell; verify we warn once per column, not once per row + public static class TestCase extends Assert + { + // Unique per column so each test gets its own throttle key + private static final AtomicInteger UNIQUE = new AtomicInteger(); + public static final String NOT_A_NUMBER_VALUE = "not-a-number"; + + // A column whose configured format rejects the value type, mimicking XML metadata that disagrees with the ResultSet. + private DisplayColumn columnThatFailsFormatting() + { + String name = "formatMismatchTestColumn-" + UNIQUE.incrementAndGet(); + Format numberFormat = new DecimalFormat("0.00"); // format() throws IllegalArgumentException on a non-Number + return new SimpleDisplayColumn() + { + @Override + public Object getValue(RenderContext ctx) + { + return NOT_A_NUMBER_VALUE; + } + + @Override + public Format getFormat() + { + return numberFormat; + } + + @Override + public String getName() + { + return name; + } + }; + } + + @Test + public void getFormattedTextWarnsOncePerColumn() + { + RenderContext ctx = new RenderContext(new ViewContext()); + DisplayColumn dc = columnThatFailsFormatting(); + long before = FORMAT_MISMATCH_THROTTLE.getExecutionCount(); + for (int row = 0; row < 1000; row++) + assertEquals("Fallback must be the unformatted value", NOT_A_NUMBER_VALUE, dc.getFormattedText(ctx)); + assertEquals("A format mismatch must warn once per column, not once per row", 1, FORMAT_MISMATCH_THROTTLE.getExecutionCount() - before); + } + + @Test + public void formatValueWarnsOncePerColumn() + { + RenderContext ctx = new RenderContext(new ViewContext()); + DisplayColumn dc = columnThatFailsFormatting(); + long before = FORMAT_MISMATCH_THROTTLE.getExecutionCount(); + for (int row = 0; row < 1000; row++) + assertEquals("Fallback must be the converted value", NOT_A_NUMBER_VALUE, dc.formatValue(ctx, NOT_A_NUMBER_VALUE, null, dc.getFormat(), null)); + assertEquals("A format mismatch must warn once per column, not once per row", 1, FORMAT_MISMATCH_THROTTLE.getExecutionCount() - before); + } + } } diff --git a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java index 06a5d29abbb..e8a6c5e6b7d 100644 --- a/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/BasePostgreSqlDialect.java @@ -924,6 +924,12 @@ public boolean supportsNativeGreatestAndLeast() return true; } + @Override + public boolean supportsNativeIsDistinctFrom() + { + return true; + } + @Override public boolean supportsIsNumeric() { diff --git a/api/src/org/labkey/api/data/dialect/SqlDialect.java b/api/src/org/labkey/api/data/dialect/SqlDialect.java index dbd16975bbe..e86a5d8c970 100644 --- a/api/src/org/labkey/api/data/dialect/SqlDialect.java +++ b/api/src/org/labkey/api/data/dialect/SqlDialect.java @@ -885,6 +885,17 @@ public SQLFragment isNumericExpr(SQLFragment expression) throw new UnsupportedOperationException(getClass().getSimpleName() + " does not implement"); } + /** + * Does the dialect natively support the standard "IS [NOT] DISTINCT FROM" predicate? PostgreSQL and Snowflake + * do; SQL Server, MySQL, and Oracle do not (SQL Server added GREATEST/LEAST in recent versions but has never + * added this predicate). Dialects that return false here get a portable CASE-based rewrite instead; see + * Method.IsDistinctFromMethodInfo. + */ + public boolean supportsNativeIsDistinctFrom() + { + return false; + } + public void handleCreateDatabaseException(SQLException e) throws ServletException { throw(new ServletException("Can't create database", e)); diff --git a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java index 3dcad2bc631..3d3fe1528bf 100644 --- a/api/src/org/labkey/api/dataiterator/SimpleTranslator.java +++ b/api/src/org/labkey/api/dataiterator/SimpleTranslator.java @@ -692,6 +692,10 @@ public class ContainerColumn implements Supplier final Set allowableContainers = new HashSet<>(); + // GH Issue 1332: a bad container value recurs on every row; warn once per distinct value, not per row + final Set loggedUnresolvedContainers = new HashSet<>(); + final Set loggedRejectedContainers = new HashSet<>(); + public ContainerColumn(UserSchema us, TableInfo tableInfo, String containerId, int idx) { this.us = us; @@ -723,7 +727,8 @@ public Object get() if (!this.us.getContainer().allowRowMutationForContainer(rowContainer)) { getRowError().addError(new SimpleValidationError("Row supplied container value: " + rowContainerVal + " cannot be used for actions against the container: " + us.getContainer().getPath())); - LOG.warn("Resolved container to {} but rejected as valid location for import into {} in {}.{}", rowContainer.getPath(), us.getContainer().getPath(), us.getSchemaName(), tableInfo.getPublicSchemaName()); + if (loggedRejectedContainers.add(rowContainerVal)) + LOG.warn("Resolved container to {} but rejected as valid location for import into {} in {}.{}", rowContainer.getPath(), us.getContainer().getPath(), us.getSchemaName(), tableInfo.getPublicSchemaName()); } else { @@ -734,8 +739,8 @@ public Object get() } else { - // only log if the incoming value is GUID-like - if (rowContainerVal instanceof String && GUID.isGUID((String)rowContainerVal)) + // only log if the incoming value is GUID-like, and only once per distinct value + if (rowContainerVal instanceof String s && GUID.isGUID(s) && loggedUnresolvedContainers.add(rowContainerVal)) { LOG.warn("Failed to resolve container value '{}' to container for import into {}.{}, defaulting to original target container of {}", rowContainerVal, us.getSchemaName(), tableInfo.getPublicSchemaName(), us.getContainer().getPath()); } diff --git a/api/src/org/labkey/api/settings/AppProps.java b/api/src/org/labkey/api/settings/AppProps.java index 064127076d6..8404b80d6f8 100644 --- a/api/src/org/labkey/api/settings/AppProps.java +++ b/api/src/org/labkey/api/settings/AppProps.java @@ -185,6 +185,11 @@ static WriteableAppProps getWriteableInstance() /** Timeout in seconds for read-only HTTP requests, after which resources like DB connections and spawned processes will be killed. Set to 0 to disable. */ int getReadOnlyHttpRequestTimeout(); + int DEFAULT_SCRIPT_EXECUTION_TIMEOUT = 60; + + /** Timeout in seconds for server-side JavaScript (e.g. trigger scripts), measured in wall-clock time including database and other Java operations invoked by the script. Set to 0 to disable. */ + int getScriptExecutionTimeout(); + int getMaxBLOBSize(); ExceptionReportingLevel getExceptionReportingLevel(); diff --git a/api/src/org/labkey/api/settings/AppPropsImpl.java b/api/src/org/labkey/api/settings/AppPropsImpl.java index 60b22b957fe..ac90bf67310 100644 --- a/api/src/org/labkey/api/settings/AppPropsImpl.java +++ b/api/src/org/labkey/api/settings/AppPropsImpl.java @@ -321,6 +321,12 @@ public int getReadOnlyHttpRequestTimeout() return lookupIntValue(readOnlyHttpRequestTimeout, 0); } + @Override + public int getScriptExecutionTimeout() + { + return lookupIntValue(scriptExecutionTimeout, DEFAULT_SCRIPT_EXECUTION_TIMEOUT); + } + @Override public int getMaxBLOBSize() { diff --git a/api/src/org/labkey/api/settings/SiteSettingsProperties.java b/api/src/org/labkey/api/settings/SiteSettingsProperties.java index 961ecb54868..8cbc673f4aa 100644 --- a/api/src/org/labkey/api/settings/SiteSettingsProperties.java +++ b/api/src/org/labkey/api/settings/SiteSettingsProperties.java @@ -90,6 +90,14 @@ public void setValue(WriteableAppProps writeable, String value) writeable.setReadOnlyHttpRequestTimeout(Integer.parseInt(value)); } }, + scriptExecutionTimeout("Timeout in seconds for server-side JavaScript such as trigger scripts. Measured in wall-clock time, including database and other Java operations invoked by the script. Set to 0 to disable.") + { + @Override + public void setValue(WriteableAppProps writeable, String value) + { + writeable.setScriptExecutionTimeout(Integer.parseInt(value)); + } + }, maxBLOBSize("Maximum file size, in bytes, to allow in database BLOBs") { @Override diff --git a/api/src/org/labkey/api/settings/WriteableAppProps.java b/api/src/org/labkey/api/settings/WriteableAppProps.java index e3691f745b0..92df26b757b 100644 --- a/api/src/org/labkey/api/settings/WriteableAppProps.java +++ b/api/src/org/labkey/api/settings/WriteableAppProps.java @@ -94,6 +94,13 @@ public void setReadOnlyHttpRequestTimeout(int timeout) storeIntValue(readOnlyHttpRequestTimeout, timeout); } + public void setScriptExecutionTimeout(int timeout) + { + if (timeout < 0) + throw new IllegalArgumentException("scriptExecutionTimeout must be >= 0"); + storeIntValue(scriptExecutionTimeout, timeout); + } + public void setMaxBLOBSize(int maxSize) { if (maxSize < 0) diff --git a/api/src/org/labkey/api/study/query/PublishResultsQueryView.java b/api/src/org/labkey/api/study/query/PublishResultsQueryView.java index b59a2109d52..bf005ad6d8f 100644 --- a/api/src/org/labkey/api/study/query/PublishResultsQueryView.java +++ b/api/src/org/labkey/api/study/query/PublishResultsQueryView.java @@ -94,22 +94,26 @@ import java.util.Map; import java.util.Objects; import java.util.Set; +import java.util.concurrent.ConcurrentHashMap; import java.util.stream.Collectors; import java.util.stream.Stream; -import static org.labkey.api.util.IntegerUtils.asInteger; -import static org.labkey.api.util.IntegerUtils.asLong; import static org.labkey.api.study.publish.StudyPublishService.LinkToStudyKeys; import static org.labkey.api.util.DOM.Attribute.id; import static org.labkey.api.util.DOM.DIV; import static org.labkey.api.util.DOM.SCRIPT; import static org.labkey.api.util.DOM.at; +import static org.labkey.api.util.IntegerUtils.asInteger; +import static org.labkey.api.util.IntegerUtils.asLong; import static org.labkey.api.util.IntegerUtils.asLongElseNull; public class PublishResultsQueryView extends QueryView { private static final Logger LOG = LogManager.getLogger(PublishResultsQueryView.class); + // GH Issue 1332: Only warn once per distinct column + private static final Set LOGGED_MULTIVALUE_COLUMNS = ConcurrentHashMap.newKeySet(); + private final SimpleFilter _filter; private final Container _targetStudyContainer; private final boolean _mismatched; @@ -303,7 +307,7 @@ public static Object getColumnValue(ColumnInfo col, RenderContext ctx) List values = ((IMultiValuedDisplayColumn)dc).getDisplayValues(ctx); if (values.size() == 1) return values.getFirst(); - else + else if (LOGGED_MULTIVALUE_COLUMNS.add(col.getFieldKey().toString())) LOG.warn("Unable to use the value returned from column : {} because this multi-value column returned more than a single value.", col.getName()); } return col.getValue(ctx); diff --git a/assay/src/org/labkey/assay/query/TypeDisplayColumn.java b/assay/src/org/labkey/assay/query/TypeDisplayColumn.java index e62b1816151..dff03ba31e0 100644 --- a/assay/src/org/labkey/assay/query/TypeDisplayColumn.java +++ b/assay/src/org/labkey/assay/query/TypeDisplayColumn.java @@ -42,6 +42,9 @@ public class TypeDisplayColumn extends DataColumn private static final FieldKey LSID_FIELD_KEY = new FieldKey(null, "LSID"); + // GH Issue 1332: a broken provider pattern misses on every row; warn once per instance, not once per cell + private boolean _loggedProviderMismatch = false; + public TypeDisplayColumn(ColumnInfo colInfo) { super(colInfo); @@ -74,7 +77,11 @@ public void renderGridCellContents(RenderContext ctx, HtmlWriter out) AssayProvider provider = AssayService.get().getProvider(protocol); if (provider != null) { - LOG.warn("Failed to match AssayProvider '{}' using pattern '{}' for LSID: {}", provider.getName(), provider.getProtocolPattern(), lsid); + if (!_loggedProviderMismatch) + { + _loggedProviderMismatch = true; + LOG.warn("Failed to match AssayProvider '{}' using pattern '{}' for LSID: {}", provider.getName(), provider.getProtocolPattern(), lsid); + } out.write(provider.getName()); return; } diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 64d0cdcc26e..97175dc35ba 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -1381,6 +1381,14 @@ public void validateCommand(SiteSettingsForm form, Errors errors) { errors.reject(ERROR_MSG, "Memory logging frequency must be non-negative"); } + if (form.getScriptExecutionTimeout() == null) + { + errors.reject(ERROR_MSG, "Script execution timeout is required; set to 0 to disable the timeout"); + } + else if (form.getScriptExecutionTimeout() < 0) + { + errors.reject(ERROR_MSG, "Script execution timeout must be non-negative"); + } } @Override @@ -1415,6 +1423,7 @@ public boolean handlePost(SiteSettingsForm form, BindException errors) throws Ex props.setSSLPort(form.getSslPort()); props.setMemoryUsageDumpInterval(form.getMemoryUsageDumpInterval()); props.setReadOnlyHttpRequestTimeout(form.getReadOnlyHttpRequestTimeout()); + props.setScriptExecutionTimeout(form.getScriptExecutionTimeout()); props.setMaxBLOBSize(form.getMaxBLOBSize()); props.setSelfReportExceptions(form.isSelfReportExceptions()); @@ -2384,6 +2393,7 @@ public static class SiteSettingsForm private int _sslPort; private int _memoryUsageDumpInterval; private int _readOnlyHttpRequestTimeout; + private Integer _scriptExecutionTimeout; private int _maxBLOBSize; private String _exceptionReportingLevel; private String _usageReportingLevel; @@ -2524,6 +2534,18 @@ public int getReadOnlyHttpRequestTimeout() return _readOnlyHttpRequestTimeout; } + /** Null when the request omits or blanks the parameter; rejected in validateCommand so an omitted value can never bind to 0 and silently disable the timeout. */ + @Nullable + public Integer getScriptExecutionTimeout() + { + return _scriptExecutionTimeout; + } + + public void setScriptExecutionTimeout(@Nullable Integer timeout) + { + _scriptExecutionTimeout = timeout; + } + public void setReadOnlyHttpRequestTimeout(int timeout) { _readOnlyHttpRequestTimeout = timeout; diff --git a/core/src/org/labkey/core/admin/customizeSite.jsp b/core/src/org/labkey/core/admin/customizeSite.jsp index 8bc659eb5ac..1de822de1e6 100644 --- a/core/src/org/labkey/core/admin/customizeSite.jsp +++ b/core/src/org/labkey/core/admin/customizeSite.jsp @@ -312,6 +312,11 @@ Click the Save button at any time to accept the current settings and continue. + + + + diff --git a/core/src/org/labkey/core/script/RhinoService.java b/core/src/org/labkey/core/script/RhinoService.java index 497a6d99b1b..71d41196269 100644 --- a/core/src/org/labkey/core/script/RhinoService.java +++ b/core/src/org/labkey/core/script/RhinoService.java @@ -40,11 +40,15 @@ import org.labkey.api.resource.Resource; import org.labkey.api.script.ScriptReference; import org.labkey.api.script.ScriptService; +import org.labkey.api.security.User; +import org.labkey.api.settings.AppProps; +import org.labkey.api.settings.WriteableAppProps; import org.labkey.api.test.TestWhen; import org.labkey.api.util.HeartBeat; import org.labkey.api.util.JunitUtil; import org.labkey.api.util.MemTracker; import org.labkey.api.util.Path; +import org.labkey.api.util.TestContext; import org.labkey.api.util.UnexpectedException; import org.labkey.api.view.HttpView; import org.mozilla.javascript.ClassShutter; @@ -199,6 +203,44 @@ public void reportTest() throws Exception test("reportTest"); } + @Test + public void timeoutTest() throws Exception + { + int original = AppProps.getInstance().getScriptExecutionTimeout(); + User user = TestContext.get().getUser(); + + try + { + // Busy-loop bounded at 15 seconds of wall-clock time; the 1-second watchdog should abort it long before that. HeartBeat ticks once per second, so the abort lands after 2-3 real seconds. + setScriptExecutionTimeout(1, user); + try + { + RhinoService.RHINO_FACTORY.getScriptEngine().eval("var start = new Date().getTime(); while (new Date().getTime() - start < 15000) {}"); + fail("Expected script to be terminated by the execution timeout"); + } + catch (Exception e) + { + assertTrue("Unexpected script error: " + e.getMessage(), e.getMessage().contains("Script execution exceeded 1 seconds")); + } + + // 0 disables the watchdog: a loop that runs well past the 1-second timeout above should complete normally + setScriptExecutionTimeout(0, user); + Object result = RhinoService.RHINO_FACTORY.getScriptEngine().eval("var start = new Date().getTime(); while (new Date().getTime() - start < 2500) {} 'completed';"); + assertEquals("completed", result); + } + finally + { + setScriptExecutionTimeout(original, user); + } + } + + private void setScriptExecutionTimeout(int seconds, User user) + { + WriteableAppProps props = AppProps.getWriteableInstance(); + props.setScriptExecutionTimeout(seconds); + props.save(user); + } + private void test(String scriptName) throws ScriptException, NoSuchMethodException { Path js = Path.parse(ScriptService.SCRIPTS_DIR + "/validationTest/" + scriptName + ".js"); @@ -1006,9 +1048,8 @@ protected void observeInstructionCount(Context cx, int instructionCount) { SandboxContext ctx = (SandboxContext)cx; long currentTime = HeartBeat.currentTimeMillis(); - final int timeout = 60; - if (currentTime - ctx.startTime > timeout*1000) - Context.reportError("Script execution exceeded " + timeout + " seconds."); + if (ctx.timeoutSeconds > 0 && currentTime - ctx.startTime > ctx.timeoutSeconds * 1000L) + Context.reportError("Script execution exceeded " + ctx.timeoutSeconds + " seconds."); } @Override @@ -1044,12 +1085,15 @@ public boolean visibleToScripts(String fullClassName) private static class SandboxContext extends Context { private final long startTime; + // resolved once per context; observeInstructionCount runs far too often for a property lookup + private final int timeoutSeconds; private SandboxContext(SandboxContextFactory factory) { super(factory); setLanguageVersion(Context.VERSION_1_8); startTime = HeartBeat.currentTimeMillis(); + timeoutSeconds = AppProps.getInstance().getScriptExecutionTimeout(); } } diff --git a/core/src/org/labkey/core/security/addUsers.jsp b/core/src/org/labkey/core/security/addUsers.jsp index 3de3d6e0a51..3565c2e4a33 100644 --- a/core/src/org/labkey/core/security/addUsers.jsp +++ b/core/src/org/labkey/core/security/addUsers.jsp @@ -40,11 +40,6 @@ boolean excludeSiteAdmins = !getUser().hasSiteAdminPermission(); // App admins can't clone permissions from site admins %>