Retract version 2.0.3

[pepijnve]

asciidoctor/asciidoctor-diagram#292 pointed to a rather bad regression in 2.0.3 when rerendering diagrams. I've pulled 2.0.3 from rubygems.org already. I think we should do the same for the java version.

@ahus1 not sure if this affects the IntelliJ plugin already, but don't upgrade to 2.0.3 just yet.

[ahus1]

asciidoctorj-diagram hasn't been released yet, therefore IntelliJ hasn't upgraded yet. No harm done!

As you pulled release 2.0.3, the master of asciidoctorj-diagram will probably fail to build.

Depending on when you plan to publish a new 2.0.4 version it is up to @robertpanzer to decide if he wants to wait, or to roll back the master to 2.0.2.

[pepijnve]

Depending on when you plan to publish a new 2.0.4 version it is up to @robertpanzer to decide if he wants to wait, or to roll back the master to 2.0.2.

ASAP. I've already reproduced the issue and know how to fix it; but the day job takes precedence.

[robertpanzer]

@pepijnve I am just experimenting with 2.0.4 and I am not sure if there is a new issue.
I have the following test case:

        File inputFile = new File("build/resources/test/sample.adoc");
        File outputFile1 = new File(inputFile.getParentFile(), "asciidoctor-diagram-process.png");
        File outputFile2 = new File(inputFile.getParentFile(), ".asciidoctor/diagram/asciidoctor-diagram-process.png.cache");
        asciidoctor.requireLibrary("asciidoctor-diagram");
        asciidoctor.convertFile(inputFile,
                options()
                        .backend("html5")
                        .toDir(new File("build"))
                        .get());
        assertThat(outputFile1.exists(), is(true)); // fails here
        assertThat(outputFile2.exists(), is(true));

using this document:

= Document Title

[ditaa,asciidoctor-diagram-process]
....
                   +-------------+
                   | Asciidoctor |-------+
                   |   diagram   |       |
                   +-------------+       | PNG out
                       ^                 |
                       | ditaa in        |
                       |                 v
 +--------+   +--------+----+    /---------------\
 |        |---+ Asciidoctor +--->|               |
 |  Text  |   +-------------+    |   Beautiful   |
 |Document|   |   !magic!   |    |    Output     |
 |     {d}|   |             |    |               |
 +---+----+   +-------------+    \---------------/
     :                                   ^
     |          Lots of work             |
     +-----------------------------------+
....

This test fails in the check if outputFile1 exists.
And indeed it seems to be generated at an unexpected location:

Instead of ./asciidoctorj-diagram/build/resources/test/Users/robertpanzer/dev/asciidoctorj-diagram/asciidoctorj-diagram/build/asciidoctor-diagram-process.png I would have expected the png at ./asciidoctorj-diagram/build/resources/test/asciidoctor-diagram-process.png.
Is this expected from your end?

[pepijnve]

Argh, can't seem to get this release right... I'll check in a minute.

[pepijnve]

Looking at this on a bigger screen, it does look like the expected behaviour. asciidoctor-diagram writes to (in this order, first defined value)

• imagesoutdir
• outdir/imagesdir
• todir/imagesdir

since you're specifying todir and imagesdir is probably not set, that's where the image files end up.

Are you seeing different behaviour wrt 2.0.2 or earlier?

[robertpanzer]

The same test worked with 2.0.2 with the same version of AsciidoctorJ (and hence Asciidoctor), so something seems to have changed since then.

[pepijnve]

Ok, that's what I suspected. In 2.0.2 the logic to pick the output dir was

  def image_output_dir(parent)
    document = parent.document

    images_dir = parent.attr('imagesoutdir', nil, true)

    if images_dir
      base_dir = nil
    else
      base_dir = parent.attr('outdir', nil, true) || doc_option(document, :to_dir)
      images_dir = parent.attr('imagesdir', nil, true)
    end

    parent.normalize_system_path(images_dir, base_dir)
  end

the 2.0.4 version is

def image_output_dir(parent)
  images_dir = parent.attr('imagesoutdir', nil, true)

  if images_dir
    base_dir = nil
  else
    base_dir = output_base_dir(parent)
    images_dir = parent.attr('imagesdir', nil, true)
  end

  parent.normalize_system_path(images_dir, base_dir)
end

def output_base_dir(parent)
  parent.normalize_system_path(parent.attr('outdir', nil, true) || doc_option(parent.document, :to_dir))
end

should do the same thing I would think, so I don't really understand where the difference is coming from.

[robertpanzer]

I'll debug tomorrow, maybe I can find a difference.
(And cross-check with 2.0.2)

[robertpanzer]

Debugging through output_base_dir (with puts super-powers) I see that doc_option(parent.document, :to_dir) already returns the absolute path that I passed to the option :to_dir, i.e. /Users/robertpanzer/dev/asciidoctorj-diagram/asciidoctorj-diagram/build.

Now parent.normalize_system_path(doc_option(parent.document, :to_dir)) seems to use the original path where the source document is located in SAFE mode and appends this to the source directory of the document /Users/robertpanzer/dev/asciidoctorj-diagram/asciidoctorj-diagram/build/resources/test/ eventually creating /Users/robertpanzer/dev/asciidoctorj-diagram/asciidoctorj-diagram/build/resources/test/Users/robertpanzer/dev/asciidoctorj-diagram/asciidoctorj-diagram/build.
This is pretty unexpected at least to me.
Note that this is in SAFE mode and I get a corresponding warning:

Sep. 27, 2020 2:04:12 NACHM. <script> convertFile
WARNUNG: path is outside of jail; recovering automatically

In Unsafe mode it works as expected and I see that my test is plain wrong, instead of assuming the resulting image in the documents source directory it should of course be in the :to_dir.
It also seems to work identically in 2.0.2 when using unsafe mode.

However in safe mode, 2.0.2 produces the file in the documents source directory /Users/robertpanzer/dev/asciidoctorj-diagram/asciidoctorj-diagram/build/resources/test, which I expect in the test (,but which will also produce an html with a wrong href).

Am I missing sth about safe mode?
Intuitively I would expect that the :to_dir option would be expected as "safe", as it can only be defined externally, and not from within the document.

That said, I'll happily update the test to use unsafe mode, fix the test accordingly and release asciidoctorj-diagram 2.0.4.
I just wonder asciidoctor-diagram is to be used in safe mode without using data-uris.

[pepijnve]

Oh wow, I didn't expect that at all. Why is normalize_system_path butchering absolute paths like that? Let me check with @mojavelinux if that's intentional behaviour or not.

[pepijnve]

@robertpanzer I'm not seeing the same behaviour locally. Entering output_base_dir (modified a bit for debugability here), I see to_dir as an absolute path.

That gets passed on to normalize_system_path. Safe mode is enabled in this example so jail gets set to a non-nil value

Finally the path resolution logic resolves the path, everything checks out and I get the correct absolute path back.

No double concatenated absolute paths in sight.

[mojavelinux]

As you have observed, normalize_system_path knows how to deal with an absolute path. However, if it determines that the absolute path is outside of the jail, then it will modify it because it is trying to prevent access a forbid location. So the real question is, why does it think the path is outside the jail? Keep in mind that safe mode will enforce the jail whereas unsafe mode will not.

[mojavelinux]

See this test: https://github.com/asciidoctor/asciidoctor/blob/master/test/paths_test.rb#L173

[pepijnve]

@mojavelinux thanks for the pointer. So this is expected behaviour. Looks like a test setup issue then where safe mode is being used with an incorrect jail.

[robertpanzer]

All I am setting is the :out_dir option.
I am unsure how I could break the jail like this.
Or do I have to set :imagesoutdir too if I set :outdir?

[pepijnve]

@robertpanzer in safe mode the jail directory is doc.base_dir. In your test case you're working with

File inputFile = new File("build/resources/test/sample.adoc");

and

.toDir(new File("build"))

The jail will be build/resources/test and the to_dir build which is outside the jail. But this isn't your 'fault'. I think my mistake is in calling normalize_system_path too early. I should probably only call that on the final output path and not on something intermediate. That is indeed a behaviour change between 2.0.2 and 2.0.4.

Even if I fix this, with the test setup above diagram is still going to want to try to write to build/<imagefile>.png which still escapes the fail, so I think you're still going to hit the 'doubled absolute path' thing.

[pepijnve]

@robertpanzer asciidoctor/asciidoctor-diagram@669fa3c is an attempt to make things more predictable. I've split the path handling logic into two parts: resolving relative paths to absolute ones and normalising paths which enforces the safe mode jail. This should avoid the premature jailing of paths which can lead to unexpected results.

Do you have a way of testing a git version of the gem or should I make a prerelease build for testing?

[robertpanzer]

@pepijnve Awesome, thank you!
If it's just that change I can test it locally without a prerelease.
I'll come back with my results

[robertpanzer]

The image is generated now again at the position where the test previously expected it in safe mode.
(I am still unsure if this is ideal, considering a use case where I'd like to generate a document into a temporary directory on a server and zip the whole thing together and serve it)

However, the cache file still seems to be created in the "butchered" :) path

[pepijnve]

Ok, well that's progress already. I must have missed something with the cache then. Let me double check.

[pepijnve]

It was another double normalisation that I missed. @robertpanzer could you give asciidoctor/asciidoctor-diagram@ae697ff a try?

[pepijnve]

@robertpanzer I've been able to reproduce the issue in a test case myself. It boils down to the way safe mode works when the output directory is not equal to or a child of the parent directory of the source file. The diagram extension is deriving the paths you expect internally, but safe mode normalisation kicks in and changes them.

This is the path getting normalised. start is the directory you expect. target is the image file name. Because safe mode is enabled, jail is set to the document base directory.

In the end because start lies outside jail you get /Users/pepijn/Projects/asciidoctor-diagram/testing/Asciidoctor_Diagram_PlantUmlBlockProcessor/should_write_respect_to_dir_in_safe_mode/build/resources/test/diag-419e2cf517604aa6ae1317f07e79444d.png as a result.

There's a simple fix for this problem: don't test this kind of setup 😄 What's a bit surprising though is that the backend does manage to write outside the jail (at least that's what it looks like since sample.html ended up in the build directory. That shouldn't be allowed either per the safe mode rules if you ask me.

@mojavelinux afaict from the source code this is by design. Could you confirm that this is the case?

[robertpanzer]

It's closer now :)
In safe mode the cache file is now generated directly next to the image, and no longer in .asciidoctor/diagram.
When using the unsafe mode however, the cache file is still at the well known location .asciidoctor/diagram.

[pepijnve]

@robertpanzer I wasn't sure which variant we want. If I pass an absolute path to normalize_system_path with no start value you get the double absolute path. If I pass a file name and the absolute path of the desired directory you get the behaviour your currently seeing. Neither is really what you would want or expect.

If you want to specify a cache directory explicitly, you can do that since 2.0.3 using the diagram-cachedir attribute.

[pepijnve]

@robertpanzer I was going through the diagram issue list and bumped into asciidoctor/asciidoctor-diagram#262 Exact same problem as what you're describing.

As @mojavelinux stated there, this is kind of a core issue related to how safe mode works. My intuitive expectation would be that the safe mode jail consists of two distinct roots: one for input and one for output. You can only read from the source documents parent directory and only write to the destination directory. Changing that is out of scope for this issue of course.

I think with the last set of fixes I did we're in a good enough state. I'm going to close this issue for the time being.

[robertpanzer]

Sorry for responding late, I am drowning a bit in work and other personal issues right now.
I would have expected that in safe mode the cache file would land at the same relative location in the jail as it does in unsafe mode relative to the to_dir.

But thank you for digging out this issue. Under that condition I guess it is fine as it is, and the personal recommendation probably has to be to never use it :D

[pepijnve]

No worries Robert, I know the feeling. My available time to work on open source stuff ebs and flows as my day job and personal life get busier and calmer as well.