This is an automated email from the ASF dual-hosted git repository. imbajin pushed a commit to branch goal-scan in repository https://gitbox.apache.org/repos/asf/hugegraph-ai.git
commit 84bc436f82771b12923f9e491a3a984b2a03864c Author: imbajin <[email protected]> AuthorDate: Sun May 31 11:44:43 2026 +0800 fix(llm): reject unsafe admin log paths - block traversal and absolute log paths before streaming - reject unset or default admin tokens for log access - add API contract coverage for admin log security - update code-scan ledgers for CS-001 --- .workflow/code-scan/code-scan-state.json | 6 +- .../code-scan/reports/final-code-scan-report.md | 6 +- .workflow/code-scan/reports/issues.md | 6 +- .workflow/code-scan/reports/test-quality-ledger.md | 6 +- hugegraph-llm/src/hugegraph_llm/api/admin_api.py | 34 ++++++- hugegraph-llm/src/tests/api/test_admin_api.py | 111 +++++++++++++++++++++ 6 files changed, 157 insertions(+), 12 deletions(-) diff --git a/.workflow/code-scan/code-scan-state.json b/.workflow/code-scan/code-scan-state.json index 1bc21dbc..25bea9be 100644 --- a/.workflow/code-scan/code-scan-state.json +++ b/.workflow/code-scan/code-scan-state.json @@ -54,6 +54,9 @@ "P5": 0 }, "issue_total": 37, + "fixes_completed": [ + "CS-001" + ], "files_touched": [ "docs/specs/2026-05-31-hugegraph-ai-code-scan-design.md", "docs/plans/2026-05-31-hugegraph-ai-code-scan.md", @@ -91,7 +94,6 @@ ], "style_only_fixes": [], "fixme_comments_added": [ - "hugegraph-llm/src/hugegraph_llm/api/admin_api.py", "hugegraph-llm/src/tests/api/test_rag_api.py", "hugegraph-llm/src/tests/config/test_config.py", "hugegraph-llm/src/tests/config/test_prompt_config.py", @@ -120,5 +122,5 @@ "uv run ruff check <edited-python-files>", "git status --short" ], - "next_recommended_action": "Fix CS-001 first, then address the client error-contract cluster and graph import contract drift." + "next_recommended_action": "Address the client error-contract cluster: CS-002, CS-004, and CS-008." } diff --git a/.workflow/code-scan/reports/final-code-scan-report.md b/.workflow/code-scan/reports/final-code-scan-report.md index e87d508b..a2639504 100644 --- a/.workflow/code-scan/reports/final-code-scan-report.md +++ b/.workflow/code-scan/reports/final-code-scan-report.md @@ -76,8 +76,12 @@ None. ## Recommended Next Fix Plan -1. Fix `CS-001` immediately: lock down `/logs` path handling and default admin credentials. +1. Fix `CS-001` immediately: lock down `/logs` path handling and default admin credentials. Status: fixed after the scan. 2. Fix the client error-contract cluster: `CS-002`, `CS-004`, `CS-008`. 3. Fix graph import correctness: `CS-013`, then add the missing round-trip tests. 4. Fix global config mutation and provider validation: `CS-011`, `CS-012`. 5. Replace fake integration tests with production-flow smoke coverage before broader refactors. + +## Fix Progress After Scan + +- `CS-001`: fixed by rejecting insecure default admin tokens and validating log file names before `log_stream()` receives a path. Verified with `SKIP_EXTERNAL_SERVICES=true uv run pytest hugegraph-llm/src/tests/api/test_admin_api.py -q`. diff --git a/.workflow/code-scan/reports/issues.md b/.workflow/code-scan/reports/issues.md index aa0e7f6f..1fc226ed 100644 --- a/.workflow/code-scan/reports/issues.md +++ b/.workflow/code-scan/reports/issues.md @@ -21,11 +21,11 @@ Issue IDs use `CS-NNN`. Priorities are P0 highest through P5 lowest. - Module: `hugegraph-llm` - Layer: API / config / security - Paths: `hugegraph-llm/src/hugegraph_llm/api/admin_api.py:33`, `hugegraph-llm/src/hugegraph_llm/api/admin_api.py:40`, `hugegraph-llm/src/hugegraph_llm/config/admin_config.py:26`, `hugegraph-llm/src/hugegraph_llm/demo/rag_demo/admin_block.py:27`, `hugegraph-llm/src/hugegraph_llm/demo/rag_demo/app.py:164` -- Status: open +- Status: fixed - Evidence: `log_file` is accepted from request input and joined with `logs` without basename normalization or allowlist. The injected `log_stream()` opens that path directly. Default auth is disabled and default tokens are predictable. - Impact: A caller with the default or known admin token can request paths outside `logs/`, exposing any readable file from the service process. -- Recommendation: Accept only server-side enum names for logs, reject absolute and traversal paths, require explicit secrets, and do not expose admin routes when auth is disabled. -- Test note: Added a `FIXME:` in `admin_api.py` for the missing path traversal contract test. +- Fix: `admin_http_api` now rejects unset/default admin tokens, rejects absolute paths, drive-qualified paths, path separators, and traversal values before calling `log_stream()`. +- Test note: Added `test_admin_api.py` coverage for traversal rejection, absolute-path rejection, default-token rejection, and valid log streaming under `logs/`. ### CS-002: Client request/error logs can leak auth secrets diff --git a/.workflow/code-scan/reports/test-quality-ledger.md b/.workflow/code-scan/reports/test-quality-ledger.md index 06019be5..07614020 100644 --- a/.workflow/code-scan/reports/test-quality-ledger.md +++ b/.workflow/code-scan/reports/test-quality-ledger.md @@ -4,7 +4,6 @@ Added specific `FIXME:` comments in these files: -- `hugegraph-llm/src/hugegraph_llm/api/admin_api.py` - `hugegraph-llm/src/tests/api/test_rag_api.py` - `hugegraph-llm/src/tests/config/test_config.py` - `hugegraph-llm/src/tests/config/test_prompt_config.py` @@ -36,7 +35,6 @@ Added specific `FIXME:` comments in these files: ## Missing Coverage Around Core Functions -- `admin_http_api` log path validation. - `HGraphSession.resolve()` with base URL path prefixes. - `ResponseValidation` success-response malformed JSON. - `GremlinManager.exec()` typed error propagation. @@ -56,3 +54,7 @@ Added specific `FIXME:` comments in these files: - `hugegraph-python-client/src/tests/api/test_response_validation.py` was run by L2 and passed: `uv run pytest hugegraph-python-client/src/tests/api/test_response_validation.py -q`. - L5 ran a targeted models/indices test set and reported `52 passed, 3 skipped`. + +## Resolved Coverage Gaps + +- `CS-001`: `hugegraph-llm/src/tests/api/test_admin_api.py` now covers admin log traversal rejection, absolute-path rejection, insecure default-token rejection, and valid log streaming. diff --git a/hugegraph-llm/src/hugegraph_llm/api/admin_api.py b/hugegraph-llm/src/hugegraph_llm/api/admin_api.py index d71330b7..87abc063 100644 --- a/hugegraph-llm/src/hugegraph_llm/api/admin_api.py +++ b/hugegraph-llm/src/hugegraph_llm/api/admin_api.py @@ -15,9 +15,10 @@ # specific language governing permissions and limitations # under the License. +import ntpath import os -from fastapi import APIRouter, status +from fastapi import APIRouter, HTTPException, status from fastapi.responses import StreamingResponse from hugegraph_llm.api.exceptions.rag_exceptions import generate_response @@ -25,11 +26,38 @@ from hugegraph_llm.api.models.rag_requests import LogStreamRequest from hugegraph_llm.api.models.rag_response import RAGResponse from hugegraph_llm.config import admin_settings +LOG_DIR = "logs" +INSECURE_ADMIN_TOKENS = {"", "xxxx"} + + +def _is_configured_admin_token(admin_token: str | None) -> bool: + return admin_token is not None and admin_token not in INSECURE_ADMIN_TOKENS + + +def _resolve_log_path(log_file: str | None) -> str: + if not log_file: + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Invalid log file name.") + if ( + os.path.isabs(log_file) + or ntpath.isabs(log_file) + or ntpath.splitdrive(log_file)[0] + or "/" in log_file + or "\\" in log_file + or os.path.normpath(log_file) in {"", ".", ".."} + ): + raise HTTPException(status_code=status.HTTP_400_BAD_REQUEST, detail="Invalid log file name.") + return os.path.join(LOG_DIR, log_file) + # FIXME: line 31: E0702: Raising dict while only classes or instances are allowed (raising-bad-type) def admin_http_api(router: APIRouter, log_stream): @router.post("/logs", status_code=status.HTTP_200_OK) async def log_stream_api(req: LogStreamRequest): + if not _is_configured_admin_token(admin_settings.admin_token): + raise HTTPException( + status_code=status.HTTP_403_FORBIDDEN, + detail="Admin token is not configured securely.", + ) if admin_settings.admin_token != req.admin_token: raise generate_response( # pylint: disable=raising-bad-type RAGResponse( @@ -37,9 +65,7 @@ def admin_http_api(router: APIRouter, log_stream): message="Invalid admin_token", ) ) - # FIXME: add an API contract test that rejects path traversal and - # absolute log paths before this value reaches the log stream. - log_path = os.path.join("logs", req.log_file) + log_path = _resolve_log_path(req.log_file) # Create a StreamingResponse that reads from the log stream generator return StreamingResponse(log_stream(log_path), media_type="text/plain") diff --git a/hugegraph-llm/src/tests/api/test_admin_api.py b/hugegraph-llm/src/tests/api/test_admin_api.py new file mode 100644 index 00000000..4032c78c --- /dev/null +++ b/hugegraph-llm/src/tests/api/test_admin_api.py @@ -0,0 +1,111 @@ +# 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. + +from unittest.mock import Mock + +import pytest +from fastapi import APIRouter, FastAPI, status +from fastapi.testclient import TestClient + +from hugegraph_llm.api.admin_api import admin_http_api +from hugegraph_llm.config import admin_settings + +pytestmark = pytest.mark.contract + + +async def _empty_log_stream(_log_path): + if False: + yield "" + + +def _make_client(log_stream=None): + router = APIRouter() + admin_http_api(router, log_stream or _empty_log_stream) + app = FastAPI() + app.include_router(router) + return TestClient(app) + + +def test_logs_rejects_path_traversal_before_streaming(monkeypatch): + monkeypatch.setattr(admin_settings, "admin_token", "secure-admin-token") + log_stream = Mock(side_effect=_empty_log_stream) + client = _make_client(log_stream) + + response = client.post( + "/logs", + json={ + "admin_token": "secure-admin-token", + "log_file": "../config_prompt.yaml", + }, + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["detail"] == "Invalid log file name." + log_stream.assert_not_called() + + +def test_logs_rejects_absolute_paths_before_streaming(monkeypatch): + monkeypatch.setattr(admin_settings, "admin_token", "secure-admin-token") + log_stream = Mock(side_effect=_empty_log_stream) + client = _make_client(log_stream) + + response = client.post( + "/logs", + json={ + "admin_token": "secure-admin-token", + "log_file": "/etc/passwd", + }, + ) + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert response.json()["detail"] == "Invalid log file name." + log_stream.assert_not_called() + + +def test_logs_rejects_default_admin_token(monkeypatch): + monkeypatch.setattr(admin_settings, "admin_token", "xxxx") + log_stream = Mock(side_effect=_empty_log_stream) + client = _make_client(log_stream) + + response = client.post( + "/logs", + json={ + "admin_token": "xxxx", + "log_file": "llm-server.log", + }, + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN + assert response.json()["detail"] == "Admin token is not configured securely." + log_stream.assert_not_called() + + +def test_logs_streams_valid_log_file_under_logs_dir(monkeypatch): + monkeypatch.setattr(admin_settings, "admin_token", "secure-admin-token") + log_stream = Mock(side_effect=_empty_log_stream) + client = _make_client(log_stream) + + response = client.post( + "/logs", + json={ + "admin_token": "secure-admin-token", + "log_file": "llm-server.log", + }, + ) + + assert response.status_code == status.HTTP_200_OK + log_stream.assert_called_once_with("logs/llm-server.log")
