From e6ab6871e98a9cb60a993e873d9c3b7c98a63af6 Mon Sep 17 00:00:00 2001 From: Viacheslav Klimov Date: Tue, 1 Sep 2026 14:42:14 +0300 Subject: [PATCH] Log rejected prevUri values and share the mail flow constants A rejected deep link is otherwise invisible to support. AdminController reuses the prevUri parameter and cookie names instead of its own copies, and its redirect target moves into a helper so the read side is covered by a test. --- .../server/controller/AdminController.java | 23 ++++-- .../auth/oauth2/PrevUriValidator.java | 23 +++++- .../AdminControllerMailOAuth2Test.java | 81 +++++++++++++++++++ .../controller/AdminControllerTest.java | 42 ++++++---- ...th2AuthorizationRequestRepositoryTest.java | 13 +-- 5 files changed, 151 insertions(+), 31 deletions(-) create mode 100644 application/src/test/java/org/thingsboard/server/controller/AdminControllerMailOAuth2Test.java diff --git a/application/src/main/java/org/thingsboard/server/controller/AdminController.java b/application/src/main/java/org/thingsboard/server/controller/AdminController.java index cbe1114864..8e8f4c4de3 100644 --- a/application/src/main/java/org/thingsboard/server/controller/AdminController.java +++ b/application/src/main/java/org/thingsboard/server/controller/AdminController.java @@ -93,6 +93,8 @@ import java.util.Optional; import static org.thingsboard.server.controller.ControllerConstants.SYSTEM_AUTHORITY_PARAGRAPH; import static org.thingsboard.server.controller.ControllerConstants.TENANT_AUTHORITY_PARAGRAPH; +import static org.thingsboard.server.service.security.auth.oauth2.HttpCookieOAuth2AuthorizationRequestRepository.PREV_URI_COOKIE_NAME; +import static org.thingsboard.server.service.security.auth.oauth2.HttpCookieOAuth2AuthorizationRequestRepository.PREV_URI_PARAMETER; @RestController @TbCoreComponent @@ -101,8 +103,7 @@ import static org.thingsboard.server.controller.ControllerConstants.TENANT_AUTHO @RequiredArgsConstructor public class AdminController extends BaseController { - private static final String PREV_URI_PATH_PARAMETER = "prevUri"; - private static final String PREV_URI_COOKIE_NAME = "prev_uri"; + private static final String DEFAULT_PREV_URI = "/settings/outgoing-mail"; private static final String STATE_COOKIE_NAME = "state"; private static final String MAIL_SETTINGS_KEY = "mail"; @@ -420,7 +421,7 @@ public class AdminController extends BaseController { @GetMapping(value = "/mail/oauth2/authorize", produces = "application/text") public String getAuthorizationUrl(HttpServletRequest request, HttpServletResponse response) throws ThingsboardException { String state = StringUtils.generateSafeToken(); - String prevUriParam = request.getParameter(PREV_URI_PATH_PARAMETER); + String prevUriParam = request.getParameter(PREV_URI_PARAMETER); if (PrevUriValidator.isValid(prevUriParam)) { CookieUtils.addCookie(response, PREV_URI_COOKIE_NAME, prevUriParam, 180); } @@ -447,12 +448,9 @@ public class AdminController extends BaseController { public void codeProcessingUrl( @RequestParam(value = "code") String code, @RequestParam(value = "state") String state, HttpServletRequest request, HttpServletResponse response) throws ThingsboardException, IOException { - Optional prevUrlOpt = CookieUtils.getCookie(request, PREV_URI_COOKIE_NAME); + String redirectUrl = getMailOAuth2RedirectUrl(request); Optional cookieState = CookieUtils.getCookie(request, STATE_COOKIE_NAME); - String baseUrl = this.systemSecurityService.getBaseUrl(TenantId.SYS_TENANT_ID, new CustomerId(EntityId.NULL_UUID), request); - String prevUri = baseUrl + prevUrlOpt.map(Cookie::getValue).filter(PrevUriValidator::isValid).orElse("/settings/outgoing-mail"); - if (cookieState.isEmpty() || !cookieState.get().getValue().equals(state)) { CookieUtils.deleteCookie(request, response, STATE_COOKIE_NAME); throw new ThingsboardException("Refresh token was not generated, invalid state param", ThingsboardErrorCode.BAD_REQUEST_PARAMS); @@ -482,7 +480,16 @@ public class AdminController extends BaseController { ((ObjectNode) jsonValue).put("tokenGenerated", true); adminSettingsService.saveAdminSettings(TenantId.SYS_TENANT_ID, adminSettings); - response.sendRedirect(prevUri); + response.sendRedirect(redirectUrl); + } + + String getMailOAuth2RedirectUrl(HttpServletRequest request) { + String baseUrl = this.systemSecurityService.getBaseUrl(TenantId.SYS_TENANT_ID, new CustomerId(EntityId.NULL_UUID), request); + String prevUri = CookieUtils.getCookie(request, PREV_URI_COOKIE_NAME) + .map(Cookie::getValue) + .filter(PrevUriValidator::isValid) + .orElse(DEFAULT_PREV_URI); + return baseUrl + prevUri; } } diff --git a/application/src/main/java/org/thingsboard/server/service/security/auth/oauth2/PrevUriValidator.java b/application/src/main/java/org/thingsboard/server/service/security/auth/oauth2/PrevUriValidator.java index 005979408c..14855ecda3 100644 --- a/application/src/main/java/org/thingsboard/server/service/security/auth/oauth2/PrevUriValidator.java +++ b/application/src/main/java/org/thingsboard/server/service/security/auth/oauth2/PrevUriValidator.java @@ -15,13 +15,27 @@ */ package org.thingsboard.server.service.security.auth.oauth2; +import lombok.extern.slf4j.Slf4j; import org.thingsboard.server.common.data.StringUtils; import java.util.Locale; +@Slf4j public class PrevUriValidator { private static final int MAX_LENGTH = 2048; + private static final int MAX_LOGGED_LENGTH = 128; + + public static boolean isValid(String prevUri) { + if (StringUtils.isEmpty(prevUri)) { + return false; + } + if (!isInAppPath(prevUri)) { + log.debug("Ignoring prevUri that is not an in-app path: [{}]", forLog(prevUri)); + return false; + } + return true; + } /** * prevUri is appended to the platform base URL, which ends right after the authority, so the single leading '/' @@ -30,8 +44,8 @@ public class PrevUriValidator { * characters nor '"', ',', ';', '\' or non-ASCII) and to pass StrictHttpFirewall, which rejects '//', '%2f' * and '%5c' in the path; a fragment would swallow the access token. */ - public static boolean isValid(String prevUri) { - if (StringUtils.isEmpty(prevUri) || prevUri.length() > MAX_LENGTH || prevUri.charAt(0) != '/') { + private static boolean isInAppPath(String prevUri) { + if (prevUri.length() > MAX_LENGTH || prevUri.charAt(0) != '/') { return false; } for (int i = 0; i < prevUri.length(); i++) { @@ -44,4 +58,9 @@ public class PrevUriValidator { return !path.contains("//") && !path.contains("%2f") && !path.contains("%5c"); } + // a rejected value is attacker-controlled: it must not be able to forge log lines + private static String forLog(String prevUri) { + return prevUri.substring(0, Math.min(prevUri.length(), MAX_LOGGED_LENGTH)).replaceAll("[^\\x20-\\x7E]", "?"); + } + } diff --git a/application/src/test/java/org/thingsboard/server/controller/AdminControllerMailOAuth2Test.java b/application/src/test/java/org/thingsboard/server/controller/AdminControllerMailOAuth2Test.java new file mode 100644 index 0000000000..b74acea73a --- /dev/null +++ b/application/src/test/java/org/thingsboard/server/controller/AdminControllerMailOAuth2Test.java @@ -0,0 +1,81 @@ +/** + * Copyright © 2016-2026 The Thingsboard Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.thingsboard.server.controller; + +import jakarta.servlet.http.Cookie; +import jakarta.servlet.http.HttpServletRequest; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; +import org.thingsboard.server.common.data.id.CustomerId; +import org.thingsboard.server.common.data.id.TenantId; +import org.thingsboard.server.service.security.system.SystemSecurityService; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.thingsboard.server.service.security.auth.oauth2.HttpCookieOAuth2AuthorizationRequestRepository.PREV_URI_COOKIE_NAME; + +@ExtendWith(MockitoExtension.class) +public class AdminControllerMailOAuth2Test { + + private static final String BASE_URL = "https://thingsboard.example.com"; + private static final String DEFAULT_PREV_URI = "/settings/outgoing-mail"; + + @Mock + private SystemSecurityService systemSecurityService; + + @InjectMocks + private AdminController adminController; + + private HttpServletRequest request; + + @BeforeEach + public void before() { + request = mock(HttpServletRequest.class); + when(systemSecurityService.getBaseUrl(any(TenantId.class), any(CustomerId.class), any(HttpServletRequest.class))).thenReturn(BASE_URL); + } + + @Test + public void testInAppPathIsTakenFromPrevUriCookie() { + givenPrevUriCookie("/settings/notifications?tab=1"); + assertThat(adminController.getMailOAuth2RedirectUrl(request)).isEqualTo(BASE_URL + "/settings/notifications?tab=1"); + } + + @ParameterizedTest + @ValueSource(strings = {"@evil.com/", "//evil.com", "https://evil.com", "/\\evil.com", "/settings#fragment"}) + public void testForgedPrevUriCookieIsIgnored(String prevUri) { + givenPrevUriCookie(prevUri); + assertThat(adminController.getMailOAuth2RedirectUrl(request)).isEqualTo(BASE_URL + DEFAULT_PREV_URI); + } + + @Test + public void testRedirectUrlWithoutPrevUriCookie() { + when(request.getCookies()).thenReturn(null); + assertThat(adminController.getMailOAuth2RedirectUrl(request)).isEqualTo(BASE_URL + DEFAULT_PREV_URI); + } + + private void givenPrevUriCookie(String prevUri) { + when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(PREV_URI_COOKIE_NAME, prevUri)}); + } + +} diff --git a/application/src/test/java/org/thingsboard/server/controller/AdminControllerTest.java b/application/src/test/java/org/thingsboard/server/controller/AdminControllerTest.java index e0866a3d86..bbdd324d82 100644 --- a/application/src/test/java/org/thingsboard/server/controller/AdminControllerTest.java +++ b/application/src/test/java/org/thingsboard/server/controller/AdminControllerTest.java @@ -39,6 +39,8 @@ import static org.mockito.ArgumentMatchers.anyString; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; +import static org.thingsboard.server.service.security.auth.oauth2.HttpCookieOAuth2AuthorizationRequestRepository.PREV_URI_COOKIE_NAME; +import static org.thingsboard.server.service.security.auth.oauth2.HttpCookieOAuth2AuthorizationRequestRepository.PREV_URI_PARAMETER; @Slf4j @DaoSqlTest @@ -116,22 +118,30 @@ public class AdminControllerTest extends AbstractControllerTest { public void testMailOAuth2AuthorizationStoresOnlyInAppPrevUri() throws Exception { loginSysAdmin(); AdminSettings mailSettings = doGet("/api/admin/settings/mail", AdminSettings.class); - ObjectNode jsonValue = JacksonUtil.fromString(mailSettings.getJsonValue().toString(), ObjectNode.class); - jsonValue.put("clientId", "clientId"); - jsonValue.put("authUri", "https://accounts.google.com/o/oauth2/v2/auth"); - jsonValue.put("redirectUri", "https://thingsboard.io/api/admin/mail/oauth2/code"); - jsonValue.set("scope", JacksonUtil.newArrayNode().add("https://mail.google.com/")); - mailSettings.setJsonValue(jsonValue); - doPost("/api/admin/settings", mailSettings, AdminSettings.class); - - Cookie prevUriCookie = doGet("/api/admin/mail/oauth2/authorize?prevUri=@evil.com/") - .andExpect(status().isOk()).andReturn().getResponse().getCookie("prev_uri"); - assertThat(prevUriCookie).isNull(); - - prevUriCookie = doGet("/api/admin/mail/oauth2/authorize?prevUri=/settings/outgoing-mail") - .andExpect(status().isOk()).andReturn().getResponse().getCookie("prev_uri"); - assertThat(prevUriCookie).isNotNull(); - assertThat(prevUriCookie.getValue()).isEqualTo("/settings/outgoing-mail"); + JsonNode originalJsonValue = mailSettings.getJsonValue(); + try { + ObjectNode jsonValue = JacksonUtil.fromString(originalJsonValue.toString(), ObjectNode.class); + jsonValue.put("clientId", "clientId"); + jsonValue.put("authUri", "https://accounts.google.com/o/oauth2/v2/auth"); + jsonValue.put("redirectUri", "https://thingsboard.io/api/admin/mail/oauth2/code"); + jsonValue.set("scope", JacksonUtil.newArrayNode().add("https://mail.google.com/")); + mailSettings.setJsonValue(jsonValue); + doPost("/api/admin/settings", mailSettings, AdminSettings.class); + + Cookie prevUriCookie = doGet("/api/admin/mail/oauth2/authorize?" + PREV_URI_PARAMETER + "=@evil.com/") + .andExpect(status().isOk()).andReturn().getResponse().getCookie(PREV_URI_COOKIE_NAME); + assertThat(prevUriCookie).isNull(); + + prevUriCookie = doGet("/api/admin/mail/oauth2/authorize?" + PREV_URI_PARAMETER + "=/settings/outgoing-mail") + .andExpect(status().isOk()).andReturn().getResponse().getCookie(PREV_URI_COOKIE_NAME); + assertThat(prevUriCookie).isNotNull(); + assertThat(prevUriCookie.getValue()).isEqualTo("/settings/outgoing-mail"); + } finally { + // the mail settings are shared by the whole test context + AdminSettings currentSettings = doGet("/api/admin/settings/mail", AdminSettings.class); + currentSettings.setJsonValue(originalJsonValue); + doPost("/api/admin/settings", currentSettings, AdminSettings.class); + } } @Test diff --git a/application/src/test/java/org/thingsboard/server/service/security/auth/oauth2/HttpCookieOAuth2AuthorizationRequestRepositoryTest.java b/application/src/test/java/org/thingsboard/server/service/security/auth/oauth2/HttpCookieOAuth2AuthorizationRequestRepositoryTest.java index 92aa8f18df..194b5d7939 100644 --- a/application/src/test/java/org/thingsboard/server/service/security/auth/oauth2/HttpCookieOAuth2AuthorizationRequestRepositoryTest.java +++ b/application/src/test/java/org/thingsboard/server/service/security/auth/oauth2/HttpCookieOAuth2AuthorizationRequestRepositoryTest.java @@ -19,6 +19,9 @@ import jakarta.servlet.http.Cookie; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.NullAndEmptySource; +import org.junit.jupiter.params.provider.ValueSource; import org.mockito.ArgumentCaptor; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; @@ -40,11 +43,11 @@ public class HttpCookieOAuth2AuthorizationRequestRepositoryTest { .isEqualTo("/dashboards/3fa13530-6597-11ed-bd76-8bd591f0ec3e?state=someState"); } - @Test - public void testPrevUriNotSavedForExternalUri() { - assertThat(savePrevUri("@evil.com/")).isNull(); - assertThat(savePrevUri("https://evil.com")).isNull(); - assertThat(savePrevUri("//evil.com")).isNull(); + @ParameterizedTest + @NullAndEmptySource + @ValueSource(strings = {"@evil.com/"}) + public void testPrevUriNotSavedForExternalUri(String prevUri) { + assertThat(savePrevUri(prevUri)).isNull(); } private String savePrevUri(String prevUri) {