Skip to content

fix(richdocuments): gate conversion with SecureViewService check#5635

Open
chrip wants to merge 1 commit intomainfrom
fix/richdocuments-secureview-conversion-bypass
Open

fix(richdocuments): gate conversion with SecureViewService check#5635
chrip wants to merge 1 commit intomainfrom
fix/richdocuments-secureview-conversion-bypass

Conversation

@chrip
Copy link
Copy Markdown
Contributor

@chrip chrip commented May 5, 2026

Server-side conversion bypassed the Secure View / watermark restriction that the viewer enforces, allowing a user with view-only secure access to download a clean copy via the conversion API. Reuse SecureViewService (same logic the viewer uses) to deny conversion for files that should be secured. Handle the documented NotFoundException so a cache miss surfaces as a clear, translated error instead of a 500.

  • Resolves: #
  • Target version: main

Summary

TODO

  • [ x] If the file is secured, the "Save as..." is not possible because this could bypass the security. We could hide the menu entry in this case. Would need changes in apps/files/src/actions/convertAction.ts

Checklist

  • Code is properly formatted
  • [x ] Sign-off message is added to all commits
  • Documentation (manuals or wiki) has been updated or is not required

Server-side conversion bypassed the Secure View / watermark restriction
that the viewer enforces, allowing a user with view-only secure access
to download a clean copy via the conversion API. Reuse SecureViewService
(same logic the viewer uses) to deny conversion for files that should be
secured. Handle the documented NotFoundException so a cache miss surfaces
as a clear, translated error instead of a 500.

Signed-off-by: Christoph Schaefer <christoph.schaefer@nextcloud.com>
try {
$secured = $this->secureViewService->shouldSecure(
$file->getInternalPath(),
$file->getStorage(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your change succeeds in blocking the conversion, but the error message given in the log is not the same. Looks like passing false to shouldSecure() allows it to take the correct branch, else you get a ForbiddenException:

Suggested change
$file->getStorage(),
$file->getStorage(),
false,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants