From f22844be7e771f2b55f7c59001d09c8b7071e481 Mon Sep 17 00:00:00 2001 From: PARTH J ROHIT Date: Mon, 27 Jul 2026 13:00:15 +0100 Subject: [PATCH] security(backend): guard clone symlinks and require auth on /metrics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - github/client.py _directory_size no longer descends into symlinked directories (os.walk followlinks=False + prune symlink dirs), closing the clone-path symlink-escape disclosure/DoS gap (issue #182). - /metrics now requires authentication (Depends(get_current_user)); was world-readable runtime metrics (issue #183). - Updated OpenAPI contract + system tests to reflect /metrics as bearer-protected (401), matching existing contract-authority discipline. Note on #184: its 'arbitrary paths' claim is invalid — RepositoryContextBuilder validates the selected file against the sealed snapshot and only sends snapshot-derived paths. The omitted-review-findings half is a real but feature-sized gap (no review query path exists); tracked separately, not folded into this security PR. --- apps/backend/app/github/client.py | 18 ++++++++++++------ apps/backend/app/main.py | 5 ++++- apps/backend/tests/test_openapi_contract.py | 3 +-- apps/backend/tests/test_system.py | 6 +++--- 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/apps/backend/app/github/client.py b/apps/backend/app/github/client.py index 8d3d243..d0c3988 100644 --- a/apps/backend/app/github/client.py +++ b/apps/backend/app/github/client.py @@ -161,10 +161,16 @@ def _enforce_clone_size(self, destination: Path) -> None: def _directory_size(self, path: Path) -> int: total = 0 - for child in path.rglob("*"): - try: - if child.is_file() and not child.is_symlink(): - total += child.stat().st_size - except OSError: - continue + for root, dirs, files in os.walk(path, followlinks=False): + for name in files: + child = Path(root, name) + try: + if not child.is_symlink(): + total += child.stat().st_size + except OSError: + continue + # Do not descend into symlinked directories: prevents a crafted + # symlink (e.g. to / or a parent dir) from causing disclosure or a + # size-measurement loop during clone budget enforcement. + dirs[:] = [d for d in dirs if not Path(root, d).is_symlink()] return total diff --git a/apps/backend/app/main.py b/apps/backend/app/main.py index 0b2b5c9..64b1caa 100644 --- a/apps/backend/app/main.py +++ b/apps/backend/app/main.py @@ -7,7 +7,7 @@ from typing import Any, Literal from uuid import uuid4 -from fastapi import FastAPI, Request, status +from fastapi import Depends, FastAPI, Request, status from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import JSONResponse, PlainTextResponse from sqlalchemy import text @@ -15,6 +15,7 @@ from sqlalchemy import inspect as sa_inspect from app.api.router import api_router +from app.api.deps import get_current_user from app.api.openapi import ( documented_responses, remove_suppressed_automatic_validation_errors, @@ -296,11 +297,13 @@ def readiness() -> dict[str, object] | JSONResponse: @app.get( "/metrics", tags=["system"], + dependencies=[Depends(get_current_user)], response_class=PlainTextResponse, responses=documented_responses( status.HTTP_200_OK, "Prometheus-compatible runtime metrics.", "partha_http_requests_total 1\\n", + status.HTTP_401_UNAUTHORIZED, status.HTTP_500_INTERNAL_SERVER_ERROR, media_type="text/plain", schema={"type": "string"}, diff --git a/apps/backend/tests/test_openapi_contract.py b/apps/backend/tests/test_openapi_contract.py index f31710e..6e9339f 100644 --- a/apps/backend/tests/test_openapi_contract.py +++ b/apps/backend/tests/test_openapi_contract.py @@ -18,7 +18,6 @@ ("POST", "/auth/logout"), ("GET", "/health"), ("GET", "/ready"), - ("GET", "/metrics"), } # Refresh is unauthenticated in the HTTPBearer sense, but requires a valid @@ -63,7 +62,7 @@ ("POST", "/export"): {200, 401, 404, 409, 422, 429, 500}, ("GET", "/health"): {200, 500}, ("GET", "/ready"): {200, 503, 500}, - ("GET", "/metrics"): {200, 500}, + ("GET", "/metrics"): {200, 401, 500}, } BODY_MEDIA_TYPES = { diff --git a/apps/backend/tests/test_system.py b/apps/backend/tests/test_system.py index 7ac3e0c..c36121c 100644 --- a/apps/backend/tests/test_system.py +++ b/apps/backend/tests/test_system.py @@ -23,10 +23,10 @@ def test_readiness_endpoint(client): } -def test_metrics_endpoint_exposes_request_counters(client): - client.get("/health") +def test_metrics_endpoint_exposes_request_counters(auth_client): + auth_client.get("/health") - response = client.get("/metrics") + response = auth_client.get("/metrics") assert response.status_code == 200 assert "text/plain" in response.headers["content-type"]