register asciidoctor.js extensions

[YoshihideShirai]

Thank you for your comment in #567.

I have suggestion regarding asciidoc.js expansions.
Instructions for use are provided in the readme.

What do you think?

[ggrossetie]

That's great, thanks for this contribution!

The Intellij IDEA plugin is using the .asciidoctor folder to put Asciidoctor related files (styles, extensions...) that are tied to a workspace. We should probably use this "convention". Since Intellij IDEA is using JRuby to load extension, they are declared in .asciidoctor/lib. @ahus1 do you think it would make sense to use a generic name? lib is somehow related to the Ruby world, maybe .asciidoctor/extensions? .asciidoctor/ext? we could use the file extension to load or not extensions depending on the runtime environment?

Not sure if we should allow user to configure an alternative directory. I like the .asciidoctor convention.

[YoshihideShirai]

@Mogztter
Thank you for your review!

The Intellij IDEA plugin is using the .asciidoctor folder to put Asciidoctor related files (styles, extensions...) that are tied to a workspace. We should probably use this "convention".

I agree your comment. I changed extensions path under like Intellij IDEA plugin. (.asciidoctor/lib)

lib is somehow related to the Ruby world

According to the Intellij IDEA plugin manual,
https://intellij-asciidoc-plugin.ahus1.de/docs/users-guide/features/advanced/asciidoctor-extensions.html
lib is not only Ruby world but also Java world.
So, I think we can use js extensions in .asciidoctor/lib.

[ahus1]

Happy to see an approach to support extensions in VS Code. While I added this functionality in IntelliJ, I always struggled with the security aspect of this feature: This could trigger executing code of a project by just opening the project in the IDE.

To prevent this, there are some safeguards in IntelliJ: In recent versions, a user would need to confirm that they trust the project. In addition to that for any Asciidoctor extension to load, the user will also need to confirm to execute the code in the .asciidoctor/lib folder every time they open the project.

BTW: styles can be located anywhere in the project including this folder. In order for them to be picked up, there needs to be an .asciidoctorconfig file as described here or a reference in the AsciiDoc file itself.

[ggrossetie]

Since we can configure the stylesdir and docinfodir from .asciidoctorconfig, it might make sense to configure the extensionsdir as well?

What do you think about using default values?

• .asciidoctor/styles
• .asciidoctor/extensions
• .asciidoctor/docinfo-files

Regarding the security aspect, I believe that VS code is also asking to trust the project.

I think that's a good idea to ask the user to confirm to execute code every time they open the project. But we should ask once for all extensions. If the user declines then we render the document but without extensions.

[ahus1]

I wouldn't object to configuring stylesdir and docinfodir to automatically configure those when the folders exist. The IntelliJ plugin wouldn't lead the way here, but if the VS Code plugin would implement it, the IntelliJ plugin would be a fast follower.

When I first added those features, this was about Asciidoctor extensions, and how to make this work with Asciidoctor. Those extensions are definitely Asciidoctor only, while parts of it might also be AsciiDoc. Eventually it might change to ".asciidocconfig" and ".asciidoc" - not sure when, and if it really should. I'm happy to keep them as ".asciidoctor" and ".asciidoctorconfig" for now.

[ggrossetie]

Eventually it might change to ".asciidocconfig" and ".asciidoc" - not sure when, and if it really should. I'm happy to keep them as ".asciidoctor" and ".asciidoctorconfig" for now.

That's true, extensions are specific to Asciidoctor. Even if the extension API is part of the AsciiDoc specification, how to load extensions will probably always be implementation specific.

Eventually, I would like to support .asciidoctorconfig in VS code. It's a great idea but I think it would be more flexible to use a configuration file format such as TOML or YAML to be able to configure more compatible features across IDE.

Anyway, we should take it one step at a time. Before merging, I think we should implement some safeguards. @YoshihideShirai would you be able to ask the user to confirm to execute code before the first preview (when they open the project)?

[YoshihideShirai]

would you be able to ask the user to confirm to execute code before the first preview (when they open the project)?

I tried to add warning dialog.
The dialog is shown before the first preview.

[ggrossetie]

Thanks!
I will give it a try as soon as possible 🤞🏻

[danyill]

Thank you for this very excellent contribution (and thanks for the nudge @Mogztter)

I have read through the code and done some initial usability testing.

I may be missing something obvious but I can't see how this capability is provided as per the README:

• Install npm package in the workspace directory. For example, the >installation command is the following:

npm install asciidoctor-emoji

Or if it is, I can't get it to work and neither can I see in the code that say, node_modules are scanned or require-ed. If it is, and they are it seems a little surprising to me that we don't have a separate security message for this.

For extensions within the .asciidoctor/lib folder this seems to work really well in several tests I did. This contributes something which is frequently requested so 👍 👍 👍 . I appreciate that any load errors are displayed to the user.

What do you think about the following questions?

• Would it be useful to recursively search folders for ".js" files in the .asciidoctor/lib folder or at perhaps one folder deep as well (e.g. .asciidoctor/lib/my-extension/*.js). Some users will store each extension within a folder.
• I'm not sure how well this will work on vscode-web and I haven't tested this (yet). The web extension guide suggests replacing fs with workspace.fs. I think we should either migrate to workspace.fs or put this feature behind a check against being in a web environment. WDYT? Guillaume may be able to provide further advice because he went through this not very long ago.
• I feel there should be a setting to disable this capability and perhaps it should be disabled by default - some administrators may wish to control this for users. It provides some surface area for attack (a malicious repo cloned and one-click). It seems like there is work happening in this area upstream, see Implement a policy-settings mechanism for approving/blocking extensions microsoft/vscode#84756 and Investigate global policy support microsoft/vscode#147756
• I think it seems reasonable that a failed extension should prevent all extensions from loading but perhaps we should indicate failure and run as many extensions as possible?

IMHO once we merge this, it would be well worth a release, I'm keen to use the capability if provides 😉

[danyill]

I'm not sure how well this will work on vscode-web and I haven't tested this (yet). The web extension guide suggests replacing fs with workspace.fs. I think we should either migrate to workspace.fs or put this feature behind a check against being in a web environment. WDYT? Guillaume may be able to provide further advice because he went through this not very long ago.

I note in #507 Guillaume said:

Please note that we cannot support the vscode.workspace.fs API because Asciidoctor.js is synchronous (and does not support async code): https://code.visualstudio.com/api/references/vscode-api#FileSystem

[danyill]

In the web environment the PR code generates an error associated with the use of fs. so we do need to do something about that:

[YoshihideShirai]

Hi, danyill.

Thank you for your comments.

Would it be useful to recursively search folders for ".js" files in the .asciidoctor/lib folder or at perhaps one folder deep as well (e.g. .asciidoctor/lib/my-extension/*.js). Some users will store each extension within a folder.

It is done.

I'm not sure how well this will work on vscode-web and I haven't tested this (yet). The web extension guide suggests replacing fs with workspace.fs.

It is done.

I feel there should be a setting to disable this capability and perhaps it should be disabled by default

WIP.

I think it seems reasonable that a failed extension should prevent all extensions from loading but perhaps we should indicate failure and run as many extensions as possible?

I have improved it to "run all extensions even if there is an error".

[YoshihideShirai]

I feel there should be a setting to disable this capability and perhaps it should be disabled by default

Done. So, I fed back everything.

would you be able to ask the user to confirm to execute code before the first preview (when they open the project)?

This feature is disabled by default. So, I deleted the warning dialog before the first preview.

[ggrossetie]

This is really good! I finally had a chance to give it a try and it's working as expected:

I left a comment regarding how this feature is enabled. I think it would be better to use an extension setting rather than a command.

I believe we can use the WorkspaceConfiguration to update/set the trust value: https://code.visualstudio.com/api/references/vscode-api#WorkspaceConfiguration

So users will have to do the following:

• Go to the extension settings and enable "Allow Asciidoctor.js extensions to be registered"
• Give their consent to activate extensions (if they have not already done it for this workspace) when previewing an AsciiDoc file

When users attempt to preview an AsciiDoc file if:

• the setting "Allow Asciidoctor.js extensions to be register" is enabled,
• and, there's at least one JavaScript file in .asciidoctor/lib,
• and, the workspace configuration do not already trust extensions to be activated on this workspace,
• then, we should show the warning message.

If users have decided to not trust extensions on this workspace then we should not ask them again. However, we should document how to manually update this configuration. In case, users previously didn't want to activate/trust extensions but they changed their minds.

[YoshihideShirai]

@Mogztter
Thank you for your explanation.
I changed this feature procedure.

• Enable the feature by Enable Asciidoctor.js extensions to be register. setting.
• Allow the security setting by warning diaglog when first AsciiDoc preview.
  You can change it by Change AsciiDoc extension scripts Security Settings command, too.

[ggrossetie]

I gave it another try and it's looking great!
I left a suggestion but otherwise 👍🏻