-
Notifications
You must be signed in to change notification settings - Fork 118
Allow custom public share tokens for form links #3311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
557abb5
be327ac
b92f1cc
174a21c
8266216
c86e612
36b8591
74d859b
c216d0a
8a96b09
8d7708a
84b9f62
921a77d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -207,17 +207,21 @@ public function newShare(int $formId, int $shareType, string $shareWith = '', ar | |
| } | ||
|
|
||
| /** | ||
| * Update permissions of a share | ||
| * Update properties of a share | ||
| * | ||
| * @param int $formId of the form | ||
| * @param int $shareId of the share to update | ||
| * @param array<string, mixed> $keyValuePairs Array of key=>value pairs to update. | ||
| * @return DataResponse<Http::STATUS_OK, int, array{}> | ||
| * @throws OCSBadRequestException Share doesn't belong to given Form | ||
| * @throws OCSBadRequestException Invalid permission given | ||
| * @throws OCSBadRequestException Invalid share token | ||
| * @throws OCSBadRequestException Share hash exists, please retry | ||
| * @throws OCSForbiddenException Custom public share tokens are not allowed | ||
| * @throws OCSForbiddenException This form is not owned by the current user | ||
| * @throws OCSForbiddenException Empty keyValuePairs, will not update | ||
| * @throws OCSForbiddenException Not allowed to update other properties than permissions | ||
| * @throws OCSForbiddenException Not allowed to update token on non-link share | ||
| * @throws OCSForbiddenException Not allowed to update unknown properties | ||
| * @throws OCSNotFoundException Could not find share | ||
| * | ||
| * 200: the id of the updated share | ||
|
|
@@ -226,7 +230,7 @@ public function newShare(int $formId, int $shareType, string $shareWith = '', ar | |
| #[NoAdminRequired()] | ||
| #[ApiRoute(verb: 'PATCH', url: '/api/v3/forms/{formId}/shares/{shareId}')] | ||
| public function updateShare(int $formId, int $shareId, array $keyValuePairs): DataResponse { | ||
| $this->logger->debug('Updating share: {shareId} of form {formId}, permissions: {permissions}', [ | ||
| $this->logger->debug('Updating share: {shareId} of form {formId}, values: {keyValuePairs}', [ | ||
| 'formId' => $formId, | ||
| 'shareId' => $shareId, | ||
| 'keyValuePairs' => $keyValuePairs | ||
|
|
@@ -256,22 +260,64 @@ public function updateShare(int $formId, int $shareId, array $keyValuePairs): Da | |
| throw new OCSForbiddenException('Empty keyValuePairs, will not update'); | ||
| } | ||
|
|
||
| //Don't allow to change other properties than permissions | ||
| if (count($keyValuePairs) > 1 || !array_key_exists('permissions', $keyValuePairs)) { | ||
| $this->logger->debug('Not allowed to update other properties than permissions'); | ||
| throw new OCSForbiddenException('Not allowed to update other properties than permissions'); | ||
| $allowedKeys = ['permissions', 'token']; | ||
| foreach (array_keys($keyValuePairs) as $key) { | ||
| if (!in_array($key, $allowedKeys, true)) { | ||
| $this->logger->debug('Not allowed to update other properties than permissions or token'); | ||
| throw new OCSForbiddenException('Not allowed to update other properties than permissions or token'); | ||
| } | ||
| } | ||
|
|
||
| if (!$this->validatePermissions($keyValuePairs['permissions'], $formShare->getShareType())) { | ||
| if (array_key_exists('permissions', $keyValuePairs) && !$this->validatePermissions($keyValuePairs['permissions'], $formShare->getShareType())) { | ||
| throw new OCSBadRequestException('Invalid permission given'); | ||
| } | ||
|
|
||
| if (array_key_exists('token', $keyValuePairs)) { | ||
| if (!$this->configService->getAllowCustomPublicToken()) { | ||
| $this->logger->debug('Custom public share tokens are not allowed.'); | ||
| throw new OCSForbiddenException('Custom public share tokens are not allowed.'); | ||
| } | ||
|
|
||
| if ($formShare->getShareType() !== IShare::TYPE_LINK) { | ||
| $this->logger->debug('Not allowed to update token on non-link share'); | ||
| throw new OCSForbiddenException('Not allowed to update token on non-link share'); | ||
| } | ||
|
|
||
| if (!is_string($keyValuePairs['token'])) { | ||
| throw new OCSBadRequestException('Invalid share token'); | ||
| } | ||
|
|
||
| $token = $keyValuePairs['token']; | ||
| if (!array_key_exists('permissions', $keyValuePairs) && $token === $formShare->getShareWith()) { | ||
| return new DataResponse($formShare->getId()); | ||
| } | ||
|
|
||
| if ($token !== $formShare->getShareWith()) { | ||
| $this->validatePublicShareToken($token); | ||
|
|
||
| try { | ||
| $existingShare = $this->shareMapper->findPublicShareByHash($token); | ||
| if ($existingShare->getId() !== $formShare->getId()) { | ||
| $this->logger->debug('Share hash already exists.'); | ||
| throw new OCSBadRequestException('Share hash exists, please retry.'); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This response confirms a token is in use somewhere on the instance, even when the caller has no other way to know that link exists. An authenticated user can probe arbitrary strings against this endpoint and enumerate tokens of private forms they don't own. Better to return the same generic "Invalid share token" that above validation already throws, so "taken" and "invalid format" look identical from the outside.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. makes sense, will do! |
||
| } | ||
| } catch (DoesNotExistException $e) { | ||
| // Just continue, this is what we expect to happen (share hash not existing yet). | ||
| } | ||
|
|
||
| $formShare->setShareWith($token); | ||
| } | ||
| } | ||
|
|
||
| $this->formsService->obtainFormLock($form); | ||
|
|
||
| $formShare->setPermissions($keyValuePairs['permissions']); | ||
| if (array_key_exists('permissions', $keyValuePairs)) { | ||
| $formShare->setPermissions($keyValuePairs['permissions']); | ||
| } | ||
| $formShare = $this->shareMapper->update($formShare); | ||
|
|
||
| if (in_array($formShare->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE], true)) { | ||
| if (array_key_exists('permissions', $keyValuePairs) | ||
| && in_array($formShare->getShareType(), [IShare::TYPE_USER, IShare::TYPE_GROUP, IShare::TYPE_USERGROUP, IShare::TYPE_CIRCLE], true)) { | ||
| if (in_array(Constants::PERMISSION_RESULTS, $keyValuePairs['permissions'], true)) { | ||
| $userFolder = $this->rootFolder->getUserFolder($form->getOwnerId()); | ||
| $uploadedFilesFolderPath = $this->formsService->getFormUploadedFilesFolderPath($form); | ||
|
|
@@ -421,4 +467,22 @@ private function validatePermissions(array $permissions, int $shareType): bool { | |
| } | ||
| return true; | ||
| } | ||
|
|
||
| /** | ||
| * @throws OCSBadRequestException If token does not satisfy basic safety checks | ||
| */ | ||
| private function validatePublicShareToken(string $token): void { | ||
| if ($token !== trim($token)) { | ||
| throw new OCSBadRequestException('Invalid share token'); | ||
| } | ||
|
|
||
| $tokenLength = strlen($token); | ||
| if ($tokenLength < Constants::PUBLIC_SHARE_TOKEN_MIN_LENGTH || $tokenLength > Constants::PUBLIC_SHARE_TOKEN_MAX_LENGTH) { | ||
| throw new OCSBadRequestException('Invalid share token'); | ||
| } | ||
|
|
||
| if (preg_match('/^[a-zA-Z0-9]+$/', $token) !== 1) { | ||
| throw new OCSBadRequestException('Invalid share token'); | ||
| } | ||
| } | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please rebase your branch on our current main and adjust this file accordingly to the changes merged into main lately. We had a bug in this file that prevented sending the requests to the server.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, on it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going from
{24,}to{8,}is a steep drop. afaikpublicLinkViewhas no brute force protection (neither does embeddedFormView, which still has no min length at all). Without throttling, that's brute-forceable with a moderate request budget against any instance that turns this on. Could you keep a higher minimum (16+) and add#[BruteForceProtection]to both view controllers? Either alone would be a big improvement.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the min length for share tokens in Files? I think we should use the same value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My own nextcloud instance even allows one character tokens in Files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think short values make sense for e.g. public campaigns.
But we must add bruteforce protection here then.