From 3c5e559f6efea6fc4f20e1a9cb7756f6c0af07c0 Mon Sep 17 00:00:00 2001 From: PhilippTheServer Date: Wed, 26 Aug 2026 17:02:16 +0200 Subject: [PATCH] feat(telemetry): capture uncaught frontend errors MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Traces and metrics see nothing that happens in a browser. A component that throws leaves the server returning 200 with healthy metrics while the shop is broken for a real customer — and for a shop, by the time somebody reports it the sale is already lost. Adds a public report endpoint and an admin read that groups errors by application, class and message. POST /v1/telemetry/errors public, rate limited, opt-in GET /v1/admin/telemetry/errors grouped, ordered by frequency The report endpoint takes no token because storefront visitors are not signed in and an error before login is exactly the one worth catching. That makes it hostile-input territory, so it is rate limited harder than the analytics ingest — a component throwing inside a render loop is the normal failure mode here and reports as fast as the browser can loop — with a closed app vocabulary, forbidden extra fields and every field bounded. Stacks are truncated rather than rejected: unbounded input from a public endpoint, but also the most useful field, and the top frames are where the fault is. The user agent is reduced at the boundary to a family and major version and only that is stored. A raw agent string is a fingerprint, but "which browser" is genuinely diagnostic, so discarding it entirely would make the reports much weaker. "Safari 18" reproduces a bug and does not recognise anyone. The reduction doubles as a filter: whatever a client sends, the output is a known family name and an integer, never a fragment of the input. Errors are grouped by message rather than by stack. The same fault reached from two routes produces two stacks and is one bug; affected_paths shows the spread instead. Documented plainly: this reports only what browsers managed to send, so an error that breaks a page badly enough to stop the reporter is the one that will not appear. Silence means no news, not no errors. Closes #50 Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01YY1ekLLeFLkAU2kvdQ8Ey4 --- .env.example | 4 + docs/observability.md | 75 +++++ src/app/db_models.py | 5 + src/app/main.py | 8 + src/app/services/frontend_errors/__init__.py | 35 +++ .../frontend_errors/functions/__init__.py | 5 + .../frontend_errors/functions/user_agent.py | 49 +++ .../frontend_errors/models/__init__.py | 25 ++ .../models/frontend_errors_db_models.py | 71 +++++ .../models/frontend_errors_models.py | 113 +++++++ .../frontend_errors/routers/__init__.py | 5 + .../routers/frontend_errors_router.py | 138 +++++++++ .../frontend_errors/services/__init__.py | 13 + .../services/frontend_errors_db_service.py | 135 +++++++++ src/app/shared/config/settings.py | 8 + tests/test_frontend_errors_integration.py | 281 ++++++++++++++++++ tests/test_frontend_errors_unit.py | 139 +++++++++ 17 files changed, 1109 insertions(+) create mode 100644 src/app/services/frontend_errors/__init__.py create mode 100644 src/app/services/frontend_errors/functions/__init__.py create mode 100644 src/app/services/frontend_errors/functions/user_agent.py create mode 100644 src/app/services/frontend_errors/models/__init__.py create mode 100644 src/app/services/frontend_errors/models/frontend_errors_db_models.py create mode 100644 src/app/services/frontend_errors/models/frontend_errors_models.py create mode 100644 src/app/services/frontend_errors/routers/__init__.py create mode 100644 src/app/services/frontend_errors/routers/frontend_errors_router.py create mode 100644 src/app/services/frontend_errors/services/__init__.py create mode 100644 src/app/services/frontend_errors/services/frontend_errors_db_service.py create mode 100644 tests/test_frontend_errors_integration.py create mode 100644 tests/test_frontend_errors_unit.py diff --git a/.env.example b/.env.example index 0dca713..1e7669f 100644 --- a/.env.example +++ b/.env.example @@ -229,6 +229,10 @@ SHOP_TIMEZONE=Europe/Berlin # ingest endpoint returns 404. STOREFRONT_ANALYTICS_ENABLED=false +# Accept uncaught error reports from the frontends. Off by default, for the same +# reason. While false the report endpoint returns 404. +FRONTEND_ERRORS_ENABLED=false + # ---------------------------------- # OpenTelemetry # ---------------------------------- diff --git a/docs/observability.md b/docs/observability.md index 8385428..6878c6f 100644 --- a/docs/observability.md +++ b/docs/observability.md @@ -131,3 +131,78 @@ which looks like a working panel. That mistake is already made and fixed here. output and Prometheus, not against the fact that setup was called. That distinction caught the real bug: instrumenting the app before configuring telemetry logged a clean start and produced no HTTP metrics at all. + +--- + +# Frontend Errors + +Uncaught errors from the storefront and the admin UI. + +Traces and metrics see nothing that happens in a browser. A component that +throws leaves the server returning 200 with healthy metrics while the shop is +broken for a real customer — and for a shop, by the time someone reports it the +sale is gone. + +``` +POST /v1/telemetry/errors report (public, rate limited, opt-in) +GET /v1/admin/telemetry/errors read, grouped (admin) +``` + +## Public, and treated as such + +Storefront visitors are not signed in, and an error that happens before login is +exactly the one worth catching — so the report endpoint takes no token. It is +therefore hostile-input territory: + +| Guard | Why | +|---|---| +| 30 requests/minute per address | A component throwing in a render loop reports as fast as the browser can loop | +| 10 errors per batch | One request cannot be a bulk insert | +| Closed `app` vocabulary | The field cannot become free text | +| `extra="forbid"` | Unknown fields are refused, not ignored | +| Stack truncated at 4000 chars | Unbounded input, and the top frames are where the fault is | +| Off unless `FRONTEND_ERRORS_ENABLED` | Returns 404 while off | + +## The user agent is reduced, never stored + +A raw user-agent string is a fingerprint. But "which browser?" is genuinely +diagnostic — a large share of frontend bugs are one engine behaving differently +— so discarding it entirely makes the reports much weaker. + +The compromise: reduce it at the boundary to a family and major version, and +store only that. `Safari 18` reproduces a bug; it does not recognise anyone. + +The reduction is also a filter. Whatever a client sends, the output is a known +family name and an integer — never a fragment of the input: + +``` +"Mozilla/5.0 Chrome/140 user=alice@example.com token=abc123" → "Chrome 140" +``` + +There is no column for an IP address, an email or a customer id, and a test +fails if one appears. + +## Errors are grouped + +One bug produces thousands of identical rows, so the read endpoint groups by +application, error class and message, ordered by frequency. + +Deliberately **not** grouped by stack: the same fault reached from two routes +produces two different stacks and is one bug. `affected_paths` shows the spread +instead, and one representative stack is returned for debugging. + +## What this cannot tell you + +It reports only what browsers managed to send. An error that breaks a page badly +enough to stop the reporter is precisely the one that will not appear — so a +quiet report is weaker evidence than a noisy one. Read silence as "no news", +never as "no errors". + +## Testing + +- `tests/test_frontend_errors_unit.py` — the user-agent reduction, including + Edge not being reported as Chrome, and that nothing from the input string + survives it. +- `tests/test_frontend_errors_integration.py` — reporting without auth, the + PII-column assertion, the raw agent never reaching storage, query-string + stripping, stale timestamps, and grouping across routes. diff --git a/src/app/db_models.py b/src/app/db_models.py index 041253a..5ec7cdc 100644 --- a/src/app/db_models.py +++ b/src/app/db_models.py @@ -42,6 +42,11 @@ # Returns (RMA) from app.services.returns.models.returns_db_models import ReturnDB # noqa: F401 +# Frontend errors (S4) +from app.services.frontend_errors.models.frontend_errors_db_models import ( # noqa: F401 + FrontendErrorDB, +) + # Storefront analytics (S2) from app.services.storefront_analytics.models.storefront_events_db_models import ( # noqa: F401 StorefrontEventDB, diff --git a/src/app/main.py b/src/app/main.py index ab0f014..5dab56e 100644 --- a/src/app/main.py +++ b/src/app/main.py @@ -12,6 +12,10 @@ from app.services.crud_item_store import router as item_store_router from app.services.customers import customers_api_router from app.services.fulfillment import fulfillment_api_router +from app.services.frontend_errors import ( + admin_frontend_errors_api_router, + frontend_errors_api_router, +) from app.services.health import health_api_router from app.services.inventory import inventory_api_router from app.services.mail import mail_api_router @@ -170,6 +174,10 @@ async def generic_exception_handler(request: Request, exc: Exception) -> JSONRes # Include analytics service router (S1) app.include_router(analytics_api_router, prefix="/v1") +# Include frontend error reporting (S4) — public ingest plus admin read +app.include_router(frontend_errors_api_router, prefix="/v1") +app.include_router(admin_frontend_errors_api_router, prefix="/v1") + # Include storefront analytics ingest (S2) — public, rate limited, opt-in app.include_router(storefront_analytics_api_router, prefix="/v1") diff --git a/src/app/services/frontend_errors/__init__.py b/src/app/services/frontend_errors/__init__.py new file mode 100644 index 0000000..2b9e6b1 --- /dev/null +++ b/src/app/services/frontend_errors/__init__.py @@ -0,0 +1,35 @@ +""" +Frontend Errors Service + +Uncaught browser errors from the storefront and the admin UI. + +S3 gave the API traces and metrics; neither can see a component that throws in +somebody's browser. The server returns 200, the metrics look healthy, and the +shop is broken for a real customer. + +Endpoints: + POST /telemetry/errors — report (public, rate limited, opt-in) + GET /admin/telemetry/errors — read, grouped (admin) +""" + +from fastapi import APIRouter + +from app.authorize import require_admin +from fastapi import Depends + +from .routers import admin_router, report_router + +frontend_errors_api_router = APIRouter( + prefix="/telemetry", + tags=["Frontend Errors"], +) +frontend_errors_api_router.include_router(report_router) + +admin_frontend_errors_api_router = APIRouter( + prefix="/admin/telemetry", + tags=["Frontend Errors"], + dependencies=[Depends(require_admin)], +) +admin_frontend_errors_api_router.include_router(admin_router) + +__all__ = ["admin_frontend_errors_api_router", "frontend_errors_api_router"] diff --git a/src/app/services/frontend_errors/functions/__init__.py b/src/app/services/frontend_errors/functions/__init__.py new file mode 100644 index 0000000..b0f57e4 --- /dev/null +++ b/src/app/services/frontend_errors/functions/__init__.py @@ -0,0 +1,5 @@ +"""Frontend error helpers.""" + +from .user_agent import coarse_browser + +__all__ = ["coarse_browser"] diff --git a/src/app/services/frontend_errors/functions/user_agent.py b/src/app/services/frontend_errors/functions/user_agent.py new file mode 100644 index 0000000..6cb5c07 --- /dev/null +++ b/src/app/services/frontend_errors/functions/user_agent.py @@ -0,0 +1,49 @@ +""" +Coarse browser identification. + +A raw user-agent string is a fingerprint: combined with a handful of other +signals it identifies a device, which is exactly what the rest of this system +refuses to collect. But "which browser?" is genuinely diagnostic — half of all +frontend bugs are one engine behaving differently — so throwing it away entirely +makes the error reports much less useful. + +The compromise is to reduce the string to a family and a major version at the +boundary and store only that. "Safari 18" is enough to reproduce a bug and not +enough to recognise anyone. +""" + +from __future__ import annotations + +import re + +_UNKNOWN = "unknown" + +# Order matters. Edge and Opera both claim to be Chrome, and Chrome claims to be +# Safari, so the most specific pattern has to win. +_PATTERNS: tuple[tuple[str, re.Pattern[str]], ...] = ( + ("Edge", re.compile(r"Edg(?:e|A|iOS)?/(\d+)")), + ("Opera", re.compile(r"OPR/(\d+)")), + ("Samsung Internet", re.compile(r"SamsungBrowser/(\d+)")), + ("Firefox", re.compile(r"Firefox/(\d+)")), + ("Chrome", re.compile(r"Chrome/(\d+)")), + ("Safari", re.compile(r"Version/(\d+).*Safari")), +) + + +def coarse_browser(user_agent: str | None) -> str: + """ + Reduce a user-agent string to "Family Major", or "unknown". + + Never returns anything derived from the original string beyond the family + name and an integer, so no amount of unusual input can smuggle a fingerprint + through. + """ + if not user_agent: + return _UNKNOWN + + for family, pattern in _PATTERNS: + match = pattern.search(user_agent) + if match: + return f"{family} {match.group(1)}" + + return _UNKNOWN diff --git a/src/app/services/frontend_errors/models/__init__.py b/src/app/services/frontend_errors/models/__init__.py new file mode 100644 index 0000000..80aae17 --- /dev/null +++ b/src/app/services/frontend_errors/models/__init__.py @@ -0,0 +1,25 @@ +"""Frontend error models.""" + +from .frontend_errors_db_models import FrontendErrorDB +from .frontend_errors_models import ( + MAX_ERRORS_PER_BATCH, + MAX_STACK_CHARS, + ErrorGroup, + FrontendApp, + FrontendErrorBatch, + FrontendErrorIngestResponse, + FrontendErrorInput, + FrontendErrorsResponse, +) + +__all__ = [ + "MAX_ERRORS_PER_BATCH", + "MAX_STACK_CHARS", + "ErrorGroup", + "FrontendApp", + "FrontendErrorBatch", + "FrontendErrorDB", + "FrontendErrorIngestResponse", + "FrontendErrorInput", + "FrontendErrorsResponse", +] diff --git a/src/app/services/frontend_errors/models/frontend_errors_db_models.py b/src/app/services/frontend_errors/models/frontend_errors_db_models.py new file mode 100644 index 0000000..7d9a2a9 --- /dev/null +++ b/src/app/services/frontend_errors/models/frontend_errors_db_models.py @@ -0,0 +1,71 @@ +""" +Frontend Error Database Model + +One row per reported uncaught error from a browser. + +As with storefront_events, what is absent matters: no IP address, no raw user +agent, no customer id, no email. `browser` holds a coarse family and major +version parsed server-side — enough to reproduce a bug, not enough to recognise +anyone. +""" + +from datetime import datetime +from uuid import UUID, uuid4 + +from sqlalchemy import DateTime, Index, String, Text, text +from sqlalchemy.dialects.postgresql import UUID as PGUUID +from sqlalchemy.orm import Mapped, mapped_column + +from app.shared.database.base import Base + + +class FrontendErrorDB(Base): + """An uncaught error reported by one of the frontends.""" + + __tablename__ = "frontend_errors" + + id: Mapped[UUID] = mapped_column( + PGUUID(as_uuid=True), + primary_key=True, + default=uuid4, + server_default=text("gen_random_uuid()"), + ) + + app: Mapped[str] = mapped_column( + String(20), nullable=False, doc="storefront | admin" + ) + name: Mapped[str] = mapped_column( + String(120), nullable=False, doc="Error class, e.g. TypeError" + ) + message: Mapped[str] = mapped_column(String(500), nullable=False) + stack: Mapped[str | None] = mapped_column( + Text, nullable=True, doc="Truncated at ingest — a full trace is unbounded input" + ) + path: Mapped[str | None] = mapped_column( + String(255), nullable=True, doc="Route only; query strings are stripped" + ) + browser: Mapped[str] = mapped_column( + String(60), + nullable=False, + server_default=text("'unknown'"), + doc="Coarse family and major version. Never the raw user agent.", + ) + + occurred_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), nullable=False + ) + created_at: Mapped[datetime] = mapped_column( + DateTime(timezone=True), nullable=False, server_default=text("now()") + ) + + __table_args__ = ( + Index("ix_frontend_errors_occurred_at", "occurred_at"), + Index("ix_frontend_errors_app_occurred", "app", "occurred_at"), + # Grouping is the only way anyone reads this table: one bug in a render + # loop produces thousands of identical rows, and the useful question is + # "which distinct errors, how often". + Index("ix_frontend_errors_grouping", "app", "name", "message"), + ) + + def __repr__(self) -> str: + return f"FrontendErrorDB(id={self.id}, app={self.app!r}, name={self.name!r})" diff --git a/src/app/services/frontend_errors/models/frontend_errors_models.py b/src/app/services/frontend_errors/models/frontend_errors_models.py new file mode 100644 index 0000000..2da0a8d --- /dev/null +++ b/src/app/services/frontend_errors/models/frontend_errors_models.py @@ -0,0 +1,113 @@ +""" +Frontend Error Schemas + +The report endpoint is public, so this schema is a boundary against hostile and +merely broken input alike. A component erroring inside a render loop is not an +attack, and it will still send thousands of reports a second if nothing stops +it. +""" + +from __future__ import annotations + +from datetime import datetime +from enum import Enum + +from pydantic import BaseModel, ConfigDict, Field, field_validator + +from app.shared.responses import BaseResponse + +MAX_ERRORS_PER_BATCH = 10 +MAX_STACK_CHARS = 4000 + + +class FrontendApp(str, Enum): + """Which application reported. Closed, so it cannot become a free-text field.""" + + STOREFRONT = "storefront" + ADMIN = "admin" + + +class FrontendErrorInput(BaseModel): + """One uncaught error.""" + + model_config = ConfigDict(extra="forbid") + + app: FrontendApp + name: str = Field(max_length=120) + message: str = Field(max_length=500) + stack: str | None = Field(default=None) + path: str | None = Field(default=None, max_length=255) + occurred_at: datetime + + @field_validator("path", mode="before") + @classmethod + def strip_query_string(cls, value: object) -> object: + """Same reasoning as storefront events: query strings carry accidents.""" + if not isinstance(value, str): + return value + for separator in ("?", "#"): + value = value.split(separator, 1)[0] + return value[:255] + + @field_validator("stack", mode="before") + @classmethod + def bound_stack(cls, value: object) -> object: + """ + Truncate rather than reject. + + A stack trace is unbounded input from a public endpoint, but it is also + the single most useful field here. Cutting it keeps the top frames, + which is where the fault is. + """ + if not isinstance(value, str): + return value + if len(value) <= MAX_STACK_CHARS: + return value + return value[:MAX_STACK_CHARS] + "\n… truncated" + + +class FrontendErrorBatch(BaseModel): + """A batch from one browser.""" + + model_config = ConfigDict(extra="forbid") + + errors: list[FrontendErrorInput] = Field( + min_length=1, max_length=MAX_ERRORS_PER_BATCH + ) + + +class FrontendErrorIngestResponse(BaseResponse): + """How many reports were kept.""" + + accepted: int + rejected: int + + +class ErrorGroup(BaseModel): + """Distinct errors, with how often and how recently they happened.""" + + app: str + name: str + message: str + occurrences: int + affected_paths: int = Field(description="Distinct routes this error occurred on") + browsers: list[str] = Field( + default_factory=list, description="Coarse browser labels that hit it" + ) + first_seen: datetime + last_seen: datetime + sample_stack: str | None = None + + +class FrontendErrorsResponse(BaseResponse): + """ + Errors grouped, because one bug produces thousands of identical rows. + + Reports only what browsers managed to send. An error that breaks the page + badly enough to stop the reporter is the one you will not see here, so a + quiet report is weaker evidence than a noisy one. + """ + + enabled: bool + total_occurrences: int + groups: list[ErrorGroup] = Field(default_factory=list) diff --git a/src/app/services/frontend_errors/routers/__init__.py b/src/app/services/frontend_errors/routers/__init__.py new file mode 100644 index 0000000..eefd7ad --- /dev/null +++ b/src/app/services/frontend_errors/routers/__init__.py @@ -0,0 +1,5 @@ +"""Frontend error routers.""" + +from .frontend_errors_router import admin_router, report_router + +__all__ = ["admin_router", "report_router"] diff --git a/src/app/services/frontend_errors/routers/frontend_errors_router.py b/src/app/services/frontend_errors/routers/frontend_errors_router.py new file mode 100644 index 0000000..d6acdf7 --- /dev/null +++ b/src/app/services/frontend_errors/routers/frontend_errors_router.py @@ -0,0 +1,138 @@ +""" +Frontend Error Router + + POST /v1/telemetry/errors — report uncaught browser errors (public) + GET /v1/admin/telemetry/errors — read them, grouped (admin) + +The report endpoint is public because the storefront's visitors are not signed +in, and an error that happens before login is exactly the one worth catching. +""" + +from __future__ import annotations + +from datetime import date + +from fastapi import APIRouter, Depends, Query, Request, status +from sqlalchemy.ext.asyncio import AsyncSession + +from app.services.analytics.functions import build_period +from app.shared.config import get_settings +from app.shared.database.session import get_session_dependency +from app.shared.exceptions import NotFoundError +from app.shared.logger import get_logger +from app.shared.rate_limit import limiter + +from ..functions import coarse_browser +from ..models import ( + ErrorGroup, + FrontendErrorBatch, + FrontendErrorIngestResponse, + FrontendErrorsResponse, +) +from ..services import FrontendErrorRepository + +logger = get_logger(__name__) + +report_router = APIRouter() +admin_router = APIRouter() + +# Deliberately tighter than the analytics ingest. A component throwing inside a +# render loop is the normal failure mode here, and it will report as fast as the +# browser can loop. +_REPORT_RATE_LIMIT = "30/minute" + + +@report_router.post( + "/errors", + response_model=FrontendErrorIngestResponse, + status_code=status.HTTP_202_ACCEPTED, + summary="Report uncaught frontend errors", + description=( + "Accepts uncaught errors from the storefront or admin UI.\n\n" + "Public, because storefront visitors are not signed in and an error " + "before login is exactly the one worth catching. Records no IP, no raw " + "user agent and no identity — the user agent is reduced server-side to " + "a coarse family and major version, which is enough to reproduce a bug " + "and not enough to recognise anyone.\n\n" + "Disabled unless `FRONTEND_ERRORS_ENABLED` is set, returning 404 while off." + ), + responses={ + 404: {"description": "Frontend error reporting is not enabled."}, + 429: {"description": "Rate limit exceeded."}, + }, +) +@limiter.limit(_REPORT_RATE_LIMIT) +async def report_errors( + request: Request, + batch: FrontendErrorBatch, + session: AsyncSession = Depends(get_session_dependency), +) -> FrontendErrorIngestResponse: + settings = get_settings() + + if not settings.frontend_errors_enabled: + raise NotFoundError( + message="Frontend error reporting is not enabled on this deployment", + context={"setting": "FRONTEND_ERRORS_ENABLED"}, + ) + + # Read here and reduced immediately. The raw string never reaches storage. + browser = coarse_browser(request.headers.get("user-agent")) + + repository = FrontendErrorRepository(session) + accepted, rejected = await repository.record(batch.errors, browser) + + if accepted: + logger.warning( + "Frontend errors reported", + extra={ + "count": accepted, + "app": batch.errors[0].app.value, + "first_message": batch.errors[0].message[:200], + "browser": browser, + }, + ) + + return FrontendErrorIngestResponse( + success=True, + message="Errors recorded", + accepted=accepted, + rejected=rejected, + ) + + +@admin_router.get( + "/errors", + response_model=FrontendErrorsResponse, + summary="Frontend errors, grouped (admin)", + description=( + "Distinct errors with occurrence counts, ordered by frequency.\n\n" + "Grouped by application, error class and message rather than by stack: " + "the same fault reached from two routes produces two stacks and is one " + "bug.\n\n" + "Reports only what browsers managed to send. An error that breaks a page " + "badly enough to stop the reporter is the one that will not appear here, " + "so a quiet report is weaker evidence than a noisy one." + ), + dependencies=[Depends(get_settings)], +) +async def list_errors( + date_from: date | None = Query(None, alias="from"), + date_to: date | None = Query(None, alias="to"), + app: str | None = Query(None, pattern="^(storefront|admin)$"), + limit: int = Query(25, ge=1, le=200), + session: AsyncSession = Depends(get_session_dependency), +) -> FrontendErrorsResponse: + settings = get_settings() + period = build_period(date_from, date_to, settings.shop_timezone, default_days=7) + + repository = FrontendErrorRepository(session) + groups = await repository.grouped(period.start, period.end, app, limit) + total = await repository.total(period.start, period.end, app) + + return FrontendErrorsResponse( + success=True, + message="Frontend errors retrieved successfully", + enabled=settings.frontend_errors_enabled, + total_occurrences=total, + groups=[ErrorGroup(**group) for group in groups], + ) diff --git a/src/app/services/frontend_errors/services/__init__.py b/src/app/services/frontend_errors/services/__init__.py new file mode 100644 index 0000000..cc5016b --- /dev/null +++ b/src/app/services/frontend_errors/services/__init__.py @@ -0,0 +1,13 @@ +"""Frontend error services.""" + +from .frontend_errors_db_service import ( + MAX_CLOCK_SKEW, + FrontendErrorRepository, + get_frontend_error_repository, +) + +__all__ = [ + "MAX_CLOCK_SKEW", + "FrontendErrorRepository", + "get_frontend_error_repository", +] diff --git a/src/app/services/frontend_errors/services/frontend_errors_db_service.py b/src/app/services/frontend_errors/services/frontend_errors_db_service.py new file mode 100644 index 0000000..90d5041 --- /dev/null +++ b/src/app/services/frontend_errors/services/frontend_errors_db_service.py @@ -0,0 +1,135 @@ +""" +Frontend Error Database Service + +Writing browser error reports, and reading them back grouped. +""" + +from __future__ import annotations + +from datetime import UTC, datetime, timedelta + +from sqlalchemy import Select, and_, distinct, func, select +from sqlalchemy.ext.asyncio import AsyncSession + +from app.shared.logger import get_logger + +from ..models import FrontendErrorDB, FrontendErrorInput + +logger = get_logger(__name__) + +# Same reasoning as storefront events: a browser clock can be wrong, but it must +# not be able to write into a period already reviewed. +MAX_CLOCK_SKEW = timedelta(hours=24) + + +class FrontendErrorRepository: + """Persistence for browser error reports.""" + + def __init__(self, session: AsyncSession) -> None: + self._session = session + + async def record( + self, errors: list[FrontendErrorInput], browser: str + ) -> tuple[int, int]: + """Store a batch, discarding implausible timestamps. Returns (accepted, rejected).""" + now = datetime.now(UTC) + earliest, latest = now - MAX_CLOCK_SKEW, now + MAX_CLOCK_SKEW + + rows: list[FrontendErrorDB] = [] + rejected = 0 + + for error in errors: + occurred = error.occurred_at + if occurred.tzinfo is None: + occurred = occurred.replace(tzinfo=UTC) + if not (earliest <= occurred <= latest): + rejected += 1 + continue + + rows.append( + FrontendErrorDB( + app=error.app.value, + name=error.name, + message=error.message, + stack=error.stack, + path=error.path, + browser=browser, + occurred_at=occurred, + ) + ) + + if rows: + self._session.add_all(rows) + await self._session.commit() + + return len(rows), rejected + + async def grouped( + self, start: datetime, end: datetime, app: str | None, limit: int + ) -> list[dict]: + """ + Distinct errors with counts, ordered by how often they happen. + + Grouped by (app, name, message) rather than by stack: the same fault + reached from two routes produces two stacks and is one bug. + """ + conditions = [ + FrontendErrorDB.occurred_at >= start, + FrontendErrorDB.occurred_at < end, + ] + if app: + conditions.append(FrontendErrorDB.app == app) + + statement: Select = ( + select( + FrontendErrorDB.app, + FrontendErrorDB.name, + FrontendErrorDB.message, + func.count(FrontendErrorDB.id).label("occurrences"), + func.count(distinct(FrontendErrorDB.path)).label("affected_paths"), + func.array_agg(distinct(FrontendErrorDB.browser)).label("browsers"), + func.min(FrontendErrorDB.occurred_at).label("first_seen"), + func.max(FrontendErrorDB.occurred_at).label("last_seen"), + # Any one stack is representative; they differ only by frame + # addresses within a group. + func.min(FrontendErrorDB.stack).label("sample_stack"), + ) + .where(and_(*conditions)) + .group_by( + FrontendErrorDB.app, FrontendErrorDB.name, FrontendErrorDB.message + ) + .order_by(func.count(FrontendErrorDB.id).desc()) + .limit(limit) + ) + + result = await self._session.execute(statement) + return [ + { + "app": row.app, + "name": row.name, + "message": row.message, + "occurrences": int(row.occurrences), + "affected_paths": int(row.affected_paths), + "browsers": sorted(b for b in (row.browsers or []) if b), + "first_seen": row.first_seen, + "last_seen": row.last_seen, + "sample_stack": row.sample_stack, + } + for row in result + ] + + async def total(self, start: datetime, end: datetime, app: str | None) -> int: + conditions = [ + FrontendErrorDB.occurred_at >= start, + FrontendErrorDB.occurred_at < end, + ] + if app: + conditions.append(FrontendErrorDB.app == app) + result = await self._session.execute( + select(func.count(FrontendErrorDB.id)).where(and_(*conditions)) + ) + return int(result.scalar_one_or_none() or 0) + + +def get_frontend_error_repository(session: AsyncSession) -> FrontendErrorRepository: + return FrontendErrorRepository(session) diff --git a/src/app/shared/config/settings.py b/src/app/shared/config/settings.py index 76f2202..4ab900a 100644 --- a/src/app/shared/config/settings.py +++ b/src/app/shared/config/settings.py @@ -323,6 +323,14 @@ class Settings(BaseSettings): "endpoint returns 404 while this is false." ), ) + frontend_errors_enabled: bool = Field( + default=False, + description=( + "Accept uncaught error reports from the frontends. Off by default, " + "for the same reason as storefront analytics. The report endpoint " + "returns 404 while this is false." + ), + ) shop_timezone: str = Field( default="Europe/Berlin", description=( diff --git a/tests/test_frontend_errors_integration.py b/tests/test_frontend_errors_integration.py new file mode 100644 index 0000000..511fc22 --- /dev/null +++ b/tests/test_frontend_errors_integration.py @@ -0,0 +1,281 @@ +""" +Integration tests for frontend error reporting (S4). + +Runs against the live stack with FRONTEND_ERRORS_ENABLED=true. + + POST /v1/telemetry/errors + GET /v1/admin/telemetry/errors +""" + +import os +import subprocess +import uuid +from datetime import UTC, datetime, timedelta + +import pytest +import requests + +from auth_helpers import admin_headers + +_BASE = os.getenv("TEST_API_URL", "http://localhost:8000") +REPORT_URL = f"{_BASE}/v1/telemetry/errors" +ADMIN_URL = f"{_BASE}/v1/admin/telemetry/errors" + +_HEADERS = admin_headers() +RUN = uuid.uuid4().hex[:8] + +SAFARI = ( + "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 " + "(KHTML, like Gecko) Version/18.3 Safari/605.1.15" +) + + +def _psql(sql: str) -> str: + result = subprocess.run( + [ + "docker", + "exec", + "opentaberna-db", + "psql", + "-U", + "opentaberna", + "-d", + "opentaberna", + "-t", + "-A", + "-c", + sql, + ], + check=True, + capture_output=True, + text=True, + ) + return result.stdout.strip() + + +def _now() -> str: + return datetime.now(UTC).isoformat() + + +def _report(errors: list[dict], user_agent: str = SAFARI) -> requests.Response: + return requests.post( + REPORT_URL, json={"errors": errors}, headers={"User-Agent": user_agent} + ) + + +def _message(suffix: str) -> str: + return f"it-{RUN}-{suffix}" + + +@pytest.fixture(scope="module", autouse=True) +def enabled_or_skip(): + response = requests.get(ADMIN_URL, headers=_HEADERS) + response.raise_for_status() + if not response.json()["enabled"]: + pytest.skip("FRONTEND_ERRORS_ENABLED is false on this deployment") + yield + _psql(f"DELETE FROM frontend_errors WHERE message LIKE 'it-{RUN}-%';") + + +# --------------------------------------------------------------------------- +# Access +# --------------------------------------------------------------------------- + + +def test_reporting_needs_no_authentication(): + """Storefront visitors are not signed in; their errors matter most.""" + response = _report( + [ + { + "app": "storefront", + "name": "TypeError", + "message": _message("anon"), + "occurred_at": _now(), + } + ] + ) + assert response.status_code == 202 + assert response.json()["accepted"] == 1 + + +def test_reading_errors_requires_admin(): + assert requests.get(ADMIN_URL).status_code in (401, 403) + + +# --------------------------------------------------------------------------- +# What must never be stored +# --------------------------------------------------------------------------- + + +def test_the_table_cannot_hold_an_ip_or_a_raw_user_agent(): + """ + The privacy posture is structural. `browser` holds a reduced label; there is + nowhere to put a fingerprint. + """ + columns = set( + _psql( + "SELECT column_name FROM information_schema.columns " + "WHERE table_name = 'frontend_errors';" + ).splitlines() + ) + + forbidden = { + "ip", + "ip_address", + "remote_addr", + "user_agent", + "email", + "customer_id", + "keycloak_user_id", + "user_id", + } + assert not (columns & forbidden), f"PII column present: {columns & forbidden}" + assert "browser" in columns + + +def test_the_raw_user_agent_is_reduced_before_storage(): + message = _message("ua") + _report( + [ + { + "app": "storefront", + "name": "TypeError", + "message": message, + "occurred_at": _now(), + } + ], + user_agent=SAFARI, + ) + + stored = _psql(f"SELECT browser FROM frontend_errors WHERE message = '{message}';") + assert stored == "Safari 18" + assert "AppleWebKit" not in stored + assert "Macintosh" not in stored + + +def test_a_query_string_carrying_a_token_is_not_stored(): + message = _message("qs") + _report( + [ + { + "app": "storefront", + "name": "TypeError", + "message": message, + "path": "/checkout?token=supersecret", + "occurred_at": _now(), + } + ] + ) + + stored = _psql(f"SELECT path FROM frontend_errors WHERE message = '{message}';") + assert stored == "/checkout" + assert "supersecret" not in stored + + +# --------------------------------------------------------------------------- +# Hostile and merely broken input +# --------------------------------------------------------------------------- + + +def test_an_unknown_app_is_refused(): + response = _report( + [ + { + "app": "wordpress", + "name": "TypeError", + "message": _message("bad"), + "occurred_at": _now(), + } + ] + ) + assert response.status_code == 422 + + +def test_stale_timestamps_are_discarded(): + old = (datetime.now(UTC) - timedelta(days=400)).isoformat() + response = _report( + [ + { + "app": "storefront", + "name": "TypeError", + "message": _message("stale"), + "occurred_at": old, + }, + { + "app": "storefront", + "name": "TypeError", + "message": _message("fresh"), + "occurred_at": _now(), + }, + ] + ) + body = response.json() + assert body["accepted"] == 1 + assert body["rejected"] == 1 + + +# --------------------------------------------------------------------------- +# Grouping +# --------------------------------------------------------------------------- + + +def test_the_same_fault_on_two_routes_is_one_group(): + """ + One bug produces thousands of identical rows. Reading them ungrouped is + useless, and grouping by stack would split one bug into many. + """ + message = _message("grouped") + _report( + [ + { + "app": "storefront", + "name": "TypeError", + "message": message, + "path": "/shop/1", + "stack": "at Product.render", + "occurred_at": _now(), + }, + { + "app": "storefront", + "name": "TypeError", + "message": message, + "path": "/shop/2", + "stack": "at Product.render (other frame)", + "occurred_at": _now(), + }, + ] + ) + + payload = requests.get(ADMIN_URL, headers=_HEADERS, params={"limit": 200}).json() + group = next(g for g in payload["groups"] if g["message"] == message) + + assert group["occurrences"] == 2 + assert group["affected_paths"] == 2 + assert group["browsers"] == ["Safari 18"] + assert group["sample_stack"] + + +def test_groups_are_ordered_by_how_often_they_happen(): + payload = requests.get(ADMIN_URL, headers=_HEADERS, params={"limit": 200}).json() + counts = [g["occurrences"] for g in payload["groups"]] + assert counts == sorted(counts, reverse=True) + + +def test_errors_can_be_filtered_to_one_application(): + _report( + [ + { + "app": "admin", + "name": "HttpErrorResponse", + "message": _message("adminonly"), + "occurred_at": _now(), + } + ] + ) + + payload = requests.get( + ADMIN_URL, headers=_HEADERS, params={"app": "admin", "limit": 200} + ).json() + + assert payload["groups"] + assert all(g["app"] == "admin" for g in payload["groups"]) diff --git a/tests/test_frontend_errors_unit.py b/tests/test_frontend_errors_unit.py new file mode 100644 index 0000000..9610296 --- /dev/null +++ b/tests/test_frontend_errors_unit.py @@ -0,0 +1,139 @@ +""" +Unit tests for frontend error reporting — schema and user-agent reduction. + +The report endpoint is public, so its schema is a boundary. And the whole point +of reducing the user agent is that a fingerprint never reaches storage, which is +a property worth pinning rather than trusting. +""" + +from datetime import UTC, datetime + +import pytest +from pydantic import ValidationError as PydanticValidationError + +from app.services.frontend_errors.functions import coarse_browser +from app.services.frontend_errors.models import ( + MAX_ERRORS_PER_BATCH, + MAX_STACK_CHARS, + FrontendErrorBatch, + FrontendErrorInput, +) + + +def _error(**overrides) -> dict: + base = { + "app": "storefront", + "name": "TypeError", + "message": "undefined is not a function", + "occurred_at": datetime.now(UTC), + } + base.update(overrides) + return base + + +# --------------------------------------------------------------------------- +# The user agent is reduced, never stored +# --------------------------------------------------------------------------- + + +@pytest.mark.parametrize( + "user_agent,expected", + [ + ( + "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/605.1.15 " + "(KHTML, like Gecko) Version/18.3 Safari/605.1.15", + "Safari 18", + ), + ( + "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 " + "(KHTML, like Gecko) Chrome/140.0.0.0 Safari/537.36", + "Chrome 140", + ), + ( + "Mozilla/5.0 (Windows NT 10.0; Win64; x64) Gecko/20100101 Firefox/133.0", + "Firefox 133", + ), + ], +) +def test_common_browsers_are_reduced_to_family_and_major(user_agent, expected): + assert coarse_browser(user_agent) == expected + + +def test_edge_is_not_reported_as_chrome(): + """ + Edge, Opera and Chrome all claim to be one another. The most specific + pattern has to win, or every error looks like it came from Chrome. + """ + edge = ( + "Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 " + "(KHTML, like Gecko) Chrome/140.0.0.0 Safari/537.36 Edg/140.0.0.0" + ) + assert coarse_browser(edge) == "Edge 140" + + +def test_nothing_from_the_original_string_survives(): + """ + The reduction is the privacy guarantee. Whatever a client sends, the result + is a known family name and an integer — never a fragment of the input. + """ + hostile = "Mozilla/5.0 Chrome/140 user=alice@example.com token=abc123" + result = coarse_browser(hostile) + + assert result == "Chrome 140" + assert "alice@example.com" not in result + assert "abc123" not in result + + +def test_an_unrecognised_agent_is_unknown_not_stored_verbatim(): + assert coarse_browser("something entirely made up") == "unknown" + assert coarse_browser(None) == "unknown" + assert coarse_browser("") == "unknown" + + +# --------------------------------------------------------------------------- +# The schema as a boundary +# --------------------------------------------------------------------------- + + +def test_query_strings_are_stripped_from_the_path(): + error = FrontendErrorInput(**_error(path="/checkout?token=secret&email=a@b.c")) + assert error.path == "/checkout" + + +def test_a_huge_stack_is_truncated_rather_than_rejected(): + """ + A stack trace is unbounded input from a public endpoint, and also the most + useful field here. Cutting it keeps the top frames, where the fault is. + """ + error = FrontendErrorInput(**_error(stack="x" * (MAX_STACK_CHARS * 3))) + + assert len(error.stack) < MAX_STACK_CHARS + 100 + assert error.stack.endswith("truncated") + + +def test_unknown_apps_are_refused(): + """A closed set, so the field cannot become free text from a public endpoint.""" + with pytest.raises(PydanticValidationError): + FrontendErrorInput(**_error(app="not-an-app")) + + +def test_extra_fields_are_refused(): + with pytest.raises(PydanticValidationError): + FrontendErrorInput(**_error(user_agent="Mozilla/5.0")) + + with pytest.raises(PydanticValidationError): + FrontendErrorInput(**_error(customer_id="123")) + + +def test_batch_size_is_capped(): + """ + A component throwing inside a render loop is the normal failure mode, and + it will report as fast as the browser can loop. + """ + with pytest.raises(PydanticValidationError): + FrontendErrorBatch(errors=[_error() for _ in range(MAX_ERRORS_PER_BATCH + 1)]) + + +def test_message_length_is_bounded(): + with pytest.raises(PydanticValidationError): + FrontendErrorInput(**_error(message="x" * 5000))