Refresh XSTS tokens rejected before NotAfter - #21
Conversation
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe transport now buffers request bodies, detects expired XSTS challenges, invalidates supported token sources, and retries once. Session token caching tracks invalidation generations and clears stale cached authorization data. ChangesXSTS token refresh
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Transport
participant TokenInvalidator
participant BaseTransport
Transport->>BaseTransport: send authenticated request with cached body
BaseTransport-->>Transport: return 401 token_expired response
Transport->>TokenInvalidator: InvalidateXSTSToken(rejected token)
Transport->>BaseTransport: retry authenticated request with refreshed token
BaseTransport-->>Transport: return retry response
🚥 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xal/sisu/session_unit_test.go (1)
170-209: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCombine into a table-driven test.
TestSessionInvalidatesRejectedXSTSTokenandTestSessionInvalidationPreservesReplacementXSTSTokenexercise the same behavior (InvalidateXSTSTokenwith a matching vs. non-matching token), differing only in the input token and expected outcome.As per coding guidelines, "Prefer table-driven tests when multiple cases exercise the same behavior; keep single-case tests straightforward when a table would add noise."
♻️ Proposed table-driven consolidation
func TestSessionInvalidateXSTSToken(t *testing.T) { tests := []struct { name string cached *xsts.Token invalidate *xsts.Token wantRemoved bool }{ { name: "removes rejected token", cached: &xsts.Token{Token: "rejected"}, invalidate: &xsts.Token{Token: "rejected"}, wantRemoved: true, }, { name: "preserves newer replacement", cached: &xsts.Token{Token: "replacement"}, invalidate: &xsts.Token{Token: "rejected"}, wantRemoved: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { session := (Config{}).New(staticMSATokenSource{}, nil) session.xsts[defaultRelyingParty] = tt.cached session.resp = &authorizationResponse{AuthorizationToken: tt.cached} invalidator, ok := any(session).(interface{ InvalidateXSTSToken(*xsts.Token) }) if !ok { t.Fatal("Session does not support XSTS token invalidation") } invalidator.InvalidateXSTSToken(tt.invalidate) _, cached := session.xsts[defaultRelyingParty] if cached == tt.wantRemoved { t.Fatalf("token cached = %t, want removed = %t", cached, tt.wantRemoved) } if (session.resp == nil) != tt.wantRemoved { t.Fatalf("resp cleared = %t, want removed = %t", session.resp == nil, tt.wantRemoved) } }) } }🤖 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 `@xal/sisu/session_unit_test.go` around lines 170 - 209, Combine TestSessionInvalidatesRejectedXSTSToken and TestSessionInvalidationPreservesReplacementXSTSToken into one table-driven TestSessionInvalidateXSTSToken test with subtests covering matching-token removal and non-matching-token preservation. Parameterize the cached token, invalidation token, and expected removal state, while retaining the interface check and assertions for both the relying-party cache and session response.Source: Coding guidelines
🤖 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 `@xal/sisu/session_unit_test.go`:
- Around line 170-209: Combine TestSessionInvalidatesRejectedXSTSToken and
TestSessionInvalidationPreservesReplacementXSTSToken into one table-driven
TestSessionInvalidateXSTSToken test with subtests covering matching-token
removal and non-matching-token preservation. Parameterize the cached token,
invalidation token, and expected removal state, while retaining the interface
check and assertions for both the relying-party cache and session response.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 554a9dac-a1b2-42af-9e4d-69c6e1a8cdeb
📒 Files selected for processing (5)
xal/nsal/transport.goxal/nsal/transport_test.goxal/sisu/session.goxal/sisu/session_unit_test.goxal/xsts/token_source.go
502bb29 to
60d1aad
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
60d1aad to
b51d917
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@xal/sisu/session.go`:
- Around line 251-283: Update Session.xstsToken to coalesce concurrent
acquisitions for each relyingParty by coordinating access around the in-flight
request, so only one request runs and waiters reuse its result. Preserve the
existing cached-token fast path, propagate the shared request’s success or error
to waiters, and retain the xstsGeneration check so invalidation still causes a
fresh acquisition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8389bd2f-481d-477c-b913-8f57902d9c73
📒 Files selected for processing (4)
xal/nsal/transport.goxal/nsal/transport_test.goxal/sisu/session.goxal/sisu/session_unit_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- xal/nsal/transport.go
b51d917 to
f0a174d
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
f0a174d to
4a63800
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
4a63800 to
2f5697b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
2f5697b to
eb5a2a3
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What changed
401response explicitly reportserror='token_expired'401responses and requests with caller-provided authorization untouchedThis is implemented in the shared NSAL transport, so MPSD, profile, social, presence, and other authenticated Xbox API clients get the same recovery behavior. Token sources that do not implement the optional invalidation interface retain the existing behavior.
Production evidence
These are sanitized excerpts from the
mcxboxbroadcastinvestigation onna-east-2; tokens, user hashes, and account identifiers are omitted.The locally cached XSTS token claimed it was valid for another ~3.5 hours:
Xbox rejected that same token on two independently authenticated services:
At the same time, a social endpoint accepted the same token:
That rules out a simple account-wide ban and shows why local
NotAftervalidation alone is insufficient: Xbox can invalidate an XSTS token early, and acceptance can differ by relying service.For the controlled confirmation, only
xboxLiveXstsTokenwas removed from a temporary copy of the Java broadcaster cache. The same Java build (145), with friend sync and notifications disabled, then performed a fresh SISU/XSTS exchange:No production cache or workload was changed during this diagnostic.
Verification
All passed. Regression coverage includes the observed multi-challenge header, body replay, re-signing, response-body closure, unrelated
401handling, and preservation of newer or concurrently acquired cached tokens.Summary by CodeRabbit
Bug Fixes
Tests