VGalaxies commented on code in PR #368: URL: https://github.com/apache/hugegraph-ai/pull/368#discussion_r3492497329
########## hugegraph-llm/src/hugegraph_llm/api/thin_api.py: ########## @@ -0,0 +1,122 @@ +# 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. + +import time +from typing import Any +from uuid import uuid4 + +from fastapi import APIRouter, status + +from hugegraph_llm.api.models.rag_requests import ( + GraphExtractRequest, + GraphImportRequest, + VidEmbeddingsRefreshRequest, +) +from hugegraph_llm.api.models.rag_response import ThinAPIResponse +from hugegraph_llm.flows import FlowName +from hugegraph_llm.flows.scheduler import SchedulerSingleton +from hugegraph_llm.utils.log import log + +thin_router = APIRouter() + + +def _generate_request_id() -> str: + return f"req-{uuid4().hex[:12]}" + + +def _envelope_ok( + data: Any, *, warnings: list[str] | None = None, next_actions: list[str] | None = None +) -> dict[str, Any]: + return { + "ok": True, + "data": data, + "error": None, + "warnings": warnings or [], + "next_actions": next_actions or [], + "meta": { + "request_id": _generate_request_id(), + "duration_ms": 0, + }, + } + + +def _envelope_err( + error_type: str, message: str, *, suggestion: str | None = None, details: Any = None +) -> dict[str, Any]: + return { + "ok": False, + "data": None, + "error": { + "type": error_type, + "message": message, + "suggestion": suggestion, + "retryable": False, + "source": "hugegraph-llm", + "details": details if details is not None else {}, + }, + "warnings": [], + "next_actions": [], + "meta": { + "request_id": _generate_request_id(), + "duration_ms": 0, + }, + } + + +def _wrap_flow_call(flow_name: FlowName, *args: Any) -> dict[str, Any]: + start = time.perf_counter() + try: + result = SchedulerSingleton.get_instance().schedule_flow(flow_name, *args) + envelope = _envelope_ok(result) + envelope["meta"]["duration_ms"] = (time.perf_counter() - start) * 1000.0 + return envelope + except Exception as exc: + log.error("Thin API flow execution failed: %s", exc, exc_info=True) + envelope = _envelope_err( + "FLOW_EXECUTION_FAILED", + "An internal error occurred during flow execution.", + suggestion="Check HugeGraph-AI service logs for details.", + ) + envelope["meta"]["duration_ms"] = (time.perf_counter() - start) * 1000.0 + return envelope + + +@thin_router.post("/graph-extract", status_code=status.HTTP_200_OK, response_model=ThinAPIResponse) +def graph_extract_api(req: GraphExtractRequest): + return _wrap_flow_call( + FlowName.GRAPH_EXTRACT, + req.graph_schema, + req.text, + req.example_prompt, + "property_graph", + req.language, Review Comment: **High: `/graph-extract` passes language as `split_type`** `hugegraph-llm/src/hugegraph_llm/api/thin_api.py:106` **Evidence** - `graph_extract_api()` passes `req.language` as the fifth flow argument, but `GraphExtractFlow.prepare()` expects `split_type` in that position and rejects anything except `document`, `paragraph`, or `sentence`. **Impact** - Normal requests with `language="zh"` or `"en"` fail before extraction runs, so the MCP graph extraction path returns `FLOW_EXECUTION_FAILED`. **Requested fix** - Pass the default split type, e.g. `"document"`, before `req.language`, or call the flow with explicit keyword arguments; update the thin API test to assert the real flow contract. ########## hugegraph-llm/src/hugegraph_llm/api/thin_api.py: ########## @@ -0,0 +1,122 @@ +# 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. + +import time +from typing import Any +from uuid import uuid4 + +from fastapi import APIRouter, status + +from hugegraph_llm.api.models.rag_requests import ( + GraphExtractRequest, + GraphImportRequest, + VidEmbeddingsRefreshRequest, +) +from hugegraph_llm.api.models.rag_response import ThinAPIResponse +from hugegraph_llm.flows import FlowName +from hugegraph_llm.flows.scheduler import SchedulerSingleton +from hugegraph_llm.utils.log import log + +thin_router = APIRouter() + + +def _generate_request_id() -> str: + return f"req-{uuid4().hex[:12]}" + + +def _envelope_ok( + data: Any, *, warnings: list[str] | None = None, next_actions: list[str] | None = None +) -> dict[str, Any]: + return { + "ok": True, + "data": data, + "error": None, + "warnings": warnings or [], + "next_actions": next_actions or [], + "meta": { + "request_id": _generate_request_id(), + "duration_ms": 0, + }, + } + + +def _envelope_err( + error_type: str, message: str, *, suggestion: str | None = None, details: Any = None +) -> dict[str, Any]: + return { + "ok": False, + "data": None, + "error": { + "type": error_type, + "message": message, + "suggestion": suggestion, + "retryable": False, + "source": "hugegraph-llm", + "details": details if details is not None else {}, + }, + "warnings": [], + "next_actions": [], + "meta": { + "request_id": _generate_request_id(), + "duration_ms": 0, + }, + } + + +def _wrap_flow_call(flow_name: FlowName, *args: Any) -> dict[str, Any]: + start = time.perf_counter() + try: + result = SchedulerSingleton.get_instance().schedule_flow(flow_name, *args) + envelope = _envelope_ok(result) + envelope["meta"]["duration_ms"] = (time.perf_counter() - start) * 1000.0 + return envelope + except Exception as exc: + log.error("Thin API flow execution failed: %s", exc, exc_info=True) + envelope = _envelope_err( + "FLOW_EXECUTION_FAILED", + "An internal error occurred during flow execution.", + suggestion="Check HugeGraph-AI service logs for details.", + ) + envelope["meta"]["duration_ms"] = (time.perf_counter() - start) * 1000.0 + return envelope + + +@thin_router.post("/graph-extract", status_code=status.HTTP_200_OK, response_model=ThinAPIResponse) +def graph_extract_api(req: GraphExtractRequest): + return _wrap_flow_call( + FlowName.GRAPH_EXTRACT, + req.graph_schema, + req.text, + req.example_prompt, + "property_graph", + req.language, + ) + + +@thin_router.post("/graph-import", status_code=status.HTTP_200_OK, response_model=ThinAPIResponse) Review Comment: **High: Thin write endpoints bypass MCP write controls** `hugegraph-llm/src/hugegraph_llm/api/thin_api.py:110` **Evidence** - `/graph-import` directly schedules `FlowName.IMPORT_GRAPH_DATA`, and `/vid-embeddings/refresh` directly schedules `FlowName.UPDATE_VID_EMBEDDINGS`; the new router is included in `app.py` under auth that defaults to disabled. **Impact** - A default HugeGraph-LLM deployment exposes graph import and VID embedding mutation without MCP readonly/admin gating or the `dry_run -> plan_hash -> confirm` controls. **Requested fix** - Remove these mutating routes from the public thin router, or require explicit authentication/admin authorization plus the same readonly and confirm/dry-run controls before scheduling mutating flows. ########## hugegraph-mcp/hugegraph_mcp/plan_hash.py: ########## @@ -0,0 +1,210 @@ +# 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. + +"""Target-bound plan hash — 防止跨图、跨用户、过期重放。 + +PlanContext 将工具名、模式、图目标、主体、readonly、payload 摘要、 +schema 摘要、nonce 和过期时间绑定到一个哈希中。 +confirm 时重新计算并比较，拒绝不匹配或过期的计划。 +""" + +from __future__ import annotations + +import hashlib +import json +import time +from dataclasses import asdict, dataclass, field +from typing import Any + +from hugegraph_mcp.config import MCPConfig +from hugegraph_mcp.envelope import ErrorType + + +# 默认计划有效期（秒） +DEFAULT_PLAN_TTL_SECONDS = 600 # 10 分钟 + + +@dataclass(frozen=True) +class PlanContext: + """计划上下文 — 绑定到特定工具、目标和时间窗口。""" + + tool_name: str + mode: str + graph_url: str + graph_name: str + graphspace: str + principal: str + readonly: bool + payload_digest: str + schema_hash: str | None + nonce: str + expires_at: int + extra_context: dict[str, Any] = field(default_factory=dict) + + +def compute_plan_hash(context: PlanContext) -> str: Review Comment: **High: Confirm plan hashes are client-forgeable** `hugegraph-mcp/hugegraph_mcp/plan_hash.py:55` **Evidence** - `compute_plan_hash()` is a plain public SHA-256 over `PlanContext`, and `verify_plan_hash()` recomputes it from caller-supplied `nonce` and `expires_at`; `manage_graph_data.py:221` passes those submitted values directly. **Impact** - A caller with writes enabled can compute a valid hash and submit `confirm=True` without first receiving a server-issued dry-run token, bypassing the documented review/confirm safety chain and choosing an arbitrary future expiry. **Requested fix** - Make confirm tokens server-issued and unforgeable, for example with server-side one-time plan records or an HMAC using a server secret, and enforce a bounded TTL. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
