Add API to allow the physical file provider to show hidden files by natemcmaster · Pull Request #280 · aspnet/FileSystem

natemcmaster · 2017-06-20T18:09:53Z

Resolves #186

The default behavior remains consistent: files beginning with a period, Hidden and System files are filtered from the watcher, enumeration, and any other file listing.

The ExclusionFilters enum allows consumers to override this behavior by passing in these flags to one of the new overrides.

cc @dazinator

blowdart · 2017-06-20T18:19:59Z

Not sure about HiddenFileOptions as a name - hidden files has a specific meaning on NTFS, and you're including that, plus system, plus dot files. Maybe ExcludedFiles?

Also is that enum a flag? Shouldn't we then mark it as such, or has that fallen out of favour? Then we'd switch to None / All as names, and remove Show from the others.

natemcmaster · 2017-06-20T18:29:29Z

Also is that enum a flag?

Yes, it's a flag.

Naming stuff is hard. ExcludedFiles sounds better than HiddenFileOptions. Is this what you had in mind?

[Flags]
public enum ExcludedFiles
{
   Default = 0xFFFF,
   None = 0,
   StartingWithPeriod = 0x0001,
   Hidden = 0x0002,
   System = 0x0004,
}

blowdart · 2017-06-20T18:50:45Z

All rather than default, but yes.

Maybe PeriodPrefixed rather than StartingWithPeriod? Period is also very american, maybe DotPrefixed?

natemcmaster · 2017-06-20T18:56:29Z

DotFiles?

ExcludedFiles.All is too ambiguous. Looks like it means "exclude all files regardless of name or attributes", even though it really means "use all filters".

blowdart · 2017-06-20T19:01:25Z

Ah yea, all doesn't make sense. Default is kinda smelly though.

Does Dot* whatever also include directories? If so, DotFiles doesn't work either.

natemcmaster · 2017-06-20T19:04:51Z

Hmm, yup, includes directories. DotPrefixed more correct. That or ExcludedFiles.FileOrDirectoryStartingWithPeriod_FyiPeriodMeansAscii0x2E

blowdart · 2017-06-20T19:05:21Z

Or PeriodMeansFullStopInProperEnglishNotSimplifiedEnglish ...

dazinator · 2017-06-20T19:53:23Z

Naming aside, I like the guts of this.

I still think its weird filtering out dotprefixed files / folders by default - (I think this is special casing for tooling that wants to ignore things like .git folder) but I understand why you want to keep the default behaviour compatible.

Flags for the win.

nil4 · 2017-06-20T20:06:40Z

Shouldn't overloads of GetDirectoryContents and Watch be also added, taking ExcludedFiles?

natemcmaster · 2017-06-20T20:34:32Z

Shouldn't overloads of GetDirectoryContents and Watch be also added, taking ExcludedFiles?

That's a good observation. I originally couldn't come up with a scenario for these overloads that aren't solved by changing options passed to the PhysicalFilesProvider constructor overload. @nil4 do you have scenarios in mind? GetDirectoryContents will be easy to overload. Watch is going to be tricky.

natemcmaster · 2017-06-20T21:48:11Z

Attempt 3 at picking good names

    [Flags]
    public enum ExcludedFiles
    {
        Sensitive = DotPrefixed | Hidden | System,
        DotPrefixed = 0x0001,
        Hidden = 0x0002,
        System = 0x0004,
        None = 0
    }

nil4 · 2017-06-20T22:00:09Z

I don't have an actual scenario in mind, it's just that the abstraction seems inconsistent if one can pass ExcludedFiles flags to GetFileInfo but no other method. Let me ask the question in reverse: assume I'd construct PhysicalFileProvider with explicit ExcludedFiles flags. What scenario requires passing different flags to GetFileInfo?

To put it another way, if directory listing and Watch coverage is bound by one set of flags, how would my app learn about the existence of a file that doesn't match them so that it can call GetFileInfo on it?

It seems to me either no overload should take ExcludedFiles flag, or all of them do, to make the abstraction self-consistent.

One argument to all methods having overloads would be the following: I'd like to use the default file provider for the static files middleware and not serve sensitive files by default, to keep my app secure. Using the same file provider, I should be able to pass explicit flags in those areas of my app where I need to enumerate the full contents of the file system, bypassing the default filters.

I agree that Watch may be trickier, but wouldn't it have to support filtering by exclude flags anyway, i.e. those passed in the constructor?

nil4 · 2017-06-20T22:03:27Z

I just saw your last commit now and it makes my feedback obsolete. Thanks! 👍

natemcmaster · 2017-06-20T22:03:41Z

FYI - I added GetDirectoryContents.

What scenario requires passing different flags to GetFileInfo?

I'd like to use the default file provider for the static files middleware and not serve sensitive files by default, to keep my app secure. Using the same file provider, I should be able to pass explicit flags in those areas of my app where I need to enumerate the full contents of the file system, bypassing the default filters.

You basically explained the scenario I'm thinking of. In many places in the code, you don't really want to allocate a new provider just to get a "hidden" file in one part of the code.

natemcmaster · 2017-06-20T22:04:22Z

~~Working on adding .Watch() too. Not has hard as I thought. Just need to flow the flags to the right place :).~~

blowdart · 2017-06-20T22:08:37Z

Sensitive works. Because you are a gentle soul :D

natemcmaster · 2017-06-20T23:04:08Z

Working on adding .Watch() too.

Scratch that. Turns out this is expensive and mangles the code. It's better to just create a new watcher altogether if you want to override the file provider's exclusion setting.

Tratcher · 2017-06-20T23:09:37Z

+        /// <paramref name="subpath" /> is absolute, if the directory does not exist, or <paramref name="subpath" /> has invalid
+        /// characters.
+        /// </returns>
+        public IDirectoryContents GetDirectoryContents(string subpath, ExcludedFiles excludedFiles)


Why add this method? It doesn't look like it's used by anything else here, and it's hidden the IFileProvider interface

Same reason for adding the GetFileInfo overload. I'd like to use the default file provider most of the time, but still be able to bypass its filtering in special cases.

pranavkm · 2017-06-20T23:59:44Z

+    /// Specifies while files are excluded by the physical file provider.
+    /// </summary>
+    [Flags]
+    public enum ExcludedFiles


FileFilterOptions \ FileFilter?

Ah yes, this still says files when it could also be directories.

ExclusionFilters?

That sounds like a good name.

pranavkm · 2017-06-21T00:01:05Z

+            {
+                return false;
+            }
+            else if (fileSystemInfo.Name.StartsWith(".") && (excluded & ExcludedFiles.DotPrefixed) != 0)


StartsWith(".", StringComparison.Ordinal)

No non ordinal string comparisons! Security!

Will update.

pranavkm · 2017-06-21T00:11:09Z

+        /// <param name="excludedFiles">Specifies which files are excluded from the directory listing.</param>
+        public PhysicalDirectoryContents(string directory, ExcludedFiles excludedFiles)
+        {
+            _directory = directory ?? throw new ArgumentNullException(nameof(directory));


Nope, we don't use throw expressions in our code.

They have Fowler's (relucant) approval. I'll conceded throw expressions are not always the right choice, but I think they're a win for simple ctor checks.

pranavkm · 2017-06-21T00:12:27Z

+    public enum ExcludedFiles
+    {
+        /// <summary>
+        /// Exclude files and directories when the name begins with a period, or has either <see cref="FileAttributes.Hidden"/> or <see cref="FileAttributes.System"/> is set on <see cref="FileSystemInfo.Attributes"/>.


Can we list the individual features here (combination of DotPrefixed, Hidden and System rather than re-iterate the description?

Will update

pranavkm · 2017-06-21T15:24:17Z

+        /// <param name="subpath">A path under the root directory</param>
+        /// <param name="excludedFiles">Overrides default file exclusion behavior.</param>
+        /// <returns>The file information. Caller must check <see cref="IFileInfo.Exists"/> property. </returns>
+        public IFileInfo GetFileInfo(string subpath, ExcludedFiles excludedFiles)


This feels weird - why have the options at two different levels? It doesn't seem intuitive, for instance, if I specified ExcludedFiles.Hidden in the ctor, and ExcludedFiles.System here, I'd sort of expect it to filter both, not just the latter.

Consider another scenario. The ctor was set to Hidden | DotPrefixed | System. If I call .GetFileInfo(".env", ExcludedFiles.None), what should happen? IMO the override behavior is easier to reason about.

pranavkm · 2017-06-21T15:26:25Z

+
+        [Theory]
+        [MemberData(nameof(Combinations))]
+        public void FiltersExcludedFiles(string filename, FileAttributes attributes, ExcludedFiles excludedFiles, bool excluded)


Make these two separate tests. One for exclusion and one for inclusion.

Not sure I understand the request. Am I missing scenarios? Or is this just a coding preference?

The excluded parameter is essentially an if and ifs in a test are generally an indication of a conflating different scenarios.

pranavkm · 2017-06-21T15:27:07Z

    <TestSdkVersion>15.3.0-*</TestSdkVersion>
    <XunitVersion>2.3.0-beta2-*</XunitVersion>
-    <MoqVersion>4.7.1</MoqVersion>
+    <MoqVersion>4.7.58</MoqVersion>


Make this a separate PR. Completely unrelated to this change.

Seems like overkill. We're already in agreement that we want to upgrade. Ryan just hasn't done this repo yet. https://github.com/aspnet/Coherence-Signed/issues/583.

That was easy: #281

natemcmaster · 2017-06-21T16:00:27Z

Attempt 4 at picking good names

    [Flags]
    public enum ExclusionFilters
    {
        Sensitive= DotPrefixed | Hidden | System,
        DotPrefixed = 0x0001,
        Hidden = 0x0002,
        System = 0x0004,
        None = 0
    }

natemcmaster · 2017-06-21T16:39:35Z

🆙 📅 thanks for all the feedback so far. Here is the public API as it currently stands in latest draft of this PR.

natemcmaster · 2017-06-21T17:23:08Z

Per in person discussion with @glennc and @Tratcher, we won't overload methods, just constructors. This scopes back this feature but still unblocks those that want to get access to 'hidden' files through this abstraction.

natemcmaster · 2017-07-17T17:19:07Z

🔔 now that we've branched for 2.1, @Tratcher are you okay with this API addition?

The default behavior remains consistent: files beginning with a period, Hidden and System files are filtered from the watcher, enumeration, and any other file listing. Adds ExclusionFilters. These flags to control the filtering behavior of the physical files provider.

Eilon · 2017-07-17T21:10:15Z

+    /// <summary>
+    /// Helpful methods for FileSystem APIs.
+    /// </summary>
+    public static class FileSystemInfoHelper


Can this type stay internal? We kind of mostly regret having so many 'pub-ternal' types, so we'd rather not add too many more...

Sure. It's not meant to be used by other components.

Yeah that would be good... less future pain for us. Can you send an update for that? (New PR of course)

evil-shrike · 2017-09-15T10:28:58Z

Hi. I as many others encountered shortcomings in PhysicalFileProvider in 2.0 and had to create my own implementation for allowing serving dot-files.
Let me suggest more enhancements to PhysicalFileProvider.

enum flags ExclusionFilters can be fine but wouldn't be safer to add an options object to ctor signature to allow future feature additions:

public PhysicalFileProvider(string root, ExclusionFilters filters)

what if it will be needed to add something more? Another argument?
I suggest creating PhysicalFileProviderOptions:

public class PhysicalFileProviderOptions 
{
    ExclusionFilters ExclusionFilters { get;set }
}

and use it as ctor argument instead:

public PhysicalFileProvider(string root, PhysicalFileProviderOptions options)

Besides flags for exclusion modes I'd suggest adding Glob-patttens

public class PhysicalFileProviderOptions 
{
    ExclusionFilters ExclusionFilters { get;set }
    string[] ExcludePaths { get;set }
}

usage:

new PhysicalFileProvider("root", new PhysicalFileProviderOptions { 
  // I need include dot-files but exclude others (not very handy btw)
  ExclusionFilters = ExclusionFilters.Sensitive & ~ExclusionFilters.DotPrefixed ,
  // exclude ".idea" subfolder with all contentl; exclude *.map files indide 'src' folder
  ExcludePaths = new[] { ".idea/", "src/**/*.map" } 
});

I know you will say - use StaticFileOptions.OnPrepareResponse callback in StaticFilesMiddleware. But the same is appliable for dot-files - why not to filter them inside middleware instead of FileProvider?

serving files with dots if it's just js/css is definitely normal, why are they excluded by default?
Moreover it's a breaking change as compared with IIS + ASP.NET 4.
So everybody have to write this expression over and over:
ExclusionFilters.Sensitive & ~ExclusionFilters.DotPrefixed
Suggestion - include dot-files by default (but exclude folders starting with ".")
anyway of default, if we have the options object I'd suggest to move "exclude dot-files" option into a property:

public class PhysicalFileProviderOptions 
{
    ExclusionFilters ExcludeByAttributes { get;set }
    string[] Exclude { get;set }
    bool IncludeDotFile { get; set } // or ExcludeDotFiles
}

make GetFileInfo and GetDirectoryContents methods virtual to allow overriding PhysicalFileProvider
make PathUtils are public as currently it's hard to implement own PhysicalFileProvider with the similar logic - we have to copy-paste methods like PathNavigatesAboveRoot, HasInvalidPathChars and so on.
Support special files inside folder like .ignore which would tell FileProvider to ignore the folder with all content inside. The file name would be passed via options object

public class PhysicalFileProviderOptions 
{
    ExclusionFilters ExcludeByAttributes { get;set }
    string[] Exclude { get;set }
    bool IncludeDotFile { get; set } // or ExcludeDotFiles
    string IgnoreFileName {get;set;}
}

Tratcher · 2017-09-15T12:54:27Z

@evil-shrike this PR was closed quite a while ago. Please open a new issue with your feedback.

natemcmaster requested a review from davidfowl June 20, 2017 18:09

dnfclas added the cla-already-signed label Jun 20, 2017

natemcmaster requested a review from BrennanConroy June 20, 2017 18:10

natemcmaster requested a review from Tratcher June 20, 2017 18:22

Tratcher reviewed Jun 20, 2017

View reviewed changes

pranavkm reviewed Jun 21, 2017

View reviewed changes

natemcmaster removed the request for review from BrennanConroy June 21, 2017 18:11

natemcmaster force-pushed the namc/hidden branch from 4decb28 to cf0f6c1 Compare July 3, 2017 17:22

Tratcher approved these changes Jul 17, 2017

View reviewed changes

natemcmaster force-pushed the namc/hidden branch from cf0f6c1 to a97b3ef Compare July 17, 2017 19:15

natemcmaster added this to the 2.1.0-preview1 milestone Jul 17, 2017

natemcmaster merged commit a97b3ef into dev Jul 17, 2017

natemcmaster deleted the namc/hidden branch July 17, 2017 20:58

natemcmaster mentioned this pull request Jul 17, 2017

PhysicalFileProvider can't find ".env" file #232

Closed

Eilon reviewed Jul 17, 2017

View reviewed changes

evil-shrike mentioned this pull request Sep 15, 2017

Add API to allow access to hidden files via PhysicalFilesProvider #186

Closed

evil-shrike mentioned this pull request Sep 15, 2017

PhysicalFileProvider: improvements for filtering options #293

Closed

aspnet-hello mentioned this pull request Dec 15, 2018

PhysicalFileProvider: improvements for filtering options dotnet/extensions#756

Closed

YuKitsune mentioned this pull request Jun 25, 2021

[Question] Why are dot-prefixed files considered "Sensitive"? dotnet/runtime#54712

Closed

Uh oh!

Conversation

natemcmaster commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blowdart commented Jun 20, 2017

Uh oh!

natemcmaster commented Jun 20, 2017

Uh oh!

blowdart commented Jun 20, 2017

Uh oh!

natemcmaster commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blowdart commented Jun 20, 2017

Uh oh!

natemcmaster commented Jun 20, 2017

Uh oh!

blowdart commented Jun 20, 2017

Uh oh!

dazinator commented Jun 20, 2017

Uh oh!

nil4 commented Jun 20, 2017

Uh oh!

natemcmaster commented Jun 20, 2017

Uh oh!

natemcmaster commented Jun 20, 2017

Uh oh!

nil4 commented Jun 20, 2017

Uh oh!

nil4 commented Jun 20, 2017

Uh oh!

natemcmaster commented Jun 20, 2017

Uh oh!

natemcmaster commented Jun 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blowdart commented Jun 20, 2017

Uh oh!

natemcmaster commented Jun 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

natemcmaster Jun 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

natemcmaster commented Jun 20, 2017 •

edited

Loading

natemcmaster commented Jun 20, 2017 •

edited

Loading

natemcmaster commented Jun 20, 2017 •

edited

Loading

natemcmaster Jun 21, 2017 •

edited

Loading

natemcmaster commented Jun 21, 2017 •

edited

Loading

natemcmaster commented Jun 21, 2017 •

edited

Loading