fix: atomically rewrite auth_token + identity on login#468
Merged
tomasz-tomczyk merged 2 commits intomainfrom May 5, 2026
Merged
fix: atomically rewrite auth_token + identity on login#468tomasz-tomczyk merged 2 commits intomainfrom
tomasz-tomczyk merged 2 commits intomainfrom
Conversation
Login persisted credentials in two separate saveGlobalConfig calls (saveAuthToken then saveAuthIdentity), leaving a window where a crash between writes could pair a fresh token with stale identity from the prior account — the failure mode that surfaced #371. Collapse them into one saveAuthSession closure that writes all four auth_* fields in a single atomic temp+rename. Test plan - Unit: TestSaveAuthSession_AtomicRewrite, _RemovesFieldsAbsentFromResponse - Integration (e2e-share): TestAuthSessionReLoginEndToEnd seeds two users on crit-web, persists user A then user B via saveAuthSession, runs `crit share` with HOME pointed at the test home (no CRIT_AUTH_TOKEN env override), and asserts the server stamps user B on the resulting comment.
These three helpers no longer have any production callers — saveAuthSession is the sole login-persistence path, clearAuthIdentity owns logout/401, and lazyBackfillAuthUserID writes its own narrower closure. The remaining tests exercised them in isolation, which means they passed forever regardless of whether anything called them. Repoint the two TestSaveGlobalConfig_* cases at saveAuthSession (which still exercises saveGlobalConfig under the hood), and drop TestSaveAndRemoveAuthToken plus the TestSaveAuthIdentity_* pair — TestSaveAuthSession_AtomicRewrite and _RemovesFieldsAbsentFromResponse cover the same ground plus auth_token.
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #468 +/- ##
==========================================
- Coverage 69.15% 69.15% -0.01%
==========================================
Files 36 36
Lines 10749 10736 -13
==========================================
- Hits 7434 7425 -9
+ Misses 2749 2746 -3
+ Partials 566 565 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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
1 participant
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.
Summary
saveGlobalConfigcalls (saveAuthTokenthensaveAuthIdentity), leaving a window where a crash between writes could pair a fresh token with stale identity from the previous account — the failure mode that surfaced feat: send + cache verified author identity on share #371. Collapse them into onesaveAuthSessionclosure that rewrites all fourauth_*fields in a single atomic temp+rename under the existing flock.saveAuthToken,saveAuthIdentity, andremoveAuthToken— none have production callers anymore.clearAuthIdentityalready owns logout / 401, andlazyBackfillAuthUserIDkeeps its own narrower closure for the legitimate identity-only backfill path.author,auth_user_name) stay;authoris the documented local pen name,auth_user_nameis opaque session metadata. The atomic-write fix here resolves the underlying feat: send + cache verified author identity on share #371 class without needing to consolidate config keys.Closes #392.
Review
Test plan
TestSaveAuthSession_AtomicRewrite,TestSaveAuthSession_RemovesFieldsAbsentFromResponse— cover the bearer + identity rewrite atomically (including partial-server-response field removal).make e2e-share):TestAuthSessionReLoginEndToEndseeds two distinct users on crit-web, persists user A's session viasaveAuthSession, re-persists as user B, runscrit sharewith HOME pointing at the test home (noCRIT_AUTH_TOKENenv override) and asserts the server stamps user B on the resulting comment.go test -race -count=1 ./...and the share integration suite both pass.🤖 Generated with Claude Code