- Gate browser session creation on a successful refresh token. When the
IdP returns no refresh token, SessionAuthBackend would silently reject
every subsequent request and bounce the user back to /oauth/login in a
loop. The callback now bails with a 400 + correlation ID + actionable
hint about offline_access *before* writing browser_sessions or setting
the cookie. Pinned by a new end-to-end unit test.
- Evict orphaned browser_sessions rows in SessionAuthBackend when the
associated refresh token is gone, instead of letting them accumulate
until TTL cleanup. Best-effort; deletion errors stay non-fatal.
- Demote identity-bearing logs in the Flow 2 OAuth callback (user_id,
scopes, audience, expires_at) from INFO to DEBUG so they don't leak
into multi-tenant log aggregation on every provision.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Coerce refresh_expires_in to int before arithmetic in both callback
paths so IdPs that serialize the field as a JSON string (e.g. AWS
Cognito) don't trigger an unhandled TypeError 500.
- Drop the orphaned oauth_session row written by _check_logged_in. The
canonical Flow 2 row is created by generate_oauth_url_for_flow2 keyed
by `state`, which is what the unified callback looks up; the
flow2_<hex> session_id was never matched and just churned the table
for 10 minutes per call.
- Match delete_cookie attributes (httponly, secure, samesite) to the
set_cookie call on logout so browsers reliably evict the cookie even
on implementations that consider security flags during deletion.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Five findings from the latest review on #758 (2 medium, 3 nit):
Medium:
- browser_oauth_routes.oauth_login_callback + oauth_routes.oauth_callback_nextcloud:
fail closed with 400 when the oauth_session row is unknown/expired. Previously
both callbacks fell through with code_verifier="" and expected_nonce=None,
silently bypassing the PKCE + nonce protections introduced in earlier rounds.
Symmetric unit tests pin both contracts.
- token_utils.verify_id_token: use secrets.compare_digest for the nonce check
instead of short-circuit !=. Mirrors the sibling PKCE verifier comparison;
closes the last secret-equality timing-side-channel surface in the auth path.
Nit:
- Tighten the comment at all 4 mcp_authorization_code/code_verifier store +
retrieve sites so a future refactor sees the field reuse immediately
(renaming the column requires a schema migration).
- _should_use_secure_cookies: explicit string normalisation instead of
bool(settings.cookie_secure). Dynaconf normally coerces but tests / direct
settings.set calls can leave the raw string in place — bool("false") is True.
New parametrized unit tests cover the coercion matrix + http/https fallback.
- oauth_routes.py:591 f-string log converted to lazy %s formatting (folded into
the Flow 2 callback rewrite).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three findings from the latest review on #758 (1 medium, 2 low):
Medium:
- browser_oauth_routes.oauth_logout: move delete_browser_session into a
finally block so an error from delete_refresh_token can no longer leave
an orphan browser_sessions row. The orphan was not exploitable
(SessionAuthBackend rejects sessions without a live refresh token), but
it lingered until the hourly cleanup cron — a correctness gap. New
regression test pins the fix.
Low:
- oauth_callback_nextcloud: drop redundant ``or None`` from
``expected_nonce=nonce``. ``nonce`` is already ``str | None`` and
``secrets.token_urlsafe`` never produces an empty string, so the
coercion was a no-op that could mislead future readers into thinking
empty-string was a valid skip-the-check path.
- storage.RefreshTokenStorage.initialize: fail fast at startup when
SQLite < 3.35, since ``DELETE ... RETURNING`` (used in
``delete_browser_session``) needs that minimum. Ubuntu 20.04 ships
3.31 and would otherwise hit OperationalError on every logout.
Prerequisite also documented in docs/installation.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Drop "(in-place)" from filter-helper docstrings; callers should
consume the return value, mutation is an implementation detail.
- Document that deck_get_archived_stacks always returns cards (an
archived stack without its cards has no audit value); point to
description_max_length for size control.
- Document that deck_get_cards applies filtering client-side, so it
is network-equivalent to deck_get_stack(include_cards=True).
- Pin the empty-list contract: a stack with all-archived cards and
include_archived_cards=False yields cards == [] (loaded but empty),
not cards is None (explicitly suppressed).
- Add explicit one-character-over-limit truncation test alongside the
existing exact-boundary test.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Seven findings from the latest review on #758 (3 medium, 4 low/nit):
Medium:
- storage.py: replace 5 ``assert self.cipher is not None`` sites with
explicit ``RuntimeError`` so missing TOKEN_ENCRYPTION_KEY can't silently
become an AttributeError under ``python -O``
- session_backend.py: document the silent-invalidation invariant —
refresh-token TTL expiry without explicit logout deliberately makes
the browser session unusable; future readers must not relax it
- server/oauth_tools.py: drop user_id from the Flow 2 session_id
identifier — use ``flow2_{secrets.token_hex(16)}`` so audit logs and
DB rows don't carry user_id in the session_id field
Low / nit:
- token_utils.py: drop _fetch_locks dict entry in finally so a probed
deployment can't grow the lock dict without bound; coalescing test
now pins the invariant with len(_fetch_locks) == 0
- browser_oauth_routes.py: strip trailing slash from settings.nextcloud_host
before constructing the well-known URL so a host configured as
``https://cloud.example.com/`` doesn't produce a double-slash
- browser_oauth_routes.py: add comment explaining the three-layer CSRF
policy on the mcp_session cookie set (SameSite=Lax + POST-only logout
+ Origin/Referer check)
- oauth_routes.py: convert all 23 f-string log calls to lazy %-style
per the CLAUDE.md / memory feedback_lazy_logging convention
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Move description_max_length validation to tool layer
(_validate_description_max_length), matching the existing
_validate_comment_message pattern; helper now trusts callers per
CLAUDE.md ("validate at system boundaries only").
- Fix mutation/return inconsistency: deck_get_stacks now uses a list
comprehension to capture _apply_stack_filters' return, matching
deck_get_stack / deck_get_archived_stacks.
- Rename include_archived -> include_archived_cards on deck_get_cards
and _apply_card_filters for consistency with deck_get_stacks.
- Route deck_get_archived_stacks through _apply_stack_filters so
future filters apply uniformly to active + archived paths.
- Trim _truncate_card_descriptions docstring to one line; add inline
comment in _apply_stack_filters explaining the breaking-change
default (mirrors Deck UI archived-card filtering).
- Replace fragile call_args[0][1] with call_args.args[1] in the
archived-stacks client test.
- Modernize Optional[X] -> X | None throughout deck.py (adjacent
cleanup called out in the review).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Validate description_max_length is positive (raises ValueError on 0
or negative); the prior code would have wiped descriptions to a
single ellipsis character on description_max_length=0.
- Extract filter logic into testable module-level helpers
(_apply_board_filters, _apply_stack_filters, _apply_card_filters)
and replace the dense `continue`-based loop in deck_get_stacks with
the reviewer's elif form.
- Document the truncation length quirk in the helper docstring (result
is description_max_length + 1 chars when truncation fires).
- Add 13 new unit tests covering the include/exclude flags on board,
stacks, and flat card lists, plus the new validation paths and an
explicit "description fits within limit, no ellipsis" case.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add filtering options to deck read tools to keep responses compact on
boards with accumulated cards/comments, and expose archived stacks so
agents can audit completed work that has been archived off the active
board.
- deck_get_board: include_acl, include_users, include_labels
- deck_get_stacks/deck_get_stack: include_cards, include_archived_cards,
description_max_length
- deck_get_cards: include_archived, description_max_length
- New deck_get_archived_stacks tool wrapping the existing client method
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Seven findings from the latest review on #758, plus a regression test
catching the substance of the cache-stampede fix:
- verify_id_token: widen id_token annotation to str | None to match
callers passing nc_token_response.get("id_token")
- extract_user_id_from_token: use JSON-RPC reserved error code -32001
instead of -1
- _get_cached: per-URL anyio.Lock dict + meta-lock coalesces concurrent
cache misses into a single IdP fetch (mirrors token_broker.py idiom)
- delete_browser_session: collapse SELECT+DELETE into atomic
DELETE ... RETURNING user_id (SQLite >= 3.35)
- new test_origin_normalise.py: parametrized port/scheme/host equivalence
cases for the CSRF Origin guard
- browser_oauth_routes: correct misleading "PR #758 finding 5" cross-
references (finding 5 was Fernet-key hardening, not CSRF)
- ASProxySession.nonce: make required, drop spurious "legacy session"
default; reword the in-flight `or None` comment to reflect that
ASProxySession is purely in-memory
- new test_get_cached_coalesces_concurrent_misses: pins the
cache-stampede protection — fires 10 concurrent _get_cached calls and
asserts exactly one HTTP fetch
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Flow 2 (oauth_authorize_nextcloud) now generates a nonce, stores it on
the oauth_session row, forwards it to the IdP, and verifies it via
expected_nonce in oauth_callback_nextcloud — closes the last replay-
protection gap (round-3 finding 1).
- _origin_matches_self fails closed when mcp_server_url is missing
instead of allowing the logout, and the diagnostic log is promoted
from warning to error so the misconfiguration is monitorable
(round-3 finding 2). New regression test pins the new behaviour.
- The five user_id-accepting helpers in oauth_tools.py (get_provisioning_status,
provision_nextcloud_access, revoke_nextcloud_access, check_provisioning_status,
check_logged_in) are renamed with leading underscores to make the
trust boundary structural rather than documentary
(round-3 finding 3).
- create_browser_session and delete_browser_session now emit audit_log
rows so session establishment / teardown match the pattern used by
the rest of the security-relevant storage operations
(round-3 nit 5). delete_browser_session selects user_id before delete
so the audit row is attributable.
- oauth_login_callback no longer reflects raw IdP-error text or
exception strings into the HTML failure page; users see a generic
"internal error occurred" message + a correlation ID, with the
detail logged server-side keyed by the same ID (round-3 nit 6).
The XSS regression test is updated to pin the stricter contract.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- oauth_login_callback's integrated-mode token-exchange branch now reuses
the shared discovery cache via get_oidc_discovery (round-2 finding 1).
- AS proxy flow now generates an OIDC nonce in oauth_authorize, stores it
on ASProxySession, forwards it to the IdP, and passes it as
expected_nonce to verify_id_token in _oauth_callback_as_proxy
(round-2 finding 2).
- Consolidate the two parallel discovery caches: oauth_routes' local
_discovery_cache and _get_cached_discovery are removed; all callers
now go through token_utils.get_oidc_discovery, which acquires the
follow_redirects=True knob it needs for Nextcloud installs without
pretty URLs (round-2 finding 3).
- Demote per-user INFO logs in oauth_tools.py (check_logged_in,
get_provisioning_status) to DEBUG; the elicitation auth URL is no
longer logged because it contains a sensitive state token
(round-2 finding 4).
Also pin nonce binding behaviour with a new unit test that asserts
_oauth_callback_as_proxy forwards session.nonce to verify_id_token, and
update test mocks to track the cache consolidation.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Blocking:
- AS proxy callback now calls verify_id_token before caching the proxy
code so a tampered IdP response can't smuggle identity claims.
Important:
- Browser OAuth flow generates and verifies an OIDC nonce; new alembic
migration 006 adds the nonce column to oauth_sessions.
- _origin_matches_self logs a warning when CSRF check is bypassed.
- oauth_tools.py uses get_shared_storage instead of fresh handles.
Nits:
- New token_utils.get_oidc_discovery shares the 5-minute cache with
verify_id_token; oauth_login (integrated) and _revoke_refresh_token_at_idp
now use it instead of issuing fresh discovery fetches.
- Drop typing.Optional from oauth_tools.py in favour of X | None.
CI:
- test.yml generates an ephemeral Fernet TOKEN_ENCRYPTION_KEY per run
with openssl, removing the dependency on a missing repo secret.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses the two remaining 🟡 findings from the PR #758 follow-up review:
1. extract_user_id_from_token previously fell back to "default_user" when
the verified access token had no sub claim. In a multi-tenant deployment
a malformed IdP token could have bucketed every request under a single
sentinel user, risking cross-tenant data exposure. The function now
raises McpError on that branch; the BasicAuth no-token sentinel path is
preserved.
2. oauth_callback_nextcloud (Flow 2) read the PKCE code_verifier from
oauth_sessions but never deleted the row, leaving the verifier valid for
the full 10-minute TTL. The row is now deleted eagerly inside the same
branch, mirroring oauth_login_callback in browser_oauth_routes.
Also wires TOKEN_ENCRYPTION_KEY through the docker-compose step in the CI
test workflow so the integration matrix can boot — every job had been
failing fast on the ${TOKEN_ENCRYPTION_KEY:?...} interpolation guard added
in PR #758 finding 5.
Tests pin both fixes (test_token_utils_user_id.py,
test_oauth_callback_session_cleanup.py).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six findings from the latest claude-bot review on PR #758:
- JWKS cache had no kid-miss refresh path (Medium): on IdP key
rotation every login failed for up to _OIDC_CACHE_TTL. Evict
and refetch once before raising, per OIDC core §10.1.1.
- _should_use_secure_cookies fell back to nextcloud_host scheme,
but the cookie is issued by the MCP server. Switch to
settings.nextcloud_mcp_server_url so split-scheme deployments
get the right Secure flag.
- _origin_matches_self compared raw netloc strings, which include
the port. Browsers omit default ports per RFC 6454 §6.2; an
mcp_server_url like :443 falsely 403'd every legitimate logout.
Normalise (scheme, host, port) tuples with default ports stripped.
- delete_oauth_session exists in storage.py — drop the stale
"we don't have this method" comment and call it eagerly so
replays can't be processed and the table doesn't accumulate
completed-but-not-yet-expired browser-login rows.
- extract_user_id_from_token's unused ctx param renamed to _ctx
to signal "intentionally unused" at the signature level.
- provisioning_decorator instantiated RefreshTokenStorage per
call. Switch to get_shared_storage() for the lock-protected
process-wide singleton.
Plus pre-push self-review catch: lazy-logging on the unchanged
except arm in session_backend.py.
Adds 5 regression tests:
- JWKS rotation: success on refetch
- JWKS rotation: still-missing-kid surfaces original error
- JWKS rotation: network error during refresh wrapped as
IdTokenVerificationError
- default-port CSRF: explicit :443 in config + portless Origin
- default-port CSRF: portless config + explicit :443 in Origin
- scheme-mismatch CSRF: same host, different scheme rejected
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
After merging master, _should_use_secure_cookies was refactored to read
from Settings instead of os.getenv, which dropped `import os` from
browser_oauth_routes.py — leaving _revoke_refresh_token_at_idp's four
remaining os.getenv() calls undefined (CI ruff F821).
Migrate the helper to the same Settings-based pattern:
- oidc_discovery_url → settings.oidc_discovery_url
- OIDC_CLIENT_ID → settings.oidc_client_id
- OIDC_CLIENT_SECRET → settings.oidc_client_secret
- NEXTCLOUD_HOST → settings.nextcloud_host
Drive-by: the previous fallback read OIDC_CLIENT_ID, but the canonical
env var per env.sample / docker-compose is NEXTCLOUD_OIDC_CLIENT_ID.
The Settings layer handles this mapping via dynaconf, so the corrected
name is now used automatically.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses all 9 findings from the review on PR #758:
Blocking:
- _revoke_refresh_token_at_idp now reads config from oauth_ctx["config"]
(the production-shaped nested dict). Previously read flat keys, causing
IdP revocation to silently no-op in production. Test fixtures rebuilt
to the realistic nested shape so the bug can't regress unnoticed.
- HTML error responses in oauth_login_callback now wrap IdP-controlled
error_body, str(e), and the attacker-controlled error/error_description
query params in html_escape. New test_browser_oauth_xss.py pins this.
Important:
- New _safe_next_url helper validates the ?next= query param at write
time (oauth_login), in oauth_logout, and on read from the session row
in oauth_login_callback. Blocks https://, // (protocol-relative), and
CRLF/whitespace injection.
- verify_id_token now caches discovery + JWKS (5-min TTL) using the
same pattern as oauth_routes._get_cached_discovery. New caching
regression test pins to one fetch per URL across multiple calls.
- /oauth/logout is now POST-only at the route layer (defeats passive
CSRF via <img src>). oauth_logout also validates Origin/Referer
against the configured mcp_server_url. Logout UI in user_info.html
converted from <a href> to <form method="post">.
- New storage.cleanup_expired_browser_sessions() called from the hourly
cleanup loop in app.py — previously these rows accumulated for users
who never explicitly logged out.
Nits:
- Demoted INFO logs that leaked oauth_config.keys() / client_id /
token-storage state to DEBUG. Operator-relevant outcome lines
(login successful, refresh token stored, logged out) stay INFO.
- verify_id_token algorithms widened to RS256, PS256, ES256 — covers
Azure AD (PS256) and Cognito/some Keycloak realms (ES256). Symmetric
and "none" remain off the allowlist.
- Migrated all Optional[X] usages in auth/storage.py to X | None per
CLAUDE.md.
Breaking change: GET /oauth/logout now returns 405. The in-tree logout
UI was migrated to a POST form; any external bookmark or curl-based
caller that relied on GET will need to switch.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three review items from the third-round review on PR #757:
- scope_authorization: split the combined logger.warning(error_msg) in
the require_scopes decorator's missing-app-password branch into two
lazy %-style logger calls (one per branch), keeping the f-string
error_msg for the exception only. The else branch also logs the
elicit_result for diagnostics. Bypassing lazy %-interpolation in
security-sensitive code formatted the message regardless of log level
and matched the repo-wide lazy-logging preference; the new code now
conforms.
- config + browser_oauth_routes: wire COOKIE_SECURE through Settings
(cookie_secure: bool | None = None) so _should_use_secure_cookies()
reads it via get_settings() rather than os.getenv. Completes the
consolidation pass that touched this file in commit 7464340 and
removes the last raw os.getenv from browser_oauth_routes.py
(import os dropped). Dynaconf auto-coerces "true"/"false" → bool;
"1"/"0" arrive as int and are normalised by an explicit bool() at
the consumer.
- elicitation: clarify the _astrolabe_settings_url docstring to call
out that the empty-string case is also a None-return path (matches
the existing `if not base:` guard).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Pre-launch hardening for the hosted Astrolabe Cloud offering. Addresses
all five findings raised in #626 (Tim Kaufmann, code review of v0.65.0).
Re-verified against master before fixing.
Finding 3 (LLM-controllable user_id) — drop user_id from the public
signatures of provision_nextcloud_access, revoke_nextcloud_access,
check_provisioning_status, check_logged_in. Tool wrappers now always
derive identity from the verified AccessToken; user_id is no longer
accepted as MCP input. Adds parameterized CI-guard test that locks the
schema.
Finding 2 (predictable session cookie) — replace mcp_session=<user_id>
cookie with a cryptographically random session_id mapped server-side
(new browser_sessions table, alembic 005). Cookie value is opaque,
expires, revocable. SessionAuthBackend looks up user_id via the new
mapping and additionally requires a refresh token to fail closed.
Finding 4 (logout doesn't revoke refresh token) — oauth_logout now
calls the IdP revocation_endpoint (RFC 7009) when advertised, deletes
the stored refresh token regardless, and clears the browser_sessions
row. Cleanup is best-effort: logout always 302s.
Finding 1 (unverified ID token decodes) — verify_id_token helper does
JWKS signature + issuer + audience + exp + nonce checks per OIDC core
3.1.3.7. Used by both OAuth callback handlers (browser + MCP). Removes
the four "verify_signature: False" decodes that previously trusted IdP
claims unconditionally. Drops dead-code _validate_token_audience in
token_broker. Refactors token_utils + provisioning_decorator to read
user_id from the verified AccessToken instead of re-decoding the JWT.
Finding 5 (hardcoded Fernet keys in docker-compose.yml) — replace the
three inline TOKEN_ENCRYPTION_KEY values with required env var
interpolation; document in env.sample.
Test coverage: 4 new unit test modules (signature pinning, browser
sessions, ID-token verification, logout + revoke + session backend).
693 unit tests pass; ruff/format/ty clean.
Migration note: existing browser admin-UI sessions become invalid on
rollout (cookies are looked up against the new browser_sessions table,
which starts empty). Users re-login. MCP API access is unaffected.
Tracked on Astrolabe Cloud POC board card #37.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The elicitation flow points users to the Astrolabe web route or the
BasicAuth REST endpoint to provision their app password. Both paths
stored the password without clearing the in-process scope cache, so a
user who provisioned through them would keep hitting
ProvisioningRequiredError for up to _SCOPE_CACHE_TTL (5 min) afterwards.
Add invalidate_scope_cache(user_id) to both write-paths (matching the
existing pattern in nc_auth_check_status), correct the now-misleading
comment in scope_authorization.py to name all three invalidation paths,
and add a one-line hint above the first elicitation patch in the test
file so future authors don't "fix" the patch target to the wrong module.
Addresses PR #757 round-3 review feedback.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Four review items from the second-round review on PR #757:
- scope_authorization: broaden the post-elicit retry message to acknowledge
the 5-minute scope-cache TTL — if the LFv2 poller is still in-flight at
acknowledge-time, the immediate retry can still hit a stale cache.
- elicitation: extract a shared `_run_elicit(ctx, message, schema, *,
log_label)` helper so `present_login_url` and
`present_provisioning_required` no longer duplicate the
hasattr-guard / try-NotImplementedError / try-Exception fallback block.
The data-acknowledged warning specific to login-flow stays in
`present_login_url` so behaviour is preserved exactly.
- elicitation: detect missing http:// / https:// scheme in
`_astrolabe_settings_url`, log a warning, and return None — caller
renders the safe tool-only fallback instead of producing a broken link.
New unit test locks this in.
- browser_oauth_routes: replace the stray
`os.getenv(\"NEXTCLOUD_HOST\")` in `_should_use_secure_cookies` with
`get_settings().nextcloud_host` for consistency with the rest of the
file (PR #757 review nit).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Lift NEXTCLOUD_PUBLIC_ISSUER_URL out of raw os.getenv reads into
Settings.nextcloud_public_issuer_url across all 8 production call sites
(app.py x2, oauth_routes.py x2, browser_oauth_routes.py,
provision_routes.py, userinfo_routes.py, elicitation.py). cli.py
remains the env-write source so the existing config-by-flag pipeline
still works.
Also addresses remaining PR #757 review nits:
- elicitation.py: align URL-present/absent wording on "open in your
browser" so users don't try clicking in the terminal
- test_scope_authorization_stored.py: lock in the deliberately-shared
fall-through branch with explicit declined/cancelled decorator tests
- test_elicitation.py: switch from monkeypatch.setenv to
patch(get_settings) since Settings is now the canonical surface
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Branch the ProvisioningRequiredError message on the elicit result so a
user who acknowledged the prompt isn't told to call
nc_auth_provision_access (which would loop an LLM that just confirmed
via elicitation). Other paths keep the existing instruction.
- Convert present_login_url's f-string logger.warning to lazy %s, matching
present_provisioning_required and the repo's lazy-logging preference.
- Add a test for NEXTCLOUD_PUBLIC_ISSUER_URL trailing-slash normalization.
- Strengthen the decorator-elicits test: split into the "accepted" and
"message_only" branches so the error-message change is regression-tested.
Refs: cbcoutinho/nextcloud-mcp-server#757#issuecomment-4363552487
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Calls out the apps-to-install matrix (user_oidc required, oidc skip,
astrolabe optional), the OIDC clients to register and what each is for,
the per-app scope advertisement requirement on the IdP side, and the
"OAuth succeeded but Nextcloud returns 401" diagnosis path.
Mined from the cbcoutinho/nextcloud-mcp-server#752 thread.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
When a tool requiring Nextcloud access is called without a stored app
password (Login Flow v2 mode), the @require_scopes decorator now invokes
MCP elicitation with a clickable Astrolabe settings URL — reconstructed
from NEXTCLOUD_PUBLIC_ISSUER_URL / NEXTCLOUD_HOST — before raising
ProvisioningRequiredError. Clients without elicitation support fall back
to the existing text error.
Surfaced by cbcoutinho/nextcloud-mcp-server#752, where users hit a 401
after OAuth and had no clickable URL to start Login Flow v2 from.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Switch the diff range from $BASE..HEAD to $BASE so the review covers
working-tree changes (committed + staged + unstaged), letting the skill
run usefully on in-progress work without requiring a commit first.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Six review items raised; four required code changes (#3, #4, #5, #6) and
two were resolved without code changes (#1 audit-only, #2 informational).
* search/verification.py — clarify the granularity asymmetry between the
whole-batch fail-open (structural API failure) and the per-item fail-open
(single bad stored doc_id). Future readers no longer need to derive why
the two paths have different blast radii from the code alone.
* models/semantic.py — `dropped_document_count` description now explicitly
notes that subtracting it from `verified_chunk_count` is not a meaningful
operation, since the two fields count different units (documents vs
chunks). Surfaces the unit mismatch where MCP clients actually see it.
* server/semantic.py — clarify the per-doc_type over-fetch comment so the
N×2 pre-merge Qdrant cost (vs the cross-app branch's 1×2) is explicit
rather than implied by "same 2× over-fetch budget".
* tests/unit/search/test_verification.py — add four new 429 unit tests
(notes/news/files/deck) mirroring the existing 5xx-keeps pattern. Locks
in that `_is_definitive_404_or_403` returns False for 429 so a future
refactor cannot accidentally treat rate-limit responses as permanent
revocations.
Audit confirmation for review item #1: all four `WebDAVClient.get_file_info`
call sites already handle the new `HTTPStatusError`-on-404 contract
(verification.py:156, tests/integration/test_rag.py:139,
tests/unit/client/test_webdav.py:153/190). No silent breakage internal to
this repo.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Deployer role:
- Add servicediscovery actions; module always creates Cloud Map namespace
and service so the policy must grant CreatePrivateDnsNamespace etc.
- Make Route53 + ACM permissions unconditional. The server module always
issues an ACM cert and writes Route53 records (no CloudFront default-cert
path exists), so gating these on route53_zone_ids was broken. Split
Route53 into hosted-zone management (always) plus record-set mutation
(scoped to caller-supplied zones, falls back to *).
- Remove unused cloudfront:* statement; no CloudFront resources in module.
- Replace acm:* wildcard with explicit cert-management action set.
Server module:
- qdrant_image_tag is now nullable with default null and validated against
use_external_qdrant, so external-qdrant callers can omit it instead of
passing a sentinel "unused" value.
- task_role_arn and efs_id outputs marked sensitive; qdrant_dns_name returns
null when use_external_qdrant = true.
- ALB SG now has matching IPv6 egress rule (was v4-only).
- nextcloud_url validates the https:// scheme.
- random_pet.subdomain keeper includes zone_name so a zone migration that
preserves zone_id still triggers regeneration.
- Pin required_version >= 1.9 on both modules.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three minor fixes from the round-11 review on PR #750:
- bm25_hybrid.py:209 — Comment said `doc_id` is `int (notes) or str (files)`,
which is backwards. Notes, news_items, and deck_cards are stored as `str`
(scanner.py:241, 666, 867); files are stored as `int` (scanner.py:425).
Updated to point readers at scanner.py as the source of truth.
- verification.py:338 — Lowered the News-API 403/404 log line from `info`
to `debug`. The News app being uninstalled or disabled is a predictable
operational state (matching the other verifiers' debug-on-not-found
paths), so this should not generate operator-dashboard noise. Transient
errors immediately below stay at `warning` because they're unexpected.
- semantic.py:809 — `nc_get_vector_sync_status` was reading
`document_receive_stream` via `getattr(..., None)`, but the attribute is
guaranteed-defined on both `AppContext` and `OAuthAppContext` (as a
field with `None` default). The defensive `getattr` masked typos that
the eviction_task_group access at semantic.py:197-199 deliberately
surfaces. Switched to direct access; the `if … is None:` value-check
below is preserved (the attribute can legitimately be None before sync
starts).
Items deliberately deferred (with rationale in the plan file):
- News verifier semaphore-hold during get_items (reviewer: "not required
here, just worth tracking"; ADR already lists follow-ups).
- Hardcoded 2× over-fetch / VERIFICATION_OVERFETCH (TODO already in code).
- Integration test for the real Qdrant eviction filter (reviewer marked
low-priority; type-preservation chain is unit-tested).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Tighten verify_search_results signature: client: Any → NextcloudClientProtocol
- Collapse 3 copy-pasted lock-justification comments to a single-line pointer
- Add logger.debug timing around the verify_search_results call site
- Add logger.debug timing around the unbounded news.get_items fetch
- Rename SemanticSearchResponse.dropped_count → dropped_document_count to make
the chunks-vs-documents unit asymmetry explicit at the API boundary
- Drop unreachable duplicate 409 branch in WebDAVClient.move_resource
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
POC: lift the homelab-grown nextcloud-mcp-server and
nextcloud-mcp-deployer-role Terraform modules into this repo so external
operators can consume them via a `git::` source.
Includes a top-level README documenting the two-phase deploy flow
(bootstrap deployer role with a copy-pasteable IAM policy, then assume the
role to deploy the MCP server) and supports both in-VPC Qdrant and
external/managed Qdrant modes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- Add concurrency-safety comments to per-verifier accessible sets in
_verify_notes/_verify_files/_verify_deck_cards. Same rationale as
accessible_by_type in verify_search_results: anyio is cooperative,
set.add() is not an await point.
- Document 401 exclusion in _is_definitive_404_or_403 (treated as
transient because it usually signals expired credentials, not
permanent denial).
- Note multi-user compounding in the news verifier semaphore comment:
N concurrent users hold N slots out of the shared budget.
- Log inaccessible doc ids with a type tag (e.g. "int:42" vs "str:42")
so ghost-record logs disambiguate id types.
- Type the BatchVerifier alias and the four verifier function signatures
with NextcloudClientProtocol instead of Any (algorithms.py exposes
the right interface; the protocol is runtime_checkable).
- Surface verified_chunk_count vs dropped_count semantics in the
nc_semantic_search tool docstring Returns block (chunks vs unique
documents).
- Add comments to the two max_concurrent=20 sites in server/semantic.py
noting they are intentionally distinct from
settings.verification_concurrency (different request phases).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
