forgejo-contrib-forgejo-cli-pulls-417.md
Watch20
Star402
Fork
You've already forked forgejo-cli
CodeIssues 44Pull requests 8Releases 11Packages 2WikiActivity
Merged
Cyborusmerged 2 commits from cyborus/oauth-save-refresh-immediately into main 2026-04-16 18:00:38 +02:00AGit
Conversation 3Commits 2Files changed 3 +49 -35
Cyborus commented 2026-04-15 23:26:03 +02:00
Member
Copy link
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
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.Cyborus added 1 commit 2026-04-15 23:26:03 +02:00
fix(oauth): save new token immediately after refreshing
All checks were successful
ci/woodpecker/pr/check Pipeline was successful
Cyborus reviewed 2026-04-15 23:27:39 +02:00
| | | | @ -67,0 +68,4 @@ |
| | | | | let api = login.api_for(url).await?; |
| | | | | if was_refreshed { |
| | | | | self.save().await?; |
| | | | | } |
Cyborus commented 2026-04-15 23:27:38 +02:00
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.
Cyborus added 1 commit 2026-04-15 23:32:13 +02:00
refactor: move keys.save() out of main...
All checks were successful
ci/woodpecker/pr/check Pipeline was successful
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.
Cyborus requested review from LordMZTE 2026-04-15 23:34:59 +02:00
LordMZTE reviewed 2026-04-16 17:36:01 +02:00
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:
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
Cyborus commented 2026-04-16 17:44:47 +02:00
Author
Member
Copy link
In theory, you could have another
fjinstance 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.
👍 1
LordMZTE approved these changes 2026-04-16 17:55:30 +02:00
Cyborus merged commit 60fb29afee into main 2026-04-16 18:00:38 +02:00
Cyborus referenced this pull request from a commit 2026-04-16 18:00:42 +02:00 Merge pull request 'fix(oauth): save new token immediately after refreshing' (#417) from cyborus/oauth-save-refresh-immediately into main
Cyborus added this to the v0.5.0 milestone 2026-04-16 20:24:54 +02:00
stalecontext referenced this pull request from stalecontext/forgejo-cli-plus 2026-07-02 21:51:40 +02:00 Upstream sync 2026-07-02 #52
Sign in to join this conversation.
Reviewers
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
](#)
No labels Kind/Breaking Kind/Bug Kind/Design Kind/Documentation Kind/Enhancement Kind/Feature Kind/Security Kind/Testing Kind/Upstream [ Priority
Critical ](/forgejo-contrib/forgejo-cli/pulls?labels=173012) [ Priority
High ](/forgejo-contrib/forgejo-cli/pulls?labels=173013) [ Priority
Low ](/forgejo-contrib/forgejo-cli/pulls?labels=173015) [ Priority
Medium ](/forgejo-contrib/forgejo-cli/pulls?labels=173014) [ Reviewed
Confirmed ](/forgejo-contrib/forgejo-cli/pulls?labels=173007) [ Reviewed
Duplicate ](/forgejo-contrib/forgejo-cli/pulls?labels=173005) [ Reviewed
Invalid ](/forgejo-contrib/forgejo-cli/pulls?labels=173006) [ Reviewed
Won't Fix ](/forgejo-contrib/forgejo-cli/pulls?labels=173008) [ Status
Abandoned ](/forgejo-contrib/forgejo-cli/pulls?labels=173011) [ Status
Blocked ](/forgejo-contrib/forgejo-cli/pulls?labels=173010) [ Status
Need More Info ](/forgejo-contrib/forgejo-cli/pulls?labels=173009) Suspicious
Milestone
Clear milestone
No items
No milestone v0.5.0
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
Notifications Subscribe
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
WritePreview
Loading…
Add table
| Rows | | | Columns | |
CancelOK
Add a link
Url Description Hint: With a URL in your clipboard, you can paste directly into the editor to create a link.
CancelOK
CancelSave
Reference in a new issue
Repository
forgejo-contrib/forgejo-cli
Title
Body
Create issue
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?
No Yes