fix: prevent transient Plane API failures from invalidating OAuth sessions - #187
fix: prevent transient Plane API failures from invalidating OAuth sessions#187akhil-vamshi-konam wants to merge 3 commits into
Conversation
…h token verification
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughPlane OAuth token verification now includes bounded SHA-256-keyed caching, stale-if-error retention, one retry for transient failures, definitive rejection handling, injectable transport, and stricter upstream response validation. ChangesOAuth verification
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthClient
participant PlaneOAuthTokenVerifier
participant VerificationCache
participant PlaneAPI
OAuthClient->>PlaneOAuthTokenVerifier: verify_token
PlaneOAuthTokenVerifier->>VerificationCache: read token result
VerificationCache-->>PlaneOAuthTokenVerifier: fresh result or miss
PlaneOAuthTokenVerifier->>PlaneAPI: verify user and installation
PlaneAPI-->>PlaneOAuthTokenVerifier: success or failure
PlaneOAuthTokenVerifier->>VerificationCache: store successful verification
PlaneOAuthTokenVerifier-->>OAuthClient: AccessToken, stale result, or rejection
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
plane_mcp/auth/plane_oauth_provider.py (2)
102-124: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer
time.monotonic()overtime.time()for TTL/age measurement.
get,put, and_evictall measure elapsed time with the wall clock. An NTP correction or manual clock change can jumptime.time()backward, making cached entries appear fresher than intended and serving a (possibly already-revoked) token pastcache_ttl/stale_ttl; a forward jump silently expires the cache. Use a monotonic clock for elapsed-time comparisons.♻️ Use monotonic clock for age/TTL
- def get(self, token: str, *, max_age_seconds: float) -> AccessToken | None: - entry = self._entries.get(self._key(token)) - if entry and (time.time() - entry.verified_at) <= max_age_seconds: + def get(self, token: str, *, max_age_seconds: float) -> AccessToken | None: + entry = self._entries.get(self._key(token)) + if entry and (time.monotonic() - entry.verified_at) <= max_age_seconds: return entry.access_token return None def put(self, token: str, access_token: AccessToken) -> None: if len(self._entries) >= self._max_entries: self._evict() - self._entries[self._key(token)] = _CacheEntry(access_token=access_token, verified_at=time.time()) + self._entries[self._key(token)] = _CacheEntry(access_token=access_token, verified_at=time.monotonic()) @@ def _evict(self) -> None: # Drop entries past the stale window first, then the oldest. - now = time.time() + now = time.monotonic()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plane_mcp/auth/plane_oauth_provider.py` around lines 102 - 124, Replace wall-clock usage with time.monotonic() for cache age tracking throughout the token cache: update CacheEntry timestamps in put and use monotonic values in get and _evict comparisons. Keep the existing TTL, stale-entry removal, and oldest-entry eviction behavior unchanged.
234-269: 🚀 Performance & Scalability | 🔵 TrivialConsider single-flight coalescing on the cache-miss path.
The stated goal is that "a burst of MCP requests must not become a burst of Plane API calls," but that only holds once an entry is cached. On a cold cache or right after
cache_ttlexpiry, concurrentverify_tokencalls for the same token each execute_verify_upstream(and, on transient failure, each retry) since nothing coalesces in-flight verifications. Under load this can still fan out to Plane. Coalescing per-token verifications (e.g. an in-flightasyncio.Future/lock keyed by the token hash) would fully realize the burst-suppression intent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plane_mcp/auth/plane_oauth_provider.py` around lines 234 - 269, Update verify_token to coalesce concurrent cache-miss verifications per token, using an in-flight Future or lock keyed by a token hash so only one caller runs _verify_upstream and its transient-failure retry sequence while others await the same result. Preserve existing cache hits, stale-cache fallback, rejection handling, and cleanup of in-flight state after completion or failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plane_mcp/auth/plane_oauth_provider.py`:
- Around line 102-124: Replace wall-clock usage with time.monotonic() for cache
age tracking throughout the token cache: update CacheEntry timestamps in put and
use monotonic values in get and _evict comparisons. Keep the existing TTL,
stale-entry removal, and oldest-entry eviction behavior unchanged.
- Around line 234-269: Update verify_token to coalesce concurrent cache-miss
verifications per token, using an in-flight Future or lock keyed by a token hash
so only one caller runs _verify_upstream and its transient-failure retry
sequence while others await the same result. Preserve existing cache hits,
stale-cache fallback, rejection handling, and cleanup of in-flight state after
completion or failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6da7d9fa-dad5-4587-b7a4-6b3769e537ca
📒 Files selected for processing (1)
plane_mcp/auth/plane_oauth_provider.py
…ing else is useful from change from 3.2.0 to 3.4.4.?
… to prevent unintended mutations
1 participant
Description
Problem
The MCP connector intermittently drops to "needs authentication", forcing users through a full browser re-auth even though their token is still valid. Two mechanisms cause this:
PlaneOAuthTokenVerifier.verify_tokenruns on every MCP request (two live calls to the Plane API) and collapses every failure intoreturn None, which FastMCP treats as "token invalid" → HTTP 401 → the client discards its session. A 10s timeout, a 502 during an API deploy, a 429, or an unparseable response from/auth/o/app-installation/(whose status code was never checked) all logged users out. Since verification runs per-request with no caching, a single short Plane API blip disconnected every active user at once.invalid_grant— which tells the client to discard all tokens and force interactive re-auth.Fix 1 — verifier: classify failures instead of collapsing them
None→ 401, and the cache entry is purged so a revoked token can never be served again.TransientVerificationError, never "token invalid".On transient failure, the last successful verification is served from cache (stale window, default 15 min); with no cache available, one retry before giving up.
Successful verifications are cached (default 60s, sha256-keyed, bounded at 1024 entries) — a burst of MCP requests no longer becomes a burst of Plane API calls (~99% fewer verification calls), and worst-case revocation latency equals the cache TTL.
Both windows are env-tunable:
PLANE_VERIFY_CACHE_TTL_SECONDS,PLANE_VERIFY_STALE_TTL_SECONDS. Setting both to0restores today's always-revalidate behavior (plus the retry) — a rollback lever without a redeploy.Fix 2 — fastmcp 3.2.0 → 3.4.4 + proactive refresh
token_expiry_threshold_seconds(new in 3.4.0, wired here, default 300): the proxy refreshes the upstream Plane token 5 minutes before expiry, so sessions no longer race the refresh at the boundary. Env:PLANE_OAUTH_PROVIDER_TOKEN_EXPIRY_THRESHOLD_SECONDS.access_token_expiry_seconds(optional, default unset = current behavior): decouples the FastMCP-issued token lifetime from the upstreamexpires_in, so clients refresh rarely. Safe by design — the FastMCP JWT is a reference token; the upstream token is still validated on every request, so revocation is unaffected. Env:PLANE_OAUTH_PROVIDER_ACCESS_TOKEN_EXPIRY_SECONDS.invalid_redirect_uri) instead of accepting registration and blocking at/authorize— the same attack, blocked one step earlier.
test_oauth_security.pynow assers URI never receives a redirect) at whichever stage it is enforced, and passeson both old and new fastmcp.Type of Change