fix: single-photo gallery downloads 404 on S3 storage backends (#1048)
* fix(gallery): route single-photo downloads through the storage backend The route resolved a local filesystem path unconditionally and handed it to res.sendFile. On an S3/R2 deployment managed photos are never on local disk, so every per-photo download failed — while download-all and secure-images worked, because they already went through getStorage(). That asymmetry is why it went unnoticed: the gallery looks healthy until a guest clicks the download button on one photo. Measured rather than assumed: because sendFile is called WITH a callback, Express does not send a response when the file is missing and the callback only logs. The request does not 404, it hangs until the client gives up. The new tests pin this — all five backend-path cases time out against the previous implementation. Two existing pieces do the work, so this mostly deletes code: - renderPhotoForDownload (#858) already owns resize-then-watermark ordering and the storage fetch, and the zip builders in this same file already use it. The inline duplicate of that logic goes. - the pass-through case branches on storage.kind(). Local disk keeps res.sendFile: it emits Content-Length, Accept-Ranges, ETag and Last-Modified and answers Range with a 206, and sharing one bare stream.pipe(res) with S3 would silently drop all of it — a resumed download would append a second full body onto the partial file. On S3 the parts that matter for a download are reproduced via stat() and getRange(). Ranges are parsed defensively; an unchecked parse yields NaN bounds and a 206 with a nonsense Content-Range, which corrupts a resumed download rather than failing it. Malformed or unsatisfiable ranges fall back to a 200. The pre-stream 404s now run before any image header is staged, so the error goes out as JSON instead of a .jpg attachment containing JSON. Co-authored-by: peipeimo <peipeimo@users.noreply.github.com> * fix(gallery): open the stream before staging download headers, honour If-Range Both from an external review round on this PR. stat() succeeding does not mean get() will — a concurrent delete or replace, or a transient backend error, lands between them. The fetch was awaited AFTER the headers went out, so: - the range branch had already called writeHead(206), leaving the outer catch nothing to do but throw ERR_HTTP_HEADERS_SENT. In practice the request hangs: the new regression test sat for the full 120s jest timeout against the previous code instead of returning. - the full branch would have sent its 500 JSON underneath the staged image/jpeg attachment headers — a .jpg file full of JSON, which is the exact failure this PR set out to stop doing on the 404 paths. Opening the stream first also lets a vanished object answer 404 and a transient failure answer 500, instead of both surfacing as a broken body. If-Range: emitting Last-Modified without honouring the validator built from it is the dangerous half of the feature. A client resuming after the object was replaced — the watcher re-importing a swapped file, an admin re-upload — would get 206 from the NEW bytes and splice two versions into one corrupt file. A validator that does not match now falls back to a full 200. 4 new tests; 3 of them fail against the previous commit, the fourth is the matching-validator control that must keep returning 206. * fix(gallery): HEAD without egress, classify render failures, stage 206 headers Round-2 findings from the external reviewer. Express routes HEAD through this GET handler and Node discards the body, but the pipe still drains the whole object out of S3 first — a metadata probe from a download manager cost a full transfer in egress and latency. Everything a HEAD needs is already in stat(). renderPhotoForDownload rejections were all reported as 404. It can equally fail because getToFile timed out, tmp filled up, or sharp died; calling that "photo not found" misleads the guest and hides the incident from us. Now classified the same way the pass-through branch already does. The 206 path uses status()+set() instead of writeHead(). writeHead commits the response immediately, so a stream that resolved and then errored before its first chunk left pipeStreamToResponse able only to destroy the connection. Staged headers flush on the first body write, so an error at byte zero now returns a clean retryable status with keep-alive intact. Credit to the reviewer for the correction — I had assumed deferring the commit required buffering. Writing the test for that surfaced one more: pipeStreamToResponse cleared Content-Type, Content-Length, ETag and Content-Disposition but not the range headers, so the 500 went out still advertising Content-Range: bytes 0-9/40 — telling a resuming client the error body IS the partial content. Not taken: binding response metadata to a fetched object version. That needs an ETag/versionId on the storage abstraction and conditional GETs in both adapters; the reviewer agreed it belongs in its own PR rather than blocking this one. Backend suites: 485 passed. * fix(gallery): answer HEAD before the counters and the render Round-3 finding. The HEAD short-circuit was inside the storage branch, which sits below both the download_count increment / access_logs insert and renderPhotoForDownload — so a download manager's metadata probe was recorded as a real download, and on a watermarked or resized gallery it also pulled the original from S3 and ran sharp over it to build a body Node then throws away. HEAD now leaves the handler right after the access checks, with no side effects and no bytes read. Content-Length is included only when the photo ships untransformed and the size is readable from stat(); a watermark or resize changes the length and the only way to learn the new one is to do the work this branch exists to avoid. HEAD may omit it. Not taken, again: binding the read to the statted object version. The reviewer already agreed in a follow-up that it needs an ETag/versionId on the storage abstraction plus conditional GETs in both adapters, and belongs in its own PR. Re-raising it does not change that. Tests assert the probe moves neither download_count nor access_logs. --------- Co-authored-by: Paul Nothaft <paul@MacStudio-von-Paul.local> Co-authored-by: peipeimo <peipeimo@users.noreply.github.com>
This commit is contained in:
peipeimo
@@ -0,0 +1,278 @@
|
||||
/**
|
||||
* Single-photo gallery downloads must go through the storage backend (#1048).
|
||||
*
|
||||
* `GET /api/gallery/:slug/download/:photoId` resolved a LOCAL filesystem path
|
||||
* unconditionally and handed it to res.sendFile. On an S3/R2 deployment
|
||||
* managed photos never exist on local disk, so every per-photo download 404'd
|
||||
* with ENOENT — while download-all and secure-images worked fine, because they
|
||||
* already went through getStorage(). The gallery looks healthy until a guest
|
||||
* clicks the download button on a single photo.
|
||||
*
|
||||
* The local branch is pinned just as hard: sendFile emits Content-Length,
|
||||
* Accept-Ranges, ETag and Last-Modified and answers Range with a 206. Routing
|
||||
* local installs through a bare stream.pipe(res) to share one code path would
|
||||
* silently drop all of that, and a resumed download would append a second full
|
||||
* body onto the partial file.
|
||||
*/
|
||||
|
||||
const path = require('path');
|
||||
const fs = require('fs');
|
||||
const os = require('os');
|
||||
|
||||
process.env.NODE_ENV = 'test';
|
||||
process.env.TEST_DATABASE_PATH = path.join(
|
||||
fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-dl-')), 'db.sqlite',
|
||||
);
|
||||
process.env.JWT_SECRET = process.env.JWT_SECRET || 'download-test-secret';
|
||||
process.env.STORAGE_PATH = fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-dl-storage-'));
|
||||
|
||||
const { Readable } = require('stream');
|
||||
|
||||
const SLUG = 'download-gallery';
|
||||
const FILENAME = 'original.jpg';
|
||||
// Deliberately not written to disk anywhere: if the route reads the
|
||||
// filesystem instead of the backend, it cannot produce these bytes.
|
||||
const mockObjectBody = Buffer.from('S3-ONLY-ORIGINAL-BYTES-not-on-local-disk');
|
||||
const mockBackendKind = { value: 's3' };
|
||||
|
||||
const mockStorage = {
|
||||
kind: () => mockBackendKind.value,
|
||||
stat: jest.fn(async () => ({ size: mockObjectBody.length, mtime: new Date('2026-08-20T10:00:00Z') })),
|
||||
get: jest.fn(async () => Readable.from([mockObjectBody])),
|
||||
getRange: jest.fn(async (key, start, end) => Readable.from([mockObjectBody.subarray(start, end + 1)])),
|
||||
delete: jest.fn(async () => undefined),
|
||||
exists: jest.fn(async () => true),
|
||||
};
|
||||
|
||||
jest.mock('../../src/services/storage', () => ({
|
||||
getStorage: () => mockStorage,
|
||||
initStorage: async () => mockStorage,
|
||||
}));
|
||||
|
||||
const request = require('supertest');
|
||||
const express = require('express');
|
||||
const cookieParser = require('cookie-parser');
|
||||
const { bootCrmDb, seedMinimal } = require('../integration/helpers/crmDb');
|
||||
|
||||
describe('single-photo download through the storage backend (#1048)', () => {
|
||||
let db; let cleanup; let app; let eventId; let photoId;
|
||||
|
||||
beforeAll(async () => {
|
||||
({ db, cleanup } = await bootCrmDb());
|
||||
await seedMinimal(db);
|
||||
|
||||
const ev = await db('events').insert({
|
||||
slug: SLUG,
|
||||
event_type: 'wedding',
|
||||
event_name: 'Downloads',
|
||||
event_date: '2026-08-01',
|
||||
host_email: 'h@example.com',
|
||||
admin_email: 'a@example.com',
|
||||
password_hash: 'x',
|
||||
share_link: `/gallery/${SLUG}/s`,
|
||||
share_token: 'download-share',
|
||||
expires_at: new Date(Date.now() + 7 * 864e5).toISOString(),
|
||||
is_active: 1,
|
||||
is_archived: 0,
|
||||
is_draft: 0,
|
||||
require_password: 0,
|
||||
allow_downloads: 1,
|
||||
created_at: new Date().toISOString(),
|
||||
}).returning('id');
|
||||
eventId = ev[0]?.id ?? ev[0];
|
||||
|
||||
const row = await db('photos').insert({
|
||||
event_id: eventId,
|
||||
filename: FILENAME,
|
||||
path: `${SLUG}/${FILENAME}`,
|
||||
type: 'individual',
|
||||
source_origin: 'managed',
|
||||
mime_type: 'image/jpeg',
|
||||
uploaded_at: new Date().toISOString(),
|
||||
}).returning('id');
|
||||
photoId = row[0]?.id ?? row[0];
|
||||
|
||||
app = express();
|
||||
app.use(express.json());
|
||||
app.use(cookieParser());
|
||||
app.use('/api/gallery', require('../../src/routes/gallery'));
|
||||
}, 120000);
|
||||
|
||||
afterAll(async () => { if (cleanup) await cleanup(); });
|
||||
|
||||
beforeEach(() => {
|
||||
mockBackendKind.value = 's3';
|
||||
mockStorage.get.mockClear();
|
||||
mockStorage.getRange.mockClear();
|
||||
});
|
||||
|
||||
it('streams the stored object instead of 404ing on a local path', async () => {
|
||||
const res = await request(app)
|
||||
.get(`/api/gallery/${SLUG}/download/${photoId}`)
|
||||
.buffer(true)
|
||||
.parse((response, cb) => {
|
||||
const chunks = [];
|
||||
response.on('data', (c) => chunks.push(c));
|
||||
response.on('end', () => cb(null, Buffer.concat(chunks)));
|
||||
});
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
// The bytes only exist in the backend — proof it did not read the disk.
|
||||
expect(res.body.equals(mockObjectBody)).toBe(true);
|
||||
expect(mockStorage.get).toHaveBeenCalledWith(`events/active/${SLUG}/${FILENAME}`);
|
||||
// Never written locally, so a filesystem read could not have served this.
|
||||
expect(fs.existsSync(path.join(process.env.STORAGE_PATH, 'events/active', SLUG, FILENAME))).toBe(false);
|
||||
});
|
||||
|
||||
it('sends Content-Length so the browser can show download progress', async () => {
|
||||
const res = await request(app).get(`/api/gallery/${SLUG}/download/${photoId}`);
|
||||
|
||||
expect(res.headers['content-length']).toBe(String(mockObjectBody.length));
|
||||
expect(res.headers['accept-ranges']).toBe('bytes');
|
||||
expect(res.headers['content-disposition']).toContain(FILENAME);
|
||||
});
|
||||
|
||||
it('answers a Range request with 206 and only the requested bytes', async () => {
|
||||
const res = await request(app)
|
||||
.get(`/api/gallery/${SLUG}/download/${photoId}`)
|
||||
.set('Range', 'bytes=0-9')
|
||||
.buffer(true)
|
||||
.parse((response, cb) => {
|
||||
const chunks = [];
|
||||
response.on('data', (c) => chunks.push(c));
|
||||
response.on('end', () => cb(null, Buffer.concat(chunks)));
|
||||
});
|
||||
|
||||
expect(res.status).toBe(206);
|
||||
expect(res.headers['content-range']).toBe(`bytes 0-9/${mockObjectBody.length}`);
|
||||
expect(res.headers['content-length']).toBe('10');
|
||||
expect(res.body.equals(mockObjectBody.subarray(0, 10))).toBe(true);
|
||||
expect(mockStorage.getRange).toHaveBeenCalledWith(`events/active/${SLUG}/${FILENAME}`, 0, 9);
|
||||
});
|
||||
|
||||
it('ignores a malformed Range rather than emitting a nonsense 206', async () => {
|
||||
const res = await request(app)
|
||||
.get(`/api/gallery/${SLUG}/download/${photoId}`)
|
||||
.set('Range', 'bytes=abc-def');
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.headers['content-range']).toBeUndefined();
|
||||
});
|
||||
|
||||
it('404s cleanly when the object is missing from the backend', async () => {
|
||||
mockStorage.stat.mockResolvedValueOnce(null);
|
||||
|
||||
const res = await request(app).get(`/api/gallery/${SLUG}/download/${photoId}`);
|
||||
|
||||
expect(res.status).toBe(404);
|
||||
// The error must not inherit the image headers staged for a successful
|
||||
// download, or the browser saves a .jpg containing JSON.
|
||||
expect(res.headers['content-type']).toMatch(/json/);
|
||||
expect(res.headers['content-disposition']).toBeUndefined();
|
||||
});
|
||||
|
||||
it('keeps res.sendFile on a local backend rather than a bare pipe', async () => {
|
||||
mockBackendKind.value = 'local';
|
||||
const abs = path.join(process.env.STORAGE_PATH, 'events/active', SLUG, FILENAME);
|
||||
fs.mkdirSync(path.dirname(abs), { recursive: true });
|
||||
fs.writeFileSync(abs, 'local-disk-bytes');
|
||||
|
||||
const res = await request(app).get(`/api/gallery/${SLUG}/download/${photoId}`);
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(mockStorage.get).not.toHaveBeenCalled();
|
||||
// sendFile's signature: conditional-request headers a raw pipe never sets.
|
||||
expect(res.headers.etag).toBeDefined();
|
||||
expect(res.headers['last-modified']).toBeDefined();
|
||||
|
||||
fs.rmSync(abs, { force: true });
|
||||
});
|
||||
|
||||
it('does not serve a partial body when the If-Range validator is stale', async () => {
|
||||
// The object was replaced since the client's last attempt. Answering 206
|
||||
// from the new bytes would let it splice two versions into one file.
|
||||
const res = await request(app)
|
||||
.get(`/api/gallery/${SLUG}/download/${photoId}`)
|
||||
.set('Range', 'bytes=0-9')
|
||||
.set('If-Range', new Date('2020-01-01T00:00:00Z').toUTCString());
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.headers['content-range']).toBeUndefined();
|
||||
expect(res.headers['content-length']).toBe(String(mockObjectBody.length));
|
||||
});
|
||||
|
||||
it('still serves 206 when the If-Range validator matches', async () => {
|
||||
const res = await request(app)
|
||||
.get(`/api/gallery/${SLUG}/download/${photoId}`)
|
||||
.set('Range', 'bytes=0-9')
|
||||
.set('If-Range', new Date('2026-08-20T10:00:00Z').toUTCString());
|
||||
|
||||
expect(res.status).toBe(206);
|
||||
expect(res.headers['content-range']).toBe(`bytes 0-9/${mockObjectBody.length}`);
|
||||
});
|
||||
|
||||
it('errors cleanly when the object vanishes between stat and get', async () => {
|
||||
// HeadObject succeeding does not mean GetObject will — a concurrent
|
||||
// delete lands here. The staged image headers must not escape with it.
|
||||
const gone = new Error('NoSuchKey');
|
||||
gone.name = 'NoSuchKey';
|
||||
mockStorage.get.mockRejectedValueOnce(gone);
|
||||
|
||||
const res = await request(app).get(`/api/gallery/${SLUG}/download/${photoId}`);
|
||||
|
||||
expect(res.status).toBe(404);
|
||||
expect(res.headers['content-type']).toMatch(/json/);
|
||||
expect(res.headers['content-disposition']).toBeUndefined();
|
||||
});
|
||||
|
||||
it('does not send 206 headers before the range fetch can fail', async () => {
|
||||
// writeHead(206) before the await would make this ERR_HTTP_HEADERS_SENT.
|
||||
mockStorage.getRange.mockRejectedValueOnce(new Error('connection reset'));
|
||||
|
||||
const res = await request(app)
|
||||
.get(`/api/gallery/${SLUG}/download/${photoId}`)
|
||||
.set('Range', 'bytes=0-9');
|
||||
|
||||
expect(res.status).toBe(500);
|
||||
expect(res.headers['content-type']).toMatch(/json/);
|
||||
expect(res.headers['content-range']).toBeUndefined();
|
||||
});
|
||||
|
||||
it('answers HEAD from stat instead of draining the object out of S3', async () => {
|
||||
const before = (await db('photos').where('id', photoId).first()).download_count || 0;
|
||||
const logsBefore = (await db('access_logs').where({ photo_id: photoId, action: 'download' })).length;
|
||||
|
||||
const res = await request(app).head(`/api/gallery/${SLUG}/download/${photoId}`);
|
||||
|
||||
expect(res.status).toBe(200);
|
||||
expect(res.headers['content-length']).toBe(String(mockObjectBody.length));
|
||||
expect(res.headers['accept-ranges']).toBe('bytes');
|
||||
// The whole point: no egress for a metadata probe.
|
||||
expect(mockStorage.get).not.toHaveBeenCalled();
|
||||
expect(mockStorage.getRange).not.toHaveBeenCalled();
|
||||
|
||||
// And no side effects: a probe is not a download.
|
||||
const after = (await db('photos').where('id', photoId).first()).download_count || 0;
|
||||
expect(after).toBe(before);
|
||||
const logsAfter = (await db('access_logs').where({ photo_id: photoId, action: 'download' })).length;
|
||||
expect(logsAfter).toBe(logsBefore);
|
||||
});
|
||||
|
||||
it('returns a clean error when the range stream dies before its first chunk', async () => {
|
||||
// Resolves, then errors — writeHead would already have committed the 206,
|
||||
// leaving a connection reset as the only possible outcome.
|
||||
const { Readable: R } = require('stream');
|
||||
mockStorage.getRange.mockImplementationOnce(async () => {
|
||||
const dead = new R({ read() { this.destroy(new Error('socket hang up')); } });
|
||||
return dead;
|
||||
});
|
||||
|
||||
const res = await request(app)
|
||||
.get(`/api/gallery/${SLUG}/download/${photoId}`)
|
||||
.set('Range', 'bytes=0-9');
|
||||
|
||||
expect(res.status).toBe(500);
|
||||
expect(res.headers['content-type']).toMatch(/json/);
|
||||
expect(res.headers['content-range']).toBeUndefined();
|
||||
});
|
||||
});
|
||||
+262
-70
@@ -35,16 +35,16 @@ const { COLOR_LABELS, SHARED_COLOR_LABEL_IDENTITY } = require('../constants/colo
|
||||
const secureImageService = require('../services/secureImageService');
|
||||
const logger = require('../utils/logger');
|
||||
const { pipeStreamToResponse } = require('../utils/streamResponse');
|
||||
const { resolvePhotoFilePath } = require('../services/photoResolver');
|
||||
const { resolvePhotoFilePath, resolvePhotoStorageKey } = require('../services/photoResolver');
|
||||
const { getEventCategoriesOrdered } = require('../utils/categoryOrder');
|
||||
const { getEventShareToken, resolveShareIdentifier, buildShareLinkVariants } = require('../services/shareLinkService');
|
||||
const { handleAsync, errorResponse } = require('../utils/routeHelpers');
|
||||
const { isGalleryHidden, guestBlockedByReveal, blockHiddenGallery } = require('../utils/revealMode');
|
||||
const { toIso } = require('../utils/dateNormalize');
|
||||
const { NotFoundError } = require('../utils/errors');
|
||||
const { ensureThumbnail, ensureHeroImage, ensurePreviewImage, withLocalCopy, resizeToBox } = require('../services/imageProcessor');
|
||||
const { ensureThumbnail, ensureHeroImage, ensurePreviewImage, withLocalCopy } = require('../services/imageProcessor');
|
||||
const downloadZipService = require('../services/downloadZipService');
|
||||
const { renderPhotoForDownload } = require('../services/downloadRendition');
|
||||
const { renderPhotoForDownload, resolveWatermarkSettings } = require('../services/downloadRendition');
|
||||
const downloadJobService = require('../services/downloadJobService');
|
||||
// Download resolutions (#858) — the standard size a gallery hands out, plus
|
||||
// validation of any guest-picked override.
|
||||
@@ -89,6 +89,42 @@ const fs = require('fs');
|
||||
// Get storage path from environment or default
|
||||
const getStoragePath = () => process.env.STORAGE_PATH || path.join(__dirname, '../../../storage');
|
||||
|
||||
/**
|
||||
* Parse a single-range `Range: bytes=` header against a known size.
|
||||
*
|
||||
* Returns null for absent, malformed, multi-range or unsatisfiable headers —
|
||||
* every one of which the caller answers with a normal 200 full body, which is
|
||||
* what a client that sent an unparseable range would get today anyway.
|
||||
* Validating matters because an unchecked parse yields NaN bounds and a 206
|
||||
* with a nonsense Content-Range, which corrupts a resumed download rather
|
||||
* than merely failing it.
|
||||
*/
|
||||
function parseByteRange(header, size) {
|
||||
if (!header || typeof header !== 'string' || !size) return null;
|
||||
const match = /^bytes=(\d*)-(\d*)$/.exec(header.trim());
|
||||
if (!match) return null;
|
||||
|
||||
const [, rawStart, rawEnd] = match;
|
||||
if (rawStart === '' && rawEnd === '') return null;
|
||||
|
||||
let start;
|
||||
let end;
|
||||
if (rawStart === '') {
|
||||
// Suffix form: the last N bytes.
|
||||
const suffix = parseInt(rawEnd, 10);
|
||||
if (!suffix) return null;
|
||||
start = Math.max(0, size - suffix);
|
||||
end = size - 1;
|
||||
} else {
|
||||
start = parseInt(rawStart, 10);
|
||||
end = rawEnd === '' ? size - 1 : parseInt(rawEnd, 10);
|
||||
}
|
||||
|
||||
if (!Number.isFinite(start) || !Number.isFinite(end)) return null;
|
||||
if (start > end || start >= size) return null;
|
||||
return { start, end: Math.min(end, size - 1) };
|
||||
}
|
||||
|
||||
// "Gallery opened" for the admin notification bell (#746). The photo-list
|
||||
// endpoint fires on every gallery page load, so notifying per hit would spam
|
||||
// the bell — debounce to at most one notification per event per window. The
|
||||
@@ -1548,6 +1584,44 @@ router.get('/:slug/download/:photoId', verifyGalleryAccess, denySlideshowToken,
|
||||
}
|
||||
const box = isVideo ? null : parseResolution(requested);
|
||||
|
||||
// A HEAD is a metadata probe, not a download. Answering it below the
|
||||
// counters recorded every probe as a real download, and answering it below
|
||||
// renderPhotoForDownload fetched and watermarked an image whose body Node
|
||||
// then discards. Both happen before this point in a GET, so HEAD leaves
|
||||
// here — with no side effects and no bytes read.
|
||||
if (req.method === 'HEAD') {
|
||||
const headUseOriginal = await getUseOriginalFilenames();
|
||||
const headHeaders = {
|
||||
'Content-Type': photo.mime_type || 'image/jpeg',
|
||||
'Content-Disposition': buildContentDisposition(pickRawDownloadName(photo, headUseOriginal)),
|
||||
'Accept-Ranges': 'bytes',
|
||||
};
|
||||
|
||||
// Content-Length only when the bytes ship untransformed AND the size can
|
||||
// be read without fetching them. A watermark or resize changes the
|
||||
// length, and the only way to learn the new one is to do the work this
|
||||
// branch exists to avoid — HEAD is allowed to omit it.
|
||||
const headWatermark = await resolveWatermarkSettings(req.event);
|
||||
if (!box && !headWatermark) {
|
||||
try {
|
||||
const headKey = resolvePhotoStorageKey(req.event, photo);
|
||||
const headStorage = getStorage();
|
||||
if (headKey && headStorage.kind() !== 'local') {
|
||||
const headStat = await headStorage.stat(headKey);
|
||||
if (!headStat) return res.status(404).json({ error: 'Photo file not found' });
|
||||
headHeaders['Content-Length'] = headStat.size;
|
||||
if (headStat.mtime) headHeaders['Last-Modified'] = new Date(headStat.mtime).toUTCString();
|
||||
}
|
||||
} catch (headErr) {
|
||||
// No length is a valid HEAD; not worth failing the probe over.
|
||||
logger.debug('HEAD probe could not stat the object', { photoId, error: headErr.message });
|
||||
}
|
||||
}
|
||||
|
||||
res.set(headHeaders);
|
||||
return res.end();
|
||||
}
|
||||
|
||||
// Admin preview (#868) downloads are excluded from the download count +
|
||||
// guest analytics — kept out of client-facing stats.
|
||||
if (!req.isAdminPreview) {
|
||||
@@ -1571,6 +1645,174 @@ router.get('/:slug/download/:photoId', verifyGalleryAccess, denySlideshowToken,
|
||||
if (res.statusCode < 400 && !req.isAdminPreview) notifySinglePhotoDownload(req.event, req);
|
||||
});
|
||||
|
||||
// #493: if the admin enabled "use original filenames", surface the
|
||||
// pre-rename camera filename in Content-Disposition. Storage path is
|
||||
// unchanged — only the user-visible download name is swapped.
|
||||
const useOriginal = await getUseOriginalFilenames();
|
||||
const downloadName = pickRawDownloadName(photo, useOriginal);
|
||||
const contentDisposition = buildContentDisposition(downloadName);
|
||||
|
||||
// The gallery's standard applies to EVERY ordinary download, single photos
|
||||
// included — otherwise a lowered standard is trivially bypassed by
|
||||
// downloading photos one at a time. `box` was resolved above, before the
|
||||
// counters. Videos have no resize path and always ship as-is.
|
||||
//
|
||||
// renderPhotoForDownload (#858) owns the resize-then-watermark ordering
|
||||
// and the storage fetch, and is what the zip builders below already use.
|
||||
// It returns null when the photo needs no transformation at all, which is
|
||||
// the default gallery's common case and lets us ship the stored bytes
|
||||
// without buffering a full-size original into memory.
|
||||
const effectiveSettings = await resolveWatermarkSettings(req.event);
|
||||
|
||||
let rendered;
|
||||
try {
|
||||
rendered = await renderPhotoForDownload(req.event, photo, box, effectiveSettings);
|
||||
} catch (renderError) {
|
||||
// Classify, the same way the pass-through branch below does. This can
|
||||
// reject because the source object is gone, but equally because
|
||||
// getToFile timed out, the tmp filesystem filled up, or sharp failed —
|
||||
// and reporting an operational failure as 404 tells the guest their
|
||||
// photo does not exist and tells us nothing.
|
||||
const gone = renderError.code === 'ENOENT'
|
||||
|| renderError.name === 'NoSuchKey'
|
||||
|| renderError.name === 'NotFound'
|
||||
|| renderError.$metadata?.httpStatusCode === 404;
|
||||
logger.error('Failed to render photo for download', {
|
||||
slug: req.params.slug,
|
||||
photoId,
|
||||
eventId: req.event.id,
|
||||
error: renderError.message,
|
||||
});
|
||||
return gone
|
||||
? res.status(404).json({ error: 'Photo file not found' })
|
||||
: res.status(500).json({ error: 'Failed to download photo' });
|
||||
}
|
||||
|
||||
if (rendered) {
|
||||
res.set({
|
||||
'Content-Type': photo.mime_type || 'image/jpeg',
|
||||
'Content-Disposition': contentDisposition,
|
||||
'Content-Length': rendered.length
|
||||
});
|
||||
|
||||
return res.send(rendered);
|
||||
}
|
||||
|
||||
// Untransformed: ship the stored bytes.
|
||||
//
|
||||
// Managed photos live behind the storage abstraction and on an S3/R2
|
||||
// deployment are not on local disk at all — resolving a filesystem path
|
||||
// unconditionally here is what made every single-photo download 404 with
|
||||
// ENOENT in S3 mode (#1048), while download-all and secure-images worked
|
||||
// because they already went through getStorage().
|
||||
//
|
||||
// resolvePhotoStorageKey returns null for external/reference photos: those
|
||||
// live on a local mount and keep the sendFile path.
|
||||
let storageKey = null;
|
||||
try {
|
||||
storageKey = resolvePhotoStorageKey(req.event, photo);
|
||||
} catch (resolveError) {
|
||||
logger.error('Failed to resolve photo storage key for download', {
|
||||
slug: req.params.slug,
|
||||
photoId,
|
||||
eventId: req.event.id,
|
||||
error: resolveError.message,
|
||||
});
|
||||
return res.status(404).json({ error: 'Photo file not found' });
|
||||
}
|
||||
|
||||
const storage = getStorage();
|
||||
if (storageKey && storage.kind() !== 'local') {
|
||||
// Deliberately NOT the local path: res.sendFile emits Content-Length,
|
||||
// Accept-Ranges, ETag and Last-Modified and answers Range requests with
|
||||
// a 206, and a bare stream.pipe(res) has none of that. On local disk
|
||||
// sendFile stays the better implementation, so it stays the branch.
|
||||
//
|
||||
// On S3 we reproduce the parts that matter for a download: the length
|
||||
// (browsers need it for the progress indicator, which matters most on
|
||||
// exactly the large files this route serves) and Range, so an
|
||||
// interrupted download resumes instead of appending a second full body
|
||||
// onto the partial file. Conditional requests are not reproduced —
|
||||
// there is no ETag here, so a client revalidating gets the whole body,
|
||||
// same as it does today.
|
||||
const stat = await storage.stat(storageKey);
|
||||
if (!stat) {
|
||||
logger.error('Photo not found in storage backend for download', {
|
||||
slug: req.params.slug,
|
||||
photoId,
|
||||
eventId: req.event.id,
|
||||
storageKey,
|
||||
});
|
||||
return res.status(404).json({ error: 'Photo file not found' });
|
||||
}
|
||||
|
||||
const lastModified = stat.mtime ? new Date(stat.mtime).toUTCString() : null;
|
||||
const headers = {
|
||||
'Content-Type': photo.mime_type || 'image/jpeg',
|
||||
'Content-Disposition': contentDisposition,
|
||||
'Accept-Ranges': 'bytes',
|
||||
};
|
||||
if (lastModified) headers['Last-Modified'] = lastModified;
|
||||
|
||||
// If-Range: a client resuming an interrupted download sends back the
|
||||
// validator it was given last time. If the object has been replaced
|
||||
// since — the watcher re-importing a swapped file, an admin re-upload —
|
||||
// answering 206 from the NEW bytes lets the client splice two different
|
||||
// versions into one corrupt file. A validator that doesn't match means
|
||||
// a full 200, which is the whole point of the header.
|
||||
const ifRange = req.headers['if-range'];
|
||||
const staleValidator = !!ifRange && (!lastModified || ifRange.trim() !== lastModified);
|
||||
const range = staleValidator ? null : parseByteRange(req.headers.range, stat.size);
|
||||
|
||||
// Open the stream BEFORE any header is staged or sent. stat() succeeding
|
||||
// does not mean get() will: a concurrent delete or replace, or a
|
||||
// transient backend error, lands here. Once writeHead(206) has gone out
|
||||
// the outer catch can do nothing but throw ERR_HTTP_HEADERS_SENT, and in
|
||||
// the non-range case it would send its 500 JSON underneath the staged
|
||||
// image/jpeg attachment headers — a .jpg file full of JSON.
|
||||
let stream;
|
||||
try {
|
||||
stream = range
|
||||
? await storage.getRange(storageKey, range.start, range.end)
|
||||
: await storage.get(storageKey);
|
||||
} catch (fetchError) {
|
||||
const gone = fetchError.code === 'ENOENT'
|
||||
|| fetchError.name === 'NoSuchKey'
|
||||
|| fetchError.name === 'NotFound'
|
||||
|| fetchError.$metadata?.httpStatusCode === 404;
|
||||
logger.error('Failed to open photo stream for download', {
|
||||
slug: req.params.slug,
|
||||
photoId,
|
||||
eventId: req.event.id,
|
||||
storageKey,
|
||||
error: fetchError.message,
|
||||
});
|
||||
return gone
|
||||
? res.status(404).json({ error: 'Photo file not found' })
|
||||
: res.status(500).json({ error: 'Failed to download photo' });
|
||||
}
|
||||
|
||||
if (range) {
|
||||
// status()+set() rather than writeHead(): writeHead commits the
|
||||
// response immediately, so a stream that resolves and THEN errors
|
||||
// before its first chunk would leave pipeStreamToResponse able only to
|
||||
// destroy the connection. Staged headers are flushed by the first body
|
||||
// write, which means an error at byte zero can still clear them and
|
||||
// return a clean, retryable status instead of a transport reset.
|
||||
res.status(206).set({
|
||||
...headers,
|
||||
'Content-Range': `bytes ${range.start}-${range.end}/${stat.size}`,
|
||||
'Content-Length': (range.end - range.start) + 1,
|
||||
});
|
||||
} else {
|
||||
res.set({ ...headers, 'Content-Length': stat.size });
|
||||
}
|
||||
pipeStreamToResponse(stream, res, {
|
||||
context: range ? `download range for photo ${photo.id}` : `download for photo ${photo.id}`,
|
||||
});
|
||||
return;
|
||||
}
|
||||
|
||||
let filePath;
|
||||
try {
|
||||
filePath = resolvePhotoFilePath(req.event, photo);
|
||||
@@ -1583,75 +1825,25 @@ router.get('/:slug/download/:photoId', verifyGalleryAccess, denySlideshowToken,
|
||||
});
|
||||
return res.status(404).json({ error: 'Photo file not found' });
|
||||
}
|
||||
|
||||
// Get watermark settings - apply if global setting OR event-level setting is enabled
|
||||
const watermarkSettings = await watermarkService.getWatermarkSettings();
|
||||
const eventWatermarkEnabled = req.event.watermark_downloads === true || req.event.watermark_downloads === 1;
|
||||
const shouldApplyWatermark = (watermarkSettings && watermarkSettings.enabled) || eventWatermarkEnabled;
|
||||
|
||||
// #493: if the admin enabled "use original filenames", surface the
|
||||
// pre-rename camera filename in Content-Disposition. Storage path is
|
||||
// unchanged — only the user-visible download name is swapped.
|
||||
const useOriginal = await getUseOriginalFilenames();
|
||||
const downloadName = pickRawDownloadName(photo, useOriginal);
|
||||
const contentDisposition = buildContentDisposition(downloadName);
|
||||
|
||||
// The gallery's standard applies to EVERY ordinary download, single photos
|
||||
// included — otherwise a lowered standard is trivially bypassed by
|
||||
// downloading photos one at a time. `box` was resolved above, before the
|
||||
// counters. Videos have no resize path and always ship as-is.
|
||||
if (shouldApplyWatermark || box) {
|
||||
// Resize BEFORE watermarking: applyWatermark sizes the mark relative to
|
||||
// its input's width, so watermarking the original and then shrinking
|
||||
// would resample the mark and waste work on discarded pixels.
|
||||
//
|
||||
// With no resize (the default 'original' standard) hand applyWatermark
|
||||
// the PATH, not a buffer: buffer inputs deliberately skip its cache, so
|
||||
// buffering here would re-run sharp over the full-size original on every
|
||||
// download and regress the pre-#858 watermark performance.
|
||||
const effectiveSettings = shouldApplyWatermark ? {
|
||||
...watermarkSettings,
|
||||
enabled: true,
|
||||
text: req.event.watermark_text || watermarkSettings?.text || 'Protected'
|
||||
} : null;
|
||||
|
||||
let buffer;
|
||||
if (!box) {
|
||||
buffer = await watermarkService.applyWatermark(filePath, effectiveSettings);
|
||||
} else {
|
||||
buffer = await resizeToBox(await fs.promises.readFile(filePath), box);
|
||||
if (shouldApplyWatermark) {
|
||||
buffer = await watermarkService.applyWatermark(buffer, effectiveSettings);
|
||||
}
|
||||
// res.download() builds Content-Disposition itself but doesn't emit the
|
||||
// RFC 5987 filename* parameter, so unicode camera filenames would lose
|
||||
// their bytes on download. Set the header explicitly and stream the
|
||||
// file with res.sendFile-equivalent semantics.
|
||||
res.set({
|
||||
'Content-Type': photo.mime_type || 'image/jpeg',
|
||||
'Content-Disposition': contentDisposition,
|
||||
});
|
||||
res.sendFile(filePath, (downloadError) => {
|
||||
if (downloadError) {
|
||||
logger.error('Error streaming gallery download', {
|
||||
slug: req.params.slug,
|
||||
photoId,
|
||||
eventId: req.event.id,
|
||||
error: downloadError.message,
|
||||
});
|
||||
}
|
||||
|
||||
res.set({
|
||||
'Content-Type': photo.mime_type || 'image/jpeg',
|
||||
'Content-Disposition': contentDisposition,
|
||||
'Content-Length': buffer.length
|
||||
});
|
||||
|
||||
res.send(buffer);
|
||||
} else {
|
||||
// res.download() builds Content-Disposition itself but doesn't emit the
|
||||
// RFC 5987 filename* parameter, so unicode camera filenames would lose
|
||||
// their bytes on download. Set the header explicitly and stream the
|
||||
// file with res.sendFile-equivalent semantics.
|
||||
res.set({
|
||||
'Content-Type': photo.mime_type || 'image/jpeg',
|
||||
'Content-Disposition': contentDisposition,
|
||||
});
|
||||
res.sendFile(filePath, (downloadError) => {
|
||||
if (downloadError) {
|
||||
logger.error('Error streaming gallery download', {
|
||||
slug: req.params.slug,
|
||||
photoId,
|
||||
eventId: req.event.id,
|
||||
error: downloadError.message,
|
||||
});
|
||||
}
|
||||
});
|
||||
}
|
||||
});
|
||||
} catch (error) {
|
||||
errorResponse(res, error, 500, 'Failed to download photo');
|
||||
}
|
||||
|
||||
@@ -55,6 +55,11 @@ function pipeStreamToResponse(stream, res, options = {}) {
|
||||
res.removeHeader('ETag');
|
||||
res.removeHeader('Content-Type');
|
||||
res.removeHeader('Content-Disposition');
|
||||
// Range headers describe the body that is no longer coming. Left behind,
|
||||
// a 500 goes out still advertising `Content-Range: bytes 0-9/40`, which
|
||||
// tells a resuming client the error response IS the partial content.
|
||||
res.removeHeader('Content-Range');
|
||||
res.removeHeader('Accept-Ranges');
|
||||
res.setHeader('Cache-Control', 'no-store');
|
||||
|
||||
if (gone) {
|
||||
|
||||
Reference in New Issue
Block a user