diff --git a/core/src/client/AssayDesigner/AssayDesigner.tsx b/core/src/client/AssayDesigner/AssayDesigner.tsx index 518b97ca595..3634428ec7a 100644 --- a/core/src/client/AssayDesigner/AssayDesigner.tsx +++ b/core/src/client/AssayDesigner/AssayDesigner.tsx @@ -140,8 +140,9 @@ export class App extends React.Component { navigate(defaultUrl: string) { this._dirty = false; - - window.location.href = this.state.returnUrl || defaultUrl; + const redirectUrl = this.state.returnUrl || defaultUrl; + // TODO refactor this and other usages in platform/core/src/client to a helper safeRedirect() function from @labkey/components + window.location.href = ActionURL.buildURL('core', 'safeRedirect', undefined, { returnUrl: redirectUrl }); } onCancel = (): void => { diff --git a/core/src/client/DataClassDesigner/DataClassDesigner.tsx b/core/src/client/DataClassDesigner/DataClassDesigner.tsx index 4a19ca02075..422d3aac188 100644 --- a/core/src/client/DataClassDesigner/DataClassDesigner.tsx +++ b/core/src/client/DataClassDesigner/DataClassDesigner.tsx @@ -68,9 +68,8 @@ class DataClassDesignerWrapper extends React.Component { navigate(defaultUrl: string) { this._dirty = false; - - const returnUrl = ActionURL.getReturnUrl(); - window.location.href = returnUrl || defaultUrl; + const redirectUrl = ActionURL.getReturnUrl() || defaultUrl; + window.location.href = ActionURL.buildURL('core', 'safeRedirect', undefined, { returnUrl: redirectUrl }); } onCancel = () => { diff --git a/core/src/client/DatasetDesigner/DatasetDesigner.tsx b/core/src/client/DatasetDesigner/DatasetDesigner.tsx index 50eec7df716..f30c22ebd52 100644 --- a/core/src/client/DatasetDesigner/DatasetDesigner.tsx +++ b/core/src/client/DatasetDesigner/DatasetDesigner.tsx @@ -66,9 +66,8 @@ class DatasetDesigner extends PureComponent { navigate(defaultUrl: string): void { this._dirty = false; - - const returnUrl = ActionURL.getReturnUrl(); - window.location.href = returnUrl || defaultUrl; + const redirectUrl = ActionURL.getReturnUrl() || defaultUrl; + window.location.href = ActionURL.buildURL('core', 'safeRedirect', undefined, { returnUrl: redirectUrl }); } navigateOnComplete(model: DatasetModel): void { diff --git a/core/src/client/DomainDesigner/DomainDesigner.tsx b/core/src/client/DomainDesigner/DomainDesigner.tsx index 08ae19c2d91..240bf106d04 100644 --- a/core/src/client/DomainDesigner/DomainDesigner.tsx +++ b/core/src/client/DomainDesigner/DomainDesigner.tsx @@ -161,9 +161,8 @@ class DomainDesigner extends React.PureComponent> { navigate = (): void => { this._dirty = false; - - const returnUrl = ActionURL.getReturnUrl(); - window.location.href = returnUrl || ActionURL.buildURL('project', 'begin', getServerContext().container.path); + const redirectUrl = ActionURL.getReturnUrl() || ActionURL.buildURL('project', 'begin', getServerContext().container.path); + window.location.href = ActionURL.buildURL('core', 'safeRedirect', undefined, { returnUrl: redirectUrl }); }; renderWarningConfirm() { diff --git a/core/src/client/IssuesListDesigner/IssuesListDesigner.tsx b/core/src/client/IssuesListDesigner/IssuesListDesigner.tsx index da294bfad17..d9dd54ba085 100644 --- a/core/src/client/IssuesListDesigner/IssuesListDesigner.tsx +++ b/core/src/client/IssuesListDesigner/IssuesListDesigner.tsx @@ -81,9 +81,8 @@ class IssuesListDesigner extends React.Component<{}, State> { navigate = (defaultUrl: string) => { this._dirty = false; - - const returnUrl = ActionURL.getReturnUrl(); - window.location.href = returnUrl || defaultUrl; + const redirectUrl = ActionURL.getReturnUrl() || defaultUrl; + window.location.href = ActionURL.buildURL('core', 'safeRedirect', undefined, { returnUrl: redirectUrl }); }; onComplete = (model: IssuesListDefModel) => { diff --git a/core/src/client/ListDesigner/ListDesigner.tsx b/core/src/client/ListDesigner/ListDesigner.tsx index f3c0ad5da80..9f5022b3788 100644 --- a/core/src/client/ListDesigner/ListDesigner.tsx +++ b/core/src/client/ListDesigner/ListDesigner.tsx @@ -104,8 +104,8 @@ export class ListDesigner extends React.Component { navigate = async (returnUrlProvider: () => Promise, model?: ListModel): Promise => { this._dirty = false; - - window.location.href = this.getReturnUrl(model) ?? (await returnUrlProvider()); + const redirectUrl = this.getReturnUrl(model) ?? (await returnUrlProvider()); + window.location.href = ActionURL.buildURL('core', 'safeRedirect', undefined, { returnUrl: redirectUrl }); }; getReturnUrl = (model?: ListModel): string => { diff --git a/core/src/client/SampleTypeDesigner/SampleTypeDesigner.tsx b/core/src/client/SampleTypeDesigner/SampleTypeDesigner.tsx index e3b0e084b06..cb68d02ba79 100644 --- a/core/src/client/SampleTypeDesigner/SampleTypeDesigner.tsx +++ b/core/src/client/SampleTypeDesigner/SampleTypeDesigner.tsx @@ -137,9 +137,8 @@ class SampleTypeDesignerWrapper extends React.PureComponent { navigate(defaultUrl: string) { this._dirty = false; - - const returnUrl = ActionURL.getReturnUrl(); - window.location.href = returnUrl || defaultUrl; + const redirectUrl = ActionURL.getReturnUrl() || defaultUrl; + window.location.href = ActionURL.buildURL('core', 'safeRedirect', undefined, { returnUrl: redirectUrl }); } render() { diff --git a/core/src/org/labkey/core/CoreController.java b/core/src/org/labkey/core/CoreController.java index acaf441e3aa..1feb49b9a91 100644 --- a/core/src/org/labkey/core/CoreController.java +++ b/core/src/org/labkey/core/CoreController.java @@ -35,7 +35,9 @@ import org.labkey.api.action.ExportAction; import org.labkey.api.action.MutatingApiAction; import org.labkey.api.action.ReadOnlyApiAction; +import org.labkey.api.action.ReturnUrlForm; import org.labkey.api.action.SimpleApiJsonForm; +import org.labkey.api.action.SimpleRedirectAction; import org.labkey.api.action.SimpleViewAction; import org.labkey.api.action.SpringActionController; import org.labkey.api.admin.AbstractFolderContext.ExportType; @@ -204,10 +206,6 @@ import static org.labkey.api.view.template.WarningService.SESSION_WARNINGS_BANNER_KEY; -/** - * User: jeckels - * Date: Jan 4, 2007 - */ public class CoreController extends SpringActionController { private static final Map _customStylesheetCache = new ConcurrentHashMap<>(); @@ -2908,4 +2906,20 @@ public void setProvider(String provider) } + // Called by various client components to ensure safe redirects, GitHub Issue #1023. This action redirects to + // local URLs only, never to an external site, even if the host is on the "Allowed External Redirect Hosts" list. + // Why is this safe? First, ActionURL is guaranteed to be a local URL (schema, host, and port are always taken + // from local AppProps, even if an absolute URL is requested via getURIString() or similar). Second, + // SimpleRedirectAction throws RedirectException which also guarantees local redirects (instances of that class + // always use getLocalURIString()). + @SuppressWarnings("unused") + @RequiresNoPermission + public static class SafeRedirectAction extends SimpleRedirectAction + { + @Override + public ActionURL getRedirectURL(ReturnUrlForm form) throws Exception + { + return form.getReturnActionURL(AppProps.getInstance().getHomePageActionURL()); + } + } } diff --git a/study/test/src/org/labkey/test/tests/study/AssayTest.java b/study/test/src/org/labkey/test/tests/study/AssayTest.java index 89a8f2bf686..a4186af26a2 100644 --- a/study/test/src/org/labkey/test/tests/study/AssayTest.java +++ b/study/test/src/org/labkey/test/tests/study/AssayTest.java @@ -29,6 +29,7 @@ import org.labkey.test.categories.Assays; import org.labkey.test.categories.Daily; import org.labkey.test.components.CustomizeView; +import org.labkey.test.components.DomainDesignerPage; import org.labkey.test.components.assay.AssayConstants; import org.labkey.test.components.domain.DomainFieldRow; import org.labkey.test.components.domain.DomainFormPanel; @@ -48,13 +49,13 @@ import org.labkey.test.util.TestDataGenerator; import org.labkey.test.util.data.TestDataUtils; -import java.io.File; import java.io.IOException; import java.util.ArrayList; import java.util.List; import java.util.Map; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; @@ -69,6 +70,7 @@ public class AssayTest extends AbstractAssayTest private static final String ISSUE_53616_ASSAY = "Issue53616Assay"; private static final String ISSUE_53616_PROJECT = "Issue53616Project" + TRICKY_CHARACTERS_FOR_PROJECT_NAMES; private static final String ISSUE_53831_PROJECT = "Issue53831Project" + TRICKY_CHARACTERS_FOR_PROJECT_NAMES; + private static final String GH_1023_PROJECT = "GH1023Project" + TRICKY_CHARACTERS_FOR_PROJECT_NAMES; private static final String SAMPLE_FIELD_TEST_ASSAY = "SampleFieldTestAssay"; private static final String SAMPLE_FIELD_PROJECT_NAME = "Sample Field Test Project" + TRICKY_CHARACTERS_FOR_PROJECT_NAMES; @@ -90,6 +92,7 @@ protected void doCleanup(boolean afterTest) throws TestTimeoutException _containerHelper.deleteProject(ISSUE_53616_PROJECT, false); _containerHelper.deleteProject(ISSUE_53625_PROJECT, false); _containerHelper.deleteProject(ISSUE_53831_PROJECT, false); + _containerHelper.deleteProject(GH_1023_PROJECT, false); _userHelper.deleteUsers(false, TEST_ASSAY_USR_PI1, TEST_ASSAY_USR_TECH1); } @@ -1142,6 +1145,27 @@ private void verifyAssayImportForLookupValidator(String assayName, FieldInfo loo checker().verifyEquals("Lookup values not as expected.", List.of("123"), dataTable.getColumnDataAsText(lookupField.getLabel())); } + @Test // GitHub Issue #1023 + public void testNoExternalReturnUrlRedirect() throws Exception + { + _containerHelper.createProject(GH_1023_PROJECT, "Assay"); + goToProjectHome(GH_1023_PROJECT); + + // Navigate to domain designer with an external returnUrl. The safeRedirect action + // should prevent external redirects, falling back to the local home page instead. + String domainDesignerUrl = WebTestHelper.buildURL("assay", GH_1023_PROJECT, "designer", + Map.of("providerName", "General", "returnUrl", "https://labkey.com")); + beginAt(domainDesignerUrl); + DomainDesignerPage domainDesignerPage = new DomainDesignerPage(getDriver()); + domainDesignerPage.fieldsPanel(); + domainDesignerPage.clickCancel(); + String postCancelUrl = getDriver().getCurrentUrl(); + assertFalse("Cancel with an external returnUrl should not navigate to an external site", + postCancelUrl.contains("labkey.com")); + assertTrue("Cancel with an external returnUrl should redirect to a local LabKey page instead of: " + postCancelUrl, + WebTestHelper.isTestServerUrl(postCancelUrl)); + } + @Override protected BrowserType bestBrowser() { diff --git a/study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java b/study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java index 202ceadbffa..d7c71b3687c 100644 --- a/study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java +++ b/study/test/src/org/labkey/test/tests/study/StudyDatasetsTest.java @@ -27,6 +27,7 @@ import org.labkey.test.TestFileUtils; import org.labkey.test.WebTestHelper; import org.labkey.test.categories.Daily; +import org.labkey.test.components.DomainDesignerPage; import org.labkey.test.components.LookAndFeelScatterPlot; import org.labkey.test.components.LookAndFeelTimeChart; import org.labkey.test.components.domain.DomainFormPanel; @@ -784,4 +785,22 @@ protected void clickViewDetailsLink(String reportName) waitForElement(link); clickAndWait(link); } + + @Test // GitHub Issue #1023 + public void testNoExternalReturnUrlRedirect() throws Exception + { + // Navigate to domain designer with an external returnUrl. The safeRedirect action + // should prevent external redirects, falling back to the local home page instead. + String domainDesignerUrl = WebTestHelper.buildURL("study", getProjectName() + "/" + getFolderName(), "defineDatasetType", + Map.of("returnUrl", "https://labkey.com")); + beginAt(domainDesignerUrl); + DomainDesignerPage domainDesignerPage = new DomainDesignerPage(getDriver()); + domainDesignerPage.fieldsPanel(); + domainDesignerPage.clickCancel(); + String postCancelUrl = getDriver().getCurrentUrl(); + assertFalse("Cancel with an external returnUrl should not navigate to an external site", + postCancelUrl.contains("labkey.com")); + assertTrue("Cancel with an external returnUrl should redirect to a local LabKey page instead of: " + postCancelUrl, + WebTestHelper.isTestServerUrl(postCancelUrl)); + } }