fix(knowledge): validate KB names on the manager, and let a bad one be deleted
A knowledge-base name is three things at once: a kb_config.json key, a
directory under data/knowledge_bases, and a path segment in every per-KB API
route. `validate_knowledge_base_name` exists to keep those three uses
compatible — its docstring says so — but it was called from one of the eight
HTTP entry points that can write a name into the config. The seven connect-*
endpoints (Obsidian, MarginNote, folder, LightRAG server, WeKnora, IMA, and
the subagent connection) did `(name or "").strip()` and wrote the result.
A name holding a "/" registers fine that way and is then unreachable.
uvicorn percent-decodes the path before routing, so %2F is a real separator
by the time Starlette matches `{kb_name}` — compiled to `[^/]+`, which cannot
span it. All 29 per-KB routes 404, delete included, while the list endpoint
takes no path parameter and keeps rendering the KB. Connected KBs are also
exempt from the orphan prune, so nothing collects it either: visible, unusable,
and removable only by hand-editing kb_config.json.
Two changes, both needed. Validation moves onto the eight `register_*`
methods, which is the seam every route already goes through — no route file
changes, since they all map ValueError to 400 — and the now-unreachable
"name is required" guards go with it. And a path-free
`POST /knowledge-bases/delete` gives the entries that predate the guard a way
out; a body is never split into segments. Widening the path route to
`{kb_name:path}` would not work: it is declared ahead of the DELETE routes
for linked folders, GitHub sources and web sources, and a greedy converter
would silently swallow all three.
Two smaller fixes ride along. The delete handler's catch-all swallowed its
own HTTPException and answered 500 with "404: ... not found" in the detail;
it now re-raises. And CreateKbModal states the rule under the field instead
of letting an untranslated English 400 be the first the user hears of it.
test_marginnote4_kb's store-collision case used "My Lib" / "My/Lib"; the
slash is now refused before that guard is reached, so it uses "My.Lib" —
still a name the rule allows, still sanitized to the same My_Lib.db.
This commit is contained in:
@@ -2814,20 +2814,52 @@ async def delete_kb_file(kb_name: str, filename: str):
|
||||
}
|
||||
|
||||
|
||||
@router.delete("/knowledge-bases/{kb_name}")
|
||||
async def delete_knowledge_base(kb_name: str):
|
||||
"""Delete a knowledge base."""
|
||||
def _delete_kb(kb_name: str) -> dict[str, str]:
|
||||
"""Delete ``kb_name``, whichever route addressed it."""
|
||||
try:
|
||||
manager, resolved_name, _ = _writable_kb(kb_name)
|
||||
success = manager.delete_knowledge_base(resolved_name, confirm=True)
|
||||
if not success:
|
||||
raise HTTPException(status_code=400, detail="Failed to delete knowledge base")
|
||||
logger.info(f"KB '{kb_name}' deleted")
|
||||
return {"message": f"Knowledge base '{kb_name}' deleted successfully"}
|
||||
except HTTPException:
|
||||
# Re-raised before the catch-all below, which used to turn a 404 from
|
||||
# ``_writable_kb`` into a 500 whose detail read "404: ... not found".
|
||||
raise
|
||||
except ValueError:
|
||||
raise HTTPException(status_code=404, detail=f"Knowledge base '{kb_name}' not found")
|
||||
except Exception as e:
|
||||
raise HTTPException(status_code=500, detail=str(e))
|
||||
if not success:
|
||||
raise HTTPException(status_code=400, detail="Failed to delete knowledge base")
|
||||
logger.info(f"KB '{kb_name}' deleted")
|
||||
return {"message": f"Knowledge base '{kb_name}' deleted successfully"}
|
||||
|
||||
|
||||
class DeleteKnowledgeBaseRequest(BaseModel):
|
||||
name: str
|
||||
|
||||
|
||||
@router.post("/knowledge-bases/delete")
|
||||
async def delete_knowledge_base_by_name(payload: DeleteKnowledgeBaseRequest):
|
||||
"""Delete a knowledge base named in the body rather than in the path.
|
||||
|
||||
The path route below cannot reach every registered name. A name is a
|
||||
``kb_config.json`` key, and until the ``register_*`` methods validated it
|
||||
the connect-* endpoints wrote whatever the user typed — including a ``/``.
|
||||
uvicorn percent-decodes the path before routing, so ``%2F`` becomes a real
|
||||
separator and ``{kb_name}`` (compiled to ``[^/]+``) cannot span it: every
|
||||
per-KB route 404s and the KB is visible in the list but unreachable.
|
||||
|
||||
A body is never split into path segments, so this reaches those entries.
|
||||
Widening the path route to ``{kb_name:path}`` would not do — it is
|
||||
declared ahead of the DELETE routes for linked folders, GitHub sources and
|
||||
web sources, and a greedy converter would silently swallow all three.
|
||||
"""
|
||||
return _delete_kb(payload.name)
|
||||
|
||||
|
||||
@router.delete("/knowledge-bases/{kb_name}")
|
||||
async def delete_knowledge_base(kb_name: str):
|
||||
"""Delete a knowledge base."""
|
||||
return _delete_kb(kb_name)
|
||||
|
||||
|
||||
@router.get("/knowledge-bases/tasks/{task_id}/stream")
|
||||
|
||||
@@ -30,6 +30,7 @@ from deeptutor.knowledge.kb_types import (
|
||||
is_connected_kb,
|
||||
)
|
||||
from deeptutor.knowledge.manifest import iter_kb_documents
|
||||
from deeptutor.knowledge.naming import validate_knowledge_base_name
|
||||
from deeptutor.services.file_io import atomic_write_json
|
||||
from deeptutor.services.rag.factory import (
|
||||
DEFAULT_PROVIDER,
|
||||
@@ -691,6 +692,7 @@ class KnowledgeBaseManager:
|
||||
|
||||
def register_knowledge_base(self, name: str, description: str = "", set_default: bool = False):
|
||||
"""Register a knowledge base"""
|
||||
name = validate_knowledge_base_name(name)
|
||||
kb_dir = self.base_dir / name
|
||||
if not kb_dir.exists():
|
||||
raise ValueError(f"Knowledge base directory does not exist: {kb_dir}")
|
||||
@@ -740,9 +742,7 @@ class KnowledgeBaseManager:
|
||||
user's existing vault directory, which the Obsidian capability reads
|
||||
live. Raises ``ValueError`` on a missing/invalid path or a name clash.
|
||||
"""
|
||||
name = (name or "").strip()
|
||||
if not name:
|
||||
raise ValueError("Knowledge base name is required.")
|
||||
name = validate_knowledge_base_name(name)
|
||||
vault = Path(vault_path).expanduser()
|
||||
if not vault.is_dir():
|
||||
raise ValueError(f"Vault path is not a directory: {vault_path}")
|
||||
@@ -785,9 +785,7 @@ class KnowledgeBaseManager:
|
||||
folder with the probe helper first; this only guards basic invariants.
|
||||
Raises ``ValueError`` on a missing/invalid path or a name clash.
|
||||
"""
|
||||
name = (name or "").strip()
|
||||
if not name:
|
||||
raise ValueError("Knowledge base name is required.")
|
||||
name = validate_knowledge_base_name(name)
|
||||
provider = normalize_provider_name(provider)
|
||||
folder = Path(external_path).expanduser()
|
||||
if not folder.is_dir():
|
||||
@@ -838,11 +836,9 @@ class KnowledgeBaseManager:
|
||||
capability drives the live agent; there is nothing on disk to retrieve or
|
||||
reconcile. Raises ``ValueError`` on a missing name/kind or a name clash.
|
||||
"""
|
||||
name = (name or "").strip()
|
||||
name = validate_knowledge_base_name(name)
|
||||
agent_kind = (agent_kind or "").strip()
|
||||
partner_id = (partner_id or "").strip()
|
||||
if not name:
|
||||
raise ValueError("Connection name is required.")
|
||||
if not agent_kind:
|
||||
raise ValueError("agent_kind is required.")
|
||||
resolved_cwd = ""
|
||||
@@ -892,10 +888,8 @@ class KnowledgeBaseManager:
|
||||
guards basic invariants. Raises ``ValueError`` on a missing name/URL or a
|
||||
name clash.
|
||||
"""
|
||||
name = (name or "").strip()
|
||||
name = validate_knowledge_base_name(name)
|
||||
server_url = (server_url or "").strip().rstrip("/")
|
||||
if not name:
|
||||
raise ValueError("Knowledge base name is required.")
|
||||
if not server_url:
|
||||
raise ValueError("LightRAG server URL is required.")
|
||||
|
||||
@@ -941,9 +935,7 @@ class KnowledgeBaseManager:
|
||||
``ValueError`` on a missing name, a name clash, or a store already
|
||||
claimed by another library.
|
||||
"""
|
||||
name = (name or "").strip()
|
||||
if not name:
|
||||
raise ValueError("Knowledge base name is required.")
|
||||
name = validate_knowledge_base_name(name)
|
||||
|
||||
self.config = self._load_config()
|
||||
knowledge_bases = self.config.setdefault("knowledge_bases", {})
|
||||
@@ -1022,12 +1014,10 @@ class KnowledgeBaseManager:
|
||||
Raises ``ValueError`` on a missing field, a half-filled credential pair,
|
||||
or a name clash.
|
||||
"""
|
||||
name = (name or "").strip()
|
||||
name = validate_knowledge_base_name(name)
|
||||
client_id = (client_id or "").strip()
|
||||
api_key = (api_key or "").strip()
|
||||
knowledge_base_id = (knowledge_base_id or "").strip()
|
||||
if not name:
|
||||
raise ValueError("Knowledge base name is required.")
|
||||
if bool(client_id) != bool(api_key):
|
||||
raise ValueError("IMA Client ID and API Key must be given together.")
|
||||
if not knowledge_base_id:
|
||||
@@ -1067,12 +1057,10 @@ class KnowledgeBaseManager:
|
||||
description: str = "",
|
||||
) -> dict:
|
||||
"""Register a self-hosted WeKnora knowledge base as a pointer KB."""
|
||||
name = (name or "").strip()
|
||||
name = validate_knowledge_base_name(name)
|
||||
server_url = (server_url or "").strip().rstrip("/")
|
||||
api_key = (api_key or "").strip()
|
||||
knowledge_base_id = (knowledge_base_id or "").strip()
|
||||
if not name:
|
||||
raise ValueError("Knowledge base name is required.")
|
||||
if not server_url or not knowledge_base_id:
|
||||
raise ValueError("WeKnora server URL and knowledge base ID are required.")
|
||||
if not api_key:
|
||||
|
||||
@@ -2760,3 +2760,42 @@ def test_lightrag_config_validates_dedicated_llm_selection(monkeypatch, tmp_path
|
||||
json={"llm_profile_id": "", "llm_model_id": ""},
|
||||
)
|
||||
assert cleared.status_code == 200
|
||||
|
||||
|
||||
def test_delete_by_body_reaches_a_name_the_path_route_cannot(monkeypatch, tmp_path: Path) -> None:
|
||||
"""A slash in the name breaks path addressing, not the manager.
|
||||
|
||||
uvicorn percent-decodes the request path before routing, so ``%2F`` is a
|
||||
real separator by the time Starlette matches ``{kb_name}`` — compiled to
|
||||
``[^/]+``, which cannot span it. Names like this were registered before
|
||||
the ``register_*`` methods validated one, and the only way out was hand
|
||||
editing ``kb_config.json``. Deleting by body sidesteps the path entirely.
|
||||
"""
|
||||
manager = _real_manager(monkeypatch, tmp_path)
|
||||
# Written straight into the config: registering it through the manager is
|
||||
# exactly what is refused now, and the point is to clean up what predates
|
||||
# that guard.
|
||||
manager.config.setdefault("knowledge_bases", {})["数学/物理"] = {
|
||||
"path": "数学/物理",
|
||||
"type": "weknora",
|
||||
"server_url": "http://localhost:8080",
|
||||
"knowledge_base_id": "kb-1",
|
||||
}
|
||||
manager._save_config()
|
||||
|
||||
with TestClient(_build_app()) as client:
|
||||
stranded = client.delete("/api/knowledge-bases/数学/物理")
|
||||
assert stranded.status_code == 404
|
||||
|
||||
removed = client.post("/api/knowledge-bases/delete", json={"name": "数学/物理"})
|
||||
|
||||
assert removed.status_code == 200
|
||||
assert "数学/物理" not in manager._load_config().get("knowledge_bases", {})
|
||||
|
||||
|
||||
def test_delete_reports_a_missing_knowledge_base_as_404(monkeypatch, tmp_path: Path) -> None:
|
||||
"""The catch-all used to swallow the 404 and answer 500 with it inside."""
|
||||
_real_manager(monkeypatch, tmp_path)
|
||||
with TestClient(_build_app()) as client:
|
||||
response = client.post("/api/knowledge-bases/delete", json={"name": "never-created"})
|
||||
assert response.status_code == 404
|
||||
|
||||
@@ -161,8 +161,12 @@ def test_register_rejects_a_name_that_derives_an_existing_store(
|
||||
"""Distinct names can still derive one SQLite file.
|
||||
|
||||
``default_db_path`` keeps only alphanumerics, ``-`` and ``_``, so "My Lib"
|
||||
and "My/Lib" both land on ``My_Lib.db``. Sharing it would merge two
|
||||
and "My.Lib" both land on ``My_Lib.db``. Sharing it would merge two
|
||||
libraries' objects and let either one's paired devices sync into the other.
|
||||
|
||||
The pair used to be "My Lib" / "My/Lib". A ``/`` is now refused by
|
||||
``validate_knowledge_base_name`` before this guard is reached, so the
|
||||
collision needs a character the name rule allows — a dot does.
|
||||
"""
|
||||
monkeypatch.setenv("DEEPTUTOR_HOME", str(tmp_path / "home"))
|
||||
PathService.reset_instance()
|
||||
@@ -171,7 +175,7 @@ def test_register_rejects_a_name_that_derives_an_existing_store(
|
||||
manager.register_marginnote4_kb("My Lib")
|
||||
|
||||
with pytest.raises(ValueError, match="already uses that MarginNote store"):
|
||||
manager.register_marginnote4_kb("My/Lib")
|
||||
manager.register_marginnote4_kb("My.Lib")
|
||||
|
||||
# A name that differs by more than punctuation is fine.
|
||||
manager.register_marginnote4_kb("Other Lib")
|
||||
|
||||
@@ -13,3 +13,33 @@ def test_validate_knowledge_base_name_allows_unicode_and_spaces() -> None:
|
||||
def test_validate_knowledge_base_name_rejects_path_and_url_separators(name: str) -> None:
|
||||
with pytest.raises(ValueError, match="reserved characters"):
|
||||
validate_knowledge_base_name(name)
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"register",
|
||||
[
|
||||
lambda m, name: m.register_obsidian_vault(name, "/tmp"),
|
||||
lambda m, name: m.register_linked_kb(name, "/tmp", "llamaindex"),
|
||||
lambda m, name: m.register_subagent_connection(name, agent_kind="claude_code"),
|
||||
lambda m, name: m.register_lightrag_server_kb(name, "http://localhost:9621"),
|
||||
lambda m, name: m.register_marginnote4_kb(name),
|
||||
lambda m, name: m.register_ima_kb(name, "", "", "lib-1"),
|
||||
lambda m, name: m.register_weknora_kb(name, "http://localhost:8080", "k", "kb-1"),
|
||||
],
|
||||
)
|
||||
def test_every_register_path_rejects_a_url_separator(tmp_path, register) -> None:
|
||||
"""The validator has to sit on the manager, not on one route.
|
||||
|
||||
``validate_knowledge_base_name`` used to be called by the create route
|
||||
alone, so the seven connect-* endpoints wrote whatever the user typed
|
||||
straight into ``kb_config.json``. A name holding a ``/`` is then
|
||||
unaddressable: uvicorn decodes the path before routing and ``{kb_name}``
|
||||
cannot span a separator, so every per-KB route 404s and the KB can be
|
||||
listed but never deleted.
|
||||
"""
|
||||
from deeptutor.knowledge.manager import KnowledgeBaseManager
|
||||
|
||||
manager = KnowledgeBaseManager(base_dir=str(tmp_path))
|
||||
with pytest.raises(ValueError, match="reserved characters"):
|
||||
register(manager, "数学/物理")
|
||||
assert manager.list_knowledge_bases() == []
|
||||
|
||||
@@ -44,6 +44,7 @@ import {
|
||||
uploadPolicyForProvider,
|
||||
validateFiles,
|
||||
} from "@/lib/knowledge-helpers";
|
||||
import { forbiddenKbNameChars, isValidKbName } from "@/lib/kb-name";
|
||||
import FileDropZone from "./FileDropZone";
|
||||
import ImaConnectionFields from "./ImaConnectionFields";
|
||||
import KnowledgeEngineIcon from "./KnowledgeEngineIcon";
|
||||
@@ -374,9 +375,15 @@ export default function CreateKbModal({
|
||||
|
||||
const trimmedServerUrl = serverUrl.trim();
|
||||
|
||||
// Mirrors the backend rule so the name is rejected under the field rather
|
||||
// than as an English 400 after Create. The connect-* paths give no error at
|
||||
// all today, which is how a "/" name got registered in the first place.
|
||||
const nameProblems = forbiddenKbNameChars(trimmed);
|
||||
|
||||
const canSubmit = (() => {
|
||||
if (submitting) return false;
|
||||
if (!trimmed) return false;
|
||||
if (!isValidKbName(trimmed)) return false;
|
||||
if (mode === "new") {
|
||||
if (isLightRagServer) {
|
||||
// The connection must pass the test before a KB is bound to it.
|
||||
@@ -589,8 +596,17 @@ export default function CreateKbModal({
|
||||
autoFocus
|
||||
disabled={submitting}
|
||||
placeholder={t("e.g. project-papers")}
|
||||
aria-invalid={nameProblems.length > 0}
|
||||
className="w-full rounded-lg border border-[var(--border)] bg-[var(--background)] px-3 py-2 text-[13px] text-[var(--foreground)] outline-none transition-colors focus:border-[var(--foreground)]/25 disabled:opacity-50"
|
||||
/>
|
||||
{nameProblems.length > 0 && (
|
||||
<p className="mt-1 text-[11px] text-[var(--destructive)]">
|
||||
{t(
|
||||
"A knowledge base name cannot contain {{chars}} — these separate paths and URLs.",
|
||||
{ chars: nameProblems.join(" ") },
|
||||
)}
|
||||
</p>
|
||||
)}
|
||||
</div>
|
||||
|
||||
{mode === "new" ? (
|
||||
|
||||
@@ -1251,13 +1251,21 @@ export async function retryKnowledgeBase(
|
||||
return (await res.json()) as KnowledgeTaskResponse;
|
||||
}
|
||||
|
||||
/**
|
||||
* Deletes by name in the body, not in the path.
|
||||
*
|
||||
* A name registered before the `register_*` methods validated one can contain
|
||||
* a `/`. uvicorn decodes `%2F` back to a separator before routing, so the
|
||||
* path route cannot match such a name and answers 404 — leaving a KB that is
|
||||
* listed but not removable from the UI. A body has no such limit, so this is
|
||||
* the one delete that reaches every registered entry.
|
||||
*/
|
||||
export async function deleteKnowledgeBase(name: string): Promise<void> {
|
||||
const res = await apiFetch(
|
||||
apiUrl(`/api/knowledge-bases/${encodeURIComponent(name)}`),
|
||||
{
|
||||
method: "DELETE",
|
||||
},
|
||||
);
|
||||
const res = await apiFetch(apiUrl(`/api/knowledge-bases/delete`), {
|
||||
method: "POST",
|
||||
headers: { "Content-Type": "application/json" },
|
||||
body: JSON.stringify({ name }),
|
||||
});
|
||||
if (!res.ok) {
|
||||
throw new Error(
|
||||
await readErrorDetail(res, `Delete failed (${res.status})`),
|
||||
|
||||
@@ -0,0 +1,32 @@
|
||||
/**
|
||||
* The client-side half of knowledge-base name validation.
|
||||
*
|
||||
* The backend is the authority — `deeptutor/knowledge/naming.py` rejects the
|
||||
* same set, and every `register_*` method calls it, so nothing depends on
|
||||
* this file being reached. It exists so the rule arrives as a hint under the
|
||||
* field the user is typing in, instead of as an English 400 after they press
|
||||
* Create.
|
||||
*
|
||||
* Keep the set in step with `_FORBIDDEN_CHARS` in that module.
|
||||
*/
|
||||
export const KB_NAME_FORBIDDEN_CHARS = '<>:"/\\|?*#%';
|
||||
|
||||
export const KB_NAME_MAX_LENGTH = 120;
|
||||
|
||||
/**
|
||||
* Returns the offending characters, in the order the backend reports them, or
|
||||
* an empty array when the name is acceptable.
|
||||
*/
|
||||
export function forbiddenKbNameChars(name: string): string[] {
|
||||
const found = new Set(
|
||||
[...name].filter((ch) => KB_NAME_FORBIDDEN_CHARS.includes(ch)),
|
||||
);
|
||||
return [...found].sort();
|
||||
}
|
||||
|
||||
export function isValidKbName(name: string): boolean {
|
||||
const trimmed = name.trim();
|
||||
if (!trimmed || trimmed === "." || trimmed === "..") return false;
|
||||
if (trimmed.length > KB_NAME_MAX_LENGTH) return false;
|
||||
return forbiddenKbNameChars(trimmed).length === 0;
|
||||
}
|
||||
@@ -3999,5 +3999,6 @@
|
||||
"The clipboard is not available in this browser.": "The clipboard is not available in this browser.",
|
||||
"attachments": "attachments",
|
||||
"references": "references",
|
||||
"That question is no longer waiting for an answer, so this was sent as a new message.": "That question is no longer waiting for an answer, so this was sent as a new message."
|
||||
"That question is no longer waiting for an answer, so this was sent as a new message.": "That question is no longer waiting for an answer, so this was sent as a new message.",
|
||||
"A knowledge base name cannot contain {{chars}} — these separate paths and URLs.": "A knowledge base name cannot contain {{chars}} — these separate paths and URLs."
|
||||
}
|
||||
|
||||
@@ -3999,5 +3999,6 @@
|
||||
"The clipboard is not available in this browser.": "当前浏览器环境无法使用剪贴板。",
|
||||
"attachments": "附件",
|
||||
"references": "引用",
|
||||
"That question is no longer waiting for an answer, so this was sent as a new message.": "这道题已经不再等待作答,所以这条内容作为新消息发送了。"
|
||||
"That question is no longer waiting for an answer, so this was sent as a new message.": "这道题已经不再等待作答,所以这条内容作为新消息发送了。",
|
||||
"A knowledge base name cannot contain {{chars}} — these separate paths and URLs.": "知识库名称不能包含 {{chars}} —— 这些字符会切断路径和 URL。"
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user