Filters should remove "rel" attributes instead of just adding rel="nofollow"

Tagging for the novice queue. Good way to brush up on regular expressions! :)

how about this attempt.

subscribe

subscribe

A few SimpleTests have been added.

I'm not sure why the PHP parser doesn't choke on this, but there's a double semicolon in

$text = filter_xss($text, $allowed_tags);;

It looks good otherwise. :)

New patch with extra ; removed.

This now validates the correct string is returned, and it isn't just blank e.t.c.

Are you just talking about me using single quotes instead of using double and \escaping?? or bigger than that?

New patch rolled with David's comments rectified, I think. This should be better.

New patch with test passing loveliness.

please review

Wouldn't a "simple" $text = preg_replace('/<a([^>]+)(rel=[^> ]+)([^>]*)>/i', '<a\\1\\3 rel="nofollow">', $text); be easier?
Or $text = preg_replace('/<a([^>]*)( rel=[^> ]+)([^>]*)>/i', '<a\\1\\3 rel="nofollow">', $text); so we don't end up with double spaces.

@tic2000

If I had that would strip the whole thing.

My code handles only the rel element *I think*. Whereas I think your regex assumes it is the last item.

Nope, it doesn't assumes that. That's why there are 3 pairs of () and there is \\1\\3. 1 is everything before rel (at least a space in the first case or nothing in the second case if rel is first attribute). 2 is rel (without the space before it, or with the space). 3 is anything after rel (if there is anything left).
Where my regexp will fail is if there is a space like rel="follow something" and I don't know if your code would handle that better. For sure it would fail on something more complex.

This should take care of any complex rel attribute.
It uses DOMDocument to remove the old attribute, if there is one and add a new clean one.

I don't like the fact that it uses some of the code in html corrector, but filter.module would need a good refactoring to get rid of that.

hook_filter should not be touched.

I didn't realise DomDocment is part of php5 core.

Your approach is better.

+1.

two of us have reviewed the latest patch and think it is good enough? I am RTBCing but unsure of normal RTBC protocol.

Although this does have some of my code in. back to needs review for someone else

Last patch has nothing of your code in it.

Adding the tests back. They got lost in the recent patches.

I've added the tests back, they didn't apply properly. We need tests for this patch, although the old ones weren't brilliant. We should probably do some more DomDocument stuff to make sure the rel attribute exists and is set to nofollow

There are already tests in HEAD for the spam deterent. I only added two for the case where rel is already set to check if the old one is removed and rel="nofollow" is added.
With DOMDocument you don't need check for rel=follow, rel='follow' because DOMDocument will automatically convert those to use double quotes.

In that case this is a fine patch. I hope to see more conversions from regex to DomDocument in the future.

There is no need to remove the attribute first.

And this one with just a simple setAttribute instead of setAttributeNode

Even nicer!

Hm. Do we need to benchmark this? Also, just one question:

+++ modules/filter/filter.module	6 Aug 2009 10:46:47 -0000
@@ -705,7 +705,19 @@
+    if (preg_match("|^<body[^>]*>(.*)</body>$|s", $htmlDom->saveXML($bodyNode), $matches)) {
+      $body_content = $matches[1];
+      // The XHTML guidelines recommend to include a space before the trailing /
+      // and > of empty elements for better rendering on HTML user agents.
+      $text = preg_replace('|<([^>]*)/>|i', '<$1 />', $body_content);
+    }

I don't understand what this part has to do with link rel tags?

This review is powered by Dreditor.

@webchick
About performance you can check #374441-23: Refactor Drupal HTML corrector (PHP5), but we can try do to a test with the regexp patch and with the dom patch and see how it goes.
About the code, as you can see when loading the HTML into the DOM we add the html and body tags too. After the processing is done we only need what's inside the body tag and the last regexp is to make <br/> become <br /> and things like that.

As DamZ suggests, we need a wrapper function for loading and saving the DOM because right now the HTML corrector uses this code. Also this patch and the patch in #16161-122: Move Read More link to end of node content. But that is for another issue.

Done some testing. Note that I don't have a perfect testing environment, just my pc.
10000 iterations using the source of my comment in #40 which has 2 links.

Patch in #20 average 5.35 seconds
Patch in #37 average 3.1 seconds

Typo fixed. Back to RTBC.

Just some extra whitespace removed

Regarding the DOM versus string parsing: DOM is generally faster then complex string parsing, and it all boils down to whether you prefer:

    // Get all HTML attributes into an array with three arrays as children. Process the rel items and remove them. Handle rel="xxx" rel='xxx' and rel=xxx.
    preg_match_all('/(\S+)=["\']?((?:.(?!["\']?\s+(?:\S+)=|[>"\']))+.)["\']?/', $text, $attr_matches);
    if (count($attr_matches) == 3 && count($attr_matches[0]) > 1) {
      foreach ($attr_matches[0] as $attribute) {
        if (strlen($attribute) >= 3 && strtolower(substr($attribute, 0, 3)) == 'rel') {
          // Replace this rel=whatever attribute with ''.
          $text = preg_replace('/' . $attribute . '/', '', $text);
        }
      }
    }
    $text = preg_replace('/<a([^>]+)>/i', '<a\\1 rel="nofollow">', $text);

or

  $htmlDom = filter_dom_load($text);
  $links = $htmlDom->getElementsByTagName('a');
  foreach($links as $link) {
    $link->setAttribute('rel', 'nofollow');
  }
  $text = filter_dom_serialize($text);

(that's supposing we properly refactor the DOM loading / serializing code that is already used elsewhere in core (in the HTML corrector)

I opened #542742: Filter: Create wrapper functions to load/serialize a DOM. for the wrapper functions.

I'd like to get #542742: Filter: Create wrapper functions to load/serialize a DOM. in first. When I do, this will require a re-roll, so marking "needs work" pre-emptively. ;)

#542742: Filter: Create wrapper functions to load/serialize a DOM. got committed. Back to work on this one. Well, review :)

Nice job, tic2000.

Awesome!

Committed to HEAD.

I'm a bit late to the party, but I just noticed this in core. And it may be bad form, but I'm re-opening the issue because I think a bit more discussion needs to happen here.

First off, when adding "nofollow" is enabled, then the functionality of this filter now encompasses that of the HTML corrector filter.

That means we could be causing redundant work, but more vitally, shouldn't adding "nofollow" and "correcting" the HTML be distinct actions? What about cases where someone may want to do the former without doing the latter; that's no longer possible. People may be surprised when they enable adding "nofollow" and then find their input also being tinkered with in other ways.

Does this not also open the door for other filters beyond the HTML corrector to also use the DOM object in their filtering? I personally would love to use it in Pathologic, but I can't use it without also "correcting" the HTML, but if the HTML is going to be corrected anyway then why not, but I'm still redundantly creating the DOM object and then unserializing it, and…

Now personally, I'm all for making more use of the PHP DOM object in filtering (#807996: [meta] Input filters and text formats), but we need to be sure the world is ready for it…

+1 for switching back to regex parsing for D7 - and then going DOM object crazy in D8.

No way. We need to refactor the filtering system for D8. In the meantime, even recreating the DOM object each time is faster then crazy regexps.

That means we could be causing redundant work, but more vitally, shouldn't adding "nofollow" and "correcting" the HTML be distinct actions? What about cases where someone may want to do the former without doing the latter; that's no longer possible. People may be surprised when they enable adding "nofollow" and then find their input also being tinkered with in other ways.

Please address this, at least.

If the HTML is not valid to begin with, then the mere concept of adding 'nofollow' is perfectly meaningless, and the behavior of the filter would be unspecificed.

Indeed it would, but I'm still not satisfied. What about the fact that this filter is doing something it shouldn't be - that it doesn't state it does? It's like taking your car to the shop to get the heater worked on, and when you get it back you find that the stereo has been replaced too, free of charge, with no mention of it made by the mechanic. In many cases it may be appreciated, but perhaps you were more happy with your old stereo and really didn't want the mechanic mucking about anywhere besides the heater.

A labored metaphor, but still…

webchick suggested I take it to a new issue. Perhaps I will in the morning.

We're getting a bunch of spam comments on D6 sites that cause Google to flag the sites as "adult" which removes them from SafeSearch results. I really think this is a bug fix that needs to be backported.

I've made a small adjustment for the D6 _filter_html() and posted it at #933538: Override rel="nofollow" problem with _filter_html().

Thanks tic2000.

subscribe

subscribe

I'm running into this as well, and agree that it should be backported to D6.

Here's a patch that removes any existing rel attributes before adding our own. It uses preg_replace only so that it will work with PHP4. I tested with the following content:

<a href="/">none</a>
<a rel="follow" href="/">before</a>
<a href="/" rel="follow">after</a>
<a href="/" rel="follow" class="test">middle</a>

Which produces:

<a href="/" rel="nofollow">none</a><br />
<a  href="/" rel="nofollow">before</a><br />
<a href="/"  rel="nofollow">after</a><br />
<a href="/"  class="test" rel="nofollow">middle</a>

I still need to fix the extra space that's showing up.

Here we go.

Testbot shouldn't be firing on this issue.

Let's give him another try. #65: 362972-65.remove-rel-6.x.patch queued for re-testing.

have you thought of these?

2 rel attributes:

<a href="http://example.com" rel="follow" rel="follow">SPAM</a>


single quote:

<a href="http://example.com" rel='follow'>SPAM</a>


no quote:

<a href="http://example.com" rel=follow>SPAM</a>

Updated issue summary.