From dba36ca1b96db97d03259fca8244ef9f5fb397d2 Mon Sep 17 00:00:00 2001 From: XiaoSeS <87064762+XiaoSeS@users.noreply.github.com> Date: Fri, 18 Sep 2026 15:34:13 +0800 Subject: [PATCH] fix(auth): bound Feishu userinfo response and stop subject leaking into displayName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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-" 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> --- .../auth/oauth/FeishuClaimsExtractor.java | 6 ++-- .../auth/oauth/FeishuOAuth2UserService.java | 29 +++++++++++++++---- .../auth/oauth/OAuthLoginFlowService.java | 17 ----------- .../auth/oauth/FeishuClaimsExtractorTest.java | 16 +++------- .../oauth/FeishuOAuth2UserServiceTest.java | 20 +++++++++++++ .../auth/oauth/OAuthLoginFlowServiceTest.java | 1 + 6 files changed, 51 insertions(+), 38 deletions(-) diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/FeishuClaimsExtractor.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/FeishuClaimsExtractor.java index a2ad65e9..ba7fded0 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/FeishuClaimsExtractor.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/FeishuClaimsExtractor.java @@ -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, diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/FeishuOAuth2UserService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/FeishuOAuth2UserService.java index d866ac41..c5e4bcb6 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/FeishuOAuth2UserService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/FeishuOAuth2UserService.java @@ -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() {}); + .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 ) {} } diff --git a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java index 3b4d265e..25e74cc9 100644 --- a/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java +++ b/server/skillhub-auth/src/main/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowService.java @@ -79,23 +79,6 @@ public class OAuthLoginFlowService { this.remoteIdentityIo = remoteIdentityIo; } - OAuthLoginFlowService(List extractorList, - AccessPolicy accessPolicy, - IdentityBindingService identityBindingService, - LegacyPlatformIdentityCore identityCore, - OAuth2UserService delegate, - RemoteIdentityIoExecutor remoteIdentityIo) { - this( - extractorList, - List.of(), - accessPolicy, - identityBindingService, - identityCore, - delegate, - remoteIdentityIo - ); - } - OAuthLoginFlowService(List extractorList, AccessPolicy accessPolicy, IdentityBindingService identityBindingService, diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/FeishuClaimsExtractorTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/FeishuClaimsExtractorTest.java index 9b70fd72..d7715045 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/FeishuClaimsExtractorTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/FeishuClaimsExtractorTest.java @@ -40,7 +40,7 @@ class FeishuClaimsExtractorTest { } @Test - void extract_allowsNullEmailAndFallsBackUsername() { + void extract_allowsNullEmailAndLeavesDisplayNameUnsetWhenFeishuSendsNoName() { Map 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-": 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 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 attrs) { return new OAuth2User() { diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/FeishuOAuth2UserServiceTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/FeishuOAuth2UserServiceTest.java index ab9c5ff1..39367a3f 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/FeishuOAuth2UserServiceTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/FeishuOAuth2UserServiceTest.java @@ -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(); diff --git a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java index e9046ef7..7ece104d 100644 --- a/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java +++ b/server/skillhub-auth/src/test/java/com/iflytek/skillhub/auth/oauth/OAuthLoginFlowServiceTest.java @@ -74,6 +74,7 @@ class OAuthLoginFlowServiceTest { }; OAuthLoginFlowService service = new OAuthLoginFlowService( List.of(extractor), + List.of(), accessPolicy, identityBindingService, identityCore,