Browse Source

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.
pull/16102/head
Viacheslav Klimov 6 days ago
parent
commit
e6ab6871e9
Failed to extract signature
  1. 23
      application/src/main/java/org/thingsboard/server/controller/AdminController.java
  2. 23
      application/src/main/java/org/thingsboard/server/service/security/auth/oauth2/PrevUriValidator.java
  3. 81
      application/src/test/java/org/thingsboard/server/controller/AdminControllerMailOAuth2Test.java
  4. 42
      application/src/test/java/org/thingsboard/server/controller/AdminControllerTest.java
  5. 13
      application/src/test/java/org/thingsboard/server/service/security/auth/oauth2/HttpCookieOAuth2AuthorizationRequestRepositoryTest.java

23
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.SYSTEM_AUTHORITY_PARAGRAPH;
import static org.thingsboard.server.controller.ControllerConstants.TENANT_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 @RestController
@TbCoreComponent @TbCoreComponent
@ -101,8 +103,7 @@ import static org.thingsboard.server.controller.ControllerConstants.TENANT_AUTHO
@RequiredArgsConstructor @RequiredArgsConstructor
public class AdminController extends BaseController { public class AdminController extends BaseController {
private static final String PREV_URI_PATH_PARAMETER = "prevUri"; private static final String DEFAULT_PREV_URI = "/settings/outgoing-mail";
private static final String PREV_URI_COOKIE_NAME = "prev_uri";
private static final String STATE_COOKIE_NAME = "state"; private static final String STATE_COOKIE_NAME = "state";
private static final String MAIL_SETTINGS_KEY = "mail"; 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") @GetMapping(value = "/mail/oauth2/authorize", produces = "application/text")
public String getAuthorizationUrl(HttpServletRequest request, HttpServletResponse response) throws ThingsboardException { public String getAuthorizationUrl(HttpServletRequest request, HttpServletResponse response) throws ThingsboardException {
String state = StringUtils.generateSafeToken(); String state = StringUtils.generateSafeToken();
String prevUriParam = request.getParameter(PREV_URI_PATH_PARAMETER); String prevUriParam = request.getParameter(PREV_URI_PARAMETER);
if (PrevUriValidator.isValid(prevUriParam)) { if (PrevUriValidator.isValid(prevUriParam)) {
CookieUtils.addCookie(response, PREV_URI_COOKIE_NAME, prevUriParam, 180); CookieUtils.addCookie(response, PREV_URI_COOKIE_NAME, prevUriParam, 180);
} }
@ -447,12 +448,9 @@ public class AdminController extends BaseController {
public void codeProcessingUrl( public void codeProcessingUrl(
@RequestParam(value = "code") String code, @RequestParam(value = "state") String state, @RequestParam(value = "code") String code, @RequestParam(value = "state") String state,
HttpServletRequest request, HttpServletResponse response) throws ThingsboardException, IOException { HttpServletRequest request, HttpServletResponse response) throws ThingsboardException, IOException {
Optional<Cookie> prevUrlOpt = CookieUtils.getCookie(request, PREV_URI_COOKIE_NAME); String redirectUrl = getMailOAuth2RedirectUrl(request);
Optional<Cookie> cookieState = CookieUtils.getCookie(request, STATE_COOKIE_NAME); Optional<Cookie> 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)) { if (cookieState.isEmpty() || !cookieState.get().getValue().equals(state)) {
CookieUtils.deleteCookie(request, response, STATE_COOKIE_NAME); CookieUtils.deleteCookie(request, response, STATE_COOKIE_NAME);
throw new ThingsboardException("Refresh token was not generated, invalid state param", ThingsboardErrorCode.BAD_REQUEST_PARAMS); 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); ((ObjectNode) jsonValue).put("tokenGenerated", true);
adminSettingsService.saveAdminSettings(TenantId.SYS_TENANT_ID, adminSettings); 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;
} }
} }

23
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; package org.thingsboard.server.service.security.auth.oauth2;
import lombok.extern.slf4j.Slf4j;
import org.thingsboard.server.common.data.StringUtils; import org.thingsboard.server.common.data.StringUtils;
import java.util.Locale; import java.util.Locale;
@Slf4j
public class PrevUriValidator { public class PrevUriValidator {
private static final int MAX_LENGTH = 2048; 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 '/' * 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' * characters nor '"', ',', ';', '\' or non-ASCII) and to pass StrictHttpFirewall, which rejects '//', '%2f'
* and '%5c' in the path; a fragment would swallow the access token. * and '%5c' in the path; a fragment would swallow the access token.
*/ */
public static boolean isValid(String prevUri) { private static boolean isInAppPath(String prevUri) {
if (StringUtils.isEmpty(prevUri) || prevUri.length() > MAX_LENGTH || prevUri.charAt(0) != '/') { if (prevUri.length() > MAX_LENGTH || prevUri.charAt(0) != '/') {
return false; return false;
} }
for (int i = 0; i < prevUri.length(); i++) { for (int i = 0; i < prevUri.length(); i++) {
@ -44,4 +58,9 @@ public class PrevUriValidator {
return !path.contains("//") && !path.contains("%2f") && !path.contains("%5c"); 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]", "?");
}
} }

81
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)});
}
}

42
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.content;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.jsonPath;
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status; 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 @Slf4j
@DaoSqlTest @DaoSqlTest
@ -116,22 +118,30 @@ public class AdminControllerTest extends AbstractControllerTest {
public void testMailOAuth2AuthorizationStoresOnlyInAppPrevUri() throws Exception { public void testMailOAuth2AuthorizationStoresOnlyInAppPrevUri() throws Exception {
loginSysAdmin(); loginSysAdmin();
AdminSettings mailSettings = doGet("/api/admin/settings/mail", AdminSettings.class); AdminSettings mailSettings = doGet("/api/admin/settings/mail", AdminSettings.class);
ObjectNode jsonValue = JacksonUtil.fromString(mailSettings.getJsonValue().toString(), ObjectNode.class); JsonNode originalJsonValue = mailSettings.getJsonValue();
jsonValue.put("clientId", "clientId"); try {
jsonValue.put("authUri", "https://accounts.google.com/o/oauth2/v2/auth"); ObjectNode jsonValue = JacksonUtil.fromString(originalJsonValue.toString(), ObjectNode.class);
jsonValue.put("redirectUri", "https://thingsboard.io/api/admin/mail/oauth2/code"); jsonValue.put("clientId", "clientId");
jsonValue.set("scope", JacksonUtil.newArrayNode().add("https://mail.google.com/")); jsonValue.put("authUri", "https://accounts.google.com/o/oauth2/v2/auth");
mailSettings.setJsonValue(jsonValue); jsonValue.put("redirectUri", "https://thingsboard.io/api/admin/mail/oauth2/code");
doPost("/api/admin/settings", mailSettings, AdminSettings.class); jsonValue.set("scope", JacksonUtil.newArrayNode().add("https://mail.google.com/"));
mailSettings.setJsonValue(jsonValue);
Cookie prevUriCookie = doGet("/api/admin/mail/oauth2/authorize?prevUri=@evil.com/") doPost("/api/admin/settings", mailSettings, AdminSettings.class);
.andExpect(status().isOk()).andReturn().getResponse().getCookie("prev_uri");
assertThat(prevUriCookie).isNull(); Cookie prevUriCookie = doGet("/api/admin/mail/oauth2/authorize?" + PREV_URI_PARAMETER + "=@evil.com/")
.andExpect(status().isOk()).andReturn().getResponse().getCookie(PREV_URI_COOKIE_NAME);
prevUriCookie = doGet("/api/admin/mail/oauth2/authorize?prevUri=/settings/outgoing-mail") assertThat(prevUriCookie).isNull();
.andExpect(status().isOk()).andReturn().getResponse().getCookie("prev_uri");
assertThat(prevUriCookie).isNotNull(); prevUriCookie = doGet("/api/admin/mail/oauth2/authorize?" + PREV_URI_PARAMETER + "=/settings/outgoing-mail")
assertThat(prevUriCookie.getValue()).isEqualTo("/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 @Test

13
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.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse; import jakarta.servlet.http.HttpServletResponse;
import org.junit.jupiter.api.Test; 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.mockito.ArgumentCaptor;
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest;
@ -40,11 +43,11 @@ public class HttpCookieOAuth2AuthorizationRequestRepositoryTest {
.isEqualTo("/dashboards/3fa13530-6597-11ed-bd76-8bd591f0ec3e?state=someState"); .isEqualTo("/dashboards/3fa13530-6597-11ed-bd76-8bd591f0ec3e?state=someState");
} }
@Test @ParameterizedTest
public void testPrevUriNotSavedForExternalUri() { @NullAndEmptySource
assertThat(savePrevUri("@evil.com/")).isNull(); @ValueSource(strings = {"@evil.com/"})
assertThat(savePrevUri("https://evil.com")).isNull(); public void testPrevUriNotSavedForExternalUri(String prevUri) {
assertThat(savePrevUri("//evil.com")).isNull(); assertThat(savePrevUri(prevUri)).isNull();
} }
private String savePrevUri(String prevUri) { private String savePrevUri(String prevUri) {

Loading…
Cancel
Save