diff --git a/backend/app/api/deps.py b/backend/app/api/deps.py index 03d16bb..aa2fbc0 100644 --- a/backend/app/api/deps.py +++ b/backend/app/api/deps.py @@ -108,6 +108,27 @@ async def get_current_user_optional( _billing_logger = _structlog.get_logger() +def _billing_fail_closed_enabled() -> bool: + """Whether billing enforcement should fail-closed on backing-store errors. + + Default policy: + * ``ENVIRONMENT=production`` → fail-closed (return True). + * Otherwise → fail-open (legacy behaviour). + * ``BILLING_FAIL_CLOSED`` env var (``1``/``true``/``yes``/``on`` or + ``0``/``false``/``no``/``off``) overrides the default in either + direction so ops can flip without a redeploy and tests can pin + the mode. + + Read at call time (not module import) so monkeypatched env vars in + tests are honoured. + """ + import os + raw = os.getenv("BILLING_FAIL_CLOSED") + if raw is not None: + return raw.strip().lower() in {"1", "true", "yes", "on"} + return os.getenv("ENVIRONMENT", "development").strip().lower() == "production" + + async def check_usage_guard(current_user: Dict[str, Any]) -> None: """Backstop per-user/per-platform cap that runs BEFORE billing. @@ -145,6 +166,17 @@ async def check_billing_limit(feature: str, current_user: Dict[str, Any]) -> Non """Check billing limit for a feature. Raises 402 if over limit. If the user has no org (testing/free solo mode), the check is skipped. + + **Fail-closed behaviour (TD-7 / m12-pr11):** when the org-fetch path + raises (e.g. Supabase outage), we historically swallowed the error + and treated the user as "no org", which silently disabled billing + enforcement for every authenticated request. That is fail-open and + unacceptable in production. We now consult + :func:`_billing_fail_closed_enabled` — when true (default in + ``ENVIRONMENT=production``, opt-in everywhere else via + ``BILLING_FAIL_CLOSED=1``) we raise 503 instead of skipping the + check. Local dev / CI keep the legacy permissive behaviour so tests + that don't seed an org still pass. """ from app.services.org import OrgService from app.services.billing import BillingService @@ -152,7 +184,19 @@ async def check_billing_limit(feature: str, current_user: Dict[str, Any]) -> Non org_service = OrgService() try: orgs = await org_service.get_user_orgs(current_user["id"]) - except Exception: + except Exception as exc: + _billing_logger.error( + "billing_org_fetch_failed", + user_id=current_user.get("id"), + feature=feature, + error=str(exc)[:200], + fail_closed=_billing_fail_closed_enabled(), + ) + if _billing_fail_closed_enabled(): + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail="Billing service is temporarily unavailable. Please try again.", + ) orgs = [] if not orgs: diff --git a/backend/tests/unit/test_billing_fail_closed.py b/backend/tests/unit/test_billing_fail_closed.py new file mode 100644 index 0000000..f5fee1c --- /dev/null +++ b/backend/tests/unit/test_billing_fail_closed.py @@ -0,0 +1,178 @@ +"""Tests for TD-7 / m12-pr11 billing fail-closed behaviour.""" +from __future__ import annotations + +import pytest +from fastapi import HTTPException + +from app.api import deps + + +# ── _billing_fail_closed_enabled ────────────────────────────────────── + + +def test_default_dev_is_fail_open(monkeypatch): + monkeypatch.delenv("BILLING_FAIL_CLOSED", raising=False) + monkeypatch.delenv("ENVIRONMENT", raising=False) + assert deps._billing_fail_closed_enabled() is False + + +def test_production_default_is_fail_closed(monkeypatch): + monkeypatch.delenv("BILLING_FAIL_CLOSED", raising=False) + monkeypatch.setenv("ENVIRONMENT", "production") + assert deps._billing_fail_closed_enabled() is True + + +def test_env_override_forces_fail_closed_in_dev(monkeypatch): + monkeypatch.setenv("ENVIRONMENT", "development") + monkeypatch.setenv("BILLING_FAIL_CLOSED", "1") + assert deps._billing_fail_closed_enabled() is True + + +def test_env_override_forces_fail_open_in_production(monkeypatch): + monkeypatch.setenv("ENVIRONMENT", "production") + monkeypatch.setenv("BILLING_FAIL_CLOSED", "0") + assert deps._billing_fail_closed_enabled() is False + + +@pytest.mark.parametrize("truthy", ["1", "true", "TRUE", "Yes", "on"]) +def test_env_truthy_variants(monkeypatch, truthy): + monkeypatch.setenv("BILLING_FAIL_CLOSED", truthy) + assert deps._billing_fail_closed_enabled() is True + + +@pytest.mark.parametrize("falsy", ["0", "false", "no", "off", ""]) +def test_env_falsy_variants(monkeypatch, falsy): + monkeypatch.setenv("BILLING_FAIL_CLOSED", falsy) + monkeypatch.delenv("ENVIRONMENT", raising=False) + assert deps._billing_fail_closed_enabled() is False + + +# ── check_billing_limit org-fetch failure path ──────────────────────── + + +class _FakeOrgService: + def __init__(self, *, orgs=None, raises=None): + self._orgs = orgs or [] + self._raises = raises + + async def get_user_orgs(self, user_id): + if self._raises is not None: + raise self._raises + return self._orgs + + +class _FakeBillingService: + def __init__(self, allowed=True, raises=None): + self._allowed = allowed + self._raises = raises + + async def check_limit(self, org_id, feature): + if self._raises is not None: + raise self._raises + return self._allowed + + +@pytest.fixture +def patch_services(monkeypatch): + """Patch OrgService / BillingService imports inside check_billing_limit.""" + state = {"org": _FakeOrgService(), "billing": _FakeBillingService()} + + import app.services.org as org_mod + import app.services.billing as billing_mod + + monkeypatch.setattr(org_mod, "OrgService", lambda: state["org"]) + monkeypatch.setattr(billing_mod, "BillingService", lambda: state["billing"]) + return state + + +@pytest.mark.asyncio +async def test_org_fetch_failure_fails_open_in_dev(monkeypatch, patch_services): + """Legacy permissive path: dev/test envs swallow the error.""" + monkeypatch.delenv("BILLING_FAIL_CLOSED", raising=False) + monkeypatch.delenv("ENVIRONMENT", raising=False) + patch_services["org"] = _FakeOrgService(raises=RuntimeError("supabase down")) + + # Should silently return (no orgs → skip enforcement). + await deps.check_billing_limit("resume_generation", {"id": "user-1"}) + + +@pytest.mark.asyncio +async def test_org_fetch_failure_fails_closed_in_production( + monkeypatch, patch_services +): + monkeypatch.delenv("BILLING_FAIL_CLOSED", raising=False) + monkeypatch.setenv("ENVIRONMENT", "production") + patch_services["org"] = _FakeOrgService(raises=RuntimeError("supabase down")) + + with pytest.raises(HTTPException) as exc: + await deps.check_billing_limit("resume_generation", {"id": "user-1"}) + assert exc.value.status_code == 503 + assert "Billing service" in exc.value.detail + + +@pytest.mark.asyncio +async def test_org_fetch_failure_fails_closed_when_env_override( + monkeypatch, patch_services +): + monkeypatch.setenv("ENVIRONMENT", "development") + monkeypatch.setenv("BILLING_FAIL_CLOSED", "1") + patch_services["org"] = _FakeOrgService(raises=RuntimeError("boom")) + + with pytest.raises(HTTPException) as exc: + await deps.check_billing_limit("resume_generation", {"id": "user-1"}) + assert exc.value.status_code == 503 + + +@pytest.mark.asyncio +async def test_org_fetch_failure_fails_open_when_override_off( + monkeypatch, patch_services +): + monkeypatch.setenv("ENVIRONMENT", "production") + monkeypatch.setenv("BILLING_FAIL_CLOSED", "0") + patch_services["org"] = _FakeOrgService(raises=RuntimeError("boom")) + + # Override forces fail-open even in production — should not raise. + await deps.check_billing_limit("resume_generation", {"id": "user-1"}) + + +@pytest.mark.asyncio +async def test_no_orgs_skips_enforcement(monkeypatch, patch_services): + """Genuine no-org user (solo mode) is unaffected by fail-closed mode.""" + monkeypatch.setenv("BILLING_FAIL_CLOSED", "1") + patch_services["org"] = _FakeOrgService(orgs=[]) + # Should return cleanly — no org → skip. + await deps.check_billing_limit("resume_generation", {"id": "user-1"}) + + +@pytest.mark.asyncio +async def test_org_present_then_billing_check_failure_still_503( + monkeypatch, patch_services +): + """Pre-existing 503 path on billing.check_limit() error is preserved.""" + monkeypatch.delenv("BILLING_FAIL_CLOSED", raising=False) + monkeypatch.delenv("ENVIRONMENT", raising=False) + patch_services["org"] = _FakeOrgService(orgs=[{"id": "org-1"}]) + patch_services["billing"] = _FakeBillingService(raises=RuntimeError("billing down")) + + with pytest.raises(HTTPException) as exc: + await deps.check_billing_limit("resume_generation", {"id": "user-1"}) + assert exc.value.status_code == 503 + + +@pytest.mark.asyncio +async def test_org_present_and_allowed_passes(monkeypatch, patch_services): + monkeypatch.delenv("BILLING_FAIL_CLOSED", raising=False) + patch_services["org"] = _FakeOrgService(orgs=[{"id": "org-1"}]) + patch_services["billing"] = _FakeBillingService(allowed=True) + await deps.check_billing_limit("resume_generation", {"id": "user-1"}) + + +@pytest.mark.asyncio +async def test_org_present_and_denied_raises_402(monkeypatch, patch_services): + monkeypatch.delenv("BILLING_FAIL_CLOSED", raising=False) + patch_services["org"] = _FakeOrgService(orgs=[{"id": "org-1"}]) + patch_services["billing"] = _FakeBillingService(allowed=False) + + with pytest.raises(HTTPException) as exc: + await deps.check_billing_limit("resume_generation", {"id": "user-1"}) + assert exc.value.status_code == 402 diff --git a/docs/architecture/WORLD_CLASS_ARCHITECTURE_BLUEPRINT.md b/docs/architecture/WORLD_CLASS_ARCHITECTURE_BLUEPRINT.md index 105caf2..3a85521 100644 --- a/docs/architecture/WORLD_CLASS_ARCHITECTURE_BLUEPRINT.md +++ b/docs/architecture/WORLD_CLASS_ARCHITECTURE_BLUEPRINT.md @@ -1041,7 +1041,7 @@ Items that are not P0/P1 but must be tracked. Reviewed quarterly. | TD-4 | `requirements.txt` unpinned ranges | Reproducibility | Add `requirements.lock` (pip-compile) | | TD-5 | Python 3.11 in CI vs 3.13 local | Dev/CI drift | Pin one and document | | TD-6 | Multiple root-level `*_PLAN.md` | No canonical doc → drift | Archive to `docs/_archive/` (this PR) | -| TD-7 | `_billing_logger` swallows org-fetch errors | Fail-open on Supabase outage | Make fail-closed in prod | +| TD-7 | `_billing_logger` swallows org-fetch errors | Fail-open on Supabase outage | SHIPPED (m12-pr11) — `check_billing_limit` now consults `_billing_fail_closed_enabled()`: defaults to fail-closed (503) in `ENVIRONMENT=production`, fail-open elsewhere; `BILLING_FAIL_CLOSED=1`/`0` overrides in either direction so ops can flip without redeploy | | TD-8 | No mutation testing | Critical paths under-tested | Stage A end | | TD-9 | No per-region observability sharding | One Sentry org = noise | Stage C | | TD-10 | Dual-namespace lazy imports in `ai_engine/client.py` | Smell of unsettled package layout | Resolved by P1-6 |