From 0c014070f93045fed9b48f568f28b2f1cca37088 Mon Sep 17 00:00:00 2001 From: Josh Gross Date: Fri, 19 Jan 2024 10:45:12 -0500 Subject: [PATCH] fix(revocation): avoid revoking expired tokens and fail gracefully (#95) Fixes #72 If an Actions job is long enough, more than an hour can pass between creating and revoking the App token in the post-job clean up step. Since the token itself is used to authenticate with the revoke API, an expired token will fail to be revoked. This PR saves the token expiration in the actions state and uses that in the post step to determine if the token can be revoked. I've also added error handling to the revoke token API call, as it's unlikely that users would want their job to fail if the token can't be revoked. --- dist/main.cjs | 1 + dist/post.cjs | 28 ++++++++-- lib/main.js | 1 + lib/post.js | 32 +++++++++-- tests/main.js | 3 +- tests/post-revoke-token-fail-response.test.js | 28 ++++++++++ tests/post-token-expired.test.js | 28 ++++++++++ tests/post-token-set.test.js | 3 + tests/snapshots/index.js.md | 52 +++++++++++++++--- tests/snapshots/index.js.snap | Bin 955 -> 1060 bytes 10 files changed, 155 insertions(+), 21 deletions(-) create mode 100644 tests/post-revoke-token-fail-response.test.js create mode 100644 tests/post-token-expired.test.js diff --git a/dist/main.cjs b/dist/main.cjs index 2c5955f..0629cdb 100644 --- a/dist/main.cjs +++ b/dist/main.cjs @@ -10420,6 +10420,7 @@ async function main(appId2, privateKey2, owner2, repositories2, core2, createApp core2.setOutput("token", authentication.token); if (!skipTokenRevoke2) { core2.saveState("token", authentication.token); + core2.setOutput("expiresAt", authentication.expiresAt); } } async function getTokenFromOwner(request2, auth, parsedOwner) { diff --git a/dist/post.cjs b/dist/post.cjs index 44bde25..ee87366 100644 --- a/dist/post.cjs +++ b/dist/post.cjs @@ -3003,12 +3003,28 @@ async function post(core2, request2) { core2.info("Token is not set"); return; } - await request2("DELETE /installation/token", { - headers: { - authorization: `token ${token}` - } - }); - core2.info("Token revoked"); + const expiresAt = core2.getState("expiresAt"); + if (expiresAt && tokenExpiresIn(expiresAt) < 0) { + core2.info("Token already expired"); + return; + } + try { + await request2("DELETE /installation/token", { + headers: { + authorization: `token ${token}` + } + }); + core2.info("Token revoked"); + } catch (error) { + core2.warning( + `Token revocation failed: ${error.message}` + ); + } +} +function tokenExpiresIn(expiresAt) { + const now = /* @__PURE__ */ new Date(); + const expiresAtDate = new Date(expiresAt); + return Math.round((expiresAtDate.getTime() - now.getTime()) / 1e3); } // lib/request.js diff --git a/lib/main.js b/lib/main.js index 233be3d..b97329d 100644 --- a/lib/main.js +++ b/lib/main.js @@ -103,6 +103,7 @@ export async function main( // Make token accessible to post function (so we can invalidate it) if (!skipTokenRevoke) { core.saveState("token", authentication.token); + core.setOutput("expiresAt", authentication.expiresAt); } } diff --git a/lib/post.js b/lib/post.js index e321294..9b294ae 100644 --- a/lib/post.js +++ b/lib/post.js @@ -21,11 +21,31 @@ export async function post(core, request) { return; } - await request("DELETE /installation/token", { - headers: { - authorization: `token ${token}`, - }, - }); + const expiresAt = core.getState("expiresAt"); + if (expiresAt && tokenExpiresIn(expiresAt) < 0) { + core.info("Token expired, skipping token revocation"); + return; + } + + try { + await request("DELETE /installation/token", { + headers: { + authorization: `token ${token}`, + }, + }); + core.info("Token revoked"); + } catch (error) { + core.warning( + `Token revocation failed: ${error.message}`) + } +} + +/** + * @param {string} expiresAt + */ +function tokenExpiresIn(expiresAt) { + const now = new Date(); + const expiresAtDate = new Date(expiresAt); - core.info("Token revoked"); + return Math.round((expiresAtDate.getTime() - now.getTime()) / 1000); } diff --git a/tests/main.js b/tests/main.js index 12c8437..6aba464 100644 --- a/tests/main.js +++ b/tests/main.js @@ -72,6 +72,7 @@ x3WQZRiXlWejSMUAHuMwXrhGlltF3lw83+xAjnqsVp75kGS6OH61 // Mock installation access token request const mockInstallationAccessToken = "ghs_16C7e42F292c6912E7710c838347Ae178B4a"; // This token is invalidated. It’s from https://docs.github.com/en/rest/apps/apps?apiVersion=2022-11-28#create-an-installation-access-token-for-an-app. + const mockExpiresAt = "2016-07-11T22:14:10Z"; mockPool .intercept({ path: `/app/installations/${mockInstallationId}/access_tokens`, @@ -84,7 +85,7 @@ x3WQZRiXlWejSMUAHuMwXrhGlltF3lw83+xAjnqsVp75kGS6OH61 }) .reply( 201, - { token: mockInstallationAccessToken }, + { token: mockInstallationAccessToken, expires_at: mockExpiresAt }, { headers: { "content-type": "application/json" } } ); diff --git a/tests/post-revoke-token-fail-response.test.js b/tests/post-revoke-token-fail-response.test.js new file mode 100644 index 0000000..5ce31ec --- /dev/null +++ b/tests/post-revoke-token-fail-response.test.js @@ -0,0 +1,28 @@ +import { MockAgent, setGlobalDispatcher } from "undici"; + +// state variables are set as environment variables with the prefix STATE_ +// https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#sending-values-to-the-pre-and-post-actions +process.env.STATE_token = "secret123"; + +// 1 hour in the future, not expired +process.env.STATE_expiresAt = new Date(Date.now() + 1000 * 60 * 60).toISOString(); + +const mockAgent = new MockAgent(); + +setGlobalDispatcher(mockAgent); + +// Provide the base url to the request +const mockPool = mockAgent.get("https://api.github.com"); + +// intercept the request +mockPool + .intercept({ + path: "/installation/token", + method: "DELETE", + headers: { + authorization: "token secret123", + }, + }) + .reply(401); + +await import("../post.js"); diff --git a/tests/post-token-expired.test.js b/tests/post-token-expired.test.js new file mode 100644 index 0000000..6479845 --- /dev/null +++ b/tests/post-token-expired.test.js @@ -0,0 +1,28 @@ +import { MockAgent, setGlobalDispatcher } from "undici"; + +// state variables are set as environment variables with the prefix STATE_ +// https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#sending-values-to-the-pre-and-post-actions +process.env.STATE_token = "secret123"; + +// 1 hour in the past, expired +process.env.STATE_expiresAt = new Date(Date.now() - 1000 * 60 * 60).toISOString(); + +const mockAgent = new MockAgent(); + +setGlobalDispatcher(mockAgent); + +// Provide the base url to the request +const mockPool = mockAgent.get("https://api.github.com"); + +// intercept the request +mockPool + .intercept({ + path: "/installation/token", + method: "DELETE", + headers: { + authorization: "token secret123", + }, + }) + .reply(204); + +await import("../post.js"); diff --git a/tests/post-token-set.test.js b/tests/post-token-set.test.js index 6ed0494..5b2479e 100644 --- a/tests/post-token-set.test.js +++ b/tests/post-token-set.test.js @@ -4,6 +4,9 @@ import { MockAgent, setGlobalDispatcher } from "undici"; // https://docs.github.com/en/actions/using-workflows/workflow-commands-for-github-actions#sending-values-to-the-pre-and-post-actions process.env.STATE_token = "secret123"; +// 1 hour in the future, not expired +process.env.STATE_expiresAt = new Date(Date.now() + 1000 * 60 * 60).toISOString(); + const mockAgent = new MockAgent(); setGlobalDispatcher(mockAgent); diff --git a/tests/snapshots/index.js.md b/tests/snapshots/index.js.md index e7c6e86..50a5112 100644 --- a/tests/snapshots/index.js.md +++ b/tests/snapshots/index.js.md @@ -69,7 +69,9 @@ Generated by [AVA](https://avajs.dev). ::add-mask::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ ␊ ::set-output name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ - ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a` + ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ + ␊ + ::set-output name=expiresAt::2016-07-11T22:14:10Z` ## main-token-get-owner-set-repo-set-to-many.test.js @@ -83,7 +85,9 @@ Generated by [AVA](https://avajs.dev). ::add-mask::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ ␊ ::set-output name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ - ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a` + ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ + ␊ + ::set-output name=expiresAt::2016-07-11T22:14:10Z` ## main-token-get-owner-set-repo-set-to-one.test.js @@ -97,7 +101,9 @@ Generated by [AVA](https://avajs.dev). ::add-mask::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ ␊ ::set-output name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ - ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a` + ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ + ␊ + ::set-output name=expiresAt::2016-07-11T22:14:10Z` ## main-token-get-owner-set-to-org-repo-unset.test.js @@ -111,7 +117,9 @@ Generated by [AVA](https://avajs.dev). ::add-mask::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ ␊ ::set-output name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ - ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a` + ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ + ␊ + ::set-output name=expiresAt::2016-07-11T22:14:10Z` ## main-token-get-owner-set-to-user-fail-response.test.js @@ -126,7 +134,9 @@ Generated by [AVA](https://avajs.dev). ::add-mask::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ ␊ ::set-output name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ - ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a` + ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ + ␊ + ::set-output name=expiresAt::2016-07-11T22:14:10Z` ## main-token-get-owner-set-to-user-repo-unset.test.js @@ -140,7 +150,9 @@ Generated by [AVA](https://avajs.dev). ::add-mask::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ ␊ ::set-output name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ - ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a` + ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ + ␊ + ::set-output name=expiresAt::2016-07-11T22:14:10Z` ## main-token-get-owner-unset-repo-set.test.js @@ -154,7 +166,9 @@ Generated by [AVA](https://avajs.dev). ::add-mask::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ ␊ ::set-output name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ - ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a` + ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ + ␊ + ::set-output name=expiresAt::2016-07-11T22:14:10Z` ## main-token-get-owner-unset-repo-unset.test.js @@ -168,7 +182,29 @@ Generated by [AVA](https://avajs.dev). ::add-mask::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ ␊ ::set-output name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ - ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a` + ::save-state name=token::ghs_16C7e42F292c6912E7710c838347Ae178B4a␊ + ␊ + ::set-output name=expiresAt::2016-07-11T22:14:10Z` + +## post-revoke-token-fail-response.test.js + +> stderr + + '' + +> stdout + + '::warning::Token revocation failed: ' + +## post-token-expired.test.js + +> stderr + + '' + +> stdout + + 'Token expired, skipping token revocation' ## post-token-set.test.js diff --git a/tests/snapshots/index.js.snap b/tests/snapshots/index.js.snap index f7989c60e0237f307e83e20af6fca8e5ef7c89d1..47808b0eb787387e49ea9ca65bfd3e6451f456ba 100644 GIT binary patch literal 1060 zcmV+<1l#*TRzVQq_UN>STu^@jIt)OP3QH@o z*UdRcP#=p300000000B+n9pz9HWbI>6~ll41=icJ4jT~+aJpKlrDS#@v_;b)O>)>$ zWGR*o7(Ci~c34ZIN>WY=^ib@&+pxp1TYKsM)xTu?A=~02*=dvk18QF^P}F;0z0X&C z5?}YbA?*$MA8$Yq5yBb5Jwa&f1duT3VF7_d;v^M(O#l~b&p6>RPWTBG{I@qjfCc{k z+R`UW>R0=C>BbU-xC>#QivSoi6Q0&^e#%FOHJl_J5}|Bt{tZQ;{-2BVb(0TtiPd=opTU5pw}JSX)jQ8Ob=EA-phIas$;;Wq}+M zXq=A^NzxH?2yutONdB9w=Z350R^c2S&Y6VE6imxbQ!!?FGr}ZxBEmU|2aYLHDaPY( z^N?!weMMc0dY%%NDUGQ>JWZ02K;R=2uYh-%A#%z-x0KP-7+ATmT}#;3?*7r&G(D$Qk!x~>HqD-S>z2RJf?7Vq>i3FQ$_Xw0F? z!t>Tzc>b#CJet6yNCddLw+8Alz(U?K(y~yWGJDdBQYDu|HMoLQqyTkapcS)V@|ERB z`n)QV4h6YD_O+{62#6Aa+*>}{C1Nk_qV31~+7);tXX0)M%f63;z=<#)`u^aAcih%@ zZP@T0d0SquwdHy{?Y7(OZLV*wZ?w0;ZErr@z{~O~RYh59<+&I~a8Ijvj}S5f9)ZJ! zx`j(cC=dZ>i8Mdo7QXK_-ImjAJFa`=dA_^hyUm{qP2$cri8>He7AP+F`2{TU(y~ZY z%_5l>?id3&10qh+u3mZd6^-$EL1{Q7qR>VkJon~>xYM}O$`D%EfB8?bFEAHj_DV4; zy3F)3CzTyQQ~`Zpfqqc4$mu>@f*8x!GHbFL9FUPbZOpP@IbqqF#WLmc_{v)P-xkl` zuaYN^}a%FpV*_k&7azGgkO;shoPrysl$WEu1Y*t5DU zlgPG?GQL;TmXAGaKjCZNKgBGT#C-oqBU4FxSUqCMdwH2`Y+DmImH}hSqjyB emQz!4Avpfh((oKTjEBE5QwO+dqKWh^uDP#S^}T)N zwuY!yN*{{|00000000B!nB7hjK@`WS#+ZK!<0LiL3_D>R5FJ1njv??6-+&{O|5 z>bd58v9+9M! zkx3kf3<956y)nGA3L@p~uH%fKMj*0{?Ks9Zc4`N^hcD{;+i&;Y)eiRdkL!Ey-)!%b zzS8SBq7hjgU-SO1rx^*7$%8<)MHyB zO#u|ukcqKHT3ooD7PKq19O}jeqLl*DI3o*o!ITTLugra&BqIeWA@|uuiU{2pN%^5q zb{Os^4YKvNW~zerR8QInVb=F)5O`fGJHFpOk@fQ0%POo^UR5?K&9#kkWxHA}FE`g$ z)>l@mTTrg9zgVTS>M5N?m15xW%wU3 zt7J0MsD#}vyhJUun1|YZhuU1h!C^7ew@y_vCVqwwk1EUVAdvqNu;{IvTd;K zz&W}~II^o=A3~VC`wcq%mpfnfS!clcpPzw4$kRpU@mMkj#Bf8)LuAt-;`Mj3^C)cl zDC>W?g{c?DW+DVc*jHK{E}0LfjF@*3vD~w$ilu_!b zA!8p>E*eMooVV1u1CwI==_45bT_GL1!{QjSKX_am{*e>@{wREt>gh#JsXB(*C}Z8+ dNx8{cm`4vBFSo20HY`LZ{x4O4(}UF$005uT#yS81