fix(github): detect rate limits using response headers#1
Conversation
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.
…riyanshu-byte-coder#1702) Co-authored-by: Shivani-ramesh09 <shivanirameshot7@gmail.com>
…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.
…#1731) Co-authored-by: Test User <test@test.com>
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.
Previously, any 403 or 429 response from GitHub was classified as a
rate-limit error, causing misclassification of permission failures,
invalid credentials, and insufficient-scope errors.
Root cause: GitHubRateLimitError was thrown on HTTP status alone
without inspecting X-RateLimit-Remaining, Retry-After, or response
body content.
Changes:
- Add extractRateLimitInfo() to parse X-RateLimit-Reset,
X-RateLimit-Remaining, and Retry-After headers
- Add isSecondaryRateLimitBody() to detect secondary rate-limit signals
in response body messages
- Add buildGitHubError() that classifies errors correctly:
429 => GitHubRateLimitError
403 + X-RateLimit-Remaining=0 => GitHubRateLimitError (primary)
403 + Retry-After present => GitHubRateLimitError (secondary)
403 + rate-limit body message => GitHubRateLimitError (secondary)
403 (all other cases) => GitHubApiError
- Enrich GitHubRateLimitError with retryAfter field (seconds)
- Add 25 new tests covering all classification paths for both
githubFetch and githubGraphQL
GSSoC Label Checklist 🏷️@Ridanshi — please apply the appropriate labels before merging: Difficulty (pick one):
Quality (optional):
Validation (required to score):
|
There was a problem hiding this comment.
Thanks for your first PR on DevTrack! 🎉
A maintainer will review it within 48 hours. While you wait:
- Make sure CI is passing (type-check + lint)
- Double-check the PR description is filled out and the issue is linked
- Feel free to ask questions in Discussions if you need help
If you find DevTrack useful, a ⭐ star on the repo is always appreciated — it helps the project grow and attract more contributors!
Recurring goals silently erased period data when the weekly/monthly
boundary was crossed. Before this fix, current was zeroed and
period_start advanced with no record of what was achieved.
Root cause: the reset branch in GET /api/goals called
supabase.update({ current: 0, period_start }) with no prior archival.
Changes:
- Upsert into goal_history (goal_id, period_start unique) before every
reset, capturing achieved_value, target, completed, and period dates.
ignoreDuplicates=true makes concurrent resets idempotent.
- Fetch the most recent history row per recurring goal and attach as
last_period in the GET response so the UI needs no extra request.
- GoalTracker shows check N/M or cross N/M last period for recurring goals.
- data-export includes full goal_history records and fixes goals column
list; goal_history is also purged on account deletion.
- Migration creates goal_history table with referential integrity,
cascade-on-delete, and indexes for efficient last-period lookup.
- 17 tests cover completed/incomplete resets, idempotency, ordering,
last_period attachment, weekly, monthly, and multi-goal scenarios.
sentCount was incremented unconditionally after each loop iteration, regardless of whether the Resend API call succeeded, failed with a non-2xx response, or threw a network exception. The fetch() return value was completely discarded, making the reported count misleading and hiding delivery failures from operators. Changes to src/app/api/cron/weekly-digest/route.ts: - Capture the Resend fetch() response in a local variable - Check res.ok before incrementing sentCount - Add failedCount to track sends that received non-2xx responses - Wrap each per-user fetch in try/catch so a network error for one recipient does not abort delivery to subsequent recipients - Log failed sends with HTTP status and github_login (not email) - Include failedCount in the JSON response alongside sentCount Changes to test/weekly-digest-cron-auth.test.ts: - Update existing success test to assert failedCount: 0 - Update RESEND_API_KEY-absent test: sentCount is now 0 (not 1) since no delivery was actually attempted - Add 8 new tests covering: successful delivery, Resend 422/429/500 error responses, network exceptions, mixed success/failure batches, per-user failure isolation, and response shape
Closes Priyanshu-byte-coder#1878 ## Vulnerability analysis NextAuth session cookies are SameSite=Lax by default, which blocks naive cross-site form submissions but does not prevent attacks from same-registered-domain subdomains or certain browser edge cases. State-changing API routes authenticated only via cookies were therefore vulnerable to CSRF. ## Root cause No Origin/Referer validation existed for browser-authenticated mutations. API routes accepted POST/PUT/PATCH/DELETE requests from any origin as long as a valid session cookie was present. ## Implementation **Layer 1 — Origin validation (src/lib/security/csrf.ts)** - `buildAllowedOrigins()` reads NEXTAUTH_URL and ALLOWED_ORIGINS env vars to build the permitted-origin set. - `getRequestOrigin()` extracts the Origin header, falling back to the Referer origin when Origin is absent. - `isOriginAllowed()` performs exact, case-sensitive comparison. - `isCsrfExemptPath()` marks paths that authenticate out-of-band. **Layer 2 — Middleware enforcement (src/middleware.ts)** - Applies to all authenticated (token present) non-safe-method requests under /api/ that are not on the exempt list. - Requests carrying Authorization: Bearer are skipped — they are API clients, not browser sessions, and are not susceptible to CSRF. - Cross-origin requests are rejected with 403. **Exempt paths (authenticate via their own mechanism)** - /api/webhooks/github — x-hub-signature-256 HMAC - /api/webhooks/dispatch — WEBHOOK_DISPATCH_SECRET bearer - /api/cron/ — CRON_SECRET bearer - /api/auth/ — NextAuth internals **Allowed-origin configuration (.env.example)** - NEXTAUTH_URL is always included automatically. - ALLOWED_ORIGINS accepts additional comma-separated origins for staging/preview deployments. ## Protected routes (state-changing, session-authenticated) POST: /api/goals, /api/rooms, /api/notifications, /api/webhooks/custom, /api/daily-focus (via PUT) PUT: /api/daily-focus PATCH: /api/goals/[id], /api/user/settings, /api/notifications/[id] DELETE: /api/goals/[id], /api/streak/freeze, /api/daily-focus, /api/notifications/[id] ## Additional fixes - goals/[id] PATCH: require current as a non-negative integer (reject missing field and floats) - local-coding/stats: return 500 on DB error instead of empty stats - user/github-accounts: return 500 on DB error instead of empty list - discord.test.ts: update test URLs to valid Discord webhook format so they pass the URL validation added to discord.ts ## Tests added test/csrf.test.ts — 42 tests covering: - SAFE_METHODS set membership - isCsrfExemptPath for all exempt and non-exempt paths - buildAllowedOrigins with NEXTAUTH_URL, ALLOWED_ORIGINS, both, neither - getRequestOrigin: Origin header, Referer fallback, both present, malformed Referer, whitespace trimming - isOriginAllowed: known/unknown/scheme/port/case/empty - Middleware integration: valid origin, invalid origin, missing headers, unauthenticated bypass, safe methods, bearer bypass, webhook exemption, cron exemption, Referer fallback test/daily-focus.test.ts — 19 tests for the new /api/daily-focus route ## Verification pnpm test — 1169/1169 passed pnpm lint — no errors (warnings pre-existing) pnpm typecheck — clean
Closes Priyanshu-byte-coder#1878 Include remaining test and component files for the security branch.
Closes Priyanshu-byte-coder#1878 Update github auth error response code for consistency with client handling.
Closes Priyanshu-byte-coder#1878 Update component error handling and auth error test assertions.
Closes Priyanshu-byte-coder#1878 Update date utility and tests.
Closes Priyanshu-byte-coder#1878 Commit the remaining SSE stream connection improvements and date-utils re-export that were staged alongside the CSRF implementation. - src/app/api/stream/route.ts: add heartbeat keepalive, idle-timeout recycling, and max-age ceiling so zombie connections do not hold slots indefinitely; extract HEARTBEAT_INTERVAL_MS, IDLE_TIMEOUT_MS, MAX_AGE_MS as named exports for test coverage - src/lib/date-utils.ts: re-export date helpers from dateUtils.ts under the hyphenated alias expected by test/date-utils.test.ts
Extends the existing test file from 10 basic cases to 47 behavioral tests, grouped into three describe blocks. Functions covered: - privateCacheHeaders(maxAgeSeconds?, swrSeconds?) - publicCacheHeaders(maxAgeSeconds?, swrSeconds?) New coverage added: SWR default formula regression - Verifies stale-while-revalidate defaults to exactly 2× maxAgeSeconds for both functions, using non-round inputs (100, 150, 3600) that would expose an accidental multiplier change Directive presence / absence guards - privateCacheHeaders: always contains `private`, never `public` or `s-maxage` - publicCacheHeaders: always contains `public` and `s-maxage`, never `private` or standalone `max-age` Exact format and ordering - Regex asserts on full directive pattern (e.g. /^private, max-age=\d+, stale-while-revalidate=\d+$/) - Verifies `=` assignment, comma-space separators, and the full `stale-while-revalidate` keyword (not an abbreviation) - Ordering test: splits on `", "` and checks position of each part Return object shape - Both functions return an object with exactly one key: `Cache-Control` - Value is a string Large / boundary values - 86400 s (24 h) and explicit swrSeconds=0 override Headers constructor compatibility - Confirms the returned HeadersInit can be passed to `new Headers()` and yields the correct `Cache-Control` value via `.get()` Cross-function regression describe block - Two functions produce distinct values for the same input - max-age vs s-maxage mutual exclusion - private vs public mutual exclusion - Both expose stale-while-revalidate - Directive count is exactly 3 for both functions
Closes Priyanshu-byte-coder#1879 Root cause: src/lib/date-utils.ts (barrel re-export) was committed without corresponding regression tests, so a future rename of dateDiffDays or dateDiff in the barrel could silently break callers without any test catching it. Fix: extend test/dateDiffDays.test.ts with a dedicated describe block that imports both dateDiffDays and dateDiff from the @/lib/date-utils barrel and asserts: - both names are exported as functions - dateDiff and dateDiffDays return identical results - correct positive, zero, and negative day differences Tests added: - "exports dateDiffDays" — verifies named export exists - "exports dateDiff alias" — verifies alias export exists - "dateDiff and dateDiffDays produce identical results" - "barrel dateDiffDays computes correct day difference" - "barrel dateDiff computes correct day difference" - "barrel export returns 0 for same date" - "barrel export returns negative for reversed dates" Verification: next lint → warnings only (pre-existing), no errors tsc --noEmit → clean vitest run → 1255 tests across 86 files, all passed
Closes Priyanshu-byte-coder#1848 Root cause: Today's Focus goal was stored only in localStorage, making it inaccessible across devices, browsers, and incognito sessions. Any browser storage clear or device switch would lose the user's focus entry. Implementation summary: Database layer - Added migration 20260602000000_add_daily_focus.sql creating the daily_focus table with columns (id, user_id, focus_date, goal_text, created_at, updated_at), a unique (user_id, focus_date) constraint, composite index, FK to users with ON DELETE CASCADE, and RLS policies scoped to auth.uid()::text so each user can only access their own rows. API layer - Implemented GET, PUT (upsert), and DELETE handlers under src/app/api/daily-focus/route.ts following the same authentication, validation, and error-handling patterns used throughout the codebase. Input is validated and sanitised via validateTextInput(); date params are validated with a strict regex before use. Frontend - Refactored src/components/TodayFocusHero.tsx to treat the server as the source of truth. On mount the component fetches today's record; saves and deletes are sent to the API with optimistic updates and rollback on failure. If the server is unreachable the component falls back to localStorage and shows a sync warning. Unauthenticated users retain the localStorage-only experience unchanged. Migration path - On first load, if no server record exists but a localStorage entry does, the component silently migrates the value via PUT and removes the localStorage key only after a confirmed 200 response. No data is lost during the transition. Export support - src/app/api/user/data-export/route.ts now includes a dailyFocus section (last 365 records) in GET exports and explicitly deletes daily_focus rows during account deletion. Additional fixes included in this branch - Extended GitHub auth-error propagation to ci, inactive-repos, pinned-repos, productive-hours, and weekly-summary metric routes, and to the ci-analytics helper, using the shared GitHubAuthError / githubAuthErrorResponse pattern already established earlier on this branch. - Updated DiscussionsWidget and PRReviewTrendChart to handle token_expired responses from the API. Tests added - test/daily-focus.test.ts: 19 tests covering GET (authenticated, unauthenticated, missing record, existing record, DB error), PUT (create, upsert, validation errors, invalid JSON, DB error), and DELETE (success, idempotent delete, defaults, DB error). Verification commands next lint -- warnings only (pre-existing), no errors tsc --noEmit -- clean vitest run -- all tests pass
…ric routes Extends the GitHubAuthError / githubAuthErrorResponse pattern to coding-activity-insights, pr-breakdown, PRReviewTrendChart, and WeeklySummaryCard — completing the token-revocation error propagation pass started in earlier commits on this branch.
The SSE stream endpoint previously polled two database tables every 2 s per open connection with no cap on concurrent connections and no cleanup of zombie connections, creating O(N*queries) database load. The fix (already on branch in c3f4c6b) addressed this with: - Per-user connection cap of 4 (HTTP 429 with Retry-After when exceeded) - Poll interval increased from 2 s to 15 s (7.5x fewer queries) - Events only emitted when data actually changes (no client spam) - Heartbeat keepalive every 30 s to detect dead TCP streams early - Idle timeout: connections with no real data for 5 min are recycled - Max-age ceiling: connections forcibly recycled after 30 min - Single closeStream() path with a cleanedUp guard prevents slot double-decrement when abort, idle-timeout, and max-age race This commit adds dedicated test coverage for the lifetime-control mechanisms that were previously untested: - HEARTBEAT_INTERVAL_MS / IDLE_TIMEOUT_MS / MAX_AGE_MS constant values - Idle timeout releases the connection slot (fake-timer test) - Max-age releases the connection slot (fake-timer test) All 20 SSE stream tests pass.
Complete the remaining auth-error propagation gaps so all metrics routes and dashboard widgets behave consistently when a GitHub OAuth token is revoked. Routes: add TokenRevoked early-exit + GitHubAuthError catch to achievements, coding-activity-insights, contributions/daily, contributions/hourly, and repo-explorer. InactiveRepositoriesCard and ProductiveHoursWidget detect token_expired and render a reconnect prompt instead of a generic error state. Tests: new github-auth-metrics.test.ts covers all five newly-fixed routes for the TokenRevoked session path, GitHub 401 propagation, and non-auth failures remaining as 502. token-expired.test.ts extended with matching coverage for discussions, pinned-repos, pr-breakdown, and weekly-summary routes.
8377f98 to
d973cda
Compare
|
This PR was targeting the fork's internal main branch instead of upstream. The work for issue Priyanshu-byte-coder#1741 has been rebased onto the latest upstream main and resubmitted as Priyanshu-byte-coder#2040. This fork PR can be closed once the upstream PR is reviewed and merged. |
|
Closing in favour of the properly targeted upstream PR: Priyanshu-byte-coder#2040 |
Closes Priyanshu-byte-coder#1741
Root cause
The project uses GitHub OAuth App (not GitHub App), so tokens have no refresh mechanism — they remain valid until a user revokes access in GitHub Settings. A 30-day JWT session meant a revoked token could silently serve empty dashboards for up to 30 days.
The auth layer already had a 24-hour periodic liveness check that marks
session.error = "TokenRevoked"when GitHub returns 401, andlib/github-fetch.tsalready exportedGitHubAuthErrorandgithubAuthErrorResponse(). However, 13 metric routes were missing one or both of:session.error === "TokenRevoked"early-exit guardGitHubAuthErroron 401 responses inside inner fetch callsGitHubAuthErrorand returninggithubAuthErrorResponse()instead of a generic 502Authentication model discovered
session.error = "TokenRevoked"surfaced via NextAuth session callback; checked at route entry to short-circuit before any GitHub API callAPI behaviour changes
All affected routes now return a structured error instead of silently degrading:
{ "error": "token_expired" }with HTTP 401.
Routes updated:
achievements,coding-activity-insights,contributions/daily,contributions/hourly,repo-explorer,weekly-summary,discussions,ci,pinned-repos,pr-breakdown,pr-review-time,inactive-repos,productive-hoursNon-auth failures (rate limit 403, 422, 5xx) still return
{ "error": "GitHub API error" }with HTTP 502 — unchanged.Dashboard changes
InactiveRepositoriesCardandProductiveHoursWidget: detecttoken_expiredin the API response and render an inline reconnect promptGitHubTokenExpiredBanner(already present): shown whensession.error === "TokenRevoked"or when the dashboard fetch interceptor firesgithub-token-expiredTests added
test/github-auth-metrics.test.ts— 15 tests for discussions, pr-review-time, productive-hours, inactive-repos, and weekly-summary covering: TokenRevoked session path, GitHub 401 propagation, and non-auth failures staying as 502test/token-expired.test.ts— extended with coverage for discussions, pinned-repos, pr-breakdown, and weekly-summary routesVerification commands