If l() is called with the option 'HTML' => TRUE, then the text is not run through check_plain. This does however mean that callers need to sanitize their data in advance. We should add documentation to the docbook block explaining this.

CommentFileSizeAuthor
#9 1179582-l_html_security_documentation_9.diff903 bytesdalin
#6 1179582-l_html_security_documentation_6.diff904 bytesdalin
#2 1179582-l_html_security_documentation.diff892 bytesdalin

Comments

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented

I should add that this is important because this is possibly the #1 security mistake that people make in custom code/themes.

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Status: Active » Needs review
StatusFileSize
new 1179582-l_html_security_documentation.diff892 bytes
dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Issue tags: +Security
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work
Issue tags: +Needs backport to D7

I applaud the idea of this patch... but it's not quite ready to go (very small problem): there are two spaces between the previous text and the text you added. There should only be one.

The wording also seems slightly awkward to me, but I guess it is OK. I think it just doesn't flow well from the previous sentences... if you can improve it, that would be good!

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

fix typo in tag

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 1179582-l_html_security_documentation_6.diff904 bytes

Lets see if I managed to hand-edit this patch file correctly. I've reworded the patch and removed the double spaces (I'm old-school when it comes to sentence spacing).

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Thanks -- I like this version much better. :)

8.x/7.x please...

droplet’s picture

droplet commented
Status: Reviewed & tested by the community » Needs work 
+++ b/includes/common.incundefined
@@ -2288,7 +2288,9 @@ function drupal_attributes(array $attributes = array()) {
+ *     'html' is TRUE. The calling function must ensure that $text is already ¶

a little space at end of line :)

Powered by Dreditor.

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Status: Needs work » Needs review
StatusFileSize
new 1179582-l_html_security_documentation_9.diff903 bytes

Fixed trailing space.

dalin’s picture

dalin
he/him
Primary language English
Location 🇨🇦, 🇲🇽, 🌍
commented
Status: Needs review » Reviewed & tested by the community
jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

dalin: normally you should not mark your own patches "reviewed and tested by the community", even if you only changed one space. It's always good to have an extra set of eyes look at it quickly.

Anyway, I've looked at it, and the space is fixed, the patch is otherwise the same as the one I RTBC'd before, so it should be RTBC. :) Sorry for missing the end-of-line space the first time.

8.x/7.x please...

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Category: feature » task
Status: Reviewed & tested by the community » Fixed

Looks like a good textual improvement.

Committed and pushed to 8.x and 7.x. Thanks!

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