Skip to content

fix(preview): show previews on shares that forbid downloading - #51

Open
skjnldsv wants to merge 3 commits into
mainfrom
fix/preview-view-only-shares
Open

skjnldsv wants to merge 3 commits into
mainfrom
fix/preview-view-only-shares

Conversation

@skjnldsv

Copy link
Copy Markdown
Contributor

Picks up nextcloud/viewer#2484 by @susnux, open since September 2024. Both things it waited on landed long ago: nextcloud/viewer#2482 in September 2024 and nextcloud/server#47831 in October 2024. The server half is live on master today, in core/Controller/PreviewController.php:156 and apps/files_sharing/lib/Controller/PublicPreviewController.php:119, and published in the OpenAPI spec. The client half was never shipped, here or in the old app.

So on a share that forbids downloading, the viewer shows nothing: the element's request for the preview is refused, and the retry then asks for the file itself, which that share refuses too. Both attempts 403 and the user gets an empty frame.

The server offers exactly one way through, and calls it what it is:

// Is this header is set it means our UI is doing a preview for no-download shares
// we check a header so we at least prevent people from using the link directly (obfuscation)
$isNextcloudPreview = $this->request->getHeader('x-nc-preview') === 'true';

An element cannot put a header on its own request, so preloadPreview() fetches the bytes and hands back an object URL.

Why this is not a port of #2484

That PR makes getPreviewIfAny async, which pushes both Images.vue and Videos.vue from a computed into watcher-and-ref shapes, and most of its 13 files are refactors against the old app's layout (PreviewUrl.js, models.ts, BasicFileInfo) that this package restructured long ago.

Only the retry changes here. Images.vue already refetches by hand when the element fails to load, for E2EE files; that path now picks what to fetch based on whether the share allows the file to be downloaded. A file nobody restricted is still shown straight from the URL and costs no extra request, and no signature changes.

I also used an object URL rather than a FileReader data URL. Base64 costs a third more memory and puts the whole image inline in the DOM, and Promise.withResolvers then isn't needed either.

The leak that came with it

Object URLs made by the component are now revoked. The existing fallback never revoked the one it made, so paging through a folder of E2EE files held every decoded file in memory until the viewer closed. Adding a second producer without fixing that would have made it worse.

Covered

352 unit tests, 5 new over preloadPreview: the header, the response type, the abort signal, and that a failure reaches the caller rather than being swallowed.

48 e2e across both engines, one new. It stubs the preview endpoint to answer 403 unless the header is present, exactly as the server does, then asserts the picture ends up decoded on screen and that the endpoint saw [undefined, 'true']: refused as the element asked, then asked again properly. I reverted the fix to check the test earns its place, and it fails with the image hidden.

Note

Not reproduced against a live view-only share. The server behaviour is read from master and mirrored in the stub, so the thing I have actually proven is that the viewer now sends the header and recovers when refused.

hideDownload alone also routes through the retry, though the server would have served that one anyway. It costs one refused request the first time; splitting that from the real share attribute would mean duplicating the server's rule here, which seems worse.

👾 This pull request was assisted by Claude Code, commits carry an Assisted-by trailer.

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 <opensource@fthiessen.de>
Assisted-by: ClaudeCode:claude-opus-5
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
@codecov

codecov Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.22%. Comparing base (a640b9f) to head (17f290e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #51      +/-   ##
==========================================
+ Coverage   88.11%   88.22%   +0.10%     
==========================================
  Files          37       37              
  Lines        2625     2649      +24     
  Branches      558      562       +4     
==========================================
+ Hits         2313     2337      +24     
  Misses        305      305              
  Partials        7        7              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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 <skjnldsv@protonmail.com>
Both fail without the revocation they check for.

Assisted-by: ClaudeCode:claude-opus-5
Signed-off-by: skjnldsv <skjnldsv@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI assisted status: review Waiting for reviews type: bug 🐛 Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant