Skip to content

fix(auth): preserve follow-scoped GitHub tokens during OAuth login#373

Open
Ridanshi wants to merge 9 commits into
Dev-Card:mainfrom
Ridanshi:fix/github-token-scope-preservation
Open

fix(auth): preserve follow-scoped GitHub tokens during OAuth login#373
Ridanshi wants to merge 9 commits into
Dev-Card:mainfrom
Ridanshi:fix/github-token-scope-preservation

Conversation

@Ridanshi

Copy link
Copy Markdown
Contributor

Closes #355

Summary

Fixes a cross-flow GitHub OAuth token lifecycle issue where normal GitHub login could silently overwrite an existing follow-capable GitHub integration token with a reduced-scope login token.

Previously:

  • the GitHub connect/follow flow stored tokens with follow-capable scopes such as user:follow
  • a later GitHub login flow performed an unconditional oAuthToken.upsert
  • the lower-scope login token overwrote the existing integration token
  • GitHub follow functionality silently broke afterward

This PR preserves higher-privilege follow-capable tokens during subsequent GitHub login flows while keeping the existing login behavior intact.


Root Cause

connect.ts stores GitHub integration tokens in oAuthToken with follow-capable scopes.

Later, auth.ts performed an unconditional:

oAuthToken.upsert(...)

using the GitHub login token (read:user user:email).

Because both flows shared the same persisted GitHub credential record:

  • the existing integration token was overwritten
  • user:follow capability was lost
  • follow automation silently failed afterward

The issue was difficult to detect because:

  • login flow worked independently
  • connect flow worked independently
  • follow flow worked independently with valid scopes
  • but cross-flow token persistence invariants were not preserved

Fix Strategy

Before replacing an existing GitHub token during login, the backend now:

  1. checks existing stored GitHub scopes
  2. detects whether the existing token contains user:follow
  3. compares incoming login token scopes
  4. preserves the existing higher-privilege token when the new token has reduced scopes

Behavior after fix:

  • existing follow-capable tokens are preserved
  • normal GitHub login still succeeds
  • JWT issuance/login flow remains unchanged
  • legitimate token upgrades are still allowed
  • new users without stored tokens still persist normally

Files Changed

  • apps/backend/src/routes/auth.ts
  • apps/backend/src/__tests__/auth.github-token.test.ts

Tests

Added focused regression coverage for:

  • preserving an existing follow-capable token
  • subsequent reduced-scope GitHub login
  • normal login flow still succeeding
  • legitimate token upgrades
  • preserving follow integration state consistency

Verification

..\..\node_modules\.bin\vitest.CMD run src\__tests__\auth.github-token.test.ts src\__tests__\connect.test.ts src\__tests__\follow.test.ts

Result:

Test Files  3 passed (3)
Tests       6 passed (6)

Notes

This PR intentionally keeps scope minimal and focused on GitHub OAuth token scope preservation.

No OAuth architecture rewrite, schema redesign, provider refactor, or unrelated auth cleanup was introduced.

@Ridanshi Ridanshi force-pushed the fix/github-token-scope-preservation branch 2 times, most recently from 1b9130c to 7520db3 Compare May 28, 2026 16:17
@Harxhit Harxhit added the gssoc:approved Required label for every approved PR. Gives the base +50 points and enables contribution tracking. label May 28, 2026
@Harxhit

Harxhit commented Jun 2, 2026

Copy link
Copy Markdown
Collaborator

Could you please resolve the merge conflicts and add terminal screen shots for lint, typecheck and unit tests proof.

@Ridanshi Ridanshi force-pushed the fix/github-token-scope-preservation branch from 7520db3 to b2130d5 Compare June 2, 2026 05:50
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown

CI Results — ❌ Some checks failed

🖥️ Backend (❌ failure)

Check Status
Lint ❌ failure
Test ❌ failure
Typecheck ❌ failure

📱 Mobile (⏭️ skipped)

Check Status
Lint ⚪ unknown
Test ⚪ unknown

🌐 Web (⏭️ skipped)

Check Status
Check ⚪ unknown
Build ⚪ unknown

🕐 Last updated: Tue, 02 Jun 2026 15:05:40 GMT

@Ridanshi

Ridanshi commented Jun 2, 2026

Copy link
Copy Markdown
Contributor Author
Screenshot 2026-06-02 115400 Screenshot 2026-06-02 115450 Screenshot 2026-06-02 115601

@ShantKhatri ShantKhatri requested a review from Harxhit June 6, 2026 17:20
@Harxhit

Harxhit commented Jun 7, 2026

Copy link
Copy Markdown
Collaborator

Please fix the merge conflicts.

Ridanshi and others added 7 commits June 8, 2026 21:21
…v-Card#300)

Public events remain fully accessible without authentication.
Private events now require the caller to be the organizer or a
confirmed attendee; unauthenticated callers receive 401 and
authenticated non-members receive 403.

Changes:
- add getRequestUserId() soft-auth helper (returns null gracefully,
  never throws, used only on read endpoints)
- add canAccessEvent() returns 'allowed' | 'unauthenticated' |
  'forbidden' so callers can issue semantically correct 401 vs 403
- GET /api/events/:slug — visibility enforced after DB fetch;
  isPublic field intentionally excluded from response body
- GET /api/events/:slug/attendees — visibility enforced before
  returning any attendee data
- Routes registered with /api/events prefix in app.ts (removes the
  double-prefix issue from the prior implementation)
- 46 tests added covering public access, private 401/403, organizer
  access, attendee access, no-sensitive-field leakage, and unchanged
  pagination/sorting behaviour
Avoid replacing an existing user:follow OAuth token with a lower-scope GitHub login token.

Closes Dev-Card#355
…import

Register @fastify/cookie in test buildApp so request.cookies is populated.
Mock the encryption module directly instead of via app.decorate so the
encrypt() call in auth.ts resolves correctly. Pass oauth_state cookie in
inject headers and fix the redirect assertion to use a URL fragment (#)
consistent with how auth.ts builds the mobile redirect URI.
…main

Rebased fix/github-token-scope-preservation onto upstream/main, which
had advanced with three changes that conflicted with this branch:

  b7d307b — event GET /:slug: replace organizerId with organizer public
             fields (organizerUsername + organizerDisplayName)
  f6ee844 — card creation transaction (cards.ts, unrelated to this PR)
  Various CI/workflow commits (no code conflicts)

Conflict resolutions
────────────────────

apps/backend/src/routes/event.ts  (3 conflict regions)

  1. EventDetails type: upstream dropped organizerId and added
     organizerUsername/organizerDisplayName.  Kept upstream's field
     names; kept the PR's visibility helpers (getRequestUserId,
     canAccessEvent) and access-control logic unchanged.

  2. GET /:slug handler: added organizer relation to the Prisma include
     so the response can supply organizerUsername/organizerDisplayName
     from the organizer join.  Retained the PR's visibility enforcement
     (401 for unauthenticated private-event callers, 403 for non-members).

  3. GET /:slug/attendees handler: kept the PR's visibility enforcement
     before returning any attendee data; used _count?.attendees with a
     fallback to attendees.length so the pagination total is correct
     whether or not _count is present in the Prisma result.

apps/backend/src/routes/auth.ts  (3 conflict regions)

  1. GitHub callback — token persistence block: merged the PR's
     shouldPreserveExistingGitHubToken() guard with upstream's
     non-fatal try/catch wrapper.  Both are preserved: the guard
     prevents a login from overwriting a follow-scoped token with a
     narrower read-only token; the try/catch ensures a storage failure
     does not abort the login flow.

  2. End of file — helper functions: dropped the now-stale
     generateState/buildOAuthState/getMobileRedirectUri definitions
     (these were moved to authService.ts by upstream); kept
     shouldPreserveExistingGitHubToken and hasScope which are new to
     this PR.

  3. Google callback — CSRF check: kept the PR's cast
     (request.cookies as any)?.oauth_state with the section comments.

apps/backend/src/__tests__/event.test.ts

  Updated mocks to match the merged route shape:
  - All GET /:slug mocks now include an organizer relation and the
    assertion was updated from organizerId to organizerUsername.
  - All GET /:slug/attendees mocks now include _count so the
    pagination total field can be resolved.

All 67 tests across auth.github-token, connect, follow, and event
suites pass.
The CI backend-ci job runs pnpm eslint against each file changed in the
PR.  Six files carried lint errors that caused the job to fail:

app.ts
  - Removed unused fastifyStatic import (no @fastify/static in routes)
  - Fixed import-x/order: moved ./routes/team.js before validateEnv
  - Added braces to bare if statement (curly rule)
  - Renamed unused catch binding to _err

routes/auth.ts
  - Fixed import-x/order: runtime imports (services, utils) before type import
  - Swapped authService.js / encryption.js order (alphabetical within group)
  - Renamed unused catch binding to _e in the /me preHandler fallback

routes/event.ts
  - Fixed import-x/order: runtime import before type import
  - Added braces to all bare if/return statements in getRequestUserId and
    canAccessEvent (curly rule)
  - Changed let cleanSlug to const (prefer-const)
  - Changed Math.random().toString(36).substring() to .slice() (unicorn rule)
  - Renamed unused catch binding to _error in event.create handler

__tests__/auth.github-token.test.ts
  - Fixed import-x/order: @fastify/cookie before fastify (alphabetical)

__tests__/event.test.ts
  - Fixed import-x/order: type imports must appear last per config
    (groups: [..., 'type']); within the type group @prisma/client before
    fastify (alphabetical)
  - Changed let mockJwtVerify to const (prefer-const)

All 67 tests across auth.github-token, connect, follow, and event suites
continue to pass.
Root cause: `pnpm typecheck $backendFiles` passed explicit file paths to
`tsc --noEmit`, which causes TypeScript to ignore tsconfig.json entirely.
Without tsconfig settings (esModuleInterop, module: ESNext, etc.), tsc
fails with dozens of errors. The step had outcome='failure' despite showing
✅ in the UI (continue-on-error changes conclusion but not outcome), so
"Fail backend if checks failed" correctly triggered exit 1.

Fixes:
- ci.yml: run `prisma generate` after install; drop file args from typecheck
- package.json: add postinstall to always generate Prisma client
- app.test.ts: set JWT_SECRET + ENCRYPTION_KEY so validateEnv() passes
- validateEnv.test.ts: add null to process.exit mock param type
- cards.test.ts: cast mockPrisma as any for app.decorate
- profiles.test.ts: remove Pick<PrismaClient> annotation that hid mock types
- connect.ts / follow.ts: fix logger format, unused vars, import order

Note: pull_request_target uses base branch workflow, so this PR's CI run
uses the pre-fix workflow. These changes take effect for future PRs after
merge to main.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@Ridanshi Ridanshi force-pushed the fix/github-token-scope-preservation branch from 664ea15 to 70c58da Compare June 8, 2026 19:11
@vercel

vercel Bot commented Jun 8, 2026

Copy link
Copy Markdown

@Ridanshi is attempting to deploy a commit to the Prashantkumar Khatri's projects Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions

github-actions Bot commented Jun 8, 2026

Copy link
Copy Markdown

CI — Checks Failed

Backend — FAIL

Check Result
Lint PASS
Test FAIL
Typecheck PASS

Mobile — SKIP

Check Result
Lint -
Test -

Web — SKIP

Check Result
Check -
Build -

Last updated: Mon, 08 Jun 2026 19:23:46 GMT

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gssoc:approved Required label for every approved PR. Gives the base +50 points and enables contribution tracking.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GitHub OAuth login silently overwrites follow-scoped token and breaks GitHub follow integration

2 participants