From 53cecba74977e731450e47e64fae459021570b73 Mon Sep 17 00:00:00 2001 From: Gal Ofri Date: Sun, 14 May 2023 14:34:50 +0300 Subject: [PATCH] Support x-ratelimit-reset handling for secondary rate limits (#2775) --- example/go.mod | 14 +++++----- example/go.sum | 12 +++++++++ github/github.go | 34 ++++++++++++++++++----- github/github_test.go | 63 +++++++++++++++++++++++++++++++++++++++++-- 4 files changed, 107 insertions(+), 16 deletions(-) diff --git a/example/go.mod b/example/go.mod index 144f51b5e4..d5a970e67a 100644 --- a/example/go.mod +++ b/example/go.mod @@ -4,21 +4,21 @@ go 1.17 require ( github.com/bradleyfalzon/ghinstallation/v2 v2.0.4 - github.com/gofri/go-github-ratelimit v1.0.1 + github.com/gofri/go-github-ratelimit v1.0.3 github.com/google/go-github/v52 v52.0.0 - golang.org/x/crypto v0.1.0 - golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be + golang.org/x/crypto v0.7.0 + golang.org/x/oauth2 v0.7.0 google.golang.org/appengine v1.6.7 ) require ( github.com/golang-jwt/jwt/v4 v4.0.0 // indirect - github.com/golang/protobuf v1.3.2 // indirect + github.com/golang/protobuf v1.5.2 // indirect github.com/google/go-github/v41 v41.0.0 // indirect github.com/google/go-querystring v1.1.0 // indirect - golang.org/x/net v0.7.0 // indirect - golang.org/x/sys v0.5.0 // indirect - golang.org/x/term v0.5.0 // indirect + golang.org/x/net v0.9.0 // indirect + golang.org/x/sys v0.7.0 // indirect + golang.org/x/term v0.7.0 // indirect ) // Use version at HEAD, not the latest published. diff --git a/example/go.sum b/example/go.sum index 71e7a992a3..f231e16988 100644 --- a/example/go.sum +++ b/example/go.sum @@ -2,12 +2,17 @@ github.com/bradleyfalzon/ghinstallation/v2 v2.0.4 h1:tXKVfhE7FcSkhkv0UwkLvPDeZ4k github.com/bradleyfalzon/ghinstallation/v2 v2.0.4/go.mod h1:B40qPqJxWE0jDZgOR1JmaMy+4AY1eBP+IByOvqyAKp0= github.com/gofri/go-github-ratelimit v1.0.1 h1:sgefSzxhnvwZ+wR9uZ4l9TnjgLuNiwipJVzJL4YLj9A= github.com/gofri/go-github-ratelimit v1.0.1/go.mod h1:OnCi5gV+hAG/LMR7llGhU7yHt44se9sYgKPnafoL7RY= +github.com/gofri/go-github-ratelimit v1.0.3 h1:Ocs2jaYokZDzgvqaajX+g04dqFyVqL0JQzoO7d2wmlk= +github.com/gofri/go-github-ratelimit v1.0.3/go.mod h1:OnCi5gV+hAG/LMR7llGhU7yHt44se9sYgKPnafoL7RY= github.com/golang-jwt/jwt/v4 v4.0.0 h1:RAqyYixv1p7uEnocuy8P1nru5wprCh/MH2BIlW5z5/o= github.com/golang-jwt/jwt/v4 v4.0.0/go.mod h1:/xlHOz8bRuivTWchD4jCa+NbatV+wEUSzwAxVc6locg= github.com/golang/protobuf v1.3.1/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= github.com/golang/protobuf v1.3.2 h1:6nsPYzhq5kReh6QImI3k5qWzO4PEbvbIW2cwSfR/6xs= github.com/golang/protobuf v1.3.2/go.mod h1:6lQm79b+lXiMfvg/cZm0SGofjICqVBUtrP5yJMmIC1U= +github.com/golang/protobuf v1.5.0/go.mod h1:FsONVRAS9T7sI+LIUmWTfcYkHO4aIWwzhcaSAoJOfIk= +github.com/golang/protobuf v1.5.2/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY= github.com/google/go-cmp v0.5.2/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= +github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.6/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE= github.com/google/go-cmp v0.5.9 h1:O2Tfq5qg4qc4AmwVlvv0oLiVAGB7enBSJ2x2DqQFi38= github.com/google/go-cmp v0.5.9/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY= @@ -21,6 +26,7 @@ golang.org/x/crypto v0.0.0-20210817164053-32db794688a5/go.mod h1:GvvjBRRGRdwPK5y golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= golang.org/x/crypto v0.1.0 h1:MDRAIl0xIo9Io2xV565hzXHw3zVseKrJKodhohM5CjU= golang.org/x/crypto v0.1.0/go.mod h1:RecgLatLF4+eUMCP1PoPZQb+cVrJcOPbHkTkbkB9sbw= +golang.org/x/crypto v0.7.0/go.mod h1:pYwdfH91IfpZVANVyUOhSIPZaFoJGxTFbZhFTx+dXZU= golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= golang.org/x/net v0.0.0-20190603091049-60506f45cf65/go.mod h1:HSz+uSET+XFnRR8LxR5pz3Of3rY3CfYBVs4xY44aLks= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= @@ -29,8 +35,10 @@ golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug golang.org/x/net v0.1.0/go.mod h1:Cx3nUiGt4eDBEyega/BKRp+/AlGL8hYe7U9odMt2Cco= golang.org/x/net v0.7.0 h1:rJrUqqhjsgNp7KqAIc25s9pZnjU7TUcSY7HcVZjdn1g= golang.org/x/net v0.7.0/go.mod h1:2Tu9+aMcznHK/AK1HMvgo6xiTLG5rD5rZLDS+rp2Bjs= +golang.org/x/net v0.9.0/go.mod h1:d48xBJpPfHeWQsugry2m+kC02ZBRGRgulfHnEXEuWns= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be h1:vEDujvNQGv4jgYKudGeI/+DAX4Jffq6hpD55MmoEvKs= golang.org/x/oauth2 v0.0.0-20180821212333-d2e6202438be/go.mod h1:N/0e6XlmueqKjAGxoOufVs8QHGRruUQn6yWY3a++T0U= +golang.org/x/oauth2 v0.7.0/go.mod h1:hPLQkd9LyjfXTiRohC/41GhcFqxisoUQ99sCUOHO9x4= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= @@ -41,11 +49,13 @@ golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.1.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.5.0 h1:MUK/U/4lj1t1oPg0HfuXDN/Z1wv31ZJ/YcPiGccS4DU= golang.org/x/sys v0.5.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.7.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.1.0/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= golang.org/x/term v0.5.0 h1:n2a8QNdAb0sZNpU9R1ALUXBbY+w51fCQDN+7EdxNBsY= golang.org/x/term v0.5.0/go.mod h1:jMB1sMXY+tzblOD4FWmEbocvup2/aLOaQEp7JmGp78k= +golang.org/x/term v0.7.0/go.mod h1:P32HKFT3hSsZrRxla30E9HqToFYAQPCMs/zFMBUFqPY= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= golang.org/x/text v0.3.2/go.mod h1:bEr9sfX3Q8Zfm5fL9x+3itogRgK3+ptLWKqgva+5dAk= golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= @@ -59,3 +69,5 @@ golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8T golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= google.golang.org/appengine v1.6.7 h1:FZR1q0exgwxzPzp/aF+VccGrSfxfPpkBqjIIEq3ru6c= google.golang.org/appengine v1.6.7/go.mod h1:8WjMMxjGQR8xUklV/ARdw2HLXBOI7O7uCIDZVag1xfc= +google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw= +google.golang.org/protobuf v1.26.0/go.mod h1:9q0QmTI4eRPtz6boOQmLYwt+qCgq0jsYwAQnmE0givc= diff --git a/github/github.go b/github/github.go index 2127a527ac..4170719a38 100644 --- a/github/github.go +++ b/github/github.go @@ -40,6 +40,7 @@ const ( headerRateRemaining = "X-RateLimit-Remaining" headerRateReset = "X-RateLimit-Reset" headerOTP = "X-GitHub-OTP" + headerRetryAfter = "Retry-After" headerTokenExpiration = "GitHub-Authentication-Token-Expiration" @@ -677,6 +678,30 @@ func parseRate(r *http.Response) Rate { return rate } +// parseSecondaryRate parses the secondary rate related headers, +// and returns the time to retry after. +func parseSecondaryRate(r *http.Response) *time.Duration { + // According to GitHub support, the "Retry-After" header value will be + // an integer which represents the number of seconds that one should + // wait before resuming making requests. + if v := r.Header.Get(headerRetryAfter); v != "" { + retryAfterSeconds, _ := strconv.ParseInt(v, 10, 64) // Error handling is noop. + retryAfter := time.Duration(retryAfterSeconds) * time.Second + return &retryAfter + } + + // According to GitHub support, endpoints might return x-ratelimit-reset instead, + // as an integer which represents the number of seconds since epoch UTC, + // represting the time to resume making requests. + if v := r.Header.Get(headerRateReset); v != "" { + secondsSinceEpoch, _ := strconv.ParseInt(v, 10, 64) // Error handling is noop. + retryAfter := time.Until(time.Unix(secondsSinceEpoch, 0)) + return &retryAfter + } + + return nil +} + // parseTokenExpiration parses the TokenExpiration related headers. // Returns 0001-01-01 if the header is not defined or could not be parsed. func parseTokenExpiration(r *http.Response) Timestamp { @@ -1156,13 +1181,8 @@ func CheckResponse(r *http.Response) error { Response: errorResponse.Response, Message: errorResponse.Message, } - if v := r.Header["Retry-After"]; len(v) > 0 { - // According to GitHub support, the "Retry-After" header value will be - // an integer which represents the number of seconds that one should - // wait before resuming making requests. - retryAfterSeconds, _ := strconv.ParseInt(v[0], 10, 64) // Error handling is noop. - retryAfter := time.Duration(retryAfterSeconds) * time.Second - abuseRateLimitError.RetryAfter = &retryAfter + if retryAfter := parseSecondaryRate(r); retryAfter != nil { + abuseRateLimitError.RetryAfter = retryAfter } return abuseRateLimitError default: diff --git a/github/github_test.go b/github/github_test.go index 9c40728f4d..2f92d3baa0 100644 --- a/github/github_test.go +++ b/github/github_test.go @@ -17,6 +17,7 @@ import ( "os" "path" "reflect" + "strconv" "strings" "testing" "time" @@ -1475,14 +1476,14 @@ func TestDo_rateLimit_abuseRateLimitErrorEnterprise(t *testing.T) { } } -// Ensure *AbuseRateLimitError.RetryAfter is parsed correctly. +// Ensure *AbuseRateLimitError.RetryAfter is parsed correctly for the Retry-After header. func TestDo_rateLimit_abuseRateLimitError_retryAfter(t *testing.T) { client, mux, _, teardown := setup() defer teardown() mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json; charset=utf-8") - w.Header().Set("Retry-After", "123") // Retry after value of 123 seconds. + w.Header().Set(headerRetryAfter, "123") // Retry after value of 123 seconds. w.WriteHeader(http.StatusForbidden) fmt.Fprintln(w, `{ "message": "You have triggered an abuse detection mechanism ...", @@ -1528,6 +1529,64 @@ func TestDo_rateLimit_abuseRateLimitError_retryAfter(t *testing.T) { } } +// Ensure *AbuseRateLimitError.RetryAfter is parsed correctly for the x-ratelimit-reset header. +func TestDo_rateLimit_abuseRateLimitError_xRateLimitReset(t *testing.T) { + client, mux, _, teardown := setup() + defer teardown() + + // x-ratelimit-reset value of 123 seconds into the future. + blockUntil := time.Now().Add(time.Duration(123) * time.Second).Unix() + + mux.HandleFunc("/", func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json; charset=utf-8") + w.Header().Set(headerRateReset, strconv.Itoa(int(blockUntil))) + w.Header().Set(headerRateRemaining, "1") // set remaining to a value > 0 to distinct from a primary rate limit + w.WriteHeader(http.StatusForbidden) + fmt.Fprintln(w, `{ + "message": "You have triggered an abuse detection mechanism ...", + "documentation_url": "https://docs.github.com/en/rest/overview/resources-in-the-rest-api#abuse-rate-limits" +}`) + }) + + req, _ := client.NewRequest("GET", ".", nil) + ctx := context.Background() + _, err := client.Do(ctx, req, nil) + + if err == nil { + t.Error("Expected error to be returned.") + } + abuseRateLimitErr, ok := err.(*AbuseRateLimitError) + if !ok { + t.Fatalf("Expected a *AbuseRateLimitError error; got %#v.", err) + } + if abuseRateLimitErr.RetryAfter == nil { + t.Fatalf("abuseRateLimitErr RetryAfter is nil, expected not-nil") + } + // the retry after value might be a bit smaller than the original duration because the duration is calculated from the expected end-of-cooldown time + if got, want := *abuseRateLimitErr.RetryAfter, 123*time.Second; want-got > 1*time.Second { + t.Errorf("abuseRateLimitErr RetryAfter = %v, want %v", got, want) + } + + // expect prevention of a following request + if _, err = client.Do(ctx, req, nil); err == nil { + t.Error("Expected error to be returned.") + } + abuseRateLimitErr, ok = err.(*AbuseRateLimitError) + if !ok { + t.Fatalf("Expected a *AbuseRateLimitError error; got %#v.", err) + } + if abuseRateLimitErr.RetryAfter == nil { + t.Fatalf("abuseRateLimitErr RetryAfter is nil, expected not-nil") + } + // the saved duration might be a bit smaller than Retry-After because the duration is calculated from the expected end-of-cooldown time + if got, want := *abuseRateLimitErr.RetryAfter, 123*time.Second; want-got > 1*time.Second { + t.Errorf("abuseRateLimitErr RetryAfter = %v, want %v", got, want) + } + if got, wantSuffix := abuseRateLimitErr.Message, "not making remote request."; !strings.HasSuffix(got, wantSuffix) { + t.Errorf("Expected request to be prevented because of secondary rate limit, got: %v.", got) + } +} + func TestDo_noContent(t *testing.T) { client, mux, _, teardown := setup() defer teardown()