Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 86 additions & 0 deletions frontend/src/__tests__/api-errors.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/**
* Issue #748: user-facing wording for API and transport failures.
*/
import {
BAD_RESPONSE_MESSAGE,
NETWORK_ERROR_MESSAGE,
friendlyError,
httpStatusMessage,
isGenericServerMessage,
isNetworkFailure,
readJson,
responseError,
} from '../api/errors';

function response(status: number, body: () => Promise<unknown>): Response {
return { ok: status < 400, status, url: '/api/auth/login', json: body } as unknown as Response;
}

describe('isNetworkFailure', () => {
test.each(['Failed to fetch', 'NetworkError when attempting to fetch resource.', 'Load failed'])(
'recognizes the browser fetch TypeError "%s"', (msg) => {
expect(isNetworkFailure(new TypeError(msg))).toBe(true);
});

test('ignores other errors', () => {
expect(isNetworkFailure(new TypeError('x is not a function'))).toBe(false);
expect(isNetworkFailure(new Error('Failed to fetch'))).toBe(false);
});
});

describe('friendlyError', () => {
test('maps a raw fetch failure and a JSON parse error', () => {
expect(friendlyError(new TypeError('Failed to fetch'))).toBe(NETWORK_ERROR_MESSAGE);
expect(friendlyError(new SyntaxError("Unexpected token '<'"))).toBe(BAD_RESPONSE_MESSAGE);
});

test('keeps an error message the server or client already made readable', () => {
expect(friendlyError(Object.assign(new Error('plan name already in use'), { status: 409 }))).toBe('plan name already in use');
});

test('falls back to the status text, then a generic sentence', () => {
expect(friendlyError(Object.assign(new Error(''), { status: 503 }))).toBe(httpStatusMessage(503));
expect(friendlyError(undefined)).toBe('Something went wrong. Try again in a moment.');
expect(friendlyError('quota exceeded')).toBe('quota exceeded');
});
});

describe('isGenericServerMessage', () => {
test('matches only the backend catch-all', () => {
expect(isGenericServerMessage('Internal server error')).toBe(true);
expect(isGenericServerMessage('internal server error.')).toBe(true);
expect(isGenericServerMessage('internal server error: db down')).toBe(false);
});
});

describe('responseError', () => {
test('prefers a specific server error, then message', async () => {
await expect(responseError(response(400, () => Promise.resolve({ error: 'bad email' })), 'Login failed'))
.resolves.toMatchObject({ message: 'bad email', status: 400 });
await expect(responseError(response(400, () => Promise.resolve({ message: 'weak password' })), 'x'))
.resolves.toMatchObject({ message: 'weak password' });
});

test('uses the friendly status text for a 5xx with no or a generic body', async () => {
await expect(responseError(response(502, () => Promise.reject(new SyntaxError('<'))), 'Login failed'))
.resolves.toMatchObject({ message: httpStatusMessage(502), status: 502, detail: 'HTTP 502 from /api/auth/login' });
await expect(responseError(response(500, () => Promise.resolve({ error: 'Internal server error' })), 'Login failed'))
.resolves.toMatchObject({ message: httpStatusMessage(500) });
});

test('uses the caller fallback for a 4xx with no message', async () => {
await expect(responseError(response(401, () => Promise.resolve({})), 'Login failed'))
.resolves.toMatchObject({ message: 'Login failed', status: 401 });
});
});

describe('readJson', () => {
test('returns the parsed body', async () => {
await expect(readJson(response(200, () => Promise.resolve({ a: 1 })))).resolves.toEqual({ a: 1 });
});

test('turns a parse failure into friendly text', async () => {
await expect(readJson(response(200, () => Promise.reject(new SyntaxError("Unexpected token '<'")))))
.rejects.toMatchObject({ message: BAD_RESPONSE_MESSAGE, status: 200 });
});
});
80 changes: 71 additions & 9 deletions frontend/src/__tests__/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,12 @@ describe('API Requests', () => {
status: 200,
json: () => Promise.reject(new SyntaxError('Unexpected token in JSON at position 0')),
});
await expect(apiRequest('/test')).rejects.toThrow(SyntaxError);
// Issue #748: rejects with friendly text, not "Unexpected token ...".
await expect(apiRequest('/test')).rejects.toMatchObject({
message: 'CUDly sent an unexpected response. Try again in a moment.',
status: 200,
detail: expect.stringContaining('Unexpected token in JSON'),
});
});

test('surfaces a timeout error when the backend hangs past timeoutMs (issue #20)', async () => {
Expand All @@ -419,7 +424,56 @@ describe('API Requests', () => {
});
}));

await expect(apiRequest('/test', { timeoutMs: 10 })).rejects.toThrow(/timed out after 10ms/);
// Issue #748: the user sees a sentence; the raw timing stays on detail.
await expect(apiRequest('/test', { timeoutMs: 10 })).rejects.toMatchObject({
message: 'CUDly took too long to respond. Try again in a moment.',
status: 0,
detail: 'Request to /test timed out after 10ms',
});
});

describe('friendly transport errors (issue #748)', () => {
test('a network failure becomes "Can\'t reach CUDly" with status 0', async () => {
fetchMock.mockRejectedValue(new TypeError('Failed to fetch'));
await expect(apiRequest('/test')).rejects.toMatchObject({
message: "Can't reach CUDly. Check your connection and try again.",
status: 0,
detail: 'Request to /test failed: Failed to fetch',
});
});

test('a caller abort is rethrown untouched', async () => {
const controller = new AbortController();
fetchMock.mockImplementation(() => {
controller.abort();
const err = new Error('The operation was aborted.');
err.name = 'AbortError';
return Promise.reject(err);
});
await expect(apiRequest('/test', { signal: controller.signal })).rejects.toMatchObject({ name: 'AbortError' });
});

test.each([
[502, 'CUDly is temporarily unavailable. Try again in a moment.'],
[503, 'CUDly is temporarily unavailable. Try again in a moment.'],
[504, 'CUDly took too long to respond. Try again in a moment.'],
[500, 'CUDly hit an unexpected error. Try again in a moment.'],
[403, "You don't have permission to do that."],
[418, 'The request could not be completed (error 418).'],
])('HTTP %i with an HTML body says what it means, not "HTTP %i"', async (status, message) => {
fetchMock.mockResolvedValue({ ok: false, status, json: () => Promise.reject(new SyntaxError("Unexpected token '<'")) });
await expect(apiRequest('/test')).rejects.toMatchObject({ message, status, detail: `HTTP ${status} from /test` });
});

test('the backend catch-all "Internal server error" gets the friendly 500 text', async () => {
fetchMock.mockResolvedValue({ ok: false, status: 500, json: () => Promise.resolve({ error: 'Internal server error' }) });
await expect(apiRequest('/test')).rejects.toThrow('CUDly hit an unexpected error. Try again in a moment.');
});

test('a specific server message still wins over the status text', async () => {
fetchMock.mockResolvedValue({ ok: false, status: 503, json: () => Promise.resolve({ error: 'maintenance until 10:00 UTC' }) });
await expect(apiRequest('/test')).rejects.toThrow('maintenance until 10:00 UTC');
});
});

test('passes the caller-provided signal through alongside the timeout controller', async () => {
Expand Down Expand Up @@ -662,15 +716,23 @@ describe('API Requests', () => {
expect(localStorageMock.setItem).toHaveBeenCalledWith('authToken', 'admin-token');
});

test('a non-JSON error body (proxy 502) yields a readable message, not a SyntaxError', async () => {
fetchMock.mockResolvedValue({
ok: false,
status: 502,
json: () => Promise.reject(new SyntaxError('Unexpected token < in JSON at position 0')),
});
// Issue #748: a proxy 502 with an HTML body used to surface as
// "Unexpected token '<'" because the error body was parsed uncaught.
test('a 502 HTML error page fails with friendly text, not a JSON parse error', async () => {
fetchMock.mockResolvedValue({ ok: false, status: 502, json: () => Promise.reject(new SyntaxError("Unexpected token '<'")) });
await expect(setupAdmin('api-key', 'admin@example.com', 'password'))
.rejects.toMatchObject({ message: 'CUDly is temporarily unavailable. Try again in a moment.', status: 502 });
});

test('a 400 keeps the server reason', async () => {
fetchMock.mockResolvedValue({ ok: false, status: 400, json: () => Promise.resolve({ error: 'admin already exists' }) });
await expect(setupAdmin('api-key', 'admin@example.com', 'password')).rejects.toThrow('admin already exists');
});

test('a network failure says the server cannot be reached', async () => {
fetchMock.mockRejectedValue(new TypeError('Failed to fetch'));
await expect(setupAdmin('api-key', 'admin@example.com', 'password'))
.rejects.toThrow('Failed to create admin (HTTP 502)');
.rejects.toThrow("Can't reach CUDly. Check your connection and try again.");
});

test('a JSON error body still surfaces the server message', async () => {
Expand Down
6 changes: 3 additions & 3 deletions frontend/src/__tests__/apikeys.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -962,7 +962,7 @@ describe('API Keys Module', () => {

await revokeApiKey('key-1');

expect(document.querySelector('.toast-error')?.querySelector('.toast-message')?.textContent).toBe('Failed to revoke API key');
expect(document.querySelector('.toast-error')?.querySelector('.toast-message')?.textContent).toBe('Failed to revoke API key: Revoke failed');
expect(document.getElementById('create-apikey-error')?.classList.contains('hidden')).toBe(true);
consoleError.mockRestore();
});
Expand All @@ -977,7 +977,7 @@ describe('API Keys Module', () => {
await revokeApiKey('key-1');

expect(consoleError).toHaveBeenCalledWith('Failed to revoke API key:', expect.any(Error));
expect(document.querySelector('.toast-error')?.querySelector('.toast-message')?.textContent).toBe('Failed to revoke API key');
expect(document.querySelector('.toast-error')?.querySelector('.toast-message')?.textContent).toBe('Failed to revoke API key: Revoke failed');
consoleError.mockRestore();
});
});
Expand Down Expand Up @@ -1031,7 +1031,7 @@ describe('API Keys Module', () => {
await deleteApiKey('key-1');

expect(consoleError).toHaveBeenCalledWith('Failed to delete API key:', expect.any(Error));
expect(document.querySelector('.toast-error')?.querySelector('.toast-message')?.textContent).toBe('Failed to delete API key');
expect(document.querySelector('.toast-error')?.querySelector('.toast-message')?.textContent).toBe('Failed to delete API key: Delete failed');
consoleError.mockRestore();
});
});
Expand Down
2 changes: 1 addition & 1 deletion frontend/src/__tests__/groups.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@
document.body.innerHTML = `
<div id="groups-list"></div>
`;
userState.setAllUsers(mockUsers as any);

Check warning on line 107 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
groupState.setCurrentEditingGroup(null);
});

Expand Down Expand Up @@ -165,7 +165,7 @@
// Set up users so group-1 has exactly 1 member
userState.setAllUsers([
{ id: 'user-1', email: 'admin@test.com', groups: ['00000000-0000-5000-8000-000000000001', 'group-1'], mfa_enabled: true },
] as any);

Check warning on line 168 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type

const group = mockGroups[0];
if (!group) throw new Error('Test data missing');
Expand Down Expand Up @@ -289,7 +289,7 @@
it('should attach click handlers to delete buttons', async () => {
(api.deleteGroup as jest.Mock).mockResolvedValue(undefined);
mockConfirmDialog.mockResolvedValue(true);
userState.setAvailableGroups(mockGroups as any);

Check warning on line 292 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type

groupList.renderGroups(mockGroups);

Expand Down Expand Up @@ -856,7 +856,7 @@
document.body.innerHTML = `
<div id="groups-list"></div>
`;
userState.setAvailableGroups(mockGroups as any);

Check warning on line 859 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
jest.clearAllMocks();
mockConfirmDialog.mockResolvedValue(true);
(api.deleteGroup as jest.Mock).mockResolvedValue(undefined);
Expand Down Expand Up @@ -910,7 +910,7 @@

await groupActions.deleteGroup('group-1');

expect(document.querySelector('.toast-error')?.textContent).toContain('Failed to delete group');
expect(document.querySelector('.toast-error')?.textContent).toContain('Failed to delete group: Server error');
});
});

Expand All @@ -929,22 +929,22 @@
</form>
`;
// Clear window properties
delete (window as any).openCreateGroupModal;

Check warning on line 932 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
delete (window as any).closeGroupModal;

Check warning on line 933 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
delete (window as any).addPermission;

Check warning on line 934 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
jest.clearAllMocks();
});

describe('setupGroupHandlers', () => {
it('should expose openCreateGroupModal on window', () => {
groupHandlers.setupGroupHandlers();
expect((window as any).openCreateGroupModal).toBeDefined();

Check warning on line 941 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
expect(typeof (window as any).openCreateGroupModal).toBe('function');

Check warning on line 942 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
});

it('should expose closeGroupModal on window', () => {
groupHandlers.setupGroupHandlers();
expect((window as any).closeGroupModal).toBeDefined();

Check warning on line 947 in frontend/src/__tests__/groups.test.ts

View workflow job for this annotation

GitHub Actions / Build frontend

Unexpected any. Specify a different type
expect(typeof (window as any).closeGroupModal).toBe('function');
});

Expand Down
5 changes: 3 additions & 2 deletions frontend/src/__tests__/settings-accounts.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2303,7 +2303,7 @@ describe('Bulk override modal (issue #119)', () => {

test('all saves fail: error shown in form, modal stays open', async () => {
(api.listAccountServiceOverrides as jest.Mock).mockResolvedValue([]);
(api.saveAccountServiceOverride as jest.Mock).mockRejectedValue(new Error('timeout'));
(api.saveAccountServiceOverride as jest.Mock).mockRejectedValue(new Error('coverage must be between 0 and 100'));

const { form, bulkList } = await openBulkModal();

Expand All @@ -2322,7 +2322,8 @@ describe('Bulk override modal (issue #119)', () => {
expect(mockShowToast).not.toHaveBeenCalled();

const errEl = document.getElementById('override-form-error') as HTMLElement;
expect(errEl.textContent).toMatch(/failed/i);
// Issue #748: the server's reason, not a blanket "Check your connection".
expect(errEl.textContent).toBe('All 2 saves failed: coverage must be between 0 and 100');

// Modal stays open so user can retry.
const modal = document.getElementById('override-modal') as HTMLElement;
Expand Down
7 changes: 7 additions & 0 deletions frontend/src/__tests__/users.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1135,6 +1135,13 @@ describe('users/userActions', () => {
);
});

it('shows the server reason when the delete fails (issue #748)', async () => {
(api.deleteUser as jest.Mock).mockRejectedValueOnce(new Error('cannot delete the last administrator'));
await userActions.deleteUser('1');

expect(document.querySelector('.toast-error')?.textContent).toContain('Failed to delete user: cannot delete the last administrator');
});

it('should reload users after deletion', async () => {
await userActions.deleteUser('1');

Expand Down
34 changes: 16 additions & 18 deletions frontend/src/api/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {
MFARecoveryCodesResponse,
MFALoginErrorCode,
} from './types';
import { fetchOrNetworkError, readJson, responseError } from './errors';

/**
* Login error raised when the server returns one of the MFA-related
Expand Down Expand Up @@ -63,21 +64,21 @@ export async function login(email: string, password: string, mfaCode?: string):
// Add content hash for CloudFront OAC
await addContentHashHeader(headers, body);

const response = await fetch(`${API_BASE}/auth/login`, {
const response = await fetchOrNetworkError(`${API_BASE}/auth/login`, {
method: 'POST',
headers,
body
});

if (!response.ok) {
const data = await response.json().catch(() => ({})) as { error?: string };
if (isMFALoginErrorCode(data.error)) {
throw new MFALoginError(data.error);
const err = await responseError(response, 'Login failed');
if (isMFALoginErrorCode(err.message)) {
throw new MFALoginError(err.message);
}
throw new Error(data.error || 'Login failed');
throw err;
}

const data = await response.json() as LoginResponse & { csrf_token?: string };
const data = await readJson<LoginResponse & { csrf_token?: string }>(response);
setAuthToken(data.token);
// Store CSRF token if provided by server
if (data.csrf_token) {
Expand Down Expand Up @@ -167,14 +168,13 @@ export async function requestPasswordReset(email: string): Promise<void> {
const headers: Record<string, string> = { 'Content-Type': 'application/json' };
await addContentHashHeader(headers, body);

const response = await fetch(`${API_BASE}/auth/forgot-password`, {
const response = await fetchOrNetworkError(`${API_BASE}/auth/forgot-password`, {
method: 'POST',
headers,
body
});
if (!response.ok) {
const data = await response.json().catch(() => ({})) as { error?: string };
throw new Error(data.error || 'Failed to send password reset email');
throw await responseError(response, 'Failed to send password reset email');
}
}

Expand All @@ -187,7 +187,7 @@ export async function resetPassword(token: string, newPassword: string): Promise
const headers: Record<string, string> = { 'Content-Type': 'application/json' };
await addContentHashHeader(headers, body);

const response = await fetch(`${API_BASE}/auth/reset-password`, {
const response = await fetchOrNetworkError(`${API_BASE}/auth/reset-password`, {
method: 'POST',
headers,
body
Expand All @@ -200,9 +200,7 @@ export async function resetPassword(token: string, newPassword: string): Promise
// different one") never reached the toast and users saw the opaque
// fallback. Read `error` first; keep `message` as a defensive
// fallback for any other shape. See issue #459.
const data = await response.json().catch(() => ({})) as { error?: string; message?: string };
const msg = data.error || data.message;
throw new Error(msg || 'Failed to reset password');
throw await responseError(response, 'Failed to reset password');
}
}

Expand Down Expand Up @@ -287,19 +285,19 @@ export async function setupAdmin(key: string, email: string, password: string):
const headers: Record<string, string> = { 'X-API-Key': key, 'Content-Type': 'application/json' };
await addContentHashHeader(headers, body);

const response = await fetch(`${API_BASE}/auth/setup-admin`, {
const response = await fetchOrNetworkError(`${API_BASE}/auth/setup-admin`, {
method: 'POST',
headers,
body
});

// A proxy 502 used to reach the user as "Unexpected token '<'" here
// because the error body was parsed with no catch (issue #748).
if (!response.ok) {
// A proxy 502 carries an HTML body; don't surface the JSON SyntaxError.
const data = await response.json().catch(() => ({})) as { error?: string };
throw new Error(data.error || `Failed to create admin (HTTP ${response.status})`);
throw await responseError(response, 'Failed to create admin');
}

const data = await response.json() as LoginResponse & { csrf_token?: string };
const data = await readJson<LoginResponse & { csrf_token?: string }>(response);
setAuthToken(data.token);
// Store CSRF token if provided by server
if (data.csrf_token) {
Expand Down
Loading
Loading