Allow to pass Asciidoctor converter templates

yann-soubeyrand commented 5 months ago

Asciidoctor allows to customize its output by passing it so called converter templates. It is done by passing it as argument the path to a folder containing these templates.

I’d need this feature to customize the rendering of AsciiDoc admonitions so that they better integrate in the theme I’m using.

I’m wondering if this is something you would be accepting a PR for? If so, how do you see this implemented:

I can add a new config option for the template folders to pass to Asciidoctor during rendering,
or we can agree on a folder convention where the users would put their templates (like for the shortcodes folder for example) and always pass this folder to Asciidoctor.

What do you think?

jmooring commented 5 months ago

AsciiDoc's converter templates are the equivalent of Hugo's Markdown render hooks. Maybe a fixed location such as:

layouts/
└── _default/
    ├── _asciidoc/
    │   ├── inline_anchor.html.slim
    │   └── inline_image.html.erb
    └── _markup/
        ├── render-image.html
        └── render-link.html

Or maybe all in one directory:

layouts/
└── _default/
    └── _markup/
        ├── inline_anchor.html.slim
        ├── inline_image.html.erb
        ├── render-image.html
        └── render-link.html

I'm not sure if the _markup directory is for Markdown only, or if it is for all forms of markup, including AsciiDoc.

AsciiDoc has a large number of convertible contexts, so this seems like it would be a very powerful feature for AsciiDoc users.

However, those who have contributed things like this in the past have not stuck around to maintain them. Which means existing maintainers (essentially one person) have a steep learning curve every time they have to (or need to) touch this stuff. And for a content format that's used by << 1% of sites in the wild, that's not a great investment.

yann-soubeyrand commented 5 months ago

Thanks for your answer @jmooring!

I really like your idea of treating converter templates as render hooks and I don’t have a strong opinion on whether they should be put together in the same folder or not. Maybe there’s a risk of conflict and the best would be to have something like this?

layouts/
└── _default/
    └── _markup/
        ├── asciidoc
        |   ├── inline_anchor.html.slim
        |   └── inline_image.html.erb
        └── markdown
            ├── render-image.html
            └── render-link.html

However this is a breaking change.

Regarding your objection on AsciiDoc oriented features, I mostly agree with you. On the other hand, this feature would “just” be a matter of passing an extra argument (--template-dir) when invoking Asciidoctor. I don’t think there will be a lot of maintenance to do on it, since the rest of the mechanism, the virtual filesystem (I don’t know if this is the right name for the feature which allows different levels of overriding for files in the layouts folder), is common to every supported markup language.

jmooring commented 5 months ago

https://docs.asciidoctor.org/asciidoctor/latest/convert/templates/#apply-your-templates

Instructing Asciidoctor to apply your templates is the easiest part. You only need to tell Asciidoctor where the templates are located and which template engine you’re using. (Technically, you don’t need to specify the template engine. But, by doing so, it makes the scan more efficient and deterministic.)

Configuration

We would need a way to set:

--template-dir
--template-engine

Although I would like to limit template-dir to layouts/_default/_markup, that isn't practical. Asciidoctor has no knowledge of Hugo's union file system, which means you can't share the converter templates via a theme or module. Provide a templateDirectory configuration option, with a default value of "". The value must be relative to the project directory.

Limit the template-engine to erb, haml, and slim. Provide a templateEngine configuration option, and fall back to erb.

Gems

To support the template engines above you must install these gems: concurrent-ruby[^1], haml, slim and tilt. If you're not using slim and haml, you still have to install concurrent-ruby and tilt. Asciidoctor throws an error if you specify a template-dir when tilt is not installed, so we should only set template-dir if the configured directory exists and contains one or more entries.

Miscellaneous

[x] What happens if the gems required for the configured template engine aren't installed? Asciidoctor throws a descriptive error and Hugo fails the build (this is a good thing).
[x] What happens if the template directory (layouts/_default/_markup) does not exist? No errors (this is a good thing).
[x] What happens if you override the Asciidoctor table of contents with outline.html.erb and set preserveToc to false? The HTML returned by Hugo's .TableOfContents method will be affected by the presence outline.html.erb; we parse Asciidoctor's output to create the internal toc data structure. Don't do both of these things at the same time.
[ ] Determine if arbitrary code execution is possible with any/all of the template engines. This could be a show stopper.
[ ] Skip the integration tests unless these gems are installed: haml, slim and tilt. Installing the Gems in the CI testing matrix would be a headache... not worth it.
[ ] Need to update documentation. Note (a) the available template engines, (b) that the template directory is relative to the project directory, (c) the required gems, and (d) that converter templates are not available with Hugo's snap package[^2].

[^1]: concurrent-ruby is recommended but not required. Asciidoctor throws a warning if it's not installed. [^2]: This is difficult, if not impossible, with strictly confined snaps. The snapcraft team effectively deprecated the ruby plugin several years ago.

Example configuration

[markup.asciidocExt]
templateDirectory = '_asciidoc-converter-templates'  # default is ""
templateEngine = 'handlebars'                        # default is handlebars (no others allowed)

Test site

git clone --single-branch -b hugo-github-issue-12314 https://github.com/jmooring/hugo-testing hugo-github-issue-12314
cd hugo-github-issue-12314
hugo server

Then visit http://localhost:1313/posts/post-1/.

jmooring commented 5 months ago

@yann-soubeyrand Comments regarding the above?

yann-soubeyrand commented 5 months ago

Wow, thanks a lot for all the work done so far! I planned to have a look this weekend at the questions you had, I haven’t seen that you already answered most of them!

The first remark that comes to my mind is that there are several implementations of Asciidoctor other than the Ruby one, for example Asciidoctor.js, which a lot easier to work with in my opinion (just npm init -y && npm install asciidoctor --save-dev && cat package.json | jq '.scripts |= { "hugo": "hugo" }' | tee package.json and then you can exec npm run hugo -- serve), but comes with different template engines: https://docs.asciidoctor.org/asciidoctor.js/latest/extend/converter/template-converter/. May I ask you why you prefer to limit the possible values for the templateEngine config?

Second, it’s more a question on the feasibility than a remark, because I’m not aware enough of the inner working of Hugo: could we imagine building a temporary union folder for the templates (in a temporary directory)? This could of course be done in a second time.

Other than that, it seems pretty appealing to me!

jmooring commented 5 months ago

several implementations of Asciidoctor

If we pursue this, we should focus on the most common implementation (gem) first. If it works with Node.js, great. If it doesn't, that's a separate proposal.

a temporary union folder for the templates

I don't see that happening. We would need to do this every time we build (and maybe rebuild) the site, probably stuffing them somewhere in the cache directory. Keep it simple.

limit the possible values for the templateEngine config

One of my concerns about this proposal is the potential for arbitrary code execution. I have no idea which, if any, of the available template engines provide this capability. For all I know, you might be able to do that with erb, haml, and/or slim. If we cannot prevent arbitrary code execution, this proposal is DOA.

jmooring commented 5 months ago

Regarding arbitrary code execution...

Testing an erb template:

<% File.delete("/home/jmooring/temp/test-a.txt") %>

Testing a slim template:

- File.delete("/home/jmooring/temp/test-b.txt")

Testing a haml template:

- File.delete("/home/jmooring/temp/test-c.txt")

All three deleted the file. This is scary, and makes this proposal DOA unless there's a template engine that prevents code execution.

See https://asciidoctor.zulipchat.com/#narrow/stream/279642-users/topic/Can.20I.20use.20the.20Liquid.20template.20engine.20with.20c/near/430334262

jmooring commented 5 months ago

Closing based on the thread above. This is insecure.

yann-soubeyrand commented 5 months ago

From the doc:

You can compose templates in any template language that’s supported by Tilt.

Maybe Handlebars could be a candidate, right?

jmooring commented 5 months ago

OK, I can make handlebars work, and it seems more secure than erb, haml, or slim. The required stack is:

sudo apt install ruby ruby-dev
gem install --user-install asciidoctor concurrent-ruby tilt tilt-handlebars

Unfortunately, tilt throws this warning for every page processed, despite having installed the tilt-handlebars gem:

WARN _index.adoc: /home/jmooring/.local/share/gem/ruby/3.0.0/gems/tilt-2.3.0/lib/tilt/mapping.rb:333: warning: Lazy loading of handlebars templates is deprecated and will be removed in Tilt 3. Require tilt/handlebars manually.

I don't know enough about handlebars to be entirely comfortable with it as a template engine.

yann-soubeyrand commented 5 months ago

Isn’t it that you need something like this in your Hugo config?

asciidocExt:
  extensions:
    - 'tilt-handlebars'

yann-soubeyrand commented 5 months ago

Regarding union filesystem, couldn’t we pass several template dirs, that is, the theme templates first, then the user supplied templates? From Asciidoctor documentation and a basic test I did, this should work as intended: https://docs.asciidoctor.org/asciidoctor/latest/convert/templates/#use-multiple-template-directories.

jmooring commented 5 months ago

Isn’t it that you need something like this in your Hugo config?

Yes, this stops tilt from trying to "lazy load" the template engine:

[markup.asciidocExt]
extensions = ['tilt-handlebars']
templateDirectory = '_asciidoc-converter-templates'
templateEngine = 'handlebars'  # can omit, this is the default and only option

couldn’t we pass several template dirs

Yes, we could change templateDirectory to templateDirectories in the above, looking for a string array instead of a string. The default would be an empty string array.

[markup.asciidocExt]
extensions = ['tilt-handlebars']
templateDirectories = ['foo','bar']
templateEngine = 'handlebars'

Asciidoctor uses a right-to-left order of precedence (last one wins) , which is opposite to other Hugo options (e.g., when using multiple themes). We will need to reverse the order when reading the array, so that the first one wins.

The above would allow you to use a directory in a theme, falling back to a directory in the root of your project, but both paths must be relative to the project root:

templateDirectories = ['themes/my-theme/adoc_templates','asciidoc-converter-templates']

Note that the directories above are outside of the layouts directory. You cannot place handlebars templates anywhere within the layouts directory. Handlebars templates use the same {{ }} notation as Go templates, causing Hugo to throw a parsing error when it loads all of the templates and encounters something like {{ thisFunctionDoesNotExist }}.

If the converter templates are provided by a module, you will need to vendor the the module (hugo mod vendor). However, Hugo does not vendor arbitrary directories so you would have to place the converter templates in the module's assets directory. This will work for both themes and modules and should be the recommended configuration:

[markup.asciidocExt]
extensions = ['tilt-handlebars']
templateDirectories = [
  'assets/foo/bar',
  'themes/my-theme/assets/adoc/',
  '_vendor/github.com/rsmith/my-module/assets/asciidoc-templates/',
]
templateEngine = 'handlebars'

The above will use the project root first, falling back to the theme, falling back to the module.

Note that hugo server will not detect changes to the converter templates. You will have to stop/start the server after editing a template.

The remaining question is: are we certain that the handlebars template engine doesn't allow arbitrary code execution? I found some past CVEs about this, but that's not unexpected.

yann-soubeyrand commented 5 months ago

The remaining question is: are we certain that the handlebars template engine doesn't allow arbitrary code execution?

Sadly, no… Asciidoctor supports a helper file (https://docs.asciidoctor.org/asciidoctor.js/latest/extend/converter/template-converter/#helpers-js-file) and I just tested that I can read a file anywhere in my home directory… We’d need to check that no template directory contains a helpers.js file, else arbitrary code execution is possible.

jmooring commented 5 months ago

We could (potentially) disable the converter templates feature when running Asciidoctor via Node. We know already it won't work with the snap package, so this is just another exception.

$ asciidoctor --version
Asciidoctor 2.0.22 [https://asciidoctor.org]
Runtime Environment (ruby 3.0.2p107 (2021-07-07 revision 0db68f0233) [x86_64-linux-gnu]) (lc:UTF-8 fs:UTF-8 in:UTF-8 ex:UTF-8)

$ npx asciidoctor --version
Asciidoctor.js 2.2.7 (Asciidoctor 2.0.22) [https://asciidoctor.org]
Runtime Environment (node v20.11.1 on linux)
CLI version 3.5.0

We could check for the existence of a helper.js file in each directory, and skip those that contain one, but this is pretty weak (e.g., the Asciidoctor team might decide to allow "other_helpers.js" too). As you pointed out here, I think we'd need an Asciidoctor CLI flag to disable this capability, and then we would have to do version checking to make sure the flag actually does something, etc.

EDIT: We already have to avoid adding empty template-dirs (meaning we have to read each directory anyway), so we can just reject any directory that contains files with extensions other than handlebars... and throw a warning.

jmooring commented 5 months ago

@yann-soubeyrand If you can build from source, please test https://github.com/gohugoio/hugo/pull/12318.

Example site:

git clone --single-branch -b hugo-github-issue-12314 https://github.com/jmooring/hugo-testing hugo-github-issue-12314
cd hugo-github-issue-12314
hugo server

yann-soubeyrand commented 5 months ago

@jmooring good job, it works as expected!

Regarding the two challenges to handle union file system:

Couldn’t we put the templates in a layout directory which we’d instruct Hugo not to parse?
To avoid vendoring, couldn’t we use the path where Hugo caches the module?

jmooring commented 5 months ago

Couldn’t we put the templates in a layout directory which we’d instruct Hugo not to parse?

If you want this proposal and draft PR to have a chance of being accepted by the project lead, the changes must be isolated to the markup/asciidocext package. Keep it simple and be happy if either or both are accepted.

To avoid vendoring, couldn’t we use the path where Hugo caches the module?

Sure, but the path will vary by OS, user, environment variables, etc., and the path will typically be external to the project directory (we don't want that). With a shared project, the only stable module path is _vendor/something.

it works as expected

I want you to break it, find holes, etc.

bep commented 5 months ago

Couldn’t we put the templates in a layout directory which we’d instruct Hugo not to parse?

You can add add ignore/include regexps to the layouts mount(s):

https://gohugo.io/hugo-modules/configuration/#module-configuration-mounts

jmooring commented 5 months ago

@yann-soubeyrand

Regarding https://github.com/asciidoctor/asciidoctor.js/issues/1727, an asciidoctor CLI flag would be useful only in conjunction with version checking (i.e., that flag will have no effect with current and previous versions).

With #12318 we reject a directory if it contains anything other than ".handlebars" files.

https://github.com/gohugoio/hugo/pull/12318/files#diff-bacb8b6486fb7c72cb6ba1b9d5dc583e1c2322733f5c61b389bbf84e19082703R97-R106

jmooring commented 5 months ago

@yann-soubeyrand What's your current opinion on this proposal? Do the restrictions (e.g., handlebars only, no access to virtual file system) limit its usefulness to the degree that we should not pursue this?

yann-soubeyrand commented 4 months ago

Hello @jmooring, sorry for the late reply… My opinion is that the restriction on handlebars only is not a problem (after all, better a single template engine than none), but the lack of access to the virtual filesystem limit the usefulness.

To elaborate, my initial use case was to enhance AsciiDoc support with Docsy theme: I wanted to adapt how AsciiDoc blocks (like code blocks, admonitions, etc) were rendered so that the theme CSS doesn’t have to be modified. However, my goal for these AsciiDoc converter templates was that they be shareable in a Docsy derived theme or even upstreamed. If it’s not possible, I’ll end up modifying the theme CSS (which can be distributed in a derived theme and/or upstreamed).

jmooring commented 4 months ago

If it’s not possible

It's not possible out-of-the-box as discussed in previous comments in this issue. Given that limitation, I'm inclined to close this proposal. I've pinged a few other AsciiDoctor site authors, and the response has been tepid.

Also, I'm not 100% convinced that we've eliminated the possibility of arbitrary code execution.

yann-soubeyrand commented 4 months ago

Yes, I think we can close this for the moment. Thanks a lot for the PoC!

jrauschenbusch commented 4 months ago

Hello @jmooring, sorry for the late reply… My opinion is that the restriction on handlebars only is not a problem (after all, better a single template engine than none)

I share the same opinion. Better one engine than none.

but the lack of access to the virtual filesystem limit the usefulness.

I'm also on the same page here. It's essentially the same issue is with respect to include:: and xref:: directives. With no real access to the union file system it always requires a mix of Hugo's Go template language (Shortcodes) and AsciiDoc. E.g. inside Hugo modules one can easily use relative paths when applying workingFolderCurrent: true, but across modules it's not possible. All by the fact that the file system is only accessible by Hugo itself.

To elaborate, my initial use case was to enhance AsciiDoc support with Docsy theme: I wanted to adapt how AsciiDoc blocks (like code blocks, admonitions, etc) were rendered so that the theme CSS doesn’t have to be modified. However, my goal for these AsciiDoc converter templates was that they be shareable in a Docsy derived theme or even upstreamed. If it’s not possible, I’ll end up modifying the theme CSS (which can be distributed in a derived theme and/or upstreamed).

The approach with converter templates was imho the right one. But as you mentioned Hugo's limitations are too restrictive for this. Providing additional CSS is ways simpler than vendoring a theme module and using the assets folder where it indeed is more a layout render hook. Furthermore these required arbitrary code execution prevention mechanisms (helper.js) seems just to be the tip of the iceberg.

github-actions[bot] commented 4 months ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

gohugoio / hugo