Opened 4 years ago

Closed 3 years ago

#13080 closed Bug (fixed)

Ugly notification when response has HTML content

Reported by: Maciej Owned by: Tade0
Priority: Normal Milestone: CKEditor 4.5.0
Component: General Version: 4.5.0 Beta
Keywords: Cc:

Description (last modified by Tade0)

Error occurs when response from imageUploadUrl link returns HTML content, ie PHP server returns stack trace.

Steps to reproduce:

  1. Configure server to return stack trace for url set in imageUploadUrl
  2. Use drag and drop to upload image
  3. Observe ugly notification that overlaps page & editor

Attachments (1)

ugly_notification.png (99.6 KB) - added by Maciej 4 years ago.

Download all attachments as: .zip

Change History (14)

Changed 4 years ago by Maciej

Attachment: ugly_notification.png added

comment:1 Changed 4 years ago by Piotr Jasiun

Milestone: CKEditor 4.5.0

comment:2 Changed 4 years ago by Piotr Jasiun

Status: newconfirmed

comment:3 Changed 3 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:4 Changed 3 years ago by Tade0

Description: modified (diff)
Status: assignedreview

comment:5 Changed 3 years ago by Tade0

Description: modified (diff)
Status: reviewassigned

comment:6 Changed 3 years ago by Tade0

Status: assignedreview

Changes and tests in branch:t/13080.

Not sure about the test. It works, but it doesn't look clean. Any suggestions how to fix this?

comment:7 Changed 3 years ago by Piotr Jasiun

Status: reviewreview_failed

You can not do data.message = responseError.replace( ': %1', '.' ); because you do not know what the responseError string will be in each language.

And about checking the `content-type` it is over-complication in my opinion. The whole code is in the fileUploadResponse listener so if someone want to have better error messages he can create his own fileUploadResponse listener. The default listener should be as simple as possible so developers we be able to create their own listeners based on this code.

comment:8 Changed 3 years ago by Piotr Jasiun

And for tests you could use sinonjs to mock editors methods.

comment:9 Changed 3 years ago by Piotrek Koszuliński

Oops. We reviewed this simultaneously :D. This is what I wrote:


I would simplify this even further. We can always display "Incorrect server response." error. Nothing more.

There are two reasons:

  1. I don't like the logic with replacing ": %1" with "." or message. What if in some language colon isn't used or sentence does not end with a dot?
  2. Checking if response format is text/plain does not mean that we know that HTML isn't there. I mean here both XSS and some other failures because we put some weird things that were supposed to be plain text inside notification.

So, I would KISS. Notification is for user, on the console we can log the rest.

As for tests, I think they are ok. Small code style issues:

Last edited 3 years ago by Piotrek Koszuliński (previous) (diff)

comment:10 Changed 3 years ago by Tade0

Status: review_failedreview

Changes in git:a076149.

I moved the error message to the console.

comment:12 Changed 3 years ago by Tade0

Status: review_failedreview

Modified the test so that it expects the behavior that's implemented.

Changes on branch:t/13080

Last edited 3 years ago by Tade0 (previous) (diff)

comment:13 Changed 3 years ago by Piotrek Koszuliński

Resolution: fixed
Status: reviewclosed

Fixed on major with git:68643de.

I made small improvements - it's good to teardown a stub, so next tests are not affected.

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy