diff --git a/frontend/src/lib/api-error.test.ts b/frontend/src/lib/api-error.test.ts new file mode 100644 index 000000000000..832908de36f5 --- /dev/null +++ b/frontend/src/lib/api-error.test.ts @@ -0,0 +1,30 @@ +import { ApiError } from 'lib/api-error' + +describe('ApiError', () => { + it('keeps string fields from the response body', () => { + const error = new ApiError('failed', 400, undefined, { + detail: 'Enter a valid email address.', + code: 'invalid', + attr: 'email', + statusText: 'Bad Request', + link: 'https://example.com', + }) + + expect(error.detail).toBe('Enter a valid email address.') + expect(error.code).toBe('invalid') + expect(error.attr).toBe('email') + expect(error.statusText).toBe('Bad Request') + expect(error.link).toBe('https://example.com') + }) + + it.each([ + ['nested field errors', { email: ['Enter a valid email address.'] }], + ['a list', ['Enter a valid email address.']], + ['a number', 42], + ])('drops a non-string detail (%s) so it cannot reach a React child', (_name, detail) => { + const error = new ApiError('failed', 400, undefined, { detail }) + + expect(error.detail).toBeNull() + expect(error.data.detail).toEqual(detail) + }) +}) diff --git a/frontend/src/lib/api-error.ts b/frontend/src/lib/api-error.ts index ac41fff41377..9901af64d413 100644 --- a/frontend/src/lib/api-error.ts +++ b/frontend/src/lib/api-error.ts @@ -6,6 +6,10 @@ export function isAccessDeniedError(error: { status?: number; code?: string | nu return error.status === 403 && error.code === 'permission_denied' } +function asString(value: unknown): string | null { + return typeof value === 'string' && value ? value : null +} + export class ApiError extends Error { /** Django REST Framework `detail` - used in downstream error handling. */ detail: string | null @@ -27,11 +31,13 @@ export class ApiError extends Error { ) { message = message || `API request failed with status: ${status ?? 'unknown'}` super(message) - this.statusText = data?.statusText || null - this.detail = data?.detail || null - this.code = data?.code || null - this.link = data?.link || null - this.attr = data?.attr || null + // Only keep string values: these fields are declared `string | null` and consumers render them + // directly into JSX, so a non-string body value would crash React. The raw body stays on `data`. + this.statusText = asString(data?.statusText) + this.detail = asString(data?.detail) + this.code = asString(data?.code) + this.link = asString(data?.link) + this.attr = asString(data?.attr) } /** diff --git a/frontend/src/queries/nodes/InsightViz/utils.ts b/frontend/src/queries/nodes/InsightViz/utils.ts index d5b9c03d405a..4fefb0e09533 100644 --- a/frontend/src/queries/nodes/InsightViz/utils.ts +++ b/frontend/src/queries/nodes/InsightViz/utils.ts @@ -202,8 +202,12 @@ export const extractValidationError = (error: Error | Record | null if (hasValidationErrorStatus(error)) { // Async queries put the error message on data.error_message, while synchronous ones use detail const anyError = error as Record + const message = anyError.detail || anyError.data?.error_message + if (typeof message !== 'string') { + return null + } // Add unbreakable space for better line breaking - return (anyError.detail || anyError.data?.error_message)?.replace('Try ', 'Try\u00A0') ?? null + return message.replace('Try ', 'Try\u00A0') } return null diff --git a/frontend/src/queries/query.test.ts b/frontend/src/queries/query.test.ts index cc9633ec3d66..e4253f350c26 100644 --- a/frontend/src/queries/query.test.ts +++ b/frontend/src/queries/query.test.ts @@ -307,7 +307,8 @@ describe('query', () => { }) }) - it('handles non-string error message', async () => { + // A non-string detail used to pass straight through and crash React when rendered + it('drops a non-string error message', async () => { jest.spyOn(api.queryStatus, 'get').mockRejectedValueOnce({ data: { query_status: { @@ -317,7 +318,7 @@ describe('query', () => { }) await expect(pollForResults('test-query-id')).rejects.toMatchObject({ - detail: { nested: 'object' }, + detail: '', }) }) }) diff --git a/frontend/src/queries/query.ts b/frontend/src/queries/query.ts index 756c2e771d54..32fe236e6a9b 100644 --- a/frontend/src/queries/query.ts +++ b/frontend/src/queries/query.ts @@ -72,9 +72,10 @@ export const QUERY_TIMEOUT_ERROR_MESSAGE = 'Query timed out' * This function safely extracts the message and code, falling back to the * original string if parsing fails. */ -export function parseErrorMessage(errorMessage: string | undefined): { message: string; code: string | null } { +export function parseErrorMessage(errorMessage: string | null | undefined): { message: string; code: string | null } { if (!errorMessage || typeof errorMessage !== 'string') { - return { message: errorMessage || '', code: null } + // Never hand back a non-string: callers render this straight into JSX. + return { message: typeof errorMessage === 'string' ? errorMessage : '', code: null } } // Try to match list format: [ErrorDetail(string='...', code='...')] diff --git a/frontend/src/scenes/experiments/MetricsView/new/MetricErrorState.tsx b/frontend/src/scenes/experiments/MetricsView/new/MetricErrorState.tsx index 6dbf9ead2e7c..580f4ef2e2fc 100644 --- a/frontend/src/scenes/experiments/MetricsView/new/MetricErrorState.tsx +++ b/frontend/src/scenes/experiments/MetricsView/new/MetricErrorState.tsx @@ -12,7 +12,7 @@ import { ExperimentMetric } from '~/queries/schema/schema-general' type MetricErrorStateProps = { error: { - detail: string | object | any + detail?: string | null statusCode?: number queryId?: string } diff --git a/frontend/src/scenes/experiments/legacy/metricsView/LegacyErrorChecklist.tsx b/frontend/src/scenes/experiments/legacy/metricsView/LegacyErrorChecklist.tsx index c87bee527e14..adf7ed5ea580 100644 --- a/frontend/src/scenes/experiments/legacy/metricsView/LegacyErrorChecklist.tsx +++ b/frontend/src/scenes/experiments/legacy/metricsView/LegacyErrorChecklist.tsx @@ -169,5 +169,5 @@ export function LegacyErrorChecklist({ error, metric }: { error: any; metric: an } // Other unexpected errors - return
{error.detail}
+ return
{typeof error.detail === 'string' ? error.detail : 'An unexpected error occurred'}
} diff --git a/frontend/src/scenes/insights/EmptyStates/EmptyStates.tsx b/frontend/src/scenes/insights/EmptyStates/EmptyStates.tsx index 24d90b3d0698..a5569e818da3 100644 --- a/frontend/src/scenes/insights/EmptyStates/EmptyStates.tsx +++ b/frontend/src/scenes/insights/EmptyStates/EmptyStates.tsx @@ -2,7 +2,7 @@ import './EmptyStates.scss' import clsx from 'clsx' import { useActions, useValues } from 'kea' -import { useEffect, useState } from 'react' +import { isValidElement, useEffect, useState } from 'react' import { TextMorph } from 'torph/react' import * as construction2Png from '@posthog/brand/hoggies/png/construction-2' @@ -699,6 +699,9 @@ export function InsightErrorState({ ) + // `title` is often fed straight from an API error body, so fall back rather than trust the declared type + const renderableTitle = typeof title === 'string' || isValidElement(title) ? title : null + return (
{/* Note that this default phrasing signals the issue is intermittent, */} {/* and that perhaps the query will complete on retry */} - {title || There was a problem completing this query} + {renderableTitle || There was a problem completing this query} {!excludeDetail && !supportOnly && ( diff --git a/frontend/src/scenes/onboarding/shared/wizard-sync/InstallationProgressView.tsx b/frontend/src/scenes/onboarding/shared/wizard-sync/InstallationProgressView.tsx index fa8c5d01d713..b1546bf6c52c 100644 --- a/frontend/src/scenes/onboarding/shared/wizard-sync/InstallationProgressView.tsx +++ b/frontend/src/scenes/onboarding/shared/wizard-sync/InstallationProgressView.tsx @@ -273,7 +273,7 @@ export function InstallationProgressContent({
)} - {phase === 'error' && error?.detail && ( + {phase === 'error' && typeof error?.detail === 'string' && (
{error.detail}
)}