fix(web-remote): guest page 500 and cookie hijack (Refs #274)

Safe EJS bootstrap for /remote/guest; panel session wins over stale guest cookie.
2026-09-10 01:27:11 +00:00 · 2026-07-20 23:29:41 +02:00
parent 421acd2791
commit ffe907e73a
10 changed files with 189 additions and 52 deletions
@@ -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)_

@@ -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,
@@ -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';
@@ -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';
            };
@@ -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);
@@ -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'
    });
 });
@@ -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 {
@@ -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/);
    });
 });
@@ -34,8 +34,8 @@
    </div>
 </div>
 <script>
-window.__guestAccess = ${JSON.stringify(typeof guestMeta !== 'undefined' ? guestMeta : {})};
-window.__guestToken = ${JSON.stringify(typeof guestToken !== 'undefined' ? guestToken : '')};
+    window.__guestAccess = ` + JSON.stringify(typeof guestMeta !== 'undefined' ? guestMeta : {}) + `;
+    window.__guestToken = ` + JSON.stringify(typeof guestToken !== 'undefined' ? guestToken : '') + `;
 </script>
 `
 }); %>
@@ -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 : '') + `;
 </script>

 `