diff --git a/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java b/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java index aee4cb7f297..f4d214d5ae8 100644 --- a/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java +++ b/api/src/org/labkey/api/study/publish/AbstractPublishConfirmAction.java @@ -116,6 +116,9 @@ else if (!_targetStudy.hasPermission(getUser(), InsertPermission.class)) if (_targetStudy != null) { + if (!_targetStudy.hasPermission(getUser(), InsertPermission.class)) + errors.reject(SpringActionController.ERROR_MSG, "You do not have permission to link data to the study in " + _targetStudy.getPath() + "."); + Study study = StudyService.get().getStudy(_targetStudy); if (study == null) { @@ -336,6 +339,7 @@ private void attemptLinkage(FORM form, BindException errors, boolean missingStudy = false; boolean badVisitIds = false; boolean badDates = false; + boolean noPermissionStudy = false; int index = 0; for (int objectId : allObjects) { @@ -380,6 +384,9 @@ private void attemptLinkage(FORM form, BindException errors, } else { + if (selected && !rowLevelTargetStudy.hasPermission(getUser(), InsertPermission.class)) + noPermissionStudy = true; + postedTargetStudies.put(objectId, rowLevelTargetStudy.getId()); if (StudyPublishService.get().getTimepointType(rowLevelTargetStudy) == TimepointType.VISIT) @@ -442,6 +449,8 @@ private void attemptLinkage(FORM form, BindException errors, if (missingStudy) errors.reject(null, "You must specify a Target Study for all selected rows."); + if (noPermissionStudy) + errors.reject(null, "You do not have permission to link data to one or more of the selected target studies."); if (missingPtid) errors.reject(null, "You must specify a Participant ID for all selected rows."); if (missingVisitId) diff --git a/filecontent/src/org/labkey/filecontent/FileContentController.java b/filecontent/src/org/labkey/filecontent/FileContentController.java index 3f01703bb95..a6fe85c88c6 100644 --- a/filecontent/src/org/labkey/filecontent/FileContentController.java +++ b/filecontent/src/org/labkey/filecontent/FileContentController.java @@ -945,7 +945,15 @@ public void validateForm(SimpleApiJsonForm form, Errors errors) { FileContentServiceImpl fileContentService = FileContentServiceImpl.getInstance(); WebdavResource resource = fileContentService.getResource(String.valueOf(fileProps.get("id"))); - if (resource != null && !resource.getActions(getUser()).isEmpty()) + + // GitHub Issue 1243: check resource container + if (resource == null || !getContainer().getEntityId().equals(resource.getContainerId())) + { + errors.reject(ERROR_MSG, String.format(FILE_PROP_ERROR, "Invalid file", fileProps.get("id"))); + return; + } + + if (!resource.getActions(getUser()).isEmpty()) { errors.reject(ERROR_MSG, String.format(FILE_PROP_ERROR, resource.getName(), "has been previously processed, properties cannot be edited")); return; diff --git a/issues/src/org/labkey/issue/IssuesController.java b/issues/src/org/labkey/issue/IssuesController.java index ee2eb357a1a..8a90727cca3 100644 --- a/issues/src/org/labkey/issue/IssuesController.java +++ b/issues/src/org/labkey/issue/IssuesController.java @@ -28,6 +28,8 @@ import org.jetbrains.annotations.Nullable; import org.json.JSONArray; import org.json.JSONObject; +import org.junit.After; +import org.junit.Before; import org.junit.Test; import org.labkey.api.action.ApiResponse; import org.labkey.api.action.ApiSimpleResponse; @@ -106,6 +108,7 @@ import org.labkey.api.security.permissions.InsertPermission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.UpdatePermission; +import org.labkey.api.security.roles.EditorRole; import org.labkey.api.security.roles.FolderAdminRole; import org.labkey.api.security.roles.OwnerRole; import org.labkey.api.security.roles.ReaderRole; @@ -115,12 +118,12 @@ import org.labkey.api.util.DOM; import org.labkey.api.util.GUID; import org.labkey.api.util.HtmlString; +import org.labkey.api.util.InputBuilder; import org.labkey.api.util.JsonUtil; import org.labkey.api.util.PageFlowUtil; import org.labkey.api.util.Pair; import org.labkey.api.util.TestContext; import org.labkey.api.util.URLHelper; -import org.labkey.api.util.InputBuilder; import org.labkey.api.view.ActionURL; import org.labkey.api.view.HtmlView; import org.labkey.api.view.HttpView; @@ -151,6 +154,7 @@ import org.labkey.issue.view.IssuesListView; import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.PropertyValues; +import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.util.LinkedCaseInsensitiveMap; import org.springframework.validation.BindException; import org.springframework.validation.Errors; @@ -1731,6 +1735,13 @@ public URLHelper getSuccessURL(IssuesDomainKindProperties form) } } + private static List getSelectableAssignmentGroups(Container c, User user) + { + return SecurityManager.getGroups(c.getProject(), true).stream() + .filter(group -> !group.isGuests() && (!group.isUsers() || user.hasRootAdminPermission())) + .toList(); + } + @Marshal(Marshaller.Jackson) @RequiresPermission(AdminPermission.class) public static class GetProjectGroupsAction extends ReadOnlyApiAction @@ -1740,7 +1751,7 @@ public Object execute(Object form, BindException errors) { List groups = new ArrayList<>(); - SecurityManager.getGroups(getContainer().getProject(), true).stream().filter(group -> !group.isGuests() && (!group.isUsers() || getUser().hasRootAdminPermission())).forEach(group -> { + getSelectableAssignmentGroups(getContainer(), getUser()).forEach(group -> { String displayText = (group.isProjectGroup() ? "" : "Site: ") + group.getName(); UserGroupForm userGroups = new UserGroupForm(); @@ -1764,8 +1775,11 @@ public Object execute(UserGroupForm form, BindException errors) if (null != form.getGroupId()) { + // ensure group is in scope for the container Group group = SecurityManager.getGroup(form.getGroupId()); - if (group != null) + boolean inScope = group != null && getSelectableAssignmentGroups(getContainer(), getUser()) + .stream().anyMatch(g -> g.getUserId() == group.getUserId()); + if (inScope) { for (User user : SecurityManager.getAllGroupMembers(group, MemberType.ACTIVE_USERS, group.isUsers())) { @@ -2458,4 +2472,79 @@ private static void ensureIssuesEnabled(Container c) } } } + + /** + * GitHub Issue #1243 regression test. + */ + public static class GetUsersForGroupScopingTestCase extends AbstractContainerScopingTest + { + private static final String PROJECT_A = "IssuesGroupScopingA"; + private static final String PROJECT_B = "IssuesGroupScopingB"; + + private Container _projectA; + private Group _groupA; + private Group _groupB; + private User _member; + + @Before + public void setup() throws Exception + { + deleteProjects(); + User admin = getAdmin(); + _projectA = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_A, admin); + Container projectB = ContainerManager.createContainer(ContainerManager.getRoot(), PROJECT_B, admin); + + // Security groups must be associated with a project, so each group's container is its owning project. + _groupA = SecurityManager.createGroup(_projectA, "ScopingGroupA", admin); + _groupB = SecurityManager.createGroup(projectB, "ScopingGroupB", admin); + + // A user who can be assigned issues in project A (UpdatePermission via Editor) and is a member of both groups. + _member = createUserInRole(_projectA, EditorRole.class); + SecurityManager.addMember(_groupA, _member); + SecurityManager.addMember(_groupB, _member); + } + + @After + public void tearDown() + { + deleteProjects(); + } + + @Test + public void doesNotEnumerateAnotherProjectsGroupMembers() throws Exception + { + // group B is in a different project from the source request. + String content = requestUsersForGroup(_projectA, _groupB).getContentAsString(); + assertFalse("A group id from another project must not enumerate that group's members through this folder", + content.contains("\"userId\" : " + _member.getUserId())); + } + + @Test + public void enumeratesOwnProjectGroupMembers() throws Exception + { + // Control: the request project's own group resolves in scope and returns its members. + String content = requestUsersForGroup(_projectA, _groupA).getContentAsString(); + assertTrue("An in-project group id must enumerate its members", + content.contains("\"userId\" : " + _member.getUserId())); + } + + private MockHttpServletResponse requestUsersForGroup(Container project, Group group) throws Exception + { + ActionURL url = new ActionURL(GetUsersForGroupAction.class, project) + .addParameter("groupId", group.getUserId()); + return get(url, getAdmin()); + } + + private void deleteProjects() + { + User admin = getAdmin(); + for (String name : new String[]{PROJECT_A, PROJECT_B}) + { + Container c = ContainerManager.getForPath("/" + name); + if (c != null) + ContainerManager.deleteAll(c, admin); + } + _projectA = null; + } + } } diff --git a/issues/src/org/labkey/issue/IssuesModule.java b/issues/src/org/labkey/issue/IssuesModule.java index 06630d6dbad..dc9aad60385 100644 --- a/issues/src/org/labkey/issue/IssuesModule.java +++ b/issues/src/org/labkey/issue/IssuesModule.java @@ -195,7 +195,8 @@ public Set getIntegrationTests() { return Set.of( org.labkey.issue.model.IssueManager.TestCase.class, - org.labkey.issue.IssuesController.MoveActionContainerScopingTestCase.class + org.labkey.issue.IssuesController.MoveActionContainerScopingTestCase.class, + org.labkey.issue.IssuesController.GetUsersForGroupScopingTestCase.class ); } diff --git a/specimen/src/org/labkey/specimen/actions/SpecimenController.java b/specimen/src/org/labkey/specimen/actions/SpecimenController.java index 059d63a9fcf..1e917b547ef 100644 --- a/specimen/src/org/labkey/specimen/actions/SpecimenController.java +++ b/specimen/src/org/labkey/specimen/actions/SpecimenController.java @@ -1182,12 +1182,16 @@ public static class SpecimenEventsRedirectAction extends SimpleViewAction getIntegrationTests() DatasetController.DatasetAuditHistoryScopingTestCase.class, DatasetUpdateService.TestCase.class, DatasetLsidImportHelper.TestCase.class, + PublishConfirmContainerScopingTest.class, CreateChildStudyAction.ContainerScopingTestCase.class, StudyController.ContainerScopingTestCase.class, ReportsController.ContainerScopingTestCase.class); diff --git a/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java b/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java new file mode 100644 index 00000000000..5685ce2b157 --- /dev/null +++ b/study/src/org/labkey/study/controllers/publish/PublishConfirmContainerScopingTest.java @@ -0,0 +1,117 @@ +/* + * Copyright (c) 2026 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.study.controllers.publish; + +import org.junit.Before; +import org.junit.Test; +import org.labkey.api.data.Container; +import org.labkey.api.exp.PropertyType; +import org.labkey.api.exp.api.ExpSampleType; +import org.labkey.api.exp.api.SampleTypeService; +import org.labkey.api.gwt.client.model.GWTPropertyDescriptor; +import org.labkey.api.security.User; +import org.labkey.api.security.permissions.AbstractContainerScopingTest; +import org.labkey.api.security.roles.EditorRole; +import org.labkey.api.study.StudyService; +import org.labkey.api.study.TimepointType; +import org.labkey.api.view.ActionURL; +import org.labkey.api.view.UnauthorizedException; +import org.labkey.api.view.ViewContext; +import org.springframework.validation.BindException; + +import java.util.List; + +public class PublishConfirmContainerScopingTest extends AbstractContainerScopingTest +{ + private Container _source; + private Container _target; + private ExpSampleType _sampleType; + + @Before + public void setup() throws Exception + { + _source = createContainer("Source"); + _target = createContainer("Target"); + + StudyService.get().createStudy(_target, getAdmin(), "STUDY-4 scoping target", TimepointType.DATE, false); + _sampleType = createSampleType(_source); + + } + + @Test + public void testConfirmRejectsTargetStudyWithoutInsertPermission() throws Exception + { + // Editor in the source folder only: holds InsertPermission where the action runs, but none in the target study + User editor = createUserInRole(_source, EditorRole.class); + + assertFalse("Linking into a target study the caller cannot insert into must be rejected", + validate(_source, _sampleType, _target, editor)); + } + + @Test + public void testConfirmAllowsTargetStudyWithInsertPermission() throws Exception + { + // Positive control: the same caller is also granted insert (Editor) in the target study, so the guard must not + // fire — proving it rejects only the cross-container case rather than every link + User editor = createUserInRole(_source, EditorRole.class); + grantRole(editor, _target, EditorRole.class); + + assertTrue("Linking into a target study the caller can insert into must be allowed", + validate(_source, _sampleType, _target, editor)); + } + + /** + * Run the publish-confirm form's validation as {@code user}, linking {@code sampleType} (in {@code source}) to the + * study in {@code target}, returns true if validation succeeds, false otherwise. + */ + private boolean validate(Container source, ExpSampleType sampleType, Container target, User user) + { + ActionURL url = new ActionURL("study", "sampleTypePublishConfirm", source); + ViewContext context = ViewContext.getMockViewContext(user, source, url, false); + + SampleTypePublishConfirmAction action = new SampleTypePublishConfirmAction(); + action.setViewContext(context); + + SampleTypePublishConfirmAction.SampleTypePublishConfirmForm form = new SampleTypePublishConfirmAction.SampleTypePublishConfirmForm(); + form.setViewContext(context); + form.setRowId(sampleType.getRowId()); + form.setTargetStudy(new String[]{target.getId()}); + form.setReturnUrl(url.toString()); + + try + { + BindException errors = new BindException(form, "form"); + action.validateCommand(form, errors); + } + catch (UnauthorizedException e) + { + return false; + } + return true; + } + + private ExpSampleType createSampleType(Container c) throws Exception + { + List props = List.of( + new GWTPropertyDescriptor("Name", "string"), + new GWTPropertyDescriptor("Date", PropertyType.DATE.getTypeUri()), + new GWTPropertyDescriptor("PTID", "string") + ); + + return SampleTypeService.get().createSampleType(c, getAdmin(), "STUDY4ScopingSamples", null, + props, List.of(), -1,-1,-1,-1,null); + } +} diff --git a/study/src/org/labkey/study/controllers/reports/ReportsController.java b/study/src/org/labkey/study/controllers/reports/ReportsController.java index 65f56b36497..3aeedeaba80 100644 --- a/study/src/org/labkey/study/controllers/reports/ReportsController.java +++ b/study/src/org/labkey/study/controllers/reports/ReportsController.java @@ -70,6 +70,7 @@ import org.labkey.api.study.reports.CrosstabReportDescriptor; import org.labkey.api.util.ImageUtil; import org.labkey.api.util.PageFlowUtil; +import org.labkey.api.util.ReturnURLString; import org.labkey.api.util.TestContext; import org.labkey.api.util.URLHelper; import org.labkey.api.util.UniqueID; @@ -290,8 +291,14 @@ public boolean handlePost(SaveReportViewForm form, BindException errors) @Override public ActionURL getSuccessURL(SaveReportViewForm form) { - if (!StringUtils.isBlank(form.getRedirectUrl())) - return new ActionURL(form.getRedirectUrl()); + // Issue: redirectUrl is client-controlled. ReturnURLString validates it at bind time. + ReturnURLString redirectUrl = form.getRedirectUrl(); + if (redirectUrl != null && !redirectUrl.isEmpty()) + { + ActionURL url = redirectUrl.getActionURL(); + if (url != null) + return url; + } // after the save just redirect to the newly created view, ask the report for its run URL Report r = ReportService.get().getReport(getContainer(), _savedReportId); @@ -461,7 +468,8 @@ public ModelAndView getView(CrosstabDesignBean form, boolean reshow, BindExcepti bean.setQueryName(form.getQueryName()); bean.setDataRegionName(form.getDataRegionName()); bean.setViewName(form.getViewName()); - bean.setRedirectUrl(form.getRedirectUrl()); + if (form.getRedirectUrl() != null) + bean.setRedirectUrl(new ReturnURLString(form.getRedirectUrl())); bean.setErrors(errors); if (!getUser().isGuest()) @@ -836,7 +844,7 @@ public static class SaveReportViewForm extends SaveReportForm private String _schemaName; private String _viewName; private String _dataRegionName; - private String _redirectUrl; + private ReturnURLString _redirectUrl; public SaveReportViewForm() { @@ -926,12 +934,12 @@ public String getDataRegionName() return _dataRegionName; } - public String getRedirectUrl() + public ReturnURLString getRedirectUrl() { return _redirectUrl; } - public void setRedirectUrl(String redirectUrl) + public void setRedirectUrl(ReturnURLString redirectUrl) { _redirectUrl = redirectUrl; } @@ -1362,5 +1370,27 @@ controller.new ParticipantReportAction(), controller.new CreateQueryReportAction() ); } + + // GitHub Issue #1243 regression test. + @Test + public void redirectUrlConstrainedToAllowableHost() + { + SaveReportViewForm offHost = new SaveReportViewForm(); + offHost.setRedirectUrl(new ReturnURLString("https://evil.example.com/phish")); + assertNull("Off-host redirectUrl must not resolve to an off-site ActionURL", + offHost.getRedirectUrl().getActionURL()); + + SaveReportViewForm local = new SaveReportViewForm(); + local.setRedirectUrl(new ReturnURLString("/study-reports/begin.view?x=1")); + ActionURL localUrl = local.getRedirectUrl().getActionURL(); + assertNotNull("A local redirectUrl should resolve to an ActionURL", localUrl); + assertTrue("A local redirectUrl should be preserved same-origin", + localUrl.getLocalURIString().contains("begin.view")); + + SaveReportViewForm blank = new SaveReportViewForm(); + blank.setRedirectUrl(new ReturnURLString("")); + assertTrue("A blank redirectUrl should be empty so the action falls through to the run URL", + blank.getRedirectUrl().isEmpty()); + } } }