IMCE image caption support

The patch works well I reviewed it and didn't see any issues. Would be nice to see this implemented.

Mostly a re-roll although does include one change - instead of only checking if a UUID is available, creates a file and gets one if not.

Reviewed and tested.

Not an ideal solution for a directory containing hundreds of images.
I think it would be better to get the uuid by ajax on file selection.

Longer term testing, revealed that with many files, performance is less than ideal as ufku suggested might be the case.
An improved patch attached.

The patch from #10 worked for me. The JS portion of that patch did not apply, because it clashes with changes made to that file by patches from https://www.drupal.org/project/imce/issues/2762473 .. which grab the UUID using the File object in pure JS rather than creating a new function or endpoint using PHP. Might be worth a look and may also be more performant. I have not tested for performance specifically though.

Following up on #11, I re-rolled patch #10 so it doesn't clash with patch #30 from Should images uploaded with IMCE be managed?. I am using both this patch #12 and #30, and have captioning as well as file management working in IMCE/CKeditor.

The patch from #12 is working great, i can choose image by using icon provided by imce (icon provided by drupal core ckeditor removed) and double click on the image to insert caption and also choose alignment. But, when double click on the image, the popup should remove the "Open file browser" link from the image url form.

Another issue will be the figure class caption and caption-img not created.

@stevechai, the patch from #12 doesn't add "Open File Browser".. I am not sure what controls that, but I don't think this patch should attempt to adjust that.

Regarding the file browser window not closing, I cannot reproduce that on my end. When I use the IMCE image icon and select and insert an image, the file browser window closes.

Regarding figure/figcaption, those don't show in the source code of the editor, but they do show up in the rendered code of the page - in my tests anyway. The image element just gets the data-caption attribute and some other attributes, and is then rendered as a figure if it contains a caption. That's working in my tests.

@caspervoogt, thanks for the reply. The figure/figcaption are added correctly. The align class was added, only the caption and caption-img was not added to the rendered result.

Which should be <figure role="group" class="caption caption-img align-center">

Ah OK, I understand.

Are the caption and caption-image a standard Drupal thing that should be added to the figure if it contains a caption? I can see the usefulness of having that.

I may take another look at that but I don't know when .. I am going on holiday quite soon. Sounds like it should be a minor adjustment e.g. "if caption is active, add caption classes to class array for figure".

Yeap, it's standard Drupal 8 thing, if using default ckeditor image upload plugin and set caption, the classes will be added to figure.

Thanks

OK, then. We will want to do the same. I will try and find time for this but it may be a few weeks.

@caspervoogt, no problem, thanks for the help. Enjoy your holiday.

Hello,

I wanted to check in to see what the current status of this is. I am trying to use the benefit of IMCE for the image library and some of the formatting, but the loss of the ability to add a caption is the problem. I tried #12 but that hasn't helped.

I won't be able to look at this for the foreseeable future. Maybe someone else can jump in.

Works for me!

- /imce-get-uuid is not secure. It allows file creation by CSRF
- "create files" permission is irrelevant. it must be a part of imce config.
- the uri sent to /imce-get-uuid must be validated and accessible by the current user's imce config.
- uri doesn't need to be added to js data. it is already available on js side.

Sorry I had not seen your revert.

I will not find the time to work on the outstanding issues any time soon, FYI. Hoping others can jump in and fill in some of these gaps.

Hi @ufku, can you help us?
This patch is really important because it solves other problems, such as resizing the image inside CKEditor.

It is worth mentioning that I tried to access the url http://d8.lndo.site/imce-get-uuid without user login and I was denied access

This is only an applicable patch, not yet patched

@thalles does this patch fix the problem outlined here?
https://www.drupal.org/project/imce/issues/3024340

It is worth mentioning that I tried to access the url http://d8.lndo.site/imce-get-uuid without user login and I was denied access

It's to address something like this (same OR different site when logged in on http://d8.lndo.site/)

On http://d8.lndo.site/

<img src="/imce-get-uuid?uri=abc" />

On any other site

<img src="http://d8.lndo.site/imce-get-uuid?uri=abc" />

Hi @medinasod, if you look at the patches of both issues you will find that they are the same solution.

@Alan D, I just make a note once @ufku spoke of the validation of the logged in user.

I hope we can resolve this issue soon.

Thanks @all!

"It is worth mentioning that I tried to access the url http://d8.lndo.site/imce-get-uuid without user login and I was denied access"

OK, what does that mean in terms of the remaining issues?

Also, I tried patch #32 (it applied cleanly) but it didn't seem to do anything; when I upload a new image and insert it, it has no UUID and I can't edit the image. What does "This is only an applicable patch, not yet patched" mean?

After apply the pacth you need clear the cache.

I did not make changes to the patch, I just left the patch applicable again, to make it easier for anyone who wanted to work on the issue.

I just wanted to say that you need to be logged in to have access to /imce-get-uuid/?uri=public%3A%2F%2Fimages%2F5-unique-animals.png

I don't know why I didn't think of clearing Drupal's cache, but I did and now patch #32 works just as expected. Thanks also for explaining about /imce-get-uuid. That is good.

CSRF issue

Say you have the link, /delete-entire-site, which is only accessible to super administrators that nukes the site (worst case example I could think of).

1. Bored hacker creates this post: <img src="/delete-entire-site" >.
2. Bored hacker visits the page.
3. The browser of the bored hacker will try to load that URL thinking it's an image, triggering the page callback
4. Bored hacker sees a broken image as the 403 on the page callback is triggered.
5. The super admin visits that page. Could be a comment for moderation or whatever.
6. The browser of the super admin will try to load that URL thinking it's an image, triggering the page callback
7. /delete-entire-site is run as the super admin is allowed to do this
8. Super admin see's the broken image and be none the wiser (until they navigate away to another page in this case).

The hacker could also post to another domain and get the super admin to visit. If the super admin was logged in, the exact same thing would happen.

Clearly this is not critical in this case, but it still messes with the core config that can be used to cause a nuisance issue for administrators of the site.

D7 used special links for these actions, non-caching tokens based on the admin session key & URL, but I am not up with what D8 does to protect against this kind of request.

Follow a patch!
Added a xss filter, about the uri, the File.getUrl() cause the issue when reopen properties of image.

Please guys, check out the new patch.

Your code need has baseURL in $.getJSON() , because which error when project web is sub folder.

$.getJSON(drupalSettings.path.baseUrl + 'imce-get-uuid/', { uri: File.uri})
              .always( function (json) {
                lines.push(' <img src="' + File.getUrl() + '" data-entity-uuid="' + json.uuid + '" data-entity-type="file"' + (File.width ? ' width="' + File.width + '"' : '') + (File.height ? ' height="' + File.height + '"' : '') + ' alt="' + File.formatName() + '" /> ');
                editor.insertHtml(lines.join('<br />'));
                win.close();
              });

Also there is wrong position of brackets

+class ImceController extends ControllerBase
+{

+ public function adminOverview(Request $request)
+ {

+ public function page($scheme, Request $request)
+ {

+ public function checkAccess($scheme)
+ {

+ public function getUuid(Request $request)
+ {

it should be on the same line with method declaration

New patch coming up.

+++ b/js/plugins/ckeditor/imce.ckeditor.js
@@ -85,25 +85,36 @@
+            $.getJSON(drupalSettings.path.baseUrl + '/imce-get-uuid/', { uri: File.uri})

Use of baseUrl broken adress image on modal properties

The #43 works!

#45commited!

Hey @thalles what version was the committed patch created from? Thanks.

Re-implemented uuid support

Removed imce-get-uuid path in favor of ajax operation "uuid"
Removed uuid loading on init for the sake of performance

Please wait for enough people to review the patches before committing.
IMCE's priorities are performance and security. Please test for them thoroughly.

Thank you @ufku. I'm uploading a version of your patch for those using a composer.patches.json file in their root directory.