On sites that use a content security policy header to restrict what assets are loaded from external sources, this module is problematic because it loads the ACE editor from a CDN. I guess there are two fixes for this:

1) Add the cdnjs.cloudflare.com domain to whitelist of allowed script srcs. This is a bad idea. Whitelisting an entire CDN makes a content security policy pointless.
2) Developer must override library definition to provide a local path to the library.

I wonder if there is a better way to support using a local copy of the library?

CommentFileSizeAuthor
#31 asset_injector-3082195.8.x-2.21.patch3.02 KBtobiasb
#30 asset_injector-3082195-30.patch3.93 KBtobiasb
#29 ace_console_screenshot.png111.12 KBdanielehrenhofer
#24 asset_injector-3082195-24.patch3.07 KBtobiasb
#19 3082195-19.patch6.58 KBtobiasb
#17 3082195-12.patch3.07 KBrandy tang
#11 interdiff-7_11.txt634 bytesJonny Gong
#11 3082195-11.patch2.99 KBJonny Gong
#7 3082195-7.patch2.38 KBJonny Gong

Issue fork asset_injector-3082195

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bkosborne created an issue. See original summary.

pookmish’s picture

pookmish commented

I understand the issue, but since the request is a bit of a rare scenario, I'm leaning towards relying on modules/themes to modify the library by either inserting the css & JavaScript to disable Ace editor. Or override it to provide a local copy of Ace editor with the respective module/theme.

pookmish’s picture

pookmish commented
Status: Active » Closed (won't fix)
bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented
Status: Closed (won't fix) » Active

I wonder if you'd re-consider this. Any site that uses a content security policy to only allow specific JS sources will run into problems with this. A simple UI config setting to specify a path override for the ACE JS source would suffice. If you leave this open, I will eventually get time to contribute a patch for it.

geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented
Status: Active » Postponed (maintainer needs more info)

> I'm leaning towards relying on modules/themes to modify the library ... to provide a local copy.

FTR: I rolled Asset Fetcher for this purpose, but it's not battle tested. please test and review.

pookmish’s picture

pookmish commented
Status: Postponed (maintainer needs more info) » Closed (won't fix)

@bkosborne feel free to reopen this if you produce a patch.

As a thought, a workaround similar to the patch would be to just use asset injector to load the ace library for you. Copy and paste the ace library into an injector and add it to the UI page. It's a hacky way to produce the same results.

Jonny Gong’s picture

Jonny Gong commented
StatusFileSize
new 3082195-7.patch2.38 KB

Hi, I made a patch for this.
Introduced the ace editor in the module and modified the reference path to ace.js.
Tested on version 8.x-2.7, it works for me.

Jonny Gong’s picture

Jonny Gong commented
Version: 8.x-2.x-dev » 8.x-2.7
Status: Closed (won't fix) » Needs review
pookmish’s picture

pookmish commented

Thanks Jonny, I like the direction you are thinking. Since `respositories` only work at the root level of composer.json, I think we could go the direction of a suggested package.

Then using the `hook_library_info_alter()` detect if the ace library exists locally and adjust the existing library definition. That approach would then not affect any existing site that works fine with CDN use but allow for this scenario to be satisfied.

pookmish’s picture

pookmish commented
Status: Needs review » Needs work
Jonny Gong’s picture

Jonny Gong commented
StatusFileSize
new 3082195-11.patch2.99 KB
new interdiff-7_11.txt634 bytes

@pookmish
Sorry, i have no time to do this for now. If i have time or any guys can help, please.

This patch fixed issue that Drupal asset aggregation will broken ace mode asset loading.
related issue: https://github.com/ajaxorg/ace/issues/655#issuecomment-7596749

Ruslan Piskarov made their first commit to this issue’s fork.

Ruslan Piskarov opened merge request !1

ruslan piskarov’s picture

ruslan piskarov
he/him
Primary language Ukrainian
Location Kyiv, Ukraine
commented
Status: Needs work » Needs review

In #13 I have improved the patch provided by @Jonny Gong in #11. Removed "docroot" path, etc. Could you review it?

pookmish’s picture

pookmish commented

The approach in #13 is very complicated to implement. What I would prefer to see is a
1. composer suggested package of "npm-asset/ace-builds" (the same package that ace editor module uses. That way it doesn't duplicate packages in the codebase.
2. since the ace editor package would then be suggested and optional, produce a hook_libraries_alter that changes the path of the ace editor to use the local path if available.

Using the approach I outlined above will prevent any breaking changes for existing users and provide the ability to simply require a new package and then ace editor would be hosted locally.

ruslan piskarov’s picture

ruslan piskarov
he/him
Primary language Ukrainian
Location Kyiv, Ukraine
commented
Status: Needs review » Needs work
randy tang’s picture

randy tang commented
StatusFileSize
new 3082195-12.patch3.07 KB
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Version: 8.x-2.7 » 8.x-2.x-dev

I agree this needs a more simple and general rewrite. Please have a look at this issue and MR for reference:
#3214406: Add option to host dragula library locally
https://git.drupalcode.org/project/layout_paragraphs/-/merge_requests/70

If the library can't be loaded locally (from /libraries folder), it falls back to the CDN version. The MR also contains tests, etc. and the approach is well tested in Webform, so I'd rate that as best practice.

Would someone like to implement the same here as new MR?

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
StatusFileSize
new 3082195-19.patch6.58 KB

The patch is just a updated version.

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Related issues: +#3319074: Update ace-editor library declaration
geek-merlin’s picture

geek-merlin
Primary language German
Location Freiburg, Germany
commented

#18: Yes!

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

Hmm I think it goes beyond just replacing the library path to load from a local version instead of the CDN, because the this module's asset_injector.ace_editor.js currently includes the following hardcoded:

          ace.config.set('basePath', '//cdnjs.cloudflare.com/ajax/libs/ace/1.8.1');
          ace.config.set('modePath', '//cdnjs.cloudflare.com/ajax/libs/ace/1.8.1');
          ace.config.set('themePath', '//cdnjs.cloudflare.com/ajax/libs/ace/1.8.1');

I think these three lines are only necessary if you're loading it from the CDN. This can probably be solved if we pass down via drupalSettings if the module used the CDN or not to load the main library, and if not, don't run these lines.

bkosborne’s picture

bkosborne
Primary language English
Location New Jersey, USA
commented

Okay, I was wrong, we need those lines regardless, because without them the library tries to auto-detect the path to those directories and it doesn't work if JS aggregation is turned on.

So, if the ace-builds version of the library is used (and stored locally), we need to provide its path via drupalSettings

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new asset_injector-3082195-24.patch3.07 KB

I updated the MR for the latest changes.

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
Status: Reviewed & tested by the community » Needs review
anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented
Status: Needs review » Needs work

Thanks @tobiasb - the README says

is taken from CDN as long as no min.js file exists

but I can't see related code, what am I missing?

Also see #18 - looks like hook_library_info_alter() and other parts like the tests are still missing?
The referenced issue is a nice template.

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented

Therefore it was "needs work". :d

anybody’s picture

anybody
Primary language German
Location Porta Westfalica
commented

Wasn't :P

See #25 + #26

Status: Needs review » Needs work

Eventually by accident ;) All good

danielehrenhofer’s picture

danielehrenhofer
Location Frankfurt
commented
StatusFileSize
new ace_console_screenshot.png111.12 KB

Due to our content security policy, loading external resources, e.g. via a CDN, is prohibited. As a result, I have now noticed that ace.js appears to be loading the resource mode-css.js via a CDN: https://cdnjs.cloudflare.com/ajax/libs/ace/1.8.1/mode-css.js

Is this known and can it be prevented somehow?

For better illustration I have added a screenshot of my developer console.

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
StatusFileSize
new asset_injector-3082195-30.patch3.93 KB

Saw the same.

ace.config.set was overridden by the old way, only workerPath worked

I replaced it and updated to latest version.

tobiasb’s picture

tobiasb
Primary language German
Location Berlin
commented
StatusFileSize
new asset_injector-3082195.8.x-2.21.patch3.02 KB
konfuzed’s picture

konfuzed commented

Minor update to https://github.com/ajaxorg/ace-builds/archive/refs/tags/v1.43.4.zip locally worked on the patch in #31