[APPS] Invalidate unusable cached OAuth tokens#434
Conversation
sdkennedy2
force-pushed
the
sdkennedy2/apps-oauth-token-hardening
branch
2 times, most recently
from
dd5653f to
5f9109c
Compare
sdkennedy2
changed the base branch from
master
to
sdkennedy2/defer-oauth-token-persistence
sdkennedy2
changed the title
There was a problem hiding this comment.
This refactor lets token reads and explicit token deletes share the same keychain deletion behavior. deleteOAuthTokenFromKeychain still ignores missing entries, but the deletion logic now lives in deleteOAuthCredentialEntry so readOAuthTokenFromKeychain can also clean up malformed or invalid cached credentials without duplicating the NoEntry handling. The split also keeps failures to create the keychain entry distinct from failures while deleting the stored password.
sdkennedy2
force-pushed
the
sdkennedy2/apps-oauth-token-hardening
branch
from
5f9109c to
19ac473
Compare
| } | ||
|
|
||
| if (!response.ok) { | ||
| // Not instantiating the error here, as it will make Jest throw in the tests. |
There was a problem hiding this comment.
We shouldn't remove this.
| }); | ||
| }; | ||
|
|
||
| export const getOAuthToken = async ( |
There was a problem hiding this comment.
This was unused.
| value.token.clientId === options.clientId && | ||
| value.token.site === site && |
There was a problem hiding this comment.
If the cached toke is for a different client id or site, we invalidate the cached token.
|
|
||
| return result; | ||
| } catch (error) { | ||
| if (isOAuthAuthError(error)) { |
There was a problem hiding this comment.
We now invalidate the stored token on a 401 and 403
There was a problem hiding this comment.
could this be too simple a heuristic? 401/403 could also be caused by granular access issues
sdkennedy2
force-pushed
the
sdkennedy2/apps-oauth-token-hardening
branch
from
19ac473 to
226674c
Compare
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Preserve refresh tokens on transient refresh failures
When an expired cached token has a refresh token and the refresh call fails for a transient reason (for example a token-endpoint 5xx or network error), this catch still deletes the cached credential. refreshOAuthToken performs the network request to the token endpoint, so not every thrown error proves the refresh token is invalid; after this deletion a non-interactive run falls into browser auth and a later retry cannot use the still-valid refresh token after the service recovers. Please only evict on refresh errors that indicate the token is unusable, such as an OAuth invalid_grant/auth failure.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| return result; | ||
| } catch (error) { | ||
| if (isOAuthAuthError(error)) { |
There was a problem hiding this comment.
could this be too simple a heuristic? 401/403 could also be caused by granular access issues
There was a problem hiding this comment.
Tell the user to try again (to trigger a new login with oauth)
2 participants
Motivation
Apps OAuth relies on cached tokens from the OS credential store so repeated uploads do not need a browser authorization flow. When that cache contains a bad token, the command can get stuck reusing it instead of recovering.
This PR handles the cases where the cached value is malformed, belongs to a different site/client, has an empty access token, cannot be refreshed, or is rejected by Datadog with an OAuth auth failure.
This is stacked on #435, which defers persistence of newly authorized tokens until the first authenticated request succeeds. This PR adds the follow-up hardening for tokens that already exist in the cache or become unusable later.
Changes
RequestErrorwith HTTP status metadata to the shared request helper.401or403, clear both the OS credential-store entry and the in-process token memo.QA Instructions
No manual QA required. The changed behavior is covered by focused unit tests for the OAuth and request helpers.
Validated with:
Blast Radius
This affects Apps plugin OAuth authentication paths that use the shared core OAuth request helper. API-key and app-key authentication are unchanged.
Expected customer-visible behavior: users with malformed, revoked, wrong-site, wrong-client, expired-unrefreshable, or API-rejected cached OAuth tokens should be prompted to authorize again instead of staying stuck on the unusable cached token.
Documentation