From 433560bea276c1ef11d17323c471c033fb75300b Mon Sep 17 00:00:00 2001 From: aditeyabaral Date: Sun, 13 Sep 2026 09:46:04 -0500 Subject: [PATCH 1/6] feat: allow /metrics to require a bearer token `/metrics` publishes request volumes, error counts, login success ratios, upstream PESU latency and restart times to anyone who asks. It is also unscrapeable by Grafana Cloud's agentless Metrics Endpoint integration, which refuses a target that is not behind authentication -- and that integration is what avoids hosting a collector. Set `METRICS_TOKEN` and the endpoint requires that bearer token, answering 401 with `WWW-Authenticate: Bearer` otherwise. Leave it unset and behaviour is byte-identical to before, so nothing breaks on merge. Both `fmt` values are covered, so the format selector is not a way around it. This is the first environment variable the app reads: config is CLI-only today and the Dockerfile's CMD is fixed, so an env var is the only channel the host can reach. `HTTPBearer(auto_error=False)` never raises on its own -- every failure path returns None -- so the 401 is raised as a `PESUAcademyError` subclass and comes out in this API's `{status, message, timestamp}` shape and in `errors_total{type="MetricsAuthorizationError"}`. `HTTPBasic` could not be used the same way: it raises `HTTPException` on a malformed credential regardless of `auto_error`, which would answer in Starlette's `{"detail": ...}` shape and bypass the error metric entirely. `PESUAcademyError` gains an optional `headers`, which is None for every existing error, so the 401 can carry `WWW-Authenticate` without a second handler. The OpenAPI override is untouched: FastAPI emits the `MetricsToken` scheme and the operation's security requirement from the dependency, and adds no phantom 422, so the existing stripper keeps working. Verified against the installed FastAPI rather than assumed. 20 new tests, 11 of which fail if enforcement is removed. 260 tests, 100% coverage. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VU7YUhP71KSsotWQ1H7CRH --- app/app.py | 7 +- app/docs/metrics.py | 17 +++++ app/exceptions/base.py | 7 +- app/exceptions/metrics.py | 13 ++++ app/metrics/auth.py | 60 ++++++++++++++++ tests/unit/test_metrics_auth.py | 124 ++++++++++++++++++++++++++++++++ tests/unit/test_openapi_docs.py | 25 +++++++ 7 files changed, 250 insertions(+), 3 deletions(-) create mode 100644 app/exceptions/metrics.py create mode 100644 app/metrics/auth.py create mode 100644 tests/unit/test_metrics_auth.py diff --git a/app/app.py b/app/app.py index 07f005f..86fc52a 100644 --- a/app/app.py +++ b/app/app.py @@ -12,7 +12,7 @@ from zoneinfo import ZoneInfo import uvicorn -from fastapi import FastAPI +from fastapi import Depends, FastAPI from fastapi.exceptions import RequestValidationError from fastapi.responses import JSONResponse, PlainTextResponse, RedirectResponse @@ -27,6 +27,7 @@ from app.docs import authenticate_docs, health_docs, metrics_docs, readme_docs from app.exceptions.base import PESUAcademyError +from app.metrics.auth import require_metrics_token from app.metrics.collector import ( AUTHENTICATION_REQUESTS, AUTHENTICATION_RESULTS, @@ -223,6 +224,7 @@ async def pesu_exception_handler(request: Request, exc: PESUAcademyError) -> JSO "message": exc.message, "timestamp": datetime.datetime.now(IST).isoformat(), }, + headers=exc.headers, ) @@ -267,6 +269,9 @@ async def health() -> JSONResponse: response_model=None, responses=metrics_docs.response_examples, tags=["Monitoring"], + # Enforced only when METRICS_TOKEN is set in the environment; open otherwise, which is what + # every existing caller and the local Docker instructions expect. + dependencies=[Depends(require_metrics_token)], ) async def metrics_endpoint(fmt: MetricsFormat = MetricsFormat.PROMETHEUS) -> Response: """Expose the collected metrics. diff --git a/app/docs/metrics.py b/app/docs/metrics.py index 2851a01..95f953d 100644 --- a/app/docs/metrics.py +++ b/app/docs/metrics.py @@ -175,6 +175,23 @@ } }, }, + 401: { + "description": ( + "The server has `METRICS_TOKEN` set and the request did not present it. The " + "response carries `WWW-Authenticate: Bearer`. While `METRICS_TOKEN` is unset this " + "cannot occur and the endpoint needs no credentials." + ), + "model": ResponseModel, + "content": { + "application/json": { + "example": { + "status": False, + "message": "Invalid or missing metrics token.", + "timestamp": "2024-07-28T22:30:10.103368+05:30", + } + } + }, + }, 500: _INTERNAL_SERVER_ERROR, }, ) diff --git a/app/exceptions/base.py b/app/exceptions/base.py index 6692ded..b447ae7 100644 --- a/app/exceptions/base.py +++ b/app/exceptions/base.py @@ -4,10 +4,13 @@ class PESUAcademyError(Exception): """Base class for all PESU Academy-related errors.""" - def __init__(self, message: str, status_code: int) -> None: - """Initialize the PESUAcademyError with a custom message and status code.""" + def __init__(self, message: str, status_code: int, headers: dict[str, str] | None = None) -> None: + """Initialize the PESUAcademyError with a custom message, status code and response headers.""" self.message = message self.status_code = status_code + # Only a 401 needs these today, to carry WWW-Authenticate. None for every other error, and + # JSONResponse accepts None, so the handler passes it through unconditionally. + self.headers = headers super().__init__(self.message) def __str__(self) -> str: diff --git a/app/exceptions/metrics.py b/app/exceptions/metrics.py new file mode 100644 index 0000000..5d1978d --- /dev/null +++ b/app/exceptions/metrics.py @@ -0,0 +1,13 @@ +"""Custom exception classes for the metrics endpoint. All errors inherit from PESUAcademyError.""" + +from app.exceptions.base import PESUAcademyError + + +class MetricsAuthorizationError(PESUAcademyError): + """Raised when the metrics endpoint is token-protected and the request did not present it.""" + + def __init__(self, message: str = "Invalid or missing metrics token.") -> None: + """Initialize the MetricsAuthorizationError with a custom message.""" + # A 401 is required to say how to authenticate, so a scraper can tell "your credentials + # are wrong" apart from "this endpoint wants no credentials at all". + super().__init__(message, status_code=401, headers={"WWW-Authenticate": "Bearer"}) diff --git a/app/metrics/auth.py b/app/metrics/auth.py new file mode 100644 index 0000000..5429320 --- /dev/null +++ b/app/metrics/auth.py @@ -0,0 +1,60 @@ +"""Optional bearer-token protection for the metrics endpoint.""" + +from __future__ import annotations + +import os +import secrets +from typing import Annotated + +from fastapi import Security + +# Imported at runtime on purpose, not under TYPE_CHECKING. FastAPI resolves a dependency's +# annotations with get_type_hints() when the route is built, and this module uses +# `from __future__ import annotations`, so a name that exists only for type checkers would be a +# NameError at import time rather than a typing nicety. +from fastapi.security import HTTPAuthorizationCredentials, HTTPBearer + +from app.exceptions.metrics import MetricsAuthorizationError + +# Read once at import, so whether this process enforces a token is fixed for its lifetime and +# cannot start or stop halfway through. An empty value counts as unset, which is what an +# environment variable declared but left blank looks like. +METRICS_TOKEN: str | None = os.environ.get("METRICS_TOKEN") or None + +# auto_error=False so this never raises by itself: every failure path -- no header, no scheme, no +# credentials, or a scheme that is not Bearer -- returns None, and the 401 is raised below as a +# PESUAcademyError. That is what keeps the body in this API's `{status, message, timestamp}` shape +# and gets the failure counted in errors_total. HTTPBasic cannot be used the same way; it raises +# HTTPException on a malformed credential regardless of auto_error, which would answer in +# Starlette's `{"detail": ...}` shape and bypass the error metric entirely. +_bearer = HTTPBearer( + auto_error=False, + scheme_name="MetricsToken", + description=( + "Set the METRICS_TOKEN environment variable on the server to require this token. While it " + "is unset the endpoint is open and any credential here is ignored." + ), +) + + +async def require_metrics_token( + credentials: Annotated[HTTPAuthorizationCredentials | None, Security(_bearer)] = None, +) -> None: + """Reject the request unless it carries the configured metrics token. + + Args: + credentials (HTTPAuthorizationCredentials | None): Parsed bearer credentials, or None when + the request carried no usable `Authorization: Bearer` header. + + Raises: + MetricsAuthorizationError: If a token is configured and the request did not present it. + """ + # Looked up on the module at call time rather than captured, so a test can swap it with + # monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", ...) + expected = METRICS_TOKEN + if expected is None: + return + # compare_digest, not ==, so a wrong token cannot be recovered a character at a time from how + # long the comparison took + if credentials is None or not secrets.compare_digest(credentials.credentials, expected): + raise MetricsAuthorizationError diff --git a/tests/unit/test_metrics_auth.py b/tests/unit/test_metrics_auth.py new file mode 100644 index 0000000..31a27dd --- /dev/null +++ b/tests/unit/test_metrics_auth.py @@ -0,0 +1,124 @@ +"""Tests for the optional bearer token on /metrics. + +Every test here pins `METRICS_TOKEN` explicitly rather than inheriting it from the environment. +tests/conftest.py calls load_dotenv() before this module imports app.app, so a METRICS_TOKEN in a +local .env would otherwise start returning 401 to every other /metrics test in the suite -- the +same class of silent, environment-dependent breakage as the ".env vanished" case. +""" + +from unittest.mock import AsyncMock, patch + +import pytest +from fastapi.testclient import TestClient + +from app.app import app +from app.exceptions.authentication import AuthenticationError +from app.metrics.collector import MetricsCollector + +TOKEN = "test-metrics-token" + + +@pytest.fixture +def client(monkeypatch): + """A client with a fresh collector and no token configured.""" + monkeypatch.setattr("app.app.metrics", MetricsCollector()) + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", None) + with ( + patch("app.app.pesu_academy.prefetch_client_with_csrf_token", new_callable=AsyncMock), + patch("app.app.pesu_academy.close_client", new_callable=AsyncMock), + ): + with TestClient(app, raise_server_exceptions=False) as test_client: + yield test_client + + +@pytest.fixture +def protected(client, monkeypatch): + """The same client, with a token required.""" + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", TOKEN) + return client + + +@pytest.mark.parametrize("query", ["", "?fmt=json", "?fmt=prometheus"]) +def test_open_when_no_token_is_configured(client, query): + """The default deployment, and what every existing caller and the Docker instructions expect.""" + assert client.get(f"/metrics{query}").status_code == 200 + + +@pytest.mark.parametrize("query", ["", "?fmt=json", "?fmt=prometheus"]) +def test_the_right_token_is_accepted_in_either_format(protected, query): + response = protected.get(f"/metrics{query}", headers={"Authorization": f"Bearer {TOKEN}"}) + assert response.status_code == 200 + + +@pytest.mark.parametrize( + ("label", "headers"), + [ + ("no header at all", {}), + ("the wrong token", {"Authorization": "Bearer not-the-token"}), + ("a prefix of the token", {"Authorization": f"Bearer {TOKEN[:-1]}"}), + ("the token without its scheme", {"Authorization": TOKEN}), + ("basic instead of bearer", {"Authorization": "Basic dXNlcjpwYXNz"}), + # HTTPBasic would raise HTTPException on this one, answering in Starlette's shape and + # skipping errors_total; HTTPBearer rejects the scheme before that can happen. + ("malformed basic credentials", {"Authorization": "Basic !!!!"}), + ("an empty bearer value", {"Authorization": "Bearer "}), + ], +) +def test_rejected_without_the_token(protected, label, headers): + response = protected.get("/metrics", headers=headers) + assert response.status_code == 401, label + + +def test_the_rejection_is_shaped_like_every_other_error(protected): + """A 401 here must not be Starlette's {"detail": ...}, which is what HTTPBasic would produce.""" + response = protected.get("/metrics") + body = response.json() + assert set(body) == {"status", "message", "timestamp"} + assert body["status"] is False + assert body["message"] == "Invalid or missing metrics token." + + +def test_the_rejection_says_how_to_authenticate(protected): + """Required of a 401, and it tells a scraper "wrong credentials" from "none wanted".""" + assert protected.get("/metrics").headers["www-authenticate"] == "Bearer" + + +def test_the_json_format_is_protected_too(protected): + """The format selector must not be a way around the token.""" + assert protected.get("/metrics?fmt=json").status_code == 401 + + +def test_a_rejection_is_counted_as_an_error_and_a_client_fault(protected): + """The middleware/handler split from the metrics work still holds for this new error.""" + protected.get("/metrics") + body = protected.get("/metrics?fmt=json", headers={"Authorization": f"Bearer {TOKEN}"}).json() + assert body["errorsByType"] == {"MetricsAuthorizationError": 1} + assert body["responsesByStatus"]["401"] == 1 + assert body["failuresByFault"] == {"client": 1} + + +def test_health_is_not_protected(protected): + """Render's own health check and the cron-job.org monitors send no credentials.""" + assert protected.get("/health").status_code == 200 + + +def test_an_empty_environment_variable_counts_as_unset(monkeypatch): + """A variable declared but left blank must not lock everyone out of the endpoint.""" + monkeypatch.setenv("METRICS_TOKEN", "") + import importlib + + import app.metrics.auth as metrics_auth + + importlib.reload(metrics_auth) + assert metrics_auth.METRICS_TOKEN is None + monkeypatch.delenv("METRICS_TOKEN") + importlib.reload(metrics_auth) + + +def test_other_errors_carry_no_stray_headers(client): + """Regression on adding `headers` to PESUAcademyError: it is None for everything else.""" + with patch("app.app.pesu_academy.authenticate", new_callable=AsyncMock) as authenticate: + authenticate.side_effect = AuthenticationError("Invalid username or password.") + response = client.post("/authenticate", json={"username": "PES1201800001", "password": "x"}) + assert response.status_code == 401 + assert "www-authenticate" not in response.headers diff --git a/tests/unit/test_openapi_docs.py b/tests/unit/test_openapi_docs.py index bf947a5..9a19eee 100644 --- a/tests/unit/test_openapi_docs.py +++ b/tests/unit/test_openapi_docs.py @@ -140,6 +140,31 @@ def test_the_metrics_format_enum_is_documented(schema): assert sorted(values) == ["json", "prometheus"] +def test_the_metrics_token_scheme_is_documented(schema): + """Swagger's Authorize button is how a reader discovers the endpoint can be protected.""" + scheme = schema["components"]["securitySchemes"]["MetricsToken"] + assert scheme["type"] == "http" + assert scheme["scheme"] == "bearer" + assert "METRICS_TOKEN" in scheme["description"] + + +def test_only_metrics_requires_the_token(schema): + """/health must stay open: Render's own health check and the uptime monitors send no token.""" + secured = {path for path, _, operation in _operations(schema) if operation.get("security")} + assert secured == {"/metrics"} + assert schema["paths"]["/metrics"]["get"]["security"] == [{"MetricsToken": []}] + + +def test_the_documented_metrics_401_matches_a_real_response(client, schema, monkeypatch): + """The 401 body a scraper gets must be the one Swagger shows.""" + documented = schema["paths"]["/metrics"]["get"]["responses"]["401"]["content"]["application/json"]["example"] + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", "some-token") + response = client.get("/metrics") + assert response.status_code == 401 + assert response.json()["message"] == documented["message"] + assert set(response.json()) == set(documented) + + def test_the_documented_400_matches_a_real_response(client, schema): """The example a reader copies must be the body they will actually receive.""" documented = schema["paths"]["/metrics"]["get"]["responses"]["400"]["content"]["application/json"]["example"] From 8d9a1d1ec1cc9304d39830b1720d4e2cd2525595 Mon Sep 17 00:00:00 2001 From: aditeyabaral Date: Sun, 13 Sep 2026 09:46:11 -0500 Subject: [PATCH 2/6] docs: document METRICS_TOKEN and how to scrape /metrics Two subsections under `/metrics`: how to turn the token on and what a rejection looks like, and how to point a scraper at the endpoint -- a Prometheus `scrape_config`, plus Grafana Cloud's collector-free Metrics Endpoint integration, which is the reason the token exists. Also states the two things that surprise people when they first graph this: counters are per-process and reset on restart, and a scrape is itself a request, so it appears in the metrics it collects. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VU7YUhP71KSsotWQ1H7CRH --- README.md | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/README.md b/README.md index d1044a2..a4655f4 100644 --- a/README.md +++ b/README.md @@ -546,6 +546,55 @@ which is `null` rather than absent when nothing has been recorded yet, so the sh +#### Protecting the endpoint + +`/metrics` is **open by default**, which is what a local run and the Docker instructions above +expect. Set the `METRICS_TOKEN` environment variable on the server to require a bearer token +instead: + +```bash +docker run --name pesu-auth -d -p 5000:5000 -e METRICS_TOKEN= pesu-auth +``` + +With it set, a request must carry that token or the endpoint answers `401` with +`WWW-Authenticate: Bearer` and the same error body as every other failure. Both formats are +covered, so `?fmt=json` is not a way around it. + +```bash +curl http://localhost:5000/metrics # 401 +curl -H "Authorization: Bearer " http://localhost:5000/metrics # 200 +``` + +The variable is read once at startup, so changing it needs a restart. Leaving it blank counts as +unset. No other endpoint is affected — `/health` in particular stays open, since uptime monitors +and the hosting platform's own health check send no credentials. + +#### Scraping the endpoint + +The default format is the Prometheus text exposition format precisely so that a scraper pointed at +this path needs no configuration. Any Prometheus-compatible collector works: + +```yaml +scrape_configs: + - job_name: pesu-auth + metrics_path: /metrics + scheme: https + static_configs: + - targets: [ "pesu-auth.onrender.com" ] + authorization: + credentials: # omit when METRICS_TOKEN is unset +``` + +Grafana Cloud can also scrape it with no collector to host, through its **Metrics Endpoint** +integration (Connections → Metrics Endpoint → Configuration → new scrape job). It requires the +endpoint to be behind authentication, which is what `METRICS_TOKEN` is for — paste the token +without the `Bearer ` prefix. Two things are worth knowing before pointing anything at it: + +- Counters are per-process and **reset on restart**. `rate()` handles that, and + `pesu_auth_process_start_time_seconds` makes the restart itself visible. +- A scrape is a request, so it appears in the metrics it collects. Subtract + `pesu_auth_route_requests_total{route="/metrics"}` for traffic without it. + ### `/readme` This endpoint redirects to the project's official GitHub repository. This endpoint does not take any request parameters. From 5dbe7d08e703eeed3e197675307c2147ed97c1b4 Mon Sep 17 00:00:00 2001 From: aditeyabaral Date: Sun, 13 Sep 2026 09:46:11 -0500 Subject: [PATCH 3/6] chore: bump version to 4.3.0 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VU7YUhP71KSsotWQ1H7CRH --- pyproject.toml | 2 +- uv.lock | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 279670f..b07d09b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "pesu-auth" -version = "4.2.0" +version = "4.3.0" description = "A simple API to authenticate PESU credentials using PESU Academy." readme = "README.md" requires-python = ">=3.14" diff --git a/uv.lock b/uv.lock index 38e3654..7f9bc05 100644 --- a/uv.lock +++ b/uv.lock @@ -630,7 +630,7 @@ wheels = [ [[package]] name = "pesu-auth" -version = "4.2.0" +version = "4.3.0" source = { editable = "." } dependencies = [ { name = "fastapi" }, From c23276008219781510ae4de0f0cbc3bf1d8d14ee Mon Sep 17 00:00:00 2001 From: aditeyabaral Date: Sun, 13 Sep 2026 10:24:12 -0500 Subject: [PATCH 4/6] fix: compare the metrics token as bytes, and pin it in the test suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects found reviewing the commit before this one. **`secrets.compare_digest` raises TypeError on a str holding any non-ASCII character.** So `Authorization: Bearer ü` -- three bytes any caller can send, and curl sends them happily -- turned the 401 into a **500** with a logged traceback, counted in `failures_total{fault="server"}`, which is the one metric worth alerting on. A passing scanner could have tripped a server-fault alert. Worse: a non-ASCII `METRICS_TOKEN` made *every* request 500, the correct one included, leaving the endpoint unreachable with nothing but a traceback to explain why -- and the README tells operators to pick a token. Now compared as bytes, which has no ASCII restriction. The two codecs are not interchangeable: a header value arrives already latin-1 decoded, so encoding it back that way recovers the exact bytes the client sent, while the configured token comes from the environment as UTF-8 with surrogates standing in for any invalid byte sequence. Encoding each back the way it arrived makes the comparison byte-exact, so a non-ASCII token now works rather than silently never matching. Ten new tests cover it, all of which fail against the str comparison. They pass header values as **bytes**: httpx refuses to encode a non-ASCII str header, so a str would fail in the client and never reach the app, which is why the unit tests missed this entirely and only curl found it. One test drops below the HTTP layer to call the dependency directly, including with a lone surrogate -- the case that rules out encoding UTF-8 on the presented side. **The suite was not hermetic, and a test of mine was hiding it.** With `METRICS_TOKEN` exported, 27 tests across test_metrics_endpoints.py and test_openapi_docs.py failed with 401s that had nothing to do with what they assert -- the trap flagged in the plan, which I then only guarded in my own new file. The suite still looked green, because `test_an_empty_environment_variable_counts_as_unset` reloaded the module and left METRICS_TOKEN as None for every test that followed it. Green for the wrong reason is worse than red. An autouse fixture in tests/conftest.py now pins it to None for the whole suite, and the reload test is replaced by one that exercises a small `_configured_token()` reader instead of mutating module state. Verified by running the suite with the variable exported, with and without this file. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VU7YUhP71KSsotWQ1H7CRH --- app/metrics/auth.py | 36 ++++++++++++++--- tests/conftest.py | 15 +++++++ tests/unit/test_metrics_auth.py | 70 ++++++++++++++++++++++++++++----- 3 files changed, 107 insertions(+), 14 deletions(-) diff --git a/app/metrics/auth.py b/app/metrics/auth.py index 5429320..68c8365 100644 --- a/app/metrics/auth.py +++ b/app/metrics/auth.py @@ -16,10 +16,20 @@ from app.exceptions.metrics import MetricsAuthorizationError + +def _configured_token() -> str | None: + """Read the configured metrics token from the environment. + + Returns: + str | None: The token, or None when it is unset or blank -- a variable declared and left + empty means "no token", not "the empty token". + """ + return os.environ.get("METRICS_TOKEN") or None + + # Read once at import, so whether this process enforces a token is fixed for its lifetime and -# cannot start or stop halfway through. An empty value counts as unset, which is what an -# environment variable declared but left blank looks like. -METRICS_TOKEN: str | None = os.environ.get("METRICS_TOKEN") or None +# cannot start or stop halfway through. +METRICS_TOKEN: str | None = _configured_token() # auto_error=False so this never raises by itself: every failure path -- no header, no scheme, no # credentials, or a scheme that is not Bearer -- returns None, and the 401 is raised below as a @@ -55,6 +65,22 @@ async def require_metrics_token( if expected is None: return # compare_digest, not ==, so a wrong token cannot be recovered a character at a time from how - # long the comparison took - if credentials is None or not secrets.compare_digest(credentials.credentials, expected): + # long the comparison took. + # + # Compared as bytes, not str: compare_digest *raises TypeError* on a str holding any non-ASCII + # character, so `Authorization: Bearer ü` would turn this 401 into a 500 with a logged + # traceback -- something any caller could do at will, and it would land in + # failures_total{fault="server"}, which is the one metric worth alerting on. A non-ASCII + # METRICS_TOKEN was worse still: every request 500ed, including the correct one. + # + # The two codecs are not interchangeable. A header value reaches us already latin-1 decoded, + # per the HTTP spec and every ASGI server, so encoding it back through latin-1 recovers the + # exact bytes the client sent; the configured token comes from the environment as UTF-8, with + # surrogates standing in for any byte sequence that was not valid UTF-8. Encoding each back the + # way it arrived makes the comparison byte-exact, so a non-ASCII token works rather than + # silently never matching. + if credentials is None or not secrets.compare_digest( + credentials.credentials.encode("latin-1", "replace"), + expected.encode("utf-8", "surrogateescape"), + ): raise MetricsAuthorizationError diff --git a/tests/conftest.py b/tests/conftest.py index 08f428f..56758af 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1,8 +1,23 @@ +import pytest from dotenv import load_dotenv load_dotenv() +@pytest.fixture(autouse=True) +def _metrics_token_unset(monkeypatch): + """Keep /metrics open unless a test asks for a token. + + METRICS_TOKEN is read from the environment when app.metrics.auth is imported, and the + load_dotenv() above runs before any test module imports the app. So a token left in a local + .env -- or merely exported in the shell -- would make roughly thirty /metrics tests across the + suite fail with a confusing 401 that has nothing to do with what they are testing. Pinning it + here makes the suite independent of the ambient environment; the tests that exercise the token + set it themselves. + """ + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", None) + + def pytest_collection_modifyitems(config, items): # Force directory-based test ordering: unit > functional > integration priority = { diff --git a/tests/unit/test_metrics_auth.py b/tests/unit/test_metrics_auth.py index 31a27dd..42dd6e2 100644 --- a/tests/unit/test_metrics_auth.py +++ b/tests/unit/test_metrics_auth.py @@ -9,10 +9,13 @@ from unittest.mock import AsyncMock, patch import pytest +from fastapi.security import HTTPAuthorizationCredentials from fastapi.testclient import TestClient from app.app import app from app.exceptions.authentication import AuthenticationError +from app.exceptions.metrics import MetricsAuthorizationError +from app.metrics.auth import _configured_token, require_metrics_token from app.metrics.collector import MetricsCollector TOKEN = "test-metrics-token" @@ -102,17 +105,66 @@ def test_health_is_not_protected(protected): assert protected.get("/health").status_code == 200 -def test_an_empty_environment_variable_counts_as_unset(monkeypatch): - """A variable declared but left blank must not lock everyone out of the endpoint.""" - monkeypatch.setenv("METRICS_TOKEN", "") - import importlib +@pytest.mark.parametrize( + ("value", "expected"), + [(None, None), ("", None), (" ", " "), ("a-token", "a-token")], +) +def test_reading_the_token_from_the_environment(monkeypatch, value, expected): + """A variable declared but left blank means "no token", not "the empty token". + + Tested through the reader rather than by reloading the module: a reload mutates the module + dict in place, which would leave the new value in force for every test that follows it. + """ + if value is None: + monkeypatch.delenv("METRICS_TOKEN", raising=False) + else: + monkeypatch.setenv("METRICS_TOKEN", value) + assert _configured_token() == expected + + +#: Header values are passed as **bytes** in the non-ASCII tests below. httpx refuses to encode a +#: non-ASCII str header value, so a str would fail in the client and never reach the app -- but +#: curl and any other raw client send those bytes happily, which is how this reached a real server. +@pytest.mark.parametrize( + "credential", + [b"\xc3\xbc", b"t\xc3\xb6k\xc3\xa9n", b"\xc3\xa9" * 200, b"\xff\xfe", b"\x80"], +) +def test_a_non_ascii_credential_is_rejected_not_a_server_error(protected, credential): + """`secrets.compare_digest` raises TypeError on a str holding any non-ASCII character. + + Comparing the presented credential as a str let any caller turn this 401 into a **500** with a + logged traceback, and put the result in `failures_total{fault="server"}` -- the one metric + worth alerting on. Against a str comparison every case here is a 500. + """ + response = protected.get("/metrics", headers={b"Authorization": b"Bearer " + credential}) + assert response.status_code == 401 + assert response.json()["message"] == "Invalid or missing metrics token." - import app.metrics.auth as metrics_auth - importlib.reload(metrics_auth) - assert metrics_auth.METRICS_TOKEN is None - monkeypatch.delenv("METRICS_TOKEN") - importlib.reload(metrics_auth) +def test_a_non_ascii_token_actually_works(client, monkeypatch): + """A token is compared byte for byte, so an operator is not silently locked out by an umlaut. + + Against a str comparison this was worse than a rejection: *every* request 500ed, the correct + one included, leaving the endpoint unreachable with only a traceback to explain why. + """ + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", "tökén-höchst") + correct = "tökén-höchst".encode() # what a client actually puts on the wire + assert client.get("/metrics", headers={b"Authorization": b"Bearer " + correct}).status_code == 200 + assert client.get("/metrics", headers={b"Authorization": b"Bearer t\xc3\xb6k\xc3\xa9n"}).status_code == 401 + + +@pytest.mark.parametrize("credential", ["ü", "tökén", "\udcff", "é" * 500]) +@pytest.mark.asyncio +async def test_the_dependency_itself_never_raises_typeerror(monkeypatch, credential): + """Pinned one level below the HTTP layer, where the TypeError actually happened. + + Includes a lone surrogate, which no HTTP client would send but which `.encode("utf-8")` would + choke on -- the reason the comparison encodes latin-1 rather than UTF-8 on this side. + """ + monkeypatch.setattr("app.metrics.auth.METRICS_TOKEN", TOKEN) + credentials = HTTPAuthorizationCredentials(scheme="Bearer", credentials=credential) + with pytest.raises(MetricsAuthorizationError): + await require_metrics_token(credentials) def test_other_errors_carry_no_stray_headers(client): From e50e7e1a197263e8f55fc05d8236580e37ac972b Mon Sep 17 00:00:00 2001 From: aditeyabaral Date: Sun, 13 Sep 2026 10:24:12 -0500 Subject: [PATCH 5/6] docs: surface the token from the /metrics intro The token was documented two screens below a long sample payload, where a reader scanning the endpoint would not find it, so the intro now points at it. The generation example also handed the token straight to `docker run` through a subshell, which meant never seeing the value the scraper needs. Also stops the app/metrics package docstring enumerating the modules in it: it named two of four, having already fallen out of date once before this change added a third. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VU7YUhP71KSsotWQ1H7CRH --- README.md | 6 ++++-- app/metrics/__init__.py | 6 +++--- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/README.md b/README.md index a4655f4..605e575 100644 --- a/README.md +++ b/README.md @@ -167,7 +167,8 @@ does not take any request parameters. ### `/metrics` This endpoint exposes counters describing the traffic this process has served and the work it did to serve it. It takes -no request parameters other than the format selector below. +no request parameters other than the format selector below. It is open by default and can be put behind a bearer token +— see [Protecting the endpoint](#protecting-the-endpoint). #### Query Parameters @@ -553,7 +554,8 @@ expect. Set the `METRICS_TOKEN` environment variable on the server to require a instead: ```bash -docker run --name pesu-auth -d -p 5000:5000 -e METRICS_TOKEN= pesu-auth +TOKEN=$(openssl rand -hex 32) # keep it: whatever scrapes the endpoint needs the same value +docker run --name pesu-auth -d -p 5000:5000 -e METRICS_TOKEN="$TOKEN" pesu-auth ``` With it set, a request must carry that token or the endpoint answers `401` with diff --git a/app/metrics/__init__.py b/app/metrics/__init__.py index 7da790c..98917a8 100644 --- a/app/metrics/__init__.py +++ b/app/metrics/__init__.py @@ -4,7 +4,7 @@ PESUAcademy client, so importing the package has no side effects and a test can swap the collector out by patching one module attribute. -Import from the modules directly -- `app.metrics.collector` for the families and the collector, -`app.metrics.prometheus` for exposition -- following the same convention as `app.exceptions`. A -re-export list here would be one more place to remember when a metric family is added. +Import from the modules directly rather than from this package, following the same convention as +`app.exceptions`. A re-export list here would be one more place to remember when a metric family or +a module is added -- and it had already fallen out of date once. """ From bccab19999d6e47bdde267487c89f1aa160b1e99 Mon Sep 17 00:00:00 2001 From: aditeyabaral Date: Sun, 13 Sep 2026 10:46:28 -0500 Subject: [PATCH 6/6] docs: drop vendor specifics and duplication, and stop benchmarking /metrics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The benchmark exists to measure authentication, so `--route metrics` is removed from its choices. It was added in #157 and should not have been: the script has no way to present a metrics token, so against a protected deployment it measured 401s and reported them as failed requests. `/readme` is now the only non-JSON route, so the comment justifying that fallback in util.py says so. The README's scraping section carried a click-path through a third-party console ("Connections → Metrics Endpoint → ...") which will rot the moment that product's UI changes, and belongs in whoever-runs-it's notes rather than this repo. The generic Prometheus `scrape_config` stays, since that is the format the endpoint actually serves. It also restated two things already documented under "How collection works" four hundred lines above -- that counters reset on restart, and that a scrape counts itself. One sentence there said the same thing twice within itself, and is trimmed to the half that carries meaning. A code comment in the collector explained seeding by naming a specific dashboard product. The reason holds for any consumer, so it no longer names one. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01VU7YUhP71KSsotWQ1H7CRH --- README.md | 13 +------------ app/metrics/collector.py | 4 ++-- scripts/benchmark/benchmark_requests.py | 2 +- scripts/benchmark/util.py | 6 +++--- 4 files changed, 7 insertions(+), 18 deletions(-) diff --git a/README.md b/README.md index 605e575..331e6b4 100644 --- a/README.md +++ b/README.md @@ -188,8 +188,7 @@ curl http://localhost:5000/metrics?fmt=json | jq # the same numbers, for a hu Everything is counted **in this process, in memory**. There is no database and no external dependency, and the counters **reset to zero when the process restarts** — which on the hosted environments is often. `processStartTimeSeconds` is -exposed so a dashboard can tell a restart apart from a drop in traffic; in PromQL, `rate()` already handles counter -resets, and `pesu_auth_process_start_time_seconds` makes the restart itself visible. +exposed so a dashboard can tell a restart apart from a drop in traffic. Collection happens at three layers, and which layer records what is deliberate: @@ -587,16 +586,6 @@ scrape_configs: credentials: # omit when METRICS_TOKEN is unset ``` -Grafana Cloud can also scrape it with no collector to host, through its **Metrics Endpoint** -integration (Connections → Metrics Endpoint → Configuration → new scrape job). It requires the -endpoint to be behind authentication, which is what `METRICS_TOKEN` is for — paste the token -without the `Bearer ` prefix. Two things are worth knowing before pointing anything at it: - -- Counters are per-process and **reset on restart**. `rate()` handles that, and - `pesu_auth_process_start_time_seconds` makes the restart itself visible. -- A scrape is a request, so it appears in the metrics it collects. Subtract - `pesu_auth_route_requests_total{route="/metrics"}` for traffic without it. - ### `/readme` This endpoint redirects to the project's official GitHub repository. This endpoint does not take any request parameters. diff --git a/app/metrics/collector.py b/app/metrics/collector.py index ab66fa8..f8a7ef0 100644 --- a/app/metrics/collector.py +++ b/app/metrics/collector.py @@ -254,8 +254,8 @@ def __init__(self, *, clock: Callable[[], float] | None = None) -> None: self._start_time = self._clock() self._values: defaultdict[str, dict[LabelKey, float]] = defaultdict(dict) # Seed the unlabelled series so a freshly started process still exposes them. Without this - # a Grafana panel has no series at all until the first request, and rate() over a series - # that springs into existence mid-window reads as a spike. + # there is no series at all until the first request, and a rate over a series that springs + # into existence mid-window reads as a spike. for family in FAMILIES: if family.labels: continue diff --git a/scripts/benchmark/benchmark_requests.py b/scripts/benchmark/benchmark_requests.py index fc990ed..30a48a3 100644 --- a/scripts/benchmark/benchmark_requests.py +++ b/scripts/benchmark/benchmark_requests.py @@ -40,7 +40,7 @@ parser.add_argument( "--route", type=str, - choices=["authenticate", "health", "readme", "metrics"], + choices=["authenticate", "health", "readme"], default="authenticate", help="The route to make the request to (default: authenticate)", ) diff --git a/scripts/benchmark/util.py b/scripts/benchmark/util.py index 4c194bd..ab07204 100644 --- a/scripts/benchmark/util.py +++ b/scripts/benchmark/util.py @@ -89,9 +89,9 @@ def make_request( follow_redirects=True, ) elapsed_time = time.time() - start_time - # Not every route answers with JSON: /readme is a 308 to GitHub, and /metrics is Prometheus - # text. An unconditional .json() crashes the sequential runner outright and, in the parallel - # runner, is swallowed as a failed request -- which silently skews the numbers being measured. + # Not every route answers with JSON: /readme is a 308 to GitHub. An unconditional .json() + # crashes the sequential runner outright and, in the parallel runner, is swallowed as a failed + # request -- which silently skews the numbers being measured. try: body = response.json() except ValueError: