fix(files_sharing): validate input in PublicPreviewController#getPreview

[mvanhorn]

Summary

Fixes two cases where PublicPreviewController::getPreview() triggers an
internal server error due to incomplete input validation:

Case A - Empty $file parameter on folder shares:
$node->get('') returns the Folder itself, then getPreview() fails because
it expects a File. Now returns 400 Bad Request early.

Case B - Non-existent file with mimeFallback=1:
When $node->get($file) throws NotFoundException, the catch block called
$file->getMimeType() on the original string parameter. Renamed the local
variable to $fileNode and added an isset() guard so the mime fallback
only runs when the file node was successfully resolved.

Fixes #59229

[joshtrichards]

Hi @mvanhorn - thanks for your bug fix!

Hope you don’t mind, but while this was being fixed I took the liberty of adding a few directly related housekeeping improvements to your PR/branch:

• clarified the controller docblock
• renamed the internal preview target variables a bit further for readability (your original change was already heading in the right direction)
• added regression tests for the reported edge cases
  • I also cleaned up the existing test calls to make the getPreview(..., $a, $mimeFallback) flag usage explicit, since the vaguely named 5th parameter ($a) makes it easy to confuse which boolean is being set¹

Footnotes

1. I considered renaming $a for clarity, but since this is a public controller method, the parameter name is part of the externally consumed request/API surface. I’ve left the name unchanged here to avoid any compatibility risk. ↩

[joshtrichards]

/backport to stable33

[joshtrichards]

/backport to stable32

[mvanhorn]

Thanks for the improvements and backports, @joshtrichards. The variable renames and docblock clarification look great.

[kesselb]

Thanks a lot 🙏

[Copilot]

Pull request overview

Fixes input-validation edge cases in the public preview endpoint to avoid internal server errors when previewing public folder shares, aligning behavior with expected 4xx responses and supporting optional MIME icon fallback.

Changes:

• Add early 400 responses for invalid folder-share $file inputs (empty path or resolving to a folder) in PublicPreviewController::getPreview().
• Fix MIME fallback handling so it only runs when a file node was successfully resolved (avoids calling file methods on a string / unresolved node).
• Extend controller tests to cover empty $file handling and MIME-fallback redirect/not-found behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
apps/files_sharing/lib/Controller/PublicPreviewController.php	Adds stricter validation around resolving $file for folder shares and guards MIME fallback to prevent 500s.
apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php	Updates existing calls for the new parameter and adds new tests for the reported error cases.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[kesselb]

Sorry @mvanhorn, my rebase made the dco check fail. Could you rebase one more time to resolve it? ;)

[mvanhorn]

Rebased in ff483f2. DCO sign-off intact on all 6 commits.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

[mvanhorn]

@kesselb @joshtrichards - pushed a510dfa addressing the three inline comments from the review:

• $shareNode->get() now catches NotPermittedException and returns 403 (the NotPermittedException note on line 133).
• Dropped the unused $e capture on the NotFoundException and InvalidArgumentException catch blocks.
• Added testPreviewFolderSubfolderReturnsBadRequest covering the Folder->400 branch that Copilot flagged.

One thing I can't fix myself: DCO is still failing and I think it's on the housekeeping commits @joshtrichards pushed - the Signed-off-by email on those is josh.t.richards@gmail.com, which I suspect doesn't match your GitHub account's primary email (that's usually the cause when DCO still fails after an author-side rebase). I already re-signed my own commits in ff483f2, but I can't re-sign yours without forging the DCO statement. Could you push a re-sign on the housekeeping commits (or let me know if you'd rather I drop them and add them back with your Co-authored-by trailer)?

[mvanhorn]

Rebased onto latest master - all 7 commits retain sign-off. DCO should be green now. Sorry for the delay!

[mvanhorn]

All three are addressed in the current head: the catch is now the non-capturing catch (NotFoundException), NotPermittedException is caught and mapped alongside it, and testPreviewFolderInvalidFile covers the folder-share → 400 branch. Resolving these — thanks for the review.

[mvanhorn]

Rebased onto latest master - DCO is green again. There was one small conflict in PublicPreviewControllerTest.php around the added tests, resolved during the rebase; the diff is otherwise unchanged.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

apps/files_sharing/tests/Controller/PublicPreviewControllerTest.php:406

• The controller now catches \OCP\Files\NotPermittedException (and returns 403), but this behavior is not covered by a unit test in this test class. Adding a dedicated test helps prevent regressions where NotPermitted would bubble up as a 500 again.

		$res = $this->controller->getPreview('token', 'file', 10, 10, true, false);
		$expected = new FileDisplayResponse($preview, Http::STATUS_OK, ['Content-Type' => 'myMime']);
		$expected->cacheFor(3600 * 24);
		$this->assertEquals($expected, $res);
	}

[kesselb]

Hey, thanks a lot again for your PR, and sorry it's still not merged!

We do require an Assisted-by line in the commit message (before the Signed-off-by trailers) to signal AI usage for tracking. Here's how it should look for you:

Assisted-by: Claude:claude-opus-4-6

Could you please squash the commits together, rebase once more, and add the Assisted-by trailer to the commit message? Please keep the sign-offs from you and Josh.

Sorry for the extra round trip on this. Let me know if that's too much trouble, I usually can't push to branches on forks directly, so if I did it myself it'd mean opening a fresh PR instead of updating this one, which is a bit more churn.

Happy to do that if you'd rather not touch it again, just let me know.

[kesselb]

To make the openapi check green:

diff --git a/apps/files_sharing/openapi.json b/apps/files_sharing/openapi.json
index c31231f8ee9..db484a93aeb 100644
--- a/apps/files_sharing/openapi.json
+++ b/apps/files_sharing/openapi.json
@@ -1474,7 +1474,8 @@
         "/index.php/apps/files_sharing/publicpreview/{token}": {
             "get": {
                 "operationId": "public_preview-get-preview",
-                "summary": "Get a preview for a shared file",
+                "summary": "Get a preview for a public share.",
+                "description": "For shares pointing to a single file, the $file parameter is ignored. For folder shares, $file must be the relative path to a file inside the shared folder.",
                 "tags": [
                     "public_preview"
                 ],
@@ -1500,7 +1501,7 @@
                     {
                         "name": "file",
                         "in": "query",
-                        "description": "File in the share",
+                        "description": "Relative path to a file inside a shared folder; ignored for single-file shares",
                         "schema": {
                             "type": "string",
                             "default": ""
diff --git a/openapi.json b/openapi.json
index 9371409d2bf..bfb6c3bbbd9 100644
--- a/openapi.json
+++ b/openapi.json
@@ -25590,7 +25590,8 @@
         "/index.php/apps/files_sharing/publicpreview/{token}": {
             "get": {
                 "operationId": "files_sharing-public_preview-get-preview",
-                "summary": "Get a preview for a shared file",
+                "summary": "Get a preview for a public share.",
+                "description": "For shares pointing to a single file, the $file parameter is ignored. For folder shares, $file must be the relative path to a file inside the shared folder.",
                 "tags": [
                     "files_sharing/public_preview"
                 ],
@@ -25616,7 +25617,7 @@
                     {
                         "name": "file",
                         "in": "query",
-                        "description": "File in the share",
+                        "description": "Relative path to a file inside a shared folder; ignored for single-file shares",
                         "schema": {
                             "type": "string",
                             "default": ""