fix(security): resolve open code-scanning alerts (XSS, int-conversion, cookie, CI perms) (#108)

jkyberneees · web-flow · commit 3218d72c2e2f · 2026-07-26T12:30:59.000+02:00
Addresses all 12 open alerts from GitHub code scanning:

High:
- go/incorrect-integer-conversion (classifier.go, skills/importer.go):
  both inet_aton-style IP parsers now reject out-of-range parts instead of
  silently truncating (300.1.1.1 no longer parses as 44.1.1.1; 10.300.1.1
  no longer slips into the private range as 10.44.1.1), matching browser
  semantics and protecting the SSRF/internal-IP checks downstream.
- go/incomplete-url-scheme-check (browser_tool.go): link extraction now
  filters javascript:, data:, and vbscript: case-insensitively.
- js/xss (ui/app.js): formatNum coerces to a finite number and the latency
  stat is coerced, so crafted non-numeric values can never reach innerHTML
  as markup.
- js/xss-through-dom (ui/app.js): renderFileChips builds DOM nodes with
  textContent instead of innerHTML.
- js/double-escaping (ui/app.js): escapeAttr replaces &amp; first so quote
  entities are no longer double-escaped.

Medium:
- go/cookie-secure-not-set (serve.go): the WS-token cookie sets
  Secure: true. The UI always also authenticates via WebSocket subprotocol
  and /api header, so browsers that drop the cookie lose nothing.
- actions/missing-workflow-permissions: top-level
  permissions: contents: read in test.yml and release.yml (the release job
  keeps its explicit contents: write).

Regression tests: IP bound cases in both packages, browser scheme-filter
test. Full test suite, vet, golangci-lint, and node --check all pass.
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
@@ -4,6 +4,9 @@ on:
   push:
     tags: ["v*"]
 
+permissions:
+  contents: read
+
 jobs:
   build:
     runs-on: ubuntu-latest
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -6,6 +6,9 @@ on:
   pull_request:
     branches: [main]
 
+permissions:
+  contents: read
+
 jobs:
   test:
     runs-on: ubuntu-latest
diff --git a/cmd/odek/browser_tool.go b/cmd/odek/browser_tool.go
@@ -408,7 +408,14 @@ func parseHTML(ctx context.Context, html, pageURL string, status int) browserSna
 		}
 		href := strings.TrimSpace(m[1])
 		text := strings.TrimSpace(m[2])
-		if href == "" || text == "" || href == "#" || strings.HasPrefix(href, "javascript:") {
+		// Skip non-navigable schemes. The check is case-insensitive and
+		// covers javascript:, data:, and vbscript: — all of which can carry
+		// script/active content the agent must never be sent into.
+		lowerHref := strings.ToLower(href)
+		if href == "" || text == "" || href == "#" ||
+			strings.HasPrefix(lowerHref, "javascript:") ||
+			strings.HasPrefix(lowerHref, "data:") ||
+			strings.HasPrefix(lowerHref, "vbscript:") {
 			continue
 		}
 		// Skip duplicates
diff --git a/cmd/odek/browser_tool_test.go b/cmd/odek/browser_tool_test.go
@@ -596,3 +596,37 @@ func TestBrowser_WrapsLinkURL(t *testing.T) {
 		t.Errorf("link URL should be wrapped as untrusted, got %q", r.Elements[0].URL)
 	}
 }
+
+// TestBrowser_SkipsDangerousLinkSchemes pins the incomplete-scheme-check fix:
+// links with script/active-content schemes (javascript:, data:, vbscript:,
+// any casing) are excluded from the extracted interactive elements.
+func TestBrowser_SkipsDangerousLinkSchemes(t *testing.T) {
+	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		w.Write([]byte(`<html><body>
+			<a href="javascript:alert(1)">js</a>
+			<a href="JavaScript:alert(1)">js-caps</a>
+			<a href="data:text/html,<script>alert(1)</script>">data</a>
+			<a href="vbscript:msgbox(1)">vbs</a>
+			<a href="/safe">safe link</a>
+		</body></html>`))
+	}))
+	defer ts.Close()
+
+	b := newTestBrowserTool()
+	callJSON(t, b, `{"action":"navigate","url":"`+ts.URL+`"}`)
+
+	result := callJSON(t, b, `{"action":"snapshot"}`)
+	var r struct {
+		Content string `json:"content"`
+	}
+	mustUnmarshal(t, result, &r)
+
+	for _, bad := range []string{"javascript:", "JavaScript:", "data:", "vbscript:"} {
+		if strings.Contains(r.Content, bad) {
+			t.Errorf("snapshot contains %q link, want it filtered: %q", bad, r.Content)
+		}
+	}
+	if !strings.Contains(r.Content, "/safe") {
+		t.Errorf("snapshot missing the safe link: %q", r.Content)
+	}
+}
diff --git a/cmd/odek/serve.go b/cmd/odek/serve.go
@@ -1783,6 +1783,13 @@ func handleStatic(wsToken string) http.HandlerFunc {
 					Path:     "/",
 					SameSite: http.SameSiteStrictMode,
 					HttpOnly: true,
+					// Secure is set even though the server usually runs on
+					// plain-http loopback: modern browsers treat localhost as a
+					// potentially trustworthy origin and accept Secure cookies
+					// there, and the UI always sends the token as a WebSocket
+					// subprotocol (and /api header), so a browser that drops
+					// the cookie loses nothing.
+					Secure: true,
 					// No explicit MaxAge/Expires so the cookie is a session cookie.
 				})
 				data = []byte(strings.Replace(string(data), "{{ODEK_WS_TOKEN}}", wsToken, 1))
diff --git a/cmd/odek/ui/app.js b/cmd/odek/ui/app.js
@@ -165,6 +165,10 @@ function formatErrorMessage(msg) {
 
 // ── Number formatting ──
 function formatNum(n) {
+  // Coerce to a finite number so a crafted (non-numeric) value can never be
+  // reinterpreted as HTML when the result lands in an innerHTML assignment.
+  n = Number(n);
+  if (!isFinite(n)) n = 0;
   if (n >= 1000) return (n / 1000).toFixed(n >= 10000 ? 0 : 1) + 'k';
   return String(n);
 }
@@ -305,8 +309,10 @@ function connect() {
           if (lastAssistant) {
             const stats = document.createElement('div');
             stats.className = 'msg-stats';
+            const lat = Number(event.latency);
+            const latSafe = isFinite(lat) ? lat : 0;
             const spans = [];
-            spans.push('<span title="Response time">⚡ ' + (event.latency < 1 ? (event.latency * 1000).toFixed(0) + 'ms' : event.latency.toFixed(1) + 's') + '</span>');
+            spans.push('<span title="Response time">⚡ ' + (latSafe < 1 ? (latSafe * 1000).toFixed(0) + 'ms' : latSafe.toFixed(1) + 's') + '</span>');
             if (event.contextTokens != null) spans.push('<span title="Input tokens (prompt)">' + formatNum(event.contextTokens) + ' in</span>');
             if (event.outputTokens != null) spans.push('<span title="Output tokens (completion)">' + formatNum(event.outputTokens) + ' out</span>');
             // Cache metrics — show only when non-zero
@@ -1268,7 +1274,9 @@ function escapeHtml(s) {
 
 function escapeAttr(s) {
   if (!s) return '';
-  return s.replace(/"/g,'&quot;').replace(/'/g,'&#39;').replace(/&/g,'&amp;');
+  // & must be replaced first — doing it last double-escapes the entities
+  // introduced by the quote replacements (&quot; → &amp;quot;).
+  return s.replace(/&/g,'&amp;').replace(/"/g,'&quot;').replace(/'/g,'&#39;');
 }
 
 // ── Markdown to HTML (safe, no DOMPurify needed since we control input) ──
@@ -1460,18 +1468,33 @@ function clearAttachedFiles() {
 }
 
 function renderFileChips() {
-  if (attachedFiles.length === 0) {
-    fileChips.innerHTML = '';
-    return;
-  }
-  fileChips.innerHTML = attachedFiles.map((f, i) =>
-    '<span class="file-chip">' +
-      '<span class="chip-icon">📎</span>' +
-      '<span class="chip-name">' + escapeHtml(f.name) + '</span>' +
-      '<span class="chip-size">' + formatFileSize(f.size) + '</span>' +
-      '<span class="chip-remove" onclick="removeAttachedFile(' + i + ')">✕</span>' +
-    '</span>'
-  ).join('');
+  // Build nodes with textContent rather than innerHTML so a file name can
+  // never be reinterpreted as markup.
+  fileChips.textContent = '';
+  attachedFiles.forEach((f, i) => {
+    const chip = document.createElement('span');
+    chip.className = 'file-chip';
+
+    const icon = document.createElement('span');
+    icon.className = 'chip-icon';
+    icon.textContent = '📎';
+
+    const name = document.createElement('span');
+    name.className = 'chip-name';
+    name.textContent = f.name;
+
+    const size = document.createElement('span');
+    size.className = 'chip-size';
+    size.textContent = formatFileSize(f.size);
+
+    const remove = document.createElement('span');
+    remove.className = 'chip-remove';
+    remove.textContent = '✕';
+    remove.addEventListener('click', () => removeAttachedFile(i));
+
+    chip.append(icon, name, size, remove);
+    fileChips.appendChild(chip);
+  });
 }
 
 function readFileAsText(file) {
diff --git a/internal/danger/classifier.go b/internal/danger/classifier.go
@@ -463,6 +463,21 @@ func parseBrowserIP(host string) net.IP {
 		nums = append(nums, uint32(val))
 	}
 
+	// inet_aton requires every leading part to fit in one byte and the final
+	// part to fit in the remaining bytes (a.b → b ≤ 0xFFFFFF, a.b.c →
+	// c ≤ 0xFFFF, a.b.c.d → d ≤ 0xFF). Reject out-of-range parts instead of
+	// silently truncating them — "300.1.1.1" must not parse as 44.1.1.1 and
+	// mislead the internal-IP checks downstream.
+	for i, n := range nums {
+		max := uint64(0xFF)
+		if i == len(nums)-1 {
+			max = (uint64(1) << (8 * uint(5-len(nums)))) - 1
+		}
+		if uint64(n) > max {
+			return nil
+		}
+	}
+
 	switch len(nums) {
 	case 1:
 		// Single number: full 32-bit address
diff --git a/internal/danger/whitebox_coverage_test.go b/internal/danger/whitebox_coverage_test.go
@@ -314,6 +314,12 @@ func TestParseBrowserIP(t *testing.T) {
 		{"::1", false, "::1"},              // IPv6
 		{"1.2.3.4.5", true, ""},            // too many parts
 		{"99999999999.1", true, ""},        // part exceeds 32 bits → nil
+		{"300.1.1.1", true, ""},            // leading part > 0xFF → nil, not 44.1.1.1
+		{"256.1", true, ""},                // leading part > 0xFF in short form
+		{"1.16777216", true, ""},           // final part > 0xFFFFFF in a.b form
+		{"1.2.65536", true, ""},            // final part > 0xFFFF in a.b.c form
+		{"1.2.3.256", true, ""},            // final part > 0xFF in a.b.c.d form
+		{"255.255.255.255", false, "255.255.255.255"}, // boundary: all parts at max
 		{"0xZZ.0.0.1", true, ""},           // bad hex
 		{"not.an.ip.addr", true, ""},       // non-numeric
 		{"", true, ""},                     // empty
diff --git a/internal/skills/importer.go b/internal/skills/importer.go
@@ -412,6 +412,19 @@ func parsePrivateIP(host string) net.IP {
 		}
 		nums = append(nums, uint32(val))
 	}
+	// inet_aton requires every leading part to fit in one byte and the final
+	// part to fit in the remaining bytes (a.b → b ≤ 0xFFFFFF, a.b.c →
+	// c ≤ 0xFFFF, a.b.c.d → d ≤ 0xFF). Reject out-of-range parts instead of
+	// silently truncating them (e.g. "300.1.1.1" → 44.1.1.1).
+	for i, n := range nums {
+		max := uint64(0xFF)
+		if i == len(nums)-1 {
+			max = (uint64(1) << (8 * uint(5-len(nums)))) - 1
+		}
+		if uint64(n) > max {
+			return nil
+		}
+	}
 	switch len(nums) {
 	case 1:
 		return net.IPv4(byte(nums[0]>>24), byte(nums[0]>>16), byte(nums[0]>>8), byte(nums[0]))
diff --git a/internal/skills/importer_test.go b/internal/skills/importer_test.go
@@ -424,6 +424,11 @@ func TestIsPrivateHost(t *testing.T) {
 		{"0x7f000001", true},      // hex 127.0.0.1
 		{"127.1", true},           // short form 127.0.0.1
 		{"0x0.0x0.0x0.0x0", true}, // hex-dotted
+		// Out-of-range parts are rejected (browser semantics: invalid IP →
+		// hostname), not silently truncated — 10.300.1.1 must not parse as
+		// 10.44.1.1 and slip into the private range.
+		{"10.300.1.1", false},
+		{"300.1.1.1", false},
 		// RFC 6598 carrier-grade NAT
 		{"100.64.0.1", true},
 		{"100.127.255.254", true},
