From 41749fd7b4b0db7171311bf119accf99a1d0f968 Mon Sep 17 00:00:00 2001 From: Latte Date: Thu, 25 Jun 2026 16:51:58 +0200 Subject: [PATCH] fix: harden get_issue parsing and surface real errors (#27); align CI image publish get_issue raised 'NoneType' object is not iterable on issues whose labels/assignees Gitea returns as null or with non-dict elements (the #13 class), which reached clients as an opaque JSON-RPC -32603 with no detail. - read_tools: skip non-dict label/assignee entries in get_issue_tool - server: detect a wrapped GiteaNotFoundError via the __cause__ chain and return 404 / JSON-RPC -32000 with a clear message; include the exception type name in masked internal errors so future masked failures are diagnosable without exposing messages or stack traces - tests: cover non-dict collection elements and the not-found / typed-error responses - ci: rewrite docker.yml to build, smoke-test and push the image to the Gitea container registry on merge to main/dev, matching the hiddenden.cafe pattern (only REGISTRY_TOKEN required) Co-Authored-By: Claude Opus 4.8 (1M context) --- .gitea/workflows/docker.yml | 256 +++++++++++------------- src/aegis_gitea_mcp/server.py | 76 ++++++- src/aegis_gitea_mcp/tools/read_tools.py | 12 +- tests/test_server.py | 80 ++++++++ tests/test_tools_extended.py | 27 +++ 5 files changed, 295 insertions(+), 156 deletions(-) diff --git a/.gitea/workflows/docker.yml b/.gitea/workflows/docker.yml index a5a0990..0e17a26 100644 --- a/.gitea/workflows/docker.yml +++ b/.gitea/workflows/docker.yml @@ -1,157 +1,125 @@ name: docker on: - push: - branches: - - main - - dev - pull_request: - branches: - - main - - dev - pull_request_review: - types: - - submitted + # Test on every branch push; registry push is gated per-step to main/dev. + push: + branches: + - '**' + pull_request: + branches: + - main + - dev jobs: - lint: - if: ${{ github.event_name != 'pull_request_review' || github.event.review.state == 'approved' }} - runs-on: ubuntu-latest - steps: - - name: Checkout - uses: actions/checkout@v4 - - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: "3.12" - - name: Install dependencies - run: | - python -m pip install --upgrade pip - pip install -r requirements-dev.txt - - name: Run lint - run: | - ruff check src tests - ruff format --check src tests - black --check src tests - mypy src + # --------------------------------------------------------------------------- + # 1. Lint: ruff + black + mypy. + # --------------------------------------------------------------------------- + lint: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r requirements-dev.txt + - name: Run lint + run: | + ruff check src tests + ruff format --check src tests + black --check src tests + mypy src - test: - if: ${{ github.event_name != 'pull_request_review' || github.event.review.state == 'approved' }} - runs-on: ubuntu-latest - steps: - - name: Checkout - uses: actions/checkout@v4 - - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: "3.12" - - name: Install dependencies - run: | - python -m pip install --upgrade pip - pip install -r requirements-dev.txt - - name: Run tests - run: pytest --cov=aegis_gitea_mcp --cov-report=term-missing --cov-fail-under=80 + # --------------------------------------------------------------------------- + # 2. Test: pytest with coverage gate. + # --------------------------------------------------------------------------- + test: + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: "3.12" + - name: Install dependencies + run: | + python -m pip install --upgrade pip + pip install -r requirements-dev.txt + - name: Run tests + run: pytest --cov=aegis_gitea_mcp --cov-report=term-missing --cov-fail-under=80 - docker-test: - if: ${{ github.event_name != 'pull_request_review' || github.event.review.state == 'approved' }} - runs-on: ubuntu-latest - needs: [lint, test] - env: - IMAGE_NAME: aegis-gitea-mcp - steps: - - name: Checkout - uses: actions/checkout@v4 + # --------------------------------------------------------------------------- + # 3. Build the Docker image, smoke-test it, push to Gitea (push events to + # main/dev only), then clean up so nothing lingers on the self-hosted + # runner. + # --------------------------------------------------------------------------- + docker: + needs: [lint, test] + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@v4 - - name: Build candidate image - run: | - SHA_TAG="${GITHUB_SHA:-${CI_COMMIT_SHA:-local}}" - docker build -f docker/Dockerfile -t ${IMAGE_NAME}:${SHA_TAG} . + - name: Compute image name & tags + id: meta + shell: bash + run: | + IMAGE="git.hiddenden.cafe/${GITHUB_REPOSITORY,,}" + echo "image=${IMAGE}" >> "$GITHUB_OUTPUT" + echo "sha_tag=${IMAGE}:sha-${GITHUB_SHA::12}" >> "$GITHUB_OUTPUT" + if [ "${GITHUB_REF_NAME}" = "main" ]; then + # Production: stable :latest + :main + echo "branch_tags=${IMAGE}:latest ${IMAGE}:main" >> "$GITHUB_OUTPUT" + else + # dev (and any other branch): tag with the branch name + echo "branch_tags=${IMAGE}:${GITHUB_REF_NAME}" >> "$GITHUB_OUTPUT" + fi - - name: Smoke-test image - run: | - SHA_TAG="${GITHUB_SHA:-${CI_COMMIT_SHA:-local}}" - docker run --rm --entrypoint python ${IMAGE_NAME}:${SHA_TAG} -c "import aegis_gitea_mcp" + - name: Build image + shell: bash + run: docker build -f docker/Dockerfile -t "${{ steps.meta.outputs.sha_tag }}" . - docker-publish: - runs-on: ubuntu-latest - needs: [lint, test, docker-test] - if: >- - (github.event_name == 'push' && (github.ref_name == 'main' || github.ref_name == 'dev')) || - (github.event_name == 'pull_request_review' && - github.event.review.state == 'approved' && - (github.event.pull_request.base.ref == 'main' || github.event.pull_request.base.ref == 'dev')) - env: - IMAGE_NAME: aegis-gitea-mcp - REGISTRY_IMAGE: ${{ vars.REGISTRY_IMAGE }} - REGISTRY_HOST: ${{ vars.REGISTRY_HOST }} - PR_BASE_REF: ${{ github.event.pull_request.base.ref }} - PR_HEAD_SHA: ${{ github.event.pull_request.head.sha }} - REGISTRY_USER: ${{ secrets.REGISTRY_USER }} - REGISTRY_TOKEN: ${{ secrets.REGISTRY_TOKEN }} - steps: - - name: Checkout - uses: actions/checkout@v4 - with: - ref: ${{ github.event.pull_request.head.sha || github.sha }} + - name: Smoke-test image + shell: bash + run: | + docker run --rm --entrypoint python "${{ steps.meta.outputs.sha_tag }}" \ + -c "import aegis_gitea_mcp" + echo "Image imports cleanly." - - name: Resolve tags - id: tags - run: | - EVENT_NAME="${GITHUB_EVENT_NAME:-${CI_EVENT_NAME:-}}" - REF_NAME="${GITHUB_REF_NAME:-${CI_COMMIT_REF_NAME:-}}" - BASE_REF="${PR_BASE_REF:-${GITHUB_BASE_REF:-${CI_BASE_REF:-}}}" - SHA_TAG="${GITHUB_SHA:-${CI_COMMIT_SHA:-local}}" + - name: Log in to Gitea Container Registry + if: github.event_name == 'push' && (github.ref_name == 'main' || github.ref_name == 'dev') + uses: docker/login-action@v3 + with: + registry: git.hiddenden.cafe + username: ${{ github.actor }} + # PAT with write:package scope, stored as the REGISTRY_TOKEN secret. + # The auto-provided GITEA_TOKEN lacks package-write permission on + # this instance, so we use a dedicated token here. + password: ${{ secrets.REGISTRY_TOKEN }} - if [ "${EVENT_NAME}" = "pull_request_review" ]; then - TARGET_BRANCH="${BASE_REF}" - SHA_TAG="${PR_HEAD_SHA:-$SHA_TAG}" - else - TARGET_BRANCH="${REF_NAME}" - fi + - name: Tag & push + if: github.event_name == 'push' && (github.ref_name == 'main' || github.ref_name == 'dev') + shell: bash + run: | + for tag in ${{ steps.meta.outputs.branch_tags }} ${{ steps.meta.outputs.sha_tag }}; do + docker tag "${{ steps.meta.outputs.sha_tag }}" "$tag" + docker push "$tag" + echo "Pushed $tag" + done - if [ "${TARGET_BRANCH}" = "main" ]; then - STABLE_TAG="latest" - elif [ "${TARGET_BRANCH}" = "dev" ]; then - STABLE_TAG="dev" - else - echo "Unsupported target branch '${TARGET_BRANCH}'" - exit 1 - fi - - echo "sha_tag=${SHA_TAG}" >> "${GITHUB_OUTPUT}" - echo "stable_tag=${STABLE_TAG}" >> "${GITHUB_OUTPUT}" - - - name: Build releasable image - id: image - run: | - IMAGE_REF="${REGISTRY_IMAGE:-${IMAGE_NAME}}" - echo "image_ref=${IMAGE_REF}" >> "${GITHUB_OUTPUT}" - docker build -f docker/Dockerfile -t ${IMAGE_REF}:${{ steps.tags.outputs.sha_tag }} . - docker tag ${IMAGE_REF}:${{ steps.tags.outputs.sha_tag }} ${IMAGE_REF}:${{ steps.tags.outputs.stable_tag }} - - - name: Login to registry - if: ${{ vars.PUSH_IMAGE == 'true' }} - run: | - if [ -z "${REGISTRY_USER}" ] || [ -z "${REGISTRY_TOKEN}" ]; then - echo "REGISTRY_USER and REGISTRY_TOKEN secrets are required when PUSH_IMAGE=true" - exit 1 - fi - - IMAGE_REF="${{ steps.image.outputs.image_ref }}" - LOGIN_HOST="${REGISTRY_HOST}" - if [ -z "${LOGIN_HOST}" ]; then - FIRST_PART="${IMAGE_REF%%/*}" - case "${FIRST_PART}" in - *.*|*:*|localhost) LOGIN_HOST="${FIRST_PART}" ;; - *) LOGIN_HOST="docker.io" ;; - esac - fi - - printf "%s" "${REGISTRY_TOKEN}" | docker login "${LOGIN_HOST}" --username "${REGISTRY_USER}" --password-stdin - - - name: Optional registry push - if: ${{ vars.PUSH_IMAGE == 'true' }} - run: | - IMAGE_REF="${{ steps.image.outputs.image_ref }}" - docker push ${IMAGE_REF}:${{ steps.tags.outputs.sha_tag }} - docker push ${IMAGE_REF}:${{ steps.tags.outputs.stable_tag }} + # Always runs — removes exactly what this run created, even on failure. + # Scoped on purpose: if the runner shares the host Docker daemon, a global + # prune would also wipe other homelab services. We never create volumes + # here, so only dangling images + build cache are swept. + - name: Cleanup + if: always() + shell: bash + run: | + docker rmi -f ${{ steps.meta.outputs.sha_tag }} ${{ steps.meta.outputs.branch_tags }} || true + docker image prune -f || true + docker builder prune -f || true diff --git a/src/aegis_gitea_mcp/server.py b/src/aegis_gitea_mcp/server.py index 261f248..087ba18 100644 --- a/src/aegis_gitea_mcp/server.py +++ b/src/aegis_gitea_mcp/server.py @@ -26,6 +26,7 @@ from aegis_gitea_mcp.gitea_client import ( GiteaAuthenticationError, GiteaAuthorizationError, GiteaClient, + GiteaNotFoundError, ) from aegis_gitea_mcp.logging_utils import configure_logging from aegis_gitea_mcp.mcp_protocol import ( @@ -128,6 +129,39 @@ _REAUTH_GUIDANCE = ( "and in your client, then re-authorize." ) +_NOT_FOUND_MESSAGE = "Resource not found in Gitea (it may not exist or be inaccessible)." + + +def _find_not_found(exc: BaseException) -> GiteaNotFoundError | None: + """Return the GiteaNotFoundError in an exception's cause chain, if any. + + Tool handlers wrap backend ``GiteaError`` (including ``GiteaNotFoundError``) + in ``RuntimeError`` before it reaches the request layer, so a not-found + condition is preserved only via ``__cause__``. Walking the chain lets the + server return an actionable "not found" instead of an opaque internal error. + """ + seen: set[int] = set() + current: BaseException | None = exc + while current is not None and id(current) not in seen: + if isinstance(current, GiteaNotFoundError): + return current + seen.add(id(current)) + current = current.__cause__ + return None + + +def _masked_internal_error(exc: BaseException, expose_details: bool) -> str: + """Build a non-sensitive internal-error message. + + The exception *type* name (e.g. ``TypeError``) carries no secrets or stack + detail, so it is always included to make masked failures diagnosable + client-side. The exception message is added only when explicitly enabled. + """ + if expose_details: + return f"Internal server error: {exc}" + return f"Internal server error ({type(exc).__name__})" + + _repo_authz_cache: BoundedTTLCache[str, bool] | None = None @@ -1337,12 +1371,25 @@ async def call_tool(request: MCPToolCallRequest) -> JSONResponse: ).model_dump(), ) - except Exception: - # Security decision: do not leak stack traces or raw exception messages. - error_message = "Internal server error" - if settings.expose_error_details: - error_message = "Internal server error (details hidden unless explicitly enabled)" + except Exception as exc: + if _find_not_found(exc) is not None: + audit.log_tool_invocation( + tool_name=request.tool, + correlation_id=correlation_id, + result_status="error", + error="gitea_not_found", + ) + return JSONResponse( + status_code=404, + content=MCPToolCallResponse( + success=False, + error=_NOT_FOUND_MESSAGE, + correlation_id=correlation_id, + ).model_dump(), + ) + # Security decision: do not leak stack traces or raw exception messages; + # the exception type name alone is safe and aids diagnosis. audit.log_tool_invocation( tool_name=request.tool, correlation_id=correlation_id, @@ -1354,7 +1401,7 @@ async def call_tool(request: MCPToolCallRequest) -> JSONResponse: status_code=500, content=MCPToolCallResponse( success=False, - error=error_message, + error=_masked_internal_error(exc, settings.expose_error_details), correlation_id=correlation_id, ).model_dump(), ) @@ -1503,14 +1550,23 @@ async def sse_message_handler(request: Request) -> JSONResponse: result_status="error", error=str(exc), ) - message = "Internal server error" - if settings.expose_error_details: - message = str(exc) + if _find_not_found(exc) is not None: + # -32000 (application error), matching the auth-error envelope. + return JSONResponse( + content={ + "jsonrpc": "2.0", + "id": message_id, + "error": {"code": -32000, "message": _NOT_FOUND_MESSAGE}, + } + ) return JSONResponse( content={ "jsonrpc": "2.0", "id": message_id, - "error": {"code": -32603, "message": message}, + "error": { + "code": -32603, + "message": _masked_internal_error(exc, settings.expose_error_details), + }, } ) diff --git a/src/aegis_gitea_mcp/tools/read_tools.py b/src/aegis_gitea_mcp/tools/read_tools.py index 34d44b2..ec6c887 100644 --- a/src/aegis_gitea_mcp/tools/read_tools.py +++ b/src/aegis_gitea_mcp/tools/read_tools.py @@ -295,8 +295,16 @@ async def get_issue_tool(gitea: GiteaClient, arguments: dict[str, Any]) -> dict[ "body": limit_text(str(issue.get("body", ""))), "state": issue.get("state", ""), "author": (issue.get("user") or {}).get("login", ""), - "labels": [label.get("name", "") for label in (issue.get("labels") or [])], - "assignees": [assignee.get("login", "") for assignee in (issue.get("assignees") or [])], + "labels": [ + label.get("name", "") + for label in (issue.get("labels") or []) + if isinstance(label, dict) + ], + "assignees": [ + assignee.get("login", "") + for assignee in (issue.get("assignees") or []) + if isinstance(assignee, dict) + ], "created_at": issue.get("created_at", ""), "updated_at": issue.get("updated_at", ""), "url": issue.get("html_url", ""), diff --git a/tests/test_server.py b/tests/test_server.py index 9861289..2b2e901 100644 --- a/tests/test_server.py +++ b/tests/test_server.py @@ -499,6 +499,86 @@ def test_sse_tools_call_http_exception(client: TestClient, monkeypatch: pytest.M assert "insufficient scope" in body["error"]["message"].lower() +def test_tool_call_not_found_maps_to_404( + client: TestClient, monkeypatch: pytest.MonkeyPatch +) -> None: + """A GiteaNotFoundError wrapped in RuntimeError surfaces as a clear 404.""" + from aegis_gitea_mcp.gitea_client import GiteaNotFoundError + + async def _fake_execute(_tool: str, _args: dict, _cid: str) -> dict: + try: + raise GiteaNotFoundError("Resource not found") + except GiteaNotFoundError as exc: + raise RuntimeError("Failed to get issue: Resource not found") from exc + + monkeypatch.setattr("aegis_gitea_mcp.server._execute_tool_call", _fake_execute) + + response = client.post( + "/mcp/tool/call", + headers={"Authorization": "Bearer valid-read"}, + json={"tool": "get_issue", "arguments": {"owner": "a", "repo": "b", "issue_number": 1}}, + ) + + assert response.status_code == 404 + assert "not found" in response.json()["error"].lower() + + +def test_tool_call_internal_error_includes_exception_type( + client: TestClient, monkeypatch: pytest.MonkeyPatch +) -> None: + """Masked internal errors name the exception type (safe) but never the message.""" + + async def _fake_execute(_tool: str, _args: dict, _cid: str) -> dict: + raise TypeError("'NoneType' object is not iterable") + + monkeypatch.setattr("aegis_gitea_mcp.server._execute_tool_call", _fake_execute) + + response = client.post( + "/mcp/tool/call", + headers={"Authorization": "Bearer valid-read"}, + json={"tool": "get_issue", "arguments": {"owner": "a", "repo": "b", "issue_number": 1}}, + ) + + assert response.status_code == 500 + error = response.json()["error"] + assert "TypeError" in error + assert "NoneType" not in error + + +def test_sse_tools_call_not_found_returns_jsonrpc_error( + client: TestClient, monkeypatch: pytest.MonkeyPatch +) -> None: + """A wrapped GiteaNotFoundError in the SSE path returns -32000 with a clear message.""" + from aegis_gitea_mcp.gitea_client import GiteaNotFoundError + + async def _fake_execute(_tool: str, _args: dict, _cid: str) -> dict: + try: + raise GiteaNotFoundError("Resource not found") + except GiteaNotFoundError as exc: + raise RuntimeError("Failed to get issue: Resource not found") from exc + + monkeypatch.setattr("aegis_gitea_mcp.server._execute_tool_call", _fake_execute) + + response = client.post( + "/mcp/sse", + headers={"Authorization": "Bearer valid-read"}, + json={ + "jsonrpc": "2.0", + "id": "nf-1", + "method": "tools/call", + "params": { + "name": "get_issue", + "arguments": {"owner": "a", "repo": "b", "issue_number": 1}, + }, + }, + ) + + assert response.status_code == 200 + body = response.json() + assert body["error"]["code"] == -32000 + assert "not found" in body["error"]["message"].lower() + + def test_call_nonexistent_tool(client: TestClient) -> None: """Unknown tools return 404 after successful auth.""" response = client.post( diff --git a/tests/test_tools_extended.py b/tests/test_tools_extended.py index 0b9df0a..4c93b3d 100644 --- a/tests/test_tools_extended.py +++ b/tests/test_tools_extended.py @@ -303,6 +303,33 @@ async def test_get_issue_tolerates_null_collections() -> None: assert result["assignees"] == [] +@pytest.mark.asyncio +async def test_get_issue_skips_non_dict_collection_elements() -> None: + """Defense-in-depth for #27: tolerate non-dict entries inside labels/assignees. + + A stray null/non-object element would otherwise raise AttributeError when + `.get()` is called on it, surfacing as an opaque internal error. + """ + + class MalformedElementsGitea(StubGitea): + async def get_issue(self, owner, repo, index): + return { + "number": index, + "title": "Issue", + "body": "Body", + "state": "open", + "user": {"login": "alice"}, + "labels": [{"name": "bug"}, None, "weird"], + "assignees": [{"login": "bob"}, None, 42], + } + + result = await get_issue_tool( + MalformedElementsGitea(), {"owner": "acme", "repo": "app", "issue_number": 1} + ) + assert result["labels"] == ["bug"] + assert result["assignees"] == ["bob"] + + @pytest.mark.asyncio @pytest.mark.parametrize( "tool,args,expected_key",