fix(auth): deny /app in multi-user BasicAuth; warn in single-user

[cbcoutinho]

Closes Deck #853. Third item from the multi-user security audit, after #1150 and #1161.

The problem

SessionAuthBackend.authenticate() returned ["authenticated", "admin"] for any caller whenever OAuth was off — no cookie, no header, no credential of any kind:

if not self.oauth_enabled:
    username = cfg("NEXTCLOUD_USERNAME", "admin")
    return AuthCredentials(["authenticated", "admin"]), SimpleUser(username)

/app is mounted unconditionally in every deployment mode (the Mount("/app", …) sits at top-level indentation in get_app(), not inside a mode conditional), so in multi-user BasicAuth anyone who could reach the port got the admin UI: webhook enable/disable, vector-viz search over the whole indexed corpus, and revoke.

Worse, the identity it granted was cfg("NEXTCLOUD_USERNAME", "admin") — and that mode forbids configuring NEXTCLOUD_USERNAME, so it resolved to a literal "admin", a user who need not exist.

The fix is mode-aware, because the two modes are genuinely different

Multi-user BasicAuth → deny. /app is a session UI and this mode has no session concept; callers authenticate per request with their own credentials against /mcp. Nothing legitimate was served by that branch — it only ever showed a static, forbidden-to-configure identity — so it now fails closed.

Single-user BasicAuth → unchanged, plus a startup warning. The server holds exactly one identity and every request already acts as it, so there is nothing to distinguish callers by. That is the deployment model for a personal instance, not a bug. What was missing is that the assumption was undocumented, so startup now warns:

⚠️  single_user_basic: the /app browser UI is UNAUTHENTICATED — anyone who can
reach this port gets admin access to it. Bind to loopback or put an
authenticating proxy in front; do not expose this port to a network you do not control.

A blanket denial would have broken working personal deployments for no security gain, which is why this isn't one gate.

On the misconfiguration hard-fail

Multi-user with NEXTCLOUD_USERNAME set already hard-fails at startup — I verified rather than assumed:

• nextcloud_username/nextcloud_password are in that mode's forbidden list (config_validators.py:97-100)
• forbidden vars append to errors, and get_app() does raise ValueError(error_msg) (app.py:1555)
• explicit MCP_DEPLOYMENT_MODE wins over auto-detection, so the mode really is multi-user when set

Confirmed live: validate_configuration returns both Forbidden configuration: NEXTCLOUD_USERNAME/NEXTCLOUD_PASSWORD errors. That behaviour is now pinned by a test so it can't regress silently — and the backend's deny deliberately does not depend on it (test_multi_user_basic_denies_even_with_username_configured).

Test coverage

tests/unit/test_app_ui_basicauth_access.py — 4 tests. The two deny tests are verified to fail against the previous behaviour, so they pin the fix rather than restate it.

2786 unit tests pass under a CI-equivalent clean environment; ruff, ruff format, ty, secrets scan green.

Breaking change

Not flagged. Multi-user BasicAuth /app access is removed, but it was never functional in that mode — it served a static identity the mode forbids configuring. No /mcp behaviour changes, and single-user is untouched.

🤖 Generated with Claude Code

---

This PR was generated with the help of AI, and reviewed by a Human

[claude]

Claude finished @cbcoutinho's task in 8m 18s —— View job

---

Claude's review

Todo list

• Gather context (read changed files, PR description)
• Review nextcloud_mcp_server/auth/session_backend.py changes
• Review nextcloud_mcp_server/app.py changes
• Review tests/unit/test_app_ui_basicauth_access.py
• Post review feedback

---

Summary

The core fix — deny /app in multi-user BasicAuth, warn in single-user — is correctly scoped and well-justified. oauth_enabled = mode == AuthMode.LOGIN_FLOW (app.py:1579) and the new multi_user_basic = mode == AuthMode.MULTI_USER_BASIC (app.py:3002) are mutually exclusive and together cover all three AuthMode values, so the new deny branch only fires where intended and doesn't touch OAuth/Login-Flow. The added tests genuinely pin the fix: test_multi_user_basic_denies_even_with_username_configured correctly checks the deny doesn't rely on the config-validator hard-fail layer as a safety net.

One thing worth verifying before merge, since it isn't covered by the new tests:

🟡 Possible unhandled exception instead of a clean deny (needs live verification)

nextcloud_mcp_server/auth/session_backend.py:88-94 — the new multi_user_basic branch returns None from authenticate(). That's new: previously authenticate() never returned None while oauth_enabled was False, so nothing downstream ever exercised the "not authenticated" branch of BasicAuth-mode /app routes.

Every route served under /app (user_info_html in auth/userinfo_routes.py:430, and the webhook/viz routes) is decorated with @requires("authenticated", redirect="oauth_login"). When unauthenticated, Starlette's requires calls request.url_for("oauth_login") to build the redirect. But:

• These routes live in browser_app = Starlette(routes=browser_routes) (app.py:2997), a separate Starlette instance mounted at /app via Mount("/app", app=browser_app) (app.py:3012).
• The named route oauth_login is registered only on the outer routes list (app.py:2915), never inside browser_routes (app.py:2938).

Starlette resolves url_for against scope["router"], which gets overwritten by whichever router last dispatched — i.e. browser_app's own router once the Mount hands off. If that's the case here, request.url_for("oauth_login") from inside browser_app would raise starlette.routing.NoMatchFound (no handler registered for it anywhere in app.py), surfacing as an unhandled 500 rather than a clean 401/403 — for every unauthenticated request to /app/* in multi-user BasicAuth mode, including the root /app/ page itself.

Practically the security property still holds (no 200 with admin access), but it's the wrong failure mode — noisy unhandled exceptions/500s instead of a controlled deny.

I wasn't able to execute a live reproduction in this review environment (no shell/interpreter execution permission here), so this needs to be verified directly: build the mcp container in multi-user-basic mode and hit an unauthenticated /app/ and /app/webhooks:

docker compose up --build -d mcp   # multi-user-basic profile
curl -i http://localhost:<port>/app/
curl -i http://localhost:<port>/app/webhooks

If either 500s, options include registering an exception handler for NoMatchFound (or a catch-all) on browser_app that returns 403, or otherwise short-circuiting before url_for is reached for this mode.

Fix this →

🟢 Test coverage nit

tests/unit/test_app_ui_basicauth_access.py exercises SessionAuthBackend.authenticate() directly against hand-built SimpleNamespace connections — good for pinning the backend logic in isolation, but there's no test that builds the real app via get_app() in multi_user_basic mode and drives an actual TestClient request through the Mount. That's exactly the level that would have caught (or ruled out) the concern above. Worth adding one such integration-style test alongside this fix.

🟢 Minor style note

app.py:3002 computes mode == AuthMode.MULTI_USER_BASIC inline at the call site, whereas the neighboring oauth_enabled derivation is hoisted into a named variable near the top of get_app() (app.py:1579). Not a blocker — just slightly less consistent with the existing pattern in this file.

Nice to have, confirmed correct

• The "why deny vs warn" reasoning (session UI has no session concept in per-request BasicAuth; single-user has exactly one identity by design) is sound and clearly documented in both the backend docstring and the PR body.
• The breaking-change call in the PR body correctly applies this repo's own gate: the removed codepath never served a legitimate purpose (a static, unconfigurable "admin" identity), so no deprecation window/BREAKING CHANGE: footer is warranted.
• I could not run ruff/ty/pytest or the pre-push-review skill in this environment (no command-execution permission for this review context) — the PR description states 2786 unit tests plus ruff/ty/format green, which I have not independently re-verified.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud