Skip to content

Commit 07ab496

Browse files
sarg3ntclaude
andcommitted
fix(errors): address Copilot review on WriteHTTPError JSON envelope (#112)
Copilot flagged three callers that would render the new {"success":false,"message":...} envelope opaquely: 1. services.templ service-control: did `await response.text()` and toasted the raw blob. 2. certificates.templ downloadCertificate: same. 3. security.templ blockIP / unblockIP: same. Also flagged the 2xx-non-JSON fallback I added in logs.templ and services.templ as too permissive — silently rendering a non-JSON 2xx response would hide an auth-redirect HTML page or proxy error. Fixes: * security.templ now defines an `extractErrorMessage(response)` helper that parses the JSON envelope when content-type advertises JSON, falling back to text otherwise. blockIP / unblockIP route through it. * services.templ service-control inlines the same JSON-first pattern. * certificates.templ downloadCertificate inlines the same pattern. * logs.templ + services.templ success-path: a 2xx with a non-JSON body now throws a "non-JSON response" error instead of being silently rendered as log lines / empty service list. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 409334d commit 07ab496

4 files changed

Lines changed: 69 additions & 11 deletions

File tree

gearbox/internal/framework/templates/pages/certificates.templ

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -470,8 +470,23 @@ templ Certificates(user *models.User, servers []models.BoxConfig, perms *models.
470470
const response = await fetch(`/api/${serverID}/certificates/${encodeURIComponent(domain)}/download`);
471471

472472
if (!response.ok) {
473-
const text = await response.text();
474-
throw new Error(text || `HTTP ${response.status}`);
473+
// errors.WriteHTTPError now returns a JSON envelope; older
474+
// http.Error paths still return text/plain. Try JSON first
475+
// so the toast surfaces the structured .message field
476+
// rather than the raw '{"success":false,...}' blob.
477+
const ct = response.headers.get('content-type') || '';
478+
let msg;
479+
if (ct.includes('application/json')) {
480+
try {
481+
const body = await response.json();
482+
msg = (body && (body.message || body.error)) || JSON.stringify(body);
483+
} catch (_) {
484+
msg = await response.text();
485+
}
486+
} else {
487+
msg = await response.text();
488+
}
489+
throw new Error(msg || `HTTP ${response.status}`);
475490
}
476491

477492
// Get filename from Content-Disposition header or use default

gearbox/internal/framework/templates/pages/logs.templ

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -494,6 +494,9 @@ templ Logs(user *models.User, servers []models.BoxConfig) {
494494
// error (see errors.WriteHTTPError) — try to parse it and
495495
// surface .message on error instead of letting JSON.parse
496496
// failures bubble up as confusing "Unexpected token" toasts.
497+
// A 2xx response with a non-JSON body is a protocol error
498+
// (e.g. an auth-redirect HTML page leaking through) and
499+
// must surface as such, not be rendered as log lines.
497500
const contentType = response.headers.get('content-type') || '';
498501
const isJSON = contentType.includes('application/json');
499502
if (!response.ok) {
@@ -508,7 +511,10 @@ templ Logs(user *models.User, servers []models.BoxConfig) {
508511
}
509512
throw new Error(msg);
510513
}
511-
return isJSON ? response.json() : { logs: await response.text() };
514+
if (!isJSON) {
515+
throw new Error(`Unexpected non-JSON response from logs API (content-type=${contentType})`);
516+
}
517+
return response.json();
512518
})
513519
.then(data => {
514520
rawLogContent = data.logs || 'No logs available';

gearbox/internal/framework/templates/pages/security.templ

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,24 @@ templ SecurityPage(user *models.User, servers []models.BoxConfig, perms *models.
200200
let blockedIPsData = [];
201201
const canAction = { if perms != nil && perms.HasPermission(models.ComponentSecurity, models.PermissionAction) { "true" } else { "false" } } === 'true';
202202

203+
// extractErrorMessage normalizes the body of a non-2xx fetch response
204+
// into a user-facing string. errors.WriteHTTPError now returns a JSON
205+
// envelope `{success:false, message:...}`; older `http.Error` paths
206+
// still return text/plain. Try JSON first so the toast surfaces
207+
// .message instead of the raw envelope, then fall back to text.
208+
async function extractErrorMessage(response) {
209+
const ct = response.headers.get('content-type') || '';
210+
if (ct.includes('application/json')) {
211+
try {
212+
const body = await response.json();
213+
return (body && (body.message || body.error)) || JSON.stringify(body);
214+
} catch (_) {
215+
return await response.text();
216+
}
217+
}
218+
return await response.text();
219+
}
220+
203221
document.addEventListener('DOMContentLoaded', function() {
204222
const serverInput = document.getElementById('default-server-id');
205223
if (serverInput) {
@@ -363,8 +381,7 @@ templ SecurityPage(user *models.User, servers []models.BoxConfig, perms *models.
363381
});
364382

365383
if (!response.ok) {
366-
const error = await response.text();
367-
throw new Error(error);
384+
throw new Error(await extractErrorMessage(response));
368385
}
369386

370387
// Clear form and refresh
@@ -391,8 +408,7 @@ templ SecurityPage(user *models.User, servers []models.BoxConfig, perms *models.
391408
});
392409

393410
if (!response.ok) {
394-
const error = await response.text();
395-
throw new Error(error);
411+
throw new Error(await extractErrorMessage(response));
396412
}
397413

398414
await loadBlockedIPs();

gearbox/internal/framework/templates/pages/services.templ

Lines changed: 25 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -217,7 +217,10 @@ templ Services(user *models.User, servers []models.BoxConfig) {
217217
// Backend returns a JSON envelope for both success and
218218
// error (see errors.WriteHTTPError); surface .message on
219219
// error so the UI doesn't show raw 'Unexpected token' from
220-
// JSON.parse on plain-text bodies.
220+
// JSON.parse on plain-text bodies. A 2xx response with a
221+
// non-JSON body (e.g. an auth-redirect HTML page leaking
222+
// through) is a protocol error and must surface as such,
223+
// not be silently swallowed as an empty list.
221224
const contentType = response.headers.get('content-type') || '';
222225
const isJSON = contentType.includes('application/json');
223226
if (!response.ok) {
@@ -232,7 +235,10 @@ templ Services(user *models.User, servers []models.BoxConfig) {
232235
}
233236
throw new Error(msg);
234237
}
235-
return isJSON ? response.json() : { services: [] };
238+
if (!isJSON) {
239+
throw new Error('Unexpected non-JSON response from services API (content-type=' + contentType + ')');
240+
}
241+
return response.json();
236242
})
237243
.then(function(data) {
238244
if (data.error) {
@@ -520,8 +526,23 @@ templ Services(user *models.User, servers []models.BoxConfig) {
520526
});
521527

522528
if (!response.ok) {
523-
const text = await response.text();
524-
throw new Error(text || 'Failed to ' + action + ' service');
529+
// errors.WriteHTTPError now returns a JSON envelope; older
530+
// http.Error paths still return text/plain. Try JSON first
531+
// so the toast surfaces the structured .message field
532+
// rather than the raw '{"success":false,...}' blob.
533+
const ct = response.headers.get('content-type') || '';
534+
let msg;
535+
if (ct.includes('application/json')) {
536+
try {
537+
const body = await response.json();
538+
msg = (body && (body.message || body.error)) || JSON.stringify(body);
539+
} catch (_) {
540+
msg = await response.text();
541+
}
542+
} else {
543+
msg = await response.text();
544+
}
545+
throw new Error(msg || 'Failed to ' + action + ' service');
525546
}
526547

527548
const data = await response.json();

0 commit comments

Comments
 (0)