From 4c48e1eed3b3838119300c6cb3ecc89df7a8f406 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Sun, 27 Apr 2025 08:40:45 -0700 Subject: [PATCH 1/5] Fix assorted exceptions from bots/crawlers --- .../AnnouncementsController.java | 76 ++++++++++--------- .../labkey/query/view/CustomViewSetKey.java | 2 +- 2 files changed, 42 insertions(+), 36 deletions(-) diff --git a/announcements/src/org/labkey/announcements/AnnouncementsController.java b/announcements/src/org/labkey/announcements/AnnouncementsController.java index a720e61e4ab..5bf73039c66 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementsController.java +++ b/announcements/src/org/labkey/announcements/AnnouncementsController.java @@ -145,6 +145,7 @@ import java.util.Date; import java.util.GregorianCalendar; import java.util.List; +import java.util.Objects; import java.util.Set; import java.util.function.Predicate; import java.util.stream.Collectors; @@ -237,7 +238,7 @@ public static AnnouncementModel copyEditableProps(AnnouncementModel target, Anno // Anyone with read permission can attempt to view the list. AnnouncementWebPart will do further permission checking. For example, // in a secure message board, those without Editor permissions will only see messages when they are on the member list @RequiresPermission(ReadPermission.class) - public class BeginAction extends SimpleViewAction + public class BeginAction extends SimpleViewAction { // Invoked via reflection @SuppressWarnings("UnusedDeclaration") @@ -283,7 +284,7 @@ private static ActionURL getListURL(Container c) @RequiresPermission(ReadPermission.class) - public class ListAction extends SimpleViewAction + public class ListAction extends SimpleViewAction { @Override public ModelAndView getView(Object o, BindException errors) @@ -313,7 +314,7 @@ public static ActionURL getAdminEmailURL(Container c, @Nullable URLHelper return } @RequiresPermission(DeletePermission.class) - public class DeleteThreadsAction extends FormHandlerAction + public class DeleteThreadsAction extends FormHandlerAction { @Override public void validateCommand(Object target, Errors errors) @@ -766,7 +767,7 @@ private boolean hasEditorPerm(int groupId) @RequiresAnyOf({InsertMessagePermission.class, InsertPermission.class}) public abstract class BaseInsertAction extends FormViewAction { - protected HttpView _attachmentErrorView; + protected HttpView _attachmentErrorView; protected abstract ModelAndView getInsertUpdateView(AnnouncementForm announcementForm, boolean reshow, BindException errors); @@ -953,10 +954,10 @@ public ModelAndView getInsertUpdateView(AnnouncementForm form, boolean reshow, B throw new UnauthorizedException(); } - ThreadView threadView = new ThreadView(c, getActionURL(), parent, perm); + ThreadView threadView = new ThreadView(c, getActionURL(), parent, perm, getUser()); threadView.setFrame(WebPartView.FrameType.DIV); - HttpView respondView = new RespondView(c, parent, form, form.getReturnUrlHelper(), errors, reshow, false); + HttpView respondView = new RespondView(c, parent, form, form.getReturnUrlHelper(), errors, reshow, false); getPageConfig().setFocusId("body"); _parent = parent; @@ -1010,7 +1011,7 @@ private static SelectBuilder getRenderAsSelect(WikiRendererType currentRendererT } @RequiresPermission(InsertPermission.class) - public class CompleteUserAction extends ReadOnlyApiAction + public static class CompleteUserAction extends ReadOnlyApiAction { @Override public ApiResponse execute(AjaxCompletionForm form, BindException errors) @@ -1093,7 +1094,7 @@ public BaseInsertView(String page, InsertBean bean, AnnouncementForm form, URLHe if (reshow) { - String rendererTypeName = (String) form.get("rendererType"); + String rendererTypeName = form.get("rendererType"); if (null == rendererTypeName) currentRendererType = DEFAULT_MESSAGE_RENDERER_TYPE; @@ -1289,10 +1290,7 @@ public boolean handlePost(AnnouncementForm form, BindException errors) // Needs to support non-ActionURL (e.g., an HTML page using the client API with embedded discussion webpart) // so we can't use getSuccessURL() URLHelper urlHelper = form.getReturnUrlHelper(); - if (null != urlHelper) - throw new RedirectException(urlHelper); - else - throw new RedirectException(getThreadURL(getContainer(), oldAnn.getParent(), oldAnn.getRowId())); + throw new RedirectException(Objects.requireNonNullElseGet(urlHelper, () -> getThreadURL(getContainer(), oldAnn.getParent(), oldAnn.getRowId()))); } @Override @@ -1361,7 +1359,7 @@ public class ThreadAction extends SimpleViewAction @Override public ThreadView getView(AnnouncementForm form, BindException errors) throws Exception { - ThreadView threadView = new ThreadView(form, getContainer(), getActionURL(), getPermissions(), isPrint()); + ThreadView threadView = new ThreadView(form, getContainer(), getActionURL(), getPermissions(), isPrint(), getUser()); threadView.setFrame(WebPartView.FrameType.PORTAL); AnnouncementModel ann = threadView.getAnnouncement(); @@ -1415,7 +1413,7 @@ public void addNavTrail(NavTree root) @RequiresPermission(ReadPermission.class) - public class RssAction extends SimpleViewAction + public class RssAction extends SimpleViewAction { // Invoked via reflection @SuppressWarnings("UnusedDeclaration") @@ -1449,7 +1447,7 @@ public ModelAndView getView(Object o, BindException errors) ActionURL url = new ActionURL(ThreadAction.class, c).addParameter("rowId", null); - WebPartView v = new RssView(pair.first, url.getURIString()); + WebPartView v = new RssView(pair.first, url.getURIString()); getResponse().setContentType("text/xml"); getPageConfig().setTemplate(PageConfig.Template.None); @@ -1526,7 +1524,7 @@ public ModelAndView getView(EmailOptionsForm form, boolean reshow, BindException form.setEmailOptionsOnPage(emailOption); setHelpTopic("createMessage"); - JspView view = new JspView("/org/labkey/announcements/emailPreferences.jsp"); + JspView view = new JspView<>("/org/labkey/announcements/emailPreferences.jsp"); view.setFrame(WebPartView.FrameType.NONE); EmailPreferencesPage page = (EmailPreferencesPage)view.getPage(); view.setTitle("Email Preferences"); @@ -1599,7 +1597,7 @@ private static int getEmailOptionIncludingInherited(Container c, User user, Stri } @RequiresPermission(AdminPermission.class) - public class SetEmailDefaultAction extends MutatingApiAction + public static class SetEmailDefaultAction extends MutatingApiAction { @Override public ApiResponse execute(AbstractConfigTypeProvider.EmailConfigFormImpl form, BindException errors) @@ -1628,7 +1626,7 @@ public ApiResponse execute(AbstractConfigTypeProvider.EmailConfigFormImpl form, // Used for testing announcement daily digest email notifications @RequiresSiteAdmin - public class SendDailyDigestAction extends MutatingApiAction + public static class SendDailyDigestAction extends MutatingApiAction { @Override public Object execute(Object o, BindException errors) @@ -1735,7 +1733,7 @@ public void validate(Errors errors) { String expires = StringUtils.trimToNull(get("expires")); if (null != expires) - DateUtil.parseDateTime(getContainer(), expires); + DateUtil.parseDateTime(expires); } catch (ConversionException x) { @@ -2080,7 +2078,7 @@ public static class AnnouncementWebPartFactory extends AlwaysAvailableWebPartFac } @Override - public WebPartView getWebPartView(@NotNull ViewContext parentCtx, @NotNull Portal.WebPart webPart) + public WebPartView getWebPartView(@NotNull ViewContext parentCtx, @NotNull Portal.WebPart webPart) { String jsp = "/org/labkey/announcements/announcementWebPartWithExpandos.jsp"; if ("simple".equals(webPart.getPropertyMap().get("style"))) @@ -2089,7 +2087,7 @@ public WebPartView getWebPartView(@NotNull ViewContext parentCtx, @NotNull Porta } @Override - public HttpView getEditView(Portal.WebPart webPart, ViewContext context) + public HttpView getEditView(Portal.WebPart webPart, ViewContext context) { return new AnnouncementsController.CustomizeAnnouncementWebPart(webPart); } @@ -2147,7 +2145,7 @@ private static String getFilterText(Settings settings, boolean displayAll, boole sb.append(rowLimit); } - if (sb.length() == 0) + if (sb.isEmpty()) sb.append("all"); sb.append(" "); @@ -2158,9 +2156,9 @@ private static String getFilterText(Settings settings, boolean displayAll, boole } - public static class AnnouncementListWebPart extends WebPartView + public static class AnnouncementListWebPart extends WebPartView { - private VBox _vbox; + private final VBox _vbox; public AnnouncementListWebPart(ViewContext ctx) { @@ -2338,23 +2336,23 @@ private ThreadView() public ThreadView(Container c, URLHelper currentURL, User user, String rowId, String entityId) { this(); - init(c, findThread(c, rowId, entityId), currentURL, getPermissions(c, user, getSettings(c)), false, false); + init(c, findThread(c, rowId, entityId), currentURL, getPermissions(c, user, getSettings(c)), false, false, user); } - public ThreadView(Container c, ActionURL url, AnnouncementModel ann, Permissions perm) + public ThreadView(Container c, ActionURL url, AnnouncementModel ann, Permissions perm, User user) { this(); - init(c, ann, url, perm, true, false); + init(c, ann, url, perm, true, false, user); } - public ThreadView(AnnouncementForm form, Container c, ActionURL url, Permissions perm, boolean print) + public ThreadView(AnnouncementForm form, Container c, ActionURL url, Permissions perm, boolean print, User user) { this(); - AnnouncementModel ann = findThread(c, (String)form.get("rowId"), (String)form.get("entityId")); - init(c, ann, url, perm, false, print); + AnnouncementModel ann = findThread(c, form.get("rowId"), form.get("entityId")); + init(c, ann, url, perm, false, print, user); } - protected void init(Container c, AnnouncementModel ann, URLHelper currentURL, Permissions perm, boolean isResponse, boolean print) + protected void init(Container c, AnnouncementModel ann, URLHelper currentURL, Permissions perm, boolean isResponse, boolean print, User user) { if (null == c || !perm.allowRead(ann)) { @@ -2509,7 +2507,7 @@ public void setUnsubscribe(boolean unsubscribe) } @RequiresLogin @RequiresPermission(ReadPermission.class) - public class SubscribeThreadAction extends FormHandlerAction + public static class SubscribeThreadAction extends FormHandlerAction { @Override public void validateCommand(SubscriptionBean target, Errors errors) @@ -2537,14 +2535,22 @@ public boolean handlePost(SubscriptionBean bean, BindException errors) throw new UnauthorizedException(); } + // We track the subscriptions on the last post for the thread + Integer postId = AnnouncementManager.getLatestPostId(ann); + if (postId == null) + { + postId = ann.getRowId(); + } + // Remove or add the thread-level subscription from the database table if (bean.isUnsubscribe()) { - new SqlExecutor(CommSchema.getInstance().getSchema()).execute("DELETE FROM comm.userlist WHERE UserId = ? AND MessageId = ?", getUser(), ann.getRowId()); + new SqlExecutor(CommSchema.getInstance().getSchema()).execute("DELETE FROM comm.userlist WHERE UserId = ? AND MessageId = ?", getUser(), postId); } else if (!ann.getMemberListIds().contains(getUser().getUserId())) { - new SqlExecutor(CommSchema.getInstance().getSchema()).execute("INSERT INTO comm.userlist (UserId, MessageId) VALUES (?, ?)", getUser(), ann.getRowId()); + new SqlExecutor(CommSchema.getInstance().getSchema()).execute("INSERT INTO comm.userlist (UserId, MessageId) SELECT ? AS UserId, ? AS MessageId WHERE NOT EXISTS (SELECT * FROM comm.userlist WHERE UserId = ? AND MessageId = ?)", + getUser(), postId, getUser(), postId); } return true; } @@ -2869,7 +2875,7 @@ public void setDiscussionSrcIdentifier(String discussionSrcIdentifier) } @RequiresPermission(ReadPermission.class) - public class GetDiscussionsAction extends ReadOnlyApiAction + public static class GetDiscussionsAction extends ReadOnlyApiAction { @Override public void validateForm(GetDiscussionsForm form, Errors errors) diff --git a/query/src/org/labkey/query/view/CustomViewSetKey.java b/query/src/org/labkey/query/view/CustomViewSetKey.java index ee70295eb58..64835d72ddb 100644 --- a/query/src/org/labkey/query/view/CustomViewSetKey.java +++ b/query/src/org/labkey/query/view/CustomViewSetKey.java @@ -116,7 +116,7 @@ static public CstmView saveCustomViewInSession(HttpServletRequest request, Query Map> fullMap = getMap(request); if (fullMap == null) { - fullMap = new HashMap<>(); + fullMap = Collections.synchronizedMap(new HashMap<>()); } Map map = fullMap.computeIfAbsent(new CustomViewSetKey(queryDef), k -> new HashMap<>()); From 53b9f1b4de0495a2335c2b63097e894e8bab0911 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Sun, 27 Apr 2025 08:41:16 -0700 Subject: [PATCH 2/5] Code cleanup --- api/src/org/labkey/api/jsp/taglib/OptionsTag.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/api/src/org/labkey/api/jsp/taglib/OptionsTag.java b/api/src/org/labkey/api/jsp/taglib/OptionsTag.java index d5b462a812a..cef67b93a11 100644 --- a/api/src/org/labkey/api/jsp/taglib/OptionsTag.java +++ b/api/src/org/labkey/api/jsp/taglib/OptionsTag.java @@ -33,8 +33,8 @@ public class OptionsTag extends SimpleTagBase public boolean isSelected(Object test) { - if (_value instanceof Collection) - return ((Collection)_value).contains(test); + if (_value instanceof Collection c) + return c.contains(test); else return Objects.equals(test, _value); } From eaec97962ed27d8f12959210f8830ef917d6cc65 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 28 Apr 2025 09:22:23 -0700 Subject: [PATCH 3/5] Avoid IllegalArgumentException on bogus return URL --- .../org/labkey/api/util/ReturnURLString.java | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/api/src/org/labkey/api/util/ReturnURLString.java b/api/src/org/labkey/api/util/ReturnURLString.java index f0d6b8a4faf..be6e671ee68 100644 --- a/api/src/org/labkey/api/util/ReturnURLString.java +++ b/api/src/org/labkey/api/util/ReturnURLString.java @@ -33,9 +33,6 @@ /** * Use ReturnURLString in Spring form beans to sanitize and convert Strings into ActionURL or URLHelper when binding parameters. - * - * User: matthewb - * Date: Nov 3, 2009 */ public class ReturnURLString { @@ -203,12 +200,20 @@ public Object convert(Class type, Object value) if (value instanceof ReturnURLString) return value; CharSequence seq; - if (value instanceof CharSequence) - seq = (CharSequence)value; + if (value instanceof CharSequence cs) + seq = cs; else seq = (String)_impl.convert(String.class, value); - return new ReturnURLString(seq); + try + { + return new ReturnURLString(seq); + } + catch (IllegalArgumentException e) + { + // Invalid URL + throw new ConversionException(e); + } } } } From 8cc4a623aada801a2e1987d4bfec6c7ada615e29 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 28 Apr 2025 13:55:02 -0700 Subject: [PATCH 4/5] Code review improvements --- .../announcements/AnnouncementsController.java | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/announcements/src/org/labkey/announcements/AnnouncementsController.java b/announcements/src/org/labkey/announcements/AnnouncementsController.java index 5bf73039c66..e858e01f9f6 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementsController.java +++ b/announcements/src/org/labkey/announcements/AnnouncementsController.java @@ -18,6 +18,7 @@ import jakarta.servlet.http.HttpServletResponse; import org.apache.commons.beanutils.ConversionException; +import org.apache.commons.lang3.EnumUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.time.DateUtils; import org.apache.logging.log4j.LogManager; @@ -1094,12 +1095,7 @@ public BaseInsertView(String page, InsertBean bean, AnnouncementForm form, URLHe if (reshow) { - String rendererTypeName = form.get("rendererType"); - - if (null == rendererTypeName) - currentRendererType = DEFAULT_MESSAGE_RENDERER_TYPE; - else - currentRendererType = WikiRendererType.valueOf(rendererTypeName); + currentRendererType = EnumUtils.getEnum(WikiRendererType.class, form.get("rendererType"), DEFAULT_MESSAGE_RENDERER_TYPE); AnnouncementModel ann = form.getBean(); assignedTo = ann.getAssignedTo(); @@ -2336,23 +2332,23 @@ private ThreadView() public ThreadView(Container c, URLHelper currentURL, User user, String rowId, String entityId) { this(); - init(c, findThread(c, rowId, entityId), currentURL, getPermissions(c, user, getSettings(c)), false, false, user); + init(c, findThread(c, rowId, entityId), currentURL, getPermissions(c, user, getSettings(c)), false, false); } public ThreadView(Container c, ActionURL url, AnnouncementModel ann, Permissions perm, User user) { this(); - init(c, ann, url, perm, true, false, user); + init(c, ann, url, perm, true, false); } public ThreadView(AnnouncementForm form, Container c, ActionURL url, Permissions perm, boolean print, User user) { this(); AnnouncementModel ann = findThread(c, form.get("rowId"), form.get("entityId")); - init(c, ann, url, perm, false, print, user); + init(c, ann, url, perm, false, print); } - protected void init(Container c, AnnouncementModel ann, URLHelper currentURL, Permissions perm, boolean isResponse, boolean print, User user) + protected void init(Container c, AnnouncementModel ann, URLHelper currentURL, Permissions perm, boolean isResponse, boolean print) { if (null == c || !perm.allowRead(ann)) { From ff471fb642d5dce9652459ea1b246541a707c9d6 Mon Sep 17 00:00:00 2001 From: labkey-jeckels Date: Mon, 28 Apr 2025 16:16:51 -0700 Subject: [PATCH 5/5] Try to track down OutOfMemoryError saving custom view; Remove more user args --- .../announcements/AnnouncementsController.java | 8 ++++---- .../labkey/query/controllers/QueryController.java | 12 ++++++++++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/announcements/src/org/labkey/announcements/AnnouncementsController.java b/announcements/src/org/labkey/announcements/AnnouncementsController.java index e858e01f9f6..05631546301 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementsController.java +++ b/announcements/src/org/labkey/announcements/AnnouncementsController.java @@ -955,7 +955,7 @@ public ModelAndView getInsertUpdateView(AnnouncementForm form, boolean reshow, B throw new UnauthorizedException(); } - ThreadView threadView = new ThreadView(c, getActionURL(), parent, perm, getUser()); + ThreadView threadView = new ThreadView(c, getActionURL(), parent, perm); threadView.setFrame(WebPartView.FrameType.DIV); HttpView respondView = new RespondView(c, parent, form, form.getReturnUrlHelper(), errors, reshow, false); @@ -1355,7 +1355,7 @@ public class ThreadAction extends SimpleViewAction @Override public ThreadView getView(AnnouncementForm form, BindException errors) throws Exception { - ThreadView threadView = new ThreadView(form, getContainer(), getActionURL(), getPermissions(), isPrint(), getUser()); + ThreadView threadView = new ThreadView(form, getContainer(), getActionURL(), getPermissions(), isPrint()); threadView.setFrame(WebPartView.FrameType.PORTAL); AnnouncementModel ann = threadView.getAnnouncement(); @@ -2335,13 +2335,13 @@ public ThreadView(Container c, URLHelper currentURL, User user, String rowId, St init(c, findThread(c, rowId, entityId), currentURL, getPermissions(c, user, getSettings(c)), false, false); } - public ThreadView(Container c, ActionURL url, AnnouncementModel ann, Permissions perm, User user) + public ThreadView(Container c, ActionURL url, AnnouncementModel ann, Permissions perm) { this(); init(c, ann, url, perm, true, false); } - public ThreadView(AnnouncementForm form, Container c, ActionURL url, Permissions perm, boolean print, User user) + public ThreadView(AnnouncementForm form, Container c, ActionURL url, Permissions perm, boolean print) { this(); AnnouncementModel ann = findThread(c, form.get("rowId"), form.get("entityId")); diff --git a/query/src/org/labkey/query/controllers/QueryController.java b/query/src/org/labkey/query/controllers/QueryController.java index 7acb15cddbf..190f838e1c0 100644 --- a/query/src/org/labkey/query/controllers/QueryController.java +++ b/query/src/org/labkey/query/controllers/QueryController.java @@ -42,6 +42,7 @@ import org.json.JSONArray; import org.json.JSONException; import org.json.JSONObject; +import org.json.JSONParserConfiguration; import org.junit.After; import org.junit.Assert; import org.junit.Before; @@ -2555,8 +2556,15 @@ else if (!isHidden) JSONObject ret = new JSONObject(); ret.put("redirect", returnUrl); - if (view != null) - ret.put("view", CustomViewUtil.toMap(view, getUser(), true)); + Map viewAsMap = CustomViewUtil.toMap(view, getUser(), true); + try + { + ret.put("view", new JSONObject(viewAsMap, new JSONParserConfiguration().withMaxNestingDepth(10))); + } + catch (JSONException e) + { + LOG.error("Failed to save view: {}", jsonView, e); + } return ret; }