Repository navigation
fix(oauth): add opt-in bounded grace period for refresh-token rotation - #387
kotovasia5120 wants to merge 1 commit into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (9)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughAdds an OAuth refresh-token grace setting from 0 to 600 seconds. The provider passes it to the SQLite store, which applies the grace deadline to consumed refresh tokens. Configuration and token behavior are covered by tests and documentation. ChangesOAuth Refresh-Token Grace Period
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant Client
participant SingleUserOAuthProvider
participant SqliteOAuthStore
Client->>SingleUserOAuthProvider: Submit refresh token
SingleUserOAuthProvider->>SqliteOAuthStore: Save token pair with consumed token hash
SqliteOAuthStore->>SqliteOAuthStore: Apply grace deadline to consumed token
SingleUserOAuthProvider->>Client: Return token pair
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This adds an opt-in, bounded refresh-token grace period that is off by default, so existing deployments behave as before. No blocking issues were found, and the replay tradeoff is documented. Security Architecture ReviewSecurity architecture risk: 🟡 Moderate · up to The feature deliberately lets a previously rotated credential mint additional tokens. Those replacement credentials can remain usable beyond the retry window. The setting is disabled by default, and client, resource, scope, and atomic rotation controls remain intact. The main risk is the accepted replay tradeoff when an operator enables it, rather than an established authorization bypass. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 7 files. (2 skipped: 2 unsupported.)
✨ 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. A rabbit checks the tokens’ time, Comment |
|
| const [leftTokens, rightTokens] = await Promise.all([ | ||
| left.exchangeRefreshToken(client, tokens.refresh_token), | ||
| right.exchangeRefreshToken(client, tokens.refresh_token), | ||
| ]); |
There was a problem hiding this comment.
Both refresh calls complete their synchronous SQLite work one after the other, despite being passed to Promise.all. The test checks reuse through two connections, but cannot catch regressions that appear only when rotations contend for a database write lock. This test-coverage gap is non-blocking.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
Executable refresh-rotation comparison script
- The authored script runs the provider calls with same-event-loop tracing and with an independent process holding a SQLite write lock, enabling a direct comparison.
Original refresh-grace test passing
- The repository's refresh-grace test was run unchanged and passed, showing that its assertions do not require overlapping rotations.
Same-event-loop refresh calls running sequentially
- Two provider calls in the test's `Promise.all` structure produced distinct tokens, while the trace showed the first transaction exited before the second read began.
Independent-process refresh calls encountering a SQLite lock
- A separate process held an uncommitted rotation while the second call waited 914 ms for its write lock, showing contention absent from the original test.
Concurrent refreshes or a retry after a lost rotation response can reuse the previous refresh token. DevSpace currently deletes that token immediately, so the retry receives
invalid_grantand a host can fall back to the owner-password authorization flow even though the original token TTL has not elapsed.This adds opt-in
oauth.refreshTokenGraceSeconds(0–600, default 0). When enabled, rotation atomically shortens the consumed token's expiry to the earlier of its existing expiry and the grace deadline. Subsequent retries cannot move that deadline, and each newly issued refresh token keeps the configured TTL. The default retains immediate consumption; default access/refresh TTLs, client/resource/scope checks, and explicit token revocation are unchanged. No database migration is needed. Enabling grace deliberately permits replay of a rotated token within the selected window, so the option is documented as a compatibility tradeoff for trusted hosts.Behavioral regression tests use real SQLite and a controlled clock for duplicate refreshes through two providers, the fixed 600-second boundary, shorter original lifetimes, persistence after all connections close, revocation, and client/resource/scope restrictions. Configuration validation, generated JSON schema, and configuration loading are covered as well.
The focused OAuth/config tests,
pnpm typecheck,pnpm build, and the fullpnpm testsuite passed locally on Windows (142 passed, 7 skipped). The initial full run hit two existing workspace assertions comparingC:\Tempwith Windows' canonicalC:\temp; aligning this process'sTEMP/TMPwith that canonical path made both the focused workspace tests and the full suite pass without workspace code changes. Platform-specific tests were skipped, and Pi sandbox integration was unavailable in this environment.A private deployment using an equivalent fixed 600-second patch also completed a subsequent ChatGPT refresh without another owner-password prompt; that observation is separate from verification of this upstream implementation.
Summary by CodeRabbit