fix(agentic): require a fresh api key when re-pointing a provider

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 <noreply@anthropic.com>
This commit is contained in:
pnoker
2026-10-21 23:53:22 +08:00
co-authored by Claude Code
parent 0866412fb8
commit aba4e752c8
2 changed files with 55 additions and 4 deletions
@@ -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()));
@@ -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<ModelProviderBO> 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));