diff --git a/src/e2e/java/teammates/e2e/cases/BaseE2ETestCase.java b/src/e2e/java/teammates/e2e/cases/BaseE2ETestCase.java index 0d2161cb165..9b06e0939cc 100644 --- a/src/e2e/java/teammates/e2e/cases/BaseE2ETestCase.java +++ b/src/e2e/java/teammates/e2e/cases/BaseE2ETestCase.java @@ -180,10 +180,6 @@ protected T loginAdminToPage(AppUrl url, Class typeOfPage */ protected void logout() { AppUrl url = createBackendUrl(Const.WebPageURIs.LOGOUT); - if (!TestProperties.TEAMMATES_FRONTEND_URL.equals(TestProperties.TEAMMATES_BACKEND_URL)) { - url = url.withParam("frontendUrl", TestProperties.TEAMMATES_FRONTEND_URL); - } - browser.goToUrl(TestProperties.TEAMMATES_FRONTEND_URL); browser.waitForPageReadyState(); browser.removeSessionStorageItem(MASQUERADE_ACCOUNT_ID_STORAGE_KEY); diff --git a/src/main/java/teammates/common/util/UrlHelper.java b/src/main/java/teammates/common/util/UrlHelper.java index d6e223d5d13..e871d5ac641 100644 --- a/src/main/java/teammates/common/util/UrlHelper.java +++ b/src/main/java/teammates/common/util/UrlHelper.java @@ -12,9 +12,6 @@ public final class UrlHelper { public static final String DEFAULT_REDIRECT_URL = "/"; - private static final int HTTPS_PORT = 443; - private static final int HTTP_PORT = 80; - private UrlHelper() { // utility class } @@ -27,70 +24,37 @@ public static String encodeQueryParam(String param) { } /** - * Returns a safe redirect URL. - * If the provided nextUrl is not safe, it returns the {@link #DEFAULT_REDIRECT_URL}. + * Returns a safe relative redirect URL. + * + *

+ * If the provided redirectUrl is not safe, or isn't relative, it returns the {@link #DEFAULT_REDIRECT_URL}. */ - public static String getSafeRedirectUrl(String nextUrl) { - return isSafeRedirectUrl(nextUrl) ? nextUrl : DEFAULT_REDIRECT_URL; + public static String getSafeRelativeRedirectUrl(String redirectUrl) { + return isSafeRelativeRedirectUrl(redirectUrl) ? redirectUrl : DEFAULT_REDIRECT_URL; } /** - * Checks whether the given URL is safe to use as a redirect target. + * Checks whether the given relative URL is safe to use as a redirect target. + * + *

+ * Rejects absolute URLs. */ - public static boolean isSafeRedirectUrl(String url) { - if (url == null) { + public static boolean isSafeRelativeRedirectUrl(String url) { + if (StringHelper.isEmpty(url)) { return false; } try { URI uri = new URI(url); if (uri.isAbsolute()) { - return isSafeAbsoluteRedirectUrl(uri); + return false; } - return isSafeRelativeRedirectUrl(url); + return url.startsWith("/") + && !url.startsWith("//"); } catch (URISyntaxException e) { return false; } } - private static boolean isSafeRelativeRedirectUrl(String url) { - return url.startsWith("/") - && !url.startsWith("//"); - } - - private static boolean isSafeAbsoluteRedirectUrl(URI uri) throws URISyntaxException { - if (!isHttp(uri) && !isHttps(uri)) { - return false; - } - - URI frontendUri = new URI(Config.APP_FRONTEND_URL); - return isSameOrigin(uri, frontendUri); - } - - private static boolean isSameOrigin(URI firstUri, URI secondUri) { - boolean isSameScheme = firstUri.getScheme().equalsIgnoreCase(secondUri.getScheme()); - boolean isSameHost = firstUri.getHost() != null - && firstUri.getHost().equalsIgnoreCase(secondUri.getHost()); - boolean isSamePort = getPort(firstUri) == getPort(secondUri); - - return isSameScheme && isSameHost && isSamePort; - } - - private static int getPort(URI uri) { - if (uri.getPort() != -1) { - return uri.getPort(); - } - - return isHttps(uri) ? HTTPS_PORT : HTTP_PORT; - } - - private static boolean isHttp(URI uri) { - return "http".equalsIgnoreCase(uri.getScheme()); - } - - private static boolean isHttps(URI uri) { - return "https".equalsIgnoreCase(uri.getScheme()); - } - } diff --git a/src/main/java/teammates/ui/servlets/LoginServlet.java b/src/main/java/teammates/ui/servlets/LoginServlet.java index d6e79f927dc..c6773204785 100644 --- a/src/main/java/teammates/ui/servlets/LoginServlet.java +++ b/src/main/java/teammates/ui/servlets/LoginServlet.java @@ -37,11 +37,12 @@ public LoginServlet() { @Override public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { - String nextUrl = UrlHelper.getSafeRedirectUrl(req.getParameter(Const.ParamsNames.NEXT_URL)); + String nextUrl = UrlHelper.getSafeRelativeRedirectUrl(req.getParameter(Const.ParamsNames.NEXT_URL)); if (!isLoginNeeded(req)) { log.request(req, HttpStatus.SC_MOVED_TEMPORARILY, "Redirect to next URL"); - String redirectUrl = resp.encodeRedirectURL(nextUrl); + String redirectUrl = Config.getFrontEndAppUrl(nextUrl).toAbsoluteString(); + redirectUrl = resp.encodeRedirectURL(redirectUrl); resp.sendRedirect(redirectUrl); return; } diff --git a/src/main/java/teammates/ui/servlets/LogoutServlet.java b/src/main/java/teammates/ui/servlets/LogoutServlet.java index af434c044f3..bcf8c19b8dc 100644 --- a/src/main/java/teammates/ui/servlets/LogoutServlet.java +++ b/src/main/java/teammates/ui/servlets/LogoutServlet.java @@ -8,6 +8,7 @@ import org.apache.http.HttpStatus; +import teammates.common.util.Config; import teammates.common.util.Logger; import teammates.common.util.UrlHelper; @@ -25,7 +26,7 @@ public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOExc Cookie cookie = getLoginInvalidationCookie(); resp.addCookie(cookie); - String frontendUrl = UrlHelper.getSafeRedirectUrl(req.getParameter("frontendUrl")); + String frontendUrl = Config.getFrontEndAppUrl(UrlHelper.DEFAULT_REDIRECT_URL).toAbsoluteString(); frontendUrl = resp.encodeRedirectURL(frontendUrl); log.request(req, HttpStatus.SC_MOVED_TEMPORARILY, "Redirect to home page after logging out"); resp.sendRedirect(frontendUrl); diff --git a/src/main/java/teammates/ui/servlets/OAuth2CallbackServlet.java b/src/main/java/teammates/ui/servlets/OAuth2CallbackServlet.java index 51c0958cbac..73e05303943 100644 --- a/src/main/java/teammates/ui/servlets/OAuth2CallbackServlet.java +++ b/src/main/java/teammates/ui/servlets/OAuth2CallbackServlet.java @@ -71,8 +71,9 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IO return; } - String nextUrl = UrlHelper.getSafeRedirectUrl(state.nextUrl()); - String redirectUrl = resp.encodeRedirectURL(nextUrl); + String nextUrl = UrlHelper.getSafeRelativeRedirectUrl(state.nextUrl()); + String redirectUrl = Config.getFrontEndAppUrl(nextUrl).toAbsoluteString(); + redirectUrl = resp.encodeRedirectURL(redirectUrl); log.info("Going to redirect to: " + redirectUrl); log.request(req, HttpStatus.SC_MOVED_TEMPORARILY, "Login successful"); diff --git a/src/main/resources/userEmailTemplate-feedbackSessionPublishedPreview.html b/src/main/resources/userEmailTemplate-feedbackSessionPublishedPreview.html index 12871120c98..6d1a8841de2 100644 --- a/src/main/resources/userEmailTemplate-feedbackSessionPublishedPreview.html +++ b/src/main/resources/userEmailTemplate-feedbackSessionPublishedPreview.html @@ -10,7 +10,7 @@ The results for the TEAMMATES session ${feedbackSessionName} in course [${courseId}] ${courseName} are now available for viewing. If a student did not receive the unique access link via email, and cannot find it in the spam box either, go to - this link to recover the access link. + this link to recover the access link.

The email below has been sent to student recipients of course: [${courseId}] ${courseName}.

diff --git a/src/test/java/teammates/common/util/UrlHelperTest.java b/src/test/java/teammates/common/util/UrlHelperTest.java index 8a0904725ec..0bf4920527e 100644 --- a/src/test/java/teammates/common/util/UrlHelperTest.java +++ b/src/test/java/teammates/common/util/UrlHelperTest.java @@ -4,10 +4,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; import static teammates.common.util.UrlHelper.encodeQueryParam; -import static teammates.common.util.UrlHelper.isSafeRedirectUrl; - -import java.net.URI; -import java.net.URISyntaxException; +import static teammates.common.util.UrlHelper.isSafeRelativeRedirectUrl; import org.testng.annotations.DataProvider; import org.testng.annotations.Test; @@ -19,23 +16,35 @@ */ public class UrlHelperTest extends BaseTestCase { - @Test - public void testIsSafeRedirectUrl_relativeUrl_returnsTrue() { - String url = "/web/instructor/home"; + @Test(dataProvider = "relativeUrls") + public void testIsSafeRelativeRedirectUrl_relativeUrl_returnsTrue(String url) { + assertTrue(isSafeRelativeRedirectUrl(url)); + } - assertTrue(isSafeRedirectUrl(url)); + @DataProvider + private Object[][] relativeUrls() { + return new Object[][] { + {"/web/instructor/home"}, + {"/web/student/home?query=value"}, + }; } - @Test - public void testIsSafeRedirectUrl_configuredFrontendUrl_returnsTrue() { - String url = Config.APP_FRONTEND_URL + "/web/instructor/home"; + @Test(dataProvider = "absoluteUrls") + public void testIsSafeRelativeRedirectUrl_absoluteUrl_returnsFalse(String url) { + assertFalse(isSafeRelativeRedirectUrl(url)); + } - assertTrue(isSafeRedirectUrl(url)); + @DataProvider + private Object[][] absoluteUrls() { + return new Object[][] { + {"https://example.com/web/instructor/home"}, + {"http://example.com/web/student/home"}, + }; } @Test(dataProvider = "externalUrls") - public void testIsSafeRedirectUrl_externalUrl_returnsFalse(String url) { - assertFalse(isSafeRedirectUrl(url)); + public void testIsSafeRelativeRedirectUrl_externalUrl_returnsFalse(String url) { + assertFalse(isSafeRelativeRedirectUrl(url)); } @DataProvider @@ -43,52 +52,65 @@ private Object[][] externalUrls() { return new Object[][] { {"https://example.com/web/instructor/home"}, {"https://evil.example.com"}, + {"evil.com/web/instructor/home"}, + {"//evil.com/web/instructor/home"}, }; } - @Test - public void testIsSafeRedirectUrl_differentPort_returnsFalse() throws URISyntaxException { - URI frontendUri = new URI(Config.APP_FRONTEND_URL); - int differentPort = frontendUri.getPort() == 8080 ? 8081 : 8080; - String url = String.format("%s://%s:%d/web/instructor/home", - frontendUri.getScheme(), frontendUri.getHost(), differentPort); - - assertFalse(isSafeRedirectUrl(url)); + @Test(dataProvider = "malformedUrls") + public void testIsSafeRelativeRedirectUrl_malformedUrl_returnsFalse(String url) { + assertFalse(isSafeRelativeRedirectUrl(url)); } - @Test - public void testIsSafeRedirectUrl_protocolRelativeUrl_returnsFalse() { - assertFalse(isSafeRedirectUrl("//example.com/web/instructor/home")); + @DataProvider + private Object[][] malformedUrls() { + return new Object[][] { + {"https://[invalid"}, + {"example.com/invalid path"}, + }; } - @Test(dataProvider = "unsupportedSchemeUrls") - public void testIsSafeRedirectUrl_unsupportedScheme_returnsFalse(String url) { - assertFalse(isSafeRedirectUrl(url)); + @Test(dataProvider = "invalidRedirectUrls") + public void testIsSafeRelativeRedirectUrl_invalidRedirectUrl_returnsFalse(String url) { + assertFalse(isSafeRelativeRedirectUrl(url)); } @DataProvider - private Object[][] unsupportedSchemeUrls() { + private Object[][] invalidRedirectUrls() { return new Object[][] { - {"javascript:alert(1)"}, - {"ftp://example.com/web/instructor/home"}, + {"?query=param"}, + {"web/instructor"}, + {"https://evil.com"}, + {"//evil.com"}, }; } - @Test - public void testIsSafeRedirectUrl_nullUrl_returnsFalse() { - assertFalse(isSafeRedirectUrl(null)); + @Test(dataProvider = "invalidUrls") + public void testIsSafeRelativeRedirectUrl_invalidUrl_returnsFalse(String url) { + assertFalse(isSafeRelativeRedirectUrl(url)); } - @Test(dataProvider = "malformedUrls") - public void testIsSafeRedirectUrl_malformedUrl_returnsFalse(String url) { - assertFalse(isSafeRedirectUrl(url)); + @DataProvider + private Object[][] invalidUrls() { + return new Object[][] { + {""}, + {null}, + {"javascript:alert(1)"}, + }; + } + + @Test(dataProvider = "unsupportedSchemaUrls") + public void testIsSafeRelativeRedirectUrl_unsupportedSchemaUrl_returnsFalse(String url) { + assertFalse(isSafeRelativeRedirectUrl(url)); } @DataProvider - private Object[][] malformedUrls() { + private Object[][] unsupportedSchemaUrls() { return new Object[][] { - {"https://[invalid"}, - {"web/instructor/home"}, + {"ftp://example.com/resource"}, + {"file:///path/to/file"}, + {"mailto:example@example.com"}, + {"../relative/path"}, }; } diff --git a/src/test/java/teammates/ui/servlets/LoginServletTest.java b/src/test/java/teammates/ui/servlets/LoginServletTest.java index a24d6832385..c2306dc906c 100644 --- a/src/test/java/teammates/ui/servlets/LoginServletTest.java +++ b/src/test/java/teammates/ui/servlets/LoginServletTest.java @@ -99,7 +99,7 @@ public void doGet_validCookieForExistingAccount_redirectsToNextUrl() throws Exce servlet.doGet(req, resp); } - assertEquals("/web/instructor/home", resp.getRedirectUrl()); + assertEquals(Config.APP_FRONTEND_URL + "/web/instructor/home", resp.getRedirectUrl()); } @Test diff --git a/src/test/java/teammates/ui/servlets/OAuth2CallbackServletTest.java b/src/test/java/teammates/ui/servlets/OAuth2CallbackServletTest.java index b9add758380..fa860fcea57 100644 --- a/src/test/java/teammates/ui/servlets/OAuth2CallbackServletTest.java +++ b/src/test/java/teammates/ui/servlets/OAuth2CallbackServletTest.java @@ -193,7 +193,7 @@ public void doGet_callbackHandlerReturnsAuthResult_redirectsToNextUrl() throws E servlet.doGet(req, resp); } - assertEquals("/web/instructor/home", resp.getRedirectUrl()); + assertEquals(Config.APP_FRONTEND_URL + "/web/instructor/home", resp.getRedirectUrl()); } private static MockedStatic mockSupportedLoginMethod(LoginMethod loginMethod, boolean isSupported) { diff --git a/src/test/resources/emails/sessionPublishedEmailCopyToInstructor.html b/src/test/resources/emails/sessionPublishedEmailCopyToInstructor.html index 1ed9d8e7c14..6998dfe28cc 100644 --- a/src/test/resources/emails/sessionPublishedEmailCopyToInstructor.html +++ b/src/test/resources/emails/sessionPublishedEmailCopyToInstructor.html @@ -10,7 +10,7 @@ The results for the TEAMMATES session First feedback session in course [idOfTypicalCourse1] Typical Course 1 with 2 Evals are now available for viewing. If a student did not receive the unique access link via email, and cannot find it in the spam box either, go to - this link to recover the access link. + this link to recover the access link.
The email below has been sent to student recipients of course: [idOfTypicalCourse1] Typical Course 1 with 2 Evals.

diff --git a/src/test/resources/emails/sessionPublishedEmailForStudent.html b/src/test/resources/emails/sessionPublishedEmailForStudent.html index 7136959cefa..2f07b7781ee 100644 --- a/src/test/resources/emails/sessionPublishedEmailForStudent.html +++ b/src/test/resources/emails/sessionPublishedEmailForStudent.html @@ -30,7 +30,7 @@