Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

image2 add-on does not handle image URLs containing queryString (http 403 error) #3394

Closed
ajssd opened this issue Sep 9, 2019 · 7 comments · Fixed by #5153
Closed

image2 add-on does not handle image URLs containing queryString (http 403 error) #3394

ajssd opened this issue Sep 9, 2019 · 7 comments · Fixed by #5153
Labels
plugin:image2 The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug.
Milestone

Comments

@ajssd
Copy link

ajssd commented Sep 9, 2019

Type of report

Bug

Provide detailed reproduction steps (if any)

  1. using the image2 add-on, choose an Image URL that contains a '?' followed by a queryString
  2. observe the HTTP 403 error when the image2 code tries to add the random number to the end of the URL

Expected result

HTTP 403 error

Actual result

No error

Other details

  • CKEditor version: 4.5.0+
  • Installed CKEditor plugins: image2

The offending code in Image2 is this:

image.setAttribute( 'src', ( config.baseHref || '' ) + src + '?' + Math.random().toString( 16 ).substring( 2 ) );
@ajssd ajssd added the type:bug A bug. label Sep 9, 2019
@Comandeer
Copy link
Member

Comandeer commented Sep 10, 2019

I can confirm the issue. It's present only in dialog, when fetching image preview. The fix seems rather easy: before adding query string, check if there isn't one already.

@Comandeer Comandeer added plugin:image2 The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. labels Sep 10, 2019
@ajssd
Copy link
Author

ajssd commented Sep 11, 2019

What is the reason for adding the query string anyway? I assume it's cache-related, but why would it be needed in this particular case? Can the code that adds the query string be removed completely?

@Comandeer
Copy link
Member

The query string is added to force an update of image preview. I'm not sure if it's really needed, yet it seems to be in image plugin from the very beginning. Probably some browsers had issues with updating image preview back then.

@ajssd
Copy link
Author

ajssd commented Mar 31, 2020

Tomas, we just upgraded to 4.13.1 and this bug is still there. Do you think you could remove the unnecessary append to the URL -- or, at least check if there is already a query string in the URL (i.e. search for "?") and if so, use "&" instead of "?".

surli added a commit to surli/ckeditor4 that referenced this issue Apr 8, 2022
  * Fix missing check of the query string presence in the src of the
    image before adding a random query string value for cache purpose
  * fixes ckeditor#3394
@surli
Copy link
Contributor

surli commented Apr 8, 2022

@Comandeer I happened to have this exact same issue when working on a custom uploader widget and testing it with images, so I opened a PR for fixing it. However it's really my first contrib on ckeditor, so might need a bit of help for providing a test to have it merged.

jacekbogdanski pushed a commit to surli/ckeditor4 that referenced this issue Apr 20, 2022
  * Fix missing check of the query string presence in the src of the
    image before adding a random query string value for cache purpose
  * fixes ckeditor#3394
@CKEditorBot CKEditorBot added this to the 4.18.1 milestone Apr 20, 2022
@CKEditorBot
Copy link
Collaborator

Closed in #5153

1 similar comment
@CKEditorBot
Copy link
Collaborator

Closed in #5153

@jacekbogdanski jacekbogdanski modified the milestones: 4.18.1, 4.19.0 May 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin:image2 The plugin which probably causes the issue. status:confirmed An issue confirmed by the development team. type:bug A bug.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants