Chris CoutinhoandClaude Opus 4.7
931ee602eb
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>
45 lines
1.5 KiB
Python
45 lines
1.5 KiB
Python
"""Tests for _safe_next_url, the open-redirect guard for ``?next=`` params.
|
|
|
|
Pins the contract that any non-path target falls back to the default,
|
|
preventing the open-redirect issue flagged on PR #758.
|
|
"""
|
|
|
|
import pytest
|
|
|
|
from nextcloud_mcp_server.auth.browser_oauth_routes import _safe_next_url
|
|
|
|
pytestmark = pytest.mark.unit
|
|
|
|
|
|
@pytest.mark.parametrize(
|
|
"raw, expected",
|
|
[
|
|
# Valid path-only targets pass through.
|
|
("/app", "/app"),
|
|
("/app/foo", "/app/foo"),
|
|
("/oauth/login", "/oauth/login"),
|
|
("/app?x=1&y=2", "/app?x=1&y=2"),
|
|
("/app#frag", "/app#frag"),
|
|
# Empty / missing → default.
|
|
("", "/default"),
|
|
(None, "/default"),
|
|
# Absolute URLs → default.
|
|
("https://evil.example.com", "/default"),
|
|
("http://evil.example.com/path", "/default"),
|
|
# Protocol-relative → default. Browser would treat as cross-origin.
|
|
("//evil.example.com", "/default"),
|
|
("//evil.example.com/path", "/default"),
|
|
# No leading slash → default.
|
|
("relative/path", "/default"),
|
|
("app", "/default"),
|
|
# Whitespace / control chars → default. Defends against tab/space
|
|
# injection that some browsers historically tolerated.
|
|
("/app\nfoo", "/default"),
|
|
("/app\tfoo", "/default"),
|
|
("/app\x00foo", "/default"),
|
|
("/app foo", "/default"),
|
|
],
|
|
)
|
|
def test_safe_next_url(raw, expected):
|
|
assert _safe_next_url(raw, "/default") == expected
|