-
Notifications
You must be signed in to change notification settings - Fork 221
♻️ Extract GitHub API utilities and integrate token refresh #3773
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 27 commits
e3dcb15
f433134
162a684
437fb3c
8f4ded6
b3aedd9
183f7d5
9b7b704
b9d4801
896dbbf
13f2557
f9d22ce
0cf8ebe
5b08f37
cc1a9a2
e4bc3e1
498d126
370ba03
07e4cd0
7fb689b
0aeeaa0
61e3146
aece9bd
164db73
d80f93e
e804aab
69c832f
85635de
dce67e7
eb68438
8541e7f
4af40bd
d3e80e7
972b2c9
8793232
663d433
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,143 @@ | ||
| import { | ||
| fromPromise, | ||
| fromValibotSafeParse, | ||
| type Result, | ||
| } from '@liam-hq/neverthrow' | ||
| import * as Sentry from '@sentry/nextjs' | ||
| import type { ResultAsync as NeverthrowResultAsync } from 'neverthrow' | ||
| import { err, ok, ResultAsync } from 'neverthrow' | ||
| import { NextResponse } from 'next/server' | ||
| import * as v from 'valibot' | ||
| import type { TokenPayload } from '../../../../../libs/github/cookie' | ||
| import { | ||
| readRefreshToken, | ||
| writeAccessToken, | ||
| writeRefreshToken, | ||
| } from '../../../../../libs/github/cookie' | ||
|
|
||
| const getClientCredentials = (): Result< | ||
| { clientId: string; clientSecret: string }, | ||
| Error | ||
| > => { | ||
| const clientId = process.env.GITHUB_CLIENT_ID | ||
| const clientSecret = process.env.GITHUB_CLIENT_SECRET | ||
| if (!clientId || !clientSecret) { | ||
| return err(new Error('Missing GitHub OAuth client credentials')) | ||
| } | ||
| return ok({ clientId, clientSecret }) | ||
| } | ||
|
|
||
| type RefreshAccessTokenResult = { | ||
| accessToken: string | ||
| refreshToken: string | ||
| refreshTokenExpiresIn: number | ||
| newExpiresAt: string | ||
| } | ||
|
|
||
| // GitHub OAuth token refresh response schema | ||
| const GitHubTokenRefreshResponseSchema = v.object({ | ||
| access_token: v.string(), | ||
| token_type: v.string(), | ||
| scope: v.optional(v.string()), | ||
| expires_in: v.number(), | ||
| refresh_token: v.string(), | ||
| refresh_token_expires_in: v.number(), | ||
| }) | ||
|
|
||
| function refreshAccessToken( | ||
| refreshToken: string, | ||
| clientId: string, | ||
| clientSecret: string, | ||
| ): ResultAsync<RefreshAccessTokenResult, Error> { | ||
| const body = new URLSearchParams({ | ||
| client_id: clientId, | ||
| client_secret: clientSecret, | ||
| grant_type: 'refresh_token', | ||
| refresh_token: refreshToken, | ||
| }).toString() | ||
|
|
||
| return ResultAsync.fromPromise( | ||
| fetch('https://github.com/login/oauth/access_token', { | ||
| method: 'POST', | ||
| headers: { | ||
| Accept: 'application/json', | ||
| 'Content-Type': 'application/x-www-form-urlencoded', | ||
| }, | ||
| body, | ||
| }), | ||
| (e) => (e instanceof Error ? e : new Error(String(e))), | ||
| ) | ||
| .andThen((res) => { | ||
| if (!res.ok) { | ||
| return err(new Error(`GitHub token refresh failed: ${res.status}`)) | ||
| } | ||
| return ResultAsync.fromPromise<unknown, Error>(res.json(), (e) => | ||
| e instanceof Error ? e : new Error(String(e)), | ||
| ) | ||
| }) | ||
| .andThen((data) => | ||
| fromValibotSafeParse(GitHubTokenRefreshResponseSchema, data), | ||
| ) | ||
| .map((data) => { | ||
| const now = Date.now() | ||
| const newExpiresAt = new Date( | ||
| now + Math.max(0, data.expires_in) * 1000, | ||
| ).toISOString() | ||
| const refreshTokenExpiresIn = data.refresh_token_expires_in | ||
|
|
||
| return { | ||
| accessToken: data.access_token, | ||
| refreshToken: data.refresh_token, | ||
| refreshTokenExpiresIn, | ||
| newExpiresAt, | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| async function handler(): Promise<NextResponse> { | ||
| // 1) Load client credentials | ||
| const creds = getClientCredentials() | ||
| if (creds.isErr()) { | ||
| Sentry.captureException(creds.error, { tags: { area: 'refresh_api' } }) | ||
| return NextResponse.json( | ||
| { ok: false, error: 'server_misconfigured' }, | ||
| { status: 500 }, | ||
| ) | ||
| } | ||
| const { clientId, clientSecret } = creds.value | ||
|
|
||
| // 2) Read refresh token and refresh via GitHub | ||
| const refreshTokenResult: NeverthrowResultAsync<TokenPayload, Error> = | ||
| readRefreshToken() | ||
| const result = await refreshTokenResult | ||
| .andThen(({ token }: TokenPayload) => | ||
| refreshAccessToken(token, clientId, clientSecret), | ||
| ) | ||
| .andThen((refreshed: RefreshAccessTokenResult) => { | ||
| const rtei = refreshed.refreshTokenExpiresIn | ||
| const refreshTtlMs = rtei * 1000 | ||
| const refreshExpiresAt = new Date(Date.now() + refreshTtlMs).toISOString() | ||
|
|
||
| // Ensure cookie writes complete and propagate failures | ||
| return fromPromise( | ||
| Promise.all([ | ||
| writeAccessToken(refreshed.accessToken, refreshed.newExpiresAt), | ||
| writeRefreshToken(refreshed.refreshToken, refreshExpiresAt), | ||
| ]), | ||
| ).map(() => NextResponse.json({ ok: true })) | ||
| }) | ||
|
|
||
| if (result.isErr()) { | ||
| Sentry.captureException(result.error, { tags: { area: 'refresh_api' } }) | ||
| return NextResponse.json( | ||
| { ok: false, error: 'refresh_failed' }, | ||
| { status: 401 }, | ||
| ) | ||
| } | ||
|
|
||
| return result.value | ||
| } | ||
|
|
||
| export async function POST() { | ||
| return handler() | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| import { fromAsyncThrowable } from '@liam-hq/neverthrow' | ||
| import * as Sentry from '@sentry/nextjs' | ||
| import { NextResponse } from 'next/server' | ||
| import { clearOrganizationIdCookie } from '../../../features/organizations/services/clearOrganizationIdCookie' | ||
| import { createClient } from '../../../libs/db/server' | ||
| import { clearTokens } from '../../../libs/github/cookie' | ||
|
|
||
| export async function POST() { | ||
| const opClearOrg = await fromAsyncThrowable(clearOrganizationIdCookie)() | ||
| const opClearTokens = await fromAsyncThrowable(clearTokens)() | ||
| const opSupabaseSignout = await fromAsyncThrowable(async () => { | ||
| const supabase = await createClient() | ||
| await supabase.auth.signOut({ scope: 'global' }) | ||
| })() | ||
|
|
||
| let hasError = false | ||
| if (opClearOrg.isErr()) { | ||
| hasError = true | ||
| Sentry.captureException(opClearOrg.error, { | ||
| tags: { area: 'logout_api', step: 'clear_org_cookie' }, | ||
| }) | ||
| } | ||
| if (opClearTokens.isErr()) { | ||
| hasError = true | ||
| Sentry.captureException(opClearTokens.error, { | ||
| tags: { area: 'logout_api', step: 'clear_tokens' }, | ||
| }) | ||
| } | ||
| if (opSupabaseSignout.isErr()) { | ||
| hasError = true | ||
| Sentry.captureException(opSupabaseSignout.error, { | ||
| tags: { area: 'logout_api', step: 'supabase_signout' }, | ||
| }) | ||
| } | ||
|
|
||
| if (hasError) { | ||
| return NextResponse.json( | ||
| { ok: false, error: 'logout_failed' }, | ||
| { status: 500 }, | ||
| ) | ||
| } | ||
|
|
||
| return NextResponse.json({ ok: true }) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,13 @@ | ||
| import { fromAsyncThrowable } from '@liam-hq/neverthrow' | ||
| import * as Sentry from '@sentry/nextjs' | ||
| import { NextResponse } from 'next/server' | ||
| import { ensureUserHasOrganization } from '../../../../components/LoginPage/services/ensureUserHasOrganization' | ||
| import { sanitizeReturnPath } from '../../../../components/LoginPage/services/validateReturnPath' | ||
| import { createClient } from '../../../../libs/db/server' | ||
| import { | ||
| writeAccessToken, | ||
| writeRefreshToken, | ||
| } from '../../../../libs/github/cookie' | ||
|
|
||
| export async function GET(request: Request) { | ||
| const { searchParams, origin } = new URL(request.url) | ||
|
|
@@ -11,9 +17,44 @@ export async function GET(request: Request) { | |
|
|
||
| if (code) { | ||
| const supabase = await createClient() | ||
| const { error } = await supabase.auth.exchangeCodeForSession(code) | ||
| const { data, error } = await supabase.auth.exchangeCodeForSession(code) | ||
| if (!error) { | ||
| await ensureUserHasOrganization() | ||
| // Persist provider tokens into encrypted HttpOnly cookies (best-effort) | ||
| const { session } = data | ||
| const access = session?.provider_token ?? '' | ||
| const refresh = session?.provider_refresh_token ?? '' | ||
| // received provider tokens for cookie write | ||
| // Access token expiration: 8 hours. | ||
| // GitHub's OAuth access tokens typically expire after 8 hours. See: https://docs.github.com/en/apps/oauth-apps/building-oauth-apps/authorizing-oauth-apps#step-3-github-redirects-back-to-your-site | ||
| const EIGHT_HOURS_MS = 8 * 60 * 60 * 1000 | ||
| const THIRTY_DAYS_MS = 30 * 24 * 60 * 60 * 1000 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GitHub refresh tokens are valid for 6 months (180 days), not 30 days. This hardcoded value causes refresh tokens to be considered expired after 30 days, even though they're still valid. Why?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I missed this point earlier.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think there's a problem with that itself, but using magic numbers is not good. |
||
|
|
||
| await fromAsyncThrowable(async () => { | ||
| if (access) { | ||
| const accessExpiresAt = new Date( | ||
| Date.now() + EIGHT_HOURS_MS, | ||
| ).toISOString() | ||
| await writeAccessToken(access, accessExpiresAt) | ||
| } | ||
| if (refresh) { | ||
| const refreshExpiresAt = new Date( | ||
| Date.now() + THIRTY_DAYS_MS, | ||
| ).toISOString() | ||
| await writeRefreshToken(refresh, refreshExpiresAt) | ||
| } | ||
| })().match( | ||
| () => undefined, | ||
| (e) => { | ||
| const err = e instanceof Error ? e : new Error(String(e)) | ||
| Sentry.captureException(err, { | ||
| tags: { | ||
| area: 'auth_cookie_write', | ||
| }, | ||
| }) | ||
| return undefined | ||
| }, | ||
|
Comment on lines
+50
to
+56
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Access tokens and refresh tokens must be obtained during login. Since this is a critical path, we want to monitor it with Sentry without stopping the app due to errors. |
||
| ) | ||
|
|
||
| const forwardedHost = request.headers.get('x-forwarded-host') | ||
| const isLocalEnv = process.env.NODE_ENV === 'development' | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These hardcoded expiration values should be documented explaining why 8 hours for access tokens and 30 days for refresh tokens were chosen, or ideally sourced from GitHub API documentation if these align with GitHub's actual token lifetimes.