Strip query params

app/download.js

@@ -22,6 +22,68 @@ const branchInfoCache = new Map()
 const commitInfoCache = new Map()
 let githubRequest
 
+/**
+ * Global rate limit state tracking.
+ * When rate limit is exhausted, we store the reset time to avoid
+ * making unnecessary requests that will fail.
+ */
+const rateLimitState = {
+  remaining: undefined,
+  reset: undefined
+}
+
+/**
+ * Update the global rate limit state from response headers.
+ * @param {number|undefined} remaining
+ * @param {number|undefined} reset
+ */
+function updateRateLimitState (remaining, reset) {
+  if (remaining !== undefined) {
+    rateLimitState.remaining = remaining
+  }
+  if (reset !== undefined) {
+    rateLimitState.reset = reset
+  }
+}
+
+/**
+ * Check if we are currently rate limited.
+ * Returns an object with `limited` boolean and `retryAfter` seconds if limited.
+ * @returns {{ limited: boolean, retryAfter: number|undefined, reset: number|undefined }}
+ */
+function isRateLimited () {
+  const now = Math.floor(Date.now() / 1000)
+
+  // If reset time has passed, we're no longer rate limited
+  if (rateLimitState.reset && now >= rateLimitState.reset) {
+    rateLimitState.remaining = undefined
+    rateLimitState.reset = undefined
+    return { limited: false, retryAfter: undefined, reset: undefined }
+  }
+
+  // If we know we have no remaining requests
+  if (rateLimitState.remaining === 0 && rateLimitState.reset) {
+    const retryAfter = Math.max(0, rateLimitState.reset - now)
+    return { limited: true, retryAfter, reset: rateLimitState.reset }
+  }
+
+  return { limited: false, retryAfter: undefined, reset: undefined }
+}
+
+/**
+ * Get the current rate limit state for external inspection.
+ * @returns {{ remaining: number|undefined, reset: number|undefined, limited: boolean, retryAfter: number|undefined }}
+ */
+function getRateLimitState () {
+  const { limited, retryAfter } = isRateLimited()
+  return {
+    remaining: rateLimitState.remaining,
+    reset: rateLimitState.reset,
+    limited,
+    retryAfter
+  }
+}
+
 /**
  * Extracts rate limit information from a headers object.
  * @param {import('http').IncomingHttpHeaders|undefined} headers
@@ -45,13 +107,17 @@ function getRateLimitInfo (headers) {
 
 /**
  * Logs a warning when rate limit remaining is depleted.
+ * Also updates the global rate limit state.
  * @param {import('http').IncomingHttpHeaders|undefined} headers
  * @param {string} context
  * @returns {{ remaining: number|undefined, reset: number|undefined }}
  */
 function logRateLimitIfDepleted (headers, context) {
   const { remaining, reset } = getRateLimitInfo(headers)
 
+  // Update global state
+  updateRateLimitState(remaining, reset)
+
   if (remaining === 0) {
     const resetTime = reset
       ? new Date(reset * 1000).toISOString()
@@ -314,14 +380,15 @@ async function getDownloadFiles(branch) {
  * @param {string} branch The name of the branch the files are located in.
  */
 async function getFilesInFolder(path, branch) {
-    const { body, statusCode } = await get({
+    const { body, statusCode, headers } = await get({
         hostname: 'api.github.com',
         path: `/repos/${repo}/contents/${path}?ref=${branch}`,
         headers: {
             'user-agent': 'github.highcharts.com',
             ...authToken
         }
     })
+    logRateLimitIfDepleted(headers, `listing files in ${path} for ${branch}`)
 
     if (statusCode !== 200) {
         console.warn(`Could not get files in folder ${path}. This is only an issue if the requested path exists in the branch ${branch}. (HTTP ${statusCode})`)
@@ -373,14 +440,15 @@ async function urlExists(url) {
  */
 async function getBranchInfo (branch) {
   return getWithCache(branchInfoCache, branch, async () => {
-    const { body, statusCode } = await githubRequest({
+    const { body, statusCode, headers } = await githubRequest({
       hostname: 'api.github.com',
       path: `/repos/${repo}/branches/${branch}`,
       headers: {
         'user-agent': 'github.highcharts.com',
         ...authToken
       }
     })
+    logRateLimitIfDepleted(headers, `fetching branch info for ${branch}`)
     if (statusCode === 200) {
       return JSON.parse(body)
     }
@@ -399,14 +467,15 @@ async function getBranchInfo (branch) {
  */
 async function getCommitInfo (commit) {
   return getWithCache(commitInfoCache, commit, async () => {
-    const { body, statusCode } = await githubRequest({
+    const { body, statusCode, headers } = await githubRequest({
       hostname: 'api.github.com',
       path: `/repos/${repo}/commits/${commit}`,
       headers: {
         'user-agent': 'github.highcharts.com',
         ...authToken
       }
     })
+    logRateLimitIfDepleted(headers, `fetching commit info for ${commit}`)
     if (statusCode === 200) {
       return JSON.parse(body)
     }
@@ -423,6 +492,24 @@ function clearGitHubCache () {
   commitInfoCache.clear()
 }
 
+/**
+ * Reset the rate limit state. Used for testing.
+ */
+function clearRateLimitState () {
+  rateLimitState.remaining = undefined
+  rateLimitState.reset = undefined
+}
+
+/**
+ * Set rate limit state directly. Used for testing.
+ * @param {number|undefined} remaining
+ * @param {number|undefined} reset
+ */
+function setRateLimitState (remaining, reset) {
+  rateLimitState.remaining = remaining
+  rateLimitState.reset = reset
+}
+
 // Export download functions
 module.exports = {
     downloadFile,
@@ -434,6 +521,10 @@ module.exports = {
     urlExists,
     getBranchInfo,
     getCommitInfo,
+    isRateLimited,
+    getRateLimitState,
     __setGitHubRequest: setGitHubRequest,
-    __clearGitHubCache: clearGitHubCache
+    __clearGitHubCache: clearGitHubCache,
+    __clearRateLimitState: clearRateLimitState,
+    __setRateLimitState: setRateLimitState
 }

app/handlers.js

@@ -9,7 +9,7 @@
 
 // Import dependencies, sorted by path name.
 const { secureToken, repo } = require('../config.json')
-const { downloadFile, downloadSourceFolder, urlExists, getBranchInfo, getCommitInfo } = require('./download.js')
+const { downloadFile, downloadSourceFolder, urlExists, getBranchInfo, getCommitInfo, isRateLimited } = require('./download.js')
 const { log } = require('./utilities')
 
 const {
@@ -132,6 +132,17 @@ function catchAsyncErrors (asyncFn) {
  * @param {import('express').Request} req Express request object.
  */
 async function handlerDefault (req, res) {
+  // Check if we're currently rate limited before making any GitHub requests
+  const rateLimitCheck = isRateLimited()
+  if (rateLimitCheck.limited) {
+    const result = {
+      ...response.rateLimited,
+      rateLimitRemaining: 0,
+      rateLimitReset: rateLimitCheck.reset
+    }
+    return respondToClient(result, res, req)
+  }
+
   let branch = await getBranch(req.path)
   let url = req.url
   let useGitDownloader = branch === 'master' || /^\/v[0-9]/.test(req.path) // version tags
@@ -288,9 +299,20 @@ async function respondToClient (result, response, request) {
     'Origin, X-Requested-With, Content-Type, Accept'
   )
 
-  // max age one hour
-  response.header('Cache-Control', 'max-age=3600')
-  response.header('CDN-Cache-Control', 'max-age=3600')
+  // For rate limited responses, don't cache and set Retry-After
+  if (status === 429) {
+    response.header('Cache-Control', 'no-store')
+    response.header('CDN-Cache-Control', 'no-store')
+    if (rateLimitReset !== undefined) {
+      const now = Math.floor(Date.now() / 1000)
+      const retryAfter = Math.max(0, rateLimitReset - now)
+      response.header('Retry-After', String(retryAfter))
+    }
+  } else {
+    // max age one hour
+    response.header('Cache-Control', 'max-age=3600')
+    response.header('CDN-Cache-Control', 'max-age=3600')
+  }
 
   if (rateLimitRemaining !== undefined) {
     response.header(RATE_LIMIT_HEADER_REMAINING, String(rateLimitRemaining))

app/interpreter.js

@@ -26,6 +26,7 @@ const BRANCH_TYPES = [
 const PRODUCTS = ['stock', 'maps', 'gantt']
 
 const replaceAll = (str, search, replace) => str.split(search).join(replace)
+const stripQuery = (url) => (typeof url === 'string' ? url.split('?')[0] : '')
 
 /**
  * Finds which branch, tag, or commit that is requested by the client. Defaults
@@ -34,6 +35,7 @@ const replaceAll = (str, search, replace) => str.split(search).join(replace)
  * @param  {string} url The request URL.
  */
 async function getBranch (url) {
+  url = stripQuery(url)
   const folders = ['adapters', 'indicators', 'modules', 'parts-3d', 'parts-map',
     'parts-more', 'parts', 'themes']
   const isValidBranchName = (str) => (
@@ -71,6 +73,7 @@ async function getBranch (url) {
  * @param  {string} url The request URL.
  */
 function getFile (branch, type, url) {
+  url = stripQuery(url)
   // Replace branches in url, since we save by commit sha
   url = url.replace(/^\/master/, '')
   url = url.replace(/^\/v[0-9]+\//, '/')
@@ -173,6 +176,7 @@ function getFileOptions (files, pathJS) {
  * @param  {string} url The request URL.
  */
 const getType = (branch, url) => {
+  url = stripQuery(url)
   const sections = [
     x => x === branch.split('/')[0], // Remove first section of branch name
     x => x === branch.split('/')[1], // Remove second section of branch name

app/message.json

@@ -28,6 +28,10 @@
           "status": 200,
           "body": "Not found"
       },
+      "rateLimited": {
+          "status": 429,
+          "body": "GitHub API rate limit exceeded. Please try again later."
+      },
       "noCache": {
           "status": 202,
           "body": "This branch has no cached files, aborting."

test/download.js

@@ -16,8 +16,12 @@ describe('download.js', () => {
       'urlExists',
       'getBranchInfo',
       'getCommitInfo',
+      'isRateLimited',
+      'getRateLimitState',
       '__setGitHubRequest',
-      '__clearGitHubCache'
+      '__clearGitHubCache',
+      '__clearRateLimitState',
+      '__setRateLimitState'
     ]
     it('should have a default export', () => {
       functions.forEach((name) => {
@@ -174,4 +178,63 @@ describe('download.js', () => {
       expect(second?.sha).to.equal('deadbeef')
     })
   })
+
+  describe('rate limiting', () => {
+    const {
+      isRateLimited,
+      getRateLimitState,
+      __clearRateLimitState,
+      __setRateLimitState
+    } = defaults
+
+    afterEach(() => {
+      __clearRateLimitState()
+    })
+
+    it('isRateLimited returns not limited by default', () => {
+      const result = isRateLimited()
+      expect(result.limited).to.equal(false)
+      expect(result.retryAfter).to.equal(undefined)
+    })
+
+    it('getRateLimitState returns state object', () => {
+      const state = getRateLimitState()
+      expect(state).to.have.property('limited')
+      expect(state).to.have.property('remaining')
+      expect(state).to.have.property('reset')
+      expect(state).to.have.property('retryAfter')
+    })
+
+    it('isRateLimited returns limited when remaining is 0 and reset is in future', () => {
+      const futureReset = Math.floor(Date.now() / 1000) + 60 // 60 seconds in future
+      __setRateLimitState(0, futureReset)
+
+      const result = isRateLimited()
+      expect(result.limited).to.equal(true)
+      expect(result.retryAfter).to.be.a('number')
+      expect(result.retryAfter).to.be.greaterThan(0)
+      expect(result.reset).to.equal(futureReset)
+    })
+
+    it('isRateLimited clears state when reset time has passed', () => {
+      const pastReset = Math.floor(Date.now() / 1000) - 10 // 10 seconds in past
+      __setRateLimitState(0, pastReset)
+
+      const result = isRateLimited()
+      expect(result.limited).to.equal(false)
+
+      // State should be cleared
+      const state = getRateLimitState()
+      expect(state.remaining).to.equal(undefined)
+      expect(state.reset).to.equal(undefined)
+    })
+
+    it('isRateLimited returns not limited when remaining is greater than 0', () => {
+      const futureReset = Math.floor(Date.now() / 1000) + 60
+      __setRateLimitState(5, futureReset)
+
+      const result = isRateLimited()
+      expect(result.limited).to.equal(false)
+    })
+  })
 })

test/interpreter.js

@@ -47,6 +47,11 @@ describe('interpreter.js', () => {
       expect(await getBranch('/6.0.7'))
         .to.equal('6.0.7')
     })
+
+    it('should ignore query strings', async () => {
+      expect(await getBranch('/feature/new-api/modules/exporting.src.js?cache=123'))
+        .to.equal('feature/new-api')
+    })
   })
 
   describe('getType', () => {
@@ -66,6 +71,11 @@ describe('interpreter.js', () => {
       expect(getType('6.0.7', '/6.0.7'))
         .to.equal('classic')
     })
+
+    it('should ignore query strings', () => {
+      expect(getType('bugfix', '/bugfix/js/modules/exporting.src.js?cache=123'))
+        .to.equal('css')
+    })
   })
 
   describe('getFile', () => {
@@ -91,6 +101,11 @@ describe('interpreter.js', () => {
       expect(getFile('6.0.7', 'classic', '/6.0.7'))
         .to.equal(false)
     })
+
+    it('should ignore query strings', () => {
+      expect(getFile('master', 'classic', '/highcharts.js?cache=123'))
+        .to.equal('highcharts.src.js')
+    })
   })
 
   // describe('getFileOptions', () => {