diff --git a/api/src/org/labkey/api/action/BaseApiAction.java b/api/src/org/labkey/api/action/BaseApiAction.java index d0944264647..516bbe61de1 100644 --- a/api/src/org/labkey/api/action/BaseApiAction.java +++ b/api/src/org/labkey/api/action/BaseApiAction.java @@ -92,13 +92,13 @@ private Marshaller findMarshaller() { Class superClass = getClass().getSuperclass(); if (null != superClass) - marshal = (Marshal) superClass.getAnnotation(Marshal.class); + marshal = superClass.getAnnotation(Marshal.class); } if (marshal == null) { Class declaringClass = getClass().getDeclaringClass(); if (declaringClass != null) - marshal = (Marshal)declaringClass.getAnnotation(Marshal.class); + marshal = declaringClass.getAnnotation(Marshal.class); } if (marshal != null) diff --git a/api/src/org/labkey/api/action/ConfirmAction.java b/api/src/org/labkey/api/action/ConfirmAction.java index 515a18598c5..afc2c83d738 100644 --- a/api/src/org/labkey/api/action/ConfirmAction.java +++ b/api/src/org/labkey/api/action/ConfirmAction.java @@ -77,7 +77,7 @@ public final ModelAndView handleRequest() throws Exception ModelAndView mv = getSuccessView(form); if (null != mv) return mv; - throw new RedirectException(getSuccessURL(form), false); + throw new RedirectException(getSuccessURL(form)); } } else diff --git a/api/src/org/labkey/api/action/FormViewAction.java b/api/src/org/labkey/api/action/FormViewAction.java index f464857d80d..fa34304a341 100644 --- a/api/src/org/labkey/api/action/FormViewAction.java +++ b/api/src/org/labkey/api/action/FormViewAction.java @@ -95,7 +95,7 @@ public ModelAndView handleRequest(FORM form, BindException errors) throws Except { URLHelper url = getSuccessURL(form); if (null != url) - return HttpView.redirect(url, false); + return HttpView.redirect(url); try (Timing ignored = MiniProfiler.step("createView")) { ModelAndView successView = getSuccessView(form); diff --git a/api/src/org/labkey/api/action/SimpleRedirectAction.java b/api/src/org/labkey/api/action/SimpleRedirectAction.java index e3fc053736d..3f992a7a637 100644 --- a/api/src/org/labkey/api/action/SimpleRedirectAction.java +++ b/api/src/org/labkey/api/action/SimpleRedirectAction.java @@ -15,10 +15,9 @@ */ package org.labkey.api.action; -import org.labkey.api.util.URLHelper; import org.labkey.api.view.ActionURL; -import org.labkey.api.view.HttpView; import org.labkey.api.view.NavTree; +import org.labkey.api.view.RedirectException; import org.springframework.validation.BindException; import org.springframework.web.servlet.ModelAndView; @@ -31,7 +30,7 @@ public abstract class SimpleRedirectAction
extends SimpleViewAction @Override public final ModelAndView getView(FORM form, BindException errors) throws Exception { - URLHelper url; + ActionURL url; try { @@ -45,7 +44,7 @@ public final ModelAndView getView(FORM form, BindException errors) throws Except if (null != getViewContext().getRequest().getHeader("template")) url.addParameter(ActionURL.Param._template.name(), getViewContext().getRequest().getHeader("template")); - return HttpView.redirect(url, url.isAllowableHost()); + throw new RedirectException(url); } @Override @@ -53,7 +52,7 @@ public final void addNavTrail(NavTree root) { } - public abstract URLHelper getRedirectURL(FORM form) throws Exception; + public abstract ActionURL getRedirectURL(FORM form) throws Exception; // Called whenever getRedirectURL() throws an exception. Standard code rethrows the exception. Override this // method to customize the handling of certain exceptions (e.g., display an error view). diff --git a/api/src/org/labkey/api/action/SpringActionController.java b/api/src/org/labkey/api/action/SpringActionController.java index 63e66778c92..6b178eb8553 100644 --- a/api/src/org/labkey/api/action/SpringActionController.java +++ b/api/src/org/labkey/api/action/SpringActionController.java @@ -79,7 +79,6 @@ import org.springframework.web.servlet.View; import org.springframework.web.servlet.ViewResolver; import org.springframework.web.servlet.mvc.Controller; -import org.springframework.web.servlet.view.RedirectView; import java.io.File; import java.io.IOException; @@ -439,9 +438,8 @@ public ModelAndView handleRequest(HttpServletRequest request, @NotNull HttpServl if (null != redirectURL) { - _log.debug("URL " + url + " was redirected to " + redirectURL + " instead"); - response.sendRedirect(redirectURL.toString()); - return null; + _log.debug("URL {} was redirected to {} instead", url, redirectURL); + throw new RedirectException(redirectURL); } // This container check used to be in checkPermissions(), but that meant actions with lenient permissions checking @@ -534,11 +532,6 @@ public ModelAndView handleRequest(HttpServletRequest request, @NotNull HttpServl ModelAndView mv = controller.handleRequest(request, response); if (mv != null) { - if (mv.getView() instanceof RedirectView) - { - // treat same as a throw redirect - throw new RedirectException(((RedirectView)mv.getView()).getUrl()); - } renderInTemplate(context, controller, pageConfig, mv); } } @@ -596,14 +589,14 @@ protected void handleException(Throwable x, ViewContext context, PageConfig page if (!"1".equals(getViewContext().getActionURL().getParameter("_retry_"))) { ActionURL url = getViewContext().cloneActionURL().addParameter("_retry_", "1"); - ExceptionUtil.doErrorRedirect(response, url.getLocalURIString()); + ExceptionUtil.unsafeRedirect(response, url.getLocalURIString()); return; } } ActionURL errorURL = ExceptionUtil.handleException(request, response, x, null, false, context, pageConfig); if (null != errorURL) - ExceptionUtil.doErrorRedirect(response, errorURL.toString()); + ExceptionUtil.unsafeRedirect(response, errorURL.toString()); } diff --git a/api/src/org/labkey/api/assay/actions/ReimportRedirectAction.java b/api/src/org/labkey/api/assay/actions/ReimportRedirectAction.java index 931d5a07781..a014074b6af 100644 --- a/api/src/org/labkey/api/assay/actions/ReimportRedirectAction.java +++ b/api/src/org/labkey/api/assay/actions/ReimportRedirectAction.java @@ -22,7 +22,6 @@ import org.labkey.api.exp.api.ExperimentService; import org.labkey.api.security.RequiresPermission; import org.labkey.api.security.permissions.InsertPermission; -import org.labkey.api.util.URLHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.NotFoundException; @@ -36,7 +35,7 @@ public class ReimportRedirectAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(ProtocolIdForm form) + public ActionURL getRedirectURL(ProtocolIdForm form) { Set selectedRunIds = DataRegionSelection.getSelected(getViewContext(), true); if (selectedRunIds.isEmpty()) diff --git a/api/src/org/labkey/api/reports/report/RedirectReport.java b/api/src/org/labkey/api/reports/report/RedirectReport.java index 9556c5a0eb7..6e9f469717a 100644 --- a/api/src/org/labkey/api/reports/report/RedirectReport.java +++ b/api/src/org/labkey/api/reports/report/RedirectReport.java @@ -15,7 +15,6 @@ */ package org.labkey.api.reports.report; -import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; import org.labkey.api.data.Container; @@ -24,17 +23,12 @@ import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.HttpView; import org.labkey.api.view.JspView; -import org.labkey.api.view.RedirectException; import org.labkey.api.view.ViewContext; import java.net.MalformedURLException; +import java.net.URISyntaxException; import java.net.URL; -/** - * User: migra - * Date: Mar 7, 2006 - * Time: 3:00:54 PM - */ public abstract class RedirectReport extends AbstractReport { private static final Logger LOG = LogHelper.getLogger(RedirectReport.class, "Reports that send the user to some other URL"); @@ -49,15 +43,17 @@ public RedirectReport() @Override public HttpView renderReport(ViewContext context) { - String url = getUrl(context.getContainer()); - // When rendering in a portal webpart, render the redirect link and thumbnail if (context.get(renderParam.reportWebPart.name()) != null) return new JspView<>("/org/labkey/api/reports/report/view/redirectReportWebPart.jsp", this); - throw new RedirectException(url); + redirect(context); + + return null; } + protected abstract void redirect(ViewContext context); + @Override public String getRunReportTarget() { @@ -85,11 +81,29 @@ public void setUrl(URLHelper url) getDescriptor().setProperty(REDIRECT_URL, redirectUrl); } + // TODO: Returning URLHelper from here would clean up a lot of messy code in these classes public @Nullable String getUrl(Container c) { return getDescriptor().getProperty(REDIRECT_URL); } + protected @Nullable URLHelper getURLHelper(ViewContext ctx) + { + String urlString = getUrl(ctx.getContainer()); + if (urlString != null) + { + try + { + return new URLHelper(urlString); + } + catch (URISyntaxException e) + { + LOG.warn("Bad URL in report {}: {}", getReportId(), urlString); + } + } + return null; + } + public @Nullable URL getURL() { String urlString = getUrl(null); @@ -111,20 +125,6 @@ public void setUrl(URLHelper url) } } - /** - * URL is not a LabKey local URL or a host local URL. - * - */ - public boolean isExternalLink() - { - String url = getUrl(null); - if (url == null) - return false; - - return (url.startsWith("http://") || url.startsWith("https://")) && - !isInternalLink(url) && !isLocalLink(url); - } - /** * URL has same hostname as this LabKey server, but is not under the LabKey webapp. * diff --git a/api/src/org/labkey/api/security/AuthFilter.java b/api/src/org/labkey/api/security/AuthFilter.java index 2dfe32c0eb8..aac2863e5c5 100644 --- a/api/src/org/labkey/api/security/AuthFilter.java +++ b/api/src/org/labkey/api/security/AuthFilter.java @@ -110,27 +110,6 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha return; } - // Versions of tomcat prior to 7.0.67 would automatically redirect if a URL with just a context path without - // a trailing slash was entered ie: http://www.labkey.org/labkey would automatically redirect to: http://www.labkey.org/labkey/ - // We need to handle it here otherwise the begin page redirect in index.html will fail: issue 25395 - - // getServletPath() will return an empty String when a URL with a context path but no trailing slash is requested - if (req.getServletPath().isEmpty()) - { - // now check if this is the case where the request URL contains only the context path - if (req.getContextPath() != null && req.getRequestURL().toString().endsWith(req.getContextPath())) - { - StringBuilder redirectURL = new StringBuilder(); - redirectURL.append(req.getRequestURL()).append("/"); - if (!StringUtils.isBlank(req.getQueryString())) - { - redirectURL.append("?").append(req.getQueryString()); - } - resp.sendRedirect(redirectURL.toString()); - return; - } - } - if (AppProps.getInstance().isSSLRequired()) { // No startup failure, so check for SSL redirection @@ -167,8 +146,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } url = new URL("https", url.getHost(), port, url.getFile()); // Use 301 redirect instead of a 302 to indicate it's a permanent move - resp.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY); - resp.setHeader("Location", resp.encodeRedirectURL(url.toString())); + ExceptionUtil.unsafeRedirect(resp, url.toString(), HttpServletResponse.SC_MOVED_PERMANENTLY); return; } else if (!AppProps.getInstance().isDevMode()) diff --git a/api/src/org/labkey/api/security/AuthenticationConfiguration.java b/api/src/org/labkey/api/security/AuthenticationConfiguration.java index eb9d8b322b9..2761e19885d 100644 --- a/api/src/org/labkey/api/security/AuthenticationConfiguration.java +++ b/api/src/org/labkey/api/security/AuthenticationConfiguration.java @@ -98,7 +98,7 @@ interface LoginFormAuthenticationConfiguration> extends PrimaryAuthenticationConfiguration { LinkFactory getLinkFactory(); - URLHelper getUrl(String secret, ViewContext ctx); + URLHelper getUrl(ViewContext ctx); /** * Allows an SSO auth configuration to specify that it should be used automatically instead of showing the standard diff --git a/api/src/org/labkey/api/security/AuthenticationManager.java b/api/src/org/labkey/api/security/AuthenticationManager.java index 4128ca435f5..e07254c6ff4 100644 --- a/api/src/org/labkey/api/security/AuthenticationManager.java +++ b/api/src/org/labkey/api/security/AuthenticationManager.java @@ -95,7 +95,7 @@ import org.labkey.api.util.URLHelper; import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; -import org.labkey.api.view.HttpView; +import org.labkey.api.view.ExternalRedirectException; import org.labkey.api.view.NavTree; import org.labkey.api.view.NotFoundException; import org.labkey.api.view.RedirectException; @@ -474,7 +474,7 @@ public ModelAndView getView(FORM form, BindException errors) throws Exception AuthenticationManager.setPrimaryAuthenticationResult(request, primaryResult); AuthenticationResult result = AuthenticationManager.handleAuthentication(request, getContainer()); - return HttpView.redirect(result.getRedirectURL(), true); + throw new ExternalRedirectException(result.getRedirectURL()); } primaryResult.getStatus().addUserErrorMessage(errors, primaryResult, null, null); diff --git a/api/src/org/labkey/api/util/ExceptionUtil.java b/api/src/org/labkey/api/util/ExceptionUtil.java index c9e4640717c..402eddb58b7 100644 --- a/api/src/org/labkey/api/util/ExceptionUtil.java +++ b/api/src/org/labkey/api/util/ExceptionUtil.java @@ -816,7 +816,7 @@ static ActionURL handleException(@NotNull HttpServletRequest request, @NotNull H if (ex instanceof RedirectException rex) { String url = rex.getURL(); - doErrorRedirect(response, url, rex.getHttpStatusCode()); + unsafeRedirect(response, url, rex.getHttpStatusCode()); return null; } @@ -1207,18 +1207,19 @@ private static void addDependenciesAndRender(int responseStatus, PageConfig page errorView.getView().render(errorView.getModel(), request, response); } - // Temporary redirect - public static void doErrorRedirect(HttpServletResponse response, String url) + // Temporary redirect. "Unsafe" in the sense that the URL is not checked against the admin-configured allow list. + // Callers are responsible for ensuring any external redirect is safe. + public static void unsafeRedirect(HttpServletResponse response, String url) { - doErrorRedirect(response, url, HttpServletResponse.SC_MOVED_TEMPORARILY); + unsafeRedirect(response, url, HttpServletResponse.SC_MOVED_TEMPORARILY); } // Pass in HTTP status code to designate temporary vs. permanent redirect - private static void doErrorRedirect(HttpServletResponse response, String url, int httpStatusCode) + public static void unsafeRedirect(HttpServletResponse response, String url, int httpStatusCode) { response.setStatus(httpStatusCode); response.setDateHeader("Expires", 0); - response.setHeader("Location", url); + response.setHeader("Location", response.encodeRedirectURL(url)); response.setContentType("text/html; charset=UTF-8"); // backup strategy! @@ -1234,7 +1235,7 @@ private static void doErrorRedirect(HttpServletResponse response, String url, in } catch (IOException x) { - LOG.error("doErrorRedirect", x); + LOG.error("unsafeRedirect", x); } } @@ -1587,7 +1588,7 @@ public String encodeURL(String s) @Override public String encodeRedirectURL(String s) { - throw new IllegalStateException(); + return s; } @Override diff --git a/api/src/org/labkey/api/util/URLHelper.java b/api/src/org/labkey/api/util/URLHelper.java index cff34c4d2be..2eeadcfb3b5 100644 --- a/api/src/org/labkey/api/util/URLHelper.java +++ b/api/src/org/labkey/api/util/URLHelper.java @@ -18,12 +18,15 @@ import com.fasterxml.jackson.annotation.JsonIgnore; import com.fasterxml.jackson.annotation.JsonValue; +import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.beanutils.ConversionException; import org.apache.commons.collections4.MultiValuedMap; import org.apache.commons.collections4.multimap.ArrayListValuedHashMap; import org.apache.commons.lang3.StringUtils; +import org.apache.commons.lang3.Strings; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.jetbrains.annotations.NotNull; import org.json.JSONObject; import org.json.JSONString; import org.junit.Assert; @@ -38,13 +41,10 @@ import org.springframework.beans.PropertyValue; import org.springframework.beans.PropertyValues; -import jakarta.servlet.http.HttpServletRequest; import java.io.Serializable; import java.lang.reflect.Array; -import java.net.MalformedURLException; import java.net.URI; import java.net.URISyntaxException; -import java.net.URL; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; @@ -56,6 +56,7 @@ import java.util.List; import java.util.Map; import java.util.TreeMap; +import java.util.function.Supplier; /** * Represents a URL, typically within this instance of LabKey Server. @@ -128,7 +129,7 @@ protected void parse(String url) throws URISyntaxException } if (-1 != p.indexOf(' ')) - p = StringUtils.replace(p, " ", "%20"); + p = Strings.CS.replace(p, " ", "%20"); if (!StringUtils.isEmpty(p)) _setURI(new URI(p)); assert _fragment == null : "The call to _setURI unexpectedly resulted in a fragment being set" ; @@ -907,41 +908,64 @@ public static String staticResourceUrl(String resourcePath) } } - public boolean isConfiguredExternalHost() + // Parameterized to allow unit testing. Caller has trimmed host and checked for null. + private static boolean isAllowedExternalHost(@NotNull String host, boolean devMode, Supplier> allowedHostsSupplier) { - String requestedHost = this.getHost(); - if (StringUtils.trimToNull(requestedHost) != null ) - return AppProps.getInstance().getExternalRedirectHosts().stream().anyMatch(requestedHost::equalsIgnoreCase); + // Allow 'localhost' for servers in dev mode + if (devMode && "localhost".equalsIgnoreCase(host)) + return true; - return false; + // Count the dots in case there's a wild card + int dotCount = StringUtils.countMatches(host, '.'); + + return allowedHostsSupplier.get().stream() + .anyMatch(allowedHost -> isMatch(host, dotCount, allowedHost)); + } + + private static boolean isMatch(@NotNull String host, int dotCount, @NotNull String allowedHost) + { + final boolean ret; + + if (allowedHost.startsWith("*.")) + { + // This wild-card pattern matches the host if 1) they have the same number of dots and 2) the host ends with + // the portion of the pattern after the wild card. + int expectedDotCount = StringUtils.countMatches(allowedHost, '.'); + ret = (dotCount == expectedDotCount && Strings.CI.endsWith(host, allowedHost.substring(2))); + } + else + { + // Non-wild-card pattern must match the entire host + ret = Strings.CI.equals(host, allowedHost); + } + + return ret; } // Issue 35896 - Disallow external redirects to URLs not on the allowlist public boolean isAllowableHost() { - String host = StringUtils.trimToNull(this.getHost()); + String host = StringUtils.trimToNull(getHost()); // We have a returnUrl that includes a server host name if (host != null) { // Check if it matches the current server's preferred host name, per the base server URL setting - String allowedHost = null; + String thisHost = null; try { - allowedHost = new URL(AppProps.getInstance().getBaseServerUrl()).getHost(); + thisHost = new URI(AppProps.getInstance().getBaseServerUrl()).getHost(); } - catch (MalformedURLException ignored) {} - - if (!host.equalsIgnoreCase(allowedHost)) + catch (URISyntaxException _) { - // Server host name that doesn't match, log and possibly reject based on config - // Allow 'localhost' for servers in dev mode - boolean isConfigured = AppProps.getInstance().isDevMode() && "localhost".equalsIgnoreCase(host); - isConfigured |= this.isConfiguredExternalHost(); + } - if (!isConfigured) + if (!host.equalsIgnoreCase(thisHost)) + { + // Server host name doesn't match base server URL; log and possibly reject based on config. + if (!isAllowedExternalHost(host, AppProps.getInstance().isDevMode(), ()->AppProps.getInstance().getExternalRedirectHosts())) { - String logMessageDetails = "returnUrl value: " + this; + String logMessageDetails = "returnUrl value: " + getURIString(true); HttpServletRequest request = HttpView.currentRequest(); if (request != null) { @@ -952,13 +976,12 @@ public boolean isAllowableHost() } } - LOG.warn("Rejected external host redirect " + logMessageDetails + - "\nPlease configure external redirect url host from: Admin gear --> Site --> Admin Console --> Settings --> External Redirect Hosts"); + LOG.warn("Rejected external host redirect {}\nIf this is a legitimate host that the server should redirect users to, please configure an external redirect host via: Admin gear --> Site --> Admin Console --> Settings --> Allowed External Redirect Hosts", logMessageDetails); return false; } else { - LOG.debug("Detected configured external host returnUrl: " + this); + LOG.debug("Detected configured external host returnUrl: {}", this); } } } @@ -1062,5 +1085,66 @@ public void testPartials() throws Exception expect("/","/"); expect("/home/project-begin.view","/home/project-begin.view"); } + + @Test + public void testAllowedHosts() + { + // localhost + assertTrue(isAllowedExternalHost("localhost", true, List::of)); + assertFalse(isAllowedExternalHost("localhost", false, List::of)); + assertTrue(isAllowedExternalHost("localhost", true, ()->List.of("localhost"))); + assertTrue(isAllowedExternalHost("localhost", true, ()->List.of("localhost"))); + + // simple hosts + List allowedHosts = List.of( + "www.labkey.org", + "www.labkey.com", + "google.com" + ); + + // good + assertTrue(isAllowedExternalHost("localhost", true, ()->allowedHosts)); + assertTrue(isAllowedExternalHost("www.labkey.org", true, ()->allowedHosts)); + assertTrue(isAllowedExternalHost("www.labkey.com", true, ()->allowedHosts)); + assertTrue(isAllowedExternalHost("google.com", true, ()->allowedHosts)); + + // bad + assertFalse(isAllowedExternalHost("labkey.org", true, ()->allowedHosts)); + assertFalse(isAllowedExternalHost("labkey.com", true, ()->allowedHosts)); + assertFalse(isAllowedExternalHost("sub.labkey.com", true, ()->allowedHosts)); + assertFalse(isAllowedExternalHost("sub.www.labkey.com", true, ()->allowedHosts)); + assertFalse(isAllowedExternalHost("www.google.com", true, ()->allowedHosts)); + + // add one more + List allowedHosts2 = new ArrayList<>(allowedHosts); + allowedHosts2.add("www.google.com"); + assertTrue(isAllowedExternalHost("www.google.com", true, ()->allowedHosts2)); + + // test wild cards + List wildCardHosts = List.of( + "*.labkey.com", + "labkey.com", + "*.lkpoc.labkey.com", + "*.trial.labkey.host", + "*.google.com" + ); + + // good + assertTrue(isAllowedExternalHost("www.labkey.com", true, ()->wildCardHosts)); + assertTrue(isAllowedExternalHost("lkpoc.labkey.com", true, ()->wildCardHosts)); + assertTrue(isAllowedExternalHost("sub1.labkey.com", true, ()->wildCardHosts)); + assertTrue(isAllowedExternalHost("sub2.labkey.com", true, ()->wildCardHosts)); + assertTrue(isAllowedExternalHost("labkey.com", true, ()->wildCardHosts)); + assertTrue(isAllowedExternalHost("sub1.lkpoc.labkey.com", true, ()->wildCardHosts)); + assertTrue(isAllowedExternalHost("sub2.lkpoc.labkey.com", true, ()->wildCardHosts)); + assertTrue(isAllowedExternalHost("sub1.trial.labkey.host", true, ()->wildCardHosts)); + assertTrue(isAllowedExternalHost("sub2.trial.labkey.host", true, ()->wildCardHosts)); + + // bad + assertFalse(isAllowedExternalHost("trial.labkey.host", true, ()->wildCardHosts)); + assertFalse(isAllowedExternalHost("sub1.sub2.lkpoc.labkey.com", true, ()->wildCardHosts)); + assertFalse(isAllowedExternalHost("google.com", true, ()->wildCardHosts)); + assertFalse(isAllowedExternalHost("sub1.sub2.labkey.com", true, ()->wildCardHosts)); + } } } diff --git a/api/src/org/labkey/api/view/ExternalRedirectException.java b/api/src/org/labkey/api/view/ExternalRedirectException.java new file mode 100644 index 00000000000..e185562050f --- /dev/null +++ b/api/src/org/labkey/api/view/ExternalRedirectException.java @@ -0,0 +1,17 @@ +package org.labkey.api.view; + +import org.jetbrains.annotations.NotNull; +import org.labkey.api.util.URLHelper; + +/** + * Can redirect externally, but only if it's an absolute url and the host is on the admin-configured allow list. + * External redirects are rarely needed; see RedirectException. + */ +public class ExternalRedirectException extends RedirectException +{ + public ExternalRedirectException(@NotNull URLHelper url) + { + boolean redirectExternally = !url.isLocalUri(HttpView.getRootContext()) && url.isAllowableHost(); + super(redirectExternally ? url.getURIString() : url.getLocalURIString()); + } +} diff --git a/api/src/org/labkey/api/view/HttpView.java b/api/src/org/labkey/api/view/HttpView.java index 6a1d966bc2e..89fe20f5c07 100644 --- a/api/src/org/labkey/api/view/HttpView.java +++ b/api/src/org/labkey/api/view/HttpView.java @@ -635,11 +635,13 @@ protected void renderInternal(Object model, PrintWriter out) throws IOException } } - public static HttpView redirect(URLHelper url, boolean allowAbsoluteUrl) + // Never redirects externally + public static HttpView redirect(URLHelper url) { - throw new RedirectException(url, allowAbsoluteUrl); + throw new RedirectException(url); } + // Never redirects externally public static HttpView redirect(ActionURL url) { throw new RedirectException(url); diff --git a/api/src/org/labkey/api/view/PermanentRedirectException.java b/api/src/org/labkey/api/view/PermanentRedirectException.java index 8a57ee905c6..51d6c7e2bef 100644 --- a/api/src/org/labkey/api/view/PermanentRedirectException.java +++ b/api/src/org/labkey/api/view/PermanentRedirectException.java @@ -10,7 +10,7 @@ public class PermanentRedirectException extends RedirectException { public PermanentRedirectException(@NotNull URLHelper url) { - super(url, false); + super(url); } @Override diff --git a/api/src/org/labkey/api/view/RedirectException.java b/api/src/org/labkey/api/view/RedirectException.java index 1c8040defec..6d832af899a 100644 --- a/api/src/org/labkey/api/view/RedirectException.java +++ b/api/src/org/labkey/api/view/RedirectException.java @@ -15,37 +15,32 @@ */ package org.labkey.api.view; +import jakarta.servlet.http.HttpServletResponse; import org.jetbrains.annotations.NotNull; import org.labkey.api.util.SkipMothershipLogging; import org.labkey.api.util.URLHelper; -import jakarta.servlet.http.HttpServletResponse; - /** * When thrown in the context of an HTTP request, sends the client a *temporary* redirect in the HTTP response. Not * treated as a loggable error. See {@link PermanentRedirectException} if a permanent redirect is desired. + * Note: This always redirects to the local server. If an external redirect is needed (this is rare), use + * {@link ExternalRedirectException} or (even rarer) {@link UnsafeExternalRedirectException}. */ public class RedirectException extends RuntimeException implements SkipMothershipLogging { private final String _url; - @Deprecated // Call the variant that takes allowAbsoluteUrl - public RedirectException(@NotNull URLHelper url) - { - this(url, false); - } - - public RedirectException(@NotNull URLHelper url, boolean allowAbsoluteUrl) + public RedirectException(@NotNull ActionURL url) { - this(!allowAbsoluteUrl || url.isLocalUri(HttpView.getRootContext()) ? url.getLocalURIString() : url.getURIString()); + this(url.getLocalURIString()); } - public RedirectException(@NotNull ActionURL url) + public RedirectException(@NotNull URLHelper url) { this(url.getLocalURIString()); } - public RedirectException(String url) + protected RedirectException(String url) { _url = url; } diff --git a/api/src/org/labkey/api/view/RedirectorServlet.java b/api/src/org/labkey/api/view/RedirectorServlet.java index 1e9afa8b19c..7daa68647d1 100644 --- a/api/src/org/labkey/api/view/RedirectorServlet.java +++ b/api/src/org/labkey/api/view/RedirectorServlet.java @@ -7,6 +7,7 @@ import jakarta.servlet.http.HttpServlet; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; +import org.labkey.api.util.ExceptionUtil; /** Simple redirector to redirect and forward from legacy context paths to the root context */ public class RedirectorServlet extends HttpServlet @@ -40,8 +41,7 @@ protected void service(HttpServletRequest request, HttpServletResponse response) originalUrl.getPort(), originalUrl.getFile().replaceFirst(_legacyContextPath, "")); - response.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY); - response.setHeader("Location", redirectUrl.toString()); + ExceptionUtil.unsafeRedirect(response, redirectUrl.toString(), HttpServletResponse.SC_MOVED_PERMANENTLY); } else { diff --git a/api/src/org/labkey/api/view/UnsafeExternalRedirectException.java b/api/src/org/labkey/api/view/UnsafeExternalRedirectException.java new file mode 100644 index 00000000000..bc35186e180 --- /dev/null +++ b/api/src/org/labkey/api/view/UnsafeExternalRedirectException.java @@ -0,0 +1,20 @@ +package org.labkey.api.view; + +import org.jetbrains.annotations.NotNull; +import org.labkey.api.util.URLHelper; + +/** + * Very rarely needed, throwing this exception redirects to an external site WITHOUT checking its host name against the + * admin-configured allow list. Use this only in cases where you are certain the URL is safe for external redirection, + * for example, a trusted user has configured a URL that is used to redirect. (Obviously, a URL coming from a returnUrl + * or other parameter is NOT safe and should use one of the other redirect exceptions.) If using this class, add a + * comment explaining why bypassing the allow list is safe. + */ +public class UnsafeExternalRedirectException extends RedirectException +{ + public UnsafeExternalRedirectException(@NotNull URLHelper url) + { + boolean redirectExternally = !url.isLocalUri(HttpView.getRootContext()); + super(redirectExternally ? url.getURIString() : url.getLocalURIString()); + } +} diff --git a/assay/src/org/labkey/assay/PlateController.java b/assay/src/org/labkey/assay/PlateController.java index 7bffbc4aeac..e33db16cdd4 100644 --- a/assay/src/org/labkey/assay/PlateController.java +++ b/assay/src/org/labkey/assay/PlateController.java @@ -125,7 +125,7 @@ public ActionURL getPlateDetailsURL(Container c) public static class BeginAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(Object o) + public ActionURL getRedirectURL(Object o) { return new ActionURL(PlateListAction.class, getContainer()); } @@ -198,7 +198,7 @@ public void setRowId(int rowId) public static class PlateDetailsAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(RowIdForm form) + public ActionURL getRedirectURL(RowIdForm form) { Plate plate = PlateManager.get().getPlate(getContainer(), form.getRowId()); if (plate == null) diff --git a/audit/src/org/labkey/audit/AuditController.java b/audit/src/org/labkey/audit/AuditController.java index a0b6f13a460..e44599c0749 100644 --- a/audit/src/org/labkey/audit/AuditController.java +++ b/audit/src/org/labkey/audit/AuditController.java @@ -53,7 +53,6 @@ import org.labkey.api.settings.AdminConsole; import org.labkey.api.settings.LookAndFeelProperties; import org.labkey.api.util.DateUtil; -import org.labkey.api.util.URLHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.HttpView; import org.labkey.api.view.JspView; @@ -109,7 +108,7 @@ public ActionURL getAuditLog(Container container, String eventType, @Nullable Da public class BeginAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(Object o) + public ActionURL getRedirectURL(Object o) { if (getContainer() != null && getContainer().isRoot()) return new ActionURL(ShowAuditLogAction.class, getContainer()); diff --git a/core/src/org/labkey/core/admin/AdminController.java b/core/src/org/labkey/core/admin/AdminController.java index 15de341b8ae..aa0688f7789 100644 --- a/core/src/org/labkey/core/admin/AdminController.java +++ b/core/src/org/labkey/core/admin/AdminController.java @@ -8249,7 +8249,7 @@ public void validateCommand(ReturnUrlForm target, Errors errors) } @Override - public @Nullable URLHelper getRedirectURL(ReturnUrlForm form) throws Exception + public @Nullable ActionURL getRedirectURL(ReturnUrlForm form) throws Exception { Set ids = DataRegionSelection.getSelected(getViewContext(), true); diff --git a/core/src/org/labkey/core/admin/AllowListType.java b/core/src/org/labkey/core/admin/AllowListType.java index 895196846d8..f2ebc545c3a 100644 --- a/core/src/org/labkey/core/admin/AllowListType.java +++ b/core/src/org/labkey/core/admin/AllowListType.java @@ -33,15 +33,17 @@ public HtmlString getDescription() return HtmlString.unsafe("""

- For security reasons, LabKey Server restricts the host names that can be used in returnUrl parameters. + For security reasons, external redirects are restricted to the host names in this list. By default, only redirects to the same LabKey instance are allowed. - Other server host names must be configured below to allow them to be automatically redirected. + Other server host names must be configured below to allow redirects to them. For more information on the security concern, please refer to the - OWASP cheat sheet. + OWASP cheat sheet.

- Add allowed hosts based on the server name or IP address, as they will be referenced in returnUrl values. - For example: www.myexternalhost.com or 1.2.3.4 + Add allowed hosts based on the server name or IP address, as they will be referenced in parameters + such as returnUrl. An asterisk (*) follow by a dot acts as a wild card that matches any leading + subdomain for that host. + Examples: www.myexternalhost.com, *.myexternalhost.com, or 1.2.3.4

"""); @@ -69,9 +71,32 @@ public void validateValueFormat(String host, BindException errors) { errors.addError(new LabKeyError("Redirect host name must not be blank.")); } - else if (!AUTHORITY_VALIDATOR.isValidAuthority(host)) + else { - errors.addError(new LabKeyError(String.format("Redirect host name %1$s is not formatted correctly", host))); + if (AUTHORITY_VALIDATOR.isValidAuthority(host)) + { + // Validate wild card patterns + int starCount = StringUtils.countMatches(host, '*'); + if (starCount > 0) + { + if (starCount > 1) + { + errors.addError(new LabKeyError(String.format("Redirect host name %1$s has multiple wild card characters", host))); + } + else if (!host.startsWith("*.")) + { + errors.addError(new LabKeyError(String.format("Redirect host name %1$s has an invalid wild card. The pattern must start with \"*.\".", host))); + } + else if (StringUtils.countMatches(host, '.') < 2) + { + errors.addError(new LabKeyError(String.format("Redirect host name %1$s with wild card is too short. The pattern must include at least two dots.", host))); + } + } + } + else + { + errors.addError(new LabKeyError(String.format("Redirect host name %1$s is not a valid host name", host))); + } } } diff --git a/core/src/org/labkey/core/login/LoginController.java b/core/src/org/labkey/core/login/LoginController.java index fe5650b8808..b47c07af6a7 100644 --- a/core/src/org/labkey/core/login/LoginController.java +++ b/core/src/org/labkey/core/login/LoginController.java @@ -103,6 +103,7 @@ import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; import org.labkey.api.view.BadRequestException; +import org.labkey.api.view.ExternalRedirectException; import org.labkey.api.view.HtmlView; import org.labkey.api.view.HttpView; import org.labkey.api.view.JspView; @@ -110,6 +111,7 @@ import org.labkey.api.view.NavTree; import org.labkey.api.view.NotFoundException; import org.labkey.api.view.RedirectException; +import org.labkey.api.view.UnsafeExternalRedirectException; import org.labkey.api.view.VBox; import org.labkey.api.view.WebPartView; import org.labkey.api.view.template.PageConfig; @@ -121,7 +123,6 @@ import org.springframework.web.servlet.ModelAndView; import org.springframework.web.servlet.mvc.Controller; -import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.Comparator; @@ -586,7 +587,7 @@ private ModelAndView redirectIfLoggedIn(AbstractLoginForm form) LoginReturnProperties properties = null != returnUrl || form.getSkipProfile() ? new LoginReturnProperties(returnUrl, form.getUrlhash(), form.getSkipProfile()) : null; - return HttpView.redirect(AuthenticationManager.getAfterLoginURL(getContainer(), properties, getUser()), true); + throw new ExternalRedirectException(AuthenticationManager.getAfterLoginURL(getContainer(), properties, getUser())); } return null; } @@ -1431,15 +1432,9 @@ public URLHelper getSuccessURL(ReturnUrlForm form) { if (null != _redirectURL) { - try - { - getViewContext().getResponse().sendRedirect(_redirectURL.getURIString()); - return null; - } - catch (IOException e) - { - throw new RuntimeException(e); - } + // It's safe to bypass checking the external redirect allow list in this case because we are redirecting + // to the administrator-provided URL from the SSO authentication configuration (e.g., CAS logout). + throw new UnsafeExternalRedirectException(_redirectURL); } return form.getReturnUrlHelper(AuthenticationManager.getWelcomeURL()); } @@ -1562,19 +1557,18 @@ public ModelAndView getView(SsoRedirectForm form, BindException errors) AuthenticationManager.setLoginReturnProperties(getViewContext().getRequest(), properties); } - String csrf = CSRFUtil.getExpectedToken(getViewContext()); - final URLHelper url; int rowId = form.getConfiguration(); - SSOAuthenticationConfiguration configuration = AuthenticationManager.getActiveSSOConfiguration(rowId); if (null == configuration) throw new NotFoundException("Authentication configuration is not valid"); - url = configuration.getUrl(csrf, getViewContext()); + url = configuration.getUrl(getViewContext()); - return HttpView.redirect(url, true); + // It's safe to bypass checking the external redirect allow list in this case because we are redirecting to + // the administrator-provided URL from the SSO authentication configuration. + throw new UnsafeExternalRedirectException(url); } @Override @@ -1777,7 +1771,7 @@ public ModelAndView getSuccessView(SetPasswordForm form) _successUrl = AppProps.getInstance().getHomePageActionURL(); // Issue 33599: allow the returnUrl for this action to redirect to an absolute URL (ex. labkey.org back to accounts.trial.labkey.host) - return HttpView.redirect(_successUrl, true); + throw new ExternalRedirectException(_successUrl); } } diff --git a/core/src/org/labkey/core/login/termsOfUse.jsp b/core/src/org/labkey/core/login/termsOfUse.jsp index 8cfa7bb0445..5face19abc0 100644 --- a/core/src/org/labkey/core/login/termsOfUse.jsp +++ b/core/src/org/labkey/core/login/termsOfUse.jsp @@ -42,7 +42,7 @@ // Redirect immediately if terms are blank or null if (HtmlString.isBlank(termsHtml)) - throw new RedirectException(returnUrl, false); + throw new RedirectException(returnUrl); ActionURL formURL = urlFor(AgreeToTermsAction.class); %> diff --git a/core/src/org/labkey/core/portal/ProjectController.java b/core/src/org/labkey/core/portal/ProjectController.java index ff8155a00e9..e71a32e1e61 100644 --- a/core/src/org/labkey/core/portal/ProjectController.java +++ b/core/src/org/labkey/core/portal/ProjectController.java @@ -462,7 +462,7 @@ public void addNavTrail(NavTree root) public static class FileBrowserAction extends org.labkey.api.action.SimpleRedirectAction { @Override - public URLHelper getRedirectURL(Object o) + public ActionURL getRedirectURL(Object o) { String p = StringUtils.trimToEmpty(getViewContext().getRequest().getParameter("path")); Path path; @@ -635,7 +635,7 @@ public ModelAndView getView(AddWebPartForm form, boolean reshow, BindException e { URLHelper successURL = getSuccessURL(form); if (null != successURL) - return HttpView.redirect(successURL, false); + return HttpView.redirect(successURL); return HttpView.redirect(getContainer().getStartURL(getUser())); } @@ -915,7 +915,7 @@ public ModelAndView getView(CustomizePortletForm customizePortletForm, boolean r handlePost(customizePortletForm, errors); URLHelper successURL = getSuccessURL(customizePortletForm); if (null != successURL) - return HttpView.redirect(successURL, false); + return HttpView.redirect(successURL); return HttpView.redirect(getContainer().getStartURL(getUser())); } diff --git a/core/src/org/labkey/core/query/ShortUrlTableInfo.java b/core/src/org/labkey/core/query/ShortUrlTableInfo.java index fb18037b356..aa084209ca2 100644 --- a/core/src/org/labkey/core/query/ShortUrlTableInfo.java +++ b/core/src/org/labkey/core/query/ShortUrlTableInfo.java @@ -19,7 +19,6 @@ import org.labkey.api.query.ValidationException; import org.labkey.api.security.User; import org.labkey.api.security.UserPrincipal; -import org.labkey.api.security.permissions.ApplicationAdminPermission; import org.labkey.api.security.permissions.Permission; import org.labkey.api.security.permissions.ReadPermission; import org.labkey.api.security.permissions.TroubleshooterPermission; diff --git a/core/src/org/labkey/core/security/SecurityController.java b/core/src/org/labkey/core/security/SecurityController.java index a6ee08fece4..6e74a24b6c3 100644 --- a/core/src/org/labkey/core/security/SecurityController.java +++ b/core/src/org/labkey/core/security/SecurityController.java @@ -398,7 +398,7 @@ private static void ensureGroupInContainer(String group, Container c) public class BeginAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(Object o) + public ActionURL getRedirectURL(Object o) { if (null == getContainer() || getContainer().isRoot()) { diff --git a/core/src/org/labkey/core/thumbnail/ThumbnailCache.java b/core/src/org/labkey/core/thumbnail/ThumbnailCache.java index 46ebc30662e..e0d656bfc23 100644 --- a/core/src/org/labkey/core/thumbnail/ThumbnailCache.java +++ b/core/src/org/labkey/core/thumbnail/ThumbnailCache.java @@ -54,7 +54,7 @@ public static CacheableWriter getThumbnailWriter(ThumbnailProvider provider, Ima URLHelper redirectURL = ThumbnailUtil.getStaticThumbnailURL(provider, imageType); if (null != redirectURL) - throw new RedirectException(redirectURL, false); + throw new RedirectException(redirectURL); } return dynamic; diff --git a/core/src/org/labkey/core/user/UserController.java b/core/src/org/labkey/core/user/UserController.java index 25632fad932..b936f108410 100644 --- a/core/src/org/labkey/core/user/UserController.java +++ b/core/src/org/labkey/core/user/UserController.java @@ -944,7 +944,7 @@ public void addNavTrail(NavTree root) public static class ShowUserPreferencesAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(Object form) + public ActionURL getRedirectURL(Object form) { String domainURI = UsersDomainKind.getDomainURI(CoreQuerySchema.NAME, CoreQuerySchema.USERS_TABLE_NAME, UsersDomainKind.getDomainContainer(), getUser()); Domain domain = PropertyService.get().getDomain(UsersDomainKind.getDomainContainer(), domainURI); diff --git a/core/src/org/labkey/core/view/ShortURLFilter.java b/core/src/org/labkey/core/view/ShortURLFilter.java index 15f4124dfcb..e3425179409 100644 --- a/core/src/org/labkey/core/view/ShortURLFilter.java +++ b/core/src/org/labkey/core/view/ShortURLFilter.java @@ -15,6 +15,7 @@ */ package org.labkey.core.view; +import org.labkey.api.util.ExceptionUtil; import org.labkey.api.view.ShortURLRecord; import org.labkey.api.view.ShortURLService; @@ -56,7 +57,7 @@ public void doFilter(ServletRequest req, ServletResponse resp, FilterChain chain if (fullURL != null) { // We found a match, do a redirect and bail out - response.sendRedirect(fullURL.getFullURL()); + ExceptionUtil.unsafeRedirect(response, fullURL.getFullURL()); } else { diff --git a/core/src/org/labkey/core/webdav/DavController.java b/core/src/org/labkey/core/webdav/DavController.java index 6c680148437..9909eb546a8 100644 --- a/core/src/org/labkey/core/webdav/DavController.java +++ b/core/src/org/labkey/core/webdav/DavController.java @@ -146,7 +146,6 @@ import javax.xml.parsers.DocumentBuilder; import javax.xml.parsers.ParserConfigurationException; import javax.xml.parsers.SAXParser; -import javax.xml.parsers.SAXParserFactory; import java.io.BufferedInputStream; import java.io.BufferedWriter; import java.io.ByteArrayInputStream; @@ -361,7 +360,7 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons } catch (RedirectException ex) { - ExceptionUtil.doErrorRedirect(response, ex.getURL()); + ExceptionUtil.unsafeRedirect(response, ex.getURL()); } catch (Exception ex) { @@ -630,12 +629,6 @@ void setRealm(String realm) super.setHeader("WWW-Authenticate", "Basic realm=\"" + realm + "\""); } - void setLocation(String value) - { - super.setHeader("Location", value); - } - - StringBuilder sbLogResponse = new StringBuilder(); @Override @@ -780,8 +773,7 @@ public ModelAndView handleRequest(HttpServletRequest request, HttpServletRespons } else if ("GET".equals(method) && HttpUtil.isBrowser(getRequest())) { - getResponse().setStatus(WebdavStatus.SC_MOVED_TEMPORARILY); - getResponse().setLocation(getLoginURL().getEncodedLocalURIString()); + ExceptionUtil.unsafeRedirect(getResponse(), getLoginURL().getLocalURIString()); } else { @@ -1274,8 +1266,15 @@ public WebdavStatus doMethod() throws DavException, IOException, RedirectExcepti String returnUrl = getRequest().getParameter(ActionURL.Param.returnUrl.name()); if (null != StringUtils.trimToNull(returnUrl)) { - String url = returnUrl + (returnUrl.indexOf('?')==-1 ? '?' : '&') + "status=" + status; - throw new RedirectException(url); + try + { + URLHelper url = new URLHelper(returnUrl + (returnUrl.indexOf('?')==-1 ? '?' : '&') + "status=" + status); + throw new RedirectException(url); + } + catch (URISyntaxException e) + { + throw new RuntimeException(e); + } } if (status == WebdavStatus.SC_CREATED) @@ -4996,8 +4995,7 @@ private WebdavStatus serveResource(WebdavResource resource, boolean content) if (req != null) { String uri = req.getEndpoint().toASCIIString(); - getResponse().setStatus(WebdavStatus.SC_MOVED_TEMPORARILY); - getResponse().setLocation(uri); + ExceptionUtil.unsafeRedirect(getResponse(), uri); return null; } } diff --git a/devtools/src/org/labkey/devtools/authentication/TestSecondaryController.java b/devtools/src/org/labkey/devtools/authentication/TestSecondaryController.java index 798a47e73ed..bbe5fc28c68 100644 --- a/devtools/src/org/labkey/devtools/authentication/TestSecondaryController.java +++ b/devtools/src/org/labkey/devtools/authentication/TestSecondaryController.java @@ -109,7 +109,7 @@ public void validateCommand(TestSecondaryForm form, Errors errors) public ModelAndView getView(TestSecondaryForm form, boolean reshow, BindException errors) { if (!getUser().isGuest()) - return HttpView.redirect(AuthenticationManager.getAfterLoginURL(getContainer(), null, getUser()), false); + return HttpView.redirect(AuthenticationManager.getAfterLoginURL(getContainer(), null, getUser())); PrimaryAuthenticationResult result = AuthenticationManager.getPrimaryAuthenticationResult(getViewContext().getSession()); diff --git a/devtools/src/org/labkey/devtools/authentication/TestSsoConfiguration.java b/devtools/src/org/labkey/devtools/authentication/TestSsoConfiguration.java index c298c4efac8..7a6c8ab5b6c 100644 --- a/devtools/src/org/labkey/devtools/authentication/TestSsoConfiguration.java +++ b/devtools/src/org/labkey/devtools/authentication/TestSsoConfiguration.java @@ -30,7 +30,7 @@ protected TestSsoConfiguration(TestSsoProvider provider, Map sta } @Override - public URLHelper getUrl(String secret, ViewContext ctx) + public URLHelper getUrl(ViewContext ctx) { ActionURL url = new ActionURL(TestSsoController.TestSsoAction.class, ContainerManager.getRoot()); url.addParameter("configuration", getRowId()); diff --git a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java index 47e9a3a844f..ade152c4c0f 100644 --- a/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java +++ b/experiment/src/org/labkey/experiment/controllers/exp/ExperimentController.java @@ -1887,7 +1887,7 @@ public ModelAndView getView(ExperimentRunForm form, BindException errors) throws } catch (FileNotFoundException e) { - getViewContext().getResponse().sendRedirect(getViewContext().getRequest().getContextPath() + "/experiment/ExperimentRunNotFound.gif"); + throw new RedirectException(new URLHelper(getViewContext().getRequest().getContextPath() + "/experiment/ExperimentRunNotFound.gif")); } finally { @@ -2511,7 +2511,7 @@ protected ModelAndView getDataView(DataForm form, BindException errors) throws I URLHelper url = h.getShowFileURL(_data); if (url != null) { - throw new RedirectException(url, false); + throw new RedirectException(url); } } } @@ -6434,7 +6434,7 @@ public boolean handlePost(CreateExperimentForm form, BindException errors) throw if (form.getReturnUrl() != null) { - throw new RedirectException(form.getReturnUrl()); + throw new RedirectException(form.getReturnActionURL()); } throw new RedirectException(ExperimentUrlsImpl.get().getShowExperimentsURL(getContainer())); } diff --git a/filecontent/src/org/labkey/filecontent/FileContentController.java b/filecontent/src/org/labkey/filecontent/FileContentController.java index 10e144ca207..34c7a8c4223 100644 --- a/filecontent/src/org/labkey/filecontent/FileContentController.java +++ b/filecontent/src/org/labkey/filecontent/FileContentController.java @@ -844,7 +844,7 @@ public void write(ApiResponse response) throws IOException public static class DesignerAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(ReturnUrlForm form) + public ActionURL getRedirectURL(ReturnUrlForm form) { ActionURL successUrl = null; FileContentService svc = FileContentService.get(); diff --git a/issues/src/org/labkey/issue/IssuesController.java b/issues/src/org/labkey/issue/IssuesController.java index 1eade2b3251..1a502be1247 100644 --- a/issues/src/org/labkey/issue/IssuesController.java +++ b/issues/src/org/labkey/issue/IssuesController.java @@ -110,11 +110,11 @@ 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.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; @@ -1884,7 +1884,7 @@ private RssBean(List issues, String filteredURLString) public class JumpToIssueAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(Object o) + public ActionURL getRedirectURL(Object o) { String issueId = (String)getProperty("issueId"); if (issueId != null) @@ -2077,7 +2077,7 @@ public void setQ(String q) public static class SearchAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(SearchForm form) + public ActionURL getRedirectURL(SearchForm form) { return urlProvider(SearchUrls.class).getSearchURL(getContainer(), form.getQ(), IssueSearchResultTemplate.NAME); } diff --git a/mothership/src/org/labkey/mothership/MothershipController.java b/mothership/src/org/labkey/mothership/MothershipController.java index cce8cc007db..65038f99e52 100644 --- a/mothership/src/org/labkey/mothership/MothershipController.java +++ b/mothership/src/org/labkey/mothership/MothershipController.java @@ -501,7 +501,7 @@ public void setErrorCode(String errorCode) public static class JumpToErrorCodeAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(ErrorCodeForm form) + public ActionURL getRedirectURL(ErrorCodeForm form) { String errorCode = StringUtils.trimToNull(form.getErrorCode()); ActionURL url; diff --git a/pipeline/src/org/labkey/pipeline/status/StatusController.java b/pipeline/src/org/labkey/pipeline/status/StatusController.java index 419e145ebc6..e60a428d6ea 100644 --- a/pipeline/src/org/labkey/pipeline/status/StatusController.java +++ b/pipeline/src/org/labkey/pipeline/status/StatusController.java @@ -87,6 +87,7 @@ import java.io.BufferedReader; import java.io.IOException; import java.io.PrintWriter; +import java.net.URISyntaxException; import java.nio.file.Files; import java.nio.file.InvalidPathException; import java.nio.file.Path; @@ -493,7 +494,15 @@ public ActionURL getRedirectURL(RowIdForm form) if (sf.getDataUrl() != null) { - throw new RedirectException(sf.getDataUrl()); + try + { + URLHelper url = new URLHelper(sf.getDataUrl()); + throw new RedirectException(url); + } + catch (URISyntaxException e) + { + throw new RuntimeException(e); + } } return urlDetails(c, form.getRowId()); diff --git a/query/src/org/labkey/query/controllers/QueryController.java b/query/src/org/labkey/query/controllers/QueryController.java index e021d74ea0a..874f0a0ad70 100644 --- a/query/src/org/labkey/query/controllers/QueryController.java +++ b/query/src/org/labkey/query/controllers/QueryController.java @@ -19,7 +19,6 @@ import com.fasterxml.jackson.annotation.JsonIgnoreProperties; import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.databind.ObjectMapper; -import com.google.genai.Chat; import com.google.genai.errors.ClientException; import com.google.genai.errors.ServerException; import jakarta.servlet.ServletException; @@ -28,7 +27,6 @@ import jakarta.servlet.http.HttpSession; import org.antlr.runtime.tree.Tree; import org.apache.commons.beanutils.ConversionException; -import org.apache.commons.beanutils.ConvertUtils; import org.apache.commons.collections4.MultiValuedMap; import org.apache.commons.collections4.multimap.ArrayListValuedHashMap; import org.apache.commons.collections4.multimap.HashSetValuedHashMap; @@ -7570,7 +7568,7 @@ public void setRowId(int rowId) public static class QueryExportAuditRedirectAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(QueryExportAuditForm form) + public ActionURL getRedirectURL(QueryExportAuditForm form) { if (form.getRowId() == 0) throw new NotFoundException("Query export audit rowid required"); diff --git a/query/src/org/labkey/query/reports/AttachmentReport.java b/query/src/org/labkey/query/reports/AttachmentReport.java index bdef0864736..728b9c3048a 100644 --- a/query/src/org/labkey/query/reports/AttachmentReport.java +++ b/query/src/org/labkey/query/reports/AttachmentReport.java @@ -45,7 +45,9 @@ import org.labkey.api.util.ImageUtil; import org.labkey.api.util.JunitUtil; import org.labkey.api.util.MimeMap; +import org.labkey.api.util.URLHelper; import org.labkey.api.view.ActionURL; +import org.labkey.api.view.RedirectException; import org.labkey.api.view.ViewContext; import org.labkey.api.writer.ContainerUser; import org.labkey.api.writer.VirtualFile; @@ -121,6 +123,16 @@ public String getTypeDescription() return null; } + @Override + protected void redirect(ViewContext context) + { + URLHelper url = getURLHelper(context); + if (url != null) + { + throw new RedirectException(url); + } + } + public @Nullable Attachment getLatestVersion() { if (null == getEntityId()) @@ -142,7 +154,7 @@ public String getTypeDescription() } // Something went horribly wrong... I guess we only have thumbnails - return attachments.get(attachments.size() - 1); + return attachments.getLast(); } public void setFilePath(String filePath) diff --git a/query/src/org/labkey/query/reports/BaseRedirectReport.java b/query/src/org/labkey/query/reports/BaseRedirectReport.java index 14b7793f5c0..1b406d7857f 100644 --- a/query/src/org/labkey/query/reports/BaseRedirectReport.java +++ b/query/src/org/labkey/query/reports/BaseRedirectReport.java @@ -24,10 +24,6 @@ import java.util.List; -/** - * User: kevink - * Date: 6/21/12 - */ public abstract class BaseRedirectReport extends RedirectReport { @Override diff --git a/query/src/org/labkey/query/reports/LinkReport.java b/query/src/org/labkey/query/reports/LinkReport.java index c3d88a6abc5..e2a10be1f26 100644 --- a/query/src/org/labkey/query/reports/LinkReport.java +++ b/query/src/org/labkey/query/reports/LinkReport.java @@ -15,25 +15,25 @@ */ package org.labkey.query.reports; -import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.jetbrains.annotations.Nullable; import org.labkey.api.reports.ReportService; import org.labkey.api.thumbnail.Thumbnail; import org.labkey.api.util.ExceptionUtil; import org.labkey.api.util.ImageUtil; +import org.labkey.api.util.URLHelper; +import org.labkey.api.util.logging.LogHelper; import org.labkey.api.view.ActionURL; +import org.labkey.api.view.UnsafeExternalRedirectException; import org.labkey.api.view.ViewContext; import org.labkey.api.writer.ContainerUser; import java.net.URL; import java.net.UnknownHostException; -/** - * User: kevink - * Date: 6/21/12 - */ public class LinkReport extends BaseRedirectReport { + private static final Logger LOG = LogHelper.getLogger(LinkReport.class, "Link report warnings"); public static final String TYPE = ReportService.LINK_REPORT_TYPE; @Override @@ -65,7 +65,7 @@ public Thumbnail generateThumbnail(@Nullable ViewContext context) catch (UnknownHostException | ClassCastException e) { // ClassCastException is most likely a rendering error in flying saucer - LogManager.getLogger(LinkReport.class).warn("Error rendering link report thumbnail: " + e.getMessage()); + LOG.warn("Error rendering link report thumbnail: {}", e.getMessage()); } catch (Exception e) { @@ -98,4 +98,17 @@ public boolean isSandboxed() { return true; } + + @Override + protected void redirect(ViewContext context) + { + URLHelper url = getURLHelper(context); + if (url != null) + { + // It's (relatively) safe to bypass the external redirect allow list here because a user who is Author or + // above has created and shared this link report. We should consider requiring higher permissions to create + // or edit a link report. + throw new UnsafeExternalRedirectException(url); + } + } } diff --git a/query/src/org/labkey/query/reports/ReportsController.java b/query/src/org/labkey/query/reports/ReportsController.java index fd4ba6b3c39..4be4cb7f4c5 100644 --- a/query/src/org/labkey/query/reports/ReportsController.java +++ b/query/src/org/labkey/query/reports/ReportsController.java @@ -1019,7 +1019,7 @@ public ModelAndView getView(ReportDesignBean form, BindException errors) thro } catch (RedirectException re) { - // Link reports throw RedirectException... pass it on + // Link and attachment reports throw RedirectException... pass it on throw re; } catch (RuntimeException e) diff --git a/search/src/org/labkey/search/SearchController.java b/search/src/org/labkey/search/SearchController.java index c43f5c5d9b9..74f4da262b4 100644 --- a/search/src/org/labkey/search/SearchController.java +++ b/search/src/org/labkey/search/SearchController.java @@ -438,7 +438,7 @@ public void export(ExportForm form, HttpServletResponse response, BindException public static class WaitForIdleAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(Object o) throws Exception + public ActionURL getRedirectURL(Object o) throws Exception { SearchService ss = AbstractSearchService.get(); ss.waitForIdle(); diff --git a/study/src/org/labkey/study/controllers/StudyController.java b/study/src/org/labkey/study/controllers/StudyController.java index bcbb2cd2b0e..83b02c438fe 100644 --- a/study/src/org/labkey/study/controllers/StudyController.java +++ b/study/src/org/labkey/study/controllers/StudyController.java @@ -263,7 +263,6 @@ import org.labkey.study.model.StudyImpl; import org.labkey.study.model.StudyManager; import org.labkey.study.model.StudySnapshot; -import org.labkey.study.model.UploadLog; import org.labkey.study.model.VisitDataset; import org.labkey.study.model.VisitDatasetType; import org.labkey.study.model.VisitImpl; @@ -6315,7 +6314,7 @@ public void addNavTrail(NavTree root) public static class DatasetDetailRedirectAction extends SimpleRedirectAction { @Override - public URLHelper getRedirectURL(DatasetDetailRedirectForm form) + public ActionURL getRedirectURL(DatasetDetailRedirectForm form) { StudyImpl study = StudyManager.getInstance().getStudy(getContainer()); if (study == null) diff --git a/wiki/src/org/labkey/wiki/WikiController.java b/wiki/src/org/labkey/wiki/WikiController.java index a9c9c35cc7a..603ffb045d0 100644 --- a/wiki/src/org/labkey/wiki/WikiController.java +++ b/wiki/src/org/labkey/wiki/WikiController.java @@ -1173,7 +1173,7 @@ public ModelAndView getView(WikiNameForm form, BindException errors) String realName = WikiSelectManager.getNameForAlias(getContainer(), name); if (null != realName) { - LOG.debug("PageAction: requested wiki name, \"" + name + "\", is an alias; redirecting to \"" + realName + "\". Referrer: " + getViewContext().getRequest().getHeader("Referer")); + LOG.debug("PageAction: requested wiki name, \"{}\", is an alias; redirecting to \"{}\". Referrer: {}", name, realName, getViewContext().getRequest().getHeader("Referer")); throw new PermanentRedirectException(getViewContext().getActionURL().clone().replaceParameter("name", realName)); }