2f8875e736e340c3c8cc31ac3d0ce3f89458bc5d
3
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Chris CoutinhoandClaude Opus 4.7
|
d717c64750 |
fix(storage): address PR #798 round-4 review (NOSONAR syntax + pg_advisory_lock + engine dispose + nits)
Addresses all 8 items in the round-4 bot review plus 4 remaining SonarQube OPEN issues that were silently broken by round 3's malformed NOSONAR markers. NOSONAR syntax fix (clears the remaining 4 OPEN SQ issues) ---------------------------------------------------------- Round 3 used ``# NOSONAR S<rule_key>`` form. SonarQube Python doesn't recognize the rule-key suffix — it treats the whole thing as a malformed suppression directive (S7632) AND lets the underlying rule keep firing (S7503 on ``_Cursor.__aenter__/__aexit__``). Switch every marker to bare ``# NOSONAR``, with the rationale moved into a preceding comment block. Affected sites: - storage.py: ``_Cursor.__aenter__``, ``_Cursor.__aexit__`` - config.py: ``get_database_ssl()`` ``return False`` + ``ssl.create_default_context()`` - test_storage_logging.py: ``SENTINEL_PASSWORD_FRAGMENT`` constant - test_storage_postgres.py: three ``bob_pw_v1`` / ``bob_pw_v2`` / ``carol_pw`` literals Bot 🔴#1 — defensive NOSONAR on get_database_ssl `return False` -------------------------------------------------------------- Bot predicted S4830 fires on the operator-opt-out path. SQ output shows it doesn't currently fire, but bare NOSONAR added defensively with rationale comment. Bot 🔴#2 — defensive NOSONAR on f-string SQL -------------------------------------------- ``update_oauth_session`` builds its SET clause via ``f"{', '.join(update_fields)}"``; ``get_audit_logs`` builds its WHERE clause via string concatenation. Both are safe (the fragments only come from this function's own branches, no user input), but the patterns trip taint analysers. Annotated both with bare NOSONAR + safety comment explaining the hardcoded-fragments invariant. Note: S2077 doesn't currently fire on these; defensive. Bot 🟡#3 — pg_advisory_lock for concurrent migrations ----------------------------------------------------- Without coordination, two pods rolling-updating simultaneously can both observe ``has_alembic=False`` and both try to apply migrations from scratch — the second crashes with "relation already exists". New ``_migration_lock()`` async context manager: - On Postgres: ``SELECT pg_advisory_lock(:lock_id)`` on a fresh connection (separate from the engine pool so it survives the ``to_thread.run_sync`` worker), held across BOTH the schema-inspect AND the migration call. Without that span, two pods could each observe "no alembic_version" before either started migrating, defeating the lock. - On SQLite: yields immediately (file-level locking serializes writes natively). Lock ID derived from ``sha256(b"nextcloud-mcp-server:migrations")[:8]`` as a stable signed int64 so we can't collide with other apps sharing the same Postgres. Bot 🟡#4 — RefreshTokenStorage.close() + lifespan wiring -------------------------------------------------------- New idempotent ``close()`` method calls ``await engine.dispose()``, nulls the engine, resets ``_initialized``. Wired into both ``app_lifespan_basic`` (BasicAuth) and the OAuth lifespan teardown, each wrapped in ``try/except Exception`` with ``logger.warning`` so a buggy dispose can't block SIGTERM. Without this, pooled asyncpg connections leak server-side slots until ``idle_in_transaction_session_timeout`` reaps them — with small pool defaults and frequent k8s rolling restarts this can starve ``max_connections``. Bot 🟢#5 — is_sqlite_url docstring on :memory: ---------------------------------------------- Updated docstring to note both file-backed and in-memory forms are recognized; caller is responsible for ``:memory:`` magic. Bot 🟢#6 — db_path via make_url(...).database --------------------------------------------- Replaced ``database_url.split("///", 1)[1]`` hack with SQLAlchemy's own URL parsing. Naturally handles in-memory (``.database is None`` → falls back to ``""``). Same lazy-import pattern as the existing ``mask_db_password`` to avoid module-import-time cost. Bot 🟢#7 — _to_sync_url unrecognized-driver guard ------------------------------------------------- Pulled ``_KNOWN_ASYNC_DRIVERS = ("aiosqlite", "asyncpg")`` into a module constant. When an unrecognized ``+<driver>`` token survives the strip, emits ``logger.warning`` with the known-supported list. Behavior unchanged for valid URLs. Bot 🟢#8 — get_audit_logs SELECT * → explicit columns ----------------------------------------------------- Replaced ``SELECT *`` with explicit column list. Future schema additions stay out of the dict return. New tests --------- - ``test_close_disposes_engine``: pins the public contract — engine nulled, state reset, second call is a no-op. - ``test_concurrent_initialize_serialized_by_advisory_lock``: spawns 3 concurrent inits against a fresh schema; asserts no "relation already exists" and exactly one ``alembic_version`` row at the end. Without the lock, this reliably fails on the second concurrent task. Docs ---- - ADR-026: new "Concurrent migrations across pods" subsection documents the advisory-lock approach + lock-ID derivation. Verification ------------ - ``uv run pytest tests/unit/`` — 1025 passed. - ``TEST_DATABASE_URL=… uv run pytest tests/integration/test_storage_postgres.py -m postgres`` — 9 passed (was 7). - ``ruff check && ruff format --check && ty check`` — clean. Expected post-push: SQ scan reports 0 OPEN issues (was 4). 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) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
51419329b0 |
fix(storage): address PR #798 round-3 review (SonarQube + pool sizing + RETURNING test)
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) <noreply@anthropic.com> |
||
|
Chris CoutinhoandClaude Opus 4.7
|
f2b7bf132f |
fix(storage): address PR #798 review feedback (credentials, asyncpg extra, TLS, pool)
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) <noreply@anthropic.com> |