fix(app authentication): Handle time difference between system and GitHub API time in JWT claims #164
Conversation
copperwall
force-pushed
the
store-time-difference-for-app-auth
branch
from
17b401c to
71f1341
Compare
There was a problem hiding this comment.
Ooops! I found a pending review that I forgot to submit, sorry!
Also gr2m/universal-github-app-jwt#28 is merged and released now, could you finish this up?
gr2m
added
the
Type: Bug
gr2m
changed the title
|
oh whoops sorry! I was out of town for a week or so, so I'm just seeing this review. I'll get back on this tomorrow |
|
no worries at all, thanks Chris! |
src/get-app-authentication.ts
Outdated
| const appAuthentication = await githubAppJwt({ | ||
| id, | ||
| privateKey, | ||
| now: timeDifference, |
There was a problem hiding this comment.
Are you still working on this?
I'm a bit confused that we are setting now which is a UNIX epoch as milliseconds to timeDifference which seems to be a time difference in seconds? Or am I missing something?
There was a problem hiding this comment.
Ah sorry I missed that, the githubAppJwt value for now should be the current time in seconds, not the difference in seconds from the GitHub time. I'm adding a fix for that right now.
|
I agree, I was thinking about that a little considering everything was passing with the previous commit. I think because I was using a mocked I'll have to try out another test case that breaks with 516c839 but passes with 8f66a4b, since that really should've happened. |
|
Thanks for confirming that on your end too (and for all the feedback so far, I really appreciate it), I think this is ready for a final review now |
…itHub API time in JWT claims
gr2m
force-pushed
the
store-time-difference-for-app-auth
branch
from
b61b705 to
109725e
Compare
…tHub API time in JWT claims
|
🎉 This PR is included in version 2.5.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
Thanks so much @gr2m! |
This adds error handling behavior for when app authentication fails due
to a mismatch in system times between the local machine and GitHub's API
servers. If an expiration (exp) or issued_at (iat) claim in the JWT
payload is invalid, the difference between the system time and GitHub's
server time is stored and further requests are made with JWTs that
include that time difference when creating the exp and iat claims.
Also, this is pending on changes in gr2m/universal-github-app-jwt#28. I tested it locally by
npm linking the changes from that PR into this branch so the tests should pass when those changes get merged in. If there's any changes in that PR I'll update the library usage in this one too.fixes #146
fixes probot/probot#1258
fixes https://github.com/octokit/rest.js/issues/1832