Copilot commented on code in PR #7876: URL: https://github.com/apache/incubator-seata/pull/7876#discussion_r2641829931
########## console/src/main/java/org/apache/seata/console/filter/MCPAuthenticationFilter.java: ########## @@ -0,0 +1,86 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.seata.console.filter; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.seata.console.security.User; +import org.springframework.http.HttpStatus; +import org.springframework.security.authentication.AuthenticationManager; +import org.springframework.security.authentication.BadCredentialsException; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.web.filter.OncePerRequestFilter; + +import java.io.IOException; + +public class MCPAuthenticationFilter extends OncePerRequestFilter { + + private final AuthenticationManager authenticationManager; + + /** + * Instantiates a new MCP authentication filter that authenticates requests + * using username and password provided in HTTP headers. + * + * @param authenticationManager the authentication manager used to validate credentials + */ + public MCPAuthenticationFilter(AuthenticationManager authenticationManager) { + this.authenticationManager = authenticationManager; + } + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain) + throws IOException, ServletException { + + Authentication existingAuth = SecurityContextHolder.getContext().getAuthentication(); + if (existingAuth != null && existingAuth.isAuthenticated()) { + chain.doFilter(request, response); + return; + } + + User user = resolveHeaders(request); + + UsernamePasswordAuthenticationToken authenticationToken = + new UsernamePasswordAuthenticationToken(user.getUsername(), user.getPassword()); Review Comment: The User object is being instantiated with a plaintext password from the request header. This password should not be stored or passed around in plaintext. The authentication process should use the password directly with the AuthenticationManager without creating an intermediate User object that holds plaintext credentials. Consider passing the credentials directly to the UsernamePasswordAuthenticationToken without creating a User object. ########## console/src/main/java/org/apache/seata/mcp/core/props/MCPProperties.java: ########## @@ -0,0 +1,121 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.seata.mcp.core.props; + +import jakarta.annotation.PostConstruct; +import org.springframework.core.env.Environment; +import org.springframework.stereotype.Component; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.TimeUnit; + +@Component +public class MCPProperties { + + private final Environment env; + + public static final String SSE_TYPE = "sse"; + + public static final String STREAMABLE_TYPE = "streamable"; + + private boolean enableAuth = true; + + private Long queryDuration = TimeUnit.DAYS.toMillis(1); + + private String mcpType = SSE_TYPE; + + private StreamableProperties streamableProperties; + + private SseServerProperties sseServerProperties; + + public MCPProperties(Environment env) { + this.env = env; + } + + public boolean isSseType() { + return mcpType.equals(SSE_TYPE); + } + + public List<String> getEndpoints() { + List<String> result = new ArrayList<>(); + if (isSseType()) { + result.add(sseServerProperties.sseEndpoint); + result.add(sseServerProperties.messageEndpoint); + } else { + result.add(streamableProperties.mcpEndpoint); + } + return result; + } + + public Long getQueryDuration() { + return queryDuration; + } + + public static class StreamableProperties { + private final String mcpEndpoint; + + public StreamableProperties(String mcpEndPoint) { + this.mcpEndpoint = mcpEndPoint; + } + } + + public static class SseServerProperties { + + private final String sseEndpoint; + + private final String messageEndpoint; + + public SseServerProperties(String sseEndpoint, String messageEndpoint) { + this.sseEndpoint = sseEndpoint; + this.messageEndpoint = messageEndpoint; + } + } + + @PostConstruct + public void init() { + mcpType = env.getProperty("spring.ai.mcp.server.protocol", "sse"); + if (mcpType.equals(STREAMABLE_TYPE)) { + String mcpEndPoint = env.getProperty("spring.ai.mcp.server.streamable-http.mcp-endpoint", "/mcp"); + streamableProperties = new StreamableProperties(mcpEndPoint); Review Comment: The variable name 'mcpEndPoint' (line 93) uses inconsistent capitalization compared to the field name 'mcpEndpoint' (line 70). The parameter name should be 'mcpEndpoint' to maintain consistent naming throughout the codebase. ########## console/src/main/java/org/apache/seata/console/filter/MCPAuthenticationFilter.java: ########## @@ -0,0 +1,86 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.seata.console.filter; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.seata.console.security.User; +import org.springframework.http.HttpStatus; +import org.springframework.security.authentication.AuthenticationManager; +import org.springframework.security.authentication.BadCredentialsException; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.web.filter.OncePerRequestFilter; + +import java.io.IOException; + +public class MCPAuthenticationFilter extends OncePerRequestFilter { + + private final AuthenticationManager authenticationManager; + + /** + * Instantiates a new MCP authentication filter that authenticates requests + * using username and password provided in HTTP headers. + * + * @param authenticationManager the authentication manager used to validate credentials + */ + public MCPAuthenticationFilter(AuthenticationManager authenticationManager) { + this.authenticationManager = authenticationManager; + } + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain) + throws IOException, ServletException { + + Authentication existingAuth = SecurityContextHolder.getContext().getAuthentication(); + if (existingAuth != null && existingAuth.isAuthenticated()) { + chain.doFilter(request, response); + return; + } + + User user = resolveHeaders(request); + + UsernamePasswordAuthenticationToken authenticationToken = + new UsernamePasswordAuthenticationToken(user.getUsername(), user.getPassword()); + + try { + Authentication authentication = authenticationManager.authenticate(authenticationToken); Review Comment: When password validation fails in CustomAuthenticationProvider, it returns a UsernamePasswordAuthenticationToken with null credentials and authorities, which is then treated as a successful authentication. In MCPAuthenticationFilter line 65, any non-null Authentication result from authenticationManager.authenticate() is stored in SecurityContext, treating it as successful. However, a failed password check should throw an AuthenticationException instead of returning a token. This creates a security vulnerability where any request with a valid username but invalid password would be authenticated. ```suggestion Authentication authentication = authenticationManager.authenticate(authenticationToken); if (authentication == null || !authentication.isAuthenticated()) { throw new BadCredentialsException("Invalid credentials"); } ``` ########## console/src/main/java/org/apache/seata/console/filter/MCPAuthenticationFilter.java: ########## @@ -0,0 +1,86 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.seata.console.filter; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.apache.seata.console.security.User; +import org.springframework.http.HttpStatus; +import org.springframework.security.authentication.AuthenticationManager; +import org.springframework.security.authentication.BadCredentialsException; +import org.springframework.security.authentication.UsernamePasswordAuthenticationToken; +import org.springframework.security.core.Authentication; +import org.springframework.security.core.AuthenticationException; +import org.springframework.security.core.context.SecurityContextHolder; +import org.springframework.web.filter.OncePerRequestFilter; + +import java.io.IOException; + +public class MCPAuthenticationFilter extends OncePerRequestFilter { + + private final AuthenticationManager authenticationManager; + + /** + * Instantiates a new MCP authentication filter that authenticates requests + * using username and password provided in HTTP headers. + * + * @param authenticationManager the authentication manager used to validate credentials + */ + public MCPAuthenticationFilter(AuthenticationManager authenticationManager) { + this.authenticationManager = authenticationManager; + } + + @Override + protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain) + throws IOException, ServletException { + + Authentication existingAuth = SecurityContextHolder.getContext().getAuthentication(); + if (existingAuth != null && existingAuth.isAuthenticated()) { + chain.doFilter(request, response); + return; + } + Review Comment: The authentication check at line 54 allows already authenticated requests to bypass the authentication process, but this creates a security issue when combined with how MCP endpoints are configured. The WebSecurityConfig adds MCP endpoints to both the ignore list (lines 109-115) and csrf-ignore list (lines 126-132), which means these endpoints bypass Spring Security entirely. This makes the MCPAuthenticationFilter unreachable for MCP endpoint requests. Either remove MCP endpoints from the ignore list in WebSecurityConfig, or remove this early return check so authentication is always enforced when the filter is enabled. ```suggestion ``` ########## console/src/main/java/org/apache/seata/console/config/MCPFiltersConfig.java: ########## @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.seata.console.config; + +import org.apache.seata.console.filter.MCPAuthenticationFilter; +import org.apache.seata.mcp.core.props.MCPProperties; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.core.Ordered; +import org.springframework.security.authentication.AuthenticationManager; + +@Configuration +public class MCPFiltersConfig { + + @Autowired + private AuthenticationManager authenticationManager; + + @Autowired + private MCPProperties mcpProperties; + + private static final Logger LOGGER = LoggerFactory.getLogger(MCPFiltersConfig.class); + + @Bean + public FilterRegistrationBean<MCPAuthenticationFilter> mcpAuthenticationFilterRegistration() { + + MCPAuthenticationFilter mcpAuthenticationFilter = new MCPAuthenticationFilter(authenticationManager); + + FilterRegistrationBean<MCPAuthenticationFilter> registration = new FilterRegistrationBean<>(); + + registration.setFilter(mcpAuthenticationFilter); + + for (String endPoint : mcpProperties.getEndpoints()) { + registration.addUrlPatterns(endPoint); + } + + registration.setName("mcpJwtAuthenticationTokenFilter"); Review Comment: The filter registration name 'mcpJwtAuthenticationTokenFilter' is misleading because this filter does not use JWT tokens. It uses basic username/password authentication from HTTP headers. The name should be 'mcpAuthenticationFilter' to accurately reflect its purpose and avoid confusion. ```suggestion registration.setName("mcpAuthenticationFilter"); ``` ########## console/src/main/java/org/apache/seata/console/config/WebSecurityConfig.java: ########## @@ -98,7 +106,13 @@ public AuthenticationManager authenticationManager() { @Bean public WebSecurityCustomizer webSecurityCustomizer() { - RequestMatcher[] ignoredMatchers = buildAntMatchers(env.getProperty("seata.security.ignore.urls", "/**")); + StringBuilder ignoreURLsBuilder = new StringBuilder(ignoreURLs); + List<String> mcpEndpoints = mcpProperties.getEndpoints(); + for (String endpoint : mcpEndpoints) { + ignoreURLsBuilder.append(",").append(endpoint); + } + RequestMatcher[] ignoredMatchers = + buildAntMatchers(ignoreURLsBuilder.toString().trim()); Review Comment: The MCP endpoints are being added to the security ignore list, which means Spring Security will completely bypass authentication for these endpoints. This conflicts with the purpose of MCPAuthenticationFilter which is meant to authenticate MCP requests. When endpoints are in the ignore list, they never reach any security filters. The MCPAuthenticationFilter registered in MCPFiltersConfig will not be invoked for these URLs. Remove the code that adds MCP endpoints to the ignore list if authentication is required for these endpoints. ```suggestion RequestMatcher[] ignoredMatchers = buildAntMatchers(ignoreURLs.trim()); ``` ########## console/src/main/java/org/apache/seata/console/config/MCPFiltersConfig.java: ########## @@ -0,0 +1,66 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You 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.apache.seata.console.config; + +import org.apache.seata.console.filter.MCPAuthenticationFilter; +import org.apache.seata.mcp.core.props.MCPProperties; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.web.servlet.FilterRegistrationBean; +import org.springframework.context.annotation.Bean; +import org.springframework.context.annotation.Configuration; +import org.springframework.core.Ordered; +import org.springframework.security.authentication.AuthenticationManager; + +@Configuration +public class MCPFiltersConfig { + + @Autowired + private AuthenticationManager authenticationManager; + + @Autowired + private MCPProperties mcpProperties; + + private static final Logger LOGGER = LoggerFactory.getLogger(MCPFiltersConfig.class); + + @Bean + public FilterRegistrationBean<MCPAuthenticationFilter> mcpAuthenticationFilterRegistration() { + + MCPAuthenticationFilter mcpAuthenticationFilter = new MCPAuthenticationFilter(authenticationManager); + + FilterRegistrationBean<MCPAuthenticationFilter> registration = new FilterRegistrationBean<>(); + + registration.setFilter(mcpAuthenticationFilter); + + for (String endPoint : mcpProperties.getEndpoints()) { + registration.addUrlPatterns(endPoint); + } + + registration.setName("mcpJwtAuthenticationTokenFilter"); + + registration.setOrder(Ordered.HIGHEST_PRECEDENCE); + + if (!mcpProperties.isEnableAuth()) + LOGGER.warn( + "The AUTH VERIFICATION of the [MCP server] is not enabled, please ensure that it is enabled as much as possible to avoid security problems"); Review Comment: The warning message suggests enabling authentication "as much as possible to avoid security problems", but this phrasing is unclear and unprofessional. The message should clearly state that disabling authentication creates a security risk. Consider rewording to: "MCP server authentication is disabled. This creates a security risk. It is strongly recommended to enable authentication by setting seata.mcp.auth.enabled=true" ```suggestion "MCP server authentication is disabled. This creates a security risk. It is strongly recommended to enable authentication by setting seata.mcp.auth.enabled=true"); ``` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
