From aba4e752c8d14749cf8f627593cc3bbd6b8f3537 Mon Sep 17 00:00:00 2001 From: pnoker Date: Wed, 21 Oct 2026 23:53:22 +0800 Subject: [PATCH] fix(agentic): require a fresh api key when re-pointing a provider MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The update path kept the stored (write-only) API key whenever the request omitted one — including when the base URL changed, so a compromised tenant session could silently redirect the unreadable stored secret to an arbitrary public host. The update path now enforces the same rule as the connectivity probe: a changed base URL without an explicit new key is rejected. Co-Authored-By: Claude Code --- .../impl/ModelProviderServiceImpl.java | 19 +++++++-- .../impl/ModelProviderServiceImplTest.java | 40 +++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/dc3-common/dc3-common-agentic/src/main/java/io/github/pnoker/common/agentic/service/impl/ModelProviderServiceImpl.java b/dc3-common/dc3-common-agentic/src/main/java/io/github/pnoker/common/agentic/service/impl/ModelProviderServiceImpl.java index bcd693b4e..b64898d68 100644 --- a/dc3-common/dc3-common-agentic/src/main/java/io/github/pnoker/common/agentic/service/impl/ModelProviderServiceImpl.java +++ b/dc3-common/dc3-common-agentic/src/main/java/io/github/pnoker/common/agentic/service/impl/ModelProviderServiceImpl.java @@ -128,10 +128,21 @@ public class ModelProviderServiceImpl implements ModelProviderService { ? AgenticModelProviderTypeEnum.OPENAI_COMPATIBLE : source.getProviderType()); value.setBaseUrl(source.getBaseUrl().trim()); - value.setApiKey( - StringUtils.isBlank(source.getApiKey()) && existing != null - ? existing.getApiKey() - : StringUtils.defaultString(source.getApiKey())); + boolean baseUrlChanged = existing != null + && !Objects.equals(existing.getBaseUrl(), value.getBaseUrl()); + if (StringUtils.isBlank(source.getApiKey()) && existing != null && !baseUrlChanged) { + // Untouched key on an untouched endpoint: keep the stored secret so the + // write-only field does not have to round-trip through the client. + value.setApiKey(existing.getApiKey()); + } else if (StringUtils.isBlank(source.getApiKey()) && baseUrlChanged) { + // Re-pointing an existing provider without a fresh key would silently send + // the stored (write-only) secret to the new host — an exfiltration vector. + // Same rule the connectivity probe already enforces. + throw new RequestException( + "Changing the base URL requires providing a new API key for the provider"); + } else { + value.setApiKey(StringUtils.defaultString(source.getApiKey())); + } value.setDefaultFlag(source.getDefaultFlag() == null ? DefaultFlagEnum.NOT_DEFAULT : source.getDefaultFlag()); value.setEnableFlag(source.getEnableFlag() == null ? EnableFlagEnum.ENABLE : source.getEnableFlag()); value.setRemark(StringUtils.defaultString(source.getRemark())); diff --git a/dc3-common/dc3-common-agentic/src/test/java/io/github/pnoker/common/agentic/service/impl/ModelProviderServiceImplTest.java b/dc3-common/dc3-common-agentic/src/test/java/io/github/pnoker/common/agentic/service/impl/ModelProviderServiceImplTest.java index beb61fb6c..94127b7c5 100644 --- a/dc3-common/dc3-common-agentic/src/test/java/io/github/pnoker/common/agentic/service/impl/ModelProviderServiceImplTest.java +++ b/dc3-common/dc3-common-agentic/src/test/java/io/github/pnoker/common/agentic/service/impl/ModelProviderServiceImplTest.java @@ -16,6 +16,7 @@ */ package io.github.pnoker.common.agentic.service.impl; +import org.mockito.ArgumentCaptor; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.lenient; import static org.mockito.Mockito.never; @@ -119,6 +120,7 @@ class ModelProviderServiceImplTest { ModelProviderBO existing = provider(7L); ModelProviderBO updated = provider(7L); updated.setBaseUrl("https://10.0.0.5/v1"); + updated.setApiKey("fresh-key"); when(modelProviderStore.get(7L, header)).thenReturn(Mono.just(existing)); when(baseUrlValidator.validateReactive("https://10.0.0.5/v1")) .thenReturn(Mono.error(new RequestException("Provider base URL is not allowed"))); @@ -130,6 +132,43 @@ class ModelProviderServiceImplTest { verify(modelProviderStore, never()).update(any(), any()); } + @Test + void updateRejectsBaseUrlChangeWithoutAFreshApiKey() { + // Re-pointing a provider while keeping the stored (write-only) key would send + // that secret to the new host — the update path must demand a fresh key. + ModelProviderBO existing = provider(8L); + existing.setApiKey("stored-secret"); + ModelProviderBO updated = provider(8L); + updated.setBaseUrl("https://attacker.example/v1"); + when(modelProviderStore.get(8L, header)).thenReturn(Mono.just(existing)); + + StepVerifier.create(service.update(updated, header)) + .expectErrorSatisfies(error -> { + assert error instanceof RequestException; + assert error.getMessage().contains("new API key"); + }) + .verify(); + + verify(modelProviderStore, never()).update(any(), any()); + } + + @Test + void updateKeepsStoredKeyWhenBaseUrlIsUntouched() { + ModelProviderBO existing = provider(9L); + existing.setApiKey("stored-secret"); + ModelProviderBO updated = provider(9L); + updated.setRemark("renamed"); + when(modelProviderStore.get(9L, header)).thenReturn(Mono.just(existing)); + when(modelProviderStore.update(any(), any())) + .thenAnswer(invocation -> Mono.just(invocation.getArgument(0, ModelProviderBO.class))); + + StepVerifier.create(service.update(updated, header)).expectNextCount(1).verifyComplete(); + + ArgumentCaptor captor = ArgumentCaptor.forClass(ModelProviderBO.class); + verify(modelProviderStore).update(captor.capture(), any()); + assert captor.getValue().getApiKey().equals("stored-secret"); + } + @Test void updateRejectsMissingId() { ModelProviderBO bo = new ModelProviderBO(); @@ -176,6 +215,7 @@ class ModelProviderServiceImplTest { ModelProviderBO existing = provider(7L); existing.setBaseUrl("https://old"); ModelProviderBO updated = provider(7L); + updated.setApiKey("fresh-key"); when(modelProviderStore.get(7L, header)).thenReturn(Mono.just(existing)); when(modelProviderStore.update(any(ModelProviderBO.class), any())).thenReturn(Mono.just(updated));