Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access token leakage when reusing the client between users #3043

Closed
legigor opened this issue Jan 9, 2024 · 7 comments · Fixed by #3051
Closed

Access token leakage when reusing the client between users #3043

legigor opened this issue Jan 9, 2024 · 7 comments · Fixed by #3051
Assignees

Comments

@legigor
Copy link

legigor commented Jan 9, 2024

Description

Reusing the github.Client instance between different users' sessions leads to leaking the access_token between sessions.

How to reproduce

This scenario works in a hosted service environment.

  1. Create a global client configured WithEnterpriseURLs
  2. For client requests, create a clone using WithAuthToken(userToken)
  3. All underlying API calls will be performed with the first used access_token, ignoring the other tokens

This test reproduces the issue

func Test_github_access_token(t *testing.T) {
	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
		w.Header().Set("Content-Type", "application/json")
		accessToken := r.Header.Get("Authorization")
		_, _ = fmt.Fprintf(w, `{"login": "%s"}`, accessToken)
	}))
	clientPreconfiguredWithURLs, err := github.NewClient(nil).WithEnterpriseURLs(srv.URL, srv.URL)
	require.NoError(t, err)

	aliseClient := clientPreconfiguredWithURLs.WithAuthToken("alise")
	bobClient := clientPreconfiguredWithURLs.WithAuthToken("bob")

	alise, _, err := aliseClient.Users.Get(context.Background(), "")
	require.NoError(t, err)
	assert.Equal(t, "Bearer alise", alise.GetLogin())

	bob, _, err := bobClient.Users.Get(context.Background(), "")
	require.NoError(t, err)
	assert.Equal(t, "Bearer bob", bob.GetLogin())
}

and the result

Error:      	Not equal: 
            	expected: "Bearer bob"
            	actual  : "Bearer alise"

Used environment

  • go version go1.21.3 darwin/amd64
  • github.com/google/go-github/v57 v57.0.0
  • github.com/google/go-github/v57 v57.0.0 h1:L+Y3UPTY8ALM8x+TV0lg+IEBI+upibemtBD8Q9u7zHs=
  • github.com/google/go-github/v57 v57.0.0/go.mod h1:s0omdnye0hvK/ecLvpsGfJMiRt85PimQh4oygmLIxHw=
@gmlewis
Copy link
Collaborator

gmlewis commented Jan 9, 2024

Have you tried the latest version after #3011?

@legigor
Copy link
Author

legigor commented Jan 16, 2024

So, I tried the new version, and the test still fails

github.com/google/go-github/v58 v58.0.0
github.com/google/go-github/v58 v58.0.0 h1:Una7GGERlF/37XfkPwpzYJe0Vp4dt2k1kCjlxwjIvzw=
github.com/google/go-github/v58 v58.0.0/go.mod h1:k4hxDKEfoWpSqFlc8LTpGd9fu2KrV1YAa6Hi6FmDNY4=

@legigor
Copy link
Author

legigor commented Jan 16, 2024

Also, I've tested it on master ref e9f5269 and test still fails with unexpected assertions

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 16, 2024

Thank you for the reproducible test case, @legigor !
When you get a chance, please review then LGTM+Approve #3051 if it fixes the problem you are seeing.

@legigor
Copy link
Author

legigor commented Jan 18, 2024

👍 So, I confirm that the test PASSED being runned on the branch https://github.com/gmlewis/go-github/tree/i3043-leaky-client-transport-copy

Thank you!

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 18, 2024

Thanks, but #3051 is blocked until I can get an LGTM+Approval from any other contributor to this repo - would you mind doing that, @legigor so I can merge it, please?

gmlewis added a commit that referenced this issue Jan 28, 2024
@legigor
Copy link
Author

legigor commented Jan 30, 2024

@gmlewis 👍 LGTM

I know it's too late, but I just wanted to express my appreciation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants