Allow to specify the token of a public share / share renaming

[artonge]

• could be a security issue. Mitigations:
  • add a warning?
  • optionally enforce password protection?
  • enforce a minimum set of dictionary words? (minimal 2, optionally configurable?)
• makes no sense for bigger server, so only for community edition
• What is the migration plan when going to Enterprise version?

[szaimen]

So should this be a separate app so that it can be removed during building the enterprise archive?

[jospoortvliet]

@szaimen yes.
Ideally we adapt and include https://apps.nextcloud.com/apps/cfg_share_links or https://apps.nextcloud.com/apps/sharerenamer in the community edition.

[artonge]

@miaulalala could you give an security opinion on that feature?

[jancborchardt]

could be a security issue. Mitigations:
add a warning?

A warning is fine, yes. When you want to give a name, you want it to be easier visible.

optionally enforce password protection?

Optional if at all, but again, the point of giving a simple share name is the ability to remember. If you then have to remember a separate password it defeats the purpose and you might as well have pasted the complicated link.

enforce a minimum set of dictionary words? (minimal 2, optionally configurable?)

Straight no on that one. Again if you change the name you want it to be simple. :)

[miaulalala]

Dictionary attacks would become a concern, yes. Token enumeration is already possible if highly unlikely, although I assume the endpoint is brute force protected, so it's expensive and time consuming.

Could also lead to token leaks as two identical tokens aren't possible, so a (guest) user might try creating shares at random to guess at other tokens. If we add the userId to the token, that could work as a preventative measure so each token only needs to be unique to each user space. The token controller will need to first check the userId against the logged-in user so somebody can't fake the userId in a request and try collision based guessing that way, but then the benefit of having a custom token is minimized.

If we do this, I'd recommend enforcing a password.

I'd also suggest a minimum length for the token. Not great if the user chooses something really short.

In short, there are definitely some risks to this, and I can't really see the benefits. Maybe a link shortener integration could be a viable alternative?

[jospoortvliet]

Dictionary attacks would become a concern, yes. Token enumeration is already possible if highly unlikely, although I assume the endpoint is brute force protected, so it's expensive and time consuming.

Could also lead to token leaks as two identical tokens aren't possible, so a (guest) user might try creating shares at random to guess at other tokens. If we add the userId to the token, that could work as a preventative measure so each token only needs to be unique to each user space. The token controller will need to first check the userId against the logged-in user so somebody can't fake the userId in a request and try collision based guessing that way, but then the benefit of having a custom token is minimized.

If we do this, I'd recommend enforcing a password.

I'd also suggest a minimum length for the token. Not great if the user chooses something really short.

In short, there are definitely some risks to this, and I can't really see the benefits. Maybe a link shortener integration could be a viable alternative?

this is basically an accepted risk - it is meant for small home user instances where I might want to share vacation photos with family using an easy to remember link. That that makes the link easy to find is something we will warn about in the UI, but it's inherent to the solution. Adding a (mandatory) password would defeat the purpose of this ;-)

On large instances it's a rather dumb idea to do it, but then we don't intend to promote it for that. It's pure for home users.

FWIW, I use the app that implements this myself.

[Pytal]

The simplest way to address the enterprise migration part I think is to have a separate custom share token alongside the generated token that is available on community instances and disabled on enterprise automatically with a check.

This removes the complexity of having to "migrate" anything such as overwriting a custom token that may have been set in-place of the generated token, as would be needed based on the implementations in the apps mentioned previously. To detect this we can use:

• feat: Add check for enterprise #48248

Unfortunately the code is quite tightly coupled at the moment which makes it infeasible to simply add a hook/plugin in a separate app to handle custom tokens. But allowing custom tokens in the existing code is not complicated.

[susnux]

have a separate custom share token alongside the generated token that is available on community instances and disabled on enterprise automatically with a check.

I think this brings confusion if one version of Nextcloud (community vs enterprise) hard disables features for the other.
Also that does not work as Enterprise is also using community images, so this check is not working always.

I think we should instead make this available to all versions, but add two admin settings:

1. Allow disable this feature.
2. Allow to enforce a password for custom tokens.

Both should be simple to add to the sharing controller.

I know also some bigger instances that would like to have this feature, one use case is to provide short sharing URLs for their customers. As the only use case for this feature is sharing "by paper" (otherwise clicking or copy past is available which does not care about the length) and this is mainly something you do with e.g. "upload pictures" on a private event like a wedding / birthday, but also for customers on e.g. a flyer.

[Pytal]

I think this brings confusion if one version of Nextcloud (community vs enterprise) hard disables features for the other. Also that does not work as Enterprise is also using community images, so this check is not working always.

I think we should instead make this available to all versions, but add two admin settings:

1. Allow disable this feature.
2. Allow to enforce a password for custom tokens.

My thinking was that the switch to an enterprise image would be secure by default rather than requiring an admin manually opting in to security. Would be fine with going with whatever @nextcloud/security thinks is reasonable

[sorbaugh]

cc @nickvergessen do you think the approach recently discussed would be ok?

[susnux]

My thinking was that the switch to an enterprise image would be secure by default rather than requiring an admin manually opting in to security.

How about to enable by default for new installations, so if you set up Nextcloud you need to configure a lot of stuff anyways can disable this if you like.
And we add a migration for existing installation to disable this new setting if users > X (100? 50?) otherwise enable.

[nickvergessen]

• Btw there is/was also feat(share): make sharelink token length configurable #47265 in the meantime

have a separate custom share token alongside the generated token that is available on community instances and disabled on enterprise automatically with a check.

That's also a no from my side.

Allow disable this feature.

I think that's a good way to go instead of doing magic and potentially upsetting things like #48248 (it opens the door to hell). It should have a visible admin setting in index.php/settings/admin/sharing

An idea regarding the update scenario that I could see: Default the config to "allow specifying short links" for new installation. And on updates we set it to "disallow" if:

• Passwords are enforced
• Expiration for shares via link or email is default enabled
• Disclaimer text is set
• Default download limit is set

This way most "enterprisy" or "we care about security" instance would be on the save side while updating and all the "usability focused" instances have the fancy feature at hand.

As for further comments, there should be a warning text at the setting as well as on the share option itself stating that this can lead to "easy access", similarly as Jan stated in #45440 (comment)
An enforced password does not need to be added in this case.

As for requirements for the token:

• No slash or dot (breaks urls)
• No colon (breaks auth)
• No space or plus (replace with -, otherwise might break url encodings, etc.)
• At least 1 character long

Unsure:

• Limit to a-zA-Z0-9\-? Might be savest to not break browsers, email clients, etc. with utf8+ in emails

---

With all that in mind, we can then simply set the token in the code place of #47265 to whatever was entered in the interface (if config allows) and check for uniqueness. Then we don't even need a database migration as we can simply utilize the existing table column.

[sorbaugh]

Hello @Pytal, small "new" req for this feature, which is to have it disabled by default! Depending on the approach you're taking this could mean: If it's an app, ship it but disable it by default. If it's not an app, add a setting to turn this on/off (off by default).

cc @jospoortvliet

[Pytal]

Thanks for your insights @nickvergessen!

With all that in mind, we can then simply set the token in the code place of #47265 to whatever was entered in the interface (if config allows) and check for uniqueness. Then we don't even need a database migration as we can simply utilize the existing table column.

My question with this approach, as opposed to the separate custom share token column I proposed, is if the feature is disabled then what happens to the shares with custom tokens and how do we detect whether a token is custom or not as they can use the same characters?

[susnux]

what happens to the shares with custom tokens

I would say: Nothing. Keep them like if they were native tokens. Otherwise every share would be invalidated.

and how do we detect whether a token is custom or not as they can use the same characters?

Is this needed?

[Pytal]

I would say: Nothing. Keep them like if they were native tokens. Otherwise every share would be invalidated.

They would still be accessible then which is a security issue no?

Is this needed?

With this approach then we have to have a way to invalidate custom shares

[nickvergessen]

They would still be accessible then which is a security issue no?

I also don't think cleaning up automatically will be needed. Just because you disable generating such links in the future, doesn't mean you want to break all existing ones as well. We can simply explain this in a description next to the setting.

[Pytal]

They would still be accessible then which is a security issue no?

I also don't think cleaning up automatically will be needed. Just because you disable generating such links in the future, doesn't mean you want to break all existing ones as well. We can simply explain this in a description next to the setting.

Sounds reasonable :)

I'll go with the replace existing token way and add a button to create a generated token as well so it's easy to make the share link non-guessable.

[nickvergessen]

I'll go with the replace existing token way and add a button to create a generated token as well so it's easy to make the share link non-guessable.

random token should still be the default and setting a simple token should be the part that requires a button being pressed 🙊

[MrRinkana]

Is the intent to make an own version of this feature or to contribute to one of Configurable Share Links or ShareRenamer (and in that case which)?

[susnux]

Is the intent to make an own version of this feature

I probably will be part of Nextcloud core, so directly implemented in the files_sharing app.