• Do not register here on develop.twiki.org, login with your twiki.org account.
• Use View topic Item7848 for generic doc work for TWiki-6.1.1. Use View topic Item7851 for doc work on extensions that are not part of a release. More... Close
• Anything you create or change in standard webs (Main, TWiki, Sandbox etc) will be automatically reverted on every SVN update.
Does this site look broken?. Use the LitterTray web for test cases.

... and thus don't work in certain TWikiApplications.

Effect you get to see something like this:

TWiki.TwistyPlugin.init("twist...show");TWiki.TwistyPlugin.init("twist...hide");
on a page as a result of the interim _TWISTYSCRIPT tag being removed too early in the rendering pipeline. I don't know what that was for as all _TWISTYSCRIPT tags are processed by the postRenderingHandler anyway... but not so if the beforeCommonTagsHandler makes that impossible. Frankly, I can't understand why that whole twisty plugin worked till now.

Here's the patch:

--- lib/TWiki/Plugins/TwistyPlugin.pm   (revision 11981)
+++ lib/TWiki/Plugins/TwistyPlugin.pm   (working copy)
@@ -153,17 +153,11 @@
     return $modeTag._twistyCloseDiv();
 }

-=pod
-Disables _TWISTYSCRIPT tags written in the topic text.
-=cut

-sub beforeCommonTagsHandler {
-       # do not uncomment, use $_[0], $_[1]... instead
-       $_[0] =~ s/%_TWISTYSCRIPT{\"(.*?)\"}%/$1/g;
-}
-
 =pod
+
 Convert the semi-variable tag to JavaScript.
+
 =cut

 sub postRenderingHandler {

By allowing users to write _TWISTYSCRIPT in the topic they will be able to write javascript code, even if this had been disabled by unchecking AllowInlineScript in configure.

But there might be another way to prevent this?

AC

Infact _TWISTYSCRIPT has been a way to sneak in javascript into the topics before already. It would be better to collect all javascript and add it to the head and thus remove the need of the _TWISTYSCRIPT workaround/backdoor all together. But that's worth a separate Item.

MD

I don't get this. Before already?

  • I have introduced _TWISTYSCRIPT on 25 Sep 2006 (SVN 11591)
  • I have written the security measure half an hour later (SVN 11592)

Now you are saying we should re-introduce the hole?

AC

Do you agree that having _TWISTYSCRIPT is bad? Sorry that I didn't stand up and say so the first time you checked it in.

MD

From IRC, 17 Nov 2006

[09:41]
MichaelDaumArthurClemens, it sneaks in javascript into the topic text...which should be free of any javascript.
everybody can type _TWISTYSCRIPT by hand
[09:42]
ArthurClemensno, that is removed first
javascript is inserted in the rendered html, not in the topic
[09:43]
MichaelDaumhow about collecting all javascript that is needed and add it to the head with the official api? [09:45]
ArthurClemensit has a different purpose
normally all twisty states are set at onload
this means you will see things collapsing at page load
[09:45]
OliverKruegerOliverKrueger remembers a similar discussion on JS inclusion in the past. [09:46]
ArthurClemensthe inserted js inits the elements as soon as they are read by the browser
collapsed or open
this makes a quiet page, no visible collapsing
so I am for js inclusion in head, but in this case it won't help
[09:46]
MichaelDaumaha i see
and why must the state be set using javascript?
the initial one.
[09:48]
ArthurClemensbecause we support graceful fallback
no js: no collapsed content
with js: elements can be open or collapsed
[09:50]
MichaelDaumwhy not set the css using a style html attr
which should be better if there's no js
[09:51]
ArthurClemensTwisty already uses this [09:51]
MichaelDaumfor setting the initial state? [09:51]
ArthurClemensyes [09:51]
MichaelDaumthen why do you need more javascript in the body? [09:52]
ArthurClemensjavascripts toggles a css class
there are classes: twistyHidden, twistyMakeHidden, twistyMakeVisible
[09:52]
MichaelDaumwhy don't you set the class to the correct value right away?
initially
it must be only one of the initially
and it seems to me that there's no need to use js to set it to an initlal value
[09:53]
ArthurClemenshow would you do it? [09:54]
***CDot has joined #twiki
CDot changes topic to: Who broke Bugs web?
[09:54]
MichaelDaumI'd generate html that sets the class to the wanted initial vavlue. [09:55]
ArthurClemenswhere do you get the value from? [09:55]
MichaelDaum(bad spelling today) [09:55]
ArthurClemens(it is read from TWIKIPREF cookie now)

So it is needed with current Twisty.

AC

I need feedback on this, because the current 'fix' leaves a potential security hole.

  • Are you sure the problem is in _TWIKISCRIPT tags, because these only call init and without the handler things should still work, albeit after onload ?
  • Is there another way to remove handwritten _TWIKISCRIPT tags?

AC

Since this plugin is now in the default set this is a release blocker for 4.1

So people need to give Arthur feedback asap.

KJL

By lack of feedback I am reverting the change.

AC

Just to clarify the problem:

All callbacks done by TWiki::handleCommonTags may occur multiple times, i.e. if a plugin decides to make use of TWiki::Func::expandCommonVariables, which is quite common. This means that the beforeCommonTagsHandler is called multiple times too. Now, this bug manifests itself if another round of handling common tags happens before the TwistyPlugin will happily generate the javascript snippets using the postRenderingHandler. Here's an informal calling sequence leading to twsity javascript left behind:

  1. engine calls handleCommonTags for the first time
  2. TWISTYTOGGLE: insert interim tag _TWISTYSCRIPT
  3. another call to handleCommonTags being nested in the first call
  4. TwistyPlugin's beforeCommonTags removes _TWISTYSCRIPT in an antempt to sanitize the topic text from javascript
  5. in one of the remaining postRenderingHandler calls, TwistyPlugin tries to generate javascript init code expanding some _TWISTYSCRIPT tags ... which have unfortunately already been removed.

For now this only happens with some non-standard plugins that call expandCommonVariables.

As I tried to explain to AC on IRC, it is not clear to me why there's any need to generate inline javascript at all to set the initial class of the twisty html markup. Given that info is stored in a cookie, why is it impossible to generate the correct html code right away sent over by the server instead of relying on dhtml on the client side? Is it impossible to read the TWIKIPREFS cookie by the server? I asked that in the above IRC session already. But that's not been answered or included in the log above.

Sorry, but in reopen this bug once again as lots of twisties in TWikiApplications are broken again. The original issue is not gone.

MD


I am sorry about that but this item has been waiting for feedback for a long time.

I understand we have several ways of solving this:

  1. Remove the javascript tags in a handler that is only called once. If there is no such handler, we can use a flag beforeCommonTagsCalled.
  2. Reading TWIKIPREF (client side) cookies on the server. I really wouldn't know how to do that. Actually it seems very simple: How to get and set cookiesin Perl/CGI programs using CGI.pm
    • In that case we would need a similar mechanism to distill the PREF value from TWIKIPREF as currently done with javascript (see /pub/TWiki/TWikiJavascripts/twikiPref.js).
    • Which lib file would be a suited location for client-side pref parsing?

AC


I found another way to prevent the _TWISTYSCRIPTs to be removed too early. I added a $needPostRenderingHandler flag that is set if a _TWISTYSCRIPT tag is inserted; the beforeCommonTagsHandler will not remove them until a proper postRenderingHandler has done so.

Here's a patch.

--- lib/TWiki/Plugins/TwistyPlugin.pm   (revision 12150)
+++ lib/TWiki/Plugins/TwistyPlugin.pm   (working copy)
@@ -31,7 +31,7 @@

 use vars qw( $VERSION $RELEASE $pluginName $debug @modes $doneHeader $twistyCount
 $prefMode $prefShowLink $prefHideLink $prefRemember
-$defaultMode $defaultShowLink $defaultHideLink $defaultRemember );
+$defaultMode $defaultShowLink $defaultHideLink $defaultRemember $needPostRenderingHandler );

 # This should always be $Rev$ so that TWiki can determine the checked-in
 # status of the plugin. It is used by the build automation tools, so
@@ -61,6 +61,7 @@

     $doneHeader = 0;
     $twistyCount = 0;
+    $needPostRenderingHandler = 0;

     $prefMode = TWiki::Func::getPreferencesValue( 'TWISTYMODE' ) || TWiki::Func::getPluginPreferencesValue( 'TWISTYMODE' ) || $defaultMode;
     $prefShowLink = TWiki::Func::getPreferencesValue( 'TWISTYSHOWLINK' ) || TWiki::Func::getPluginPreferencesValue( 'TWISTYSHOWLINK' ) || $defaultShowLink;
@@ -161,8 +162,12 @@
 =cut

 sub beforeCommonTagsHandler {
-       # do not uncomment, use $_[0], $_[1]... instead
-       $_[0] =~ s/%_TWISTYSCRIPT{\"(.*?)\"}%/$1/g;
+
+    return if $needPostRenderingHandler; # don't remove _TWISTYSCRIPT too early
+                                         # see Item3159
+
+    # do not uncomment, use $_[0], $_[1]... instead
+    $_[0] =~ s/\%_TWISTYSCRIPT{\"(.*?)\"}\%/$1/g;
 }

 =pod
@@ -173,7 +178,9 @@

 sub postRenderingHandler {
     # do not uncomment, use $_[0], $_[1]... instead
-       $_[0] =~ s/%_TWISTYSCRIPT{\"(.*?)\"}%/<script type="text\/javascript\"\>$1<\/script>/g;
+    if ($_[0] =~ s/\%_TWISTYSCRIPT{\"(.*?)\"}\%/<script type="text\/javascript\"\>$1<\/script>/g) {
+      $needPostRenderingHandler = 0;
+    }
 }

 sub _twistyBtn {
@@ -270,6 +277,7 @@

 sub _createJavascriptTriggerCall {
        my($idTag) = @_;
+        $needPostRenderingHandler = 1; # notifies postRenderingHandler and beforeCommonTagsHandler
        return '%_TWISTYSCRIPT{"TWiki.TwistyPlugin.init("'.$idTag.'");"}%';
 }

Nevertheless, we should be able to get away with _TWISTYSCRIPT.

MD


Follow-up in TWiki:Codev/ReadingTWikiPrefCookieFromCore

AC


I believe Michael has committed the interim patch. That would relieve this item from blocking status.

AC

Alright, I checked in the above patch and tested my TWikiapps. It seems as though the most common cases have been captured. Note, however that I still a mini chance to sneak in a handwritten _TWISTYSCRIPT in a sequence of expansions that the TwistyPlugin will convert to proper javascript. But I think most people will never ever see find that out.

Ok, I think we can release 4.1 with this version of the TwistyPlugin. I downgrade the prio to Enhancement.

MD

The current version in svn works out fine. Still people using the current version on twiki.org are suffering from this bug. How about uploading a new version.

MD

I released all default plugins to twiki.org in connection with the 4.1 release. Including this one.

KJL

ItemTemplate
Summary Twisty tags are removed by beforeCommonTagsHandler
ReportedBy TWiki:Main.MichaelDaum
Codebase

SVN Range TWiki-4.1, Thu, 09 Nov 2006, build 11947
AppliesTo Extension
Component TwistyPlugin
Priority Enhancement
CurrentState Closed
WaitingFor

Checkins 11994 12143 12154
TargetRelease minor
ReleasedIn

Edit | Attach | Watch | Print version | History: r24 < r23 < r22 < r21 < r20 | Backlinks | Raw View | Raw edit | More topic actions
Topic revision: r24 - 2007-06-06 - TWikiUserMapping_MichaelDaum
 
This site is powered by the TWiki collaboration platform Powered by PerlCopyright © 2008-2019 by the contributing authors. All material on this collaboration platform is the property of the contributing authors.
Ideas, requests, problems regarding TWiki? Send feedback