From 0df7f58b265352504f9da62456b3f6e71c86e894 Mon Sep 17 00:00:00 2001 From: ScreenTinker Date: Tue, 28 Jul 2026 20:40:14 -0500 Subject: [PATCH] Parse MAX_FILE_SIZE, and document what else caps an upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to #233, which made the upload ceiling configurable — the right call, 500MB is genuinely too low for video. An environment variable is a string, so the value reached multer's limits.fileSize as text where a number is expected. That survives some comparisons through coercion and misbehaves in others, which is the worst kind of bug to find later; the line directly above it already used parseInt for the same reason. It is parsed properly now, and a suffix is accepted — someone raising a limit for video is choosing "about 2GB", and 2147483648 is easy to mistype by a factor of ten. An unparseable value falls back to the default rather than becoming NaN or zero. Either would reject every upload on the instance, from a typo in an env file, with nothing on screen to explain it. The documentation matters as much as the code here. MAX_FILE_SIZE is the LAST limit in the chain: nginx caps the request body with client_max_body_size and returns 413 before the app is reached — our own deployment sets 500M — and Cloudflare caps uploads per plan at the edge. Raising the variable alone often changes nothing, so the README now says so, with the nginx directive and a note that an upload failing with nothing in the server log never reached the server. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Uaeo9MvzKoyXuN6ZsbhtkL --- README.md | 21 +++++++ server/config.js | 10 +++- server/lib/parse-size.js | 37 ++++++++++++ server/test/max-file-size-config.test.js | 76 ++++++++++++++++++++++++ 4 files changed, 143 insertions(+), 1 deletion(-) create mode 100644 server/lib/parse-size.js create mode 100644 server/test/max-file-size-config.test.js diff --git a/README.md b/README.md index 519bf7c..2d93841 100644 --- a/README.md +++ b/README.md @@ -136,8 +136,29 @@ Schema migrations run automatically on first boot — no manual migration comman | `PING_TIMEOUT` | Socket.IO Engine.IO pong wait (ms). Lower = faster dead-socket detection; higher = more forgiving of laggy clients. | `30000` | | `HEARTBEAT_INTERVAL` | App-level offline-checker frequency (ms). How often the server sweeps the device list looking for stale heartbeats. | `10000` | | `HEARTBEAT_TIMEOUT` | How long without an app-level heartbeat (ms) before marking a device offline. Raise for slow/jittery networks. | `45000` | +| `MAX_FILE_SIZE` | Largest upload the server will accept. Bytes, or a suffix (`2GB`, `1500MB`). **A reverse proxy caps this independently** — see below. | `500MB` | | `COMMAND_QUEUE_TTL_MS` | How long the server holds commands and playlist-updates for a device that's offline at emit time (ms). Flushed in order on reconnect within this window; dropped past TTL. | `30000` | +#### Raising the upload limit + +`MAX_FILE_SIZE` sets what **the application** accepts. It is usually not the only limit, and it +is the last one in the chain — so raising it on its own often changes nothing and the upload +still fails with a `413`: + +- **nginx** (or any reverse proxy) caps the request body with `client_max_body_size`. The + default is 1MB, and a typical signage deployment sets 500M. Raise it to match: + + ```nginx + client_max_body_size 2048M; # must be >= MAX_FILE_SIZE + ``` + +- **Cloudflare** caps uploads per plan (100MB on Free/Pro at the time of writing) and returns + 413 at the edge, before your server is involved. Large uploads need a plan that allows them, + or a route that bypasses the proxy. + +If an upload fails and nothing appears in the server log, the request never reached the app — +check the proxy first. + ### Optional Integrations All integrations are optional. The app works fully without any of them. diff --git a/server/config.js b/server/config.js index ba1816e..3189cf4 100644 --- a/server/config.js +++ b/server/config.js @@ -20,6 +20,8 @@ function parseBillingRateTable(raw) { return null; } +const { parseSize } = require('./lib/parse-size'); + module.exports = { port: process.env.PORT || 3001, httpsPort: process.env.HTTPS_PORT || 3443, @@ -59,7 +61,13 @@ module.exports = { // #148 Item 4: TCP SO_KEEPALIVE idle delay — OS-level dead-peer probing independent of the // app ping, so a half-open TCP can't persist indefinitely. tcpKeepAliveMs: parseInt(process.env.TCP_KEEPALIVE_MS) || 20000, - maxFileSize: process.env.MAX_FILE_SIZE || 500 * 1024 * 1024, // 500MB + // Upload ceiling, #233. Accepts bytes or a suffix (MAX_FILE_SIZE=2GB). An env var is a + // string, so this must be parsed rather than used directly — multer's limits.fileSize wants a + // number, and an unparseable value falls back to the default rather than becoming NaN, which + // would reject every upload. NOTE: a reverse proxy caps the request body independently + // (nginx client_max_body_size, and any CDN in front) and returns 413 before the app is + // reached, so raising this alone is not enough — see the README. + maxFileSize: parseSize(process.env.MAX_FILE_SIZE, 500 * 1024 * 1024), // default 500MB thumbnailWidth: 320, screenshotQuality: 70, // SSL: drop your Cloudflare Origin cert + key in certs/ folder diff --git a/server/lib/parse-size.js b/server/lib/parse-size.js new file mode 100644 index 0000000..1fbaf8a --- /dev/null +++ b/server/lib/parse-size.js @@ -0,0 +1,37 @@ +'use strict'; + +// Parse a human-written size into bytes. +// +// Environment variables are always STRINGS. `process.env.MAX_FILE_SIZE || default` therefore +// yields the string "2000000000", which then travels into multer's limits.fileSize where a +// number is expected — it happens to survive some comparisons through coercion and misbehaves +// in others, which is the worst kind of bug to chase. +// +// A plain byte count is also unfriendly for this particular setting: the person raising an +// upload limit for video is choosing "about 2GB", and 2147483648 is easy to mistype by a +// factor of ten. So a suffix is accepted as well. +// +// Deliberately strict: anything unparseable returns the fallback rather than 0 or NaN. A typo +// in an env var must not silently become "reject every upload", which is what a NaN limit or a +// zero would do. + +const UNITS = { b: 1, kb: 1024, mb: 1024 ** 2, gb: 1024 ** 3, tb: 1024 ** 4 }; + +function parseSize(value, fallback) { + if (value == null || value === '') return fallback; + if (typeof value === 'number') return Number.isFinite(value) && value > 0 ? Math.floor(value) : fallback; + + const raw = String(value).trim().toLowerCase().replace(/\s+/g, ''); + // Bare digits are bytes, which is what the variable meant before suffixes were understood. + if (/^\d+$/.test(raw)) { + const n = Number(raw); + return n > 0 ? n : fallback; + } + const m = raw.match(/^(\d+(?:\.\d+)?)(b|kb|mb|gb|tb)$/); + if (!m) return fallback; + const n = parseFloat(m[1]); + if (!Number.isFinite(n) || n <= 0) return fallback; + return Math.floor(n * UNITS[m[2]]); +} + +module.exports = { parseSize }; diff --git a/server/test/max-file-size-config.test.js b/server/test/max-file-size-config.test.js new file mode 100644 index 0000000..c0dd1e6 --- /dev/null +++ b/server/test/max-file-size-config.test.js @@ -0,0 +1,76 @@ +'use strict'; + +// #233: the upload ceiling was hard-coded at 500MB, which is too low for video. Making it an +// env var is right, but env vars are STRINGS — `process.env.MAX_FILE_SIZE || default` hands +// multer's limits.fileSize a string where it wants a number. That survives some comparisons by +// coercion and misbehaves in others, which is the worst kind of bug to track down later. +// +// The other half is that a bad value must not become NaN or 0. Either would reject every upload +// on the instance, from a typo in an env file, with nothing on screen explaining why. Falling +// back to the documented default is the only safe reading of an unparseable limit. + +const { test } = require('node:test'); +const assert = require('node:assert/strict'); +const { parseSize } = require('../lib/parse-size'); + +const MB = 1024 * 1024; +const GB = 1024 * MB; +const DEFAULT = 500 * MB; + +test('THE BUG: the result is a number, never the raw string', () => { + const v = parseSize('2000000000', DEFAULT); + assert.equal(typeof v, 'number', 'multer wants a number'); + assert.equal(v, 2000000000); +}); + +test('a bare number is bytes, which is what the variable meant before suffixes', () => { + assert.equal(parseSize('1500000000', DEFAULT), 1500000000); + assert.equal(parseSize(1500000000, DEFAULT), 1500000000); +}); + +test('a suffix is understood, because nobody should have to type 2147483648', () => { + assert.equal(parseSize('2GB', DEFAULT), 2 * GB); + assert.equal(parseSize('2gb', DEFAULT), 2 * GB); + assert.equal(parseSize('1500MB', DEFAULT), 1500 * MB); + assert.equal(parseSize('750kb', DEFAULT), 750 * 1024); + assert.equal(parseSize('900b', DEFAULT), 900); +}); + +test('spacing and case do not matter', () => { + assert.equal(parseSize(' 2 GB ', DEFAULT), 2 * GB); + assert.equal(parseSize('2Gb', DEFAULT), 2 * GB); +}); + +test('a fractional size is allowed and floored to whole bytes', () => { + assert.equal(parseSize('1.5GB', DEFAULT), Math.floor(1.5 * GB)); + assert.equal(Number.isInteger(parseSize('1.5GB', DEFAULT)), true); +}); + +test('THE SAFETY RULE: an unparseable value falls back, it does not become NaN or 0', () => { + // A NaN or zero limit rejects every upload on the instance. A typo in an env file must not + // do that silently. + for (const bad of ['nonsense', '2 gigabytes', 'GB', '-5MB', '0', '0GB', '', ' ', null, undefined, NaN, {}]) { + const v = parseSize(bad, DEFAULT); + assert.equal(v, DEFAULT, `${JSON.stringify(bad)} should fall back`); + assert.ok(Number.isFinite(v) && v > 0); + } +}); + +test('the shipped default is unchanged at 500MB', () => { + // Raising the ceiling for everyone silently is not the point of this change. + delete process.env.MAX_FILE_SIZE; + delete require.cache[require.resolve('../config')]; + const config = require('../config'); + assert.equal(config.maxFileSize, DEFAULT); + assert.equal(typeof config.maxFileSize, 'number'); +}); + +test('config honours the env var end to end', () => { + process.env.MAX_FILE_SIZE = '3GB'; + delete require.cache[require.resolve('../config')]; + const config = require('../config'); + assert.equal(config.maxFileSize, 3 * GB); + assert.equal(typeof config.maxFileSize, 'number'); + delete process.env.MAX_FILE_SIZE; + delete require.cache[require.resolve('../config')]; +});