Item14361: Update config {ImageFormat} and library support for svg

[BramVan-Oosterhout]

• Config {GraphvizPlugin}{ImageFormat} = to support inline="on"
• library=Web.Topic parameter now honoured by type="svg"

Please review

[MichaelDaum]

This patch changes more than actuall attributed in the pull request's title. Can we make this a minimal one to address the issue at hand?

[BramVan-Oosterhout]

Apologies, The comment should refer to inline "off" rather than inline "on"

Config {GraphvizPlugin}{ImageFormat} = to support inline="off"
library=Web.Topic parameter now honoured by type="svg"

The inline "on" was supported by the {SvgFormat} provided in the current release.

I am not sure what you mean with "Can we make this a minimal one to address the issue at hand?"

The changes in Core.pm are supporting the library as follows (I use the new line numbers).

The current release supports svg images only if they are in the same directory as the svg code.
Adding the complete library path to the image in the svg is required for both inline "on" and inline "off"
This is addressed in line 193 and 194.

inline "on" processing remains unchanged in line 195-203

inline "off" needs to write the updated svg back to file. line 205
and the {ImageFormat} needs to be updated with the requested style, width and height, line 206

Non svg images also require the {ImageFormat} to be updated. line 209

The update of {ImageFormat} is the same, so I placed that in _updateFormat for readbility. line 220-233 rather than repeat the code?

Do you prefer to repeat the original code for the update of {ImageFormat}?

      $result = $this->{imageFormat};

      $style ||= '';
      $width ||= '';
      $height ||= '';
      $width = "width='".$width."'" if $width;
      $height = "height='".$height."'" if $height;

      $result =~ s/\$style/$style/g;
      $result =~ s/\$width/$width/g;
      $result =~ s/\$height/$height/g;

Instead of:

      $result = _updateFormat( $style, $width, $height, $this->{imageFormat} );

And should I change the title to:
Item14361: Update config {ImageFormat} and add library parameter support for svg

Or did you mean something else? Perhaps the issue raised in Item15299? But that Item does not deal with the svg embedded images.

[MichaelDaum]

Yes, well, the _updateFormat doesn't really seem to be related nor required, imho. The title is just fine. The only concern I have is security by setting the library path to some mallicious server side directory that you could pull in data from using dot. Does you patch protect against setting imagepath or library to /etc?

[BramVan-Oosterhout]

Happy to replace _updateFormat.

No, the patch does not reject a dot file containing imagepath=. That can be dealt with by scanning the svg file. I am reading it anyway to adjust the image links. One can insist that the path is:

• inside `$Foswiki::cfg{PubDir}
• does not contain ..
  Any other constraints you can think of?

Hmmm. Thinking about this more, the above does not protect against a malicious script attached to a topic. Should one check each file with File::Type? That can be done. As long as one limits imagepath to a single directory that won't be too onerous. If one allows the full imagepath syntax, one would need to repeat the lookup, which may be too much. Anyway, full imagepath is currently not supported through library or the manipulation of the svg.

I believe the library parameter is safe, because it is normalised to a Web.Topic name.

Changing the imagepath as a parameter on dot requires admin access to Foswiki or admin access to the server.

The former one can deny by scanning Foswiki::cfg{GraphvizPlugin}{DotCmd}. Is that useful?

The latter seems over the top.

As an alternative to all this, one could add a config setting{GraphvizPlugin}{AllowImagesInSvg} that specifically allows the use of images in svg and dispense with the checking. That allows the Admin to decide if they want to take the risk and at the same time make the risk explicit.

What do you think?