Conversation
… into Development
…ing; update tsconfig to exclude express-rate-limit module
…Page components; manage UI visibility and scrolling during intro
- Updated BlogPage component to utilize dynamic CSS for styling based on blog post data. - Improved BlogListPage component to display featured and additional blog posts with enhanced layout. - Introduced a new data structure for blog posts to support rich content blocks (paragraphs, lists, callouts). - Added server-side refresh token management in authService and localStorage for improved security. - Implemented a scheduled GitHub Action for cleaning up old refresh tokens in the database. - Created migration scripts for managing refresh tokens in Supabase, including RLS policies for security. - Added CSS styles for blog components to enhance visual presentation and responsiveness.
…store for local testing
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…in permissions Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements a secure, server-side refresh token management system for authentication, moving sensitive refresh tokens from client-side storage to encrypted server-side storage. It includes database migrations, API endpoints with admin controls, scheduled cleanup automation, and updates to frontend authentication flows. Additionally, it refactors the blog system to support multiple posts with dynamic routing.
Key changes:
- Introduces AES-256-GCM encrypted storage for refresh tokens with HttpOnly session cookies
- Adds database table, RLS policies, and helper functions for token lifecycle management
- Implements API endpoints for storing, refreshing, revoking, and cleaning up tokens with JWT-based admin authentication
- Migrates frontend from localStorage to sessionStorage for tokens and integrates server-side refresh flow
- Adds blog listing page and refactors blog display to support multiple posts with dynamic slugs
Reviewed changes
Copilot reviewed 18 out of 19 changed files in this pull request and generated 30 comments.
Show a summary per file
| File | Description |
|---|---|
| api/auth/index.ts | Adds encryption helpers and new endpoints for server-side refresh token management with admin controls |
| src/server/authServer.js | Implements dev-only wrapper for /api/auth endpoints using in-memory storage and adds undefined variable references |
| sql/migrations/002_create_refresh_tokens.sql | Creates refresh tokens table with indexes and cleanup/revocation helper functions |
| sql/migrations/003_refresh_tokens_rls.sql | Enables RLS and creates service-role-only policy for refresh tokens table |
| scripts/cleanup_refresh_tokens.js | Provides cleanup script for automated token maintenance via RPC or fallback query |
| .github/workflows/refresh-token-cleanup.yml | Schedules daily automated cleanup of old refresh tokens |
| src/services/localStorage.ts | Migrates token storage from localStorage to sessionStorage to reduce XSS risk |
| src/services/authService.ts | Updates sign-in/sign-up/sign-out to use server-side token storage endpoints |
| src/App.tsx | Implements server-side refresh on startup and removes client-side session management |
| src/components/WelcomePage.tsx | Adds intro overlay scroll locking and cross-component event coordination |
| src/components/IntroBubbles.tsx | Refactors to use dynamic CSS rules instead of inline styles |
| src/components/Header.tsx | Adds intro-active state to hide header during intro overlay |
| src/components/Footer.tsx | Adds intro-active state to hide footer during intro overlay |
| src/data/blogPosts.ts | Defines blog post data structure and exports multiple blog posts |
| src/components/BlogPage.tsx | Refactors to display dynamic blog posts by slug with suggested posts |
| src/components/BlogListPage.tsx | Creates new blog listing page with featured post and grid layout |
| src/components/BlogPage.css | Updates styles to support dynamic blog post display |
| src/components/BlogListPage.css | Adds styles for blog listing page layout |
| tsconfig.json | Excludes express-rate-limit tsconfig to resolve build conflicts |
…te Google Sheets integration
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 33 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (2)
sql/migrations/003_refresh_tokens_rls.sql:14
- The RLS policy
refresh_tokens_service_onlyusesUSING (true)andWITH CHECK (true), which allows all operations from any role without checking JWT claims. This means the policy doesn't actually restrict access - any authenticated user could potentially access or modify refresh tokens if they bypass the API and call Supabase directly.
The policy should check for current_setting('request.jwt.claims', true)::json->>'role' IN ('service', 'admin') or use Supabase's service role authentication properly to ensure only server-side code with the service role key can access these rows.
CREATE POLICY refresh_tokens_service_only
ON fitbuddyai_refresh_tokens
FOR ALL
USING (true)
WITH CHECK (true);
src/server/authServer.js:703
- Empty block statement.
const { password, ...safe } = data || {};
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
- Updated README.md to clarify encryption key requirements for refresh tokens. - Improved key rotation support in authService and authServer for refresh tokens. - Enhanced cleanup logic in cleanup_refresh_tokens script to handle expired and revoked tokens more effectively. - Added auto-generated CSS variables for blog posts to improve styling consistency. - Refactored BlogListPage and BlogPage components to utilize generated CSS variables and improve safety checks for color values. - Updated error handling in various services to log warnings without disrupting user experience.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 32 out of 35 changed files in this pull request and generated 20 comments.
Comments suppressed due to low confidence (1)
api/auth/index.ts:178
- Unexpected any. Specify a different type.
export async function requireAdmin(req: any): Promise<AdminJwtPayload | null> {
| app.post('/api/webhook/questionnaire', async (req, res) => { | ||
| try { | ||
| // Prefer server-side config for the target webhook to avoid open proxy. | ||
| const configured = process.env.SHEET_WEBHOOK_URL; | ||
| const body = req.body || {}; | ||
| const payload = body?.payload || body; | ||
|
|
||
| const target = configured; | ||
| // Do NOT accept client-provided targets. Require server configuration. | ||
| if (!target) { | ||
| console.warn('[authServer] /api/webhook/questionnaire: no target configured on server'); | ||
| return res.status(400).json({ message: 'No webhook target configured on server.' }); | ||
| } | ||
|
|
||
| // Forward the request to the target webhook | ||
| try { | ||
| const response = await fetch(target, { | ||
| method: 'POST', | ||
| headers: { 'Content-Type': 'application/json' }, | ||
| body: JSON.stringify(payload), | ||
| }); |
There was a problem hiding this comment.
The webhook proxy endpoint accepts a target URL from server configuration only (process.env.SHEET_WEBHOOK_URL), which is good for preventing SSRF. However, the endpoint does not validate or sanitize the configured URL before using it. If an administrator mistakenly sets SHEET_WEBHOOK_URL to a malicious or internal URL (e.g., http://localhost:6379/ for Redis), the server would blindly forward requests to it.
Consider adding URL validation to ensure the target is an allowed external URL (e.g., starts with https://script.google.com/ or matches an allowlist pattern). This provides defense-in-depth against configuration errors.
| fitbuddyai_tos_accepted_v1?: unknown; | ||
| }; |
There was a problem hiding this comment.
The BackupPayload type includes a field fitbuddyai_tos_accepted_v1?: unknown (line 20), which uses the unknown type. Later in the code (line 226), this is used in a Partial<BackupPayload> context where the value is conditionally set.
Using unknown for this field makes it difficult to work with and doesn't provide type safety. Based on usage in tosService.ts (line 95-99 in the diff, not shown but referenced), this appears to be a structured object. Consider defining a proper type for this field, such as:
fitbuddyai_tos_accepted_v1?: Record<string, { tos?: boolean; privacy?: boolean; timestamp?: number }>;Or at minimum use Record<string, unknown> instead of unknown to indicate it's an object.
| export async function requireAdmin(req: any): Promise<AdminJwtPayload | null> { | ||
| const jwtSecret = process.env.JWT_SECRET; | ||
| if (!jwtSecret) { | ||
| // Fail fast if JWT_SECRET is not set | ||
| console.error('[api/auth/index] Missing JWT_SECRET in environment; admin actions are disabled'); | ||
| return null; | ||
| } | ||
| const authHeader = String(req.headers['authorization'] || req.headers['Authorization'] || ''); | ||
| const match = authHeader.match(/^Bearer\s+(.+)$/i); | ||
| if (!match) return null; | ||
| const token = match[1]; | ||
| try { | ||
| const decoded = jwt.verify(token, jwtSecret) as string | AdminJwtPayload; | ||
| if ( | ||
| typeof decoded === 'object' && | ||
| decoded !== null && | ||
| 'role' in decoded && | ||
| (decoded as AdminJwtPayload).role && | ||
| ((decoded as AdminJwtPayload).role === 'service' || (decoded as AdminJwtPayload).role === 'admin') | ||
| ) { | ||
| return decoded as AdminJwtPayload; | ||
| } | ||
| return null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } |
There was a problem hiding this comment.
The requireAdmin function is exported (line 178) but it's designed to be called from within the same module's request handler. However, it's not clear if this function is intended to be used by other modules or only internally. If it's meant to be used elsewhere (e.g., in other API routes), the export makes sense. If it's only used within this file, it should not be exported or should be documented as internal-only.
Additionally, the function accepts req: any which loses type safety. Consider defining a request interface or using a more specific type from your framework (e.g., VercelRequest or Request from the framework you're using).
| if (data.refresh_token) { | ||
| try { | ||
| await supabase.auth.setSession({ | ||
| access_token: data.access_token, | ||
| refresh_token: data.refresh_token, | ||
| }); | ||
| } catch (e) { | ||
| // Swallow Supabase client errors here; app can still rely on | ||
| // the stored access token for server-side operations. | ||
| } |
There was a problem hiding this comment.
The refresh endpoint does not return the refresh_token to the client (line 375), which is correct for security. However, on line 287, when attempting to restore a session in the frontend, the code checks for data.refresh_token and only sets the session on the Supabase client if one is present. Since the server never returns a refresh_token, this condition will never be true, and the Supabase client session will never be restored via this flow.
This means the frontend Supabase client will not have a valid session after a refresh, which could cause issues with any client-side Supabase operations. Either the backend should return a refresh_token (reducing security), or the frontend should be updated to handle sessions that only have an access_token, or document that client-side Supabase operations requiring a refresh_token are not supported after server-side refresh.
| if (data.refresh_token) { | |
| try { | |
| await supabase.auth.setSession({ | |
| access_token: data.access_token, | |
| refresh_token: data.refresh_token, | |
| }); | |
| } catch (e) { | |
| // Swallow Supabase client errors here; app can still rely on | |
| // the stored access token for server-side operations. | |
| } | |
| try { | |
| const sessionPayload: any = { | |
| access_token: data.access_token, | |
| }; | |
| if (data.refresh_token) { | |
| sessionPayload.refresh_token = data.refresh_token; | |
| } | |
| await supabase.auth.setSession(sessionPayload); | |
| } catch (e) { | |
| // Swallow Supabase client errors here; app can still rely on | |
| // the stored access token for server-side operations. |
| // Attempt to clear server-side stored refresh token. Prefer `sendBeacon` | ||
| // during unloads; fall back to a short-await fetch to ensure revocation. | ||
| try { | ||
| const revokeUrl = '/api/auth?action=clear_refresh'; | ||
| let revoked = false; | ||
| // try navigator.sendBeacon first (non-blocking, reliable during unload) | ||
| try { | ||
| if (typeof navigator !== 'undefined' && typeof navigator.sendBeacon === 'function') { | ||
| const blob = new Blob([JSON.stringify({})], { type: 'application/json' }); | ||
| try { revoked = navigator.sendBeacon(revokeUrl, blob); } catch { revoked = false; } | ||
| } | ||
| } catch { | ||
| revoked = false; | ||
| } |
There was a problem hiding this comment.
The signOutAndRevoke function attempts to use navigator.sendBeacon first, but the payload sent is an empty JSON object {}. The backend endpoint /api/auth?action=clear_refresh expects to extract the session_id from cookies (line 374-375 in authServer.js), not from the request body. However, sendBeacon sends the blob as the request body, and there's no indication that the server reads cookies when processing beacon requests.
Additionally, on line 324, the code sets revoked = false if sendBeacon throws, but then on line 327, it also sets revoked = false in a catch block. Both of these should be unnecessary since revoked is initialized as false at line 319. The logic should be simplified and the sendBeacon payload doesn't need to be sent since the server uses cookies.
| useEffect(() => { | ||
| try { | ||
| if (showIntro) { | ||
| // Dispatch start event for other components | ||
| window.dispatchEvent(new CustomEvent('fitbuddyai-intro-start')); | ||
| try { document.body.classList.add('intro-active'); } catch (e) {} | ||
| // Lock scrolling: save scroll pos and fix body | ||
| try { | ||
| introScrollRef.current = window.scrollY || window.pageYOffset || 0; | ||
| document.body.style.position = 'fixed'; | ||
| document.body.style.top = `-${introScrollRef.current}px`; | ||
| document.body.style.left = '0'; | ||
| document.body.style.right = '0'; | ||
| document.body.style.overflow = 'hidden'; | ||
| // also disable html overflow for some browsers | ||
| try { document.documentElement.style.overflow = 'hidden'; } catch (e) {} | ||
| } catch (e) {} | ||
| } else { | ||
| // Dispatch end event and restore scrolling | ||
| window.dispatchEvent(new CustomEvent('fitbuddyai-intro-end')); | ||
| try { | ||
| const y = introScrollRef.current || 0; | ||
| document.body.style.position = ''; | ||
| document.body.style.top = ''; | ||
| document.body.style.left = ''; | ||
| document.body.style.right = ''; | ||
| document.body.style.overflow = ''; | ||
| try { document.documentElement.style.overflow = ''; } catch (e) {} | ||
| window.scrollTo(0, y); | ||
| introScrollRef.current = null; | ||
| } catch (e) {} | ||
| } | ||
| } catch (e) { | ||
| // ignore in non-browser environments | ||
| } | ||
| return () => { | ||
| // Ensure cleanup if component unmounts while intro active | ||
| try { | ||
| document.body.style.position = ''; | ||
| document.body.style.top = ''; | ||
| document.body.style.left = ''; | ||
| document.body.style.right = ''; | ||
| document.body.style.overflow = ''; | ||
| try { document.documentElement.style.overflow = ''; } catch (e) {} | ||
| try { document.body.classList.remove('intro-active'); } catch (e) {} | ||
| if (introScrollRef.current !== null) { | ||
| try { window.scrollTo(0, introScrollRef.current); } catch (e) {} | ||
| introScrollRef.current = null; | ||
| } | ||
| } catch (e) {} | ||
| }; | ||
| }, [showIntro]); |
There was a problem hiding this comment.
The welcome page locks scrolling when the intro is active by setting document.body.style.position = 'fixed' (line 59) and document.body.style.top = -${introScrollRef.current}px` (line 60). This is a common pattern to prevent background scrolling, but it has a side effect on mobile Safari: the address bar visibility can change when the position becomes fixed, causing a layout shift.
Additionally, the cleanup in the useEffect return (lines 85-100) restores the scroll position using window.scrollTo(0, y) at line 78 and again at line 96. If the component unmounts while the intro is active, both cleanup blocks will attempt to restore scrolling. Consider: 1) Using a single cleanup approach, 2) Adding a check to see if scrolling was already restored, 3) Testing on mobile Safari to ensure no jarring layout shifts occur.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request introduces a secure, server-side refresh token management system for authentication, including encrypted storage of refresh tokens, supporting database migrations, API endpoints, and scheduled cleanup. It also updates the frontend to integrate with this new flow, improving security and reliability for user sessions.
Backend: Secure Refresh Token Management
api/auth/index.ts[1] [2].Database: Schema & Policies
fitbuddyai_refresh_tokenstable, indexes, and helper functions for cleanup and revocation.Automation: Cleanup
scripts/cleanup_refresh_tokens.js) to run the cleanup function or fallback deletion via Supabase.Frontend: Session Handling & Routing
src/App.tsx) to store and clear refresh tokens via the new backend endpoints, and to refresh the access token using the server-side stored refresh token, removing direct client-side refresh token storage [1] [2] [3].BlogListPageand support dynamic blog slugs [1] [2].