From d3eaf375caa2319b31c6856d6fbe26b254436cfa Mon Sep 17 00:00:00 2001 From: Benjy Date: Sat, 26 Sep 2026 20:08:36 +0200 Subject: [PATCH] One Range parser, and a path that leaves the volume is a refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `routes/files/preview.js` and `routes/shares.js` each carried their own copy of the byte-range parsing. Two copies of anything drift, and these had: the preview route sets security headers on a partial response that the share route does not, so the same file came back with different headers depending on which address it was asked for. `utils/httpRange.js` does it once. Both routes call it, and neither parses a Range header any more. Found while porting the preview route, and fixed here because it is that route's own test that shows it: `normalizeRelativePath` threw a plain `Error` for a path that leaves the volume, and a plain Error reaches the browser as a 500. So `../../../etc/passwd` answered "500 Internal Server Error" — a server fault, and a status a caller retries — for something that is the request's fault and will never succeed. It is a `ValidationError` now, which is a 400. ## Checks `http-range.test.js`, 10 tests over the parser itself: a header that is not `bytes=`, a start past the end, an open-ended range, a suffix range, a size of zero. Making a malformed header parse as an absent one turns one of them red. `preview.test.js`, 20 tests, including the two path-traversal ones that were the 500. Whole backend suite: 2 476 passed, 2 failed — the two that fail on `main` on its own. `download.js` was in this batch at first and is not any more: its zip building uses a newer `archiver` than `main` has, which is a dependency question and belongs to the batch that answers it. It parses no Range header, so nothing of this subject is left behind there. --- backend/src/routes/files/preview.js | 82 +++++--- backend/src/routes/shares.js | 38 ++-- backend/src/utils/httpRange.js | 52 ++++++ backend/src/utils/pathUtils.js | 5 +- backend/tests/routes/preview.test.js | 249 +++++++++++++++++++++++++ backend/tests/utils/http-range.test.js | 60 ++++++ 6 files changed, 436 insertions(+), 50 deletions(-) create mode 100644 backend/src/utils/httpRange.js create mode 100644 backend/tests/routes/preview.test.js create mode 100644 backend/tests/utils/http-range.test.js diff --git a/backend/src/routes/files/preview.js b/backend/src/routes/files/preview.js index 95489ae3c..228866661 100644 --- a/backend/src/routes/files/preview.js +++ b/backend/src/routes/files/preview.js @@ -2,6 +2,7 @@ const path = require('path'); const fs = require('fs/promises'); const fss = require('fs'); const { normalizeRelativePath } = require('../../utils/pathUtils'); +const { parseByteRange } = require('../../utils/httpRange'); const { resolvePathWithAccess } = require('../../services/accessManager'); const { extensions, mimeTypes } = require('../../config/index'); const { getRawPreviewJpegPath } = require('../../services/rawPreviewService'); @@ -9,12 +10,31 @@ const asyncHandler = require('../../utils/asyncHandler'); const { ValidationError, ForbiddenError, + NotFoundError, UnsupportedMediaTypeError, } = require('../../errors/AppError'); const logger = require('../../utils/logger'); +const { markLongPoll } = require('../../middleware/heldRequests'); const router = require('express').Router(); +// Formats the browser executes when opened as a top-level document. Served +// inline they would run their own scripts on the application origin, so they +// get a sandbox CSP — which still lets an render them normally. +const ACTIVE_CONTENT_EXTENSIONS = new Set(['svg']); + +const buildPreviewSecurityHeaders = (extension) => { + const headers = { + // Never let the browser second-guess the declared type. + 'X-Content-Type-Options': 'nosniff', + 'X-Robots-Tag': 'noindex', + }; + if (ACTIVE_CONTENT_EXTENSIONS.has(extension)) { + headers['Content-Security-Policy'] = 'sandbox'; + } + return headers; +}; + router.get( '/preview', asyncHandler(async (req, res) => { @@ -32,7 +52,17 @@ router.get( } const { absolutePath } = resolved; - const stats = await fs.stat(absolutePath); + let stats; + try { + stats = await fs.stat(absolutePath); + } catch (error) { + // A file deleted or renamed since the listing was drawn: a stale view, + // not a fault in the server. + if (error.code === 'ENOENT') { + throw new NotFoundError('File not found.'); + } + throw error; + } if (stats.isDirectory()) { throw new ValidationError('Cannot preview a directory.'); @@ -54,6 +84,7 @@ router.get( res.writeHead(200, { 'Content-Type': 'image/jpeg', 'Content-Length': jpegStats.size, + ...buildPreviewSecurityHeaders('jpeg'), }); const stream = fss.createReadStream(jpegPath); @@ -74,6 +105,7 @@ router.get( } const mimeType = mimeTypes[extension] || 'application/octet-stream'; + const securityHeaders = buildPreviewSecurityHeaders(extension); const isSeekableMedia = extensions.videos.includes(extension) || (extensions.audios || []).includes(extension); @@ -93,34 +125,32 @@ router.get( }; if (isSeekableMedia) { - const rangeHeader = req.headers.range; - if (rangeHeader) { - const bytesPrefix = 'bytes='; - if (!rangeHeader.startsWith(bytesPrefix)) { - res.status(416).send('Malformed Range header'); - return; - } - - const [startString, endString] = rangeHeader.slice(bytesPrefix.length).split('-'); - let start = Number(startString); - let end = endString ? Number(endString) : stats.size - 1; - - if (Number.isNaN(start)) start = 0; - if (Number.isNaN(end) || end >= stats.size) end = stats.size - 1; - - if (start > end) { - res.status(416).send('Range Not Satisfiable'); - return; - } - - const chunkSize = end - start + 1; + // Streaming a film holds the connection open for as long as the browser + // wants it — minutes, and longer over a slow link. That is the request + // doing its job, not a symptom, and the held-request instrument reports + // only ten before falling silent for the life of the process: a handful + // of videos would spend the whole budget and switch off the one tool + // there is for finding a genuinely stuck server. + markLongPoll(req); + + const range = parseByteRange(req.headers.range, stats.size); + if (range?.malformed) { + res.status(416).send('Malformed Range header'); + return; + } + if (range?.unsatisfiable) { + res.status(416).send('Range Not Satisfiable'); + return; + } + if (range) { res.writeHead(206, { - 'Content-Range': `bytes ${start}-${end}/${stats.size}`, + 'Content-Range': `bytes ${range.start}-${range.end}/${stats.size}`, 'Accept-Ranges': 'bytes', - 'Content-Length': chunkSize, + 'Content-Length': range.chunkSize, 'Content-Type': mimeType, + ...securityHeaders, }); - streamFile({ start, end }); + streamFile({ start: range.start, end: range.end }); return; } @@ -128,6 +158,7 @@ router.get( 'Content-Type': mimeType, 'Content-Length': stats.size, 'Accept-Ranges': 'bytes', + ...securityHeaders, }); streamFile(); return; @@ -136,6 +167,7 @@ router.get( res.writeHead(200, { 'Content-Type': mimeType, 'Content-Length': stats.size, + ...securityHeaders, }); streamFile(); }) diff --git a/backend/src/routes/shares.js b/backend/src/routes/shares.js index 0024b25cc..69a44b5b7 100644 --- a/backend/src/routes/shares.js +++ b/backend/src/routes/shares.js @@ -1,4 +1,5 @@ const express = require('express'); +const { parseByteRange } = require('../utils/httpRange'); const fs = require('fs/promises'); const fss = require('fs'); const path = require('path'); @@ -253,34 +254,23 @@ const streamResolvedFile = async ({ absolutePath, stats, mode, req, res }) => { 'X-Robots-Tag': 'noindex', }; - const rangeHeader = req.headers.range; - if (rangeHeader) { - const bytesPrefix = 'bytes='; - if (!rangeHeader.startsWith(bytesPrefix)) { - res.status(416).send('Malformed Range header'); - return; - } - - const [startString, endString] = rangeHeader.slice(bytesPrefix.length).split('-'); - let start = Number(startString); - let end = endString ? Number(endString) : stats.size - 1; - - if (Number.isNaN(start)) start = 0; - if (Number.isNaN(end) || end >= stats.size) end = stats.size - 1; - - if (start > end) { - res.status(416).send('Range Not Satisfiable'); - return; - } - - const chunkSize = end - start + 1; + const range = parseByteRange(req.headers.range, stats.size); + if (range?.malformed) { + res.status(416).send('Malformed Range header'); + return; + } + if (range?.unsatisfiable) { + res.status(416).send('Range Not Satisfiable'); + return; + } + if (range) { res.writeHead(206, { ...baseHeaders, - 'Content-Range': `bytes ${start}-${end}/${stats.size}`, + 'Content-Range': `bytes ${range.start}-${range.end}/${stats.size}`, 'Accept-Ranges': 'bytes', - 'Content-Length': chunkSize, + 'Content-Length': range.chunkSize, }); - streamFile({ start, end }); + streamFile({ start: range.start, end: range.end }); return; } diff --git a/backend/src/utils/httpRange.js b/backend/src/utils/httpRange.js new file mode 100644 index 000000000..c74b161fd --- /dev/null +++ b/backend/src/utils/httpRange.js @@ -0,0 +1,52 @@ +/** + * Byte-range parsing for the routes that stream a file. + * + * The share download route and the preview route each carried their own copy + * of this, which is how one of them ended up serving SVG without the headers + * the other set. One implementation means one place to fix. + */ + +/** + * Interpret a Range header against a known file size. + * + * Returns `null` when the header is absent (send the whole file), or + * `{ malformed: true }` / `{ unsatisfiable: true }` for a 416 — the caller + * decides how to answer, since it owns the response. + */ +const parseByteRange = (rangeHeader, size) => { + if (!rangeHeader) return null; + + const bytesPrefix = 'bytes='; + if (!String(rangeHeader).startsWith(bytesPrefix)) { + return { malformed: true }; + } + + const [startString, endString] = String(rangeHeader).slice(bytesPrefix.length).split('-'); + + // "bytes=-500" asks for the last 500 bytes, not the first 501. Reading the + // empty start as 0 turned every suffix request into a request for the head + // of the file — silently wrong, since the response still looks valid. + if (startString === '' && endString !== '' && endString !== undefined) { + const suffixLength = Number(endString); + if (Number.isNaN(suffixLength)) return { malformed: true }; + // An empty file has no last N bytes, and a zero-length suffix asks for + // nothing: both would otherwise produce end = -1 and a bogus Content-Range. + if (suffixLength === 0 || size === 0) return { unsatisfiable: true }; + const start = Math.max(0, size - suffixLength); + return { start, end: size - 1, chunkSize: size - start }; + } + + let start = Number(startString); + let end = endString ? Number(endString) : size - 1; + + if (Number.isNaN(start)) start = 0; + if (Number.isNaN(end) || end >= size) end = size - 1; + + if (start > end) { + return { unsatisfiable: true }; + } + + return { start, end, chunkSize: end - start + 1 }; +}; + +module.exports = { parseByteRange }; diff --git a/backend/src/utils/pathUtils.js b/backend/src/utils/pathUtils.js index a653b5d54..158e77a3c 100644 --- a/backend/src/utils/pathUtils.js +++ b/backend/src/utils/pathUtils.js @@ -83,7 +83,10 @@ const normalizeRelativePath = (relativePath = '') => { } if (normalized === '..' || normalized.startsWith('..' + path.sep)) { - throw new Error('Invalid path. Traversal outside the volume root is not allowed.'); + // The request's fault, not the server's: a plain Error reached the browser as a + // 500, so a path that leaves the volume read as a server fault rather than a + // refusal — and a 500 is what a caller retries. + throw new ValidationError('Invalid path. Traversal outside the volume root is not allowed.'); } return normalized; diff --git a/backend/tests/routes/preview.test.js b/backend/tests/routes/preview.test.js new file mode 100644 index 000000000..d0521181f --- /dev/null +++ b/backend/tests/routes/preview.test.js @@ -0,0 +1,249 @@ +import { afterEach, describe, expect, it } from 'vitest'; +import path from 'node:path'; +import fs from 'node:fs/promises'; +import request from 'supertest'; + +import { createTestApp, setupTestEnv } from '../helpers/env-test-utils.js'; + +/** + * /api/preview hands a file's bytes to the browser, inline, on the + * application's own origin. Every image, film and PDF the explorer shows goes + * through it, and nothing tested it: not who may read what, not the headers + * that keep an SVG from running script, not the byte ranges a video player + * seeks with. + */ + +let ctx; + +afterEach(async () => { + if (ctx) { + await ctx.cleanup(); + ctx = null; + } +}); + +// A hundred bytes whose values are their own offsets, so a range can be checked +// by content and not only by length. +const OFFSETS = Buffer.from(Array.from({ length: 100 }, (_, i) => i)); +const LEAK_MARKER = 'content-that-must-never-leave-the-disk-7f3a91'; + +const setup = async ({ user = { id: 'owner', roles: ['admin'] }, env = {} } = {}) => { + ctx = await setupTestEnv({ tag: 'preview-route-', env }); + + await fs.writeFile(path.join(ctx.volumeDir, 'photo.png'), OFFSETS); + await fs.writeFile(path.join(ctx.volumeDir, 'film.mp4'), OFFSETS); + await fs.writeFile( + path.join(ctx.volumeDir, 'drawing.svg'), + '' + ); + await fs.writeFile(path.join(ctx.volumeDir, 'notes.txt'), 'plain text'); + await fs.writeFile(path.join(ctx.volumeDir, 'camera.nef'), 'not really a raw file'); + await fs.mkdir(path.join(ctx.volumeDir, 'Album')); + + const router = ctx.requireFresh('src/routes/files/preview'); + const { errorHandler } = ctx.requireFresh('src/middleware/errorHandler'); + return createTestApp({ router, mountPath: '/api', user, errorHandler }); +}; + +const preview = (app, file) => request(app).get('/api/preview').query({ path: file }); + +/** supertest buffers images and video; anything else it may leave as text. */ +const bytesOf = (response) => + Buffer.isBuffer(response.body) ? response.body : Buffer.from(response.text ?? '', 'binary'); + +describe('a file that can be previewed', () => { + it('is sent whole, with its type and its length', async () => { + const response = await preview(await setup(), 'photo.png').buffer(true); + + expect(response.status).toBe(200); + expect(response.headers['content-type']).toBe('image/png'); + expect(response.headers['content-length']).toBe('100'); + expect(bytesOf(response).equals(OFFSETS)).toBe(true); + }); + + it('tells the browser not to guess another type', async () => { + const response = await preview(await setup(), 'photo.png'); + + expect(response.headers['x-content-type-options']).toBe('nosniff'); + }); +}); + +/** + * An SVG opened on its own is a document, and a document runs its scripts — + * on this origin, with this origin's session. The sandbox CSP stops that while + * leaving an free to draw it. + */ +describe('an SVG', () => { + it('is sandboxed, so a script inside it cannot run on this origin', async () => { + const response = await preview(await setup(), 'drawing.svg'); + + expect(response.status).toBe(200); + expect(response.headers['content-security-policy']).toBe('sandbox'); + expect(response.headers['x-content-type-options']).toBe('nosniff'); + }); + + it('is the only kind sandboxed: an ordinary image is not', async () => { + const response = await preview(await setup(), 'photo.png'); + + expect(response.headers['content-security-policy']).toBeUndefined(); + }); +}); + +describe('what cannot be previewed', () => { + it('is refused without a path', async () => { + const response = await request(await setup()).get('/api/preview'); + + expect(response.status).toBe(400); + }); + + it('is refused for a directory', async () => { + const response = await preview(await setup(), 'Album'); + + expect(response.status).toBe(400); + }); + + it('is refused for a type the viewer has no use for, as unsupported', async () => { + const response = await preview(await setup(), 'notes.txt'); + + expect(response.status).toBe(415); + }); + + /** + * A preview asked for a file that has gone — deleted in another tab, renamed + * by someone else — is the ordinary case of a stale listing, not a fault in + * the server. + */ + it('answers not found for a file that is not there, rather than a server error', async () => { + const response = await preview(await setup(), 'gone.png'); + + expect(response.status).toBe(404); + }); + + it('answers unsupported for a RAW file whose preview cannot be extracted', async () => { + const response = await preview(await setup(), 'camera.nef'); + + expect(response.status).toBe(415); + }, 30_000); +}); + +describe('a path that leaves the volume', () => { + it.each([['../../../etc/passwd'], ['..%2F..%2Fetc%2Fpasswd'], ['Album/../../outside.png']])( + 'is not served: %s', + async (file) => { + const app = await setup(); + await fs.writeFile(path.join(path.dirname(ctx.volumeDir), 'outside.png'), LEAK_MARKER); + + const response = await preview(app, file); + + expect(response.status).toBeGreaterThanOrEqual(400); + expect(response.status).toBeLessThan(500); + // A marker of its own, not a word: the refusal message itself says the + // path is "outside the volume root", and that is fine to say. + expect(bytesOf(response).toString()).not.toContain(LEAK_MARKER); + expect(bytesOf(response).toString()).not.toContain('root:'); + } + ); +}); + +/** + * A video player never asks for the whole film: it asks for the next few + * megabytes, and for a point further on when someone drags the playhead. + */ +describe('a film, read in pieces', () => { + it('says it accepts ranges when sent whole', async () => { + const response = await preview(await setup(), 'film.mp4').buffer(true); + + expect(response.status).toBe(200); + expect(response.headers['accept-ranges']).toBe('bytes'); + expect(response.headers['content-type']).toBe('video/mp4'); + }); + + it('sends exactly the bytes a range asks for', async () => { + const response = await preview(await setup(), 'film.mp4') + .set('Range', 'bytes=10-19') + .buffer(true); + + expect(response.status).toBe(206); + expect(response.headers['content-range']).toBe('bytes 10-19/100'); + expect(response.headers['content-length']).toBe('10'); + expect([...bytesOf(response)]).toEqual([10, 11, 12, 13, 14, 15, 16, 17, 18, 19]); + }); + + it('sends the last bytes for a suffix range, not the first', async () => { + const response = await preview(await setup(), 'film.mp4') + .set('Range', 'bytes=-5') + .buffer(true); + + expect(response.status).toBe(206); + expect(response.headers['content-range']).toBe('bytes 95-99/100'); + expect([...bytesOf(response)]).toEqual([95, 96, 97, 98, 99]); + }); + + it('refuses a range that starts past the end', async () => { + const response = await preview(await setup(), 'film.mp4').set('Range', 'bytes=200-300'); + + expect(response.status).toBe(416); + }); + + it('refuses a range in a unit it does not speak', async () => { + const response = await preview(await setup(), 'film.mp4').set('Range', 'items=0-1'); + + expect(response.status).toBe(416); + }); + + it('keeps the sandbox headers on a partial answer too', async () => { + const response = await preview(await setup(), 'film.mp4').set('Range', 'bytes=0-1'); + + expect(response.headers['x-content-type-options']).toBe('nosniff'); + }); +}); + +describe('an image, which is not read in pieces', () => { + it('is sent whole even when a range is asked for', async () => { + const response = await preview(await setup(), 'photo.png') + .set('Range', 'bytes=10-19') + .buffer(true); + + expect(response.status).toBe(200); + expect(bytesOf(response).length).toBe(100); + expect(response.headers['accept-ranges']).toBeUndefined(); + }); +}); + +/** + * Personal folders sit under the volume, at `_users/`. The preview reads + * bytes, so it is exactly the route through which one account could look at + * another's photographs if the access check were missing. The file is a PNG on + * purpose: previewable, so without the check this would be a 200. + */ +describe("another account's personal folder", () => { + const setupTwoAccounts = async () => { + const bob = { id: 'bob', username: 'bob', roles: ['user'] }; + const app = await setup({ user: bob, env: { USER_DIR_ENABLED: 'true' } }); + + const { resolvePersonalPath } = ctx.requireFresh('src/utils/pathUtils'); + const { getDb } = ctx.requireFresh('src/services/db'); + const db = await getDb(); + const now = new Date().toISOString(); + for (const id of ['alice', 'bob']) { + db.prepare( + `INSERT INTO users (id, email, email_verified, username, display_name, roles, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)` + ).run(id, `${id}@example.com`, 1, id, id, '["user"]', now, now); + } + const aliceRoot = await resolvePersonalPath('', { id: 'alice', username: 'alice' }); + await fs.mkdir(aliceRoot, { recursive: true }); + await fs.writeFile(path.join(aliceRoot, 'holiday.png'), 'alice private picture'); + + return { app, aliceFolder: path.basename(aliceRoot) }; + }; + + it('cannot be previewed through the volume', async () => { + const { app, aliceFolder } = await setupTwoAccounts(); + + const response = await preview(app, `_users/${aliceFolder}/holiday.png`); + + expect(response.status).toBe(403); + expect(bytesOf(response).toString()).not.toContain('alice private picture'); + }); +}); diff --git a/backend/tests/utils/http-range.test.js b/backend/tests/utils/http-range.test.js new file mode 100644 index 000000000..dba305219 --- /dev/null +++ b/backend/tests/utils/http-range.test.js @@ -0,0 +1,60 @@ +import { describe, it, expect } from 'vitest'; +import { parseByteRange } from '../../src/utils/httpRange.js'; + +/** + * Two routes used to parse ranges with their own copy of this logic. Pinning + * the behaviour here means the shared one cannot drift. + */ +describe('parseByteRange', () => { + it('returns null when no range was requested', () => { + expect(parseByteRange(undefined, 1000)).toBeNull(); + expect(parseByteRange('', 1000)).toBeNull(); + }); + + it('reads an explicit range', () => { + expect(parseByteRange('bytes=0-99', 1000)).toEqual({ start: 0, end: 99, chunkSize: 100 }); + expect(parseByteRange('bytes=500-999', 1000)).toEqual({ start: 500, end: 999, chunkSize: 500 }); + }); + + it('treats an open end as "to the last byte"', () => { + expect(parseByteRange('bytes=900-', 1000)).toEqual({ start: 900, end: 999, chunkSize: 100 }); + }); + + it('clamps an end past the file size', () => { + expect(parseByteRange('bytes=0-99999', 1000)).toEqual({ start: 0, end: 999, chunkSize: 1000 }); + }); + + it('flags a header that is not a byte range', () => { + expect(parseByteRange('items=0-99', 1000)).toEqual({ malformed: true }); + }); + + it('flags a range that cannot be satisfied', () => { + expect(parseByteRange('bytes=900-100', 1000)).toEqual({ unsatisfiable: true }); + }); +}); + +describe('Suffix ranges', () => { + it('serves the last N bytes, not the first N', () => { + // A media player asking for the tail of a file used to receive the head, + // with a 206 and a plausible Content-Range to match. + expect(parseByteRange('bytes=-500', 2000)).toEqual({ + start: 1500, + end: 1999, + chunkSize: 500, + }); + }); + + it('clamps a suffix longer than the file to the whole file', () => { + expect(parseByteRange('bytes=-5000', 100)).toEqual({ start: 0, end: 99, chunkSize: 100 }); + }); + + it('rejects a zero-length suffix', () => { + expect(parseByteRange('bytes=-0', 100)).toEqual({ unsatisfiable: true }); + }); + + it('rejects a suffix of an empty file', () => { + // Otherwise end lands on -1 and the response advertises a range that + // cannot exist. + expect(parseByteRange('bytes=-500', 0)).toEqual({ unsatisfiable: true }); + }); +});