From 6813f02b34666768f0141973244a3db539c17127 Mon Sep 17 00:00:00 2001 From: Rolando Santamaria Maso Date: Sun, 26 Jul 2026 12:25:47 +0200 Subject: [PATCH] fix(security): resolve open code-scanning alerts (XSS, int-conversion, cookie, CI perms) 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 & 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. --- .github/workflows/release.yml | 3 ++ .github/workflows/test.yml | 3 ++ cmd/odek/browser_tool.go | 9 +++- cmd/odek/browser_tool_test.go | 34 +++++++++++++++ cmd/odek/serve.go | 7 ++++ cmd/odek/ui/app.js | 51 ++++++++++++++++------- internal/danger/classifier.go | 15 +++++++ internal/danger/whitebox_coverage_test.go | 6 +++ internal/skills/importer.go | 13 ++++++ internal/skills/importer_test.go | 5 +++ 10 files changed, 131 insertions(+), 15 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index cff18d55..d3d02456 100644 --- 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 index 4f6dc163..ade483bc 100644 --- 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 index 84743572..3334836f 100644 --- 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 index 25063b64..5a056f4e 100644 --- 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(` + js + js-caps + data + vbs + safe link + `)) + })) + 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 index 3b2237d7..b23d3e2c 100644 --- 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 index c2bd3218..8dbfe91a 100644 --- 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('⚡ ' + (event.latency < 1 ? (event.latency * 1000).toFixed(0) + 'ms' : event.latency.toFixed(1) + 's') + ''); + spans.push('⚡ ' + (latSafe < 1 ? (latSafe * 1000).toFixed(0) + 'ms' : latSafe.toFixed(1) + 's') + ''); if (event.contextTokens != null) spans.push('' + formatNum(event.contextTokens) + ' in'); if (event.outputTokens != null) spans.push('' + formatNum(event.outputTokens) + ' out'); // Cache metrics — show only when non-zero @@ -1268,7 +1274,9 @@ function escapeHtml(s) { function escapeAttr(s) { if (!s) return ''; - return s.replace(/"/g,'"').replace(/'/g,''').replace(/&/g,'&'); + // & must be replaced first — doing it last double-escapes the entities + // introduced by the quote replacements (" → &quot;). + return s.replace(/&/g,'&').replace(/"/g,'"').replace(/'/g,'''); } // ── 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) => - '' + - '📎' + - '' + escapeHtml(f.name) + '' + - '' + formatFileSize(f.size) + '' + - '✕' + - '' - ).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 index a5e08d34..5b61bac0 100644 --- 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 index f05063d2..eacc3dee 100644 --- 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 index b6bee300..8b86de07 100644 --- 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 index 3eb379d8..42670126 100644 --- 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},