PhysicalFileProvider can't find ".env" file
HaoK opened this issue · 24 comments
Maybe files that start with .causing issues?
Found by aspnet/Configuration#505
Simple repro:
[Fact]
public void CanAddDotEnvFile()
{
#if NET451
_basePath = AppDomain.CurrentDomain.GetData("APP_CONTEXT_BASE_DIRECTORY") as string ??
AppDomain.CurrentDomain.BaseDirectory ??
string.Empty;
#else
_basePath = AppContext.BaseDirectory ?? string.Empty;
#endif
File.WriteAllText(Path.Combine(_basePath, "works.env"), "foo");
Assert.True(File.Exists(Path.Combine(_basePath, "works.env")));
Assert.True(new PhysicalFileProvider(_basePath).GetFileInfo("works.env").Exists);
File.WriteAllText(Path.Combine(_basePath, ".env"), "foo");
Assert.True(File.Exists(Path.Combine(_basePath, ".env")));
Assert.True(new PhysicalFileProvider(_basePath).GetFileInfo(".env").Exists); // Failes
}
If I dig a little after #51, it specifically references globbing. I am not globbing.
My use case is that I'm explicitly trying to load the file. I shouldn't lose access to files that start with .. The only automatic behaviour you should really want is to not have them included in non-verbose/non-all directory listings.
Otherwise, they're just files like any other.
I don't have context on the original design decisions. Changing current behavior could be an issue. For now, users can workaround by using System.IO directly.
cc @muratg That said, it's worth revisiting this behavior. This is not the first time the issue has been raised. IMO the necessity of dropping out to System.IO indicates a problem in the abstraction.
@natemcmaster Agreed. We should look into this in 2.0 and reconsider the behavior.
Is fixing this in 1.* really that big of a regression? I know there would be some component of assumption, but the current behaviour is fairly inconsistent with expectations on platforms that do support dot-files.
Would stand to reason more people are being blindsided by current behaviour than those who are intentionally relying on it.
I suppose that's a possibility, although "files that start with a dot are secure" is not something I'd say is true enough to need to stick to. The implementation in that case will have lead people to a bad assumption.
"Files that start with a dot" are simply exempt from globbing/bulk operations. So the idea that there's a security angle here assumes the security analysis done earlier was correct. It only compounds the error to assume that the behaviour needs to be preserved.
Worth remembering throughout: I'm only looking for a single file, not a glob. Might be a way to address it in 1.x by making the distinction that way?
There's a convention around . directory & files that they're not included in normal operations, be they github checkins, or, well, anything else. It's not saying they're secured, just that there's an assumption they're ignored. Hence the security concern.
There's a convention around . directory & files that they're not included in normal operations, be they github checkins, or, well, anything else. It's not saying they're secured, just that there's an assumption they're ignored. Hence the security concern.
It's one thing to ignore them in most operations. It's another to block explicitly reading them when you want. Given that IFileProvider is the abstraction for reading files. The equivalent would be if File.Read didn't let you open .env files. Kinda broken.
True. If you could fix it, but ensure the static file provider didn't serve them, and it didn't serve them if they were hidden/system that would mollify my concerns. What about excluding them from directory listings too?
True. If you could fix it, but ensure the static file provider didn't serve them, and it didn't serve them if they were hidden/system that would mollify my concerns. What about excluding them from directory listings too?
Well the logic for not serving . Files can live in the static file handler. That kind of check shouldn't go in the low level file abstractions layer of GetFileInfo since we end up using that for reading config files (which we should allow and is currently blocked as a result)
Sure, but both would need updated at the same time.
We could separate the two concerns - have a regular PhysicalFileProvider and a SafeToServePhysicalFileProvider which we use in StaticFiles and such.
StaticFiles already makes decisions based on extension doesn't it?
I guess if we want this behavior changed in a non breaking way, we can just introduce a flag to PhysicalFileProvider that optionally disables the call blocking some of the checks in here (not sure if we want to allow only ".", or also hidden but never system.
Then for configuration at least, we can switch to setting the allowHiddenFiles = true as the default provider.
We'd still need to update the static file handler too. I'd be careful about using Hidden, as that has an actual meaning on NTFS.
Well what I'm suggesting is there would be no change in behavior for the static file handler since its using the same code path, we'd just add an opt out...
IFileProvider and PhysicalFileProvider were originally built for StaticFiles and most of the file access restrictions were implemented inside PhysicalFileProvider. About the only thing StaticFiles does itself is check the file extension to look up the mime-type.
When PhysicalFileProvider started getting used in a more general context non of the design decisions were re-evaluated.
I like the opt-in model - keep the default behavior as is, and have an option to serve all files as this is being used in a more general context than it was originally designed for.
cc @Eilon
Wouldn't that assume that dot files actually represent something that needs to be secured in the first place?
@atrauzzi yes because in the *nix world that's generally true. Starting with a . means it's hidden.
But, I agree that for the purposes of components such as loading config files, these rules don't make sense, and we should have a way to load any reachable file.
@Eilon - As someone coming from the unix world, it's very obvious that there's some conflation of "hidden" and "secure" here. I'm only highlighting this because numerous times in this discussion, it keeps getting mentioned, despite me pointing out the misunderstanding.