Item571: Formatted Search is broken

I don't know what exactly is broken, but look at this topic: http://trunk.foswiki.org/System/PatternSkin

The TOC includes links from another topic: PatternSkinColorSettings

The long TOC becomes normal when the screenshot SEARCH is removed. This is a recent bug (plm 1 week).

• ArthurClemens - 26 Dec 2008

It is the SEARCH

I am not sure the actual search is broken.

I think there has been a change in PatternSkin topic that makes the $pattern find itself that finds itself.

What is this search doing inside html comments anyway?

Same search is also goofed up in SkinBrowser. It is supposed to find just the screen shot but it finds most if the topic incl the TOC creating a recursive loop.

• KennethLavrsen - 26 Dec 2008

I have checked in a fix for the search in the PatternSkin topic.

It may still be interesting to find out why it failed in the first place but this $pattern has always been a mystery to get to work and it scores a 7 or 8 on the NerdoMeter.

I could at least see one or two things that were wrong according to the documentation of Search and $pattern.

• KennethLavrsen - 26 Dec 2008

I have tried to place the new PatternSkin topic in a TWiki 4.2.4. And in 4.2.4 the SkinBrowser works fine.

Someone has goofed the formatted search feature very recently in Foswiki.

Release blocker of the major big ones.

Why do we have all these unit tests if they cannot catch things like this? And why is it people do not test their changes in a browser where you can see the changes clearly?

The problem is the $pattern feature is totally toasted. It can best be seen in the SkinBrowser topic

• KennethLavrsen - 26 Dec 2008

It is distro:19eb8a7b38db that causes the problems.

It was SVN 1482 that destroyed the formatted search feature

Author: CrawfordCurrie

Date: 2008-12-21 00:03:11 +0000 (Sun, 21 Dec 2008)

New Revision: 1482

Trac url: http://trac.foswiki.org/changeset/1482

Log: http://foswiki.org/Tasks/Item528 : validation of tainted data done using an unconditional approach. Still some uses of untaintUnchecked, but they are in the minority.

Next step - what in that mega check-in relates to search

• KennethLavrsen - 26 Dec 2008

I found the exact code that causes the trouble.

SVN 1481 had this sub in Search.pm

sub getTextPattern {
    my ( $text, $pattern ) = @_;

    $pattern =~
      s/([^\\])([\$\@\%\&\#\'\`\/])/$1\\$2/go;    # escape some special chars
    $pattern = Foswiki::Sandbox::untaintUnchecked($pattern);

    my $OK = 0;
    eval { $OK = ( $text =~ s/$pattern/$1/is ); };
    $text = '' unless ($OK);

    return $text;
}

And the new 1482 changed it to this

sub _extractPattern {
    my ( $text, $pattern ) = @_;

    # Pattern comes from topic, therefore tainted
    $pattern = Foswiki::Sandbox::untaint($pattern, \&Foswiki::validatePattern);

    try {
        $text =~ s/$pattern/$1/is;
    } catch Error::Simple with {
        $text = '';
    };

    return $text;
}

Now there are two things I can see.

The validatePattern function does two operations. How can we end up with $1 containing the same as in the old code?

And even when I replace just the first few lines the try/catch code does not behave the same way as the eval code.

Over again!

I reverted the change I did to PatternSkin.

The SkinBrowser and PatternSkin topics are excellent test cases for the problem.

Closing this as the error is fixed but clearly the eval has to be replaced by something safer

• KennethLavrsen - 26 Dec 2008