Skip to content

Conversation

@becm
Copy link
Contributor

@becm becm commented Feb 13, 2025

Not checking for expired tokens triggers failures on first fetch/push after expiration.
Many Oauth2 implementations use JWT, where expiration time stamp is stored in structured data.

Fixes #268
Fixes #1408
Fixes #1784

@becm becm changed the title fix(generic): save new refresh_token value fix(generic): refresh of local tokens Feb 14, 2025
@becm
Copy link
Contributor Author

becm commented Feb 15, 2025

I'd very much like to have any approach towards identifying local token expiration.
Alternative idea:

  • the username is (for the generic provider, at the moment) basically unused (fixed value).
  • we can change the stored value to OAUTH_USER@<expiration>.

The last approach changing the API for the storage format (#1464) is now stale for almost a year.
Neither suggestions here would need additional code to be touched.

The username approach is likely even waaayyy more compact.
For fully transparent handling of Git credential flow using store, the full username must be retained for the auth_token credential.
Due to limitations of Git using a BasicAuth header, a colon is not a valid separator.

@becm becm force-pushed the oauth-refresh-token branch from b467d37 to 482a1ea Compare February 15, 2025 16:37
@becm becm changed the title fix(generic): refresh of local tokens fix(generic): check local token expiry Feb 15, 2025
@becm
Copy link
Contributor Author

becm commented Feb 16, 2025

Some major caveat found while trying to prototype a "username@expiration" approach:
API may only return expires_in for the new access_token, not an update refresh_token!

Some storage back ends may also be thrown off by the changing username…

Relative time in protocol additionally indicates this is to be used as a transient element.
Committing that value to permanent storage (after calculating an absolute date) feels like a data flow violation.

For now I'd definitely favor using structured token detection (transparent, backward-compatible, non-intrusive).

@becm becm force-pushed the oauth-refresh-token branch from 482a1ea to c8a7e32 Compare February 25, 2025 19:53
@lostmsu
Copy link

lostmsu commented Jun 26, 2025

@mjcheetham @mpysson can you please take a look at this change? It's already been iterated upon.

@lostmsu
Copy link

lostmsu commented Jun 26, 2025

@mjcheetham @mpysson there've been no changes in this project for 5 months. Can I co-maintain it? Not sure if I can do it long term but I promise to go through the currently open pull requests at the very least. Better than nothing.

@itsjustnickdev
Copy link

any updates on this push?

@dscho
Copy link
Contributor

dscho commented Sep 30, 2025

any updates on this push?

@itsjustnickdev cautiously: yes. I opened #2059.

However, during the extended time of ownership limbo, Git Credential Manager's release process has withered to the point where no releases can be made as of time of writing (notarizing, for example, is broken, yet it is required for Git Credential Manager to work on macOS).

Therefore, the primary concern right now is to get that release process into a healthy shape.

If you want to look into #2059 in the meantime, that would be really cool, of course!

Copy link
Contributor

@mjcheetham mjcheetham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about the merits of using the JsonWebToken type from the https://www.nuget.org/packages/Microsoft.IdentityModel.JsonWebTokens package?

add decode support to Base64Url converter
override GenericHostProvider credential query to check for token expiry
add expiry check for refresh token
add generic StructuredToken class with expiry status property
add minimal JWT data classes for content decoding and extraction
@becm becm force-pushed the oauth-refresh-token branch from d7e4f76 to 2efb138 Compare November 14, 2025 15:39
@becm becm requested a review from a team as a code owner November 14, 2025 15:39
@becm
Copy link
Contributor Author

becm commented Nov 14, 2025

@mjcheetham replaced handling of JWT with a generic StructuredToken placeholder.

This should make expanding support (or replace implementation) fairly easily later on.
Kept the minimalist JWT extraction approach (fancy 3-liner plus boilerplate) for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

6 participants