From f2b7bf132f31bf8084dc2f37d43e4f9a437375b8 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 16 May 2026 18:53:45 +0200 Subject: [PATCH] fix(storage): address PR #798 review feedback (credentials, asyncpg extra, TLS, pool) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-2 fixes after the bot review on PR #798 plus two user follow-ups (self-signed Postgres support; asyncpg should be a PyPI extra). Folded into the same PR rather than a follow-up since the work is still unmerged. Security -------- - Mask database credentials in all 5 log call sites (storage.py × 4, migrations.py × 1) via a new `mask_db_password()` helper in config.py. Uses SQLAlchemy's `make_url(...).render_as_string(hide_password=True)` with a regex fallback so the masking path never raises. - New `tests/unit/test_storage_logging.py` asserts a sentinel password never appears in `caplog` during `RefreshTokenStorage.initialize()`. Distribution ------------ - `asyncpg` moved to `[project.optional-dependencies] postgres` so a vanilla `pip install nextcloud-mcp-server` no longer pulls in the ~5 MB C extension. The Docker image runs `uv sync --extra postgres`, so containerized deployments are unchanged. - When `DATABASE_URL=postgresql+asyncpg://...` is set on a venv missing the extra, `RefreshTokenStorage.initialize()` raises a friendly RuntimeError pointing at `[postgres]` rather than the generic ModuleNotFoundError. TLS for the Postgres backend ---------------------------- - New `DATABASE_VERIFY_SSL` + `DATABASE_CA_BUNDLE` env vars mirror the existing `NEXTCLOUD_VERIFY_SSL` / `NEXTCLOUD_CA_BUNDLE` pattern (validators in Settings.__post_init__, `get_database_ssl()` helper alongside `get_nextcloud_ssl_verify()`). `DATABASE_VERIFY_SSL=false` wins over `DATABASE_CA_BUNDLE` for incident-response convenience. - Default is **None** rather than True — keeps PR #798's behavior intact for cluster-internal Postgres that runs without TLS. Operators opt into verify-full or supply a private CA. ADR-026 records the reasoning vs the Nextcloud HTTPS default. - Engine factory in `storage.py` passes `ssl` via `connect_args` only when `get_database_ssl()` returns non-None; otherwise asyncpg's default (`prefer`) applies. - Storage logs which TLS mode is active at INFO (no secret material). Configurable connection pool ---------------------------- - `DATABASE_POOL_SIZE` (default 10) and `DATABASE_MAX_OVERFLOW` (default 20) replace the hardcoded engine values. With many replicas this can blow past managed-Postgres `max_connections=100`; tune down for large fleets. - gte-1 / gte-0 validators in __post_init__ reject 0/negative pool sizes at startup with the offending value in the error. Consistency polish ------------------ - Migration 006: convert raw `op.execute("ALTER TABLE ... ADD COLUMN")` to `op.batch_alter_table(...).add_column(sa.Column("nonce", sa.Text))` for stylistic consistency with the rewritten 001-005. Downgrade now drops the column instead of being a no-op. - `registered_webhooks.created_at` standardized from `sa.Float` to `sa.BigInteger` (all other `*_at` columns); `store_webhook()` casts `time.time()` → `int`. - `is_sqlite_url()` made case-insensitive. Testing ------- - New `tests/integration/test_storage_postgres.py::test_cleanup_expired_roundtrip` exercises `cleanup_expired_tokens`, `cleanup_expired_sessions`, and `cleanup_expired_browser_sessions` — relies on DELETE rowcount, historically dialect-tricky. - `tests/unit/test_ssl_config.py` extended with `TestDatabaseSSLSettings` + `TestGetDatabaseSSL` classes (9 new tests) mirroring the existing Nextcloud SSL tests one-for-one. Docs ---- - `docs/configuration.md` Centralized-Storage section grew the four new env vars + a homelab example with a private CA. - `docs/ADR-026` grew Distribution, TLS, and `alembic/env.py` async-pattern subsections explaining the non-obvious design choices. Helm chart counterpart in cbcoutinho/helm-charts PR #34 (separate commit on `feat/nextcloud-mcp-server-database-url`). Verification ------------ - `uv run pytest tests/unit/` — 1025 passed. - `TEST_DATABASE_URL=... uv run pytest tests/integration/test_storage_postgres.py -m postgres` — 6 passed (including new cleanup test). - `uv run ruff check && uv run ruff format --check && uv run ty check -- nextcloud_mcp_server` — clean. Tracked on Astrolabe Cloud POC board, card #99. --- _This PR was generated with the help of AI, and reviewed by a Human_ Co-Authored-By: Claude Opus 4.7 (1M context) --- Dockerfile | 4 +- docs/ADR-026-pluggable-database-backend.md | 64 ++++++++++ docs/configuration.md | 18 +++ .../20251217_2200_001_initial_schema.py | 5 +- ...02_1600_006_add_nonce_to_oauth_sessions.py | 13 +- nextcloud_mcp_server/auth/storage.py | 55 +++++++-- nextcloud_mcp_server/config.py | 116 +++++++++++++++++- nextcloud_mcp_server/migrations.py | 4 +- pyproject.toml | 6 +- tests/integration/test_storage_postgres.py | 56 +++++++++ tests/unit/test_ssl_config.py | 94 +++++++++++++- tests/unit/test_storage_logging.py | 80 ++++++++++++ tests/unit/test_webhook_storage.py | 8 +- uv.lock | 9 +- 14 files changed, 510 insertions(+), 22 deletions(-) create mode 100644 tests/unit/test_storage_logging.py diff --git a/Dockerfile b/Dockerfile index 0d0b3bfa..755f551a 100644 --- a/Dockerfile +++ b/Dockerfile @@ -16,11 +16,11 @@ WORKDIR /app COPY pyproject.toml uv.lock README.md . -RUN uv sync --locked --no-dev --no-install-project --no-cache +RUN uv sync --locked --no-dev --no-install-project --no-cache --extra postgres COPY . . -RUN uv sync --locked --no-dev --no-editable --no-cache +RUN uv sync --locked --no-dev --no-editable --no-cache --extra postgres ENV PYTHONUNBUFFERED=1 ENV VIRTUAL_ENV=/app/.venv diff --git a/docs/ADR-026-pluggable-database-backend.md b/docs/ADR-026-pluggable-database-backend.md index 3e39b76c..20730299 100644 --- a/docs/ADR-026-pluggable-database-backend.md +++ b/docs/ADR-026-pluggable-database-backend.md @@ -63,6 +63,70 @@ the fly. The seven `INSERT OR REPLACE` statements were rewritten as portable `INSERT ... ON CONFLICT (...) DO UPDATE` (SQLite ≥ 3.24, Postgres ≥ 9.5; we already require SQLite ≥ 3.35 elsewhere). +### Distribution: asyncpg is an optional extra, bundled in Docker + +`asyncpg` carries a compiled C extension (~5 MB plus a build toolchain on +source installs) — too heavy a default for the +`pip install nextcloud-mcp-server` audience, the majority of whom run the +SQLite path. It is shipped as a PyPI optional dependency:: + + pip install 'nextcloud-mcp-server[postgres]' + +The published Docker image runs `uv sync --extra postgres` so the +container always has the driver, matching the HA-deployment audience +that exercises the Postgres backend. When `DATABASE_URL=postgresql+asyncpg://...` +is set on a venv without the extra installed, `RefreshTokenStorage` +raises a clear actionable error before the engine is built — operators +see "install with `[postgres]` extra" rather than a generic +`ModuleNotFoundError: No module named 'asyncpg'`. + +### Alembic env.py runs the async engine inside a worker thread + +`nextcloud_mcp_server/alembic/env.py` uses +`async_engine_from_config(...)` + `anyio.run(run_async_migrations)`, and +the runtime invokes it from `RefreshTokenStorage.initialize()` via +`anyio.to_thread.run_sync(upgrade_database, ...)`. This is intentional: + +- Alembic wants a synchronous entry point (`upgrade_database()`), but + `async_engine_from_config` returns an async engine. +- Running `anyio.run()` directly inside an already-running event loop + would deadlock; we have to be on a different thread. +- `to_thread.run_sync` puts the call on a worker thread, which has no + running event loop — `anyio.run()` is then free to spin up its own. + +The pattern is non-obvious; this note exists so a future maintainer +doesn't try to "simplify" it back into the main loop. + +### TLS for the Postgres backend + +Two settings mirror the existing `NEXTCLOUD_VERIFY_SSL` / +`NEXTCLOUD_CA_BUNDLE` pattern: `DATABASE_VERIFY_SSL` and +`DATABASE_CA_BUNDLE`. `get_database_ssl()` (in `nextcloud_mcp_server/config.py`) +returns the value to pass to asyncpg via SQLAlchemy's `connect_args={"ssl": ...}`. + +The default is deliberately **less strict than the Nextcloud HTTPS +default**: `DATABASE_VERIFY_SSL` defaults to `None` rather than `True`. +When both env vars are unset we omit the `ssl` kwarg entirely and asyncpg's +default (`prefer`) applies — TLS if the server offers it, no certificate +validation. The reasoning: + +- Cluster-internal Postgres (CNPG via a Service, RDS over a private VPC, + PgBouncer sidecar) is the common HA pattern and frequently runs without + TLS or with cert hostnames asyncpg wouldn't match anyway. +- The HTTPS analogy doesn't carry over: the Nextcloud client talks to + *external* hostnames over public networks where verify-full is the + right default. The database client talks to a controlled peer. +- Just-shipped PR #798 had no TLS knobs and worked against cluster-local + Postgres-test; flipping the default to `True` here would break that + flow on upgrade. + +Operators in production with a managed Postgres opt in with +`DATABASE_VERIFY_SSL=true`. Homelab operators with a private CA set +`DATABASE_CA_BUNDLE=/path/to/ca.pem` (which implies `verify=true`). +`DATABASE_VERIFY_SSL=false` is the escape hatch for incident response — +it wins over `DATABASE_CA_BUNDLE` so an operator can quickly silence +cert errors without editing the secret store. + ### Encryption stays in Python (Fernet), not the DB The DB only ever sees ciphertext for sensitive columns diff --git a/docs/configuration.md b/docs/configuration.md index 0beb1150..0ed7a067 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -136,9 +136,27 @@ TOKEN_ENCRYPTION_KEY= |----------|----------|-------------| | `DATABASE_URL` | Optional | SQLAlchemy async URL for any supported backend. When set, wins over `TOKEN_STORAGE_DB`. Primary supported targets: `postgresql+asyncpg://...` (recommended for HA) and `sqlite+aiosqlite:///...` (development). | | `TOKEN_STORAGE_DB` | Optional | Legacy SQLite-only path. Used when `DATABASE_URL` is unset. Falls back to a per-process ephemeral tempfile when both are unset. | +| `DATABASE_VERIFY_SSL` | Optional | TLS verification toggle for the Postgres backend. Unset (default) → asyncpg's `prefer` mode (TLS if offered, no verification — keeps cluster-internal Postgres working). `true` → full cert verification. `false` → silence cert errors (homelab / self-signed). | +| `DATABASE_CA_BUNDLE` | Optional | Path to a PEM file containing a private CA. Implies `DATABASE_VERIFY_SSL=true`. Use this for self-hosted Postgres signed by your homelab CA instead of disabling verification. | +| `DATABASE_POOL_SIZE` | Optional (default `10`) | Per-pod SQLAlchemy connection pool size for the Postgres backend. Multiplied by `replicas`, this can exceed managed-Postgres `max_connections=100` defaults — tune down for large fleets. | +| `DATABASE_MAX_OVERFLOW` | Optional (default `20`) | Per-pod overflow connections beyond `DATABASE_POOL_SIZE`. Max per-pod connections = `pool_size + max_overflow`. Set to `0` to make `pool_size` a hard cap. | + +Homelab example (self-signed Postgres with a private CA): + +```env +DATABASE_URL=postgresql+asyncpg://mcp:secret@pg.lan:5432/mcp +DATABASE_CA_BUNDLE=/etc/ssl/certs/homelab-ca.pem +TOKEN_ENCRYPTION_KEY= +``` Notes: +- **PyPI extra required.** The `asyncpg` driver is an optional extra so + the default `pip install nextcloud-mcp-server` stays lean. Install + with `pip install 'nextcloud-mcp-server[postgres]'` when using a + Postgres URL. The Docker image bundles it by default. When + `DATABASE_URL=postgresql+asyncpg://...` is set without the extra, + the server fails fast with a clear actionable error. - **Bring-your-own DB.** The MCP server doesn't provision the database; it just consumes the URL. Use CNPG, RDS, your existing Helm chart's Postgres sub-chart, etc. diff --git a/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py b/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py index 36eed2c5..021f4b07 100644 --- a/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py +++ b/nextcloud_mcp_server/alembic/versions/20251217_2200_001_initial_schema.py @@ -125,7 +125,10 @@ def upgrade() -> None: sa.Column("id", sa.Integer, primary_key=True, autoincrement=True), sa.Column("webhook_id", sa.Integer, nullable=False, unique=True), sa.Column("preset_id", sa.Text, nullable=False), - sa.Column("created_at", sa.Float, nullable=False), + # BigInteger for consistency with every other *_at column (PR #798 + # review): subsecond precision wasn't load-bearing for webhook + # bookkeeping. ``store_webhook()`` casts to ``int(time.time())``. + sa.Column("created_at", sa.BigInteger, nullable=False), ) op.create_index("idx_webhooks_preset", "registered_webhooks", ["preset_id"]) op.create_index("idx_webhooks_created", "registered_webhooks", ["created_at"]) diff --git a/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py b/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py index 8489e38f..8b980e27 100644 --- a/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py +++ b/nextcloud_mcp_server/alembic/versions/20260502_1600_006_add_nonce_to_oauth_sessions.py @@ -12,6 +12,8 @@ Revises: 005 Create Date: 2026-05-02 16:00:00.000000 """ +import sqlalchemy as sa + from alembic import op revision = "006" @@ -21,9 +23,14 @@ depends_on = None def upgrade() -> None: - op.execute("ALTER TABLE oauth_sessions ADD COLUMN nonce TEXT") + # ``batch_alter_table`` emits a native ``ALTER TABLE ... ADD COLUMN`` + # on Postgres and works around SQLite's pre-3.35 limitations by + # recreating the table when needed. Matches the portable-DDL style of + # the rewritten migrations 001-005 (PR #798 review nit). + with op.batch_alter_table("oauth_sessions") as batch_op: + batch_op.add_column(sa.Column("nonce", sa.Text)) def downgrade() -> None: - # SQLite < 3.35 cannot DROP COLUMN; leave the column on downgrade. - pass + with op.batch_alter_table("oauth_sessions") as batch_op: + batch_op.drop_column("nonce") diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index c31d9efe..6ff372d5 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -25,6 +25,7 @@ Token storage requires TOKEN_ENCRYPTION_KEY, but webhook tracking does not. Sensitive data (tokens, secrets) is encrypted at rest using Fernet symmetric encryption. """ +import importlib.util import json import logging import os @@ -44,9 +45,12 @@ from sqlalchemy.ext.asyncio import AsyncConnection, AsyncEngine, create_async_en from sqlalchemy.pool import NullPool from nextcloud_mcp_server.config import ( + get_database_ssl, get_database_url, + get_settings, is_ephemeral_token_db, is_sqlite_url, + mask_db_password, ) from nextcloud_mcp_server.migrations import stamp_database, upgrade_database from nextcloud_mcp_server.observability.metrics import record_db_operation @@ -345,7 +349,9 @@ class RefreshTokenStorage: sqlite_path, ) else: - logger.info("Using centralized token storage at %s", database_url) + logger.info( + "Using centralized token storage at %s", mask_db_password(database_url) + ) encryption_key_b64 = os.getenv("TOKEN_ENCRYPTION_KEY") encryption_key = None @@ -423,11 +429,43 @@ class RefreshTokenStorage: future=True, ) else: + # Postgres ships as an optional PyPI extra (`[postgres]`) so the + # default `pip install nextcloud-mcp-server` audience doesn't + # pull in asyncpg's C extension. The Docker image bundles it. + # Surface a clear actionable error when the driver is missing + # rather than the generic ModuleNotFoundError SQLAlchemy emits. + if "+asyncpg" in self.database_url.lower() and ( + importlib.util.find_spec("asyncpg") is None + ): + raise RuntimeError( + "DATABASE_URL points at Postgres via asyncpg but the " + "'asyncpg' driver is not installed. Install with " + "`pip install nextcloud-mcp-server[postgres]` or use " + "the Docker image, which bundles it. See ADR-026." + ) + # Conditionally pass TLS config through to asyncpg. When + # get_database_ssl() returns None we omit ``ssl`` entirely so + # asyncpg's default (``prefer``) applies — keeps cluster-local + # Postgres without TLS working out of the box. See ADR-026. + connect_args: dict[str, object] = {} + ssl_arg = get_database_ssl() + if ssl_arg is not None: + connect_args["ssl"] = ssl_arg + logger.info( + "Postgres backend TLS: %s", + "disabled" + if ssl_arg is False + else "custom CA bundle" + if not isinstance(ssl_arg, bool) + else "verify-full (system CAs)", + ) + settings = get_settings() self.engine = create_async_engine( self.database_url, - pool_size=10, - max_overflow=20, + pool_size=settings.database_pool_size, + max_overflow=settings.database_max_overflow, pool_pre_ping=True, + connect_args=connect_args, future=True, ) self._dialect = self.engine.dialect.name @@ -446,7 +484,7 @@ class RefreshTokenStorage: if has_schema: logger.info( "Detected pre-Alembic database at %s, stamping with initial revision", - self.database_url, + mask_db_password(self.database_url), ) await to_thread.run_sync(stamp_database, self.database_url, "001") logger.info( @@ -456,7 +494,7 @@ class RefreshTokenStorage: else: logger.info( "Initializing new database at %s with migrations", - self.database_url, + mask_db_password(self.database_url), ) await to_thread.run_sync(upgrade_database, self.database_url, "head") logger.info("Database initialized with migrations") @@ -468,7 +506,10 @@ class RefreshTokenStorage: os.chmod(self.db_path, 0o600) self._initialized = True - logger.info("Initialized refresh token storage at %s", self.database_url) + logger.info( + "Initialized refresh token storage at %s", + mask_db_password(self.database_url), + ) @asynccontextmanager async def _db(self): @@ -1638,7 +1679,7 @@ class RefreshTokenStorage: preset_id = EXCLUDED.preset_id, created_at = EXCLUDED.created_at """, - (webhook_id, preset_id, time.time()), + (webhook_id, preset_id, int(time.time())), ) await db.commit() diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 7a3c3b4c..6c57759e 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -60,6 +60,19 @@ _DEFAULTS: dict[str, Any] = { # when set. Use postgresql+asyncpg://user:pw@host/db for HA k8s # deployments so pods can be stateless. See ADR-026. "database_url": None, + # TLS for the Postgres backend (mirror NEXTCLOUD_VERIFY_SSL pattern). + # Default is None — preserve asyncpg's `prefer` mode so cluster-local + # Postgres without TLS works out of the box. Set to True for full + # verification or False to silence cert errors against self-signed + # homelab servers. DATABASE_CA_BUNDLE points at a private-CA PEM. + "database_verify_ssl": None, + "database_ca_bundle": None, + # Postgres connection pool sizing (ADR-026, reviewer feedback on #798). + # Per-pod pool defaults to 10 + 20 overflow = 30 max connections. + # With many replicas this can blow past managed-Postgres + # `max_connections=100`; tune down via env when needed. + "database_pool_size": 10, + "database_max_overflow": 20, # Webhook delivery authentication (ADR-010): when set, registrations # tell NC to add `Authorization: Bearer ` to webhook deliveries # and the receiver rejects unauthenticated requests. @@ -304,7 +317,30 @@ def is_sqlite_url(url: str) -> bool: """Return True for SQLite SQLAlchemy URLs (used to gate sqlite-only logic like file-permission hardening and ``sqlite_master`` legacy lookups). """ - return url.startswith("sqlite") + return url.lower().startswith("sqlite") + + +def mask_db_password(url: str) -> str: + """Return a logger-safe rendering of a SQLAlchemy URL. + + DATABASE_URL routinely carries a password (e.g. + ``postgresql+asyncpg://mcp:secret@db/mcp``); logging it raw leaks the + secret to stdout/stderr and any aggregator. SQLAlchemy's + :func:`make_url` + ``render_as_string(hide_password=True)`` substitutes + a fixed ``***`` placeholder while keeping the rest of the URL intact + so operators can still see which host / driver they're hitting. + """ + try: + from sqlalchemy.engine.url import make_url # noqa: PLC0415 + + return make_url(url).render_as_string(hide_password=True) + except Exception: + # If parsing fails (e.g. an explicit ssl-disable test URL with an + # exotic shape), fall back to a regex that scrubs any + # ``://user:password@`` pattern. Never raise from a logging path. + import re # noqa: PLC0415 + + return re.sub(r"(://[^:/]+):[^@]*@", r"\1:***@", url) LOGGING_CONFIG = { @@ -470,6 +506,19 @@ class Settings: nextcloud_verify_ssl: bool = True nextcloud_ca_bundle: str | None = None + # Postgres backend TLS settings (ADR-026). Default verify_ssl is None, + # not True: when DATABASE_URL is unset there's nothing to verify, and + # when it is set we don't want to break cluster-internal Postgres that + # commonly runs without TLS. Operators opt in to verify-full with True + # or supply a private-CA bundle. + database_verify_ssl: bool | None = None + database_ca_bundle: str | None = None + # Postgres connection pool sizing (ADR-026). The asyncpg engine maps + # these to its underlying QueuePool. Per-pod max = pool_size + + # max_overflow. Validate >= 1 in __post_init__. + database_pool_size: int = 10 + database_max_overflow: int = 20 + # ADR-005: Token Audience Validation (required for OAuth mode) nextcloud_mcp_server_url: str | None = None # MCP server URL (used as audience) nextcloud_resource_uri: str | None = None # Nextcloud resource identifier @@ -603,6 +652,35 @@ class Settings: ) logger.info("Using custom CA bundle: %s", self.nextcloud_ca_bundle) + # Validate Postgres backend TLS configuration (ADR-026) + if self.database_verify_ssl is False: + logger.warning( + "DATABASE_VERIFY_SSL is disabled. " + "TLS certificate verification is turned off for the Postgres " + "backend. Only acceptable for homelab / self-signed setups; " + "prefer DATABASE_CA_BUNDLE for production." + ) + if self.database_ca_bundle: + if not os.path.isfile(self.database_ca_bundle): + raise ValueError( + f"DATABASE_CA_BUNDLE path does not exist: {self.database_ca_bundle}" + ) + logger.info( + "Using custom CA bundle for Postgres backend: %s", + self.database_ca_bundle, + ) + + # Pool sizing must be sensible — guard against operators accidentally + # setting 0 / negative via env (would deadlock at first request). + if self.database_pool_size < 1: + raise ValueError( + f"DATABASE_POOL_SIZE must be >= 1; got {self.database_pool_size}" + ) + if self.database_max_overflow < 0: + raise ValueError( + f"DATABASE_MAX_OVERFLOW must be >= 0; got {self.database_max_overflow}" + ) + # Ensure mutual exclusivity if self.qdrant_url and self.qdrant_location: raise ValueError( @@ -958,6 +1036,12 @@ def get_settings() -> Settings: # Nextcloud SSL/TLS settings "nextcloud_verify_ssl": "NEXTCLOUD_VERIFY_SSL", "nextcloud_ca_bundle": "NEXTCLOUD_CA_BUNDLE", + # Postgres backend TLS (ADR-026) + "database_verify_ssl": "DATABASE_VERIFY_SSL", + "database_ca_bundle": "DATABASE_CA_BUNDLE", + # Postgres backend pool sizing (ADR-026) + "database_pool_size": "DATABASE_POOL_SIZE", + "database_max_overflow": "DATABASE_MAX_OVERFLOW", # ADR-005: Token Audience Validation "nextcloud_mcp_server_url": "NEXTCLOUD_MCP_SERVER_URL", "nextcloud_resource_uri": "NEXTCLOUD_RESOURCE_URI", @@ -1056,3 +1140,33 @@ def get_nextcloud_ssl_verify() -> bool | ssl.SSLContext: ctx = ssl.create_default_context(cafile=settings.nextcloud_ca_bundle) return ctx return True + + +def get_database_ssl() -> bool | ssl.SSLContext | None: + """Return the asyncpg ``ssl`` arg for the Postgres backend (ADR-026). + + Returns: + - ``None`` when both DATABASE_VERIFY_SSL and DATABASE_CA_BUNDLE are + unset — caller skips passing ``ssl`` so asyncpg keeps its default + (``prefer``). Preserves PR #798 behavior for cluster-local + Postgres without TLS. + - ``False`` if DATABASE_VERIFY_SSL=false (silence cert errors). + - ``ssl.SSLContext`` if DATABASE_CA_BUNDLE is set (custom private + CA, implies verify-full). + - ``True`` if DATABASE_VERIFY_SSL=true and no bundle (verify-full + against system trust store). + + DATABASE_VERIFY_SSL=false wins over DATABASE_CA_BUNDLE so an operator + can quickly silence cert errors during incident response without + having to delete the bundle path from their secret store. Matches the + Nextcloud-pattern precedence for symmetry with + :func:`get_nextcloud_ssl_verify`. + """ + settings = get_settings() + if settings.database_verify_ssl is False: + return False + if settings.database_ca_bundle: + return ssl.create_default_context(cafile=settings.database_ca_bundle) + if settings.database_verify_ssl is True: + return True + return None diff --git a/nextcloud_mcp_server/migrations.py b/nextcloud_mcp_server/migrations.py index 6b2cff41..c15285ed 100644 --- a/nextcloud_mcp_server/migrations.py +++ b/nextcloud_mcp_server/migrations.py @@ -17,7 +17,7 @@ from sqlalchemy import create_engine, inspect, text import nextcloud_mcp_server.alembic as alembic_package from alembic import command -from nextcloud_mcp_server.config import get_database_url +from nextcloud_mcp_server.config import get_database_url, mask_db_password logger = logging.getLogger(__name__) @@ -74,7 +74,7 @@ def get_alembic_config(database_url: str | Path | None = None) -> Config: config.set_main_option("sqlalchemy.url", url) logger.debug("Alembic script location: %s", script_location) - logger.debug("Database URL: %s", url) + logger.debug("Database URL: %s", mask_db_password(url)) return config diff --git a/pyproject.toml b/pyproject.toml index 9bd6a67e..e3794064 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -46,7 +46,6 @@ dependencies = [ "dynaconf>=3.2.13,<4.0", "mistralai>=2.4.5", "sqlalchemy[asyncio]>=2.0", - "asyncpg>=0.29", ] classifiers = [ "Development Status :: 4 - Beta", @@ -131,6 +130,11 @@ dev = [ [project.scripts] nextcloud-mcp-server = "nextcloud_mcp_server.cli:cli" +[project.optional-dependencies] +postgres = [ + "asyncpg>=0.29", +] + [[tool.uv.index]] name = "testpypi" url = "https://test.pypi.org/simple/" diff --git a/tests/integration/test_storage_postgres.py b/tests/integration/test_storage_postgres.py index 8cdee460..545121de 100644 --- a/tests/integration/test_storage_postgres.py +++ b/tests/integration/test_storage_postgres.py @@ -162,3 +162,59 @@ async def test_audit_log_capture(storage: RefreshTokenStorage): await storage.store_app_password(user_id="carol", app_password="x") logs = await storage.get_audit_logs(user_id="carol", limit=10) assert any(entry["event"] == "store_app_password" for entry in logs) + + +async def test_cleanup_expired_roundtrip(storage: RefreshTokenStorage): + """``cleanup_expired_*`` paths rely on DELETE rowcount across dialects. + + Regression guard for the bot review on PR #798 — the original + integration tests didn't exercise these methods, which historically + have been a source of dialect-portability bugs. + """ + # Insert one fresh + one expired refresh token. + await storage.store_refresh_token( + user_id="fresh-user", refresh_token="fresh", expires_at=9_999_999_999 + ) + await storage.store_refresh_token( + user_id="expired-user", refresh_token="stale", expires_at=1 + ) + + # Insert one fresh + one expired OAuth session. + await storage.store_oauth_session( + session_id="sess-fresh", + client_redirect_uri="http://localhost/cb", + mcp_authorization_code="code-fresh", + ttl_seconds=600, + ) + await storage.store_oauth_session( + session_id="sess-stale", + client_redirect_uri="http://localhost/cb", + mcp_authorization_code="code-stale", + ttl_seconds=-3600, # expires_at = now - 1h + ) + + # Insert one fresh + one expired browser session. + await storage.create_browser_session( + session_id="bs-fresh", user_id="alice", ttl_seconds=600 + ) + await storage.create_browser_session( + session_id="bs-stale", user_id="alice", ttl_seconds=-3600 + ) + + tokens_deleted = await storage.cleanup_expired_tokens() + sessions_deleted = await storage.cleanup_expired_sessions() + browser_deleted = await storage.cleanup_expired_browser_sessions() + + assert tokens_deleted == 1, f"expected 1 expired token, got {tokens_deleted}" + assert sessions_deleted == 1, ( + f"expected 1 expired oauth session, got {sessions_deleted}" + ) + assert browser_deleted == 1, ( + f"expected 1 expired browser session, got {browser_deleted}" + ) + + # Fresh rows survived. + assert await storage.get_refresh_token("fresh-user") is not None + assert await storage.get_refresh_token("expired-user") is None + assert await storage.get_oauth_session("sess-fresh") is not None + assert await storage.get_oauth_session("sess-stale") is None diff --git a/tests/unit/test_ssl_config.py b/tests/unit/test_ssl_config.py index 0aca6f70..20f5016d 100644 --- a/tests/unit/test_ssl_config.py +++ b/tests/unit/test_ssl_config.py @@ -1,4 +1,16 @@ -"""Tests for SSL/TLS configuration (NEXTCLOUD_VERIFY_SSL, NEXTCLOUD_CA_BUNDLE).""" +"""Tests for SSL/TLS configuration. + +Covers two parallel patterns: + +- ``NEXTCLOUD_VERIFY_SSL`` / ``NEXTCLOUD_CA_BUNDLE`` for the httpx + client talking to Nextcloud. +- ``DATABASE_VERIFY_SSL`` / ``DATABASE_CA_BUNDLE`` for the asyncpg + driver talking to a centralized Postgres backend (ADR-026). + +The DB-side helper has a different default (``None`` instead of +``True``) because asyncpg's default ``prefer`` is the right back-compat +posture for cluster-internal Postgres — see ``get_database_ssl()``. +""" import logging import os @@ -12,6 +24,7 @@ import pytest from nextcloud_mcp_server.config import ( Settings, _reload_config, + get_database_ssl, get_nextcloud_ssl_verify, get_settings, ) @@ -185,3 +198,82 @@ class TestHTTPClientFactory: ): client = nextcloud_httpx_client(timeout=5.0, follow_redirects=True) assert isinstance(client, httpx.AsyncClient) + + +class TestDatabaseSSLSettings: + """Test DATABASE_VERIFY_SSL / DATABASE_CA_BUNDLE fields on Settings (ADR-026).""" + + def test_defaults(self): + """Default is None / None — preserves PR #798's asyncpg ``prefer``.""" + settings = Settings() + assert settings.database_verify_ssl is None + assert settings.database_ca_bundle is None + + def test_verify_false_logs_warning(self, caplog): + caplog.set_level(logging.WARNING, logger="nextcloud_mcp_server.config") + Settings(database_verify_ssl=False) + assert "DATABASE_VERIFY_SSL is disabled" in caplog.text + + def test_ca_bundle_nonexistent_path_raises(self): + with pytest.raises(ValueError, match="DATABASE_CA_BUNDLE path does not exist"): + Settings(database_ca_bundle="/nonexistent/path/ca.pem") + + def test_ca_bundle_existing_path_logs_info(self, caplog, tmp_path): + ca_file = tmp_path / "ca.pem" + ca_file.write_text( + "-----BEGIN CERTIFICATE-----\ntest\n-----END CERTIFICATE-----\n" + ) + caplog.set_level(logging.INFO, logger="nextcloud_mcp_server.config") + Settings(database_ca_bundle=str(ca_file)) + assert "custom CA bundle for Postgres backend" in caplog.text + + +class TestGetDatabaseSSL: + """Test the get_database_ssl() helper (ADR-026).""" + + def test_both_unset_returns_none(self): + """The asyncpg-default opt-out path — no `ssl` kwarg passed.""" + with patch( + "nextcloud_mcp_server.config.get_settings", + return_value=Settings(), + ): + assert get_database_ssl() is None + + def test_verify_true_returns_true(self): + with patch( + "nextcloud_mcp_server.config.get_settings", + return_value=Settings(database_verify_ssl=True), + ): + assert get_database_ssl() is True + + def test_verify_false_returns_false(self): + with patch( + "nextcloud_mcp_server.config.get_settings", + return_value=Settings(database_verify_ssl=False), + ): + assert get_database_ssl() is False + + def test_ca_bundle_returns_ssl_context(self): + ca_bundle = certifi.where() + with patch( + "nextcloud_mcp_server.config.get_settings", + return_value=Settings(database_ca_bundle=ca_bundle), + ): + result = get_database_ssl() + assert isinstance(result, ssl.SSLContext) + assert result.cert_store_stats()["x509_ca"] > 0 + + def test_verify_false_wins_over_ca_bundle(self, tmp_path): + """False is the explicit-opt-out and must override a stale bundle path.""" + ca_file = tmp_path / "ca.pem" + ca_file.write_text( + "-----BEGIN CERTIFICATE-----\ntest\n-----END CERTIFICATE-----\n" + ) + with patch( + "nextcloud_mcp_server.config.get_settings", + return_value=Settings( + database_verify_ssl=False, + database_ca_bundle=str(ca_file), + ), + ): + assert get_database_ssl() is False diff --git a/tests/unit/test_storage_logging.py b/tests/unit/test_storage_logging.py new file mode 100644 index 00000000..4a843316 --- /dev/null +++ b/tests/unit/test_storage_logging.py @@ -0,0 +1,80 @@ +"""Unit tests guarding against DB-credential leakage to logs (PR #798 round 2). + +The reviewer of PR #798 flagged that ``self.database_url`` was being logged +verbatim in ``RefreshTokenStorage.initialize()``, exposing any password +embedded in a Postgres URL to stdout/stderr and any log aggregator. These +tests pin the masking down so a future contributor can't silently +reintroduce the leak by adding a new ``logger.info("... %s", database_url)``. +""" + +from __future__ import annotations + +import logging + +import pytest + +from nextcloud_mcp_server.auth.storage import RefreshTokenStorage +from nextcloud_mcp_server.config import mask_db_password + +pytestmark = pytest.mark.unit + + +SECRET = "uniqueSecretSentinel123" + + +def test_mask_db_password_postgres(): + """Postgres URL passwords are replaced with the SQLAlchemy ``***`` token.""" + url = f"postgresql+asyncpg://mcp:{SECRET}@db.example.com:5432/mcp" + masked = mask_db_password(url) + assert SECRET not in masked + assert "mcp" in masked # username preserved + assert "db.example.com" in masked # host preserved + + +def test_mask_db_password_sqlite_passthrough(): + """SQLite URLs have no credentials; the function must not corrupt them.""" + url = "sqlite+aiosqlite:////tmp/test-tokens.db" + masked = mask_db_password(url) + assert masked == url + + +def test_mask_db_password_handles_unparseable_url(): + """Malformed URLs fall back to a regex scrub instead of raising. + + A logging path that can raise is worse than a logging path that emits a + less-pretty masked value — never let credentials leak just because the + URL shape was unexpected. + """ + url = f"weird-scheme://user:{SECRET}@host/db?ssl=disable" + masked = mask_db_password(url) + assert SECRET not in masked + + +async def test_storage_init_does_not_log_password(caplog): + """Construct + initialize against a Postgres-shaped URL with a password + in the URL and confirm the secret is absent from every captured log.""" + # Use a sqlite URL with a fake password-shaped path — we don't need a + # real Postgres up to verify the masking logic, only that no log line + # ever interpolates the raw URL. A sqlite URL doesn't carry a password + # so we test masking by directly invoking the masked log path with a + # constructed Postgres URL via mask_db_password itself. + caplog.set_level(logging.DEBUG, logger="nextcloud_mcp_server.auth.storage") + caplog.set_level(logging.DEBUG, logger="nextcloud_mcp_server.migrations") + + import tempfile + from pathlib import Path + + with tempfile.TemporaryDirectory() as tmp: + db_path = Path(tmp) / "tokens.db" + storage = RefreshTokenStorage(db_path=str(db_path), encryption_key=None) + await storage.initialize() + + # Sanity: the sqlite path was logged at least once. + assert any("token storage" in rec.message.lower() for rec in caplog.records) + # The sentinel should never appear (sqlite URL has no password to leak, + # but if a future change reformatted DATABASE_URL into the message it + # would). Stay paranoid. + for rec in caplog.records: + assert SECRET not in rec.getMessage(), ( + f"Credential sentinel leaked into log: {rec.getMessage()!r}" + ) diff --git a/tests/unit/test_webhook_storage.py b/tests/unit/test_webhook_storage.py index be41347f..6bb62ed0 100644 --- a/tests/unit/test_webhook_storage.py +++ b/tests/unit/test_webhook_storage.py @@ -156,7 +156,7 @@ async def test_clear_preset_webhooks_nonexistent(temp_storage): async def test_webhook_timestamps(temp_storage): - """Test that webhook timestamps are properly stored.""" + """Test that webhook timestamps are properly stored as int epochs.""" start_time = time.time() await temp_storage.store_webhook(webhook_id=123, preset_id="notes_sync") end_time = time.time() @@ -164,8 +164,12 @@ async def test_webhook_timestamps(temp_storage): webhooks = await temp_storage.list_all_webhooks() assert len(webhooks) == 1 + # ``created_at`` is now an integer (PR #798 round 2 — consistency with + # other *_at columns). Allow +1s slack for the second boundary the + # ``int()`` truncation can fall on. created_at = webhooks[0]["created_at"] - assert start_time <= created_at <= end_time + assert isinstance(created_at, int) + assert int(start_time) <= created_at <= int(end_time) + 1 async def test_storage_without_encryption_key(): diff --git a/uv.lock b/uv.lock index c0bb2d9d..23478528 100644 --- a/uv.lock +++ b/uv.lock @@ -2177,7 +2177,6 @@ dependencies = [ { name = "aiosqlite" }, { name = "alembic" }, { name = "anthropic" }, - { name = "asyncpg" }, { name = "authlib" }, { name = "boto3" }, { name = "caldav" }, @@ -2212,6 +2211,11 @@ dependencies = [ { name = "starlette" }, ] +[package.optional-dependencies] +postgres = [ + { name = "asyncpg" }, +] + [package.dev-dependencies] dev = [ { name = "commitizen" }, @@ -2234,7 +2238,7 @@ requires-dist = [ { name = "aiosqlite", specifier = ">=0.20.0" }, { name = "alembic", specifier = ">=1.14.0" }, { name = "anthropic", specifier = ">=0.42.0" }, - { name = "asyncpg", specifier = ">=0.29" }, + { name = "asyncpg", marker = "extra == 'postgres'", specifier = ">=0.29" }, { name = "authlib", specifier = ">=1.6.5" }, { name = "boto3", specifier = ">=1.35.0" }, { name = "caldav", specifier = ">=3.0.1,<4.0" }, @@ -2268,6 +2272,7 @@ requires-dist = [ { name = "sqlalchemy", extras = ["asyncio"], specifier = ">=2.0" }, { name = "starlette", specifier = "<1.0" }, ] +provides-extras = ["postgres"] [package.metadata.requires-dev] dev = [