Copilot commented on code in PR #8032:
URL: https://github.com/apache/incubator-seata/pull/8032#discussion_r3328723964
##########
console/src/main/java/org/apache/seata/console/config/WebSecurityConfig.java:
##########
@@ -136,6 +141,9 @@ public SecurityFilterChain securityFilterChain(HttpSecurity
http, Authentication
})
.addFilterBefore(
new JwtAuthenticationTokenFilter(tokenProvider),
UsernamePasswordAuthenticationFilter.class)
+ .addFilterBefore(
+ new
MCPBusinessDataSourceFilter(businessDataSourcesProperties),
+ JwtAuthenticationTokenFilter.class)
Review Comment:
The `MCPBusinessDataSourceFilter` is registered before
`JwtAuthenticationTokenFilter` in the security chain, and
`OncePerRequestFilter` applies it to every request. As a result, any
unauthenticated client can register a new business data source by sending an
`X-DB-Config` header to any URL on the console (including ignored URLs). This
is a remote code path that opens arbitrary JDBC connections to
attacker-controlled hosts and exposes credentials in MCP queries. The filter
should be restricted to only the MCP endpoints (e.g. via `shouldNotFilter`
checking the configured MCP paths) and should require an authenticated
principal — register it after JwtAuthenticationTokenFilter, or perform an
authentication/authorization check inside the filter before calling
`registerDataSourceFromJson`.
##########
console/src/main/java/org/apache/seata/mcp/core/props/BusinessDataSourcesProperties.java:
##########
@@ -0,0 +1,374 @@
+/*
+ * 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 com.fasterxml.jackson.databind.JsonNode;
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.seata.common.util.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.beans.factory.InitializingBean;
+import org.springframework.core.env.ConfigurableEnvironment;
+import org.springframework.core.env.EnumerablePropertySource;
+import org.springframework.core.env.Environment;
+import org.springframework.core.env.PropertySource;
+import org.springframework.stereotype.Component;
+
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+import static org.apache.seata.common.DefaultValues.DEFAULT_DB_MAX_CONN;
+import static org.apache.seata.common.DefaultValues.DEFAULT_DB_MIN_CONN;
+
+@Component
+public class BusinessDataSourcesProperties implements InitializingBean {
+
+ public BusinessDataSourcesProperties(Environment env, ObjectMapper
objectMapper) {
+ this.env = env;
+ this.objectMapper = objectMapper;
+ }
+
+ private final Environment env;
+
+ private final ObjectMapper objectMapper;
+
+ private static final Map<String, DataSourceProperties> datasources = new
ConcurrentHashMap<>();
+
+ private static final Map<String, String> dataSourcesNamesAndResourceIds =
new ConcurrentHashMap<>();
+
+ private static final String BASE_PREFIX = "seata.businessDataSources.";
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(BusinessDataSourcesProperties.class);
+
+ @Override
+ public void afterPropertiesSet() {
+
+ Set<String> dataSourceNames = getDataSourceNames();
+
+ for (String name : dataSourceNames) {
+ DataSourceProperties props = new DataSourceProperties();
+ String prefix = BASE_PREFIX + name + ".";
+ props.setEnabled(env.getProperty(prefix + "enabled",
Boolean.class, true));
+ props.setDbType(env.getProperty(prefix + "dbType", "mysql"));
+ props.setDriverClassName(env.getProperty(prefix +
"driverClassName", "com.mysql.cj.jdbc.Driver"));
+ props.setUrl(env.getProperty(prefix + "url"));
+ props.setUsername(env.getProperty(prefix + "username"));
+ props.setPassword(env.getProperty(prefix + "password"));
+ props.setDatasource(env.getProperty(prefix + "datasource",
"druid"));
+ props.setMinConn(env.getProperty(prefix + "minConn",
Integer.class, DEFAULT_DB_MIN_CONN));
+ if (props.getMinConn() <= 0 || props.getMinConn() >
DEFAULT_DB_MIN_CONN) {
+ LOGGER.warn("The minimum number of connections for a data
source: {} is not compliant", name);
+ continue;
+ }
+ props.setMaxConn(env.getProperty(prefix + "maxConn",
Integer.class, DEFAULT_DB_MAX_CONN));
+ if (props.getMaxConn() <= 0 || props.getMaxConn() >
DEFAULT_DB_MAX_CONN) {
+ LOGGER.warn("The maximum number of connections for a data
source: {} is not compliant", name);
+ continue;
+ }
Review Comment:
This validation is incorrect. `DEFAULT_DB_MIN_CONN` is `10` (see
`common/src/main/java/org/apache/seata/common/DefaultValues.java:554`), so this
rejects any data source whose configured `minConn` exceeds the default value of
10, which is contrary to the intent (the configured value is the user's chosen
value, not a ceiling). The same logical mistake repeats below for `maxConn`
against `DEFAULT_DB_MAX_CONN`. Either remove these upper-bound checks or
compare against `props.getMaxConn()` for `minConn`, and a documented hard
ceiling for `maxConn`.
##########
console/src/main/java/org/apache/seata/mcp/store/db/AbstractMCPDataSourceProvider.java:
##########
@@ -0,0 +1,256 @@
+/*
+ * 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.store.db;
+
+import org.apache.seata.common.ConfigurationKeys;
+import org.apache.seata.common.exception.ShouldNeverHappenException;
+import org.apache.seata.common.exception.StoreException;
+import org.apache.seata.common.util.StringUtils;
+import org.apache.seata.core.constants.DBType;
+import org.apache.seata.mcp.core.props.BusinessDataSourcesProperties;
+
+import javax.sql.DataSource;
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Stream;
+
+import static org.apache.seata.common.DefaultValues.DEFAULT_DB_MAX_CONN;
+import static org.apache.seata.common.DefaultValues.DEFAULT_DB_MIN_CONN;
+
+public abstract class AbstractMCPDataSourceProvider {
+
+ private String resourceId;
+
+ protected static final Map<String,
BusinessDataSourcesProperties.DataSourceProperties> DATASOURCE_PROPERTIES =
+ BusinessDataSourcesProperties.getDatasources();
+
+ private static final String MYSQL_DRIVER_CLASS_NAME =
"com.mysql.jdbc.Driver";
+
+ private static final String MYSQL8_DRIVER_CLASS_NAME =
"com.mysql.cj.jdbc.Driver";
+
+ private static final String MYSQL_DRIVER_FILE_PREFIX = "mysql-connector-j";
+
+ private static final Map<String, ClassLoader> DRIVER_LOADERS;
+
+ private static final long DEFAULT_DB_MAX_WAIT = 5000;
+
+ static {
+ DRIVER_LOADERS = createMysqlDriverClassLoaders();
+ }
+
+ public DataSource generate() {
+ return doGenerate();
+ }
+
+ public DataSource generateByResourceId(String resourceId) {
+ this.resourceId = resourceId;
+ return generate();
+ }
Review Comment:
The base class defines a `validate()` method that loads the JDBC driver
class and produces a clear error message when the driver cannot be found,
mirroring the prior art at
`core/src/main/java/org/apache/seata/core/store/db/AbstractDataSourceProvider.java:83-123`
where `generate()` is `validate(); return doGenerate();`. Here, `generate()`
only calls `doGenerate()`, so `validate()` is dead code and the carefully built
error path is never executed — driver loading failures will surface as opaque
pool initialization errors instead. Call `validate()` from `generate()` (or
from `generateByResourceId`) before delegating to `doGenerate()`.
##########
console/src/main/java/org/apache/seata/console/filter/MCPBusinessDataSourceFilter.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.mcp.core.props.BusinessDataSourcesProperties;
+import org.springframework.http.HttpStatus;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class MCPBusinessDataSourceFilter extends OncePerRequestFilter {
+
+ private final BusinessDataSourcesProperties businessDataSourcesProperties;
+
+ private final Set<String> processedConfigs = ConcurrentHashMap.newKeySet();
+
+ public MCPBusinessDataSourceFilter(BusinessDataSourcesProperties
properties) {
+ this.businessDataSourcesProperties = properties;
+ }
+
+ @Override
+ protected void doFilterInternal(HttpServletRequest request,
HttpServletResponse response, FilterChain filterChain)
+ throws ServletException, IOException {
+ String combinedHeader = request.getHeader("X-DB-Config");
+ if (combinedHeader != null && !combinedHeader.isEmpty()) {
+ String[] jsonConfigs = combinedHeader.split(";");
+ for (String jsonDBConfig : jsonConfigs) {
+ if (processedConfigs.contains(jsonDBConfig.trim())) {
+ continue;
+ }
+ try {
+
businessDataSourcesProperties.registerDataSourceFromJson(jsonDBConfig.trim());
+ processedConfigs.add(jsonDBConfig.trim());
Review Comment:
`processedConfigs` is an unbounded `Set` keyed by the raw JSON header
string. Because the filter is reachable on every request and adds entries when
registration succeeds (and never evicts), an attacker — or even normal use with
rotating credentials/whitespace differences — can grow this set without limit,
leaking memory. Additionally, since the same
`BusinessDataSourcesProperties.datasources` static map is also unbounded and
overwritten by URL-derived `resourceId`, repeated registrations can silently
replace existing entries. Consider keying the dedup cache on `resourceId`,
bounding its size, and/or scoping it per-authenticated-user.
##########
console/src/main/java/org/apache/seata/mcp/store/SqlExecutionTemplate.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.store;
+
+import org.apache.seata.common.exception.StoreException;
+import org.apache.seata.common.util.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.stereotype.Service;
+
+import javax.sql.DataSource;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+@Service
+public class SqlExecutionTemplate {
+
+ private static final Pattern SELECT_PATTERN =
+ Pattern.compile("^\\s*SELECT\\b.*", Pattern.CASE_INSENSITIVE |
Pattern.DOTALL);
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(SqlExecutionTemplate.class);
+
+ private DataSource getDataSource(String resourceId) {
+ try {
+ return DataSourceFactory.getDataSource(resourceId);
+ } catch (Exception e) {
+ LOGGER.error("Failed to get the data source, resourceId: {}",
resourceId, e);
+ throw new StoreException("Unable to get the data source: " +
resourceId);
+ }
+ }
+
+ private boolean validateQuerySql(String sql) {
+ if (sql == null || StringUtils.isBlank(sql)) {
+ return false;
+ }
+ return SELECT_PATTERN.matcher(sql).matches();
+ }
+
+ public List<Map<String, Object>> query(String resourceId, String sql,
Object... params) {
+ Connection conn = null;
+ PreparedStatement ps = null;
+ ResultSet rs = null;
+
+ try {
+ if (!validateQuerySql(sql)) {
+ throw new StoreException("The query valid failed,Only query
operations are allowed：" + sql);
+ }
+ conn = getConnection(resourceId);
+ if (params == null || params.length == 0) {
+ if ((sql.contains("where") || sql.contains("WHERE"))) {
+ throw new StoreException(
+ "Query contains WHERE clause but no parameters
were provided. This may lead to unintended full table scans and is not
allowed.");
+ }
+ }
Review Comment:
`runSql` calls `sqlExecutionTemplate.query(resourceId, sql)` with no
parameters. Inside `SqlExecutionTemplate.query`, when `params` is empty and the
SQL contains `where`/`WHERE`, a `StoreException` is thrown. This means the only
user-facing SQL execution tool can never run any query containing a `WHERE`
clause, which defeats the purpose of the tool. Additionally,
`sql.contains("where")` is a naïve substring check that also matches
identifiers like `somewhere` or `nowhere` in column/table names or string
literals. Reconsider this guard (e.g. drop the “must have parameters” rule for
ad‑hoc queries, or implement a real SQL parser-based check).
##########
namingserver/src/main/resources/application.yml:
##########
@@ -60,6 +60,39 @@ seata:
csrf-ignore-urls: /naming/v1/**,/api/v1/naming/**
ignore:
urls:
/,/**/*.css,/**/*.js,/**/*.html,/**/*.map,/**/*.svg,/**/*.png,/**/*.jpeg,/**/*.ico,/api/v1/auth/login,/version.json,/naming/v1/health,/error
+
+ # MCP Server Configuration
+ mcp:
+ # Maximum query time interval, The unit is milliseconds, Default one day
+ query:
+ max-query-duration: 86400000
+
+ # Business data source configuration
+ businessDataSources:
+ dataSource1:
+ # Whether this data source is enabled
+ enabled: true
+ dbType: mysql
+ driverClassName: com.mysql.cj.jdbc.Driver # Currently, only mysql is
supported
+ url:
+ username: root
+ password: Il02mayi
Review Comment:
The `dataSource1` block contains what appears to be a real database password
(`Il02mayi`) committed to the repository. Even if this is a test/local
password, shipping credentials in the default `application.yml` is a security
anti-pattern and may be flagged by secret scanners. Replace with a placeholder
such as an empty string or a clearly fake value (e.g. `password:
<your-password>`), and document how to override via environment variables or
external configuration. Also note that the `url:` field is intentionally empty
— the data source will fail validation at startup, which is fine, but the
example config shouldn't ship with a populated password while the URL is blank.
##########
console/src/main/java/org/apache/seata/mcp/store/DataSourceFactory.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.store;
+
+import jakarta.annotation.PostConstruct;
+import org.apache.seata.common.exception.StoreException;
+import org.apache.seata.mcp.core.props.BusinessDataSourcesProperties;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.stereotype.Component;
+
+import javax.sql.DataSource;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+@Component
+public class DataSourceFactory {
+
+ private static final Map<String, DataSource> dataSourceMap = new
ConcurrentHashMap<>();
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(DataSourceFactory.class);
+
+ @PostConstruct
+ public void init() {
+ DataSourceFactory.initAllDataSources();
+ }
+
+ public static void initAllDataSources() {
+ Map<String, BusinessDataSourcesProperties.DataSourceProperties>
datasources =
+ BusinessDataSourcesProperties.getDatasources();
+ datasources.forEach(
+ (resourceId, props) ->
dataSourceMap.computeIfAbsent(resourceId, key -> createDataSource(props, key)));
+ }
Review Comment:
`dataSourceMap` is a `static` field but is populated via the instance
lifecycle hook `@PostConstruct`. Combined with the static lookup methods in
`BusinessDataSourcesProperties` and `DataSourceFactory`, this couples Spring
lifecycle to global state and makes it impossible to test (or to have a second
`BusinessDataSourcesProperties` bean). It also means data sources opened here
are never closed on context shutdown — there is no `@PreDestroy` calling
`((Closeable) ds).close()` for each pool, so reload/restart leaks pool threads
and connections. Consider making the factory a regular bean managed by Spring
with proper destroy semantics, or at minimum add a `@PreDestroy` that closes
all entries.
##########
console/src/main/java/org/apache/seata/mcp/tools/BusinessDataSourceTools.java:
##########
@@ -0,0 +1,80 @@
+/*
+ * 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.tools;
+
+import org.apache.seata.mcp.core.props.BusinessDataSourcesProperties;
+import org.apache.seata.mcp.service.BusinessDataSourceService;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springaicommunity.mcp.annotation.McpTool;
+import org.springaicommunity.mcp.annotation.McpToolParam;
+import org.springframework.stereotype.Service;
+
+import java.util.Collections;
+import java.util.List;
+import java.util.Map;
+
+@Service
+public class BusinessDataSourceTools {
+
+ private final BusinessDataSourceService dataSourceService;
+
+ public BusinessDataSourceTools(BusinessDataSourceService
dataSourceService) {
+ this.dataSourceService = dataSourceService;
+ }
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(BusinessDataSourceTools.class);
+
+ @McpTool(
+ description =
+ "Get the identity and name of the business data source.
Important!!!: key is name, value is resourceId")
+ public Map<String, String> getResourceIds() {
+ LOGGER.info("User try to get resource ids");
+ return
BusinessDataSourcesProperties.getDataSourcesNamesAndResourceIds();
+ }
+
+ @McpTool(description = "Get all available table names")
+ public List<String> getTableNames(
+ @McpToolParam(description = "The identity of the data source,
start with jdbc://", required = true)
+ String resourceId) {
+ LOGGER.info("User try to get all table names, resource id {}",
resourceId);
+ return dataSourceService.getTableNamesBySchema(resourceId);
+ }
+
+ @McpTool(description = "Obtained by table nameSchema")
+ public List<Map<String, Object>> getTableSchema(
+ @McpToolParam(description = "Table Name") String tableName,
+ @McpToolParam(description = "The identity of the data source,
start with jdbc://", required = true)
+ String resourceId) {
+ LOGGER.info("User try to get table schema, tableName: {}, resourceId:
{}", tableName, resourceId);
+ return dataSourceService.getTableSchemaByTableName(resourceId,
tableName);
+ }
+
+ @McpTool(description = "Execute the SQL query result, It can only be used
to query business data!!!")
Review Comment:
The MCP tool description text is grammatically broken and will be sent
verbatim to LLMs as the tool's description, which can degrade tool selection
accuracy. "Obtained by table nameSchema" looks like a translation artifact —
consider e.g. `"Get the schema (columns) of a table by its name"`. Similarly on
line 67, `"Execute the SQL query result, It can only be used to query business
data!!!"` is awkward; consider `"Execute a SQL SELECT query against a business
data source. Read-only: only SELECT queries are allowed."`.
##########
console/src/main/java/org/apache/seata/mcp/store/SqlExecutionTemplate.java:
##########
@@ -0,0 +1,154 @@
+/*
+ * 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.store;
+
+import org.apache.seata.common.exception.StoreException;
+import org.apache.seata.common.util.StringUtils;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.stereotype.Service;
+
+import javax.sql.DataSource;
+import java.sql.Connection;
+import java.sql.PreparedStatement;
+import java.sql.ResultSet;
+import java.sql.ResultSetMetaData;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.regex.Pattern;
+
+@Service
+public class SqlExecutionTemplate {
+
+ private static final Pattern SELECT_PATTERN =
+ Pattern.compile("^\\s*SELECT\\b.*", Pattern.CASE_INSENSITIVE |
Pattern.DOTALL);
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(SqlExecutionTemplate.class);
+
+ private DataSource getDataSource(String resourceId) {
+ try {
+ return DataSourceFactory.getDataSource(resourceId);
+ } catch (Exception e) {
+ LOGGER.error("Failed to get the data source, resourceId: {}",
resourceId, e);
+ throw new StoreException("Unable to get the data source: " +
resourceId);
+ }
+ }
+
+ private boolean validateQuerySql(String sql) {
+ if (sql == null || StringUtils.isBlank(sql)) {
+ return false;
+ }
+ return SELECT_PATTERN.matcher(sql).matches();
+ }
+
+ public List<Map<String, Object>> query(String resourceId, String sql,
Object... params) {
+ Connection conn = null;
+ PreparedStatement ps = null;
+ ResultSet rs = null;
+
+ try {
+ if (!validateQuerySql(sql)) {
+ throw new StoreException("The query valid failed,Only query
operations are allowed：" + sql);
+ }
+ conn = getConnection(resourceId);
+ if (params == null || params.length == 0) {
+ if ((sql.contains("where") || sql.contains("WHERE"))) {
Review Comment:
The naïve `sql.contains("where") || sql.contains("WHERE")` check produces
false positives for any query whose column/table name or literal contains the
substring `where` (e.g. `nowhere`, `somewhere`, ``where_clause`` as a column).
Combined with the requirement that any matched query must also have parameters,
this can both wrongly block legitimate queries and (by being trivially
bypassable with `WhErE`) fail to block intended ones. If the goal is to prevent
unbounded scans, use a real SQL parser or remove this constraint.
##########
console/src/main/java/org/apache/seata/mcp/store/DataSourceFactory.java:
##########
@@ -0,0 +1,88 @@
+/*
+ * 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.store;
+
+import jakarta.annotation.PostConstruct;
+import org.apache.seata.common.exception.StoreException;
+import org.apache.seata.mcp.core.props.BusinessDataSourcesProperties;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+import org.springframework.stereotype.Component;
+
+import javax.sql.DataSource;
+import java.util.Map;
+import java.util.concurrent.ConcurrentHashMap;
+
+@Component
+public class DataSourceFactory {
+
+ private static final Map<String, DataSource> dataSourceMap = new
ConcurrentHashMap<>();
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(DataSourceFactory.class);
+
+ @PostConstruct
+ public void init() {
+ DataSourceFactory.initAllDataSources();
+ }
+
+ public static void initAllDataSources() {
+ Map<String, BusinessDataSourcesProperties.DataSourceProperties>
datasources =
+ BusinessDataSourcesProperties.getDatasources();
+ datasources.forEach(
+ (resourceId, props) ->
dataSourceMap.computeIfAbsent(resourceId, key -> createDataSource(props, key)));
+ }
+
+ public static DataSource getDataSource(String resourceId) {
+ return dataSourceMap.computeIfAbsent(resourceId, key -> {
+ BusinessDataSourcesProperties.DataSourceProperties props =
+ BusinessDataSourcesProperties.getDatasources().get(key);
+ if (props == null) {
+ throw new StoreException("Cannot find datasource properties: "
+ key);
+ }
+ return createDataSource(props, key);
+ });
+ }
+
+ public static void removeErrorDataSource(String resourceId, Exception e) {
+ dataSourceMap.remove(resourceId);
+ LOGGER.info("Delete Business DataSource, resourceId: {}", resourceId);
+ throw new StoreException(
+ "The Business DataSource: " + resourceId + " can't be
connected due to: " + e.getMessage());
+ }
+
+ public static DataSource createDataSource(
+ BusinessDataSourcesProperties.DataSourceProperties
dataSourceProperties, String resourceId) {
+ if (dataSourceProperties == null) {
+ throw new StoreException("Cannot find datasource properties:" +
dataSourceProperties);
+ }
Review Comment:
`dataSourceProperties` was just checked to be `null`, so this error message
will always render as `"Cannot find datasource properties:null"`, omitting the
requested resource id. Use `resourceId` in the message instead.
##########
console/src/main/java/org/apache/seata/mcp/store/db/AbstractMCPDataSourceProvider.java:
##########
@@ -0,0 +1,256 @@
+/*
+ * 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.store.db;
+
+import org.apache.seata.common.ConfigurationKeys;
+import org.apache.seata.common.exception.ShouldNeverHappenException;
+import org.apache.seata.common.exception.StoreException;
+import org.apache.seata.common.util.StringUtils;
+import org.apache.seata.core.constants.DBType;
+import org.apache.seata.mcp.core.props.BusinessDataSourcesProperties;
+
+import javax.sql.DataSource;
+import java.io.File;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.Objects;
+import java.util.stream.Stream;
+
+import static org.apache.seata.common.DefaultValues.DEFAULT_DB_MAX_CONN;
+import static org.apache.seata.common.DefaultValues.DEFAULT_DB_MIN_CONN;
+
+public abstract class AbstractMCPDataSourceProvider {
+
+ private String resourceId;
+
+ protected static final Map<String,
BusinessDataSourcesProperties.DataSourceProperties> DATASOURCE_PROPERTIES =
+ BusinessDataSourcesProperties.getDatasources();
Review Comment:
`DATASOURCE_PROPERTIES` is initialized at class-load time from
`BusinessDataSourcesProperties.getDatasources()`. Because Java captures the
reference once, this works only because the underlying static map happens to be
the same `ConcurrentHashMap` instance — which it is — but the design is
fragile: any future refactor that returns a copy will silently break dynamic
lookups. More importantly, this assumes
`BusinessDataSourcesProperties.afterPropertiesSet()` has run before any
provider class is loaded; with Spring's eager singleton phase that's usually
true, but the new `MCPBusinessDataSourceFilter` registers data sources at
request time via `registerDataSourceFromJson`, and providers loaded later read
from this same map — so dynamic registrations do flow through, but the
dependency is implicit. Consider injecting `BusinessDataSourcesProperties` (and
looking up the live map on each call) instead of caching a static reference.
##########
namingserver/pom.xml:
##########
@@ -173,6 +174,11 @@
<groupId>com.squareup.okhttp3</groupId>
<artifactId>okhttp</artifactId>
</dependency>
+ <dependency>
+ <groupId>mysql</groupId>
+ <artifactId>mysql-connector-java</artifactId>
+ <version>${mysql8.version}</version>
+ </dependency>
Review Comment:
`namingserver/pom.xml` adds an explicit dependency on
`mysql:mysql-connector-java:8.0.27`, but the global parent
`dependencies/pom.xml:110` manages the MySQL connector at version `5.1.42`, and
the `groupId:artifactId` for MySQL 8 is the new coordinate
`com.mysql:mysql-connector-j` (the `mysql:mysql-connector-java` GAV stops at
8.0.32 and is no longer maintained). Two issues: (1) hardcoding `8.0.27` here
bypasses the centrally managed version and creates a maintenance hazard; (2)
using the legacy GAV ties the project to an old/abandoned coordinate. Prefer
adding the version property to the parent and using the new
`com.mysql:mysql-connector-j` artifact, consistent with the driver class
`com.mysql.cj.jdbc.Driver` configured elsewhere in this PR.
##########
console/src/main/java/org/apache/seata/console/filter/MCPBusinessDataSourceFilter.java:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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.mcp.core.props.BusinessDataSourcesProperties;
+import org.springframework.http.HttpStatus;
+import org.springframework.web.filter.OncePerRequestFilter;
+
+import java.io.IOException;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+public class MCPBusinessDataSourceFilter extends OncePerRequestFilter {
+
+ private final BusinessDataSourcesProperties businessDataSourcesProperties;
+
+ private final Set<String> processedConfigs = ConcurrentHashMap.newKeySet();
+
+ public MCPBusinessDataSourceFilter(BusinessDataSourcesProperties
properties) {
+ this.businessDataSourcesProperties = properties;
+ }
+
+ @Override
+ protected void doFilterInternal(HttpServletRequest request,
HttpServletResponse response, FilterChain filterChain)
+ throws ServletException, IOException {
+ String combinedHeader = request.getHeader("X-DB-Config");
+ if (combinedHeader != null && !combinedHeader.isEmpty()) {
+ String[] jsonConfigs = combinedHeader.split(";");
+ for (String jsonDBConfig : jsonConfigs) {
+ if (processedConfigs.contains(jsonDBConfig.trim())) {
+ continue;
+ }
+ try {
+
businessDataSourcesProperties.registerDataSourceFromJson(jsonDBConfig.trim());
+ processedConfigs.add(jsonDBConfig.trim());
+ } catch (Exception e) {
+ if (!response.isCommitted()) {
+ response.sendError(
+ HttpStatus.BAD_REQUEST.value(),
+ "The business database parameter in the
request header is incorrect: "
+ + e.getMessage());
+ return;
+ }
+ }
+ }
Review Comment:
When `registerDataSourceFromJson` throws and the response is already
committed, the code falls through the `if (!response.isCommitted())` block,
skips the `return`, and then `filterChain.doFilter(...)` is invoked at line 65
with potentially partial header processing. Likewise, after a successful
`sendError`, the inner `return` only exits the enclosing method when not
committed; consider moving the `return` outside the inner `if` so any failed
registration always short-circuits the filter chain rather than continuing to
call downstream filters with a partially-valid header.
--
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]
