diff --git a/assay/package-lock.json b/assay/package-lock.json index c000076459a..232ab795b20 100644 --- a/assay/package-lock.json +++ b/assay/package-lock.json @@ -8,7 +8,7 @@ "name": "assay", "version": "0.0.0", "dependencies": { - "@labkey/components": "7.40.0" + "@labkey/components": "7.40.1" }, "devDependencies": { "@labkey/build": "9.1.4", @@ -3821,9 +3821,9 @@ } }, "node_modules/@labkey/components": { - "version": "7.40.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.40.0.tgz", - "integrity": "sha512-yOBkfRaW6O+mWakUrKlcCFSI/UADW+eHqJPufVZxDDtZhdbMS7o6slMX6nQ9VOb0UQIV2MCtDjPwvI/WUVrA3w==", + "version": "7.40.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.40.1.tgz", + "integrity": "sha512-SLPVdi/0zVlp9obXk7/AqVwmvixrB4mLUXNb0Me/T9HXx95vEADrz5axADFMXBquivNGpPvxeQOTaSGAlHLThw==", "license": "SEE LICENSE IN LICENSE.txt", "dependencies": { "@hello-pangea/dnd": "18.0.1", diff --git a/assay/package.json b/assay/package.json index 7fa52aba408..c4d109111f1 100644 --- a/assay/package.json +++ b/assay/package.json @@ -55,7 +55,7 @@ } }, "dependencies": { - "@labkey/components": "7.40.0" + "@labkey/components": "7.40.1" }, "devDependencies": { "@labkey/build": "9.1.4", diff --git a/core/package-lock.json b/core/package-lock.json index 3e3f8f3d1e0..ae5e9a5b21f 100644 --- a/core/package-lock.json +++ b/core/package-lock.json @@ -8,7 +8,7 @@ "name": "labkey-core", "version": "0.0.0", "dependencies": { - "@labkey/components": "7.40.0", + "@labkey/components": "7.40.1", "@labkey/themes": "1.9.3" }, "devDependencies": { @@ -3824,9 +3824,9 @@ } }, "node_modules/@labkey/components": { - "version": "7.40.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.40.0.tgz", - "integrity": "sha512-yOBkfRaW6O+mWakUrKlcCFSI/UADW+eHqJPufVZxDDtZhdbMS7o6slMX6nQ9VOb0UQIV2MCtDjPwvI/WUVrA3w==", + "version": "7.40.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.40.1.tgz", + "integrity": "sha512-SLPVdi/0zVlp9obXk7/AqVwmvixrB4mLUXNb0Me/T9HXx95vEADrz5axADFMXBquivNGpPvxeQOTaSGAlHLThw==", "license": "SEE LICENSE IN LICENSE.txt", "dependencies": { "@hello-pangea/dnd": "18.0.1", diff --git a/core/package.json b/core/package.json index fed2642e0b4..6098e1a5c3b 100644 --- a/core/package.json +++ b/core/package.json @@ -57,7 +57,7 @@ } }, "dependencies": { - "@labkey/components": "7.40.0", + "@labkey/components": "7.40.1", "@labkey/themes": "1.9.3" }, "devDependencies": { diff --git a/core/src/client/AssayDesigner/AssayDesigner.tsx b/core/src/client/AssayDesigner/AssayDesigner.tsx index ec9b6acae32..9f866319365 100644 --- a/core/src/client/AssayDesigner/AssayDesigner.tsx +++ b/core/src/client/AssayDesigner/AssayDesigner.tsx @@ -136,8 +136,9 @@ class AssayDesigner 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 929d92f1691..9c83f7ae853 100644 --- a/core/src/client/DataClassDesigner/DataClassDesigner.tsx +++ b/core/src/client/DataClassDesigner/DataClassDesigner.tsx @@ -66,9 +66,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 82b271e3f80..f7504d327c2 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 c78eb51a691..0ff8aed6b54 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 3515cf9e0fa..4896391279a 100644 --- a/core/src/client/IssuesListDesigner/IssuesListDesigner.tsx +++ b/core/src/client/IssuesListDesigner/IssuesListDesigner.tsx @@ -82,9 +82,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 6b8e3fb3ac0..96ac31d955d 100644 --- a/core/src/client/ListDesigner/ListDesigner.tsx +++ b/core/src/client/ListDesigner/ListDesigner.tsx @@ -103,8 +103,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 54c4e69a667..9f70eb8376b 100644 --- a/core/src/client/SampleTypeDesigner/SampleTypeDesigner.tsx +++ b/core/src/client/SampleTypeDesigner/SampleTypeDesigner.tsx @@ -144,9 +144,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 ccdbe48e6fb..21007d4d610 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/core/src/org/labkey/core/analytics/AnalyticsServiceImpl.java b/core/src/org/labkey/core/analytics/AnalyticsServiceImpl.java index 2f823ce2c45..3e8ca5c2666 100644 --- a/core/src/org/labkey/core/analytics/AnalyticsServiceImpl.java +++ b/core/src/org/labkey/core/analytics/AnalyticsServiceImpl.java @@ -50,6 +50,7 @@ public class AnalyticsServiceImpl implements AnalyticsService { private static final String SEPARATOR = ","; private static final String GOOGLE_TAG_MANAGER_URL = "https://www.googletagmanager.com"; + private static final String GOOGLE_URL = "https://www.google.com"; private static final String ANALYTICS_CSP_KEY = AnalyticsServiceImpl.class.getName(); public static AnalyticsServiceImpl get() @@ -123,8 +124,9 @@ public void resetCSP() if (getTrackingStatus().contains(TrackingStatus.ga4FullUrl)) { - ContentSecurityPolicyFilter.registerAllowedSources(ANALYTICS_CSP_KEY, Directive.Connection, "https://*.googletagmanager.com", "https://*.google-analytics.com", "https://*.analytics.google.com"); - ContentSecurityPolicyFilter.registerAllowedSources(ANALYTICS_CSP_KEY, Directive.Image, "https://www.googletagmanager.com"); + // Per https://developers.google.com/tag-platform/security/guides/csp (plus other variants we have seen in the wild) + ContentSecurityPolicyFilter.registerAllowedSources(ANALYTICS_CSP_KEY, Directive.Connection, "https://*.googletagmanager.com", "https://*.google-analytics.com", "https://*.analytics.google.com", GOOGLE_URL); + ContentSecurityPolicyFilter.registerAllowedSources(ANALYTICS_CSP_KEY, Directive.Image, GOOGLE_TAG_MANAGER_URL); } } diff --git a/devtools/test/src/org/labkey/test/tests/devtools/AuthenticationProviderReorderTest.java b/devtools/test/src/org/labkey/test/tests/devtools/AuthenticationProviderReorderTest.java index fa06af5649d..c80d8d92888 100644 --- a/devtools/test/src/org/labkey/test/tests/devtools/AuthenticationProviderReorderTest.java +++ b/devtools/test/src/org/labkey/test/tests/devtools/AuthenticationProviderReorderTest.java @@ -66,7 +66,7 @@ public void testReorderConfigurations() signOut(); assertSsoLinkOrder(config_uncool, config_cool); - clickAndWait(Locator.button("Sign In")); + clickAndWait(Locator.linkWithText("Sign In")); assertSsoLinkOrder(config_uncool, config_cool); simpleSignIn(); @@ -79,7 +79,7 @@ public void testReorderConfigurations() signOut(); assertSsoLinkOrder(config_cool, config_uncool); - clickAndWait(Locator.button("Sign In")); + clickAndWait(Locator.linkWithText("Sign In")); assertSsoLinkOrder(config_cool, config_uncool); simpleSignIn(); } diff --git a/experiment/package-lock.json b/experiment/package-lock.json index a1c4f8f1cb4..e3fc56dc660 100644 --- a/experiment/package-lock.json +++ b/experiment/package-lock.json @@ -8,7 +8,7 @@ "name": "experiment", "version": "0.0.0", "dependencies": { - "@labkey/components": "7.40.0" + "@labkey/components": "7.40.1" }, "devDependencies": { "@labkey/build": "9.1.4", @@ -3659,9 +3659,9 @@ } }, "node_modules/@labkey/components": { - "version": "7.40.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.40.0.tgz", - "integrity": "sha512-yOBkfRaW6O+mWakUrKlcCFSI/UADW+eHqJPufVZxDDtZhdbMS7o6slMX6nQ9VOb0UQIV2MCtDjPwvI/WUVrA3w==", + "version": "7.40.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.40.1.tgz", + "integrity": "sha512-SLPVdi/0zVlp9obXk7/AqVwmvixrB4mLUXNb0Me/T9HXx95vEADrz5axADFMXBquivNGpPvxeQOTaSGAlHLThw==", "license": "SEE LICENSE IN LICENSE.txt", "dependencies": { "@hello-pangea/dnd": "18.0.1", diff --git a/experiment/package.json b/experiment/package.json index 3072fa53f40..78a0bd410a2 100644 --- a/experiment/package.json +++ b/experiment/package.json @@ -13,7 +13,7 @@ "test-integration": "cross-env NODE_ENV=test jest --ci --runInBand -c test/js/jest.config.integration.js" }, "dependencies": { - "@labkey/components": "7.40.0" + "@labkey/components": "7.40.1" }, "devDependencies": { "@labkey/build": "9.1.4", diff --git a/pipeline/package-lock.json b/pipeline/package-lock.json index d0d957c8395..6ca8952d036 100644 --- a/pipeline/package-lock.json +++ b/pipeline/package-lock.json @@ -8,7 +8,7 @@ "name": "pipeline", "version": "0.0.0", "dependencies": { - "@labkey/components": "7.40.0" + "@labkey/components": "7.40.1" }, "devDependencies": { "@labkey/build": "9.1.4", @@ -2914,9 +2914,9 @@ } }, "node_modules/@labkey/components": { - "version": "7.40.0", - "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.40.0.tgz", - "integrity": "sha512-yOBkfRaW6O+mWakUrKlcCFSI/UADW+eHqJPufVZxDDtZhdbMS7o6slMX6nQ9VOb0UQIV2MCtDjPwvI/WUVrA3w==", + "version": "7.40.1", + "resolved": "https://labkey.jfrog.io/artifactory/api/npm/libs-client/@labkey/components/-/@labkey/components-7.40.1.tgz", + "integrity": "sha512-SLPVdi/0zVlp9obXk7/AqVwmvixrB4mLUXNb0Me/T9HXx95vEADrz5axADFMXBquivNGpPvxeQOTaSGAlHLThw==", "license": "SEE LICENSE IN LICENSE.txt", "dependencies": { "@hello-pangea/dnd": "18.0.1", diff --git a/pipeline/package.json b/pipeline/package.json index 47ae736c4fb..5f235e99f74 100644 --- a/pipeline/package.json +++ b/pipeline/package.json @@ -14,7 +14,7 @@ "build-prod": "npm run clean && cross-env NODE_ENV=production PROD_SOURCE_MAP=source-map webpack --config node_modules/@labkey/build/webpack/prod.config.js --color --progress --profile" }, "dependencies": { - "@labkey/components": "7.40.0" + "@labkey/components": "7.40.1" }, "devDependencies": { "@labkey/build": "9.1.4", diff --git a/query/src/org/labkey/query/sql/QIfDefined.java b/query/src/org/labkey/query/sql/QIfDefined.java index a6c6e819519..8640cedf2c8 100644 --- a/query/src/org/labkey/query/sql/QIfDefined.java +++ b/query/src/org/labkey/query/sql/QIfDefined.java @@ -48,7 +48,6 @@ void setQuerySelect(QuerySelect select) @Override public FieldKey getFieldKey() { - assert null != select; return ((QExpr)getFirstChild()).getFieldKey(); } diff --git a/study/src/org/labkey/study/query/VisualizationVisitTagTable.java b/study/src/org/labkey/study/query/VisualizationVisitTagTable.java new file mode 100644 index 00000000000..e69de29bb2d 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 9b8883dd629..3ecc2ab0049 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; @@ -782,4 +783,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)); + } }