fix(auth): address PR #758 follow-up review
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>
This commit is contained in:
Chris Coutinho
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
af25c281bf
commit
2d340a5a6b
@@ -244,6 +244,197 @@ async def test_verify_id_token_missing_token_rejected():
|
||||
)
|
||||
|
||||
|
||||
async def test_verify_id_token_recovers_after_kid_rotation():
|
||||
"""Unknown kid → JWKS is refetched once and verification succeeds.
|
||||
|
||||
Pins the fix for the PR #758 follow-up review: previously a kid-miss
|
||||
raised immediately, so every login failed for up to _OIDC_CACHE_TTL
|
||||
after the IdP rotated its signing key.
|
||||
"""
|
||||
rotated_key = rsa.generate_private_key(public_exponent=65537, key_size=2048)
|
||||
rotated_pem = rotated_key.private_bytes(
|
||||
encoding=serialization.Encoding.PEM,
|
||||
format=serialization.PrivateFormat.TraditionalOpenSSL,
|
||||
encryption_algorithm=serialization.NoEncryption(),
|
||||
)
|
||||
|
||||
def _build_rotated_jwks() -> dict:
|
||||
pub = rotated_key.public_key().public_numbers()
|
||||
return {
|
||||
"keys": [
|
||||
{
|
||||
"kty": "RSA",
|
||||
"use": "sig",
|
||||
"kid": "rotated-key",
|
||||
"alg": "RS256",
|
||||
"n": _b64u_uint(pub.n),
|
||||
"e": _b64u_uint(pub.e),
|
||||
}
|
||||
]
|
||||
}
|
||||
|
||||
jwks_fetches = {"count": 0}
|
||||
|
||||
def rotation_handler(request: httpx.Request) -> httpx.Response:
|
||||
url = str(request.url)
|
||||
if url == DISCOVERY_URL:
|
||||
return httpx.Response(200, json={"issuer": ISSUER, "jwks_uri": JWKS_URI})
|
||||
if url == JWKS_URI:
|
||||
jwks_fetches["count"] += 1
|
||||
# First fetch: stale JWKS (without rotated kid).
|
||||
# Subsequent fetches: post-rotation JWKS (with rotated kid).
|
||||
jwks = (
|
||||
_build_jwks() if jwks_fetches["count"] == 1 else _build_rotated_jwks()
|
||||
)
|
||||
return httpx.Response(
|
||||
200,
|
||||
content=json.dumps(jwks).encode(),
|
||||
headers={"content-type": "application/json"},
|
||||
)
|
||||
return httpx.Response(404)
|
||||
|
||||
transport = httpx.MockTransport(rotation_handler)
|
||||
|
||||
def fake_client(**kwargs):
|
||||
kwargs["transport"] = transport
|
||||
return httpx.AsyncClient(**kwargs)
|
||||
|
||||
now = int(time.time())
|
||||
token = jwt.encode(
|
||||
{
|
||||
"iss": ISSUER,
|
||||
"aud": "test-client",
|
||||
"sub": "alice",
|
||||
"iat": now,
|
||||
"exp": now + 60,
|
||||
},
|
||||
rotated_pem,
|
||||
algorithm="RS256",
|
||||
headers={"kid": "rotated-key"},
|
||||
)
|
||||
|
||||
with patch(
|
||||
"nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client",
|
||||
side_effect=fake_client,
|
||||
):
|
||||
# Prime the cache with the stale JWKS by triggering a verification
|
||||
# that misses on the rotated kid.
|
||||
payload = await verify_id_token(
|
||||
token, discovery_url=DISCOVERY_URL, expected_audience="test-client"
|
||||
)
|
||||
|
||||
assert payload["sub"] == "alice"
|
||||
assert jwks_fetches["count"] == 2, (
|
||||
"JWKS should be refetched once on kid miss "
|
||||
f"(actual fetches: {jwks_fetches['count']})"
|
||||
)
|
||||
|
||||
|
||||
async def test_verify_id_token_rotation_retry_still_misses():
|
||||
"""Refresh that still doesn't include the kid surfaces the original error."""
|
||||
fetches = {"count": 0}
|
||||
|
||||
def handler(request: httpx.Request) -> httpx.Response:
|
||||
url = str(request.url)
|
||||
if url == DISCOVERY_URL:
|
||||
return httpx.Response(200, json={"issuer": ISSUER, "jwks_uri": JWKS_URI})
|
||||
if url == JWKS_URI:
|
||||
fetches["count"] += 1
|
||||
return httpx.Response(
|
||||
200,
|
||||
content=json.dumps(_build_jwks()).encode(),
|
||||
headers={"content-type": "application/json"},
|
||||
)
|
||||
return httpx.Response(404)
|
||||
|
||||
transport = httpx.MockTransport(handler)
|
||||
|
||||
def fake_client(**kwargs):
|
||||
kwargs["transport"] = transport
|
||||
return httpx.AsyncClient(**kwargs)
|
||||
|
||||
now = int(time.time())
|
||||
token = _sign(
|
||||
{
|
||||
"iss": ISSUER,
|
||||
"aud": "test-client",
|
||||
"sub": "alice",
|
||||
"iat": now,
|
||||
"exp": now + 60,
|
||||
},
|
||||
kid="never-existed",
|
||||
)
|
||||
|
||||
with patch(
|
||||
"nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client",
|
||||
side_effect=fake_client,
|
||||
):
|
||||
with pytest.raises(IdTokenVerificationError, match="No JWKS key matches"):
|
||||
await verify_id_token(
|
||||
token, discovery_url=DISCOVERY_URL, expected_audience="test-client"
|
||||
)
|
||||
|
||||
assert fetches["count"] == 2, "JWKS should be refetched once before raising"
|
||||
|
||||
|
||||
async def test_verify_id_token_rotation_retry_network_error_wraps():
|
||||
"""A 500 on the kid-miss refresh fetch surfaces as IdTokenVerificationError.
|
||||
|
||||
Pins the fail-closed branch in the new refresh block: a network error
|
||||
during JWKS refetch must not bubble out as a bare exception — it has
|
||||
to be wrapped in IdTokenVerificationError so the caller's existing
|
||||
error handling stays correct.
|
||||
"""
|
||||
fetches = {"jwks": 0}
|
||||
|
||||
def handler(request: httpx.Request) -> httpx.Response:
|
||||
url = str(request.url)
|
||||
if url == DISCOVERY_URL:
|
||||
return httpx.Response(200, json={"issuer": ISSUER, "jwks_uri": JWKS_URI})
|
||||
if url == JWKS_URI:
|
||||
fetches["jwks"] += 1
|
||||
# First fetch: stale-but-valid JWKS. Second (refresh): 500.
|
||||
if fetches["jwks"] == 1:
|
||||
return httpx.Response(
|
||||
200,
|
||||
content=json.dumps(_build_jwks()).encode(),
|
||||
headers={"content-type": "application/json"},
|
||||
)
|
||||
return httpx.Response(500, content=b"upstream broke")
|
||||
return httpx.Response(404)
|
||||
|
||||
transport = httpx.MockTransport(handler)
|
||||
|
||||
def fake_client(**kwargs):
|
||||
kwargs["transport"] = transport
|
||||
return httpx.AsyncClient(**kwargs)
|
||||
|
||||
now = int(time.time())
|
||||
token = _sign(
|
||||
{
|
||||
"iss": ISSUER,
|
||||
"aud": "test-client",
|
||||
"sub": "alice",
|
||||
"iat": now,
|
||||
"exp": now + 60,
|
||||
},
|
||||
kid="not-cached-yet",
|
||||
)
|
||||
|
||||
with patch(
|
||||
"nextcloud_mcp_server.auth.token_utils.nextcloud_httpx_client",
|
||||
side_effect=fake_client,
|
||||
):
|
||||
with pytest.raises(
|
||||
IdTokenVerificationError, match="Failed to refresh JWKS after kid miss"
|
||||
):
|
||||
await verify_id_token(
|
||||
token, discovery_url=DISCOVERY_URL, expected_audience="test-client"
|
||||
)
|
||||
|
||||
assert fetches["jwks"] == 2
|
||||
|
||||
|
||||
async def test_verify_id_token_caches_discovery_and_jwks():
|
||||
"""Discovery + JWKS must be cached: two verifications, one fetch each.
|
||||
|
||||
|
||||
@@ -192,6 +192,74 @@ async def test_logout_allows_same_origin_post(storage):
|
||||
assert await storage.get_browser_session_user("sid-Y") is None
|
||||
|
||||
|
||||
async def test_logout_allows_same_origin_post_with_explicit_default_port(storage):
|
||||
"""mcp_server_url has explicit :443; browser Origin omits the port.
|
||||
|
||||
RFC 6454 §6.2: browsers omit default ports in Origin headers. The
|
||||
netloc string ``mcp.example.com:443`` would never match ``mcp.example.com``
|
||||
without port normalisation, blocking every legitimate logout.
|
||||
"""
|
||||
await storage.create_browser_session(session_id="sid-PE", user_id="alice")
|
||||
|
||||
request = _build_request(
|
||||
cookie="sid-PE",
|
||||
oauth_context={
|
||||
"storage": storage,
|
||||
"config": {
|
||||
"mcp_server_url": "https://mcp.example.com:443",
|
||||
"discovery_url": None,
|
||||
},
|
||||
},
|
||||
headers={"origin": "https://mcp.example.com"},
|
||||
)
|
||||
|
||||
response = await oauth_logout(request)
|
||||
assert response.status_code == 302
|
||||
assert await storage.get_browser_session_user("sid-PE") is None
|
||||
|
||||
|
||||
async def test_logout_allows_same_origin_post_with_default_port_in_origin(storage):
|
||||
"""Symmetric case: config omits port, Origin includes :443."""
|
||||
await storage.create_browser_session(session_id="sid-PI", user_id="alice")
|
||||
|
||||
request = _build_request(
|
||||
cookie="sid-PI",
|
||||
oauth_context={
|
||||
"storage": storage,
|
||||
"config": {
|
||||
"mcp_server_url": "https://mcp.example.com",
|
||||
"discovery_url": None,
|
||||
},
|
||||
},
|
||||
headers={"origin": "https://mcp.example.com:443"},
|
||||
)
|
||||
|
||||
response = await oauth_logout(request)
|
||||
assert response.status_code == 302
|
||||
assert await storage.get_browser_session_user("sid-PI") is None
|
||||
|
||||
|
||||
async def test_logout_blocks_scheme_mismatch(storage):
|
||||
"""Same hostname but different scheme must be treated as cross-origin."""
|
||||
await storage.create_browser_session(session_id="sid-SC", user_id="alice")
|
||||
|
||||
request = _build_request(
|
||||
cookie="sid-SC",
|
||||
oauth_context={
|
||||
"storage": storage,
|
||||
"config": {
|
||||
"mcp_server_url": "https://mcp.example.com",
|
||||
"discovery_url": None,
|
||||
},
|
||||
},
|
||||
headers={"origin": "http://mcp.example.com"},
|
||||
)
|
||||
|
||||
response = await oauth_logout(request)
|
||||
assert response.status_code == 403
|
||||
assert await storage.get_browser_session_user("sid-SC") == "alice"
|
||||
|
||||
|
||||
async def test_logout_allows_referer_when_origin_missing(storage):
|
||||
"""Some browsers strip Origin on POST; Referer is the fallback signal."""
|
||||
await storage.create_browser_session(session_id="sid-Z", user_id="alice")
|
||||
|
||||
Reference in New Issue
Block a user