Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion docs/API_v3.md
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,12 @@ Update a single or all properties of an option-object
| Parameter | Type | Description |
|------------------|----------|-------------|
| _keyValuePairs_ | Array | Array of key-value pairs to update |
- Restrictions: Currently only the _permissions_ can be updated.
- Restrictions:
- Allowed keys are _permissions_ and _token_.
- _token_ updates are only available when the admin setting _allowCustomPublicShareTokens_ is enabled.
- _token_ can only be updated on link shares.
- _token_ must be unique among link shares and only contain alphanumeric characters.
- _token_ length must be between 8 and 256 characters.
- Response: **Status-Code OK**, as well as the id of the share object.

```
Expand Down
6 changes: 6 additions & 0 deletions lib/Constants.php
Original file line number Diff line number Diff line change
Expand Up @@ -15,17 +15,23 @@ class Constants {
*/
public const CONFIG_KEY_ALLOWPERMITALL = 'allowPermitAll';
public const CONFIG_KEY_ALLOWPUBLICLINK = 'allowPublicLink';
public const CONFIG_KEY_ALLOWCUSTOMPUBLICTOKEN = 'allowCustomPublicShareTokens';
public const CONFIG_KEY_ALLOWSHOWTOALL = 'allowShowToAll';
public const CONFIG_KEY_CREATIONALLOWEDGROUPS = 'creationAllowedGroups';
public const CONFIG_KEY_RESTRICTCREATION = 'restrictCreation';
public const CONFIG_KEYS = [
self::CONFIG_KEY_ALLOWPERMITALL,
self::CONFIG_KEY_ALLOWPUBLICLINK,
self::CONFIG_KEY_ALLOWCUSTOMPUBLICTOKEN,
self::CONFIG_KEY_ALLOWSHOWTOALL,
self::CONFIG_KEY_CREATIONALLOWEDGROUPS,
self::CONFIG_KEY_RESTRICTCREATION
];

public const PUBLIC_SHARE_TOKEN_MIN_LENGTH = 8;
public const PUBLIC_SHARE_TOKEN_MAX_LENGTH = 256;
public const PUBLIC_SHARE_HASH_REQUIREMENT = '[a-zA-Z0-9]{' . self::PUBLIC_SHARE_TOKEN_MIN_LENGTH . ',' . self::PUBLIC_SHARE_TOKEN_MAX_LENGTH . '}';

/**
* Maximum String lengths, the database is set to store.
*/
Expand Down
2 changes: 1 addition & 1 deletion lib/Controller/PageController.php
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ public function internalLinkView(string $hash): Response {
#[NoAdminRequired()]
#[NoCSRFRequired()]
#[PublicPage()]
#[FrontpageRoute(verb: 'GET', url: '/s/{hash}', requirements: ['hash' => '[a-zA-Z0-9]{24,}'])]
#[FrontpageRoute(verb: 'GET', url: '/s/{hash}', requirements: ['hash' => Constants::PUBLIC_SHARE_HASH_REQUIREMENT])]
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.

Going from {24,} to {8,} is a steep drop. afaik publicLinkView has 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.

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.

What's the min length for share tokens in Files? I think we should use the same value.

Copy link
Copy Markdown
Author

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

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.

I think short values make sense for e.g. public campaigns.
But we must add bruteforce protection here then.

public function publicLinkView(string $hash): Response {
try {
$share = $this->shareMapper->findPublicShareByHash($hash);
Expand Down
84 changes: 74 additions & 10 deletions lib/Controller/ShareApiController.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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.');
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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The 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);
Expand Down Expand Up @@ -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');
}
}
}
6 changes: 5 additions & 1 deletion lib/Service/ConfigService.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,10 @@ public function getAllowPermitAll(): bool {
public function getAllowPublicLink(): bool {
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWPUBLICLINK, 'true'));
}
public function getAllowShowToAll() : bool {
public function getAllowCustomPublicToken(): bool {
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWCUSTOMPUBLICTOKEN, 'false'));
}
public function getAllowShowToAll(): bool {
return json_decode($this->config->getAppValue($this->appName, Constants::CONFIG_KEY_ALLOWSHOWTOALL, 'true'));
}
private function getUnformattedCreationAllowedGroups(): array {
Expand All @@ -57,6 +60,7 @@ public function getAppConfig(): array {
return [
Constants::CONFIG_KEY_ALLOWPERMITALL => $this->getAllowPermitAll(),
Constants::CONFIG_KEY_ALLOWPUBLICLINK => $this->getAllowPublicLink(),
Constants::CONFIG_KEY_ALLOWCUSTOMPUBLICTOKEN => $this->getAllowCustomPublicToken(),
Constants::CONFIG_KEY_ALLOWSHOWTOALL => $this->getAllowShowToAll(),
Constants::CONFIG_KEY_CREATIONALLOWEDGROUPS => $this->getCreationAllowedGroups(),
Constants::CONFIG_KEY_RESTRICTCREATION => $this->getRestrictCreation(),
Expand Down
6 changes: 3 additions & 3 deletions openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -5157,7 +5157,7 @@
"/ocs/v2.php/apps/forms/api/v3/forms/{formId}/shares/{shareId}": {
"patch": {
"operationId": "share_api-update-share",
"summary": "Update permissions of a share",
"summary": "Update properties of a share",
"description": "This endpoint allows CORS requests",
"tags": [
"share_api"
Expand Down Expand Up @@ -5254,7 +5254,7 @@
}
},
"400": {
"description": "Invalid permission given",
"description": "Share hash exists, please retry",
"content": {
"application/json": {
"schema": {
Expand Down Expand Up @@ -5282,7 +5282,7 @@
}
},
"403": {
"description": "Not allowed to update other properties than permissions",
"description": "Not allowed to update unknown properties",
"content": {
"application/json": {
"schema": {
Expand Down
14 changes: 14 additions & 0 deletions src/FormsSettings.vue
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.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure, on it

Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,13 @@
@update:modelValue="onAllowPublicLinkChange">
{{ t('forms', 'Allow sharing by link') }}
</NcCheckboxRadioSwitch>
<NcCheckboxRadioSwitch
ref="switchAllowCustomPublicShareTokens"
v-model="appConfig.allowCustomPublicShareTokens"
type="switch"
@update:modelValue="onAllowCustomPublicShareTokensChange">
{{ t('forms', 'Allow custom public share tokens') }}
</NcCheckboxRadioSwitch>
<NcCheckboxRadioSwitch
ref="switchAllowPermitAll"
v-model="appConfig.allowPermitAll"
Expand Down Expand Up @@ -115,6 +122,13 @@ export default {
el.loading = false
},

async onAllowCustomPublicShareTokensChange(newVal) {
const el = this.$refs.switchAllowCustomPublicShareTokens
el.loading = true
await this.saveAppConfig('allowCustomPublicShareTokens', newVal)
el.loading = false
},

async onAllowPermitAllChange(newVal) {
const el = this.$refs.switchAllowPermitAll
el.loading = true
Expand Down
Loading
Loading