From 26e7c742ab31fa7a637f28018b2d6142d83908dc Mon Sep 17 00:00:00 2001 From: skjnldsv Date: Fri, 18 Sep 2026 14:23:56 +0200 Subject: [PATCH 1/3] fix(preview): show previews on shares that forbid downloading Since the download permission started being enforced, the preview endpoint refuses a plain request for a file the share will not hand over, and the viewer showed nothing at all: the element's request was refused, and the retry asked for the file itself, which that share refuses too. The server offers one way through. core/Controller/PreviewController.php and the public-share controller beside it serve the preview when the request carries `x-nc-preview: true`. An element cannot set a header on its own request, so the retry now fetches the preview by hand for a file that may not be downloaded, and hands the element an object URL. Only the retry changes, so a file nobody restricted is still shown from the URL directly and costs no extra request. The header is obfuscation rather than a boundary, which is what the server calls it: it keeps a preview URL from being useful when pasted elsewhere, and does not decide who may see the file. Object URLs made here are now revoked. The one the fallback already made never was, so paging through a folder of E2EE files held every one of them until the viewer closed. Co-authored-by: Ferdinand Thiessen Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: skjnldsv --- __tests__/mediaPreloader.spec.ts | 53 ++++++++++++++++++ e2e/previews.spec.ts | 36 ++++++++++++ lib/components/Images.vue | 39 ++++++++++++- lib/services/mediaPreloader.ts | 26 +++++++++ playground/App.vue | 3 + .../dav/files/playground/restricted.jpg | Bin 0 -> 4889 bytes 6 files changed, 155 insertions(+), 2 deletions(-) create mode 100644 __tests__/mediaPreloader.spec.ts create mode 100644 playground/public/remote.php/dav/files/playground/restricted.jpg diff --git a/__tests__/mediaPreloader.spec.ts b/__tests__/mediaPreloader.spec.ts new file mode 100644 index 0000000..e34efeb --- /dev/null +++ b/__tests__/mediaPreloader.spec.ts @@ -0,0 +1,53 @@ +/*! + * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors + * SPDX-License-Identifier: AGPL-3.0-or-later + */ +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const axiosGet = vi.hoisted(() => vi.fn()) +vi.mock('@nextcloud/axios', () => ({ default: { get: axiosGet } })) +vi.mock('@nextcloud/files/dav', () => ({ getClient: () => ({ getFileContents: vi.fn() }) })) + +const { preloadPreview } = await import('../lib/services/mediaPreloader.ts') + +describe('preloadPreview', () => { + beforeEach(() => { + axiosGet.mockReset() + axiosGet.mockResolvedValue({ data: new Blob(['x'], { type: 'image/jpeg' }) }) + vi.spyOn(URL, 'createObjectURL').mockReturnValue('blob:preview') + }) + + it('says the request comes from the viewer', async () => { + // Without this the server refuses the preview of a share that may + // not be downloaded, and the element has nothing to show + await preloadPreview('/core/preview?fileId=1') + + expect(axiosGet).toHaveBeenCalledWith( + '/core/preview?fileId=1', + expect.objectContaining({ headers: { 'x-nc-preview': 'true' } }), + ) + }) + + it('asks for the bytes rather than a parsed body', async () => { + await preloadPreview('/core/preview?fileId=1') + + expect(axiosGet.mock.calls[0]![1].responseType).toBe('blob') + }) + + it('hands back something an element can be pointed at', async () => { + expect(await preloadPreview('/core/preview?fileId=1')).toBe('blob:preview') + }) + + it('drops the request when the viewer moves on', async () => { + const controller = new AbortController() + await preloadPreview('/core/preview?fileId=1', controller.signal) + + expect(axiosGet.mock.calls[0]![1].signal).toBe(controller.signal) + }) + + it('lets a failure reach the caller, which reports it', async () => { + axiosGet.mockRejectedValue(new Error('forbidden')) + + await expect(preloadPreview('/core/preview?fileId=1')).rejects.toThrow('forbidden') + }) +}) diff --git a/e2e/previews.spec.ts b/e2e/previews.spec.ts index 8cbccd1..1b8d0a0 100644 --- a/e2e/previews.spec.ts +++ b/e2e/previews.spec.ts @@ -35,6 +35,42 @@ test.describe('Previews', () => { expect(url.searchParams.get('etag')).toBe('etag-2') }) + test('asks for the preview by hand when the share forbids downloading', async ({ page }) => { + // The server refuses a plain request for the preview of a share that + // cannot be downloaded, and serves it when the request says it comes + // from the viewer. An element cannot set that header on its own + // request, so a refusal here has to be answered by fetching it. + const headers: Array = [] + await page.route('**/core/preview*', async (route) => { + const header = route.request().headers()['x-nc-preview'] + headers.push(header) + if (header !== 'true') { + await route.fulfill({ status: 403, contentType: 'text/plain', body: 'Forbidden' }) + return + } + await route.fulfill({ contentType: 'image/jpeg', body: IMAGE }) + }) + + const viewer = new ViewerPage(page) + await viewer.open('restricted.jpg', 'previews') + await viewer.waitForOpen() + + // The picture is on screen, which it could not be without the retry + const image = viewer.container.locator('img').first() + await expect(image).toBeVisible() + await expect(async () => { + const decoded = await image.evaluate((element: HTMLImageElement) => ({ + complete: element.complete, + width: element.naturalWidth, + })) + expect(decoded.complete).toBe(true) + expect(decoded.width).toBeGreaterThan(0) + }).toPass({ timeout: 10_000 }) + + // Refused once as the element asked, then asked for again with the header + expect(headers).toEqual([undefined, 'true']) + }) + test('loads the file itself when there is no preview', async ({ page }) => { let asked = false await page.route('**/core/preview*', async (route) => { diff --git a/lib/components/Images.vue b/lib/components/Images.vue index 5e106fc..16e734c 100644 --- a/lib/components/Images.vue +++ b/lib/components/Images.vue @@ -81,7 +81,8 @@ import NcLoadingIcon from '@nextcloud/vue/components/NcLoadingIcon' import PlayCircleOutline from 'vue-material-design-icons/PlayCircleOutline.vue' import { useViewerProps } from '../composables/useViewerProps.ts' import { logger } from '../services/logger.ts' -import { preloadMedia } from '../services/mediaPreloader.ts' +import { preloadMedia, preloadPreview } from '../services/mediaPreloader.ts' +import { canDownload } from '../utils/canDownload.ts' import { t } from '../utils/l10n.ts' import { findLivePhotoPeerFromFileId } from '../utils/livePhotoUtils.ts' import { getPreviewIfAny } from '../utils/previewUtils.ts' @@ -192,6 +193,29 @@ const livePhotoSrc = computed(() => livePhoto.value?.encodedSource ?? null) */ let inFlight: { controller: AbortController, source: string } | null = null +/** + * The object URL this component made, if it made one. + * + * An object URL holds its blob until it is revoked, so paging through a + * folder of E2EE files or of previews fetched by hand would otherwise keep + * every one of them in memory for as long as the viewer is open. Only URLs + * made here are released: `localSource` belongs to whoever passed it. + */ +let ownedUrl: string | null = null + +/** + * Show a blob this component fetched, releasing the one it showed before. + * + * @param url an object URL made here + */ +function showOwnedUrl(url: string): void { + if (ownedUrl !== null) { + URL.revokeObjectURL(ownedUrl) + } + ownedUrl = url + data.value = url +} + // Load data when component mounts or file changes. Keyed on the source, as // two files can be shown under one name and it is the source that says // which bytes to fetch. @@ -254,7 +278,14 @@ async function loadData() { // use: an E2EE file, or a preview the server cannot produce. Fetch the // bytes by hand and show those instead. if (fallback.value) { - data.value = await preloadMedia(props.file, signal) + // Which retry depends on what the share allows. A file that may be + // downloaded is fetched whole, which is what an E2EE file needs. One + // that may not is refused at the source too, so its preview is the + // only thing left to ask for, and only with a header an element + // cannot set on its own request. + showOwnedUrl(canDownload(props.file) + ? await preloadMedia(props.file, signal) + : await preloadPreview(previewPath.value, signal)) return } @@ -346,6 +377,10 @@ watch(turns, () => { onUnmounted(() => { inFlight?.controller.abort() + if (ownedUrl !== null) { + URL.revokeObjectURL(ownedUrl) + ownedUrl = null + } }) /** diff --git a/lib/services/mediaPreloader.ts b/lib/services/mediaPreloader.ts index 688bc1b..1e58599 100644 --- a/lib/services/mediaPreloader.ts +++ b/lib/services/mediaPreloader.ts @@ -8,6 +8,7 @@ import type { IFile } from '@nextcloud/files' import type { ResponseDataDetailed, WebDAVClient } from 'webdav' +import axios from '@nextcloud/axios' import { getClient } from '@nextcloud/files/dav' // Manually load a WebDAV media from its filename, then expose the received Blob as an object URL. @@ -18,3 +19,28 @@ export async function preloadMedia(file: IFile, signal?: AbortSignal): Promise return URL.createObjectURL(new Blob([response.data], { type: response.headers['content-type'] })) } + +/** + * Fetch a preview the element cannot ask for itself. + * + * A share with download turned off has the preview endpoint refuse a plain + * request, and the server offers one way through: the `x-nc-preview` header + * (`core/Controller/PreviewController.php`, and the public-share controller + * beside it). An `img` element cannot set a header on its own request, so + * the bytes are fetched here and handed over as an object URL instead. + * + * The server calls this obfuscation rather than a boundary, and so should + * we: it is what keeps the preview URL from being useful when pasted + * elsewhere, not what decides who may see the file. + * + * @param url the preview URL to fetch + * @param signal aborts the request when the viewer moves to another file + */ +export async function preloadPreview(url: string, signal?: AbortSignal): Promise { + const response = await axios.get(url, { + headers: { 'x-nc-preview': 'true' }, + responseType: 'blob', + signal, + }) + return URL.createObjectURL(response.data as Blob) +} diff --git a/playground/App.vue b/playground/App.vue index 4a5c9b8..595f2ab 100644 --- a/playground/App.vue +++ b/playground/App.vue @@ -38,6 +38,9 @@ const startSlideshow = flags.has('slideshow') const fixtures: Fixture[] = [ { name: 'photo.jpg', mime: 'image/jpeg', editable: true }, ...(withPreviews ? [{ name: 'previewed.jpg', mime: 'image/jpeg', hasPreview: true }] : []), + // A share that forbids downloading: the preview endpoint refuses the + // element's own request, so the viewer has to ask for it by hand + ...(withPreviews ? [{ name: 'restricted.jpg', mime: 'image/jpeg', hasPreview: true, noDownload: true }] : []), { name: 'gradient.jpg', mime: 'image/jpeg', editable: true }, { name: 'portrait.jpg', mime: 'image/jpeg', editable: true }, { name: 'photo.avif', mime: 'image/avif', editable: true }, diff --git a/playground/public/remote.php/dav/files/playground/restricted.jpg b/playground/public/remote.php/dav/files/playground/restricted.jpg new file mode 100644 index 0000000000000000000000000000000000000000..bf1afe5b11d885fd0c31fc39ab67bd511aae8368 GIT binary patch literal 4889 zcmex=>ukC3pCfH06P05XITq?4J21E^7eo0A(TN+S4wfI*Oh z(Sh*+Gov5_lOQ9rAmjfd4B|kif*gwkSeQA05|RRp49uu9Lc)m)!FoViQ1vjfGXeF8 z3Ze-!FbMv?#lXYN2(*w{kinkenTFZ5I0p_!1_c&|1_34p>M-;9y#&529Bjnmsv}Di zy$(%n)#eHlS)&;dw1L*#z=$byu?OyEXECki{Mfmlr^|W zc*DTJdsONhAZG+#CMgyZjtqediVK__+8SxjHD8-0Iz?L}EMU#TNT*G$sa&UM%nfu+ zrHefX4{5BWQ7epTLm Date: Fri, 18 Sep 2026 14:30:54 +0200 Subject: [PATCH 2/3] test(preview): cover the restricted-share retry in the component The branch was only exercised end to end, so the unit run never entered it. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: skjnldsv --- __tests__/component/media.spec.ts | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/__tests__/component/media.spec.ts b/__tests__/component/media.spec.ts index 7fbb1e1..5dd5974 100644 --- a/__tests__/component/media.spec.ts +++ b/__tests__/component/media.spec.ts @@ -14,6 +14,7 @@ import { makeFile } from '../factories.ts' // media components perform, so we replace it with a deterministic fake blob URL. vi.mock('../../lib/services/mediaPreloader.ts', () => ({ preloadMedia: vi.fn(async () => 'blob:mock-preloaded-media'), + preloadPreview: vi.fn(async () => 'blob:mock-preloaded-preview'), })) // An svg is read and sanitized rather than handed to the element, so the @@ -93,9 +94,10 @@ import Images from '../../lib/components/Images.vue' import Videos from '../../lib/components/Videos.vue' import { usePlyrPlayer } from '../../lib/composables/usePlyrPlayer.ts' import { logger } from '../../lib/services/logger.ts' -import { preloadMedia } from '../../lib/services/mediaPreloader.ts' +import { preloadMedia, preloadPreview } from '../../lib/services/mediaPreloader.ts' const preloadMediaMock = vi.mocked(preloadMedia) +const preloadPreviewMock = vi.mocked(preloadPreview) /** * Build the full ViewerProps set with sensible defaults for a mounted media component. @@ -128,6 +130,7 @@ function mountImages(overrides: Partial = {}) { beforeEach(() => { preloadMediaMock.mockClear() + preloadPreviewMock.mockClear() }) describe('Images.vue', () => { @@ -184,6 +187,27 @@ describe('Images.vue', () => { expect(wrapper.emitted('errored')).toBeUndefined() }) + it('asks for the preview by hand when the share forbids downloading', async () => { + // The share refuses the file itself, so fetching it would fail the + // same way the element's own request just did. Only the preview is + // still available, and only to a request that carries the header. + const file = makeFile({ + basename: 'restricted.jpg', + attributes: { hasPreview: true, hideDownload: true }, + }) + const wrapper = mountImages({ file, files: [file] }) + await flushPromises() + + await wrapper.find('img').trigger('error') + await flushPromises() + + expect(preloadPreviewMock).toHaveBeenCalledTimes(1) + expect(preloadPreviewMock.mock.calls[0]![0]).toContain('/core/preview') + expect(preloadMediaMock).not.toHaveBeenCalled() + expect(wrapper.find('img').attributes('src')).toBe('blob:mock-preloaded-preview') + expect(wrapper.emitted('errored')).toBeUndefined() + }) + it('falls back for a file whose preview fails to load', async () => { const file = makeFile({ basename: 'previewed.jpg', attributes: { hasPreview: true } }) const wrapper = mountImages({ file, files: [file] }) From 17f290e55c7be0d6a4f8f061d25b83fc82000d26 Mon Sep 17 00:00:00 2001 From: skjnldsv Date: Fri, 18 Sep 2026 14:35:58 +0200 Subject: [PATCH 3/3] test(preview): cover the object URLs the component releases Both fail without the revocation they check for. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: skjnldsv --- __tests__/component/media.spec.ts | 38 +++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/__tests__/component/media.spec.ts b/__tests__/component/media.spec.ts index 5dd5974..e8e5a3c 100644 --- a/__tests__/component/media.spec.ts +++ b/__tests__/component/media.spec.ts @@ -208,6 +208,44 @@ describe('Images.vue', () => { expect(wrapper.emitted('errored')).toBeUndefined() }) + it('releases the blob it fetched when the viewer closes', async () => { + // An object URL holds its blob until it is revoked, so a folder of + // these would otherwise stay in memory for as long as the viewer is + // open + const revoke = vi.spyOn(URL, 'revokeObjectURL').mockImplementation(() => {}) + const file = makeFile({ basename: 'broken.jpg' }) + const wrapper = mountImages({ file, files: [file] }) + await flushPromises() + await wrapper.find('img').trigger('error') + await flushPromises() + + wrapper.unmount() + + expect(revoke).toHaveBeenCalledWith('blob:mock-preloaded-media') + revoke.mockRestore() + }) + + it('releases the previous blob when it fetches another', async () => { + const revoke = vi.spyOn(URL, 'revokeObjectURL').mockImplementation(() => {}) + const first = makeFile({ basename: 'first.jpg' }) + const second = makeFile({ basename: 'second.jpg' }) + const wrapper = mountImages({ file: first, files: [first, second] }) + await flushPromises() + await wrapper.find('img').trigger('error') + await flushPromises() + expect(revoke).not.toHaveBeenCalled() + + preloadMediaMock.mockResolvedValueOnce('blob:second-media') + await wrapper.setProps({ file: second }) + await flushPromises() + await wrapper.find('img').trigger('error') + await flushPromises() + + expect(revoke).toHaveBeenCalledWith('blob:mock-preloaded-media') + revoke.mockRestore() + wrapper.unmount() + }) + it('falls back for a file whose preview fails to load', async () => { const file = makeFile({ basename: 'previewed.jpg', attributes: { hasPreview: true } }) const wrapper = mountImages({ file, files: [file] })