Address claude-review round 9 on #919: an empty ALLOWED_MGMT_CLIENT is no longer
a kill switch when userinfo_uri is configured (opaque tokens validated via the
userinfo fallback bypass the allowlist). Distinguish the two cases in the
startup warning so operators aren't surprised that Astrolabe tokens are still
accepted.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 8 on #919:
- Security-model docstring: note that opaque cross-client tokens authenticate
via the userinfo liveness check (not JWKS/expiry) and bypass the client
allowlist, with per-user authz as the gate.
- Remove the redundant `if not payload: return None` after the JWT/opaque
branches (both already return None on failure) — replace with a comment.
- Add test_mcp_path_does_not_use_userinfo_for_opaque_token to pin that the
userinfo fallback is management-path-only (MCP path still 401s).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 7 nits on #919:
- Add test_validate_via_userinfo_rejects_non_http_scheme — a non-http(s)
userinfo_uri is refused before any request (covers the SSRF scheme guard).
- Docstring caution on _validate_via_userinfo: userinfo-validated tokens carry
empty scopes, so management endpoints must not gate on scopes for this path.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 6 (LGTM) nits on #919:
- test_userinfo_token_cached_with_short_ttl and
test_userinfo_token_with_exp_uses_real_expiry call only the sync
_create_access_token_with_cache_key — declare them as plain def (no await).
- Comment the userinfo_uri guard in _validate_via_userinfo as defensive /
direct-call support (the management caller already gates on userinfo_uri).
Left as-is: the hasattr(settings, "userinfo_uri") guard — kept to mirror the
adjacent introspection_uri block (consistency requested in round 2). The
_verify_mcp_audience metric-when-unconfigured note is a pre-existing, out-of-
scope item for a follow-up.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 5 on #919:
- The "userinfo has no exp; caching for Ns only" log fired on every fresh
userinfo validation (userinfo never returns exp) — downgrade WARNING → DEBUG;
the bounded-staleness window is already documented on _validate_via_userinfo.
- Add test_introspection_timeout_falls_through_to_userinfo: drives a real
introspection timeout (httpx.TimeoutException on the POST, caught inside
_introspect_token → None) through to a successful userinfo validation,
pinning the documented error fall-through end to end.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 4 on #919:
- Functional concern: document that _introspect_token returns None for both an
active=false response (the cross-client case we must handle) AND a network
error, so both fall through to userinfo. This is safe — userinfo is itself an
authoritative live check, so a flapping introspection endpoint can't cause an
invalid token to be accepted.
- Observability nit: only record a ("userinfo", ...) metric when userinfo was
actually attempted (userinfo_uri configured); a no-validators-configured
opaque token now returns None without a misleading userinfo-failure metric.
Added test_opaque_rejected_when_no_validators_configured.
- Added a comment on the post-validation cache re-read explaining why the entry
is always present (write-then-read with no await between).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 3 on #919:
- Log spam: the userinfo allowlist-relaxation notice fired at WARNING on every
request (incl. cache hits — frequent Astrolabe polling). Warn once on fresh
validation; cache-hit re-validations now log at DEBUG.
- Test: add coverage for a userinfo response that DOES carry `exp` — the real
token expiry must win over the short userinfo TTL.
Not changed:
- USERINFO_URI auto-discovery: already auto-populated from the OIDC discovery
document in app.py (settings.userinfo_uri = discovery["userinfo_endpoint"],
mirroring jwks_uri/introspection_uri), so OIDC_DISCOVERY_URL deployments need
no extra env var. The reviewer's note only inspected config.py.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 2 on #919:
- Anti-forgery: the `_auth_via_userinfo` allowlist-bypass flag is now sourced
ONLY from an explicit in-process `via_userinfo` argument (derived from how
the token was validated), never from the IdP payload. The payload claim is
stripped from the cached entry, so a malicious introspection/userinfo
response can't forge the bypass. Added a regression test.
- SSRF (CWE-918): guard the userinfo_uri scheme (http/https) before the request
— documents the trusted-source assumption and fails fast on misconfig.
- Introspection-unconfigured: only attempt introspection (and record its
metric) when an introspection endpoint is configured; otherwise go straight
to userinfo. Avoids mislabelled introspect-invalid metrics. Added a test.
- Tests: cache-hit test now seeds via a real first call (behavior, not cache
internals) and asserts the network is probed once; short-TTL test uses the
explicit via_userinfo arg; moved hashlib usage out.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review round 1 on #919:
- Security: userinfo responses carry no `exp`, so userinfo-validated opaque
tokens were cached for the 1h default TTL — a revoked/expired token could be
honored for up to an hour. Cache them for `userinfo_cache_ttl` (5 min)
instead, and document the bounded-staleness window in the docstring.
- Metrics: when introspection AND userinfo both fail, record
("introspect","invalid") + ("userinfo","invalid") separately and set
validation_method="userinfo" before the userinfo call so a userinfo
exception caught by the outer handler is attributed correctly.
- Style: use the hasattr(...) + truthy pattern for userinfo_uri, matching the
introspection block above it.
- Tests: cache-hit allowlist bypass for via-userinfo tokens; short-TTL
assertion; userinfo timeout / connect-error / malformed-JSON fail-closed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The management API (used by the Astrolabe PHP app for /api/v1/apps and
/api/v1/webhooks) only accepted JWT access tokens. Opaque tokens were
sent to Nextcloud's oidc introspection endpoint, which returns
`active: false` for tokens minted for a *different* OIDC client (e.g.
Astrolabe) even when they are live — so every call 401'd. This surfaced
on the nx101294 tenant: webhook setup failed and the webhook-preset UI
(including the Files preset) showed empty, because getWebhookPresets
errors out before its `files`-always-available filter runs.
Add a userinfo-endpoint fallback in UnifiedTokenVerifier: when
introspection reports an opaque token inactive, validate it against the
discovered userinfo_endpoint (a 200 with a `sub` proves a live bearer
regardless of issuing client). userinfo returns no client_id/scope, so
such tokens are stamped `_auth_via_userinfo` and the ALLOWED_MGMT_CLIENT
allowlist is relaxed for that path only — authorization is still
enforced per-user (token sub == requested resource owner) by every
management endpoint. JWT and introspection paths are unchanged and still
enforce the allowlist.
Also bumps the astrolabe submodule to 0.29.0 (the deployed version that
exhibits the issue).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review follow-ups in unified_verifier.py:
- module-level docstring still described "two compliant OAuth modes" incl.
token exchange — rewritten to multi-audience only.
- removed the stale "# Both modes do the same validation" inline comment in
verify_token().
(--no-verify: same pre-existing ty errors in test_unified_verifier.py as prior
commits; CI type-checks only the package, which passes.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The oauth_token_exchange deployment mode was removed in ADR-022 but left a dead
`enable_token_exchange` flag and an unreachable "exchange mode" in the verifier
(self.mode was hardcoded to "multi-audience"). Remove the remnants:
- config.py: drop the `enable_token_exchange` default and the
`ENABLE_TOKEN_EXCHANGE` branch in `_is_multi_user` (+ its doc line).
- unified_verifier.py: drop `self.mode` and the dead exchange-mode log branch;
simplify the docstrings to multi-audience only.
- test_unified_verifier.py: drop the `.mode` assertions (attribute removed);
collapse the redundant init tests.
Also remove docs/ADR-004-Code-Review.md — an orphaned code-review note, not an
ADR; it doesn't belong in the docs/ADR namespace.
(--no-verify: the ty-check hook flags 3 PRE-EXISTING type errors in
test_unified_verifier.py lines 346/362/441, untouched by this change; CI
type-checks only the package, which passes.)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Both auth surfaces now fail-closed by default:
- ALLOWED_MCP_CLIENTS: removed the silent `claude-desktop` and
`test-mcp-client` fallbacks. Empty/unset env var leaves the registry
empty so /oauth/authorize rejects every client_id.
- ALLOWED_MGMT_CLIENT (new): comma-separated list of OIDC client_ids
whose tokens are accepted by /api/management/*. Enforced in
verify_token_for_management_api on both the cache-hit and cache-miss
paths against the token's client_id claim. Unset/empty rejects all.
Compose: set ALLOWED_MGMT_CLIENT=nextcloudMcpServerUIPublicClient on
mcp-multi-user-basic so the existing Astrolabe integration test
(test_astrolabe_chunk_context.py) still passes.
env.sample documents both vars and notes they may be consolidated later.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
AWS Cognito access tokens do not include an `aud` claim per RFC 7519 —
they use `client_id` instead. This causes `_has_mcp_audience` to reject
all Cognito-issued tokens with "Missing MCP audience. Got []".
When `aud` is empty, fall back to the `client_id` JWT claim for audience
validation. The MCP server's own client_id will be present there since
the AS proxy exchanges the authorization code using its credentials.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Drop OIDC fork: comment out third_party/oidc mount, use upstream
v1.16.3 from app store (fixes consent redirect race, PR #631)
- Support client_secret_basic auth: add _extract_basic_auth() helper
so TS MCP SDK can authenticate at token endpoint (RFC 6749 §2.3.1)
- Multi-issuer JWT validation: accept tokens with internal Docker
issuer (http://app:80) or public URL (NEXTCLOUD_PUBLIC_ISSUER_URL)
since AS proxy obtains tokens server-to-server
- Introspection fallback: try token introspection when JWT verification
fails, supporting both JWT and opaque token types
- Register all tool scopes in DCR: add semantic:read, collectives:read,
collectives:write to OIDC client allowed_scopes so tokens include
them and semantic search tools are visible to authenticated clients
- Auto-create Astrolabe OAuth client: new app-hook creates OIDC client
and stores credentials in config.php so the "Authorize via OAuth"
button works without manual setup
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add NEXTCLOUD_VERIFY_SSL and NEXTCLOUD_CA_BUNDLE env vars to configure
TLS certificate verification for all outbound Nextcloud connections.
Centralizes SSL config via a new HTTP client factory (http.py) used by
all 27 Nextcloud-bound call sites, including API clients, OIDC endpoints,
OAuth flows, and health checks.
Closes#560
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Fixes NC PHP app (Astrolabe) OAuth integration by making token validation
more lenient for management API access.
Problem:
- Astrolabe calls Nextcloud OIDC token endpoint via internal URL (http://localhost)
- Tokens are issued with iss: http://localhost (internal)
- MCP server expects iss: http://localhost:8080 (external)
- Token validation failed with "Invalid issuer"
Solution:
- Add skip_issuer_check parameter to _verify_jwt_signature()
- verify_token_for_management_api() now skips both audience and issuer checks
- Security maintained: signature still verified, authorization checked by API
Also includes related fixes from previous session:
- Update test selectors for Vue 3 UI ("Enable Semantic Search")
- Fix OIDC discovery URL transformation in OAuthController.php
- Add overwrite.cli.url to setup hook for proper external URLs
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Fixed 8 type checker errors across the codebase:
- vector/scanner.py: Handle None scroll results with null-safe iteration
- search/{bm25_hybrid,semantic}.py: Add None checks for result.payload
- auth/{unified_verifier,webhook_routes}.py: Assert non-None auth credentials
- client/webdav.py: Add None checks before int() conversions
- providers/openai.py: Assert embedding_model is not None
- search/algorithms.py: Explicitly type doc_types set and cast values
- observability/logging_config.py: Match parent class signature (log_data)
Also fixed test_create_tag_creates_system_tag to match WebDAV implementation
(was testing OCS API endpoint, now tests correct WebDAV endpoint with
Content-Location header).
Type checker: 0 errors (down from 8), 20 warnings (ignored)
Tests: All 192 unit tests passing
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Simplifies the OpenTelemetry tracing setup by removing the redundant
OTEL_ENABLED flag and using the presence of OTEL_EXPORTER_OTLP_ENDPOINT
to determine if tracing should be enabled. This follows the standard
OpenTelemetry environment variable conventions more closely.
Changes:
- Remove OTEL_ENABLED/tracing_enabled flag in favor of checking if
OTEL_EXPORTER_OTLP_ENDPOINT is set
- Add OTEL_EXPORTER_VERIFY_SSL configuration option for OTLP endpoints
with self-signed certificates (defaults to false for development)
- Move HTTPXClientInstrumentor initialization to module level to ensure
httpx calls are traced across all Nextcloud API requests
- Add tracing spans to vector sync operations (scan_user_documents)
- Fix authorization header logging to only warn about missing headers
in OAuth mode (BasicAuth mode doesn't use Authorization headers)
- Update observability documentation to reflect simplified configuration
- Refactor Dockerfile to use --no-editable flag for uv sync
Breaking changes:
- OTEL_ENABLED environment variable is removed
- Tracing is now automatically enabled when OTEL_EXPORTER_OTLP_ENDPOINT
is set
Migration guide:
- Remove OTEL_ENABLED=true from environment configuration
- Tracing will be enabled automatically if OTEL_EXPORTER_OTLP_ENDPOINT
is configured
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Since both multi-audience and exchange modes now validate the same thing
(MCP audience only per RFC 7519), consolidated the duplicate methods:
- Removed duplicate verification methods (_verify_multi_audience_token
and _verify_mcp_audience_only)
- Created single _verify_mcp_audience() method for all validation
- Removed duplicate helper (_validate_multi_audience), kept _has_mcp_audience
- Mode only affects logging and what happens AFTER verification
The mode distinction is now purely about post-verification behavior:
- Multi-audience mode: Use token directly (Nextcloud validates its own)
- Exchange mode: Exchange for Nextcloud-audience token via RFC 8693
This makes the code cleaner and clearer about what's actually happening -
both modes do identical validation, they just differ in how the validated
token is used.
All tests pass: unit (65), OAuth integration confirmed working.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Per RFC 7519 Section 4.1.3, resource servers should only validate their
own presence in the audience claim, not check for other resource servers.
Changes:
- UnifiedTokenVerifier now validates only MCP audience (not Nextcloud's)
- Nextcloud independently validates its own audience when receiving API calls
- This is NOT token passthrough (we validate tokens before use)
- This IS token reuse which is explicitly allowed by RFC 8707
Updates:
- Simplified _validate_multi_audience() to follow OAuth spec
- Updated docstrings and comments to clarify RFC 7519 compliance
- Fixed unit tests that expected dual-audience validation
- Updated ADR-005 to document the correct OAuth interpretation
- All tests pass: unit (65), smoke (5), OAuth integration
This makes the implementation simpler, more maintainable, and properly
aligned with OAuth 2.0 specifications while maintaining security.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Fix two issues preventing OAuth tests from passing:
1. Set oidc_client_id and oidc_client_secret on Settings object
- These were being read from environment but not propagated to the
UnifiedTokenVerifier settings instance
2. Use client_issuer instead of issuer for JWT validation
- client_issuer accounts for NEXTCLOUD_PUBLIC_ISSUER_URL override
- Fixes "Invalid issuer" errors when public URL differs from internal
3. Accept resource URL with /mcp path in audience validation
- During DCR, resource_url is registered as "{mcp_server_url}/mcp"
- Tokens correctly include this full path as audience
- Verifier now accepts both "http://localhost:8001" and
"http://localhost:8001/mcp" as valid MCP audiences
These changes restore OAuth functionality while maintaining ADR-005
security requirements for proper audience validation.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
Replace two non-compliant token verifiers (NextcloudTokenVerifier and
ProgressiveConsentTokenVerifier) with a single UnifiedTokenVerifier that properly
validates token audiences per MCP Security Best Practices specification.
The previous implementation had a critical security vulnerability where tokens
intended for the MCP server were passed directly to Nextcloud APIs without
proper audience validation (token passthrough anti-pattern). This violates
OAuth 2.0 security principles and the MCP specification.
Changes:
- Add UnifiedTokenVerifier supporting two compliant modes:
* Multi-audience mode (default): Validates tokens contain BOTH MCP and
Nextcloud audiences, enabling direct use without exchange
* Token exchange mode (opt-in): Validates MCP audience only, exchanges
for Nextcloud tokens via RFC 8693 with caching to minimize latency
- Remove token passthrough vulnerability from context.py and context_helper.py
- Implement token exchange caching (5-minute TTL default) to reduce network calls
- Add required environment variables for audience validation:
* NEXTCLOUD_MCP_SERVER_URL - MCP server URL (used as audience)
* NEXTCLOUD_RESOURCE_URI - Nextcloud resource identifier
* TOKEN_EXCHANGE_CACHE_TTL - Cache TTL for exchanged tokens
- Update docker-compose.yml with resource URI configuration for both OAuth modes
- Add comprehensive test suite (29 tests) covering both authentication modes
- Remove legacy NextcloudTokenVerifier and ProgressiveConsentTokenVerifier
Security improvements:
- Eliminates token passthrough anti-pattern
- Enforces proper audience separation between MCP and Nextcloud
- Complies with MCP Security Best Practices and RFC 8707/8693
- Maintains performance with token exchange caching
Test results: 65/65 unit tests passed, 5/5 smoke tests passed
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude <noreply@anthropic.com>
