From 7adae7c6b5e7adb0c1b1dcccbe77158e41166399 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 09:23:02 -0400 Subject: [PATCH 01/12] refactor: inject request for guest cookie lookup in token manager Eliminates two unnecessary/undesirable things: (1) a external static helper method that was only called here and only once; (2) accessing global state and `$_COOKIE` directly. Signed-off-by: Josh --- lib/TokenManager.php | 62 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 11 deletions(-) diff --git a/lib/TokenManager.php b/lib/TokenManager.php index 10d5582ac7..b948a88cd1 100644 --- a/lib/TokenManager.php +++ b/lib/TokenManager.php @@ -21,6 +21,7 @@ use OCP\Files\NotFoundException; use OCP\Files\NotPermittedException; use OCP\IL10N; +use OCP\IRequest; use OCP\IURLGenerator; use OCP\Share\Exceptions\ShareNotFound; use OCP\Share\IManager; @@ -40,13 +41,19 @@ public function __construct( private PermissionManager $permissionManager, private IEventDispatcher $eventDispatcher, private LoggerInterface $logger, + private IRequest $request, ) { } /** * @throws Exception */ - public function generateWopiToken(string $fileId, ?string $shareToken = null, ?string $editoruid = null, bool $direct = false): Wopi { + public function generateWopiToken( + string $fileId, + ?string $shareToken = null, + ?string $editoruid = null, + bool $direct = false, + ): Wopi { [$fileId, , $version] = Helper::parseFileId($fileId); $owneruid = null; $hideDownload = false; @@ -121,15 +128,45 @@ public function generateWopiToken(string $fileId, ?string $shareToken = null, ?s $this->eventDispatcher->dispatchTyped(new BeforeNodeReadEvent($file)); $serverHost = $this->urlGenerator->getAbsoluteURL('/'); - $guestName = $editoruid === null ? $this->prepareGuestName($this->helper->getGuestNameFromCookie()) : null; - return $this->wopiMapper->generateFileToken($fileId, $owneruid, $editoruid, $version, $updatable, $serverHost, $guestName, $hideDownload, $direct, 0, $shareToken); + + $guestName = $editoruid === null ? $this->prepareGuestName($this->getGuestNameFromCookie()) : null; + + return $this->wopiMapper->generateFileToken( + $fileId, + $owneruid, + $editoruid, + $version, + $updatable, + $serverHost, + $guestName, + $hideDownload, + $direct, + 0, + $shareToken + ); + } + + private function getGuestNameFromCookie(): ?string { + $guestUser = $this->request->getCookie('guestUser'); + + if ($guestUser === '' || $guestUser === null) { + return null; + } + + return $guestUser; } /** * This method is receiving the results from the TOKEN_TYPE_FEDERATION generated on the opener server * that is created in {@link newInitiatorToken} */ - public function upgradeToRemoteToken(Wopi $wopi, Wopi $remoteWopi, string $shareToken, string $remoteServer, string $remoteServerToken): Wopi { + public function upgradeToRemoteToken( + Wopi $wopi, + Wopi $remoteWopi, + string $shareToken, + string $remoteServer, + string $remoteServerToken, + ): Wopi { if ($remoteWopi->getTokenType() !== Wopi::TOKEN_TYPE_INITIATOR) { return $wopi; } @@ -150,7 +187,7 @@ public function upgradeToRemoteToken(Wopi $wopi, Wopi $remoteWopi, string $share return $wopi; } - public function upgradeFromDirectInitiator(Direct $direct, Wopi $wopi) { + public function upgradeFromDirectInitiator(Direct $direct, Wopi $wopi): Wopi { $wopi->setTokenType(Wopi::TOKEN_TYPE_REMOTE_GUEST); $wopi->setEditorUid(null); $wopi->setRemoteServer($direct->getInitiatorHost()); @@ -207,7 +244,13 @@ public function generateWopiTokenForTemplate( ); } - public function newInitiatorToken($sourceServer, ?Node $node = null, $shareToken = null, bool $direct = false, $userId = null): Wopi { + public function newInitiatorToken( + string $sourceServer, + ?Node $node = null, + ?string $shareToken = null, + bool $direct = false, + ?string $userId = null + ): Wopi { if ($node !== null) { $wopi = $this->generateWopiToken((string)$node->getId(), $shareToken, $userId, $direct); $wopi->setServerHost($sourceServer); @@ -226,7 +269,7 @@ public function extendWithInitiatorUserToken(Wopi $wopi, string $initiatorUserHo return $wopi; } - public function prepareGuestName(?string $guestName = null) { + public function prepareGuestName(?string $guestName = null): string { if (empty($guestName)) { return $this->trans->t('Anonymous guest'); } @@ -244,13 +287,10 @@ public function prepareGuestName(?string $guestName = null) { } /** - * @param string $accessToken - * @param string $guestName - * @return void * @throws Exceptions\ExpiredTokenException * @throws Exceptions\UnknownTokenException */ - public function updateGuestName(string $accessToken, string $guestName) { + public function updateGuestName(string $accessToken, string $guestName): void { $wopi = $this->wopiMapper->getWopiForToken($accessToken); $wopi->setGuestDisplayname($this->prepareGuestName($guestName)); $this->wopiMapper->update($wopi); From d071a02dd834a2ee205aebbf0cc84973ef9c968f Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 09:24:16 -0400 Subject: [PATCH 02/12] refactor(Helper): drop no longer needed static getGuestNameFromCookie() Signed-off-by: Josh --- lib/Helper.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/lib/Helper.php b/lib/Helper.php index 3503449738..785c2fc19d 100644 --- a/lib/Helper.php +++ b/lib/Helper.php @@ -81,13 +81,6 @@ public static function getNewFileName(Folder $folder, $filename) { return $filename; } - public function getGuestNameFromCookie() { - if ($this->userId !== null || !isset($_COOKIE['guestUser']) || $_COOKIE['guestUser'] === '') { - return null; - } - return $_COOKIE['guestUser']; - } - public function getShareFromNode(Node $node): ?IShare { try { $storage = $node->getStorage(); From 64486c5ca1de97dc646478edc1e123e1c2c6f298 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 09:46:16 -0400 Subject: [PATCH 03/12] refactor(Helper): add typing and docs Signed-off-by: Josh --- lib/Helper.php | 55 +++++++++++++++++++++++++++++++++++++------------- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/lib/Helper.php b/lib/Helper.php index 785c2fc19d..01d5a667ca 100644 --- a/lib/Helper.php +++ b/lib/Helper.php @@ -1,4 +1,5 @@ format('Y-m-d\TH:i:s.u\Z'); } @@ -70,17 +74,40 @@ public static function toISO8601($time) { return false; } - public static function getNewFileName(Folder $folder, $filename) { + /** + * Return a unique file name by appending an incrementing suffix if needed. + * + * @param Folder $folder Folder to check for name collisions. + * @param string $filename Proposed file name. + * @return string Collision-free file name. + * @throws \RuntimeException If regex/PCRE outright fails (unlikely). + */ + public static function getNewFileName(Folder $folder, string $filename): string { $fileNum = 1; while ($folder->nodeExists($filename)) { $fileNum++; - $filename = preg_replace('/(\.| \(\d+\)\.)([^.]*)$/', ' (' . $fileNum . ').$2', $filename); + $newFilename = preg_replace('/(\.| \(\d+\)\.)([^.]*)$/', ' (' . $fileNum . ').$2', $filename); + + if ($newFilename === null) { + // unlikely unless regex is broken + throw new \RuntimeException('Failed to generate a unique filename'); + } + + $filename = $newFilename; } return $filename; } + /** + * Resolve the share associated with a node from shared storage. + * + * TODO: Put this elsewhere. + * + * @param Node $node File node to inspect. + * @return IShare|null The share backing the node, or null if it is not shared. + */ public function getShareFromNode(Node $node): ?IShare { try { $storage = $node->getStorage(); From 1904f8beac4931235f69184cda9729a67c1d329f Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 10:25:30 -0400 Subject: [PATCH 04/12] refactor(Helper): streamline control flow of parseFileId Signed-off-by: Josh --- lib/Helper.php | 48 +++++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/lib/Helper.php b/lib/Helper.php index 01d5a667ca..94bb9fd9f6 100644 --- a/lib/Helper.php +++ b/lib/Helper.php @@ -25,38 +25,40 @@ public function __construct( } /** - * Parse the WOPI file identifier string. + * Parse the WOPI/richdocuments file identifier string. + * + * Breaks the WOPI-encoded file identifier into Nextcloud fileId and optional instanceId, version, and template parts. + * + * Format examples: + * - {fileId} + * - {fileId}_{instanceId} + * - {fileId}_{instanceId}_{version} + * + * For template-based documents, the file part contains a template marker and may be + * encoded as "{fileId}/{templateId}". * * @param string $fileId WOPI-encoded file identifier. * @return array{0: string, 1: string, 2: string, 3: string|null} - * @throws \Exception If the identifier does not match the expected format. + * @throws \InvalidArgumentException If the identifier does not match the expected format. */ public static function parseFileId(string $fileId): array { - $arr = explode('_', $fileId); - $templateId = null; - if (count($arr) === 1) { - $fileId = $arr[0]; - $instanceId = ''; - $version = '0'; - } elseif (count($arr) === 2) { - [$fileId, $instanceId] = $arr; - $version = '0'; - } elseif (count($arr) === 3) { - [$fileId, $instanceId, $version] = $arr; - } else { - throw new \Exception('$fileId has not the expected format'); + $parts = explode('_', $fileId); + + if (count($parts) > 3) { + throw new \InvalidArgumentException('$fileId does not match the expected format'); } - if (str_contains($fileId, '-')) { - [$fileId, $templateId] = array_pad(explode('/', $fileId), 2, null); + $fileIdPart = $parts[0]; + $instanceId = $parts[1] ?? ''; + $version = $parts[2] ?? '0'; + $templateId = null; + + $hasTemplateMarker = str_contains($fileIdPart, '-'); + if ($hasTemplateMarker) { + [$fileIdPart, $templateId] = array_pad(explode('/', $fileIdPart, 2), 2, null); } - return [ - $fileId, - $instanceId, - $version, - $templateId, - ]; + return [$fileIdPart, $instanceId, $version, $templateId]; } /** From e007bbb902738764ddc7d052191bff85d9c92bec Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 10:37:35 -0400 Subject: [PATCH 05/12] docs(Helper): tidy up parseFileId docs Signed-off-by: Josh --- lib/Helper.php | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/lib/Helper.php b/lib/Helper.php index 94bb9fd9f6..e2f811cbd8 100644 --- a/lib/Helper.php +++ b/lib/Helper.php @@ -25,20 +25,18 @@ public function __construct( } /** - * Parse the WOPI/richdocuments file identifier string. + * Parse a WOPI file identifier string into its components. * - * Breaks the WOPI-encoded file identifier into Nextcloud fileId and optional instanceId, version, and template parts. - * - * Format examples: + * Supports standard and template-based identifiers. Standard formats include: * - {fileId} * - {fileId}_{instanceId} * - {fileId}_{instanceId}_{version} * - * For template-based documents, the file part contains a template marker and may be + * For template-based documents, the file part contains a template marker and is expected to be * encoded as "{fileId}/{templateId}". * - * @param string $fileId WOPI-encoded file identifier. - * @return array{0: string, 1: string, 2: string, 3: string|null} + * @param string $fileId The WOPI-encoded file identifier string to parse. + * @return array{0: string, 1: string, 2: string, 3: string|null} [fileId, instanceId, version, templateId] * @throws \InvalidArgumentException If the identifier does not match the expected format. */ public static function parseFileId(string $fileId): array { From fcd8ece71c1344c38b81d561a72d0d9da65092f7 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 10:53:05 -0400 Subject: [PATCH 06/12] chore(TokenManager): re-add override for logged in users with guest cookies Signed-off-by: Josh --- lib/TokenManager.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/TokenManager.php b/lib/TokenManager.php index b948a88cd1..9bb8c4f201 100644 --- a/lib/TokenManager.php +++ b/lib/TokenManager.php @@ -1,5 +1,6 @@ userId === null) { + return null; + } + $guestUser = $this->request->getCookie('guestUser'); if ($guestUser === '' || $guestUser === null) { From c27bee8846403b2d7779921af84197f5d132759e Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 11:00:11 -0400 Subject: [PATCH 07/12] chore(Helper): re-add dropped time precision comment Signed-off-by: Josh --- lib/Helper.php | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/lib/Helper.php b/lib/Helper.php index e2f811cbd8..359bb3f0ef 100644 --- a/lib/Helper.php +++ b/lib/Helper.php @@ -1,6 +1,6 @@ format('Y-m-d\TH:i:s.u\Z'); From 7b601c950b701c7c99a6b0fc85b8e55e635bf9ba Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 11:07:52 -0400 Subject: [PATCH 08/12] chore: fixup typo in TokenManager Signed-off-by: Josh --- lib/TokenManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/TokenManager.php b/lib/TokenManager.php index 9bb8c4f201..984c236bd2 100644 --- a/lib/TokenManager.php +++ b/lib/TokenManager.php @@ -1,6 +1,6 @@ Date: Wed, 29 Apr 2026 12:27:39 -0400 Subject: [PATCH 09/12] chore: cast fileId to int in WopiController.php Signed-off-by: Josh --- lib/Controller/WopiController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/Controller/WopiController.php b/lib/Controller/WopiController.php index 8f9593d892..df5f7d5715 100644 --- a/lib/Controller/WopiController.php +++ b/lib/Controller/WopiController.php @@ -594,7 +594,7 @@ public function putFile( if ($isPutRelative) { // the new file needs to be installed in the current user dir $userFolder = $this->rootFolder->getUserFolder($wopi->getEditorUid()); - $file = $userFolder->getFirstNodeById($fileId); + $file = $userFolder->getFirstNodeById((int)$fileId); if ($file === null) { return new JSONResponse([], Http::STATUS_NOT_FOUND); } From c1f91c18fc123e09ffede88dafd0be36c48643fb Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 12:29:30 -0400 Subject: [PATCH 10/12] chore: cast fileId to int in TokenManager where necessary Signed-off-by: Josh --- lib/TokenManager.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/TokenManager.php b/lib/TokenManager.php index 984c236bd2..84df927930 100644 --- a/lib/TokenManager.php +++ b/lib/TokenManager.php @@ -102,7 +102,7 @@ public function generateWopiToken( } /** @var File $file */ - $file = $userFolder->getFirstNodeById($fileId); + $file = $userFolder->getFirstNodeById((int)$fileId); // Check node readability (for storage wrapper overwrites like terms of services) if ($file === null || !$file->isReadable()) { @@ -133,7 +133,7 @@ public function generateWopiToken( $guestName = $editoruid === null ? $this->prepareGuestName($this->getGuestNameFromCookie()) : null; return $this->wopiMapper->generateFileToken( - $fileId, + (int)$fileId, $owneruid, $editoruid, $version, From b31e253810a9ddeecc86654eba77b78668003b3e Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 12:32:13 -0400 Subject: [PATCH 11/12] chore: drop no longer relevant Helper class entry from psalm-baseline Signed-off-by: Josh --- tests/psalm-baseline.xml | 5 ----- 1 file changed, 5 deletions(-) diff --git a/tests/psalm-baseline.xml b/tests/psalm-baseline.xml index 9e0fe758a0..f513d21fbe 100644 --- a/tests/psalm-baseline.xml +++ b/tests/psalm-baseline.xml @@ -80,11 +80,6 @@ - - - - - From ec3243de175170140aa68769474ff8fe48558c92 Mon Sep 17 00:00:00 2001 From: Josh Date: Wed, 29 Apr 2026 21:22:32 -0400 Subject: [PATCH 12/12] chore(TokenManager): php-cs fixup Signed-off-by: Josh --- lib/TokenManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/TokenManager.php b/lib/TokenManager.php index 84df927930..3f28c1031c 100644 --- a/lib/TokenManager.php +++ b/lib/TokenManager.php @@ -255,7 +255,7 @@ public function newInitiatorToken( ?Node $node = null, ?string $shareToken = null, bool $direct = false, - ?string $userId = null + ?string $userId = null, ): Wopi { if ($node !== null) { $wopi = $this->generateWopiToken((string)$node->getId(), $shareToken, $userId, $direct);