diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml index 17cd5705..263cab5f 100644 --- a/.github/codeql/codeql-config.yml +++ b/.github/codeql/codeql-config.yml @@ -63,6 +63,9 @@ query-filters: id: go/disabled-certificate-check paths: - betterdesk-agent/agent/agent.go + # API cert pinning uses VerifyPeerCertificate; development-only + # insecure TLS is additionally gated by !release and explicit opt-in. + - betterdesk-support-agent/apihttp.go # --- Test harness + global cookieParser (routes validate session/auth) --- - exclude: diff --git a/web-nodejs/public/js/rdclient/filetransfer.js b/web-nodejs/public/js/rdclient/filetransfer.js index 0f3999f8..01776f27 100644 --- a/web-nodejs/public/js/rdclient/filetransfer.js +++ b/web-nodejs/public/js/rdclient/filetransfer.js @@ -134,7 +134,7 @@ class RDFileTransfer { static joinLocalPath(base, relativePath) { const rel = String(relativePath || '').replace(/\\/g, '/').replace(/^\/+/, ''); - if (!base) return rel.replace(/\//g, '/'); + if (!base) return rel; const sep = base.includes('\\') ? '\\' : '/'; if (!rel) return base; const parts = rel.split('/').filter(Boolean); diff --git a/web-nodejs/routes/bd-api.routes.js b/web-nodejs/routes/bd-api.routes.js index 9ed30bc8..f3a409a4 100644 --- a/web-nodejs/routes/bd-api.routes.js +++ b/web-nodejs/routes/bd-api.routes.js @@ -38,6 +38,7 @@ const { requireDeviceToken, requireTokenDeviceMatch, } = require('../middleware/deviceAuth'); +const { doubleCsrfProtection } = require('../middleware/csrf'); // --------------------------------------------------------------------------- // Help requests & chat are stored on the Go server (single source of truth). @@ -995,7 +996,7 @@ router.get('/notifications', requireAuth, async (req, res) => { // POST /api/bd/notifications/:id/read — mark single notification read // --------------------------------------------------------------------------- -router.post('/notifications/:id/read', requireAuth, async (req, res) => { +router.post('/notifications/:id/read', doubleCsrfProtection, requireAuth, async (req, res) => { try { const userId = sessionUserId(req); if (!userId) { @@ -1017,7 +1018,7 @@ router.post('/notifications/:id/read', requireAuth, async (req, res) => { // POST /api/bd/notifications/read-all — mark all notifications read // --------------------------------------------------------------------------- -router.post('/notifications/read-all', requireAuth, async (req, res) => { +router.post('/notifications/read-all', doubleCsrfProtection, requireAuth, async (req, res) => { try { const userId = sessionUserId(req); if (!userId) { diff --git a/web-nodejs/server.js b/web-nodejs/server.js index 272cc9f5..7b420953 100644 --- a/web-nodejs/server.js +++ b/web-nodejs/server.js @@ -140,6 +140,9 @@ const sessionMiddleware = session({ } }); app.use(sessionMiddleware); +// Generate the double-submit token before routes that need route-level CSRF +// protection, including the session-authenticated notification endpoints. +app.use(csrfTokenProvider); // Cache version — changes on every restart/deployment, stable during runtime. // Used in ?v= query strings so browsers cache assets per deployment. @@ -230,7 +233,7 @@ app.use((req, res, next) => { next(); }); -// CSRF protection — generate token for views, validate on POST/PUT/DELETE/PATCH. +// CSRF protection — validate on POST/PUT/DELETE/PATCH. // Skip CSRF for device-facing API routes (/api/bd/*) — these MUST authenticate // via Bearer access token (session-cookie fallback is rejected in requireDeviceAuth). // @@ -240,7 +243,6 @@ app.use((req, res, next) => { // non-browser HTTP client, so it is unsafe as a CSRF-bypass signal. Tauri // desktop clients receive the CSRF token via `csrfTokenProvider` and must // echo it back in the `X-CSRF-Token` header (csrf-csrf double-submit). -app.use(csrfTokenProvider); app.use((req, res, next) => { if (req.path.startsWith('/api/bd/')) { return next(); diff --git a/web-nodejs/services/updateService.js b/web-nodejs/services/updateService.js index a0202ede..826d9d58 100644 --- a/web-nodejs/services/updateService.js +++ b/web-nodejs/services/updateService.js @@ -3553,6 +3553,9 @@ function restoreFromBackup(backupName) { const target = resolveManifestTarget(backupFilePath); const src = resolvePathUnderRoot(backupPath, backupFilePath); const dest = resolvePathUnderRoot(target.targetRoot, target.filePath); + if (isProtectedRuntimePath(dest)) { + throw new Error(`Refusing to restore protected runtime path: ${backupFilePath}`); + } if (fs.existsSync(src)) { fs.mkdirSync(path.dirname(dest), { recursive: true }); fs.copyFileSync(src, dest); @@ -3566,6 +3569,9 @@ function restoreFromBackup(backupName) { } const target = resolveManifestTarget(backupFilePath); const dest = resolvePathUnderRoot(target.targetRoot, target.filePath); + if (isProtectedRuntimePath(dest)) { + throw new Error(`Refusing to remove protected runtime path: ${backupFilePath}`); + } if (fs.existsSync(dest)) { fs.rmSync(dest, { force: true }); removed++; diff --git a/web-nodejs/services/userScopeService.js b/web-nodejs/services/userScopeService.js index caff3623..02c6c985 100644 --- a/web-nodejs/services/userScopeService.js +++ b/web-nodejs/services/userScopeService.js @@ -109,11 +109,11 @@ async function warnUnknownPeerIds(db, peerIds) { try { const row = await lookup(peerId); if (!row) { - console.warn(`[userScope] peer grant references unknown device id: ${peerId}`); + console.warn('[userScope] peer grant references unknown device id:', peerId); warned += 1; } } catch (err) { - console.warn(`[userScope] peer id lookup failed for ${peerId}:`, err.message); + console.warn('[userScope] peer id lookup failed for', peerId, err.message); warned += 1; } } diff --git a/web-nodejs/tests/bd-api.routes.test.js b/web-nodejs/tests/bd-api.routes.test.js index 233ebbf0..0f31cffb 100644 --- a/web-nodejs/tests/bd-api.routes.test.js +++ b/web-nodejs/tests/bd-api.routes.test.js @@ -3,6 +3,8 @@ const request = require('supertest'); const express = require('express'); const session = require('express-session'); +const cookieParser = require('cookie-parser'); +const { csrfTokenProvider } = require('../middleware/csrf'); jest.mock('../services/authService', () => ({})); @@ -83,21 +85,38 @@ describe('BD-API register rename guard', () => { describe('BD-API notification center', () => { let app; + const getCsrfCredentials = async () => { + const res = await request(app).get('/test-csrf'); + const csrfCookie = (res.headers['set-cookie'] || []) + .find((cookie) => cookie.startsWith('__csrf')); + return { + token: res.body.token, + cookie: csrfCookie?.split(';', 1)[0], + }; + }; beforeEach(() => { jest.clearAllMocks(); app = express(); app.use(express.json()); + app.use(cookieParser()); app.use(session({ secret: 'notification-test-secret', resave: false, saveUninitialized: true, + cookie: { + secure: true, + httpOnly: true, + sameSite: 'lax', + }, })); app.use((req, _res, next) => { req.session.userId = 1; req.session.user = { id: 1, username: 'admin', role: 'admin' }; next(); }); + app.use(csrfTokenProvider); + app.get('/test-csrf', (req, res) => res.json({ token: res.locals.csrfToken })); app.use('/api/bd', bdApiRoutes); db.getReadNotificationIds.mockResolvedValue(new Set()); @@ -148,16 +167,24 @@ describe('BD-API notification center', () => { it('does not expose registration notifications without enrollment permission', async () => { app = express(); app.use(express.json()); + app.use(cookieParser()); app.use(session({ secret: 'notification-test-secret', resave: false, saveUninitialized: true, + cookie: { + secure: true, + httpOnly: true, + sameSite: 'lax', + }, })); app.use((req, _res, next) => { req.session.userId = 2; req.session.user = { id: 2, username: 'viewer', role: 'viewer' }; next(); }); + app.use(csrfTokenProvider); + app.get('/test-csrf', (req, res) => res.json({ token: res.locals.csrfToken })); app.use('/api/bd', bdApiRoutes); db.getPendingRegistrations.mockResolvedValue([{ id: 8, @@ -173,6 +200,13 @@ describe('BD-API notification center', () => { expect(db.getPendingRegistrations).not.toHaveBeenCalled(); }); + it('rejects state changes without a CSRF token', async () => { + const res = await request(app).post('/api/bd/notifications/read-all'); + + expect(res.status).toBe(403); + expect(db.markAllNotificationsRead).not.toHaveBeenCalled(); + }); + it('marks both help and registration notifications read', async () => { db.getPendingRegistrations.mockResolvedValue([{ id: 9, @@ -184,7 +218,11 @@ describe('BD-API notification center', () => { data: [{ id: 'help-1', created_at: '2026-08-23T11:00:00.000Z' }], }); - const res = await request(app).post('/api/bd/notifications/read-all'); + const csrf = await getCsrfCredentials(); + const res = await request(app) + .post('/api/bd/notifications/read-all') + .set('Cookie', csrf.cookie) + .set('X-CSRF-Token', csrf.token); expect(res.status).toBe(200); expect(db.markAllNotificationsRead).toHaveBeenCalledWith(1, [ diff --git a/web-nodejs/tests/filetransfer.logic.test.js b/web-nodejs/tests/filetransfer.logic.test.js index 8a7ab500..f665beb3 100644 --- a/web-nodejs/tests/filetransfer.logic.test.js +++ b/web-nodejs/tests/filetransfer.logic.test.js @@ -46,6 +46,7 @@ describe('RDFileTransfer static helpers', () => { expect(RDFileTransfer.joinLocalPath('/home/me/dl', 'proj/readme.txt')) .toBe('/home/me/dl/proj/readme.txt'); expect(RDFileTransfer.joinLocalPath('/tmp/out', '')).toBe('/tmp/out'); + expect(RDFileTransfer.joinLocalPath('', 'proj/readme.txt')).toBe('proj/readme.txt'); }); }); diff --git a/web-nodejs/tests/updateService.consoleSync.test.js b/web-nodejs/tests/updateService.consoleSync.test.js index 01cc4c04..b18b4b1f 100644 --- a/web-nodejs/tests/updateService.consoleSync.test.js +++ b/web-nodejs/tests/updateService.consoleSync.test.js @@ -3,6 +3,7 @@ const fs = require('fs'); const os = require('os'); const path = require('path'); +const config = require('../config/config'); const { createConsoleDeployGraph } = require('../lib/consoleDeployGraph'); const { GITHUB_COMPARE_FILE_LIMIT, @@ -11,6 +12,7 @@ const { getDownloadRetryDelayMs, ensureGoServerSignalRelayPorts, restoreServerBinaryBackup, + restoreFromBackup, } = require('../services/updateService'); describe('updateService console sync helpers', () => { @@ -107,4 +109,55 @@ describe('updateService console sync helpers', () => { error: 'Server binary backup path failed validation', }); }); + + test('rejects traversal paths from a backup manifest', () => { + const backupRoot = path.join(config.dataDir, 'backups'); + const backupName = `pre-update-${Date.now()}-${process.pid}`; + const backupPath = path.join(backupRoot, backupName); + fs.mkdirSync(backupPath, { recursive: true }); + fs.writeFileSync(path.join(backupPath, 'manifest.json'), JSON.stringify({ + files: ['../outside.txt'], + })); + + try { + expect(() => restoreFromBackup(backupName)) + .toThrow('Invalid path in backup manifest'); + } finally { + fs.rmSync(backupPath, { recursive: true, force: true }); + } + }); + + test('refuses to restore protected runtime files from a backup manifest', () => { + const backupRoot = path.join(config.dataDir, 'backups'); + const backupName = `pre-update-${Date.now()}-${process.pid}`; + const backupPath = path.join(backupRoot, backupName); + fs.mkdirSync(backupPath, { recursive: true }); + fs.writeFileSync(path.join(backupPath, 'manifest.json'), JSON.stringify({ + files: ['console/.env'], + })); + + try { + expect(() => restoreFromBackup(backupName)) + .toThrow('Refusing to restore protected runtime path'); + } finally { + fs.rmSync(backupPath, { recursive: true, force: true }); + } + }); + + test('refuses to remove protected runtime files from a backup manifest', () => { + const backupRoot = path.join(config.dataDir, 'backups'); + const backupName = `pre-update-${Date.now()}-${process.pid}`; + const backupPath = path.join(backupRoot, backupName); + fs.mkdirSync(backupPath, { recursive: true }); + fs.writeFileSync(path.join(backupPath, 'manifest.json'), JSON.stringify({ + removeOnRestore: ['console/.env'], + })); + + try { + expect(() => restoreFromBackup(backupName)) + .toThrow('Refusing to remove protected runtime path'); + } finally { + fs.rmSync(backupPath, { recursive: true, force: true }); + } + }); }); diff --git a/web-nodejs/tests/userScopeService.test.js b/web-nodejs/tests/userScopeService.test.js index 8eb59866..0a043068 100644 --- a/web-nodejs/tests/userScopeService.test.js +++ b/web-nodejs/tests/userScopeService.test.js @@ -41,7 +41,10 @@ describe('userScopeService peer grants', () => { }; await expect(userScopeService.warnUnknownPeerIds(db, ['1', 'missing'])).resolves.toBeUndefined(); - expect(warn).toHaveBeenCalledWith(expect.stringContaining('unknown device id: missing')); + expect(warn).toHaveBeenCalledWith( + '[userScope] peer grant references unknown device id:', + 'missing' + ); warn.mockRestore(); }); });