Rendering should not access FS by default (ImageHrefResolver)

[mattfbacon]

The default behavior of ImageHrefResolver.resolve_string is unsafe and seems like a bit of a footgun especially after we saw https://github.com/typst/typst/commit/00f7588#diff-afe406d9ab2906c6716cd07df82d089d25473a9fe7fc89408c633d20afd92d54R142-R145 where the default behavior had created a vulnerability in typst.

In my opinion there are two options that seem acceptable:

1. Make the default resolver not resolve any resources and always consider them not found (i.e., error)
2. Require passing a resource resolver for rendering, and provide no-op and FS-based resolvers as part of the library.

(Or do 1 and then change to 2 at the next opportunity for breaking changes)

Thoughts?

[RazrFalcon]

resvg works the way SVG spec specifies it to work. As simple as that. If you don't want to load external images then it's very easy to do so.

where the default behavior had created a vulnerability in typst

What kind of vulnerability? It cannot read a password or other stuff.

[LaurenzV]

The "vulnerability" is that Typst sandboxes the file system so that when compiling a Typst document, only files within the specified root can be accessed, not outside of it. However, since resvg (by default) automatically attempts to read any files linked via an SVG, this could be circumvented. Not a big attack surface since it only allows loading images and you would need to know the exact path to the image, but still.

I do also think that resvg (as a library, for the resvg binary I think t's fine if it accesses fs by default) should by default not try to access the file system. I think a lot of upstream crates might assume that resvg doesn't do any file accesses since it's "just a library for rendering SVGs", even though as you mentioned this technically also does include loading files from different locations.

Or maybe it's enough to just make a more visibile disclaimer that resvg does this by default, and that it can be disabled.

[mattfbacon]

I think a lot of upstream crates might assume that resvg doesn't do any file accesses since it's "just a library for rendering SVGs"

Exactly, this is my point. I believe in cases like this the content resolution strategy should be provided by the caller, interpreting FS access as an effect just like errors.

It cannot read a password or other stuff.

I mentioned the typst vulnerability to show that the default behavior is not obvious and the mistake was not noticed by anyone in the fairly active and skilled group of typst contributors. I did not mean to exaggerate the possible impacts.

[RazrFalcon]

I think a lot of upstream crates might assume that resvg doesn't do any file accesses since it's "just a library for rendering SVGs", even though as you mentioned this technically also does include loading files from different locations.

Ability to link external files is the core SVG feature. If we disable it people start complaining why external images are not loading. There are no good solutions. For now, just read the docs...

[mattfbacon]

I agree it is a core feature, that's why I suggested it that the resolver could be required when calling the renderer. I disagree that "there are no good solutions" and I don't see any issues with my suggestions.

As it is the docs are not obvious enough on this point to support "just read the docs" and the location with the relevant information is nested down multiple levels.

[RazrFalcon]

I don't see any issues with my suggestions

I do.

[mattfbacon]

Please tell me what they are, I am curious.