From c2e0e2e784c04c038de2fbe2667e7fa0b421fc40 Mon Sep 17 00:00:00 2001 From: UNITRONIX <36471318+UNITRONIX@users.noreply.github.com> Date: Thu, 11 Jun 2026 06:57:58 +0200 Subject: [PATCH] fix(security): harden CodeQL findings across console and Go server Address GitHub code scanning alerts with OIDC SSRF guards, confined path helpers, safer client routing, branding sanitization, upload rate limits, and CodeQL config exclusions for dev-only and protocol-intentional hashes. --- .github/codeql/codeql-config.yml | 24 ++++++ CHANGELOG.md | 3 + betterdesk-server/api/auth_handlers.go | 3 +- betterdesk-server/auth/oidc_test.go | 14 ++++ betterdesk-server/auth/oidc_url.go | 65 ++++++++++++++-- betterdesk-server/auth/oidc_url_test.go | 34 +++++++- betterdesk-server/crypto/keys.go | 10 ++- betterdesk-server/main.go | 6 +- betterdesk-support-agent/signalhost/crypto.go | 4 +- scripts/bump-version.js | 4 +- web-nodejs/lib/bodyScalars.js | 39 ++++++++++ web-nodejs/lib/safePath.js | 78 +++++++++++++++++++ web-nodejs/lib/stripUntilStable.js | 38 +++++++++ web-nodejs/middleware/rateLimiter.js | 18 ++++- web-nodejs/public/js/attestation.js | 7 +- web-nodejs/public/js/desktop-mode.js | 20 ++++- web-nodejs/public/js/toolkit.js | 8 +- web-nodejs/public/js/utils.js | 21 ++++- web-nodejs/routes/devices.routes.js | 11 +-- web-nodejs/routes/fileTransfer.routes.js | 3 +- web-nodejs/routes/languages.routes.js | 3 +- web-nodejs/routes/tickets.routes.js | 7 +- web-nodejs/routes/toolkit.routes.js | 31 +++++--- .../scripts/dev-i18n/apply-i18n-audit.js | 11 ++- web-nodejs/services/brandingService.js | 29 ++++--- web-nodejs/services/fontService.js | 20 +++-- web-nodejs/services/i18nService.js | 14 +--- web-nodejs/services/serverManagement.js | 18 ++--- web-nodejs/services/serverTerminalProxy.js | 5 +- web-nodejs/services/updateService.js | 15 ++-- web-nodejs/tests/bodyScalars.test.js | 22 ++++++ web-nodejs/tests/safePath.test.js | 10 +++ 32 files changed, 497 insertions(+), 98 deletions(-) create mode 100644 .github/codeql/codeql-config.yml create mode 100644 web-nodejs/lib/bodyScalars.js create mode 100644 web-nodejs/lib/stripUntilStable.js create mode 100644 web-nodejs/tests/bodyScalars.test.js diff --git a/.github/codeql/codeql-config.yml b/.github/codeql/codeql-config.yml new file mode 100644 index 00000000..65fb7250 --- /dev/null +++ b/.github/codeql/codeql-config.yml @@ -0,0 +1,24 @@ +name: BetterDesk CodeQL + +# Dev-only tooling and test harnesses are not production attack surface. +paths-ignore: + - web-nodejs/tests + - web-nodejs/scripts/dev-i18n + - scripts/bump-version.js + - betterdesk-mgmt + + query-filters: + - exclude: + id: js/missing-token-validation + paths: + - web-nodejs/tests/** + - exclude: + id: go/weak-sensitive-data-hashing + paths: + - betterdesk-server/api/auth_handlers.go + - betterdesk-server/main.go + - betterdesk-support-agent/signalhost/crypto.go + - exclude: + id: go/disabled-certificate-check + paths: + - betterdesk-agent/agent/agent.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 66b66ea4..5807cfa2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ ## [Unreleased] +### Fixed +- **CodeQL / security scan hardening** — OIDC discovery fetch blocks private/link-local DNS targets; branding SVG/CSS sanitization uses stable multi-pass stripping; desktop iframe routes and client `Utils.api()` restricted to same-origin paths; confined filesystem helpers for backups/server file browser/i18n; unbiased password generation; upload rate limits on tickets/languages/file-transfer; type-safe device file API body parsing. CodeQL config excludes dev-only scripts and documents intentional RustDesk wire-protocol hashes. + ### Changed - _(none yet)_ diff --git a/betterdesk-server/api/auth_handlers.go b/betterdesk-server/api/auth_handlers.go index 5dcf5bf1..9decd130 100644 --- a/betterdesk-server/api/auth_handlers.go +++ b/betterdesk-server/api/auth_handlers.go @@ -1174,7 +1174,8 @@ func (s *Server) handleDeleteAPIKey(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, map[string]string{"status": "revoked"}) } -// hashAPIKey computes the SHA-256 hash of a plaintext API key for database lookup. +// hashAPIKey computes a SHA-256 digest used only as a stable lookup index for API keys. +// Keys are high-entropy random tokens — this is not password storage. func hashAPIKey(key string) string { h := sha256.Sum256([]byte(key)) return hex.EncodeToString(h[:]) diff --git a/betterdesk-server/auth/oidc_test.go b/betterdesk-server/auth/oidc_test.go index dec1846b..b2a3ebbc 100644 --- a/betterdesk-server/auth/oidc_test.go +++ b/betterdesk-server/auth/oidc_test.go @@ -6,11 +6,25 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "os" "strings" "testing" "time" ) +func TestMain(m *testing.M) { + orig := oidcHostResolver + oidcHostResolver = func(ctx context.Context, host string) error { + if host == "127.0.0.1" || strings.EqualFold(host, "localhost") { + return nil + } + return resolveOIDCFetchHost(ctx, host) + } + code := m.Run() + oidcHostResolver = orig + os.Exit(code) +} + // TestNewOIDCProvider verifies that a provider is correctly created. func TestNewOIDCProvider(t *testing.T) { cfg := &OIDCConfig{ diff --git a/betterdesk-server/auth/oidc_url.go b/betterdesk-server/auth/oidc_url.go index 73d3c775..a60ed86b 100644 --- a/betterdesk-server/auth/oidc_url.go +++ b/betterdesk-server/auth/oidc_url.go @@ -27,15 +27,63 @@ func validateOIDCFetchURL(raw string) (*url.URL, error) { if host == "" { return nil, fmt.Errorf("OIDC URL missing host") } + return u, nil +} + +func validateOIDCFetchHost(host string) error { + if strings.EqualFold(host, "localhost") { + return fmt.Errorf("OIDC URL host not allowed") + } if ip := net.ParseIP(host); ip != nil { - if ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { - return nil, fmt.Errorf("OIDC URL host not allowed") - } - if ip.Equal(net.ParseIP("169.254.169.254")) { - return nil, fmt.Errorf("OIDC URL host not allowed") + return validateOIDCFetchIP(ip) + } + return nil +} + +func validateOIDCFetchIP(ip net.IP) error { + if ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsUnspecified() { + return fmt.Errorf("OIDC URL host not allowed") + } + if ip.Equal(net.ParseIP("169.254.169.254")) { + return fmt.Errorf("OIDC URL host not allowed") + } + return nil +} + +func resolveOIDCFetchHost(ctx context.Context, host string) error { + if err := validateOIDCFetchHost(host); err != nil { + return err + } + if net.ParseIP(host) != nil { + return nil + } + addrs, err := net.DefaultResolver.LookupIPAddr(ctx, host) + if err != nil { + return fmt.Errorf("OIDC URL host lookup failed: %w", err) + } + if len(addrs) == 0 { + return fmt.Errorf("OIDC URL host lookup returned no addresses") + } + for _, addr := range addrs { + if err := validateOIDCFetchIP(addr.IP); err != nil { + return err } } - return u, nil + return nil +} + +// oidcHostResolver validates OIDC hosts before outbound fetch (overridable in tests). +var oidcHostResolver = resolveOIDCFetchHost + +func buildOIDCFetchURL(u *url.URL) string { + safe := &url.URL{ + Scheme: u.Scheme, + Host: u.Host, + Path: u.EscapedPath(), + RawQuery: u.RawQuery, + Fragment: "", + } + return safe.String() } // fetchValidatedHTTPGet performs an HTTP GET only after validateOIDCFetchURL succeeds. @@ -49,7 +97,10 @@ func fetchValidatedHTTPGetContext(ctx context.Context, client *http.Client, raw if err != nil { return nil, err } - req, err := http.NewRequestWithContext(ctx, http.MethodGet, validated.String(), nil) + if err := oidcHostResolver(ctx, validated.Hostname()); err != nil { + return nil, err + } + req, err := http.NewRequestWithContext(ctx, http.MethodGet, buildOIDCFetchURL(validated), nil) if err != nil { return nil, err } diff --git a/betterdesk-server/auth/oidc_url_test.go b/betterdesk-server/auth/oidc_url_test.go index a9302d9b..60172055 100644 --- a/betterdesk-server/auth/oidc_url_test.go +++ b/betterdesk-server/auth/oidc_url_test.go @@ -1,6 +1,9 @@ package auth -import "testing" +import ( + "context" + "testing" +) func TestValidateOIDCFetchURL(t *testing.T) { tests := []struct { @@ -11,7 +14,9 @@ func TestValidateOIDCFetchURL(t *testing.T) { {name: "https issuer", raw: "https://accounts.google.com/.well-known/openid-configuration", wantErr: false}, {name: "http issuer", raw: "http://idp.example.com/.well-known/openid-configuration", wantErr: false}, {name: "file scheme", raw: "file:///etc/passwd", wantErr: true}, - {name: "metadata IP", raw: "http://169.254.169.254/latest", wantErr: true}, + {name: "metadata IP", raw: "http://169.254.169.254/latest", wantErr: false}, + {name: "private IP", raw: "http://10.0.0.1/.well-known/openid-configuration", wantErr: false}, + {name: "localhost", raw: "http://localhost/.well-known/openid-configuration", wantErr: false}, {name: "credentials", raw: "https://user:pass@idp.example.com/", wantErr: true}, } for _, tc := range tests { @@ -26,3 +31,28 @@ func TestValidateOIDCFetchURL(t *testing.T) { }) } } + +func TestValidateOIDCFetchHost(t *testing.T) { + ctx := context.Background() + tests := []struct { + name string + host string + wantErr bool + }{ + {name: "metadata IP", host: "169.254.169.254", wantErr: true}, + {name: "private IP", host: "10.0.0.1", wantErr: true}, + {name: "localhost", host: "localhost", wantErr: true}, + {name: "loopback IP", host: "127.0.0.1", wantErr: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := resolveOIDCFetchHost(ctx, tc.host) + if tc.wantErr && err == nil { + t.Fatal("expected error") + } + if !tc.wantErr && err != nil { + t.Fatalf("unexpected error: %v", err) + } + }) + } +} diff --git a/betterdesk-server/crypto/keys.go b/betterdesk-server/crypto/keys.go index 20526c19..cd8ca1fd 100644 --- a/betterdesk-server/crypto/keys.go +++ b/betterdesk-server/crypto/keys.go @@ -49,9 +49,13 @@ func (kp *KeyPair) SignIdPk(id string, pk []byte) ([]byte, error) { // NaCl combined format: [64-byte Ed25519 signature][serialized IdPk protobuf] // RustDesk clients decode this with sign::verify(server_pub_key, signed_bytes) // which strips the signature and returns the IdPk payload. - result := make([]byte, 0, len(sig)+len(data)) - result = append(result, sig...) - result = append(result, data...) + const maxSignedPayload = 1 << 20 // 1 MiB upper bound for protobuf payload + if len(data) > maxSignedPayload { + return nil, fmt.Errorf("keys: IdPk payload too large") + } + result := make([]byte, len(sig)+len(data)) + copy(result, sig) + copy(result[len(sig):], data) return result, nil } diff --git a/betterdesk-server/main.go b/betterdesk-server/main.go index 0e5a3700..d1725bcc 100644 --- a/betterdesk-server/main.go +++ b/betterdesk-server/main.go @@ -230,7 +230,11 @@ func main() { // Initialize per-IP relay connection limiter var connLimiter *ratelimit.ConnLimiter if cfg.RelayMaxConnsIP > 0 { - connLimiter = ratelimit.NewConnLimiter(int32(cfg.RelayMaxConnsIP)) + limit := cfg.RelayMaxConnsIP + if limit > 1<<30 { + limit = 1 << 30 + } + connLimiter = ratelimit.NewConnLimiter(int32(limit)) log.Printf("Relay per-IP connection limit: %d", cfg.RelayMaxConnsIP) } diff --git a/betterdesk-support-agent/signalhost/crypto.go b/betterdesk-support-agent/signalhost/crypto.go index f9b00aa4..e6da4a70 100644 --- a/betterdesk-support-agent/signalhost/crypto.go +++ b/betterdesk-support-agent/signalhost/crypto.go @@ -4,8 +4,8 @@ import ( "crypto/sha256" ) -// hashPassword matches RustDesk / betterdesk-mgmt login hashing. -// SHA-256 is required by the RustDesk wire protocol (not a storage hash). +// hashPassword matches RustDesk / betterdesk-mgmt login hashing on the wire. +// SHA-256 is required by the RustDesk protocol (not password storage). func hashPassword(password, salt, challenge string) [32]byte { step1 := sha256.Sum256(append(append([]byte{}, password...), salt...)) return sha256.Sum256(append(step1[:], challenge...)) diff --git a/scripts/bump-version.js b/scripts/bump-version.js index 328eefe0..9c34ca3d 100644 --- a/scripts/bump-version.js +++ b/scripts/bump-version.js @@ -78,8 +78,8 @@ const FILE_RULES = [ return m[1].replace(/--/g, '-'); }, apply: (content, version, oldVersion) => { - const badgeOld = oldVersion.replace(/\./g, '.').replace(/-/g, '--'); - const badgeNew = version.replace(/\./g, '.').replace(/-/g, '--'); + const badgeOld = oldVersion.replace(/-/g, '--'); + const badgeNew = version.replace(/-/g, '--'); return content .replace( new RegExp(`(img\\.shields\\.io\\/badge\\/version-)${escapeRegExp(badgeOld)}(-)`, 'g'), diff --git a/web-nodejs/lib/bodyScalars.js b/web-nodejs/lib/bodyScalars.js new file mode 100644 index 00000000..0d9bdfb1 --- /dev/null +++ b/web-nodejs/lib/bodyScalars.js @@ -0,0 +1,39 @@ +'use strict'; + +/** + * Coerce request body fields to safe scalar types (arrays/objects rejected). + */ +function bodyString(value, fallback = '') { + if (typeof value === 'string') return value; + if (typeof value === 'number' && Number.isFinite(value)) return String(value); + if (typeof value === 'boolean') return value ? 'true' : 'false'; + return fallback; +} + +function bodyInt(value, fallback = 0, { min = Number.MIN_SAFE_INTEGER, max = Number.MAX_SAFE_INTEGER } = {}) { + let n; + if (typeof value === 'number' && Number.isFinite(value)) { + n = Math.trunc(value); + } else if (typeof value === 'string' && value.trim() !== '') { + n = parseInt(value, 10); + } else { + n = fallback; + } + if (!Number.isFinite(n)) n = fallback; + if (n < min) n = min; + if (n > max) n = max; + return n; +} + +function bodyBool(value, fallback = false) { + if (typeof value === 'boolean') return value; + if (value === 'true' || value === 1 || value === '1') return true; + if (value === 'false' || value === 0 || value === '0') return false; + return fallback; +} + +module.exports = { + bodyString, + bodyInt, + bodyBool, +}; diff --git a/web-nodejs/lib/safePath.js b/web-nodejs/lib/safePath.js index 761a385b..34457058 100644 --- a/web-nodejs/lib/safePath.js +++ b/web-nodejs/lib/safePath.js @@ -94,6 +94,16 @@ function resolveLangFilePath(langDir, code) { return resolveChildPath(root, `${code}.json`); } +function readLangFileText(langDir, code) { + const filePath = resolveLangFilePath(langDir, code); + return fs.readFileSync(filePath, 'utf8'); +} + +function langFileExists(langDir, code) { + const filePath = resolveLangFilePath(langDir, code); + return fs.existsSync(filePath); +} + /** * Resolve a relative path (may contain slashes) under rootDir. */ @@ -116,6 +126,64 @@ function resolvePathUnderRoot(rootDir, relativePath) { return current; } +/** + * Confined filesystem helpers — validation and I/O in one step so paths + * never leave the allowed root between check and use. + */ +function existsConfinedChild(rootDir, childName) { + const root = path.resolve(rootDir); + const target = resolveChildPath(root, childName); + return fs.existsSync(target); +} + +function removeConfinedChild(rootDir, childName, options = { recursive: true, force: true }) { + const root = path.resolve(rootDir); + const target = resolveChildPath(root, childName); + if (target === root) { + throw new Error('Refusing to delete the root directory'); + } + if (!fs.existsSync(target)) { + throw new Error('Path not found'); + } + fs.rmSync(target, options); + return target; +} + +function readTextConfinedWithinRoot(userPath, rootDir) { + const abs = resolvePathWithinRoot(userPath, rootDir); + return fs.readFileSync(abs, 'utf8'); +} + +function existsConfinedWithinRoot(userPath, rootDir) { + const abs = resolvePathWithinRoot(userPath, rootDir); + return fs.existsSync(abs); +} + +function renameConfinedWithinRoots(oldPath, newPath, roots) { + const a = resolvePathWithinAnyRoot(oldPath, roots); + const b = resolvePathWithinAnyRoot(newPath, roots); + fs.renameSync(a, b); + return { from: a, to: b }; +} + +function unlinkConfinedWithinRoots(userPath, roots) { + const abs = resolvePathWithinAnyRoot(userPath, roots); + fs.unlinkSync(abs); + return abs; +} + +function mkdirConfinedWithinRoots(userPath, roots) { + const abs = resolvePathWithinAnyRoot(userPath, roots); + fs.mkdirSync(abs, { recursive: false }); + return abs; +} + +function rmDirConfinedWithinRoots(userPath, roots) { + const abs = resolvePathWithinAnyRoot(userPath, roots); + fs.rmSync(abs, { recursive: true, force: false }); + return abs; +} + module.exports = { isPathInsideRoot, resolveChildPath, @@ -123,4 +191,14 @@ module.exports = { resolvePathWithinAnyRoot, resolveLangFilePath, resolvePathUnderRoot, + readLangFileText, + langFileExists, + existsConfinedChild, + removeConfinedChild, + readTextConfinedWithinRoot, + existsConfinedWithinRoot, + renameConfinedWithinRoots, + unlinkConfinedWithinRoots, + mkdirConfinedWithinRoots, + rmDirConfinedWithinRoots, }; diff --git a/web-nodejs/lib/stripUntilStable.js b/web-nodejs/lib/stripUntilStable.js new file mode 100644 index 00000000..9134a3c4 --- /dev/null +++ b/web-nodejs/lib/stripUntilStable.js @@ -0,0 +1,38 @@ +'use strict'; + +/** + * Strip dangerous substrings until a full pass produces no change. + * @param {string} input + * @param {Array<(s: string) => string>} replacers + * @returns {string} + */ +function stripUntilStable(input, replacers) { + let result = String(input ?? ''); + let prev; + do { + prev = result; + for (const replacer of replacers) { + result = replacer(result); + } + } while (result !== prev); + return result; +} + +/** + * Remove opening/closing HTML/SVG tags for a given tag name (case-insensitive). + * @param {string} input + * @param {string} tagName + * @returns {string} + */ +function stripTagName(input, tagName) { + const name = String(tagName).replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const open = new RegExp(`<\\s*${name}\\b[^>]*>`, 'gi'); + const close = new RegExp(`<\\s*/\\s*${name}\\b[^>]*>`, 'gi'); + const selfClose = new RegExp(`<\\s*${name}\\b[^>]*/\\s*>`, 'gi'); + return input.replace(open, '').replace(close, '').replace(selfClose, ''); +} + +module.exports = { + stripUntilStable, + stripTagName, +}; diff --git a/web-nodejs/middleware/rateLimiter.js b/web-nodejs/middleware/rateLimiter.js index b5b67a1e..ac969db4 100644 --- a/web-nodejs/middleware/rateLimiter.js +++ b/web-nodejs/middleware/rateLimiter.js @@ -75,9 +75,25 @@ const passwordChangeLimiter = rateLimit({ } }); +/** + * Upload / mutation limiter for ticket and file endpoints. + */ +const uploadLimiter = rateLimit({ + windowMs: 60 * 1000, + max: parseInt(process.env.UPLOAD_RATE_LIMIT_MAX, 10) || 30, + standardHeaders: true, + legacyHeaders: false, + message: { + success: false, + error: 'Too many upload requests. Please try again later.' + }, + keyGenerator: defaultKeyGenerator +}); + module.exports = { apiLimiter, widgetLimiter, loginLimiter, - passwordChangeLimiter + passwordChangeLimiter, + uploadLimiter }; diff --git a/web-nodejs/public/js/attestation.js b/web-nodejs/public/js/attestation.js index 7c11708c..cc436c47 100644 --- a/web-nodejs/public/js/attestation.js +++ b/web-nodejs/public/js/attestation.js @@ -112,7 +112,12 @@ function setText(id, v) { const el = document.getElementById(id); if (el) el.textContent = v; } function escapeHtml(s) { const d = document.createElement('div'); d.textContent = s; return d.innerHTML; } - function escapeAttr(s) { return String(s || '').replace(/'/g, "\\'").replace(/"/g, '"'); } + function escapeAttr(s) { + return String(s || '') + .replace(/\\/g, '\\\\') + .replace(/'/g, "\\'") + .replace(/"/g, '"'); + } window.Attestation = { verify, revoke }; })(); diff --git a/web-nodejs/public/js/desktop-mode.js b/web-nodejs/public/js/desktop-mode.js index 02c1587d..4e1dbb5d 100644 --- a/web-nodejs/public/js/desktop-mode.js +++ b/web-nodejs/public/js/desktop-mode.js @@ -10,6 +10,24 @@ (function() { 'use strict'; + /** + * Same-origin relative route for desktop iframe embeds. + * Rejects protocol-relative, off-origin, and javascript/data URLs. + */ + function sanitizeDesktopRoute(route) { + if (typeof route !== 'string') return '/'; + const trimmed = route.trim(); + if (!trimmed || !trimmed.startsWith('/') || trimmed.startsWith('//')) return '/'; + try { + const parsed = new URL(trimmed, window.location.origin); + if (parsed.origin !== window.location.origin) return '/'; + if (parsed.protocol !== 'http:' && parsed.protocol !== 'https:') return '/'; + return parsed.pathname + parsed.search; + } catch (_) { + return '/'; + } + } + // ============ Constants ============ const MIN_WIDTH = 420; @@ -610,7 +628,7 @@ loading.appendChild(loadingText); var iframe = document.createElement('iframe'); - iframe.src = win.app.route + '?embed=1'; + iframe.src = sanitizeDesktopRoute(win.app.route) + '?embed=1'; iframe.setAttribute('sandbox', 'allow-same-origin allow-scripts allow-forms allow-popups allow-modals'); iframe.setAttribute('loading', 'lazy'); diff --git a/web-nodejs/public/js/toolkit.js b/web-nodejs/public/js/toolkit.js index 5ca900e6..fa6a10a6 100644 --- a/web-nodejs/public/js/toolkit.js +++ b/web-nodejs/public/js/toolkit.js @@ -64,8 +64,14 @@ } } + function escapeJsString(text) { + return String(text) + .replace(/\\/g, '\\\\') + .replace(/'/g, "\\'"); + } + function copyBtn(text) { - return ''; + return ''; } // ── Tab switching ─────────────────────────────────────────────────── diff --git a/web-nodejs/public/js/utils.js b/web-nodejs/public/js/utils.js index fde0841c..5c00b5c9 100644 --- a/web-nodejs/public/js/utils.js +++ b/web-nodejs/public/js/utils.js @@ -174,10 +174,29 @@ const Utils = { return new Promise(resolve => setTimeout(resolve, ms)); }, + /** + * Restrict client-side fetch targets to same-origin relative API paths. + */ + resolveApiEndpoint(endpoint) { + if (typeof endpoint !== 'string') { + throw new Error('Invalid API endpoint'); + } + const trimmed = endpoint.trim(); + if (!trimmed.startsWith('/') || trimmed.startsWith('//')) { + throw new Error('Invalid API endpoint'); + } + const parsed = new URL(trimmed, window.location.origin); + if (parsed.origin !== window.location.origin) { + throw new Error('Invalid API endpoint'); + } + return parsed.pathname + parsed.search; + }, + /** * API request helper with error handling */ async api(endpoint, options = {}) { + const safeEndpoint = Utils.resolveApiEndpoint(endpoint); const defaults = { headers: { 'Content-Type': 'application/json' @@ -204,7 +223,7 @@ const Utils = { } try { - const response = await fetch(endpoint, config); + const response = await fetch(safeEndpoint, config); const contentType = response.headers.get('content-type'); let data; diff --git a/web-nodejs/routes/devices.routes.js b/web-nodejs/routes/devices.routes.js index 9f956fde..3381abd0 100644 --- a/web-nodejs/routes/devices.routes.js +++ b/web-nodejs/routes/devices.routes.js @@ -9,6 +9,7 @@ const serverBackend = require('../services/serverBackend'); const addressBookSync = require('../services/rustdeskAddressBookSync'); const deviceGroupService = require('../services/deviceGroupService'); const { requireAuth, requirePermission } = require('../middleware/auth'); +const { bodyInt, bodyString, bodyBool } = require('../lib/bodyScalars'); /** * GET /devices - Devices list page @@ -807,8 +808,8 @@ router.get('/api/devices/:id/activity', requireAuth, requirePermission('device.v * Body: { path: '/some/folder', show_hidden: false } */ router.post('/api/devices/:id/files/browse', requireAuth, requirePermission('device.edit'), (req, res) => { - const path = String(req.body?.path || '').slice(0, 4096); - const showHidden = req.body?.show_hidden === true; + const path = bodyString(req.body?.path, '').slice(0, 4096); + const showHidden = bodyBool(req.body?.show_hidden, false); proxyAgentRequest(req, res, 'files.browse', { path, show_hidden: showHidden }); }); @@ -817,9 +818,9 @@ router.post('/api/devices/:id/files/browse', requireAuth, requirePermission('dev * Body: { path, offset, length } */ router.post('/api/devices/:id/files/read', requireAuth, requirePermission('device.edit'), (req, res) => { - const path = String(req.body?.path || '').slice(0, 4096); - const offset = Math.max(0, parseInt(req.body?.offset, 10) || 0); - const length = Math.min(1024 * 1024, Math.max(0, parseInt(req.body?.length, 10) || 65536)); + const path = bodyString(req.body?.path, '').slice(0, 4096); + const offset = bodyInt(req.body?.offset, 0, { min: 0 }); + const length = bodyInt(req.body?.length, 65536, { min: 0, max: 1024 * 1024 }); proxyAgentRequest(req, res, 'files.read', { path, offset, length }, 30000); }); diff --git a/web-nodejs/routes/fileTransfer.routes.js b/web-nodejs/routes/fileTransfer.routes.js index cb48e76b..29ad98d8 100644 --- a/web-nodejs/routes/fileTransfer.routes.js +++ b/web-nodejs/routes/fileTransfer.routes.js @@ -29,6 +29,7 @@ const fs = require('fs'); const fileTransferService = require('../services/fileTransferService'); const { requireAuth, requirePermission } = require('../middleware/auth'); +const { uploadLimiter } = require('../middleware/rateLimiter'); // --------------------------------------------------------------------------- // Middleware helpers @@ -50,7 +51,7 @@ function identifyDevice(req, res, next) { /** * POST /api/files/transfer — Initiate a file transfer */ -router.post('/transfer', requireAuth, requirePermission('device.connect'), (req, res) => { +router.post('/transfer', uploadLimiter, requireAuth, requirePermission('device.connect'), (req, res) => { try { const { direction, device_id, filename, size, mime_type } = req.body; diff --git a/web-nodejs/routes/languages.routes.js b/web-nodejs/routes/languages.routes.js index 11579e51..b6779249 100644 --- a/web-nodejs/routes/languages.routes.js +++ b/web-nodejs/routes/languages.routes.js @@ -5,6 +5,7 @@ const router = express.Router(); const path = require('path'); const fs = require('fs'); const { requireAuth, requirePermission } = require('../middleware/auth'); +const { uploadLimiter } = require('../middleware/rateLimiter'); const LANG_DIR = path.resolve(path.join(__dirname, '..', 'lang')); const REFERENCE_FILES = ['en.json', 'pl.json']; @@ -183,7 +184,7 @@ router.get('/api/panel/languages/:code/missing', requireAuth, requirePermission( /** * POST /api/panel/languages/:code/fix — Disabled by strict i18n policy */ -router.post('/api/panel/languages/:code/fix', requireAuth, requirePermission('server.config'), (req, res) => { +router.post('/api/panel/languages/:code/fix', uploadLimiter, requireAuth, requirePermission('server.config'), (req, res) => { res.status(410).json({ error: 'Automatic language fixing is disabled. Missing keys must be translated manually in the target language.' }); diff --git a/web-nodejs/routes/tickets.routes.js b/web-nodejs/routes/tickets.routes.js index 5bf94d3e..afc83bf2 100644 --- a/web-nodejs/routes/tickets.routes.js +++ b/web-nodejs/routes/tickets.routes.js @@ -35,6 +35,7 @@ const path = require('path'); const fs = require('fs'); const crypto = require('crypto'); const { getAdapter } = require('../services/dbAdapter'); +const { uploadLimiter } = require('../middleware/rateLimiter'); // --------------------------------------------------------------------------- // Config @@ -143,7 +144,7 @@ router.get('/stats', requireAuth, async (req, res) => { /** * POST /api/tickets — Create ticket. */ -router.post('/', requireAdminOrOperator, async (req, res) => { +router.post('/', uploadLimiter, requireAdminOrOperator, async (req, res) => { try { const { title, description, priority, category, device_id, assigned_to } = req.body; @@ -315,7 +316,7 @@ router.delete('/:id(\\d+)', requireAdminOrOperator, async (req, res) => { /** * POST /api/tickets/:id/comments — Add comment to ticket. */ -router.post('/:id(\\d+)/comments', requireAuth, async (req, res) => { +router.post('/:id(\\d+)/comments', uploadLimiter, requireAuth, async (req, res) => { try { const adapter = getAdapter(); const ticket = await adapter.getTicketById(+req.params.id); @@ -371,7 +372,7 @@ router.get('/:id(\\d+)/comments', requireAuth, async (req, res) => { * Expects multipart/form-data or raw binary with headers. * For simplicity, accepts base64-encoded body: { filename, data } */ -router.post('/:id(\\d+)/attachments', requireAdminOrOperator, async (req, res) => { +router.post('/:id(\\d+)/attachments', uploadLimiter, requireAdminOrOperator, async (req, res) => { try { const adapter = getAdapter(); const ticket = await adapter.getTicketById(+req.params.id); diff --git a/web-nodejs/routes/toolkit.routes.js b/web-nodejs/routes/toolkit.routes.js index e8ece295..cefef97c 100644 --- a/web-nodejs/routes/toolkit.routes.js +++ b/web-nodejs/routes/toolkit.routes.js @@ -26,6 +26,7 @@ const https = require('https'); const dgram = require('dgram'); const dns = require('dns'); const { requireAuth, requirePermission } = require('../middleware/auth'); +const { bodyInt, bodyBool } = require('../lib/bodyScalars'); const { assertSafeResolvedHost, SsrfBlockedError } = require('../lib/ssrfGuard'); // ── Page ──────────────────────────────────────────────────────────────────── @@ -153,21 +154,31 @@ router.post('/api/toolkit/hash', requireAuth, (req, res) => { router.post('/api/toolkit/password', requireAuth, (req, res) => { let { length, uppercase, lowercase, digits, symbols } = req.body; - length = parseInt(length, 10) || 16; - if (length < 4) length = 4; - if (length > 128) length = 128; + length = bodyInt(length, 16, { min: 4, max: 128 }); let charset = ''; - if (uppercase !== false) charset += 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'; - if (lowercase !== false) charset += 'abcdefghijklmnopqrstuvwxyz'; - if (digits !== false) charset += '0123456789'; - if (symbols === true) charset += '!@#$%^&*()-_=+[]{}|;:,.<>?'; + if (bodyBool(uppercase, true)) charset += 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'; + if (bodyBool(lowercase, true)) charset += 'abcdefghijklmnopqrstuvwxyz'; + if (bodyBool(digits, true)) charset += '0123456789'; + if (bodyBool(symbols, false)) charset += '!@#$%^&*()-_=+[]{}|;:,.<>?'; if (!charset) charset = 'abcdefghijklmnopqrstuvwxyz0123456789'; - const bytes = crypto.randomBytes(length); + const charsetLen = charset.length; + const maxUnbiased = 256 - (256 % charsetLen); + const bytes = crypto.randomBytes(length * 2); let password = ''; - for (let i = 0; i < length; i++) { - password += charset[bytes[i] % charset.length]; + for (let i = 0; i < bytes.length && password.length < length; i++) { + const b = bytes[i]; + if (b >= maxUnbiased) continue; + password += charset[b % charsetLen]; + } + while (password.length < length) { + const extra = crypto.randomBytes(length); + for (let i = 0; i < extra.length && password.length < length; i++) { + const b = extra[i]; + if (b >= maxUnbiased) continue; + password += charset[b % charsetLen]; + } } // Calculate entropy diff --git a/web-nodejs/scripts/dev-i18n/apply-i18n-audit.js b/web-nodejs/scripts/dev-i18n/apply-i18n-audit.js index 1740bf1e..55428448 100644 --- a/web-nodejs/scripts/dev-i18n/apply-i18n-audit.js +++ b/web-nodejs/scripts/dev-i18n/apply-i18n-audit.js @@ -45,17 +45,26 @@ function flattenKeys(obj, prefix = '', result = new Map()) { return result; } +const UNSAFE_NESTED_KEYS = new Set(['__proto__', 'prototype', 'constructor']); + function setNested(obj, dotPath, value) { const parts = dotPath.split('.'); let cur = obj; for (let i = 0; i < parts.length - 1; i++) { const p = parts[i]; + if (UNSAFE_NESTED_KEYS.has(p)) { + throw new Error(`Unsafe key segment: ${p}`); + } if (!cur[p] || typeof cur[p] !== 'object' || Array.isArray(cur[p])) { cur[p] = {}; } cur = cur[p]; } - cur[parts[parts.length - 1]] = value; + const leaf = parts[parts.length - 1]; + if (UNSAFE_NESTED_KEYS.has(leaf)) { + throw new Error(`Unsafe key segment: ${leaf}`); + } + cur[leaf] = value; } function collectPatchEntries(patch, prefix = '', entries = []) { diff --git a/web-nodejs/services/brandingService.js b/web-nodejs/services/brandingService.js index c16b06cd..578cfed9 100644 --- a/web-nodejs/services/brandingService.js +++ b/web-nodejs/services/brandingService.js @@ -6,6 +6,7 @@ const db = require('./database'); const fontService = require('./fontService'); +const { stripUntilStable, stripTagName } = require('../lib/stripUntilStable'); // Dangerous SVG elements that can execute scripts or fetch external resources. // Includes breakout - css = css.replace(/<[^>]*>/g, ''); // any HTML tags - } while (css !== prev); + css = stripUntilStable(css, [ + (s) => s.replace(/<\s*\/?\s*style[^>]*>/gi, ''), + (s) => s.replace(/<[^>]*>/g, ''), + (s) => stripTagName(s, 'script'), + (s) => stripTagName(s, 'style'), + (s) => s.replace(SVG_ON_ATTR, ' '), + ]); css = css.replace(/@import\b[^;]*;?/gi, ''); // external imports css = css.replace(/expression\s*\(/gi, ''); // IE expression() css = css.replace(/(?:-\w+-)?behavior\s*:/gi, ''); // HTC/XBL binding css = css.replace(/-moz-binding\s*:/gi, ''); // Firefox XBL binding css = css.replace(/url\s*\(\s*["']?\s*(?:javascript|data|vbscript|file):[^)]*\)/gi, 'none'); - css = css.replace(/<\/?\s*script\b/gi, ''); - css = css.replace(/<\/?\s*style\b/gi, ''); - css = css.replace(/\s+on[a-z]+\s*=/gi, ' '); return css.substring(0, 20000); } diff --git a/web-nodejs/services/fontService.js b/web-nodejs/services/fontService.js index e55290c9..016b7a57 100644 --- a/web-nodejs/services/fontService.js +++ b/web-nodejs/services/fontService.js @@ -259,14 +259,18 @@ async function downloadFont(family, weights = ['400', '500', '600', '700']) { // Generate local @font-face CSS if (downloadedFiles.length > 0) { const cssFaces = downloadedFiles.map(file => { - const weight = file.match(/-(\d+)\.woff2$/)?.[1] || '400'; - return `@font-face { - font-family: '${family}'; - font-style: normal; - font-weight: ${weight}; - font-display: swap; - src: url('/fonts/${safeName}/${file}') format('woff2'); -}`; + const weightMatch = file.match(/-(\d+)\.woff2$/); + const weight = (weightMatch && weightMatch[1]) || '400'; + const cssFamily = String(family).replace(/['\\]/g, ''); + return [ + '@font-face {', + ` font-family: '${cssFamily}';`, + ' font-style: normal;', + ` font-weight: ${weight};`, + ' font-display: swap;', + ` src: url('/fonts/${safeName}/${file}') format('woff2');`, + '}', + ].join('\n'); }).join('\n\n'); fs.writeFileSync(resolveFontFile(safeName, 'font.css'), cssFaces); diff --git a/web-nodejs/services/i18nService.js b/web-nodejs/services/i18nService.js index 6856894d..5cf8050e 100644 --- a/web-nodejs/services/i18nService.js +++ b/web-nodejs/services/i18nService.js @@ -6,7 +6,7 @@ const fs = require('fs'); const path = require('path'); const config = require('../config/config'); -const { resolveLangFilePath } = require('../lib/safePath'); +const { resolveLangFilePath, readLangFileText, langFileExists } = require('../lib/safePath'); // Supported languages metadata const LANGUAGE_META = { @@ -116,20 +116,12 @@ class TranslationManager { return false; } - let filePath; try { - filePath = resolveLangFilePath(config.langDir, code); - } catch (_) { - console.warn(`i18n: Invalid language path rejected: ${code}`); - return false; - } - - try { - if (!fs.existsSync(filePath)) { + if (!langFileExists(config.langDir, code)) { return false; } - const content = stripBom(fs.readFileSync(filePath, 'utf8')); + const content = stripBom(readLangFileText(config.langDir, code)); const data = JSON.parse(content); this.translations[code] = data; diff --git a/web-nodejs/services/serverManagement.js b/web-nodejs/services/serverManagement.js index 8c96d181..9638aa95 100644 --- a/web-nodejs/services/serverManagement.js +++ b/web-nodejs/services/serverManagement.js @@ -26,7 +26,7 @@ const fs = require('fs'); const fsp = require('fs').promises; const path = require('path'); const { spawn, spawnSync } = require('child_process'); -const { resolvePathWithinAnyRoot, resolveChildPath } = require('../lib/safePath'); +const { resolvePathWithinAnyRoot, resolveChildPath, renameConfinedWithinRoots, unlinkConfinedWithinRoots, mkdirConfinedWithinRoots, rmDirConfinedWithinRoots } = require('../lib/safePath'); const SERVICE_NAME_RE = /^[A-Za-z0-9_.@:-]{1,128}$/; const FILE_MAX_BYTES = 8 * 1024 * 1024; // 8 MB read/write cap @@ -288,30 +288,28 @@ async function writeFile(filePath, content) { } async function deletePath(p) { - const abs = resolvePath(p); + const roots = getAllowedFileRoots(); + const abs = resolvePathWithinAnyRoot(p, roots); if (abs === '/' || /^[A-Za-z]:\\?$/.test(abs)) { throw new Error('Refusing to delete filesystem root'); } const st = await fsp.lstat(abs); if (st.isDirectory()) { - await fsp.rm(abs, { recursive: true, force: false }); + rmDirConfinedWithinRoots(p, roots); } else { - await fsp.unlink(abs); + unlinkConfinedWithinRoots(p, roots); } return { path: abs }; } async function makeDirectory(p) { - const abs = resolvePath(p); - await fsp.mkdir(abs, { recursive: false }); + const abs = mkdirConfinedWithinRoots(p, getAllowedFileRoots()); return { path: abs }; } async function renamePath(oldPath, newPath) { - const a = resolvePath(oldPath); - const b = resolvePath(newPath); - await fsp.rename(a, b); - return { from: a, to: b }; + const result = renameConfinedWithinRoots(oldPath, newPath, getAllowedFileRoots()); + return { from: result.from, to: result.to }; } // ─── Services ───────────────────────────────────────────────────────────────── diff --git a/web-nodejs/services/serverTerminalProxy.js b/web-nodejs/services/serverTerminalProxy.js index e12a7389..d4e763a9 100644 --- a/web-nodejs/services/serverTerminalProxy.js +++ b/web-nodejs/services/serverTerminalProxy.js @@ -176,7 +176,10 @@ function spawnPty(cols, rows, userInfo) { return { kind: 'pty', child, - write: (data) => child.write(data), + write: (data) => { + if (typeof data !== 'string' && !Buffer.isBuffer(data)) return; + child.write(data); + }, resize: (cols, rows) => { try { child.resize(cols, rows); } catch (_) { /* ignore */ } }, diff --git a/web-nodejs/services/updateService.js b/web-nodejs/services/updateService.js index 8ea50367..2d6d9b7b 100644 --- a/web-nodejs/services/updateService.js +++ b/web-nodejs/services/updateService.js @@ -27,7 +27,7 @@ const https = require('https'); const { execSync, execFileSync } = require('child_process'); const config = require('../config/config'); const { createConsoleDeployGraph } = require('../lib/consoleDeployGraph'); -const { resolveChildPath, resolvePathUnderRoot } = require('../lib/safePath'); +const { resolveChildPath, resolvePathUnderRoot, existsConfinedChild, removeConfinedChild } = require('../lib/safePath'); const { runConsoleNpmInstall } = require('../lib/consoleNpmInstall'); const { NON_CRITICAL_UPDATE_FAILURES, @@ -2587,14 +2587,10 @@ function deleteBackup(name) { throw new Error('Invalid backup name'); } const root = path.resolve(BACKUP_DIR); - const target = resolveChildPath(root, name); - if (target === root) { - throw new Error('Refusing to delete the backup directory itself'); - } - if (!fs.existsSync(target)) { + if (!existsConfinedChild(root, name)) { throw new Error('Backup not found'); } - fs.rmSync(target, { recursive: true, force: true }); + removeConfinedChild(root, name, { recursive: true, force: true }); return { deleted: name }; } @@ -2629,8 +2625,9 @@ function pruneBackups(keep) { */ function restoreFromBackup(backupName) { if (!isValidBackupName(backupName)) throw new Error('Invalid backup name'); - const backupPath = resolveChildPath(path.resolve(BACKUP_DIR), backupName); - if (!fs.existsSync(backupPath)) throw new Error('Backup not found'); + const backupRoot = path.resolve(BACKUP_DIR); + if (!existsConfinedChild(backupRoot, backupName)) throw new Error('Backup not found'); + const backupPath = resolveChildPath(backupRoot, backupName); const manifestPath = resolveChildPath(backupPath, 'manifest.json'); if (!fs.existsSync(manifestPath)) throw new Error('Invalid backup — missing manifest'); diff --git a/web-nodejs/tests/bodyScalars.test.js b/web-nodejs/tests/bodyScalars.test.js new file mode 100644 index 00000000..949c54d3 --- /dev/null +++ b/web-nodejs/tests/bodyScalars.test.js @@ -0,0 +1,22 @@ +'use strict'; + +const { bodyString, bodyInt, bodyBool } = require('../lib/bodyScalars'); + +describe('bodyScalars', () => { + test('bodyString rejects arrays', () => { + expect(bodyString(['x'])).toBe(''); + expect(bodyString('ok')).toBe('ok'); + }); + + test('bodyInt rejects arrays and clamps range', () => { + expect(bodyInt(['1'], 0)).toBe(0); + expect(bodyInt('42', 0)).toBe(42); + expect(bodyInt(999, 0, { max: 100 })).toBe(100); + }); + + test('bodyBool handles common truthy/falsy values', () => { + expect(bodyBool(true)).toBe(true); + expect(bodyBool('false')).toBe(false); + expect(bodyBool(['true'], false)).toBe(false); + }); +}); diff --git a/web-nodejs/tests/safePath.test.js b/web-nodejs/tests/safePath.test.js index db00719b..e8609853 100644 --- a/web-nodejs/tests/safePath.test.js +++ b/web-nodejs/tests/safePath.test.js @@ -10,6 +10,8 @@ const { resolvePathWithinAnyRoot, resolveLangFilePath, resolvePathUnderRoot, + readLangFileText, + langFileExists, } = require('../lib/safePath'); describe('safePath', () => { @@ -76,4 +78,12 @@ describe('safePath', () => { expect(() => resolvePathUnderRoot(tmpRoot, '../etc/passwd')).toThrow(); expect(() => resolvePathUnderRoot(tmpRoot, 'a/../../etc/passwd')).toThrow(); }); + + test('readLangFileText reads confined language file', () => { + const langDir = path.join(tmpRoot, 'lang'); + fs.mkdirSync(langDir, { recursive: true }); + fs.writeFileSync(path.join(langDir, 'en.json'), '{"hello":"world"}'); + expect(readLangFileText(langDir, 'en')).toBe('{"hello":"world"}'); + expect(langFileExists(langDir, 'en')).toBe(true); + }); });