FileUploadEditor - minor problem with customized path segments

[LSNicholls]

Before submitting the bug report, please read and check the following items

• [X] This isn't a feature request or a discussion topic (Please use discussions for that)
• [X] I have searched open and closed issues for duplicates
• [X] I have searched the documentation on the Serenity Platform website https://serenity.is/docs/
• [X] I have read the frequently asked questions https://serenity.is/docs/startsharp/faq
• [X] I have searched my problem in the discussions and the old issues https://github.com/serenity-is/Serenity/discussions

What happened?

I accidentally included a column value containing a colon (":") in my path segments as part of the FilenameFormat attribute and got this error

System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'relativePath')
Serenity.PathHelper.SecureCombine(String root, String relativePath) in src\Serenity.Net.Core\Helpers\PathHelper.cs:line 50

The error that appeared to the user was "An error occurred on your request", or similar.

... I realize it was stupid and avoidable on my part, but wonder if there is someplace where invalid path characters needs to be checked and is not. That's why I'm putting this in as a bug rather than an ER.

What did you expect to happen?

A helpful error specific to path uploading issues, whether on the front end, such as

"Your file could not be loaded to the path specified: <...>"

... or at least a more explicit exception type in the log.

How to reproduce?

Use a value in one of the pipe-delimited path segments that contains a colon.

What Serenity Nuget Versions are you seeing the problem on? (separated by comma)

everything is 6.5.1

Relevant log output

System.ArgumentOutOfRangeException: Specified argument was out of the range of valid values. (Parameter 'relativePath')
   at Serenity.PathHelper.SecureCombine(String root, String relativePath) in src\Serenity.Net.Core\Helpers\PathHelper.cs:line 50
   at Serenity.Web.DiskUploadStorage.FilePath(String path) in src\Serenity.Net.Services\Upload\DiskUploadStorage.cs:line 55
   at Serenity.Web.DiskUploadStorage.WriteFile(String path, Stream source, OverwriteOption overwrite) in src\Serenity.Net.Services\Upload\DiskUploadStorage.cs:line 269
   at Serenity.Web.DiskUploadStorage.CopyFrom(IUploadStorage store, String sourcePath, String targetPath, OverwriteOption overwrite) in src\Serenity.Net.Services\Upload\DiskUploadStorage.cs:line 158
   at Serenity.Web.CombinedUploadStorage.CopyFrom(IUploadStorage source, String path, String targetPath, OverwriteOption overwrite) in src\Serenity.Net.Services\Upload\CombinedUploadStorage.cs:line 66
   at Serenity.Web.DefaultUploadStorage.CopyFrom(IUploadStorage sourceStorage, String sourcePath, String targetPath, OverwriteOption overwrite) in src\Serenity.Net.Web\Upload\DefaultUploadStorage.cs:line 67
   at Serenity.Web.UploadStorageExtensions.CopyTemporaryFile(IUploadStorage uploadStorage, CopyTemporaryFileOptions options) in src\Serenity.Net.Services\Upload\UploadStorageExtensions.cs:line 43
   at Serenity.Services.FileUploadBehavior.CopyTemporaryFile(ISaveRequestHandler handler, IFilesToDelete filesToDelete) in src\Serenity.Net.Web\Upload\FileUploadBehavior.cs:line 312
   at Serenity.Services.FileUploadBehavior.OnAfterSave(ISaveRequestHandler handler) in src\Serenity.Net.Web\Upload\FileUploadBehavior.cs:line 339
   at Serenity.Services.SaveRequestHandler<T1,T2,T3>.AfterSave() in src\Serenity.Net.Services\RequestHandlers\Save\SaveRequestHandler.cs:line 58
   at GQC.Work.QCTaskBatchSaveHandler.AfterSave()
   at Serenity.Services.SaveRequestHandler<T1,T2,T3>.Process(IUnitOfWork unitOfWork, TSaveRequest request, SaveRequestType requestType) in src\Serenity.Net.Services\RequestHandlers\Save\SaveRequestHandler.cs:line 381
   at Serenity.Services.SaveRequestHandler<T1,T2,T3>.Create(IUnitOfWork uow, TSaveRequest request) in src\Serenity.Net.Services\RequestHandlers\Save\SaveRequestHandler.cs:line 629
   at lambda_method448(Closure , Object , Object[] )

Serene template version

No response

Sergen version

No response

Code editor

No response

Operating System

No response

Node.js version

No response

TypeScript version

No response

Database type and version

No response

On which device do you see the problem?

No response

On which operating system do you see the problem?

No response

On which browsers do you see the problem?

No response

On what version of the browsers do you see the problem?

No response

Additional information

This occurred because I was passing a lookup text value into a form and the text value was of the form code: description. I had to do this because the form is only ever an insert and doesn't have good access to the underlying fields in various lookup tables at that point. I easily fixed it by parsing out the code value, before the colon, which is what I really needed for the path.

It was obviously not a common problem. But it could have been easier to find and fix, with slightly better error messaging... So this bug report is to say "Since you allow people to custom-specify the paths, which is great, then there may be some specialized error handling to bullet-proof this wonderful functionality."

Thank you for listening, as always.

[VictorTomaili]

Colon is not supported by windows. The error message will be more clear, but this is not an issue, actually. You can create a custom expression field in your row without these characters to use this value.

[LSNicholls]

OK thank you for changing it to an enhancement. Of course I know that colon is not supported but I do see checks for invalid file path characters in the code, and wondered why this mistake on my part didn't trigger a more appropriate exception. As I said, I only suggested that maybe there was someplace in the code that this check should be done but wasn't being done.

[volkanceylan]

The error is raised from the SecureCombine. There is no way that function to know where the relative path was used. I think the error is ok. If you pass a null value to some function it raises ArgumentNull, it does not say where the null value was passed from.

[volkanceylan]

Also it is IFilenameFormatSanitizer's responsibility to check for invalid characters. If you don't have a custom sanitizer (i think you have because we introduced that feature on your request) you can do such checks in its SanitizePlaceholder method.

[LSNicholls]

I absolutely do, @volkanceylan, and I thank you for that feature every day. :-)

I guess the problem is that I didn't understand why this line in the SanitizePlaceholder method didn't check for a colon:

value = Serenity.StringHelper.SanitizeFilePath((value ?? "").Replace('\\', '/'));

... I look at the StringHelper.SanitizeFilePathPath method and it seemed like Path.GetInvalidPathChars would be taking care of this. But, I think I understand now, it's not, because a colon is legal in a non-relative path.

So, I think my current implementation, where I take care of that one path segment that will contain a colon, is probably the best option. If I did it in the sanitizer I would be precluding a full path from being accepted.

Thank you as always!