Skip to content

Fix entiredb token-refresh: doc, caller context, and proxy#90

Closed
Soph wants to merge 1 commit into
mainfrom
fix/entiredb-token-refresh-ctx-proxy
Closed

Fix entiredb token-refresh: doc, caller context, and proxy#90
Soph wants to merge 1 commit into
mainfrom
fix/entiredb-token-refresh-ctx-proxy

Conversation

@Soph

@Soph Soph commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Problems

Three issues in the Entire DB token path (internal/auth/entiredb.go):

  1. Doc comment was backwards. 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.
  2. Refresh ignored the caller's context. It ran on 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.
  3. Refresh bypassed the proxy. A bare &http.Transport{} leaves Proxy nil, so HTTP(S)_PROXY/NO_PROXY were ignored for the refresh request (unlike the default transport the git transport uses). Set Proxy: http.ProxyFromEnvironment.

Scope

auth.Resolve, LookupEntireDBCredential, lookupEntireDBToken, and newConn gain a leading ctx parameter; all in-module call sites (one production caller each, plus sha256convert's openSource which already has a ctx, and the test call sites) are updated. getTokenWithRefresh/refreshAccessToken already took ctx.

Tests

TestGetTokenWithRefreshHonorsContext stores an expired token + refresh token and a cancelled context, then asserts the refresh fails with an error wrapping context.Canceled — proving the context reaches the HTTP request. Full go 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-sha256 through newConnauth.ResolveLookupEntireDBCredentialgetTokenWithRefresh, replacing context.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.ProxyFromEnvironment so HTTP(S)_PROXY / NO_PROXY match 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 TestGetTokenWithRefreshHonorsContext to assert a pre-cancelled context yields context.Canceled from refresh.

Reviewed by Cursor Bugbot for commit 61ba091. Configure here.

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
@Soph

Soph commented Jun 18, 2026

Copy link
Copy Markdown
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 Soph closed this Jun 18, 2026
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants