From e27b7f32072b96c6894f020f343d0349125803e3 Mon Sep 17 00:00:00 2001 From: secustor Date: Mon, 15 Jan 2024 15:39:13 +0100 Subject: [PATCH 1/5] fix(Github): use `x-ratelimit-remaining` and 429 status code for rate limit detection Signed-off-by: secustor --- .changeset/sweet-ravens-glow.md | 6 +++++ .../src/reading/GithubUrlReader.ts | 15 +++++++---- .../src/github/GithubIntegration.test.ts | 25 +++++++++++++++++++ .../src/github/GithubIntegration.ts | 8 ++++++ 4 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 .changeset/sweet-ravens-glow.md diff --git a/.changeset/sweet-ravens-glow.md b/.changeset/sweet-ravens-glow.md new file mode 100644 index 0000000000..045f7edbf2 --- /dev/null +++ b/.changeset/sweet-ravens-glow.md @@ -0,0 +1,6 @@ +--- +'@backstage/integration': minor +'@backstage/backend-common': patch +--- + +Fix rate limit detection by looking for HTTP status code 429 and updating the header `x-ratelimit-remaining` to look for in case of a 403 code is returned diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index 764431386d..83c21b2529 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -149,10 +149,7 @@ export class GithubUrlReader implements UrlReader { // GitHub returns a 403 response with a couple of headers indicating rate // limit status. See more in the GitHub docs: // https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting - if ( - response.status === 403 && - response.headers.get('X-RateLimit-Remaining') === '0' - ) { + if (this.integration.isRateLimited(response)) { message += ' (rate limit exceeded)'; } @@ -350,10 +347,18 @@ export class GithubUrlReader implements UrlReader { const response = await fetch(urlAsString, init); if (!response.ok) { - const message = `Request failed for ${urlAsString}, ${response.status} ${response.statusText}`; + let message = `Request failed for ${urlAsString}, ${response.status} ${response.statusText}`; if (response.status === 404) { throw new NotFoundError(message); } + + // GitHub returns a 403 response with a couple of headers indicating rate + // limit status. See more in the GitHub docs: + // https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting + if (this.integration.isRateLimited(response)) { + message += ' (rate limit exceeded)'; + } + throw new Error(message); } diff --git a/packages/integration/src/github/GithubIntegration.test.ts b/packages/integration/src/github/GithubIntegration.test.ts index d8e6020a64..36c802da52 100644 --- a/packages/integration/src/github/GithubIntegration.test.ts +++ b/packages/integration/src/github/GithubIntegration.test.ts @@ -78,6 +78,31 @@ describe('GithubIntegration', () => { ), ).toBe('https://github.com/backstage/backstage/edit/master/README.md'); }); + + describe('isRateLimited', () => { + const integration = new GithubIntegration({ host: 'h.com' }); + + it.each` + status | ratelimitRemaining | expected + ${404} | ${100} | ${false} + ${429} | ${undefined} | ${true} + ${429} | ${100} | ${true} + ${403} | ${100} | ${false} + ${403} | ${0} | ${true} + `( + '(statusCode: $status, header: $ratelimitRemaining) === $expected', + ({ status, ratelimitRemaining, expected }) => { + const headers = new Headers({ + 'x-ratelimit-remaining': ratelimitRemaining, + }); + const result = integration.isRateLimited({ + status, + headers, + } as Response); + expect(expected).toBe(result); + }, + ); + }); }); describe('replaceGithubUrlType', () => { diff --git a/packages/integration/src/github/GithubIntegration.ts b/packages/integration/src/github/GithubIntegration.ts index 50fc129ccf..e2b163dc2d 100644 --- a/packages/integration/src/github/GithubIntegration.ts +++ b/packages/integration/src/github/GithubIntegration.ts @@ -65,6 +65,14 @@ export class GithubIntegration implements ScmIntegration { resolveEditUrl(url: string): string { return replaceGithubUrlType(url, 'edit'); } + + isRateLimited(response: Response): boolean { + return ( + response.status === 429 || + (response.status === 403 && + response.headers.get('x-ratelimit-remaining') === '0') + ); + } } /** From 633250f0e5b8023c19492b4ab351b51bd3ab0313 Mon Sep 17 00:00:00 2001 From: secustor Date: Mon, 15 Jan 2024 16:03:43 +0100 Subject: [PATCH 2/5] chore: use ConsumedResponse to pass eslint validation Signed-off-by: secustor --- packages/integration/src/github/GithubIntegration.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/integration/src/github/GithubIntegration.ts b/packages/integration/src/github/GithubIntegration.ts index e2b163dc2d..7a6ac5675d 100644 --- a/packages/integration/src/github/GithubIntegration.ts +++ b/packages/integration/src/github/GithubIntegration.ts @@ -20,6 +20,7 @@ import { GithubIntegrationConfig, readGithubIntegrationConfigs, } from './config'; +import { ConsumedResponse } from '@backstage/errors'; /** * A GitHub based integration. @@ -66,7 +67,7 @@ export class GithubIntegration implements ScmIntegration { return replaceGithubUrlType(url, 'edit'); } - isRateLimited(response: Response): boolean { + isRateLimited(response: ConsumedResponse): boolean { return ( response.status === 429 || (response.status === 403 && From 7120dfcb871c815955ad6c709b35a98fc93e5618 Mon Sep 17 00:00:00 2001 From: secustor Date: Mon, 15 Jan 2024 16:11:30 +0100 Subject: [PATCH 3/5] fixup! chore: use ConsumedResponse to pass eslint validation Signed-off-by: secustor --- packages/integration/package.json | 1 + yarn.lock | 1 + 2 files changed, 2 insertions(+) diff --git a/packages/integration/package.json b/packages/integration/package.json index 7e57293903..29a4273698 100644 --- a/packages/integration/package.json +++ b/packages/integration/package.json @@ -35,6 +35,7 @@ "dependencies": { "@azure/identity": "^4.0.0", "@backstage/config": "workspace:^", + "@backstage/errors": "workspace:^", "@octokit/auth-app": "^4.0.0", "@octokit/rest": "^19.0.3", "cross-fetch": "^4.0.0", diff --git a/yarn.lock b/yarn.lock index 230f6d3e8d..7740ad7363 100644 --- a/yarn.lock +++ b/yarn.lock @@ -4285,6 +4285,7 @@ __metadata: "@backstage/cli": "workspace:^" "@backstage/config": "workspace:^" "@backstage/config-loader": "workspace:^" + "@backstage/errors": "workspace:^" "@octokit/auth-app": ^4.0.0 "@octokit/rest": ^19.0.3 "@types/luxon": ^3.0.0 From 626e790ec9c2b7f5b1b368e24d66dcaa08098a58 Mon Sep 17 00:00:00 2001 From: secustor Date: Mon, 15 Jan 2024 16:15:44 +0100 Subject: [PATCH 4/5] fixup! chore: use ConsumedResponse to pass eslint validation Signed-off-by: secustor --- packages/integration/api-report.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/integration/api-report.md b/packages/integration/api-report.md index 634e03dbfc..f830437cf1 100644 --- a/packages/integration/api-report.md +++ b/packages/integration/api-report.md @@ -4,6 +4,7 @@ ```ts import { Config } from '@backstage/config'; +import { ConsumedResponse } from '@backstage/errors'; import { RestEndpointMethodTypes } from '@octokit/rest'; // @public @@ -567,6 +568,8 @@ export class GithubIntegration implements ScmIntegration { // (undocumented) static factory: ScmIntegrationsFactory; // (undocumented) + isRateLimited(response: ConsumedResponse): boolean; + // (undocumented) resolveEditUrl(url: string): string; // (undocumented) resolveUrl(options: { From 75b51a87b6b392616a28cde39fb30de0918b5e1f Mon Sep 17 00:00:00 2001 From: secustor Date: Thu, 25 Jan 2024 19:32:54 +0100 Subject: [PATCH 5/5] return interface instead of boolean Signed-off-by: secustor --- .../src/reading/GithubUrlReader.ts | 2 +- packages/integration/api-report.md | 8 +++++++- .../src/github/GithubIntegration.test.ts | 6 ++++-- .../src/github/GithubIntegration.ts | 19 ++++++++++++------- packages/integration/src/index.ts | 1 + packages/integration/src/types.ts | 9 +++++++++ 6 files changed, 34 insertions(+), 11 deletions(-) diff --git a/packages/backend-common/src/reading/GithubUrlReader.ts b/packages/backend-common/src/reading/GithubUrlReader.ts index edd5269649..462821a20f 100644 --- a/packages/backend-common/src/reading/GithubUrlReader.ts +++ b/packages/backend-common/src/reading/GithubUrlReader.ts @@ -333,7 +333,7 @@ export class GithubUrlReader implements UrlReader { // GitHub returns a 403 response with a couple of headers indicating rate // limit status. See more in the GitHub docs: // https://docs.github.com/en/rest/overview/resources-in-the-rest-api#rate-limiting - if (this.integration.isRateLimited(response)) { + if (this.integration.parseRateLimitInfo(response).isRateLimited) { message += ' (rate limit exceeded)'; } diff --git a/packages/integration/api-report.md b/packages/integration/api-report.md index f830437cf1..07b4f7d012 100644 --- a/packages/integration/api-report.md +++ b/packages/integration/api-report.md @@ -568,7 +568,7 @@ export class GithubIntegration implements ScmIntegration { // (undocumented) static factory: ScmIntegrationsFactory; // (undocumented) - isRateLimited(response: ConsumedResponse): boolean; + parseRateLimitInfo(response: ConsumedResponse): RateLimitInfo; // (undocumented) resolveEditUrl(url: string): string; // (undocumented) @@ -697,6 +697,12 @@ export type PersonalAccessTokenCredential = AzureCredentialBase & { personalAccessToken: string; }; +// @public +export interface RateLimitInfo { + // (undocumented) + isRateLimited: boolean; +} + // @public export function readAwsS3IntegrationConfig( config: Config, diff --git a/packages/integration/src/github/GithubIntegration.test.ts b/packages/integration/src/github/GithubIntegration.test.ts index 36c802da52..70f1c4a16c 100644 --- a/packages/integration/src/github/GithubIntegration.test.ts +++ b/packages/integration/src/github/GithubIntegration.test.ts @@ -95,11 +95,13 @@ describe('GithubIntegration', () => { const headers = new Headers({ 'x-ratelimit-remaining': ratelimitRemaining, }); - const result = integration.isRateLimited({ + const result = integration.parseRateLimitInfo({ status, headers, } as Response); - expect(expected).toBe(result); + expect(result).toMatchObject({ + isRateLimited: expected, + }); }, ); }); diff --git a/packages/integration/src/github/GithubIntegration.ts b/packages/integration/src/github/GithubIntegration.ts index 7a6ac5675d..880a2bed21 100644 --- a/packages/integration/src/github/GithubIntegration.ts +++ b/packages/integration/src/github/GithubIntegration.ts @@ -15,7 +15,11 @@ */ import { basicIntegrations, defaultScmResolveUrl } from '../helpers'; -import { ScmIntegration, ScmIntegrationsFactory } from '../types'; +import { + RateLimitInfo, + ScmIntegration, + ScmIntegrationsFactory, +} from '../types'; import { GithubIntegrationConfig, readGithubIntegrationConfigs, @@ -67,12 +71,13 @@ export class GithubIntegration implements ScmIntegration { return replaceGithubUrlType(url, 'edit'); } - isRateLimited(response: ConsumedResponse): boolean { - return ( - response.status === 429 || - (response.status === 403 && - response.headers.get('x-ratelimit-remaining') === '0') - ); + parseRateLimitInfo(response: ConsumedResponse): RateLimitInfo { + return { + isRateLimited: + response.status === 429 || + (response.status === 403 && + response.headers.get('x-ratelimit-remaining') === '0'), + }; } } diff --git a/packages/integration/src/index.ts b/packages/integration/src/index.ts index 5738da3da0..700d7dc67c 100644 --- a/packages/integration/src/index.ts +++ b/packages/integration/src/index.ts @@ -37,5 +37,6 @@ export type { ScmIntegration, ScmIntegrationsFactory, ScmIntegrationsGroup, + RateLimitInfo, } from './types'; export type { ScmIntegrationRegistry } from './registry'; diff --git a/packages/integration/src/types.ts b/packages/integration/src/types.ts index cb4b78eb7c..0f0fca942e 100644 --- a/packages/integration/src/types.ts +++ b/packages/integration/src/types.ts @@ -108,3 +108,12 @@ export interface ScmIntegrationsGroup { export type ScmIntegrationsFactory = (options: { config: Config; }) => ScmIntegrationsGroup; + +/** + * Encapsulates information about the RateLimit state + * + * @public + */ +export interface RateLimitInfo { + isRateLimited: boolean; +}