From ffe907e73a844da8d9ee1ea0133155f48e48b19e Mon Sep 17 00:00:00 2001 From: UNITRONIX <36471318+UNITRONIX@users.noreply.github.com> Date: Mon, 20 Jul 2026 23:29:41 +0200 Subject: [PATCH] fix(web-remote): guest page 500 and cookie hijack (Refs #274) Safe EJS bootstrap for /remote/guest; panel session wins over stale guest cookie. --- CHANGELOG.md | 3 + web-nodejs/middleware/guestAccess.js | 15 +++- web-nodejs/public/js/rdclient/connection.js | 17 ++++- web-nodejs/public/js/remote.js | 1 + web-nodejs/routes/auth.routes.js | 6 ++ web-nodejs/routes/remote.routes.js | 79 ++++++++++++++------- web-nodejs/services/wsRelay.js | 36 +++++----- web-nodejs/tests/guestAccess.test.js | 79 ++++++++++++++++++++- web-nodejs/views/remote-guest.ejs | 4 +- web-nodejs/views/remote.ejs | 1 + 10 files changed, 189 insertions(+), 52 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ced007c4..0e5edf8b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ ## [Unreleased] +### Fixed +- **Guest Web Remote 500 + cookie hijack (#274):** `/remote/guest` no longer crashes during EJS render (guest bootstrap JSON uses the same safe pattern as the viewer). Panel sessions with `device.connect` win over a stale `betterdesk.guest` / `bd.guest` cookie, so operators are not hard-403’d on other device IDs after opening a guest link. Guest cookie is cleared on login and on `GET /remote` dashboard; RD WebSocket upgrades prefer `?guest=` on the session URL. + ### Changed - _(none yet)_ diff --git a/web-nodejs/middleware/guestAccess.js b/web-nodejs/middleware/guestAccess.js index 79092b92..04a50f86 100644 --- a/web-nodejs/middleware/guestAccess.js +++ b/web-nodejs/middleware/guestAccess.js @@ -13,13 +13,20 @@ function hashToken(token) { return crypto.createHash('sha256').update(String(token)).digest('hex'); } -function getGuestToken(req) { - const q = String(req.query?.guest || req.query?.t || '').trim(); - if (q) return q; +/** Explicit guest link query (?guest= / ?t=) — conscious navigation. */ +function getGuestTokenFromQuery(req) { + return String(req.query?.guest || req.query?.t || '').trim(); +} + +function getGuestTokenFromCookie(req) { const c = req.cookies && req.cookies[GUEST_COOKIE]; return c ? String(c).trim() : ''; } +function getGuestToken(req) { + return getGuestTokenFromQuery(req) || getGuestTokenFromCookie(req); +} + function setGuestCookie(res, token, expiresAt) { let maxAge = MAX_COOKIE_AGE_MS; if (expiresAt) { @@ -75,6 +82,8 @@ module.exports = { GUEST_COOKIE, hashToken, getGuestToken, + getGuestTokenFromQuery, + getGuestTokenFromCookie, setGuestCookie, clearGuestCookie, attachGuestGrant, diff --git a/web-nodejs/public/js/rdclient/connection.js b/web-nodejs/public/js/rdclient/connection.js index 4daf29fc..30949708 100644 --- a/web-nodejs/public/js/rdclient/connection.js +++ b/web-nodejs/public/js/rdclient/connection.js @@ -26,6 +26,19 @@ class RDConnection { get state() { return this._state; } + /** + * Guest Access Link token for WS auth (?guest=) — do not rely only on cookie. + * @returns {string} + */ + _guestQuerySuffix() { + try { + const q = new URLSearchParams(window.location.search); + const token = q.get('guest') || q.get('t') || window.__guestToken || ''; + if (token) return `?guest=${encodeURIComponent(token)}`; + } catch (_) { /* ignore */ } + return ''; + } + // ---- Event emitter ---- on(event, fn) { @@ -54,7 +67,7 @@ class RDConnection { connectRendezvous() { return new Promise((resolve, reject) => { this._setState('rendezvous'); - const url = `${this.wsBase}/ws/rendezvous`; + const url = `${this.wsBase}/ws/rendezvous${this._guestQuerySuffix()}`; const ws = new WebSocket(url); ws.binaryType = 'arraybuffer'; @@ -110,7 +123,7 @@ class RDConnection { connectRelay() { return new Promise((resolve, reject) => { this._setState('relay'); - const url = `${this.wsBase}/ws/relay`; + const url = `${this.wsBase}/ws/relay${this._guestQuerySuffix()}`; const ws = new WebSocket(url); ws.binaryType = 'arraybuffer'; diff --git a/web-nodejs/public/js/remote.js b/web-nodejs/public/js/remote.js index 4b2a8e7e..e5a9ec55 100644 --- a/web-nodejs/public/js/remote.js +++ b/web-nodejs/public/js/remote.js @@ -76,6 +76,7 @@ e.stopPropagation(); const token = new URLSearchParams(window.location.search).get('guest') || new URLSearchParams(window.location.search).get('t') + || window.__guestToken || ''; window.location.href = token ? ('/remote/guest?t=' + encodeURIComponent(token)) : '/remote/guest'; }; diff --git a/web-nodejs/routes/auth.routes.js b/web-nodejs/routes/auth.routes.js index 356f0e11..69cf6a1c 100644 --- a/web-nodejs/routes/auth.routes.js +++ b/web-nodejs/routes/auth.routes.js @@ -20,6 +20,7 @@ try { }; } const { guestOnly, requireAuth } = require('../middleware/auth'); +const { clearGuestCookie } = require('../middleware/guestAccess'); const { loginLimiter, passwordChangeLimiter } = require('../middleware/rateLimiter'); /** @@ -184,6 +185,9 @@ router.post('/api/auth/login', loginLimiter, async (req, res) => { if (user.emergencyMode) { req.session.emergencyMode = true; } + + // Drop stale guest cookie so Web Remote is not guest-hijacked after login + clearGuestCookie(res); // Log successful login await db.logAction(user.id, 'login', `User logged in`, req.ip); @@ -426,6 +430,7 @@ router.get('/api/auth/oidc/session', async (req, res) => { }; req.session.goToken = token; req.session.authMethod = 'oidc'; + clearGuestCookie(res); req.session.save((saveErr) => { if (saveErr) { @@ -542,6 +547,7 @@ function finalizeLoginSession(req, res, pendingUser, method) { username: pendingUser.username, role: pendingUser.role }; + clearGuestCookie(res); try { await db.updateLastLogin(pendingUser.id); diff --git a/web-nodejs/routes/remote.routes.js b/web-nodejs/routes/remote.routes.js index 1bbe598e..25e45418 100644 --- a/web-nodejs/routes/remote.routes.js +++ b/web-nodejs/routes/remote.routes.js @@ -8,12 +8,15 @@ const router = express.Router(); const fs = require('fs'); const db = require('../services/database'); const config = require('../config/config'); -const { requireRdClientAuth, rdClientGuestOnly, normalizeRdClientReturnUrl } = require('../middleware/auth'); +const logger = require('../lib/logger').child('REMOTE'); +const { requireRdClientAuth, rdClientGuestOnly, normalizeRdClientReturnUrl, roleHasPermission } = require('../middleware/auth'); const { rdClientPageLimiter } = require('../middleware/rateLimiter'); const betterdeskApi = require('../services/betterdeskApi'); const { getGuestToken, + getGuestTokenFromQuery, setGuestCookie, + clearGuestCookie, attachGuestGrant, peerAllowedByGrant, } = require('../middleware/guestAccess'); @@ -21,8 +24,17 @@ const { async function requireRemoteAccess(req, res, next) { const deviceId = req.params.deviceId; - // Multi-device Guest Access Link — token present means guest-only path (no panel login fallback) + // Panel session with device.connect wins over a stale guest cookie (avoids hijack 403). + const role = req.session && req.session.user && req.session.user.role; + if (req.session && req.session.userId && role !== 'pro' && roleHasPermission(role, 'device.connect')) { + return requireRdClientAuth('device.connect')(req, res, next); + } + + const queryToken = getGuestTokenFromQuery(req); const guestToken = getGuestToken(req); + + // Guest Access Link — hard deny only for an explicit ?guest= / ?t= without a valid grant. + // Cookie-only failures fall through to panel auth / login. if (guestToken && deviceId) { try { const grant = await attachGuestGrant(req, betterdeskApi, deviceId); @@ -30,13 +42,17 @@ async function requireRemoteAccess(req, res, next) { setGuestCookie(res, guestToken, grant.expires_at); return next(); } - } catch { - // invalid grant + } catch (err) { + logger.warn('Guest grant validate failed:', err.message || err); } - return res.status(403).render('errors/403', { - title: req.t('guest_access.invalid_title', 'Invalid guest link'), - message: req.t('guest_access.device_denied', 'This guest link is invalid, expired, or does not allow this device.'), - }); + if (queryToken) { + logger.info('Guest remote deny (explicit query, invalid/expired/not allowed):', deviceId); + return res.status(403).render('errors/403', { + title: req.t('guest_access.invalid_title', 'Invalid guest link'), + message: req.t('guest_access.device_denied', 'This guest link is invalid, expired, or does not allow this device.'), + }); + } + logger.debug('Stale guest cookie ignored; falling through to panel auth'); } // Legacy mesh single-device share @@ -104,29 +120,42 @@ router.get('/remote/guest', rdClientPageLimiter, async (req, res) => { }); const data = result.data || {}; if (!data.valid) { + logger.info('Guest peers invalid/expired'); return res.status(403).render('errors/403', { title: req.t('guest_access.invalid_title', 'Invalid guest link'), message: data.error || req.t('guest_access.expired', 'This guest link is invalid or expired.'), }); } setGuestCookie(res, token, data.expires_at); - res.render('remote-guest', { - title: req.t('guest_access.title', 'Guest Remote'), - activePage: 'remote', - guestToken: token, - guestMeta: { - view_only: !!data.view_only, - expires_at: data.expires_at || '', - label: data.label || '', - devices: data.devices || [], - }, - }); + const guestMeta = { + view_only: !!data.view_only, + expires_at: data.expires_at || '', + label: data.label || '', + devices: data.devices || [], + }; + try { + res.render('remote-guest', { + title: req.t('guest_access.title', 'Guest Remote'), + activePage: 'remote', + guestToken: token, + guestMeta, + }); + } catch (renderErr) { + logger.error('Guest remote-guest render failed:', renderErr); + if (!res.headersSent) { + res.status(500).type('text/plain').send( + 'Guest Remote page failed to render. Check console logs (LOG_LEVEL=info) and try again after updating.' + ); + } + } } catch (err) { - const msg = err.response?.data?.error || err.message; - return res.status(403).render('errors/403', { - title: req.t('guest_access.invalid_title', 'Invalid guest link'), - message: msg, - }); + logger.warn('Guest /remote/guest peers/API error:', err.response?.data?.error || err.message); + if (!res.headersSent) { + return res.status(403).render('errors/403', { + title: req.t('guest_access.invalid_title', 'Invalid guest link'), + message: err.response?.data?.error || err.message, + }); + } } }); @@ -134,6 +163,7 @@ router.get('/remote/guest', rdClientPageLimiter, async (req, res) => { * GET /remote - RdClient operator dashboard (device list + connect) */ router.get('/remote', rdClientPageLimiter, requireRdClientAuth('device.connect'), (req, res) => { + clearGuestCookie(res); res.render('remote-dashboard', { title: req.t('remote_dashboard.title'), activePage: 'remote', @@ -205,6 +235,7 @@ router.get('/remote/:deviceId', rdClientPageLimiter, requireRemoteAccess, async device: device || { id: deviceId, hostname: '', platform: '', note: '' }, serverPubKey: serverPubKey, capabilities, + guestToken: req.guestToken || getGuestTokenFromQuery(req) || '', layout: 'viewer' }); }); diff --git a/web-nodejs/services/wsRelay.js b/web-nodejs/services/wsRelay.js index a386118d..51dd6d7d 100644 --- a/web-nodejs/services/wsRelay.js +++ b/web-nodejs/services/wsRelay.js @@ -103,26 +103,26 @@ function initWsProxy(server, sessionMiddleware) { let hasGuest = false; if (!hasUser) { try { - const { GUEST_COOKIE } = require('../middleware/guestAccess'); - const raw = request.headers.cookie || ''; - const names = [GUEST_COOKIE, 'bd.guest', 'betterdesk.guest']; - for (const cookieName of names) { - const match = raw.split(';').map((p) => p.trim()).find((p) => p.startsWith(cookieName + '=')); - if (match) { - const val = decodeURIComponent(match.slice(cookieName.length + 1) || ''); - if (val) { - hasGuest = true; - request.guestToken = val; - break; - } - } + // Prefer ?guest= on WS URL (session pages always append it for guests) + const g = url.searchParams.get('guest') || url.searchParams.get('t'); + if (g) { + hasGuest = true; + request.guestToken = g; } - // Also accept ?guest= on WS URL (fallback) if (!hasGuest) { - const g = url.searchParams.get('guest') || url.searchParams.get('t'); - if (g) { - hasGuest = true; - request.guestToken = g; + const { GUEST_COOKIE } = require('../middleware/guestAccess'); + const raw = request.headers.cookie || ''; + const names = [GUEST_COOKIE, 'bd.guest', 'betterdesk.guest']; + for (const cookieName of names) { + const match = raw.split(';').map((p) => p.trim()).find((p) => p.startsWith(cookieName + '=')); + if (match) { + const val = decodeURIComponent(match.slice(cookieName.length + 1) || ''); + if (val) { + hasGuest = true; + request.guestToken = val; + break; + } + } } } } catch { diff --git a/web-nodejs/tests/guestAccess.test.js b/web-nodejs/tests/guestAccess.test.js index 4a562ca1..dd295af9 100644 --- a/web-nodejs/tests/guestAccess.test.js +++ b/web-nodejs/tests/guestAccess.test.js @@ -1,7 +1,14 @@ /** * Guest access middleware / allowlist helpers */ -const { peerAllowedByGrant, getGuestToken } = require('../middleware/guestAccess'); +const { + peerAllowedByGrant, + getGuestToken, + getGuestTokenFromQuery, + getGuestTokenFromCookie, + clearGuestCookie, + GUEST_COOKIE, +} = require('../middleware/guestAccess'); describe('guestAccess helpers', () => { test('peerAllowedByGrant checks allowlist', () => { @@ -10,9 +17,75 @@ describe('guestAccess helpers', () => { expect(peerAllowedByGrant(null, 'A')).toBe(false); }); - test('getGuestToken reads query guest or t', () => { + test('getGuestToken reads query guest or t before cookie', () => { expect(getGuestToken({ query: { guest: 'abc' }, cookies: {} })).toBe('abc'); expect(getGuestToken({ query: { t: 'xyz' }, cookies: {} })).toBe('xyz'); - expect(getGuestToken({ query: {}, cookies: { 'bd.guest': 'cookieTok' } })).toBe('cookieTok'); + expect(getGuestToken({ query: {}, cookies: { [GUEST_COOKIE]: 'cookieTok' } })).toBe('cookieTok'); + expect(getGuestToken({ + query: { guest: 'fromQuery' }, + cookies: { [GUEST_COOKIE]: 'fromCookie' }, + })).toBe('fromQuery'); + }); + + test('getGuestTokenFromQuery ignores cookie', () => { + expect(getGuestTokenFromQuery({ + query: {}, + cookies: { [GUEST_COOKIE]: 'cookieTok' }, + })).toBe(''); + expect(getGuestTokenFromQuery({ query: { t: 'q' }, cookies: {} })).toBe('q'); + }); + + test('getGuestTokenFromCookie ignores query', () => { + expect(getGuestTokenFromCookie({ + query: { guest: 'q' }, + cookies: { [GUEST_COOKIE]: 'c' }, + })).toBe('c'); + expect(getGuestTokenFromCookie({ query: { guest: 'q' }, cookies: {} })).toBe(''); + }); + + test('clearGuestCookie clears with matching path', () => { + const cleared = []; + const res = { + clearCookie(name, opts) { + cleared.push({ name, opts }); + }, + }; + clearGuestCookie(res); + expect(cleared).toHaveLength(1); + expect(cleared[0].name).toBe(GUEST_COOKIE); + expect(cleared[0].opts.path).toBe('/'); + }); +}); + +describe('remote-guest EJS bootstrap serialization', () => { + test('guestMeta serializes outside template-literal interpolation', () => { + const ejs = require('ejs'); + const fs = require('fs'); + const path = require('path'); + const tpl = fs.readFileSync(path.join(__dirname, '../views/remote-guest.ejs'), 'utf8'); + const guestMeta = { + view_only: false, + expires_at: '2026-07-21T00:00:00Z', + label: 'lab`el ${x}', + devices: [{ id: '6700120', hostname: 'DIAMOS `Serwer` 2', platform: 'windows' }], + }; + const html = ejs.render(tpl, { + title: 'Guest Remote', + guestToken: 'tok`en${x}', + guestMeta, + _: (k) => k, + lang: 'en', + appName: 'BetterDesk', + cacheVersion: '1', + translations: {}, + user: null, + branding: {}, + availableLanguageList: [], + cspNonce: 'n', + }, { filename: path.join(__dirname, '../views/remote-guest.ejs') }); + expect(html).toContain('window.__guestAccess ='); + expect(html).toContain('6700120'); + expect(html).toContain(JSON.stringify(guestMeta)); + expect(html).not.toMatch(/500 - Server Error/); }); }); diff --git a/web-nodejs/views/remote-guest.ejs b/web-nodejs/views/remote-guest.ejs index f421501d..c7f9d016 100644 --- a/web-nodejs/views/remote-guest.ejs +++ b/web-nodejs/views/remote-guest.ejs @@ -34,8 +34,8 @@ ` }); %> diff --git a/web-nodejs/views/remote.ejs b/web-nodejs/views/remote.ejs index 88690c8f..b81e3a70 100644 --- a/web-nodejs/views/remote.ejs +++ b/web-nodejs/views/remote.ejs @@ -434,6 +434,7 @@ window.__initialDeviceId = ` + JSON.stringify(deviceId) + `; window.__initialDeviceName = ` + JSON.stringify(device && device.hostname ? device.hostname : '') + `; window.__capabilities = ` + JSON.stringify(capabilities || { transport: 'rd' }) + `; + window.__guestToken = ` + JSON.stringify((typeof guestToken !== 'undefined' && guestToken) ? guestToken : '') + `; `