This is an automated email from the ASF dual-hosted git repository.
wy471x pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/shenyu.git
The following commit(s) were added to refs/heads/master by this push:
new 4837a182e4 fix(admin): reject cross-namespace discovery ids on proxy
selector create (#7514)
4837a182e4 is described below
commit 4837a182e45bb2845e0d3f71e3367f87883a09de
Author: Sean-Walker0 <[email protected]>
AuthorDate: Sat Oct 10 21:56:45 2026 +0800
fix(admin): reject cross-namespace discovery ids on proxy selector create
(#7514)
* fix(admin): reject cross-namespace discovery ids on proxy selector create
create() loaded the nested discovery by id only and checked existence
by turning it into fillDiscovery=false, so a proxy selector could bind
to a discovery belonging to another namespace, and an unknown id was
silently ignored, leaving a selector with no discovery wiring.
Validate that the discovery exists and belongs to the target
namespace before using it; both failures now reject the create with a
ValidFailException inside the transaction. Regression tests cover the
cross-namespace id, the unknown id (both red on the previous code,
which accepted them), and the same-namespace success path.
Fixes #7467
* fix(admin): choose the processor from the referenced discovery's stored
type
Review follow-up: when an existing discovery id is referenced, the
discovery processor was still chosen from the request payload before
the row was loaded, so a caller could declare a different
discoveryType than the stored one and the create flow ran against the
wrong processor. The referenced row is now the source of truth.
Also documents the rejection paths more strongly
(verifyNoInteractions on the handler/rel mappers) and asserts the
accept path wires the handler to the referenced discovery id.
* fix(admin): reject cross-namespace discovery ids on proxy selector update
Follow-up to the create-path fix, as flagged in review: update()
loads the full rel -> handler -> discovery chain but never compared
the discovery's namespace with the request namespace, so an update
request for one namespace could rewrite another namespace's discovery
(serverList, props) and handler wiring.
Assert the loaded discovery belongs to the request namespace before
any write, mirroring create(). Regression tests cover the
cross-namespace rejection (red before this commit) and the
same-namespace success path.
---------
Co-authored-by: Sean-Walker0
<[email protected]>
---
.../service/impl/ProxySelectorServiceImpl.java | 13 +-
.../service/impl/ProxySelectorServiceImplTest.java | 181 +++++++++++++++++++++
2 files changed, 192 insertions(+), 2 deletions(-)
diff --git
a/shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/ProxySelectorServiceImpl.java
b/shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/ProxySelectorServiceImpl.java
index 03462919f6..9e47973beb 100644
---
a/shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/ProxySelectorServiceImpl.java
+++
b/shenyu-admin/src/main/java/org/apache/shenyu/admin/service/impl/ProxySelectorServiceImpl.java
@@ -243,18 +243,25 @@ public class ProxySelectorServiceImpl implements
ProxySelectorService {
ProxySelectorDO proxySelectorDO =
ProxySelectorDO.buildProxySelectorDO(proxySelectorAddDTO);
String proxySelectorId = proxySelectorDO.getId();
if (proxySelectorMapper.insert(proxySelectorDO) > 0) {
- DiscoveryProcessor discoveryProcessor =
discoveryProcessorHolder.chooseProcessor(proxySelectorAddDTO.getDiscovery().getDiscoveryType());
+ DiscoveryProcessor discoveryProcessor;
DiscoveryDO discoveryDO;
String discoveryId;
boolean fillDiscovery;
if
(StringUtils.hasLength(proxySelectorAddDTO.getDiscovery().getId())) {
discoveryDO =
discoveryMapper.selectById(proxySelectorAddDTO.getDiscovery().getId());
+ Assert.notNull(discoveryDO, "Discovery does not exist: " +
proxySelectorAddDTO.getDiscovery().getId());
+ Assert.isTrue(Objects.equals(discoveryDO.getNamespaceId(),
proxySelectorAddDTO.getNamespaceId()),
+ "Discovery does not belong to namespace: " +
proxySelectorAddDTO.getNamespaceId());
discoveryId = proxySelectorAddDTO.getDiscovery().getId();
- fillDiscovery = Objects.nonNull(discoveryDO);
+ fillDiscovery = true;
+ // the stored discovery is the source of truth for the
processor type; the
+ // payload could otherwise declare a different type than the
referenced row
+ discoveryProcessor =
discoveryProcessorHolder.chooseProcessor(discoveryDO.getDiscoveryType());
} else {
discoveryId = UUIDUtils.getInstance().generateShortUuid();
discoveryDO = buildDiscovery(proxySelectorAddDTO, currentTime,
discoveryId);
fillDiscovery = discoveryMapper.insertSelective(discoveryDO) >
0;
+ discoveryProcessor =
discoveryProcessorHolder.chooseProcessor(proxySelectorAddDTO.getDiscovery().getDiscoveryType());
discoveryProcessor.createDiscovery(discoveryDO);
}
if (fillDiscovery) {
@@ -395,6 +402,8 @@ public class ProxySelectorServiceImpl implements
ProxySelectorService {
Assert.notNull(discoveryHandlerDO, "Discovery handler does not exist:
" + discoveryHandlerId);
DiscoveryDO discoveryDO =
discoveryMapper.selectById(discoveryHandlerDO.getDiscoveryId());
Assert.notNull(discoveryDO, "Discovery does not exist: " +
discoveryHandlerDO.getDiscoveryId());
+ Assert.isTrue(Objects.equals(discoveryDO.getNamespaceId(),
proxySelectorAddDTO.getNamespaceId()),
+ "Discovery does not belong to namespace: " +
proxySelectorAddDTO.getNamespaceId());
// Validate all related records before performing any update.
proxySelectorMapper.update(proxySelectorDO);
// update discovery handler
diff --git
a/shenyu-admin/src/test/java/org/apache/shenyu/admin/service/impl/ProxySelectorServiceImplTest.java
b/shenyu-admin/src/test/java/org/apache/shenyu/admin/service/impl/ProxySelectorServiceImplTest.java
new file mode 100644
index 0000000000..749318abb4
--- /dev/null
+++
b/shenyu-admin/src/test/java/org/apache/shenyu/admin/service/impl/ProxySelectorServiceImplTest.java
@@ -0,0 +1,181 @@
+/*
+ * 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.shenyu.admin.service.impl;
+
+import org.apache.shenyu.admin.discovery.DiscoveryProcessor;
+import org.apache.shenyu.admin.discovery.DiscoveryProcessorHolder;
+import org.apache.shenyu.admin.exception.ValidFailException;
+import org.apache.shenyu.admin.mapper.DiscoveryHandlerMapper;
+import org.apache.shenyu.admin.mapper.DiscoveryMapper;
+import org.apache.shenyu.admin.mapper.DiscoveryRelMapper;
+import org.apache.shenyu.admin.mapper.DiscoveryUpstreamMapper;
+import org.apache.shenyu.admin.mapper.ProxySelectorMapper;
+import org.apache.shenyu.admin.model.dto.ProxySelectorAddDTO;
+import org.apache.shenyu.admin.model.entity.DiscoveryDO;
+import org.apache.shenyu.admin.model.entity.DiscoveryHandlerDO;
+import org.apache.shenyu.admin.model.entity.DiscoveryRelDO;
+import org.apache.shenyu.admin.utils.ShenyuResultMessage;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyString;
+import org.mockito.ArgumentCaptor;
+import static org.mockito.Mockito.lenient;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoInteractions;
+import static org.mockito.Mockito.when;
+
+/**
+ * Test case for {@link ProxySelectorServiceImpl} namespace validation on
create.
+ */
+@ExtendWith(MockitoExtension.class)
+public final class ProxySelectorServiceImplTest {
+
+ @Mock
+ private ProxySelectorMapper proxySelectorMapper;
+
+ @Mock
+ private DiscoveryMapper discoveryMapper;
+
+ @Mock
+ private DiscoveryRelMapper discoveryRelMapper;
+
+ @Mock
+ private DiscoveryUpstreamMapper discoveryUpstreamMapper;
+
+ @Mock
+ private DiscoveryHandlerMapper discoveryHandlerMapper;
+
+ @Mock
+ private DiscoveryProcessorHolder discoveryProcessorHolder;
+
+ @Mock
+ private DiscoveryProcessor discoveryProcessor;
+
+ @InjectMocks
+ private ProxySelectorServiceImpl proxySelectorService;
+
+ @Test
+ public void createRejectsDiscoveryFromAnotherNamespace() {
+ final ProxySelectorAddDTO dto = buildAddDto("namespace-a",
"discovery-1");
+ DiscoveryDO foreignDiscovery = new DiscoveryDO();
+ foreignDiscovery.setId("discovery-1");
+ foreignDiscovery.setNamespaceId("namespace-b");
+ foreignDiscovery.setDiscoveryType("zookeeper");
+ when(proxySelectorMapper.insert(any())).thenReturn(1);
+
when(discoveryMapper.selectById("discovery-1")).thenReturn(foreignDiscovery);
+
+ assertThrows(ValidFailException.class, () ->
proxySelectorService.create(dto));
+ verifyNoInteractions(discoveryHandlerMapper, discoveryRelMapper);
+ }
+
+ @Test
+ public void createRejectsUnknownDiscoveryId() {
+ final ProxySelectorAddDTO dto = buildAddDto("namespace-a",
"missing-discovery");
+ when(proxySelectorMapper.insert(any())).thenReturn(1);
+ when(discoveryMapper.selectById("missing-discovery")).thenReturn(null);
+
+ assertThrows(ValidFailException.class, () ->
proxySelectorService.create(dto));
+ verifyNoInteractions(discoveryHandlerMapper, discoveryRelMapper);
+ }
+
+ @Test
+ public void createAcceptsDiscoveryFromSameNamespace() {
+ final ProxySelectorAddDTO dto = buildAddDto("namespace-a",
"discovery-1");
+ DiscoveryDO ownDiscovery = new DiscoveryDO();
+ ownDiscovery.setId("discovery-1");
+ ownDiscovery.setNamespaceId("namespace-a");
+ ownDiscovery.setDiscoveryType("zookeeper");
+ when(proxySelectorMapper.insert(any())).thenReturn(1);
+
when(discoveryMapper.selectById("discovery-1")).thenReturn(ownDiscovery);
+
lenient().when(discoveryProcessorHolder.chooseProcessor(anyString())).thenReturn(discoveryProcessor);
+
lenient().when(discoveryHandlerMapper.insertSelective(any())).thenReturn(1);
+
lenient().when(discoveryRelMapper.insertSelective(any())).thenReturn(1);
+
+ assertEquals(ShenyuResultMessage.CREATE_SUCCESS,
proxySelectorService.create(dto));
+ ArgumentCaptor<DiscoveryHandlerDO> handlerCaptor =
ArgumentCaptor.forClass(DiscoveryHandlerDO.class);
+
verify(discoveryHandlerMapper).insertSelective(handlerCaptor.capture());
+ assertEquals("discovery-1", handlerCaptor.getValue().getDiscoveryId());
+ }
+
+ private ProxySelectorAddDTO buildAddDto(final String namespaceId, final
String discoveryId) {
+ ProxySelectorAddDTO dto = new ProxySelectorAddDTO();
+ dto.setName("test-proxy-selector");
+ dto.setPluginName("tcp");
+ dto.setNamespaceId(namespaceId);
+ dto.setForwardPort(9295);
+ dto.setListenerNode("/shenyu/discovery");
+ dto.setProps("{}");
+ dto.setHandler("");
+ ProxySelectorAddDTO.Discovery discovery = new
ProxySelectorAddDTO.Discovery();
+ discovery.setId(discoveryId);
+ discovery.setDiscoveryType("zookeeper");
+ dto.setDiscovery(discovery);
+ return dto;
+ }
+
+ @Test
+ public void updateRejectsDiscoveryFromAnotherNamespace() {
+ final ProxySelectorAddDTO dto = buildAddDto("namespace-a",
"discovery-1");
+ final DiscoveryRelDO relDO = new DiscoveryRelDO();
+ relDO.setDiscoveryHandlerId("handler-1");
+ final DiscoveryHandlerDO handlerDO = new DiscoveryHandlerDO();
+ handlerDO.setId("handler-1");
+ handlerDO.setDiscoveryId("discovery-1");
+ final DiscoveryDO foreignDiscovery = new DiscoveryDO();
+ foreignDiscovery.setId("discovery-1");
+ foreignDiscovery.setNamespaceId("namespace-b");
+ foreignDiscovery.setDiscoveryType("zookeeper");
+
when(discoveryRelMapper.selectByProxySelectorId(any())).thenReturn(relDO);
+
when(discoveryHandlerMapper.selectById("handler-1")).thenReturn(handlerDO);
+
when(discoveryMapper.selectById("discovery-1")).thenReturn(foreignDiscovery);
+
+ assertThrows(ValidFailException.class, () ->
proxySelectorService.update(dto));
+ verify(discoveryMapper,
never()).updateSelective(any(DiscoveryDO.class));
+ }
+
+ @Test
+ public void updateAcceptsDiscoveryFromSameNamespace() {
+ final ProxySelectorAddDTO dto = buildAddDto("namespace-a",
"discovery-1");
+ final DiscoveryRelDO relDO = new DiscoveryRelDO();
+ relDO.setDiscoveryHandlerId("handler-1");
+ final DiscoveryHandlerDO handlerDO = new DiscoveryHandlerDO();
+ handlerDO.setId("handler-1");
+ handlerDO.setDiscoveryId("discovery-1");
+ final DiscoveryDO ownDiscovery = new DiscoveryDO();
+ ownDiscovery.setId("discovery-1");
+ ownDiscovery.setNamespaceId("namespace-a");
+ ownDiscovery.setDiscoveryType("zookeeper");
+
when(discoveryRelMapper.selectByProxySelectorId(any())).thenReturn(relDO);
+
when(discoveryHandlerMapper.selectById("handler-1")).thenReturn(handlerDO);
+
when(discoveryMapper.selectById("discovery-1")).thenReturn(ownDiscovery);
+
lenient().when(discoveryProcessorHolder.chooseProcessor(anyString())).thenReturn(discoveryProcessor);
+ lenient().when(proxySelectorMapper.update(any())).thenReturn(1);
+
lenient().when(discoveryHandlerMapper.updateSelective(any())).thenReturn(1);
+ lenient().when(discoveryMapper.updateSelective(any())).thenReturn(1);
+
+ assertEquals(ShenyuResultMessage.UPDATE_SUCCESS,
proxySelectorService.update(dto));
+ }
+}
