Skip to content

Commit

Permalink
Performance Profiler: Add a guard to prevent invalid performance urls…
Browse files Browse the repository at this point in the history
… from being saved. (#98295)

* extract usePerformanceReport into own hook and guard agains invalid urls being saved.

* Use URL class for validation instead of regex.

Co-authored-by: Luis Felipe Zaguini <[email protected]>

* use canParse method to validate url

* fallback to regex when `canParse` is not available

---------

Co-authored-by: Luis Felipe Zaguini <[email protected]>
  • Loading branch information
vykes-mac and zaguiini authored Jan 13, 2025

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent b931027 commit 26608bb
Showing 3 changed files with 210 additions and 102 deletions.
113 changes: 113 additions & 0 deletions client/hosting/performance/hooks/usePerformanceReport.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
import { useState, useCallback, useEffect } from 'react';
import { useUrlBasicMetricsQuery } from 'calypso/data/site-profiler/use-url-basic-metrics-query';
import { useUrlPerformanceInsightsQuery } from 'calypso/data/site-profiler/use-url-performance-insights';
import { TabType } from 'calypso/performance-profiler/components/header';

function isValidURL( url: string ) {
if ( 'canParse' in URL ) {
return URL.canParse( url );
}
return /^(https?:\/\/)?([a-z0-9-]+\.)+[a-z]{2,}(:[0-9]{1,5})?(\/[^\s]*)?$/i.test( url );
}

export const usePerformanceReport = (
setIsSavingPerformanceReportUrl: ( isSaving: boolean ) => void,
refetchPages: () => void,
savePerformanceReportUrl: (
pageId: string,
wpcom_performance_report_url: { url: string; hash: string }
) => Promise< void >,
currentPageId: string,
wpcom_performance_report_url: { url: string; hash: string } | undefined,
activeTab: TabType
) => {
const { url = '', hash = '' } = wpcom_performance_report_url || {};

const [ retestState, setRetestState ] = useState( 'idle' );

const {
data: basicMetrics,
isError: isBasicMetricsError,
isFetched: isBasicMetricsFetched,
isLoading: isLoadingBasicMetrics,
refetch: requeueAdvancedMetrics,
} = useUrlBasicMetricsQuery( url, hash, true );

const { final_url: finalUrl, token } = basicMetrics || {};

useEffect( () => {
if ( token && finalUrl && isValidURL( finalUrl ) ) {
setIsSavingPerformanceReportUrl( true );
savePerformanceReportUrl( currentPageId, { url: finalUrl, hash: token } )
.then( () => {
refetchPages();
} )
.finally( () => {
setIsSavingPerformanceReportUrl( false );
} );
}
// We only want to run this effect when the token changes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ token ] );

const {
data,
status: insightsStatus,
isError: isInsightsError,
isLoading: isLoadingInsights,
} = useUrlPerformanceInsightsQuery( url, token ?? hash );

const performanceInsights = data?.pagespeed;

const mobileReport =
typeof performanceInsights?.mobile === 'string' ? undefined : performanceInsights?.mobile;
const desktopReport =
typeof performanceInsights?.desktop === 'string' ? undefined : performanceInsights?.desktop;

const performanceReport = activeTab === 'mobile' ? mobileReport : desktopReport;

const desktopLoaded = typeof performanceInsights?.desktop === 'object';
const mobileLoaded = typeof performanceInsights?.mobile === 'object';

const getHashOrToken = (
hash: string | undefined,
token: string | undefined,
isReportLoaded: boolean
) => {
if ( hash ) {
return hash;
} else if ( token && isReportLoaded ) {
return token;
}
return '';
};

const testAgain = useCallback( async () => {
setRetestState( 'queueing-advanced' );
const result = await requeueAdvancedMetrics();
setRetestState( 'polling-for-insights' );
return result;
}, [ requeueAdvancedMetrics ] );

if (
retestState === 'polling-for-insights' &&
insightsStatus === 'success' &&
( activeTab === 'mobile' ? mobileLoaded : desktopLoaded )
) {
setRetestState( 'idle' );
}

return {
performanceReport,
url: finalUrl ?? url,
hash: getHashOrToken( hash, token, activeTab === 'mobile' ? mobileLoaded : desktopLoaded ),
isLoading:
isLoadingBasicMetrics ||
isLoadingInsights ||
( activeTab === 'mobile' ? ! mobileLoaded : ! desktopLoaded ),
isError: isBasicMetricsError || isInsightsError,
isBasicMetricsFetched,
testAgain,
isRetesting: retestState !== 'idle',
};
};
104 changes: 2 additions & 102 deletions client/hosting/performance/site-performance.tsx
Original file line number Diff line number Diff line change
@@ -3,12 +3,10 @@ import { useMobileBreakpoint } from '@automattic/viewport-react';
import { Button } from '@wordpress/components';
import { translate } from 'i18n-calypso';
import moment from 'moment';
import { useCallback, useEffect, useMemo, useState } from 'react';
import { useEffect, useMemo, useState } from 'react';
import { useSiteSettings } from 'calypso/blocks/plugins-scheduled-updates/hooks/use-site-settings';
import InlineSupportLink from 'calypso/components/inline-support-link';
import NavigationHeader from 'calypso/components/navigation-header';
import { useUrlBasicMetricsQuery } from 'calypso/data/site-profiler/use-url-basic-metrics-query';
import { useUrlPerformanceInsightsQuery } from 'calypso/data/site-profiler/use-url-performance-insights';
import {
DeviceTabProvider,
useDeviceTab,
@@ -31,6 +29,7 @@ import { PerformanceReportLoading } from './components/PerformanceReportLoading'
import { ReportUnavailable } from './components/ReportUnavailable';
import { DeviceTabControls } from './components/device-tab-control';
import { ExpiredReportNotice } from './components/expired-report-notice/expired-report-notice';
import { usePerformanceReport } from './hooks/usePerformanceReport';
import { useSitePerformancePageReports } from './hooks/useSitePerformancePageReports';

import './style.scss';
@@ -44,105 +43,6 @@ const statsQuery = {
max: 0,
};

const usePerformanceReport = (
setIsSavingPerformanceReportUrl: ( isSaving: boolean ) => void,
refetchPages: () => void,
savePerformanceReportUrl: (
pageId: string,
wpcom_performance_report_url: { url: string; hash: string }
) => Promise< void >,
currentPageId: string,
wpcom_performance_report_url: { url: string; hash: string } | undefined,
activeTab: TabType
) => {
const { url = '', hash = '' } = wpcom_performance_report_url || {};

const [ retestState, setRetestState ] = useState( 'idle' );

const {
data: basicMetrics,
isError: isBasicMetricsError,
isFetched: isBasicMetricsFetched,
isLoading: isLoadingBasicMetrics,
refetch: requeueAdvancedMetrics,
} = useUrlBasicMetricsQuery( url, hash, true );
const { final_url: finalUrl, token } = basicMetrics || {};
useEffect( () => {
if ( token && finalUrl ) {
setIsSavingPerformanceReportUrl( true );
savePerformanceReportUrl( currentPageId, { url: finalUrl, hash: token } )
.then( () => {
refetchPages();
} )
.finally( () => {
setIsSavingPerformanceReportUrl( false );
} );
}
// We only want to run this effect when the token changes.
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [ token ] );
const {
data,
status: insightsStatus,
isError: isInsightsError,
isLoading: isLoadingInsights,
} = useUrlPerformanceInsightsQuery( url, token ?? hash );

const performanceInsights = data?.pagespeed;

const mobileReport =
typeof performanceInsights?.mobile === 'string' ? undefined : performanceInsights?.mobile;
const desktopReport =
typeof performanceInsights?.desktop === 'string' ? undefined : performanceInsights?.desktop;

const performanceReport = activeTab === 'mobile' ? mobileReport : desktopReport;

const desktopLoaded = typeof performanceInsights?.desktop === 'object';
const mobileLoaded = typeof performanceInsights?.mobile === 'object';

const getHashOrToken = (
hash: string | undefined,
token: string | undefined,
isReportLoaded: boolean
) => {
if ( hash ) {
return hash;
} else if ( token && isReportLoaded ) {
return token;
}
return '';
};

const testAgain = useCallback( async () => {
setRetestState( 'queueing-advanced' );
const result = await requeueAdvancedMetrics();
setRetestState( 'polling-for-insights' );
return result;
}, [ requeueAdvancedMetrics ] );

if (
retestState === 'polling-for-insights' &&
insightsStatus === 'success' &&
( activeTab === 'mobile' ? mobileLoaded : desktopLoaded )
) {
setRetestState( 'idle' );
}

return {
performanceReport,
url: finalUrl ?? url,
hash: getHashOrToken( hash, token, activeTab === 'mobile' ? mobileLoaded : desktopLoaded ),
isLoading:
isLoadingBasicMetrics ||
isLoadingInsights ||
( activeTab === 'mobile' ? ! mobileLoaded : ! desktopLoaded ),
isError: isBasicMetricsError || isInsightsError,
isBasicMetricsFetched,
testAgain,
isRetesting: retestState !== 'idle',
};
};

const SitePerformanceContent = () => {
const dispatch = useDispatch();
const { activeTab, setActiveTab } = useDeviceTab();
95 changes: 95 additions & 0 deletions client/hosting/performance/test/use-performance-report.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
/**
* @jest-environment jsdom
*/

import { renderHook } from '@testing-library/react';
import { useUrlBasicMetricsQuery } from 'calypso/data/site-profiler/use-url-basic-metrics-query';
import { useUrlPerformanceInsightsQuery } from 'calypso/data/site-profiler/use-url-performance-insights';
import { usePerformanceReport } from '../hooks/usePerformanceReport';

jest.mock( 'calypso/data/site-profiler/use-url-basic-metrics-query' );
jest.mock( 'calypso/data/site-profiler/use-url-basic-metrics-query' );
jest.mock( 'calypso/data/site-profiler/use-url-performance-insights' );

describe( 'usePerformanceReport', () => {
const mockSetIsSaving = jest.fn();
const mockRefetchPages = jest.fn();
const mockSavePerformanceReportUrl = jest.fn().mockResolvedValue( undefined );

beforeEach( () => {
jest.clearAllMocks();

( useUrlBasicMetricsQuery as jest.Mock ).mockReturnValue( {
data: null,
isError: false,
isFetched: false,
isLoading: false,
refetch: jest.fn(),
} );

( useUrlPerformanceInsightsQuery as jest.Mock ).mockReturnValue( {
data: null,
status: 'idle',
isError: false,
isLoading: false,
} );
} );

it( 'should not call savePerformanceReportUrl when finalUrl is not a valid URL but token exists', () => {
( useUrlBasicMetricsQuery as jest.Mock ).mockReturnValue( {
data: {
final_url: 'false',
token: 'some-token',
},
isError: false,
isFetched: true,
isLoading: false,
refetch: jest.fn(),
} );

renderHook( () =>
usePerformanceReport(
mockSetIsSaving,
mockRefetchPages,
mockSavePerformanceReportUrl,
'123',
{ url: 'test.com', hash: 'test-hash' },
'mobile'
)
);

expect( mockSavePerformanceReportUrl ).not.toHaveBeenCalled();
expect( mockSetIsSaving ).not.toHaveBeenCalled();
} );

it( 'should call savePerformanceReportUrl when finalUrl is a valid URL and token exists', () => {
// Mock the basic metrics query to return valid finalUrl and token
( useUrlBasicMetricsQuery as jest.Mock ).mockReturnValue( {
data: {
final_url: 'https://final-url.com',
token: 'valid-token',
},
isError: false,
isFetched: true,
isLoading: false,
refetch: jest.fn(),
} );

renderHook( () =>
usePerformanceReport(
mockSetIsSaving,
mockRefetchPages,
mockSavePerformanceReportUrl,
'123',
{ url: 'test.com', hash: 'test-hash' },
'mobile'
)
);

expect( mockSavePerformanceReportUrl ).toHaveBeenCalledWith( '123', {
url: 'https://final-url.com',
hash: 'valid-token',
} );
expect( mockSetIsSaving ).toHaveBeenCalledWith( true );
} );
} );

0 comments on commit 26608bb

Please sign in to comment.