Fix [GitHub] token handling

Fix #2728
badges · Jan 11, 2019 · 0275b3c · 0275b3c
1 parent c4efdc8
commit 0275b3c
Show file tree

Hide file tree

Showing 3 changed files with 128 additions and 67 deletions.
diff --git a/services/github/github-api-provider.integration.js b/services/github/github-api-provider.integration.js
@@ -4,70 +4,98 @@ const { expect } = require('chai')
 const serverSecrets = require('../../lib/server-secrets')
 const GithubApiProvider = require('./github-api-provider')
 
-describe('Github API provider with token pool', function() {
+describe('Github API provider', function() {
   const baseUrl = process.env.GITHUB_URL || 'https://api.github.com'
   const reserveFraction = 0.333
 
-  let githubApiProvider
+  let token
   before(function() {
-    githubApiProvider = new GithubApiProvider({
-      baseUrl,
-      withPooling: true,
-      reserveFraction,
-    })
-
-    const { gh_token: token } = serverSecrets
+    token = serverSecrets.gh_token
     if (!token) {
       throw Error('The integration tests require a gh_token to be set')
     }
-
-    githubApiProvider.addToken(token)
   })
 
-  const headers = []
-  async function performOneRequest() {
-    const { res } = await githubApiProvider.requestAsPromise(
-      require('request'),
-      '/repos/rust-lang/rust',
-      {}
-    )
-    expect(res.statusCode).to.equal(200)
-    headers.push(res.headers)
-  }
+  let githubApiProvider
 
-  before('should be able to run 10 requests', async function() {
-    this.timeout('20s')
-    for (let i = 0; i < 10; ++i) {
-      await performOneRequest()
-    }
+  context('without token pool', function() {
+    before(function() {
+      githubApiProvider = new GithubApiProvider({
+        baseUrl,
+        withPooling: false,
+        globalToken: token,
+        reserveFraction,
+      })
+    })
+
+    it('should be able to run 10 requests', async function() {
+      this.timeout('20s')
+      for (let i = 0; i < 10; ++i) {
+        await githubApiProvider.requestAsPromise(
+          require('request'),
+          '/repos/rust-lang/rust',
+          {}
+        )
+      }
+    })
   })
 
-  it('should decrement the limit remaining with each request', function() {
-    for (let i = 1; i < headers.length; ++i) {
-      const current = headers[i]
-      const previous = headers[i - 1]
-      expect(+current['x-ratelimit-remaining']).to.be.lessThan(
-        +previous['x-ratelimit-remaining']
+  context('with token pool', function() {
+    let githubApiProvider
+    before(function() {
+      githubApiProvider = new GithubApiProvider({
+        baseUrl,
+        withPooling: true,
+        reserveFraction,
+      })
+      githubApiProvider.addToken(token)
+    })
+
+    const headers = []
+    async function performOneRequest() {
+      const { res } = await githubApiProvider.requestAsPromise(
+        require('request'),
+        '/repos/rust-lang/rust',
+        {}
       )
+      expect(res.statusCode).to.equal(200)
+      headers.push(res.headers)
     }
-  })
 
-  it('should update the token with the final limit remaining and reset time', function() {
-    const lastHeaders = headers.slice(-1)[0]
-    const reserve = reserveFraction * +lastHeaders['x-ratelimit-limit']
-    const usesRemaining = +lastHeaders['x-ratelimit-remaining'] - reserve
-    const nextReset = +lastHeaders['x-ratelimit-reset']
+    before('should be able to run 10 requests', async function() {
+      this.timeout('20s')
+      for (let i = 0; i < 10; ++i) {
+        await performOneRequest()
+      }
+    })
 
-    const tokens = []
-    githubApiProvider.standardTokens.forEach(t => {
-      tokens.push(t)
+    it('should decrement the limit remaining with each request', function() {
+      for (let i = 1; i < headers.length; ++i) {
+        const current = headers[i]
+        const previous = headers[i - 1]
+        expect(+current['x-ratelimit-remaining']).to.be.lessThan(
+          +previous['x-ratelimit-remaining']
+        )
+      }
     })
 
-    // Confidence check.
-    expect(tokens).to.have.lengthOf(1)
+    it('should update the token with the final limit remaining and reset time', function() {
+      const lastHeaders = headers.slice(-1)[0]
+      const reserve = reserveFraction * +lastHeaders['x-ratelimit-limit']
+      const usesRemaining = +lastHeaders['x-ratelimit-remaining'] - reserve
+      const nextReset = +lastHeaders['x-ratelimit-reset']
+
+      const tokens = []
+      githubApiProvider.standardTokens.forEach(t => {
+        tokens.push(t)
+      })
 
-    const [token] = tokens
-    expect(token.usesRemaining).to.equal(usesRemaining)
-    expect(token.nextReset).to.equal(nextReset)
+      // Confidence check.
+      expect(tokens).to.have.lengthOf(1)
+
+      const [token] = tokens
+      expect(token.usesRemaining).to.equal(usesRemaining)
+      expect(token.nextReset).to.equal(nextReset)
+    })
   })
 })
diff --git a/services/github/github-api-provider.js b/services/github/github-api-provider.js
@@ -1,6 +1,16 @@
 'use strict'
 
+const Joi = require('joi')
 const { TokenPool } = require('../../lib/token-pool')
+const { nonNegativeInteger } = require('../validators')
+
+const headerSchema = Joi.object({
+  'x-ratelimit-limit': nonNegativeInteger,
+  'x-ratelimit-remaining': nonNegativeInteger,
+  'x-ratelimit-reset': nonNegativeInteger,
+})
+  .required()
+  .unknown(true)
 
 // Provides an interface to the Github API. Manages the base URL.
 class GithubApiProvider {
@@ -48,11 +58,31 @@ class GithubApiProvider {
   }
 
   updateToken(token, headers) {
-    const rateLimit = +headers['x-ratelimit-limit']
-    const reserve = this.reserveFraction * rateLimit
-    const usesRemaining = +headers['x-ratelimit-remaining'] - reserve
+    let rateLimit, totalUsesRemaining, nextReset
+    try {
+      ;({
+        'x-ratelimit-limit': rateLimit,
+        'x-ratelimit-remaining': totalUsesRemaining,
+        'x-ratelimit-reset': nextReset,
+      } = Joi.attempt(headers, headerSchema))
+    } catch (e) {
+      const logHeaders = {
+        'x-ratelimit-limit': headers['x-ratelimit-limit'],
+        'x-ratelimit-remaining': headers['x-ratelimit-remaining'],
+        'x-ratelimit-reset': headers['x-ratelimit-reset'],
+      }
+      console.log(
+        `Invalid GitHub rate limit headers ${JSON.stringify(
+          logHeaders,
+          undefined,
+          2
+        )}`
+      )
+      return
+    }
 
-    const nextReset = +headers['x-ratelimit-reset']
+    const reserve = this.reserveFraction * rateLimit
+    const usesRemaining = totalUsesRemaining - reserve
 
     token.update(usesRemaining, nextReset)
   }
@@ -63,13 +93,7 @@ class GithubApiProvider {
   }
 
   tokenForUrl(url) {
-    const { globalToken } = this
-    if (globalToken) {
-      // When a global gh_token is configured, use that in place of our token
-      // pool. This produces more predictable behavior, and more predictable
-      // failures when that token is exhausted.
-      return { id: globalToken }
-    } else if (url.startsWith('/search')) {
+    if (url.startsWith('/search')) {
       return this.searchTokens.next()
     } else {
       return this.standardTokens.next()
@@ -83,11 +107,17 @@ class GithubApiProvider {
     const { baseUrl } = this
 
     let token
-    try {
-      token = this.tokenForUrl(url)
-    } catch (e) {
-      callback(e)
-      return
+    let tokenString
+    if (this.withPooling) {
+      try {
+        token = this.tokenForUrl(url)
+      } catch (e) {
+        callback(e)
+        return
+      }
+      tokenString = token.id
+    } else {
+      tokenString = this.globalToken
     }
 
     const options = {
@@ -97,16 +127,18 @@ class GithubApiProvider {
       headers: {
         'User-Agent': 'Shields.io',
         Accept: 'application/vnd.github.v3+json',
-        Authorization: `token ${token.id}`,
+        Authorization: `token ${tokenString}`,
       },
     }
 
     request(options, (err, res, buffer) => {
       if (err === null) {
-        if (res.statusCode === 401) {
-          this.invalidateToken(token)
-        } else if (res.statusCode < 500) {
-          this.updateToken(token, res.headers)
+        if (this.withPooling) {
+          if (res.statusCode === 401) {
+            this.invalidateToken(token)
+          } else if (res.statusCode < 500) {
+            this.updateToken(token, res.headers)
+          }
         }
       }
       callback(err, res, buffer)

diff --git a/services/github/github-constellation.js b/services/github/github-constellation.js
@@ -37,6 +37,7 @@ class GithubConstellation {
     this.apiProvider = new GithubApiProvider({
       baseUrl,
       globalToken,
+      withPooling: !globalToken,
       onTokenInvalidated: tokenString => this.onTokenInvalidated(tokenString),
     })
   }