This is an automated email from the ASF dual-hosted git repository. lizhimins pushed a commit to branch rocketmq-studio in repository https://gitbox.apache.org/repos/asf/rocketmq-dashboard.git
commit afe2ac79e452b8df845ea03164388adb00a26c77 Author: yx9o <[email protected]> AuthorDate: Fri Jul 24 13:43:28 2026 +0800 fix: avoid returning General Settings API key (#512) Fix security issue where General Settings endpoint returned API key in plaintext. Replace with apiKeyConfigured boolean flag and support explicit key clearing. --- docs/api-spec.md | 18 +++++- .../studio/settings/GeneralSettingsUpdateDTO.java | 73 ++++++++++++++++++++++ .../studio/settings/GeneralSettingsVO.java | 12 ++++ .../studio/settings/SettingsController.java | 5 +- .../rocketmq/studio/settings/SettingsService.java | 10 ++- .../studio/settings/SettingsControllerTest.java | 71 ++++++++++++++++++--- .../studio/settings/SettingsServiceTest.java | 61 ++++++++++++++++-- web/src/api/generalSettings.test.ts | 47 ++++++++++++-- web/src/api/settings.ts | 14 ++++- web/src/pages/settings/index.tsx | 38 +++++++++-- 10 files changed, 318 insertions(+), 31 deletions(-) diff --git a/docs/api-spec.md b/docs/api-spec.md index 2b0bd143..e75158f9 100644 --- a/docs/api-spec.md +++ b/docs/api-spec.md @@ -1483,7 +1483,7 @@ GET /api/settings/general | `sessionTimeout` | `number` | 会话超时（分钟，5-1440） | | `requireLogin` | `boolean` | 是否需要登录 | | `llmProvider` | `string` | LLM 提供商: `openai` / `azure` / `ollama` / `qwen` | -| `apiKey` | `string` | API Key | +| `apiKeyConfigured` | `boolean` | 是否已配置 API Key；响应不会返回密钥内容 | | `model` | `string` | 模型名称 | | `baseUrl` | `string` | Base URL | @@ -1493,7 +1493,21 @@ GET /api/settings/general POST /api/settings/general/save ``` -**Request Body:** 同 14.1 响应格式 +**Request Body:** + +| 字段 | 类型 | 必填 | 说明 | +|------|------|------|------| +| `theme` | `string` | 是 | 主题: `light` / `dark` / `system` | +| `compact` | `boolean` | 是 | 紧凑模式 | +| `desktopNotify` | `boolean` | 是 | 桌面通知 | +| `notifySound` | `boolean` | 是 | 通知声音 | +| `sessionTimeout` | `number` | 是 | 会话超时（分钟，5-1440） | +| `requireLogin` | `boolean` | 是 | 是否需要登录 | +| `llmProvider` | `string` | 是 | LLM 提供商 | +| `apiKey` | `string` | 否 | 新 API Key；省略或传空值时保留现有密钥 | +| `clearApiKey` | `boolean` | 否 | 传 `true` 时显式清除现有密钥，优先级高于 `apiKey` | +| `model` | `string` | 是 | 模型名称 | +| `baseUrl` | `string` | 是 | Base URL | **Response `data`:** `null` diff --git a/server/src/main/java/com/rocketmq/studio/settings/GeneralSettingsUpdateDTO.java b/server/src/main/java/com/rocketmq/studio/settings/GeneralSettingsUpdateDTO.java new file mode 100644 index 00000000..fbfea401 --- /dev/null +++ b/server/src/main/java/com/rocketmq/studio/settings/GeneralSettingsUpdateDTO.java @@ -0,0 +1,73 @@ +/* + * 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 com.rocketmq.studio.settings; + +import jakarta.validation.constraints.Max; +import jakarta.validation.constraints.Min; +import jakarta.validation.constraints.NotBlank; +import jakarta.validation.constraints.NotNull; +import lombok.AllArgsConstructor; +import lombok.Builder; +import lombok.Data; +import lombok.NoArgsConstructor; +import lombok.ToString; + +@Data +@Builder +@NoArgsConstructor +@AllArgsConstructor +public class GeneralSettingsUpdateDTO { + @NotBlank + private String theme; + @NotNull + private Boolean compact; + @NotNull + private Boolean desktopNotify; + @NotNull + private Boolean notifySound; + @NotNull + @Min(5) + @Max(1440) + private Integer sessionTimeout; + @NotNull + private Boolean requireLogin; + @NotBlank + private String llmProvider; + @ToString.Exclude + private String apiKey; + private boolean clearApiKey; + @NotBlank + private String model; + @NotNull + private String baseUrl; + + public GeneralSettingsVO toSettings() { + return GeneralSettingsVO.builder() + .theme(theme) + .compact(compact) + .desktopNotify(desktopNotify) + .notifySound(notifySound) + .sessionTimeout(sessionTimeout) + .requireLogin(requireLogin) + .llmProvider(llmProvider) + .apiKey(apiKey) + .clearApiKey(clearApiKey) + .model(model) + .baseUrl(baseUrl) + .build(); + } +} diff --git a/server/src/main/java/com/rocketmq/studio/settings/GeneralSettingsVO.java b/server/src/main/java/com/rocketmq/studio/settings/GeneralSettingsVO.java index 8dd3bd6e..bfb60833 100644 --- a/server/src/main/java/com/rocketmq/studio/settings/GeneralSettingsVO.java +++ b/server/src/main/java/com/rocketmq/studio/settings/GeneralSettingsVO.java @@ -16,10 +16,13 @@ */ package com.rocketmq.studio.settings; +import com.fasterxml.jackson.annotation.JsonProperty; import lombok.AllArgsConstructor; import lombok.Builder; import lombok.Data; import lombok.NoArgsConstructor; +import lombok.ToString; +import org.springframework.util.StringUtils; @Data @Builder @@ -33,7 +36,16 @@ public class GeneralSettingsVO { private int sessionTimeout; private boolean requireLogin; private String llmProvider; + @JsonProperty(access = JsonProperty.Access.WRITE_ONLY) + @ToString.Exclude private String apiKey; + @JsonProperty(access = JsonProperty.Access.WRITE_ONLY) + private boolean clearApiKey; private String model; private String baseUrl; + + @JsonProperty(value = "apiKeyConfigured", access = JsonProperty.Access.READ_ONLY) + public boolean isApiKeyConfigured() { + return StringUtils.hasText(apiKey); + } } diff --git a/server/src/main/java/com/rocketmq/studio/settings/SettingsController.java b/server/src/main/java/com/rocketmq/studio/settings/SettingsController.java index a9a91234..a84923de 100644 --- a/server/src/main/java/com/rocketmq/studio/settings/SettingsController.java +++ b/server/src/main/java/com/rocketmq/studio/settings/SettingsController.java @@ -17,6 +17,7 @@ package com.rocketmq.studio.settings; import com.rocketmq.studio.common.domain.Result; +import jakarta.validation.Valid; import lombok.RequiredArgsConstructor; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PostMapping; @@ -40,8 +41,8 @@ public class SettingsController { } @PostMapping("/general/save") - public Result<Void> saveGeneralSettings(@RequestBody GeneralSettingsVO settings) { - settingsService.saveGeneralSettings(settings); + public Result<Void> saveGeneralSettings(@Valid @RequestBody GeneralSettingsUpdateDTO request) { + settingsService.saveGeneralSettings(request.toSettings()); return Result.ok(); } diff --git a/server/src/main/java/com/rocketmq/studio/settings/SettingsService.java b/server/src/main/java/com/rocketmq/studio/settings/SettingsService.java index 5505d27f..2c2db875 100644 --- a/server/src/main/java/com/rocketmq/studio/settings/SettingsService.java +++ b/server/src/main/java/com/rocketmq/studio/settings/SettingsService.java @@ -19,6 +19,7 @@ package com.rocketmq.studio.settings; import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.stereotype.Service; +import org.springframework.util.StringUtils; import java.util.List; import java.util.UUID; @@ -37,8 +38,15 @@ public class SettingsService { } - public void saveGeneralSettings(GeneralSettingsVO settings) { + public synchronized void saveGeneralSettings(GeneralSettingsVO settings) { log.info("Saving general settings"); + GeneralSettingsVO currentSettings = settingsRepository.loadGeneralSettings(); + if (settings.isClearApiKey()) { + settings.setApiKey(""); + } else if (!StringUtils.hasText(settings.getApiKey()) && currentSettings != null) { + settings.setApiKey(currentSettings.getApiKey()); + } + settings.setClearApiKey(false); settingsRepository.saveGeneralSettings(settings); } diff --git a/server/src/test/java/com/rocketmq/studio/settings/SettingsControllerTest.java b/server/src/test/java/com/rocketmq/studio/settings/SettingsControllerTest.java index 42a3c0e6..f82bb1ad 100644 --- a/server/src/test/java/com/rocketmq/studio/settings/SettingsControllerTest.java +++ b/server/src/test/java/com/rocketmq/studio/settings/SettingsControllerTest.java @@ -30,8 +30,10 @@ import java.util.Collections; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.is; import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.argThat; import static org.mockito.Mockito.doNothing; import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoInteractions; import static org.mockito.Mockito.when; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.post; @@ -77,26 +79,81 @@ class SettingsControllerTest { .andExpect(jsonPath("$.data.sessionTimeout", is(30))) .andExpect(jsonPath("$.data.requireLogin", is(true))) .andExpect(jsonPath("$.data.llmProvider", is("openai"))) + .andExpect(jsonPath("$.data.apiKey").doesNotExist()) + .andExpect(jsonPath("$.data.apiKeyConfigured", is(true))) + .andExpect(jsonPath("$.data.clearApiKey").doesNotExist()) .andExpect(jsonPath("$.data.model", is("gpt-4"))); } @Test void saveGeneralSettingsShouldReturnSuccess() throws Exception { - GeneralSettingsVO settings = GeneralSettingsVO.builder() - .theme("light") - .compact(false) - .sessionTimeout(60) - .build(); doNothing().when(settingsService).saveGeneralSettings(any(GeneralSettingsVO.class)); mockMvc.perform(post("/api/settings/general/save") .contentType(MediaType.APPLICATION_JSON) - .content(objectMapper.writeValueAsString(settings))) + .content(""" + { + "theme": "light", + "compact": false, + "desktopNotify": true, + "notifySound": false, + "sessionTimeout": 60, + "requireLogin": true, + "llmProvider": "openai", + "apiKey": "sk-new", + "apiKeyConfigured": false, + "model": "gpt-4", + "baseUrl": "" + } + """)) .andExpect(status().isOk()) .andExpect(jsonPath("$.code", is(200))) .andExpect(jsonPath("$.data").doesNotExist()); - verify(settingsService).saveGeneralSettings(any(GeneralSettingsVO.class)); + verify(settingsService).saveGeneralSettings(argThat(settings -> + "sk-new".equals(settings.getApiKey()) && settings.isApiKeyConfigured())); + } + + @Test + void saveGeneralSettingsShouldAcceptExplicitApiKeyClearWithoutBindingResponseState() throws Exception { + doNothing().when(settingsService).saveGeneralSettings(any(GeneralSettingsVO.class)); + + mockMvc.perform(post("/api/settings/general/save") + .contentType(MediaType.APPLICATION_JSON) + .content(""" + { + "theme": "light", + "compact": false, + "desktopNotify": true, + "notifySound": false, + "sessionTimeout": 60, + "requireLogin": true, + "llmProvider": "openai", + "clearApiKey": true, + "apiKeyConfigured": true, + "model": "gpt-4", + "baseUrl": "" + } + """)) + .andExpect(status().isOk()); + + verify(settingsService).saveGeneralSettings(argThat(settings -> + settings.isClearApiKey() && !settings.isApiKeyConfigured())); + } + + @Test + void saveGeneralSettingsShouldRejectIncompleteReplacement() throws Exception { + mockMvc.perform(post("/api/settings/general/save") + .contentType(MediaType.APPLICATION_JSON) + .content(""" + { + "theme": "light", + "apiKey": "sk-new" + } + """)) + .andExpect(status().isBadRequest()); + + verifyNoInteractions(settingsService); } @Test diff --git a/server/src/test/java/com/rocketmq/studio/settings/SettingsServiceTest.java b/server/src/test/java/com/rocketmq/studio/settings/SettingsServiceTest.java index 430021af..024a9a75 100644 --- a/server/src/test/java/com/rocketmq/studio/settings/SettingsServiceTest.java +++ b/server/src/test/java/com/rocketmq/studio/settings/SettingsServiceTest.java @@ -68,17 +68,68 @@ class SettingsServiceTest { } @Test - void saveGeneralSettingsShouldDelegateToRepository() { - GeneralSettingsVO settings = GeneralSettingsVO.builder() + void saveGeneralSettingsShouldPreserveExistingApiKeyWhenOmitted() { + GeneralSettingsVO existing = GeneralSettingsVO.builder() + .apiKey("sk-existing") + .build(); + GeneralSettingsVO update = GeneralSettingsVO.builder() .theme("light") .compact(false) .sessionTimeout(60) .build(); - doNothing().when(settingsRepository).saveGeneralSettings(settings); + when(settingsRepository.loadGeneralSettings()).thenReturn(existing); + + settingsService.saveGeneralSettings(update); + + assertThat(update.getApiKey()).isEqualTo("sk-existing"); + verify(settingsRepository).saveGeneralSettings(update); + } + + @Test + void saveGeneralSettingsShouldReplaceExistingApiKey() { + GeneralSettingsVO existing = GeneralSettingsVO.builder() + .apiKey("sk-existing") + .build(); + GeneralSettingsVO update = GeneralSettingsVO.builder() + .apiKey("sk-new") + .build(); + when(settingsRepository.loadGeneralSettings()).thenReturn(existing); + + settingsService.saveGeneralSettings(update); + + assertThat(update.getApiKey()).isEqualTo("sk-new"); + verify(settingsRepository).saveGeneralSettings(update); + } + + @Test + void saveGeneralSettingsShouldClearApiKeyOnlyWhenExplicitlyRequested() { + GeneralSettingsVO existing = GeneralSettingsVO.builder() + .apiKey("sk-existing") + .build(); + GeneralSettingsVO update = GeneralSettingsVO.builder() + .clearApiKey(true) + .build(); + when(settingsRepository.loadGeneralSettings()).thenReturn(existing); + + settingsService.saveGeneralSettings(update); + + assertThat(update.getApiKey()).isEmpty(); + assertThat(update.isClearApiKey()).isFalse(); + verify(settingsRepository).saveGeneralSettings(update); + } + + @Test + void saveGeneralSettingsShouldLetClearTakePrecedenceOverReplacementApiKey() { + GeneralSettingsVO update = GeneralSettingsVO.builder() + .apiKey("sk-new") + .clearApiKey(true) + .build(); - settingsService.saveGeneralSettings(settings); + settingsService.saveGeneralSettings(update); - verify(settingsRepository).saveGeneralSettings(settings); + assertThat(update.getApiKey()).isEmpty(); + assertThat(update.isClearApiKey()).isFalse(); + verify(settingsRepository).saveGeneralSettings(update); } @Test diff --git a/web/src/api/generalSettings.test.ts b/web/src/api/generalSettings.test.ts index 1a0700cf..b8928fb0 100644 --- a/web/src/api/generalSettings.test.ts +++ b/web/src/api/generalSettings.test.ts @@ -19,7 +19,7 @@ import MockAdapter from 'axios-mock-adapter'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import client from './client'; import { getGeneralSettings, saveGeneralSettings } from './settings'; -import type { GeneralSettings } from './settings'; +import type { GeneralSettings, GeneralSettingsUpdate } from './settings'; const mock = new MockAdapter(client); const settings: GeneralSettings = { @@ -30,10 +30,21 @@ const settings: GeneralSettings = { sessionTimeout: 60, requireLogin: true, llmProvider: 'openai', - apiKey: 'sk-test', + apiKeyConfigured: true, model: 'gpt-5', baseUrl: 'https://api.example.com/v1', }; +const editableSettings: GeneralSettingsUpdate = { + theme: settings.theme, + compact: settings.compact, + desktopNotify: settings.desktopNotify, + notifySound: settings.notifySound, + sessionTimeout: settings.sessionTimeout, + requireLogin: settings.requireLogin, + llmProvider: settings.llmProvider, + model: settings.model, + baseUrl: settings.baseUrl, +}; describe('general settings API', () => { beforeEach(() => { @@ -52,12 +63,38 @@ describe('general settings API', () => { await expect(getGeneralSettings()).resolves.toEqual(settings); }); - it('persists all editable settings fields', async () => { + it('sends a replacement API key without requiring the stored secret', async () => { + const update: GeneralSettingsUpdate = { ...editableSettings, apiKey: 'sk-new' }; + mock.onPost('/settings/general/save').reply((config) => { + expect(JSON.parse(config.data)).toEqual(update); + return [200, { code: 200, data: null }]; + }); + + await expect(saveGeneralSettings(update)).resolves.toBeUndefined(); + }); + + it('omits blank and response-only API key fields when preserving the stored secret', async () => { + const update = { + ...settings, + apiKey: ' ', + } as GeneralSettingsUpdate & { apiKeyConfigured: boolean }; + mock.onPost('/settings/general/save').reply((config) => { + const body = JSON.parse(config.data); + expect(body).not.toHaveProperty('apiKey'); + expect(body).not.toHaveProperty('apiKeyConfigured'); + return [200, { code: 200, data: null }]; + }); + + await expect(saveGeneralSettings(update)).resolves.toBeUndefined(); + }); + + it('sends the explicit API key clear flag', async () => { + const update: GeneralSettingsUpdate = { ...editableSettings, clearApiKey: true }; mock.onPost('/settings/general/save').reply((config) => { - expect(JSON.parse(config.data)).toEqual(settings); + expect(JSON.parse(config.data)).toEqual(update); return [200, { code: 200, data: null }]; }); - await expect(saveGeneralSettings(settings)).resolves.toBeUndefined(); + await expect(saveGeneralSettings(update)).resolves.toBeUndefined(); }); }); diff --git a/web/src/api/settings.ts b/web/src/api/settings.ts index e8b67e64..19829f70 100644 --- a/web/src/api/settings.ts +++ b/web/src/api/settings.ts @@ -26,11 +26,16 @@ export interface GeneralSettings { sessionTimeout: number; requireLogin: boolean; llmProvider: string; - apiKey: string; + apiKeyConfigured: boolean; model: string; baseUrl: string; } +export type GeneralSettingsUpdate = Omit<GeneralSettings, 'apiKeyConfigured'> & { + apiKey?: string; + clearApiKey?: boolean; +}; + export interface DataSource { key: string; name: string; @@ -46,8 +51,11 @@ export async function getGeneralSettings() { return res.data.data; } -export async function saveGeneralSettings(data: Partial<GeneralSettings>) { - await client.post('/settings/general/save', data); +export async function saveGeneralSettings(data: GeneralSettingsUpdate) { + const payload = { ...data } as GeneralSettingsUpdate & { apiKeyConfigured?: boolean }; + delete payload.apiKeyConfigured; + if (!payload.apiKey?.trim()) delete payload.apiKey; + await client.post('/settings/general/save', payload); } // ─── Data Sources ─────────────────────────────────────────────── diff --git a/web/src/pages/settings/index.tsx b/web/src/pages/settings/index.tsx index 2e5a3218..9344eea0 100644 --- a/web/src/pages/settings/index.tsx +++ b/web/src/pages/settings/index.tsx @@ -18,6 +18,7 @@ import { useEffect, useState } from 'react'; import { Button, + Checkbox, Descriptions, Divider, Flex, @@ -50,7 +51,7 @@ import PageHeader from '../../components/PageHeader'; import { useLang } from '../../i18n/LangContext'; import StatusBadge from '../../components/StatusBadge'; import { getGeneralSettings, saveGeneralSettings } from '../../api/settings'; -import type { GeneralSettings } from '../../api/settings'; +import type { GeneralSettingsUpdate } from '../../api/settings'; import { createDataSource, deleteDataSource, @@ -72,15 +73,20 @@ const typeTagColor: Record<string, string> = { // ─── General Settings Tab ─────────────────────────────────────────────────── const GeneralSettingsTab = () => { - const [form] = Form.useForm(); + const [form] = Form.useForm<GeneralSettingsUpdate>(); const [loading, setLoading] = useState(true); const [saving, setSaving] = useState(false); + const [apiKeyConfigured, setApiKeyConfigured] = useState(false); + const clearApiKey = Form.useWatch('clearApiKey', form); useEffect(() => { let cancelled = false; void getGeneralSettings() .then((settings) => { - if (!cancelled) form.setFieldsValue(settings); + if (!cancelled) { + setApiKeyConfigured(settings.apiKeyConfigured); + form.setFieldsValue({ ...settings, apiKey: undefined, clearApiKey: false }); + } }) .catch(() => { if (!cancelled) message.error('通用设置加载失败，请稍后重试'); @@ -94,10 +100,14 @@ const GeneralSettingsTab = () => { }; }, [form]); - const handleFinish = async (values: GeneralSettings) => { + const handleFinish = async (values: GeneralSettingsUpdate) => { setSaving(true); try { await saveGeneralSettings(values); + setApiKeyConfigured( + values.clearApiKey ? false : apiKeyConfigured || Boolean(values.apiKey?.trim()), + ); + form.setFieldsValue({ apiKey: undefined, clearApiKey: false }); message.success('设置已保存'); } catch { message.error('设置保存失败，请稍后重试'); @@ -187,10 +197,26 @@ const GeneralSettingsTab = () => { /> </Form.Item> - <Form.Item label="API Key" name="apiKey"> - <Input.Password placeholder="sk-..." /> + <Form.Item + label="API Key" + name="apiKey" + extra={apiKeyConfigured ? '已配置；留空将保留现有密钥' : '尚未配置'} + > + <Input.Password placeholder="sk-..." disabled={clearApiKey} /> </Form.Item> + {apiKeyConfigured && ( + <Form.Item name="clearApiKey" valuePropName="checked" wrapperCol={{ offset: 4, span: 14 }}> + <Checkbox + onChange={(event) => { + if (event.target.checked) form.setFieldValue('apiKey', undefined); + }} + > + 清除已保存的 API Key + </Checkbox> + </Form.Item> + )} + <Form.Item label="模型名称" name="model"> <Input placeholder="qwen-max" /> </Form.Item>
