From ade718973b46c769ab8a9a570f14cce9ea50f44d Mon Sep 17 00:00:00 2001 From: Dave Liu <7david12liu@gmail.com> Date: Tue, 14 Jul 2026 00:01:52 -0700 Subject: [PATCH] Redact Strava callback failures (#242) --- docs/officers/EVENTS_SHOP_MEMBERS.md | 36 +++++ src/pages/account/Account.test.tsx | 210 ++++++++++++++++++++++++++- src/pages/account/StravaCallback.tsx | 17 ++- 3 files changed, 258 insertions(+), 5 deletions(-) diff --git a/docs/officers/EVENTS_SHOP_MEMBERS.md b/docs/officers/EVENTS_SHOP_MEMBERS.md index 3ac0c44..94496e0 100644 --- a/docs/officers/EVENTS_SHOP_MEMBERS.md +++ b/docs/officers/EVENTS_SHOP_MEMBERS.md @@ -387,6 +387,42 @@ Officer review steps after the source merge: **Escalation:** membership lead plus treasurer and privacy/security owner. Add the platform owner for source or publication evidence. Use the private incident path if real account or membership data appears. +## Strava callback failure privacy — SOURCE ONLY, NOT LIVE + +**Status: NOT AVAILABLE YET** + +**Purpose:** give a member one plain next step when a Strava connection fails without showing a provider message, callback detail, or technical error on the page. + +**Approver:** membership lead plus platform/security owner. + +**Prerequisites:** issue [#242](https://github.com/Run-MPRC/Run-MPRC.github.io/issues/242) must be merged for source review. Calling the wording live also requires a protected website publication and an exact revision check on `runmprc.com`. This source change does not deploy Firebase, contact Strava, change provider settings, use production data, or prove live behavior. + +Officer review steps after the source merge: + +1. Keep the callback wording marked **NOT AVAILABLE YET**. +2. Ask the platform owner for the exact #242 issue, pull request, merged commit, and synthetic frontend test result. +3. Confirm the tests use only made-up callback values and a mocked exchange result. +4. Confirm a signed-out visitor sees only the fixed sign-in instruction. +5. Confirm a made-up provider query failure shows `We could not connect Strava. Please return to My Account and try again.` +6. Confirm a made-up exchange failure shows the same sentence. +7. Confirm no made-up provider detail appears on the page or in browser console output. +8. Confirm missing-code and failed-security-check results still stop before an exchange. +9. Confirm only a successful exchange returns to My Account, and the visible `Back to account` link still works without an exchange. +10. Confirm the failure sentence is announced as an urgent screen-reader alert. +11. Record website publication, `runmprc.com`, Firebase, Strava, production-data, and live-behavior evidence as separate results. + +**Expected result:** the reviewed source uses one fixed, actionable sentence for both a callback query failure and an exchange failure. It does not inspect, display, or log the rejected exchange value. Existing sign-in, missing-code, failed-security-check, success, and Back-to-account behavior stays in place. This does not remove the callback details from the browser address. + +**Stop conditions:** any real member or Strava account; a request for a callback URL, authorization code, state value, provider error, private browser history, or screenshot containing private values; a real provider call; a production Firebase or Strava change; a raw detail in the page or console; or a claim that source, tests, merge, or a green workflow proves the wording is live. + +**Success proof:** for source completion, record the exact #242 issue, reviewed pull request, merged commit, intended old-source failures, green synthetic callback tests, relevant full checks, and independent privacy review. For live availability, separately record the approved website publication, the published revision, and a dated `runmprc.com` check that uses no real account or callback value. Record Firebase deployment, Strava/provider configuration, and production-data actions as **not performed** for this frontend-only change. + +**Undo:** before publication, use one reviewed frontend revert or safe roll-forward. After publication, use the same protected website release path and verify the replacement revision on `runmprc.com`. Do not undo by changing a member account, callback value, Firebase record, or Strava setting. + +**Escalation:** membership lead plus platform/security owner. Add the privacy owner and use the private incident path if any callback or provider detail appeared. Do not copy the detail into an issue, message, screenshot, or AI tool. + +No system diagram changes for this source slice because page structure, data movement, permissions, account ownership, and deployment topology are unchanged. + ## Refund amount and returned-result guards — SOURCE ONLY, NOT LIVE **Purpose:** make an invalid partial amount stop, and record a refund complete only when Stripe returns a matching final success. diff --git a/src/pages/account/Account.test.tsx b/src/pages/account/Account.test.tsx index b701659..0743dd2 100644 --- a/src/pages/account/Account.test.tsx +++ b/src/pages/account/Account.test.tsx @@ -6,20 +6,37 @@ import { join } from 'path'; import { act, fireEvent, render, screen, waitFor, } from '@testing-library/react'; -import { MemoryRouter } from 'react-router-dom'; +import { + MemoryRouter, Route, Routes, useNavigationType, +} from 'react-router-dom'; import { useServiceLocator } from '../../services/ServiceLocatorContext'; +import { useAuth } from '../../services/hooks/useAuth'; import { ensureMyProfile, getMyProfile, listMyRegistrations, updateMyProfile, } from '../../services/account/accountService'; +import { + stravaExchangeCode, + verifyStravaState, +} from '../../services/strava/stravaService'; import { AccountContent } from './Account'; +import StravaCallback from './StravaCallback'; jest.mock('../../services/ServiceLocatorContext', () => ({ useServiceLocator: jest.fn(), })); +jest.mock('../../services/hooks/useAuth', () => ({ + useAuth: jest.fn(), +})); + +jest.mock('../../services/strava/stravaService', () => ({ + stravaExchangeCode: jest.fn(), + verifyStravaState: jest.fn(), +})); + jest.mock('../../services/account/accountService', () => { const actual = jest.requireActual('../../services/account/accountService'); return { @@ -462,3 +479,194 @@ describe('Account profile recovery', () => { expect(css).toMatch(/\.verification-resend__button:focus-visible\s*\{[\s\S]*outline:/); }); }); + +const STRAVA_CALLBACK_FAILURE = 'We could not connect Strava. Please return to My Account and try again.'; + +function CallbackAccountDestination() { + const navigationType = useNavigationType(); + + return ( +
+ Account destination + {navigationType} +
+ ); +} + +function renderStravaCallback( + entry = '/account/strava/callback?code=synthetic-code&state=synthetic-state', +) { + return render( + + + } /> + } /> + + , + ); +} + +describe('Strava callback error boundary', () => { + beforeEach(() => { + jest.clearAllMocks(); + (useServiceLocator as jest.Mock).mockReturnValue({ + services: { firebaseResources: { app } }, + isReady: true, + }); + (useAuth as jest.Mock).mockReturnValue({ + isAuthenticated: true, + isLoading: false, + }); + (verifyStravaState as jest.Mock).mockReturnValue(true); + (stravaExchangeCode as jest.Mock).mockResolvedValue({ + ok: true, + athleteId: null, + }); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + test.each([ + ['Firebase services are not ready', false, false], + ['authentication is loading', true, true], + ])('waits without callback work when %s', async (_case, isReady, isLoading) => { + (useServiceLocator as jest.Mock).mockReturnValue({ + services: isReady ? { firebaseResources: { app } } : null, + isReady, + }); + (useAuth as jest.Mock).mockReturnValue({ + isAuthenticated: true, + isLoading, + }); + + renderStravaCallback(); + + expect(await screen.findByText('Connecting your Strava...')).toBeInTheDocument(); + expect(verifyStravaState).not.toHaveBeenCalled(); + expect(stravaExchangeCode).not.toHaveBeenCalled(); + }); + + test('requires sign-in before exposing or acting on query failure details', async () => { + (useAuth as jest.Mock).mockReturnValue({ + isAuthenticated: false, + isLoading: false, + }); + + renderStravaCallback( + '/account/strava/callback?error=signed-out-provider-canary&code=hidden-code&state=hidden-state', + ); + + expect(await screen.findByText('You need to be signed in to connect Strava.')) + .toBeInTheDocument(); + expect(document.body).not.toHaveTextContent(/signed-out-provider-canary|hidden-code|hidden-state/); + expect(verifyStravaState).not.toHaveBeenCalled(); + expect(stravaExchangeCode).not.toHaveBeenCalled(); + }); + + test('replaces a provider query error with one fixed actionable alert', async () => { + renderStravaCallback( + '/account/strava/callback?error=provider%3Aprivate-query-canary&code=hidden-code&state=hidden-state', + ); + + const alert = await screen.findByRole('alert'); + expect(alert).toHaveTextContent(STRAVA_CALLBACK_FAILURE); + expect(alert).toHaveAttribute('aria-live', 'assertive'); + expect(alert).toHaveAttribute('aria-atomic', 'true'); + expect(document.body).not.toHaveTextContent(/private-query-canary|hidden-code|hidden-state/); + expect(verifyStravaState).not.toHaveBeenCalled(); + expect(stravaExchangeCode).not.toHaveBeenCalled(); + expect(screen.getByRole('link', { name: 'Back to account' })) + .toHaveAttribute('href', '/account'); + }); + + test('stops a missing code before state verification or exchange', async () => { + renderStravaCallback('/account/strava/callback?state=synthetic-state'); + + expect(await screen.findByRole('alert')).toHaveTextContent( + 'Missing authorization code from Strava.', + ); + expect(verifyStravaState).not.toHaveBeenCalled(); + expect(stravaExchangeCode).not.toHaveBeenCalled(); + }); + + test('stops an invalid state before exchange', async () => { + (verifyStravaState as jest.Mock).mockReturnValue(false); + + renderStravaCallback(); + + expect(await screen.findByRole('alert')).toHaveTextContent( + 'Security check failed (state mismatch). Please try connecting again.', + ); + expect(verifyStravaState).toHaveBeenCalledTimes(1); + expect(verifyStravaState).toHaveBeenCalledWith('synthetic-state'); + expect(stravaExchangeCode).not.toHaveBeenCalled(); + }); + + test('verifies state before one exact exchange and replaces the account route on success', async () => { + const calls: string[] = []; + (verifyStravaState as jest.Mock).mockImplementation(() => { + calls.push('verify'); + return true; + }); + (stravaExchangeCode as jest.Mock).mockImplementation(async () => { + calls.push('exchange'); + return { ok: true, athleteId: null }; + }); + + renderStravaCallback(); + + expect(await screen.findByText('Account destination')).toBeInTheDocument(); + expect(calls).toEqual(['verify', 'exchange']); + expect(verifyStravaState).toHaveBeenCalledWith('synthetic-state'); + expect(stravaExchangeCode).toHaveBeenCalledWith(app, 'synthetic-code'); + expect(stravaExchangeCode).toHaveBeenCalledTimes(1); + expect(screen.getByTestId('callback-navigation-type')).toHaveTextContent('REPLACE'); + }); + + test('does not navigate while the exchange result is pending', async () => { + (stravaExchangeCode as jest.Mock).mockReturnValue(new Promise(() => { + // Keep this synthetic exchange pending until the test unmounts the page. + })); + + renderStravaCallback(); + + await waitFor(() => expect(stravaExchangeCode).toHaveBeenCalledTimes(1)); + expect(screen.getByText('Connecting your Strava...')).toBeInTheDocument(); + expect(screen.queryByText('Account destination')).not.toBeInTheDocument(); + }); + + test.each([ + ['ordinary error', () => new Error('ordinary-provider-canary')], + ['hostile message getter', () => Object.defineProperty({}, 'message', { + configurable: true, + get() { + throw new Error('message-getter-canary'); + }, + })], + ])('does not inspect or render an %s rejection', async (_case, makeRejection) => { + const consoleSpies = [ + jest.spyOn(console, 'log').mockImplementation(() => undefined), + jest.spyOn(console, 'warn').mockImplementation(() => undefined), + jest.spyOn(console, 'error').mockImplementation(() => undefined), + ]; + (stravaExchangeCode as jest.Mock).mockRejectedValueOnce(makeRejection()); + + renderStravaCallback(); + + const alert = await screen.findByRole('alert'); + expect(alert).toHaveTextContent(STRAVA_CALLBACK_FAILURE); + expect(document.body).not.toHaveTextContent(/ordinary-provider-canary|message-getter-canary/); + expect(screen.queryByText('Account destination')).not.toBeInTheDocument(); + expect(verifyStravaState).toHaveBeenCalledTimes(1); + expect(stravaExchangeCode).toHaveBeenCalledTimes(1); + consoleSpies.forEach((spy) => expect(spy).not.toHaveBeenCalled()); + }); +}); diff --git a/src/pages/account/StravaCallback.tsx b/src/pages/account/StravaCallback.tsx index 633db02..a376344 100644 --- a/src/pages/account/StravaCallback.tsx +++ b/src/pages/account/StravaCallback.tsx @@ -5,6 +5,8 @@ import { useAuth } from '../../services/hooks/useAuth'; import SEO from '../../components/SEO'; import { stravaExchangeCode, verifyStravaState } from '../../services/strava/stravaService'; +const STRAVA_CALLBACK_FAILURE = 'We could not connect Strava. Please return to My Account and try again.'; + function StravaCallback() { const [params] = useSearchParams(); const { services, isReady } = useServiceLocator(); @@ -25,7 +27,7 @@ function StravaCallback() { } if (err) { setStatus('error'); - setMessage(`Strava returned an error: ${err}`); + setMessage(STRAVA_CALLBACK_FAILURE); return; } if (!code) { @@ -42,9 +44,9 @@ function StravaCallback() { stravaExchangeCode(services.firebaseResources.app, code) .then(() => setStatus('done')) - .catch((e) => { + .catch(() => { setStatus('error'); - setMessage(e?.message || 'Failed to exchange code with Strava.'); + setMessage(STRAVA_CALLBACK_FAILURE); }); }, [services, isReady, isAuthenticated, isLoading, code, state, err]); @@ -65,7 +67,14 @@ function StravaCallback() { {status === 'error' && ( <>

Connection failed

-

{message}

+

+ {message} +

Back to account