From c105a5941ea1fdb9ef76b518e40e2b85cb215e40 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Tue, 28 Apr 2026 14:37:18 -0500 Subject: [PATCH] Security: fix IDORs, XSS, rate limits, SSRF validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit HIGH 1 (teams IDOR): POST/DELETE /api/teams/:id/devices now require the caller to own the device before assigning or detaching it. Without this check, any team member could pull any device into their team via UUID guess and gain remote-control access. HIGH 2 (schedules IDOR): PUT /api/schedules/:id now re-verifies ownership of every changed target field — device_id, group_id, content_id, widget_id, layout_id, playlist_id. Previously only the schedule owner was checked, letting users fire arbitrary content on victim devices via update. HIGH 3 (filename XSS): file.originalname captured by multer bypassed sanitizeBody. New safeFilename() wraps every INSERT path (multipart upload, remote URL, YouTube). Frontend sinks now go through esc() in content-library.js, device-detail.js, video-wall.js. Web player gets an inline escHtml helper for its info overlay where filenames, device name, and serverUrl land in innerHTML. HIGH 4 (kiosk public XSS): config.idleTimeout is now coerced via the existing safeNumber() helper at both interpolation sites. A crafted value with a newline can no longer escape the JS line comment to inject arbitrary code into the public render endpoint. HIGH 5 (folder DoS): POST /api/folders enforces a per-user cap of 100 folders (429 on overflow). Superadmin exempt. MED 1 (SSRF): ImageLoader.decodeUrl rejects any URL scheme other than http(s) so a malicious remote_url can't read local files via file://. On the server, validateRemoteUrl() is extracted and now also runs on PUT /api/content/:id remote_url updates — previously the SSRF check only fired on POST. MED 2 (fingerprint takeover): the WS device:register fingerprint reclaim path now rejects takeover while the target device is online or within 24h of its last heartbeat. A leaked fingerprint can no longer hijack an active display. MED 3 (npm audit): bumped uuid 9.x -> 14.0.0 (v3/v5/v6 buffer bounds CVE; we only use v4 so not exploitable, but clears the audit). path- to-regexp resolved to 0.1.13 via npm audit fix. 0 vulns remaining. MED 4 (folder admin consistency): ownedFolder() and the content.js folder_id move check now both treat only superadmin as privileged, matching GET /api/folders. Previously a plain "admin" could rename or delete folders they couldn't see, and could move content into folders they couldn't list. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../remotedisplay/player/util/ImageLoader.kt | 8 ++ frontend/js/views/content-library.js | 20 ++--- frontend/js/views/device-detail.js | 6 +- frontend/js/views/video-wall.js | 3 +- server/package-lock.json | 16 ++-- server/package.json | 2 +- server/player/index.html | 14 ++-- server/routes/content.js | 81 ++++++++++++------- server/routes/folders.js | 24 +++++- server/routes/kiosk.js | 4 +- server/routes/schedules.js | 32 ++++++-- server/routes/teams.js | 21 ++++- server/ws/deviceSocket.js | 16 ++++ 13 files changed, 175 insertions(+), 72 deletions(-) diff --git a/android/app/src/main/java/com/remotedisplay/player/util/ImageLoader.kt b/android/app/src/main/java/com/remotedisplay/player/util/ImageLoader.kt index c607496..66b3733 100644 --- a/android/app/src/main/java/com/remotedisplay/player/util/ImageLoader.kt +++ b/android/app/src/main/java/com/remotedisplay/player/util/ImageLoader.kt @@ -44,6 +44,14 @@ object ImageLoader { } fun decodeUrl(url: String, maxW: Int, maxH: Int): Bitmap? { + // Reject anything that isn't HTTP/HTTPS. URL.openConnection() otherwise + // happily handles file://, jar:, ftp:, etc. — which would let a server-supplied + // remote_url read local files off the device or talk to internal services. + val scheme = try { URL(url).protocol?.lowercase() } catch (_: Throwable) { null } + if (scheme != "http" && scheme != "https") { + Log.w(TAG, "Rejecting non-http(s) URL scheme: $scheme") + return null + } return try { val bytes = URL(url).openConnection().apply { connectTimeout = 10_000 diff --git a/frontend/js/views/content-library.js b/frontend/js/views/content-library.js index 8f51d1c..2f22bd5 100644 --- a/frontend/js/views/content-library.js +++ b/frontend/js/views/content-library.js @@ -322,7 +322,7 @@ async function loadContent() {
${c.mime_type === 'video/youtube' ? `
- ${c.filename} + ${esc(c.filename)}
@@ -339,18 +339,18 @@ async function loadContent() { Remote
` : c.thumbnail_path - ? `${c.filename}` + ? `${esc(c.filename)}` : c.mime_type?.startsWith('video/') ? `
` - : `${c.filename}` + : `${esc(c.filename)}` }
-
${c.filename}
+
${esc(c.filename)}
${c.mime_type === 'video/youtube' ? 'YouTube' : c.remote_url ? 'Remote URL' : (c.mime_type?.startsWith('video/') ? 'Video' : 'Image')} ${c.duration_sec ? ` · ${Math.floor(c.duration_sec / 60)}:${String(Math.floor(c.duration_sec % 60)).padStart(2, '0')}` : ''} @@ -469,12 +469,12 @@ function showEditModal(contentItem, onSave) {
- +
${isRemote ? `
- +
` : ''}
@@ -576,13 +576,13 @@ function showPreview(content) { ${isYoutube ? `` : isVideo - ? `` - : `` + ? `` + : `` }
-
${content.filename}
-
${content.mime_type} ${content.remote_url ? '(Remote URL)' : ''}
+
${esc(content.filename)}
+
${esc(content.mime_type)} ${content.remote_url ? '(Remote URL)' : ''}
`; diff --git a/frontend/js/views/device-detail.js b/frontend/js/views/device-detail.js index 32cf055..cd0305c 100644 --- a/frontend/js/views/device-detail.js +++ b/frontend/js/views/device-detail.js @@ -319,7 +319,7 @@ async function loadDevice(deviceId, activeTab = null) {
- +
@@ -493,7 +493,7 @@ function renderPlaylist(assignments) {
` }
-
${a.filename || a.widget_name || 'Unknown'}
+
${esc(a.filename || a.widget_name || 'Unknown')}
${a.widget_id && !a.content_id ? `Widget (${a.widget_type || 'custom'})` : a.mime_type === 'video/youtube' ? 'YouTube' : a.mime_type?.startsWith('video/') ? 'Video' : 'Image'} ${a.zone_id ? ` · Zone: ${a.zone_id.slice(0,8)}` : ''} @@ -934,7 +934,7 @@ async function setupPlaylistActions(device) {
` } -
${c.filename}
+
${esc(c.filename)}
`).join('') || '

No media uploaded yet

'} diff --git a/frontend/js/views/video-wall.js b/frontend/js/views/video-wall.js index a76cbe4..d42d505 100644 --- a/frontend/js/views/video-wall.js +++ b/frontend/js/views/video-wall.js @@ -1,5 +1,6 @@ import { api } from '../api.js'; import { showToast } from '../components/toast.js'; +import { esc } from '../utils.js'; const API = (url, opts = {}) => fetch('/api' + url, { headers: { 'Content-Type': 'application/json', Authorization: `Bearer ${localStorage.getItem('token')}`, ...opts.headers }, ...opts }).then(r => r.json()); @@ -98,7 +99,7 @@ async function renderWallEditor(container, wallId) {

Content

diff --git a/server/package-lock.json b/server/package-lock.json index 23bd109..f318199 100644 --- a/server/package-lock.json +++ b/server/package-lock.json @@ -22,7 +22,7 @@ "socket.io": "^4.7.2", "stripe": "^20.4.1", "unzipper": "^0.12.3", - "uuid": "^9.0.0" + "uuid": "^14.0.0" } }, "node_modules/@emnapi/runtime": { @@ -2555,9 +2555,9 @@ } }, "node_modules/path-to-regexp": { - "version": "0.1.12", - "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-0.1.12.tgz", - "integrity": "sha512-RA1GjUVMnvYFxuqovrEqZoxxW5NUZqbwKtYz/Tt7nXerk0LbLblQmrsgdeOxV5SFHf0UDggjS/bSeOZwt1pmEQ==", + "version": "0.1.13", + "resolved": "https://registry.npmjs.org/path-to-regexp/-/path-to-regexp-0.1.13.tgz", + "integrity": "sha512-A/AGNMFN3c8bOlvV9RreMdrv7jsmF9XIfDeCd87+I8RNg6s78BhJxMu69NEMHBSJFxKidViTEdruRwEk/WIKqA==", "license": "MIT" }, "node_modules/prebuild-install": { @@ -3443,16 +3443,16 @@ } }, "node_modules/uuid": { - "version": "9.0.1", - "resolved": "https://registry.npmjs.org/uuid/-/uuid-9.0.1.tgz", - "integrity": "sha512-b+1eJOlsR9K8HJpow9Ok3fiWOWSIcIzXodvv0rQjVoOVNpWMpxf1wZNpt4y9h10odCNrqnYp1OBzRktckBe3sA==", + "version": "14.0.0", + "resolved": "https://registry.npmjs.org/uuid/-/uuid-14.0.0.tgz", + "integrity": "sha512-Qo+uWgilfSmAhXCMav1uYFynlQO7fMFiMVZsQqZRMIXp0O7rR7qjkj+cPvBHLgBqi960QCoo/PH2/6ZtVqKvrg==", "funding": [ "https://github.com/sponsors/broofa", "https://github.com/sponsors/ctavan" ], "license": "MIT", "bin": { - "uuid": "dist/bin/uuid" + "uuid": "dist-node/bin/uuid" } }, "node_modules/vary": { diff --git a/server/package.json b/server/package.json index 4ea80bb..dd405c0 100644 --- a/server/package.json +++ b/server/package.json @@ -22,6 +22,6 @@ "socket.io": "^4.7.2", "stripe": "^20.4.1", "unzipper": "^0.12.3", - "uuid": "^9.0.0" + "uuid": "^14.0.0" } } diff --git a/server/player/index.html b/server/player/index.html index 5367ea8..157b099 100644 --- a/server/player/index.html +++ b/server/player/index.html @@ -946,20 +946,24 @@ infoDiv.onclick = () => { infoDiv.style.display = 'none'; }; document.body.appendChild(infoDiv); + // Escape user-controllable values before injecting into innerHTML — filenames, + // device names, and server URLs are stored on the server and could contain HTML. + const escHtml = (s) => s == null ? '' : String(s).replace(/&/g,'&').replace(//g,'>').replace(/"/g,'"').replace(/'/g,'''); + // Update info overlay content periodically setInterval(() => { const el = document.getElementById('infoContent'); if (!el) return; const item = playlist[currentIndex]; el.innerHTML = ` - Device ID: ${config.deviceId?.slice(0, 8) || 'N/A'}...
- Device Name: ${config.deviceName || 'N/A'}
- Server: ${config.serverUrl || 'N/A'}
+ Device ID: ${escHtml(config.deviceId?.slice(0, 8) || 'N/A')}...
+ Device Name: ${escHtml(config.deviceName || 'N/A')}
+ Server: ${escHtml(config.serverUrl || 'N/A')}
Status: ${socket?.connected ? 'Connected' : 'Disconnected'}
- Now Playing: ${item?.filename || 'Nothing'} (${currentIndex + 1}/${playlist.length})
+ Now Playing: ${escHtml(item?.filename || 'Nothing')} (${currentIndex + 1}/${playlist.length})
Resolution: ${screen.width}x${screen.height}
Uptime: ${Math.floor(performance.now() / 60000)}m
- Platform: ${navigator.platform}
+ Platform: ${escHtml(navigator.platform)}
Cache: Service Worker ${navigator.serviceWorker?.controller ? 'Active' : 'Inactive'} `; }, 2000); diff --git a/server/routes/content.js b/server/routes/content.js index 40d8347..303a65b 100644 --- a/server/routes/content.js +++ b/server/routes/content.js @@ -7,6 +7,37 @@ const { db } = require('../db/database'); const upload = require('../middleware/upload'); const config = require('../config'); const { checkStorageLimit, checkRemoteUrl } = require('../middleware/subscription'); +const { sanitizeString } = require('../middleware/sanitize'); + +// Multer captures file.originalname directly from the multipart filename header, +// bypassing sanitizeBody. Apply the same HTML-escape here so a filename like +// `">.jpg` is stored as `"><img...` and +// renders as text in every UI sink. Umlauts, spaces, dots, and other unicode are +// preserved — sanitizeString only touches `& < > " '`. +function safeFilename(name) { + return sanitizeString(name || ''); +} + +// SSRF gate for remote_url. Returns null if valid, else { status, error }. +// Used by both POST /remote and PUT /:id so a user can't bypass the check by +// uploading a benign URL and then PUT-updating it to file:///etc/passwd. +function validateRemoteUrl(url) { + let parsed; + try { parsed = new URL(url); } + catch { return { status: 400, error: 'Invalid URL format' }; } + if (!['http:', 'https:'].includes(parsed.protocol)) { + return { status: 400, error: 'URL must use http or https' }; + } + const hostname = parsed.hostname.toLowerCase(); + const isPrivate = hostname === 'localhost' || hostname === '0.0.0.0' || + hostname.startsWith('127.') || hostname.startsWith('10.') || + hostname.startsWith('192.168.') || hostname.startsWith('169.254.') || + /^172\.(1[6-9]|2[0-9]|3[0-1])\./.test(hostname) || + hostname.startsWith('fc') || hostname.startsWith('fd') || hostname === '::1' || + hostname.endsWith('.local') || hostname.endsWith('.internal'); + if (isPrivate) return { status: 400, error: 'Internal URLs are not allowed' }; + return null; +} // List content for current user (admins see all). // folder_id filter: omit for everything; "root" or "" for root-level only; for that folder. @@ -96,7 +127,7 @@ router.post('/', checkStorageLimit, upload.single('file'), async (req, res) => { db.prepare(` INSERT INTO content (id, user_id, filename, filepath, mime_type, file_size, duration_sec, thumbnail_path, width, height) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?) - `).run(id, req.user.id, req.file.originalname, filepath, req.file.mimetype, req.file.size, durationSec, thumbnailPath, width, height); + `).run(id, req.user.id, safeFilename(req.file.originalname), filepath, req.file.mimetype, req.file.size, durationSec, thumbnailPath, width, height); const content = db.prepare('SELECT * FROM content WHERE id = ?').get(id); res.status(201).json(content); @@ -111,26 +142,8 @@ router.post('/remote', checkRemoteUrl, (req, res) => { try { const { url, name, mime_type } = req.body; if (!url) return res.status(400).json({ error: 'url is required' }); - // Validate URL format - try { - const parsed = new URL(url); - if (!['http:', 'https:'].includes(parsed.protocol)) { - return res.status(400).json({ error: 'URL must use http or https' }); - } - // Block private/internal IPs (SSRF protection) - const hostname = parsed.hostname.toLowerCase(); - const isPrivate = hostname === 'localhost' || hostname === '0.0.0.0' || - hostname.startsWith('127.') || hostname.startsWith('10.') || - hostname.startsWith('192.168.') || hostname.startsWith('169.254.') || - /^172\.(1[6-9]|2[0-9]|3[0-1])\./.test(hostname) || // 172.16.0.0 - 172.31.255.255 - hostname.startsWith('fc') || hostname.startsWith('fd') || hostname === '::1' || // IPv6 private - hostname.endsWith('.local') || hostname.endsWith('.internal'); - if (isPrivate) { - return res.status(400).json({ error: 'Internal URLs are not allowed' }); - } - } catch { - return res.status(400).json({ error: 'Invalid URL format' }); - } + const urlErr = validateRemoteUrl(url); + if (urlErr) return res.status(urlErr.status).json({ error: urlErr.error }); const id = uuidv4(); const filename = name || url.split('/').pop()?.split('?')[0] || 'remote_content'; @@ -139,7 +152,7 @@ router.post('/remote', checkRemoteUrl, (req, res) => { db.prepare(` INSERT INTO content (id, user_id, filename, filepath, mime_type, file_size, remote_url) VALUES (?, ?, ?, '', ?, 0, ?) - `).run(id, req.user.id, filename, mimeType, url); + `).run(id, req.user.id, safeFilename(filename), mimeType, url); const content = db.prepare('SELECT * FROM content WHERE id = ?').get(id); res.status(201).json(content); @@ -179,7 +192,7 @@ router.post('/youtube', async (req, res) => { db.prepare(` INSERT INTO content (id, user_id, filename, filepath, mime_type, file_size, remote_url, thumbnail_path) VALUES (?, ?, ?, '', 'video/youtube', 0, ?, ?) - `).run(id, req.user.id, filename, embedUrl, thumbnailUrl); + `).run(id, req.user.id, safeFilename(filename), embedUrl, thumbnailUrl); const content = db.prepare('SELECT * FROM content WHERE id = ?').get(id); res.status(201).json(content); @@ -226,18 +239,26 @@ router.put('/:id', (req, res) => { const { filename, mime_type, remote_url, folder, folder_id } = req.body; const updates = []; const values = []; - if (filename !== undefined) { updates.push('filename = ?'); values.push(filename); } + if (filename !== undefined) { updates.push('filename = ?'); values.push(safeFilename(filename)); } if (mime_type !== undefined) { updates.push('mime_type = ?'); values.push(mime_type); } - if (remote_url !== undefined) { updates.push('remote_url = ?'); values.push(remote_url || null); } + if (remote_url !== undefined) { + if (remote_url) { + const urlErr = validateRemoteUrl(remote_url); + if (urlErr) return res.status(urlErr.status).json({ error: urlErr.error }); + } + updates.push('remote_url = ?'); + values.push(remote_url || null); + } if (folder !== undefined) { updates.push('folder = ?'); values.push(folder || null); } if (folder_id !== undefined) { - // Verify the destination folder belongs to the same user (admins can move anywhere). - let target = null; + // Verify the destination folder belongs to the same user. Only superadmin gets + // cross-user access — matches the policy in routes/folders.js so a plain "admin" + // can't move content into a folder they can't see in GET /api/folders. if (folder_id) { - target = db.prepare('SELECT user_id FROM content_folders WHERE id = ?').get(folder_id); + const target = db.prepare('SELECT user_id FROM content_folders WHERE id = ?').get(folder_id); if (!target) return res.status(400).json({ error: 'Invalid folder_id' }); - const isAdmin = ['admin', 'superadmin'].includes(req.user.role); - if (!isAdmin && target.user_id !== req.user.id) { + const isSuperadmin = req.user.role === 'superadmin'; + if (!isSuperadmin && target.user_id !== req.user.id) { return res.status(403).json({ error: 'Cannot move content to another user\'s folder' }); } } diff --git a/server/routes/folders.js b/server/routes/folders.js index 37f17ee..fe57ecc 100644 --- a/server/routes/folders.js +++ b/server/routes/folders.js @@ -5,15 +5,25 @@ const { db } = require('../db/database'); const UUID_RE = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i; +// Per-user folder cap. The route has no rate limit (multer doesn't go through the +// global API limiter chain), so without a count cap a single account could insert +// millions of rows. 100 is a generous ceiling for a real organisational hierarchy +// — admins/superadmins are exempt because they may manage cross-user data. +const MAX_FOLDERS_PER_USER = 100; + // Verify a folder belongs to the current user (or null = root, also allowed). // Returns the row, or null if it exists but isn't owned by the user. +// +// Only superadmin gets cross-user access — matching the GET /api/folders listing +// (which has always been superadmin-only). The previous mismatch let a regular +// "admin" mutate folders they couldn't see, so the inconsistency was exploitable. function ownedFolder(req, folderId) { if (!folderId) return { id: null }; if (!UUID_RE.test(folderId)) return null; const row = db.prepare('SELECT * FROM content_folders WHERE id = ?').get(folderId); if (!row) return null; - const isAdmin = ['admin', 'superadmin'].includes(req.user.role); - if (!isAdmin && row.user_id !== req.user.id) return null; + const isSuperadmin = req.user.role === 'superadmin'; + if (!isSuperadmin && row.user_id !== req.user.id) return null; return row; } @@ -33,6 +43,16 @@ router.post('/', (req, res) => { if (!name) return res.status(400).json({ error: 'name is required' }); if (name.length > 100) return res.status(400).json({ error: 'name too long' }); + const isSuperadmin = req.user.role === 'superadmin'; + if (!isSuperadmin) { + const { count } = db.prepare('SELECT COUNT(*) AS count FROM content_folders WHERE user_id = ?').get(req.user.id); + if (count >= MAX_FOLDERS_PER_USER) { + return res.status(429).json({ + error: `Folder limit reached (${MAX_FOLDERS_PER_USER}). Delete unused folders before creating more.` + }); + } + } + const parentId = req.body.parent_id || null; if (parentId) { const parent = ownedFolder(req, parentId); diff --git a/server/routes/kiosk.js b/server/routes/kiosk.js index 9272048..86e05b6 100644 --- a/server/routes/kiosk.js +++ b/server/routes/kiosk.js @@ -134,14 +134,14 @@ router.get('/:id/render', (req, res) => { }); }); - // Idle screen after ${config.idleTimeout || 60} seconds of no interaction + // Idle screen after ${safeNumber(config.idleTimeout, 60)} seconds of no interaction let idleTimer; function resetIdleTimer() { document.getElementById('idleOverlay').style.display = 'none'; clearTimeout(idleTimer); idleTimer = setTimeout(() => { document.getElementById('idleOverlay').style.display = 'flex'; - }, ${(config.idleTimeout || 60) * 1000}); + }, ${safeNumber(config.idleTimeout, 60) * 1000}); } document.getElementById('idleOverlay').addEventListener('click', resetIdleTimer); ['touchstart', 'click', 'mousemove'].forEach(e => document.addEventListener(e, resetIdleTimer)); diff --git a/server/routes/schedules.js b/server/routes/schedules.js index 9f5035b..22c3cff 100644 --- a/server/routes/schedules.js +++ b/server/routes/schedules.js @@ -140,7 +140,8 @@ router.post('/', (req, res) => { router.put('/:id', (req, res) => { const schedule = db.prepare('SELECT * FROM schedules WHERE id = ?').get(req.params.id); if (!schedule) return res.status(404).json({ error: 'Schedule not found' }); - if (!['admin','superadmin'].includes(req.user.role) && schedule.user_id !== req.user.id) return res.status(403).json({ error: 'Access denied' }); + const isAdmin = ['admin','superadmin'].includes(req.user.role); + if (!isAdmin && schedule.user_id !== req.user.id) return res.status(403).json({ error: 'Access denied' }); // If changing target, enforce mutual exclusion const newDeviceId = req.body.device_id !== undefined ? req.body.device_id : schedule.device_id; @@ -152,13 +153,28 @@ router.put('/:id', (req, res) => { return res.status(400).json({ error: 'Either device_id or group_id is required' }); } - // Ownership check if changing to a new group - if (req.body.group_id && req.body.group_id !== schedule.group_id) { - const group = db.prepare('SELECT user_id FROM device_groups WHERE id = ?').get(req.body.group_id); - if (!group) return res.status(404).json({ error: 'Group not found' }); - if (!['admin','superadmin'].includes(req.user.role) && group.user_id !== req.user.id) { - return res.status(403).json({ error: 'Access denied' }); - } + // Re-verify ownership on every target field that is changing. Without this, a user + // could create a schedule on their own device and then PUT in another user's + // device_id / content_id / playlist_id to fire arbitrary content on victim devices. + function verifyOwnership(table, id) { + if (!id) return null; + const row = db.prepare(`SELECT user_id FROM ${table} WHERE id = ?`).get(id); + if (!row) return { status: 404, error: `${table.replace(/_/g, ' ').slice(0, -1)} not found` }; + if (!isAdmin && row.user_id !== req.user.id) return { status: 403, error: 'Access denied' }; + return null; + } + const ownershipChecks = [ + ['devices', req.body.device_id, schedule.device_id], + ['device_groups', req.body.group_id, schedule.group_id], + ['content', req.body.content_id, schedule.content_id], + ['widgets', req.body.widget_id, schedule.widget_id], + ['layouts', req.body.layout_id, schedule.layout_id], + ['playlists', req.body.playlist_id, schedule.playlist_id], + ]; + for (const [table, newVal, oldVal] of ownershipChecks) { + if (newVal === undefined || newVal === oldVal || !newVal) continue; + const err = verifyOwnership(table, newVal); + if (err) return res.status(err.status).json({ error: err.error }); } const fields = ['device_id', 'group_id', 'zone_id', 'content_id', 'widget_id', 'layout_id', 'playlist_id', 'title', diff --git a/server/routes/teams.js b/server/routes/teams.js index 62e9289..5783e7d 100644 --- a/server/routes/teams.js +++ b/server/routes/teams.js @@ -163,18 +163,35 @@ function checkTeamAccess(req, res) { return true; } -// Assign device to team +// Assign device to team. The caller must own the device (or be an admin) — without +// this check, any team member could pull any device into their team by guessing the +// UUID and then read/control it via the team-membership grants in routes/devices.js. router.post('/:id/devices', (req, res) => { if (!checkTeamAccess(req, res)) return; const { device_id } = req.body; if (!device_id) return res.status(400).json({ error: 'device_id required' }); + + const device = db.prepare('SELECT user_id FROM devices WHERE id = ?').get(device_id); + if (!device) return res.status(404).json({ error: 'Device not found' }); + const isAdmin = ['admin', 'superadmin'].includes(req.user.role); + if (!isAdmin && device.user_id !== req.user.id) { + return res.status(403).json({ error: 'You do not own this device' }); + } + db.prepare('UPDATE devices SET team_id = ? WHERE id = ?').run(req.params.id, device_id); res.json({ success: true }); }); -// Remove device from team +// Remove device from team. Only the device owner (or an admin) can detach a device +// from a team — otherwise a team member could orphan another user's device. router.delete('/:id/devices/:deviceId', (req, res) => { if (!checkTeamAccess(req, res)) return; + const device = db.prepare('SELECT user_id FROM devices WHERE id = ?').get(req.params.deviceId); + if (!device) return res.status(404).json({ error: 'Device not found' }); + const isAdmin = ['admin', 'superadmin'].includes(req.user.role); + if (!isAdmin && device.user_id !== req.user.id) { + return res.status(403).json({ error: 'You do not own this device' }); + } db.prepare('UPDATE devices SET team_id = NULL WHERE id = ? AND team_id = ?').run(req.params.deviceId, req.params.id); res.json({ success: true }); }); diff --git a/server/ws/deviceSocket.js b/server/ws/deviceSocket.js index 7e3bb7f..f4cb9ba 100644 --- a/server/ws/deviceSocket.js +++ b/server/ws/deviceSocket.js @@ -152,6 +152,22 @@ module.exports = function setupDeviceSocket(io) { // Someone reinstalled - link them back to existing device const oldDevice = db.prepare('SELECT * FROM devices WHERE id = ?').get(existing.device_id); if (oldDevice) { + // Fingerprint reclaim guard: a leaked/duplicated fingerprint shouldn't be enough + // to take over a live device. Reject the reclaim if the device is currently + // online OR has been online within the last 24h — by then a real reinstall has + // had plenty of time to come back, but a credential thief is more likely caught. + const liveConn = heartbeat.getConnection(existing.device_id); + const RECLAIM_GRACE_SECONDS = 24 * 60 * 60; + const lastBeat = oldDevice.last_heartbeat || 0; + const secondsSince = Math.floor(Date.now() / 1000) - lastBeat; + if (liveConn || (oldDevice.status === 'online') || secondsSince < RECLAIM_GRACE_SECONDS) { + console.warn(`Fingerprint reclaim rejected for ${existing.device_id}: device active (status=${oldDevice.status}, ${secondsSince}s since last heartbeat, liveConn=${!!liveConn})`); + socket.emit('device:auth-error', { + error: 'This display is currently active. If you reinstalled the app, the original device must be offline for 24 hours before its slot can be reclaimed.' + }); + return; + } + // Fingerprint matched — this is a reinstalled app reconnecting to its old device. // Issue a fresh token so the app can authenticate going forward. const newToken = generateDeviceToken();