Typing in block with "Filtered HTML" + Multilink:

[21:Incident & Problem Management]

Got: Incident & Problem Management as visible result and
Incident & Problem Management as HTML

CommentFileSizeAuthor
#7 multilink_833768.patch1.62 KBdob_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Andy Inman’s picture

Status: Active » Postponed (maintainer needs more info)

Thanks for the report. I wonder if the HTML filter is interfering somehow. Could you try a test using a different input format without the HTML filter? (e.g. use the "Full HTML" filter with MultiLink added.)

MyXelf’s picture

Status: Postponed (maintainer needs more info) » Active

Hi:

This is not related to the HTML filter. This happens due to the default value set to the $options array passed to the l() function in the _multilink_process() function.

By default the 'html' property inside the $options array is set to false inside the l() function in common.inc. According to lline #1615 in includes/common.inc:

return '<a href="'. check_url(url($path, $options)) .'"'. drupal_attributes($options['attributes']) .'>'. ($options['html'] ? $text : check_plain($text)) .'</a>';

means that when the property is set to false, it will run the $text value thru the check_plain() function, which will encode all html entities found. In this case the provided $text value is already encoded, and that is why the double encoding of the text. The only choice is to run non-encoded text values.

I hope you get the idea

PS: I guess that for security reasons this is a "won't fix". I'm setting this issue to "active", letting the maintainer of the module say the last word about it.

Andy Inman’s picture

Thanks MyXelft - but when you say it's already encoded, surely that's due to the HTML filter, or am I misunderstanding? Well, now I seem to remember deciding not to set $options['html'] to avoid any security implication. I think it could be made a configurable option, since access to filters is controllable, so could be allowed for trusted users.

Re-ordering the filter configuration to place MultiLink *before* the HTML filter in the processing order should allow it to work, I think. That would have similar security implications of course.

I'll leave this open for now, to consider adding a configurable option.

MyXelf’s picture

Hi netgenius:

Today is my MultiLink day :-D

I don't understand how a configuration option would look like. I said it is not related to the filter because in my case, I build links at my custom module and at this level no more filters are in the way.

The strings to ML can come from a variety of sources (fixed strings, variables db-table, node titles and HTML filters), so some can be encoded and some not. I had to mimic the _multilink_process() function in my own code due to the inability to modify the $options array to set $html = true ... but I did it because I trust myself :-D

HTH

MyXelf

Andy Inman’s picture

Ok, now I understand. I was thinking a configurable option that, if enabled, includes 'html' => TRUE in the call to url(). Since access to individual format filters can be controlled, then the admin could enabled the option for trusted users only (in a filter format which is only accessible to trusted users.) It's a very easy feature to add, so I think I'll do it anyway.

Andy Inman’s picture

Category: bug » feature

Changing this to feature request, as its really not a bug.

dob_’s picture

FileSize
1.62 KB

Added a option to allow html in links for the multilink_filter

Andy Inman’s picture

Thanks dob_ - your patch looks fine (reading the code, I haven't tested it.) The only thing I would add is that the description of the html setting should probably have a short security warning.

dob_’s picture

Issue summary: View changes

Do you plan to add this patch?

It's a weird issue that i am not able to edit links with a WYSIWYG editor without the patch.

Andy Inman’s picture

@dob_ because of the danger of opening up a security hole (XSS) I would not want to add your patch without a clear warning message. For example, warning in bold on the configuration page, plus maybe a further warning message with drupal_set_message after enabling that option. I have absolutely zero free time right now, but if you want to add that to your patch, then yes, I think we can include it in a new version.