forgejo-contrib/forgejo-cli
20
407
Fork
You've already forked forgejo-cli
51

fix(oauth): save new token immediately after refreshing #417

Merged
Cyborus merged 2 commits from cyborus/oauth-save-refresh-immediately into main 2026年04月16日 18:00:38 +02:00 AGit
Member
Copy link

Changes Made

If an OAuth token needed refreshed, it would be refreshed at the start of the program, and saved to disk at the end. This caused problems if fj is killed with Ctrl+C, or if another instance of fj is run before the first one finishes. Both of these result in the refresh token being used up but the new one not being saved, so further attempts to refresh it fail.

This is now all avoided by immediately saving the new token to disk.

Fixes #406

Code of Conduct

  • I agree to act in accordance with the CoC & AI Agreement.
  • This contribution was not generated by an LLM, even in part.
### Changes Made If an OAuth token needed refreshed, it would be refreshed at the start of the program, and saved to disk at the end. This caused problems if `fj` is killed with `Ctrl+C`, or if another instance of `fj` is run before the first one finishes. Both of these result in the refresh token being used up but the new one not being saved, so further attempts to refresh it fail. This is now all avoided by immediately saving the new token to disk. Fixes #406 ### Code of Conduct - [x] I agree to act in accordance with the CoC & AI Agreement. - [x] This contribution was not generated by an LLM, even in part.
fix(oauth): save new token immediately after refreshing
All checks were successful
ci/woodpecker/pr/check Pipeline was successful
e0e1d04255
@ -67,0 +68,4 @@
letapi=login.api_for(url).await?;
ifwas_refreshed{
self.save().await?;
}
Author
Member
Copy link

login.api_for(url) can't go after self.save() because of the borrow checker.

`login.api_for(url)` can't go after `self.save()` because of the borrow checker.
refactor: move keys.save() out of main
All checks were successful
ci/woodpecker/pr/check Pipeline was successful
1d455ac0d5
 
Now that it's called right after refreshing, the only other place that
modifies the keys file is `auth` commands. Now `keys.save()` is only
called when it needs to be, instead of writing the file on every run.
LordMZTE left a comment
Copy link

This is technically still not entirely correct. In theory, you could have another fj instance read the key while another is refreshing it, causing it to be refreshed again, invalidating the key of the first instance.

For this to be completely sound, we'd have to implement some sort of locking mechanism:

  • Read key
  • If expired:
    • Wait for any existing lock to be released
    • If there was a lock, read key again
    • If the key has changed, abort refresh and use new key, assuming that another fj instance has just refreshed it
    • Acquire lock, refresh key, save to disk, release lock
  • Do work with key

If this is worth implementing however, is sort of unclear :P

This is technically still not entirely correct. In theory, you could have another `fj` instance read the key while another is refreshing it, causing it to be refreshed again, invalidating the key of the first instance. For this to be completely sound, we'd have to implement some sort of locking mechanism: - Read key - If expired: - Wait for any existing lock to be released - If there was a lock, read key again - If the key has changed, abort refresh and use new key, assuming that another fj instance has just refreshed it - Acquire lock, refresh key, save to disk, release lock - Do work with key If this is worth implementing however, is sort of unclear :P
Author
Member
Copy link

In theory, you could have another fj instance read the key while another is refreshing it, causing it to be refreshed again, invalidating the key of the first instance.

I believe, since the same refresh token cannot be used twice, this would actually only cause one of the two to fail to refresh, in which case only the one that succeeds will write the new key. It's about a 1-second time frame (I checked!) in which they could conflict with each other, specifically only when the key is being refreshed, and the keys file should still be in a valid state afterwards. So my opinion is that it isn't worth the effort of implementing a lock mechanism.

> In theory, you could have another `fj` instance read the key while another is refreshing it, causing it to be refreshed again, invalidating the key of the first instance. I *believe*, since the same refresh token cannot be used twice, this would actually only cause one of the two to fail to refresh, in which case only the one that succeeds will write the new key. It's about a 1-second time frame (I checked!) in which they could conflict with each other, specifically only when the key is being refreshed, and the keys file should still be in a valid state afterwards. So my opinion is that it isn't worth the effort of implementing a lock mechanism.
Cyborus added this to the v0.5.0 milestone 2026年04月16日 20:24:54 +02:00
Sign in to join this conversation.
No reviewers
Labels
Clear labels
Kind/Breaking
Breaking change that won't be backward compatible
Kind/Bug
Something is not working
Kind/Design
Discussion about UI/UX design
Kind/Documentation
Documentation changes
Kind/Enhancement
Improve existing functionality
Kind/Feature
New functionality
Kind/Security
This is security issue
Kind/Testing
Issue or pull request related to testing
Kind/Upstream
This is an issue with upstream software (Forgejo) that is probably not our fault
Priority
Critical
 The priority is critical
Priority
High
 The priority is high
Priority
Low
 The priority is low
Priority
Medium
 The priority is medium
Reviewed
Confirmed
 Issue has been confirmed
Reviewed
Duplicate
 This issue or pull request already exists
Reviewed
Invalid
 Invalid issue
Reviewed
Won't Fix
 This issue won't be fixed
Status
Abandoned
 Somebody has started to work on this but abandoned work
Status
Blocked
 Something is blocking this issue or pull request
Status
Need More Info
 Feedback is required to reproduce issue or to continue work
Suspicious
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo-contrib/forgejo-cli!417
Reference in a new issue
forgejo-contrib/forgejo-cli
No description provided.
Delete branch "cyborus/oauth-save-refresh-immediately"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?