ggrossetie / asciidoctor-emoji

Asciidoctor.js extension to add emoji in your document!
MIT License
7 stars 5 forks source link

Update @asciidoctor/core dep version #89

Closed xavier-calland closed 1 year ago

xavier-calland commented 1 year ago

@asciidoctor/core is now version 3.0.2

It would be great to have a version of this plugin compatible with the new version on @asciidoctor/core

ggrossetie commented 1 year ago

It would be great to have a version of this plugin compatible with the new version on @asciidoctor/core

I believe this extension is already compatible with Asciidoctor.js 3.x. The peerDepencies could include 3.x: >=2.2 < 4

xavier-calland commented 1 year ago

Yes and no. The only change that need to be made (besides peerDependency) is the CDN because @asciidoctor won't follow redirects, so safe mode won't work properly (this seems more like an @asciidoctor issue than an asciidoctor-emoji one).

ggrossetie commented 1 year ago

The only change that need to be made (besides peerDependency) is the CDN because https://github.com/asciidoctor won't follow redirects, so safe mode won't work properly (this seems more like an https://github.com/asciidoctor issue than an asciidoctor-emoji one).

We should probably add a test case. Are you using data-uri and/or allow-uri-read attributes? Asciidoctor.js is using https://github.com/ggrossetie/unxhr but as far as I remember it supports redirect.

@mojavelinux does Asciidoctor core (Ruby) disallow redirects in SAFE mode?

xavier-calland commented 1 year ago

A test exists and it fails when using @asciidoctor/core 3.0.2 and CDN twemoji.maxcdn.com

xavier-calland commented 1 year ago

When running tests with : @asciidoctor/core version 3.0.2 and CDN https://twemoji.maxcdn.com/ I have this result :

  Registration
    ✔ should register the extension

  Conversion
    ✔ should convert an existing emoji into an image with the size 2x (34px)
    ✔ should convert an existing emoji into an image with the size 4x (68px)
    ✔ should convert an existing emoji into an image with the size in pixel (42px)
    1) should convert an existing emoji into an inline image
    When extension is not registered
      ✔ should not convert an existing emoji
      ✔ should not convert an non existing emoji
    When extension is registered
      ✔ should convert an existing emoji into an image
Skipping emoji inline macro. ooops not found
      ✔ should return an error message if the emoji does not exist

  8 passing (399ms)
  1 failing

  1) Conversion
       should convert an existing emoji into an inline image:
     AssertionError: expected '<div class="paragraph">\n<p><span cla…' to include '<span class="emoji"><img src="data:im…'
      at Context.<anonymous> (test/test.js:76:21)
      at process.processImmediate (node:internal/timers:471:21)

It seems the problem is in generate_data_uri_from_uri function.

The debug show that

mojavelinux commented 1 year ago

@mojavelinux does Asciidoctor core (Ruby) disallow redirects in SAFE mode?

If allow-uri-read is enabled, it will also follow redirects. The only restriction is whether it can use network access at all, not how many redirects there are.

ggrossetie commented 1 year ago

If allow-uri-read is enabled, it will also follow redirects. The only restriction is whether it can use network access at all, not how many redirects there are.

Yes, that's what I thought.

The unxhr settings are with async: false, so it uses request.js (and http, https) which doesn't seems to handle redirects.

It should, see: https://github.com/ggrossetie/unxhr/blob/4fbc39978b99180c24cd31ac802c2b4ae5dca3c6/lib/XMLHttpRequest.js#L420C79-L441

I will try to reproduce this issue in unxhr.