From 528f5040fbbdb809031580293d9df1fadeb3317a Mon Sep 17 00:00:00 2001 From: Adam Rauch Date: Thu, 1 Oct 2026 10:38:36 -0700 Subject: [PATCH] GH Issue 1631: ViewContext.getString() (#8118) ## Rationale Stop repeated URL parameters from causing unhandled ClassCastExceptions ([GH Issue 1631](https://github.com/LabKey/internal-issues/issues/1631)). ## Changes - Introduce `ViewContext.getString()` which checks the value type before returning it. It returns `null` or a `String`, or else it throws a clear `BadRequestException`, which avoids logging and mothership reporting. - Use this safer method in `StudyController`, `ReportsController`, `AnnouncementsController`, etc. - Simple unit test for getString() - Introduce `ViewContext.get(String)` and deprecate `ViewContext.get(Object)` --- .../AnnouncementsController.java | 2 +- api/src/org/labkey/api/ApiModule.java | 2 + .../report/ExternalScriptEngineReport.java | 2 +- .../reports/report/ScriptEngineReport.java | 2 +- .../reports/report/ScriptProcessReport.java | 2 +- .../api/reports/report/ScriptReport.java | 6 +-- api/src/org/labkey/api/view/ViewContext.java | 43 ++++++++++++++++++- .../query/reports/ReportsController.java | 12 +++--- .../study/controllers/StudyController.java | 14 +++--- .../reports/ReportsController.java | 9 +--- .../security/SecurityController.java | 6 +-- .../study/reports/ParticipantReport.java | 2 +- 12 files changed, 68 insertions(+), 34 deletions(-) diff --git a/announcements/src/org/labkey/announcements/AnnouncementsController.java b/announcements/src/org/labkey/announcements/AnnouncementsController.java index de52aaa3f29..49b8d6b69f3 100644 --- a/announcements/src/org/labkey/announcements/AnnouncementsController.java +++ b/announcements/src/org/labkey/announcements/AnnouncementsController.java @@ -1953,7 +1953,7 @@ private static ActionURL getPageURL(ViewContext ctx) { // This is set to the outer page URL in the case of rendering a dynamic webpart; use it instead of // the getWebPart URL. - String returnUrl = (String)ctx.get(ActionURL.Param.returnUrl.name()); + String returnUrl = ctx.getString(ActionURL.Param.returnUrl.name()); if (null != returnUrl) { diff --git a/api/src/org/labkey/api/ApiModule.java b/api/src/org/labkey/api/ApiModule.java index be94e95f0d6..d484ce6e7ee 100644 --- a/api/src/org/labkey/api/ApiModule.java +++ b/api/src/org/labkey/api/ApiModule.java @@ -200,6 +200,7 @@ import org.labkey.api.view.LabKeyKaptchaServlet; import org.labkey.api.view.Portal; import org.labkey.api.view.RedirectorServlet; +import org.labkey.api.view.ViewContext; import org.labkey.api.view.ViewServlet; import org.labkey.api.view.WebPartFactory; import org.labkey.api.webdav.WebdavResolverImpl; @@ -507,6 +508,7 @@ public void registerServlets(ServletContext servletCtx) URIUtil.TestCase.class, ValidEmail.TestCase.class, VersionNumber.TestCase.class, + ViewContext.TestCase.class, XmlBeansUtil.TestCase.class ); } diff --git a/api/src/org/labkey/api/reports/report/ExternalScriptEngineReport.java b/api/src/org/labkey/api/reports/report/ExternalScriptEngineReport.java index a394302d5a0..2f85fa265e3 100644 --- a/api/src/org/labkey/api/reports/report/ExternalScriptEngineReport.java +++ b/api/src/org/labkey/api/reports/report/ExternalScriptEngineReport.java @@ -371,7 +371,7 @@ protected Object runScript(ScriptEngine engine, ViewContext context, List K handleParameters(ScriptEngineReport report, Collection parameters, ParameterHandler handler) throws IOException { - String sections = (String) HttpView.currentContext().get(renderParam.showSection.name()); + String sections = HttpView.currentContext().getString(renderParam.showSection.name()); List sectionNames = Collections.emptyList(); if (sections != null) diff --git a/api/src/org/labkey/api/reports/report/ScriptProcessReport.java b/api/src/org/labkey/api/reports/report/ScriptProcessReport.java index eed0041dd2b..0d0b7a95f63 100644 --- a/api/src/org/labkey/api/reports/report/ScriptProcessReport.java +++ b/api/src/org/labkey/api/reports/report/ScriptProcessReport.java @@ -184,7 +184,7 @@ public Thumbnail cleanup(ScriptProcessReport report, ContainerUser context) private static K handleParameters(ScriptProcessReport report, Collection parameters, ParameterHandler handler) throws IOException { - String sections = (String)HttpView.currentContext().get(renderParam.showSection.name()); + String sections = HttpView.currentContext().getString(renderParam.showSection.name()); List sectionNames = Collections.emptyList(); if (sections != null) diff --git a/api/src/org/labkey/api/reports/report/ScriptReport.java b/api/src/org/labkey/api/reports/report/ScriptReport.java index 6768d2c5042..47e189eea8e 100644 --- a/api/src/org/labkey/api/reports/report/ScriptReport.java +++ b/api/src/org/labkey/api/reports/report/ScriptReport.java @@ -147,7 +147,7 @@ public Results _generateResults(ViewContext context, boolean allowAsyncQuery) th if (!StringUtils.isEmpty(filterParam)) { - final String filterValue = (String)context.get(filterParam); + final String filterValue = context.getString(filterParam); if (filterValue != null) { @@ -365,12 +365,12 @@ public ActionURL getEditReportURL(ViewContext context) @Override public HttpView getRunReportView(ViewContext context) throws Exception { - String tabId = (String) context.get("tabId"); + String tabId = context.getString("tabId"); if (null == tabId) tabId = context.getActionURL().getParameter("tabId"); - String webpartString = (String) context.get(Report.renderParam.reportWebPart.name()); + String webpartString = context.getString(Report.renderParam.reportWebPart.name()); boolean webpart = (null != webpartString && BooleanFormat.getInstance().parseObject(webpartString)); // Module-based reports are always read-only, but we still allow viewing the report source in the source tab. diff --git a/api/src/org/labkey/api/view/ViewContext.java b/api/src/org/labkey/api/view/ViewContext.java index d8c01ba4a8b..19247f921af 100644 --- a/api/src/org/labkey/api/view/ViewContext.java +++ b/api/src/org/labkey/api/view/ViewContext.java @@ -20,6 +20,8 @@ import jakarta.servlet.http.HttpSession; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; +import org.junit.Assert; +import org.junit.Test; import org.labkey.api.data.Container; import org.labkey.api.data.ContainerManager; import org.labkey.api.security.HasPermission; @@ -42,6 +44,7 @@ import org.springframework.context.MessageSourceResolvable; import org.springframework.context.NoSuchMessageException; import org.springframework.context.support.ResourceBundleMessageSource; +import org.springframework.mock.web.MockHttpServletRequest; import java.io.Closeable; import java.util.ArrayList; @@ -56,8 +59,6 @@ /** * Holds many of the key pieces of context about an HTTP request, including the {@link HttpServletRequest} and * {@link HttpServletResponse} objects, the {@link User} making the request, etc. - * User: matthewb - * Date: Mar 20, 2005 */ public class ViewContext implements MessageSource, ContainerContext, ContainerUser, ApplicationContextAware, HasPermission, HasHttpRequest { @@ -213,11 +214,31 @@ public Map getExtendedProperties() return _map; } + public Object get(String key) + { + return _map.get(key); + } + + @Deprecated // Left behind so not every module needs to be recompiled immediately. TODO: Remove public Object get(Object key) { return _map.get(key); } + /* + * Safer and more convenient than using get() with a String cast. Returns _map.get(key) if it's null or a String. + * Otherwise, throws BadRequestException. See GH Issue 1631. + */ + public @Nullable String getString(String key) + { + Object value = _map.get(key); + if (value == null) + return null; + if (value instanceof String stringValue) + return stringValue; + throw new BadRequestException(String.format("Expected a single string value for key: %s", key)); // No logging, no mothership + } + public Object put(String key, Object value) { return _map.put(key, value); @@ -524,4 +545,22 @@ public boolean isRobot() var r = getRequest(); return null != r && PageFlowUtil.isRobotUserAgent(r.getHeader("User-Agent")); } + + public static class TestCase extends Assert + { + @Test + public void testGetString() + { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.addParameter("single", "value"); + request.addParameter("repeated", "one", "two"); + ViewContext context = new ViewContext(request, null, null); + context.put("number", 42); + + assertEquals("value", context.getString("single")); + assertNull(context.getString("missing")); + assertThrows(BadRequestException.class, () -> context.getString("repeated")); + assertThrows(BadRequestException.class, () -> context.getString("number")); + } + } } diff --git a/query/src/org/labkey/query/reports/ReportsController.java b/query/src/org/labkey/query/reports/ReportsController.java index d4413d2302f..8e507987eda 100644 --- a/query/src/org/labkey/query/reports/ReportsController.java +++ b/query/src/org/labkey/query/reports/ReportsController.java @@ -1318,10 +1318,10 @@ public static class StreamFileAction extends SimpleViewAction @Override public ModelAndView getView(Object o, BindException errors) throws Exception { - String sessionKey = (String) getViewContext().get(ImageUtil.FILE_SESSION_PARAM); - String deleteFile = (String) getViewContext().get(ImageUtil.DELETE_FILE_PARAM); - String attachment = (String) getViewContext().get(ImageUtil.ATTACHMENT_PARAM); - String cacheFile = (String) getViewContext().get(ImageUtil.CACHE_FILE_PARAM); + String sessionKey = getViewContext().getString(ImageUtil.FILE_SESSION_PARAM); + String deleteFile = getViewContext().getString(ImageUtil.DELETE_FILE_PARAM); + String attachment = getViewContext().getString(ImageUtil.ATTACHMENT_PARAM); + String cacheFile = getViewContext().getString(ImageUtil.CACHE_FILE_PARAM); if (sessionKey != null) { FileLike file = ImageUtil.getFileFromSession(getViewContext().getRequest(), sessionKey); @@ -2234,8 +2234,8 @@ public static class ReportSectionsAction extends ReadOnlyApiAction public ApiResponse execute(Object o, BindException errors) { ApiSimpleResponse response = new ApiSimpleResponse(); - ReportIdentifier reportId = ReportService.get().getReportIdentifier((String)getViewContext().get(ReportDescriptor.Prop.reportId.name()), getViewContext().getUser(), getViewContext().getContainer()); - String sections = (String)getViewContext().get(Report.renderParam.showSection.name()); + ReportIdentifier reportId = ReportService.get().getReportIdentifier(getViewContext().getString(ReportDescriptor.Prop.reportId.name()), getViewContext().getUser(), getViewContext().getContainer()); + String sections = getViewContext().getString(Report.renderParam.showSection.name()); if (reportId != null) { Report report = reportId.getReport(getViewContext()); diff --git a/study/src/org/labkey/study/controllers/StudyController.java b/study/src/org/labkey/study/controllers/StudyController.java index 05e5476f0eb..8e1a4ec86ca 100644 --- a/study/src/org/labkey/study/controllers/StudyController.java +++ b/study/src/org/labkey/study/controllers/StudyController.java @@ -817,7 +817,7 @@ protected Report getReport(QueryReportForm form) { if (_report == null) { - String reportId = (String)getViewContext().get(DATASET_REPORT_ID_PARAMETER_NAME); + String reportId = getViewContext().getString(DATASET_REPORT_ID_PARAMETER_NAME); ReportIdentifier identifier = ReportService.get().getReportIdentifier(reportId, getViewContext().getUser(), getViewContext().getContainer()); if (identifier != null) @@ -838,7 +838,7 @@ protected ModelAndView getHtmlView(QueryReportForm form, BindException errors) t return HttpView.redirect(createRedirectURLfrom(DatasetAction.class, context)); } - int datasetId = NumberUtils.toInt((String)context.get(Dataset.DATASET_KEY), -1); + int datasetId = NumberUtils.toInt(context.getString(Dataset.DATASET_KEY), -1); Dataset def = StudyManager.getInstance().getDatasetDefinition(getStudyRedirectIfNull(), datasetId); if (def != null) @@ -896,7 +896,7 @@ private DatasetDefinition getDatasetDefinition() } else { - String entityId = (String)getViewContext().get("entityId"); + String entityId = getViewContext().getString("entityId"); if (null != entityId) _def = StudyManager.getInstance().getDatasetDefinitionByEntityId(getStudyRedirectIfNull(), entityId); } @@ -1151,7 +1151,7 @@ public ModelAndView getView(ParticipantForm form, BindException errors) if (!errorMsg.isEmpty()) return HtmlView.err(errorMsg.toString()); - String viewName = (String) getViewContext().get(DATASET_VIEW_NAME_PARAMETER_NAME); + String viewName = getViewContext().getString(DATASET_VIEW_NAME_PARAMETER_NAME); CohortFilter cohortFilter = CohortFilterFactory.getFromURL(getContainer(), getUser(), getViewContext().getActionURL(), DatasetQueryView.DATAREGION); // display the next and previous buttons only if we have a cached participant index @@ -2090,7 +2090,7 @@ public ModelAndView getView(VisitForm form, boolean reshow, BindException errors redirectToSharedVisitStudy(study, getViewContext().getActionURL()); - int id = NumberUtils.toInt((String)getViewContext().get("id")); + int id = NumberUtils.toInt(getViewContext().getString("id")); _v = StudyManager.getInstance().getVisitForRowId(study, id); if (_v == null) { @@ -2113,7 +2113,7 @@ public boolean handlePost(VisitForm form, BindException errors) redirectToSharedVisitStudy(study, getViewContext().getActionURL()); // UNDONE: how do I get struts to handle this checkbox? - postedVisit.setShowByDefault(null != StringUtils.trimToNull((String)getViewContext().get("showByDefault"))); + postedVisit.setShowByDefault(null != StringUtils.trimToNull(getViewContext().getString("showByDefault"))); // UNDONE: reshow is broken for this form, but we have to validate Collection visits = StudyManager.getInstance().getVisitManager(study).getVisits(); @@ -3116,7 +3116,7 @@ public void validateCommand(DeleteDatasetRowsForm target, Errors errors) @Override public boolean handlePost(DeleteDatasetRowsForm form, BindException errors) { - String originalSourceLsid = (String)getViewContext().get("sourceLsid"); + String originalSourceLsid = getViewContext().getString("sourceLsid"); Dataset.PublishSource publishSource = _def.getPublishSource(); if (form.getPublishSourceId() != null && publishSource != null) diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index 5ecec32c213..adbc5d3db7d 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -158,7 +158,7 @@ public static class StreamFileAction extends SimpleViewAction @Override public ModelAndView getView(Object o, BindException errors) throws Exception { - String sessionKey = (String) getViewContext().get(ImageUtil.FILE_SESSION_PARAM); + String sessionKey = getViewContext().getString(ImageUtil.FILE_SESSION_PARAM); if (null == sessionKey) { //TODO: Return a GIF that says not found?? @@ -539,13 +539,6 @@ public ActionURL getSuccessURL(CrosstabDesignBean crosstabDesignBean) @Override public void addNavTrail(NavTree root) { -/* - ViewContext context = getViewContext(); - int datasetId = null == context.get(DatasetDefinition.DATASETKEY) ? 0 : Integer.parseInt((String) context.get(DatasetDefinition.DATASETKEY)); - int visitRowId = null == context.get("visitRowId") ? 0 : Integer.parseInt((String) context.get("visitRowId")); - - return _appendNavTrail(root, "Crosstab View Builder", datasetId, visitRowId); -*/ setHelpTopic("crosstabReports"); root.addChild("Crosstab Report Builder"); } diff --git a/study/src/org/labkey/study/controllers/security/SecurityController.java b/study/src/org/labkey/study/controllers/security/SecurityController.java index 344d8c36000..abef61aacd7 100644 --- a/study/src/org/labkey/study/controllers/security/SecurityController.java +++ b/study/src/org/labkey/study/controllers/security/SecurityController.java @@ -143,7 +143,7 @@ public boolean handlePost(Object o, BindException errors) @Override public ActionURL getSuccessURL(Object o) { - String redirect = (String)getViewContext().get("redirect"); + String redirect = getViewContext().getString("redirect"); if (redirect != null) return new ActionURL(redirect); @@ -441,7 +441,7 @@ private MutableSecurityPolicy policyFromPost(Map group2Perm, Se @Override public ActionURL getSuccessURL(Object o) { - String redirect = (String) getViewContext().get("redirect"); + String redirect = getViewContext().getString("redirect"); if (redirect != null) return new ActionURL(redirect); @@ -608,7 +608,7 @@ public boolean handlePost(StudySecurityForm form, BindException errors) @Override public ActionURL getSuccessURL(StudySecurityForm studySecurityForm) { - String redirect = (String) getViewContext().get("redirect"); + String redirect = getViewContext().getString("redirect"); if (redirect != null) return new ActionURL(redirect); diff --git a/study/src/org/labkey/study/reports/ParticipantReport.java b/study/src/org/labkey/study/reports/ParticipantReport.java index bf28077fe90..3fd24d30e41 100644 --- a/study/src/org/labkey/study/reports/ParticipantReport.java +++ b/study/src/org/labkey/study/reports/ParticipantReport.java @@ -85,7 +85,7 @@ public HttpView renderReport(ViewContext context) JspView view = new JspView<>("/org/labkey/study/view/participantReport.jsp", form); - String rwp = (String)context.get("reportWebPart"); + String rwp = context.getString("reportWebPart"); form.setExpanded(rwp == null); form.setAllowOverflow(!BooleanUtils.toBoolean(rwp));