From 8df13751aa1b31cd00d64001b8c18480b7e0840d Mon Sep 17 00:00:00 2001 From: Chris Opperwall Date: Tue, 15 Sep 2020 14:19:28 -0700 Subject: [PATCH] fix(app authentication): Handle time difference between system and GitHub API time in JWT claims (#164) Co-authored-by: Gregor Martynus --- src/auth.ts | 2 +- src/get-app-authentication.ts | 17 +- src/get-installation-authentication.ts | 5 +- src/hook.ts | 50 ++++- src/types.ts | 1 + test/index.test.ts | 263 +++++++++++++++++++++++++ 6 files changed, 325 insertions(+), 13 deletions(-) diff --git a/src/auth.ts b/src/auth.ts index b329ed66c..f24205db3 100644 --- a/src/auth.ts +++ b/src/auth.ts @@ -12,7 +12,7 @@ export async function auth( options: AuthOptions ): Promise { if (options.type === "app") { - return getAppAuthentication(state.id, state.privateKey); + return getAppAuthentication(state); } if (options.type === "installation") { diff --git a/src/get-app-authentication.ts b/src/get-app-authentication.ts index b4170ac5e..bd45769d6 100644 --- a/src/get-app-authentication.ts +++ b/src/get-app-authentication.ts @@ -1,12 +1,17 @@ import { githubAppJwt } from "universal-github-app-jwt"; -import { AppAuthentication } from "./types"; +import { AppAuthentication, State } from "./types"; -export async function getAppAuthentication( - id: number, - privateKey: string -): Promise { - const appAuthentication = await githubAppJwt({ id, privateKey }); +export async function getAppAuthentication({ + id, + privateKey, + timeDifference, +}: State): Promise { + const appAuthentication = await githubAppJwt({ + id, + privateKey, + now: timeDifference && Math.floor(Date.now() / 1000) + timeDifference, + }); return { type: "app", diff --git a/src/get-installation-authentication.ts b/src/get-installation-authentication.ts index 22a2ddad5..78e5bd61f 100644 --- a/src/get-installation-authentication.ts +++ b/src/get-installation-authentication.ts @@ -56,10 +56,7 @@ export async function getInstallationAuthentication( } } - const appAuthentication = await getAppAuthentication( - state.id, - state.privateKey - ); + const appAuthentication = await getAppAuthentication(state); const request = customRequest || state.request; const { diff --git a/src/hook.ts b/src/hook.ts index c9e594a17..491639519 100644 --- a/src/hook.ts +++ b/src/hook.ts @@ -9,9 +9,21 @@ import { Route, State, } from "./types"; +import { RequestError } from "@octokit/request-error"; const FIVE_SECONDS_IN_MS = 5 * 1000; +function isNotTimeSkewError(error: RequestError) { + return !( + error.message.match( + /'Expiration time' claim \('exp'\) must be a numeric value representing the future time at which the assertion expires/ + ) || + error.message.match( + /'Issued at' claim \('iat'\) must be an Integer representing the time that the assertion was issued/ + ) + ); +} + export async function hook( state: State, request: RequestInterface, @@ -25,10 +37,44 @@ export async function hook( (endpoint.url as string).replace(request.endpoint.DEFAULTS.baseUrl, "") ) ) { - const { token } = await getAppAuthentication(state.id, state.privateKey); + const { token } = await getAppAuthentication(state); endpoint.headers.authorization = `bearer ${token}`; - return request(endpoint as EndpointOptions); + let response; + try { + response = await request(endpoint as EndpointOptions); + } catch (error) { + // If there's an issue with the expiration, regenerate the token and try again. + // Otherwise rethrow the error for upstream handling. + if (isNotTimeSkewError(error)) { + throw error; + } + + // If the date header is missing, we can't correct the system time skew. + // Throw the error to be handled upstream. + if (typeof error.headers.date === "undefined") { + throw error; + } + + const diff = Math.floor( + (Date.parse(error.headers.date) - Date.parse(new Date().toString())) / + 1000 + ); + + console.warn(error.message); + console.warn( + `[@octokit/auth-app] GitHub API time and system time are different by ${diff} seconds. Retrying request with the difference accounted for.` + ); + + const { token } = await getAppAuthentication({ + ...state, + timeDifference: diff, + }); + endpoint.headers.authorization = `bearer ${token}`; + return request(endpoint as EndpointOptions); + } + + return response; } const { token, createdAt } = await getInstallationAuthentication( diff --git a/src/types.ts b/src/types.ts index b2c3b02c8..fb3b72ccd 100644 --- a/src/types.ts +++ b/src/types.ts @@ -81,6 +81,7 @@ export type StrategyOptions = { clientSecret?: string; request?: OctokitTypes.RequestInterface; cache?: Cache; + timeDifference?: number; }; export type StrategyOptionsWithDefaults = StrategyOptions & { diff --git a/test/index.test.ts b/test/index.test.ts index ec3b68208..76991d9af 100644 --- a/test/index.test.ts +++ b/test/index.test.ts @@ -1210,6 +1210,269 @@ test("auth.hook() uses app auth for marketplace URL", async () => { expect(mock.done()).toBe(true); }); +test("auth.hook(): handle 401 due to an exp timestamp in the past", async () => { + const mock = fetchMock + .sandbox() + .get("https://api.github.com/app", (_url, options) => { + const auth = (options.headers as { [key: string]: string | undefined })[ + "authorization" + ]; + const [_, jwt] = (auth || "").split(" "); + const payload = JSON.parse(atob(jwt.split(".")[1])); + + // By default the mocked time will set the payload to 570 (10 minutes - 30 seconds in seconds) + // By returning an error for that exp with an API time of 30 seconds in the future, + // the new request will be made with a JWT that has an expiration set 30 seconds further in the future. + if (payload.exp < 600) { + return { + status: 401, + body: { + message: + "'Expiration time' claim ('exp') must be a numeric value representing the future time at which the assertion expires.", + documentation_url: "https://developer.github.com/v3", + }, + headers: { + date: new Date(Date.now() + 30000).toUTCString(), + }, + }; + } + + return { + status: 200, + body: [], + }; + }); + + const auth = createAppAuth({ + id: APP_ID, + privateKey: PRIVATE_KEY, + }); + + const requestWithMock = request.defaults({ + request: { + fetch: mock, + }, + }); + const requestWithAuth = requestWithMock.defaults({ + request: { + hook: auth.hook, + }, + }); + + global.console.warn = jest.fn(); + + const promise = requestWithAuth("GET /app"); + + const { data } = await promise; + + expect(data).toStrictEqual([]); + expect(mock.done()).toBe(true); + + // @ts-ignore + expect(global.console.warn.mock.calls.length).toEqual(2); + expect(global.console.warn).toHaveBeenNthCalledWith( + 1, + "'Expiration time' claim ('exp') must be a numeric value representing the future time at which the assertion expires." + ); + expect(global.console.warn).toHaveBeenNthCalledWith( + 2, + `[@octokit/auth-app] GitHub API time and system time are different by 30 seconds. Retrying request with the difference accounted for.` + ); +}); + +test("auth.hook(): handle 401 due to an exp timestamp in the past with 800 second clock skew", async () => { + const fakeTimeMs = 1029392939; + const githubTimeMs = fakeTimeMs + 800000; + clock = install({ now: fakeTimeMs, toFake: ["Date", "setTimeout"] }); + const mock = fetchMock + .sandbox() + .get("https://api.github.com/app", (_url, options) => { + const auth = (options.headers as { [key: string]: string | undefined })[ + "authorization" + ]; + const [_, jwt] = (auth || "").split(" "); + const payload = JSON.parse(atob(jwt.split(".")[1])); + + // The first request will send an expiration that is 200 seconds before GitHub's mocked API time. + // The second request will send an adjusted expiration claim based on the 800 seconds skew and trigger a 200 response. + if (payload.exp <= Math.floor(githubTimeMs / 1000)) { + return { + status: 401, + body: { + message: + "'Expiration time' claim ('exp') must be a numeric value representing the future time at which the assertion expires.", + documentation_url: "https://developer.github.com/v3", + }, + headers: { + date: new Date(githubTimeMs).toUTCString(), + }, + }; + } + + return { + status: 200, + body: [], + }; + }); + + const auth = createAppAuth({ + id: APP_ID, + privateKey: PRIVATE_KEY, + }); + + const requestWithMock = request.defaults({ + request: { + fetch: mock, + }, + }); + const requestWithAuth = requestWithMock.defaults({ + request: { + hook: auth.hook, + }, + }); + + global.console.warn = jest.fn(); + + const promise = requestWithAuth("GET /app"); + + const { data } = await promise; + + expect(data).toStrictEqual([]); + expect(mock.done()).toBe(true); + + // @ts-ignore + expect(global.console.warn.mock.calls.length).toEqual(2); + expect(global.console.warn).toHaveBeenNthCalledWith( + 1, + "'Expiration time' claim ('exp') must be a numeric value representing the future time at which the assertion expires." + ); + expect(global.console.warn).toHaveBeenNthCalledWith( + 2, + `[@octokit/auth-app] GitHub API time and system time are different by 800 seconds. Retrying request with the difference accounted for.` + ); +}); + +test("auth.hook(): handle 401 due to an iat timestamp in the future", async () => { + const mock = fetchMock + .sandbox() + .get("https://api.github.com/app", (_url, options) => { + const auth = (options.headers as { [key: string]: string | undefined })[ + "authorization" + ]; + const [_, jwt] = (auth || "").split(" "); + const payload = JSON.parse(atob(jwt.split(".")[1])); + + // By default the mocked time will set the payload.iat to -30. + // By returning an error for that exp with an API time of 30 seconds in the future, + // the new request will be made with a JWT that has an issued_at set 30 seconds further in the future. + if (payload.iat < 0) { + return { + status: 401, + body: { + message: + "'Issued at' claim ('iat') must be an Integer representing the time that the assertion was issued.", + documentation_url: "https://developer.github.com/v3", + }, + headers: { + date: new Date(Date.now() + 30000).toUTCString(), + }, + }; + } + + return { + status: 200, + body: [], + }; + }); + + const auth = createAppAuth({ + id: APP_ID, + privateKey: PRIVATE_KEY, + }); + + const requestWithMock = request.defaults({ + request: { + fetch: mock, + }, + }); + const requestWithAuth = requestWithMock.defaults({ + request: { + hook: auth.hook, + }, + }); + + global.console.warn = jest.fn(); + + const promise = requestWithAuth("GET /app"); + const { data } = await promise; + + expect(data).toStrictEqual([]); + expect(mock.done()).toBe(true); + + // @ts-ignore + expect(global.console.warn.mock.calls.length).toEqual(2); + expect(global.console.warn).toHaveBeenNthCalledWith( + 1, + "'Issued at' claim ('iat') must be an Integer representing the time that the assertion was issued." + ); + expect(global.console.warn).toHaveBeenNthCalledWith( + 2, + `[@octokit/auth-app] GitHub API time and system time are different by 30 seconds. Retrying request with the difference accounted for.` + ); +}); + +test("auth.hook(): throw 401 error in app auth flow without timing errors", async () => { + const mock = fetchMock + .sandbox() + .get("https://api.github.com/app", { + status: 401, + body: { + message: "Bad credentials", + documentation_url: "https://developer.github.com/v3", + }, + }) + .get("https://api.github.com/marketplace_listing/plan", { + status: 401, + body: { + message: + "'Issued at' claim ('iat') must be an Integer representing the time that the assertion was issued.", + documentation_url: "https://developer.github.com/v3", + }, + }); + + const auth = createAppAuth({ + id: APP_ID, + privateKey: PRIVATE_KEY, + }); + + const requestWithMock = request.defaults({ + request: { + fetch: mock, + }, + }); + const requestWithAuth = requestWithMock.defaults({ + request: { + hook: auth.hook, + }, + }); + + global.console.warn = jest.fn(); + + try { + await requestWithAuth("GET /app"); + throw new Error("Should not resolve"); + } catch (error) { + expect(error.status).toEqual(401); + } + + try { + await requestWithAuth("GET /marketplace_listing/plan"); + throw new Error("Should not resolve"); + } catch (error) { + expect(error.status).toEqual(401); + } +}); + test("auth.hook(): handle 401 in first 5 seconds (#65)", async () => { const FIVE_SECONDS_IN_MS = 1000 * 5;