feat(auth): OAuth account linking for Twitch and YouTube integrations (#69) #70
No reviewers
Labels
No labels
auth
autorelease: pending
autorelease: tagged
bug
dependencies
devops
docker
documentation
duplicate
enhancement
github_actions
good first issue
help wanted
invalid
javascript
maintenance
question
refactor
ui
wontfix
No milestone
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
gitchegumi/GitchAlerts!70
Loading…
Reference in a new issue
No description provided.
Delete branch "issue/69-oauth-account-linking"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Implements OAuth account linking for Twitch and YouTube platform integrations.
Changes
/api/channels/[channelSlug]/integrations/[provider]for connect/disconnect/statusSecurity
Testing
Checklist
Related
Closes #69
Semgrep OSS / Semgrep Finding: javascript.node-crypto.security.gcm-no-tag-length.gcm-no-tag-length
The call to 'createDecipheriv' with the Galois Counter Mode (GCM) mode of operation is missing an expected authentication tag length. If the expected authentication tag length is not specified or otherwise checked, the application might be tricked into verifying a shorter-than-expected authentication tag. This can be abused by an attacker to spoof ciphertexts or recover the implicit authentication key of GCM, allowing arbitrary forgeries.
Show more details
Thanks for putting this together. I’m requesting changes before approval because the current OAuth flow has two blockers that will make account linking unreliable and potentially bind accounts to the wrong app user.
apps/web/src/lib/auth.tsoverrides the default Twitch and Google identity scopes. Twitch is configured withscope: '', and Google is configured with only the YouTube readonly scope. NextAuth’s provider defaults include the identity scopes needed forprofile.email/profile.name(openid email profilefor Google andopenid user:read:emailfor Twitch). Since the sign-in callback rejects whenprofile.emailis missing, these connect flows can fail before a linked account is saved. Google should preserveopenid email profilewhile adding the YouTube scope, and Twitch should preserve the default identity/email scope.The linking callback looks up
prisma.user.findUnique({ where: { email } })using the OAuth provider profile email, rather than linking to the currently authenticated app user who clicked Connect. For integrations, the Twitch/YouTube account email may legitimately differ from the app login email, so normal linking can fail. Worse, if that OAuth email belongs to another app user, the callback links the integration to that other user and the JWT callback can hydrate the session as that user after the OAuth round trip. The flow needs an authenticated-user binding, for example via a signed state/cookie or a dedicated linking endpoint that records the current session user before starting OAuth.The crypto helper appears to have been updated to specify
authTagLength, so I’m not treating the existing code-scanning thread as a blocker here. Once the scopes and current-user binding are fixed, this should be much closer to approvable.Thanks for the update. The provider scope issue from my previous review is fixed, and the dedicated linking endpoint is the right direction, but I still can’t approve this yet because the new link-state flow stores the wrong identifier.
apps/web/src/app/api/auth/link/route.tscreates the linking cookie fromtoken.sub. In this app, the durable database user id is carried in the custom JWT fieldtoken.userIdand then copied intosession.user.idinapps/web/src/lib/auth.ts. For OIDC users,token.subcan be the provider subject rather than the PrismaUser.id, so the OAuth callback later doesprisma.user.findUnique({ where: { id: linkingUserId } })with a non-database id and linking fails for those users.There is a second related problem after a successful link: returning
truefrom the Twitch/GooglesignIncallback lets NextAuth complete the OAuth sign-in as the integration provider. The existingjwtcallback then tries to hydrate an app user from the integration provider email orauthProvider_authSubject; for a legitimately different Twitch/YouTube email, that can leave the JWT withoutuserId/role, and the session callback throwsMissing authenticated user. The flow should preserve or restore the authenticated app user after linking, not replace the session with the linked provider identity.Suggested fix: have
/api/auth/linkrequire and storetoken.userIdinstead oftoken.sub, and make the Twitch/Google callback path explicitly hydrate the JWT/session as that same app user after the account is linked. A regression test for an app user linking a Google/Twitch account with a different provider email would catch both cases.Pull Request Review: Recommend Changes ⚠️
I have reviewed the changes in this PR. While the implementation of OAuth linking for Twitch and YouTube is clean and well-structured, there are a few architectural, security, and integration issues that must be addressed before this can be merged.
1. 🔒 Critical: Session Hijacking / Cookie Handling Bug in NextAuth Callbacks
File:
apps/web/src/lib/auth.tssignIncallback, the linking cookiemsa_linking_stateis consumed and deleted by callingconsumeLinkingUserId(). Immediately after, thejwtcallback runs during the same request cycle and attempts to read the linking state viapeekLinkingUserId(). Because the cookie was already deleted insignIn,peekLinkingUserId()returnsnull.token.userIdwill not be set, causing a"Missing authenticated user"failure in thesessioncallback.signIncallback. Peek at it insignInto verify the linking status, and then consume (read & delete) it inside thejwtcallback when the session token is actually being hydrated.2. 🏗️ Monorepo Architecture: Move
crypto.tsto Shared PackageFile:
apps/web/src/lib/crypto.tsapps/webpackage. However, background services likeapps/workerwill need to decrypt these stored Twitch and YouTube tokens to check live stream statuses and handle webhooks in the background. Placing this insideapps/webisolates the crypto utility.crypto.tstopackages/shared/src/crypto.tsand export it frompackages/shared/src/index.tsso that both the web application and background worker can share the exact same encryption/decryption routines.3. ⚙️ Configuration: Document
ENCRYPTION_KEYFile:
packages/database/prisma/schema.prisma/.env.exampleENCRYPTION_KEYto be defined (minimum 16 characters) or they will throw an error. This new environment variable is missing from.env.example.ENCRYPTION_KEYto.env.examplewith a brief comment explaining that it is used to encrypt platform OAuth tokens.4. 🔀 Redirection Flow: Seamless Client-Side Triggering
File:
apps/web/src/app/api/auth/link/route.ts&page.tsx/api/auth/linkGETs/api/auth/signin/${provider}. In NextAuth v4, GET requests to/api/auth/signin/:providerrender the sign-in page with a "Sign in with..." button instead of immediately redirecting to the provider, adding an extra click for the user./api/auth/linksimply set the cookie and return a status payload, then call NextAuth'ssignIn(provider)function directly in the client component. This sends a POST request and seamlessly redirects the user to the provider without intermediate screens.Dock, here are the changes you requested. All four items from the latest review have been addressed in commit
a958e33.What changed
1. 🔒 Session hijacking / cookie handling bug (
apps/web/src/lib/auth.ts)Fix: The
signIncallback now peeks at the linking cookie (peekLinkingUserId()) instead of consuming it, so the cookie survives into thejwtcallback. Thejwtcallback now consumes the cookie (consumeLinkingUserId()— read + delete) when hydrating the session token, restoring the original app user after the OAuth round trip. The redundantconsumeLinkingUserId()call in thesessioncallback has been removed — the cookie is already consumed by the timesessionruns.This prevents:
2. 🏗️ Monorepo architecture — moved
crypto.tsto shared packageFix:
apps/web/src/lib/crypto.ts→packages/shared/src/crypto.ts, exported frompackages/shared/src/index.ts. Both the web app and background workers can now importencrypt/decryptfrom@multi-stream-alerts/shared. The import inauth.tsupdated accordingly. Added 6 unit tests for the crypto module (round-trip, random IV, tamper detection, too-short ciphertext, missing key, short key).3. ⚙️ Configuration — documented
ENCRYPTION_KEYin.env.exampleFix: Added
ENCRYPTION_KEY=<strong-random-key-min-16-chars>with a comment explaining it's used to encrypt OAuth tokens for linked Twitch/YouTube accounts at rest, separate fromINSTANCE_ENCRYPTION_KEY, and must be consistent across web app and workers.4. 🔀 Redirection flow — seamless client-side triggering
Fix:
/api/auth/linknow sets the signed cookie and returns a JSON payload ({ provider, ok: true }) instead of redirecting to the NextAuth signin page. The integrations page (page.tsx) fetches/api/auth/link?provider=Xto set the cookie, then callssignIn(provider, { callbackUrl })fromnext-auth/reactclient-side, which POSTs to the NextAuth signin endpoint and seamlessly redirects to the OAuth provider — no intermediate sign-in page, no extra click.Verification
pnpm --filter @multi-stream-alerts/shared typecheck— cleanpnpm --filter @multi-stream-alerts/web typecheck— cleanpnpm --filter @multi-stream-alerts/web test— 122/122 passingpnpm --filter @multi-stream-alerts/shared test— 11 pass (6 new crypto tests + 5 existing), 2 pre-existing version test failures (unrelated — version bumped to 0.1.10 but tests expect 0.1.0)Ready for another review pass.
Re-Review: Approved! 🚀
Thank you for the quick and thorough updates! You have addressed all 4 review points perfectly:
signInand consuming injwtcallbacks, completely resolving the session takeover/hijack vulnerability and cross-email login bugs.packages/shared, making them accessible to the worker services.signIn(provider)directly via POST, creating a seamless connection user experience.The new unit tests for
crypto.tsare a great addition.I have approved the PR! There are just two very minor TypeScript compiler issues in the new test file that should be addressed before merging to keep the typecheck build passing:
🛠️ Minor Fixes Recommended
1. Import path extension
packages/shared/src/__tests__/crypto.test.ts(Line 4)import { encrypt, decrypt } from '../crypto.ts';specifies the.tsextension explicitly, which raises compilation errors under standard TypeScript configuration.2. Strict null check on Buffer mutation
packages/shared/src/__tests__/crypto.test.ts(Line 30)buf[buf.length - 1]triggersObject is possibly 'undefined'in strict null check environments.