Browse Source
The scheme is restored from the oauth2_auth_request cookie, which the client can replace, so checking it only while the authorization request is built still let a forged cookie point the token redirect at any host. Both handlers now re-check it on read, sharing the rule with OAuth2AppTokenFactory. The success handler keeps prevUri out of the base URL as well, so the error redirect no longer appends /login to an in-app path.pull/16102/head
Viacheslav Klimov
6 days ago
8 changed files with 279 additions and 195 deletions
@ -0,0 +1,65 @@ |
|||
/** |
|||
* 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.service.security.auth.oauth2; |
|||
|
|||
import lombok.extern.slf4j.Slf4j; |
|||
import org.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; |
|||
import org.thingsboard.server.common.data.StringUtils; |
|||
|
|||
import java.util.Locale; |
|||
import java.util.Set; |
|||
import java.util.regex.Pattern; |
|||
|
|||
@Slf4j |
|||
public class CallbackUrlSchemeValidator { |
|||
|
|||
private static final Pattern SCHEME_PATTERN = Pattern.compile("[a-zA-Z][a-zA-Z0-9+.-]*"); |
|||
private static final Set<String> FORBIDDEN_SCHEMES = Set.of("http", "https", "javascript", "data", "file", "vbscript"); |
|||
private static final int MAX_LOGGED_LENGTH = 128; |
|||
|
|||
/** |
|||
* The redirect carrying the access token is built as callbackUrlScheme + ':', so only a mobile app scheme may |
|||
* pass: a web scheme would send the token to whatever host follows it. |
|||
*/ |
|||
public static boolean isValid(String callbackUrlScheme) { |
|||
return !StringUtils.isEmpty(callbackUrlScheme) |
|||
&& SCHEME_PATTERN.matcher(callbackUrlScheme).matches() |
|||
&& !FORBIDDEN_SCHEMES.contains(callbackUrlScheme.toLowerCase(Locale.ROOT)); |
|||
} |
|||
|
|||
/** |
|||
* The attribute is restored from the oauth2_auth_request cookie, which the client can replace, so the scheme is |
|||
* checked again on read and not only when the authorization request is built. |
|||
*/ |
|||
public static String getCallbackUrlScheme(OAuth2AuthorizationRequest authorizationRequest) { |
|||
String callbackUrlScheme = authorizationRequest != null ? |
|||
authorizationRequest.getAttribute(TbOAuth2ParameterNames.CALLBACK_URL_SCHEME) : null; |
|||
if (StringUtils.isEmpty(callbackUrlScheme)) { |
|||
return null; |
|||
} |
|||
if (!isValid(callbackUrlScheme)) { |
|||
log.warn("Ignoring invalid callback url scheme: [{}]", forLog(callbackUrlScheme)); |
|||
return null; |
|||
} |
|||
return callbackUrlScheme; |
|||
} |
|||
|
|||
// a rejected value is attacker-controlled: it must not be able to forge log lines
|
|||
private static String forLog(String value) { |
|||
return value.substring(0, Math.min(value.length(), MAX_LOGGED_LENGTH)).replaceAll("[^\\x20-\\x7E]", "?"); |
|||
} |
|||
|
|||
} |
|||
@ -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.service.security.auth.oauth2; |
|||
|
|||
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.springframework.security.oauth2.core.endpoint.OAuth2AuthorizationRequest; |
|||
|
|||
import static org.assertj.core.api.Assertions.assertThat; |
|||
|
|||
public class CallbackUrlSchemeValidatorTest { |
|||
|
|||
@ParameterizedTest |
|||
@ValueSource(strings = {"tbmobile", "tb-mobile.app1", "TbMobile+1"}) |
|||
public void testMobileAppSchemeIsValid(String callbackUrlScheme) { |
|||
assertThat(CallbackUrlSchemeValidator.isValid(callbackUrlScheme)).isTrue(); |
|||
} |
|||
|
|||
@ParameterizedTest |
|||
@NullAndEmptySource |
|||
@ValueSource(strings = { |
|||
"https://evil.com", |
|||
"http://evil.com", |
|||
"https", |
|||
"HTTPS", |
|||
"javascript", |
|||
"data", |
|||
"file", |
|||
"vbscript", |
|||
"//evil.com", |
|||
"tbmobile/evil.com", |
|||
"tbmobile:evil.com", |
|||
"tbmobile evil", |
|||
"1tbmobile", |
|||
"tbmobile@evil.com" |
|||
}) |
|||
public void testInvalidSchemeIsRejected(String callbackUrlScheme) { |
|||
assertThat(CallbackUrlSchemeValidator.isValid(callbackUrlScheme)).isFalse(); |
|||
} |
|||
|
|||
@Test |
|||
public void testValidSchemeIsTakenFromAuthorizationRequest() { |
|||
assertThat(CallbackUrlSchemeValidator.getCallbackUrlScheme(givenAuthorizationRequest("tbmobile"))).isEqualTo("tbmobile"); |
|||
} |
|||
|
|||
@Test |
|||
public void testForgedSchemeFromAuthorizationRequestIsIgnored() { |
|||
assertThat(CallbackUrlSchemeValidator.getCallbackUrlScheme(givenAuthorizationRequest("https://evil.com"))).isNull(); |
|||
} |
|||
|
|||
@Test |
|||
public void testAuthorizationRequestWithoutScheme() { |
|||
assertThat(CallbackUrlSchemeValidator.getCallbackUrlScheme(givenAuthorizationRequest(null))).isNull(); |
|||
assertThat(CallbackUrlSchemeValidator.getCallbackUrlScheme(null)).isNull(); |
|||
} |
|||
|
|||
private OAuth2AuthorizationRequest givenAuthorizationRequest(String callbackUrlScheme) { |
|||
OAuth2AuthorizationRequest.Builder builder = OAuth2AuthorizationRequest.authorizationCode() |
|||
.authorizationUri("testUri").clientId("testId"); |
|||
if (callbackUrlScheme != null) { |
|||
builder.attributes(attributes -> attributes.put(TbOAuth2ParameterNames.CALLBACK_URL_SCHEME, callbackUrlScheme)); |
|||
} |
|||
return builder.build(); |
|||
} |
|||
|
|||
} |
|||
@ -1,102 +0,0 @@ |
|||
/** |
|||
* 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.service.security.auth.oauth2; |
|||
|
|||
import jakarta.servlet.http.Cookie; |
|||
import jakarta.servlet.http.HttpServletRequest; |
|||
import jakarta.servlet.http.HttpServletResponse; |
|||
import org.junit.jupiter.api.BeforeEach; |
|||
import org.junit.jupiter.api.Test; |
|||
import org.junit.jupiter.params.ParameterizedTest; |
|||
import org.junit.jupiter.params.provider.ValueSource; |
|||
import org.mockito.ArgumentCaptor; |
|||
import org.springframework.security.oauth2.client.OAuth2AuthorizedClientService; |
|||
import org.thingsboard.server.common.data.id.CustomerId; |
|||
import org.thingsboard.server.common.data.id.TenantId; |
|||
import org.thingsboard.server.dao.oauth2.OAuth2ClientService; |
|||
import org.thingsboard.server.service.security.model.token.JwtTokenFactory; |
|||
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.verify; |
|||
import static org.mockito.Mockito.when; |
|||
import static org.thingsboard.server.service.security.auth.oauth2.HttpCookieOAuth2AuthorizationRequestRepository.PREV_URI_COOKIE_NAME; |
|||
|
|||
public class Oauth2AuthenticationSuccessHandlerBaseUrlTest { |
|||
|
|||
private static final String BASE_URL = "https://thingsboard.example.com"; |
|||
|
|||
private final SystemSecurityService systemSecurityService = mock(SystemSecurityService.class); |
|||
private final Oauth2AuthenticationSuccessHandler successHandler = new Oauth2AuthenticationSuccessHandler( |
|||
mock(JwtTokenFactory.class), mock(OAuth2ClientMapperProvider.class), mock(OAuth2ClientService.class), |
|||
mock(OAuth2AuthorizedClientService.class), mock(HttpCookieOAuth2AuthorizationRequestRepository.class), |
|||
systemSecurityService); |
|||
|
|||
private HttpServletRequest request; |
|||
private HttpServletResponse response; |
|||
|
|||
@BeforeEach |
|||
public void before() { |
|||
request = mock(HttpServletRequest.class); |
|||
response = mock(HttpServletResponse.class); |
|||
when(systemSecurityService.getBaseUrl(any(TenantId.class), any(CustomerId.class), any(HttpServletRequest.class))).thenReturn(BASE_URL); |
|||
} |
|||
|
|||
@Test |
|||
public void testInAppPathIsAppendedToBaseUrl() { |
|||
givenPrevUriCookie("/dashboards/3fa13530-6597-11ed-bd76-8bd591f0ec3e?state=someState"); |
|||
assertThat(successHandler.getBaseUrl(request, response, null)) |
|||
.isEqualTo(BASE_URL + "/dashboards/3fa13530-6597-11ed-bd76-8bd591f0ec3e?state=someState"); |
|||
} |
|||
|
|||
@ParameterizedTest |
|||
@ValueSource(strings = {"@evil.com/", "//evil.com", "https://evil.com", "/\\evil.com", "/dashboards#fragment"}) |
|||
public void testForgedPrevUriCookieIsIgnored(String prevUri) { |
|||
givenPrevUriCookie(prevUri); |
|||
assertThat(successHandler.getBaseUrl(request, response, null)).isEqualTo(BASE_URL); |
|||
} |
|||
|
|||
@Test |
|||
public void testForgedPrevUriCookieIsDeleted() { |
|||
givenPrevUriCookie("@evil.com/"); |
|||
|
|||
successHandler.getBaseUrl(request, response, null); |
|||
|
|||
ArgumentCaptor<Cookie> cookieCaptor = ArgumentCaptor.forClass(Cookie.class); |
|||
verify(response).addCookie(cookieCaptor.capture()); |
|||
assertThat(cookieCaptor.getValue().getName()).isEqualTo(PREV_URI_COOKIE_NAME); |
|||
assertThat(cookieCaptor.getValue().getMaxAge()).isZero(); |
|||
} |
|||
|
|||
@Test |
|||
public void testBaseUrlWithoutPrevUriCookie() { |
|||
when(request.getCookies()).thenReturn(null); |
|||
assertThat(successHandler.getBaseUrl(request, response, null)).isEqualTo(BASE_URL); |
|||
} |
|||
|
|||
@Test |
|||
public void testCallbackUrlSchemeIgnoresPrevUri() { |
|||
givenPrevUriCookie("/dashboards/3fa13530-6597-11ed-bd76-8bd591f0ec3e"); |
|||
assertThat(successHandler.getBaseUrl(request, response, "tbmobile")).isEqualTo("tbmobile:"); |
|||
} |
|||
|
|||
private void givenPrevUriCookie(String prevUri) { |
|||
when(request.getCookies()).thenReturn(new Cookie[]{new Cookie(PREV_URI_COOKIE_NAME, prevUri)}); |
|||
} |
|||
|
|||
} |
|||
Loading…
Reference in new issue