Example from image_style_deliver:

    drupal_add_http_header('Status', '500 Internal Server Error');
    print t('Error generating image.');
    drupal_exit();

If Russion language enabled I get the following message "Ошибка генерации изображения." instead of "Ошибка генерации изображения.". Because the content-type header is not correctly specified.
--
I do not understand why drupal has to send such messages.

CommentFileSizeAuthor
#8 D7-image-8.patch1.01 KBjoshi.rohit100
#1 image.patch1.03 KBdroplet

Comments

droplet’s picture

Version: 7.x-dev » 8.x-dev
Component: base system » image system
Issue tags: +Security
StatusFileSize
new1.03 KB

Missing the charset
and according to API doc, we always need to include charset in header:

Note: When sending a Content-Type header, always include a 'charset' type, too. This is necessary to avoid security bugs (e.g. UTF-7 XSS).

droplet’s picture

Status: Active » Needs review

needs review

kscheirer’s picture

Issue tags: -Security, -encoding

#1: image.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Security, +encoding

The last submitted patch, image.patch, failed testing.

claudiu.cristea’s picture

Version: 8.x-dev » 7.x-dev

In D8 we are using now \Symfony\Component\HttpFoundation\Response to respond and that one is automatically adding the content type header. Moving this issue back to D7 as it has no scope in D8.

dcam’s picture

Issue summary: View changes
Issue tags: +Needs reroll

#1 is a patch made for Drupal 8, but the function is the same now in D7 as it was in D8 when the patch was first made. I think the only reason the patch doesn't apply to D7 is because the D8 directory structure was different.

So #1 basically just needs to be rerolled. The two lines will have to be copied manually into the same places in D7.

dcam’s picture

Issue tags: +Novice
joshi.rohit100’s picture

Status: Needs work » Needs review
StatusFileSize
new1.01 KB

Rerolled #1 . Please review now.

dcam’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs reroll

If I knew how to manually trigger these errors for testing, I would. As it is, the code looks alright. I checked core for other instances where we're outputting error Status headers such as in drupal_deliver_html_page(). It does has the same Content-Type header with charset.

Since I don't know how to manually test for this issue, I don't know if there's a way to add an automated test. It seems like we might want one, but since this is only a minor issue then maybe it's not important. I'll go ahead and RTBC it on the basis of the code review. If a committer thinks it needs additional testing/automated tests then it will be bumped back down to Needs Work.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: D7-image-8.patch, failed testing.

Status: Needs work » Needs review

dcam queued 8: D7-image-8.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: D7-image-8.patch, failed testing.

Status: Needs work » Needs review

lauriii queued 8: D7-image-8.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: D7-image-8.patch, failed testing.

Status: Needs work » Needs review

dcam queued 8: D7-image-8.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: D7-image-8.patch, failed testing.

Status: Needs work » Needs review

dcam queued 8: D7-image-8.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 8: D7-image-8.patch, failed testing.

Status: Needs work » Needs review

dcam queued 8: D7-image-8.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community
David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

Yeah, I agree, something like this probably doesn't need an automated test.

  • David_Rothstein committed c684532 on 7.x
    Issue #1099732 by joshi.rohit100, droplet | Chi: Fixed Incorrect...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.