Skip to content

fix: don't override OAuth token with an invalid response

Tomas Vik (OOO back on 2024-05-09) requested to merge fix-oauth-refresh into main

This MR fixes an issue where we mistakenly returned an invalid response as a token refresh response. Now we throw an error.

Bug explanation

It's possible that two VS Code windows will initiate token refresh almost at the same time. They both use the same refresh token, but GitLab API will only succeed for the first window, for the second window, it will report that the refresh token is no longer valid.

The error response from API returns JSON object {"error": "...", "message": "..."}. This object was passed down as the token response which was supposed to have different properties, namely {"token": "..."}.

We then went on and stored the undefined token property from the error response in the OS Keychain as if it was a valid token.

sequenceDiagram
    participant V1 as VS Code Window A
    participant V2 as VS Code Window B
    participant GA as GitLab API
    participant K as OS Keychain
    V1->>GA: refresh token with token X
    GA->>V1: new token
    V2 ->>GA: refresh token with token X (not valid anymore)
    GA-->>V2: invalid refresh token response
    V1->>K: store valid tokens
    V2->>K: store empty object

Error

Error example
[error]: Refreshing OAuth token failed: already refreshed. This is most likely caused by another VS Code window refreshing the token.You can retry the operation. If this error persists, remove and re-add your GitLab account.
         Error: Refreshing OAuth token failed: already refreshed. This is most likely caused by another VS Code window refreshing the token. You can retry the operation. If this error persists, remove and re-add your GitLab account.
             at Function.exchangeToken (/Users/tomas/.vscode-insiders/extensions/gitlab.gitlab-workflow-3.57.2/src/gitlab/gitlab_service.ts:1067:15)
             at processTicksAndRejections (node:internal/process/task_queues:96:5)
             at Yc.#a (/Users/tomas/.vscode-insiders/extensions/gitlab.gitlab-workflow-3.57.2/src/gitlab/token_exchange_service.ts:98:22)
             at as.fetch (/Users/tomas/.vscode-insiders/extensions/gitlab.gitlab-workflow-3.57.2/src/gitlab/gitlab_service.ts:304:29)
             at as.getOpenMergeRequestForCurrentBranch (/Users/tomas/.vscode-insiders/extensions/gitlab.gitlab-workflow-3.57.2/src/gitlab/gitlab_service.ts:945:17)
             at as.getPipelineAndMrForCurrentBranch (/Users/tomas/.vscode-insiders/extensions/gitlab.gitlab-workflow-3.57.2/src/gitlab/gitlab_service.ts:979:16)
             at Function.getPipelineAndMrForHead (/Users/tomas/.vscode-insiders/extensions/gitlab.gitlab-workflow-3.57.2/src/current_branch_refresher.ts:150:32)
             at Function.getState (/Users/tomas/.vscode-insiders/extensions/gitlab.gitlab-workflow-3.57.2/src/current_branch_refresher.ts:177:38)
             at on.refresh (/Users/tomas/.vscode-insiders/extensions/gitlab.gitlab-workflow-3.57.2/src/current_branch_refresher.ts:130:24)
             at on.clearAndSetIntervalAndRefresh (/Users/tomas/.vscode-insiders/extensions/gitlab.gitlab-workflow-3.57.2/src/current_branch_refresher.ts:115:5)
             at /Users/tomas/.vscode-insiders/extensions/gitlab.gitlab-workflow-3.57.2/src/current_branch_refresher.ts:94:9

This error happens very rarely. It sometimes happens when two VS Code windows try to refresh token at the same time. This can only happen once in two hours and on my laptop with 3+ windows open happened once every 2-4 days.

Before this fix, the VS Code discarded the tokens. After this fix, it will show this error every 2-4 days and retries without user noticing.


I added the If this error persists, remove and re-add your GitLab account. sentence because there is still a very unlikely scenario that the refresh mechanism breaks:

  1. Extension makes API call to refresh the token
  2. API sends new token (the old is now invalid)
  3. The VS Code process dies before storing the new token in the keychain.

In this unlikely scenario, the user must remove the account and add a new one.

I never saw this issue, but it is, in theory, possible.

Relates to #579 (closed)

Edited by Tomas Vik (OOO back on 2024-05-09)

Merge request reports