Add support for parsing templates text from the file system fs instead of the host operating system's file system

[aantelov87]

Now that we have fs package, it would be useful to add ParseFS from the standard library package html/template into the safehtml/template API.

func (t *Template) ParseFS(fs fs.FS, patterns ...string) (*Template, error)

[kele]

Currently, the safehtml/template API supports parsing templates from TrustedSources. The fs.FS is an interface that can be arbitrarily implemented, therefore it cannot be trusted as is.

The way to go could be implementing it using embed.FS as the parameter instead.

[jba]

I actually need to use embed.FS in a project (golang/go#48410), so I'm willing to make this happen. Here are two designs. The first is minimal:

func ParseFS(fs embed.FS, patterns ...string) (*Template, error)
func (*Template) ParseFS(fs embed.FS, patterns ...string) (*Template, error)

That mimics the standard library changes, except for using embed.FS instead of fs.FS.

But I think it would be worth doing something more general, in anticipation of other trusted FSs. This would also let code be agnostic about where the FS came from, much like you can do with file paths now:

type TrustedFS
func TrustedFSFromConstant(stringConstant) TrustedFS
// perhaps TrustedFSFrom{Flag,EnvVar}, etc.
func TrustedFSFromEmbed(embed.FS) TrustedFS

func ParseFS(fs TrustedFS, patterns ...string) (*Template, error)
func (*Template) ParseFS(fs TrustedFS, patterns ...string) (*Template, error)

A third design would allow you to create a TrustedSource from an FS. I haven't thought through whether that can work—does every operation on a TrustedSource make sense with an FS as well as with a file path? For example, what would

func TrustedSourceFromConstantDir(dir stringConstant, src TrustedSource, filename string) (TrustedSource, error)

do if the middle argument was an FS? Although it seems more elegant to have a single concept, it may end up being more confusing.

[aantelov87]

I do prefer the second design. In my opinion, it is the most clear and simple.

[jba]

I realized that if we add

func ToTrustedFS(TrustedSource) TrustedFS

then we don't need any TrustedFSFromXXX functions, so that makes it even simpler.

[kele]

I see the use case of embed.FS and having specialized functions to support this.

What is the use case of the other helpers, taking into account that we have ParseGlob?

[jba]

@kele, none right now that couldn't be done in other ways, but I see two reasons to generalize to TrustedFS now:

1. It would allow for easier testing, because the same call to ParseFS could take an embed.FS in production code or ToTrustedFS(TrustedSourceFromConstant("testdir")) in test code.
2. It would open the door to other FS implementations. For example, there's no reason to disallow parsing templates from a zip file under the programmer's control. If you wanted to support that you would just need to add a function to convert a zip.Reader (which implements fs.FS) to a TrustedFS.

I think the argument for TrustedFS is stronger than the one for ToTrustedFS (that is, converting an existing TrustedSource to a TrustedFS).

[kele]

1. It would allow for easier testing, because the same call to ParseFS could take an embed.FS in production code or ToTrustedFS(TrustedSourceFromConstant("testdir")) in test code.

Shouldn't one swap out the whole Template instead of the filesystem?

2. It would open the door to other FS implementations. For example, there's no reason to disallow parsing templates from a zip file under the programmer's control. If you wanted to support that you would just need to add a function to convert a zip.Reader (which implements fs.FS) to a TrustedFS.

How exactly would that work? Would we (a) keep adding more and more types to convert from to a TrustedFS, or (b) we'd allow users to somehow implement a TrustedFS themselves?

For (a): I assume we'd limit who could implement TrustedFS by e.g. adding a private method? If yes, I'd be okay with that from a security point of view, but I wonder whether we have enough cases to justify adding a layer of abstraction here.

For (b): How could we make sure we won't get unsafe TrustedFS implementations?

[jba]

@kele TrustedFS cannot be an interface, because that can't be done in a safe way. Even with a private method, clients could still embed the interface in their own structs. It would instead look like TrustedSource:

type TrustedFS struct {
    fsys fs.FS
}

[kele]

In this case, we will own all the constructors of TrustedFS and provide functions like FromEmbed(TrustedFS), FromConstantPath(stringConstant), FromZip(???), correct?

I struggle to see how we could provide any constructors other than from a string constant and an embed.FS. If these are the only two we can support, then I'm not sure it's worth it to introduce the a TrustedFS type, because this is one more thing that a developer needs to understand in order to use the API.

[jba]

Yes, you would own all the constructors.

I can't think of other constructors either at present. I see TrustedFS as future-proofing. If you don't think that's worth it, I'm fine with using an embed.FS directly.

[jba]

@kele, I pushed #8 but can't add reviewers. PTAL.

[aantelov87]

Closing the issue. Feature implemented in commit d6f0e11.