From 51419329b065b19f6ccddb56899d334ad77c2bab Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Sat, 16 May 2026 19:33:23 +0200 Subject: [PATCH] fix(storage): address PR #798 round-3 review (SonarQube + pool sizing + RETURNING test) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-3 fixes. Two threads: - 9 OPEN SonarQube issues caused the "E Security Rating on New Code" gate failure. The bot's diagnosis (sa.text(text_sql) → SQL injection) was a wrong guess; the actual SQ rules firing were different. - Bot's substantive concerns: pool defaults too aggressive, delete_browser_session RETURNING path untested on Postgres, schema_version legacy table created on Postgres, stale module docstring. - User's underlying question on the pool: "isn't 1 connection enough?" Right-sized to 2+5 and documented the concurrency model in ADR-026 so the rationale is durable. SonarQube quality-gate fixes (clears all 9 OPEN issues) ------------------------------------------------------- - BLOCKER S6418: rename `SECRET` constant in test_storage_logging.py to `SENTINEL_PASSWORD_FRAGMENT` + NOSONAR with rationale. - CRITICAL S3776: extract `_build_postgres_engine()` from `initialize()` (was complexity 26 > 15); incidentally creates a clean unit-test seam for engine args. - CRITICAL S4423: `ssl.create_default_context(cafile=...)` is flagged as "weak protocol" — Python 3.10+ already negotiates the strongest available protocol. Explicitly pass `purpose=ssl.Purpose.SERVER_AUTH` and NOSONAR with the Python-version rationale. - MAJOR S3358: split the TLS-mode nested ternary in the engine factory into a `_describe_ssl_arg()` helper. - MAJOR S2068 ×3: bind test app-password literals to local vars and put `# NOSONAR S2068` on the same line as the literal (anchoring requirement) instead of on the closing paren. - MINOR S7503 ×2: `# NOSONAR S7503` on `_Cursor.__aenter__/__aexit__` — they MUST be `async` per the context-manager protocol. Pool sizing right-sized (answers "why so many connections?") ------------------------------------------------------------ - `DATABASE_POOL_SIZE` default 10 → **2**. - `DATABASE_MAX_OVERFLOW` default 20 → **5**. - Per-pod max drops from 30 to 7. With 3 replicas, total = 21 connections (was 90) — well under managed-Postgres `max_connections=100`. - New INFO log at startup: `Postgres engine ready: pool_size=N max_overflow=M (per-pod max K connections)`. Surfaces the active sizing without grepping config. - New ADR-026 § "Concurrency model and pool sizing" explains asyncpg's single-flight connection semantics, the MCP workload shape (read-mostly point lookups), why-not-1 (multi-user serialization), and the tune-up/tune-down recipe. - `docs/configuration.md` table updated with new defaults + homelab-vs-prod tuning guidance, linking the ADR. RETURNING path covered on Postgres ---------------------------------- - New `test_browser_session_delete_returning` exercises the `DELETE … RETURNING user_id` path — the only RETURNING clause in the storage layer and the most dialect-sensitive SQL in this PR. Asserts both present-row (returns True, row gone) and absent-row (returns False) branches. Schema portability polish ------------------------- - `alembic 001`: gate `schema_version` table creation on `op.get_bind().dialect.name == "sqlite"`. The table exists purely to match the fingerprint of pre-Alembic SQLite databases; fresh Postgres installs no longer carry the dead legacy table. Misc polish ----------- - Module docstring: "SQLite-based" → "SQL-backed", with a sentence on the DATABASE_URL opt-in and an ADR-026 link. - Comment on `_wrap_row` noting `row._mapping` is the documented RowMapping accessor in SQLAlchemy 2.x despite the underscore. Skipped (rationale in PR reply) ------------------------------- - `_qmark_to_named` SQL-comment handling: docstring already notes the limitation; no `?` in storage SQL comments today. - Module-level `anyio.Lock()`: established precedent confirmed by the bot itself. - `get_audit_logs` `SELECT *`: pre-existing pattern, out of scope. Verification ------------ - `uv run pytest tests/unit/` — 1025 passed. - `TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres` — 7 passed. - `ruff check && ruff format --check && ty check` — clean. - Confirmed `schema_version` absent on fresh Postgres, still present on fresh SQLite. 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) --- docs/ADR-026-pluggable-database-backend.md | 39 +++++ docs/configuration.md | 11 +- .../20251217_2200_001_initial_schema.py | 24 +-- nextcloud_mcp_server/auth/storage.py | 137 ++++++++++++------ nextcloud_mcp_server/config.py | 33 +++-- tests/integration/test_storage_postgres.py | 46 +++++- tests/unit/test_storage_logging.py | 17 ++- 7 files changed, 229 insertions(+), 78 deletions(-) diff --git a/docs/ADR-026-pluggable-database-backend.md b/docs/ADR-026-pluggable-database-backend.md index 20730299..3f21dbf6 100644 --- a/docs/ADR-026-pluggable-database-backend.md +++ b/docs/ADR-026-pluggable-database-backend.md @@ -97,6 +97,45 @@ the runtime invokes it from `RefreshTokenStorage.initialize()` via The pattern is non-obvious; this note exists so a future maintainer doesn't try to "simplify" it back into the main loop. +### Concurrency model and pool sizing + +The reviewer's natural reaction to seeing `DATABASE_POOL_SIZE=10` (the +round-2 default) was: *isn't 1 connection enough for an MCP server?* +This subsection records why a small pool is right, why 1 is not the +target default, and what the workload actually looks like. + +**asyncpg connection semantics.** Each asyncpg connection is +**single-flight** — only one query can be in flight at a time on a +given connection. SQLAlchemy serializes additional requests in the +pool queue. So the question is never "how many requests does the MCP +server handle" but "how many concurrent storage operations are in +flight at the peak". + +**MCP storage workload shape.** Each MCP tool call typically performs +1–3 storage operations: a token lookup (`get_refresh_token` or +`get_app_password`), maybe an audit-log write, occasionally a session +update. Lookups are sub-millisecond point queries; writes are short. +The hot path is read-mostly. No long-running transactions, no batch +loads. + +**Why not 1?** A single-user (homelab) deployment genuinely works on +`pool_size=1, max_overflow=2`. But the default ships for multi-user +OAuth deployments where ≥2 concurrent client requests are normal; on +`pool_size=1` those serialize on a single connection and you measure +a latency cliff. The defaults `pool_size=2, max_overflow=5` (max 7 +per pod) cover typical multi-user MCP burst with two-replica +headroom. With 3 k8s replicas the total is 21 connections — well +under managed-Postgres `max_connections=100` (RDS, CNPG default). + +**How to tune.** `DATABASE_POOL_SIZE` / `DATABASE_MAX_OVERFLOW` env +vars adjust the per-pod pool live (server restart). The startup +``Postgres engine ready: pool_size=N max_overflow=M (per-pod max K +connections)`` log line surfaces the active sizing so operators can +see the per-replica footprint at a glance. For a fleet of N replicas, +estimate worst-case Postgres connection count as +`N × (pool_size + max_overflow)` and stay comfortably below the +server's `max_connections`. + ### TLS for the Postgres backend Two settings mirror the existing `NEXTCLOUD_VERIFY_SSL` / diff --git a/docs/configuration.md b/docs/configuration.md index 0ed7a067..bb037d84 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -138,8 +138,15 @@ TOKEN_ENCRYPTION_KEY= | `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. | +| `DATABASE_POOL_SIZE` | Optional (default `2`) | Per-pod SQLAlchemy connection pool size for the Postgres backend. asyncpg connections are single-flight, so this only needs to cover concurrent storage ops (not concurrent tool calls). See [ADR-026 § Concurrency model and pool sizing](ADR-026-pluggable-database-backend.md). | +| `DATABASE_MAX_OVERFLOW` | Optional (default `5`) | Per-pod burst connections beyond `DATABASE_POOL_SIZE`. Max per-pod = `pool_size + max_overflow` (default 7). Set to `0` for a hard cap. With 3 replicas the default totals 21 connections — well under managed-Postgres `max_connections=100`. | + +Operators with very high concurrency (many MCP clients per pod, or +expensive Nextcloud round-trips holding storage locks) should tune these +up; single-user / homelab deployments can drop to `DATABASE_POOL_SIZE=1 +DATABASE_MAX_OVERFLOW=2` for the smallest possible footprint. The +server logs the configured sizes at startup so over-allocation is +visible without grepping config. Homelab example (self-signed Postgres with a private CA): 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 021f4b07..849e81da 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 @@ -111,14 +111,18 @@ def upgrade() -> None: ["mcp_authorization_code"], ) - # Legacy schema-version table; superseded by alembic_version. Retained - # so pre-Alembic databases that get stamped into the migration chain - # still match the schema fingerprint they had on disk. - op.create_table( - "schema_version", - sa.Column("version", sa.Integer, primary_key=True, autoincrement=False), - sa.Column("applied_at", sa.Float, nullable=False), - ) + # Legacy schema-version table; superseded by alembic_version. Only + # created on SQLite because it exists *purely* to match the + # fingerprint of pre-Alembic SQLite databases that get stamped into + # the migration chain (see ``RefreshTokenStorage.initialize()``). + # Fresh Postgres installs have no pre-Alembic history and don't + # need it. PR #798 round-3 review (#4). + if op.get_bind().dialect.name == "sqlite": + op.create_table( + "schema_version", + sa.Column("version", sa.Integer, primary_key=True, autoincrement=False), + sa.Column("applied_at", sa.Float, nullable=False), + ) op.create_table( "registered_webhooks", @@ -144,7 +148,9 @@ def downgrade() -> None: op.drop_index("idx_webhooks_created", table_name="registered_webhooks") op.drop_index("idx_webhooks_preset", table_name="registered_webhooks") op.drop_table("registered_webhooks") - op.drop_table("schema_version") + # ``schema_version`` is only created on SQLite (see ``upgrade()``). + if op.get_bind().dialect.name == "sqlite": + op.drop_table("schema_version") op.drop_index("idx_oauth_sessions_mcp_code", table_name="oauth_sessions") op.drop_table("oauth_sessions") op.drop_table("oauth_clients") diff --git a/nextcloud_mcp_server/auth/storage.py b/nextcloud_mcp_server/auth/storage.py index 6ff372d5..5d18f445 100644 --- a/nextcloud_mcp_server/auth/storage.py +++ b/nextcloud_mcp_server/auth/storage.py @@ -1,8 +1,14 @@ """ Persistent Storage for MCP Server State -This module provides SQLite-based storage for multiple concerns across both -BasicAuth and OAuth authentication modes: +This module provides SQL-backed storage for multiple concerns across both +BasicAuth and OAuth authentication modes. The default backend is SQLite +(file-based or per-process tempfile); set ``DATABASE_URL`` to a +``postgresql+asyncpg://...`` URL for HA k8s deployments where pods need +to be stateless. See :doc:`ADR-026 ` +for the design. + +Concerns covered: 1. **Refresh Tokens** (OAuth mode only, for background jobs) - Securely stores encrypted refresh tokens for offline access @@ -145,9 +151,26 @@ class _Row: def _wrap_row(row) -> _Row | None: if row is None: return None + # ``row._mapping`` is the documented public RowMapping accessor in + # SQLAlchemy 2.x (the leading underscore is historical); it returns + # a column-name → value mapping that survives the row being + # tuple-iterated. See SQLAlchemy 2.x ``Row.mapping`` docs. return _Row(tuple(row), dict(row._mapping)) +def _describe_ssl_arg(ssl_arg: object) -> str: + """Render the ``ssl`` value for the startup log line. + + Split out of the engine factory to avoid a nested-ternary + SonarQube finding (``S3358``) and to make the cases readable. + """ + if ssl_arg is False: + return "disabled" + if isinstance(ssl_arg, bool): + return "verify-full (system CAs)" + return "custom CA bundle" + + def _wrap_rows(rows) -> list[_Row]: """Wrap a list of SQLAlchemy rows; iterator never yields ``None``.""" return [_Row(tuple(r), dict(r._mapping)) for r in rows] @@ -182,10 +205,14 @@ class _Cursor: async def fetchall(self) -> list[_Row]: return _wrap_rows(self._result.fetchall()) - async def __aenter__(self) -> "_Cursor": + # NOSONAR S7503 on the next two methods — Python's async-context-manager + # protocol *requires* ``__aenter__`` / ``__aexit__`` to be coroutines + # even when the body has nothing to await; dropping ``async`` would + # break ``async with _Cursor(...)``. + async def __aenter__(self) -> "_Cursor": # NOSONAR S7503 return self - async def __aexit__(self, *exc: object) -> None: + async def __aexit__(self, *exc: object) -> None: # NOSONAR S7503 # SQLAlchemy Result closes when the connection closes; no-op here. return None @@ -419,8 +446,8 @@ class RefreshTokenStorage: # Create the shared async engine for the chosen backend. SQLite uses # NullPool (per-call connections, matches the prior aiosqlite-direct - # behavior); Postgres uses the default pool with pre-ping so dropped - # connections from idle k8s networks are retried transparently. + # behavior); Postgres uses a small bounded pool — see + # ``_build_postgres_engine`` for sizing rationale. if is_sqlite: self.engine = create_async_engine( self.database_url, @@ -429,45 +456,7 @@ 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=settings.database_pool_size, - max_overflow=settings.database_max_overflow, - pool_pre_ping=True, - connect_args=connect_args, - future=True, - ) + self.engine = self._build_postgres_engine() self._dialect = self.engine.dialect.name # Check database state with the SQLAlchemy inspector so the legacy @@ -511,6 +500,64 @@ class RefreshTokenStorage: mask_db_password(self.database_url), ) + def _build_postgres_engine(self) -> AsyncEngine: + """Construct the AsyncEngine for a Postgres ``DATABASE_URL``. + + Split out from :meth:`initialize` so cognitive complexity stays + under the SonarQube ``S3776`` threshold and so a future + engine-arg unit test has a single seam to mock. + + Defaults to ``pool_size=2, max_overflow=5`` (max 7 connections + per pod) — see ADR-026 § "Concurrency model and pool sizing" + for the rationale. asyncpg connections are single-flight, so + the pool only needs to cover the typical multi-user MCP burst, + not every potential in-flight tool call. + """ + # asyncpg ships as an optional PyPI extra (`[postgres]`) so the + # default `pip install nextcloud-mcp-server` audience doesn't + # pull in the 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. + 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", _describe_ssl_arg(ssl_arg)) + + settings = get_settings() + engine = create_async_engine( + self.database_url, + pool_size=settings.database_pool_size, + max_overflow=settings.database_max_overflow, + pool_pre_ping=True, + connect_args=connect_args, + future=True, + ) + # Log the configured sizing so operators can spot + # over-allocation at startup without grepping config. + logger.info( + "Postgres engine ready: pool_size=%d max_overflow=%d " + "(per-pod max %d connections)", + settings.database_pool_size, + settings.database_max_overflow, + settings.database_pool_size + settings.database_max_overflow, + ) + return engine + @asynccontextmanager async def _db(self): """Open a backend-agnostic connection. diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 6c57759e..396e4ca1 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -67,12 +67,14 @@ _DEFAULTS: dict[str, Any] = { # 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, + # Postgres connection pool sizing (ADR-026 → "Concurrency model and + # pool sizing"). Per-pod defaults to 2 + 5 overflow = 7 max + # connections. asyncpg connections are single-flight, so the pool + # only needs to cover typical multi-user MCP burst — not every + # potential in-flight tool call. Tune up with DATABASE_POOL_SIZE / + # DATABASE_MAX_OVERFLOW for high-traffic prod fleets. + "database_pool_size": 2, + "database_max_overflow": 5, # Webhook delivery authentication (ADR-010): when set, registrations # tell NC to add `Authorization: Bearer ` to webhook deliveries # and the receiver rejects unauthenticated requests. @@ -515,9 +517,12 @@ class Settings: 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 + # max_overflow. Defaults are intentionally small (2 + 5 = 7) because + # asyncpg connections are single-flight and the typical MCP workload + # is light read-mostly point lookups. Validate >= 1 / >= 0 in + # __post_init__. + database_pool_size: int = 2 + database_max_overflow: int = 5 # ADR-005: Token Audience Validation (required for OAuth mode) nextcloud_mcp_server_url: str | None = None # MCP server URL (used as audience) @@ -1166,7 +1171,15 @@ def get_database_ssl() -> bool | ssl.SSLContext | None: if settings.database_verify_ssl is False: return False if settings.database_ca_bundle: - return ssl.create_default_context(cafile=settings.database_ca_bundle) + # ``ssl.create_default_context()`` on Python 3.10+ already negotiates + # the strongest available protocol (TLS 1.2+ with secure ciphers); + # we pin Python 3.11+ in pyproject.toml. ``purpose=SERVER_AUTH`` is + # the default but spelt out here so static analysers (SonarQube + # ``S4423``) can see it explicitly. NOSONAR S4423 + return ssl.create_default_context( # NOSONAR S4423 + purpose=ssl.Purpose.SERVER_AUTH, + cafile=settings.database_ca_bundle, + ) if settings.database_verify_ssl is True: return True return None diff --git a/tests/integration/test_storage_postgres.py b/tests/integration/test_storage_postgres.py index 545121de..00a428fd 100644 --- a/tests/integration/test_storage_postgres.py +++ b/tests/integration/test_storage_postgres.py @@ -111,13 +111,23 @@ async def test_refresh_token_roundtrip(storage: RefreshTokenStorage): async def test_app_password_roundtrip(storage: RefreshTokenStorage): - """Store + retrieve + replace + delete a scoped app password.""" - await storage.store_app_password(user_id="bob", app_password="pw-1") - assert await storage.get_app_password("bob") == "pw-1" + """Store + retrieve + replace + delete a scoped app password. + + The ``app_password=`` keyword-arg literals below trigger SonarQube's + hard-coded-credential heuristic (``S2068``) even though these are + obvious test fixtures with no production reach. The literals are + bound to local variables so the NOSONAR marker can anchor to the + same line as the literal — SQ doesn't pick up the marker if it + sits on a different physical line. + """ + bob_pw_v1 = "pw-1" # NOSONAR S2068 — localhost test fixture, never deployed + await storage.store_app_password(user_id="bob", app_password=bob_pw_v1) + assert await storage.get_app_password("bob") == bob_pw_v1 # Replace path exercises the ON CONFLICT DO UPDATE on the singleton row. - await storage.store_app_password(user_id="bob", app_password="pw-2") - assert await storage.get_app_password("bob") == "pw-2" + bob_pw_v2 = "pw-2" # NOSONAR S2068 — localhost test fixture, never deployed + await storage.store_app_password(user_id="bob", app_password=bob_pw_v2) + assert await storage.get_app_password("bob") == bob_pw_v2 assert await storage.delete_app_password("bob") is True assert await storage.get_app_password("bob") is None @@ -159,7 +169,8 @@ async def test_webhook_tracking(storage: RefreshTokenStorage): async def test_audit_log_capture(storage: RefreshTokenStorage): """Audit events from upstream methods land in audit_logs.""" - await storage.store_app_password(user_id="carol", app_password="x") + carol_pw = "x" # NOSONAR S2068 — localhost test fixture, never deployed + await storage.store_app_password(user_id="carol", app_password=carol_pw) logs = await storage.get_audit_logs(user_id="carol", limit=10) assert any(entry["event"] == "store_app_password" for entry in logs) @@ -218,3 +229,26 @@ async def test_cleanup_expired_roundtrip(storage: RefreshTokenStorage): 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 + + +async def test_browser_session_delete_returning(storage: RefreshTokenStorage): + """Exercise the ``DELETE … RETURNING user_id`` path on Postgres. + + ``delete_browser_session`` is the only RETURNING clause in the + storage layer and the most dialect-sensitive SQL in this PR — it + needed SQLite ≥ 3.35 specifically because of RETURNING. Bot review + on PR #798 round 2 flagged that the existing cleanup test didn't + actually exercise this path. Asserts both the present and absent + cases so the asyncpg result-handling for RETURNING is covered. + """ + await storage.create_browser_session( + session_id="bs-returning", user_id="alice", ttl_seconds=600 + ) + assert await storage.get_browser_session_user("bs-returning") == "alice" + + assert await storage.delete_browser_session("bs-returning") is True + assert await storage.get_browser_session_user("bs-returning") is None + + # Deleting a nonexistent session returns False (RETURNING yields no + # row → rowcount path). + assert await storage.delete_browser_session("never-existed") is False diff --git a/tests/unit/test_storage_logging.py b/tests/unit/test_storage_logging.py index 4a843316..270369c5 100644 --- a/tests/unit/test_storage_logging.py +++ b/tests/unit/test_storage_logging.py @@ -19,14 +19,19 @@ from nextcloud_mcp_server.config import mask_db_password pytestmark = pytest.mark.unit -SECRET = "uniqueSecretSentinel123" +# Synthetic leak-detection sentinel — embedded into test-only URLs so we +# can grep ``caplog`` and prove the masking path never emits the literal +# password substring. Not a real credential. NOSONAR S6418 +SENTINEL_PASSWORD_FRAGMENT = "uniqueSecretSentinel123" # NOSONAR S6418 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" + url = ( + f"postgresql+asyncpg://mcp:{SENTINEL_PASSWORD_FRAGMENT}@db.example.com:5432/mcp" + ) masked = mask_db_password(url) - assert SECRET not in masked + assert SENTINEL_PASSWORD_FRAGMENT not in masked assert "mcp" in masked # username preserved assert "db.example.com" in masked # host preserved @@ -45,9 +50,9 @@ def test_mask_db_password_handles_unparseable_url(): 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" + url = f"weird-scheme://user:{SENTINEL_PASSWORD_FRAGMENT}@host/db?ssl=disable" masked = mask_db_password(url) - assert SECRET not in masked + assert SENTINEL_PASSWORD_FRAGMENT not in masked async def test_storage_init_does_not_log_password(caplog): @@ -75,6 +80,6 @@ async def test_storage_init_does_not_log_password(caplog): # 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(), ( + assert SENTINEL_PASSWORD_FRAGMENT not in rec.getMessage(), ( f"Credential sentinel leaked into log: {rec.getMessage()!r}" )