mirror of
https://github.com/iflytek/skillhub.git
synced 2026-09-24 00:55:35 +00:00
fix(auth): bound Feishu userinfo response and stop subject leaking into displayName
Three defects found reviewing this batch against the R1-A2 spec. Response size limit. The spec's scope line asks for "远程 I/O 超时与响应大小 限制"; only the timeouts were implemented, so a misconfigured or hostile OAUTH2_FEISHU_BASE_URI could stream an unbounded body into the parser. Reads at most 64 KB before parsing, mirroring the 10 MB cap the shared WebClientConfig already applies. Uses InputStream.readNBytes rather than adding commons-io or guava, neither of which skillhub-auth declares. Synthesized displayName. Falling back to "feishu-<open_id>" wrote the external subject into UserAccount.displayName and into UserActivatedEvent, carrying it somewhere event consumers may log it -- against the R1-A gate that logs must not contain the subject. Now stops at name -> en_name like the GitHub and GitLab extractors. Unused mobile attribute. A phone number was extracted into the principal attributes and read by nothing. It is PII the spec did not ask for and it widened the redaction surface for free. Also drops a constructor overload that only passed List.of() through, and a test that duplicated the blank-subject path. Signed-off-by: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com>
This commit is contained in:
parent
f29ac241fc
commit
dba36ca1b9
6 changed files with 51 additions and 38 deletions
|
|
@ -41,13 +41,13 @@ public class FeishuClaimsExtractor implements OAuthClaimsExtractor {
|
|||
// in real time, so they carry no verification signal; keep emailVerified false.
|
||||
boolean emailVerified = false;
|
||||
|
||||
// name -> en_name and stop, matching the GitHub and GitLab extractors. Falling back to the
|
||||
// subject would write it into UserAccount.displayName and into UserActivatedEvent, pushing
|
||||
// the external subject somewhere event consumers may log it.
|
||||
String username = (String) attrs.get("name");
|
||||
if (username == null || username.isBlank()) {
|
||||
username = (String) attrs.get("en_name");
|
||||
}
|
||||
if (username == null || username.isBlank()) {
|
||||
username = "feishu-" + subject;
|
||||
}
|
||||
|
||||
return new OAuthClaims(
|
||||
FeishuOAuth2UserService.PROVIDER,
|
||||
|
|
|
|||
|
|
@ -2,12 +2,14 @@ package com.iflytek.skillhub.auth.oauth;
|
|||
|
||||
import com.fasterxml.jackson.annotation.JsonIgnoreProperties;
|
||||
import com.fasterxml.jackson.annotation.JsonProperty;
|
||||
import com.fasterxml.jackson.databind.ObjectMapper;
|
||||
import java.io.IOException;
|
||||
import java.io.InputStream;
|
||||
import java.time.Duration;
|
||||
import java.util.Collections;
|
||||
import java.util.LinkedHashMap;
|
||||
import java.util.Map;
|
||||
import org.springframework.beans.factory.annotation.Autowired;
|
||||
import org.springframework.core.ParameterizedTypeReference;
|
||||
import org.springframework.http.HttpHeaders;
|
||||
import org.springframework.http.MediaType;
|
||||
import org.springframework.http.client.ClientHttpRequestFactory;
|
||||
|
|
@ -35,6 +37,11 @@ public class FeishuOAuth2UserService implements ProviderOAuth2UserService {
|
|||
private static final Duration CONNECT_TIMEOUT = Duration.ofSeconds(5);
|
||||
private static final Duration READ_TIMEOUT = Duration.ofSeconds(10);
|
||||
|
||||
/** A Feishu user_info payload is well under 1 KB; this only needs to stop an unbounded body. */
|
||||
private static final int MAX_RESPONSE_BYTES = 64 * 1024;
|
||||
|
||||
private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();
|
||||
|
||||
/**
|
||||
* Uses an external-service client that is intentionally not customized with application
|
||||
* tracing. Trace context must not be propagated to the external Feishu service.
|
||||
|
|
@ -61,6 +68,19 @@ public class FeishuOAuth2UserService implements ProviderOAuth2UserService {
|
|||
return factory;
|
||||
}
|
||||
|
||||
/**
|
||||
* Reads at most {@link #MAX_RESPONSE_BYTES} before parsing, so a misconfigured or hostile
|
||||
* {@code OAUTH2_FEISHU_BASE_URI} cannot stream an unbounded body into the parser. Reading one
|
||||
* byte past the cap is what distinguishes an oversized payload from one that exactly fills it.
|
||||
*/
|
||||
private static FeishuUserResponse readBounded(InputStream body) throws IOException {
|
||||
byte[] bytes = body.readNBytes(MAX_RESPONSE_BYTES + 1);
|
||||
if (bytes.length > MAX_RESPONSE_BYTES) {
|
||||
throw new IOException("Feishu user info response exceeds " + MAX_RESPONSE_BYTES + " bytes");
|
||||
}
|
||||
return OBJECT_MAPPER.readValue(bytes, FeishuUserResponse.class);
|
||||
}
|
||||
|
||||
@Override
|
||||
public String getProvider() {
|
||||
return PROVIDER;
|
||||
|
|
@ -76,8 +96,7 @@ public class FeishuOAuth2UserService implements ProviderOAuth2UserService {
|
|||
response = restClient.get()
|
||||
.uri(userInfoUri)
|
||||
.header(HttpHeaders.AUTHORIZATION, "Bearer " + userRequest.getAccessToken().getTokenValue())
|
||||
.retrieve()
|
||||
.body(new ParameterizedTypeReference<FeishuUserResponse>() {});
|
||||
.exchange((request, clientResponse) -> readBounded(clientResponse.getBody()));
|
||||
} catch (Exception e) {
|
||||
// The cause carries the detail for operators; the OAuth2Error description stays generic
|
||||
// because an upstream message can quote the request URI, which holds the access token.
|
||||
|
|
@ -117,7 +136,6 @@ public class FeishuOAuth2UserService implements ProviderOAuth2UserService {
|
|||
putIfPresent(attributes, "avatar_url", data.avatarUrl());
|
||||
putIfPresent(attributes, "email", data.email());
|
||||
putIfPresent(attributes, "enterprise_email", data.enterpriseEmail());
|
||||
putIfPresent(attributes, "mobile", data.mobile());
|
||||
if (!attributes.containsKey(userNameAttributeName)) {
|
||||
throw new OAuth2AuthenticationException(
|
||||
new OAuth2Error("feishu_userinfo_error", "Feishu user info missing " + userNameAttributeName, null)
|
||||
|
|
@ -143,7 +161,6 @@ public class FeishuOAuth2UserService implements ProviderOAuth2UserService {
|
|||
@JsonProperty("en_name") String enName,
|
||||
@JsonProperty("avatar_url") String avatarUrl,
|
||||
@JsonProperty("email") String email,
|
||||
@JsonProperty("enterprise_email") String enterpriseEmail,
|
||||
@JsonProperty("mobile") String mobile
|
||||
@JsonProperty("enterprise_email") String enterpriseEmail
|
||||
) {}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -79,23 +79,6 @@ public class OAuthLoginFlowService {
|
|||
this.remoteIdentityIo = remoteIdentityIo;
|
||||
}
|
||||
|
||||
OAuthLoginFlowService(List<OAuthClaimsExtractor> extractorList,
|
||||
AccessPolicy accessPolicy,
|
||||
IdentityBindingService identityBindingService,
|
||||
LegacyPlatformIdentityCore identityCore,
|
||||
OAuth2UserService<OAuth2UserRequest, OAuth2User> delegate,
|
||||
RemoteIdentityIoExecutor remoteIdentityIo) {
|
||||
this(
|
||||
extractorList,
|
||||
List.of(),
|
||||
accessPolicy,
|
||||
identityBindingService,
|
||||
identityCore,
|
||||
delegate,
|
||||
remoteIdentityIo
|
||||
);
|
||||
}
|
||||
|
||||
OAuthLoginFlowService(List<OAuthClaimsExtractor> extractorList,
|
||||
AccessPolicy accessPolicy,
|
||||
IdentityBindingService identityBindingService,
|
||||
|
|
|
|||
|
|
@ -40,7 +40,7 @@ class FeishuClaimsExtractorTest {
|
|||
}
|
||||
|
||||
@Test
|
||||
void extract_allowsNullEmailAndFallsBackUsername() {
|
||||
void extract_allowsNullEmailAndLeavesDisplayNameUnsetWhenFeishuSendsNoName() {
|
||||
Map<String, Object> attrs = new HashMap<>(Map.of("open_id", "ou_456"));
|
||||
|
||||
OAuthClaims claims = extractor.extract(userRequest(), user(attrs));
|
||||
|
|
@ -48,7 +48,9 @@ class FeishuClaimsExtractorTest {
|
|||
assertThat(claims.subject()).isEqualTo("ou_456");
|
||||
assertThat(claims.email()).isNull();
|
||||
assertThat(claims.emailVerified()).isFalse();
|
||||
assertThat(claims.providerLogin()).isEqualTo("feishu-ou_456");
|
||||
// Must not synthesize "feishu-<open_id>": providerLogin is written to displayName and into
|
||||
// UserActivatedEvent, so a synthesized value would carry the subject into event consumers.
|
||||
assertThat(claims.providerLogin()).isNull();
|
||||
}
|
||||
|
||||
@Test
|
||||
|
|
@ -77,16 +79,6 @@ class FeishuClaimsExtractorTest {
|
|||
.hasMessageContaining("open_id");
|
||||
}
|
||||
|
||||
@Test
|
||||
void extract_rejectsMissingOpenIdWithoutFabricatingASubject() {
|
||||
Map<String, Object> attrs = new HashMap<>();
|
||||
attrs.put("name", "张三");
|
||||
|
||||
assertThatThrownBy(() -> extractor.extract(userRequest(), permissiveUser(attrs)))
|
||||
.isInstanceOf(OAuth2AuthenticationException.class)
|
||||
.hasMessageContaining("open_id");
|
||||
}
|
||||
|
||||
/** An {@link OAuth2User} that does not enforce the name attribute, unlike DefaultOAuth2User. */
|
||||
private OAuth2User permissiveUser(Map<String, Object> attrs) {
|
||||
return new OAuth2User() {
|
||||
|
|
|
|||
|
|
@ -81,6 +81,26 @@ class FeishuOAuth2UserServiceTest {
|
|||
server.verify();
|
||||
}
|
||||
|
||||
@Test
|
||||
void loadUser_rejectsOversizedResponseBody() {
|
||||
RestClient.Builder restClientBuilder = RestClient.builder();
|
||||
MockRestServiceServer server = MockRestServiceServer.bindTo(restClientBuilder).build();
|
||||
// 64 KB cap; pad a structurally valid envelope past it so the size check fires, not the parser.
|
||||
String padding = "x".repeat(70 * 1024);
|
||||
server.expect(requestTo("https://open.feishu.cn/open-apis/authen/v1/user_info"))
|
||||
.andRespond(withSuccess(
|
||||
"{\"code\":0,\"msg\":\"" + padding + "\",\"data\":{\"open_id\":\"ou_123\"}}",
|
||||
MediaType.APPLICATION_JSON
|
||||
));
|
||||
FeishuOAuth2UserService service = new FeishuOAuth2UserService(restClientBuilder);
|
||||
|
||||
assertThatThrownBy(() -> service.loadUser(userRequest()))
|
||||
.isInstanceOf(OAuth2AuthenticationException.class)
|
||||
.satisfies(ex -> assertThat(((OAuth2AuthenticationException) ex).getError().getErrorCode())
|
||||
.isEqualTo("feishu_userinfo_error"));
|
||||
server.verify();
|
||||
}
|
||||
|
||||
@Test
|
||||
void loadUser_errorDescriptionDoesNotEchoUpstreamTextOrToken() {
|
||||
RestClient.Builder restClientBuilder = RestClient.builder();
|
||||
|
|
|
|||
|
|
@ -74,6 +74,7 @@ class OAuthLoginFlowServiceTest {
|
|||
};
|
||||
OAuthLoginFlowService service = new OAuthLoginFlowService(
|
||||
List.of(extractor),
|
||||
List.of(),
|
||||
accessPolicy,
|
||||
identityBindingService,
|
||||
identityCore,
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue