From 74bd3c2e987bf53d9f97fe2c3b1166c41bd3da58 Mon Sep 17 00:00:00 2001 From: MarcoPad Dev Date: Fri, 17 Jul 2026 18:12:23 -0700 Subject: [PATCH] security(web): remove creds from image URLs, auth-fail state, tighten CSP Minor PR-review follow-ups for the local PWA: - Macro images load via a header-authenticated fetch -> blob object URL in both token and relay modes, so no credential ever sits in an image URL; prior object URLs are revoked on re-render. - Local-mode auth failure now shows a clear "Access token required" state and stops the reconnect/toast loop (one-shot), instead of retrying forever. - CSP connect-src tightened from 'self' ws: wss: to 'self' (same-origin WS is covered); verified the live WebSocket still connects. Co-Authored-By: Claude Opus 4.8 (1M context) --- web/index.html | 2 +- web/js/app.js | 87 ++++++++++++++++++++++++++++++++++++++++++++------ 2 files changed, 79 insertions(+), 10 deletions(-) diff --git a/web/index.html b/web/index.html index 5d087f8..da17cdd 100644 --- a/web/index.html +++ b/web/index.html @@ -3,7 +3,7 @@ - + diff --git a/web/js/app.js b/web/js/app.js index 7857565..6eb12b7 100644 --- a/web/js/app.js +++ b/web/js/app.js @@ -34,6 +34,13 @@ class MacroPadApp { // Guards against out-of-order macro fetches this._macroReqId = 0; + // Blob object URLs created for macro images (revoked on re-render) + this._objectUrls = []; + + // Set once a local-mode auth failure has been surfaced, to avoid + // reconnect loops and repeated toasts. + this.localAuthFailed = false; + // Macro editor state (null = create mode) this.editingMacroId = null; @@ -204,12 +211,44 @@ class MacroPadApp { } handleAuthError() { - this.showToast('Authentication failed', 'error'); if (this.relayMode) { + this.showToast('Authentication failed', 'error'); // Clear stored password and redirect to login sessionStorage.removeItem(`macropad_${this.sessionId}`); window.location.href = `/${this.sessionId}`; + return; } + + // Local (token) mode: a wrong/missing token makes every request 401 and + // the WebSocket close with 1008, which would otherwise reconnect forever + // and spam toasts. Surface a single, clear message and stop retrying. + if (this.localAuthFailed) return; + this.localAuthFailed = true; + + this.shouldReconnect = false; + clearTimeout(this.reconnectTimer); + this.teardownSocket(); + this.updateConnectionStatus(false, 'Access denied'); + this.renderAuthRequired(); + } + + renderAuthRequired() { + const container = document.getElementById('macro-grid'); + if (!container) return; + container.textContent = ''; + + const wrap = document.createElement('div'); + wrap.className = 'empty-state'; + + const p1 = document.createElement('p'); + p1.textContent = 'Access token required'; + + const p2 = document.createElement('p'); + p2.className = 'hint'; + p2.textContent = "Open the link from the desktop app's URL or QR code."; + + wrap.append(p1, p2); + container.appendChild(wrap); } handleDesktopDisconnected() { @@ -396,15 +435,38 @@ class MacroPadApp { macroImageUrl(macro) { if (!macro.image_path) return null; - const basePath = this.getApiUrl(`/api/image/${macro.image_path}`); - // Credentials go as query params since can't set headers. + // No credential ever goes on the URL; auth is via a header on a + // fetch() below, and the image is shown as a blob object URL. + return this.getApiUrl(`/api/image/${macro.image_path}`); + } + + // Auth-only headers (no Content-Type) for GET fetches like images. + getAuthHeaders() { + const headers = {}; if (this.relayMode && this.password) { - return `${basePath}?password=${encodeURIComponent(this.password)}`; + headers['X-MacroPad-Password'] = this.password; + } else if (!this.relayMode && this.localToken) { + headers['X-MacroPad-Token'] = this.localToken; } - if (!this.relayMode && this.localToken) { - return `${basePath}?token=${encodeURIComponent(this.localToken)}`; + return headers; + } + + // Fetch a macro image with the auth header and display it as a blob + // object URL, so no credential is ever placed on an src. + async loadMacroImage(url, img, placeholder) { + try { + const res = await fetch(url, { headers: this.getAuthHeaders() }); + if (!res.ok) throw new Error('image request failed'); + const blob = await res.blob(); + const objectUrl = URL.createObjectURL(blob); + // Track for best-effort revocation on the next render. + this._objectUrls.push(objectUrl); + img.src = objectUrl; + } catch (e) { + // Fall back to the placeholder if the image can't be loaded. + img.style.display = 'none'; + placeholder.style.display = ''; } - return basePath; } // Rendering (safe DOM construction only - no user value reaches innerHTML) @@ -430,6 +492,12 @@ class MacroPadApp { const container = document.getElementById('macro-grid'); if (!container) return; + // Best-effort: revoke object URLs from the previous render to avoid leaks. + if (this._objectUrls && this._objectUrls.length) { + this._objectUrls.forEach((u) => URL.revokeObjectURL(u)); + } + this._objectUrls = []; + container.textContent = ''; const macroEntries = Object.entries(this.macros); @@ -464,14 +532,15 @@ class MacroPadApp { const img = document.createElement('img'); img.className = 'macro-image'; img.alt = ''; - img.src = imageSrc; placeholder.style.display = 'none'; - // Fall back to the placeholder if the image fails to load. + // Fall back to the placeholder if the (blob) image fails to load. img.addEventListener('error', () => { img.style.display = 'none'; placeholder.style.display = ''; }); card.appendChild(img); + // Header-authenticated fetch -> blob object URL (no credential on src). + this.loadMacroImage(imageSrc, img, placeholder); } card.appendChild(placeholder);