Dollar sign is valid character for smb share name but not allowed since Nextcloud 16

[KurtCa]

Hey,
I had two problems after updating to 16.0.1.

1.try mounting external storage SMB/CIFS with a share that ends with $ e.g. sharename$ these are "hidden" shares. They used to work fine in at least 13, 14 and 15.

The shares are not connected after the update.

I "fixed" it with the hack in #15238

2. Can't sync files that are 0 byte

I "fixed" it with the hack in #13639

replaced
if ($partStorage->instanceOfStorage(Storage\IWriteStreamStorage::class)) {
by
if (false && $partStorage->instanceOfStorage(Storage\IWriteStreamStorage::class)) {
in file:
nextcloud/apps/dav/lib/Connector/Sabre/File.php

[kesselb]

Please use the issue template: https://github.com/nextcloud/server/blob/master/.github/ISSUE_TEMPLATE/Bug_report.md

It's quite impossible to reproduce your report without these information ...

[KurtCa]

Steps to reproduce

1. Create a hidden share on a windows server by adding $ at the end of the filename.
2. A share like te$t wil fail too.
3. Try to connect this share with smb in to nextcloud.

Expected behaviour
The share wil connect.

Actual behaviour
The share does not connect.

Server configuration

Operating system:
ubuntu 18.04.2 LTS
Web server:
Apache/2.4.29
Database:
mysql 5.7.26
PHP version:
7.2.18
Nextcloud version: (see Nextcloud admin page)
16.0.1.1
Updated from an older Nextcloud/ownCloud or fresh install:
15.x

[KurtCa]

Steps to reproduce
1.Create a file from 0 byte on windows.
2.Sync with the windows app

Expected behaviour
The File will sync to the server

Actual behaviour
Error, the file is not syncing

Server configuration

Operating system:
ubuntu 18.04.2 LTS
Web server:
Apache/2.4.29
Database:
mysql 5.7.26
PHP version:
7.2.18
Nextcloud version: (see Nextcloud admin page)
16.0.1.1
Updated from an older Nextcloud/ownCloud or fresh install:
15.x

[kesselb]

Thank you ;) Anything in the logs (Server and/or Client)?

A share like te$t wil fail too.

You mean test$?

[KurtCa]

No, I mean te$t. I just want to make it clear that it doesn't matter where the $ is used in the share name.
Log:
{"reqId":"8zMTvIyeycORplAm4OTl","level":3,"time":"2019-05-20T08:43:05+00:00","remoteAddr":"xxx.xxx.xxx..xxx","user":"x","app":"files_external","method":"GET","url":"/index.php/apps/files_external/userglobalstorages/34?testOnly=false","message":"A placeholder was not substituted: xxxxxxxx$ for mount type \OCA\Files_External\Lib\Storage\SMB","userAgent":"Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:66.0) Gecko/20100101 Firefox/66.0","version":"16. 0.1.1"}

[kesselb]

No, I mean te$t. I just want to make it clear that it doesn't matter where the $ is used in the share name

Ok. $ is part of the name of your share? (and not a placeholder you want replaced by nextcloud)

[KurtCa]

I tested a solution that i found here on github and than the hidden shares with the $ at the end worked.
I had one share with a $ sign in the name and this share didn't work. That's why i mentioned it.

[kesselb]

Thanks for your patience 👍

[kesselb]

Regression from #14174

There is a check if all placeholders for external storage configuration is replaced. A placeholder starts with an $. If your share name contains $ the configuration looks incomplete.

• A share name must be no more than 80 characters in length.
• The following characters are illegal in a share name: \ / [ ] : | < > + = ; , * ? "
• Control characters in range 0x00 through 0x1F, inclusive, are illegal in a share name.
• All other Unicode characters are legal.
• Names are case preserving and case insensitive.

https://docs.microsoft.com/en-us/rest/api/storageservices/naming-and-referencing-shares--directories--files--and-metadata

Looks like we need to use another character for placeholder ...

cc @nextcloud/server-triage @blizzz

[nickvergessen]

We could use square brackets instead, because they are illegal as per your list.

[blizzz]

I kept the $ to be backwards compatible with the original $user value. Going with the squared brackets or another character would mean to migrate old configs.

The logic to look out for incomplete configs was added. It could be dropped, however this could turn out bad in error cases.

[kesselb]

No. I know the report is very hard to read but this issue is about te$t. Dollar is a valid character for smb shares. A name like te$t is valid and should be possible. I see three possible solutions:

A) Pick another character for placeholders.
B) Make the check for incomplete configs less strict.
C) Add a way to escape dollar (e.g. \$).