fix: tighter auth error handling + restrict X-GitHub-Token forwarding#490
fix: tighter auth error handling + restrict X-GitHub-Token forwarding#490rainxchzed merged 9 commits intomainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDebounce token-specific 401s before clearing credentials, add success notifications to reset debounce, extract bearer tokens in the network interceptor, remove injected CoroutineScope from client wiring, add a user-facing "releases temporarily unavailable" string and surface it via DetailsViewModel, and update docs describing passthrough header behavior. ChangesToken-aware 401 Debouncing
Release Unavailability Messaging
Sequence DiagramsequenceDiagram
participant Client
participant Interceptor as UnauthorizedInterceptor
participant State as AuthenticationStateImpl
participant Store as TokenStore
participant UI as ViewModel
Client->>Interceptor: HTTP request (Authorization: Bearer ...)
Interceptor->>Interceptor: extractBearerToken() -> tokenKey
alt 401 response
Interceptor->>State: notifySessionExpired(tokenKey)
activate State
State->>State: if no snapshot -> snapshot tokenKey + timestamp
State->>State: else if same tokenKey within 60s -> increment counter
alt counter >= 2
State->>Store: verify current token == failing tokenKey
alt token matches
State->>Store: clear token
State->>UI: emit sessionExpiredEvent
else token rotated
State->>State: reset counters
end
end
deactivate State
else non-401 response
Interceptor->>State: notifyRequestSucceeded(tokenKey)
activate State
State->>State: if tokenKey matches snapshot -> reset counters
deactivate State
end
Interceptor->>Client: return response
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AuthenticationStateImpl.kt (1)
31-33: ⚡ Quick winPrefix the new private state fields with
_for consistency.These mutable private state properties don't follow the repo's underscore-prefix convention. Renaming them to
_failingTokenSnapshot,_firstFailureAtMillis, and_consecutiveFailureswould match the rest of the Kotlin state surface.As per coding guidelines, "Use private underscore prefix for private state properties: _state, _events, _mutableState".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AuthenticationStateImpl.kt` around lines 31 - 33, Rename the three private mutable fields in AuthenticationStateImpl: change failingTokenSnapshot → _failingTokenSnapshot, firstFailureAtMillis → _firstFailureAtMillis, and consecutiveFailures → _consecutiveFailures; update every reference/usages (reads, writes, constructors, and any copy/serialization code) to use the new names so the class follows the private underscore-prefix convention (look for occurrences of failingTokenSnapshot, firstFailureAtMillis, and consecutiveFailures to update).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AuthenticationStateImpl.kt`:
- Around line 46-66: notifySessionExpired() currently reads
tokenStore.currentToken() inside the coroutine which misattributes late 401s to
the current token and never resets on successful responses; change its API to
accept the failing request's token snapshot (pass token.accessToken from the
request context where notifySessionExpired() is invoked), update the logic in
AuthenticationStateImpl to use that passed tokenKey instead of calling
tokenStore.currentToken(), clear/reset the streak only when a non-401 response
is observed for the same tokenKey, and rename the private mutable fields to use
the underscore-prefix convention (_failingTokenSnapshot, _firstFailureAtMillis,
_consecutiveFailures) while preserving existing behavior otherwise.
---
Nitpick comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AuthenticationStateImpl.kt`:
- Around line 31-33: Rename the three private mutable fields in
AuthenticationStateImpl: change failingTokenSnapshot → _failingTokenSnapshot,
firstFailureAtMillis → _firstFailureAtMillis, and consecutiveFailures →
_consecutiveFailures; update every reference/usages (reads, writes,
constructors, and any copy/serialization code) to use the new names so the class
follows the private underscore-prefix convention (look for occurrences of
failingTokenSnapshot, firstFailureAtMillis, and consecutiveFailures to update).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 65d780ce-079a-4501-b907-2110628e3763
📒 Files selected for processing (4)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AuthenticationStateImpl.ktcore/presentation/src/commonMain/composeResources/values/strings.xmlfeature/details/presentation/src/commonMain/kotlin/zed/rainxch/details/presentation/DetailsViewModel.kt
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 162: Clarify that the parenthetical "(only on the lazy-fetch DB-miss
path)" describes backend behavior, not client-side logic: update the sentence
mentioning `/v1/repo` to state that the client (BackendApiClient.getRepo /
TokenStore.currentToken) always sends the `X-GitHub-Token` header on requests,
and that the backend decides (on a lazy-fetch DB miss) whether to forward that
header to GitHub; make the client/backend boundary explicit so readers know
getRepo doesn't conditionally omit the header.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.kt`:
- Around line 173-181: The new comment in BackendApiClient.kt conflicts with the
existing `**/data/**/*.kt` guideline and the PR description: either update the
guideline and docs to match the current behavior (forward X_GITHUB_TOKEN_HEADER
to passthrough routes) or change the code to restrict forwarding to only
`/v1/search` and `/v1/search/explore`. To fix, choose one of the two options and
apply consistent changes: A) If keeping forwarding, update the comment in
BackendApiClient.kt, the PR description/commit message, and the
`**/data/**/*.kt` guideline to mention forwarding for repo/releases/readme/user
with the quota rationale, ensuring symbols referenced (getRepo, getReleases,
getReadme, getUser, currentUserGithubToken(), X_GITHUB_TOKEN_HEADER) remain
unchanged; or B) If restricting forwarding, remove or stop injecting
currentUserGithubToken()/X_GITHUB_TOKEN_HEADER from the methods getRepo,
getReleases, getReadme, and getUser (and any helper that adds the header),
update BackendApiClient.kt comment and PR description accordingly, and
run/update tests that expect header forwarding.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 918f43f4-8d4f-4ea1-bc1e-2ec94be3570e
📒 Files selected for processing (2)
CLAUDE.mdcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.kt
| // X-GitHub-Token is forwarded on every backend route that does a live | ||
| // GitHub passthrough (search, search/explore, repo lazy-fetch, releases, | ||
| // readme, user). Sending the header lets the backend make the upstream | ||
| // call under the user's own 5000/hr OAuth quota instead of the | ||
| // anonymous 60/hr-per-IP shared bucket — without it a popular repo's | ||
| // releases list can poison the backend's negative cache for 15 min on | ||
| // a single quota burst. DB-only routes (categories, topics, events, | ||
| // auth/device/*, badge) ignore the header and don't get it. | ||
|
|
There was a problem hiding this comment.
AI summary overstates scope of changes, and this comment entrenches a guideline conflict.
Two issues here:
1. Inconsistent summary — the AI summary claims token retrieval and X_GITHUB_TOKEN_HEADER injection were removed from getRepo, getReleases, getReadme, and getUser. The actual code shows those four methods are unchanged (no ~ markers on lines 182–255); all four still call currentUserGithubToken() and forward the header. The only change in this file is this comment block.
2. Guideline conflict made explicit — the new comment formally endorses forwarding the token to all passthrough routes, which directly conflicts with the **/data/**/*.kt coding guideline:
"Only send
X-GitHub-Tokenheader on/v1/searchand/v1/search/exploreendpoints; never on other endpoints or in logs"
This file also matches **/network/**/*.kt, whose guideline says the opposite (forward to /v1/repo, /v1/releases, /v1/readme, /v1/user). Before this change the conflict was implicit; the comment now documents the contradicting policy intentionally, yet the **/data/**/*.kt rule has not been updated. One of these two guidelines must be revised to reflect the agreed-upon policy. The PR description ("Stop leaking the user's PAT to non-search endpoints") also contradicts the commit message ("keep forwarding to passthrough routes") and the actual code, compounding the confusion.
Please reconcile the two guidelines — either update **/data/**/*.kt to align with the passthrough-route policy (adding the quota-management rationale), or restrict the code to search/search-explore only and remove the forwarding from getRepo, getReleases, getReadme, and getUser. As per coding guidelines: "**/data/**/*.kt: Only send X-GitHub-Token header on /v1/search and /v1/search/explore endpoints; never on other endpoints or in logs" and "**/network/**/*.kt: Forward X-GitHub-Token header to GitHub API passthrough routes …".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/BackendApiClient.kt`
around lines 173 - 181, The new comment in BackendApiClient.kt conflicts with
the existing `**/data/**/*.kt` guideline and the PR description: either update
the guideline and docs to match the current behavior (forward
X_GITHUB_TOKEN_HEADER to passthrough routes) or change the code to restrict
forwarding to only `/v1/search` and `/v1/search/explore`. To fix, choose one of
the two options and apply consistent changes: A) If keeping forwarding, update
the comment in BackendApiClient.kt, the PR description/commit message, and the
`**/data/**/*.kt` guideline to mention forwarding for repo/releases/readme/user
with the quota rationale, ensuring symbols referenced (getRepo, getReleases,
getReadme, getUser, currentUserGithubToken(), X_GITHUB_TOKEN_HEADER) remain
unchanged; or B) If restricting forwarding, remove or stop injecting
currentUserGithubToken()/X_GITHUB_TOKEN_HEADER from the methods getRepo,
getReleases, getReadme, and getUser (and any helper that adds the header),
update BackendApiClient.kt comment and PR description accordingly, and
run/update tests that expect header forwarding.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
AGENTS.md (1)
94-94: AGENTS.md documentation is accurate; the suggested improvement is optional.The X-GitHub-Token forwarding policy in AGENTS.md Line 94 is correct and matches the implementation in
BackendApiClient.kt— tokens are forwarded to/v1/search,/v1/search/explore,/v1/repo,/v1/releases,/v1/readme, and/v1/user./v1/auth/exchangedoes not exist in the codebase, so there is no current security risk.If you wish to add explicit defensive language about never forwarding the token on auth routes (to prevent future regression), the suggested diff is reasonable. However, this is a documentation enhancement rather than a correction of an existing error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` at line 94, AGENTS.md correctly documents X-GitHub-Token forwarding, but to avoid future regressions add a brief defensive sentence clarifying that auth routes never receive the token and reference the implementation points: mention BackendApiClient.currentUserGithubToken() (private, never logged) and AuthenticationStateImpl (debounces 401s) and explicitly list the passthrough routes (/v1/search, /v1/search/explore, /v1/repo/{owner}/{name}, /v1/releases/{owner}/{name}, /v1/readme/{owner}/{name}, /v1/user/{username}) and DB-only routes so readers know which endpoints must not receive the token; keep the change to documentation only.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@AGENTS.md`:
- Line 94: AGENTS.md correctly documents X-GitHub-Token forwarding, but to avoid
future regressions add a brief defensive sentence clarifying that auth routes
never receive the token and reference the implementation points: mention
BackendApiClient.currentUserGithubToken() (private, never logged) and
AuthenticationStateImpl (debounces 401s) and explicitly list the passthrough
routes (/v1/search, /v1/search/explore, /v1/repo/{owner}/{name},
/v1/releases/{owner}/{name}, /v1/readme/{owner}/{name}, /v1/user/{username}) and
DB-only routes so readers know which endpoints must not receive the token; keep
the change to documentation only.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/interceptor/UnauthorizedInterceptor.kt (1)
46-55:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBreak the streak on every non-401 response.
200..299is too narrow for the debounce contract. A401 -> 403/404/5xx -> 401sequence is not consecutive auth failure, but this branch still leaves the first 401 counted and can clear a valid session on the next 401. Call the reset hook for anystatusCode != 401.Suggested change
when { statusCode == 401 -> { plugin.scope.launch { plugin.authenticationState.notifySessionExpired(tokenKey) } } - statusCode in 200..299 -> { + else -> { plugin.scope.launch { plugin.authenticationState.notifyRequestSucceeded(tokenKey) } } }As per coding guidelines, "401 responses from passthrough routes do not indicate session expiration—AuthenticationStateImpl must debounce consecutive 401s under the same token before clearing the session."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/interceptor/UnauthorizedInterceptor.kt` around lines 46 - 55, The current branch only treats 200..299 as a "non-401" reset, which misses other non-401 responses and breaks the debounce logic; inside UnauthorizedInterceptor (around the when handling statusCode) change the logic so that statusCode == 401 triggers plugin.scope.launch { plugin.authenticationState.notifySessionExpired(tokenKey) } and any other statusCode (i.e., statusCode != 401) triggers plugin.scope.launch { plugin.authenticationState.notifyRequestSucceeded(tokenKey) } (or the reset hook used to clear the 401 streak), ensuring every non-401 response resets the consecutive-401 debounce for the given tokenKey.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/interceptor/UnauthorizedInterceptor.kt`:
- Around line 47-55: The current UnauthorizedInterceptor uses
plugin.scope.launch to call
plugin.authenticationState.notifySessionExpired(tokenKey) and
notifyRequestSucceeded(tokenKey), which allows those updates to run
out-of-order; remove the fire-and-forget launches and invoke
plugin.authenticationState.notifySessionExpired(tokenKey) and
plugin.authenticationState.notifyRequestSucceeded(tokenKey) directly (inline) in
the suspend pipeline so the auth-state methods execute in response order instead
of being scheduled on plugin.scope.
---
Duplicate comments:
In
`@core/data/src/commonMain/kotlin/zed/rainxch/core/data/network/interceptor/UnauthorizedInterceptor.kt`:
- Around line 46-55: The current branch only treats 200..299 as a "non-401"
reset, which misses other non-401 responses and breaks the debounce logic;
inside UnauthorizedInterceptor (around the when handling statusCode) change the
logic so that statusCode == 401 triggers plugin.scope.launch {
plugin.authenticationState.notifySessionExpired(tokenKey) } and any other
statusCode (i.e., statusCode != 401) triggers plugin.scope.launch {
plugin.authenticationState.notifyRequestSucceeded(tokenKey) } (or the reset hook
used to clear the 401 streak), ensuring every non-401 response resets the
consecutive-401 debounce for the given tokenKey.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2bff8eeb-f272-4f32-8aa4-fbd65adaa94d
📒 Files selected for processing (4)
CLAUDE.mdcore/data/src/commonMain/kotlin/zed/rainxch/core/data/network/interceptor/UnauthorizedInterceptor.ktcore/data/src/commonMain/kotlin/zed/rainxch/core/data/repository/AuthenticationStateImpl.ktcore/domain/src/commonMain/kotlin/zed/rainxch/core/domain/repository/AuthenticationState.kt
1 participant
Stack of three fixes for the "Retry failed to load releases: HTTP 403" + spurious "session expired" prompts the user reported. Backend-side investigation in flight; everything here is safe to ship regardless of what backend confirms.
A — Stop leaking the user's PAT to non-search backend endpoints
BackendApiClientwas sending the user's GitHub token viaX-GitHub-Tokenon/v1/repo,/v1/releases,/v1/readme,/v1/user, and/v1/auth/exchange, even though the project notes explicitly scope the header to/v1/searchand/v1/search/explore. The other endpoints don't accept it server-side, so we were leaking the PAT for no upside. This change strips it from every endpoint the spec doesn't list and adds a comment so it doesn't drift again.B — Don't sign the user out on a single 401
AuthenticationStateImpl.notifySessionExpiredused to clear the token and emit the session-expired dialog on the very first 401 reported byUnauthorizedInterceptor. That nukes a still-valid token whenever the 401 comes from:Now it tracks consecutive 401s under the same token within a 60-second window. Sign-out only fires on the second hit. Different token (re-auth landed) or a long gap resets the streak. Logs the deferred-vs-cleared decision so we can audit if it ever feels too lenient.
C — Friendlier message when retrying releases fails
DetailsViewModel.retryReleasesused to bury the cause in alogger.warnand leave the UI on a generic "failed" card. The user sees"HTTP 403"in logs which reads like a real error. Now we keep the diagnostic log (cause stays for support), and emit aDetailsEvent.OnMessagewith the newreleases_unavailable_temporarilystring so the snackbar tells the user what's actually happening: the list will come back on its own.Test plan
HTTP 403line./v1/repo,/v1/releases,/v1/readme,/v1/user,/v1/auth/exchangeshows noX-GitHub-Tokenheader./v1/searchand/v1/search/explorestill send it.Summary by CodeRabbit
Bug Fixes
Improvements
Documentation