6748: Stricter file and folder name validation #6792

BenedekFarkas · 2016-04-17T22:51:15Z

Fixing that some special characters were accepted in file and folder names.

Reworked the PR to have the following functionality:

Extended the capability of the CreateFolder, RenameFolder, CreateFile and RenameFile functions of FileSystemStorageProvider to validate the file/folder name against a larger set of special characters that aren't allowed or considered unsafe.
The list of characters include those suggested below, except for ~, which should be allowed.
This validation now happens on a low level, so removed validation from certain MediaLibrary components and improved the error message handling, so the user is notified of the characters causing trouble.
Applied the same validation to AzureFileSystem too.
If this PR is accepted, then 7572: FileSystemStorageProvider shouldn't allow creating folders with "&" in their name #7577 can be closed, although I didn't include any client-side validation, IMO we don't win that much with that added complexity (= having to use a regex).

dnfclas · 2016-04-17T22:57:31Z

@BenedekFarkas, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR.

Thanks, DNFBOT;

sebastienros · 2016-04-18T16:26:16Z

Can we ask @hannan-azam to test this fix? He's the most knowledgeable about this feature after you ;)

BenedekFarkas · 2016-04-18T17:31:30Z

Sure!

hannan-azam · 2016-04-18T19:38:14Z

Sure, I'll do the testing and will get back with results.

hannan-azam · 2016-04-18T21:13:07Z

@sebastienros @BenedekFarkas During the testing i found another issue with a recent commit 2440e68 (13th April) in media-library.js line 385. Please see the attached screen shot:

Script is trying to call a method "contains" which doesn't exist in standard string prototype and causes a javascript error. We should use indexOf method and the return value -1 or positive to write the conditional statement.
The Script has a glitch. While using Azure Media Storage, it concludes wrong directory separator for the top level folders. For example in current scenario i had Azure Media Storage turned on. I clicked the top level folder "Users". With the condition 'Users'.contains('/') ? '/' : '\\' it will always conclude the directory separator to be \\ instead of /.
Because of this error, the screen keeps showing the loading icon and stops working.

                        var separator = deepestChildPath.contains('/') ? '/' : '\\';

BenedekFarkas · 2016-04-18T21:46:31Z

Hm, I tested that one quite extensively though, but you're right about the JS problem anyway. That change btw will be unnecessary once #6756 is fixed - I applied it as a temporary solution, because it was easier to make it work there first (to be able to use the Media Library properly with Azure Storage and test other things).

hannan-azam · 2016-04-18T21:49:09Z

@sebastienros @BenedekFarkas Regarding the actual issue, below are my findings and recommendations:

I read this article https://perishablepress.com/stop-using-unsafe-characters-in-urls/ which references RFC 1738 http://www.ietf.org/rfc/rfc1738.txt with recommendations on not using the characters `blank/empty space and " < > # % { } | \ ^ ~ [ ]`` for the URIs. I have pasted the description from RFC on why these characters are unsafe at the end.

Recommendation: HttpUnallowed should include the Unsafe characters mentioned in the RFC.

Unsafe:

Characters can be unsafe for a number of reasons. The space
character is unsafe because significant spaces may disappear and
insignificant spaces may be introduced when URLs are transcribed or
typeset or subjected to the treatment of word-processing programs.
The characters "<" and ">" are unsafe because they are used as the
delimiters around URLs in free text; the quote mark (""") is used to
delimit URLs in some systems. The character "#" is unsafe and should
always be encoded because it is used in World Wide Web and in other
systems to delimit a URL from a fragment/anchor identifier that might
follow it. The character "%" is unsafe because it is used for
encodings of other characters. Other characters are unsafe because
gateways and other transport agents are known to sometimes modify
such characters. These characters are "{", "}", "|", "", "^", "~",
"[", "]", and "`".

All unsafe characters must always be encoded within a URL. For
example, the character "#" must be encoded within URLs even in
systems that do not normally deal with fragment or anchor
identifiers, so that if the URL is copied into another system that
does use them, it will not be necessary to change the URL encoding.

sebastienros · 2016-05-26T19:57:11Z

Conflicts. Do we still need it?

BenedekFarkas · 2016-05-26T21:18:30Z

I've been meaning to come back to this. I'll try to check it soon and see what to do.

src/Orchard.Web/Modules/Orchard.MediaLibrary/Services/MediaLibraryService.cs

# Conflicts: # src/Orchard.Web/Modules/Orchard.MediaLibrary/Services/MediaLibraryService.cs

…vider, so that MediaLibrary components don't need to do it separately

…rovider

BenedekFarkas · 2024-04-08T16:04:16Z

On the latest episode of reviving old PRs, please see the PR description for what changed.
If you can test file system and Azure storage (e.g., with Azurite), that's a bonus (I did, but extra set of eyes help!).

sebastienros · 2024-04-11T17:08:54Z

src/Orchard.Web/Modules/Orchard.Azure/Services/FileSystems/AzureFileSystem.cs

@@ -260,6 +273,11 @@ public class AzureFileSystem {
            path = ConvertToRelativeUriPath(path);
            newPath = ConvertToRelativeUriPath(newPath);

+            if (FileSystemStorageProvider.FileNameContainsInvalidCharacters(Path.GetFileName(newPath), out var invalidCharacters)) {
+                throw new ArgumentException(
+                    string.Format("The new file name contains invalid character(s): {0}", string.Join(", ", invalidCharacters)));


If that shows to the user, translate

Addressed in 08ae5dd.
I created a new type of exception to be able to handle such cases correctly and without breaking changes or reaching through abstractions.

The strings passed when building those exceptions are still un-translated in AzureFileSystem.cs (while they are localizable in other places). It seems like that message is then never used directly, because in the catch statements a different message is "forwarded" as error text: however, I think it would still be better to have these strings localizable, so we can use the exception message directly.

That's what we discussed yesterday with Sebastien and after your comment I dug even deeper into it, but localization doesn't work for AzureFileSystem, because it's not a registered type in the DI container, which the LocalizationModule relies on to find where translations can be injected. I added an Autofac module to register AzureFileSystem and with that, LocalizationModule finds it, but translated strings aren't applied still. My guess is that in this case, the Activated handler of this registered component is never called (even if it's registered with .AutoActivate(), because it's not injected/used as a dependency (it's a parent class).

I don't think it's worth pursuing this any further.

This is already a big improvement, because:

This validation is now on the storage provider-level, so it doesn't just apply to Media Library.

Validation is now applied for the Azure storage provider too with the same logic as the file system.

Media Library didn't have validation at all possible places in the first place.

AzureFileSystem already had non-localized exception messages, so unless the Azure storage is heavily refactored, we'll just have to live with that.

src/Orchard/FileSystems/Media/FileSystemStorageProvider.cs

sebastienros · 2024-04-11T17:11:54Z

src/Orchard/FileSystems/Media/FileSystemStorageProvider.cs

+        }
+
+        public static bool FileNameContainsInvalidCharacters(string folderName, out char[] invalidCharacters) {
+            invalidCharacters = folderName.Distinct().Intersect(InvalidFileNameCharacters).ToArray();


string.IndexOfAny()

Addressed in 15dc19c.

…racters precisely at various user-facing components

…ed due to invalid characters

* 6748: Stricter file and folder name validation (#6792) * Media Library: More strict file and folder name validation, fixes #6748 * Resetting MediaLibraryService changes to 1.10.x * Code styling in FileSystemStorageProvider * Adding string file and folder name validation to FileSystemStorageProvider, so that MediaLibrary components don't need to do it separately * Applying the same file and folder name validation to AzureFileSystem too * Code styling and fixes in AzureFileSystem, MediaLibrary and IStorageProvider * Simplifying invalid character detection * Code styling * Adding InvalidNameCharacterException to be able to handle invalid characters precisely at various user-facing components * Updating MediaLibrary not to log an error when a file can't be uploaded due to invalid characters --------- Co-authored-by: Lombiq <github@lombiq.com> * #6793: Adding a content-independent culture selector shape for the front-end (#8784) * Adds a new CultureSelector shape for front-end * fixed query string culture change * Moving NameValueCollectionExtensions from Orchard.DynamicForms and Orchard.Localization to Orchard.Framework * Code styling * Simplifying UserCultureSelectorController and removing the addition of the culture to the query string * EOF empty lines and code styling * Fixing that the main Orchard.Localization should depend on Orchard.Autoroute * Code styling in LocalizationService * Updating LocalizationService to not have to use IEnumerable.Single * Matching culture name matching in LocalizationService culture- and casing-invariant --------- Co-authored-by: Sergio Navarro <jersio@hotmail.com> Co-authored-by: psp589 <pablosanchez589@gmail.com> * #8640: Fixing consistency between different Enumeration Field flavors' data storage (#8789) * Reworking EnumerationField's logic to store/retrieve its (selected) values * Fixing exception when creating new item with CheckboxList flavor, adding more nullchecks and compactness * Code styling in EnumerationFieldDriver * Code styling in EnumerationField editor template * Fixing that EnumerationFieldDriver and the EnumerationField editor template should read SelectedValues instead of Values directly --------- Co-authored-by: Matteo Piovanelli <MatteoPiovanelli-Laser@users.noreply.github.com> * Fixing merge --------- Co-authored-by: Lombiq <github@lombiq.com> Co-authored-by: Sergio Navarro <jersio@hotmail.com> Co-authored-by: psp589 <pablosanchez589@gmail.com> Co-authored-by: Matteo Piovanelli <MatteoPiovanelli-Laser@users.noreply.github.com>

Media Library: More strict file and folder name validation, fixes #6748

c744388

dnfclas added the cla-required label Apr 17, 2016

dnfclas added cla-signed and removed cla-required labels Apr 17, 2016

sebastienros added the needs-sign-off label Apr 18, 2016

dnfadmin removed the cla-signed label Dec 11, 2017

BenedekFarkas mentioned this pull request Aug 26, 2019

7572: FileSystemStorageProvider shouldn't allow creating folders with "&" in their name #7577

Closed

sebastienros reviewed Mar 21, 2024

View reviewed changes

src/Orchard.Web/Modules/Orchard.MediaLibrary/Services/MediaLibraryService.cs Outdated Show resolved Hide resolved

agriffard reviewed Mar 21, 2024

View reviewed changes

src/Orchard.Web/Modules/Orchard.MediaLibrary/Services/MediaLibraryService.cs Outdated Show resolved Hide resolved

BenedekFarkas added 7 commits March 26, 2024 13:58

Merge branch '1.10.x' into issue/6748

475000c

# Conflicts: # src/Orchard.Web/Modules/Orchard.MediaLibrary/Services/MediaLibraryService.cs

Merge branch '1.10.x' into issue/6748

cd313f9

Resetting MediaLibraryService changes to 1.10.x

c72dc2b

Code styling in FileSystemStorageProvider

d3e02cd

Adding string file and folder name validation to FileSystemStoragePro…

7aa946d

…vider, so that MediaLibrary components don't need to do it separately

Applying the same file and folder name validation to AzureFileSystem too

248ccb5

Code styling and fixes in AzureFileSystem, MediaLibrary and IStorageP…

c3c70ec

…rovider

BenedekFarkas changed the title ~~Media Library: More strict file and folder name validation, fixes #6748~~ 6748: Media Library: More strict file and folder name validation Apr 8, 2024

BenedekFarkas changed the title ~~6748: Media Library: More strict file and folder name validation~~ 6748: Stricter file and folder name validation Apr 8, 2024

BenedekFarkas linked an issue Apr 8, 2024 that may be closed by this pull request

Stricter file and folder name validation #6748

Closed

BenedekFarkas requested review from MatteoPiovanelli-Laser and sebastienros April 8, 2024 16:03

BenedekFarkas requested a review from agriffard April 8, 2024 16:03

sebastienros reviewed Apr 11, 2024

View reviewed changes

BenedekFarkas added 5 commits April 12, 2024 10:48

Merge branch '1.10.x' into issue/6748

71b2ccd

Simplifying invalid character detection

15dc19c

Code styling

531deba

Adding InvalidNameCharacterException to be able to handle invalid cha…

08ae5dd

…racters precisely at various user-facing components

Updating MediaLibrary not to log an error when a file can't be upload…

e43e3d9

…ed due to invalid characters

BenedekFarkas requested a review from sebastienros April 12, 2024 11:09

Merge branch '1.10.x' into issue/6748

8bb939c

BenedekFarkas self-assigned this Apr 18, 2024

sebastienros approved these changes Apr 18, 2024

View reviewed changes

BenedekFarkas merged commit 0b86413 into 1.10.x Apr 18, 2024
3 checks passed

BenedekFarkas mentioned this pull request May 14, 2024

FileSystemStorageProvider shouldn't allow creating folders with "&" in their name #7572

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

6748: Stricter file and folder name validation #6792

6748: Stricter file and folder name validation #6792

BenedekFarkas commented Apr 17, 2016 •

edited

dnfclas commented Apr 17, 2016

sebastienros commented Apr 18, 2016

BenedekFarkas commented Apr 18, 2016

hannan-azam commented Apr 18, 2016

hannan-azam commented Apr 18, 2016

BenedekFarkas commented Apr 18, 2016

hannan-azam commented Apr 18, 2016 •

edited

sebastienros commented May 26, 2016

BenedekFarkas commented May 26, 2016 •

edited

BenedekFarkas commented Apr 8, 2024 •

edited

sebastienros Apr 11, 2024

BenedekFarkas Apr 12, 2024

MatteoPiovanelli-Laser Apr 12, 2024

BenedekFarkas Apr 12, 2024 •

edited

sebastienros Apr 11, 2024

BenedekFarkas Apr 12, 2024

6748: Stricter file and folder name validation #6792

6748: Stricter file and folder name validation #6792

Conversation

BenedekFarkas commented Apr 17, 2016 • edited

dnfclas commented Apr 17, 2016

sebastienros commented Apr 18, 2016

BenedekFarkas commented Apr 18, 2016

hannan-azam commented Apr 18, 2016

hannan-azam commented Apr 18, 2016

BenedekFarkas commented Apr 18, 2016

hannan-azam commented Apr 18, 2016 • edited

sebastienros commented May 26, 2016

BenedekFarkas commented May 26, 2016 • edited

BenedekFarkas commented Apr 8, 2024 • edited

sebastienros Apr 11, 2024

Choose a reason for hiding this comment

BenedekFarkas Apr 12, 2024

Choose a reason for hiding this comment

MatteoPiovanelli-Laser Apr 12, 2024

Choose a reason for hiding this comment

BenedekFarkas Apr 12, 2024 • edited

Choose a reason for hiding this comment

sebastienros Apr 11, 2024

Choose a reason for hiding this comment

BenedekFarkas Apr 12, 2024

Choose a reason for hiding this comment

BenedekFarkas commented Apr 17, 2016 •

edited

hannan-azam commented Apr 18, 2016 •

edited

BenedekFarkas commented May 26, 2016 •

edited

BenedekFarkas commented Apr 8, 2024 •

edited

BenedekFarkas Apr 12, 2024 •

edited