fix(#87): address Copilot review on metrics insights

Three findings, all valid:

1. The prev-window stats slice was un-trimmed. GetStatsHistory takes
   a lower bound only, so prevRaw spanned both windows; the `prevOnly`
   index list I computed was promptly discarded with `_ = prevOnly`,
   so every prev-window aggregate was actually a union aggregate and
   the resulting delta_pct was ~0 % regardless of actual change.
   Fixed by collapsing prevRaw to entries strictly before `since`.

2. APIMetricsLogErrorsHandler returned HTTP 403 when the user lacked
   `logs:view`, but the README and the frontend's loadLogErrors() both
   expected the structured {available:false, reason:…} envelope, so
   users without log access saw the generic "Failed to load log lines"
   error instead of the documented hint. Switched to the envelope.

3. escapeJSArg only handled the JS string context (\\ and '), but the
   value is interpolated into a double-quoted HTML onclick attribute,
   so an embedded " could break out and a < could start a new tag.
   Hardened to also escape &<>" — defence in depth on the source-IP
   path where values flow from agent-collected data.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Author: sarg3nt

gearbox/internal/framework/handler/api_metrics_insights.go

@@ -85,24 +85,27 @@ func (h *Handler) APIMetricsSummaryHandler(w http.ResponseWriter, r *http.Reques
 		http.Error(w, "Failed to load stats history", http.StatusInternalServerError)
 		return
 	}
-	prev, err := h.db.GetStatsHistory(boxID, prevSince, 5000)
+	prevRaw, err := h.db.GetStatsHistory(boxID, prevSince, 5000)
 	if err != nil {
 		h.logger.Error("metrics summary: prev stats history", "error", err)
 		http.Error(w, "Failed to load stats history", http.StatusInternalServerError)
 		return
 	}
-	// prev contains both windows; trim to "previous-window only".
-	prevOnly := make([]int, 0, len(prev))
-	for i, s := range prev {
+	// GetStatsHistory takes a lower bound only, so prevRaw spans both the
+	// previous AND current windows. Trim to entries strictly before `since`
+	// so the prev-window aggregates (max counter, avg response time, …)
+	// reflect the prior window in isolation — otherwise every delta vs.
+	// prior is ~0% because we'd be comparing each window to its own union
+	// with the current window.
+	prev := prevRaw[:0]
+	for _, s := range prevRaw {
 		if s.CollectedAt.Before(since) {
-			prevOnly = append(prevOnly, i)
+			prev = append(prev, s)
 		}
 	}
 
 	cards := []kpiCard{}
 
-	_ = prevOnly
-
 	currReqMax := maxStatField(curr, func(s database.StatsSnapshot) float64 { return float64(s.TotalRequests) })
 	prevReqMax := maxStatField(prev, func(s database.StatsSnapshot) float64 { return float64(s.TotalRequests) })
 	currReqRate := perMinute(currReqMax, since, until)
@@ -434,8 +437,18 @@ func (h *Handler) APIMetricsLogErrorsHandler(w http.ResponseWriter, r *http.Requ
 		http.Error(w, "Forbidden: insufficient permissions to view metrics", http.StatusForbidden)
 		return
 	}
+	// Lack of `logs:view` is the only "you're allowed on the metrics page,
+	// but this one sub-panel needs more permission" case we have. Return a
+	// soft envelope rather than 403 so the drawer can render the documented
+	// "logs unavailable" hint instead of the generic "fetch failed" error.
 	if !h.authManager.HasPermission(r, "logs", "view") {
-		http.Error(w, "Forbidden: log access required for log correlation", http.StatusForbidden)
+		h.writeJSON(w, map[string]interface{}{
+			"server_id":  chi.URLParam(r, "boxID"),
+			"available":  false,
+			"reason":     "Logs permission required to correlate access-log lines with metrics.",
+			"matchCount": 0,
+			"matches":    []haproxyLogLine{},
+		})
 		return
 	}
 

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

@@ -1866,9 +1866,22 @@ templ History(user *models.User, servers []models.BoxConfig) {
 		}
 
 		// escapeJSArg escapes a string for safe inclusion as a single-quoted
-		// JS argument inside an HTML onclick attribute.
+		// JS argument inside a double-quoted HTML attribute, e.g.
+		//   <div onclick="openDrillDown('source', 'VALUE')">
+		// It must therefore neutralise BOTH the JS string context (\\ and ')
+		// AND the HTML attribute context (", <, >, & — otherwise a value
+		// containing `"` would break out of the attribute and a `<` could
+		// start a new tag). Backend names come from operator-controlled
+		// HAProxy config, but source IPs and country codes flow from
+		// agent-collected data, so defence-in-depth is warranted.
 		function escapeJSArg(s) {
-			return String(s).replace(/\\/g, '\\\\').replace(/'/g, "\\'");
+			return String(s)
+				.replace(/\\/g, '\\\\')
+				.replace(/'/g, "\\'")
+				.replace(/&/g, '&amp;')
+				.replace(/"/g, '&quot;')
+				.replace(/</g, '&lt;')
+				.replace(/>/g, '&gt;');
 		}
 
 		// Move page header content to main header