Fix entiredb token-refresh: doc, caller context, and proxy#90
Closed
Soph wants to merge 1 commit into
Closed
Conversation
Three issues in the Entire DB token path:
- getTokenWithRefresh's doc comment claimed it returns the stale token with a
nil error on refresh failure; it actually surfaces the error (issue #7).
Corrected the comment to match the code.
- The refresh ran on context.Background(), so a cancelled sync couldn't abort
an in-flight refresh. Thread the caller's context from newConn → auth.Resolve
→ LookupEntireDBCredential → lookupEntireDBToken → getTokenWithRefresh.
- refreshAccessToken built a bare &http.Transport{}, which leaves Proxy nil
and ignores HTTP(S)_PROXY/NO_PROXY. Set Proxy: http.ProxyFromEnvironment to
match the default transport.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Entire-Checkpoint: d98ef5fdf952
nodo
approved these changes
Jun 18, 2026
Contributor
Author
|
Superseded by #95, which removes the Entire DB credential-store integration entirely. mirror-pipeline (the only library consumer talking to entiredb) authenticates at the transport layer and never uses this path, and client-side entire:// auth is handled by git-remote-entire's contexts.json model — so the code this PR fixes has no live producer of the credentials it reads. Closing in favor of the removal. |
Soph
added a commit
that referenced
this pull request
Jun 18, 2026
git-sync carried a bespoke Entire credential path: auth.Resolve fell back to an active-user lookup in ~/.config/entire/hosts.json plus a file/keyring token store and OAuth refresh-token handling (client_id=entire-cli). Nothing in the product produces that layout anymore — the mirror-pipeline worker (the only library consumer that talks to entiredb) supplies credentials directly at the transport layer (GitHub installation tokens + per-request entire-core repo-scoped bearers), and client-side entire:// auth is owned by the separate git-remote-entire helper using the newer contexts.json model. So the lookup only ever read a store no current producer writes. Drop entiredb.go and tokenstore.go and the LookupEntireDBCredential fallback; auth.Resolve now resolves explicit token/bearer credentials only and otherwise returns nil so the git credential helper is consulted on a 401, exactly as for any other remote. This also drops the github.com/zalando/go-keyring dependency and, with the file token store gone, removes the syscall.Flock usage that broke the Windows build (so the package now cross-compiles for windows cleanly). Supersedes the entiredb token-refresh fix (#90) and the tokenstore Windows flock fix (#92), both of which were polishing this now-deleted code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Entire-Checkpoint: de15fe82f1f3
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problems
Three issues in the Entire DB token path (
internal/auth/entiredb.go):getTokenWithRefresh's comment claimed it "returns the stale token with a nil error rather than propagating the refresh error" — the opposite of what the code does (it surfaces the error, per issue Add ci and lint move to camelCase JSON #7). Corrected to match.context.Background(), so a cancelled sync (Ctrl-C) couldn't abort an in-flight token refresh until the 30s client timeout. Thread the caller's context:newConn → auth.Resolve → LookupEntireDBCredential → lookupEntireDBToken → getTokenWithRefresh.&http.Transport{}leavesProxynil, soHTTP(S)_PROXY/NO_PROXYwere ignored for the refresh request (unlike the default transport the git transport uses). SetProxy: http.ProxyFromEnvironment.Scope
auth.Resolve,LookupEntireDBCredential,lookupEntireDBToken, andnewConngain a leadingctxparameter; all in-module call sites (one production caller each, plussha256convert'sopenSourcewhich already has actx, and the test call sites) are updated.getTokenWithRefresh/refreshAccessTokenalready tookctx.Tests
TestGetTokenWithRefreshHonorsContextstores an expired token + refresh token and a cancelled context, then asserts the refresh fails with an error wrappingcontext.Canceled— proving the context reaches the HTTP request. Fullgo test ./...passes.🤖 Generated with Claude Code
Note
Medium Risk
Touches authentication and token refresh on the sync path; behavior changes (cancellation, proxy) are intentional but affect credential resolution in proxied or interrupted runs.
Overview
Fixes three issues on the Entire DB OAuth token path used when resolving HTTP auth.
Caller context is threaded from sync/
convert-sha256throughnewConn→auth.Resolve→LookupEntireDBCredential→getTokenWithRefresh, replacingcontext.Background()on refresh so a cancelled run (e.g. Ctrl-C) can abort in-flight refresh instead of waiting on the 30s HTTP client timeout.The token refresh HTTP client now sets
Proxy: http.ProxyFromEnvironmentsoHTTP(S)_PROXY/NO_PROXYmatch default git transport behavior (a bare&http.Transport{}skipped the proxy).getTokenWithRefresh’s doc comment is corrected to state refresh failures are returned to the caller (issue #7), not silently returning a stale token.Adds
TestGetTokenWithRefreshHonorsContextto assert a pre-cancelled context yieldscontext.Canceledfrom refresh.Reviewed by Cursor Bugbot for commit 61ba091. Configure here.