Skip to content

fix(security): add centralized CSRF protection#3

Closed
Ridanshi wants to merge 103 commits into
mainfrom
fix/csrf-protection-1878
Closed

fix(security): add centralized CSRF protection#3
Ridanshi wants to merge 103 commits into
mainfrom
fix/csrf-protection-1878

Conversation

@Ridanshi
Copy link
Copy Markdown
Owner

@Ridanshi Ridanshi commented Jun 3, 2026

Closes Priyanshu-byte-coder#1878

Security analysis

DevTrack uses NextAuth JWT session cookies (SameSite=Lax, HttpOnly, Secure in production) for all browser-authenticated routes. Before this fix, no Origin or Referer validation existed — any cross-origin page could issue authenticated state-changing requests (POST/PUT/PATCH/DELETE) on behalf of a logged-in user by exploiting cookie auto-attachment.

SameSite=Lax provides partial CSRF mitigation (blocks cross-site form POSTs) but does not protect against:

  • Cross-site fetch/XHR from malicious pages (allowed on same registered-domain subdomains)
  • Older browsers with weaker SameSite defaults
  • Future cookie configuration changes

Authentication architecture discovered

Client type Auth mechanism CSRF risk
Browser (logged-in user) NextAuth JWT cookie Vulnerable — fixed here
GitHub webhook delivery HMAC x-hub-signature-256 Not vulnerable — exempted
Vercel Cron / sync jobs Authorization: Bearer CRON_SECRET Not vulnerable — exempted
Local-coding CLI agent Authorization: Bearer <api-key> Not vulnerable — exempted
NextAuth internals (/api/auth/*) Managed by NextAuth Not vulnerable — exempted

Root cause

No CSRF protection existed anywhere in the codebase. All browser-session mutation routes accepted requests from any origin as long as a valid session cookie was present.

Protection strategy

Centralized Origin/Referer validation in src/lib/csrf.ts, enforced at the middleware layer (src/middleware.ts) before any route handler runs.

Decision logic in checkCsrfOrigin():

Request type Decision
GET / HEAD / OPTIONS Pass (not mutations)
Authorization: Bearer … present Pass (machine client — cannot be CSRF'd)
Origin matches trusted set Pass
Origin present, not trusted 403 Forbidden
No Origin, Referer matches trusted set Pass
No Origin, Referer not trusted 403 Forbidden
No Origin, no Referer Pass (server-side / native client)

Trusted origins are built from:

  • NEXTAUTH_URL (required; canonical app origin)
  • NEXT_PUBLIC_APP_URL (optional; alternate public origin)
  • ALLOWED_ORIGINS (comma-separated list of additional trusted origins)
  • http://localhost:3000 and http://127.0.0.1:3000 in non-production environments

Protected routes

All POST, PUT, PATCH, DELETE requests under /api/ that do not carry a Bearer token are now protected, including:

  • POST/PATCH/DELETE /api/goals, /api/goals/[id]
  • PATCH /api/user/settings
  • DELETE /api/user/data-export
  • POST /api/daily-note, /api/contact
  • POST/DELETE /api/local-coding/keys
  • POST/PATCH/DELETE /api/webhooks/custom, /api/webhooks/custom/[id]
  • PATCH/DELETE /api/notifications, /api/notifications/[id]
  • POST/DELETE /api/streak/freeze
  • POST /api/rooms, /api/integrations/jira/credentials

Exempted routes

Route Reason
/api/webhooks/github HMAC x-hub-signature-256 authentication
/api/cron/* Bearer CRON_SECRET
/api/wakatime/sync Bearer CRON_SECRET
/api/sponsors/sync Bearer CRON_SECRET
/api/notifications/discord-sync Bearer CRON_SECRET
/api/local-coding/sync Bearer API-key
/api/auth/* NextAuth-managed endpoints

Any request with Authorization: Bearer … is also globally exempt.

Configuration changes

Added to .env.example:

ALLOWED_ORIGINS=http://localhost:3000,https://devtrack.app

Tests added (test/csrf.test.ts — 61 tests)

  • isMutationMethod: POST/PUT/PATCH/DELETE → true; GET/HEAD/OPTIONS → false
  • isCsrfExemptPath: all exempt prefixes pass; protected routes fail correctly
  • checkCsrfOrigin: trusted origin, untrusted origin, missing headers, Bearer exemption, Referer fallback, ALLOWED_ORIGINS, NEXT_PUBLIC_APP_URL, each mutation method, precedence rules, webhook/cron/integration bypass scenarios

Verification

pnpm lint      — warnings only (pre-existing, unrelated)
pnpm typecheck — clean
pnpm test      — 61 CSRF tests pass

YuktiNandwana and others added 30 commits June 1, 2026 00:22
Priyanshu-byte-coder#357)

* feat: add review cycle time metric with weekly trend chart and slowest repos

* fix: resolve all review regressions, fix PR count, restore combined-accounts and query optimization
* Add repository contribution distribution chart

* Add repository contribution distribution chart

* Fix repository chart theme styles and env tracking

* Address repository chart review fixes

* Replace raw error colors with theme variables

* style(chart): resolve recharts colors dynamically from theme-aware CSS variables
…nc (Priyanshu-byte-coder#1783)

Root cause
----------
The goals sync route read an optional repository filter from stored goal
rows and interpolated the raw value directly into a GitHub Search API
URL:

  const repo =
    (goal as any).repo ||
    (goal as any).repository ||
    (goal as any).repo_name ||
    null;

  const repoQualifier = repo ? `+repo:${repo}` : "";
  fetch(`…/search/commits?q=author:${login}${repoQualifier}+author-date:…`)

A stored value such as "octocat/Hello-World+author:victim" would inject
an additional author qualifier into the GitHub Search API query, silently
expanding the commit search beyond the authenticated user's own history
and producing incorrect goal progress counts.

Additionally, `(goal as any)` bypassed TypeScript type safety across all
three field name variants, and the raw query string used `+` concatenation
instead of URLSearchParams, making the parameter boundary unclear.

Fix
---

src/app/api/goals/sync/route.ts

- Introduce REPO_IDENTIFIER_RE — a strict regex accepting only
  "owner/repo" where owner is 1–39 chars (alphanumeric + hyphens,
  no leading/trailing hyphens) and repo is 1–100 chars (alphanumeric,
  dots, hyphens, underscores). The same constraint previously applied
  to the public repo-analytics endpoint.

- Export extractValidRepoFromGoal(goal: ActivityGoal): string | null
  which reads the first non-null value from goal.repo → goal.repository
  → goal.repo_name, trims it, validates it against the regex, and
  additionally rejects "." and ".." as repo names. Any value that does
  not match returns null so the repo filter is omitted from the query.

- Define the ActivityGoal typed interface to replace (goal as any),
  documenting the three optional field names and providing proper type
  coverage for downstream code.

- Move GitHub Search URL construction to URLSearchParams so the combined
  qualifier string is encoded as a single atomic value and cannot be
  split by embedded special characters. Applied to both the commit
  search loop and the PR search call.

test/goals-sync-repo-validation.test.ts — 28 new tests:
  Valid inputs: standard, dots/underscores, org hyphens, all three
    field names, whitespace trimming, max lengths
  Null/empty: all-null, empty string, whitespace-only
  Query injection (regression for Priyanshu-byte-coder#1757): embedded +author:, space-
    separated qualifier, ampersand, URL-encoded %2B
  Extra path segments: three-segment, four-segment paths
  Path traversal: owner/.., owner/.
  Invalid formats: bare name, leading/trailing slash, hyphen rules,
    owner >39 chars, repo >100 chars
  Field priority: repo > repository > repo_name; invalid higher-
    priority field does not fall through to a valid lower-priority field

Closes Priyanshu-byte-coder#1757
…shu-byte-coder#1544)

* test: fix broken unit test suite and failing assertions

* fix(auth): resolve unauthorized redirect loops and fix e2e test failures

* chore: remove debug files fix.js and fix2.js

* fix: resolve duplicated code in GoalTracker.tsx
… Improvements (Priyanshu-byte-coder#1549)

* test: fix broken unit test suite and failing assertions

* feat(a11y): implement comprehensive accessibility improvements

* chore: remove patch_project_metrics.py and relax jsx-a11y rules

* fix: resolve duplicated code in GoalTracker.tsx

* fix: resolve accessibility label error
…hu-byte-coder#1655)

* feat(Priyanshu-byte-coder#964): add theme toggle to AppNavbar for public profile and all non-dashboard pages

- Add ThemeToggle to AppNavbar desktop nav and mobile menu
- Conditionally hidden on /dashboard/* routes where DashboardHeader already provides it
- Ensures unauthenticated visitors on /u/:username can toggle dark/light mode
- Theme preference persisted to localStorage via existing ThemeContext

E2E test coverage:
- Add 'public profile page theme toggle works without authentication' test in theme.spec.js
- Scope contribution graph range button click to #contribution-activity (strict mode fix)
- Fix notifications spec: use correct NEXTAUTH_SECRET and proper mock shapes
- Fix theme spec: use .first() selector to handle multiple ThemeToggle instances

Closes Priyanshu-byte-coder#964

* fix(theme): ensure light mode is consistently applied across all pages

- Replaced hardcoded #050505 background with var(--background) in globals.css for the landing page
- Swapped hardcoded dark theme colors (borders, text, backgrounds) in LandingPage.tsx to use CSS variables from globals.css
- Updated AppNavbar scrolled state and mobile menu background to use color-mix with var(--background) instead of hardcoded rgba dark colors

This ensures that when a user switches to light mode, the landing page and navbar fully respect the theme.

* fix(theme): make footer respect light mode

- Removed forced 'dark' class from the global Footer
- Replaced hardcoded #e8e8e8 and white text with var(--foreground)
- Replaced hardcoded #9ca3af with var(--muted-foreground)

This fixes the issue where the footer was stuck in dark mode on some pages or had invisible white-on-white text on the landing page in light mode.

* refactor: remove stale LandingNav and LandingFooter

* fix(test): update landing footer e2e test selector
…byte-coder#1658)

* feat: add dynamic scroll progress ring to Back to Top button

* feat: smooth scroll animation for navbar anchor links
Priyanshu-byte-coder#1660)

* feat: implement resilient React Error Boundaries for critical dashboard widgets

* feat: wrap dashboard widgets with WidgetErrorBoundary in page.tsx
…riyanshu-byte-coder#1684)

* refactor: centralize streak calculation (Priyanshu-byte-coder#1434)

* fix: import toDateStr in weekly summary route

* fix: disable secure NextAuth cookies in Playwright server mode

* fix: restore default NextAuth cookie handling for Playwright E2E

* fix: force non-secure NextAuth cookies in Playwright server mode

* fix: allow Playwright E2E session cookie through middleware

* fix: repair JSX/TS syntax blocking CI

* fix: resolve dashboard page syntax errors

* fix: resolve merge conflicts with upstream/main in streak-unification PR

* fix(e2e): resolve strict mode violation for Goals heading locator

getByRole('heading', { name: 'Goals' }) matched two elements:
1. <h2>Goals & Insights</h2> (section heading from dashboard restructure)
2. <h2>Goals</h2> (GoalTracker heading)

Adding exact: true matches only the GoalTracker heading and avoids
the Playwright strict mode violation.
…er#1705)

* chore: clean up and refresh leaderboard branch

* fix: restore missing leaderboard data file

* fix: replace file with clean, conflict-free code

* fix: resolve manual merge conflicts and remove markers
…oder#1707)

* fix: replace tiny dropdown chevron with proper icon

* fix: resolve merge conflicts and update dropdown chevron
* fix: make wrapped loading skeleton responsive

* fix: prevent long repository names from breaking wrapped slide layout
…nshu-byte-coder#1718)

* fix(security): remove internal data from webhook POST response

Strip verbose response payload from the GitHub webhook handler that
exposed internal user IDs, account types, and system metadata to
external callers via GitHub's webhook delivery logs.

All three response paths now return a minimal { received: true }
acknowledgment. Internal processing details remain logged server-side
via the existing logError infrastructure.

Closes Priyanshu-byte-coder#1665

* test(webhook): add response shape assertions to prevent data leakage regression

Adds three test cases verifying that the webhook POST handler does not
expose internal fields (githubId, accountType, userMatched, etc.) in
any of its response paths.
jahnavi-karanwal and others added 18 commits June 2, 2026 13:36
Use --foreground instead of --destructive-foreground for logout button text to ensure readability in light mode.

Closes Priyanshu-byte-coder#1831
…r#1435) (Priyanshu-byte-coder#1685)

* fix: harden IP and bound in-memory rate limiters (Priyanshu-byte-coder#1435)

* fix: disable secure NextAuth cookies in Playwright server mode

* fix: restore default NextAuth cookie handling for Playwright E2E

* fix: force non-secure NextAuth cookies in Playwright server mode

* fix: allow Playwright E2E session cookie through middleware

* fix: repair JSX/TS syntax blocking CI

* fix: resolve dashboard page syntax errors

* fix: resolve merge conflict markers in GoalTracker.tsx

* fix: rename handleSaveBioPlain to handleSaveBio in settings

* fix(e2e): resolve strict mode violation for Goals heading locator

* fix(header): remove duplicate Dashboard h1 causing Playwright strict mode violation

* fix(e2e): update button name from 'Add goal' to 'Create goal' after upstream merge
…-byte-coder#1607)

* feat: add sticky sidebar navigation for dashboard sections

* Save work before rebase

* Minor changes
…byte-coder#362)

* perf: cache metrics api requests with 1 hour revalidation

* fix: resolve labeler permissions and e2e test strict mode

* fix: use pull_request_target for labeler to allow write permissions from forks

* fix(e2e): fix playwright timeouts and standalone static serving

* Fix syntax errors, TS errors, and API merge conflict

* Fix labeler workflow permissions and checkout

* Fix e2e test ports and labeler trigger

* Add continue-on-error to labeler

* Fix PR checks for cached metrics changes
This package was incorrectly added as a direct dependency (it's a Windows-specific
SWC native binary) causing CI failures on Linux: EBADPLATFORM error.

Also remove @emnapi/core and @emnapi/runtime which are WASM runtime internals
that should not appear as direct project dependencies.
- og/user: use next/og instead of @vercel/og (bundled in Next.js)
- dashboard: fix PinnedRepos → PinnedReposWidget
- u/[username]: use lib fetchPublicProfile, fix syncGitHubAchievementsForUser import, add userId field to ExtendedPublicProfileData
- WrappedExperience: move slides useMemo before keydown useEffect
- webhooks.test.ts: add 'as any' casts to Supabase mock objects
- goals/[id]/route.ts: deduplicate two merged PATCH handler versions, fix missing closing brace
- metrics/prs/route.ts: merge duplicate catch blocks into one
- DashboardHeader.tsx: remove duplicate useState/useEffect, fix broken JSX structure
- rooms/MembersPanel.tsx: add eslint-disable-next-line for img element
- rooms/MessageFeed.tsx: move eslint-disable comment to correct position before ternary
File had no 'packages' field causing ERR_PNPM_INVALID_WORKSPACE_CONFIGURATION.
This is not a monorepo; workspace config is unnecessary.
…rters

These were direct dependencies that got removed from package.json but remained
in the lockfile specifiers, causing ERR_PNPM_OUTDATED_LOCKFILE on Vercel.
…s flex row

A merged PR placed CodingActivityInsightsCard, PRReviewTrendChart, IssueMetrics,
CIAnalytics, DiscussionsWidget, PinnedReposWidget, InactiveRepositoriesCard,
TopRepos, LanguageBreakdown, GoalTracker, and RecentActivity inside the flex-row
quick-actions container, squeezing all dashboard content into one horizontal row.
All these widgets already exist in the proper sections below.
Closes Priyanshu-byte-coder#1878

Security analysis
─────────────────
Authentication architecture:
  - NextAuth JWT session cookie (SameSite=Lax, HttpOnly, Secure in prod)
    used by all browser-authenticated routes
  - Bearer CRON_SECRET for /api/cron/*, /api/wakatime/sync,
    /api/sponsors/sync, /api/notifications/discord-sync
  - Bearer API-key for /api/local-coding/sync
  - HMAC x-hub-signature-256 for /api/webhooks/github

Root cause:
  No CSRF protection existed anywhere in the codebase. All browser-session
  mutation routes (POST/PUT/PATCH/DELETE) accepted requests from any origin.
  SameSite=Lax provides partial mitigation but does not cover older browsers,
  top-level form submissions, or future cookie configuration changes.

Protection strategy
───────────────────
Centralized Origin / Referer validation in src/lib/csrf.ts, enforced for
every mutation API request at the middleware layer (src/middleware.ts).

Decision table for checkCsrfOrigin():
  - GET/HEAD/OPTIONS               → pass (not mutations)
  - Authorization: Bearer present  → pass (machine client, no cookie)
  - Origin matches trusted set     → pass
  - Origin present, untrusted      → 403 Forbidden
  - No Origin, Referer trusted     → pass
  - No Origin, Referer untrusted   → 403 Forbidden
  - No Origin, no Referer          → pass (server-side / native client)

Trusted origins: NEXTAUTH_URL, NEXT_PUBLIC_APP_URL, ALLOWED_ORIGINS env var.
In non-production: localhost:3000 and 127.0.0.1:3000 are always trusted.

Protected routes (examples)
───────────────────────────
POST/PATCH/DELETE /api/goals, /api/goals/[id]
PATCH             /api/user/settings
DELETE            /api/user/data-export  (account deletion)
POST              /api/daily-note
POST/DELETE       /api/local-coding/keys
POST/PATCH/DELETE /api/webhooks/custom, /api/webhooks/custom/[id]
PATCH/DELETE      /api/notifications, /api/notifications/[id]
POST/DELETE       /api/streak/freeze
POST              /api/rooms, /api/integrations/jira/credentials
POST              /api/goals/sync, /api/contact

Exempted routes
───────────────
/api/webhooks/github         — HMAC signature authentication
/api/cron/*                  — Bearer CRON_SECRET
/api/wakatime/sync           — Bearer CRON_SECRET
/api/sponsors/sync           — Bearer CRON_SECRET
/api/notifications/discord-sync — Bearer CRON_SECRET
/api/local-coding/sync       — Bearer API-key
/api/auth/*                  — NextAuth internal endpoints

Any request with Authorization: Bearer is also exempt (machine clients
cannot be tricked into attaching browser session cookies).

Files changed
─────────────
src/lib/csrf.ts         — new: checkCsrfOrigin(), isCsrfExemptPath(),
                          isMutationMethod(); reads NEXTAUTH_URL,
                          NEXT_PUBLIC_APP_URL, ALLOWED_ORIGINS
src/middleware.ts       — CSRF check runs first on all /api/* mutations;
                          matcher extended to /api/:path*; rate limiting
                          scoped to /api/metrics/* and /api/contact
src/lib/github-fetch.ts — add GitHubAuthError and githubAuthErrorResponse
                          for pre-existing achievement-progress route
.env.example            — document ALLOWED_ORIGINS

Tests added (test/csrf.test.ts — 61 tests)
───────────────────────────────────────────
isMutationMethod: POST/PUT/PATCH/DELETE true; GET/HEAD/OPTIONS false
isCsrfExemptPath: all exempt prefixes pass; all protected routes fail
checkCsrfOrigin:
  - GET requests always pass
  - Bearer token exemption for all mutation methods
  - non-Bearer auth not exempt
  - trusted Origin passes for all mutation methods
  - untrusted Origin rejected for all mutation methods
  - Referer fallback: trusted passes, untrusted rejected, malformed rejected
  - Origin takes precedence over Referer (both directions)
  - no headers allowed (server-side / native client)
  - ALLOWED_ORIGINS: single, multiple, malformed entries, unlisted rejected
  - NEXT_PUBLIC_APP_URL trusted, unrelated origin rejected
  - webhook/cron/bearer integration scenarios all pass

Verification
────────────
next lint    → warnings only (pre-existing), no errors
tsc --noEmit → clean
vitest run   → 61 CSRF tests pass; total suite: 1229 passed
               (remaining failures are pre-existing branch issues
                unrelated to CSRF, confirmed via git stash)
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Jun 3, 2026

GSSoC Label Checklist 🏷️

@Ridanshi — please apply the appropriate labels before merging:

Difficulty (pick one):

  • level:beginner — 20 pts
  • level:intermediate — 35 pts
  • level:advanced — 55 pts
  • level:critical — 80 pts

Quality (optional):

  • quality:clean — ×1.2 multiplier
  • quality:exceptional — ×1.5 multiplier

Validation (required to score):

  • gssoc:approved — counts for points
  • gssoc:invalid / gssoc:spam / gssoc:ai-slop — does not score

Type labels (type:*) are auto-detected from files and title. Review and adjust if needed.
Points formula: (difficulty × quality_multiplier) + type_bonus

@Ridanshi Ridanshi force-pushed the fix/csrf-protection-1878 branch from 6e2741e to 4e8e8c0 Compare June 3, 2026 16:53
@Ridanshi
Copy link
Copy Markdown
Owner Author

Ridanshi commented Jun 4, 2026

Closing this PR — CSRF protection for state-changing API routes was already contributed to upstream by another contributor (PR Priyanshu-byte-coder#1983, merged as commit b469777 on 2026-06-04), and issue Priyanshu-byte-coder#1878 is closed. The code on this branch is superseded by that upstream merge.

@Ridanshi Ridanshi closed this Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Missing CSRF protection on state-changing API routes — cross-site requests can modify user data